feat(approvals): once, this chat and always tool approval scopes - #273
Conversation
Plain parent workspace commands and file writes in Ask workspaces can now be allowed once, for this chat, or always. Rules match only the same tool with the exact trimmed command or exact normalized workspace-relative path in the same workspace, so a rule is never broader than the approval card. - Main: ToolApprovalRuleBook (in-memory chat rules, persisted always rules, strict normalization), coordinator scope fencing, llm-client matching and granting, and IPC to list and revoke rules. - Desktop: approval card scope buttons and Settings -> Tool approvals. - Aiden Remote contract revision 16: PendingApproval.scopes and an optional allow-only scope on approval responses, echoed back. - iOS and Android: "More allow options" menus that send an offered scope. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The new path normalization lets a remembered approval match a different file; please fix this before merging.
Reviewed changes This review covers the approval-scope implementation across the desktop main process and UI, Aiden Remote, and the iOS and Android clients.
- Remembered approval scopes. Adds once, chat, and always choices for the parent workspace command and file-mutation tools, with local persistence and revocation in Settings.
- Remote approval contract. Adds optional offered scopes to revision 16, validates and fingerprints scoped responses, and exposes the choices in both native clients.
- Approval fencing. Limits remembered rules to exact workspace/tool/target keys and keeps specialized, Bot, and structured approvals once-only.
⚠️ Deleted chats retain remembered targets
Deleting a chat never clears its chat-scoped rules, leaving up to 100 exact command or path strings per deleted chat resident until the app quits. The per-chat limit therefore does not bound accumulation across deleted chats in a long-running session.
Technical details
# Clear chat-scoped approvals on deletion
## Affected sites
- [main/services/tool-approval-rules.ts](https://github.com/sambitcreate/aiden-agent/blob/baaa33c5059a3bb26b1e251e4852452d812c724a/main/services/tool-approval-rules.ts#L276-L279) — `forgetChat` is added but has no production caller.
- [main/services/chat-application-service.ts](https://github.com/sambitcreate/aiden-agent/blob/baaa33c5059a3bb26b1e251e4852452d812c724a/main/services/chat-application-service.ts#L315-L320) — successful deletion commits at `chatStore.remove` before completion.
## Required outcome
- Clear a chat's in-memory approval rules after deletion succeeds, while retaining them if deletion fails or rolls forward.GPT Luna ｜ 𝕏
Hermes Review BotConfidence: 4 Engine: SummaryThis change introduces scoped tool approvals ( Confidence Score: 4/54/5: Traced end-to-end across main process, IPC, remote protocol revision 17, mobile clients, and settings view; identified error-state clearing bug in settings revoke action. 📁 Important Files Changed
Findings
📊 Sequence DiagramsequenceDiagram
participant UI as ChatPane / Mobile Client
participant Main as llmClient (beforeToolCall)
participant Coord as ToolApprovalCoordinator
participant Book as ToolApprovalRuleBook (DataStore / Memory)
Main->>Book: match(chatId, target)
alt Rule remembered (chat or always)
Book-->>Main: "chat" | "always"
Main-->>Main: Skip prompt & run tool
else No rule remembered
Main->>Coord: request(prompt with scopes: ["once", "chat", "always"])
Coord->>UI: chat:approval (prompt with scopes)
UI->>Coord: decide(approvalId, allow, scope)
Coord-->>Main: approvalOutcome = "allowed"
Main->>Coord: takeDecisionPayload(streamId, toolCallId)
Coord-->>Main: { scope: "chat" | "always" }
Main->>Book: grant(scope, chatId, target, workspaceLabel)
Main-->>Main: Run tool
end
Machine-Readable Findings[
{
"file": "renderer/components/settings/tool-approval-settings.tsx",
"start_line": 126,
"end_line": 134,
"severity": "P2",
"comment_type": "logic",
"confidence": 0.95,
"title": "Revocation error is immediately erased by finally load()",
"mechanism": "When an action fails, catch sets the error message, but finally immediately executes await load(), which unconditionally calls setError(null) upon successfully reading the rules list, preventing error feedback from displaying to the user.",
"repair": "Move await load() inside the try block so reloading only occurs after successful mutations and does not overwrite the caught error.",
"id": "aab5c5b3fad688c5"
}
]
|
|
Run failed. View the logs →
|
very-hermes-bot
left a comment
very-hermes-bot
left a comment
There was a problem hiding this comment.
Inline review — 2 finding(s) anchored to the diff. See the pinned summary comment for the overview.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes This re-review covers the delta since the prior Pullfrog review (baaa33c5 → ac210eb4): one commit fixing the path-trimming concern raised in that review, plus its regression test and a memory-file note.
- Preserved whitespace in remembered file targets.
normalizePathPatternno longer trims, so distinct filesystem names (" notes.md"vs"notes.md") can no longer share an approval key. The regression test covers persisted grant/match behavior and would have failed under the old trimming; all seven rule-book tests pass on this head, and the touched files lint and typecheck cleanly. - Verified equivalence on both sides of the rule.
llm-clientpasses raw tool args to the remembered-rule lookup and to the post-allow grant, so the target computed at prompt time and the target granted after an allow are built by the same normalization from the same raw input — no caller reintroduces the collapse, and persisted documents still reject non-canonical patterns.
The remaining open thread (deleted chats retaining in-memory chat rules until quit) is unchanged by this commit, author-recorded as a follow-up in the PR body, and bounded per chat in memory — not a merge blocker.
GLM Flash ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes This re-review covers the delta since the prior Pullfrog review (ac210eb4 → a7b4f34): one commit hardening the remembered-rule normalizers in response to the open inline review findings, plus its regression test and a memory-file note.
- Rejected parent segments in remembered file targets.
normalizePathPatternnow rejects any path containing a literal..segment before normalization, so traversal-shaped arguments stay once-only instead of being collapsed to a canonical pattern that could merge distinct targets (the P1 finding). The check runs on the raw input, and persisted documents with such patterns are dropped at load through the same normalizer. - Closed the control-character bypass in command rule targets.
normalizeCommandPatternnow inspects the raw, untrimmed value and permits only tab, newline, and carriage return among control characters — previously, embedded ANSI escapes or bells passed andtrim()silently stripped edge controls, so the old check could be defeated in both positions (the P2 finding). - Added regression coverage for both hardenings. The new test asserts
..paths in interior, doubled, and leading positions and seven control characters in embedded and leading positions all yield no rule target, while tab/newline commands still qualify — the assertions fail against the previous code. All 8 rule-book tests pass on this head, the touched files lint cleanly, and both new test files remain registered in the roottestchain and the CI registry. Match-side consistency was re-verified:llm-clientcomputes the target once and reuses it for the remembered-rule lookup and the post-allow grant, so an unsafe path can neither offer scopes nor match a persisted rule.
The remaining open thread (deleted chats retaining in-memory chat rules until quit) is unchanged by this commit, author-recorded as a follow-up in the PR body, and bounded per chat in memory — not a merge blocker.
GLM Flash ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes This re-review covers the delta since the prior Pullfrog review (a7b4f34 → 647df59): one commit extending the parent-segment rejection in remembered file targets to Windows and mixed separators, plus regression coverage.
- Closed the backslash gap in parent-segment rejection. The pre-normalization check in
normalizePathPatternnow splits on both/and\, so..\outside.txt,src\..\outside.txt, and mixed forms likesrc/..\outside.txtare rejected as rule targets and stay once-only, matching the documented contract that..paths are never remembered regardless of separator style. - Extended the regression test. The three new path assertions fail against the previous
split("/")check, which passed raw backslash traversal through to a stored pattern. All 8 rule-book tests pass on this head and the touched files lint cleanly. As before, the target is computed once inllm-clientand reused for both the remembered-rule lookup and the post-allow grant, so a rejected path can neither offer scopes nor match a persisted rule.
The previously noted follow-ups (calling forgetChat on chat deletion, live-refreshing Settings) remain author-documented and unchanged.
GLM Flash ｜ 𝕏
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # docs/aiden-remote-api-v1.md # package.json # scripts/ci-test-registry.json
There was a problem hiding this comment.
ℹ️ No critical issues — one minor PR-description mismatch below.
Reviewed changes This re-review covers the merge delta from 0ce43e5b to a6b16f77.
- Renumbered the Remote contract. Moved approval scopes to revision 17 after main's #275 agent-interrupt contract claimed revision 16, keeping the OpenAPI schema, fixtures, docs, and iOS/Android assertions in sync.
- Preserved test registration through the merge. Checked the root test-chain changes; subagent UI suites remain reached through
pretest:subagents, and the CI policy inventory passes.
ℹ️ Nitpicks
- The PR description still says the approval-scope contract is revision 16; this head uses revision 17 after syncing main. Please update the summary to match.
GPT Luna ｜ 𝕏
very-hermes-bot
left a comment
very-hermes-bot
left a comment
There was a problem hiding this comment.
Inline review — 1 finding(s) anchored to the diff. See the pinned summary comment for the overview.
| setError(actionError instanceof Error ? actionError.message : "Couldn’t revoke that rule."); | ||
| } finally { | ||
| setPending(null); | ||
| await load(); | ||
| } |
There was a problem hiding this comment.
[P2 · logic] Revoke failure error state is immediately cleared by post-action reload
Confidence: 0.95
When revoking an approval rule fails, the catch block sets the error state, but the finally block immediately runs load() which resets setError(null) upon fetching rules, erasing user feedback.
Repair: Use toast.error to display the failure notification or pass a flag to load() to avoid resetting error when an action fails.
very-hermes-bot
left a comment
very-hermes-bot
left a comment
There was a problem hiding this comment.
Inline review — 1 finding(s) anchored to the diff. See the pinned summary comment for the overview.
| try { | ||
| await action(); | ||
| toast.success(success); | ||
| } catch (actionError) { | ||
| setError(actionError instanceof Error ? actionError.message : "Couldn’t revoke that rule."); | ||
| } finally { | ||
| setPending(null); | ||
| await load(); | ||
| } |
There was a problem hiding this comment.
[P2 · logic] Revocation error is immediately erased by finally load()
Confidence: 0.95
When an action fails, catch sets the error message, but finally immediately executes await load(), which unconditionally calls setError(null) upon successfully reading the rules list, preventing error feedback from displaying to the user.
Repair: Move await load() inside the try block so reloading only occurs after successful mutations and does not overwrite the caught error.
2 participants
Summary
Adds three approval scopes for the parent agent's plain workspace tools (
run_command,write_file,edit_file) in Ask workspaces:A rule is keyed by
[workspaceId, toolName, exact pattern]. The pattern is the exact trimmed command, or the normalized workspace-relative path. There are no prefixes or globs, and absolute,~and..paths are only ever offered once. So a rule is never broader than what the card showed.Source: the wave's Tier 1/2 item "Tool approval scopes: once, this chat, always".
Changes per surface
ToolApprovalRuleBook(main/services/tool-approval-rules.ts) holds per-chat rules in memory (max 100 per chat, oldest evicted) and persists always rules totool-approval-rules.json(max 200).ToolApprovalCoordinatorhonors a scope only if it was offered, it is an allow, and it is a known name.llm-clientskips the prompt when a remembered rule matches, and grants the chosen scope after an allow.chat:listApprovalRules,chat:revokeApprovalRule,chat:revokeAllApprovalRules.chat:approvealso acceptsscope.docs/settings-design-system.md.PendingApproval.scopesis optional, has at least 2 entries, starts withonce, and is present only whencanAllow.POST /approvals/{id}/respondaccepts an optionalscope, but only withallowand only if it was offered. Otherwise it returns400 invalid_request. The response echoes the scope, and the scope is part of the idempotency fingerprint.docs/aiden-remote-api-v1.mdis updated too.Tests
main/services/tool-approval-rules.test.tscovers:renderer/components/settings/tool-approval-settings.test.tsxrenders the rows, revoke labels, busy, empty, loading and error states, plus the scope helpers.tool-approval.test.ts: scope fencing.aiden-remote-streams.test.ts: offered, allowed and chosen; idempotency conflict; host path.aiden-remote-router.test.ts: body validation and echo.aiden-remote-protocol.test.ts: Ajv against the fixture and request schema.remote-approval.test.tsandsettings-section.test.ts.testchain andscripts/ci-test-registry.json.tsc --noEmitand eslint on the touched files.test:aiden-remote,test:settings-design,test:ci-policyandtest:agents-instructions.AidenChatTests,AidenRemoteClientTests,AidenRemotePhase0Tests).AidenChatTest(56) andAidenBotContractTest(15).Notes
Follow-ups
ToolApprovalRuleBook.forgetChaton chat deletion. Chat rules are in memory and bounded today.onChange. Today it reloads on open and after each revoke.🤖 Generated with Claude Code