Expand function approval: permission modes, allowlist, and approve-always#195
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
skill-check — worker0 verified, 13 skipped (no docs/).
Three for three. Nicely done. |
📝 WalkthroughWalkthroughThis PR replaces a per-conversation auto-accept toggle with a comprehensive three-mode permission system ( ChangesPermission Modes and Approval Settings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
|
andersonleal
approved these changes
May 28, 2026
Introduce per-session permission modes (manual / auto / full) enforced server-side in the turn-orchestrator hook, replacing the client-only auto-accept toggle. - manual: prompt for everything except yaml allows + approved_always - auto: additionally skip the user-curated always_allow list - full: skip every prompt (persistent banner + one-click revert) Backend: new approval_settings state scope with set_mode / add_always_allow / remove_always_allow / approve_always / get_settings / clear_settings handlers (all human-only; the hook hard-denies them for agent calls). consultBefore snapshots settings and applies full > approved_always (all modes) > always_allow (auto only) > yaml policy. A still-parked sibling is released when a later approve-always or mode switch covers it. Frontend: mode picker in the composer, full-permissions banner, inline "approve always" button on prompts, and a Configuration-screen allowlist manager (tree grouped by ::, tri-state branches, queued destructive confirmation, orchestration/runtime namespaces filtered out). Default mode + allowlist persist in localStorage and seed new conversations.
…hestrator Adds an integration suite that runs the real consultBefore (not mocked) against seeded per-session approval_settings, exercising the full ordering end-to-end via function_execute: - full mode executes immediately, no parking, no approvals write - approved_always honored in manual mode (executes without parking) - auto + always_allow hit executes without parking - manual with no grant parks (falls through to policy needs_approval) - always_allow dormant in manual (still parks) - auto parks a call not on the allowlist - agent attempt to call a human-only approval fn is denied (self-escalation) Harness gains a realistic policy::check_permissions default (needs_approval) so the fall-through path is exercised; existing parallel-approval tests are unaffected (they mock dispatchWithHook).
Fixes the harness node lint CI step — the new settings/test files were hand-written without biome 2.4.10 and had formatting diffs. No behavior change.
ytallo
force-pushed
the
feat/function-approval-modes
branch
from
a16eee8 to
3ad73e4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
harness/tests/approval-gate/settings.test.ts (1)
17-29: ⚡ Quick winMake the mock state store scope-aware to prevent false positives.
storecurrently keys only bypayload.key, so a wrong scope could still pass tests if the session key matches.🧪 Suggested test-mock hardening
function makeIii(initial: unknown = null) { const store = new Map<string, unknown>(); - if (initial !== null) store.set('sess-1', initial); + const mk = (scope: string, key: string) => `${scope}:${key}`; + if (initial !== null) store.set(mk(SETTINGS_STATE_SCOPE, 'sess-1'), initial); const calls: TriggerCall[] = []; const trigger = vi.fn(async (req: TriggerCall) => { calls.push(req); if (req.function_id === 'state::get') { - return store.get(req.payload.key) ?? null; + return store.get(mk(req.payload.scope, req.payload.key)) ?? null; } if (req.function_id === 'state::set') { - if (req.payload.value === null) store.delete(req.payload.key); - else store.set(req.payload.key, req.payload.value); + const k = mk(req.payload.scope, req.payload.key); + if (req.payload.value === null) store.delete(k); + else store.set(k, req.payload.value); return null; } throw new Error(`unexpected trigger ${req.function_id}`); }); return { iii: { trigger } as unknown as ISdk, calls, store }; }🤖 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 `@harness/tests/approval-gate/settings.test.ts` around lines 17 - 29, The in-memory mock state store uses only payload.key which ignores request.scope and allows cross-scope collisions; update the trigger mock (the vi.fn handling TriggerCall) to include scope in the Map key (e.g., compose a composite key from req.payload.scope and req.payload.key) for both the 'state::get' and 'state::set' branches so reads/writes are isolated per scope, and ensure deletions use the same composite key strategy.
🤖 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 `@console/web/src/components/permissions/FullModeConfirmDialog.tsx`:
- Around line 29-59: FullModeConfirmDialog currently enables full permissions on
a single click; require a typed confirmation by adding local state (e.g.,
confirmText via useState) and an input field below the descriptions that asks
the user to type an explicit phrase (e.g., "enable full") before allowing
escalation; update the confirm button handler in the component (the onClick that
calls onConfirm and onOpenChange) to only call onConfirm when confirmText
exactly matches the required phrase, disable the confirm button otherwise, and
clear/reset confirmText when the dialog is closed via onOpenChange(false),
keeping the existing props/open logic and accessibility labels for the input.
In `@console/web/src/components/permissions/FunctionAllowlistTree.tsx`:
- Around line 123-124: When adding items to the destructive queue in
FunctionAllowlistTree.tsx, avoid duplicates by merging existing queue and the
incoming destructive array into a de-duplicated list before calling
setDestructiveQueue; replace setDestructiveQueue((prev) => [...prev,
...destructive]) with logic that builds a new array from prev and destructive
while filtering duplicates (e.g., use a Set or filter by id) and use that result
for setDestructiveQueue; apply the same de-duplication change to the second
occurrence at the other call site (the lines around the second
setDestructiveQueue invocation).
In `@console/web/src/components/permissions/PermissionModePicker.tsx`:
- Around line 39-55: The ModeToggle and FullModeConfirmDialog don't respect the
component's disabled prop: ensure ModeToggle receives disabled={disabled} so it
renders non-interactive and update handleSelect to early-return if disabled (it
already blocks selection but confirm path must too); pass disabled to
FullModeConfirmDialog and guard its onConfirm callback so it only calls
onChange('full') when disabled is false (or check inside the callback before
invoking onChange); also prevent opening the confirm dialog by avoiding
setPendingFull(true) when disabled.
In `@console/web/src/hooks/use-approval-settings.ts`:
- Around line 88-108: The optimistic rollback uses the shared variable previous
and can revert newer updates if requests complete out-of-order; for each mutator
(e.g., setMode using setSettings, setApprovalMode, activeRef, sessionId)
introduce a per-instance sequence guard (e.g., seqRef incremented before the
optimistic update, capture currentSeq locally), set the optimistic state, then
on success only apply next state if currentSeq === seqRef.current and on catch
only restore previous if currentSeq === seqRef.current; apply the same pattern
to the other mutators referenced (the functions around lines using previous,
setSettings, activeRef, sessionId) so rollbacks are gated by the sequence token.
In `@console/web/src/lib/backend/approval-settings.ts`:
- Around line 32-59: coerceEntries and coerceSettings currently coerce values
too permissively (accepting NaN/Infinity and turning objects into string IDs);
tighten validation in coerceEntries to only accept entries where
entry.function_id is a primitive string (typeof === 'string' and non-empty) and
entry.granted_at is a finite number (Number.isFinite), and drop the liberal
String()/Number() conversions; likewise, in coerceSettings ensure mode_set_at is
Number.isFinite before using it and keep DEFAULT_APPROVAL_SETTINGS otherwise,
and continue to use PermissionMode narrowing for mode. Update the
implementations of coerceEntries, coerceSettings, and their uses of
AlwaysAllowEntry, approved_always, always_allow, and mode_set_at accordingly.
In `@harness/docs/workers/approval-gate.md`:
- Around line 85-86: Links in the markdown use an incorrect "harness/..." prefix
and resolve incorrectly; update the broken links (e.g. the link text
"[src/approval-gate/schemas.ts](harness/src/approval-gate/schemas.ts)") to
correct relative paths from the docs file (for example
"../../src/approval-gate/schemas.ts" or the appropriate ../ path) and fix the
other occurrences noted (including the `approval_settings` scope link on the
later lines) so they point to the real files rather than "harness/..." prefixed
paths.
In `@harness/src/approval-gate/settings/get-settings.ts`:
- Around line 18-20: The handler currently uses PayloadSchema.parse(payload)
which throws on invalid input; change it to use PayloadSchema.safeParse(payload)
inside the registerGetSettings handler and, if safeParse returns success: false,
return a structured RPC error reply (use the existing mutationError(...) pattern
used by sibling handlers) instead of throwing; keep the successful path calling
readSettings(iii, parsed.session_id) and returning the GetSettingsReply as
before.
In `@harness/src/turn-orchestrator/hook.ts`:
- Around line 74-79: Wrap the readSettings call in a try/catch so failures do
not propagate into policy evaluation: call
extractSessionId(function_call.arguments) then try { settings = await
readSettings(iii, session_id) } catch (err) { processLogger.warn(...) ; return {
kind: 'deny' } } (or otherwise return an appropriate deny/pending outcome)
before calling settingsVerdict or consultBefore; this ensures failures in
readSettings (referenced by session_id, readSettings, settingsVerdict) are
handled locally and do not let consultBefore throw.
---
Nitpick comments:
In `@harness/tests/approval-gate/settings.test.ts`:
- Around line 17-29: The in-memory mock state store uses only payload.key which
ignores request.scope and allows cross-scope collisions; update the trigger mock
(the vi.fn handling TriggerCall) to include scope in the Map key (e.g., compose
a composite key from req.payload.scope and req.payload.key) for both the
'state::get' and 'state::set' branches so reads/writes are isolated per scope,
and ensure deletions use the same composite key strategy.
🪄 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: 91b96a6c-bdb8-4986-8d03-cc7bf4bc65fb
📒 Files selected for processing (53)
console/web/src/components/chat/AutoAcceptToggle.tsxconsole/web/src/components/chat/ChatPanel.tsxconsole/web/src/components/chat/ChatView.tsxconsole/web/src/components/chat/Composer.tsxconsole/web/src/components/chat/FunctionCallGroup.tsxconsole/web/src/components/chat/FunctionCallMessage.tsxconsole/web/src/components/chat/Message.tsxconsole/web/src/components/chat/MessageList.tsxconsole/web/src/components/permissions/AlwaysAllowButton.tsxconsole/web/src/components/permissions/DefaultPermissionModePicker.tsxconsole/web/src/components/permissions/FullModeConfirmDialog.tsxconsole/web/src/components/permissions/FullPermissionsBanner.tsxconsole/web/src/components/permissions/FunctionAllowlistTree.tsxconsole/web/src/components/permissions/PermissionModePicker.tsxconsole/web/src/hooks/use-approval-settings.tsconsole/web/src/hooks/use-auto-accept-approvals.test.tsconsole/web/src/hooks/use-auto-accept-approvals.tsconsole/web/src/hooks/use-conversations.tsconsole/web/src/lib/backend/approval-settings.tsconsole/web/src/lib/backend/auto-accept.test.tsconsole/web/src/lib/backend/auto-accept.tsconsole/web/src/lib/chat-activity.test.tsconsole/web/src/lib/permissions/allowlist-filter.test.tsconsole/web/src/lib/permissions/allowlist-filter.tsconsole/web/src/lib/storage.tsconsole/web/src/pages/Configuration/index.tsxconsole/web/src/pages/Examples/sections/composer-variants.tsxconsole/web/src/pages/Playground/index.tsxconsole/web/src/types/chat.tsharness/docs/workers/approval-gate.mdharness/src/approval-gate/main.tsharness/src/approval-gate/schemas.tsharness/src/approval-gate/settings/add-always-allow.tsharness/src/approval-gate/settings/approve-always.tsharness/src/approval-gate/settings/clear-settings.tsharness/src/approval-gate/settings/get-settings.tsharness/src/approval-gate/settings/human-only.tsharness/src/approval-gate/settings/register.tsharness/src/approval-gate/settings/remove-always-allow.tsharness/src/approval-gate/settings/reply.tsharness/src/approval-gate/settings/set-mode.tsharness/src/approval-gate/settings/store.tsharness/src/approval-gate/settings/types.tsharness/src/approval-gate/settings/verdict.tsharness/src/index.tsharness/src/turn-orchestrator/function-awaiting-approval/ports.tsharness/src/turn-orchestrator/function-awaiting-approval/run.tsharness/src/turn-orchestrator/hook.tsharness/tests/approval-gate/settings.test.tsharness/tests/integration/mode-approval.e2e.test.tsharness/tests/integration/parallel-approval-harness.tsharness/tests/integration/parallel-approval.e2e.test.tsharness/tests/turn-orchestrator/hook.test.ts
💤 Files with no reviewable changes (10)
- console/web/src/lib/backend/auto-accept.test.ts
- console/web/src/hooks/use-auto-accept-approvals.test.ts
- console/web/src/lib/backend/auto-accept.ts
- console/web/src/hooks/use-auto-accept-approvals.ts
- console/web/src/types/chat.ts
- console/web/src/pages/Playground/index.tsx
- console/web/src/components/chat/AutoAcceptToggle.tsx
- console/web/src/components/chat/ChatPanel.tsx
- console/web/src/hooks/use-conversations.ts
- console/web/src/lib/chat-activity.test.ts
Comment on lines
+29
to
+59
| <Dialog open={open} onOpenChange={onOpenChange}> | ||
| <DialogContent className="max-w-md"> | ||
| <DialogTitle className="text-[14px]"> | ||
| enable full permissions | ||
| </DialogTitle> | ||
| <DialogDescription className="mt-3 text-ink"> | ||
| full permissions let the agent run any function in {target} without | ||
| asking — including writing files, executing shell commands, sending | ||
| messages, and reading secrets. | ||
| </DialogDescription> | ||
| <DialogDescription className="mt-2 text-ink-faint"> | ||
| you can revert from the banner at the top of the chat at any time. | ||
| </DialogDescription> | ||
| <div className="mt-6 flex justify-end gap-2"> | ||
| <button | ||
| type="button" | ||
| onClick={() => onOpenChange(false)} | ||
| className="font-mono text-[12px] px-3 py-1 border border-rule text-ink-faint hover:text-ink hover:border-ink transition-colors" | ||
| > | ||
| cancel | ||
| </button> | ||
| <button | ||
| type="button" | ||
| onClick={() => { | ||
| onConfirm() | ||
| onOpenChange(false) | ||
| }} | ||
| className="font-mono text-[12px] px-3 py-1 border border-ink bg-ink text-bg hover:bg-bg hover:text-ink transition-colors" | ||
| > | ||
| enable full | ||
| </button> |
There was a problem hiding this comment.
Typed confirmation for full mode is missing.
This dialog currently allows one-click escalation to full mode, but the PR objective requires typed confirmation before enabling full permissions.
Suggested patch
+import { useState } from 'react'
import {
Dialog,
DialogContent,
DialogDescription,
DialogTitle,
} from '`@/components/ui/Dialog`'
@@
export function FullModeConfirmDialog({
@@
}: FullModeConfirmDialogProps) {
+ const [confirmText, setConfirmText] = useState('')
const target =
scope === 'default' ? 'every new conversation' : 'this conversation'
+ const canConfirm = confirmText.trim().toLowerCase() === 'full'
return (
@@
<DialogDescription className="mt-2 text-ink-faint">
you can revert from the banner at the top of the chat at any time.
</DialogDescription>
+ <div className="mt-4">
+ <label className="block font-mono text-[12px] text-ink-faint mb-1">
+ type <span className="text-ink">full</span> to confirm
+ </label>
+ <input
+ value={confirmText}
+ onChange={(e) => setConfirmText(e.target.value)}
+ className="w-full border border-rule bg-bg px-2 py-1 text-[12px] font-mono"
+ />
+ </div>
<div className="mt-6 flex justify-end gap-2">
@@
<button
type="button"
onClick={() => {
+ if (!canConfirm) return
onConfirm()
onOpenChange(false)
}}
+ disabled={!canConfirm}
className="font-mono text-[12px] px-3 py-1 border border-ink bg-ink text-bg hover:bg-bg hover:text-ink transition-colors"
>
enable full🤖 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 `@console/web/src/components/permissions/FullModeConfirmDialog.tsx` around
lines 29 - 59, FullModeConfirmDialog currently enables full permissions on a
single click; require a typed confirmation by adding local state (e.g.,
confirmText via useState) and an input field below the descriptions that asks
the user to type an explicit phrase (e.g., "enable full") before allowing
escalation; update the confirm button handler in the component (the onClick that
calls onConfirm and onOpenChange) to only call onConfirm when confirmText
exactly matches the required phrase, disable the confirm button otherwise, and
clear/reset confirmText when the dialog is closed via onOpenChange(false),
keeping the existing props/open logic and accessibility labels for the input.
Comment on lines
+123
to
+124
| setDestructiveQueue((prev) => [...prev, ...destructive]) | ||
| } |
There was a problem hiding this comment.
De-duplicate destructive queue entries before enqueueing.
The same destructive function ID can be queued multiple times via repeated toggles, which leads to duplicate confirmations and duplicate add attempts.
Suggested patch
const [destructiveQueue, setDestructiveQueue] = useState<string[]>([])
@@
+ function enqueueDestructive(ids: string[]) {
+ setDestructiveQueue((prev) => {
+ const seen = new Set(prev)
+ const next = [...prev]
+ for (const id of ids) {
+ if (allowlist.has(id) || seen.has(id)) continue
+ seen.add(id)
+ next.push(id)
+ }
+ return next
+ })
+ }
+
function setSubtreeAllowed(node: TreeNode, value: boolean) {
@@
if (destructive.length > 0) {
- setDestructiveQueue((prev) => [...prev, ...destructive])
+ enqueueDestructive(destructive)
}
@@
if (!isAutoAcceptable(id)) {
- setDestructiveQueue((prev) => [...prev, id])
+ enqueueDestructive([id])
return
}Also applies to: 138-139
🤖 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 `@console/web/src/components/permissions/FunctionAllowlistTree.tsx` around
lines 123 - 124, When adding items to the destructive queue in
FunctionAllowlistTree.tsx, avoid duplicates by merging existing queue and the
incoming destructive array into a de-duplicated list before calling
setDestructiveQueue; replace setDestructiveQueue((prev) => [...prev,
...destructive]) with logic that builds a new array from prev and destructive
while filtering duplicates (e.g., use a Set or filter by id) and use that result
for setDestructiveQueue; apply the same de-duplication change to the second
occurrence at the other call site (the lines around the second
setDestructiveQueue invocation).
Comment on lines
+39
to
+55
| <ModeToggle<PermissionMode> | ||
| value={value} | ||
| onChange={handleSelect} | ||
| variant="radio" | ||
| aria-label="permission mode" | ||
| options={[ | ||
| { value: 'manual', label: 'manual' }, | ||
| { value: 'auto', label: 'auto' }, | ||
| { value: 'full', label: 'full' }, | ||
| ]} | ||
| /> | ||
| <FullModeConfirmDialog | ||
| open={pendingFull} | ||
| onOpenChange={setPendingFull} | ||
| onConfirm={() => onChange('full')} | ||
| scope="conversation" | ||
| /> |
There was a problem hiding this comment.
Propagate disabled state to the toggle and confirm action.
When disabled is true, selection is blocked in handleSelect, but the control still appears interactive and the full-confirm path can still call onChange('full'). Please gate both UI and confirm callback.
Suggested patch
<ModeToggle<PermissionMode>
value={value}
onChange={handleSelect}
+ disabled={disabled}
variant="radio"
aria-label="permission mode"
options={[
{ value: 'manual', label: 'manual' },
{ value: 'auto', label: 'auto' },
{ value: 'full', label: 'full' },
]}
/>
<FullModeConfirmDialog
open={pendingFull}
onOpenChange={setPendingFull}
- onConfirm={() => onChange('full')}
+ onConfirm={() => {
+ if (!disabled && value !== 'full') onChange('full')
+ }}
scope="conversation"
/>🤖 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 `@console/web/src/components/permissions/PermissionModePicker.tsx` around lines
39 - 55, The ModeToggle and FullModeConfirmDialog don't respect the component's
disabled prop: ensure ModeToggle receives disabled={disabled} so it renders
non-interactive and update handleSelect to early-return if disabled (it already
blocks selection but confirm path must too); pass disabled to
FullModeConfirmDialog and guard its onConfirm callback so it only calls
onChange('full') when disabled is false (or check inside the callback before
invoking onChange); also prevent opening the confirm dialog by avoiding
setPendingFull(true) when disabled.
Comment on lines
+88
to
+108
| const setMode = useCallback( | ||
| async (mode: PermissionMode) => { | ||
| // Optimistic — flip the UI now so the click feels responsive even | ||
| // when the backend round-trip is slow. Revert on failure. | ||
| let previous: ApprovalSettings | null = null | ||
| setSettings((s) => { | ||
| previous = s | ||
| return { ...s, mode, mode_set_at: Date.now() } | ||
| }) | ||
| try { | ||
| const next = await setApprovalMode(sessionId, mode) | ||
| if (activeRef.current === sessionId) setSettings(next) | ||
| } catch (err) { | ||
| console.error('[approval-settings] set_mode failed', err) | ||
| if (activeRef.current === sessionId && previous) { | ||
| setSettings(previous) | ||
| } | ||
| } | ||
| }, | ||
| [sessionId], | ||
| ) |
There was a problem hiding this comment.
Prevent stale rollback from out-of-order mutator failures
Line 92, Line 112, Line 143, and Line 168 use previous rollback without mutation ordering guards. If two updates overlap, an earlier failure can revert a later success and leave UI state stale.
Suggested fix (sequence-gated optimistic updates)
+ const mutationSeqRef = useRef(0)
const setMode = useCallback(
async (mode: PermissionMode) => {
+ const seq = ++mutationSeqRef.current
let previous: ApprovalSettings | null = null
setSettings((s) => {
previous = s
return { ...s, mode, mode_set_at: Date.now() }
})
try {
const next = await setApprovalMode(sessionId, mode)
- if (activeRef.current === sessionId) setSettings(next)
+ if (activeRef.current === sessionId && seq === mutationSeqRef.current) {
+ setSettings(next)
+ }
} catch (err) {
console.error('[approval-settings] set_mode failed', err)
- if (activeRef.current === sessionId && previous) {
+ if (
+ activeRef.current === sessionId &&
+ seq === mutationSeqRef.current &&
+ previous
+ ) {
setSettings(previous)
}
}
},
[sessionId],
)Also applies to: 110-139, 141-164, 166-196
🤖 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 `@console/web/src/hooks/use-approval-settings.ts` around lines 88 - 108, The
optimistic rollback uses the shared variable previous and can revert newer
updates if requests complete out-of-order; for each mutator (e.g., setMode using
setSettings, setApprovalMode, activeRef, sessionId) introduce a per-instance
sequence guard (e.g., seqRef incremented before the optimistic update, capture
currentSeq locally), set the optimistic state, then on success only apply next
state if currentSeq === seqRef.current and on catch only restore previous if
currentSeq === seqRef.current; apply the same pattern to the other mutators
referenced (the functions around lines using previous, setSettings, activeRef,
sessionId) so rollbacks are gated by the sequence token.
Comment on lines
+32
to
+59
| function coerceEntries(raw: unknown): AlwaysAllowEntry[] { | ||
| const list = Array.isArray(raw) ? raw : [] | ||
| return list | ||
| .filter( | ||
| (entry): entry is Record<string, unknown> => | ||
| !!entry && typeof entry === 'object', | ||
| ) | ||
| .map( | ||
| (entry): AlwaysAllowEntry => ({ | ||
| function_id: String(entry.function_id ?? ''), | ||
| granted_at: Number(entry.granted_at ?? 0), | ||
| granted_by: 'user_click', | ||
| }), | ||
| ) | ||
| .filter((entry) => entry.function_id.length > 0) | ||
| } | ||
|
|
||
| function coerceSettings(raw: unknown): ApprovalSettings { | ||
| if (!raw || typeof raw !== 'object') return DEFAULT_APPROVAL_SETTINGS | ||
| const r = raw as Record<string, unknown> | ||
| const mode: PermissionMode = | ||
| r.mode === 'auto' || r.mode === 'full' ? r.mode : 'manual' | ||
| return { | ||
| mode, | ||
| always_allow: coerceEntries(r.always_allow), | ||
| approved_always: coerceEntries(r.approved_always), | ||
| mode_set_at: typeof r.mode_set_at === 'number' ? r.mode_set_at : 0, | ||
| } |
There was a problem hiding this comment.
Harden coercion to reject non-finite numbers and non-string function IDs.
Current coercion can admit invalid data (e.g., NaN, Infinity, or function_id from object stringification), which can leak malformed settings into UI state.
Suggested tightening
function coerceEntries(raw: unknown): AlwaysAllowEntry[] {
const list = Array.isArray(raw) ? raw : []
return list
.filter(
(entry): entry is Record<string, unknown> =>
!!entry && typeof entry === 'object',
)
.map(
- (entry): AlwaysAllowEntry => ({
- function_id: String(entry.function_id ?? ''),
- granted_at: Number(entry.granted_at ?? 0),
- granted_by: 'user_click',
- }),
+ (entry): AlwaysAllowEntry | null => {
+ const function_id =
+ typeof entry.function_id === 'string' ? entry.function_id : ''
+ const granted_at = entry.granted_at
+ const safeGrantedAt =
+ typeof granted_at === 'number' &&
+ Number.isFinite(granted_at) &&
+ Number.isInteger(granted_at) &&
+ granted_at >= 0
+ ? granted_at
+ : 0
+ if (!function_id) return null
+ return {
+ function_id,
+ granted_at: safeGrantedAt,
+ granted_by: 'user_click',
+ }
+ },
)
- .filter((entry) => entry.function_id.length > 0)
+ .filter((entry): entry is AlwaysAllowEntry => entry !== null)
}
function coerceSettings(raw: unknown): ApprovalSettings {
if (!raw || typeof raw !== 'object') return DEFAULT_APPROVAL_SETTINGS
const r = raw as Record<string, unknown>
const mode: PermissionMode =
r.mode === 'auto' || r.mode === 'full' ? r.mode : 'manual'
return {
mode,
always_allow: coerceEntries(r.always_allow),
approved_always: coerceEntries(r.approved_always),
- mode_set_at: typeof r.mode_set_at === 'number' ? r.mode_set_at : 0,
+ mode_set_at:
+ typeof r.mode_set_at === 'number' &&
+ Number.isFinite(r.mode_set_at) &&
+ Number.isInteger(r.mode_set_at) &&
+ r.mode_set_at >= 0
+ ? r.mode_set_at
+ : 0,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function coerceEntries(raw: unknown): AlwaysAllowEntry[] { | |
| const list = Array.isArray(raw) ? raw : [] | |
| return list | |
| .filter( | |
| (entry): entry is Record<string, unknown> => | |
| !!entry && typeof entry === 'object', | |
| ) | |
| .map( | |
| (entry): AlwaysAllowEntry => ({ | |
| function_id: String(entry.function_id ?? ''), | |
| granted_at: Number(entry.granted_at ?? 0), | |
| granted_by: 'user_click', | |
| }), | |
| ) | |
| .filter((entry) => entry.function_id.length > 0) | |
| } | |
| function coerceSettings(raw: unknown): ApprovalSettings { | |
| if (!raw || typeof raw !== 'object') return DEFAULT_APPROVAL_SETTINGS | |
| const r = raw as Record<string, unknown> | |
| const mode: PermissionMode = | |
| r.mode === 'auto' || r.mode === 'full' ? r.mode : 'manual' | |
| return { | |
| mode, | |
| always_allow: coerceEntries(r.always_allow), | |
| approved_always: coerceEntries(r.approved_always), | |
| mode_set_at: typeof r.mode_set_at === 'number' ? r.mode_set_at : 0, | |
| } | |
| function coerceEntries(raw: unknown): AlwaysAllowEntry[] { | |
| const list = Array.isArray(raw) ? raw : [] | |
| return list | |
| .filter( | |
| (entry): entry is Record<string, unknown> => | |
| !!entry && typeof entry === 'object', | |
| ) | |
| .map( | |
| (entry): AlwaysAllowEntry | null => { | |
| const function_id = | |
| typeof entry.function_id === 'string' ? entry.function_id : '' | |
| const granted_at = entry.granted_at | |
| const safeGrantedAt = | |
| typeof granted_at === 'number' && | |
| Number.isFinite(granted_at) && | |
| Number.isInteger(granted_at) && | |
| granted_at >= 0 | |
| ? granted_at | |
| : 0 | |
| if (!function_id) return null | |
| return { | |
| function_id, | |
| granted_at: safeGrantedAt, | |
| granted_by: 'user_click', | |
| } | |
| }, | |
| ) | |
| .filter((entry): entry is AlwaysAllowEntry => entry !== null) | |
| } | |
| function coerceSettings(raw: unknown): ApprovalSettings { | |
| if (!raw || typeof raw !== 'object') return DEFAULT_APPROVAL_SETTINGS | |
| const r = raw as Record<string, unknown> | |
| const mode: PermissionMode = | |
| r.mode === 'auto' || r.mode === 'full' ? r.mode : 'manual' | |
| return { | |
| mode, | |
| always_allow: coerceEntries(r.always_allow), | |
| approved_always: coerceEntries(r.approved_always), | |
| mode_set_at: | |
| typeof r.mode_set_at === 'number' && | |
| Number.isFinite(r.mode_set_at) && | |
| Number.isInteger(r.mode_set_at) && | |
| r.mode_set_at >= 0 | |
| ? r.mode_set_at | |
| : 0, | |
| } | |
| } |
🤖 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 `@console/web/src/lib/backend/approval-settings.ts` around lines 32 - 59,
coerceEntries and coerceSettings currently coerce values too permissively
(accepting NaN/Infinity and turning objects into string IDs); tighten validation
in coerceEntries to only accept entries where entry.function_id is a primitive
string (typeof === 'string' and non-empty) and entry.granted_at is a finite
number (Number.isFinite), and drop the liberal String()/Number() conversions;
likewise, in coerceSettings ensure mode_set_at is Number.isFinite before using
it and keep DEFAULT_APPROVAL_SETTINGS otherwise, and continue to use
PermissionMode narrowing for mode. Update the implementations of coerceEntries,
coerceSettings, and their uses of AlwaysAllowEntry, approved_always,
always_allow, and mode_set_at accordingly.
Comment on lines
+85
to
+86
| [src/approval-gate/schemas.ts](harness/src/approval-gate/schemas.ts)); | ||
| per-session permission settings live in scope `approval_settings` |
There was a problem hiding this comment.
Fix broken relative doc links in newly added sections.
These links are likely unresolved from harness/docs/workers/approval-gate.md because they use harness/... as a relative path.
📎 Suggested markdown link fix
-[src/approval-gate/schemas.ts](harness/src/approval-gate/schemas.ts));
+[src/approval-gate/schemas.ts](../../src/approval-gate/schemas.ts));
-| [src/approval-gate/settings/](harness/src/approval-gate/settings/) | Per-session mode/allow-list store, mutations, and handler registration (`readSettings`, `isHumanOnlyApprovalFunction`, `registerSettingsHandlers`). |
-| [src/approval-gate/schemas.ts](harness/src/approval-gate/schemas.ts) | `STATE_SCOPE`, `SETTINGS_STATE_SCOPE`, wire schemas, `ApprovalSettingsSchema`, `parsePolicyReply`, `pendingKey`, `ApprovalDecisionSchema`, `ResolvePayloadSchema`. |
+| [src/approval-gate/settings/](../../src/approval-gate/settings/) | Per-session mode/allow-list store, mutations, and handler registration (`readSettings`, `isHumanOnlyApprovalFunction`, `registerSettingsHandlers`). |
+| [src/approval-gate/schemas.ts](../../src/approval-gate/schemas.ts) | `STATE_SCOPE`, `SETTINGS_STATE_SCOPE`, wire schemas, `ApprovalSettingsSchema`, `parsePolicyReply`, `pendingKey`, `ApprovalDecisionSchema`, `ResolvePayloadSchema`. |Also applies to: 133-134
🤖 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 `@harness/docs/workers/approval-gate.md` around lines 85 - 86, Links in the
markdown use an incorrect "harness/..." prefix and resolve incorrectly; update
the broken links (e.g. the link text
"[src/approval-gate/schemas.ts](harness/src/approval-gate/schemas.ts)") to
correct relative paths from the docs file (for example
"../../src/approval-gate/schemas.ts" or the appropriate ../ path) and fix the
other occurrences noted (including the `approval_settings` scope link on the
later lines) so they point to the real files rather than "harness/..." prefixed
paths.
Comment on lines
+18
to
+20
| const handler: RemoteFunctionHandler<unknown, GetSettingsReply> = async (payload) => { | ||
| const parsed = PayloadSchema.parse(payload); | ||
| return readSettings(iii, parsed.session_id); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the target file with line numbers around the snippet
sed -n '1,120p' harness/src/approval-gate/settings/get-settings.ts | nl -ba
# Find other "settings" RPC handlers (same folder) to compare parse vs safeParse patterns
ls -R harness/src/approval-gate/settings | sed -n '1,200p'
rg -n "safeParse\\(|\\.parse\\(" harness/src/approval-gate/settings --glob='*.ts'
# Specifically inspect any sibling handlers that look like other RPC registrations
rg -n "register.*Settings|RemoteFunctionHandler|readSettings" harness/src/approval-gate/settings --glob='*.ts'Repository: iii-hq/workers
Length of output: 101
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the target file with line numbers around the snippet
sed -n '1,120p' harness/src/approval-gate/settings/get-settings.ts | cat -n
# List the settings RPC files to find siblings
ls -R harness/src/approval-gate/settings | sed -n '1,200p'
# Search for safeParse vs parse usage in this folder
rg -n "safeParse\\(" harness/src/approval-gate/settings --glob='*.ts' || true
rg -n "\\.parse\\(" harness/src/approval-gate/settings --glob='*.ts' || true
# Inspect sibling handlers that register RPC endpoints
rg -n "register.*|RemoteFunctionHandler|readSettings|registerGetSettings" harness/src/approval-gate/settings --glob='*.ts'Repository: iii-hq/workers
Length of output: 7841
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect relevant sibling handlers and reply helpers
sed -n '1,120p' harness/src/approval-gate/settings/set-mode.ts | cat -n
sed -n '1,120p' harness/src/approval-gate/settings/clear-settings.ts | cat -n
sed -n '1,120p' harness/src/approval-gate/settings/add-always-allow.ts | cat -n
sed -n '1,200p' harness/src/approval-gate/settings/reply.ts | cat -n
sed -n '1,120p' harness/src/approval-gate/settings/types.ts | cat -nRepository: iii-hq/workers
Length of output: 6393
Replace PayloadSchema.parse(payload) with safeParse in registerGetSettings to keep RPC responses structured
harness/src/approval-gate/settings/get-settings.ts currently throws on invalid input (PayloadSchema.parse(payload)), while sibling settings handlers use PayloadSchema.safeParse and return mutationError(...) to keep the client-visible reply contract consistent.
Suggested patch
-import { sessionIdField, type GetSettingsReply } from './types.js';
+import { sessionIdField, type GetSettingsReply, type MutationReply } from './types.js';
+import { mutationError } from './reply.js';
@@
export function registerGetSettings(iii: ISdk): void {
- const handler: RemoteFunctionHandler<unknown, GetSettingsReply> = async (payload) => {
- const parsed = PayloadSchema.parse(payload);
- return readSettings(iii, parsed.session_id);
+ const handler: RemoteFunctionHandler<unknown, GetSettingsReply | MutationReply> = async (
+ payload,
+ ) => {
+ const parsed = PayloadSchema.safeParse(payload);
+ if (!parsed.success) return mutationError(parsed.error.message);
+ return readSettings(iii, parsed.data.session_id);
};🤖 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 `@harness/src/approval-gate/settings/get-settings.ts` around lines 18 - 20, The
handler currently uses PayloadSchema.parse(payload) which throws on invalid
input; change it to use PayloadSchema.safeParse(payload) inside the
registerGetSettings handler and, if safeParse returns success: false, return a
structured RPC error reply (use the existing mutationError(...) pattern used by
sibling handlers) instead of throwing; keep the successful path calling
readSettings(iii, parsed.session_id) and returning the GetSettingsReply as
before.
Comment on lines
+74
to
+79
| const session_id = extractSessionId(function_call.arguments); | ||
| const settings = session_id ? await readSettings(iii, session_id) : null; | ||
|
|
||
| if (settings && settingsVerdict(settings, function_call.function_id) === 'allow') { | ||
| return { kind: 'allow' }; | ||
| } |
There was a problem hiding this comment.
Handle settings-read failures before policy consult.
Line 75 performs readSettings(...) outside the guarded policy path. If state::get fails, consultBefore can throw instead of returning a deny/pending outcome, which can break turn progression.
Suggested fix
const session_id = extractSessionId(function_call.arguments);
- const settings = session_id ? await readSettings(iii, session_id) : null;
+ let settings = null;
+ if (session_id) {
+ try {
+ settings = await readSettings(iii, session_id);
+ } catch (err) {
+ logger.warn('approval settings read failed; falling back to policy consult', {
+ function_id: function_call.function_id,
+ session_id,
+ err: String(err),
+ });
+ settings = null;
+ }
+ }
if (settings && settingsVerdict(settings, function_call.function_id) === 'allow') {
return { kind: 'allow' };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const session_id = extractSessionId(function_call.arguments); | |
| const settings = session_id ? await readSettings(iii, session_id) : null; | |
| if (settings && settingsVerdict(settings, function_call.function_id) === 'allow') { | |
| return { kind: 'allow' }; | |
| } | |
| const session_id = extractSessionId(function_call.arguments); | |
| let settings = null; | |
| if (session_id) { | |
| try { | |
| settings = await readSettings(iii, session_id); | |
| } catch (err) { | |
| logger.warn('approval settings read failed; falling back to policy consult', { | |
| function_id: function_call.function_id, | |
| session_id, | |
| err: String(err), | |
| }); | |
| settings = null; | |
| } | |
| } | |
| if (settings && settingsVerdict(settings, function_call.function_id) === 'allow') { | |
| return { kind: 'allow' }; | |
| } |
🤖 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 `@harness/src/turn-orchestrator/hook.ts` around lines 74 - 79, Wrap the
readSettings call in a try/catch so failures do not propagate into policy
evaluation: call extractSessionId(function_call.arguments) then try { settings =
await readSettings(iii, session_id) } catch (err) { processLogger.warn(...) ;
return { kind: 'deny' } } (or otherwise return an appropriate deny/pending
outcome) before calling settingsVerdict or consultBefore; this ensures failures
in readSettings (referenced by session_id, readSettings, settingsVerdict) are
handled locally and do not let consultBefore throw.
3 tasks
andersonleal
added a commit
that referenced
this pull request
May 29, 2026
PR #195 removed the per-conversation `autoAccept` boolean (permission mode is now backend-owned: manual/auto/full). The session exporter still referenced `conversation.autoAccept`, which broke the CI `tsc -b` build (TS2339/TS2353). Local `tsc --noEmit` did not surface it; `tsc -b` (the SPA build) does. Remove the stale `- Auto-accept:` header line and its test fixture/assertion.
andersonleal
added a commit
that referenced
this pull request
May 29, 2026
…irectory bodies (#201) * feat(console): custom function-call views, session export, markdown directory bodies Render function calls in the chat transcript with namespace-specific views instead of raw JSON: - directory (skills/prompts/registry/download), engine (functions/triggers/ workers + register), web (fetch), worker (list/op). FunctionCallMessage now dispatches preview/terminal/id-label across all families via ??-chains; unknown function ids fall back to the default request/response JSON panes. - MarkdownPane renders directory bodies (skill .md, prompt body, worker README) as real markdown via the shared <Markdown> renderer, not raw monospace. - Add ExportSessionButton + export-session lib to download a conversation. - Each view family ships parsers + parser unit tests + Examples-page fixtures. - Sync console/Cargo.lock to console v0.1.5. Verified: tsc clean, 408/408 console-web tests pass. * fix(console): drop stale conversation.autoAccept from session export PR #195 removed the per-conversation `autoAccept` boolean (permission mode is now backend-owned: manual/auto/full). The session exporter still referenced `conversation.autoAccept`, which broke the CI `tsc -b` build (TS2339/TS2353). Local `tsc --noEmit` did not surface it; `tsc -b` (the SPA build) does. Remove the stale `- Auto-accept:` header line and its test fixture/assertion. * feat(console): add custom function views section to the examples playground Wire the directory/engine/web/worker fixtures into the spec-sheet playground. A new CustomFunctionViewsSection renders every fixture (defaultOpen so the custom terminal/preview pane shows), grouped per family with a count heading, plus a 'function views' TOC entry. The fixtures existed but weren't rendered anywhere — now each namespace renderer is visible in the gallery. * chore(console): add dev:playground script to run the examples playground `pnpm dev:playground` runs the dev server with VITE_PLAYGROUND=1, which enables the examples + playground views (reachable at #/examples and #/playground). Matches how CI sets the flag; no new dependency.
4 tasks
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
Expands function approval from a single client-side auto-accept toggle into a server-enforced permission system with three modes plus per-session and per-conversation trust controls.
Permission modes (per session, enforced in the turn-orchestrator)
allowrules and any "approve always" grants).The orchestrator's
consultBeforesnapshots settings once per call and evaluates in order: full → approved_always (all modes) → always_allow (auto only) → yaml policy. Race-safe: settings changes affect future checks only.Approve always (per-conversation)
An "approve always" button on each approval prompt remembers the decision for the rest of the conversation and stops asking for that function in every mode. A still-parked sibling of the same function is released on the same wake when the grant (or a mode switch) lands, so the batch never hangs.
Auto-mode allowlist (per-user defaults)
Managed on the Configuration screen via a function tree grouped by
::(nested namespaces, tri-state branch toggles, entry counts). Orchestration/runtime namespaces (state::,turn::,approval::,ui::,models-catalog::,provider-*, …) are filtered out so only agent-callable tools show. Potentially-destructive ids (write/delete/exec/send/credential/…) require a confirmation, queued one at a time on select-all. The default mode + allowlist persist in localStorage and seed new conversations.Security
The six settings mutators (
approval::set_mode,add_always_allow,remove_always_allow,approve_always,get_settings,clear_settings) are human-only: agent-issued calls are hard-denied inconsultBefore(human_only_function), so the agent can't self-escalate. Full mode is deliberately floor-free and gated behind a typed confirmation in the UI.Test plan
consultBeforeordering, human-only denial, settings mutations (hook.test.ts,settings.test.ts)mode-approval.e2e.test.ts) — realconsultBeforethroughfunction_execute: full executes immediately, approved_always honored in manual, auto+allowlist executes, manual/dormant-allowlist/auto-miss all park, human-only self-escalation deniedparallel-approval.e2e.test.ts) — approve-always releases a parked siblingpnpm typecheckclean (harness + console/web); 968 backend + 306 frontend tests passapproval::*handlers register at boot)Summary by CodeRabbit
New Features
Refactor
Documentation