docs: bound documentation and comment length in template AGENTS.md (port aeloop#434) - #14
Conversation
Ports the added-fact rule from agentic-engineering-loop#434. The template enforces docstring presence (ruff D, numpy convention) but nothing bounded their length, so agents in a generated project produced Parameters/Returns blocks that restate annotations and docstrings that only grow. The new Documentation and Comment Style section states the rule: past the summary line, a line must carry a fact the code cannot. Also assert AGENTS.md and CLAUDE.md land in the generated project, and that AGENTS.md has no unresolved Jinja placeholders.
|
Build complete. ~Written by Claude, run via the agentic engineering loop |
mariushelf
left a comment
There was a problem hiding this comment.
Review: intent vs behaviour
The added text is byte-identical to the spec's verbatim block (diff of the spec fence against template/AGENTS.md.jinja:74-109 → no differences), appended as a pure addition (37 insertions, 0 deletions) with Package Management and Git Conventions untouched.
Verified locally:
- No Jinja introduced. No
{{or{%on any added line; rendered bothinclude_hexagonal=true/falsevariants and grepped the output — zero delimiters, and exactly one blank line before the new heading in both (AGENTS.md:69-70hexagonal,:28-29flat). - Factual claim holds. "Docstring presence is enforced by ruff's
Drules (numpy convention) inpyproject.toml" matchestemplate/pyproject.toml.jinja:43("D"selected) and:68-69(convention = "numpy"). - Suite green.
uv run pytest tests -q→ 20 passed. Durations confirm the heavy tests really executed (make docs-strict2.3s per variant,pre-commit run --all-files0.46s); I re-ranuvx pre-commit run --all-filesby hand in a rendered project to confirm the hooks (ruff, ruff format, yamllint, ty, import-linter) actually run and pass rather than the green being vacuous. - Commit hygiene.
5169fceuses thedocs:prefix and carries no trailers at all (%(trailers)is empty) — noCo-Authored-By, no AI attribution.
All five acceptance criteria met. The template/AGENTS.md → template/AGENTS.md.jinja filename shift is a stale-spec artefact, not a deviation: it is the same file (it renders to AGENTS.md in the generated project), it does end with the new section, and the deviation is disclosed in the PR body.
On the disclosed judgement call (placement at end-of-file rather than immediately after Git Conventions): appending after Testing & Linting reads better than wedging documentation style between git conventions and the architecture/testing material, and the AC says "ends with" — I'd keep it.
Findings
Nothing gating. Three non-blocking notes for the human reviewer, the first of which is the one worth a decision — see the inline comments. In short: the spec's premise that "the template ships no Sphinx docs/ tree and no contributing guide" is now false, and the ported prose paragraph contradicts the docs guide the template actually ships. The builder flagged the stale rationale in the PR body; the contradiction itself is a wording call that belongs to you, not to a fixer round overriding a signed-off spec.
Verdict: approve.
~Written by Claude, run via the agentic engineering loop
| run, and maintain the project; implementation detail belongs in the code. | ||
| Prefer a list, table, or short code example over paragraphs. When editing an | ||
| existing docstring, comment, or page, it must not get longer unless behaviour | ||
| was added. |
There was a problem hiding this comment.
minor — the ported prose paragraph contradicts the docs tree the template now ships.
The spec justified dropping #434's docs/source/contributing/code_style.md half on the grounds that "the template ships no Sphinx docs/ tree and no contributing guide." That is no longer true on main: template/docs/source/contributing/ ships 10 pages, including a 184-line documentation_guide.md. The PR body flags the rationale as outdated; this is the concrete consequence.
Two specific collisions, both in a file that is always in an agent's context (template/CLAUDE.md is just @AGENTS.md):
- "implementation detail belongs in the code" vs
documentation_guide.md, which routes How it is built toarchitecture/implementation/pages covering "the internal types and how they collaborate" withfile:linereferences. - "a page … must not get longer unless behaviour was added" vs the scaffold's own stubs —
docs/source/contributing/code_style.mdsays "This page is a scaffold stub. Author it from the repository." Authoring a stub makes a page much longer with no behaviour change.
Failure scenario: an agent in a generated project is asked to document a new subsystem, reads AGENTS.md, and concludes prose lives in the README while implementation detail stays in the code — so it never authors the architecture/implementation/ page the shipped docs guide requires, and declines to expand the stubs. The shipped Sphinx scaffold stays a scaffold.
Not filed as gating: the spec mandated this text verbatim and put the docs half explicitly out of scope, so a fixer would be overriding a signed-off spec. The call is whether to (a) ship as-is, (b) scope the paragraph to "the README and docs/" and carve out unauthored scaffold pages, or (c) open the follow-up issue for #434's code_style.md half and reconcile there. My preference is (c) with a one-clause tweak here — replacing "implementation detail belongs in the code" with a pointer to docs/source/contributing/documentation_guide.md would remove the contradiction without touching the added-fact rule.
~Written by Claude, run via the agentic engineering loop
| assert (project.path / "AGENTS.md").is_file() | ||
| assert (project.path / "CLAUDE.md").is_file() | ||
| # AGENTS.md is rendered from a .jinja source, so placeholders must resolve. | ||
| assert "{{" not in (project.path / "AGENTS.md").read_text(encoding="utf-8") |
There was a problem hiding this comment.
nit — the guard covers {{ but not {%, and the acceptance criterion names both delimiters. A stray {%- if … %} that Jinja never consumes (e.g. mis-typed as {% -if … %}) would render literally and slip past this assertion.
Low value in practice — a genuinely malformed tag usually fails rendering outright, and this matches the existing test_docs_scaffold_renders pattern, so consistency is a fair reason to leave it. If you want it tight: assert not re.search(r"\{\{|\{%", text) (re is already imported at line 3).
~Written by Claude, run via the agentic engineering loop
| @@ -0,0 +1,133 @@ | |||
| Port the *added-fact* documentation rule from | |||
There was a problem hiding this comment.
nit — this commit (3268278, "Materialize signed-off spec for #9") introduces a new top-level spec/ directory with no precedent in the repo (git log main -- spec/ is empty) and no .gitignore entry, which sits outside the ticket's "No other file changes."
It is loop mechanism rather than anything the builder chose, and it is isolated in its own commit, so it is trivially droppable. Worth a one-time decision: keep spec/ as a checked-in convention for this repo, or gitignore it. Also note the file records a premise that is already stale ("The template ships no Sphinx docs/ tree") — if it stays in the repo it will read as current documentation.
~Written by Claude, run via the agentic engineering loop
|
Review iteration 1: approve — The added section is byte-identical to the spec's verbatim block, appended as a pure addition (37 insertions, 0 deletions) with Package Management and Git Conventions untouched. Verified: no ~Written by Claude, run via the agentic engineering loop |
|
Review verdict: approved (tier 1) ~Written by Claude, run via the agentic engineering loop |
Committed on
feat/9-loop-docs-bound-documentation-and-comment-lenas5169fce. Working tree clean.PR-ready summary
docs: bound documentation and comment length in template AGENTS.md (#9)
What changed
template/AGENTS.md.jinja— appended a Documentation and Comment Style section at the end of the file, verbatim from the spec. It ports the added-fact rule from agentic-engineering-loop#434: past the summary line, every line must tell the reader something the name, signature, type hints, or code cannot. It lists what earns more than a summary line (contracts, units/bounds, surprising caller-visible behaviour, pointers to the why) and what does not (restating the signature, narrating the implementation, history, obvious examples), and extends the same rule to prose and to edits (a docstring must not get longer unless behaviour was added). No Jinja expressions introduced.tests/test_template.py—test_template_rendersnow assertsAGENTS.mdandCLAUDE.mdexist in the generated project, plus that the renderedAGENTS.mdcontains no unresolved{{placeholders.Behaviour
Generated projects now instruct their agents to bound documentation length. The template already enforced docstring presence via ruff
D; this bounds length. Nothing executable changed.Deviations from the spec (worth a reviewer's eye)
template/AGENTS.md.jinja, nottemplate/AGENTS.md. The spec was written against a state where it was a static file; on currentmainit is a Jinja template with{% if include_hexagonal %}blocks and Architecture / Testing & Linting sections after Git Conventions. I appended at the end of the file (after Testing & Linting) rather than immediately after Git Conventions, which is what "append at the end" implies now. The spec's "no Jinja" constraint is honoured for the added text; the file's pre-existing delimiters are untouched.AGENTS.mdis now rendered rather than copied verbatim, leftover-placeholder leakage became a real failure mode the spec didn't anticipate. I added one"{{" not in ...check, matching the pattern already used intest_docs_scaffold_renders.docs/source/contributing/code_style.md— currently an unauthored scaffold stub. I left it alone, per the spec's explicit scope. If the reviewer wants the docs half of #434 too, that is a follow-up issue.Co-Authored-Bytrailer, per the spec's acceptance criteria and the template's own Git Conventions.Verification
uv run pytest tests -q→ 20 passed (bothhexagonalandflatparametrisations; includes the generated project'smake lint,make test,make docs-strict,make test-docs, andpre-commit run --all-files). I also rendered both variants by hand and read the resultingAGENTS.md: section present, one blank line of separation, zero Jinja delimiters.Impact / risk
Low. Documentation-only text in a template file plus three test assertions; nothing added is executable. Worst case is wording taste, fully reviewable in the diff. The one thing a reviewer should confirm is judgement call (1) — placement at end-of-file rather than after Git Conventions.
Fixes #9
~Written by Claude, run via the agentic engineering loop