Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,57 @@ Use the `dev-container` skill for build, start, attach, and exec inside the cont
- Prefer `assert condition, "message"` over `if not condition: raise ValueError("message")` for internal invariant checks. (Formatting, imports, and typing are enforced by `pre-commit` — see `.pre-commit-config.yaml`.)
- PR bodies follow `.github/pull_request_template.md` — a one-line Summary plus 2–5 detail bullets. Resist the agent default of long, multi-section descriptions.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates a duplicate top-level section — the file already has ## Coding style (lowercase 's') at line 38 with assert guidance. Recommend merging the new content into the existing section rather than creating a parallel one.

Suggested change
## Coding Style

(and remove the old ## Coding style section above, consolidating its content here)

## Coding Style

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a coding style section just above. Suggestion to remove.


### Checks/Asserts

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In Arena we prefer asserts over if...raises in most cases where such a check represents a coding error

Also: "(i.e)" on the next line should be "(i.e.,)".

In Arena we prefer asserts over if...raises in most cases where such an check represents a coding error

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a duplicate of what we aleady state above on line 40, eg
- Prefer assert condition, "message"overif not condition: raise ValueError("message")for internal invariant checks. (Formatting, imports, and typing are enforced bypre-commit— see.pre-commit-config.yaml.)

(i.e) something that shouldn't be recovered from. Asserts are briefer than the exception based counterpart.
Comment on lines +47 to +48

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Two minor grammar issues: "such an check" should be "such a check", and "(i.e)" is missing its comma and a closing parenthesis around the clause — the standard abbreviation is "(i.e.,)".

Suggested change
In Arena we prefer asserts over if...raises in most cases where such an check represents a coding error
(i.e) something that shouldn't be recovered from. Asserts are briefer than the exception based counterpart.
In Arena we prefer asserts over if...raises in most cases where such a check represents a coding error
(i.e., something that shouldn't be recovered from). Asserts are briefer than the exception based counterpart.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

The raises-based approach is more verbose (for example it always uses multiple lines), and because
this type of error is not intended to be recovered from, it offers no advantage.

### Docstrings

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the follwing is a pretty verbose paragraph to describe how to write (good) docstrings. Slightly ironic given the existing doc explicitly states "Resist the agent default of long, multi-section descriptions." :)
It's generally recommended to keep the agents.md at or below 150-300 lines of text as it might add to general context rot in other areas. This section alone would add a considerably large part.

Did you have any luck in adding a concise section to the coding style section like

- Docstrings: Google style (https://google.github.io/styleguide/pyguide.html#383-functions-and-methods).
- Prefer one line; a 2–3 line paragraph may follow if needed.
- Document `Args` and `Returns`, but **not** `Raises`. Omit `Returns` when it only returns None
    or the summary already covers it.

maybe with a short example?

@cvolkcvolk cvolkcvolk Jun 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eg we refer to the google styleguide (Edit: maybe without linking the URL) and then only state our deltas explicitely


Prefer short docstrings. Where a single line is not sufficient, a short (i.e. 2-3 line)
paragraph can follow. Document args and returns, but not raises.
We follow google-style for docstrings which is repeated below:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

google-style -- URL?


A docstring should give enough information to write a call to the function without reading the function’s code. The docstring should describe the function’s calling syntax and its semantics, but generally not its implementation details, unless those details are relevant to how the function is to be used. For example, a function that mutates one of its arguments as a side effect should note that in its docstring. Otherwise, subtle but important details of a function’s implementation that are not relevant to the caller are better expressed as comments alongside the code than within the function’s docstring.

#### Args
List each parameter by name. A description should follow the name, and be separated by a colon followed by either a space or newline. If the description is too long to fit on a single 80-character line, use a hanging indent of 2 or 4 spaces more than the parameter name (be consistent with the rest of the docstrings in the file). The description should include required type(s) if the code does not contain a corresponding type annotation. If a function accepts *foo (variable length argument lists) and/or **bar (arbitrary keyword arguments), they should be listed as *foo and **bar.

#### Returns: (or Yields: for generators)
Describe the semantics of the return value, including any type information that the type annotation does not provide. If the function only returns None, this section is not required. It may also be omitted if the docstring starts with “Return”, “Returns”, “Yield”, or “Yields” (e.g. """Returns row from Bigtable as a tuple of strings.""") and the opening sentence is sufficient to describe the return value. Do not imitate older ‘NumPy style’ (example), which frequently documented a tuple return value as if it were multiple return values with individual names (never mentioning the tuple). Instead, describe such a return value as: “Returns: A tuple (mat_a, mat_b), where mat_a is …, and …”. The auxiliary names in the docstring need not necessarily correspond to any internal names used in the function body (as those are not part of the API). If the function uses yield (is a generator), the Yields: section should document the object returned by next(), instead of the generator object itself that the call evaluates to.

#### Raises
We don't document raises in Arena

#### Example
Here is an example docstring

```python
def fetch_smalltable_rows(
table_handle: smalltable.Table,
keys: Sequence[bytes | str],
require_all_keys: bool = False,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is Google's internal example (smalltable.Table). Consider swapping for a small Arena-relevant snippet to make the guide more concrete for contributors. Even a simple get_observation_space(env_cfg: EnvCfg) -> gym.spaces.Space would anchor the guidance in the project domain.

) -> Mapping[bytes, tuple[str, ...]]:
"""Fetches rows from a Smalltable.
Retrieves rows pertaining to the given keys from the Table instance
represented by table_handle. String keys will be UTF-8 encoded.
Args:
table_handle: An open smalltable.Table instance.
keys: A sequence of strings representing the key of each table
row to fetch. String keys will be UTF-8 encoded.
require_all_keys: If True only rows with values set for all keys will be
returned.
Returns:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring example should have a blank line between the summary and the extended description per Google style:

    """Fetches rows from a Smalltable.

    Retrieves rows pertaining to the given keys from the Table instance
    ...

Without it, the example contradicts the guidance it's illustrating.

A dict mapping keys to the corresponding table row data
fetched. Each row is represented as a tuple of strings.
"""
Comment on lines +78 to +90

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 The example docstring is missing the blank lines between the summary sentence, the extended description paragraph, and the Args: section that Google style requires. As written, an AI agent following this example would produce non-conforming docstrings.

Suggested change
"""Fetches rows from a Smalltable.
Retrieves rows pertaining to the given keys from the Table instance
represented by table_handle. String keys will be UTF-8 encoded.
Args:
table_handle: An open smalltable.Table instance.
keys: A sequence of strings representing the key of each table
row to fetch. String keys will be UTF-8 encoded.
require_all_keys: If True only rows with values set for all keys will be
returned.
Returns:
A dict mapping keys to the corresponding table row data
fetched. Each row is represented as a tuple of strings.
"""
"""Fetches rows from a Smalltable.
Retrieves rows pertaining to the given keys from the Table instance
represented by table_handle. String keys will be UTF-8 encoded.
Args:
table_handle: An open smalltable.Table instance.
keys: A sequence of strings representing the key of each table
row to fetch. String keys will be UTF-8 encoded.
require_all_keys: If True only rows with values set for all keys will be
returned.
Returns:
A dict mapping keys to the corresponding table row data
fetched. Each row is represented as a tuple of strings.
""

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

```


## Conventions

### Wrapped Environment
Expand Down
Loading