[DeJeune]

Branch strategy

• Active development targets main.

What this PR does

Before this PR: Claude settings construction mixed environment, MCP, lifecycle state, hooks, and permission policy; fixed approvals were maintained as parallel tool-name lists, and bypassPermissions still prompted inconsistently across runtimes.

After this PR: settings construction is split by responsibility, Claude conduct rules use one declarative evaluator, Cherry-owned MCP tools declare structured approval entries, and Full Access bypasses per-call approvals consistently across Claude, Pi, and DSH while explicit safety blocks remain enforced.

Fixes # N/A

Why we need it and why it was done in this way

The following tradeoffs were made: SDK and bridge arrays/sets remain as derived boundary formats, while the structured tool policy registry is the only maintained source; Assistant-only entries are filtered by actual MCP availability.

The following alternatives were considered: a mutable GoF Builder, a hook-per-rule directory, and attaching cross-runtime policy to the AI-SDK-only ToolEntry were rejected because they preserve the wrong ownership or synchronization boundary.

Links to places where the discussion took place: N/A

Breaking changes

Full Access (bypassPermissions) no longer shows per-call approval cards for knowledge-base edits, image generation, CLI installation, or Assistant settings/file tools on any Agent runtime. Users who relied on those prompts should switch the Agent to Approve for Me or Ask Before Acting; disabled tools, shared-global installation blocks, protected built-in destructive-operation blocks, and unattended responder requirements remain enforced.

Special notes for your reviewer

The seven commits intentionally separate mechanical responsibility extraction from the policy behavior change. Validation covered pnpm lint, pnpm test:lint, formatting, type checks, i18n, documentation links, all main/renderer/package tests, and the DSH bridge's independent test/build; transient local disk pressure and two unrelated timing/teardown flakes passed on rerun.

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

• Branch: This PR targets main
• PR: The PR description is expressive enough and will help future contributors
• Code: Write code that humans can understand and Keep it simple
• Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
• Upgrade: Impact of this change on upgrade flows was considered and addressed if required
• Documentation: A user-guide update was considered and is present (link) or not required. Check this only when the PR introduces or changes a user-facing feature or behavior.
• Self-review: I have reviewed my own code (e.g., via /gh-pr-review, gh pr diff, or GitHub UI) before requesting review from others

Release note

action required: Full Access now bypasses per-call approvals for knowledge-base edits, image generation, CLI installation, and Assistant settings/file tools across all Agent runtimes. Switch to Approve for Me or Ask Before Acting if those operations should still prompt.

[claude]

Code review complete. No actionable findings.

The declarative guard table architecture correctly separates policy from implementation, with explicit bypass behavior declarations. Safety blocks (disabled tools, global installs, protected built-in destructive operations, non-bypassable cross-Session delegation) remain enforced under Full Access across all runtimes. The lifecycle-managed session state service properly cleans up resources. Test coverage is comprehensive.

[claude]

Code review complete. No actionable findings.

The declarative guard table architecture correctly separates policy from implementation, with explicit bypass behavior declarations. Safety blocks (disabled tools, global installs, protected built-in destructive operations, non-bypassable cross-Session delegation) remain enforced under Full Access across all runtimes. The lifecycle-managed session state service properly cleans up resources. Test coverage is comprehensive.

[claude]

Security Observation: Fail-Open on Condition Exceptions

This catch block treats exceptions from guard conditions as "no match", which could bypass security rules if a condition throws unexpectedly.

Failure scenario:

1. A malformed input, filesystem error, or bug in a condition detector (e.g., detectGlobalInstall, isPathWithinAllowedRoots) causes an exception
2. The rule is skipped, potentially allowing a deny rule like global-install or workspace-escape to be bypassed
3. The tool proceeds with the runtime's default permission-mode semantics instead of the guard's intended protection

Impact:

• Deny rules (global-install, builtin-destructive) could be bypassed if their condition functions throw
• Ask rules could be skipped, allowing tools without prompts

Suggested consideration:

• For deny rules, consider whether failing closed (returning {} to match) would be safer
• At minimum, consider logging at error level rather than warn to ensure visibility of potential bypasses

Suggested change

	} catch (error) {
	} catch (error) {
	logger.error('Guard condition threw — treating as no match', { ruleId: rule.id, toolName: ctx.toolName, error })
	return null
	}

Note: The current conditions appear to internalize their own failure postures (e.g., treating unresolvable paths as outside the workspace), but this fail-open behavior is still worth explicit consideration for defense in depth.

[claude]

Code Clarity: Meaningless bypassBehavior

This rule has bypassBehavior: 'skipInteractiveEffect' but no effect field, making the bypassBehavior declaration meaningless.

Analysis:

