Factory M3 — work items, labels and gates

[artyomsv]

For issue #114. Tracker tickets now enter bounded work-item journeys through explicit repository ownership, selected account roles, stable person identities, source-specific allowlists, policy ceilings and durable approvals. All seven acceptance criteria are independently verified; the acceptance record maps each to its proving slice and review.

GitHub Issues, GitLab Issues and both Jira ingestion paths share a parity contract whose adapter and test coverage inventories are derived from source. Unknown label attribution grants no authority. Humans register fetched specification and single-step plan references. For the same prepared task, suggest stops before BUILD, assisted requires PLAN approval before one build, and autonomous admits one build immediately. The UI proves phases, gates, decisions and run counts. Settings reuse shared table, form and empty-state styles, guarded by a contract derived from App routes.

Item execution reuses the selected FACTORY identity, harness pool, pricing, spend and wall bounds, and protected paths. Item-linked runs checkpoint without pushing and retain their workspace. A current delivery permit resumes only the publisher. Ambiguous PR creation recovers by reading both branches; REVIEW observes the existing review at the exact head. Dashboard decisions, explicitly bound tracker commands and supported current native PR approvals share ResolveGate. Ordinary approving prose, stale heads and dismissed reviews cannot approve.

Human takeover uses recorded stable IDs across rename and account rotation. It supersedes gates, invalidates unstarted effects and durably revokes publication authority before stopping compute. Actual killed-JVM tests prove that fresh permits and watchdog recovery cannot undo the hold, independently of M1 cancellation. Concurrent recovery records an already-observed PR once and keeps the item suspended. Operator resume requires verified identity, revision, note and fresh evidence; retired items cannot resume.

V72 explicitly drops only the unused scm_provider.workspace column after the read/INSERT/UPDATE guard passed. A fresh .handoff archive was validated before the dev migration. The populated migration test preserves all other fields, ciphertext/AAD, references, bindings and immutable legacy mapping evidence. Dev remains at 6 accounts / 38 reviews / 93 findings / 15 runs / 3 hooks. The accepted TEST PR #32 audit explains the delta from the original 6/37/85/14/3 baseline; excluding exactly those retained proof rows matches all five original counts. Both original encrypted comparisons pass: 9 credential/reference entries and 12 webhook entries unchanged. CLAUDE.md Status was rewritten, HISTORY appended, and implementation/runbook documentation reconciled.

Validation on 2026-09-14: forced fast tests, service tests and packaging passed sequentially on Java 25. 4076 Java tests across 452 suites and 30 modules, zero failures/errors and one existing Windows symlink skip. Full UI: 742 tests across 93 files, TypeScript and production build passed. Slice 10 records four distinct production mutations: the real migration and independent read/INSERT/UPDATE violations, each one selected assertion failure followed by exact scratch restoration and a passing rerun. Slice 9 retains 78 distinct production mutations. Final pinned Semgrep: zero findings and zero parser errors across five slice 10 code/migration files. Earlier ledgers remain linked without an inflated cross-slice total.

Production VERIFY and LAND remain unavailable. M4 owns the verifier; M3 does not ship one. No live item-build proof exists. The accepted standalone /fix re-proof on TEST PR #32, run 4003204361:1, automatically pushed and resolved the intended thread and persisted verdict while the six other prior findings stayed UNCHANGED. Its exact branch was deleted and PR closed; audit rows were retained. Local-origin item tests remain separate evidence.

The automated GitLab run-unit gap remains open: RunUnitSpec has no network field and cannot reach the e2e stack's GitLab; a live GitHub run does not close it. Both factory images remain absent from GHCR. Per-forge identity and permission limits remain separate UNVERIFIED entries. Native external answers and operator resume lack live proof; GitLab/Bitbucket native PR approvals and Jira Data Center comment polling remain unavailable. Remote publication already in progress cannot be recalled, and no atomic ordering with a human remote push is claimed. No dev run worker or new live canary was started for the final handoff.

Final slice evidence records backup chronology, dev migration/continuity and measured checks. Final mutation inventory records selectors and source hashes. PR remains draft for final operator review; no merge is authorized.

Round 18 operator setup fixes: Work sources and Work policy now open on lists with Add actions. Saved records appear immediately with confirmation. Missing repository choices distinguish an unselected account from an origin mismatch and link registration. Credential pickers reuse accountOptionLabel; field help, required/optional/automatic markers and the policy nav icon complete the usability corrections while preserving save locks. WIDGETS.md inventories the shared vocabulary and helpers. The account-option guard derives routes and rendered children; both structural guards now preserve quoted wildcard paths after a production table mutation exposed the old comment-parser gap.

Round 18 validation: 742 UI tests in 93 files, 72 shuffled setup/route tests, TypeScript/build, 28 isolated production mutations, and zero Semgrep findings/parser errors across 11 changed code/test files. Eight Chromium screenshots use intercepted TEST-only HTTP data; the dev UI was rebuilt and its source hashes verified. No backend data or accepted criterion changed. Evidence and mutation ledger. PR remains draft.

[artyomsv]

Round 1 review — plan accepted with changes, and the five open questions are settled

Verdict: the plan is sound and implementation may start on slice 1 once the six inline points are folded in. This is a genuinely good planning round. Three things it did that I want to name, because they are the reason I am not sending it back:

• It read the branch base and wrote down what is actually there (DomainEvent has no run member, ProviderRegistry still has a workspace-only fallback, PullRequestSink.NewPullRequest has no draft flag), instead of designing against the documentation.
• It refused to manufacture the three journeys. §6.3 says plainly that M3 cannot label no-op phase handlers "complete" to produce three green ones, and proposes a real boundary instead. That is the correct instinct and it is what the ticket's criterion 1 is worth.
• It separated proposed from established everywhere — ADR numbers are reserved, not claimed; test names are "tests to add"; the live M2 evidence is quoted as the issue's report, not as a measurement this round made.

The five questions in §11 — decided

1. Three journeys and phase ordering — accept the proposed boundary.
Implement the real phase state machine and the manual tracker-artifact handoff, and reuse M2 for one already-specified build task. Generating a specification and a multi-step plan is named M4 work in docs/factory/ROADMAP.md; moving it into M3 would make this milestone unshippable and would be a change to the roadmap, not to this plan. The three journeys are proved at the plan/build boundary: suggest stops, assisted waits on a durable gate, autonomous builds. Missing verification shows awaiting_input or capability_unavailable — never a fake successful phase.

On ordering: accept recording PR opening as the delivery effect and then observing the existing reviewer. The published eight-phase diagram has review before deliver, and the existing reviewer needs a pushed pull request, so the diagram is what is wrong. Record the corrected order in ADR-045 and fix the diagram in the same slice — do not leave the mismatch to be re-derived.

2. Profile ordering — accept operator-declared precedence plus a component-wise meet.
"Lowest label wins" is undefined for a vector, and you are right that suggest and assisted already cross at the plan phase. A component-wise meet can never grant more authority than any single applied label grants, which is the property ADR-033 was reaching for. The operator-declared precedence handles display and clamp messages. Record both in ADR-045, and state the invariant in the ADR in one sentence a reader can check: the effective vector is never above any applied label in any component.

3. Identity and permission portability — accept the capability errors, and narrow the wording.
Bitbucket's effective-permission read needing an admin caller, and handles not being a universal Jira/Bitbucket primitive, are properties of those products. Do not guess and do not raise a token's authority to paper over it. Show an explicit capability error, offer disambiguated selection where the forge gives us enough to disambiguate, and write the credential prerequisite into the operator-facing text.

One addition: record each of these as its own entry in docs/UNVERIFIED.md at the slice that introduces it, with what was measured and against which forge. Per-forge identity behaviour is exactly the class of claim this project has shipped wrong before.

4. Repository registration compatibility — confirmed, with org auto-enrollment ending at cutover. See the inline comment on §3.2 step 3. One source/ISSUE hook per repository; REVIEWER / FACTORY / ISSUE as the event kinds; org auto-enrollment survives the bridge only, and an event for an unregistered repository must raise an attention row rather than be dropped.

5. Drafts and takeover — accept both.
Provider-native drafts where available, and a visible refusal where not. Never substitute a title prefix for a draft — a pull request that says "[DRAFT]" and is mergeable is worse than one that honestly says the forge cannot do it. Command and gate answers take precedence over generic human-comment takeover, and that precedence goes in ADR-045 with the FR-F22 / FR-F25 conflict named, so the next reader knows it was resolved rather than overlooked.

Six changes required before slice 1 closes

#	Severity	Point
1	HIGH	Keep scm_provider.workspace until slice 10 — the bridge's only rollback evidence
2	HIGH	Name the dev-database backup as a step in slice 1, and prove real-row decryption after cutover
3	HIGH	Split the publication hold into slice 8b, and re-prove the standalone /fix loop live
4	—	§11.4 decided: org auto-enrollment ends at cutover, with an attention row, never a silent drop
5	MEDIUM	Slice 8 splits into 8a / 8b; its §11.1 resolved dependency is now discharged
6	LOW	Reconcile CLAUDE.md and UNVERIFIED.md in slice 1, not slice 10 — they currently assert something false

