fix: preserve subagent session ownership#261
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds read-only session attachments for externally-owned runtimes, an explicit AgentRoleSet override flag across CLI/server APIs, planner base/active fields for resumed-role handling, client-side read-only write gates, and tests updating planner/session runtime/attach behaviors. ChangesRead-only attachment and explicit agent role tracking
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/app/internal/runtimeattach/lease.go`:
- Around line 42-44: The Recover path currently ignores resp.ReadOnly and
converts that case into ErrEmptyControllerLease; update Recover (and the similar
block around lines 51-57) to mirror Activate's handling by returning
Lease{ReadOnly: true}, nil when resp.ReadOnly is true instead of returning
ErrEmptyControllerLease, ensuring the Lease struct's ReadOnly field is preserved
for downstream callers (reference Recover, Activate, Lease,
ErrEmptyControllerLease, and resp.ReadOnly to locate the spots to change).
In `@server/launch/planner.go`:
- Around line 93-110: The current code mutates baseActive.OpenAIBaseURL from
meta.Continuation.OpenAIBaseURL, causing BaseSettings to carry role-specific
state; instead, do not modify baseActive — keep baseActive =
EffectiveSettings(...) unchanged and derive any role-specific OpenAIBaseURL into
a separate local (e.g., roleURL) or only set continuation.OpenAIBaseURL after
applyPersistedSubagentRoleSettings/when determining active, so
applyPersistedSubagentRoleSettings and EffectiveSettings operate on the true
baseline; ensure SetContinuationContext receives continuation.OpenAIBaseURL from
the role-specific active value (not written back into baseActive) and add a
regression test that resumes a session with a role-specific openai_base_url then
clears or switches roles to verify workspace/base config remains correct.
- Around line 145-151: The code marks all role.Sources as "subagent"
unconditionally, which can label blocked/locked overrides as applied; update the
logic after calling sourceReportWithSubagentRoleSources so you only set
effectiveSources[key] = "subagent" for keys that were actually applied to
resolved (e.g., compare the resolved values vs the base/effectiveSource before
the role was applied or check that the locked setting didn’t prevent the
change—specifically for model compare resolved.ActiveSettings.Model to the
role’s model) and only then call applyReviewerInheritance and assign
effectiveSource.Sources = effectiveSources; apply the same fix in the other
similar block (lines ~219-233) that also uses
sourceReportWithSubagentRoleSources.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e34db98-3436-41d0-948f-3037c4b75589
📒 Files selected for processing (25)
cli/app/app.gocli/app/internal/runtimeattach/activity.gocli/app/internal/runtimeattach/activity_test.gocli/app/internal/runtimeattach/lease.gocli/app/internal/runtimeattach/lease_test.gocli/app/launch_planner.gocli/app/run_prompt.gocli/app/runtime_attachment.gocli/app/runtime_attachment_test.gocli/app/session_lifecycle.gocli/app/ui_runtime_client.gocli/app/ui_runtime_client_control.gocli/builder/main.gocli/builder/main_test.godocs/dev/decisions.mddocs/src/content/docs/headless.mdserver/launch/planner.goserver/launch/planner_test.goserver/runtime/meta_context.goserver/runtime/subagents_test.goserver/sessionruntime/service.goserver/sessionruntime/service_test.goshared/serverapi/run_prompt.goshared/serverapi/run_prompt_test.goshared/serverapi/session_runtime.go
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cca0b0e1f6
ℹ️ 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".
| if s.externalSessionRuntimeActive(sessionID) { | ||
| return serverapi.SessionRuntimeActivateResponse{ReadOnly: true}, nil |
There was a problem hiding this comment.
The new externalSessionRuntimeActive short-circuit can misclassify a session as externally owned during normal local handoff: ReleaseSessionRuntime deletes s.handles[sessionID] before closeFn() unregisters the runtime, so a concurrent ActivateSessionRuntime can observe IsSessionRuntimeActive=true and handles[sessionID]==nil and incorrectly return ReadOnly=true. In that window, a legitimate reconnect/reattach can be forced into read-only mode even though no external controller actually exists.
Useful? React with 👍 / 👎.
| } else { | ||
| runtimeClient.SetReadOnly(true) | ||
| } |
There was a problem hiding this comment.
Read-only attach only toggles sessionRuntimeClient via SetReadOnly(true), but worktree mutations bypass ensureWritable and still issue control RPCs using controllerLeaseIDValue (which remains the default placeholder lease). In a headless-owned read-only attach, /worktree create/switch/delete paths now fail with invalid-lease/recovery errors instead of a deterministic read-only rejection, which is a functional regression for those commands.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c0ef0ef54
ℹ️ 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".
| return base, source | ||
| } | ||
| resolved := cloneSettings(base) | ||
| _ = applyBuiltInRoleHeuristics(&resolved, normalizedRole, persistedRoleProviderID(base), allowModelOverride) |
There was a problem hiding this comment.
The persisted-role resume path applies built-in role heuristics using persistedRoleProviderID(base) before any role/provider overrides are considered. When the base model is an alias and provider identity is only available via provider_override (or role-specific provider fields), persistedRoleProviderID can be empty, so resumed fast sessions skip fast heuristics (model/priority/context tuning) even though explicit --agent fast would apply them. This breaks the new “reapply persisted role on continue” behavior for provider-aliased configs.
Useful? React with 👍 / 👎.
| if lease.ReadOnly { | ||
| return lease, nil, nil |
There was a problem hiding this comment.
Returning leaseManager=nil for read-only attachments leaves the runtime plan without a controller lease ID, but lifecycle transitions still send session_id and therefore require a non-empty lease. In practice, attaching to an active headless session as read-only makes transition actions (for example /new, /resume, /open) fail with controller-lease validation errors instead of a deterministic read-only flow. This regression is triggered specifically in the new read-only attach path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bd46fcf75
ℹ️ 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".
| if s.externalSessionRuntimeActive(sessionID) { | ||
| return serverapi.SessionRuntimeActivateResponse{ReadOnly: true}, nil |
There was a problem hiding this comment.
ActivateSessionRuntime now short-circuits to ReadOnly=true based on externalSessionRuntimeActive, but that decision is made from two unsynchronized data sources and returned immediately. If the external/headless runtime exits in that tiny window, the caller still gets a read-only attach (no lease) instead of taking ownership, and the follow-up attach flow can fail because the runtime is already gone. This creates a timing-dependent failure when connecting around headless shutdown; the read-only response should be confirmed against current state (or retried) before returning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3a7f12406
ℹ️ 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".
| func (o RunPromptOverrides) HasAny() bool { | ||
| return strings.TrimSpace(o.AgentRole) != "" || | ||
| return o.AgentRoleSet || | ||
| strings.TrimSpace(o.AgentRole) != "" || |
There was a problem hiding this comment.
HasAny now returns true when AgentRoleSet is true, which makes PlanLaunchSession fetch auth state before applying overrides (server/sessionlaunch/service.go, auth lookup gate). For --agent default|none|self this is only clearing persisted role and does not use provider capabilities, so a transient/invalid auth store now causes run planning to fail even though the operation should be local. This regresses continuation flows specifically when users explicitly clear role while auth state cannot be loaded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 852c916425
ℹ️ 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".
| default: | ||
| return resolvedSessionAction{}, errReadOnlyRuntime |
There was a problem hiding this comment.
When a TUI attaches read-only to an active headless runtime, resolveReadOnlySessionAction falls into the default branch for UIActionLogout and returns errReadOnlyRuntime, so /logout exits with a read-only error instead of running the logout/reauth flow. This is a functional regression specific to read-only attachments: ui_input_controller_commands.go still emits UIActionLogout, but the new read-only transition switch only allows new/resume/open/exit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17e15785b6
ℹ️ 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".
| if providerOverride := strings.TrimSpace(settings.ProviderOverride); providerOverride != "" { | ||
| return providerOverride | ||
| } | ||
| provider, err := llm.InferProviderFromModel(settings.Model) | ||
| if err != nil { |
There was a problem hiding this comment.
When reapplying a persisted role, persistedRoleProviderID only checks provider_capabilities.provider_id, provider_override, then model-name inference. For aliased models (e.g. team aliases) without provider_override, this returns empty even if openai_base_url (or auth-selected provider) clearly implies OpenAI, so applyBuiltInRoleHeuristics skips fast-role tuning on resume. This makes persisted fast sessions behave differently from explicit --agent fast (no gpt-5.4-mini/priority-mode heuristic), which is a regression in the new role-resume behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/app/internal/worktreemutation/service.go`:
- Around line 122-124: The read-only guard currently inside
controlContextWithLease() causes non-mutating calls like List() to be blocked;
move the check into the mutation-only path by removing the ReadOnly check from
controlContextWithLease() and instead enforcing it at the start of runMutation()
so Create, Switch, and Delete operations return ErrReadOnlyRuntime while List
and other read-only flows that call controlContextWithLease() continue to work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 162cf1e8-53be-4813-be21-4d23b1f8ec74
📒 Files selected for processing (14)
cli/app/internal/runtimeattach/lease.gocli/app/internal/runtimeattach/lease_test.gocli/app/internal/worktreemutation/service.gocli/app/internal/worktreemutation/service_test.gocli/app/session_lifecycle.gocli/app/session_lifecycle_test.gocli/app/ui_runtime_client.gocli/app/ui_worktrees.goserver/launch/planner.goserver/launch/planner_test.goserver/sessionlaunch/service.goserver/sessionlaunch/service_test.goserver/sessionruntime/service.goshared/serverapi/run_prompt.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/app/internal/runtimeattach/lease.go
- cli/app/ui_runtime_client.go
- server/launch/planner.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c310dbe609
ℹ️ 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".
| } else { | ||
| runtimeClient.SetReadOnly(true) |
There was a problem hiding this comment.
In the read-only attach branch, the runtime client is only marked via SetReadOnly(true) and keeps its default placeholder lease ID, so read-only sessions still issue /wt status list requests with a non-owner lease. worktreemutation.Service.List always sends ControllerLeaseID, and server-side worktree handling enforces RequireControllerLease, so these read-only status/list flows fail with invalid-lease errors instead of allowing deterministic read-only inspection (or a clear local read-only rejection).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af6949f09d
ℹ️ 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".
| if current.closing { | ||
| return nil, nil, activationClaimOwner, errors.Join(serverapi.ErrSessionAlreadyControlled, fmt.Errorf("session %q is closing its active controller", sessionID)) |
There was a problem hiding this comment.
Returning ErrSessionAlreadyControlled as soon as current.closing is set makes activation fail during legitimate handoff windows: ReleaseSessionRuntime now sets closing=true before closeFn() completes, but the CLI release path uses a fixed 3s timeout (cli/app/internal/runtimeattach/lease.go), so a slow shutdown can time out client-side and the very next /new//open attach attempt hits this branch and aborts instead of waiting/retrying ownership transfer. This introduces a timing-dependent regression for normal lifecycle transitions under slower runtime teardown.
Useful? React with 👍 / 👎.
1 participant
Summary
Verification
Summary by CodeRabbit
New Features
Documentation