feat(cli): refuse a staged review whose verdict stage cannot be named (#1821) - #2029
feat(cli): refuse a staged review whose verdict stage cannot be named (#1821)#2029jerryfane wants to merge 21 commits into
Conversation
3184acd to
30017b8
Compare
fa72972 to
f91a338
Compare
Verdict accepted. Two P1s and a P2, and one of them is blocked on this PR's parent rather than on me.Bound by hand, because the job row cannot: this responds to the review of PR #2029 at exact head The stacked-base finding is the one I asked for, and it did more than I askedI put the stacked base in the dispatch prompt and asked what a That is exactly the trap AGENTS.md records for a squash-merged parent - the reviewed branch commits become non-ancestors of the base - and it was reproduced with a synthetic merge rather than inferred from file overlap. This half is blocked on #2026 landing, not on a code change here: rebuilding now would rebase onto a parent that has not merged, and #2026 is itself awaiting review. I am not force-pushing a speculative rebase under a live review queue. The two P1s are real and are mine
P2, ordering of the refusal: the misconfigured-staged-review refusal returns after What I am doing, in order
Signed: gm-staged |
An `agent ask` dispatched as "review this PR at this head" returns a real review verdict, and it vanishes. Both RecordReviewFindingsToLedger call sites are inside `job.Type == "review"`, so for an ask the writer is never CALLED - its internal type guard is defence in depth and its recordLedgerSkip can never fire. No row, no event, nothing distinguishing a lost verdict from a review that found nothing. Measured on 2026-09-08: 15 succeeded gm-review-opus ask jobs, all 15 carrying a decision and 11 carrying findings, produced ZERO ledger rows and ZERO explaining events. Of those 15 decisions, 6 were changes_requested. Five were later superseded by bound verdicts; ONE IS LIVE - PR #2029 at 8bdf8f5, an objection at that PR's current head, invisible to every PR-keyed surface. Positive control on that zero: the same join over review-TYPE jobs the same day returns 28 jobs and 80 rows, so the instrument reports non-zero when there is something to find. THIS RECORDS THE LOSS; IT DOES NOT CLOSE IT, and that is the scope decision, not a shortcut. review_finding_observations is keyed by repo + pull_request + head_sha. A row missing all three answers no query anyone can write, and a synthesised key would assert a binding nobody established - the #2054 class. So an unbound verdict gets a RECORDED skip rather than a silent one or a fabricated row, and a bound-but-untyped verdict is named as the separate defect it is. Making a BOUND ask write real observations is deliberately excluded: that path inherits #2059's field-name mismatch, where the wire reads id/title/state while reviewers emit uid/evidence/disposition, and it would convert 15 silent losses into 15 empty OPEN findings. #2059 first. Five test arms. The production-path arm is the one that matters and is also the base control: it drives AdvanceJob with the measured shape and compiles on main, where it fails with the defect verbatim - the only event on the ask job is `succeeded`. The other three arms call the helper directly and would all pass against a branch where the helper exists and nothing invokes it, which is the same defect one level up. The fourth is a control on over-firing: an implement result, a bare answer and a nil result must not be treated as review verdicts.
185ad0c to
a6a9937
Compare
…#1821) Fourth and last slice of the staged review. STACKED ON #2026, which adds the config declaration this reads; it will need a rebuild on main once that squash-merges, because a squash makes the parent's commits non-ancestors. The design correction that shaped this: JobRequest carries no Delegations. A fan-out is emitted by the parent AGENT's own gitmoot_result and expanded by the engine, so a two-stage review is NOT an enqueue of parent-plus-child. That is the better shape - the child necessarily arrives through ensureDelegatedReviewEvidence, a chokepoint that already refuses a parent whose children crashed, are unrecognised, are abstaining or are parked, and already refuses a fan-out that declared delegations and produced ZERO children - and it means the dispatcher's whole job is to REFUSE a declaration it cannot honour and to TELL an honourable one what to delegate. So the non-fallback clause now has four independent enforcement points, and no single edit weakens it: - the parent's result is a fan-out, so it can never be a verdict (#1685); - a declared-but-absent child is an unfilled slot, not a pass; - a runtime that cannot execute terminates BLOCKED, and a blocked child makes the gate refuse the parent (#2022); - a verdict stage that cannot be NAMED refuses the dispatch (this slice). And the verdict it does produce cannot exceed what its preflight found runnable (#2024). The refusal happens BEFORE the job row exists, which is the same judgement as #1819's foreign-commit arm and #1817's dispatch precondition: a dispatch that cannot possibly produce a valid review should not become a job somebody later has to interpret. It names the config key to fix, because an operator reading a refusal needs the remedy and not just the verdict. An unreadable config is NOT a refusal. The declaration is the opt-in and a config that cannot be read cannot express one, so failing closed there would break unstaged reviews on the same repo. A mutant that fails closed is killed by TestStagedReviewMissingConfigDoesNotRefuse. The preflight block is appended AFTER dispatchPromptHeadContradictionWarnings for #1819's reason, and it is asserted to contain no commit-shaped token: an agent name COULD be hex-shaped one day ("deadbeef" is a legal agent name), so the test checks the RENDERED block rather than trusting the input. A mutant that interpolates a SHA is killed by it. Seven tests, four mutants killed: accept an unregistered agent, accept an implement-only agent, fail closed on an unreadable config, and interpolate a commit-shaped token. The should-succeed arm is explicit - a declared, registered, review-capable agent is accepted, and an undeclared repo takes the unstaged path byte-identically. Reuses the existing agentHasCapability rather than adding a second copy; my first draft added one and the compiler caught the duplicate. Verification: build, vet, gofmt, generate clean, -run filtered tests including the pre-existing TestReviewBriefDoesNotTripTheHeadContradictionScan, which passes unchanged. No untargeted internal/cli or ./... run.
) COMMIT ONE OF TWO. This is the MARKER; the evidence ceiling that reads it is the next commit, separate so a reviewer can read the marker land before the thing that consumes it. WHY A MARKER AND NOT AN INFERENCE, which is the whole reason this commit exists as its own thing. PR #2024 carried the same ceiling and inferred "this is a staged pair" from the parent having DECLARED `evidence`. A reviewer broke that with the case the inference misses: an honest review COORDINATOR also declares evidence=static_only for itself, because RESULT_CONTRACT.md instructs it to when it orchestrates without executing - while its lens children genuinely DO execute in their own worktrees. Reproduced before accepting: ceiling handed to EVERY child of an honest coordinator: "static_only" lens evidence downgraded to "static_only" despite TestsRun=[go test ./...] That contaminated precisely the rows ensureDelegatedReviewEvidence depends on, since the gate treats lens children as the only evidence a fan-out produces. #2024 was closed for it and the mechanism moves here. So the staged relationship is now STATED by the dispatch that creates it: JobRequest.StagedReviewVerdictAgent and JobPayload.staged_review_verdict_agent are set from the same config resolution that produces the preflight prompt, so a preflight can never carry instructions naming one agent while its marker names another. Consumers ask the marker instead of deducing the relationship from a property other job shapes share. Additive and omitempty: every job that is not a staged preflight serializes byte-identically, which a test asserts rather than a comment claiming it. No consumer yet, deliberately - that is commit two. A marker landing one commit before its reader is not scaffold; a reader landing before its marker would be.
…to the marker (#1821) COMMIT TWO OF TWO. The marker landed in the previous commit; this is the ceiling that reads it. It is the mechanism PR #2024 carried and was closed for, with the scoping defect fixed. WHAT #2024 GOT WRONG. It inferred "this is a staged pair" from the parent having DECLARED evidence, and wired the ceiling into the fully generic delegationRequest. A reviewer broke it with the case that inference misses - an honest review COORDINATOR declares static_only for itself, because RESULT_CONTRACT.md instructs it to when it orchestrates without executing, while its lens children genuinely execute in their own worktrees. Reproduced before accepting: ceiling handed to EVERY child of an honest coordinator: "static_only" lens evidence downgraded to "static_only" despite TestsRun=[go test ./...] That contaminated precisely the rows ensureDelegatedReviewEvidence reads as a fan-out's only evidence, so it attacked the gate's own evidence source. WHAT THIS DOES INSTEAD. stagedVerdictCeiling requires BOTH conditions: the parent must carry the explicit staged marker, and this delegation must be the one the marker NAMES. An unmarked coordinator imposes nothing on anything, and a marked preflight that also delegated something else constrains only its verdict. An undeclared preflight imposes nothing either: silence is not a finding, and defaulting it would clamp on the engine's own default rather than on an observation. THERE IS DELIBERATELY NO PROPAGATION. #2024 carried a fallback passing a parent's own inherited ceiling further down, to stop a chain "laundering" the constraint by inserting a silent hop. With an explicit marker that hole does not exist, and the fallback was the mechanism that spread the contamination. The verdict child does not carry the marker, so nothing beyond it is constrained. The clamp still runs at the mailbox seam BEFORE the result is stored, so no consumer reads the unclamped claim, and records inherited_evidence_clamped - because an overruled producer and one that declared static_only itself are different facts with the same stored value. evidence_declared stays TRUE on a clamp: #1817 built that flag to separate a producer that spoke from a silent one, and an overruled producer still spoke. TESTS. Nine, and the first is the P1 regression: an unmarked coordinator's lens children are never clamped. It asserts stagedVerdictCeiling rather than the clamp, because the defect was in deciding WHO inherits, not in the arithmetic. The P3 from the same verdict is answered by a real engine chain rather than two hops of hand-built payloads: preflight -> verdict child -> coordinator continuation, asserting the verdict child inherits, does NOT carry the marker, and the continuation carries neither. Measured while writing it: a delegation child's own delegations do not become grandchildren at this depth, they route into a continuation of the original parent - so the test asserts the chain the engine actually produces rather than the one I assumed. Direction is pinned in both senses, including the case a mutation survived on #2024: static under static must stay static, because two admissions that nothing ran cannot add up to an execution claim.
An advisory review of my own commits found the gap and it is the same shape twice over: every test in the ceiling file either calls stagedVerdictCeiling directly or hand-builds a JobPayload, so a mutant deleting either field from Enqueue's payload literal left all of them green while neither the marker nor the ceiling ever reached the store. That is the pool-seam gap from #2018 and the coordinator gap from #2024 in a third place: the decision was tested, the WIRING was not. Verified by running that mutant. Dropping both fields from the payload literal now fails three tests, including this one; before it, it failed none of them. Also verified, because I asserted it without measuring first: a permission-blocked review never reaches the staged resolution at all. The two blocked returns are at agent_dispatch.go:310 and :362 and the resolution is at :574, so a blocked job carries no marker by construction rather than by omission - which is correct, since a job that never runs has no verdict stage to constrain.
8bdf8f5 to
db43773
Compare
An `agent ask` dispatched as "review this PR at this head" returns a real review verdict, and it vanishes. Both RecordReviewFindingsToLedger call sites are inside `job.Type == "review"`, so for an ask the writer is never CALLED - its internal type guard is defence in depth and its recordLedgerSkip can never fire. No row, no event, nothing distinguishing a lost verdict from a review that found nothing. Measured on 2026-09-08: 15 succeeded gm-review-opus ask jobs, all 15 carrying a decision and 11 carrying findings, produced ZERO ledger rows and ZERO explaining events. Of those 15 decisions, 6 were changes_requested. Five were later superseded by bound verdicts; ONE IS LIVE - PR #2029 at 8bdf8f5, an objection at that PR's current head, invisible to every PR-keyed surface. Positive control on that zero: the same join over review-TYPE jobs the same day returns 28 jobs and 80 rows, so the instrument reports non-zero when there is something to find. THIS RECORDS THE LOSS; IT DOES NOT CLOSE IT, and that is the scope decision, not a shortcut. review_finding_observations is keyed by repo + pull_request + head_sha. A row missing all three answers no query anyone can write, and a synthesised key would assert a binding nobody established - the #2054 class. So an unbound verdict gets a RECORDED skip rather than a silent one or a fabricated row, and a bound-but-untyped verdict is named as the separate defect it is. Making a BOUND ask write real observations is deliberately excluded: that path inherits #2059's field-name mismatch, where the wire reads id/title/state while reviewers emit uid/evidence/disposition, and it would convert 15 silent losses into 15 empty OPEN findings. #2059 first. Five test arms. The production-path arm is the one that matters and is also the base control: it drives AdvanceJob with the measured shape and compiles on main, where it fails with the defect verbatim - the only event on the ask job is `succeeded`. The other three arms call the helper directly and would all pass against a branch where the helper exists and nothing invokes it, which is the same defect one level up. The fourth is a control on over-firing: an implement result, a bare answer and a nil result must not be treated as review verdicts.
Decision: Findings
Tests Run
Raw runtime output was retained in local Gitmoot state and is not posted here. |
…ceiling, and provide the worktree it assumes (#1821) ALL FOUR #2029 FINDINGS ACCEPTED at head db43773, none contested. P1, AND THE MOST SERIOUS THING FOUND IN MY WORK TODAY. The marker did ONE thing: pick which delegation receives InheritedEvidence. Nothing checked that the preflight emitted a review delegation targeting that agent, and the preflight's result is untrusted input. So a preflight could emit an ask child, or a review delegated to a DIFFERENT registered agent, and that child received NO ceiling while ensureDelegatedReviewEvidence still counted its approved decision as the fan-out's evidence. The approval then existed without the configured strong reviewer ever running - the campaign's non-fallback rule failing at the one point it is for. enforceStagedVerdictContract now refuses a marked preflight unless: exactly one delegation, action review, the agent the marker names, that agent carries review, and it can access this repo. Refusing is the only safe answer - dispatching the wrong shape unconstrained is what produced the hole, and silently dropping extra children would discard work without saying so. Identity is re-checked at advance rather than trusted from dispatch, because an agent can lose a capability or a repo grant in between. P1: AN "EXECUTED" CLAIM NAMING NO EXECUTION LIFTED THE CEILING ENTIRELY, since executed is the permissive value and clamps nothing. The general substantiation check could not cover it: that check runs only when len(Delegations) == 0, so a staged fan-out - whose defining shape is exactly one delegation - is exempt BY CONSTRUCTION. A marked preflight declaring executed with no tests_run and no changes_made now clamps to static_only. The bar is one non-empty entry, on purpose: this separates "executed, and here is what" from "executed" asserted against nothing, and a cheap stage that ran one real command is legitimate. P1: A CONFIG THAT CANNOT BE READ NOW REFUSES. The old code collapsed every error into unstaged, arguing a read failure "says nothing about whether a declaration exists". That is true, and it is the reason to refuse: LoadStagedReview parses the WHOLE config, so a malformed staged value for ANOTHER repository silently unstages this one and the review runs on the cheap agent with no marker - a silent fallback reached through a typo elsewhere in the file. Only fs.ErrNotExist is unstaged, because an absent config cannot be hiding a declaration, which keeps a fresh home byte-identical. agent_dispatch.go's swallowed pathsFromFlag error is propagated too; ignoring it would have made this correction unreachable through that call site. P2: THE SOUNDNESS ARGUMENT ASSUMED A SHARED WORKTREE THAT WAS NEVER PROVIDED. delegationRequest inherited no WorktreePath, and because a staged preflight emits exactly ONE read-only child, readOnlyFanoutNeedsWorktree is false - it needs two or more siblings - so the worker could allocate a different exact-head worktree or a shared checkout, with a different runtime and different credentials. Then "cannot execute here" is not a fact about the child's tree and a static_only ceiling can downgrade evidence the verdict agent legitimately produced. The marked child now inherits the preflight's worktree, and stagedVerdictWorktree reuses stagedVerdictCeiling's own answer as the discriminator so ceiling and environment cannot disagree. Five mutants killed, each gated on applied-and-builds: removing the enforcement, allowing extra siblings, dropping the substantiation clamp, dropping the worktree inheritance, and collapsing config errors back into unstaged. Full packages green: internal/workflow 109s, internal/cli 646s, internal/config 0.5s.
All four accepted, none contested. Stale-verdict notice.
Attribution: implementer is the P1: the verdict delegation is now enforced, not trustedThis is the most serious thing found in my work today. The marker did exactly one thing: pick which delegation receives So a preflight could emit an ask child, or a review delegated to a different registered agent, and that child received no ceiling while
Refusing is the only safe answer. Dispatching the wrong shape unconstrained is what produced the hole, and silently dropping extra children would discard work the preflight asked for without saying so. Identity is re-checked at advance rather than trusted from dispatch. P1: an executed claim naming no execution no longer lifts the ceiling
The general substantiation check could not cover this. It runs only when The bar is one non-empty entry, deliberately low: it separates "executed, and here is what" from "executed" asserted against nothing, and a cheap stage that ran one real command is a legitimate result. It clamps rather than refuses, because the preflight ran and its verdict child is still wanted. P1: a config that cannot be read now refusesMy old comment argued a read failure is not a refusal because it "says nothing about whether a declaration exists". That is true, and it is the reason to refuse. Only
P2: the soundness argument assumed a worktree that was never provided
The marked child now inherits the preflight's worktree. Mutants
Each gated on applied and builds. Verification
One test skips as root and says so: mode Signed: gm-staged |
Decision: Findings
Tests Run
Raw runtime output was retained in local Gitmoot state and is not posted here. |
Decision: Findings
Tests Run
Raw runtime output was retained in local Gitmoot state and is not posted here. |
…tree race, resolve staging before mutating (#1821) ALL THREE #2029 ROUND-TWO FINDINGS ACCEPTED at head a7e1591. One is a WITHDRAWAL: my previous fix introduced a live race and removing it is the fix. P1: THE ZERO-DELEGATION CASE IS ENFORCED BEFORE THE EARLY RETURN. My contract check sat AFTER dispatchDelegations' len==0 return, so a marked preflight that approved and delegated NOTHING passed through untouched - and because StagedReviewVerdictAgent is not part of ResultIsFanOut it was then treated as an ordinary review and could enter the native approval lifecycle. An approval with the configured verdict agent never running: the same hole the contract exists to close, reached by emitting nothing instead of emitting the wrong thing. MY OWN SUBTEST ASSERTED THE BYPASS AS CORRECT. I wrote "no delegations is the existing early return, not a refusal" and then tested for exactly that, which is why one round of review did not find it: the test agreed with the code. An explicitly blocked or failed preflight is exempt, because it emitted no verdict delegation for the honest reason - it could not perform the review, which is #1823's own requirement - and turning that into a dispatch error would convert a correct refusal into an engine failure. A marked preflight with NO RESULT is refused, since it is indistinguishable from one that never ran. P1 WITHDRAWN, NOT FIXED: THE VERDICT CHILD NO LONGER INHERITS THE PREFLIGHT'S WORKTREE. Round two added that inheritance to make ApplyInheritedEvidenceCeiling's soundness argument true. The review found the lifetime hole: AdvanceJob has already deferred cleanup of the parent-owned read-only worktree, so the parent's own advance force-removes the path the queued child was handed, and the child inherited neither the read-only seat nor the runtime config directory that make it usable. Handing over a directory that is about to be deleted is worse than not sharing one. So the comment now records the TRUTH rather than the intent: the environment half of that argument is NOT established, the ceiling can under-report when the verdict seat is richer than the preflight's, and making it sound needs either worktree ownership transferred until the verdict child is terminal or a ceiling computed from the child's actual seat. Both are larger than a fix round. The question is escalated; it is not papered over, and the test that pinned the withdrawn feature is deleted rather than weakened to match it. P2: THE STAGED DECLARATION IS RESOLVED AND VALIDATED BEFORE ANY MUTATION, with only its prompt block appended later. It used to resolve after a reviewing task and a detached worktree already existed, and a refusal returned past the Enqueue-error rollback - leaving an ownerless worktree and possibly a task with no job, once per retry. A refusal must cost nothing. The append still has to follow #1819's contradiction scan, so the two halves are separated deliberately. Two mutants killed, gated on applied-and-builds: enforcement back after the early return, and refusing a blocked preflight too. Full packages green: internal/workflow 117s, internal/cli 819s, internal/config 0.4s.
Round three. All three accepted, and one is a withdrawal.
P1: the zero-delegation case is enforced before the early returnMy contract check sat after An approval with the configured verdict agent never running: the same hole the contract exists to close, reached by emitting nothing instead of the wrong thing. My own subtest asserted the bypass as correct. I wrote "no delegations is the existing early return, not a refusal" and tested for exactly that. That is why a full round of review did not find it - the test agreed with the code, so there was no disagreement inside the artifact for anyone to notice. Two exemptions, both deliberate:
P1 withdrawn, not fixed: the worktree inheritance is goneYou are right, and this is the finding I was most worried about before reading it. Round two added the inheritance to make Handing over a directory that is about to be deleted is worse than not sharing one. So the inheritance is withdrawn and the comment now records the truth rather than the intent:
Both are larger than a fix round, so the question is escalated rather than papered over, and the test that pinned the withdrawn feature is deleted rather than weakened to match it. I would rather this PR carry an honest open question than a mechanism that races a directory removal. P2: staging resolves before any mutationIt used to resolve after a reviewing task and a detached worktree already existed, and a refusal returned past the Only the prompt block is appended later, because that append still has to follow #1819's contradiction scan. The two halves are separated deliberately, not moved together. Mutants
Each gated on applied and builds. Verification
Signed: gm-staged |
The withdrawn P1 is now filed as an issue, per phobos's rulingThe soundness gap this PR records in a code comment is filed as #2082, because a live soundness gap in merged campaign work does not survive inside a merged PR's prose. It states the gap in the direction phobos required - the environment half of the argument is NOT established, not partially established - and names both candidate remedies as the different products they are:
The issue also records the failure direction that makes this easy to miss: the ceiling fails safe against a lying verdict and unsafe against an honest one, so it can downgrade evidence a richer verdict seat legitimately produced. And the zero-delegation finding is filed as a class#2083. My subtest asserted the bypass as correct, so the test and the defect were written from one wrong belief and the artifact contained no disagreement for a reviewer to find. That is a category past #2076's impossible fixtures: an impossible fixture makes a defect invisible; a ratifying test makes it look decided. Mutation testing cannot reach it by construction - the mutant restores the behaviour the test demands and is scored as survived-because-correct. The issue names three candidate detectors, best first, with the false-positive cost of the only one that is close to mechanizable. Signed: gm-staged |
Decision: Findings
Tests Run
Raw runtime output was retained in local Gitmoot state and is not posted here. |
…onsumer (#1821) ALL THREE #2029 ROUND-THREE FINDINGS ACCEPTED at head b56dd23. One is fixed structurally, one narrowed, and the third's remedy stays escalated with its false claim corrected. P1: ENFORCEMENT NOW PRECEDES EVERY REVIEW-RESULT CONSUMER. Round two moved it above dispatchDelegations' early return, which closed the zero-delegation bypass and left it DOWNSTREAM of everything else AdvanceJob does with a review result. So a marked preflight could still have its findings written to the #1822 ledger and move the task to changes_requested - the CHEAP stage acting as an ordinary reviewer - before anything checked whether it was allowed to be one. Moved to the top of AdvanceJob, above the high-risk lens normalization, for the same reason that block sits early: a refusal must precede the consumers rather than race them. Two new refusals close the rest of that boundary. A marked preflight must DECLARE its evidence, because without a declaration stagedVerdictCeiling returns "" - no ceiling at all - so an omitted declaration removed the mechanism as effectively as naming the wrong agent; shape and identity were checked while the one field the ceiling is computed FROM was not. And a marked preflight may not return its own changes_requested verdict or its own findings, because a preflight answers whether the review can be performed and the verdict belongs to the stage it names. P1: CHANGES_MADE IS NOT EXECUTION EVIDENCE. stagedPreflightNamedExecution accepted any nonblank changes_made entry, so evidence=executed with tests_run=[] and changes_made=["no changes"] returned the PERMISSIVE executed ceiling while naming no command and running nothing. A read-only preflight's changes_made is prose about what it would change; only tests_run identifies something it ran. P2: THE FALSE CLAIM IS CORRECTED, THE REMEDY STAYS IN #2082. Removing the worktree inheritance ended the use-after-cleanup race and did not establish the shared environment, and ApplyInheritedEvidenceCeiling's comment still ASSERTED both the head and the environment as fact. It now states which half holds: the head is shared, the environment is not, delegationRequest transfers only InheritedEvidence, and a single read-only child does not qualify for readOnlyFanoutNeedsWorktree - so the child can run in a different seat with different credentials. Four mutants killed, each gated on applied-and-builds: enforcement back at dispatch time, dropping the declared-evidence requirement, letting the preflight request changes and report findings, and counting changes_made as execution. THE LAST ONE SURVIVED FIRST: my ceiling test left changes_made nil, so it could not tell the two predicates apart. The reviewer's exact case - tests_run empty, changes_made ["no changes"] - now pins it. The ordering claim is tested through AdvanceJob rather than the contract, because a contract-level test cannot observe that the ledger stayed empty. PRE-EXISTING ON MAIN, NOT FROM THIS BRANCH: internal/cli's TestBlockedAdvanceSettlesQueriedJobState, TestBlockedAdvanceEmitsRoutableTerminal Event and TestRuntimeCredentialCurationForegroundAndDaemonE2E fail on origin/main at 476f4f3. Verified in a detached worktree at origin/main with no local changes. Reported separately. Green here: workflow 119s, daemon 16s, config 0.4s.
f7 fixed. f4 is #2082 and deliberately out of scope for this round.
f4 (P2) is tracked, not neglectedThe finding is correct and it is #2082's exact subject: the verdict child does not inherit f7 (P1): the reuse answered a nearby questionRound four made
M3 is the control that matters. A denial list is a clamp, and an eager one refuses real evidence. Two of the five mutants initially survived, and both were my own instrumentsM2 survived because the list contained dead phrases. M4 "survived" because my mutant script's test regex did not include the tests I had just written. The code was correct and the instrument was not. Then M2 and M5 reported ANCHOR MISSED twice from |
Decision: Findings
Tests Run
Raw runtime output was retained in local Gitmoot state and is not posted here. |
Round six P1: the list shape was the defect, not the list contents
The finding is exact. Enumerating voices is unbounded. Two properties carry the correctness, and both are now pinned by tests rather than by the comment: Order. The negation must precede the verb, so Clause locality. M2 is the regression proof. Restoring contiguous-only matching reproduces the reported behaviour and fails the new fixtures, including the reviewer's exact string. A test was removed, and why that is not a weakening
It is replaced by guards for the two ways the new shape can fail, which is what the old test was doing for the old shape: Known limitation, stated rather than discovered laterA sentence like |
Decision: Findings
Tests Run
Raw runtime output was retained in local Gitmoot state and is not posted here. |
Round seven: the character window was the defect, and widening it would not have been a fix
The finding is exact and it names my own arbitrary number. Widening 32 to 64 would move the boundary, not remove it, and the next natural sentence would be 65. So the bound is gone and replaced by two things that are properties of the text rather than of my guess:
First-verb anchoring is not decoration; it is what keeps a real execution counted. M4 is reported as not counted rather than as a kill. M2 already exercises the same property from the other side: it is exactly "stop caring where the negation sits relative to the verb", and it fails on the two first-verb controls. New fixtures: the reviewer's 41-character string, a longer 60-character variant, and the two first-verb controls above. What this round did not doI did not add the reviewer's sentence as a literal. Three rounds have now shown that enumerating this class does not converge: a literal list missed the passive voice, and a character window missed a long clause. Each time the fix was to make the rule describe the text's structure instead of its surface. |
Decision: Findings
Tests Run
Needs
Raw runtime output was retained in local Gitmoot state and is not posted here. |
Round eight: the fourth revision, so I stopped revising the threshold and removed the property
The finding is right and it defeats the rule I shipped last round. In Four rounds on one predicate:
Each fix described the sentence more precisely; each was beaten by a sentence it did not anticipate. A rule needing a fourth revision is not converging on a threshold, it is telling you the property is not the one being measured. Position is gone entirely. The rule is now: an entry claims an execution when some clause names a runnable target and carries no denial. No ordering, no window, no verb list. The tradeoff is deliberate and it is asserted, not implied
Two fixtures assert the clamp rather than leaving it to be discovered. Two defects my own fixtures caught, both in the splittingSplitting clauses on Clauses are now split on tokens: a token ending in M2 survived the first run and that was a real gap, not a harness fault. Every fixture I had put the denial and the target in the same clause, so dropping the requirement that they share one changed no outcome. The fixture that kills it is M4 is the control that keeps a failing suite counted. The denial phrases are phrases: |
Decision: Findings
Tests Run
Raw runtime output was retained in local Gitmoot state and is not posted here. |
Round eight: I stopped arguing about sentences and measured the ledger
The finding is right, and its suggested word is measurably wrong
A repository whose product emits What I added, and it is a regression I introducedThe literal list I replaced in round five carried Every one of those escaped between rounds six and eight. Found by measuring, not by review. Both structural exits are measurably closedI checked whether to abandon the blacklist entirely. Both alternatives are shut, on this store:
So the honest statement is in the source: this is a best-effort filter over producer prose, complete for the shapes measured here and defeatable by a sentence nobody has written yet. The durable fix is a producer-side contract - a stage declares execution in a field, not a paragraph - which changes every runtime and does not belong in this PR. M3 survived its first run and that was my fixture's fault. My contamination controls put |
Decision: Findings
Tests Run
Raw runtime output was retained in local Gitmoot state and is not posted here. |
…he none-run fixtures (#1821)
Round nine: the phrase, and two fixtures that could not isolate what they claimed
f1 (P1): the census ruled out the word and never measured the phraseCorrect, and the distinction is the reusable part. Round eight refused Measured now, on the same 23,841 entries: Zero. So the contextual phrase clamps nothing that exists and cannot contaminate, while closing your probe exactly. A word a product emits as a result state is unusable as a denial marker; the same word in a phrase naming the sandbox as the actor is not. f3 (P2): my fixtures were satisfied by a rule other than the one they namedAlso correct, and it is the sharpest of the three. Both A fixture that two rules both satisfy tests neither. Two target-bearing fixtures added with no second denial marker, and your mutant now fails: What I am not claimingThis is round nine, and the source says plainly that the predicate is a best-effort filter over producer prose. The durable fix is now filed as #2104: the structured execution fields already exist and are populated in 0 of 3,828 review results, so the schema is not the missing part, the producers are. That needs an owner ruling and spans every runtime, which is why it is an issue and not this PR. |
Decision: Findings
Tests Run
Raw runtime output was retained in local Gitmoot state and is not posted here. |
…ejected generalisations recorded (#1821)
Round ten: the passive voice, two rejected generalisations, and a mutant that found dead code
The P1 was the passive of the sentence round nine fixed
Two cheaper generalisations, both rejected on measurement
Clamping a full suite run is the expensive direction. A control fixture pins the rejection: A mutant survived, and it was right toI wrote the window as The cause is that The fixture is kept, because the behaviour is real, with its comment corrected to say the splitter is what guards it. M1 and M2 were first reported as surviving by a shell loop that mangled the regex anchors. Re-run in Python, both die. Had I trusted the shell result I would have reported this predicate as untested and sent someone hunting a defect that does not exist. |
…viewed heads (#1419) BOTH #2066 ROUND-FIVE P1s ACCEPTED at head e487f0d. The other four items at that head confirm earlier rounds rather than reporting defects. P1: THE ROW'S OWN Line DISAMBIGUATES ITS LOCATOR. Round five reused splitLocator, which cuts at the FIRST colon, and that is wrong in both directions for paths this store accepts: pkg:a.go:10 -> the first cut leaves "a.go:10", Atoi fails, the raw locator is kept, and three rounds on one file stay three buckets; pkg:10 -> the first cut leaves "10", Atoi succeeds, and a real FILENAME is folded to "pkg". Cutting at the LAST colon fixes the first and keeps the second wrong, because "pkg:10" is genuinely ambiguous AS TEXT. It is not ambiguous in the ROW: Line was parsed and stored separately when the observation was written. So the suffix is stripped only when it MATCHES the recorded line, which also absorbs the whitespace shape ("a.go: 10") that db.splitPathLine trims and splitLocator does not - the parser disagreement the review names. An observation with no recorded line keeps its locator whole. That direction is deliberate: it can leave two spellings of one file in separate buckets and under-report, and it can never fold two files together and INVENT a relocation. P1: THE REVIEWED HEAD IS PART OF THE IDENTITY. Both rungs omitted it, so two children of one coordinator that reviewed DIFFERENT heads collapsed into a single round - a coordinator may declare dependent review legs, deferred legs are enqueued after their dependencies settle, and delegationHeadSHA resolves the then-current PR mirror, so a push between those dispatches gives them different heads. Same-head siblings still collapse, because the head is equal for them; only a head change separates them, which is exactly a new round. I WROTE THAT COMMENT BEFORE THE CODE AND CAUGHT IT MYSELF: the first version of this commit explained the head in prose while both rungs still omitted it, which is precisely the false-claim shape I filed against my own #2029 comment one round earlier. Fixed before running anything. Three mutants killed, each gated on applied-and-builds: reverting to splitLocator's first-colon cut, stripping the suffix without checking the recorded line, and dropping the head from both rungs. The fixtures now set File AND Line together, because the store writes them together and a File-only fixture models a row the writer does not produce. Full internal/workflow green in 79s.
Decision: Findings
Tests Run
Raw runtime output was retained in local Gitmoot state and is not posted here. |
f4 is disposed, not ignored: withdrawn with a reason, per a ratified dispositionFive rounds have now re-derived the same finding, and #2082's own body already recorded its disposition:
Nobody read it back. Four reviewer slots re-derived a finding whose tracking issue says it does not block, which is this campaign's own subject: a record that answers the question, unread by the people asking it. That disposition is not self-ratifying - it was written by the seat that authored the PR, in an issue about its own work. It has now been ratified by phobos (2026-09-09), who did not author this PR, on the grounds of failure direction rather than convenience:
phobos also checked something I had not: What this is notNot answered. Nothing was fixed. Recording it answered would assert a check nobody made. Not a reviewer being told to disregard a live finding. The narrow precedent, stated so it cannot spread: a reviewer may not be instructed to set aside a live finding; a finding may be withdrawn when a tracked issue records its disposition and a party who did not author the PR ratifies it. The difference is that the second leaves a record with a reason and a name on it.
The design choice between #2082's two remedies - transfer worktree ownership until the verdict child is terminal, or compute the ceiling from the child's actual seat - is an owner decision and nobody should build either tonight. This PR stays a draft until it passes; it fails criterion 1 regardless. |
1 participant
Refs #1821, #1823. Fourth and last slice of the staged review, campaign #1818.
STACKED ON #2026, which adds the config declaration this reads. Base is
feat/1821-verdict-agent-declaration, notmain. It will need a rebuild onmainonce #2026 squash-merges, because a squash makes the parent's commits non-ancestors of the base - the exact trap that silently re-added ~10,200 deleted lines on an earlier stack in this repo.The design correction that shaped this slice
JobRequestcarries noDelegations. A fan-out is emitted by the parent agent's owngitmoot_resultand expanded by the engine, so a two-stage review is not an enqueue of parent-plus-child. My own design note on #1821 implied it was; reading the code corrected it.That is the better shape, because the child then necessarily arrives through
ensureDelegatedReviewEvidence- a chokepoint that already refuses a parent whose children crashed, are unrecognised, are abstaining or are parked, and already refuses a fan-out that declared delegations and produced zero children. Staging inherits a defended guard instead of inventing one.It also means the dispatcher's entire job is: refuse a declaration it cannot honour, and tell an honourable one what to delegate.
The clause now has four independent enforcement points
No single edit weakens it:
undispatchedFanOuts, pre-existingblocked, and a blocked child makes the gate refuse the parentAnd the verdict it does produce cannot exceed what its preflight found runnable (#2024).
Two decisions worth reviewing hardest
The refusal happens before the job row exists. Same judgement as #1819's foreign-commit arm and #1817's dispatch precondition: a dispatch that cannot possibly produce a valid review should not become a job somebody later has to interpret. The message names the config key to fix, because an operator reading a refusal needs the remedy and not just the verdict.
An unreadable config is not a refusal. The declaration is the opt-in, and a config that cannot be read cannot express one - so failing closed there would break unstaged reviews on the same repo. A mutant that fails closed is killed.
Tests and mutants
Seven tests. Four mutants, all killed:
...RefusesAnUnregisteredVerdictAgent...RefusesAnAgentWithoutTheReviewCapability...MissingConfigDoesNotRefuse...CarriesNoCommitShapedTokenThe preflight block is appended after
dispatchPromptHeadContradictionWarningsfor #1819's reason, and the token test checks the rendered block rather than trusting the input: an agent name could be hex-shaped one day -deadbeefis a legal agent name - and a staged review whose prompt names a commit outside the PR's history would be refused at dispatch for every repo that enabled it.The pre-existing
TestReviewBriefDoesNotTripTheHeadContradictionScanpasses unchanged.Should-succeed arms are explicit: a declared, registered, review-capable agent is accepted, and an undeclared repo takes the unstaged path byte-identically.
Reuses the existing
agentHasCapabilityrather than adding a second copy - my first draft added one and the compiler caught the duplicate, which is the cheapest form of the fewer-definitions rule.Verification
Build, vet,
gofmt,go generateclean,-runfiltered tests. No untargetedinternal/clior./...run - deliberate under the coordinator's one-gate-at-a-time hold. CI is the full gate.Not verified
Deployed behaviour, and no repo declares a verdict agent yet, so the staged path is unreachable in production until one does. The preflight prompt has not been exercised against a live model: it is prompt text whose effect is an agent's fan-out, and the guards that catch a parent which ignores it - unfilled slot, fan-out-is-never-a-verdict - are the ones that already existed.
Signed: gm-staged
Attribution: the head half is unavailable, by design
This PR was implemented in-session, so its durable attribution is a self-recorded row:
That row is honest about who and permanently silent about which head.
gitmoot job record --head-shadocuments itself as "recorded as display metadata only: it is NOT stored in the job payload and no merge policy reads it (#1990)", and the store agrees: of 408 session-recorded implement rows, zero carry a head, against 2,212 of 2,636 overall.So a gate asking "who implemented this head" cannot be satisfied for in-session work while that quarantine stands, however diligently the lane records. Independence of the review is therefore verifiable on the who half and unverifiable on the head half, and that is a property of #1990 rather than a defect in this review or a gap anyone can close here.
Disclosed rather than worked around: no row was recorded on another seat's behalf, because a false who on an already headless what is worse than an absent one.