Repository navigation
Conversation
A deploy or backend denial after a repository-credential revision is already active failed the claim without continuation, so cleanup stopped that runtime while the Agent stayed running and maintenance ended. Fail that claim through the same continuation as an exhausted dependency outage. A denial before the active pointer is published still retires the candidate.
|
🦞👀 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
P1
|
Codex review: needs real behavior proof before merge. What this changesThis PR preserves active repository-bearing runtimes after authorization denial while requesting session closure, recording the denial, and scheduling maintenance. Example: An active Agent loses its deploying actor’s Agent deploy grant during repository maintenance.
Review scores
ProductKind: Bug fix · Worth it: Needs a maintainer decision · Fix scope: Complete Merge readiness⛔ Blocked before merge - 5 items remain This PR addresses a source-verifiable runtime teardown problem and resolves the earlier scheduling and audit findings, but its authorization policy still needs an owner decision before merge. Priority: P1 Decision needed
Before merge
Findings
Tests
Agent review detailsHow this fits togetherThe controller worker consumes durable revision claims, rechecks IAM, and coordinates Compute and repository credentials; PostgreSQL records claim outcomes, cleanup obligations, and audit evidence. flowchart TB
Q[PostgreSQL work queue] --> W[Controller worker]
W --> I[IAM Driver]
I --> D[Authorization denial]
D --> C[Repository session cleanup]
D --> A[Claim failure and audit]
A --> Q
W --> R[Compute runtime]
Technical reviewBest possible solution: Preserve active Compute under an explicitly approved denial policy while enforcing the chosen repository-authority boundary through the concrete credential service. Do we have a high-confidence way to reproduce the issue? Current-main worker failure, queue cleanup transfer, and runtime retirement establish the reported path; the author also supplies a before-fix PostgreSQL assertion failure, which this review did not execute. Is this the best way to solve the issue? Reusing bounded continuation is a focused repair, but selecting by reason code also changes behavior for credential and Configuration denials, so the exact authorization policy must be approved. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 07590c64e27a. Merge-risk optionsMaintainer options:
Provenance checked
TestingProof path: in-process harness. SecurityNeeds attention: The authority policy and containment outcome for unconfirmed broker closure remain unresolved. EvidenceSecurity concerns:
What I checked:
Likely related people:
LabelsLabel changes: No label changes. Label justifications:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
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 HistoryReview history (3 earlier review cycles)
Reviewed October 12, 2026, 7:07 PM ET / 23:07 UTC (Revision 4). |
Authorization denial still keeps an already-active repository-credential runtime. A missing session or a changed binding still queues retirement.
After the active pointer is published, AUTHORIZATION_DENIED or ACTOR_REVOKED keeps Compute, closes open repository sessions, records the denial, and schedules maintenance that is not treated as a superseded deployment. A dependency outage does not write that audit. Session-recovery and binding failures still retire the runtime.
|
Follow-up is on What shipped in this patch:
What I cannot decide from the code: The review asked which revoked permissions may leave an active workload running, and whether existing credential sessions must close independently of Compute retirement. This repository does not define a broader permission list for that case. PR #628 keeps the runtime for an exhausted dependency outage. This patch uses that continuation only for Please confirm that those two codes should keep Compute after the revision is already active, or name the codes that must still call @clawsweeper re-review |
|
🦞👀 Re-review progress:
|
|
Product CI on The re-review on that commit says the earlier scheduling and denial-audit findings are addressed. Two choices are still open, and this repository does not record which one to take.
A live Git or repository API exchange on a real host was not run. The PostgreSQL cases use the boundary drivers. The current authorization reference says revocation blocks later admission and does not retract already delivered bytes or stop the Agent by itself. |
|
Free's clanker here (an AI agent working on Free's behalf). Reviewed at Needs action
Logic, checked
Policy (the "Maintainer decision" section) Smaller notes
|
Keep the ComputePreparationFailedError branch from openclaw#1994 ahead of the repository-authority result, then the shared permanent failure list.
Maintenance after a failed deployment continues only for a repository-credential revision. A revision without those credentials still ends superseded. A continued authorization denial logs permanent, matching the failed claim. Restore the controller worker flow's terminal-result and work-attempt sentences. The published deploy denial stays in reconciliation, and the dated note stays in the flow history.
|
Resolved the two items that needed a change, and the maintenance gate. The branch now includes current main. In
A continued authorization denial logs Each later maintenance pass appends another Which |
|
Free's clanker here (an AI agent working on Free's behalf). Re-checked at
The Small nit, not blocking: no test pins the new gate. A PostgreSQL case where an active revision without repository credentials has a CI on From my side this is ready for a maintainer. The IAM question in "Maintainer decision" (which denial codes may keep an already-running workload up) still needs a maintainer's answer. |
freeqaz-openai
left a comment
freeqaz-openai
left a comment
There was a problem hiding this comment.
P2, please describe “closure requested; unconfirmed closure is retried,” not “bearers are closed.” The author’s latest disposition already leaves the unsettled-close policy to the owner; no need to redo the scheduling, audit, or non-repository gate fixes. The owning reconciliation reference, worker history, and the PR’s Risks paragraph currently claim more than the implementation or published-denial test establishes.
I verified the exact worker, lifecycle, and owning test blobs at 051920b4, then checked the actual selected methods and test predicates with invented close outcomes. The lifecycle distinguished confirmed (settled: true), returned-unsettled, and an ordinary close failure (both false); the worker wrapper did not propagate a confirmation boolean. With an explicitly closing record, both test predicates were true in all three cases: “close was called” and “none open/opening.” Negative controls for no close call and a still-open record failed those predicates, and claim loss propagated. This is a useful test of handoff to cleanup, not confirmation at the broker. I did not exercise a real bearer or establish that any post-denial exchange succeeded.
Owner acceptance case: if confirmation is required before keeping Compute, make the returned-unsettled case distinguishable at that decision and prove the selected transport rejection boundary. If keeping Compute while cleanup is pending is intentional, say exactly that, test the unsettled/restart path, and qualify what the broker actually fences. The complete Claw owner and final-effect hold still applies; denial code alone doesn’t select an approved denied operation. No bot rerun, production exchange, or merge requested.
freeqaz-openai
left a comment
freeqaz-openai
left a comment
There was a problem hiding this comment.
Two applyable documentation edits for the close-handoff finding, both checked against the exact changed lines at 051920b4. They describe what the candidate does today; they do not endorse it as the eventual policy. If the owner chooses to require confirmed closure before continuing Compute, document that implemented ordering instead.
Please use the same distinction in the PR evidence summary: “the worker requested closure; the attempt was no longer open or opening” is supported, whereas “sessions were closed” is not established by those predicates. No new full review or bot rerun is needed for these suggestions; the existing authority-scope decision and real broker/transport proof remain open.
The reconciliation reference and worker history said open sessions were closed. The worker asks for closure and continues Compute while the broker outcome is unknown. Published-denial output shows close was called and the attempt was no longer open or opening.
|
Free's clanker here (an AI agent working on Free's behalf). What it does (head Why it needs a human pass: this is IAM policy plus credential custody, not a mechanical change. Earlier review findings (docs, the non-repo gate, the earlier main conflict) were all fixed, and the logic was judged ready for a maintainer at this head on 10-16. Decisions for you (the PR's own "Maintainer decision" section):
CI / conflict state:
This is not a mechanical sync, so we have not pushed a merge. What to validate after a resync:
No new bugs found in this pass. @freeqaz-openai flagging this one for your validation. |
Resync the active-runtime authority fix onto current main. Conflict hunks take main; the preserve-runtime behavior is reapplied on that base.
Main moved the worker catch, credential close, and maintenance expiry since the last review. An authorization denial of an already-running repository revision still requests session closure and keeps Compute. A revision without repository credentials still ends maintenance as REVISION_SUPERSEDED.
|
Resynced onto current main at Credential-proxy close still closes PostgreSQL on this head, Node 24:
The owner questions are unchanged: which of those denial operations may keep Compute, and whether Compute may stay up while broker closure is unconfirmed. |
The resync note put controller-worker.md at 2520 words. The hard limit is 2500.
|
Follow-up |
Problem
After a repository-credential revision is already the active runtime, a later deploy or backend denial failed that claim without continuation. Cleanup then inserted
repository_cleanup:retireand stopped the workload. The Agent stayeddesiredRuntimeState: runningon the same active revision, and maintenance was not scheduled again.PR #628 already keeps that runtime when a dependency outage exhausts retries. A denial before the active pointer is published still retires the candidate, and that behavior stays.
Change
AUTHORIZATION_DENIEDandACTOR_REVOKEDfor a repository-credential revision setpreserveActiveRuntime. When that revision is already the running active revision, including a denial found after the active pointer is published, the worker:repository_cleanup:retireand withoutstopRevisionopeningoropen. After that request, the attempt is no longer open or opening. Session cleanup does not stop Compute. If the broker does not confirm closure, durable cleanup retries it, and Compute can still be running while that outcome is unknown.authorization_denialin the same transaction as the claim failure, with the IAM request and decisionA later maintenance pass is not rejected as
REVISION_SUPERSEDEDwhile that revision is still the running active revision and the reconcile failed with one of those two codes. The pass rechecks IAM before opening credential material.A dependency outage still continues the runtime and does not write an authorization-denial audit or request repository-session closure. A denial before the active pointer is published still retires the candidate. Session recovery and binding changes still retire an active runtime. Revisions without repository credentials are unchanged.
What Problem This Solves
An already-active Agent with repository credentials was stopped by cleanup after its deployer lost
deploy, while the Agent record still said it was running and nothing queued the next maintenance pass. A denial discovered after the pointer was published had the same end state, and the denial audit was dropped.Evidence
Before this change, maintenance after the
deployrestriction recordedAUTHORIZATION_DENIEDand then failed because a retire row existed:After this change, on macOS with Docker PostgreSQL, the controller worker, native IAM, and the boundary repository driver:
Those lines show the worker requested repository closure and that the attempt was no longer open or opening. They do not show that the broker confirmed closure.
Real behavior proof
scripts/ci/prepare.mjs --lane postgres-application, native IAM, and the boundary repository and Compute drivers.scripts/ci/prepare.mjs --lane postgres-application, then ran the worker against that database for the published denial, the maintenance denial, the dependency outage, the pre-publish denial, the missing-session refusal, and the binding-change retirement.AUTHORIZATION_DENIED. The worker requested repository-session closure, and the attempt was no longer open or opening, withoutstopRevision. That does not show the broker confirmed closure. Oneauthorization_denialaudit recorded actiondeploy. The next maintenance pass ran and denied again without a retire row. The outage path wrote no denial audit and did not request closure. The pre-publish denial leftactive_revision_idunset and retired only after the claim wasfailed_permanent.Verification
AUTHORIZATION_DENIEDand then1 !== 0retire rows.pnpm exec eslinton the worker files passed.pnpm exec tsc --build tsconfig.json --pretty falsepassed.python3 .agents/skills/local-dev/scripts/validate_flow_doc.py --kind flow-doc --doc docs/flows/controller-worker.mdpassed.Maintainer decision
This patch keeps Compute only for
AUTHORIZATION_DENIEDandACTOR_REVOKED. On that path the worker requests repository-session closure and leaves the attempt no longer open or opening, without stopping Compute. Unconfirmed closure is retried, and Compute can still be running while that outcome is unknown. It does not choose a broader set of revoked permissions, and it does not decide whether confirmed closure is required before Compute continues. Please confirm that those two codes should leave an already-running workload up, or name the codes that must still retire it.Risks
An Agent whose deployer loses
deploykeeps its current workload until a later stop, delete, or an explicit retirement path. The worker requests repository-session closure on that denial. The attempt is no longer open or opening. Unconfirmed closure is retried, and Compute can still be running while that outcome is unknown. This description is the current handoff, not a decision that an unsettled broker close should leave Compute running. The next maintenance pass does not open new repository credential material while the denial still stands. A denial before publish still stops the candidate. A missing session or a changed repository binding still retires the active runtime. A dependency outage still keeps both the workload and the open session.