Skip to content

🤖 feat: restore task_workspace_lifecycle with archive/unarchive for peer workspaces - #3940

Open
ThomasK33 wants to merge 22 commits into
mainfrom
archive-8m4t
Open

🤖 feat: restore task_workspace_lifecycle with archive/unarchive for peer workspaces#3940
ThomasK33 wants to merge 22 commits into
mainfrom
archive-8m4t

Conversation

@ThomasK33

Copy link
Copy Markdown
Member

Summary

Restores the task_workspace_lifecycle tool — removed from the executable toolset by #3825 as collateral of the sub-agent lifecycle consolidation — trimmed to the two reversible verbs archive and unarchive, scoped strictly to workspace-turn peer workspaces the calling workspace created via task(kind="workspace").

Background

Orchestrating agents can create peer workspaces with task({ kind: "workspace" }) but had no tool to archive them, so loop skills (e.g. issue-triage-loop) that still instruct agents to call task_workspace_lifecycle accumulated un-archivable peer workspaces in the sidebar. The Zod result schemas and the transcript renderer (WorkspaceLifecycleToolCall) survived the removal, so this is mostly a resurrection from 88580ca7d^unarchive is the only genuinely new backend surface (the historical tool never implemented it).

Implementation

  • Narrowed live input schema (TaskWorkspaceLifecycleToolInputSchema): only archive/unarchive, no force. The historical 4-action args schema stays untouched so old transcripts still parse; delete_worktree/remove are not model-invocable — task_remove remains the sole irreversible verb.
  • Backend (taskService.ts): archiveOwnedWorkspaceTurnWorkspace + helpers restored verbatim from 88580ca7d^; new unarchiveOwnedWorkspaceTurnWorkspace mirrors it with interruption hard-disabled (defense-in-depth: an archived workspace should never have active turns; if a race surfaces one, unarchive reports active instead of interrupting).
  • Authorization uses durable workspace-turn ownership records (taskHandleStore.isWorkspaceOwnedBy) as the sole source of truth; workspace config tags are hints only.
  • Lock layering: a dedicated per-target workspaceLifecycleLocks MutexMap wraps the flow; workspaceService.archive internally takes the task-tree lifecycle lock for the same key, so an invariant comment forbids calling these helpers while holding withTaskTreeLifecycleLock (same-key non-reentrant acquisition would deadlock).
  • Registration: tool factory, baseTools, PTC bridging (BridgeableToolName + RESULT_SCHEMAS), PRESERVE_OUTPUT_TOOLS for shared transcripts; removed from explore/plan/desktop agent allowlists (+ regenerated builtin agent/skill/docs artifacts). Zero frontend changes.

Validation

Live dogfooding in a dev-server-sandbox instance (screenshots/video in the workspace transcript):

FlowResult
task(kind="workspace")archivearchived; peer hidden from sidebar, listed under project-page Archived Workspaces
Archive a non-owned workspaceinvalid_scope
unarchive by wst_ handleunarchived; peer restored to sidebar
task(mode:"existing") follow-up post-unarchivesucceeds (refused while archived)
Untracked file in worktree → archiverequires_confirmation with paths → re-call with acknowledged_untracked_pathsarchived

Targeted suites: tool layer (6), taskService lifecycle (9, incl. concurrent-handle serialization and the archive→refusal→unarchive round-trip), schema gate rejecting remove/delete_worktree/force, renderer UI tests (21). The 3 pre-existing taskService.test.ts failures reproduce identically on a clean-HEAD probe worktree.

Risks

Low-to-moderate, contained to agent-driven workspace lifecycle: the archive path re-reads metadata under the per-target lock (idempotency covered by tests), and misuse surfaces as safe statuses (invalid_scope/active/requires_confirmation) rather than destructive actions. The riskiest surface — irreversible removal — is deliberately not restored.


📋 Implementation Plan

Restore task_workspace_lifecycle: agent-driven archive/unarchive of workspace-turn peer workspaces

Problem

