Reset per-agent 'Always allow' session permissions (#259)#260
Reset per-agent 'Always allow' session permissions (#259)#260germanescobar wants to merge 2 commits into
Conversation
Clicking 'Always allow' on an approval card forwarded the decision to the agent CLI, but Controller never offered a way to revoke it — for Claude, the rules persisted in the live session and required --resume; for Codex, they lived in the live app-server thread. The only escape valves were killing the session or restarting Controller. This adds a per-agent Reset button in Settings that clears the rules for the current worktree: * Codex: `CodexAppServerManager.resetAllSessions()` terminates the live `codex app-server` child and clears every thread runtime. The next turn spawns a fresh app-server. Returns the count of runtimes dropped for the toast. * Claude: every active Claude child for the worktree is terminated, and the worktree is marked in a per-process `revokedWorktrees` set (`server/lib/session-permissions.ts`). On the next session start, `consumeClaudeSessionRevocation` drops `resumeSessionId` so Claude starts a fresh session id. This is the only revocation path the Claude control protocol exposes — there's no public 'remove rules' message. * Anita: no permission prompts to revoke, but the endpoint still acknowledges the call so the UI doesn't surface a confusing error. UI: * New 'Reset' button under each agent row in agents-section.tsx, with an AlertDialog confirmation that explains the per-agent cost (Codex: 'session may end mid-turn'; Claude: 'conversation context is reset'). * The button is only rendered when Settings was opened from a view that carries a project/worktree context (preSettingsViewRef in App.tsx). Otherwise it's hidden — there's no worktree to scope the reset to. Tests: * `server/lib/__tests__/session-permissions.test.ts` covers the per-worktree revocation flag. * `server/lib/__tests__/agents-reset.test.ts` covers the route handler: 400 on missing fields, 404 on unknown worktree/agent, 200 for each provider with the right counters and flag side-effect. 501/501 tests pass; tsc -p server/tsconfig.json clean; vite build clean.
|
@codex 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 Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3763492253
ℹ️ 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".
| const effectiveResumeSessionId = | ||
| providerId === "claude" && consumeClaudeSessionRevocation(worktree.id) | ||
| ? undefined | ||
| : resumeSessionId; |
There was a problem hiding this comment.
When a Claude reset is pending for a resumed session, this consumes the flag and starts the CLI without --resume, but the rest of handleSessionStream still branches on the original resumeSessionId. In that scenario the later resumed-session path pre-persists the user's prompt to the old session and sets userMessageWritten, so the new fresh Claude session created by run.started can miss the user message while the old runtime is left marked active. The same flag can also be consumed by a brand-new Claude turn where there is no resume id to drop, leaving the old session permissions intact for a later resume.
Useful? React with 👍 / 👎.
The Claude reset path consumed the per-worktree revocation flag and
passed `undefined` as `resumeSessionId` to the provider spawn, but
the rest of `handleSessionStream` still branched on the original
`resumeSessionId`:
- `streamSessionId` was seeded from the original id.
- The resumed-session pre-persist guarded on the original id and
called `persistSessionStart(resumeSessionId)`.
In the resumed + reset case this wrote the controller's user_message
to the about-to-be-revoked session, set `userMessageWritten`, and
then the new fresh Claude session from `run.started` skipped the
user_message because the flag was already set — the new session was
empty while the old one carried the user's prompt.
The flag was also consumed on brand-new (no-resume) turns, burning
the one-shot without actually dropping a resume id and leaving the
original session resume-able for a later turn.
Fix: thread `effectiveResumeSessionId` through the two late
references (`streamSessionId`, the pre-persist guard, and the
persist call), and only consume the flag when the caller passed a
`resumeSessionId` we can actually drop. Add a regression test that
mirrors the gate so a future refactor doesn't regress the contract.
Reported by the Codex PR review bot on #260.
|
Thanks — addressed in d0b0c41. You were right on both sub-points. The fix in
Added a regression test in |
1 participant
Closes #259.
Clicking Always allow on an approval card forwards the decision to the agent CLI, but Controller never offered a way to revoke it. For Claude the rules persisted in the live session and survived
--resume; for Codex they lived in the live app-server thread. The only escape valves were killing the session, restarting Controller, orrm -rfon~/.claude/.This PR adds a per-agent Reset session permissions button in Settings that clears the rules for the current worktree.
Server (
POST /api/agents/:agentId/session-permissions/reset)CodexAppServerManager.resetAllSessions()terminates the livecodex app-serverchild and clears every thread runtime. The next turn spawns a fresh app-server. Returns the count of runtimes dropped.stopSessionRuntime, and the worktree is recorded in a per-processrevokedWorktreesset (server/lib/session-permissions.ts). On the next session start,consumeClaudeSessionRevocationdropsresumeSessionIdso Claude starts a fresh session id — losing session-scopedupdatedPermissionsbut leaving the conversation history alone. This is the only revocation path the Claude control protocol exposes (there's no public "remove rules" message).400 on missing
projectId/worktreeId; 404 on unknown worktree / agent.Client
agents-section.tsx, with anAlertDialogconfirmation that explains the per-agent cost (Codex: "session may end mid-turn"; Claude: "conversation context is reset"; Anita: "no permission prompts yet").App.tsxthreadspreSettingsViewRef.current(the view the user came from) intoSettingsPage→AgentsSectionasworktreeContext.Tests
server/lib/__tests__/session-permissions.test.ts— per-worktree revocation flag (set, consume-once, scoped per worktree, re-markable after consume).server/lib/__tests__/agents-reset.test.ts— route handler: 400 on missing fields, 404 on unknown worktree/agent, 200 for each provider with the right counters and flag side-effect.501/501 tests pass (was 492);
tsc -p server/tsconfig.jsonclean;vite buildclean.Manual verification checklist (to be filled in)