Repository navigation
test(manifest): cover the safety contract's read/write mode resolution - #610
Conversation
|
@codex review Generated by Claude Code |
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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 386f16795f
ℹ️ 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".
| /// written out in full rather than derived. | ||
| #[test] | ||
| fn each_enum_writes_back_the_word_its_own_parser_reads() { | ||
| for v in [Lifecycle::Start, Lifecycle::Stop, Lifecycle::Single] { |
There was a problem hiding this comment.
When a new Lifecycle, Mode, or AgentStatus variant is added, these array literals remain valid without including it, so an incorrect as_str implementation for the new variant will still leave this test green. This defeats the stated goal of preventing variants from slipping past the parser/formatter round-trip check; add a compile-time exhaustiveness mechanism or otherwise assert that the enumerated cases cover every variant.
Useful? React with 👍 / 👎.
Review gate status — Codex is rate-limited, falling back to the local panel
So no cross-model review ran on this PR. CLAUDE.md §"PR review — non-negotiable" makes Codex primary; this is the explicit-refusal case, not silence, and there is no Codex CLI in this unattended cloud environment to try the other route with. Polling for a verdict naming The fallback named by that same rule is running instead — the vendored
That panel is same-model. Whatever it concludes, this PR will say so plainly: a maintainer reading it should not have to work out which reviewer let something through. Generated by Claude Code |
Correction: Codex did review — my previous comment was wrongThe usage-limit message was not the end of it. Codex posted a COMMENTED review on The finding is correct and I am fixing it, not rejecting it:
That is exactly right, and it lands on the one gap the test’s own doc comment admitted rather than closed ("The arms are exhaustive, so a new variant cannot slip past The local panel is still running and its results will be recorded here too, but it is no longer the gate. Generated by Claude Code |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary
Testing
|
Codex's P2 is fixed in
|
| # | Finding | Fix |
|---|---|---|
| F1 | every_suffix_the_app_spec_publishes_is_inferred_as_a_write read one line of app-spec.md and guarded with a count floor. Two reachable silent passes: suffixes published on a reflowed continuation line were skipped entirely; and swapping *.insert for *.export kept the count at 8 while .insert lost its only coverage in the repo — then dropping ".insert" from SUFFIXES left the whole suite green |
reads the whole bullet; asserts the set; extraction failure reported separately from contract drift |
| F2 | .save / .publish pinned by nothing |
the_suffixes_the_code_adds_beyond_the_spec_are_pinned_too |
| F3 | no added test went through validate_app_safety, which has four early continues before effective_mode — the resolver could be right while the gate stopped asking |
safety_check_demands_a_block_from_a_dotted_write_command_with_no_declared_mode, on an un-annotated revision.bump |
| F4 | two doc comments asserted falsehoods about the suite | corrected — see below |
F4 is the one I am least comfortable about, because CLAUDE.md's "ground every load-bearing claim in a citable artifact" applies to a comment as much as to a reply, and I shipped two that did not survive a grep:
- "
overridden… which no test reached" — false.app_lock::tests::compile_honors_mode_read_on_mode_overridable_commandreaches it end to end; the note it asserts is emitted only underif resolved.overridden. - "Nothing checked that the hand-written word is the word the derive accepts" — false for six of the eight rows. Both
Modewords viaapp_lock's lock strings, all threeAgentStatuswords viaregistry::catalog'sstatus_str,Lifecycle::Singlevia the catalog JSON golden. OnlyLifecycle::Start/Stopwere unpinned. - The
AgentStatusconsumer list namedcommands::app:1288, which matches an unrelatedRunEvent::RunEndstatus.
Both now state what was already covered and what is actually new.
Mutation ledger for this commit
Every fix proven under a mutation that was green before it and red after: the Lifecycle::Resume compile gate; three coordinated renames (Available→"ready", Mode::Read→"r", Lifecycle::Start→"begin") and one one-sided (Planned→"available"); the continuation-line scenario; the *.insert→*.export swap; a backtick-stripped bullet; .insert dropped from code while the spec names it; .save and .publish dropped; .bump dropped reaching the gate test end to end; validate_app_safety short-circuited to never ask; an emptied grounded-table row slice; and mode-overridable misspelled in a fixture.
Gates on 08fefc34, pinned toolchain 1.95.0: cargo fmt --all -- --check pass, cargo clippy --all-targets -- -D warnings pass, cargo test 65 binaries, 2090 passed, 0 failed, 1 ignored.
And the thing worth more than this PR
Reviewing the gate these tests feed turned up a live safety hole in shipped v0.146.0, filed as #611: validate_app_safety does not recurse into for-each do: bodies, so wrapping a write-mode node in for-each: '[1]' skips the safety: requirement entirely. Reproduced end to end — tekla.close refused at top level, accepted inside a loop, and compile then stamps it mode: write into the lock, so the compiler knows and the gate never asked. 10-core/app-spec.md:462 promises the opposite ("anywhere in the tree"), and the sibling validate_app_agents does recurse.
Filed rather than fixed here: this PR is test-only by remit, and that is a production change to a safety guard, which wants a human.
Generated by Claude Code
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Leaving this open for a human — the gate is not satisfiable on the final commitFinal head is No cross-model review covers this commit. Codex reviewed
The routine's limit is explicit: fix and re-run the panel once; if it is still not clean, stop and leave the PR open. That is the honest reading of where this is, so it stays open. What I got wrong, since it is the most useful thing for you to knowOne docstring took three attempts. It claimed first that nothing checked the
So exactly one thing in that test is new coverage: the exhaustiveness gate. Which is precisely what Codex's P2 asked for, and nothing more. I would rather the comment say that than oversell. That pattern — a justification written into the tree that does not survive a grep — is the thing worth your eye on the rest of this diff. The tests themselves held up under every mutation I and four reviewers could construct (see the two ledgers in the PR body); it was the prose that kept being wrong. Also worth your time, independent of this PR#611 — a live safety hole in shipped I think #611 matters more than this PR does. HousekeepingAn empty PENDING review from Generated by Claude Code |
Comment audit found five false claims — corrected in
|
| # | Claim | Verdict | Correct fact |
|---|---|---|---|
| 1 | with ".bump" deleted, cargo test -- --skip manifest::agent::tests is 2070 passed, 0 failed |
FALSE on its own tree | 1589 passed, 1 failed. The skip does not exclude validate.rs's safety_check_demands_a_block_from_a_dotted_write_command_with_no_declared_mode — the gate test this same PR adds, built on revision.bump. A recipe presented as reproducible that does not reproduce. Replaced with the correctly-scoped wording the validate.rs docstring already had; the bare total is gone, since it rots on every test added to the crate |
| 2 | Lifecycle::Start/Stop were the unpinned rows; Lifecycle::Single pinned via the catalog JSON golden; "six of eight" |
FALSE, inverted | Seven of eight were already pinned. Start and Stop by manifest::expose, which emits the word through as_str (expose.rs:101) and asserts the variant back after re-parsing (:289, :507, :508). Lifecycle::Single was the one genuinely unpinned row — changing it to "start" leaves 1590 passed / 0 failed outside the module. The catalog golden credited with it is parse input into a String field and never reaches Lifecycle::as_str |
| 3 | "Three of the four rows were already covered end-to-end" | FALSE | Two. (fixed, Some(Read)) had no prior coverage through effective_mode either — the validator's conflict check compares against mode_of (validate.rs:1381). That row is newly pinned too, not the pure defence-in-depth I called it |
| 4 | validate.rs: "every other safety test declares an explicit mode:" |
FALSE (minor) | Omits safety_check_skips_read_mode_nodes, the carve-out the agent.rs header states correctly. Added |
| 5 | variants blamed std::iter::successors; the extraction comment described an indentation test |
FALSE (minor) | The walk is a hand-written while let (same non-termination property, different name); trim_start runs first, so continuation lines are captured however indented — over-capture, which fails loudly |
Also corrected: the grounded table claimed a live safety consequence for all seven rows, but tekla save-attributes is status: planned — no valid app reaches it today, so the consequence does not yet bite for that row. Caveat stated rather than the row dropped; it is still the only shipped command grounding the save-attributes legacy arm.
Two pre-existing rots in the same file, fixed while it was open — the second is the more interesting one:
mode_of's own production doc comment listed ten of the twelve suffixes, omitting*.export-pdfsand*.export. A reader trusting it would have concludednavisworks clash.exportis a read. That is precisely the misreading these tests exist to prevent, sitting in the doc comment of the function under test.- "All 3 commands declare
category: curated" sat two lines above its own "(26 total)".
The honest shape of this PR
The code has held up: every added test is non-vacuous, ~35 mutations across three rounds all caught, two independent reviewers failed to break it. The prose has been wrong four times in the same place. That docstring's pin accounting took four attempts, and three of the four errors were in the direction of flattering the new test. The current version states only what was measured, with the measurement commands named.
That pattern is my recommendation for where a human reviewer spends their attention on this diff: not the assertions, the justifications.
Still: no cross-model review covers this head. Codex reviewed 386f167, raised the P2 that started all of this, and has been rate-limited since. e4ee7832 is three commits past what it read. The PR stays open for a human — unchanged from the decision above, and now with one more reason.
Generated by Claude Code
Audit tail closed in
|
| Cited | Actually | What the sentence described |
|---|---|---|
app_lock:570 |
:573 |
the mode recorded in the .lock (:570 is the compile note beside it) |
registry::catalog:216 |
:256 / :264 |
where the word enters the published catalog (:216 is status_str's signature) |
registry::catalog:111 |
:115 |
the site that compares the literal (:111 is the "available" serde default) |
Two claims were more certain than the code supports, and in both cases the honest version was already written elsewhere in the same file — which is its own small lesson:
- "the walk cannot miss it" sat 55 lines above
variants's own "one residual hole, named rather than hidden", which correctly notes a new variant can be given its own=> Nonearm, satisfying the compiler while orphaning itself from the walk. Now states what the gate does and does not guarantee. - "the spec and the code have to name the same things" ignores that
app-spec.md:806introduces its list with "e.g.". The four entries the code carries beyond the spec are therefore not a divergence — they are the open contract questionthe_suffixes_the_code_adds_beyond_the_spec_are_pinned_toodocuments. The real divergence is the spec naming a suffix the code does not match, or dropping one it still does, which is what the assertions actually check.
Tally, for whoever reviews this
Four rounds of comment correction on a test-only diff. The assertions were never the problem — ~35 mutations across three rounds, four reviewers, nothing broke a test. Every single defect was in the prose, and the recurring failure was overstating how little existing coverage there was, which flattered the new tests. Worth knowing before reading the docstrings: they are now accurate, but they got there by being wrong four times, and the measurements they quote are the ones to spot-check if you want to verify anything.
Head is 6279012c. Still open for a human: Codex reviewed 386f167, raised the P2 that began this, and has been rate-limited since — so no cross-model review covers this head or the four commits before it.
Generated by Claude Code
Round-2 panel: one reachable silent pass, closed in
|
| scenario | before | after |
|---|---|---|
| blank line then an indented continuation paragraph adding suffixes | GREEN | RED |
| fenced block in the rule, then more suffixes | GREEN | RED |
| a third bullet extending the rule | GREEN | RED |
| the eight moved into a nested sub-list | RED | RED |
| soft-wrap reflow (round-1 finding) | RED | RED |
*.insert swapped for *.export, count stays 8 |
RED | RED |
| stale recap earlier in the file, rule narrowed | GREEN | RED |
The cost is over-capture: the region also holds the "Pure-read nodes" sentence, so a reword putting a *.-token on a read line makes this test red. That is the right way round — it fails loudly, naming the token.
Also corrected, each measured first
- The gate test's rationale was wrong about why it earns its place. It cited the four early
continues invalidate_app_safety; each is already caught by two to five pre-existing safety tests. What nothing else catches is a normalising refactor at the gate —cmd_name.rsplit('.').next()— which kills all twelve dotted entries while leaving the two bare legacy names working. Measured: 1608 passed, 1 failed, this test alone in the suite. The docstring now says that. - The grounded table now leads with what it is uniquely load-bearing for. One reviewer reversed its own round-1 position here, with the measurement: as a
SUFFIXESpin the table is fully redundant (zero of seven rows is a sole catcher), but it is the sole catcher across all 2090 tests of a shipped command drifting out of the convention — renamerevision.bumptorevision.incrementand a live Revit write becomes a read. No fixture test can see that, because what changed is content, not code. I verified both halves. - "The five revit-2026 / navisworks / autocad-2026 rows" — six, contradicting the assertion two lines below it. Found by the fourth reviewer; introduced by the commit whose purpose was fixing false claims.
variantsdocumented one residual hole; there are two. Collapsing the arms to_ => Noneremoves the gate silently, and nomatchcan forbid a wildcard in thematchdoing the forbidding.- The negative loop's six rows shared one message naming neither the arm nor the consequence; each now carries the loosening it guards against.
Where this leaves it
Head be930e49. Gates green: fmt, clippy -D warnings, 65 binaries / 2090 passed / 0 failed.
The panel's own summary of the diff, which I think is fair: it takes the dotted convention from 0 of 12 suffixes pinned anywhere to 12 of 12, the e2e gate test has verified teeth, and the deletion was provably vacuous. Against that, six rounds of correction — one real silent pass and the rest prose, with the recurring error being overstatement in the direction that flattered the new tests.
Still open for a human, and the case is now stronger. Codex reviewed 386f167; the head is five commits past that and Codex has been rate-limited throughout. One reviewer put it exactly right: an approval naming 08fefc34 would not cover commits that rewrote the very comments under review.
Generated by Claude Code
|
@codex review Re-requesting on the current head The head is four commits past
Worth your attention most: whether the widened extraction over-captures (the region also holds the "Pure-read nodes" sentence), and whether the measurements quoted in the docstrings hold — those claims have been wrong before in this very PR. Generated by Claude Code |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review Retrying on head The only commit you have reviewed is Most worth attacking:
Generated by Claude Code |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review Head is now Fixed in
|
| Finding (reviewer) | Fix |
|---|---|
A sibling #### subsection naming *.purge was invisible: the region stopped at the first ####, suite stayed green (silent-failure-hunter, pr-test-analyzer) |
region now runs to the next ## |
A withdrawal note ("*.insert was withdrawn") kept *.insert counted as published (code-reviewer) |
tokens split into the normative bullet list vs everything else; a token outside the list fails with its own message |
A *.list read example failed with "the spec promises .list implies a write", the opposite of the spec (code-reviewer, pr-test-analyzer) |
same split, accurate message |
A bare, un-backticked *.purge added to the bullet left the suite green (comment-analyzer, O1) |
tokens are now found with or without backticks |
| Grounded table asserted a count of 7, which a duplicate-entry swap preserves (silent-failure-hunter) | asserts the set of convention entries; rename and mode-annotation messages say what to do |
effective_mode truth table lacked the reachable row (non-overridable, node restates write); a spurious overridden: true stayed green (pr-test-analyzer) |
row added |
Enum walk started from a hardcoded first, so a variant added ahead of it was never visited (code-reviewer, silent-failure-hunter) |
also walked backward from last; the two walks must agree. The "two residual holes" comment is corrected |
validate.rs claimed each early continue is caught by "two to five" tests (code-reviewer, pr-test-analyzer, comment-analyzer) |
Measured instead: only frozen has a test; flipping each of the other three to push an error leaves the suite green. Comment now says so |
Gates on 1c0b245b, pinned 1.95.0: cargo fmt --all -- --check 0, cargo clippy --all-targets -- -D warnings 0, cargo test 0 (65 binaries, 2090 passed, 0 failed).
Recorded, deliberately not acted on
comment-analyzer's remaining output is clarity and prose, which the routine says not to rewrite unattended. For a human reviewer, the factual items:
agent.rs:823says "five"revit-2026/navisworks/autocad-2026rows; the table has six.agent.rs:1128says nothing else in the file would notice amode_ofthat always answersRead; four other tests do.agent.rs:1330and:1383say "the one escape" wherevariantsdocuments two.app_lock:570should be:568(note body566-569).- The
("delete.thing", …)row label names a loosening it does not guard against. - Bare totals (
2090,1608,36,29) and about 20 cross-file line citations will go stale on unrelated edits.
Not in this PR
validate_app_safety does not recurse into for-each do: bodies. That is #611, fixed by #612. Two reviewers independently re-found it.
Housekeeping
An empty PENDING review from pawellisowski (id 5387982345) sits on this PR; deleting it needs a permission this unattended session does not have. It is invisible to everyone but its author.
Generated by Claude Code
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
🔴 CI red on
|
|
@codex review Head is now
Named as residual in the comments, not fixed: a write suffix spelled without the leading Gates on Still no cross-model review of this head: Codex last read Generated by Claude Code |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Addendum to the above: it is now two commits, and it will keep recurringNot a repeat of the blocker already reported — the shape changed. To whichever session is pushing these: your clone's git identity is git config user.name "Pawel Lisowski"
git config user.email "pawellisowski@o2.pl"That is the preventive half and it needs no force-push. Restamping the two already committed does, and the remedy in my previous comment is unchanged and still correct for both — Everything else on Generated by Claude Code |
Sweeper stopping here: left open, and I broke one check on this PRThe check I broke. I cannot repair it: the fix is to rewrite those two commits, which needs a force-push, and the routine bars that. A new commit does not help, since the gate checks every commit in the range. For a human, either of:
The tree at Codex. It last read Final local panel on
Same-model panel only, so per the carve-out this PR does not self-merge. Generated by Claude Code |
698d8aa to
7d1d5a2
Compare
|
@codex review Head is now No review covers this commit. The only review on record is yours from 2026-10-02 against Your P2 on What changed since you last looked, and where I would spend your attention:
Separately, and not fixed here: #611 is a live safety hole in shipped v0.146.0 — Generated by Claude Code |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
✅ Gate satisfied — ready to merge, but my session cannot press the buttonCodex has now reviewed the final commit and found nothing. Every condition in §Git workflow's carve-out is met, verified individually just now:
What stopped me: the merge call was refused by my own session's permission classifier, not by GitHub. The command, for whoever picks it up — note that gh pr merge 610 --squash --admin --delete-branch --body-file <body>Supply the squash body explicitly. Leave it out and GitHub generates one, and a generated body carries a One thing to carry forward, since it is the honest summary of this PRThe test logic was sound throughout — ~35 mutations across three rounds, four local reviewers, Codex twice, and nothing broke a test that should have held. Every defect found after the first commit was in the prose, plus one real silent pass in the spec-parity test. The recurring error was overstating how little coverage already existed, which flattered the new tests, and it recurred six times including twice in commits whose stated purpose was fixing it. The docstrings quote specific measurements; they are accurate now, but they are the part of this diff least worth taking on trust. Also still open and worth more than this PR: #611, the Generated by Claude Code |
1 participant
Summary
Read/write classification determines whether an app node must declare a
safety:block. Add regression coverage for dotted write names, manifest and node mode precedence, and the published enum words used by manifests, catalogs and lockfiles.Lifecycle,ModeandAgentStatuswire words and parser round-trips using exhaustive match helpers. Replace a legacy test of a test-local enum with an unknown-status assertion against the productionAgenttype.Updated against current
main, preserving its mode-basis tests and the 27-command Tekla manifest assertion. The PR history is consolidated under the maintainer identity so the commit-author gate can pass. Comments describe current coverage rather than historical suite counts.Scope and limits
Tests and documentation only; no runtime behavior or dependencies change. The spec tokenizer follows the current
*.<suffix>spelling and contiguous bullet-list formatting. The enum walks require exhaustive match arms, but do not enumerate variants automatically: a new variant wired only through its own arms can remain unreachable. These limits are documented in the tests.The nested-node safety defect reported as #611 has already been addressed on
mainby #612. This PR adds classification coverage; it does not claim to fix that defect.Validation
Verified on
7d1d5a2dc:cargo fmt --all -- --check: passed.cargo clippy --locked --all-targets -- -D warnings: passed.cargo test --locked: full local Windows suite passed, exit 0.sync_stats.py --check: passed.AWARE_HOME, using the shipped Navisworks manifest:clash.exportwithout safety is rejected by validation and installation; with safety it validates, installs and compiles, and the lock recordsmode: write. Compilation also records write classification for the unsafe preview without executing it.Review rounds: 1 (self-feeding: 0) — rounds: Codex CLI review of the operator-requested refresh; outcome: no actionable findings; operator: explicitly requested the refresh and aware-agent-tag workflow on 2026-10-11. Historical reviews remain recorded in the PR conversation.