AGENTS.md: what @module is for, and no external tools without approval - #1765
Conversation
`fjs/AGENTS.md` §2 reserves `@module` for a package entry point. Nothing enforced it, and it drifted twice in a week: onto 102 `types.ts` and proof files, corrected in #1756, and again on a parallel branch that hit the same rule and filed it as debt rather than applying it. `tsc` cannot see the tag's placement, so the rule needs its own gate — the shape main established for the file-scope-typedef prohibition, in the same job. Both directions are checked, as two steps. A guard for stray tags alone would also pass on a tree that had lost the tag from every entry point, which is the failure the second half catches; the drift so far has been in the first direction only because nothing was looking in either. `grep -L` cannot carry the verdict in its exit status. It reports whether any file *matched*, not whether it listed one, so `xargs grep -L @module` exits 0 on a clean tree and the obvious spelling of the second guard can never fail. It pipes into `grep -q .` instead. The first guard's `grep -qv` has the right semantics as written. Each guard was proven against the string CI will actually run, extracted from the generated `ci.yml` rather than read off the generator source, and each fails on its own violation and no other: `@module` added to a `types.ts`, added to a `proof.f.mjs`, and removed from an entry point. The proof asserts both steps are generated, since a guard that silently stopped being emitted is precisely the failure it exists to prevent — and those assertions were themselves checked by removing each guard in turn. The existing typedef guard has no such assertion; left alone rather than widening this change. `fjs/ci/todo/node26-job-is-this-repo-only.md` records what this revealed: `fjs ci` is offered to other projects, but `node26` carries this repository's own gates — `npm run ci-update` against a script a consumer will not have, and now three convention checks. Pre-existing and not introduced here; naming it rather than fixing it inside this change. `npx tsc` clean, suite 3528/3528, `fjs/ci` proofs 29/29. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7vAocRuxfmWJDnujCoMup
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
functionalscript | 9cb8471 | Commit Preview URL Branch Preview URL |
Aug 28 2026, 10:28 PM |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 502744fb72
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91aacf21ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The guard failed CI on its own commit. It matched `@module` anywhere in a `.ts`/`.mjs` file, and `fjs/ci/proof.f.mjs` — added in the same change — contains the string three times, in the assertions naming these very steps. So the guard flagged the code that asserts the guard exists. My verification ran in the wrong order and that is the whole lesson here: I extracted the emitted commands and exercised them against three violations, then added the proof assertions, and never re-ran the guards afterwards. Everything I checked was correct when I checked it. The final tree was never checked at all. The pattern now matches the tag where JSDoc puts it — the same anchoring the file-scope-typedef guard uses — so a line that mentions `@module` in a string or a `//` comment is not a declaration of it. That distinction is what the rule was always about; the bare string only happened to work while nothing in the repository discussed the tag. Both guards also end in a plain `grep` rather than `grep -q`. The `-q` form exits on its first match and closes the pipe, so the failing run printed `grep: write error: Broken pipe` and no indication of which file was at fault. Without it the log names the offending paths. Six cases now run against the strings extracted from the regenerated `ci.yml`, after every edit: the clean tree; a file that only mentions the tag (must not fire); a real tag on a `types.ts` and on a `proof.f.mjs` (must fire); an entry point stripped of its tag (must fire); and a failing run, checked for naming the file. The proof assertions match `grep -rlE` and `xargs grep -LE`, the parts that tell the two steps apart, so neither can be satisfied by the other's step — re-checked by removing each guard in turn. `npx tsc` clean, suite 3528/3528, `fjs/ci` proofs 29/29. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7vAocRuxfmWJDnujCoMup
…n-hrijwz' into claude/review-todos-documentation-hrijwz
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b01fb4e88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Reverts the two generated CI steps and their proof assertions. The CI generator, its proof and `.github/workflows/ci.yml` are byte-identical to `main` again. The guards were the argument against themselves. A pattern over source text cannot tell a JSDoc tag from the same characters in a string or a comment, so the first flagged `fjs/ci/proof.f.mjs` — the file whose assertions named the guard — and the second could not have reported a missing tag in any file that mentioned the tag anywhere. Anchoring the pattern fixed both instances without fixing the category: the check still only approximated the question it claimed to answer, and its failures were silent ones. AGENTS.md gains §6. No external tool is called from our code without approval first; where a rule needs real analysis the answer is a tool that parses what it checks, ESLint or Clippy, proposed and approved rather than a pattern that resembles one. And leaving the check undone beats the machinery: a rule no available tool can express stays written down and unenforced, which is honest and cheaper. That makes the `@module` rule unenforced again — deliberately. It drifted twice, which argued for a gate, but the gate has to be worth trusting. Two things left standing, both open to reversal: `fjs/ci/todo/node26-job-is-this-repo-only.md` stays. It records that `fjs ci` is offered to other projects while `node26` carries this repository's own gates — `npm run ci-update` hard-references this tree, and `Setup.nodeExtra` does not reach the canonical jobs. Pre-existing, and the Codex P1 on the reverted change was about the same seam. The file-scope `@typedef` guard is still generated. It is the same shape as what this reverts, so §6 covers it, but removing it was not asked and is a separate call. `npx tsc` clean, suite 3533/3533, `fjs/ci` proofs 30/30. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7vAocRuxfmWJDnujCoMup
@module placement ruleThere was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1932280516
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review caught the issue describing a tree that no longer exists. It listed "both halves of the `@module` placement rule" among the repository-only gates `fjs ci` ships to consumers, and the revert in 1932280 removed them — so it recorded as shipped a pair of steps the same pull request had already taken out, and told whoever picks it up that consumers fail on a rule this branch deliberately leaves unenforced. The inventory is now the two that are actually generated: `npm run ci-update` with its drift check, and the file-scope `@typedef` prohibition. The `@module` attempt is kept as history rather than deleted — it is why the issue exists — but stated as reverted, with the reason and a pointer to root `AGENTS.md` §6, which now rules the approach out. Nothing else in the file assumed the gates: the two options, the question of whether any outside project runs `fjs ci`, and the README disagreement all stand as written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7vAocRuxfmWJDnujCoMup
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approved, and I think this is the right reversal.
I verified the reasoning rather than just the diff. The claim that the second guard could not fail holds by construction: grep -L lists files containing no match, so an entry point that mentions @module anywhere — prose, a string, an assertion naming the guard — is excluded from the candidate list and never flagged, tag present or not. I built exactly that file and confirmed it is not listed. A check that cannot fail is worth less than no check, because it reads as coverage.
The first guard's failure was concrete, not hypothetical: at 91aacf21e it flagged fjs/ci/proof.f.mjs, the file whose assertions named it. The anchored version at 0b01fb4e8 did fix that, and I verified it worked — planted tags in proof.f.mjs, in types.ts, and a removed tag on a real entry point all failed it. So this is not a retreat from something broken; it is dropping something that worked in favour of not approximating analysis with text matching. That is a judgement I'd make the same way.
One thing the new rule sharpens for me, and it corrects something I told you on #1750. \s matches exactly one whitespace character:
' * @typedef' MATCH
'\t* @typedef' MATCH
' * @typedef' MISS
'\t\t* @typedef' MISS
I approved that guard saying it caught tab-indented continuations. True for the single tab I tested; a tag indented two levels escapes it. The surviving @typedef guard has the same blind spot the new rule describes — it is prior art, and §6 says predating instances are not precedent, so nothing here needs changing. Worth knowing that the one grep guard still in CI is weaker than its green result suggests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37dc5bedb9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Thanks — and I checked the The rule is file-scope So the single-whitespace anchor is load-bearing. Loosening it to There is a real gap, just a different one. An indented file-scope block escapes entirely: /**
* @typedef {number} X
*/
export const a = 1 // not flagged
Which I think sharpens §6 rather than softening it. Three people have now reasoned about this one pattern — you twice, me twice, Codex three times — and the readings have been wrong in different directions each time: I thought the anchor was arbitrary, you thought it was weak, and it is actually precise for a reason none of us stated. A tool that parses JSDoc would answer "is this typedef at file scope" directly instead of encoding it in a whitespace count that reads as an accident. No change here: the guard predates §6, it is doing its job, and nothing in this PR touches it. Worth a comment beside it saying why the single Generated by Claude Code |
Review checked `fjs/ci/README.md`, which I had not read past its opening. It names `cov` and `ci-update` as the commands a consuming `package.json` must provide, shows the typical definitions, and explains that a project chains its own generators into `ci-update` so the drift check covers them for free. This repository's `node ./fjs/module.mjs ci && …` spelling is explained there too: it avoids depending on the package bin before the package is installed. So the step is an extension point, and listing it as this repository's private gate was wrong. That correction cuts the other way from how I argued it. I claimed on the pull request that `node26` is already unusable for consumers, so a convention gate added to it changes nothing for them. The opposite holds: a project that follows the documented setup has a working `node26`, and the one step outside the contract — the file-scope `@typedef` prohibition — is then the only thing standing between them and a green build. The reviewer who called the reverted `@module` gates a real consumer regression was right, and my rebuttal rested on not having read the contract. The issue is rescoped to that one step, and retitled, since "ships this repository's own gates" overstated a job that is mostly portable by design. The narrowness is the finding rather than a softening of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7vAocRuxfmWJDnujCoMup
Priority drops P3 to P5. Reviewers — human and agent — catch this class of thing, and no project outside this repository is known to run `fjs ci` at all, which the issue already asks whoever picks it up to check first. It is worth the day a consumer turns up and not much before. The file is renamed with it. `node26-job-is-this-repo-only` asserted the claim the previous commit retracted: most of that job is the documented `cov`/`ci-update` contract and works for a conforming consumer. Only the file-scope `@typedef` gate sits outside it, so the name says that instead. Free to do now — the file is new in this branch and nothing links to it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7vAocRuxfmWJDnujCoMup
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe7ee2d876
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
todo/README.md asks for a heading that names the action or the thing, not
the full context. "ships one convention consumers never agreed to" carried
the diagnosis in the title; the Problem section already states it, at
length and more accurately.
Kept the `{slug}.` prefix — it is what 151 of the 279 issue files do, and
the neighbours this branch touched are among them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T7vAocRuxfmWJDnujCoMup
Two issues asked the same question. #1756 answered it — strip the tag, `fjs/AGENTS.md` §2 was right — and applied that to 102 files, then deleted `fjs/todo/module-tag-on-types-ts.md` and missed this one. Every premise in it is now false. "90 of the 94 `types.ts` files carry it anyway" is 0 of 98. Its three tasks are done: the convention was decided, applied to every `types.ts`, and `fjs/web/types.ts` — named as the minority case — matches. Nothing cites it. Nothing to move before deleting: the decision it asked for lives in `fjs/AGENTS.md` §2, which never changed, and the CI-enforcement question it did not ask about is settled the other way by §6. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7vAocRuxfmWJDnujCoMup
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approved. Deleting the duplicate is right and I checked the premise rather than the reasoning: zero types.ts files on main still carry @module, so #1756 really did answer it. The surviving note is renamed to what is actually left of the finding, and P5 matches that.
§2 said the tag belongs only to `module.f.mjs` / `module.mjs`. That is the rule #1756 enforced across 102 files, and it had the reason backwards. `deno doc` reads the tag and nothing else. A file whose leading block carries it gets that prose as `module_doc`; a file without it gets no `module_doc` at all — the block is dropped, not demoted. Checked against the Deno the repository pins, for `.mjs` and `.ts` alike, and the tag need not be in the first block, only in some block. So the tag is what makes a leading block *be* documentation, and it goes wherever a file has module documentation to publish: `module.f.mjs`, `types.ts`, `private.ts`. A block holding only `@import` tags has nothing to attach and wants none. Where the documentation is never published the tag buys nothing, which is `proof.*`. 98 of the 102 stripped files had real prose, so their module documentation is currently invisible to `deno doc` — the output `fjs/website/todo/publish-deno-doc-to-website.md` plans to publish. §2 says so plainly rather than describing a tree that does not exist, and points at the restore. `fjs/todo/module-tag-restore.md` (P3) carries the inventory, split by the three decisions it actually contains: 89 `types.ts` with prose, mechanical; 11 proof files where the rule says leave them but that is worth confirming; and two files that are neither — `browser.mjs` has real prose and reads like the first group, `testlib.f.mjs` held only `@import` tags and wants no tag at all. `todo/jsdoc-verification.md` (P4) asks how any of this could be checked. Three passes over one tag with no signal at any point is the argument for looking; §6 ruling out text patterns is why the answer has to parse, and "not worth it, leave it to review" is a legitimate outcome to record. Documentation only. `npx tsc` clean, suite 3533/3533. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7vAocRuxfmWJDnujCoMup
|
Flagging that this approval is at And your premise check needs one correction. You verified that zero
The deletion of What landed in
Title and body are updated to match; both were describing the earlier shape of the PR. Generated by Claude Code |
@module is for, and no external tools without approval
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73bb49e494
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…d claim Three review findings, all against text I wrote in the previous commit. §1.2 still showed a `proof.f.mjs` leading block carrying `@module`, while the §2 I had just rewritten said a proof's documentation is never published so the tag has nothing to attach to. Same file, opposite instructions for the same shape. The example loses the tag and says why. The restore issue omitted `private.ts`. All 16 have prose and none carry the tag — stripped by #1750 rather than #1756, so they were not in the 102 I counted, but it is the same reading and the same fix, and §2 names `private.ts` explicitly. As written the issue could have been completed in full while leaving 16 files violating the rule it exists to satisfy, and its last task says to delete §2's non-compliance note. And the publication rationale was overstated. The website issue plans `deno doc --html **/module.f.mjs`, a glob excluding every `types.ts` and `private.ts`, so restoring the tag alone puts nothing on the website. The tag is necessary, not sufficient: it decides whether `deno doc` *can* see a file's module documentation, and what the build is pointed at is a separate question. Both §2 and the issue say that now, and widening the glob is named as the other half. Also added: a task to correct `todo/migrate-typescript-to-mjs.md`, which restates the old rule verbatim under "Module header and import ordering" rather than linking §2, so it did not move when §2 did. Documentation only. `npx tsc` clean, suite 3533/3533. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7vAocRuxfmWJDnujCoMup
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7d8e2da26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex P2 on b7d8e2d: fjs/todo/module-tag-restore.md told the website issue to widen its deno doc glob to types.ts and private.ts alike. Doing that would put all 16 private.ts modules and their exported _ types on the public API site, which is what separate-private-types.md exists to prevent: those types are outside the public declaration closure, and its Stage 2 drops their generated declarations from the package. Restoring the tag in private.ts still stands - the prose is reachable by deno doc for a contributor either way - so the split is about audience, not about the tag. AGENTS.md section 2 said documentation to publish, which conflated the two; it now says documentation a reader is meant to get from deno doc, and names which reader per file kind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T7vAocRuxfmWJDnujCoMup
Documentation only. Two rules written down, three issues filed or resolved, and the CI generator restored to
main.@moduleis what publishes a module docfjs/AGENTS.md§2 said the tag belongs only tomodule.f.mjs/module.mjs. That is the rule #1756 enforced across 102 files, and it had the reason backwards.deno docreads the tag and nothing else. A file whose leading block carries it gets that prose asmodule_doc; a file without it gets nomodule_docat all — the block is dropped, not demoted. Verified against the Deno the repository pins, for.mjsand.tsalike, and the tag need not be in the first block, only in some block.So the tag makes a leading block be documentation, and goes wherever a file has module-level documentation a reader is meant to get from
deno doc—module.f.mjs,types.ts,private.ts. A block holding only@importtags has nothing to attach and wants none. Where the documentation reaches no reader the tag buys nothing, which isproof.*.Which reader, though, is a separate question the tag does not answer — the tag is necessary, not sufficient. It decides whether
deno doccan see a file's module documentation; what a documentation build is then pointed at is its own decision.module.f.mjsandtypes.tsare public API surface.private.tsis not:fjs/todo/separate-private-types.mdputs those types outside the public declaration closure and its Stage 2 drops their generated declarations from the package, so the tag belongs there for contributors reading the sources while a public site stays pointed away. Both documents now say this rather than leaving it implied — review caught the elision twice.98 of the 102 files #1756 stripped had real prose, and #1750 did the same to 16
private.ts. That module documentation is currently invisible todeno doc. §2 says so plainly rather than describing a tree that does not exist, and points at the restore.No external tools from our code without approval
§6 is new, and this PR is its first application: it began as two generated
grepsteps guarding the old@modulerule, and removes them.A pattern over source text cannot tell a JSDoc tag from the same characters in a string or a comment. The first guard flagged
fjs/ci/proof.f.mjs— the file whose assertions named the guard — failing CI on its own commit. The second could not have reported a missing tag in any file that mentioned the tag anywhere, so it was a check that could not fail, which reads exactly like one that passes. Anchoring the pattern fixed both instances without fixing the category.So: no external tool called from our code without approval first; where a rule needs real analysis the answer is a tool that parses what it checks — ESLint, Clippy — proposed and approved, never a pattern that resembles one. And leaving the check undone beats the machinery, because a rule no available tool can express stays written down and unenforced, which is honest and cheaper than a guard whose failures are silent. The CI generator, its proof and
.github/workflows/ci.ymlare byte-identical tomain.Issues
fjs/todo/module-tag-restore.md(P3) — the restore inventory, split by the four decisions it actually contains: 89types.tswith prose, mechanical; 16private.tsstripped by fjs: keep private types out of authored .mjs (Stage 1) #1750 on the same reading, restored for contributors but kept out of any public documentation build; 11 proof files where the rule says leave them, though that is worth confirming; and two that are neither —browser.mjshas real prose,testlib.f.mjsheld only@importtags and correctly wants no tag.todo/jsdoc-verification.md(P4) — how a JSDoc rule could be checked at all, given §6 rules out text patterns. Three passes over one tag with no signal is the argument for looking; "not worth it, leave it to review" is a legitimate outcome to record.fjs/ci/todo/node26-typedef-gate-reaches-consumers.md(P5) — narrowed after review corrected me:covandci-updateare a documented consumer contract, so a conforming project has a workingnode26, and the file-scope@typedefgate is the only step outside it.todo/types-ts-module-tag.mddeleted — it asked for a decision and its application; the decision is now made the other way and applying it ismodule-tag-restore.md. Its premise ("90 of 94 carry it") is false in both directions.Gates:
npx tscclean, suite 3533/3533,fjs/ciproofs 30/30.No changelog entry: documentation, issues, and a revert to
main's generated output.🤖 Generated with Claude Code
https://claude.ai/code/session_01T7vAocRuxfmWJDnujCoMup