An orchestrating agent can create peer workspaces with task({ kind: "workspace" }) (workspace turns), but has no tool to archive them. The purpose-built tool task_workspace_lifecycle (added in PR #3633) was removed from the executable toolset by PR #3825 (88580ca7d, "simplify persistent sub-agent lifecycle") as collateral of the sub-agent lifecycle consolidation. Loop skills (e.g. issue-triage-loop) still instruct agents to call it, and orchestrator workflows accumulate un-archivable peer workspaces in the sidebar.

Design decision

Restore task_workspace_lifecycle as an executable tool, trimmed to archive + unarchive, scoped strictly to workspace-turn targets owned by the calling workspace. Do not overload task_remove (its model-facing contract is "irreversible removal of inactive sub-agents"; archive is reversible — mixing them invites destructive model mistakes and bloats a clean result schema).

Key facts making this cheap (verified on HEAD and at 88580ca7d^):

  • Ownership tracking already exists: taskHandleStore.isWorkspaceOwnedBy(ownerWorkspaceId, workspaceId) (HEAD src/node/services/taskHandleStore.ts:247) checks createdWorkspace flags on durable workspace-turn handle records stored under the owner's session dir (never pruned, so ownership cannot silently expire). Authorization must use these handle records as the sole source of truth. The mux.taskOwnerWorkspaceId / mux.taskHandleId / mux.taskTurnId tags stamped on created workspaces (src/constants/workspaceTags.ts) are correlation/recovery/UI hints only — config metadata is not an authorization surface.
  • The Zod schemas survive on HEAD (src/common/utils/tools/toolDefinitions.ts ~1221: TaskWorkspaceLifecycle*), including unarchive action + unarchived/already_unarchived statuses.
  • The transcript renderer survives on HEAD (src/browser/features/Tools/WorkspaceLifecycleToolCall.tsx, registered in getToolComponent.ts:227, icon in ToolPrimitives.tsx:288) → zero frontend work.
  • The deleted backend (archiveOwnedWorkspaceTurnWorkspace, resolveOwnedWorkspaceLifecycleTarget, helpers) and tool file (134 lines) + test file (142 lines) are recoverable via git show '88580ca7d^:<path>'.
  • workspaceService.archive(workspaceId, acknowledgedUntrackedPaths?) (HEAD workspaceService.ts:7237) already handles lifecycle locking, init-abort, active-descendant refusal, and the lossy-snapshot untracked-file confirmation; workspaceService.unarchive exists (exposed via ORPC workspace.unarchive, router.ts:4516).
  • Historical gap: the pre-removal action enum was only ["archive","delete_worktree","remove"]unarchive never had an implementation; the schema on HEAD is already extended for it. unarchiveOwnedWorkspaceTurnWorkspace is the only genuinely new backend code.

Alternatives considered (rejected)

ApproachWhy rejectedNet LoC
Extend task_remove with an action/archive modeSemantic mismatch (irreversible vs reversible), result-schema bloat (requires_confirmation, already_archived), re-muddies #3825's separation of execution state vs retention~+120
New task_archive tool nameRequires new renderer + registration wiring; breaks issue-triage-loop skill compat; historical transcripts diverge from live tool~+260
Out-of-band xum api workspace archive via bashRequires reachable server + auth token inside worktree workspaces (typically absent); no ownership scoping — any workspace could archive anything; no transcript card0 (unsupported)
Restore trimmed task_workspace_lifecycle (chosen)Mostly resurrection; schemas + renderer already on HEAD; skill-compatible~+320 product code (plus ~200 test)

Scope

In scope:

  • archive and unarchive actions for workspace-turn targets (wst_* handle IDs or owned peer workspaceIds).
  • Ownership enforcement, active-turn handling (interrupt_active), untracked-file confirmation round-trip.
  • Tool registration (definitions, factory map, availability lists), tests, dogfooding.

Out of scope (explicitly not restored):

  • remove / delete_worktree actions — task_remove stays the only irreversible verb; these can be resurrected later by the same recipe.
  • Archive of sub-agent (agent-task) children — 🤖 feat: simplify persistent sub-agent lifecycle #3825 deliberately replaced that with the active→inactive→removed lifecycle; non-wst_ task IDs stay invalid_scope.
  • Auto-unarchive-on-send for workspace turns (explicit unarchive instead; createWorkspaceTurn mode "existing" keeps refusing archived targets).
  • UI changes.

Implementation

Recovery source for all restored code: git show '88580ca7d^:<path>'.

Phase 1 — Schema & tool definition (src/common/utils/tools/toolDefinitions.ts)

  1. Add a narrowed live-input schema next to the existing kept schemas (~line 1249). Do not modify TaskWorkspaceLifecycleActionSchema (4 actions) or TaskWorkspaceLifecycleToolArgsSchema — both are still referenced by the result schema and the renderer's parsing of historical transcripts (getToolComponent.ts:229, src/common/types/tools.ts:309):

    exportconstTaskWorkspaceLifecycleToolInputSchema=z.object({action: z.enum(["archive","unarchive"]).describe(/* reversible archive/unarchive only */),targets: z.array(TaskWorkspaceLifecycleTargetSchema).min(1).describe(/* wst_* taskId or owned workspaceId */),interrupt_active: z.boolean().nullish().describe(/* archive only */),acknowledged_untracked_paths: z.record(z.string(),z.array(z.string())).nullish(),}).strict();

    Reuses TaskWorkspaceLifecycleTargetSchema (exactly-one-of taskId/workspaceId superRefine). Drops force (only applied to the un-restored remove action). All optional params .nullish() per repo tool-schema convention.

    Split rationale: the broad historical args schema + TaskWorkspaceLifecycleToolArgs type (src/common/types/tools.ts:309) stay untouched for historical-transcript parsing and renderer compatibility; the narrowed input schema is what TOOL_DEFINITIONS.task_workspace_lifecycle.schema advertises to models. Export a live input type in types/tools.ts only if the tool file needs it — do not repoint the existing type.

  2. Add the TOOL_DEFINITIONS entry (~line 2286, adjacent to task_remove), description rewritten for the trimmed contract:

    • reversible archive/unarchive of full workspaces this workspace created via task(kind="workspace");
    • scoped by durable workspace-turn ownership records — cannot act on arbitrary user workspaces or sub-agent children;
    • active workspace turns are refused unless interrupt_active: true;
    • archive may return requires_confirmation with untracked paths → re-call with acknowledged_untracked_paths;
    • archived targets refuse task(kind="workspace", mode="existing") follow-ups until unarchived.
  3. Register availability: add "task_workspace_lifecycle" to the baseTools array in getAvailableTools (~line 3349, next to task_remove).

  4. PTC bridging parity (2 lines): add "task_workspace_lifecycle" to the BridgeableToolName union (~3198) and RESULT_SCHEMAS (~3228, → TaskWorkspaceLifecycleToolResultSchema) so RLM/PTC sandbox sessions get the same task-tool surface as task_remove.

Phase 2 — Backend restoration (src/node/services/taskService.ts)

No name collisions on HEAD; coerceNonEmptyString, WORKSPACE_TURN_TASK_TAGS, isWorkspaceArchived, TaskWorkspaceLifecycleToolTargetResultSchema import, and the WorkspaceLifecycleResult type alias (line 213) are already present.

Restore verbatim-then-adapt from 88580ca7d^:src/node/services/taskService.ts:

  1. private readonly workspaceLifecycleLocks = new MutexMap<string>() (historical line 1200) — a dedicated per-target-workspace lock outsideworkspaceService.archive. Do not wrap the call in withTaskTreeLifecycleLock externally: archive already takes that lock internally for the same key, and a non-reentrant same-key acquisition would deadlock. Implementation invariant (enforce via code comment on the lock field): no code path may acquire the task-tree lifecycle lock and then call these workspace-lifecycle helpers.
  2. resolveOwnedWorkspaceLifecycleTarget(ownerWorkspaceId, action, target) (historical 7401) — resolves wst_* handle → workspaceId via taskHandleStore.getWorkspaceTurn (HEAD :173), then gates on taskHandleStore.isWorkspaceOwnedBy (HEAD :247; records are never pruned, so ownership cannot silently expire); non-wst_ taskIds and non-owned workspaceIds → invalid_scope.
  3. withWorkspaceLifecycleLock (historical 7388), lifecycleTargetFields (historical ~7454), findWorkspaceLifecycleMetadata (historical ~7473).
  4. handleActiveWorkspaceLifecycleTurns (historical 7495) — both callees survive on HEAD: listWorkspaceTurnTasks (:8496) and interruptWorkspaceTurn (:8512). Filters owner's turns with status queued|starting|running targeting the resolved workspace; returns active + activeTaskIds unless interruptActive, in which case it interrupts each turn.
  5. archiveOwnedWorkspaceTurnWorkspace(ownerWorkspaceId, target, options) (historical 7208) — under the lifecycle lock: not_found (metadata absent) / already_archived (idempotent) / active-turn handling / workspaceService.archive(workspaceId, acknowledgedUntrackedPaths); maps kind: "confirm-lossy-untracked-files"requires_confirmation + paths. workspaceService.archive errors surface as status: "error" (includes the active-descendant-sub-agent refusal, ACTIVE_DESCENDANT_ARCHIVE_ERROR).
  6. NewunarchiveOwnedWorkspaceTurnWorkspace(ownerWorkspaceId, target) (~30 lines, mirrors archive): resolve + lock → not_found / already_unarchived (via isWorkspaceArchived) → workspaceService.unarchive(workspaceId) (HEAD :7514, Result<void>) → unarchived | error. interrupt_active applies to archive only. An archived workspace should never have active turns (archive refuses while active; createWorkspaceTurn refuses archived targets), but as defense-in-depth unarchive still runs the active-turn check with interruption hard-disabled: if a race/corruption surfaces an active turn, return active — never interrupt on unarchive, even if the caller passed interrupt_active: true. Document this in the tool description.

Phase 3 — Tool file + registration

  1. Restore src/node/services/tools/task_workspace_lifecycle.ts (134 lines historical → ~100 trimmed): keep normalizeTarget/targetKey dedup and rejectInvalidWorkspaceTaskId (non-wst_invalid_scope); dispatch only archivearchiveOwnedWorkspaceTurnWorkspace and unarchiveunarchiveOwnedWorkspaceTurnWorkspace; keep the planFileOnly throw (same pattern as task_remove.ts:17); input schema = the new narrowed TaskWorkspaceLifecycleToolInputSchema; results parsed against the kept TaskWorkspaceLifecycleToolResultSchema.
  2. src/common/utils/tools/tools.ts: import createTaskWorkspaceLifecycleTool (~line 42) and register task_workspace_lifecycle: wrap(createTaskWorkspaceLifecycleTool(config)) (~line 773).
  3. Agent allowlists (src/node/builtinAgents/): add task_workspace_lifecycle to tools.remove in explore.md (line ~30, alongside task_remove), plan.md (line ~23), and desktop.md (line ~43) so read-only/plan/desktop agents can't mutate workspace lifecycle. Regeneration is automatic: the Make rule $(BUILTIN_AGENTS_GENERATED): src/node/builtinAgents/*.md scripts/generate-builtin-agents.sh (Makefile:246) rebuilds builtInAgentContent.generated.ts, and make typecheck (part of static-check) lists it as a prerequisite — so a stale generated file cannot pass validation. Commit the regenerated file. No changes to the policy engine — it filters dynamically by name.
  4. src/common/utils/messages/transcriptShare.ts (~line 133): add "task_workspace_lifecycle" to PRESERVE_OUTPUT_TOOLS so shared transcripts keep target-status rows.
  5. src/browser/features/Settings/Sections/TasksSection.agents.ts (~line 62): add to the agent-template remove arrays where task_remove appears, for template parity.
  6. Frontend: zero changes — WorkspaceLifecycleToolCall.tsx already renders all statuses including unarchived/already_unarchived (exhaustive Record<TaskWorkspaceLifecycleStatus, StatusMeta>, lines 101–125) with existing UI tests.

Phase 4 — Tests (behavioral, no tautologies)

Restore + adapt src/node/services/tools/task_workspace_lifecycle.test.ts (historical file; all utilities — TestTempDir, createTestToolConfig, mockToolCallOptions — survive on HEAD in tools/testHelpers.ts):

  • Tool layer: forwards owner/target/options to the scoped taskService API; dedupes duplicate targets; non-wst_ taskId → invalid_scope without touching taskService; planFileOnly throws; unarchive routes to the unarchive method (replaces the historical delete_worktree/remove routing test); acknowledged_untracked_paths forwarding when the target is specified by taskId (not workspaceId) — the tool must pass the full by-workspaceId map so the backend can apply it after handle→workspaceId resolution.
  • taskService layer (in taskService.test.ts, alongside existing workspace-turn tests):
    • non-owned workspaceId → invalid_scope; owned (createdWorkspace record) → proceeds.
    • archive idempotency (already_archived) and unarchive idempotency (already_unarchived).
    • active turn → active + activeTaskIds; with interrupt_active: true → interrupts then archives.
    • workspaceService.archive returning confirm-lossy-untracked-filesrequires_confirmation with paths; second call with acknowledged_untracked_pathsarchived.
    • archive → createWorkspaceTurn(mode: "existing") refused; after unarchive → succeeds again (the round-trip that motivates unarchive).
    • unarchive addressed by wst_* taskId and by workspaceId (both resolution paths).
    • unarchive never interrupts: with a (synthetic) active turn present, unarchive returns active even when interrupt_active: true.
  • Schema tests (toolDefinitions.test.ts): live input schema rejects remove/delete_worktree actions and rejects force (behavioral gate: irreversible verbs and their escape hatch must not be model-invocable through this tool). Built-in agent remove-lists are covered by codegen + typecheck staleness (above); add an agent-definition test only if an existing suite already asserts remove-lists for task_remove (follow precedent, don't invent a new tautology).

Phase 5 — Validation & dogfooding gate

  1. make static-check + targeted suites: bun test src/node/services/tools/task_workspace_lifecycle.test.ts src/node/services/taskService.test.ts src/common/utils/tools/toolDefinitions.test.ts and the renderer UI test (WorkspaceLifecycleToolCall.ui.test.tsx) to confirm no schema drift.
  2. Live dogfooding (see next section) before declaring done.

Dogfooding

Environment: make dev-server-sandbox (project skill dev-server-sandbox) — the web dev server, not the Electron desktop app. Isolated temp XUM_ROOT, free backend + Vite ports, seeded providers.jsonc/config.json. Run as a monitored background bash task (filter: "ready|listening|localhost|ERROR|EADDRINUSE|failed|Failed", timeout_secs: 1800).

This environment is already validated in this workspace (pre-flight run on the unmodified branch): sandbox boots in ~1 min (temp root /tmp/mux-dev-server-*, backend 127.0.0.1:<port>, Vite 127.0.0.1:<port>), and agent-browser --session <s> open http://127.0.0.1:<vite-port>/ connects headlessly — no Electron, no Xvfb. The app renders the full sidebar and, on the project page, an "Archived Workspaces (N)" section — the exact UI surface that verifies archive state transitions. Seeded config.json contains real projects; dogfood against a scratch project (or launch with DEV_SERVER_SANDBOX_ARGS="--clean-projects" and add one).

agent-browser flow (per agent-browser core + dogfood skills): snapshot -i for element refs → click/fill by @eN ref → re-snapshot after page changes; screenshot <file>.png for stills; record start/stop <file>.webm for videos (per the agent-browser/dogfood skill workflow); errors/console for renderer errors after each step.

Evidence requirements (per dogfood skill: repro-first, evidence per step): screenshots per step AND a record start/record stop webm video of the core archive→unarchive flow, paced human-watchably (sleep 1 between actions). All evidence attached via attach_file for reviewer verification.

Script (each step evidenced with an agent-browser screenshot, attached via attach_file):

  1. In the sandbox app, create a parent workspace on a scratch project; send it a prompt instructing: “create a peer workspace via task({kind:"workspace", ...}), wait for its turn to settle, then call task_workspace_lifecycle({action:"archive", targets:[{workspaceId:...}]})”.
  2. Verify: peer workspace appears in the sidebar → after archive it disappears from the active sidebar and shows as archived on the project page. Screenshot both states.
  3. Negative check: instruct the agent to archive a workspace it did not create → expect invalid_scope in the tool card. Screenshot.
  4. Untracked-file confirmation: create the peer workspace, drop an untracked file in its worktree (with snapshot archive behavior enabled), archive → expect requires_confirmation card listing paths; re-call with acknowledged_untracked_paths → archived. Screenshot the confirmation card.
  5. Unarchive: instruct {action:"unarchive"} → workspace returns to the sidebar; then a task(kind="workspace", mode="existing") follow-up succeeds. Screenshot.
  6. Transcript card sanity: confirm the WorkspaceLifecycleToolCall card renders archived/unarchived/invalid_scope statuses correctly in the real app (not just Storybook).

Deliverables: screenshots per step; if a step can't run headless, fall back to the integration-test equivalent and state exactly which steps were verified live vs. by test.

Acceptance criteria

  1. A parent agent can archive a peer workspace it created via task(kind="workspace") using task_workspace_lifecycle({action:"archive"}), and unarchive it with {action:"unarchive"}.
  2. Non-owned workspaces, arbitrary user workspaces, and sub-agent (non-wst_) task IDs are refused with invalid_scope.
  3. Active workspace turns block archive unless interrupt_active: true (which interrupts, then archives); unarchive never interrupts regardless of interrupt_active.
  4. Lossy-snapshot archive requires the requires_confirmationacknowledged_untracked_paths round-trip; force no longer exists on the input schema.
  5. Archived targets refuse mode:"existing" follow-ups; unarchive restores them.
  6. remove/delete_worktree are not model-invocable through this tool (schema-rejected); task_remove behavior unchanged.
  7. Historical transcripts with old task_workspace_lifecycle calls still render (renderer untouched, result schema unchanged).
  8. Explore/plan/desktop agents do not receive the tool; plan mode throws at execute as defense-in-depth.
  9. make static-check green; all Phase 4 suites green; dogfooding evidence captured.
  10. The issue-triage-loop skill's documented call task_workspace_lifecycle({action:"archive", targets:[{workspaceId}], interrupt_active:false}) works verbatim.

Risks & mitigations

  • Lock layering (workspaceLifecycleLocks wrapping workspaceService.archive's internal task-tree lock): restore the historical ordering exactly; the archive path re-reads metadata after acquiring the lifecycle lock, so stale-metadata races are handled as before. Covered by the idempotency tests.
  • Schema drift since removal: the kept result schema gained unarchived/already_unarchived; the restored backend emits only statuses in the kept union, and toolDefinitions.test.ts already validates the schemas — renderer exhaustiveness (Record<Status, Meta>) fails typecheck if a status is added without UI handling.
  • Model confusion with task_remove: mitigated by the description contrast (reversible vs irreversible) and by invalid_scope for sub-agent IDs; watch Codex review for wording feedback.
  • 🤖 feat: simplify persistent sub-agent lifecycle #3825 intent: the removal rationale was sub-agent retention-class cleanup; this restoration keeps sub-agents out of scope entirely, so it does not reintroduce the mixed lifecycle 🤖 feat: simplify persistent sub-agent lifecycle #3825 eliminated.

Estimate

~320 net LoC product code (schema ~25, taskService ~180, tool ~100, registration ~15) + ~250 LoC tests. No migrations; upgrade/downgrade safe (config format untouched; archive fields already exist).


Generated with xum • Model: anthropic:claude-fable-5 • Thinking: xhigh • Cost: $53.33

@mintlify

mintlifyBot commented Aug 24, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

ProjectStatusPreviewUpdated (UTC)
Mux🟢 ReadyView PreviewAug 24, 2026, 10:58 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@ThomasK33

Copy link
Copy Markdown
MemberAuthor

@codex review

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:035ad4d751

ℹ️ 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".

Comment threadsrc/node/services/taskService.ts Outdated
Comment threadsrc/node/services/taskService.ts Outdated
Comment threadsrc/node/services/taskService.ts Outdated

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit:035ad4d751

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ 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.

Comment threadsrc/node/services/taskService.ts Outdated
@ThomasK33

Copy link
Copy Markdown
MemberAuthor

Addressed all four review findings in 743b93b:

  • Serialize the active-turn check with follow-up creation (taskService.ts:8658): createWorkspaceTurn now persists its handle record under the same per-workspace workspaceLifecycleLocks that archive holds for its check+archive section, with an archived re-check at persist time. Either the follow-up handle is visible to archive's active-turn check, or the archive lands first and the follow-up is refused — no more silent truncation. Covered by the new "serializes archive with follow-up handle persistence" test. Lock nesting is acyclic (mutex → lifecycle lock; the archive path never acquires the task mutex — documented at the persist site).
  • Block archive while the target owns active turns (taskService.ts:8861): collectActiveWorkspaceLifecycleTurns now includes turns owned by the target workspace (nested delegation) alongside caller-owned turns targeting it; interrupt_active settles both sets. Covered by "archive blocks on active turns owned by the target".
  • Preflight archive confirmation before interrupting turns (taskService.ts:8667): when interrupt_active is set and no acknowledgement was supplied, workspaceService.preflightArchive() runs before any interruption; an unacknowledged lossy confirmation now returns requires_confirmation with the work still running. The archive call retains its own re-validation for changed paths. Covered by "preflights lossy confirmation before interrupting active turns".
  • Require approval before destructive archive policies (security): agent-driven archive now fails closed when the "Delete checkout" worktree archive behavior is configured (it would run git worktree remove --force with no snapshot or user confirmation), directing the agent to user-mediated archive instead. The snapshot acknowledged-paths round-trip is retained: it is the tool's designed confirmation surface, scoped by durable ownership records to workspaces the agent itself created, and snapshot mode preserves tracked state with the lossy-untracked set explicitly enumerated. Covered by "refuses archive when worktree archive behavior deletes checkouts".

@ThomasK33

Copy link
Copy Markdown
MemberAuthor

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:743b93b203

ℹ️ 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".

Comment threadsrc/node/services/taskService.ts
Comment threadsrc/node/services/taskService.ts Outdated
Comment threadsrc/node/services/taskService.ts Outdated
Comment threadsrc/node/services/taskService.ts Outdated
Comment threadsrc/node/services/taskService.ts
@ThomasK33

Copy link
Copy Markdown
MemberAuthor

Addressed all five round-2 findings in ef03f32:

  • Lock nested turn creation against archiving its owner: the persist section in createWorkspaceTurn now acquires the lifecycle locks for BOTH the owner and the target (sorted keys, acyclic with the creation mutex), and re-checks that neither is archived. Archiving a peer therefore serializes with that peer starting nested turns; a nested turn losing the race is refused with "owner workspace was archived during turn creation". Covered by "serializes nested turn creation with archiving its owner".
  • Refuse archive while any target activity is live: new workspaceService.listLiveWorkspaceActivity (stream + terminal PTYs via terminalService.hasWorkspaceSessions + desktop session via DesktopSessionManager.has) is consulted before any interruption. A stream not explained by a running delegated turn, or any terminal/desktop session, returns active with an explanatory note — interrupt_active never applies to user activity. Covered by "refuses archive while the target has live non-turn activity".
  • Preflight archive blockers after path acknowledgement: preflightArchive now runs before interruption on every interrupt_active archive. Blockers (e.g. ACTIVE_DESCENDANT_ARCHIVE_ERROR) surface as errors without interrupting, and a fresh confirmation is returned when the acknowledged set no longer covers the preflight paths. Covered by "re-confirms when acknowledged paths no longer cover the preflight" and the updated preflight test.
  • Enforce the delete-policy guard inside archive: workspaceService.archive gains forbidWorktreeCheckoutDeletion, enforced in archiveUnlocked against the same behavior read that drives the snapshot decision — and that read is now passed into the afterArchive worktree hook (AfterArchiveHookArgs.worktreeArchiveBehavior), so a keep→delete settings flip mid-archive can no longer delete a checkout that was never snapshotted (this also hardens user-mediated archive). The TaskService pre-check remains as a friendly early refusal before any interruption.
  • Ignore turns that settle during lifecycle interruption: on an interruption failure the handle is re-read; now-terminal (or vanished) handles are skipped instead of aborting the archive with the set partially interrupted. Covered by "interruption tolerates turns that settled after collection".

@ThomasK33

Copy link
Copy Markdown
MemberAuthor

@codex review

The tool was removed from the executable toolset by #3825 as collateral of the
sub-agent lifecycle consolidation, leaving orchestrating agents unable to
archive peer workspaces created via task(kind="workspace"). Restore it with
only the reversible verbs:
- Live input schema (TaskWorkspaceLifecycleToolInputSchema) exposes only
archive/unarchive; the historical args schema stays intact so old
transcripts still parse. delete_worktree/remove stay non-invocable;
task_remove remains the sole irreversible verb.
- taskService: restore archiveOwnedWorkspaceTurnWorkspace + helpers from
88580ca^ and add the previously unimplemented
unarchiveOwnedWorkspaceTurnWorkspace. Authorization uses durable
workspace-turn ownership records (taskHandleStore.isWorkspaceOwnedBy).
Unarchive never interrupts active turns, even as defense-in-depth.
- Register tool + availability + PTC bridging; remove from explore/plan/
desktop agent allowlists; preserve output in shared transcripts.
- Frontend untouched: renderer/result schema survived the removal.
…-policy guard
- Serialize workspace-turn handle persistence with owned-workspace archive via
the shared workspaceLifecycleLocks (archived re-check at persist time), so a
follow-up can no longer slip between archive's active-turn check and its
stream stop. Lock order mutex -> lifecycle lock is acyclic: the archive path
never acquires the task mutex.
- Archive/unarchive active-turn checks now also cover turns OWNED BY the
target workspace (nested delegation); interrupt_active settles those too.
- When interrupt_active is set without acknowledged paths, preflightArchive
runs BEFORE any interruption so a lossy-snapshot confirmation cannot leave
work terminated but the workspace unarchived.
- Model-facing archive fails closed under the 'Delete checkout' worktree
archive behavior (would delete the checkout without user confirmation).
…flight, sink-enforced delete guard, settled-turn tolerance
- createWorkspaceTurn persists handles under sorted lifecycle locks for BOTH
owner and target, so archiving a peer serializes against that peer starting
nested turns; a nested turn racing its owner's archive is refused.
- Archive refuses when the target has live non-turn activity (user stream,
terminal PTYs, desktop session) — interrupt_active covers delegated turns
only. New workspaceService.listLiveWorkspaceActivity +
terminalService.hasWorkspaceSessions + DesktopSessionManager.has.
- preflightArchive now runs before interruption on every interrupt_active
archive (not only unacknowledged ones), surfacing blockers like active
descendant sub-agents and re-confirming when acknowledged paths no longer
cover the fresh untracked set.
- Delete-checkout policy enforced at the sink: workspaceService.archive gains
forbidWorktreeCheckoutDeletion, checked against the same behavior read that
drives snapshot/deletion; that read is now passed to the afterArchive
worktree hook so a keep->delete settings flip mid-archive can no longer
delete a checkout that was never snapshotted.
- Lifecycle interruption skips turns that settled after collection instead of
aborting the archive mid-set.
@ThomasK33

Copy link
Copy Markdown
MemberAuthor

@codex review

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:81ef051d21

ℹ️ 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".

Comment threadsrc/node/services/taskService.ts Outdated
Comment threadsrc/node/services/taskService.ts
@chatgpt-codex-connector

This comment has been minimized.

… clean up created workspace on owner_archived refusal
@ThomasK33

Copy link
Copy Markdown
MemberAuthor

@codex review

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:daa3e8d0dd

ℹ️ 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".

Comment threadsrc/node/services/taskService.ts
Comment threadsrc/node/services/taskService.ts Outdated
Comment threadsrc/node/services/taskService.ts Outdated
Comment threadsrc/node/services/taskService.ts Outdated
Comment threadsrc/common/utils/tools/toolDefinitions.ts
…sensitive interrupt_active, guard archived-workspace activity admission, suppress disposable cleanup on archive, reject blank acknowledged paths
@ThomasK33

Copy link
Copy Markdown
MemberAuthor

@codex review

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit:96ed7d8d2c

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ 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.

Comment threadsrc/node/services/taskService.ts Outdated

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:96ed7d8d2c

ℹ️ 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".

Comment threadsrc/node/services/terminalService.ts
Comment threadsrc/node/services/taskService.ts Outdated
Comment threadsrc/node/services/desktop/DesktopSessionManager.ts Outdated
Comment threadsrc/node/services/workspaceService.ts
Comment threadsrc/node/services/taskService.ts
Comment threadsrc/node/services/taskService.ts
Comment threadsrc/node/services/taskService.ts Outdated
…p/queue admission with archive, pin archive policy through interruption, gate on active workflow runs, defer nested disposable cleanup, serialize unarchive under tree lock
@ThomasK33

Copy link
Copy Markdown
MemberAuthor

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:6d5c00ba3b

ℹ️ 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".

Comment threadsrc/node/services/terminalService.ts
Comment threadsrc/node/services/taskService.ts
Comment threadsrc/node/services/workspaceService.ts
Comment threadsrc/node/services/tools/task_workspace_lifecycle.ts Outdated
Comment threadsrc/node/services/workspaceService.ts

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:62db201e4f

ℹ️ 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".

Comment threadsrc/node/services/backgroundProcessManager.ts
Comment threadsrc/node/services/workspaceService.ts
…efused open reservations
P1: an unreadable/missing meta.json could hide a surviving crash orphan. Now:
- RuntimeBackgroundHandle.writeMeta propagates persistence failures and spawn
aborts (terminating the process, whose exit trap self-heals the directory)
when the initial record cannot be written — a process the crash-orphan gate
cannot see must not run.
- The orphan probe trusts only the exit marker for records it cannot read or
parse, failing closed otherwise.
- Failed spawns remove their output directory so recordless directories from
spawn errors cannot permanently over-refuse archives.
P2: openNative/recordExternalEditorOpen added the sticky in-memory reservation
before their refusal checks, so a rejected open (archiving/archived/unknown
workspace/marker failure) permanently gated model-driven archives until
restart. Newly added reservations now roll back on refusal — but only until
the durable marker persists, after which the Set is just a cache of it.
@ThomasK33

Copy link
Copy Markdown
MemberAuthor

@codex review

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:8aee92da6a

ℹ️ 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".

Comment threadsrc/node/services/backgroundProcessExecutor.ts
Comment threadsrc/node/services/backgroundProcessManager.ts Outdated
Comment threadsrc/node/services/taskService.ts Outdated
Comment threadsrc/node/services/workspaceService.ts

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit:8aee92da6a

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ 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.

Comment threadsrc/node/services/backgroundProcessManager.ts
…s, executeBash archive pairing
- Restart-unique process directories (local runtimes): spawn skips display
names whose durable directory may still belong to a live process from a
previous session, so a surviving crash orphan's meta.json/exit_code is never
shared with (and settled by) a newer same-name process.
- The crash-orphan probe fails closed when the spawn-record directory itself
is unreadable (only ENOENT/ENOTDIR mean no records).
- Workflow activity scans used by archive gates are now strict: an unreadable
run store or run record refuses archive (caller) / reads as active (sink)
instead of silently reporting no runs, so a crash-recovered run cannot
resume into an archived workspace. Heuristic callers keep the lenient scan.
- executeBash pairs with archive admission like sends/terminals/workflows: a
synchronous preflight count held for the command's duration, checked by the
refuseLiveUserActivity gate, so an admitted command cannot resume against a
captured/removed checkout or re-wake a stopped Coder workspace.
@ThomasK33

Copy link
Copy Markdown
MemberAuthor

@codex review

@ThomasK33

Copy link
Copy Markdown
MemberAuthor

@codex review

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit:d79137a33a

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ 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.

Comment threadsrc/node/services/taskService.ts

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:d79137a33a

ℹ️ 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".

Comment threadsrc/node/services/backgroundProcessManager.ts
Comment threadsrc/node/services/workspaceService.ts
Comment threadsrc/node/services/workspaceService.ts
Comment threadsrc/node/services/backgroundProcessManager.ts Outdated
…hive pairing
- P1: concurrent same-name spawns could both pass the in-memory allocator and
the async disk checks before either registered, sharing one output directory
whose meta.json/exit_code the first exit would settle under the other still-
running process. Process IDs are now reserved synchronously when a candidate
is chosen (reservedProcessIds), kept in sync through the disk-dedup loop, and
released on registration or failure.
- stageAttachment pairs with archive admission (sync archivingWorkspaces guard,
archived-state refusal, preflight counter held for the upload): staging
writes into the checkout a snapshot archive would capture/remove.
- getFileCompletions pairs likewise (degrading to empty results): its refresh
runs git through the target runtime, which could re-wake a Coder workspace
the archive hook just stopped. The refresh closure holds its own admission
because it can outlive the calling request.
Remaining round-19 findings are tracked as follow-ups: MCP server lifecycle
pairing (#3946) and staged-attachment snapshot placement (#3947).
@ThomasK33

Copy link
Copy Markdown
MemberAuthor

@codex review

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:a995722cc9

ℹ️ 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".

Comment threadsrc/node/services/workspaceService.ts Outdated
Comment threadsrc/browser/utils/openInEditor.ts Outdated
Comment threadsrc/common/utils/messages/transcriptShare.ts
Migrated background processes record pid 0 (exec streams expose no PID) and
their exit marker is written by the in-process handle, not a detached trap —
so after an unclean shutdown a surviving migrated child left a markerless
running record the probe deliberately skipped, blinding the archive gate.
The probe now fails closed on such records instead of skipping them, and skips
tracked processes by ID (directory name = process ID) so live migrated
processes remain owned by the in-memory gates. Clean shutdowns and natural
exits still settle records via updateMetaFile / the exit marker, so only
genuine unclean-exit survivors trip the gate (routing to user-mediated
archive).
@ThomasK33

Copy link
Copy Markdown
MemberAuthor

@codex review

…ch, redact shared lifecycle paths
- recordExternalEditorOpen requires a real config workspace entry before any
filesystem work: the marker path joins the raw ID beneath the sessions
directory, so unknown (possibly traversal-crafted, e.g. ../../.ssh) IDs must
never reach it. Rejected IDs roll their reservation back.
- The renderer records editor opens immediately before each deep-link launch,
after every deterministic compatibility check (custom-in-browser, Zed/custom
vs Docker/devcontainer, missing deep link), so a refused open can no longer
persist a sticky durable marker that permanently gates snapshot archives.
Custom-editor opens remain recorded by the backend route.
- Shared transcripts with includeToolOutput=false keep lifecycle results (the
card renders from statuses) but redact paths / error / note, which can name
local files the exporter chose not to share.
@ThomasK33

Copy link
Copy Markdown
MemberAuthor

@codex review

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit:196f2d1536

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ 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.

Comment threadsrc/node/services/tools/task_workspace_lifecycle.ts

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:196f2d1536

ℹ️ 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".

Comment threadsrc/node/services/terminalService.ts Outdated
Comment threadsrc/node/orpc/router.ts Outdated
Comment threadsrc/browser/utils/openInEditor.ts
@ThomasK33

Copy link
Copy Markdown
MemberAuthor

@codex review

Round 22: the lossy-archive acknowledgement finding is acknowledged as a real product gap and tracked in #3950 (user-origin approval / non-lossy snapshot design, covering task_remove too). The model round trip is the accepted contract for this restoration PR; see the inline reply for the full rationale. No code changes this round.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit:196f2d1536

ℹ️ 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".

@chatgpt-codex-connector

This comment has been minimized.

…, preserve browser popup activation
- TerminalService.openNative and recordExternalEditorOpen now delete a durable
marker they just created when the launch deterministically fails (all launchers
throw only before their detached spawn), so a single failed launch can no longer
permanently refuse future model-driven snapshot/Coder-stop archives. Pre-existing
markers and concurrent opens' launch evidence are preserved; marker writes and
rollbacks are serialized per workspace.
- The custom-editor route records via recordExternalEditorOpenForLaunch and rolls
the marker back when EditorService validation fails without spawning.
- Browser-mode openInEditor opens a blank placeholder synchronously during the
click's transient user activation and navigates it after admission, closing it
on refusal; popup-blocked placeholders fall back to the legacy direct open.
@ThomasK33

Copy link
Copy Markdown
MemberAuthor

@codex review

Round 23 fixes pushed in 85118cf: failed native-terminal/custom-editor launches now roll back a durable marker they just created (pre-existing markers and concurrent opens' launch evidence preserved; writes/rollbacks serialized per workspace), and browser-mode editor opens consume the click's transient user activation via a synchronously opened placeholder that is navigated after admission and closed on refusal.

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:85118cf4bf

ℹ️ 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".

Comment on lines +137 to +139
// No placeholder was needed (Electron) or it was popup-blocked: fall back to a direct
// open, which shares the blocked popup's fate but never regresses it.
openUrl(deepLink);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Refuse browser opens when the placeholder is blocked

When browser popup blocking makes the synchronous window.open("about:blank") return null, this fallback runs only after the awaited recording RPC has consumed transient user activation, so it is also blocked. Because openUrl does not report that failure, the function returns success after persisting an editor-open marker even though no editor opened, and future model-driven Snapshot/Coder-stop archives remain refused across restarts. Refuse before recording when the placeholder is blocked, or use rollback-capable admission when the fallback cannot launch.

Useful? React with 👍 / 👎.

this.nativeTerminalOpenTokens.set(workspaceId, tokens);
}
tokens.add(token);
return { token, markerPreexisted: preexisting !== "absent" };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Track marker ancestry across concurrent failed launches

When two first-time openNative() calls reach marker admission concurrently and both launch attempts fail, the second serialized admission sees the marker created by the first call and records it as pre-existing. The first rollback then preserves the marker because the second token remains, and the second rollback preserves it because markerPreexisted is true, leaving durable and in-memory false evidence that permanently refuses later Snapshot/Coder-stop archives even though neither terminal opened. Fresh evidence after 85118cf is that the new serialization conflates an earlier in-flight marker with evidence from a prior launch; track ownership for the whole concurrent batch so its last failed token can remove the marker. recordExternalEditorOpenForLaunch() mirrors the same defect for concurrent failed custom-editor opens.

Useful? React with 👍 / 👎.

Comment on lines +7978 to +7979
if (options?.refuseLiveUserActivity === true) {
const liveActivity = this.listLiveWorkspaceActivity(workspaceId);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Wait for workspace initialization before model-driven archive

When a newly created owned workspace's delegated turn has settled but its background initialization is still running, this gate reports no live activity because initialization exists only in initStateManager/initAbortControllers. archiveUnlocked() later aborts and immediately clears that state without retaining or awaiting the runBackgroundInit() promise started fire-and-forget in create(), so Snapshot capture and checkout deletion—or a Coder stop—can proceed while the init hook is still writing or reconnecting. Count running initialization as live activity, or retain its settlement promise and await it after abort before continuing archive.

AGENTS.md reference: AGENTS.md:L150-L150

Useful? React with 👍 / 👎.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@ThomasK33