Repository navigation
Conversation
finalizeRevision checks repository authority again before it commits the active pointer. When that check failed with anything other than a repository authority refusal (for example an IAM outage), the pass retried with DEPENDENCY_UNAVAILABLE but dropped the error, so its worker.completed line carried no cause, on a plain retry or an exhausted one. Keep the failure fields from the caught error, as a failure in dispatch does. A teardown test makes IAM fail only after preparation's own check and asserts the logged cause (finding 2272).
Count the beforeCommit activations so the case fails clearly if the pass stops activating before the pointer commit (review nit).
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
clawsweeper:intake:bug
|
Codex review: needs maintainer review before merge. What this changesThis PR preserves existing failure diagnostics when a repository authority re-check encounters a dependency failure. Example: IAM throws Error during the repository authority re-check before the active revision pointer commits.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep this PR open: it repairs a documented diagnostic omission with a focused change and no introduced correctness or security defect found. Priority: P2 Before mergeNone. FindingsNone. Tests
Agent review detailsHow this fits togetherThe controller worker consumes durable PostgreSQL revision work, checks authority through selected Drivers, updates lifecycle state, and emits structured completion logs. flowchart TB
A[PostgreSQL revision work] --> B[Controller worker]
B --> C[Revision preparation]
C --> D[Repository authority re-check]
D --> E[IAM Driver]
E --> F[Failure classification]
F --> G[Structured completion log]
Technical reviewBest possible solution: Keep the existing authority and retry decisions and attach diagnostics through the worker’s established failure classifier. Do we have a high-confidence way to reproduce the issue? Current main visibly discards the caught error at this re-check; the PR reports the new assertion failing on the base and passing with the repair, but this review did not execute tests. Is this the best way to solve the issue? Using the existing failure classifier at the catch that loses the error is the smallest complete repair and preserves lifecycle behavior. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 2c8835276fe0. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 23, 2026, 4:50 PM ET / 20:50 UTC. |
Problem
Before it commits the active pointer,
finalizeRevisionchecks a repository revision's authority again. When that check fails with anything other than a repository authority refusal (an IAM outage, say), the pass retries withDEPENDENCY_UNAVAILABLEbut drops the error. Itsworker.completedline then has nocause, on a plain retry and on an exhausted one.docs/reference/controller.mdsays a failed or deferred version pass addscause, and a failure caught during dispatch already does. #3352 made the exhausted path log the failure fields; this is the same gap one step earlier.Change
apps/controller/src/worker.ts: the retry branch of that catch now keepsrevisionFailureLogFields(error), the same fields a failure in dispatch logs. The permanentRepositoryCredentialAuthorityErrorbranch is unchanged: its code already names the cause. No new log field.tests/integration/postgres-worker-agent-revision-teardown.test.mjs: a new case makes IAM fail only after preparation's own authority check (anactivateRevisionhook withbeforeCommitactivation), so only the re-check before the commit fails. It asserts one activation, thenDEPENDENCY_UNAVAILABLEandcause: "Error"on the logged retry.No other catch in
finalizeRevisiondrops an error this way: the post-activation catches already pass failure fields, the stopped-candidate catch rethrows everything except a yielded stop, and the exposure-withdrawal catch is best effort after the outcome is committed.Verification
causeisundefined); with the fix it passes.tsc --build tsconfig.json tests/tsconfig.json, eslint and prettier are clean on the touched files.Risks
Low. Log-only: the outcome, code and queue state are unchanged. One more retry log line carries the existing failure fields.