Repository navigation
Verify held item builds with the operator's checks - #184
Conversation
Verify runs the operator-declared check commands on a clean copy of the held checkpoint, in a container without the model credential. Passed, failed and unverified are distinct outcomes; failed and unverified open a gate (retry build or stop) instead of failing the item. Part of #115
Retry build rewinds to the build phase, the verify result gets its own topic and claim key, tails ride the event, the stored execution keeps its old component, the commands join the preparation binding, and the verify time limit is bounded by the run worker's ack budget. Part of #115
Seventeen tasks from the contract to the live proof, each verified in CI. The spec now states that the verify time limit covers prepare and checks together, bounded by the run worker's maximum wall clock. Part of #115
WorkVerification carries passed, failed or unverified with bounded check tails. WorkExecution keeps its attempt id and gains the result, so stored history still reads. VerifyWork and RunWorkVerified are the two new messages; the verification rides its own topic. Part of #115
The commands decide what passed means for a build, so the gate that approves a preparation approves the exact list, in order, and its time limit. Older versions keep their hash and carry no commands. Part of #115
Failed or unverified checks open a gate in every verify mode but off, auto included, instead of failing the item: verify fails a step, never a work item. Part of #115
ExecuteRun carries the previous held run and its head, so a retried build continues from its own work. Every older constructor and wither keeps the value or starts from the base. Part of #115
A fresh clone imports the agent's bundles through the publisher's copy-out-of-reach path and resets to the gated head. Verify and the retried build both use it. Part of #115
spire-verify-prepare rebuilds the held checkpoint in a fresh workspace with the remote removed and reports the head it prepared. spire-clone can start a retried build from another held run's checkpoint. Part of #115
A verify unit is a prepare container in the publisher image and one agent-image container per check. The spec refuses a check that sees the handoff, writes anything but the verify volume, keeps the agent's entrypoint, or receives a credential-named variable. Part of #115
A prepare container rebuilds the checkpoint in a new volume, then each check runs with the agent image's entrypoint overridden and only that volume mounted. Verify resources carry the hold label and their own role, so cancel reaches them and discovery never mistakes one for the unit. Output tails are bounded in lines, line length and size. Part of #115
A VerifyWork on the work channel claims its own slot beside the build, runs the verify unit against the kept build, classifies what it saw and stores the result before sending it on cs.run-verifications. A restart reports a verify it lost as could-not-run. A retried build that reaches its checkpoint releases the held build it started from. Part of #115
Build setup keeps the check commands, one per line with blank lines dropped, and a verify time limit bounded by the deployment's wall clock. Preparation copies both under binding version 5, so a later edit leaves a prepared task's checks alone. work_verify_effect is the verify outbox. Part of #115
CHECKPOINT_MISSING and VERIFY_PREPARE_FAILED map to CLONE_FAILED, so every cause a producer can emit stays in the closed set. The watchdog's fake runtime answers the two verify methods. Part of #115
Starting verify projects a work_verify_effect row; a dispatcher claims it and sends VerifyWork on the work topic. The result is stored encrypted, then applied once: passed completes verify, failed or unverified open a result gate in every mode but off. Retry build rewinds to build and counts a run; Stop names the operator's decision. Delivery now needs a passed result for the bound checks, so the tests drive the real flow. Part of #115
The latest applied verify that did not pass gives the retry its start point and a bounded prompt section naming the failed command and its output. A missing checkpoint starts again from the base and says so. Part of #115
Build setup gains a check commands box, one per line with blank lines dropped, and a verify time limit bounded by the deployment's maximum. The summary and the plan decision show the checks a build will run. Part of #115
The verify step shows passed, failed or not checked with distinct icons, and a table of the checks whose output opens on demand. A verify result gate is named for what it decides: Retry build or Stop. A recorded attempt without an outcome is no longer shown as a verification. Part of #115
The image contract gains two verified clauses: /bin/sh is present, since verify runs each check as /bin/sh -c, and the image runs as uid 1001, the owner of the verify workspace the publisher writes. Part of #115
ADR-046 records where checks are declared, where they run, the three outcomes and the result gate. FR-F20 names the operator as the one who declares checks. ADR-033's phase order now matches the code. The run topology describes the verify unit, and UNVERIFIED lists what no test establishes yet. Part of #115
work_verify_effect references the work item, its attempt and its run, so a cleanup that kept the row failed on the foreign key, left the test's profiles behind, and every later test collided with them. Part of #115
M3 allowed one attempt per phase per generation, which refused the second build a Retry build starts. What that rule protected is that two attempts of one phase never run at once, so a partial unique index on started attempts keeps exactly that. A hand-registered preparation now copies the repository's checks too, and the tests that pinned the old stop or reused the fixture's own ticket numbers are updated. Part of #115
- Checks run with /workspace as their working directory, whatever the agent image's WORKDIR, so a relative command means the verified tree. - A passing check keeps only its last 2 KiB of output; the deciding check keeps its full tail. Twenty full tails exceeded the broker's record limit and would have been resent for ever. - Verify time counts toward the item's wall clock. - A retry whose checkpoint unit is gone starts from the base instead of refusing the build and failing the item. Part of #115
The wait was floored to whole seconds, so it ended a fraction before the deadline and a still-running check was reported as not timed out. Part of #115
Ten mutants, all killed; the delivery guard needed its own test first. Part of #115
An agent's copy of a Codex sign-in has its refresh token emptied, so it cannot renew the 10-day access token. A seat therefore stopped working 10 days after sign-in while the settings screen still said "Ready": item #40's build failed with "refresh_token: empty string" and a 401. The orchestrator holds the whole file, so it now renews it. Every 15 minutes a seat whose access token expires within 3 days is renewed at the vendor's token endpoint with the vendor CLI's own client, read from the codex-cli 0.156.1 binary. The new tokens are stored under a compare-and-set on the ciphertext, so a sign-in made meanwhile wins, and one orchestrator renews at a time because refresh tokens rotate. A 400/401 answer marks the seat refused and the screen says "Sign in again"; other failures are retried at the next sweep. The refresh token still never reaches an agent. Part of #115
The core names no vendor. The renewal's client id and token address move from a constant in SeatRenewal to spire.factory.seat-renewal, keyed by harness name like the agent image, with how they were measured written beside them. Part of #115
Adding the renewal clients to FactoryConfig broke every test fake of that interface. They now live in SeatRenewalConfig under spire.seat-renewal.clients, which no fake implements. Part of #115
The factory can deliver a draft pull request, and the reviewer skips drafts. Item #41's pull request therefore waited at the review step for a review that never came. Delivery ends the factory's work on the pull request, so the review step now marks a draft ready before it waits for the review. PullRequestSink gains markReady. GitHub has no REST call for it, so the GitHub sink reads the pull request and sends the GraphQL markPullRequestReadyForReview mutation with its node id; one already ready is answered with no write. Other forges refuse with ready_for_review_unsupported, and the item says so instead of waiting silently. A failed attempt waits as ready_for_review_pending and is tried again on the next pass. Part of #115
The reviewer account's author allowlist skipped item #41's pull request, because the factory account opened it, so the item's review step waited for a review that never came. The allowlist decides whose pull requests the review spend covers; a factory pull request was admitted through its work item's own gates. A pull request whose author is the repository's factory account, matched by stable provider id, is now reviewed whatever the allowlist says. Someone using the bot's handle under another id is still skipped. Part of #115
The saga checks the author allowlist twice: when it routes the event to a repository, and again when it handles the pull request. The first check admitted the factory's pull request, the second skipped it, and the dashboard showed a review stuck at received with no title or head (PR #42 on the test repository, 2026-10-08). Both checks now share the factory exception. Part of #115
The review step read a running or failed review of the build's head as review_head_not_observed, "the review and its posted result do not both cover this build", which told the operator nothing while the reviewer worked or after it failed. It now reports review_in_progress and review_failed for those, and keeps the other reasons for a review of another head, an unposted result and a degraded one. Part of #115
The rules provider reads the command's rules text, never a discovered reference, yet it ran again at level 2 whenever a level-1 item revealed a fresh reference. The factory's pull request links its ticket, so its review carried the .codespire rules twice: an item with no uri escapes the uri dedup. The rules provider is now FirstLevelOnly, like the code provider. Part of #115
Three items on the test repository proved the three verify outcomes: #43 passed and reached the land gate with no human step, #45 failed, was retried from its checkpoint and passed, and #47 was unverified (tool missing) and stopped. Every build was paid by a seat at cost 0. The record lists the identifiers, the six defects the first pass found and fixed, and the limits that remain. HISTORY gains the milestone, the Status snapshot is rewritten, and UNVERIFIED states what the live proof does and does not establish. Part of #115
- A failed or unverified result for an item no longer selected, or whose verify is now off, threw "Verify is off" and wedged the item. It now ends not eligible; a tracker outage waits as 503 and the result sweep applies it once the tracker answers. The sweep catches per row, so one unreadable result no longer starves the rest. - A verify that never answers is given up past its own limit plus 15 minutes and reported unverified (verify_could_not_run) through the normal result path, so a person can retry or stop. - A review that is over but not completed (refused, cancelled, superseded, observed) names what happened instead of "in progress". - A closed draft is refused before any write, and a definite refusal is asked about again only after 10 minutes, not every 5 seconds. - A retry that falls back to the base gets a note in its prompt, and the decision panel no longer promises the previous commits. - The decision panel's answer words come from one map, not nested ternaries; the review step no longer calls Optional.get(). Part of #115
- The retry fallback note was written with real line breaks inside a Java string and did not compile; it is one escaped string again, and readableStart keeps its own javadoc. - The check-command rule lives in one class, VerifyCommands, used by the build setup, the preparation and the verify command. - DockerVerifyUnits builds prepare and check containers through two named methods instead of a boolean flag. - WorkRunAssembly no longer calls Optional.get(). - The allowlist javadoc sits on authorAllowed again, BuildDefaults has one javadoc for its record, and CLAUDE.md names ADR-042..046. Part of #115
- WorkVerifyDispatcher.claim reads its staleness check and its command from two named helpers instead of one 36-line method. - Seat renewal tests now pin a 400 answer (OAuth invalid_grant) as a refusal and a seat with no refresh token as refused without asking the vendor; only 401 was tested before. - The methods and classes M4 verify grew past the size rules are recorded as tech debt, with the split each one wants. Part of #115
artyomsv
left a comment
artyomsv
left a comment
There was a problem hiding this comment.
This PR adds operator-command verification of held builds, persisted results and retry handling, plus subscription renewal. I found one high-severity watchdog integration defect and five medium-severity correctness defects. The reviewed commit has passing CI, but these lifecycle, race, budget and diagnostic cases are not covered by the current tests.
Findings: 1 HIGH, 5 MEDIUM. Reviewed head 8e75a18377 as @artyomsv.
artyomsv
left a comment
artyomsv
left a comment
There was a problem hiding this comment.
❌ CHANGES REQUESTED
Posted as a comment: GitHub does not let an account approve or request changes on its own pull request. The label carries the verdict.
Reviewed PR #184 at 8e75a18; base ea1288b. Service/feature: global/m4-verify.
Changes requested: six inline findings, one HIGH and five MEDIUM. The highest-impact defect is the orphan watchdog killing actively running verification. The remaining findings cover conflicting result replay, exceeding the remaining work-item time budget, cleanup lost after retirement, stale renewal refusals rejecting fresh credentials, and truncation dropping the actual end of check output.
Per-lens results:
- Security: reviewed credential selection/renewal, secret-bearing environment and logs, encrypted result storage/AAD, checkpoint bundle/path handling, and API boundaries. No separate new security finding. CI's incremental Semgrep scanned 96 files with zero introduced findings; the full scan reported three alerts, which were not independently triaged in this review.
- Correctness/design: six findings with concrete scenarios and fixes in their inline threads.
- Rules: applied root CLAUDE.md and matching global rules. No repository-local rules or additional directory CLAUDE.md files were found. The recorded method/class size debt is already tracked in techdebt/global/4-2-m4-verify-grew-methods-and-classes-past-the-size-rules.md and is not duplicated as a new finding.
- QA: checked verification, worker, Docker, renewal and UI coverage. The missing regressions are specified in each finding; in particular, a Docker check test does not prove safety against the enabled watchdog. CI at the exact reviewed SHA passed Java fast/service tests and packaging plus dashboard checks. Dashboard logs report 962 passing tests across 106 files. Java logs report successful Gradle test tasks but do not expose aggregate test counts. Service jobs cover agent-image, gateway, review-worker, runtime-docker, orchestrator and run-worker. Other required checks were green; the merge check was skipped.
Rules applied from ~/.claude/rules:
api-versioning-and-contracts.md; blog-material-capture.md; clean-code-general.md; clean-code-java.md; clean-code-react.md; docker-infrastructure.md; documentation-maintenance.md; local-environment.md; local-port-ranges.md; no-local-builds.md; no-synthetic-data.md; observability-and-logging.md; secrets-and-env-handling.md; seo-react.md; tech-debt-tracking.md. The Spring-specific requirements were considered against this Quarkus repository; entity-versioning rules do not apply to the preparation binding-version fixture.
Lessons consulted: the four lens MEMORY.md indexes and the monorepo project MEMORY.md, plus security-officer/project_codespire_m4_verify_review.md, code-reviewer/project_codespire_tracker_outage_empties_policy.md, rules-compliance/project_code_spire_m4_verify_184_review_2026_10.md and qa/code-spire-m4-verify.md. Previously addressed items were not repeated.
Validation limits: no local builds, tests or linters were run, in accordance with the host rule. Findings are based on static traces and existing CI evidence; their proposed regressions have not been executed. No repository files were modified. CI logs read include run 37909551560 (tests/dashboard/packaging) and run 37909551427 (Semgrep).
Agent findings: 0 resolved, 6 open. CI: 17/18 checks passing. Head 8e75a18377, reviewed by @artyomsv.
Open findings:
- [code-quality/H-1] HIGH — #184 (comment)
- [code-quality/M-1] MEDIUM — #184 (comment)
- [code-quality/M-2] MEDIUM — #184 (comment)
- [code-quality/M-3] MEDIUM — #184 (comment)
- [code-quality/M-4] MEDIUM — #184 (comment)
- [code-quality/M-5] MEDIUM — #184 (comment)
artyomsv
added
the
review: changes-requested
Each test pins one finding from the review of 8e75a18 and is expected to fail until its fix lands: - H-1: a live verify must count as an executing run, or the orphan watchdog stops its container. - M-1: a late result must not override the one stored for its attempt. - M-2: verify gets only the item's remaining wall time. - M-3: a retired build's workspace removal stays owed until it happens. - M-4: a slow refusal must not reject a newer sign-in. - M-5: check output past the budget keeps its end, not its start. The tail collection moves into RollingTail with its behaviour unchanged. Part of #115
- H-1: a verify registers its held build as executing, so the orphan watchdog no longer stops the verify container during a sweep. - M-1: a repeated delivery applies the result stored first, never the incoming one. - M-2: the verify time limit is capped by the item's remaining wall time, as the build's is. - M-3: retiring a superseded build records its workspace removal as owed, and recovery removes it after a failure or a restart. - M-4: a refused renewal marks the seat only while it still holds the sign-in that was refused. - M-5: check output past the budget drops its oldest text, so the final error stays in the tail. Part of #115
artyomsv
left a comment
artyomsv
left a comment
There was a problem hiding this comment.
Re-reviewed all six prior findings and the 15-file fix delta from 8e75a18 to 735599e. Five findings are fixed; code-quality/M-3 remains open because recovery clears the cleanup obligation when containers are gone but held volumes survive a partial teardown. No additional finding threads are being opened; the remaining scenario and regression request are recorded on the existing M-3 thread.
Findings: none. Reviewed head 735599ed07 as @artyomsv.
artyomsv
left a comment
artyomsv
left a comment
There was a problem hiding this comment.
❌ CHANGES REQUESTED
Posted as a comment: GitHub does not let an account approve or request changes on its own pull request. The label carries the verdict.
Re-review of PR #184 at 735599e.
PR base: ea1288b.
Fix delta reviewed: 8e75a18..735599e (15 files). Service/feature: global/m4-verify.
Five threads verified fixed and resolved: code-quality/H-1 (watchdog registration), M-1 (canonical stored result), M-2 (remaining time budget), M-4 (renewal refusal compare-and-set), and M-5 (rolling log suffix). Each thread names the code and regression reviewed. No new finding threads were posted.
One MEDIUM finding remains: code-quality/M-3. WorkRunWorker.releaseRetired, lines 223–224, skips resource removal when localUnit returns empty, then clears release_pending. DockerRunRuntime.destroyHeld removes containers before volumes, while discoverUnits enumerates containers only. A failure or process death after removing containers can therefore leave volumes behind; recovery then clears the obligation without removing them. Use the run-ID handle and publicationHeld to detect surviving held resources, as releasePublished already does, and test recovery with no discovered containers but retained volumes. The current regression retains a discoverable container and does not cover this failure.
Existing thread: #184 (comment)
Re-review record: #184 (review)
Per-lens assessment:
- Security: reviewed the credential snapshot comparison, SQL parameter order, encrypted result replay, publication-key-bound removal and bounded output. No new security finding in the fix delta. CI Semgrep (run 38530490123) scanned 96 files with 372 rules and zero introduced findings. Its separate full scan covered 1,404 files and reported three alerts; those alerts were not independently triaged here.
- Correctness/design: five original defects addressed; cleanup remains incomplete under partial teardown. Reviewed the changed methods with their recovery, watchdog, cancellation and aggregate-transition callers.
- Rules: no new blocking rule finding in this delta. Applied root CLAUDE.md, the user's review/no-build/git instructions, and the matching global rules listed below. Existing class/method-size debt remains recorded in the PR's debt entry. No new endpoints, wire fields, dependencies or configuration keys were added by these fixes.
- QA: reviewed all added/changed tests for whether they exercise the original defects. The cleanup test covers a still-discoverable workspace but misses the remaining volume-only case. CI evidence below is for the exact reviewed head. No builds, tests or linters were run locally, and no repository files were modified.
Rules applied: ~/.claude/rules/blog-material-capture.md, clean-code-general.md, clean-code-java.md, observability-and-logging.md, secrets-and-env-handling.md, no-local-builds.md, no-synthetic-data.md. No repository-local rules or additional directory CLAUDE.md files were found; the matching .claude/reviews/global/m4-verify.md state file is absent.
Lessons consulted: the four reviewer MEMORY.md indexes and monorepo project MEMORY.md, plus security-officer/project_codespire_m4_verify_review.md, code-reviewer/project_codespire_tracker_outage_empties_policy.md, rules-compliance/project_code_spire_m4_verify_184_review_2026_10.md and qa/code-spire-m4-verify.md. The code-spire-specific Quarkus conventions and existing debt were considered rather than applying unrelated Spring assumptions.
CI validation: run 38530490138 completed successfully on 735599e. Read fast-test, dashboard and service/packaging job logs. Dashboard: 962 passed tests in 106 files, with TypeScript and production-build steps successful. Java testFast succeeded; service tests for agent-image, gateway, review-worker, orchestrator, run-worker and runtime-docker succeeded (15m 8s), followed by successful packaging (32s). Java test counts are not exposed in these Gradle logs. The PR check watcher finished successfully; the merge job was skipped.
Residual limits: the partial-teardown finding is established by the source path and is not covered by the added test; no local failure injection was performed. Runtime tests and static registration assertions are not a new live production acceptance run.
Verdict: changes requested until code-quality/M-3 is addressed.
Agent findings: 5 resolved, 1 open. CI: 17/18 checks passing. Head 735599ed07, reviewed by @artyomsv.
Open findings:
- [code-quality/M-3] MEDIUM — #184 (comment)
The daemon lists units by their containers, so a teardown that stopped after removing them left volumes no unit named. Recovery then cleared the owed removal without removing them. The hold is now asked by run id, as the published-build release already does, and removed when present. Part of #115
artyomsv
left a comment
artyomsv
left a comment
There was a problem hiding this comment.
Re-reviewed the three-file delta from 735599e to 96289a4 and verified the remaining cleanup finding. Recovery now detects held volumes by run ID after their containers are gone, and the new regression verifies deletion before recording release. No new findings; all six registered findings are now addressed, and CI passed at the reviewed head.
Findings: none. Reviewed head 96289a4dc3 as @artyomsv.
artyomsv
left a comment
artyomsv
left a comment
There was a problem hiding this comment.
✅ APPROVED
Posted as a comment: GitHub does not let an account approve or request changes on its own pull request. The label carries the verdict.
Reviewed PR #184 at 96289a4.
PR base: ea1288b.
Re-review delta: 735599e..96289a4 (three files). Service/feature: global/m4-verify.
No new findings. Verified and resolved the final open thread, code-quality/M-3: releaseRetired now constructs a run-ID handle when no container is discovered, checks publicationHeld (which includes volumes), and removes the held resources before recording release. The new regression sets container presence false and surviving volumes true, then asserts both deletion and its ordering before release. The prior implementation fails that assertion by making zero removal calls. The other five registered findings were resolved in the preceding review; their implementations were not changed by this delta.
Per-lens result:
- Security: checked run-ID scoping, publication-key validation, and error paths around resource removal. The fix retains requireHeld's ownership check and does not introduce credentials or new logging. No new finding.
- Correctness/design: traced immediate retirement and scheduled recovery through resource discovery, publicationHeld, destroyHeld, and store.released. A removal failure retains the pending flag; resources already gone can complete cleanup. No new finding.
- Rules: applied the previously loaded root CLAUDE.md, user instructions, and matching global clean-code-general.md, clean-code-java.md, observability-and-logging.md, secrets-and-env-handling.md, no-local-builds.md and no-synthetic-data.md. Root instructions are unchanged; no nested CLAUDE.md or repository rules were found. Read the newly added matching .claude/reviews/global/m4-verify.md; recorded resolved items were not re-raised. Existing size debt remains tracked. No new blocking rule finding.
- QA: reviewed the changed regression and shared fixture against existing retirement/publication tests, and checked that the new test distinguishes the pre-fix implementation. CI passed on the exact reviewed head. No local builds, tests or linters were run.
CI evidence: run 38533558507, head 96289a4, completed successfully. Fast tests succeeded in 1m 9s. Service logs show successful test tasks for agent-image, gateway, review-worker, runtime-docker, orchestrator and run-worker, followed by BUILD SUCCESSFUL (14m 41s); packaging succeeded (42s). Dashboard logs report 962 passing tests in 106 files; its TypeScript and build checks also passed. Java aggregate test counts are not exposed in these Gradle logs. All PR checks passed except the intentionally skipped merge job.
CI Semgrep run 38533558727: incremental scan, 97 targets / 372 rules / zero introduced findings. The separate full scan covered 1,405 targets and reported three alerts; those were not independently triaged in this review.
Context retained from prior rounds: the four reviewer checklists and MEMORY.md indexes, monorepo project MEMORY.md, and the code-spire-specific security, tracker-outage, rules-compliance and QA lessons previously consulted. The QA lesson that Gradle logs do not expose aggregate test counts remains applicable.
Residual limits: the added regression simulates surviving volumes in the worker fixture; no live Docker failure injection was performed in this review. Previously recorded low-priority items and broader feature limitations in the review state remain outside this fix delta. No repository files were modified.
Verdict: approve. One thread resolved in this round, six of six registered findings resolved overall, zero new threads.
Agent findings: 6 resolved, 0 open. CI: 17/18 checks passing. Head 96289a4dc3, reviewed by @artyomsv.
artyomsv
added
review: approved
1 participant
M4 slice 1 (verify): check commands in the build setup run on a clean copy of the checkpoint, with no credentials. PASSED delivers; FAILED and UNVERIFIED open a gate with Retry build (from the checkpoint) or Stop.
docs/superpowers/specs/2026-10-07-factory-m4-verify-design.mddocs/superpowers/plans/2026-10-07-factory-m4-verify.mddocs/factory/M4-VERIFY-ACCEPTANCE.md.claude/reviews/global/m4-verify.mdLive on
spire-test: #43 passed, #45 failed then passed on retry, #47 was unverified, and #48 ran from issue to land gate with only write, label and approve. Two branch reviews found fourteen defects; each is fixed with a test.Part of #115