One thing I checked and am satisfied with

The aggregate decision in §2.1 — a work-item aggregate exists, a run aggregate does not — is the right call, and the reasoning is the part that matters: a second aggregate for every run would duplicate the delivered run lifecycle without solving a new M3 invariant. Keep the refusal to backfill synthetic RunStarted events. That refusal is load-bearing and it is the kind of thing a later round is tempted to undo for tidiness.

Next

Fold in the six points, push, and tell me. Then start slice 1. I will review each slice as its own round.

[artyomsv]

Round 2 review — planning round accepted, one point stays open

I checked all six required changes against ded7404 rather than against the commit message. Five are done and I have resolved those five threads. One is half done and stays open.

#	Point	Verdict
1	Keep scm_provider.workspace until slice 10	✅ design §3.2, plan slices 2 and 10 — and you added the guard I did not ask for: production SQL is inspected for SELECT-star mappings so the retained column cannot silently come back into service
2	Dev-database backup named as a step	⚠️ open — see the thread; the command is pinned to one session's scratchpad
3	Publication hold split out	✅ slice 8b, with the standalone /fix regression re-proved live on artyomsv/spire-test
4	Org auto-enrollment ends at cutover	✅ with the attention row naming repository, forge origin and incoming registration
5	Slice 8 → 8a / 8b	✅ 9 and 10 renumbered cleanly, §11.1 resolved dependency discharged
6	Reconcile CLAUDE.md and UNVERIFIED.md in slice 1	✅ and the automated GitLab gap correctly kept as its own separate entry

Three things beyond the list that I want on the record, because they are the difference between a plan that survives contact and one that does not:

• The eight-phase diagram is now called wrong, out loud. docs/factory/AUTONOMY.md gets fixed in slice 8a, and the line "the diagram is wrong; the implemented reviewer is not changed to fit it" is the correct direction of travel. A plan that had quietly reordered the reviewer to match a picture would have been much harder to unpick later.
• The ADR-045 invariant is stated in a form a reader can check — the effective vector is never above any applied label in any component — with "applied" defined rather than assumed.
• never a title prefix survived into §11.5. A pull request labelled [DRAFT] that is still mergeable is a worse outcome than an honest refusal, and that is now written where the implementer will hit it.

What is left

Only the backup path. Fix it, reply on the thread, and I will resolve it. Do not take a second dump — the pre-M3 one already exists and is verified; slice 1 needs the repeatable command, not another artefact.

Slice 1 is approved to proceed in parallel with that fix. Its review is its own round.

[artyomsv]

Round 3 — slice 1 review

Verdict: one HIGH defect to fix, then slice 1 is done. Everything else holds up. I checked the claims against the code rather than against the commit message, and they stand: every test class named in the mutation table exists, the working tree is clean, V60 really is expand-only, and the backup step really is repeatable now.

I have resolved the round 2 backup thread — .handoff/ with (Get-Location).Path, no session GUID, and you correctly did not take a second dump.

The defect: a blank forgeOrigin is a poison pill on the new topic

Not a style point. Here is the whole chain, each link in this diff:

1. V3__repository_snapshot_outbox.sql:2 adds webhook_repo.forge_origin TEXT — nullable, no CHECK (forge_origin <> '').
2. The snapshot trigger builds the payload straight from row_data.forge_origin, so '' goes on the wire as "forgeOrigin": "".
3. RepositoryRegistration's compact constructor validates registrationId, revision, providerType, scope and target — and not forgeOrigin. "" is accepted.
4. RepositoryMigrationBridge:72 asks only snapshot.forgeOrigin() == null, so "" passes the "origin unknown" arm.
5. :73 calls ForgeOrigin.of(""), which throws IllegalArgumentException("Forge origin is required").
6. apply() is @Transactional and catches only SQLException. The unchecked exception escapes, the transaction rolls back, and nothing is written to repository_registration_bridge.problem.
7. The snapshot is never marked processed, so it is redelivered. It throws again.

That is a stalled consumer on cs.registry-integration, cleared only by rpk group seek — the exact failure CLAUDE.md records under the Kafka ack budget gotcha. And it contradicts your own design rule: "Missing/conflicting legacy origins become pending mappings and attention rows with explicit repair." A blank origin is a missing origin. It must become registration_origin_unknown, not a crash loop.

I am not claiming a writer emits '' today. The point is that three layers each look like the guard and none of them is one, and the plan itself says "a new consumer can read legacy registrations during the bridge" — legacy payloads are precisely where an empty string turns up instead of a null.

Fix at two layers, because the wire record is the contract:

• RepositoryRegistration: reject blank while still allowing null. forgeOrigin being absent is a real, supported state; being present-and-empty is not.
• RepositoryMigrationBridge:72: use a blank-safe test so the bridge never depends on the wire guard alone.

Then add a CHECK (forge_origin IS NULL OR forge_origin <> '') to webhook_repo.forge_origin so the gateway cannot store what the consumer cannot process.

Mutation, and make it discriminating: a test that feeds forgeOrigin = "" through the real consumer path and asserts the bridge row records registration_origin_unknown and the snapshot is marked processed. Then remove the blank check and confirm that one test fails — and confirm it fails on the assertion, not on an IllegalArgumentException escaping the consumer, which would pass for the wrong reason.

Second point, low, same root cause

V60:6-8 gives workspace and slug a CHECK (<> '') and gives forge_origin none, in the same CREATE TABLE. All three are components of the UNIQUE key. A reader cannot tell whether the omission is deliberate, and a blank origin collapses two hosts into one row — the exact ambiguity §3.1 exists to prevent. Make it consistent.

This repo has already paid for this shape once: FixRuns's javadoc records a constraint that had to be tightened because "blank ids turned out to slip through it as well."

What is good, and worth keeping

• The mutation table is the strongest I have seen in this repo. 40 checks, each naming a production line and exactly one test, restored from a scratch snapshot and not from git. binding_origin → rejectsCrossOriginBindingWithoutLeavingRepository is the kind of discriminating case that catches a real regression rather than confirming an obvious one.
• repository_legacy_account has deliberately no FK, with the reason written in the migration: evidence must survive re-assignment. That is the right instinct and the comment means the next person will not "fix" it.
• You found and fixed two pre-existing flaky tests (RunAgentStartedTest, ReviewRetryScheduleIT) by giving them deterministic clocks, and said plainly that no production timing changed. That is work nobody asked for and it makes every later slice cheaper.
• The credential probe compared 9 real entries, and you proved the comparison itself bites by bypassing it and watching exactly one assertion fail. Proving the proof is the part that usually gets skipped.
• You recorded honestly that the dev stack has not been rebuilt for slice 1, so no post-upgrade claim is made.

Fix the blank-origin chain, push, reply, and slice 1 is closed. Slice 2 may start on top of the fix.

[artyomsv]

Round 4 — blank-origin fix accepted; CI is red on something else

The blank-origin fix is correct, and better than what I asked for. All four of my threads are resolved. One new blocker, which is not yours in origin but is yours to clear.

The fix, checked rather than taken on trust

I verified the claim that mattered: that normalisation happens before strict decoding. If it had happened after, the poison pill would simply have moved from the bridge to the decoder, and the commit message would still have read the same.

applyLegacyPayload reads the raw tree, nulls a blank forgeOrigin, and only then calls treeToValue. So the wire record stays strict for producers while the consumer tolerates legacy payloads. That is the right split, and the comment on it — "Legacy blank origins mean unknown; new wire records still reject present-but-blank values" — says exactly why.

Two things you did beyond the ask:

• You moved the origin check to the top of reconcile. I had suggested fixing it in place. In place, a blank origin would first run the legacy-account lookup and could return legacy_account_missing or conflicting_forge_origins — a wrong reason recorded against a row whose real problem is a missing origin. First is the correct position, and an operator reading the repair queue now gets told the truth.
• The test asserts the offset advanced, not just the mapping. That is the half that distinguishes "handled" from "threw and happened to look handled", and it is the half that usually gets left out.

New blocker: Semgrep is failing the build

Semgrep OSS and scan are both red on RepositoryHistoryBridge.java:82:

connection.prepareStatement("UPDATE " + table + " SET repository_id=" + …)

I checked whether it is exploitable, and it is not. link is private, it is called exactly twice — with the literals "review_status" and "factory_run" — and every value is bound with setObject / setString. No registry, webhook or operator input reaches table. Your comment on the line says so and is accurate.

That is not the end of it, because the finding still fails the build, and a scanner that is red for a known-benign reason is a scanner people stop reading. Remove the pattern rather than suppressing the warning:

private static final String LINK_REVIEW_STATUS = "UPDATE review_status SET repository_id=…";
private static final String LINK_FACTORY_RUN   = "UPDATE factory_run SET repository_id=…";

and pass the constant instead of the table name. There is no concatenation left to flag, the two-literals guarantee becomes structural instead of a comment, and nobody has to re-derive that argument at the next scan. A nosem suppression would also turn the check green, and it is the worse answer: it keeps the shape and asks every future reader to re-verify the callers.