• bypassBehavior: 'skipInteractiveEffect' only affects rules with an interactive effect (deny/ask)
• This rule has no effect field—it only has a headless override
• The skipHeadlessDenyInBypass: true on line 128 already handles the bypass case for the headless denial

Behavior verification (from tests):

• Interactive turn: no decision (undefined) — canUseTool handles it elsewhere
• Headless turn, no bypass: deny
• Headless turn, with bypass: undefined (allowed via skipHeadlessDenyInBypass)

The rule works correctly, but bypassBehavior is dead code here.

Suggested fix:
Either remove bypassBehavior or document this pattern if it's intentional for consistency with other rules.

Suggested change

	// Installing third-party skill code needs a responder — except under bypassPermissions, the
	{
	// Installing third-party skill code needs a responder — except under bypassPermissions, the
	// user's explicit opt-in to unattended installation.
	id: 'skill-install',
	bypassBehavior: 'enforce', // No interactive effect to skip; headless deny is explicitly lifted below
	match: { tool: 'mcp__skills__install_skill' },
	headless: {
	predicate: 'turn-headless',
	reason:
	'This channel or scheduled turn cannot approve a skill installation. Use bypassPermissions for unattended installation, or install it from an interactive turn.',
	skipHeadlessDenyInBypass: true
	}
	},

Or simply remove bypassBehavior since it has no effect on headless-only rules.

[vaayne]

This review was translated automatically.

Found 1 blocking cross-runtime security contract gap and 1 shared-layer architecture boundary issue. The current non-bypassable built-in Agent guard only integrates with Claude, while Pi and DSH's Full Access can still bypass similar restrictions, which is inconsistent with the PR documentation and the product contract in #18898. Suggest REQUEST_CHANGES: first integrate the hard limits into the evaluator shared by all three runtimes, and move tool names exclusively for Main use out of src/shared/.

---

Original Content

发现 1 个阻断性跨运行时安全契约缺口和 1 个 shared-layer 架构边界问题。当前不可绕过的 built-in Agent guard 只接入 Claude，Pi 与 DSH 的 Full Access 仍可绕过同类限制，与 PR 文档及 #18898 的产品契约不一致。建议 REQUEST_CHANGES：先将硬限制接入三个 runtime 共用的 evaluator，并将仅供 Main 使用的工具名称移出 src/shared/。

[vaayne]

This review was translated automatically.

LGTM. Re-review confirmed that original code findings have been closed, and no new correctness, security, or architecture issues were found in the incremental changes; cross-runtime permission unification continues to be tracked by #18898.

---

Original Content

LGTM. 复审确认原有代码 findings 已闭环，增量未发现新的 correctness、security 或 architecture 问题；跨 runtime 权限统一继续由 #18898 跟进。

[vaayne]

This review was translated automatically.

Non-blocking: The Breaking changes in the PR description still claims that protected built-in destructive-operation blocks and unattended responder requirements remain in effect under Full Access for all runtimes; the updated breaking-change document has clarified these restrictions as Claude-only, and left cross-runtime unification for #18898. Please sync the PR description to avoid making overly broad guarantees for Pi and DSH in the release notes. The code review conclusion remains approved.

---

Original Content

非阻断：PR 描述的 Breaking changes 仍声称 protected built-in destructive-operation blocks 和 unattended responder requirements 在所有 runtime 的 Full Access 下保持生效；更新后的 breaking-change 文档已经将这些限制明确为 Claude-only，并把跨 runtime 统一留给 #18898。请同步 PR 描述，避免 release note 对 Pi 和 DSH 作出过宽保证。代码复审结论仍为通过。

[claude]

Review Summary

This is a well-structured refactoring that unifies tool approval policy across runtimes. After reviewing the policy registry, guard table evaluation logic, runtime consistency, and session state management, I found no high-confidence correctness bugs, security issues, or contract violations.

Key verification points:

• ✅ Policy registry correctly classifies all tools with proper bypass semantics
• ✅ Guard table correctly implements deny > ask folding and bypass behavior
• ✅ All runtimes derive consistently from the centralized registry
• ✅ Session state service correctly manages session-keyed state
• ✅ Breaking change documentation is accurate and complete
• ✅ Tests comprehensively cover bypass and headless scenarios

The documented behavioral change (Full Access lifting per-call approvals) matches the implementation, and the known gap about guard coverage (#18898) is properly acknowledged.

[zhangjiadi225]

LGTM. The implementation centralizes Cherry-owned tool approval metadata, preserves the explicit non-bypassable limits, and provides a sound transition toward the shared evaluator tracked in #18898. One non-blocking documentation consistency note remains.

[zhangjiadi225]

[A1] Non-blocking: this paragraph says Full Access lifts every approval prompt and that only disabled tools or global installs remain restricted, but session_create and session_send still require live approval in every mode. Please mention that cross-session delegation remains the explicit exception, matching the newer 2026-08-18 breaking-change note.