Factory M2: deliver, and close the loop - #119
Open
artyomsv wants to merge 41 commits into
Open
Conversation
Observe mode's contract is "register only, no diff, no LLM, no comments", and policy.observeOnly() was consulted in exactly one place: the pull request event path. Every /command reached onManualCommand, which never read it -- so /review and /finding both acted for real under a mode whose whole point is look-but-do-not-touch. M2 adds /fix, and the plan first said /fix alone would check the policy while the other two kept the hole as a separate bug. That routes around the gap instead of closing it, and leaves the next command to inherit it a fourth time. The debt entry deliberately refused to decide this unilaterally, calling it a product question: an operator who explicitly types a command might reasonably expect it to work regardless of the passive default. That reading does not survive contact with who can type one. The author is gated by the per-provider allowlist, not by operator role, and an empty allowlist means "review everyone" by design -- so any commenter could force a paid re-review while the operator believed the deployment was only watching. The operator's override is the setting they already own. One gate, placed after the allowlist and ahead of the command switch. Both positions are load-bearing. After the allowlist, because that gate answers whether this person's command counts at all, and reporting "the deployment is passive" about someone who was never authorized names the wrong cause. Before the switch, because a command added below it arrives ungated, which is exactly how /review and then /finding got in. The refusal is silent, and here that is forced rather than chosen. Every other silent refusal in this saga argues for its silence -- a reply confirms to a prober that a command is wired. This one could not reply even without that argument: posting a comment is the exact thing observe mode forbids, so answering would break the mode in the act of enforcing it. The timeline records ManualCommandObserveOnly, distinct from the authorization refusal's ManualCommandSkipped, because an operator reading "nothing happened" needs to know which of the two it was. Five tests, all mutation-verified. Dropping the return leaves the note and fails exactly the two behaviour cases; moving the gate ahead of the allowlist fails exactly the ordering case. Two are negative controls, so a gate that refused everything would not pass.
Round one of review found the gate shipped in the previous commit was correct and covered one of three paths. The other two were reported by three lenses independently. An author's reply is the widest and the gate could not reach it. An @-mention makes a reply eligible regardless of thread ownership AND removes the per-thread turn cap, so where /review lost one paid call this loses an unbounded number. It is not reachable on a deployment that never went active, which is why it reads as theoretical -- but the realistic case is the operator gesture the slider exists for: running active, then flipping to observe to pause the bot, at which point every thread is still bot-owned and every reply is answered and billed. The archived-review notice posts a fixed-text comment, and no placement inside onManualCommand could ever have reached it: the archived gate runs in handle() ahead of the whole switch. Observe registers with status 'observed', archiveRow refuses only 'reviewing', so an observed row archives cleanly and the author's next push triggers a live comment on a deployment that is supposed to post nothing. Both are gated in IntegrationSaga rather than in the classes that own the behaviour, deliberately. The defect was enforcement scattered across classes with one site missed, so every observeOnly() read now lives in one file and "where is observe enforced?" has a single answer. The scoping mistake is worth recording. The task was taken from a debt entry whose own suggested fix said one gate "closes all three paths (/review, /finding, and any future /command)". True, and the wrong frame: it enumerates commands, while the contract is about action commands. ReviewPolicy's javadoc says observe "emits NO action commands", and that invariant is now asserted over the whole event vocabulary rather than per branch, so a branch added later inherits the assertion instead of needing someone to remember it. Per-branch tests found these one at a time, which is how the second and third survived the round that fixed the first. Four test holes, each mutation-verified. A gate ANDed with "the allowlist is empty" passed all five original cases, because every one used an empty allowlist and the ordering case's author is refused a branch earlier -- so the gate would have been inert on every deployment past first contact. The note's lane and text were pinned by nothing, which matters because the refusal is deliberately silent and the note is the operator's only signal. The /finding case reddened by crashing rather than asserting, on a fake reaching a null DataSource -- the recorded trap, instance eight, and completing one fake would only have moved it, so all three on the path are completed. And the handler-less case claimed to pin "the gate precedes the switch" when replicating the gate per branch passes it; its javadoc now says what it actually pins. The definition of observe mode was widened with the behaviour. Six surfaces still described the PR-event path only, including the tooltip an operator reads while flipping the switch -- and that drift is the exact condition the retired debt entry named as the reason to leave the code alone. One thing is deliberately NOT gated and is now written down rather than left to be re-derived: an operator's own authenticated REST action. The dashboard Re-run button and POST /api/runs are spire-admin, which makes them the operator exercising a posture they own, and they are the only way to review a single pull request without flipping the whole deployment active. The allowlist argument that justifies refusing a /command does not describe them. Separately, ApkUpgradeIsNotCachedTest could not pass on Windows. Its matrix parser matched \n while core.autocrlf gives CRLF on disk, and Java's . excludes \r, so .*\n never reached the newline and the include: block "was not found" -- a message about the workflow, for a fault in the parser. Green in CI on Linux, red on every developer machine, which makes testFast permanently red locally. Confirmed against pristine master. The Dockerfile splitter in the same file already used \r?\n; two patterns did not. One finding is filed rather than fixed: a settings-read fault falls back to the seed mode, which may be active, so a single failed SELECT makes one event fail open. Pre-existing and shared by every observeOnly() caller, but this change adds three more.
M2 dispatches a factory run from a review finding, and the trigger is a /fix comment on the pull request. This is the vocabulary half: the constant in spire-contract that both the translating service and the acting service read, and the shared set the three webhook edges hand out. One set rather than three copies is load-bearing here. A provider left out routes /fix to the conversation path, where it becomes an ordinary reply -- so the operator watches the bot answer a question nobody asked while no run ever starts. That is the failure the shared set was introduced to prevent, and /fix is the command where it costs most, because the thing that does not happen is a paid agent run that pushes a branch. The parity test now covers /fix on all three providers, asserting the thread ref as well as the command and args. The ref is what makes a fix dispatchable at all: the finding it targets comes from the thread the command was typed in, so a provider that carried the command and dropped the ref would produce a valid-looking command with no target. The three fixtures are parameterised by comment text rather than copied a fourth time, since the copies are what let the providers diverge before. Mutation-verified: removing FIX from the shared set fails the parity case on all three providers and the exact-set guard, so the test proves translation rather than the constant's existence. The saga handler lands next. Until it does, a /fix comment is translated and reaches the switch's default branch, which logs "no handler" and does nothing -- exactly the silence WebhookCommandsTest's own javadoc warns reads like a lost webhook. That is why these two tasks are one pull request and this commit is not independently shippable.
The M2 trigger: a review finding becomes a factory run with no tracker in the loop. This slice recognises the command and resolves its target, or refuses in a way the author can act on. Dispatch is the next slice. The finding comes from the THREAD, not from the command's arguments, and that is what makes this a complete task specification without anyone typing one -- the thread already carries repository, commit, file, line, severity and the reviewer's own message. It also means a /fix with no thread has no target at all. That is refused rather than guessed, because guessing would point a paid agent at whatever finding happened to be newest. FindingProjection gains findByThread. Newest row for a thread wins, which is the rule newerThreadRef already settled for reconciliation and for the same reason: several rows share one thread ref across rounds, and keeping an arbitrary older one targets a finding the author has already watched close. That method throws on a read fault instead of answering empty, which is deliberately unlike the log-and-continue style of every method beside it. Empty reaches a human as "no finding on this thread" -- a claim about their repository, which they will act on by hunting for a comment that is in front of them. Unknown is not zero, and here unknown is not absent, so the record dead-letters where an operator sees it. Three orderings are load-bearing, and each is a lesson already paid for. The registration check is first because an unregistered pull request clears every gate ahead of it: archived answers false for a row that does not exist, and the provider resolves by workspace when the review carries no stored type. The thread is null-checked BEFORE normalization, because rootOf binds its argument into a statement immediately and a null throws an NPE inside a catch(SQLException) that cannot see it -- the trap ADR-024's archived notice paid for. And the lookup is keyed on the conversation ROOT, because Bitbucket threads by immediate parent, so a /fix typed as a reply to the bot's own answer carries that answer's id while the finding hangs off the root. Refusals here SPEAK, unlike the authorization refusal two gates up. The distinction is who is being answered: an unlisted author is a possible prober, and a reply tells them the command is wired at the price of an API call per probe. An author who cleared the allowlist and used the command in the wrong place is a colleague, and silence sends them hunting a lost webhook -- which this project has already paid for once with the conversation turn cap. An already-resolved finding is refused too. Reconciliation closed it, so a fix run would spend an agent to produce an empty diff. Only RESOLVED closes the door; a finding judged STILL_OPEN is exactly what /fix is for. Seven tests, four mutations, each killing exactly its own: dropping the registration gate, keying the lookup on the raw ref instead of the root, dropping the resolved check, and dropping the null-thread guard. No idempotency claim yet, on purpose. Nothing here spends, and the guard belongs with the spend -- where the LLM idempotency claim already sits. It arrives with dispatch.
Round one over the /fix command path. The three orderings the slice was built around all held; every defect was somewhere else. An empty author allowlist admitted everyone. It means "review everyone" by deliberate design, which is right for one spend-capped model call and wrong for a command whose output is a branch pushed as the machine account -- and allowlistFor answers List.of() for an unresolvable provider too, so that is a second everyone-answer. AUTONOMY.md Rule 3 already names this threat in as many words: a drive-by contributor, and the factory writing their code with the operator's credentials. /fix now denies by default; /review and /finding are untouched. Taken now rather than with dispatch because the tests encoded "empty = allowed" as the passing case, so every hour it stayed the gate got harder to change. Three places said the refusals SPEAK and nothing was emitted. /finding's refusal emits RefuseFinding and reaches the author; there is no RefuseFix anywhere in the tree. The corroborating detail settles it: the fixture never assigned commands or workerCredentials, so it could not have supported a speaking refusal. Reworded rather than built -- the reply needs a new ActionCommand member, a contract-snapshot entry and a worker handler, all of the dispatch slice's surface, and emitting a whole wire type so a javadoc stops lying is the tail wagging the dog. The refusals did gain the durable row the observe gate's own argument demands, and the skipped/refused split /finding makes. A /finding-filed finding was a valid fix target with no description. Its message and suggestion are NULL by design, so FR-F27's "complete task specification" would have been a severity, a path and a line -- and TargetFinding carried no origin, so dispatch could not have detected it either. Refused here, because by then the target is accepted and the only options left are paying for a run on an empty spec or retracting. "RESOLVED" was a literal where FindingVerdict.Status exists, is already imported in that file, and is what the write side spells. There is no CHECK constraint on the column, so a rename would keep compiling and stop matching in silence, on the guard that decides whether a paid run starts. Two lessons are worth more than any single fix. A test can be killed by a later commit in the same pull request. The T1 guard aCommandWithNoHandlerIsAlsoRefusedInObserveMode drove "fix" to prove an UNENUMERATED command is gated; T3 gave fix a handler two commits later, so it silently became a test of an enumerated command while staying green. Proven dead by narrowing the gate to the enumerated set -- the suite still passed. Vacuity does not only arrive with a test; it can arrive afterwards, from the same author. And writing the test that was asked for falsified a production comment. FIND_BY_THREAD's javadoc claimed several rows share a thread ref across rounds and the newest is live. ATTACH_THREAD_REF orders by (thread_ref = ?) DESC, so a row already carrying the ref beats the newest unattached one, deliberately -- and at most one row ever carries a ref. That is also why flipping the query to ASC changed nothing. The honest fix was to correct the reasoning rather than invent an assertion that would make a false claim look tested. Four fixture holes let real regressions pass, each now mutation-verified. Filing the durable row under the branch ref instead of the conversation root passed everything: the fixture overrode BOTH appendEvent overloads, which is exactly why it read as safe, while both bodies discarded the argument that tells them apart. startLine == endLine made those two components interchangeable. The description was asserted on two of its four parts, so the severity and the asker could both vanish. And findByThread was faked in every saga test while containing real SQL, a deliberate throw and a row mapping. The no-finding refusal asserted something false on Bitbucket, which threads by immediate parent: only the bot's comments get a review_thread row, so a /fix typed as a reply to another human's reply matches nothing while the finding sits visibly a few comments up. It now says what it could not do. The functional gap is filed rather than absorbed. The /fix argument rule is written down while it is still cheap: /fix takes none, and the text after it must never reach a prompt's instruction part. Feeding it to an agent holding a clone and a push token would let a commenter author instructions to it. Two items are deferred to dispatch and recorded so they cannot be lost: the in-thread reply, and the spend claim keying on commentId rather than the thread -- a second genuine /fix after a failed run must be allowed, the same comment redelivered must not pay twice.
FR-F32 says a fix run records the finding it addresses and dispatch refuses past N runs for that finding. Counting per finding does not bound the loop the requirement is about. Every hop of "a finding spawns a fix, whose review raises a finding, which spawns a fix" raises a NEW finding with a new identity, so a per-finding counter sees one run for each and never reaches N -- while reporting itself satisfied. The runaway runs free and the cap says it is holding. So there are two. Per finding stops repeated attempts at one stubborn finding, which is the job that axis is actually good at. Per review bounds the chain, and under ADR-040 a fix pushes to the branch the review already watches, so one review IS the chain. V54 gives factory_run the join nothing had: review_id, finding_ref and kind. Their absence is what made BOTH halves of the milestone uncomputable -- the cap had nothing to count and "run cost on the pull request" had no key to sum by. Nullable, because every run M0 and M1 dispatched has no review and never will; backfilling one would be inventing a fact. The CHECK is written both ways round, because either direction alone permits a row that lies. A FIX row carrying no target makes the per-finding cap stop counting it, which is the cap failing open on the axis that exists to bound spend. A non-fix row carrying a finding ref makes the same cap count a run that never addressed it. That constraint turned out to be the guard, and the query's kind filter is belt-and-braces. A mutation removing the filter survived every behavioural test, because the only non-fix row a fixture can build has a null review_id and the review_id predicate excludes it anyway. Rather than invent an assertion to make a redundant line look tested, the tests now assert the CHECK itself -- in both directions -- and the comment says which half is load-bearing. It matters because that constraint is exactly what loosens when SPEC and PLAN runs arrive, both already in its own kind list: the day one of those carries a review, the filter starts working. nextAttempt derives the attempt from the runs already dispatched. RunIds embeds the attempt and a run id must be unique, or the worker's claim drops the second dispatch as a redelivery -- a run accepted and never run. So FR-F32's N is unreachable while every fix for one finding derives the same id, which is what pinning the attempt to 1 does today. A read fault refuses rather than allowing, which is the opposite of this deployment's other unset-means-unlimited defaults and deliberately so. An unset cap is an operator's choice; an unreadable count is an unknown, and the thing past this gate is a paid agent holding a push token. Unknown is not zero, and here unknown is not "within budget". ChargeKind gains FIX, which V42's CHECK has admitted since the ledger learned about runs. Its own kind rather than a BUILD because the two answer different questions of one ledger: what a repository costs to build against, versus what the reviewer costs when it fixes what it finds. Collapsing them makes the second unanswerable, and it is the one M2 exists to make true. Thirteen tests, six mutations. Five killed exactly one test each; the sixth is the survivor described above, kept as a finding rather than papered over.
ADR-040, the publisher half. M2's exit criterion needs a fix to land on the pull request's own source branch: reconciliation is keyed per review, so a second pull request is a second review with no prior run, and the finding that caused the work could never resolve. The publisher refused exactly that push, and its javadoc called both refusals security controls rather than hygiene -- correctly. So the namespace rule and the branch-not-base rule lift together, and only under an explicit SPIRE_BRANCH_MODE=existing. They lift together because a human's branch is neither under spire/ nor different from the branch we cloned; lifting one without the other admits nothing useful. The mode is declared, never inferred. Reading it off branch.equals(base) was the tempting shape and is wrong: an inference is a default, and a default is what an orchestrator bug reaches by accident. An unrecognised spelling is refused rather than falling back to the permissive mode, which is the same defect with an extra step. Both are mutation-verified, and the inference mutation is killed by a test that predates this change. What does not lift is the reason the floor exists. This process holds the only write credential in the run unit and the push gate judges paths, not refs, so a command naming a trunk would fast-forward it with whatever the agent produced. main and master are refused in EVERY mode, and so is the pull request's destination branch. That destination arrives as its own variable rather than being guessed. A deployment whose trunk is develop or release/2026.1 is not covered by a main/master list, and the orchestrator knows the real answer because it read the pull request -- while this process must not be able to make an API call to find out. The convention list is the floor that survives an orchestrator forgetting to pass one; the variable is the truth. Identification stays with the orchestrator, which resolves the target from review_status. Nothing here proves a branch is really an open pull request's source branch, and the comments say so, because a floor that believes it is an identification is the more dangerous of the two. One pre-existing test had to change, and the reason is worth recording. theBranchMustLiveInTheFactoryNamespace looped over names "refused purely for living outside the namespace" and included main. That premise is now false for main alone -- the trunk floor catches it first and says something stronger. Rather than loosen the assertion to accept either message, main and master left that loop and gained their own case covering BOTH modes, which is more coverage than the loop ever gave them. A floor that applies only to the mode that needs it is not a floor.
The orchestrator half of ADR-040. The publisher refuses trunk names and the pull request's destination branch, but it cannot tell whether a branch really is an open pull request's source branch: it holds the only write credential in the run unit and under ADR-039 makes no API call. So the identification lives here, against review_status, and the publisher's checks are the floor that survives a bug in this class rather than a substitute for it. Resolving a target and being allowed to push to it are kept apart. An empty Optional cannot distinguish "no such review" from "that pull request is merged", and the caller has to tell an author which it was. The blank check is not defensive noise. source_branch defaults to the empty string rather than null, so a null check alone passes a blank ref to the publisher, where it fails isValidRefName inside a container after the agent has been paid. And writing this found something the design assumed and the code cannot do. ADR-040 puts fork pull requests out of scope for existing mode, and NOTHING in this deployment records whether a pull request came from a fork -- not review_status, not PullRequestEventReceived, not any ingress. That did not matter while the factory only pushed branches it created under spire/. It matters now: a fix pushes to the branch source_branch names, in the repository workspace/slug names, and for a fork those two do not belong together. Either the base repo has no such branch and the push creates a stray one attached to no pull request, or it does and a machine-authored commit from a different diff lands on someone else's work. The publisher's floor does not catch it either -- that floor refuses trunks and the destination branch, and a fork's source branch is neither. So it is filed as High rather than hidden behind a field that is always false, which would read as a check and be none. isPushable() says plainly in its javadoc that it does not exclude forks and cannot. The trigger is ordinary -- an outside contributor's pull request, a finding, a maintainer typing /fix -- and it is unreachable only because no caller sets existing mode yet. That is exactly why it is written down now, ahead of the slice that would trigger it, with the sentence that belongs in the operator documentation: existing mode must not be enabled on a repository that accepts fork pull requests until this is closed. Five tests, two mutations, each killing exactly its own.
Round one over the fix-run identity slice. Every defect was in a CLAIM: three comments asserted a guard the schema did not provide, one javadoc asserted a floor that was optional, and four mutations survived. The destination floor was optional exactly when it was needed. It ran only when SPIRE_PROTECTED_BRANCH was present, while the class javadoc said the destination is refused in EVERY mode. A dispatch that forgets one map entry skipped it silently, and a trunk called develop -- which the main/master convention list does not cover -- would be fast-forwarded. Existing mode now refuses to start without it, blank included, because review_status.dest_branch defaults to the empty string and copying it through unconditionally yields blank, not missing. V54's CHECK admitted blank ids. '' IS NOT NULL is true, so a FIX row with empty-string ids passed and was counted by neither cap for any real id -- the cap failing open for exactly that row. Not hypothetical: this schema already uses blank-not-null for source_branch and dest_branch. Rewritten as two explicit arms with btrim, because kind is NOT NULL and a CHECK that evaluates to NULL passes. The trunk floor was exact-match. Measured against the pinned JGit: Main, MAIN, HEAD, refs/heads/main, heads/main, -main, and names carrying a zero-width space or a Cyrillic a all pass isValidRefName and the floor. None of them reaches refs/heads/main -- forge refs are case-sensitive -- so this is not a bypass. It is a machine creating a branch a person reads as the trunk, which is worth refusing on its own. Invisible characters are refused; an ordinary umlaut is not, and a test asserts that half. FixTargets guarded a blank source branch and ignored commit, which carries the identical NOT NULL DEFAULT '' and the identical failure -- Env.required refuses it inside the container, after the agent is paid. The class documented that hazard at length for one of the two columns. ADR-040 section 3 also asks for a provider and repository match that nothing performed; belongsTo adds it. Three surviving mutations are now killed. nextAttempt read the review axis instead of the finding axis and every test agreed with both, because each seeded one run for one finding on one review; per-review numbering would report "attempt 3" for a finding's FIRST fix, contradicting the per-finding refusal message in the same class. isBlank became isEmpty with nothing seeding whitespace. And the both-modes property was pinned for trunks but not for destinations. The fourth is the interesting one, and it took two reviewers and an unrelated fix to settle. A mutation survived and the conclusion drawn was "the schema must be guarding it" rather than "my fixture cannot build the row". The second was correct: the CHECK's right side was an AND, so a non-FIX row satisfied it by failing either conjunct. Then the blank-id fix rewrote the constraint as two explicit arms -- which, as a side effect nobody set out to produce, forbids a non-fix row from carrying a review at all. So the original claim is true again for a reason unrelated to the original argument, and the filter is belt-and-braces until the constraint is relaxed for SPEC and PLAN runs. That is written down with its expiry, because the answer has now been reached wrongly twice. Docs: SMOKE-TEST's PUBLISHER_MISCONFIGURED row was the one place a reader learns what makes the publisher refuse and still described the rules as namespace-only. ADR-040 claimed the refusal covers "the repository default branch" when the code refuses two literal names -- its own javadoc calls that a convention list, not a truth -- and never named the variable carrying the half it does describe. One suspicion of mine was falsified and is recorded as such: the two new variables are NOT an .env.example violation. None of the eight existing publisher variables is there either, because they are per-run container environment rather than operator config. Adding them would have invented a contract. qa did not deliver a report and a direct request went unanswered, so its section is unknown rather than clean. The mutation work here is mine and code-reviewer's.
ADR-040 puts fork pull requests out of scope for the existing branch mode, and until now that was a sentence a reader had to obey rather than a check: nothing in the deployment recorded fork provenance. No ingress parsed the source repository, review_status had no column, and the event had no component. That mattered because a fix pushes to the branch source_branch NAMES, in the repository workspace/slug names -- and for a fork those two do not belong together. Either the base repository has no branch of that name and the push creates a stray one attached to no pull request, or it does and a machine-authored commit from a different diff lands on someone else's work. The publisher's floor does not catch it: that floor refuses trunks and the destination branch, and a fork's source branch is neither. Each provider spells the fact differently, which is exactly the shape that has diverged in this repository before. GitHub compares two repository full names, Bitbucket compares two repository full names nested under source and destination rather than head and base, and GitLab compares two numeric project ids. One provider getting it backwards would let forks through on that SCM alone while its own per-provider test passed, so the gateway asserts all three agree and asserts BOTH answers. A missing signal reads as not-a-fork rather than as a fork. GitHub omits the head repository once it has been deleted, and that is a pull request nothing can be pushed to anyway -- the branch-mode gate downstream refuses it on the branch, which is a more accurate reason than "fork". The component is additive with a convenience constructor and a wither. Adding one to a wire record silently drops it at every rebuild site because the shorter constructors stay valid and everything still compiles, so the components are enumerated once next to the record, the way RunFinished already does it. The contract snapshot fired on the change, which is its job, and was re-baselined deliberately. V55 defaults the column to false, and that default is a claim rather than a convenience. Every row written before this migration came from a deployment that could not distinguish the two, so the honest reading of an old row is "unknown" -- calling it false is safe only because nothing consumes it yet. A row refreshed by the next pull-request event carries the real answer, which is why the saga writes it on every event and not only the first. setFromFork is its own write rather than a fourteenth parameter on registerHeader. That method already takes thirteen, and a component added to a call that long is dropped silently at a rebuild site for the same reason the wire records are. It also cost instance TEN of this project's recorded fake-coverage trap, in the same shape as the previous nine: a new production write on a path the fixtures already exercised, added without re-reading which methods that path reaches. Seven tests went red on an NPE from a real DataSource opened by a plain unit test. Four mutations, each killing exactly one test: GitHub always reporting not-a-fork, GitLab the same, the gate clause dropped, and the saga no longer persisting it. The last needed an assertion that did not exist -- the fixture recorded the write and nothing checked it, which is the hole this round found twice in other people's code. Retires techdebt/spire-orchestrator/2-3-a-fork-pull-request-is- indistinguishable-from-a-branch-one.md, filed hours ago in the slice that discovered it. ADR-040 now says the rule is enforced and by what.
CLAUDE.md says test tasks driving the real Docker daemon are serialised by a shared build service, and a guard derives that module list by scanning rather than trusting a declaration. The declared members are the four that drive the daemon. spire-gateway and spire-orchestrator are not among them, and both start Quarkus Dev Services Postgres. With parallel execution on, running both test tasks in one invocation contends. The symptom is what makes it worth a file: tests fail that have nothing to do with the change. Measured twice today -- a mutation of FixTargets.belongsTo reported a failure in FixRunsTest, a class that never touches FixTargets; and a fork-provenance change reported six ProviderResourceTest failures in an untouched package. Both passed on a forced re-run in isolation, and the second nearly landed a commit over a red build because the verification command's exit code was masked by a pipe into grep. It is the same failure the existing serialisation prevents, one category across: the guard covers modules sharing the Docker daemon and misses modules sharing Dev Services. CLAUDE.md's note that the lock "covers one Gradle invocation" reads as reassurance and is not -- the gap is INSIDE one invocation, between two modules nobody thought to declare. Filed rather than fixed here because widening the build service is a change to how every service module's tests are scheduled, which deserves its own slice and its own verification rather than riding along in a milestone commit.
ADR-040's two halves were built and could not meet: the publisher understood SPIRE_BRANCH_MODE and SPIRE_PROTECTED_BRANCH, and nothing could set them. ExecuteRun now carries both, and the run unit passes them through. Two components rather than one boolean. The protected branch is a NAME the orchestrator read from the pull request, and the publisher must not be able to make an API call to find it -- a deployment whose trunk is develop is covered by no convention list the publisher could hold. The mode is explicit, never inferred from branch.equals(baseBranch), which is the same reasoning the publisher's own parser uses: an inference is a default, and a default is what a bug reaches by accident. Additive, with a convenience constructor and a wither. Adding a component to a wire record keeps every shorter constructor valid, so a rebuild site still compiles while quietly losing the new value -- the trap this repository records. The components are enumerated once in the wither, and a test asserts it carries every other one through rather than only the two it sets. The contract snapshot fired and was re-baselined deliberately. A run pushing to an existing branch without naming a destination is refused at construction. The publisher refuses it too, and that refusal is the floor -- but it fires inside a container after an image pull and a clone, and reports as a misconfigured publisher rather than as a command that should never have been sent. The publisher environment becomes a HashMap because the two variables are conditional and Map.of cannot express that. Writing them unconditionally would be worse than verbose: a blank SPIRE_PROTECTED_BRANCH reads to the publisher as "present but blank", which its own required-in-existing-mode check treats exactly as absent -- so the distinction would survive here and be lost there. A test asserts a build run carries NEITHER key, since absence is the M0 rule and the safe direction.
The rules live in their own class rather than in IntegrationSaga, which is the shape ConversationFindings already set for /finding: the saga dispatches on a result and the rules are unit-testable without a saga fixture. That saga is also already past this project's size guideline, so growing it is a choice worth not making. Every refusal carries a reason. The author typed a command, and a silent "nothing happened" is the symptom this project has paid for twice; a bare empty answer would force the caller to re-do every check to explain it. The cap is consulted BEFORE the target is proven pushable, and the order is deliberate. A capped finding on a merged pull request should be told it is capped: the cap is a durable fact an operator set, while "merged" is a state that changed. Reporting the transient reason would send someone to reopen a pull request the cap would refuse anyway. base and branch are the same branch, which is exactly what ADR-040's existing mode exists to permit and what the default mode refuses -- the fix is committed onto the branch the review already watches. The destination rides along so the publisher can refuse it. An unrecognised provider type refuses rather than guessing. The row stores whatever was registered, and a value this build does not know means the registration and the build disagree -- an operator-visible fault, not something to guess past on the way to spending money. The uncomfortable part is that this creates TWO encodings of one rule. isPushable() answers a boolean; whyNotPushable answers which cause, because an author needs to know which. That is the shape this repository has paid for hardest -- two credential scrubbers whose rules quietly diverged, and the weaker one ran in the container holding the write token. So a test drives all thirty-six combinations of state, fork, branch and commit and asserts the two never disagree. Both halves are mutation-verified: dropping either cause from one side fails the agreement guard AND the case for that cause. The caps are constants here rather than settings, and that is a gap this slice inherits rather than introduces. FR-F32 bounds a runaway loop, which is not ADR-025's spend cap where unset is an operator's deliberate opt-in -- reading them from configuration wants a startup refusal when unset, which is its own change.
FR-F32's caps read factory_run, and nothing wrote the three columns they read. V54 added them; the projection did not fill them, so a dispatch would have produced a row the caps could not see -- the cap failing open, which is the direction that matters on the axis that exists to bound spend. QueuedRun grows them additively with a convenience constructor and a wither. Adding components to a record leaves every shorter constructor valid, so the REST endpoint's call site keeps compiling while silently dropping them; enumerating them once in asFixFor is what this repository does instead. V54 refuses a FIX row naming neither target and a non-FIX row naming either, so a caller cannot half-apply it. The tests that matter here go through the real writer. Every existing case in FixRunsTest hand-writes its INSERT, which proves the queries and proves nothing about the projection -- a writer that dropped kind, review_id or finding_ref would leave all of them green while the caps counted nothing. Writing those cases exposed a fixture bug that had been silent. The @beforeeach deleted rows by run-id prefix, LIKE 'run::TEST-%', and every id built through RunIds spells the platform first -- run::github:... -- so the cleanup matched nothing and rows leaked between cases. It only surfaced because the new tests are the first here to build a realistic id; six previously-green assertions went red at once, counting each other's rows. Now deleted by workspace, which every id these tests create carries.
The previous round's qa reported after that round's record was written, and it was worth waiting for. It measured 1057 orchestrator and 58 publisher tests on the committed bytes, confirmed the revised kind-filter reading, and found three surviving mutations that three other lenses and two of my own sweeps had all missed. A negative cap refused every fix. The guards read > 0 and the javadoc said "non-positive means unlimited", but the test only ever passed zero. Changing both guards to != 0 passed everything, so an operator writing -1 -- the usual spelling of unlimited -- would have had every fix refused, with the message "this finding has already had -1 fix run(s)". The two caps could be cross-coupled and nothing noticed. ANDing the guards together passed every case, because every fixture set BOTH caps. An operator setting a per-finding cap and leaving the chain unlimited would have had the cap they set silently disabled by the one they did not -- the exact failure two axes exist to prevent, and the same shared-fixture shape that hid three earlier survivors in this same file. And V54's factory_run_kind_closed was asserted by nothing: deleting the constraint left the full module green. It is the one the migration's own comment says exists so a typo'd literal in a writer cannot produce a row no cap counts and no filter matches. Its sibling constraint had four tests; this one had none. Each is now mutation-verified to kill exactly its own test. The review record is corrected rather than quietly overwritten: it said qa's section was unknown, which was true when written. It also now carries qa's two infrastructure failures that impersonate regressions on this machine -- a Testcontainers port timeout where the 665-skip count is the tell that the suite aborted rather than failed, and a 266-failure run where every failure was the Dev Services Postgres dying under load. Both went green on re-run with identical bytes, which independently corroborates the contention entry filed earlier from the same symptom reached by a different route.
Round one over the dispatch slice. The production code was correct -- code-review traced every argument position and SQL index by hand and found no defect that runs today -- and the guard was where the weakness was. Six mutations survived 8/8 green, all from one cause: both fakes in FixDispatchTest discarded every argument they were handed, so nothing asserted what the dispatch ASKED FOR, only what it did with the canned answer. Transposing reviewId and threadRef in the cap call makes both caps count on keys that match nothing and fail open forever, with no symptom at all. The structural finding is the one worth keeping. isPushable() answered a boolean and the dispatch re-derived the causes, which is two encodings of one rule -- the shape that produced two credential scrubbers here whose rules quietly diverged. I had written a 36-case test asserting they agreed, and that test could only ever check WHETHER, never WHICH: swapping the fork and state causes passed it, and so did a fourth cause the boolean did not model at all, so the invariant held only on the subspace the matrix happened to enumerate. So the rule is now derived. whyNotPushable returns an enum and isPushable() is its emptiness; the dispatch owns the wording in an exhaustive switch. A cause added to the read model without wording fails the BUILD rather than a loop, the agreement is structural, and "OPEN" never leaves FixTargets -- which also closes the two-spellings finding. belongsTo was declared, tested, and called by nothing. That was the same shape inverted: plan resolved coordinates from the review and REPORTED them, rather than being told the ones the comment arrived on and PROVING they match. It now takes the RepoRef and refuses a mismatch, which is what ADR-040 section 3 asks for in as many words. The unused TargetFinding parameter goes with it -- every decision it would enable is already made upstream, and the next slice needs the encrypted columns it deliberately does not carry. RunIds.of could throw out of plan, which breaks the sealed-result contract: an escaping exception is not a Refused, it dead-letters through a channel that acks on receipt, and the author who typed /fix gets exactly the silence this class exists to avoid. threadRef is the reachable one -- forge-supplied, with no upstream guard on its characters. The ON CONFLICT re-arm compared everything about a run's identity except the three columns that say what it is FOR. The method's own javadoc claims a differing retry matches no row and is refused; that was silently false for them, so a BUILD row re-armed as FIX would stay BUILD and NEITHER cap would count it. RunKind replaces four spellings of one vocabulary -- two CHECK constraints and two Java literals -- with an enum. Not a merge with ChargeKind: that one names a kind of CALL and carries REVIEW, RECONCILE and FOLLOWUP, which no run can be, while this one carries SPEC and PLAN, which are not calls the ledger charges yet. And isBlank became isEmpty on the commit column and survived BOTH test classes, because whitespace was seeded for the branch and not the commit. That is the same finding as an earlier round, on the axis that round did not cover. Two small ones: a toString assertion that could not fail, because it checked for a quote character the format never emits and the redaction was carried entirely by the line below it; and ExecuteRun refusing a blank destination but not one equal to the branch, which is the other half of what the publisher checks. .gitignore gains *.orig. That is not hygiene -- I ran mutation probes in the live worktree while telling every review agent to probe on a copy, and qa caught the tree mid-swap with a V54 constraint replaced by a comment. It also named a consequence I had not considered: a mutated migration applied to Dev Services and then restored leaves a Flyway checksum mismatch that surfaces later as a validate error looking nothing like the change under review.
Two review lenses independently found the same shape: a review row whose dest_branch is blank reached ExecuteRun's compact constructor, which throws on a blank protected branch in existing mode. An exception on a Kafka consumer is a redelivery, so the /fix path would have refused that review forever with nothing said to the author. dest_branch is the third column with NOT NULL DEFAULT '' and the only one nobody guarded; it now joins source_branch and commit_sha in whyNotPushable. The 36-case agreement matrix could not have caught it. It asserts two readings of one rule agree, and a rule missing from both agrees with itself perfectly -- proved by adding a fifth cause keyed on dest_branch, which survived all 8 cases because every one of them named "develop". The matrix gains dest_branch and null-provenance axes. V55 becomes nullable with no default. Its first draft argued a false default was safe because nothing consumed the column; that stopped being true when FixDispatch began refusing forks and RunUnitBuilder began writing SPIRE_BRANCH_MODE. A pre-V55 row is not a branch pull request, it is an unrecorded one -- ADR-023's "unknown is never zero" applied to a boolean. Old rows now refuse with PROVENANCE_UNKNOWN, worded as "push once and try again" rather than as a claim that the pull request is a fork, and FixTargets reads the column with getObject rather than getBoolean, which maps SQL NULL to false. Also: - The publisher honours SPIRE_PROTECTED_BRANCH in every mode, so the run worker no longer drops it outside existing mode -- a floor the caller asked for was being discarded by the caller's own plumbing. - The unrecognised-SCM refusal gains a test; every prior case named "github", so deleting the branch left the suite green and moved the failure into Optional.get(). - A legacy-JSON wire test pins the claim that a command serialised before ADR-040 reads as namespace mode. A round trip cannot: it only proves the new version agrees with itself. - Two orphaned javadocs re-homed, and an assertion message that printed a map holding SPIRE_GIT_SECRET now prints its keys. The long-lived-shared-source-branch gap (a develop -> main release pull request is a truthful row whose source several people share) and the stale pr_state re-open gap are filed rather than fixed: both need a dispatch-time re-read from the forge, so they want one design.
The fourth column pair that fails by throwing rather than by answering wrongly. FixDispatch builds Planned from source_branch and dest_branch, and ExecuteRun refuses a run whose branch equals its protected branch -- correctly, since a fix pushes to a SOURCE branch and such a run names its own destination. It refuses by throwing, and on a Kafka consumer a throw is a redelivery. No forge produces that row, which is the argument for guarding it rather than against: what FixTargets reads is what the deployment last SAW, so the guard is against the row and not against the forge. Both sides are stripped, because ExecuteRun compares stripped. An exact match here would let a trailing space through to the throw it exists to prevent, and the test carries that case. A new test also builds an ExecuteRun from a Planned and asserts it is accepted. That is the end-to-end property the guards exist for, stated once instead of inferred from four of them -- and it is where a fifth unguarded column will fail next time, in this class rather than in a consumer.
The REST endpoint owned this inline, and /fix is about to need the same three outcomes. Re-implementing them would put two readings of "did the record land?" in the tree, which is the shape that produced two credential scrubbers here whose rules diverged -- and the weaker one ran in the container holding the write token. RunLaunch answers an outcome rather than throwing: the REST caller turns each into a 503 with its own wording and a saga has no 503 to say it in, so the shared code cannot pick the exception. The split follows that line exactly -- what a failed dispatch WRITES ON THE ROW moves, what it SAYS TO AN HTTP CALLER stays. The row write stays with each caller, deliberately, so a run can never exist on the bus without a row. RunLaunch only ever updates a row it may assume exists, and its projection fake throws from queued() to say so. RunResource's switch over the outcome is exhaustive, so a fourth outcome fails the build rather than falling into whichever branch is last. RunResourceTest is unchanged apart from two constant references that followed the members they name; its behavioural assertions are the proof the extraction changed nothing. RunLaunchTest adds the direct cover the new class earns, including the case that discriminates the whole rule: an ack failure that MAY have landed is uncertain, not a miss. Mutating the classification to ignore mayHaveLanded fails that test and the REST test that predates it. 1089 orchestrator tests across 123 suites, 0 failures.
FR-F27's premise is that a review finding IS a complete task specification, so nothing a commenter types reaches the agent. /fix takes no arguments and FixPrompt reads none: a prompt assembled from comment text would be a commenter authoring instructions for an agent holding a clone and a push token. TargetFinding deliberately carries no message -- deciding whether a thread names an open finding does not need the finding's text, and putting decrypted source quotations on that path would widen it for nothing. specFor is the second read, reached only once a run is about to be paid for. It binds the review id in the WHERE as well as passing it as the AAD, so a finding id from another review reads as absent rather than as a decryption failure an operator would mistake for a broken key. It throws on a decrypt fault where every neighbour falls back to treating the column as legacy plaintext. The neighbours are right for themselves: they render it, and old text beats nothing. Here the value becomes a prompt that is paid for, so a fallback would spend money on ciphertext -- and there is nothing to fall back to, since V36 created this table with both columns encrypted and shipped no backfill. The finding's own words arrive fenced, with the surrounding text saying the fenced part is a report rather than orders. A finding message is model output derived from a diff a contributor wrote, so a sentence addressed to an agent can travel from a pull request into a review comment into this prompt. That fence is the cheap half of the defence and is documented as no more than that; the load-bearing half is that the agent holds no write credential, the publisher holds the only one, the push gate judges paths and ADR-040 bounds the branch. Four mutations killed: dropping the review filter, skipping the decryption, and the two prompt cases. The decryption test also reads the raw column back, because equal plaintext would otherwise pass just as well on a projection that never encrypted.
The T2+T3 review deferred this here with its key already decided: the claim goes on the COMMENT, not on (review_id, finding_ref). Those two are the cap's axes, and the cap is a different question -- "has this finding had too many runs" is meant to have a nonzero answer, and a second genuine /fix after a failed run is exactly what it permits up to a bound. Keying the claim there would forbid it. The gap it closes is one the existing guard cannot see, and the test drives the sequence rather than describing it. A run id is derived from the finding's thread plus nextAttempt, and nextAttempt COUNTS the rows already written -- so a redelivered command derives a HIGHER attempt, a different run id, and passes straight through ON CONFLICT (run_id), which catches every other duplicate. The numbering defeats the one mechanism that would have stopped it. V56 adds comment_id plus a unique index partial on kind='FIX'. The saga reads before it writes and that read produces the refusal an author can act on; the index is the backstop for a race that should not be reachable, since cs.integration keys by review id so both deliveries land on one partition and one consumer in order. It is a constraint rather than only a query because if it ever does fire, a dead-lettered record an operator sees is the right answer to a duplicate SPEND. The ON CONFLICT re-arm compares the comment like the three components an earlier round found missing. Without it a re-arm could move a claim from one comment to another, releasing the first to buy again, while the unique index -- an index, not a row comparison -- saw nothing move. Its test carries the negative control: an identical retry still re-arms, or the comparison would just be refusing everything. fixRunFor throws on a read fault rather than answering empty, because empty is the answer that AUTHORISES a paid run and an unreadable table must not be able to give it. FixRuns takes the same posture for the same reason -- ADR-023 applied to a guard rather than to a number. Four mutations killed, each by exactly its intended test.
Closes T5b(ii): /fix now becomes an ExecuteRun on cs.run-commands, in ADR-040's existing mode, pushing to the pull request's own source branch so the next review round reconciles the finding that caused the work. The assembly is its own class rather than more of IntegrationSaga, which is already past the size guideline with a debt entry saying so. The saga decides whether the command is ADMISSIBLE -- who asked, is the review registered, does the thread name an open finding worth fixing -- and FixRunDispatcher decides whether it is DISPATCHABLE, then does it. The order of the refusals is a decision, not an accident, since the author gets exactly one message. The duplicate check is FIRST: every gate below it is a reason to refuse a NEW request, and a redelivery is not one -- a repeat delivery told about a spend cap reads as a lost request rather than a finished one, and the author asks again. After that, durable operator facts outrank transient ones, which is the argument FixDispatch already makes for its own caps. Nothing is written and nothing is packed until every gate has passed, and the credential is selected LAST of the checks because selecting stamps last_used_at -- a write, and a consumed rotation slot, which above the other checks happened for every request they then refused. The row is written before the launch, so a crash between them leaves a row with no command rather than a command with no row: the latter is a paid run neither cap counts. harness and model come from configuration with no defaults. The REST endpoint takes them from its request body; /fix has no request, and letting a commenter choose the model would let them choose the price. An operator who has not set both has not enabled the feature, and the refusal names the key rather than saying "not configured". Also found while wiring: the saga's thread-ref guard checked the reference and not the value. ThreadRef is a bare record over a String and every ingress builds one from Jackson's asText(), which answers "" for a node that is not there -- so a blank one is reachable. It used to be refused four layers down by findByThread matching nothing; it is now the key both caps count on and the subject of the run id, so it is refused where the reason is still legible. And the two FR-F32 cap constants were pinned only against themselves: the axis test compares what was passed against the constants, so widening either to 999 changed both sides and stayed green. Five mutations on the dispatcher, each killing exactly its intended test: transposing the review and finding on the row, dropping the existing-branch mode, ignoring the row refusal, emptying the prompt, and checking the spend cap ahead of the claim. 1127 orchestrator tests across 125 suites, 0 failures.
The .env.example contract lists every key a service reads. These two are operator config on the orchestrator, unlike the publisher variables an earlier round correctly kept out of it -- those are per-run container environment, not settings anyone sets.
Nothing in this codebase could open a pull request. The reviewer only ever commented on ones other people opened, so a factory run ended at a pushed branch -- and a branch nobody reviews is not a delivery. This is the port and the GitHub adapter; GitLab and Bitbucket follow. Idempotency is the reason findByHead is on the port rather than inside one adapter. What triggers an open is a Kafka record, redelivered on every consumer restart, and by then the push has happened -- so the branch exists and the API would cheerfully open a second pull request from the same head. GitHub happens to refuse that; GitLab and Bitbucket do not. "Let the forge decide" is one forge's behaviour, not a rule, so every adapter looks first. The lookup throws on a read fault rather than answering empty. Empty is the answer that AUTHORISES opening one, so an adapter that cannot reach its forge would open a duplicate on every redelivery -- the exact failure the lookup exists to prevent, arriving through the lookup. NothingToPropose is named by the PORT, not by each adapter. "The agent changed nothing" arrives as a 4xx on every forge and reads like an error on every forge -- GitHub's is a 422 about "no commits between", which sends an operator looking for a permission problem that does not exist. Normalising exactly that kind of per-forge wording is what a port is for. An unmatched 4xx stays a fault, so a wrong guess degrades safely. The already-exists 422 is still reachable despite looking first, because two deliveries can race between the read and the write. The loser re-reads and answers the winner's pull request, since the caller needs a number either way. If the re-read finds nothing the original exception is raised: inventing a success for a pull request nobody can see would record the run as delivered with nothing behind it. The factory mark is a fixed marker at the top of the description, not a label. GitHub and GitLab have label APIs for pull requests and Bitbucket Cloud does not, so a label would be a mark that exists on two forges out of three -- worse than no mark, because a consumer learns to trust it and is then silently wrong on the third. A pull request opened by the machine account is one the reviewer's own author allowlist might skip, which AUTONOMY.md names as the silent failure. The adapter reuses GitHubClient rather than building its own HTTP client, because RedirectHandlingHasOneHomeTest permits exactly one hand-rolled redirect policy in the repository. A second client here would fail the build, which is the check working. SCM-MAPPING gains §8 with all four forges, and it says plainly that no column has been measured against a live API -- the tests drive a WireMock stub this repository wrote, which establishes what the adapter does and nothing about what GitHub does. The quoted error strings are the least reliable rows and UNVERIFIED.md now carries that. ARCHITECTURE §3.4's sketch is replaced by the shipped signature; the three differences were forced by forges rather than chosen. 14 adapter tests, 8 body tests, 7 port tests. Ten mutations killed, each by exactly its intended test -- including skipping the lookup, dropping state=open, dropping the owner prefix from the head filter, and accepting a zero pull-request number.
Completes the three PullRequestSink adapters and wires them into ProviderClients, the composition root already allowed to name a provider. All three forges are supported, so unlike threadSource there is no degraded path: a fourth type cannot open a pull request at all, and pretending otherwise would record a run as delivered with nothing behind it. Two of SCM-MAPPING §8's divergences are tested rather than described, because both are silent when wrong. GitLab numbers a merge request twice. iid is the per-project number in the URL and in every API path; id is a global identifier that addresses nothing a human sees. Reading id yields a number that looks entirely valid, stores cleanly in factory_run.pr_id, and points at another project's merge request -- nothing fails. The stub carries BOTH values and they differ, so the wrong one is reachable; a stub with only iid would let the mutation pass by returning zero, which the ref refuses for an unrelated reason. Neither GitLab nor Bitbucket refuses a duplicate. GitHub does, which is what made "let the forge decide" look like a rule. Here the find-first call is not a second line of defence -- it is the only thing between a redelivered record and two pull requests, and both tests stub a SUCCEEDING create to prove the lookup is what stopped it. Two more per-forge shapes that fail quietly: - GitLab spells the open state "opened". A wrong value is not an error on that API -- an unrecognised state filter is ignored and everything comes back, so a merged merge request would suppress a new one. - Bitbucket nests where the other two are flat (source.branch.name, links.html.href). Jackson reads a missing node as an empty one, so a wrong nesting sends no branch at all and the forge answers about a different field. 25 adapter tests across the two modules. Six mutations killed: reading id instead of iid, GitHub's state spelling on GitLab, skipping the lookup, flattening Bitbucket's branch fields, reading the self link instead of the html one, and dropping the state clause from the query. testFast green: contract 133, github 83, gitlab 87, bitbucket 75, arch 46 -- 0 failures.
A pull request is unique per (head, base) PAIR on every forge, and the lookup was keyed on the head alone -- strictly WIDER than the rule the forge itself enforces, since GitHub's duplicate refusal fires only when both match. So the lookup could answer a pull request aimed at another base, the caller would record that as this run's delivery, and the one that should exist would never open. ADR-040's existing-branch mode makes it reachable by design: it pushes onto a branch that already has one. The Bitbucket test in the same commit already named this failure -- "the caller would record someone else's pull request as the run's output" -- and the state clause was added for it while the base clause was not. Also from the same round: - The nothing-to-propose classification now needs the forge's STATUS as well as its wording, and Bitbucket matches its full phrase rather than the two generic words "no changes". The asymmetry decides it: an unmatched failure degrades safely, staying the forge's own fault, but a falsely matched one reports a run as "the agent changed nothing" when the forge refused for another reason. The match runs against a 500-character raw body snippet, so without the status an HTML page from a proxy is scanned by the same substring test as a validation response. - No adapter matches an already-exists wording any more. That case is identifiable by BEHAVIOUR: on any refusal that is not nothing-to-propose, ask the forge whether one exists now. The table's own Bitbucket cell admits the phrasing is unknown, so the guard would never have fired there and a genuine race would read as a failure. - A fault on the RE-READ no longer replaces the refusal that started it. It is attached as suppressed, so an operator reads the create's 403 rather than the lookup's 503 -- the cause replaced by its own diagnosis. - Bitbucket refuses a branch name carrying a quote or backslash instead of interpolating it into its query language. A quote is legal in a git refname and URLEncoder protects the transport, not the parser, so x" OR state="OPEN would widen the clause to the repository's first open pull request. Reachable: /fix reads the source branch from the webhook projection, which a pull-request author controls. - ProviderClients now ASSERTS the FACTORY role. The previous javadoc said that was impossible because the role is part of the lookup key rather than the row; the row had it all along and the mapper simply did not read the column. ScmProvider carries it now and every construction site states which account it stands in for. Two claims I had written were wrong and are corrected rather than patched around: - The port and ProviderClients said the reviewer's author allowlist would skip a pull request the reviewer itself opened. Nothing gates pull-request authorship -- the bot-authored check covers comments and commands only -- and an empty allowlist means everyone, so by default it WOULD review its own. The real consequences are narrower and still sufficient, and are now stated instead. - FactoryPullRequestBody claimed the whole body was orchestrator- authored with only the paths agent-influenced. The task is ExecuteRun.prompt, which for a fix run is FixPrompt's output -- model-derived and entirely multi-line -- and it was interpolated raw where one line was reserved. It is cut to one bounded line now, in the body as it already was in the title, and the fence widens past a path that would close it. PullRequestRef refuses a URL that is not http(s), since it becomes an href. The host is deliberately NOT pinned to the API host: Bitbucket serves its web pages from a different one. The three adapter suites had already diverged in round one -- GitLab had no missing-URL case, so half its read guard was asserted by nothing. Each now carries the cases the others had. Twelve mutations killed, each by exactly its intended test. testFast: contract 134, github 86, gitlab 92, bitbucket 79, arch 46. Orchestrator 1142 tests, 0 failures.
Nothing joined a run to a review. factory_run has carried review_id and finding_ref since V54 and no query read them beside anything, so neither the caps' evidence nor "what did this cost" could be shown to a person -- and there was no list endpoint at all, so every "which run..." question needed a database. That is the gap techdebt/spire-ui records. GET /api/runs is viewer-and-admin, matching the detail endpoint beside it. Reading which runs exist is not the privilege that matters here; DISPATCHING is, and that stays admin-only on the POST -- asserted by a viewer who may list and may not dispatch. An unrecognised filter value is refused rather than ignored. Silently dropping a mistyped status=faield returns every run, which reads as "nothing is stuck" -- the most dangerous possible answer to the question that page is opened to ask. Statuses are validated against the projection's own set, and FactoryRunStatusesAreCompleteTest DERIVES that set by reflecting over the constants rather than listing the same nine names twice and agreeing with itself. Cost is a type, not a long. RunCost makes unknown unrepresentable as zero, which is ADR-023 applied where this project has already been bitten once: SUM skips NULL, so a run with one unpriced line reports the priced remainder -- a number that looks like a total, is smaller than the truth, and carries no sign anything is missing. The join counts null lines precisely to catch that, and a total over a list is unknown if any member is. Zero stays a KNOWN answer, because an UNMETERED model really does cost nothing and hiding that would be the mirror error. The wire form is a nullable Long rather than an OptionalLong: an Optional serialises only when a Jackson module is registered and its shape differs between them, and a money field whose JSON depends on module registration can silently become 0 in one service and not another. A build run names no review and no finding rather than carrying blanks. V54's CHECK already refuses a non-FIX row that names either; the read must not invent them back, since a blank renders as a link to nothing. Eight mutations, eight kills -- and one of them earned its keep by catching a weak test of mine. Widening the endpoint to @permitAll left every case green, including the anonymous one: that 401 comes from the deployment's auth policy before any annotation is consulted, so it passes whatever the method is annotated with. Only an authenticated caller holding neither role can tell the annotation apart from the wall behind it, and that case is now there. 1170 orchestrator tests across 129 suites, 0 failures.
M2's roadmap asks for "a finding reference on ReviewDetail.FindingView, so the dashboard has an id to dispatch with". It is already there, and it arrived through conversation linking rather than through this task: FindingView.threadRef. So what T8 actually owed was not a new field but the assertion that the two are the SAME key, which nothing made. They are reached by different code. The dashboard reads review_finding.thread_ref through ReviewProjection; the dispatch resolves a comment to its conversation root and calls FindingProjection.findByThread with it, and both fix caps count on that same value. Two paths to one key is the shape this branch has already paid for -- two encodings of one rule that agree until one is edited. The test drives a finding to a posted comment and then looks it up using the value the DASHBOARD would render, not one it invented. The third case shows why the key is the thread ref and not the row id: a second round re-records a review's findings delete-then-insert, so the id changes and the ref does not. It asserts the id CHANGED first -- without that, the test would prove nothing about why the ref is the key. And a finding with no posted comment carries no ref rather than a blank one. A blank renders as a dispatchable control that cannot work: the lookup matches nothing and the author is told the thread names no finding, about a finding right in front of them. 1173 orchestrator tests, 0 failures.
Round-1 review of T8. It found no production defect -- and two tests of mine that were guarding almost nothing. The status guard was the worse of the two, and both of its halves were wrong in the same direction. Its NOT_A_STATUS allowlist was unreachable: the one entry's VALUE already failed the shape filter beside it, so deleting the allowlist changed nothing while the javadoc called it the thing keeping the derivation honest. And that shape filter -- value must be lower_snake_case -- was a SILENT exclusion: a status spelled with a digit, a hyphen or a capital would have been dropped from the derived set, leaving it equal to a STATUSES that also omitted it. Green about a status nobody can filter for, which is the exact failure the file exists to prevent. Removing the shape filter made reflection sweep up the class's SQL constants, needing an allowlist that grows with every query and that nothing checks for staleness. So the reflection went instead. It was the weaker half regardless: it compared Java to Java, and both halves could agree while a migration added a tenth value to the table. The test now reads factory_run_status_closed -- the CHECK the projection actually writes against -- and compares that. A second case asserts the constraint is found by name and that the lookup really answers empty for a name that is not there, so a rename cannot leave it comparing nothing to nothing. The second was an assertion that could not fail. It claimed a row id does not survive a round, by asserting the new id differs from the old -- but FIND_BY_THREAD is ORDER BY id DESC LIMIT 1 over a monotonic serial, so a higher id after a second insert is true by construction. Delete the delete-then-insert entirely and it still passed: it measured the sequence, not the replacement. It now asserts the OLD row is gone and that exactly one remains, which is what makes an id unusable as a dispatch key. Its comment also said "round two" while passing round = 1; the code was the honest half and the comment is corrected. Also from the round: - The cost subquery now filters archived_at like every other llm_charge read. Nothing writes that column today, so the predicate is inert and looks dead -- it is there because the day purge lands, this page would total lines every other cost surface excludes. - costOf reads wasNull() into a local immediately. It was correct only because it sat to the left of another getLong in the same short-circuit, which any reordering-for-readability would have broken silently. Its two unreachable branches are now labelled as defensive rather than left looking tested. - RunCost.zero(). unknown() is an ABSORBING element, not an identity, so the obvious fold seeded with it answers unknown for every input -- including a list where every cost is known. - limit is parsed from a String. As an Integer query parameter a failed conversion is mapped to 404 by JAX-RS, so ?limit=abc answered "there is no such endpoint" about an endpoint that exists. - Both filters now accept either case. ?kind=fix worked and ?status=QUEUED was a 400 -- two conventions in one query string. - RunFilter's javadoc claimed a record removes the transposition hazard. A canonical constructor is positional, so it moves it. What actually polices it is named instead. - A selected-but-never-read column dropped, addExact so an overflow cannot be reported as "costs less than nothing", and the page-size constants moved beside their siblings. Three more mutations killed: zero() made unknown, the limit parse reverted to a 404, and the status case-fold removed. 1178 orchestrator tests, 0 failures.
The factory had no UI at all. Dispatch resolution and the credential
pool were curl, and every "which run..." question needed a database --
techdebt/spire-ui/4-3-the-factory-has-no-screens-at-all.md.
The whole design turns on one recorded trap: a status the UI's type
system cannot see. ReviewStatus is a compile-time union and the value
arrives as runtime JSON, so an unlisted one fell into the SUCCESS branch
-- refused rendered as five green segments, a degraded run as "clean".
RunStatus lists all nine values AND every reader handles one absent from
it: an unrecognised status is NAMED ("Unknown (x)") and takes the
warning pill, never the completed one, and is not claimed to be still
running either. Both wrong answers are wrong differently -- busy makes a
finished run spin forever, ok makes a broken one look clean -- and
finished-but-flagged is the one that leads an operator to look. The test
drives a value deliberately absent from the union, cast at the boundary,
which is exactly how one arrives in production.
Cost carries ADR-023 to the screen. formatCost already renders null as
an em dash and a real zero as zero, so "nobody knows" survives from the
ledger through RunCost to the table without becoming free.
Built from the vocabulary the stylesheet already has -- card, prov-table,
pill, wh-empty -- rather than a runs-* vocabulary of its own. The first
draft invented five classes and styles.contract.test.ts refused them,
which is that check doing its job: four screens once shipped completely
unstyled behind a fully green suite because they asked for classes
nothing defined. Reusing the review pills also means a run reads in the
same colour language as a review rather than a private one, and the
change adds no CSS at all.
A review id is parsed with a regex rather than split on "/", because a
GitLab workspace may itself contain slashes -- a naive split sends an
operator to a page that does not exist, and a dead link reads as a bug
in the review rather than in the link.
No prompt panel. V43 leaves the dispatched prompt out of the read model
deliberately (DATA-MODEL 5); showing it means storing it encrypted like
run_event.payload, which is a decision rather than a panel.
Three mutations killed: an unknown status given the completed pill, an
unknown status rendered blank, and an unknown cost rendered as $0.000.
spire-ui 483 tests across 61 files, tsc --noEmit silent.
The factory was in no deploy artifact. This adds the run worker to both packaged stacks -- built and pulled -- with its own OIDC client, the publisher image it refuses to start without, and the Docker socket it needs to place run units. It is behind a compose profile, and that is a security decision rather than a convenience. A Docker socket is ROOT-EQUIVALENT ON THE HOST -- docs/SECURITY.md says exactly that under "What is NOT mitigated" -- and the run worker is the one service that executes untrusted model output. A compromised run worker is a compromised machine, and that should be something an operator opted into rather than something that happened because they typed `up`. Verified both ways: `config --services` lists seven services by default and eight with --profile factory. DockerSocketMountsAreOptInTest enforces it, and states the rule about the SOCKET rather than about this service -- naming the service would guard today's file and miss the second one that ever needs a daemon. It also asserts the run worker is PRESENT in both stacks, because deleting it would otherwise leave the socket rule vacuously satisfied, and that a second service has not quietly acquired a mount. Two mutations killed: removing the profile line, and deleting the service from the pulled stack. Kubernetes deliberately does NOT get the run worker, and the README says why at length. There is one runtime implementation and WorkerRuntimes says so: "M0 has one arm. Selecting between them by configuration is M5's job." So a Kubernetes deployment would have to mount the NODE's socket into a pod -- precisely what SECURITY.md promises the Kubernetes arm removes. Shipping that chart template would ship the exact thing that sentence says does not happen. The chart, the overlays and the rendered manifests are therefore untouched, and there is no drift to check. deploy/.env.example gains a separated factory block that leads with the socket warning rather than mixing the keys in above it, and names the cheapest mitigation: a daemon that is not this host. testFast green; spire-arch 49 tests; the chart self-test passes.
M2's exit criterion, the half a container can establish. Real containers, a real smart-HTTP remote, the real publisher image, and a shell script standing in for the model -- the machinery M0WalkingSkeletonTest already uses, so there is no network model and no spend. What it proves: a fix run pushes onto a branch that already existed on the remote, which M0's publisher refused outright; the content is read back FROM THE REMOTE rather than from what the run reported, because a reported push is not a push; and NO spire/ branch is created beside it, since a second branch is the outcome ADR-040 exists to avoid. TestOrigin gains branchFrom and commitOf. It could not previously make a branch a human would have made -- every branch on that remote came from the publisher, inside spire/ -- so the case could not be set up at all. Two findings, both from my own tests being wrong first: A test asserted the publisher refuses a run whose branch equals its destination. It cannot: ExecuteRun's compact constructor refuses that outright, so the command never exists. The test failed and the test was what was wrong. It now asserts the construction refusal, which is the guard that actually fires, and says why the publisher's own floor is unreachable from here. And the trunk case does not prove what it first claimed. Deleting PublisherConfig.looksLikeATrunk leaves it GREEN. A control probe -- refusing every branch -- reddened the two permitted-push cases, so mutations do reach the container and the survival is real. Measuring it showed the run dies as RUNTIME_UNAVAILABLE, init container failed with exit 1, before the publisher is consulted: WorkspaceClone.populate calls checkout().setCreateBranch(true) and a clone has already materialised the default branch locally. Two independent guards with the outer firing first -- defence in depth working, and a claim no container test establishes. The assertion stays, because "the trunk never moves" is true and worth pinning; the javadoc now says which guard refused it, and UNVERIFIED.md carries the gap so nobody later leans on "we tested it end to end". 5 tests, 0 failures.
T11's reconciliation half is blocked on a missing runtime capability, not on effort. A run unit lands on Docker's default bridge -- RunUnitSpec has no network and DockerRunRuntime never sets one -- so it cannot resolve the e2e stack's gitlab service. The runtime's own comment already names this for egress; M2 found it also blocks ingress to the stack's own services. The obvious workaround is rejected: compose.e2e.yml binds GitLab to loopback because it holds an admin token that is a public constant in this repository. Undoing that to make a test pass weakens the stack AND leaves the test exercising a topology no deployment uses. Filed as High with the two real options, and recorded in UNVERIFIED so nobody describes the exit criterion as automated.
Five defects on the /fix arm, four of them reachable from a comment. The escaping NPE. MachineAccounts.resolve did not guarantee a login, and a blank one is stored as SQL null, so it reached MachineAccountCredential's requireNonNull. RunResource guarded that and said why; the /fix path re-derived the same lookup and dropped it. On the REST arm a throw is a 500 the caller reads. On the Kafka consumer it escapes the saga: redelivered forever, author told nothing. The guard moved into resolve() so neither caller can forget it again, and RunResource reads the registration back on the failure path to keep naming which of the two causes it was. The fix claim keyed on a bare forge comment id. Every ingress passes the forge's own id straight through, and it is unique within one forge and nowhere else -- two providers, or two self-hosted GitLabs whose note ids both start at 1, collide. Unscoped, a legitimate /fix is refused while naming another workspace's run id in this review's durable history, and the race the index backstops dead-letters after pool.select() has spent a rotation slot. V56 is unmerged so it is amended, not stacked: the key is (review_id, comment_id), and a new CHECK makes a FIX row name the comment that asked for it. The push allowlist matched on a username. authorAllowed accepts either a handle or a stable id, which is right for a command costing one model call. /fix authorises a commit pushed as the machine account, and a handle can be released and re-registered by somebody else, so /fix now matches on providerUserId alone. Two broker outages retired a finding forever. Both caps counted rows whose dispatch was never acknowledged -- runs that never executed and never spent, and which the projection already treats as re-armable. The filter names the cause, not the status: a run that executed and then died still counts, because the cap is about money already gone. An unreachable second refusal. FixDispatch.Planned carried the raw provider type, so the dispatcher re-parsed it with a word-for-word identical refusal that no input could reach and no test covered. Planned now carries the parsed ScmType and the copy is gone. Also: RunLaunch.Outcome loses an isReArmable() default that only its own test consulted, and FactoryPullRequestBody says plainly that nothing calls it in production yet. Seven mutations, each killing exactly its test. Two of them were false survivals first: the harness restored with git checkout, which reverted the fixes under test, and its perl patterns used a bare newline against CRLF files so three mutations never applied at all. A mutation that does not compile or does not apply measures nothing.
The `factory` profile started a run worker nothing could dispatch to. SPIRE_FACTORY_AGENT_IMAGE_CODEX, _FIX_HARNESS and _FIX_MODEL reached neither packaged stack and neither .env.example, so POST /api/runs refused every request for want of an agent image and /fix refused every comment for want of a harness -- with a healthy worker and an empty queue as the only symptom. Both compose files now pass all four keys and deploy/.env.example documents them beside the ones that start the worker. Five doc blocks introduced on this branch sat in a stacked pair, so the first of each was discarded -- the trap techdebt/global/4-2-orphaned-javadoc-blocks-are-discarded-with-no-guard records. Two were the design record itself: why providerType is nullable, and why asFixFor is a wither. In the two test files the orphan belonged to the test a later one was inserted in front of, so those move down rather than merge. Two techdebt entries were edited on this branch and then falsified by it. The UI entry claimed spire-ui referenced no run status and that there was no list endpoint at all; Tasks 8 and 9 built both, so it is narrowed to the three surfaces that really do still have no screen, and retitled. The class-size entry carried ~530 / ~450 physical-line estimates for the two factory classes; measured on its own preferred measure they are 350 and 404 code lines, and FactoryRunProjection crossed 300 on this branch, up 66%. Approximating was a deliberate choice and it was the wrong one: an approximate number is not maintained either, only harder to check. The two network entries were one debt filed twice -- same root cause (no network field on RunUnitSpec), same first option (a per-run user-defined network), different symptoms and different criticality. Merged into one High entry covering both: the unit is isolated from nothing, and can reach nothing.
.claude/agent-memory/ was swept into the previous commit by a broad `git add -A`. It is local scratch, not project content -- review dispositions are what belong in the repository, and those live in .claude/reviews/. Untracked here and ignored from now on so the next broad add cannot repeat it. The files stay on disk.
The prompt fence was closable from inside it. A finding message is model output about a diff a contributor wrote, so a sentence planted in a pull request can be quoted into a review comment and from there into the prompt. Writing inside the fence buys nothing the surrounding text does not already account for; writing the END marker is different -- it closes the fence, and everything after it reads as the orchestrator's own voice, the one position in the prompt not labelled as contributor-derived. Both markers are now neutered wherever a value carries one, which shows the finding in full and still ends the fence where this class ends it. The three headers sit outside the fence and were unbounded. Every value there is model output, including the path -- it is the path the model reported, not one matched against a diff hunk -- so a newline in any of them wrote an unfenced line. Each is bounded to one line now. /fix proceeded silently on a spend gate that could not read the ledger. Failing open is the project posture and is not changed here: refusing on a failed READ turns an outage into something that reads as policy, and the attention row already tells the operator. What differs on this arm is the size of what proceeds -- a review call is one LLM call, a fix run is a container with a wall clock and a push credential -- so it now warns in its own log rather than relying on a panel. Three quality items from the same round. The dispatcher's two refusal helpers returned an Optional used purely as a control-flow carrier and unwrapped with .get(); they return the refusal or null. Runs.tsx grew a ReviewCell for what was a nested ternary calling reviewPath twice and casting the result. And the method-size debt entry, which exists so the rule is not silently suspended for one package, gained the largest member of its own set: dispatch is 87 code lines and five parameters. Two mutations, each killing exactly its test.
Three merge-gate documents, all of which describe the state this PR changes. CLAUDE.md's Status snapshot said "Next is M2" and, in one line, said something false rather than merely stale: "spire-run-worker is not in deploy/ yet". It is, in both packaged stacks, behind the factory profile. Rewritten rather than appended to, as that section instructs. The delivery claim carries its qualifier in the same sentence: the loop M2 exists to close has never been run end to end in one place. Measured rather than estimated: 2877 Java tests across 323 suites, 0 failures, 1 skipped (testFast + testServices), and 483 spire-ui tests across 61 files. The previous figures were from 2026-09-03 and were 329 tests short. The nightly testE2e tier was not re-run, and the entry now says so instead of quoting its figure as if it had been. docs/CONTRACT.md listed six of the nine SPI ports. PullRequestSink is this PR's omission; ThreadSource and IdentitySource predate it. All three are added together, because CLAUDE.md names this block as the source of truth for ports and a list with three silent gaps is not one. docs/HISTORY.md gains the M2 entry: what shipped, what the whole-PR round found, and the two tests of mine that were wrong before any production code was. Also recorded there is the mutation harness that produced three false survivals in one run -- it restored with git checkout, reverting the fixes under test, and its patterns used a bare newline against CRLF files so three mutants never applied at all. And DECISIONS.md is described as holding ADR-001..020. It holds ADR-001..040.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Delivers M2 — Deliver, and close the loop (#113), the software factory's shippable boundary:
a review finding becomes a dispatched fix run, the fix lands where the finding lives, and the
existing reviewer reconciles it closed.
Scope is the corrected plan from #110, not the 2026-09-01 draft. Every task is TDD — tests first,
RED confirmed, then GREEN — and every guard is mutation-verified: break the production line, confirm
exactly the intended test fails.
Checklist
Prerequisites
e6f1a9e+e92a5f2. It was three paths, not three commands.real forge, real credentials and a real model key.
docs/UNVERIFIED.md§B records that cancel,steer, the watchdog, the push gate and the charge ledger "have only ever met a WireMock LLM
and a local origin". Flagged rather than silently skipped; the automated exit-criterion
scenario in
spire-e2e(below) covers what a container can cover.Tasks
e6f1a9e, reviewed + fixed ine92a5f2)/fixin the command vocabulary (85c1398):CommentCommands.FIX, the shared gateway set, parity-tested on all three SCMs/fixsaga handler (13ce642): resolves the target finding from the thread, refuses in a way the author can act on. Idempotency moves to T4, where the spend is.4fa75e1): V54 (review_id,finding_ref,kind+ a two-way CHECK), FR-F32 on two axes,nextAttempt,ChargeKind.FIX.Re-scoped: the saga→command wiring and
SpendGatemove to T5b, so this slice is the datamodel and the policy, fully tested, rather than half of each
existingbranch mode, publisher half (5ff6d67): the namespace andbranch≠base rules lift together under an explicit mode, never inferred;
main/masterand thepull request's destination branch refused in every mode
4acff11, reviewed + fixed inb4f1e44)a365d79, reviewed + fixed in182f3bd+cf1f6fe): all three ingresses read it,V55stores it,FixTargetsrefuses on it. Closes the blocker below and retires its debt entry:FixTargetsresolves the target fromreview_statusand separates resolved from pushable, so a refusal can say which it was.Writing it found a blocker — see below.
ExecuteRunwithSPIRE_BRANCH_MODE=existing,SpendGate, thecommentIdspend claim T3's review deferred here, and non-fast-forward as are-dispatchable outcome
PullRequestSinkport + GitHub adapter, withfindByHeadidempotency and thefactory-authored attribute
PullRequestSinkadaptersGET /api/runswithRunCost(unknown is never zero), therun↔review join, and the pin that the finding a dashboard shows is the one a dispatch
resolves. Two items of the plan came out differently and are recorded rather than ticked:
the finding reference on
ReviewDetail.FindingViewalready existed asthreadRef, so thework was the assertion rather than the field; and run cost is not posted on the pull
request — it is on the runs screen and the API. Posting it belongs with the pull-request
opening step, which is M3
or dispatch control, and no dispatch button on the review detail: those are actions, and the
three surfaces that still have none are in
techdebt/spire-ui/4-3-three-factory-surfaces-still-have-no-screen.md. No prompt panel — V43leaves the prompt out of the read model on purpose
spire-run-workerin both packaged compose stacks, behind thefactoryprofile,because the Docker socket it mounts is root-equivalent on the host. Helm and kustomize
deliberately do NOT get it — a Kubernetes deployment would mount the node's socket into a
pod, precisely what
SECURITY.mdpromises that arm removes. The chart, overlays and renderedmanifests are untouched, and that is the decision rather than an omission
with real containers (
Adr040ExistingBranchTest). The reconciliation half is not, and isblocked rather than skipped: a run unit lands on the default bridge and cannot resolve the
spire-e2estack'sgitlabservice, becauseRunUnitSpechas no network field. RebindingGitLab off loopback would undo a deliberate security control, so it is not the answer. Filed as
techdebt/spire-runtime-docker/2-3-a-run-unit-has-no-network-so-it-is-neither-isolated-nor-reachable.mdand carried in
docs/UNVERIFIED.md. See the T11 section belowCritical, 3 Medium, 1 HIGH rules violation and 14 more, all fixed and mutation-verified.
Dispositions in
.claude/reviews/global/m2-t12-whole-pr.mdCLAUDE.md's Status snapshot rewritten (one line in it was false, notmerely stale), the M2 entry in
docs/HISTORY.mdwith its qualifier in the same sentence asthe delivery claim, and
docs/CONTRACT.md's port block completed✅ Blocker found and closed — fork pull requests
Building T5b(i) surfaced this: nothing in the deployment recorded whether a pull request came
from a fork. Not
review_status, notPullRequestEventReceived, not any ingress.It mattered because a fix pushes to the branch
source_branchnames, in the repositoryworkspace/slugnames — and for a fork those two do not belong together. Either the base repo hasno branch of that name and the push creates a stray one, or it does and a machine-authored
commit from a different diff lands on someone else's work. The publisher's floor does not catch
it: that floor refuses trunks and the destination branch, and a fork's source branch is neither.
ADR-040 already put forks out of scope. The decision was right; the enforcement did not exist.
Security verified the claim, confirmed High, and added one refinement worth having: today the init
clone would usually fail first, because
WorkspaceClonefetchesrefs/heads/*only — anaccident of the default refspec, not a control, and the bad case survives whenever the fork branch
tip is also a base-repo branch tip.
Closed in
a365d79, end to end: three ingresses read it (two repository names on GitHub andBitbucket, two numeric project ids on GitLab), the wire record carries it additively with a wither,
V55gives it a column, the saga persists it on every event, andFixTargets.isPushable()refuseson it. The gateway asserts all three providers agree — one spelling it backwards would let forks
through on that SCM alone while its own per-provider test passed. Four mutations, each killing
exactly one test. The debt entry filed hours earlier is retired and ADR-040 now says the rule is
enforced and by what.
Exit criterion
pushes to that pull request's source branch under ADR-040's
existingmode, and the nextreview round reconciles the original finding as resolved — with
mainand the pullrequest's destination branch proven still refused in that same mode.
T1 — Observe-mode gate
policy.observeOnly()was consulted in exactly one place: the pull-request event path. Every/commandreachedonManualCommand, which never read it — so/reviewand/findingboth actedfor real under a mode whose stated contract is no comments at all.
The plan first said
/fixalone would check the policy while the other two kept the hole as aseparate bug. That routes around the gap and leaves the next command to inherit it a fourth time.
The debt entry had deliberately refused to decide this, calling it a product question: an
operator who explicitly types a command might reasonably expect it to work regardless of a passive
default. That reading does not survive contact with who can type one. The author is gated by the
per-provider allowlist, not by operator role, and an empty allowlist means "review everyone" by
design — so any commenter could force a paid re-review while the operator believed the deployment
was only watching. The operator's override is the setting they already own.
One gate, after the allowlist and ahead of the command switch. Both positions are load-bearing.
After the allowlist, because that gate answers whether this person's command counts at all, and
reporting "the deployment is passive" about someone never authorized names the wrong cause.
Before the switch, because a command added below it arrives ungated — exactly how
/reviewandthen
/findinggot in.The refusal is silent, and here that is forced rather than chosen. Every other silent refusal in
this saga argues for its silence (a reply confirms to a prober that a command is wired). This one
could not reply even without that argument: posting a comment is the exact thing observe mode
forbids, so answering would break the mode in the act of enforcing it. The timeline records
ManualCommandObserveOnly, distinct from the authorization refusal'sManualCommandSkipped,because an operator reading "nothing happened" needs to know which of the two it was.
Five tests, mutation-verified in both directions:
return— the gate notes but does not stopTwo of the five are negative controls, so a gate that refused everything would not pass them.
techdebt/global/3-2-slash-finding-bypasses-observe-mode.mdis retired. The decision and itsreasoning are recorded in
docs/factory/ROADMAP.md§ M2 and carried forward indocs/HISTORY.md,where the original entry claimed the opposite.
T1 review round — four lenses, and the gate was one of three doors
Semgrep 2/2 clean. qa reproduced 1016/1016 on a
git archivecopy and ran every probe there.The gate as written was correct. Everything found was a path it could not reach. Three lenses
independently reported the same shape:
policy.observeOnly()had two call sites, and closing the/commandhole left two more open.An author's reply — the widest path, and the gate could not reach it. An @-mention makes a reply
eligible regardless of thread ownership and removes the per-thread turn cap, so where
/reviewlost one paid call this loses an unbounded number. Not reachable on a deployment that never went
active, which is why it reads as theoretical — but the realistic case is the operator gesture the
slider exists for: run active, then flip to observe to pause the bot, at which point every thread is
still bot-owned and every reply is answered and billed. qa proved it by probe against unmodified
HEAD, not by argument.The archived-review notice posts a comment, and no placement inside
onManualCommandcould everhave reached it — the archived gate runs in
handle()ahead of the whole switch. Reachable end toend: observe registers with
status='observed',archiveRowrefuses only'reviewing', so anobserved row archives cleanly and the author's next push triggers a live comment.
Both are gated in
IntegrationSagarather than in the classes that own the behaviour, deliberately:the defect was enforcement scattered with one site missed, so every
observeOnly()read now lives inone file.
The scoping mistake, which is the part worth keeping
The task was taken from a debt entry whose own fix said one gate "closes all three paths (
/review,/finding, and any future/command)". True, and the wrong frame — it enumerates commands,while the contract is about action commands.
ReviewPolicy's javadoc says observe "emits NOaction commands", and that invariant is now asserted over the whole event vocabulary rather than
per branch, so a branch added later inherits the assertion. Per-branch tests found these one at a
time, which is how the second and third survived the round that fixed the first.
Four test holes, each mutation-verified
observeOnly() && allowlistFor(id).isEmpty()passed all five original tests/findingcase reddened by crashing, not assertingDataSource— the recorded trap, instance eight. Completing one fake only moves it, so all three on the path are completedFinal mutation matrix, all four killing exactly their target:
aReplyIsNotAnsweredInObserveMode+ the class guardtheArchivedNoticeIsNotPostedInObserveMode+ the class guard&& allowlist.isEmpty()anAllowlistedAuthorIsStillRefusedInObserveModereviewCommandIsRefusedInObserveModeThe definition was widened with the behaviour
Six surfaces still described the PR-event path only — including the tooltip an operator reads while
flipping the switch. That drift is the exact condition the retired debt entry named as the reason to
leave the code alone.
One thing is deliberately NOT gated, and it reverses my own first call. I argued for gating the
admin REST re-run, reasoning a 409 is not a comment so the silence argument does not apply. Security
supplied the fact that settles it: the admin re-run is the only route to review a single pull
request while still observing. Gating it leaves "go globally active" as the only option. Both
endpoints are
spire-admin, so the allowlist argument that justifies refusing a/commanddoes notdescribe them. The line — SCM-originated triggers are refused; an operator's own authenticated REST
action is the override — is now written into all six surfaces.
Also rejected, and worth recording because it is the tempting fix: a backstop in
CommandsEmitter.emit, the one funnel everyActionCommandpasses through. It would close all threeat once and would be a worse bug —
ResultSagaemits to continue an in-flight pipeline, and themode is a live slider, so a flip mid-review would strand a run in
reviewingwith nothing on the busto move it on. That is the permanent-
reviewingfailure this saga's own comment records havingalready fixed once.
A pre-existing break found while verifying
ApkUpgradeIsNotCachedTestcould not pass on Windows. Its matrix parser matched\nwhilecore.autocrlfgives CRLF on disk, and Java's.excludes\r, so.*\nnever reached the newlineand the
include:block "was not found" — a message about the workflow, for a fault in the parser.Green in CI on Linux, red on every developer machine, which makes
testFast— the pre-commit loopCLAUDE.mdprescribes — permanently red locally. Confirmed against pristineorigin/master. TheDockerfile splitter in the same file already used
\r?\n; two patterns did not. Fixed andmutation-verified.
One finding is filed rather than fixed: a settings-read fault falls back to the seed mode, which
may be
active, so a single failedSELECTmakes one event fail open. Pre-existing and shared byevery
observeOnly()caller — but this change adds three more.Full dispositions:
.claude/reviews/global/m2-t1-observe-gate.md.Verification:
testFastgreen ·:spire-orchestrator:testgreen ·spire-ui471 tests / 60files,
tsc --noEmitsilent.T2+T3 review round — four lenses, 1 Critical + 1 High + 6 Important
Reviewed together as one
/fixcommand path: T2 alone is two lines and not independentlyshippable. Semgrep 7/7 clean. qa reproduced 1030/1030 orchestrator and 74/74 gateway on a
git archivecopy before reporting. Fixes ind9a6adb.The three orderings the slice was built around all held. Every defect was somewhere else.
The two that would have shipped blind
An empty author allowlist admitted everyone to a command that pushes code. The allowlist means
"review everyone" by deliberate design — right for one spend-capped model call, wrong for a branch
pushed as the machine account — and
allowlistForanswersList.of()for an unresolvable providertoo, so that is a second everyone-answer.
AUTONOMY.mdRule 3 already names this threat in asmany words: "a drive-by contributor … the factory writes and merges their code using the operator's
credentials", and rules that the factory's actor list must be its own. So the design document
already forbade what was built.
Fixed in this round rather than with dispatch, for the reason security gave: the tests encoded
"empty = allowed" as the passing case, so every hour it stayed the gate got harder to change.
A
/finding-filed finding was a valid/fixtarget with no description. Itsmessageandsuggestionare NULL by design (DATA-MODEL §5 keeps quoted text out of the replayable log), soFR-F27's "complete task specification" would have been a severity, a path and a line — and
TargetFindingcarried noorigin, so the dispatch could not have detected it either. Refused here,because by then the target is accepted and the only options left are paying for a run on an empty
spec or retracting one.
Two lessons worth more than any single fix
A test can be killed by a later commit in the same pull request. T1's guard
aCommandWithNoHandlerIsAlsoRefusedInObserveModedrove"fix"to prove an unenumerated commandis gated. T3 gave
fixa handler two commits later, so it silently became a test of an enumeratedcommand — while staying green. qa proved it dead by narrowing the gate to the enumerated set: the
suite still passed. Vacuity does not only arrive with a test; it can arrive afterwards, from the same
author.
Writing the test qa asked for falsified my own production comment.
FIND_BY_THREAD's javadocclaimed several rows share a thread ref across rounds and the newest is live.
ATTACH_THREAD_REForders by
(thread_ref = ?) DESC, so a row already carrying the ref beats the newest unattached one— deliberately, so a redelivery lands where it landed first — and at most one row ever carries a
ref. That is also why qa's
DESC→ASCmutation survived: the match set has one element. Thehonest fix was to correct the reasoning, not to invent an assertion that would make a false claim
look tested.
Also fixed
/finding's refusal emitsRefuseFinding; there is noRefuseFixin the tree. The corroborating detail settles it — thefixture never assigned
commandsorworkerCredentials, so it could not have supported aspeaking refusal. Reworded, not built: the reply needs a new
ActionCommand, acontract-snapshot entry and a worker handler — all dispatch's surface. The refusals did gain the
durable row the observe gate's own argument demands, and
/finding'sskipped:/refused:split."RESOLVED"was a literal whereFindingVerdict.Statusexists, is already imported in thatfile, and is what the write side spells. No CHECK constraint on the column, so a rename would keep
compiling and stop matching in silence — on the guard deciding whether a paid run starts.
and only the bot's comments get a
review_threadrow, so a/fixtyped as a reply to anotherhuman's reply matches nothing while the finding sits visibly a few comments up.
rootOf's ownjavadoc documents the gap and calls it "harmless for the anchor" — not harmless for a message
making a claim about the reader's repository. Now says what it could not do; the functional gap is
filed, not absorbed.
/fixtakes no arguments, and that is now written down while it is cheap. Feedingcomment-supplied text to an agent holding a clone and a push token would let a commenter author
instructions to it.
Four fixture holes, each now mutation-verified
appendEventoverloads, which is exactly why it read as safe, while both bodies discarded the argument that tells them apartstartLine()→endLine()44, 44, making them interchangeablefindByThreadORDER BYflippedFinal matrix — six mutations, each killing exactly one test: the allowlist gate, the origin gate, the
durable row's ref, the start line, the severity, and the observe gate's breadth. That last one now
fails the guard qa proved was dead.
Deferred to dispatch, recorded so it cannot be lost
RefuseFix+ contract entry + worker handler).commentId, not(reviewId, threadRef)— a second genuine/fixafter a failed run must be allowed; the same comment redelivered must not pay twice.
Full dispositions:
.claude/reviews/global/m2-t23-fix-command.md.Verification:
testFast·:spire-orchestrator:test·:spire-gateway:testall green.T4+T5+T5b(i) review round — 2 High, 1 Critical, 6 Important
Semgrep 8/8 clean. Security measured
isValidRefNameagainst the pinned JGit rather than reasoningabout it. qa did not deliver a report and a direct request went unanswered, so its section is
unknown rather than clean — the mutation work below is mine and code-review's. Fixes in
b4f1e44.Every defect was in a CLAIM. The code was sound; three comments asserted a guard the schema did
not provide, one javadoc asserted a floor that was optional, and four mutations survived.
The two that mattered
The destination floor was optional exactly when it was needed. It ran only when
SPIRE_PROTECTED_BRANCHwas present, while the class javadoc said the destination is refused inevery mode. A dispatch that forgets one map entry skips it silently — and a trunk called
develop,which the
main/masterlist does not cover, gets fast-forwarded.existingmode now refuses tostart without it, blank included:
review_status.dest_branchdefaults to'', so copying itthrough unconditionally yields blank rather than missing.
V54's CHECK admitted blank ids.
'' IS NOT NULLis true, so a FIX row with empty-string idspassed and was counted by neither cap for any real id — the cap failing open for exactly that
row. Not hypothetical: this schema already uses blank-not-null for
source_branchanddest_branch.The reasoning error, and its unusually clean epilogue
A mutation survived and I concluded "the schema must be guarding it" rather than "my fixture
cannot build the row". The second was correct — the CHECK's right side is an
AND, so a non-FIXrow satisfies it by failing either conjunct.
Then security found the blank-id hole in the same CHECK, and closing it meant rewriting the
constraint as two explicit arms — which, as a side effect nobody set out to produce, forbids a
non-fix row from carrying a review at all. So my original claim is true again, for a reason that
had nothing to do with my original argument, and the filter is belt-and-braces until the
constraint is relaxed for SPEC and PLAN runs. That is now in the code with its expiry, because the
answer has been reached wrongly twice.
This is the same shape as
FIND_BY_THREAD's "newest row wins" two commits earlier. Asserting aguarantee, failing to kill the mutation, and crediting the guarantee instead of doubting the test.
Three more surviving mutations, now killed
nextAttemptreads the review axis, not the finding axisisBlank()→isEmpty()existingmodeMeasured, not reasoned
Security probed
isValidRefNameagainst the JGit this repo pins.Main,MAIN,HEAD,refs/heads/main,heads/main,-main, and names carrying a zero-width space or a Cyrillicаallpass it and passed the exact-match floor. None of them reaches
refs/heads/main— forge refsare case-sensitive, and
refs/heads/mainas a branch name pushes torefs/heads/refs/heads/main.So it is not a bypass; it is a machine creating a branch a person reads as the trunk, which earns a
refusal on its own. Invisible characters are refused, ordinary non-ASCII is not, and a test asserts
that second half.
Two things I never guarded
commithas the identicalNOT NULL DEFAULT ''assourceBranchand the identical failure —Env.requiredrefuses it inside the container, after the agent has been paid. The classdocumented that hazard at length for one of the two columns.
Docs
SMOKE-TEST.md'sPUBLISHER_MISCONFIGUREDrow is the one place a reader learns what makes thepublisher refuse, and still described the rules as namespace-only. ADR-040 claimed the refusal covers
"the repository default branch" while the code refuses two literal names — its own javadoc calls
that "a convention list, not a truth" — and never named
SPIRE_PROTECTED_BRANCH.One suspicion of mine was falsified. I flagged the two new variables as an
.env.examplecontract violation. They are not: none of the eight existing publisher variables is there either,
because they are per-run container environment, not operator config. Adding them would have invented
a contract.
The fork gap, verified
Security confirmed the claim, the High severity, and that "unreachable today" is true — with one
refinement worth having. The init clone would usually fail first, because
WorkspaceClonefetchesrefs/heads/*only and a fork's head is normally unreachable from a base branch. That is anaccident of the default refspec, not a control, and the bad case survives whenever the fork branch
tip is also a base-repo branch tip. Its phrasing for the remedy is the right one: the row is the
key, the forge is the proof.
Full dispositions:
.claude/reviews/global/m2-t45-fix-identity.md.Verification:
testFast·:spire-orchestrator:test·:spire-publisher:testall green. Sixmutations, each killing exactly its own test.
(One mutation initially killed the wrong test — a red run naming a case unrelated to the change.
Re-run in isolation it killed the right one. That is concurrent-Gradle contention on the shared Dev
Services database, the exact signature this project has recorded for it.)
T5c review round — the guard rule, and the matrix that could not see it
Four lenses over the fork-provenance and pushable-rule slice. Fixes in
182f3bdandcf1f6fe.Both reviews measured
2cac818, so roughly half of what they reported had already been closed by182f3bdwhile they ran; what follows is only what survived that.Security and qa independently found the same defect, by different routes.
review_statushasthree columns carrying
NOT NULL DEFAULT ''. Two were guarded and tested twice over. The third,dest_branch, was guarded by nothing — and it is the one that becomes the run command's protectedbranch, where
ExecuteRun's compact constructor throws on a blank inexistingmode. On a Kafkaconsumer an exception is a redelivery: the moment T5b(ii) wires the dispatch up, a review row with no
recorded destination would have refused forever and said nothing to the author who typed
/fix.The lesson: a matrix can only check whether, never which — and never a cause it does not vary
The 36-case matrix exists to keep two readings of one rule in agreement, after that exact shape
produced two credential scrubbers here whose rules quietly diverged. It does that job. It cannot do
this one: a rule missing from both readings agrees with itself perfectly. qa proved it rather
than argued it, by adding a fifth cause keyed on
destBranch— it survived all eight cases, becauseevery one of them named
develop.Confirmed again during verification: dropping the new
destBranchclause fails the two dedicatedtests and leaves the now-162-case matrix green. The matrix gains
destBranchand null-provenanceaxes anyway, because the next cause may well be one it can see.
V55 becomes nullable, which is ADR-023's rule applied to a boolean
V55's first draft defaulted
from_forktofalseand argued — in the migration, at length — thatthe default was safe because nothing consumed the column yet. That was true for about a day.
FixDispatchnow refuses forks andRunUnitBuildernow writesSPIRE_BRANCH_MODEfrom a chainstarting at that column, so the default stopped being a placeholder and became an answer the
migration invented. A pre-V55 fork review that never sees another pull-request event would have
authorised a push on the strength of it.
Old rows now say
NULLand are refused with a cause of their own — worded "push once and tryagain", deliberately not "your pull request is a fork", since the row is very probably an
ordinary branch pull request and the author gets one message.
FixTargetsreads the column withgetObjectrather thangetBoolean, which maps SQLNULLtofalsesilently and with the righttype. Unknown is never zero — the same rule the cost ledger runs on.
Also closed
SPIRE_PROTECTED_BRANCHin every mode; the run worker wrote it only inexistingmode, whilethe command happily represents
(existingBranch=false, protectedBranch="develop"). Now writtenwhenever it is known.
"github", so deleting the branchleft the suite green and moved the failure into
Optional.get()— the sameexception-where-a-refusal-belongs shape as the blank destination.
before ADR-040 reads as namespace mode. A round trip could not: it proves only that the new version
agrees with itself, and under ADR-014's short retention the in-flight payload during a rolling
upgrade is written by the old one.
isPushableandExecuteRun), and anassertion message that printed a map holding
SPIRE_GIT_SECRETnow prints its keys.Filed, not fixed — and they want one design
develop → mainis acompletely truthful row: open, not a fork, real refs. Its source is a branch several people
share, and ADR-040's "the destination is the truth" covers
developonly as a destination. Themachine account would commit straight to it.
—
techdebt/spire-orchestrator/3-3-a-long-lived-shared-branch-passes-every-fix-check.mdpr_stateis what the deployment last SAW, not what is true now. Every pull-request eventwrites
OPEN, so a redelivery after a merge flips a closed pull request back to pushable. It livedonly in a javadoc on the class that has it, where nobody planning the next slice would find it; it
is now in
docs/UNVERIFIED.mdand in ADR-040.Both need a dispatch-time re-read from the forge — which the orchestrator may do and the publisher
(ADR-039) may not. One design, not two.
Verification:
:spire-contract:test·:spire-orchestrator:test(Fix*,IntegrationSagaPolicy,FactoryRunProjection— 128 tests) ·:spire-run-worker:test(RunUnitBuilderTest— 24 tests) allgreen, 0 failures. Six mutations, each killing exactly its intended test — including the one that
confirms the matrix is blind to
destBranch.T5b(ii) — The dispatch itself
/fixis now a run.IntegrationSaga.requestFix→FixRunDispatcher→ExecuteRunoncs.run-commandsin ADR-040'sexistingmode, pushing to the pull request's own source branch sothe next review round reconciles the finding that caused the work.
Four commits, each mutation-verified: the launch extraction, the prompt, the claim, the wiring.
Nothing shared was written twice
The write-row-then-emit logic was inline in
RunResource—new RunCommand.ExecuteRun(...)appeared exactly once in main code, and so did
projection.queued(...). Re-implementing the publishfor
/fixwould have put two readings of "did the record land?" in the tree, which is the shapethat produced two credential scrubbers here whose rules diverged — and the weaker one ran in the
container holding the write token.
RunLaunchtakes the publish and its fault classification. It answers an outcome rather thanthrowing, because the REST caller turns each into a 503 with its own wording and a saga has no 503 to
say it in. The split follows that line exactly: what a failed dispatch writes on the row moved,
what it says to an HTTP caller stayed.
RunResourceTestis unchanged apart from two constantreferences that followed the members they name, and its behavioural assertions are the proof the
extraction changed nothing.
The rest turned out not to be shared logic —
MachineAccounts,LlmModelPricer,FactoryCloneUrls,RunCredentialsare already beans, and only the rendering of their failuresdiffers per caller. So no further extraction was invented.
The order of the refusals is a decision
An author gets one message. The duplicate check is first, ahead of everything: every gate below
it is a reason to refuse a new request, and a redelivery is not one — a repeat delivery told about
a spend cap reads as a lost request rather than a finished one, and the author says
/fixagain.After that, durable operator facts outrank transient ones, which is the argument
FixDispatchalready makes for its own caps.
Nothing is written and nothing is packed until every gate has passed. The credential is selected
last of the checks, because selecting stamps
last_used_at— a write, and a consumed rotationslot, which above the other checks happened for every request they then refused. The row is written
before the launch: a crash between them must leave a row with no command (recoverable, visible)
rather than a command with no row, which is a paid run neither cap counts.
The claim, and the gap it closes
T3's review deferred this here with the key already decided: the comment, not
(reviewId, findingRef). Those two are the cap's axes, and the cap is a different question — asecond genuine
/fixafter a failed run is exactly what it permits up to a bound.The gap is one the existing guard cannot see, and the test drives the sequence rather than
describing it. A run id is derived from the finding's thread plus
nextAttempt, andnextAttemptcounts the rows already written — so a redelivered command derives a higher attempt, a
different run id, and passes straight through
ON CONFLICT (run_id), which catches every otherduplicate. The numbering defeats the one mechanism that would have stopped it.
V56addscomment_idplus a unique index partial onkind='FIX'. The saga reads before it writesand that read produces the refusal; the index is the backstop for a race that should not be reachable
(
cs.integrationkeys by review id, so both deliveries land on one partition and one consumer, inorder). It is a constraint rather than only a query because if it does fire, a dead-lettered record
an operator sees is the right answer to a duplicate spend.
The prompt is the finding, and nothing a commenter typed
TargetFindingdeliberately carries no message — deciding whether a thread names an open findingdoes not need the finding's text.
specForis the second read, reached only once a run is about tobe paid for, and it throws on a decrypt fault where every neighbour falls back to legacy
plaintext. The neighbours are right for themselves: they render it, and old text beats nothing. Here
the value becomes a prompt that is paid for, so a fallback would spend money on ciphertext — and V36
created that table with both columns encrypted and shipped no backfill, so there is nothing to fall
back to.
The finding's own words arrive fenced, with the surrounding text saying the fenced part is a
report rather than orders. A finding message is model output derived from a diff a contributor wrote,
so a sentence addressed to an agent can travel from a pull request into a review comment into this
prompt. That fence is documented as the cheap half of the defence and no more: the load-bearing
half is that the agent holds no write credential, the publisher holds the only one, the push gate
judges paths, and ADR-040 bounds the branch.
harnessandmodelhave no request to come fromThe REST endpoint takes them from its body.
/fixtakes no arguments — letting a commenter choosethe model would let them choose the price. So they come from configuration, with no defaults: a
deployment that has not set both has not enabled
/fix, and the refusal names the missing key ratherthan picking one. That is the shape the spend cap already uses, where unset is a deliberate decision
rather than a crash. Both are in
.env.example, unlike the publisher variables an earlier roundcorrectly kept out of it — those are per-run container environment, not settings anyone sets.
Two things found while wiring, and one more the branch was carrying
ThreadRefis a bare record over aString, and every ingress builds one from Jackson'sasText(), which answers""for a nodethat is not there — so a blank one is reachable. It used to be refused four layers down by
findByThreadmatching nothing. It is now the key both caps count on and the subject of the runid, so it is refused where the reason is still legible.
refused:
ExecuteRunrejects a run whose branch equals its protected branch — correctly — andrejects by throwing, which on a Kafka consumer is a redelivery. No forge produces that row, which
is the argument for guarding it:
FixTargetsreads what the deployment last saw.passed against the constants, so widening either to 999 changed both sides of its own assertion and
stayed green. Reported by qa, confirmed, fixed.
Verification
testFastgreen ·:spire-orchestrator:test1127 tests across 125 suites, 0 failures.Mutations, each killing exactly its intended test: transposing the review and the finding on the row;
dropping the existing-branch mode; ignoring the row refusal; emptying the prompt; checking the spend
cap ahead of the claim; moving a claim on a re-arm; dropping the
kind='FIX'filter from the claimread; skipping the decryption; dropping the review filter from
specFor; and ignoringmayHaveLanded()in the launch — that last one also fails the REST test that predates theextraction, which is what makes the extraction safe.
T8–T11
T8 — the run↔review join
Nothing joined a run to a review.
factory_runhas carriedreview_idandfinding_refsince V54and no query read them beside anything, so neither the caps' evidence nor "what did this cost" could
be shown to a person — and there was no list endpoint at all.
GET /api/runsis viewer-and-admin, matching the detail endpoint. An unrecognised filter value isrefused rather than ignored: silently dropping a mistyped
status=faieldreturns every run, whichreads as "nothing is stuck" — the most dangerous possible answer to the question that page is opened
to ask.
Cost is a type, not a
long.RunCostmakes unknown unrepresentable as zero, which is ADR-023where this project has already been bitten:
SUMskips NULL, so a run with one unpriced line reportsthe priced remainder — a number that looks like a total, is smaller than the truth, and carries no
sign anything is missing. The join counts null lines precisely to catch that.
The roadmap's third T8 item — "a finding reference on
ReviewDetail.FindingView" — was alreadybuilt, as
threadRef, arriving via conversation linking. So the work was not a field but theassertion that the ref the dashboard shows is the ref the dispatch resolves, which nothing made.
Review round (1 lens, code-reviewer). No production defect. It found two tests of mine guarding
almost nothing, both failing silently:
FIND_BY_THREADisORDER BY id DESC LIMIT 1over a serial,so "the new id differs" is true by construction — delete the delete-then-insert entirely and it
still passed. It measured the sequence, not the replacement.
would have silently dropped a status spelled with a digit or capital — green about a status nobody
can filter for. Removing the filter made reflection sweep up the class's SQL constants, so the
reflection went instead: the test now reads
factory_run_status_closed, the CHECK the projectionactually writes against. It compared Java to Java; both halves could agree while a migration added
a tenth value.
Plus:
archived_atfiltered like every otherllm_chargeread;wasNull()read into a local (itwas correct only because it sat left of another
getLongin a short-circuit);RunCost.zero(),because
unknown()is an absorbing element and the obvious fold seeded with it answers unknownfor every input;
limitparsed from a String, since a failedIntegerconversion is mapped to 404by JAX-RS. Full dispositions:
.claude/reviews/global/m2-t8-run-review-joins.md.Eleven mutations killed. One earned its keep: widening the endpoint to
@PermitAllleft everycase green — including the anonymous one, because that 401 comes from the deployment's auth policy
before any annotation is read. Only an authenticated caller holding neither role discriminates.
T9 — the Runs screen
The factory had no UI at all. The design turns on one recorded trap: a status the UI's type system
cannot see.
RunStatuslists all nine values and every reader handles one absent from it — anunrecognised status is named, takes the warning pill, and is not claimed to be running either. Both
wrong answers are wrong differently: busy makes a finished run spin forever, ok makes a broken one
look clean. The test drives a value deliberately absent from the union.
Built from the vocabulary the stylesheet already has. The first draft invented five classes and
styles.contract.test.tsrefused them — that check doing its job, after four screens once shippedcompletely unstyled behind a green suite. The change adds no CSS at all.
Three mutations killed.
spire-ui483 tests / 61 files,tsc --noEmitsilent.T10 — the run worker in
deploy/Both packaged stacks, behind a compose profile, and that is a security decision rather than a
convenience. A Docker socket is root-equivalent on the host —
SECURITY.mdsays so under "What isNOT mitigated" — and the run worker is the one service that executes untrusted model output.
Verified both ways: 7 services by default, 8 with
--profile factory.DockerSocketMountsAreOptInTeststates the rule about the socket, not about this service:naming the service would guard today's file and miss the second one that ever needs a daemon. It also
asserts the service is present, because deleting it would otherwise satisfy the rule vacuously.
Kubernetes deliberately does not get the run worker. There is one runtime implementation and
WorkerRuntimessays so. A K8s deployment would mount the node's socket into a pod — preciselywhat
SECURITY.mdpromises the Kubernetes arm removes. Shipping that template would ship the exactthing that sentence says does not happen. The chart, overlays and rendered manifests are untouched.
T11 — the exit criterion, and what a container can and cannot prove
Adr040ExistingBranchTestruns the real thing: real containers, a real smart-HTTP remote, the realpublisher image, a shell script standing in for the model. A fix run pushes onto a branch that
already existed, the content is read back from the remote, and no
spire/branch is createdbeside it.
Two of my own tests were wrong first, and both are worth recording.
One asserted the publisher refuses a run whose branch equals its destination. It cannot —
ExecuteRun's constructor refuses that outright, so the command never exists.The other is the interesting one. The trunk case asserted the publisher's floor refuses
main.Deleting
looksLikeATrunkleft it green. A control probe — refusing every branch — reddened thepermitted-push cases, so mutations do reach the container and the survival was real. Measuring it
showed the run dies as
init container failed with exit 1before the publisher is consulted, becauseWorkspaceClonecallssetCreateBranch(true)and a clone has already materialised the defaultbranch locally. Two independent guards, the outer firing first — defence in depth working, and a
claim no container test establishes. The assertion stays; the javadoc now names which guard refused
it, and
docs/UNVERIFIED.mdcarries the gap.What T11 does NOT yet cover, stated rather than ticked: the reconciliation loop — finding →
/fix→ run → next round resolves the original finding — needs the run worker inside thespire-e2eGitLab stack. It is not incompose.e2e.yml, and putting it there means giving thatstack a Docker socket and a deterministic agent image. That is real work, not a formality, and it is
why the checklist item stays open.
T12 — the whole-PR round
Over
584d61c..HEAD: 14 commits, 57 files, +6023 lines. Three lenses of four. qa terminated ona session rate limit while preparing a probe, so its report does not exist. The build lane was run
directly instead —
testFast,testServices, the full:spire-orchestrator:test,spire-uivitest and
tsc --noEmit, all green — but that is coverage of the result, not of the questionqa was asked, which is whether the tests are the right ones. Its questions are open and are
recorded to open the next round, not quietly dropped.
One Critical, and it was on the arm with no user
/fixthrew an NPE out of the saga when the FACTORY account had no resolved login.MachineAccounts.resolvedid not guaranteebotUsername,ProviderRegistrystores a blank as SQLNULL, and it reached
MachineAccountCredential'srequireNonNull.RunResourcehad guarded exactly this, and said why in a comment. The/fixpath re-derivedthe same lookup and dropped the guard. That asymmetry is the finding worth keeping: on a REST arm a
throw is a 500 the caller reads, so the defect is loud and local. On a Kafka consumer it escapes
— the record is redelivered on every restart and the author who typed
/fixis told nothing at all.My own dispatcher test could not catch it either, because its credential fake returns a constant.
So the guard moved into
MachineAccounts.resolve, the one method that resolves the factory'spush identity. Two callers each remembering the same check is the shape this repository keeps paying
for.
RunResourcereads the registration back on the failure path, so it still names which of thetwo causes it was — an operator registers a missing account and re-saves a login-less one.
Three security findings, all on
/fixA comment id is the forge's, not the world's. The fix claim was keyed on a bare comment id,
which every ingress passes straight through from the forge. Two providers, or two self-hosted
GitLabs whose note ids both start at 1, produce the same value for unrelated comments — and unscoped
that refuses a legitimate
/fixwhile writing another workspace's run id into this review'sdurable history. In the race the index exists to backstop, it dead-letters after
pool.select()has already spent a rotation slot. V56 is unmerged, so it was amended rather than stacked:
(review_id, comment_id), plus a CHECK that a FIX row names the comment that asked for it.A username in the allowlist authorises a review, not a push. The shared author gate accepts a
handle or a stable id, which is right for a command whose blast radius is one paid model call.
/fixauthorises a commit pushed as the machine account, and a forge handle can be released andre-registered by somebody else — so an operator who listed
alicehas listed whoever holds thathandle next.
CLAUDE.mdstates the rule by name ("author identity is data (stableproviderUserId), never a gate"); it now holds where it matters most.The prompt fence was closable from inside it. A finding message is model output about a diff a
contributor wrote, so a planted sentence can reach the prompt. Writing inside the fence buys
nothing the surrounding text does not account for — writing the END marker closes it, and
everything after reads as the orchestrator's own voice, the one position not labelled as
contributor-derived. Both markers are neutered in any value now, and the three headers above the
fence are bounded to one line each.
Two broker outages retired a finding forever
Both caps counted rows whose dispatch was never acknowledged — runs that never executed and never
spent, and which the projection already treats as re-armable. With
MAX_PER_FINDING = 2, two blipsretired a finding permanently while telling its author it "has already had 2 fix run(s)" about two
runs that landed nowhere.
The filter names the cause, not the status.
AND status <> 'failed'is the obvioussimplification and it is wrong: it would also exclude every run that started, spent and then died,
which is exactly what FR-F32 exists to bound. My first version of the test survived that mutation
and was strengthened with a ran-then-failed row before it was believed.
The mutation harness produced three false survivals in one run
Worth more than any single finding. It restored with
git checkout, which reverts to HEAD — soit deleted the fix under test and left every later mutant uncompilable, and a compile failure looks
exactly like "no test failed" to a grep. Its
perlpatterns then used a bare\nagainst CRLFfiles, so three mutations never applied at all and were scored as survivals.
A mutant that does not compile, or does not apply, measures nothing. The harness now snapshots to
scratch rather than trusting git, asserts each mutation actually changed the file, and reports
INVALID rather than SURVIVED when a run did not execute tests. All nine mutations were then
re-run and killed.
Also fixed
factoryprofile started a run worker nothing could dispatch to.SPIRE_FACTORY_AGENT_IMAGE_CODEX,_FIX_HARNESSand_FIX_MODELreached neither stack andneither
.env.example, soPOST /api/runsrefused every request and/fixevery comment — witha healthy worker and an empty queue as the only symptom.
FixDispatch.Plannedcarried the raw provider type, so thedispatcher re-parsed it with a word-for-word identical refusal no input could reach and no test
covered.
Plannedcarries the parsedScmTypeand the copy is gone.techdebt/global/4-2-orphaned-javadoc-blocks…records. Two were the design record itself.RunLaunch.Outcome.isReArmable()had no production caller and its test asserted the predicateagreed with the type it was derived from. Removed.
spire-uireferenced no runstatus and that there was no list endpoint at all; T8 and T9 built both. The class-size entry
carried
~530/~450physical-line estimates; measured on its own preferred measure the files are350 and 404 code lines, and
FactoryRunProjectioncrossed 300 on this branch (+66%).different symptoms. Merged into one High entry.
Optional-as-control-flow pair, the nested ternary inRuns.tsx, and the method-sizeentry that exists "so the rule is not silently suspended for one package" and had not gained the
largest member of its own set.
Not fixed, and why
FactoryPullRequestBodyhas no production caller. It is the orchestrator half of T7; the stepthat runs after a fix run pushes is M3. Deleting delivered work to satisfy a reachability check is
the wrong trade — the class says so in its own javadoc instead.
turns an outage into something that reads as policy, and one arm behaving differently would give
this project two postures for one gate. The log line was the part actually missing.
run-workerholds the schema-owner DB role and the shared Tink keyset. Real, and adeployment-topology gap rather than a code change: it wants a least-privilege role and a keyset
split, which is M5's Kubernetes arm.
Verification
2877 Java tests across 323 suites, 0 failures, 1 skipped (
testFast+testServices);483
spire-uitests across 61 files;tsc --noEmitsilent. Nine mutations, each killingexactly its intended test.
Dispositions, including what qa should be asked first next round:
.claude/reviews/global/m2-t12-whole-pr.md.