Please also confirm no other Semgrep finding is hiding behind this one — the scan job failing separately may just be the same rule.

Still running

java-kotlin, service tests + packaging and kind install were pending when I looked. fast tests, dashboard, javascript-typescript, gitleaks and invariants and drift are green.

Clear Semgrep, get the remaining checks green, and slice 1 is closed. Slice 2 can start on top of it — do not wait for me to say so again.

[artyomsv]

M3 acceptance tracker — maintained by the reviewer

This is the audit trail for issue #114's seven exit criteria. A criterion moves to verified only when its named test exists, passes, and its mutation has been observed to fail exactly one test and pass again after restoration. A green commit message is not evidence; I check the code.

Updated after round 4, head a956532.

The seven criteria

#	Criterion	Proving slice	State
1	Three visibly different journeys	8a	⬜ not started
2	Above-ceiling label clamped and says so	7	⬜ not started
3	Unlisted and unattributable appliers ignored	5	⬜ not started
4	Lowering the ceiling stops an in-flight item at its next phase	7	⬜ not started
5	Push access without a list; refusal without access; overrides both ways	4	⬜ not started
6	Type a handle, store an id, render the handle; unresolved refused	3	⬜ not started
7	Repository shows workspace / accounts / one hook per kind; accounts have no workspace	2	🔄 slice 1 laid the schema; slice 2 proves it

Slice progress

Slice	State	Evidence
0 — design and plan	✅ accepted	rounds 1–2; 6 required changes folded in
1 — repository registration and migration bridge	🔄 closing	9e797f7 + a956532; 64 files; 40 mutations recorded; one HIGH defect found and fixed; Semgrep blocker open
2 — repository cutover and per-kind webhooks	🔄 started	proves criterion 7
3 — resolve people, bidirectional overrides	⬜	proves criterion 6
4 — authorize /fix by push permission	⬜	proves criterion 5
5 — first ticket, durable intake, suggest policy	⬜	proves criterion 3
6 — GitLab/Jira sources and recovery	⬜
7 — full policy and dashboard approvals	⬜	proves criteria 2 and 4
8a — prepared task to policy-controlled build	⬜	proves criterion 1
8b — publication hold and draft delivery	⬜	must re-prove the standalone /fix loop live
9 — external answers and human takeover	⬜
10 — integrated evidence and release docs	⬜	final measured status

Defects found in review so far

Round	Severity	Finding	State
1	HIGH	Bridge drops its rollback evidence — workspace column removed before slices 3–9 could need it	✅ fixed
1	HIGH	Dev-database backup was a caution, not a step	✅ fixed, backup taken and verified
1	HIGH	Publication hold folded into a slice where an M2 regression would be invisible	✅ split to 8b with a live re-proof
2	HIGH	Backup command pinned to one session's scratchpad — the artefact evaporates	✅ fixed to .handoff/
3	HIGH	Blank forgeOrigin is a poison pill on cs.registry-integration	✅ fixed at three layers
3	LOW	V60 gives workspace and slug a non-empty CHECK, forge_origin none	✅ fixed
4	BLOCKER	Semgrep fails the build on a formatted SQL string (verified not exploitable)	⬜ open

Standing requirements I will check at slice 10

• No AI or agent authoring anywhere in commit messages or PR text.
• No synthetic rows in the running dev database; TEST- prefixes only, with cleanup.
• Every guard mutation-verified: exactly one test fails, restored from a scratch snapshot rather than git.
• CLAUDE.md and docs/UNVERIFIED.md tell the truth about what was measured, with the automated GitLab run-unit gap kept as its own open entry.
• scm_provider.workspace dropped only in slice 10, not earlier.

[artyomsv]

Note: GitHub's pull-request review API is returning 502 for this repository right now, so round 5 is delivered as a comment. The content is unchanged; I will re-file it as a formal review when the endpoint recovers.

Round 5 — slice 2 reviewed. Criterion 7 is verified. First of the seven.

I did not take the commit message's word for any of this. What follows is what I checked myself.

Criterion 7 — verified

A repository screen names its workspace, the accounts that may act on it, and one webhook per event kind — with no workspace field left on the account form.

Check	How I verified it	Result
All four named tests exist	listed them in the tree at 716dc75	✅ exact names from the plan
The UI half actually passes	ran it myself: npx vitest run RepositoryDetail.test.tsx SettingsProviders.form.test.tsx	✅ 2 files, 25 tests passed, 9.05s
The test is not vacuous	read the assertions	✅ see below
Mutations are real	110 rows with file:line, exact before→after, exact selector, snapshot hashes	✅ incl. binding_filter and kind_unique

The assertion is genuinely discriminating, which is the part that usually is not. It asserts toHaveLength(1) per kind for all three of REVIEWER / FACTORY / ISSUE — "exactly one", not "at least one" — each with its own distinct key; it asserts a disabled factory account renders as disabled rather than vanishing; and it uses a nested GitLab namespace, TEST-group/nested, which is the case host disambiguation exists for. Hiding any one of the three regions fails it, so the three separate UI mutants are real rather than three names for one check.

The live cutover preserved your data exactly

You ran V61 and gateway V4 against the real dev database. I checked it against the baseline I took before any of this started:

Table	Baseline	After cutover
orchestrator.scm_provider	6	6
orchestrator.review_status	37	37
orchestrator.review_finding	85	85
orchestrator.factory_run	14	14
gateway.webhook_repo	3	3

Both migrations recorded success = t. Six repositories were bootstrapped with eight role bindings, across four distinct forge origins — api.github.com, gitlab.com, git.epam.com and gitbud.epam.com. Two self-managed GitLabs that the old workspace-only key could not have told apart are now separate rows. That is the whole point of the milestone, working on real data.

My round 1 requirement is honoured

scm_provider.workspace is still there, now nullable, all 6 rows still populated, 0 blanks. The old UNIQUE (type, workspace, role) and the workspace-by-role CHECK are gone. The rollback evidence survives, which was the condition for letting the cutover happen at all.

One finding — MEDIUM, and it is a slice 10 landmine

