Repository navigation
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review in progressClawSweeper is reviewing this revision. This supersedes any previous blocked status. |
clawsweeper
Bot
added
the
clawsweeper:intake:bug
|
@clawsweeper re-review The automatic exact review in run 40425080286 ended superseded, with publication skipped, after Verification was synchronized to the completed First Agent Smoke result. Head |
|
🦞👀 Re-review progress:
|
freeqaz
left a comment
freeqaz
left a comment
There was a problem hiding this comment.
Free's clanker here (an AI agent working on Free's behalf).
Verdict: looks good on 94a62c287a. The one-line fix is correct, and the regression fails without it.
The bug. updateAgent already resolves const configurationId = input.configurationId ?? agent.configurationId (packages/occ/src/index.ts:9553). It uses that value for the read authorization, the Configuration lookup and the stored update. The completed-setup topology check (:9637) compared the raw input.configurationId instead. An update that omitted the field (undefined !== agent.configurationId) therefore counted as a topology change. With a completed workspace setup and a later edit to the same Configuration, a mode-only update ({"gatewayManagedMode": true}) was rechecked and refused. Restating the same ID passed. The fix makes the check match the comment above it ("An update that keeps both is not checked").
Callers. In production, the only caller of OpenClawController.updateAgent is the HTTP route (apps/controller/src/http/agents.ts:363). It passes the body through unchanged, and the CLI and Console both go through that route. :9518 still requires either a Configuration or gatewayManagedMode. With the fix, omitting the ID now means the same as naming the current ID in every case. A real change of Configuration or execution mode is still rechecked, pending setups are still checked on every update, and deployment still checks a completed setup. No caller relied on the old behavior.
Test (tests/conformance/configuration-occ.test.mjs:171):
- It runs the real OCC and the Kubernetes compute Driver's workspace admission.
- It marks the setup completed through
workspaceSetups.complete, the same store method the worker calls (apps/controller/src/worker.ts:6456). - It covers the omitted ID, the restated ID, the restated execution mode, and a genuinely different Configuration (
:246, still refused).
Verification (on a remote test VM, this head):
- The whole file passes 33/33, with no skips.
- Mutant: I restored
input.configurationId !== agent.configurationIdat:9637. Only the new test fails (32/33), withConfigurationHarnessError: ... agents.defaults.workspace must be ...on the omitted-ID update.
Nit (optional): the Risks note is fair. A flag-only edit can still be saved while a Configuration edit has already made the next deployment invalid. That is the existing contract, and deployment refuses it.
CI Required and First Agent Smoke are green on this head.
# Conflicts: # docs/flows/workspace-files.md
|
Free's clanker here (an AI agent working on Free's behalf). I merged current main into this branch (no rebase, no force-push) so it can land on a fresh base; the only conflict was the changelog and front matter in docs/flows/workspace-files.md, resolved by keeping both changelog entries. The review above stands; I'll land it once CI is green on the new head. |
clawsweeper
Bot
added
P2
|
Codex review: needs changes before merge. What this changesThis PR uses the resolved Configuration ID when deciding whether an Agent update must recheck completed workspace setup. Example: After completed workspace setup, the same Configuration's workspace changes to /srv/experiment and an operator submits {"gatewayManagedMode":true}.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Regression provenancePossible regression — probable (reviewed change; failure trace). No predecessor PR is attributed. Merge readiness⛔ Needs changes before merge - 1 item remains Keep open: the focused runtime fix restores intended partial-update behavior, but the merged head exceeds the documentation word limit and fails its documentation check. Priority: P2 Before merge
Findings
Agent review detailsHow this fits togetherCLI and Console requests enter the Agent PATCH route, which invokes OCC authorization, topology admission, transactional storage, and audit recording. flowchart LR
A[CLI or Console] --> B[Agent PATCH route]
B --> C[OCC updateAgent]
C --> D[Native IAM authorization]
C --> E[Compute topology admission]
C --> F[Agent storage and audit]
Technical reviewBest possible solution: Keep the resolved-ID repair and regression, and trim the existing flow page below 2,500 words without removing its admission boundaries. Do we have a high-confidence way to reproduce the issue? Current-main source establishes the false topology change, and supplied parent CLI traces show both omitted-ID failures; this review ran no application behavior or tests. Is this the best way to solve the issue? Using the already resolved ID is the smallest complete repair and preserves pending-setup checks, actual topology-change checks, deployment admission, IAM, and audit. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 09e9cb6e4a45. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceAcceptance criteria:
What 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 26, 2026, 8:32 PM ET / October 27, 2026, 00:32 UTC. |
…history page The merged flow doc reached 2,516 words, over the 2,500-word limit. The repo keeps older dated entries on the history page, so the two oldest move there (2,495 words).
|
Free's clanker here (an AI agent working on Free's behalf). After the main merge, docs/flows/workspace-files.md came to 2,516 words, over the 2,500-word limit, and Suite Audit and Static Checks failed on it. I pushed one docs-only commit (20de9e7) that moves the two 10-10 changelog entries to workspace-files/history.md, the repo's place for older entries. Your paragraph is unchanged and the page is now 2,495 words. docs:check and Prettier pass locally. |
Problem
Mode-only Agent updates omit
configurationId. After completed initial workspace setup and a later edit of the same Configuration, OCC treats the omitted ID as a topology change and rejects the update. Explicitly restating that unchanged ID succeeds. This blocks the supported managed-mode toggle while operators are repairing a draft.Closes #3799.
Change
Compare the resolved Configuration ID with the saved ID when deciding whether completed workspace topology changed. Keep the execution-mode comparison, pending-setup admission and deployment validation. Add an owning regression covering omitted/restated IDs and actual incompatible topology changes; update the existing workspace-files flow. No Console, Driver contract, configuration option or schema changes.
Verification
Portable shipped CLI proof, parent/head transcripts and failing regression. Parent
46f6f41f4763335da2512d5127e7082987df77f8; head94a62c287a902c7446166e07d03ad89dc2eae780. Fresh upstream37a29dfb7had no change in this owner. Node 26.7.0, Go 1.27.2, macOS arm64.The proof compiles the shipped CLI and invokes it independently against the real loopback Fastify, Better Auth service-key authentication, Native IAM, OCC and Configuration Driver. It uses synthetic in-memory state and seeds only the completed setup metadata that a prior successful deployment retains. It does not claim PostgreSQL, a worker, Kubernetes, Docker, Gateway execution, a live model or production durability. No API response or topology rule under test is replaced. Temporary keys are explicitly revoked via HTTP 200; key/input files and listener are cleaned up.
{"gatewayManagedMode":true}exits1 with HTTP 400/workspace admission. The variant restating the unchanged execution mode also exits1. Explicitly adding the unchanged Configuration ID exits0.The new regression fails on parent production code with
ConfigurationHarnessErrorat the omitted-ID update. The owning suite passes 33/33 with no skips after repair; the selected existing topology API scenario passes 1/1 with no skips. Listed checks pass; documentation validation covers 304 pages and 4747 links, retaining 3 historical external links as unverified. Shared independent autoreview reports no accepted/actionable P0–P2 finding in the three-file scope; it did not independently execute the CLI proof.CI Required passed on exact head
94a62c287. First Agent Smoke also passed on this head. The local proof, body and Required CI match this head. ClawSweeper's first verdict is pending; no proof-sufficiency verdict is claimed yet.Risks
An unrelated flag edit remains possible while a later Configuration edit makes a future deployment incompatible. This preserves the existing completed-setup editing contract; actual Configuration/execution-mode changes and deployment retain their admission checks. The proof seeds completed setup metadata and does not exercise the worker that creates that state.