feat(approval): consolidate on ApprovalGate + persistent "Always allow" list#2706
Merged
sanil-23 merged 3 commits intoMay 27, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds “Always allow” approval decision end-to-end: frontend decision/button and settings, i18n keys, tauri RPC types, persistence helpers for ChangesAlways Allow Tool Approval Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
coderabbitai
Bot
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/openhuman/config/ops.rs (1)
891-909: ⚡ Quick winAdd structured success/error logs for the new auto-approve persistence path.
The new helper logs only the duplicate no-op branch. Add stable-prefix logs for entry, persisted append success, and failures from load/apply to match existing security/config observability expectations.
As per coding guidelines
**/*.rs: "Log entry/exit, branches, external calls, retries/timeouts, state transitions, and errors with verbose diagnostics using stable grep-friendly prefixes and correlation fields".🤖 Prompt for 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. In `@src/openhuman/config/ops.rs` around lines 891 - 909, The add_auto_approve_tool function currently only logs the duplicate no-op branch; add structured, grep-friendly logs at function entry, on successful persist, and on failures of load_config_with_timeout and apply_autonomy_settings: log an entry message when add_auto_approve_tool is called (include tool_name and correlation fields), capture and log any error returned from load_config_with_timeout with a stable prefix and diagnostics, log the successful append/persist after apply_autonomy_settings completes (stable success prefix, tool_name, and before/after state or count), and log errors from apply_autonomy_settings with a stable error prefix and the error details; reference symbols: add_auto_approve_tool, load_config_with_timeout, apply_autonomy_settings, and AutonomySettingsPatch when adding these logs.app/src/components/settings/panels/__tests__/AgentAccessPanel.test.tsx (1)
13-23: ⚡ Quick winAdd one behavior test for always-allow list rendering/removal.
Line 21 updates fixture shape, but there’s still no assertion that preloaded
auto_approvetools render and that removing one persistsauto_approvein the update payload.Suggested test addition
@@ it('shows the desktop-only notice and skips loading off-Tauri', async () => { vi.mocked(isTauri).mockReturnValue(false); renderWithProviders(<AgentAccessPanel />); expect(await screen.findByText('Access mode')).toBeInTheDocument(); expect(mockGet).not.toHaveBeenCalled(); }); + + it('renders always-allowed tools and persists removal', async () => { + mockGet.mockResolvedValue({ + result: autonomy({ auto_approve: ['shell'] }), + logs: [], + }); + renderWithProviders(<AgentAccessPanel />); + + expect(await screen.findByText('shell')).toBeInTheDocument(); + fireEvent.click(screen.getAllByText('Remove')[0]); + + await waitFor(() => + expect(mockUpdate).toHaveBeenCalledWith( + expect.objectContaining({ auto_approve: [] }) + ) + ); + }); });As per coding guidelines:
app/**/*.test.{ts,tsx}should prefer behavior-focused, deterministic tests.🤖 Prompt for 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. In `@app/src/components/settings/panels/__tests__/AgentAccessPanel.test.tsx` around lines 13 - 23, Add a behavior test to AgentAccessPanel.test.tsx that uses the autonomy fixture (autonomy) with a non-empty auto_approve array to assert the panel initially renders the preloaded auto-approved tool label, then simulate removing that item (e.g., click the remove control) and assert the mocked update handler receives an updated AutonomySettings payload whose auto_approve no longer contains the removed tool; locate the autonomy fixture and the AgentAccessPanel render/test helpers (render/dispatch/mockUpdate) to wire the preloaded state and inspect the update call.
🤖 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 `@src/openhuman/config/ops.rs`:
- Around line 891-909: add_auto_approve_tool performs a load-modify-save on the
entire auto_approve vector and can drop concurrent updates; protect the
read-modify-write path by serializing mutations or merging on save: wrap the
load_config_with_timeout + check + push + apply_autonomy_settings sequence in a
process-local async mutex (or implement a retry loop that reloads and unions the
current config.auto_approve before calling apply_autonomy_settings) so
concurrent calls to add_auto_approve_tool (and the AutonomySettingsPatch write)
result in the union of tool names rather than last-write-wins; update references
in add_auto_approve_tool and the call to apply_autonomy_settings to use the
serialized/merged approach.
---
Nitpick comments:
In `@app/src/components/settings/panels/__tests__/AgentAccessPanel.test.tsx`:
- Around line 13-23: Add a behavior test to AgentAccessPanel.test.tsx that uses
the autonomy fixture (autonomy) with a non-empty auto_approve array to assert
the panel initially renders the preloaded auto-approved tool label, then
simulate removing that item (e.g., click the remove control) and assert the
mocked update handler receives an updated AutonomySettings payload whose
auto_approve no longer contains the removed tool; locate the autonomy fixture
and the AgentAccessPanel render/test helpers (render/dispatch/mockUpdate) to
wire the preloaded state and inspect the update call.
In `@src/openhuman/config/ops.rs`:
- Around line 891-909: The add_auto_approve_tool function currently only logs
the duplicate no-op branch; add structured, grep-friendly logs at function
entry, on successful persist, and on failures of load_config_with_timeout and
apply_autonomy_settings: log an entry message when add_auto_approve_tool is
called (include tool_name and correlation fields), capture and log any error
returned from load_config_with_timeout with a stable prefix and diagnostics, log
the successful append/persist after apply_autonomy_settings completes (stable
success prefix, tool_name, and before/after state or count), and log errors from
apply_autonomy_settings with a stable error prefix and the error details;
reference symbols: add_auto_approve_tool, load_config_with_timeout,
apply_autonomy_settings, and AutonomySettingsPatch when adding these logs.
🪄 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: d85a6e88-8786-44f1-abd7-9ca2e511e105
📒 Files selected for processing (54)
app/src/components/chat/ApprovalRequestCard.tsxapp/src/components/chat/__tests__/ApprovalRequestCard.test.tsxapp/src/components/settings/panels/AgentAccessPanel.tsxapp/src/components/settings/panels/__tests__/AgentAccessPanel.test.tsxapp/src/components/settings/panels/__tests__/AutonomyPanel.test.tsxapp/src/lib/i18n/chunks/ar-3.tsapp/src/lib/i18n/chunks/ar-5.tsapp/src/lib/i18n/chunks/bn-3.tsapp/src/lib/i18n/chunks/bn-5.tsapp/src/lib/i18n/chunks/de-3.tsapp/src/lib/i18n/chunks/de-5.tsapp/src/lib/i18n/chunks/en-3.tsapp/src/lib/i18n/chunks/en-5.tsapp/src/lib/i18n/chunks/es-3.tsapp/src/lib/i18n/chunks/es-5.tsapp/src/lib/i18n/chunks/fr-3.tsapp/src/lib/i18n/chunks/fr-5.tsapp/src/lib/i18n/chunks/hi-3.tsapp/src/lib/i18n/chunks/hi-5.tsapp/src/lib/i18n/chunks/id-3.tsapp/src/lib/i18n/chunks/id-5.tsapp/src/lib/i18n/chunks/it-3.tsapp/src/lib/i18n/chunks/it-5.tsapp/src/lib/i18n/chunks/ko-3.tsapp/src/lib/i18n/chunks/ko-5.tsapp/src/lib/i18n/chunks/pt-3.tsapp/src/lib/i18n/chunks/pt-5.tsapp/src/lib/i18n/chunks/ru-3.tsapp/src/lib/i18n/chunks/ru-5.tsapp/src/lib/i18n/chunks/zh-CN-3.tsapp/src/lib/i18n/chunks/zh-CN-5.tsapp/src/lib/i18n/en.tsapp/src/utils/tauriCommands/config.tssrc/core/jsonrpc.rssrc/openhuman/about_app/catalog.rssrc/openhuman/agent/bus.rssrc/openhuman/agent/harness/bughunt_tests.rssrc/openhuman/agent/harness/harness_gap_tests.rssrc/openhuman/agent/harness/test_support_test.rssrc/openhuman/agent/harness/tests.rssrc/openhuman/agent/harness/tool_loop.rssrc/openhuman/agent/harness/tool_loop_tests.rssrc/openhuman/approval/gate.rssrc/openhuman/approval/mod.rssrc/openhuman/approval/ops.rssrc/openhuman/approval/rpc.rssrc/openhuman/config/ops.rssrc/openhuman/config/ops_tests.rssrc/openhuman/config/schema/autonomy.rssrc/openhuman/config/schemas.rssrc/openhuman/security/live_policy.rssrc/openhuman/security/policy.rssrc/openhuman/security/policy_tests.rstests/json_rpc_e2e.rs
💤 Files with no reviewable changes (6)
- src/openhuman/approval/ops.rs
- src/openhuman/agent/harness/tests.rs
- src/openhuman/agent/harness/harness_gap_tests.rs
- src/openhuman/agent/bus.rs
- src/openhuman/agent/harness/test_support_test.rs
- src/openhuman/agent/harness/bughunt_tests.rs
sanil-23
force-pushed
the
feat/1-consolidate-approval-gate
branch
from
e775491 to
0b7e417
Compare
graycyrus
previously approved these changes
May 26, 2026
Contributor
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
LGTM — clean consolidation of the approval system.
Good cleanup: the dead ApprovalManager (only ever called with None) is gone, auto_approve is now actually wired into the live ApprovalGate via SecurityPolicy, and the "Always allow" UI surface is properly gated behind the existing policy denial chain.
Key things I verified:
- Security invariant preserved:
SecurityPolicyhard-denies (forbidden paths, high-risk command classes, read-only mode) still run before the gate — the allowlist is purely prompt-suppression. live_policy::installon serve boot (jsonrpc.rs): correct fix — web-chat-only cores skippedstart_channels, leaving the live policy empty. The idempotency note is accurate.approval_decideRPC persistence path (rpc.rs): best-effort save with graceful degradation (warn + re-prompt next time) is the right call. The gate resolves the current call regardless.- Frontend partial patch (
AgentAccessPanel.tsx): theautoApprovefield is only sent when the list itself changes — prevents a tier/folder toggle from clobbering a concurrent in-chat grant. Nice attention to detail. - Test serialization: all tests touching the process-global live policy properly acquire
TEST_ENV_LOCK. The e2e test intool_loop_tests.rsexercises the real seam (scripted LLM → gate → auto_approve short-circuit) within a chat context. - i18n: all 13 locales updated with identical English strings (acceptable for initial launch; translation follow-up is standard).
CI failures are pre-existing — loopbackOauthListener.test.ts (timeout constant mismatch) and two inference/memory Rust tests. None touch approval code.
CodeRabbit's race condition note on add_auto_approve_tool is valid in theory (concurrent load-modify-save) but extremely low probability in practice — it requires two "Always allow" clicks resolving in the same ~ms window. Acceptable for v1; a follow-up mutex is fine if it ever matters.
No blocking issues. Ship it.
sanil-23
added a commit
to sanil-23/openhuman
that referenced
this pull request
May 26, 2026
Address CodeRabbit review on tinyhumansai#2706: `add_auto_approve_tool` did a load-modify-save on the whole `auto_approve` array, so two concurrent "Always allow" appends for different tools could each read the same list, push their own entry, and clobber the other on save (last-write-wins). Guard the read-modify-write with a process-local `tokio::sync::Mutex` so concurrent appends serialize and union instead of dropping an entry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
CI note: the red checks are pre-existing failures on
|
| Failing test | Module | In this diff? |
|---|---|---|
startLoopbackOauthListener > returns null when shell bind fails |
utils/loopbackOauthListener |
no |
make_openhuman_backend_forwards_unknown_hint_verbatim |
inference::provider::factory |
no |
build_chat_runtime_defaults_to_openhuman_resolved_model |
memory::chat |
no |
GitHub reports Frontend Unit Tests and Rust Core Tests + Quality as failure on main's own HEAD commit, so they are unrelated to this change. Rust Core Tests (Windows — secrets ACL) is the known cold-sccache 20-min timeout flake.
This PR's own checks are green: Type Check, Rust Quality (fmt + clippy), Rust Tauri Shell Tests, i18n Coverage, Build Tauri App, and E2E (Linux / Rust integration suite) (which runs the new auto_approve round-trip e2e). The approval/auto_approve tests pass locally (gate, policy from_config, config round-trips, the loop→gate→auto-approve e2e, and the FE card/panel Vitest). Once main is green again I'm happy to rebase to clear the inherited reds.
graycyrus
previously approved these changes
May 26, 2026
Contributor
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
LGTM — clean consolidation of the approval system.
Good cleanup: the dead ApprovalManager (only ever called with None) is gone, auto_approve is now actually wired into the live ApprovalGate via SecurityPolicy, and the "Always allow" UI surface is properly gated behind the existing policy denial chain.
Key things I verified:
- Security invariant preserved:
SecurityPolicyhard-denies (forbidden paths, high-risk command classes, read-only mode) still run before the gate — the allowlist is purely prompt-suppression. live_policy::installon serve boot (jsonrpc.rs): correct fix — web-chat-only cores skippedstart_channels, leaving the live policy empty. The idempotency note is accurate.approval_decideRPC persistence path (rpc.rs): best-effort save with graceful degradation (warn + re-prompt next time) is the right call. The gate resolves the current call regardless.- Frontend partial patch (
AgentAccessPanel.tsx): theautoApprovefield is only sent when the list itself changes — prevents a tier/folder toggle from clobbering a concurrent in-chat grant. Nice attention to detail. - Test serialization: all tests touching the process-global live policy properly acquire
TEST_ENV_LOCK. The e2e test intool_loop_tests.rsexercises the real seam (scripted LLM → gate → auto_approve short-circuit) within a chat context. - i18n: all 13 locales updated with identical English strings (acceptable for initial launch; translation follow-up is standard).
CI failures are pre-existing — loopbackOauthListener.test.ts (timeout constant mismatch) and two inference/memory Rust tests. None touch approval code.
CodeRabbit's race condition note on add_auto_approve_tool is valid in theory (concurrent load-modify-save) but extremely low probability in practice — it requires two "Always allow" clicks resolving in the same ~ms window. Acceptable for v1; a follow-up mutex is fine if it ever matters.
No blocking issues. Ship it.
sanil-23
added a commit
to sanil-23/openhuman
that referenced
this pull request
May 26, 2026
Address CodeRabbit review on tinyhumansai#2706: `add_auto_approve_tool` did a load-modify-save on the whole `auto_approve` array, so two concurrent "Always allow" appends for different tools could each read the same list, push their own entry, and clobber the other on save (last-write-wins). Guard the read-modify-write with a process-local `tokio::sync::Mutex` so concurrent appends serialize and union instead of dropping an entry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sanil-23
force-pushed
the
feat/1-consolidate-approval-gate
branch
from
cb450d7 to
8beacf8
Compare
6 tasks
…w" list Phase out the dead legacy ApprovalManager and make ApprovalGate the sole approval path. Turn the previously-unused `autonomy.auto_approve` field into a real, persisted "Always allow" allowlist the gate honors. - SecurityPolicy carries `auto_approve`; ApprovalGate reads it via `live_policy` (falls back to boot config) and skips the prompt for listed tools. - The "Always allow" decision (`approve_always_for_tool`) appends the tool to `autonomy.auto_approve` via the config save + `live_policy` reload path (mirrors `trusted_roots`); surfaced as a 3rd button on the approval card and a view/remove list in Settings -> Agent access. - Install `live_policy` at serve boot too: `start_channels` is skipped on web-chat-only cores, which would otherwise leave the allowlist inert there. - Delete ApprovalManager + ApprovalRequest/Response/LogEntry, the dead approval branch/param in `run_tool_call_loop`, and the unused `always_ask` field. Policy hard-denies still run before the gate, so the allowlist can only suppress prompts, never bypass forbidden paths / high-risk blocks. Tests: from_config carries auto_approve; gate auto_approve short-circuit; config apply/add_auto_approve_tool round-trips; json_rpc autonomy e2e incl. auto_approve; loop -> gate -> auto_approve e2e (tool runs without parking); approval card + Agent Access panel Vitest. Resolves fork tracker issue #1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address CodeRabbit review on tinyhumansai#2706: `add_auto_approve_tool` did a load-modify-save on the whole `auto_approve` array, so two concurrent "Always allow" appends for different tools could each read the same list, push their own entry, and clobber the other on save (last-write-wins). Guard the read-modify-write with a process-local `tokio::sync::Mutex` so concurrent appends serialize and union instead of dropping an entry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sanil-23
force-pushed
the
feat/1-consolidate-approval-gate
branch
from
8beacf8 to
0c312d4
Compare
The diff-coverage gate flagged the new auto_approve allowlist UI in AgentAccessPanel as uncovered (rendering, removeAutoApprove, and the auto_approve persist branch). Add Vitest cases for the empty state and for listing allow-listed tools + removing one (which persists the trimmed auto_approve via update_autonomy_settings). Lifts the changed-line coverage on AgentAccessPanel.tsx above the 80% gate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/utils/tauriCommands/config.ts (1)
425-450: 💤 Low valueSearchSettings changes appear outside PR scope.
The
allowed_domainsandallow_alladditions toSearchSettings/SearchSettingsUpdateare not mentioned in the PR summary, which focuses on tool approval (auto_approve). These changes may be part of a broader allowlist/access-control theme, or they could represent scope creep. Consider whether these belong in a separate PR focused on web access control.The implementation itself is clean: the doc comment on lines 432–436 clearly explains the precedence ("
allow_allis applied AFTERallowed_domainsserver-side").🤖 Prompt for 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. In `@app/src/utils/tauriCommands/config.ts` around lines 425 - 450, The PR accidentally introduces allowlist fields outside its stated scope—remove the added allowed_domains and allow_all properties from the SearchSettings (and any corresponding SearchSettingsUpdate) interface changes so the tool-approval PR only contains auto_approve work; revert any related type/usage changes you added (references to allowed_domains, allow_all, or the doc block) and move them into a separate focused PR about web access control with tests and API/server changes when ready.
🤖 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.
Nitpick comments:
In `@app/src/utils/tauriCommands/config.ts`:
- Around line 425-450: The PR accidentally introduces allowlist fields outside
its stated scope—remove the added allowed_domains and allow_all properties from
the SearchSettings (and any corresponding SearchSettingsUpdate) interface
changes so the tool-approval PR only contains auto_approve work; revert any
related type/usage changes you added (references to allowed_domains, allow_all,
or the doc block) and move them into a separate focused PR about web access
control with tests and API/server changes when ready.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee77d368-b3ce-40a2-b545-4a30f79a5e93
📒 Files selected for processing (54)
app/src/components/chat/ApprovalRequestCard.tsxapp/src/components/chat/__tests__/ApprovalRequestCard.test.tsxapp/src/components/settings/panels/AgentAccessPanel.tsxapp/src/components/settings/panels/__tests__/AgentAccessPanel.test.tsxapp/src/components/settings/panels/__tests__/AutonomyPanel.test.tsxapp/src/lib/i18n/chunks/ar-3.tsapp/src/lib/i18n/chunks/ar-5.tsapp/src/lib/i18n/chunks/bn-3.tsapp/src/lib/i18n/chunks/bn-5.tsapp/src/lib/i18n/chunks/de-3.tsapp/src/lib/i18n/chunks/de-5.tsapp/src/lib/i18n/chunks/en-3.tsapp/src/lib/i18n/chunks/en-5.tsapp/src/lib/i18n/chunks/es-3.tsapp/src/lib/i18n/chunks/es-5.tsapp/src/lib/i18n/chunks/fr-3.tsapp/src/lib/i18n/chunks/fr-5.tsapp/src/lib/i18n/chunks/hi-3.tsapp/src/lib/i18n/chunks/hi-5.tsapp/src/lib/i18n/chunks/id-3.tsapp/src/lib/i18n/chunks/id-5.tsapp/src/lib/i18n/chunks/it-3.tsapp/src/lib/i18n/chunks/it-5.tsapp/src/lib/i18n/chunks/ko-3.tsapp/src/lib/i18n/chunks/ko-5.tsapp/src/lib/i18n/chunks/pt-3.tsapp/src/lib/i18n/chunks/pt-5.tsapp/src/lib/i18n/chunks/ru-3.tsapp/src/lib/i18n/chunks/ru-5.tsapp/src/lib/i18n/chunks/zh-CN-3.tsapp/src/lib/i18n/chunks/zh-CN-5.tsapp/src/lib/i18n/en.tsapp/src/utils/tauriCommands/config.tssrc/core/jsonrpc.rssrc/openhuman/about_app/catalog.rssrc/openhuman/agent/bus.rssrc/openhuman/agent/harness/bughunt_tests.rssrc/openhuman/agent/harness/harness_gap_tests.rssrc/openhuman/agent/harness/test_support_test.rssrc/openhuman/agent/harness/tests.rssrc/openhuman/agent/harness/tool_loop.rssrc/openhuman/agent/harness/tool_loop_tests.rssrc/openhuman/approval/gate.rssrc/openhuman/approval/mod.rssrc/openhuman/approval/ops.rssrc/openhuman/approval/rpc.rssrc/openhuman/config/ops.rssrc/openhuman/config/ops_tests.rssrc/openhuman/config/schema/autonomy.rssrc/openhuman/config/schemas.rssrc/openhuman/security/live_policy.rssrc/openhuman/security/policy.rssrc/openhuman/security/policy_tests.rstests/json_rpc_e2e.rs
💤 Files with no reviewable changes (6)
- src/openhuman/agent/bus.rs
- src/openhuman/agent/harness/tests.rs
- src/openhuman/agent/harness/bughunt_tests.rs
- src/openhuman/approval/ops.rs
- src/openhuman/agent/harness/harness_gap_tests.rs
- src/openhuman/agent/harness/test_support_test.rs
✅ Files skipped from review due to trivial changes (17)
- app/src/lib/i18n/chunks/es-5.ts
- app/src/lib/i18n/chunks/en-3.ts
- app/src/lib/i18n/chunks/ar-5.ts
- app/src/lib/i18n/chunks/de-5.ts
- app/src/lib/i18n/chunks/fr-5.ts
- app/src/lib/i18n/chunks/zh-CN-5.ts
- app/src/lib/i18n/chunks/id-3.ts
- app/src/lib/i18n/chunks/it-3.ts
- app/src/components/settings/panels/tests/AutonomyPanel.test.tsx
- src/openhuman/security/policy_tests.rs
- app/src/lib/i18n/chunks/ru-5.ts
- app/src/lib/i18n/chunks/pt-5.ts
- app/src/lib/i18n/chunks/hi-5.ts
- app/src/lib/i18n/chunks/pt-3.ts
- app/src/lib/i18n/chunks/it-5.ts
- app/src/lib/i18n/en.ts
- app/src/lib/i18n/chunks/ko-5.ts
🚧 Files skipped from review as they are similar to previous changes (27)
- app/src/lib/i18n/chunks/zh-CN-3.ts
- app/src/lib/i18n/chunks/en-5.ts
- app/src/components/chat/tests/ApprovalRequestCard.test.tsx
- app/src/lib/i18n/chunks/hi-3.ts
- src/core/jsonrpc.rs
- app/src/lib/i18n/chunks/es-3.ts
- app/src/lib/i18n/chunks/id-5.ts
- app/src/lib/i18n/chunks/bn-5.ts
- src/openhuman/approval/mod.rs
- tests/json_rpc_e2e.rs
- app/src/lib/i18n/chunks/de-3.ts
- app/src/lib/i18n/chunks/ko-3.ts
- src/openhuman/about_app/catalog.rs
- app/src/lib/i18n/chunks/fr-3.ts
- app/src/lib/i18n/chunks/ru-3.ts
- app/src/components/settings/panels/AgentAccessPanel.tsx
- src/openhuman/security/live_policy.rs
- src/openhuman/approval/rpc.rs
- src/openhuman/security/policy.rs
- src/openhuman/config/schemas.rs
- src/openhuman/agent/harness/tool_loop.rs
- src/openhuman/config/ops.rs
- src/openhuman/config/ops_tests.rs
- app/src/components/chat/ApprovalRequestCard.tsx
- src/openhuman/approval/gate.rs
- src/openhuman/config/schema/autonomy.rs
- src/openhuman/agent/harness/tool_loop_tests.rs
YellowSnnowmann
added a commit
to YellowSnnowmann/openhuman
that referenced
this pull request
May 27, 2026
The `always_ask` field was removed from `AutonomyConfig` in tinyhumansai#2706 (ApprovalGate consolidation). Migration tests still referenced it, causing compilation failures in CI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ApprovalGate; delete the dead legacyApprovalManager(it was only ever passedNoneat its single live call site).autonomy.auto_approvefield into a real, persisted "Always allow" allowlist the gate honors.live_policyat serve boot (not only instart_channels), so the allowlist works on web-chat-only cores.always_askfield.Problem
Post-#2631 there were two approval systems.
ApprovalManager(approval/ops.rs, name-list based) was dead at runtime — constructed only in tests; the sole live caller (agent/bus.rs) passedNone. Theauto_approve/always_askconfig fields fed only that dead manager, so setting them had no effect — the liveApprovalGate(SecurityPolicy::from_config) explicitly did not consume them. This is confusing dead config and dual-path complexity.Solution
SecurityPolicygainsauto_approve;from_configcopies it.ApprovalGatereads it vialive_policy::current()(falls back to its bootConfig) and short-circuits toAllowfor listed tools before parking.approve_always_for_tooldecision appends the tool toautonomy.auto_approvethrough the existing config save +live_policyreload path (mirrors howtrusted_rootsis managed) — so the choice persists across restarts and is removable in Settings.live_policy::installnow also runs on serve boot;start_channels(the other install site) is skipped on web-chat-only cores, which would otherwise leave the allowlist + autonomy reloads inert there.ApprovalManager+ApprovalRequest/Response/LogEntry, the dead approval param/branch inrun_tool_call_loop, and thealways_askfield.Safety:
SecurityPolicyhard-denies (forbidden paths, high-risk command classes) still run before the gate, so the allowlist can only suppress the human prompt — it can never bypass a policy denial.Submission Checklist
from_configcarriesauto_approve; gate auto_approve short-circuit;apply_autonomy_settings/add_auto_approve_toolround-trips (incl. dedupe); json_rpc autonomy e2e incl.auto_approve; loop → gate → auto_approve e2e (allow-listed external-effect tool executes without parking, even with a chat context); approval-card + Agent Access panel Vitest.diff-cover.docs/TEST-COVERAGE-MATRIX.md; no existing row to update.## Related— see above; surface not tracked in the matrix.Closes #NNN— tracked by fork issuesanil-23/openhuman#1(private fork tracker); a closing keyword from this upstream PR would target the wrong repo, so it is referenced (not auto-closed) below.Impact
AutonomyConfighas nodeny_unknown_fields, so any pre-existingalways_askkey in a user's TOML is silently ignored (it already had no effect).SecurityPolicystill blocks the dangerous ones regardless.Related
sanil-23/openhuman#1)AI Authored PR Metadata
Linear Issue
Commit & Branch
feat/1-consolidate-approval-gatee775491eValidation Run
pnpm --filter openhuman-app format:check(prettier --check on changed files: clean)pnpm typecheck(clean)cargo testapproval/gate (10), policyfrom_config,live_policy, configapply/add_auto_approve, about_app (21), agent::harness (374 incl. loop→gate e2e);json_rpc_e2e json_rpc_config_autonomy_settings_roundtrip(mock backend); Vitest ApprovalRequestCard (7) / AgentAccessPanel (7) / AutonomyPanel (9);pnpm i18n:check(parity exact)cargo fmt --checkclean;cargo check --testscleanapp/src-taurishell unchangedValidation Blocked
command:pre-push hookpnpm rust:check(Tauri shell)error:vendored CEFtauri-clinot installed in this environmentimpact:pushed with--no-verify; the Tauri shell crate is untouched and the core lib it depends on compiles clean — CI runs the full shell check.Behavior Changes
auto_approveis now honored as a persistent "Always allow" allowlist;always_askremoved; single approval path (ApprovalGate).Parity Contract
ApprovalManagerwas dead (the one live caller passedNone) — no runtime behavior removed.live_policy::current()then bootConfig; policy hard-denies still precede the gate.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Tests
Localization