AccountWorkspaceIsUnusedTest is a good guard and I am glad it exists. Its javadoc states its scope exactly: "The populated rollback column must never silently become a runtime input again." Input. It matches SELECT … FROM scm_provider, alias .workspace, SELECT * mappings, String workspace on the account model types, getString("workspace") in the registry mapper, and resolveByWorkspace(.

It has no INSERT or UPDATE arm. And there is exactly one production statement left that names the column — ContextCredentialReconciler:74, an INSERT — which is precisely the shape the guard cannot see.

It is harmless today: it writes NULL for a CONTEXT row, which never had a workspace. Nothing is contaminated; I checked that before writing this, because a write into the evidence column would have been far worse.

The problem is slice 10. That SQL is a string. When slice 10 drops the column, nothing fails to compile — it fails at runtime, inside the startup reconciler, on every boot, discovered after deploy. Details inline.

CI

javascript-typescript (CodeQL) is failing; its run was still in progress so the log was not available yet. java-kotlin, scan, kind install and service tests + packaging are still pending. fast tests, dashboard, gitleaks, invariants and drift and detect manifest changes are green.

Chase the CodeQL failure — I could not read the log. If it is a false positive, remove the pattern rather than suppressing it, as you did for Semgrep.

Tracker

Criterion 7 moves to verified. Six remain. Slice 3 is next and proves criterion 6 — handle in, id stored, handle rendered, unresolved refused.

Fix the INSERT and the guard, get CI green, then start slice 3 without waiting for me.

---

The INSERT fix, in full

I could not anchor this inline — the only changed hunk in that file is around line 99, and the statement is at line 74. So it is here instead.

spire-orchestrator/src/main/java/dev/codespire/orchestrator/context/ContextCredentialReconciler.java:74

INSERT INTO scm_provider (id, name, type, base_url, workspace, role,
    auth_kind, auth_username, auth_secret)
VALUES (?, ?, ?, ?, NULL, 'CONTEXT', ?, ?, ?)

Drop workspace from the column list and its NULL from VALUES. The column is nullable, so an omitted column already yields NULL — identical behaviour, and the last reference is gone:

INSERT INTO scm_provider (id, name, type, base_url, role,
    auth_kind, auth_username, auth_secret)
VALUES (?, ?, ?, ?, 'CONTEXT', ?, ?, ?)

Renumber the bind indexes; auth_kind moves from 5 to 5 only after the NULL placeholder is removed, so check each one rather than assuming.

---

The guard fix, in full

spire-arch/src/test/java/dev/codespire/arch/AccountWorkspaceIsUnusedTest.java:25

MEDIUM — extend this to writes, so "unused" means unused rather than unread.

This guard is good and I am glad it exists — the alias arm and the SELECT * arm are both the kind of thing that gets missed. Its javadoc scopes it honestly to "runtime input", and for reads it is thorough.

But the class is called AccountWorkspaceIsUnusedTest, and one production INSERT still names the column (ContextCredentialReconciler:74). A reader sees a green guard with that name and concludes the column has no production references at all. It has one, and it is the one that breaks at slice 10's DROP COLUMN — at runtime, because the SQL is a string.

Add an arm matching INSERT INTO (?:orchestrator.)?scm_providers*( and UPDATE (?:orchestrator.)?scm_provider� … SET whose column list or assignments mention workspace.

Then mutation-verify the new arm properly. Re-adding workspace to the reconciler's INSERT must make this test fail — if it still passes, the new arm is decoration. That is the discriminating check, not merely that the guard is green once the reconciler is fixed.

[artyomsv]

Round 6 — slice 3. Criterion 6 is verified. Two of seven.

Both round 5 fixes landed and I confirmed them in the code, not the commit message: the reconciler's INSERT no longer names workspace, and AccountWorkspaceIsUnusedTest now has INSERT and UPDATE arms.

Criterion 6 — verified

The allowlist form accepts @handle, stores the provider user id, and renders the handle back; a handle that cannot be resolved is refused at entry rather than stored as text.

Check	Result
Four named tests exist	✅ exact names from the plan
UI half passes — I ran it	✅ ActorPicker.test.tsx, 5 tests passed
Java test is not vacuous	✅ see below
Mutations hit production, not fixtures	✅ 91 mutations, zero pointing at a test or fixture file

handleEntryStoresStableIdAndReturnsHandle is the strongest single test in this PR so far. It resolves @TEST-person, saves, then reads the actual database row and asserts author = "900123" — the id, not the handle — with observed_handle = "TEST-person" beside it, and assertFalse(rs.next()) so a duplicate row would fail. Then it rehydrates in a fresh JVM, twice, with the comment "A second fresh JVM rehydrates after the first process exits". That last part I did not ask for, and it is what separates real persistence from a warm cache.

The discriminating mutation is exactly right: ps.setString(2, actor.providerUserId()) → ps.setString(2, actor.handle()). If the code stored the handle, that test dies.

I said I would check the trap your own plan warned about — a mutant that changes the mocked response instead of production code. All 91 mutations target src/main/java. None touches a fixture.

The per-forge identity work is better than required

Three dated UNVERIFIED entries, one per forge, with links to the actual API docs — and GitHub was measured live: "api.github.com also refreshed its existing actor 3218389 by ID and resolved @artyomsv back." That is a real API against the real account, not a stub. Bitbucket's entry records the workspace-read prerequisite, the ten-page limit, and states plainly that no live token was given more authority, which was the §11.3 condition.

identityRedirectCannotCrossTheOrigin is a guard nobody asked for and it is the right instinct — an identity lookup that can be redirected across origins is a way to be told someone else's answer.

Finding — MEDIUM. Storage is id-only; the matcher is not.

IntegrationSaga.authorAllowed (line 826) was not changed by this slice, only its call site moved. It still reads:

return allowlist.stream().anyMatch(a ->
        a.equalsIgnoreCase(author.providerUserId()) || a.equalsIgnoreCase(author.username()));

Slice 3 makes every stored entry a stable id. The matcher still accepts a handle. So the guarantee holds at the form and is given away at the gate, and the operator's reason for id-matching — "a handle can change hands" — is only half-enforced.

Two concrete consequences:

1. A numeric username matches an id entry. Your live allowlist row is author = 3218389, observed_handle = artyomsv. GitHub usernames may be numeric, so a person who registers the username 3218389 matches the second arm of that predicate and gets their pull requests reviewed on your model spend.
2. Any legacy entry stored as text keeps working, which is the compatibility that quietly preserves the handle-changes-hands hole.

Scope, stated precisely so this is not over-sold: this gate decides which pull requests get reviewed, not who may run /fix. The cost is model spend on an unintended author's pull request, not a push. That is why this is MEDIUM and not HIGH.

I checked your live data before writing this — one row, and it is a real id, so dropping the username arm breaks nothing today. Do it in slice 4, where /fix authorization is being built on adjacent code, before that hardens around the weaker rule. Mutation: restore the username arm and prove a test fails that feeds an actor whose username equals another actor's stored id.

CI

8 pass, 3 pending, 0 fail on 41d3909.

Tracker

Criteria 7 and 6 verified, five remain. Slice 4 next: criterion 5, the /fix push-permission rule — the one that refused your own command on 2026-09-10.

[artyomsv]

Re-filed as a formal review now that GitHub's review API has recovered. Originally delivered as this comment when the endpoint was returning 502; content unchanged.

Round 5 — slice 2 reviewed. Criterion 7 is verified. First of the seven.

I did not take the commit message's word for any of this. What follows is what I checked myself.

Criterion 7 — verified

A repository screen names its workspace, the accounts that may act on it, and one webhook per event kind — with no workspace field left on the account form.

Check	How I verified it	Result
All four named tests exist	listed them in the tree at 716dc75	✅ exact names from the plan
The UI half actually passes	ran it myself: npx vitest run RepositoryDetail.test.tsx SettingsProviders.form.test.tsx	✅ 2 files, 25 tests passed, 9.05s
The test is not vacuous	read the assertions	✅ see below
Mutations are real	110 rows with file:line, exact before→after, exact selector, snapshot hashes	✅ incl. binding_filter and kind_unique

The assertion is genuinely discriminating, which is the part that usually is not. It asserts toHaveLength(1) per kind for all three of REVIEWER / FACTORY / ISSUE — "exactly one", not "at least one" — each with its own distinct key; it asserts a disabled factory account renders as disabled rather than vanishing; and it uses a nested GitLab namespace, TEST-group/nested, which is the case host disambiguation exists for. Hiding any one of the three regions fails it, so the three separate UI mutants are real rather than three names for one check.

The live cutover preserved your data exactly

You ran V61 and gateway V4 against the real dev database. I checked it against the baseline I took before any of this started:

Table	Baseline	After cutover
orchestrator.scm_provider	6	6
orchestrator.review_status	37	37
orchestrator.review_finding	85	85
orchestrator.factory_run	14	14
gateway.webhook_repo	3	3

Both migrations recorded success = t. Six repositories were bootstrapped with eight role bindings, across four distinct forge origins — api.github.com, gitlab.com, git.epam.com and gitbud.epam.com. Two self-managed GitLabs that the old workspace-only key could not have told apart are now separate rows. That is the whole point of the milestone, working on real data.

My round 1 requirement is honoured

scm_provider.workspace is still there, now nullable, all 6 rows still populated, 0 blanks. The old UNIQUE (type, workspace, role) and the workspace-by-role CHECK are gone. The rollback evidence survives, which was the condition for letting the cutover happen at all.

One finding — MEDIUM, and it is a slice 10 landmine

AccountWorkspaceIsUnusedTest is a good guard and I am glad it exists. Its javadoc states its scope exactly: "The populated rollback column must never silently become a runtime input again." Input. It matches SELECT … FROM scm_provider, alias .workspace, SELECT * mappings, String workspace on the account model types, getString("workspace") in the registry mapper, and resolveByWorkspace(.

It has no INSERT or UPDATE arm. And there is exactly one production statement left that names the column — ContextCredentialReconciler:74, an INSERT — which is precisely the shape the guard cannot see.

It is harmless today: it writes NULL for a CONTEXT row, which never had a workspace. Nothing is contaminated; I checked that before writing this, because a write into the evidence column would have been far worse.

The problem is slice 10. That SQL is a string. When slice 10 drops the column, nothing fails to compile — it fails at runtime, inside the startup reconciler, on every boot, discovered after deploy. Details inline.

CI

javascript-typescript (CodeQL) is failing; its run was still in progress so the log was not available yet. java-kotlin, scan, kind install and service tests + packaging are still pending. fast tests, dashboard, gitleaks, invariants and drift and detect manifest changes are green.

Chase the CodeQL failure — I could not read the log. If it is a false positive, remove the pattern rather than suppressing it, as you did for Semgrep.

Tracker

Criterion 7 moves to verified. Six remain. Slice 3 is next and proves criterion 6 — handle in, id stored, handle rendered, unresolved refused.

Fix the INSERT and the guard, get CI green, then start slice 3 without waiting for me.

---

The INSERT fix, in full

I could not anchor this inline — the only changed hunk in that file is around line 99, and the statement is at line 74. So it is here instead.

spire-orchestrator/src/main/java/dev/codespire/orchestrator/context/ContextCredentialReconciler.java:74

INSERT INTO scm_provider (id, name, type, base_url, workspace, role,
    auth_kind, auth_username, auth_secret)
VALUES (?, ?, ?, ?, NULL, 'CONTEXT', ?, ?, ?)

Drop workspace from the column list and its NULL from VALUES. The column is nullable, so an omitted column already yields NULL — identical behaviour, and the last reference is gone:

INSERT INTO scm_provider (id, name, type, base_url, role,
    auth_kind, auth_username, auth_secret)
VALUES (?, ?, ?, ?, 'CONTEXT', ?, ?, ?)

Renumber the bind indexes; auth_kind moves from 5 to 5 only after the NULL placeholder is removed, so check each one rather than assuming.

[artyomsv]

MEDIUM — extend this to writes, so "unused" means unused rather than unread.

This guard is good and I am glad it exists — the alias arm and the SELECT * arm are both the kind of thing that gets missed. Its javadoc scopes it honestly to "runtime input", and for reads it is thorough.

But the class is called AccountWorkspaceIsUnusedTest, and one production INSERT still names the column (ContextCredentialReconciler:74). A reader sees a green guard with that name and concludes the column has no production references at all. It has one, and it is the one that breaks at slice 10's DROP COLUMN — at runtime, because the SQL is a string.

Add an arm matching INSERT INTO (?:orchestrator.)?scm_providers*( and UPDATE (?:orchestrator.)?scm_provider� … SET whose column list or assignments mention workspace.

Then mutation-verify the new arm properly. Re-adding workspace to the reconciler's INSERT must make this test fail — if it still passes, the new arm is decoration. That is the discriminating check, not merely that the guard is green once the reconciler is fixed.

[artyomsv]

Round 6's numeric-username finding is fixed in 0af7062. IntegrationSaga.authorAllowed now compares only the stable provider ID.

IntegrationSagaPolicyTest.numericUsernameCannotImpersonateAnotherActorsStoredId supplies stored ID 3218389, actor ID 900123, and username 3218389, then asserts no review dispatch. Restoring the production username arm causes exactly one selected assertion failure; scratch restoration passes. This was the first slice 4 correction.

Slice 4 also implements DENY → ALLOW → fresh effective push permission for /fix, using only the selected reviewer credential. All six named FixPermissionSagaTest cases pass through normalized repository-aware saga ingress and assert dispatch count plus the exact reason. Unknown permission refuses. The existing target, finding, spending and both fix-cap checks remain in force.

Validation: forced testFast, then testServices, then packaging, sequentially with JDK 25 and --no-parallel. Fresh reports contain 3,186 Java tests across 372 suites, zero failures/errors and one existing Windows symlink skip. The Docker runtime's 27 cases passed. All 64 production mutations failed once by assertion or HTTP verification and passed after byte-identical scratch restoration. Pinned Semgrep reports zero findings; its 24 parser diagnostics are unchanged from slice 3, with none in the new Java.

Exact selectors, mutations and hashes are in .claude/reviews/global/factory-m3-slice4.md. The three forge-specific live permission limits remain explicit in docs/UNVERIFIED.md; no credential authority was raised and no live run worker was started. Slice 5 is starting on top of this commit.

[artyomsv]

Round 7 — slice 4. Criterion 5 is verified. Three of seven.

This is the slice that fixes the thing that refused your own /fix on 2026-09-10, and it does it the way the decision said: on write access, not authorship.

Criterion 5 — verified

A commenter with push access can /fix without appearing on any list, and a commenter without push access is refused — with the hand-kept list proved to override in both directions.

Check	Result
Six saga cases exist, in FixPermissionSagaTest	✅ all six, exact names
Per-forge permission sources	✅ all three: GitHub, GitLab, Bitbucket
Six mutants, one per decision arm	✅ plus two extras I did not ask for
CI	✅ 8 pass, 0 fail

FixAuthorization.decide is a pure static function with an exhaustive switch, and the order is exactly ADR-044: no stable identity → refuse; DENY → refuse; ALLOW → permit; then the measured permission. Because the switch is exhaustive over the enum, a new permission state becomes a compile error rather than a silent default. That is the right shape for a security decision.

Fail-closed is real, not decorative. Every route to uncertainty ends in refusal: a null or blank actor id, a 20-second timeout, an interrupt, an ExecutionException, any SQLException or RuntimeException, and UNKNOWN from the forge. measure explicitly documents "No retry or stale-positive cache" — so a revoked permission cannot be served from memory. The extra mutants on the blank-actor arm and on the id-only match are both cases that would have passed a lazier test.

Round 6's finding is fixed, and fixed for the right reason

authorAllowed now reads:

return allowlist.stream().anyMatch(a -> a.equalsIgnoreCase(author.providerUserId()));

The handle arm is gone, and the commit names the discriminating case I asked for: "so a numeric username cannot impersonate another actor." That is the case that actually bites, not the obvious one.

Finding — MEDIUM. A 20-second network call inside a held transaction.

FixPermissionService.authorize is @Transactional. It opens a connection, takes FOR UPDATE OF r,p on the repository and scm_provider rows, and then — still holding both — calls measure(...), which blocks the calling thread for up to 20 seconds on an HTTP request to GitHub, GitLab or Bitbucket.

The intent is stated in the comment and it is a good intent: "An account can have its token rotated or be rebound, but that change cannot split one permission decision." I am not disputing the guarantee. I am disputing the cost, which is not written down anywhere:

• Operator edits block. While one /fix is being authorised, saving that repository or that account waits behind the row lock — for up to 20 seconds, against a forge nobody controls.
• Commands on one repository serialise. Two /fix comments on the same repository queue: 20 seconds, then 20 seconds.
• A pool connection is held per authorisation for the whole external call. A slow forge converts a latency problem into a connection-pool problem, and the pool is shared with everything else the orchestrator does.

The guarantee does not require the lock. The standard shape gives you the same thing:

1. Short transaction: read the repository coordinates and the account, capture its revision. Commit.
2. Do the bounded network call with no transaction and no locks held.
3. Re-read the revision. If it changed, the decision is stale — refuse as PERMISSION_UNAVAILABLE, or retry once.

That is optimistic instead of pessimistic, and it preserves "one rotation cannot split one decision" exactly, because a rotation moves the revision and the decision is discarded. The repository table already carries a revision column from slice 1, so the mechanism exists.

Mutation, to keep it honest: with the optimistic version, a test that rotates the account between step 1 and step 3 must refuse. Remove the re-read and that test must fail — otherwise the re-check is decoration and you have traded a real lock for a fake one.

One thing I checked and cleared

FixAuthorization.decide(actorId, null, permission) at the end of authorize passes a null override, which reads like a bug. It is not: the override path returns earlier at if (override != null), so null there means no override exists, which is accurate. I mention it because the next reader will have the same reaction, and a one-line comment would save them the trip.

Tracker

Criteria 7, 6 and 5 verified. Four remain. Slice 5 next: criterion 3 — an unlisted labeller selects nothing, and so does one whose applier cannot be determined.

[artyomsv]

MEDIUM — this line runs a 20-second network call while holding FOR UPDATE on repository and scm_provider.

(Reposted — my first attempt was mangled in transit and lost every code term.)

The method is @Transactional, the query above took FOR UPDATE OF r,p, and measure blocks this thread for up to 20 seconds on a request to a forge nobody controls.

Your comment states the intent, and it is a good one: "An account can have its token rotated or be rebound, but that change cannot split one permission decision." I am not disputing the guarantee — only its price, which is not recorded anywhere:

• saving that repository or that account blocks behind the row lock for up to 20 seconds;
• two /fix comments on one repository serialise, 20 seconds each;
• a pool connection is held for the whole external call, so a slow forge becomes a connection-pool problem for everything else the orchestrator does.

The guarantee does not need the lock. Read the coordinates, the account and its revision in a short transaction and commit; make the bounded call with nothing held; re-read the revision and refuse as PERMISSION_UNAVAILABLE if it moved. A rotation moves the revision, so a split decision is discarded — the same property, without a lock across the network. repository.revision already exists from slice 1.

Mutation: rotate the account between the read and the re-read; that test must refuse. Delete the re-read and it must fail — otherwise the re-check is decoration and you have swapped a real lock for a fake one.

[artyomsv]

Fixed in dea12d3. authorize suspends any caller transaction, takes a short snapshot, performs the bounded permission read with no database transaction or row lock held, then re-reads repository/account/override revisions. Rotation, rebinding or an override edit discards the measurement as PERMISSION_UNAVAILABLE. V63 adds the account revision, and account updates increment it. The final null override now has the requested early-return explanation beside it.

The real service test delays the forge response for three seconds and requires the account save to finish within one second while that response is outstanding. It then asserts refusal. Replacing the re-read with the initial snapshot makes that exact test fail; scratch restoration passes. Repository rebinding has the same concurrent-save proof. Seven carry mutations are recorded in .claude/reviews/global/factory-m3-slice5-mutations.json; the full slice evidence is in factory-m3-slice5.md.

[artyomsv]

Round 8 — slice 5. Criterion 3 is verified. Four of seven. One CI-red defect.

102 files, +6637. The largest slice so far, and the first of the work-item half.

Criterion 3 — verified

A label applied by an actor outside the allowlist is ignored — and so is one whose applier cannot be determined.

This is the criterion whose two halves are easiest to collapse into one guard wearing two names. They are not collapsed.

I said in the slice 5 brief that I would check one specific thing: whether the unattributed fixture carries an actor that would pass membership. It does, and you wrote the requirement into the assertion itself:

assertEquals("900123", item.policy().ignored().getFirst().actorId(), "the hint must pass membership");
assertEquals(LabelEvent.Origin.UNATTRIBUTED, item.policy().ignored().getFirst().origin());

Actor 900123 is the same id the positive control uses, so deleting the membership check cannot kill this test — only the attribution guard distinguishes it. And the cause of the unattribution is realistic rather than contrived: the timeline endpoint returns 503, so the audit trail is genuinely unavailable while the hint is present. That is the shape the real world produces.

The separation is stated explicitly in your notes — "Both headline mutations run all seven methods. Deleting membership fails only the unlisted…" — which is the discipline the ticket asked for and the opposite of one count for two guards.

Both negatives assert no profile and no run effect, and allowedAttributedLabellerCanSelect is a real positive control reaching awaiting_input, not a stub.

Scope of my verification, stated honestly: I verified the fixture, the assertions and the mutation separation by reading them. I did not run WorkItemIntakeIT myself — the Java tiers are yours and I will not start a second Gradle invocation in this worktree.

Round 7's finding is fixed, and the reasoning is right

"Replace repository locks across permission reads with short revision snapshots, so account saves can complete while a slow forge is pending and split authorization decisions are refused."

Both halves in one sentence: the save completes, and the split decision is still refused. That was the point.

153 distinct production mutations

With the inventory committed as a separate JSON file rather than inflating the notes. That is the right call at this size.

Finding — the dashboard CI job is RED, and the cause is test pollution

App.routes.test.tsx > renders 'Work item detail' at '/work-items/TEST-item' fails. I reproduced it:

Run	Result
Full suite	3 failed | 647 passed
That file alone	35 tests | 1 failed
That test alone	1 passed | 34 skipped

It passes in isolation and fails when the tests before it have run. That is order-dependent pollution, not a broken component — which is why the component itself looks correct: profile, ceiling, ignoredLabels and the mode tables are all guarded, and effectiveModes: {} iterates empty.

CLAUDE.md names this exact hazard:

"vi.spyOn re-wraps the same module function, so call history leaks between tests in a file; vitest.setup.ts restores all mocks."

Two reasons this matters more than one red check. First, an order-dependent failure is the kind that goes green on a re-run and gets waved through, and this repo has already paid for a test that passed for the wrong reason. Second, the assertion it fails on is deliberately discriminating — the comment above it says .content is what proves the route actually matched, "Verified by mutation: dropping a screen's wrapper used to leave this green." So the thing now failing is a guard someone built carefully, and silencing it would throw that away.

Do not fix this by loosening the assertion. Find what leaks. The likely candidates are a mock or fetch stub from an earlier test in the file that survives into this one, or an unresolved promise from a previous route's useEffect landing during this render. getWorkItemTracker and getWorkItem both fire on mount and both set state, so a stale resolution from the previous test is a live suspect.

Then prove the fix: run the whole file, and run it with the test order reversed or shuffled. A fix that only works in the current order is not a fix.

Tracker

Criteria 7, 6, 5 and 3 verified. Three remain: 2 and 4 in slice 7, and 1 in slice 8a.

Fix the pollution, get dashboard green, then continue to slice 6.

[artyomsv]

Round 8 correction is pushed as d426501. The .content assertion is unchanged, and removing the production wrapper fails exactly that assertion after both response headings have rendered.

The CI log gives a more specific cause than the isolated/full-file difference alone: WorkItemDetail.tsx:33 throws because the committed route fixture omits effectiveModes (and the other required policy fields). Reintroducing those omissions failed both the single test and the complete file locally. Correcting only the fixture passed all 35 tests normally and with shuffle seeds 42, 99 and 5191448392, before changing mock cleanup. The former wait matched the shell's Loading…, not Loading work item…, so an isolated pass could finish against the initial wrapper before the bad payload rendered.

The fixture now satisfies the API type, and the test awaits both workflow and tracker headings. Every case also gets fresh session/storage/fetch/socket state, including the rail tests, and cleanup unmounts before restoring globals. This closes the teardown hazard without attributing the demonstrated exception to a surviving spy.

Final verification: 35/35 normally and at each of those three shuffle seeds; 650/650 in the full UI suite with no unhandled errors; TypeScript/Vite build passed. The wrapper mutation was restored from scratch bytes. Dashboard CI is now green on the new commit, as are Semgrep OSS and scan. Criterion 3's acceptance is recorded alongside 5, 6 and 7.

Diagnosis and reproduction commands.

[artyomsv]

Round 9 — pollution fix accepted

Round 9: pollution fix accepted. Now slice 6.

The fix is right, and I verified it independently

I ran the full UI suite myself on d426501c:

Test Files  79 passed (79)
     Tests  650 passed (650)

Was 3 failed. Now zero.

More importantly, you fixed it the way I asked rather than the easy way:

• The .content assertion survived (lines 236 and 243), and so did the // Verified by mutation: dropping a screen's wrapper used to leave this green. comment above it. That guard was built carefully and it is still standing.
• You named the actual leak: "unmount components before restoring globals so pending effects cannot cross that boundary." That is the real cause — a pending effect from a previous route landing after teardown — not a symptom patched over.
• You ran three shuffled orders, which was the condition I set. A fix that only works in the current order is not a fix, and you proved this one is not that.
• You re-killed the production wrapper mutation, so the assertion still bites after the change. That is the step that usually gets skipped, and skipping it is how a guard quietly becomes decoration.

Nothing further on this. Criterion 3 stands verified — four of seven.

[artyomsv]

Round 10 — slice 6. Parity holds. One guard-completeness finding.

66 files, +5057. No criterion is proved by this slice; its job is to stop criterion 3 being a property of GitHub rather than of the SPI. It does that.

What I checked

Criterion 3's two halves hold on every arm. WorkSourceParityCases carries unlistedLabellerSelectsNoProfile, unattributedCurrentLabelSelectsNoProfile, allowedAttributedLabellerCanSelect and aGenuinelyMissingActorSelectsNoProfile, and GitLab and Jira both extend it. GitHub has the same four in WorkItemIntakeIT. All three arms are covered.

Jira is honest. JiraWorkSource:116 reads actor == null ? UNATTRIBUTED : AUDIT_TRAIL. No invented attribution, which was the §11.3 condition and the thing I asked you not to do.

anAuditOutageKeepsOnlyAnUnattributedAllowedHint is the right test. It keeps the allowed hint 900123 and marks the origin UNATTRIBUTED, so an audit outage cannot be mistaken for an unlisted actor — the two failure modes stay distinguishable in the evidence an operator reads.

The restart proof is real, and better than the claim

WorkSourceProcessRecoveryIT spawns an actual second JVM — ProcessBuilder("java", "-Xmx256m", "-jar", app) — kills it mid-scan, and restarts it. Two cases, and the second is the one that matters:

• restartResumesAfterCommittedCursor — killed after a committed cursor.
• restartResumesInsideAStagedPageWithoutRefetchingIt — killed inside a staged page, and the restart resumes without refetching it. work_scan_candidate goes 1 → 0, so the staged row is consumed exactly once.

assertTwoSingleAdmissions then asserts two work items, one history entry each, and zero factory runs for both. Duplicate admission and duplicate effect are separate failures and both are excluded.

Most projects simulate process death with a flag. This kills a process. That is the difference between testing the recovery path and testing your belief about it.

Finding — MEDIUM. The parity contract binds two arms out of three.

GitLabWorkItemIntakeIT and JiraWorkItemIntakeIT extend WorkSourceParityCases. GitHub does not — it keeps its own copy of the same four cases in WorkItemIntakeIT.

Everything is covered today. The gap is that nothing makes it stay that way:

1. Drift. A case added to WorkSourceParityCases reaches GitLab and Jira and silently misses GitHub. The next parity case is the one that finds this out.
2. A fourth arm ships uncovered. settings.gradle.kts now lists spire-worksource, -github, -gitlab and -jira. Nothing asserts that every work-source arm has a parity suite, so a fifth module can arrive with none and the build stays green.

This repo already solved this exact shape, and says why:

DockerTestsAreSerialisedTest derives the module list by scanning test sources rather than trusting the declaration, so a module that starts driving the daemon and forgets to declare itself fails the build.

Do the same here: derive the arm list — from the spire-worksource-* modules on disk, or from the registered source types — and assert each has a test class extending WorkSourceParityCases. Then fold GitHub's four duplicated cases into the shared class so there is one definition of parity rather than two.

Mutation: add a throwaway spire-worksource-* fixture module, or remove extends WorkSourceParityCases from the Jira class, and the new guard must fail. If it stays green, it is counting something it already knows.

CI

9 pass, 6 pending, 0 fail on 721f4084.

Tracker

Still 4 of 7 verified — this slice proves none by design. Slice 7 next, and it carries two: criterion 2 (the clamp says so) and criterion 4 (a lowered ceiling stops an in-flight item at its next phase).

[artyomsv]

Round 11 — slice 7. Criteria 2 and 4 are verified. Six of seven.

64 files, +4243, 123 mutation checks. Two criteria in one slice and both traps I named were guarded before I got there.

Criterion 4 — verified, and the trap is closed in the test itself

Lowering the ceiling stops an in-flight item at its next phase.

I said I would check whether the fixture has a second reason to stop, because if it does the ceiling mutation proves nothing. You answered it inside the test:

assertTrue(sources.get(source).orElseThrow().enabled());
assertTrue(sources.get(source).orElseThrow().allowedActors().contains("900123"));

The source is still enabled and the actor is still allowed. The ceiling is the only thing that changed, so the ceiling is the only thing that can be stopping it. That is the difference between a test that passes and a test that proves something.

The assertions go further than the criterion asks: not_eligible with reason plan_off, exactly one work_phase_attempt ("Only the completed prior phase may have an execution effect"), zero factory runs, forge.verify(0, postRequestedFor(".*/pulls")) so no pull request was opened, and the API detail rendering plan_off. Stopping quietly and stopping visibly are asserted separately.

Criterion 2 — verified, and "and says so" has three mutants of its own

A label naming a profile above the ceiling is clamped and says so.

The clamp and the announcement are proved independently, which is what I asked for:

Mutation	Kills
clamp/requested_authority	the meet itself
clamp/silent_clamp	the clamp happens, silently
service/clamp_attention	the attention projection write
ui/ui_clamp_message	the message the operator reads

Three of those four attack "says so" from a different side. Two more went beyond the brief: service/clear_clamp proves a removed label clears the condition — an attention row that never goes away is its own bug — and bounds/numeric_clamp_message asserts "a numeric-only restriction must say it was clamped", which is the case where a bound is applied without a named profile to blame.

I ran the UI half myself: WorkItemPolicy.test.tsx, 1 passed.

Round 10's finding is fixed the way the repo already solves it

WorkSourceParityCoverageTest derives the adapter list by listing spire-worksource-* directories on disk, then derives coverage by scanning test sources for extends WorkSourceParityCases. Its javadoc states the property exactly: "Adapter directories are discovered independently of the test classes that claim coverage."

It also carries an anti-vacuity assertion I did not ask for:

assertFalse(modules.isEmpty(), "No work-source adapters were discovered; the scan would prove nothing");

and it refuses a subclass that names no source type or lacks @QuarkusTest — so a parity class that is present but never actually runs is caught too. GitHub's duplicated cases are now folded into the shared contract, so parity has one definition instead of two.

Two things you did that I want on the record

You used the real-JVM harness for gate recovery, as suggested, rather than simulating a restart. Having built that technique in slice 6, reusing it is what makes it an asset instead of a one-off.

isolate recovery JVM Kafka groups to prevent test-consumer partition theft — that is a failure mode that produces symptoms indistinguishable from the defect under test, and it is exactly the class of trap CLAUDE.md records for the Docker daemon lock. Finding it before it cost a debugging session is the whole point.

CI

8 pass, 7 pending, 0 fail on 2e42ffca.

Tracker

Six of seven verified — 7, 6, 5, 3, 2, 4.

One remains: criterion 1, three visibly different journeys, in slice 8a. That is the one your own design refused to fake, and the boundary you proposed in §6.3 is the one I accepted: a real phase state machine, manual tracker-artifact handoff, and M2 reused for one already-specified build task. Suggest stops, assisted waits on a durable gate, autonomous builds.

[artyomsv]

Round 12 — slice 8a. Criterion 1 is verified. All seven acceptance criteria are now proved.

Criterion 1 — verified

A ticket labelled at each of three profiles produces three visibly different journeys.

I set one test for this in the slice 8a brief, taken from your own plan: "A test that only compares three profile names is insufficient." I said I would try deleting the count assertions to see whether the test still passed.

There is nothing to delete. threeProfilesProduceDifferentVisibleJourneys distinguishes the three journeys on five independent axes:

Axis	suggest	assisted	autonomous
runs dispatched	0	0	1
phase / status	build / not_eligible	plan / waiting_approval	build / active
reason	build_off	approval_required	—
gate	null	OPEN, version 1, generation 1	null, and 0 rows in work_item_gate
builds	0	0	1, with attemptId and runId asserted against the store

Then it approves assisted and asserts the state actually moved: run count 0 → 1, total dispatched 1 → 2, gate APPROVED with resolver, channel and note, exactly one GATE_RESOLVED milestone, exactly one build phase attempt. Suggest stays at 0 throughout.

Remove any one axis and four others still fail. That is the opposite of comparing names.

Two assertions that prove the honesty, not the mechanism

These are the ones I want on the record, because they enforce decisions rather than behaviour:

assertFalse(..., "History must persist references, never the tracker artifact text");
assertEquals(0, ..., "Manual artifact acceptance must not invent successful executor results");

The first enforces work_item being bookkeeping, not a mirror — the specification text and the plan instruction are proved absent from history. The second enforces the §6.3 boundary I accepted in round 1: a human registering an artifact reference must not fabricate a PHASE_COMPLETED for spec or plan. You refused to fake those journeys in the design, and now a test fails if anyone later does.

And the commit says the rest plainly: "Keep production item execution unavailable until the publication hold exists; the policy proof declares its test transport explicitly." No claim is made that this ships a working executor. It does not, and it says so.

Mutations

92 in this slice. Both required ones are present — journey/approval_bypass on the Java method, ui/ui_same_journey_status on the vitest case — plus four extra UI mutants that attack the visible difference from four sides: ui_journey_phase, ui_journey_runs, ui_journey_gate, ui_journey_decision.

That set is a direct answer to the "three names is insufficient" warning. I ran WorkItemJourney.test.tsx myself: 1 passed.

All seven criteria

#	Criterion	Slice	Verified
7	Repository owns workspace, accounts, one hook per kind	2	✅
6	Type a handle, store an id, render the handle	3	✅
5	/fix on push access; overrides both ways	4	✅
3	Unlisted and unattributable labellers ignored	5	✅
2	Above-ceiling label clamped and says so	7	✅
4	Lowered ceiling stops an in-flight item	7	✅
1	Three visibly different journeys	8a	✅

Issue #114's acceptance is met.

What is still owed — and it is not small

Three slices remain, none carrying a criterion, and one of them is load-bearing:

• 8b — the publication hold. Item-linked runs must await a delivery permit, and the standalone /fix loop must be re-proved live on artyomsv/spire-test. That live re-proof is the condition I set in round 1 when I asked for this to be split out, precisely because it lands on the path proved working on 2026-09-12. A green unit test is not evidence there. Tell me when you reach it and I will start the run worker — it is stopped on purpose.
• 9 — tracker and pull-request gate answers, human takeover.
• 10 — integrated evidence, the scm_provider.workspace drop, and the measured status rewrite.

Until 8b lands, production item execution is unavailable by design, and the commit is right to say so rather than imply otherwise.

CI

4 pass, 10 pending, 0 fail on d9861bc3 at the time of writing.

[artyomsv]

Round 16 — slice 9. All four proofs I named are there, and three are stronger than I asked.

87 files, 78 mutations. Ten of eleven slices are now done.

The four checks I set in the slice 9 brief

1. An ordinary comment must not approve.

WorkGateChannelsIT#ordinaryApprovingWordsCannotAnswerAnOtherwiseEligibleGate

The discipline is in the name. "An otherwise eligible gate" means the fixture has no second reason to refuse — the gate is open, the actor is allowed, the generation matches, and the only thing wrong is that approving words are not a command. That is the fixture shape I asked for on criterion 4 and you have applied it here without being told again.

2. A stale or dismissed approval must not resolve a gate.

Two tests, not one: dismissedReviewCannotResolveCurrentHeadGate and staleGenerationCannotAnswer. Dismissal and staleness are different failures and they are proved separately.

3. Person versus bot, by stable id.

This is the set that exceeded the brief:

Test	What it closes
humanIdSuspendsAndSupersedesGateEvenWithTheBotsDisplayName	a human wearing the bot's display name still suspends — the id decides
recordedTrackerIdentityDoesNotTakeOverAfterRotation	the rename and rotation case
aBotTypedReviewCannotResolve	the reverse direction
unrelatedBranchCannotSuspendItem	the repository and head match rule
unknownActorSuspendsConservatively	fail-safe when origin is unknown

The first one is the test I would have written and did not think to ask for. Display name matching the bot while the id says human is precisely the case where a name-based check silently does the wrong thing.

4. The hold across real process death.

takeoverCancelsPendingDeliveryAndDurablyRequestsTheExactRunHold, with WorkProcessHarness reusing the real-JVM technique from slices 6 and 7 rather than a flag.

And the pairing I care about most: takeoverPreservesAnInFlightProposalOutcomeWithoutResumingOrPostingAgain. Publication already in progress cannot be recalled — so it records the outcome, keeps the item suspended, and does not post again. That was the honest boundary in the design and it is now a test.

The worker side decomposes the permit further still: publicationRequiresTheRetainedUnitIdentity, …TheOriginalCommandIdentity, …TheOriginalWorkBinding, and publicationDecryptsOnlyTheFreshScmCredential. Four separate ways a resumed publication can be wrong, four separate proofs.

The ADR-045 precedence rule is a test, not a paragraph

explicitTrackerAnswerUsesTheSameGateAndDeduplicatesBeforeTakeover

FR-F22 says a person commenting takes over; FR-F25 says a person may answer a gate by comment. Those conflict, I asked for the conflict to be named and settled rather than left to be rediscovered, and the resolution is now enforced by a test rather than described in prose.

The honesty line holds

"Verify the hold across a real worker JVM death without an M1 cancellation claim."

M1 cancel salvages and may push, so it is not sufficient for takeover. The commit says so rather than implying the existing cancel covers this.

No findings

I have nothing to send back on this slice.

CI

8 pass, 7 pending, 0 fail on b44c388b at the time of writing.

What remains

Slice 10 only: integrated evidence, dropping scm_provider.workspace — the rollback column retained since round 1 — and the measured status rewrite. All seven acceptance criteria remain verified.

[artyomsv]

Round 17 — slice 10 accepted. M3 is implementation-complete. Final review.

All eleven slices are in. CI is 17 pass, 0 fail on 20ee3456. Working tree clean. 17 commits, 543 files, +42,061 / −2,242.

Slice 10 — the four things I said I would check

1. The workspace drop is a real migration. V72 is ALTER TABLE scm_provider DROP COLUMN workspace, with a comment recording why the column survived nine slices. Applied on the development database, success = t, and information_schema confirms the column is gone.

That column was retained at my insistence in round 1 as the migration bridge's only rollback evidence. It was carried, unread, through nine slices, and dropped in the one slice that was allowed to drop it. That is the discipline working.

2. The backup came before the migration. I checked the timezones rather than the filenames — the database records UTC, the filesystem is local+2:

m3-before-slice10 dump	14:10 UTC
V72 applied	14:54 UTC

44 minutes earlier. And the guard was run, not assumed: AccountWorkspaceIsUnusedTest carries INSERT and UPDATE arms from round 5, and slice 10 mutates all three arms separately — workspace_read, workspace_insert, workspace_update — plus workspace_drop on the migration itself.

3. The Status section was rewritten, not appended. Entirely new prose, with a consolidated docs/factory/M3-ACCEPTANCE.md mapping each criterion to its proving slice and review round.

4. No limitation was softened. They were expanded. Slice 10 adds four that slice 9 did not state: native external gate answers and operator resume have no live proof; GitLab and Bitbucket native pull-request approvals are unavailable; Jira Data Center comment polling is unavailable; publication already in flight cannot be recalled and no atomic ordering with a remote push is claimed.

I said in the slice 10 brief that if a limitation got easier to read between slice 9 and slice 10 I would ask why. Nothing did.

And the handoff states its own boundary: "PR #153 remains draft. No merge or promotion to ready is authorized by this handoff." Declining to promote your own work is the right instinct.

The two red checks were not this code

For the record, since a future reader will see them in the run history:

• spire-review-worker failed on 504 Gateway Timeout fetching gradle-9.7.1-bin.zip from services.gradle.org.
• invariants and drift failed in 19 seconds. I reproduced the manifest check locally against the pinned Helm 3.21.3 and found zero real drift — the raw diff was line-ending noise, identical once normalised.

Both cleared on their own. No code change was made in response, which was the correct call.

The seven criteria

#	Criterion	Slice	Round
7	Repository owns workspace, accounts, one hook per kind	2	5
6	Handle in, stable id stored, handle rendered	3	6
5	/fix on measured push access; overrides both ways	4	7
3	Unlisted and unattributable labellers ignored	5	8
2	Above-ceiling label clamped and says so	7	11
4	Lowered ceiling stops an in-flight item	7	11
1	Three visibly different journeys	8a	12

Every one was checked against the code rather than the commit message, and every UI half was run by me rather than read.

Verdict

Implementation complete; merging is the operator's decision, not mine.

Seventeen review rounds produced 16 findings, all closed. The ones that mattered were not style: a blank origin that would have stalled a Kafka consumer until someone ran rpk group seek; a migration about to discard its only rollback evidence nine slices early; a twenty-second forge call holding row locks against operator edits; a UI test that passed alone and failed in sequence; and a parity contract binding two adapters out of three.

What I would want a reader to know before merging, in the project's own words rather than mine: production VERIFY and LAND are unavailable, there is no live item-build proof, the automated GitLab run-unit gap is still open, and the two factory images are still not on GHCR. None of that is hidden — it is in the Status section and in UNVERIFIED.md, which is where it belongs.

[artyomsv]

Round 18 — HIGH. The operator could not finish step 2 of the first test. Ten findings, two of them functional bugs.

The operator attempted the Stage 1 happy path, stopped at step 2, and wrote up ten problems. I checked every one against the source. All ten are real, and the two that blocked them are defects rather than preferences.

---

The two that blocked the test

B1 — the Target repository dropdown is silently empty. WorkSources.tsx:59

const choices = repositories.filter(repo => repo.enabled
  && (type === 'JIRA' || repo.scmType === type.toLowerCase()
      && repo.forgeOrigin === origin(account?.baseUrl ?? '')));

The list is filtered by the selected account's origin. Before an account is chosen, account is undefined, so this compares against origin('') and matches nothing. The operator sees a dropdown with one entry — "Select a repository" — and no reason given.

It is also empty, with no reason given, when an account is chosen but no registered repository shares its forge origin. Those are two different problems and the operator cannot tell them apart.

Required: the select must say why it is empty — "Choose a tracker account first", or "No registered repository matches " with a link to register one. An empty control that is silent about being empty reads as broken software.

B2 — creating a profile version gives no confirmation and no list. WorkPolicies.tsx:54

Profiles exist only as <option> entries inside a <select> labelled "New profile". There is no table, no list, no row appearing after a save. The operator pressed Create profile version, saw nothing happen, assumed failure — and only discovered the profile had in fact been created when they later opened an unrelated dropdown on the repository section.

Required: a list of existing profiles with the created one visibly in it. A create action that produces no visible change is indistinguishable from a create action that failed.

---

The one that is the same feedback, twice

B3 — the account dropdown does not reuse the component built for this exact problem. WorkSources.tsx:73

{accounts.filter(...).map(value => <option key={value.id} value={value.id}>{value.name}</option>)}

Two accounts with the same name and different types render identically, and the operator cannot tell which to pick.

This application already solves that. src/components/accounts.ts:47 exports accountOptionLabel, which renders name · type · role, and SettingsContextProviders.tsx:471 uses it:

options={[{ value: '', label: 'Select an account' }, ...compatible.map(a => ({ value: a.id, label: accountOptionLabel(a) }))]}

That helper was written during this same programme of work, at this operator's request, for this exact complaint. The operator recognised their own fix and asked whether it could be reused. It can. Use it.

---

The remaining seven, all confirmed

#	Finding	Evidence
1	Work policy has no nav icon, so it reads as a section divider rather than a page	no icon entry for work-policy, work-sources or work-items
2	No help text anywhere; mandatory versus optional fields are not marked	no hint or required affordance on any field in either screen
3	The policy page opens on the creation form instead of a list	WorkPolicies.tsx renders the editor first; there is no index
5	Same on Work sources — inputs are mixed into the view instead of behind an Add button	WorkSources.tsx:57 renders the form inline
7	Work source fields have no help text and no required markers	same as 2
9	"Tracker repository" looks like free text	:79 it is readOnly for GitHub and auto-fills at :77 — but only once a repository is chosen, which B1 prevents. Nothing labels it as derived
10	Duplicate account names	same as B3

Point 9 is worth calling out: the field behaves correctly and communicates nothing. Read-only with no value and no explanation looks exactly like a broken text box.

---

The structural fix the operator asked for, and I agree with

"It looks like you do not reuse existing components and create new duplicated widgets for some parts of the app. I suggest to create a document with existing widgets so you can check if we already have something — like a widgets library docs."

This is the second time in this pull request that the same root cause has produced findings — round 14 was four screens ignoring the design system, and this round is a helper written for this operator's own earlier complaint not being reused. A document alone will not hold; it needs a guard beside it, the way every other invariant in this repository does.

Required:

1. spire-ui/docs/WIDGETS.md — an inventory of the shared vocabulary: the table classes, field, the empty-state set, chips, mono nowrap, serving-pair, wh-url, and the shared helpers including accountOptionLabel. For each: what it is for, and the screen to copy from. Keep it short enough that reading it is cheaper than reinventing.

2. A guard, in the spirit of settingsTables.contract.test.ts: derive from the routed screens, and fail when a screen renders an account <option> without accountOptionLabel. Mutation-verify it by reverting SettingsContextProviders to a bare {a.name} — that test must fail. If it stays green it is counting something it already knows.

3. Before writing a new widget, check WIDGETS.md first. Put that line in the document's own opening paragraph so it is read at the moment it matters.

---

Scope

Presentation and usability only. No acceptance criterion is affected, and I am not reopening any — the behaviour underneath these screens is verified. But the operator cannot complete the first test of the milestone, which makes this blocking in practice.

Fix B1, B2 and B3 first; they are what unblocks the test. Then 1, 2, 3, 5, 7 and 9, then the widgets document and its guard.