🤖 feat: opt new user-created workspaces in to unrelated messaging by default - #4440
Conversation
This comment has been minimized.
This comment has been minimized.
|
@codex review |
|
@codex security review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
🛡️ Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 92e8be31c6
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
|
@codex review |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ae0703f92
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a103c800f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Pausing this PR for an owner decision. It is not ready, and it will not be merged in its current state. Why: the review loop is not converging. The review budget is used up (three Codex code+security pairs = six reviews). Each fix to when the default consent becomes visible has opened another window where a freshly created workspace is discoverable before its creator finishes setting it up:
Proposed scope reduction (needs owner acceptance, because it narrows "all new workspaces"):
The three round-3 threads stay open until that decision is made. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9836ac075e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df1bf424e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60a984c0b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b88b06780
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Halting this PR without merging. The review budget is used up and the review loop is not converging. State at
Open defects introduced by this PR
Proposed next step |
|
Correction to the halt record: CI on |
…erred checkouts after sanitization; restore post-write re-read
ThomasK33
changed the title
|
Resuming with a reduced scope, approved by the owner ("take a safer/progressive path"). Scope now: default consent for workspaces a user creates (new, scratch, multi-project, fork). Delegated What changed in the design
Review budget: the original six rounds are used up. I am declaring a bounded extension of 2 rounds for this re-scoped diff: one fix round (blockers only) and one verification round. If the second round finds any new blocker, I stop without merging. |
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Hiding the Codex review summary: Codex approved head |
…-default UI test A second manual parent send raced the first child's report wake-up turn after #4440/#4449 and was intermittently never dispatched (7/10 local runs), which dequeued the PR. One parent step now reawakens both. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
…exit (coder#4814) ## Summary `create()` and `fork()` now clear the pending default-consent mark on every exit that did not hand it to a retained owner. That covers an Err after registration, and a deferred checkout that is skipped because the workspace is being removed. No surviving row keeps `unrelatedWorkspaceConsentPending` after its creation has settled. A new multi-project workspace also refuses a project named `.xum`/`.mux`. That is the only way its always-fresh container root could hold an override file, so multi-project needs no registration-time sanitization. ## Background These are follow-ups for coder#4440's default consent, as tracked in coder#4455. coder#4783 replaced the in-memory pending set with a durable row mark, which made item 1 durable: - `create()` returns Err after registering the row. Examples are the `!completeMetadata` return, or a throw from `secretsToRecord` before the deferred checkout's settlement is retained. The kept row keeps the mark. - The removing/aborted `else` branch skips the deferred checkout, so nothing clears the mark. - `fork()` has the same class of leak: `inheritFromFork` can throw after its row is registered. A leftover mark is inert today, but coder#4453 (next in this cluster) turns the mark into a deferred grant point, so every creation path must finalize it. This is PR 1 of the approved consent-cluster plan: coder#4455 → coder#4305 → coder#4453. ## Implementation - **One finalization per creation path, not one patch per exit.** That per-exit patching is what did not converge in coder#4440. - `create()` gets a `finally` that calls `clearPendingDefaultUnrelatedConsent` unless the deferred checkout's settlement took ownership of the mark. That settlement already grants or clears it. - `fork()` gets a `finally` that clears the mark once the fork's ID exists. - After a successful grant, both are cheap no-ops, because the snapshot check finds no mark. - **Item 2 (a failed deferred checkout stays off)** is intentional and stays fail-closed. A comment at the branch now says so. - **Item 3 (multi-project sanitization)** is not needed: - Overrides are read only from `<container>/{.xum,.mux}/mcp.local.jsonc`. - `ContainerManager.createContainer` refuses to reuse a directory (non-recursive `mkdir`), so the container holds only project symlinks. - The one exception is a project whose folder is named `.xum`/`.mux`. Its link would alias the overrides path into that project's checkout, and the launch-time read (`readOverridesFile`) follows symlinks. `createMultiProject` now refuses such a project with a clear error, which registration-time sanitization would also do, since the prune refuses symlinked paths. - Existing workspaces and rename/restore paths are unchanged. - **Item 4:** an uncontended `create()`-level test covers the deferred-checkout lifecycle. ## Validation New tests use a real `Config` + `WorkspaceService`: - A deferred-checkout `create()` whose secrets read throws after registration → Err, and no mark on the kept row. Fails on `main` (mark `true`). - A `fork()` whose `inheritFromFork` throws → Err, and no mark on the kept row. Fails on `main`. - `createMultiProject` with a `.mux` project → refused before any runtime is created. Fails on `main`. - Item 4: `create()` announces a deferred checkout with the mark set and no consent, then settles it into a valid generation with no mark. `workspaceService*`, `config`, and `multiProject` suites: 940 pass, 0 fail (Bun 1.3.5). `make static-check` is green. ## Risks Low. The change is confined to default-consent bookkeeping and fails closed: the new code only ever clears a mark, never grants one. The multi-project refusal affects only new workspaces that include a project folder literally named `.xum`/`.mux`. Follow-up (out of scope, filed separately): a `create()`/`fork()` that returns Err after registration still keeps the registered row. This PR only fixes the mark. Refs coder#4455, coder#4440, coder#4783 --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$12.63`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=12.63 -->
…messaging when their creating turn settles (coder#4838) ## Summary Delegated `task(kind:"workspace")` targets are now opted in to unrelated messaging, once, when their creating turn settles in the process that created them. Every other outcome clears the pending mark (fail closed). An explicit toggle always wins, mode `"existing"` never re-grants, and disposable targets never get a mark. A startup resolver clears marks whose creator died. Fixes coder#4453 ## Background coder#4440 tried to default-grant at creation and never converged: each review round found another post-creation window. This is PR3 of the consent-cluster plan. It builds on: - coder#4783: the durable pending mark (`unrelatedWorkspaceConsentPending`); the grant is a config CAS that requires it, and a toggle deletes it. - coder#4772: the per-handle live-owner lock; every handle write passes `OwnedWorkspaceTurnHandleStore.afterUpsert`, the single terminal-write chokepoint. - coder#4814: `create()`'s single `finally` that clears the mark on exits it did not hand off. ## Implementation 1. `WorkspaceService.create` option `defaultUnrelatedConsent: "after-setup" | "caller-finalizes" | "none"` replaces `skipDefaultUnrelatedWorkspaceConsent`. `"caller-finalizes"` writes the mark, never grants, and hands the mark to the caller only on success. It asserts `awaitMaterialization` (a deferred checkout would grant from `materializeDeferredCheckout`). 2. The shared grant transform clears instead of granting when the row has `pendingRemoval`. 3. `WorkspaceTurnManager`, mode `"new"`: - takes the handle's live-owner lock **before** `create()`; - adds the handle to an in-memory `creationConsentFinalizers` Set after `create()` returns Ok (not for disposable targets); - one `await using` finalizer covers every exit before the handle record persists: drop from the Set, clear the mark, release the lock (this subsumes the old release on the admission-error path). 4. `afterHandleWrite` (the store callback): on a terminal write of a `createdWorkspace && !disposableWorkspace` record, `Set.delete` → grant, otherwise clear; then release the lock in a `finally`. 5. Startup resolver (last commit, droppable): `clearOrphanedDelegatedConsentDefaults()` from `TaskService` startup. For each marked row tagged `mux.taskHandleId`, under the handle's settlement lock: skip handles this manager holds or is creating; require evidence the handle is real (its live-owner lock file exists, or a handle record that created this target), since tags are caller-supplied; then try-lock the handle and clear only if the lock was free or its owner is dead. It never grants. 6. Copy: tool descriptions, `task_list`, the schema description, the modal, and regenerated `docs/hooks/tools.mdx` and built-in skill content. The grant publishes metadata through synchronous emitters only, so it cannot re-enter `workspaceTurnSettlementLocks`. ### Transition table (also a code comment at `afterHandleWrite`) | Event (any backend) | Row before | Action | Row after | | --- | --- | --- | --- | | Mode `"new"`, not disposable: lock H taken, then `create()` writes the row | none | registration write includes the mark and tag H | mark set, no consent | | `create()` returns Err | mark set | rollback removes the row, or `create()`'s `finally` clears the mark | gone, or no mark | | Exit before the handle record persists (invalid AI setting, reawaken change, admission error, target/owner archived) | mark set | the creation finalizer clears the mark, drops the Set entry, releases H | no mark, off | | First handle write (reservation) | mark set | none: consent is off, so every backend answers `not_found` and discovery hides the target | mark set | | Explicit toggle during the turn (any backend) | mark set | the toggle deletes the mark in the same CAS | toggle value, no mark | | First terminal write of the creating handle in the creating process (completed, error, Stop, interrupt, validation or admission error, in-process stale) | mark set | `Set.delete` → grant, before H is released | valid generation, no mark | | Same write, mark already gone | no mark | CAS no-op | unchanged: toggle wins | | Any other terminal write: another backend, a restarted process, stale settlement, later rewrites | any | clear (snapshot check: no write when the mark is gone) | no mark | | The grant's config write fails | mark set | logged; the resolver clears it at the next startup | off | | Crash between `create()` and the record, or between the terminal write and the grant | mark set, H dead | startup resolver: try-lock H → clear | no mark, off | | Crash while the turn is live | mark set | lazy stale settlement (lock taken) clears; the resolver also clears | no mark, off | | Mode `"existing"` follow-up settles | no mark | the record has `createdWorkspace:false`, so no hook | unchanged: never re-grants | | Disposable target | none | `create()` option `"none"`: no mark, ever | off | | Row pending removal at grant time | mark set | the grant transform clears the mark instead | no mark | | Downgrade to a pre-coder#4783 or coder#4783-era build, then upgrade | mark set | the old build never grants from it; the new build clears it at startup or stale settlement | whatever the user set | Consent means reachability, not admission: the grant runs after the terminal write and before the reservation is dropped, and in-process admission still refuses while the reservation exists. Across backends, admission is the documented courtesy (coder#4801 §4), the same as for user-created workspaces. ## Validation `workspaceTurnManager.delegatedConsent.test.ts` uses real `WorkspaceTurnManager` + real `WorkspaceService` (create, grant, clear, toggle) per backend, each backend on its own `Config` for one root, with promise barriers (no sleeps): - unreachable during the creating turn, opted in after completed / error / interrupt; disposable never marked; - a toggle-off during the turn wins, from this backend and from another backend; an opt-in keeps its generation; mode `"existing"` after an opt-out never re-grants; - another backend's stale settlement of a dead creator clears and never grants; - exits before the record persists (invalid explicit model, target archived during creation) clear the mark and release the lock file; - a failed grant write still settles the turn and leaves the mark; the next startup's resolver clears it; - the resolver ignores a caller-supplied `mux.taskHandleId` tag with no lock file or record; - resolver with a live creator paused after `create()` (own and other backend both skip; the later settlement grants) and with a dead creator (cleared; the later settlement stays off). Mutation checks: removing the finalizer's clear, the `afterHandleWrite` clear, the grant's mark check, the resolver's own-handle skip, or its live-lock check each fails at least one test. The file passed 5/5 runs under a concurrent `taskService` suite load. Related suites (`workspaceTurnManager*`, `workspaceService*`, `taskService*`, `config`, `task_send_message`, `task_list`) pass. ## Risks Medium, confined to delegated-target consent. The failure direction is "stays off" everywhere except the creating process's first terminal write. The live-owner lock is now held during `create()`; nobody contends for a fresh handle ID, stale settlement ignores targets without a record, and the resolver skips live holders. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high -->
1 participant
Summary
Root workspaces that a user creates (new, scratch, multi-project, fork) are now opted in to unrelated (cross-task-tree) messaging by default, so agents in other task trees can reach them with
task_send_messagewithout a manual toggle. Existing workspaces are unchanged, sub-agents stay off, and the per-workspace toggle still turns it off permanently. Delegatedtask(kind: "workspace")targets follow in #4453.Background
Cross-tree messaging requires recipient consent (
unrelatedWorkspaceConsent). It was off by default, and the refusal is reported asnot_foundto hide the target's existence, so new workspaces looked missing to other agents.This PR is delivered progressively. Granting consent to delegated targets at creation opened a new timing window with every fix (see the halt comments below), so that part moved to its own designed follow-up (#4453).
Implementation
mintUnrelatedWorkspaceConsent()mints the generation and asserts that it passes the fail-closed reader. The existing toggle uses the same helper.task_list(scope:"instance")and admission:createScratch()/createMultiProject(): no setup steps; written atomically with the entry.create()with an immediate checkout: after registration-time plugin-override sanitization, before announcing.create()with a deferred checkout: aftermaterializeDeferredCheckouthas populated and sanitized the checkout, before the init hook. It does not rely onwaitForInit, which a second backend sharing the root does not observe. An explicit toggle made between the announcement and the grant cancels it. Removal, a failed checkout or a failed sanitization never grant.fork(): last, after sanitization, goal inheritance and the pending branch-summary marker. It gets its own generation, never the source's.WorkspaceTurnManagerpassesskipDefaultUnrelatedWorkspaceConsent(🤖 feat: opt delegated task(kind:workspace) targets in to unrelated messaging by default #4453).grantCreationUnrelatedWorkspaceConsentre-reads config after its write and reports only the persisted value.Config.saveConfigswallows write failures (🤖 fix: config writes swallow save failures while the in-process snapshot keeps the edit #4444), and editConfig's transform runs on an uncached read, so a re-read does detect them.task_send_messagedescription andtask_idparameter,task_listinstance-scope description, schema description. Generated docs are refreshed.Validation
New and updated tests. Each fails on the previous behavior or under a targeted mutation, and passes now:
create(): persists a valid generation, with none while registration sanitization runs; withskipDefaultUnrelatedWorkspaceConsent, nothing is persisted, announced or pending.materializeDeferredCheckouthas no consent during sanitization, grants before the init hook and publishes; failed sanitization and failed checkout never grant.createWorkspaceTurncreates delegated targets with the skip flag.Local gates ran with the repo-pinned Bun 1.3.5 (the PATH default here is 1.2.15, which produced unrelated local failures).
Risks
Review history
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$34.04