refactor(approval-gate): port to idiomatic TypeScript - #161
Merged
Conversation
Zod schemas + inferred types replace hand-rolled `typeof` validation and
`as Record<string, unknown>` casts at every boundary. Wire shape, error
codes, and `{ ok }` envelope unchanged.
- `schemas.ts` (new, replaces `types.ts`) — Zod schemas, inferred types,
derived JSON schema for `approval::resolve`, key helpers, typed
`state::set` and `turn::step` payloads.
- `resolve.ts` (renamed from `pending.ts`) — `ResolvePayloadSchema.transform()`
normalises the `tool_call_id` fallback and emits a single non-optional
`function_call_id` (no `!`). Error codes `missing_id` / `bad_decision`
/ `state_write_failed` preserved via a small classifier on the parsed
Zod error path.
- `policy-consult.ts` (deleted) — `parsePolicyReply` and `PolicyOutcome`
collapse into `schemas.ts` next to `PolicyReplySchema`; the discriminated
union output is the outcome shape (no `kind`/`decision` renaming).
- `redact.ts` (new, extracted from `denial.ts`) — pure recursive
redaction via `Object.fromEntries`; immutability guarded by test.
- `on-decision-written.ts` — `StateEventSchema` + `parsePendingKey`
replace inline `typeof` chains and `indexOf('/')` parsing; tolerant
key split preserves `/` inside `function_call_id` for forward
compatibility. Silent-warn semantics preserved with an explicit
comment.
- `register.ts` — passes `request_format: ResolvePayloadJsonSchema` on
`approval::resolve` so the engine's directory exposes a typed signature.
- `turn-orchestrator/hook.ts` — import path migrated to `schemas.js`;
local variable renamed `decision` → `outcome` to avoid `.decision.decision`.
- `iii-sdk` imported directly (no `runtime/iii.js` indirection).
- Tests: new `schemas.test.ts` + `redact.test.ts`, shared
`_helpers/fakeIii.ts`, regression guards on wire error strings,
redact immutability, and tolerant key parsing.
377 / 377 tests green. TSC strict + biome clean.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughApproval coordination now routes operator decisions via ChangesApproval Gate & Policy System Refactor
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
skill-check — worker0 verified, 11 skipped (no docs/).
Three for three. Nicely done. |
sergiofilhowz
approved these changes
May 19, 2026
…ecated configurations This commit refactors the approval-gate to enhance the handling of approval states and decisions. Key changes include: - Removed the `approval_gate` section from `config.yaml` as it is no longer needed. - Simplified the approval process by directly routing decisions to per-call `turn::approval_resume` functions in the turn-orchestrator. - Eliminated the `approval_gate.approval_state_scope` configuration, fixing the scope to `approvals` in code. - Updated documentation to reflect the new approval flow and removed outdated references to state triggers and adapters that are no longer in use. These changes improve clarity and maintainability of the approval-gate functionality, ensuring a more efficient approval process.
…uration This commit refactors the policy handling within the harness-node to improve clarity and maintainability. Key changes include: - Removed the deprecated `policy_function_id` from `config.yaml` and related documentation, as it is now directly handled in the orchestrator. - Introduced a new `check-permissions.ts` file that encapsulates the logic for checking permissions, replacing the previous `policy-fn.ts`. - Updated the `dispatchWithHook` function to directly call the new permissions check, simplifying the approval process. - Enhanced the redaction logic in `redact.ts` to prevent stack overflow from deeply nested structures. - Added validation to ensure that session and function IDs do not contain reserved characters. These changes streamline the policy evaluation process and improve the overall structure of the codebase, ensuring a more efficient and robust implementation.
…-gate note Replace the deleted src/harness/policy.ts source-layout row with the new src/harness/policy/ module files, and remove the dead note describing the removed approval::on_decision_written adapter.
Resolved conflicts in turn-orchestrator states/functions.ts and the turn-orchestrator doc by combining both designs: keep this branch's approval refactor (STATE_SCOPE, registerApprovalResume, 3-arg dispatchWithHook, no policy_function_id) and main's duration_ms function-execution timing. Reordered store.ts imports to satisfy biome.
ytallo
marked this pull request as ready for review
May 20, 2026 15:19
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
harness-node/src/harness/policy/compile.ts (1)
57-60: ⚡ Quick winConsider adding regex safety validation to defensive-code against pathological patterns in policy config.
The code at line 59 compiles regex patterns from
iii-permissions.yamlwithout checking for catastrophic backtracking. While the file is a trusted, git-tracked system configuration with reasonable patterns (e.g.,^session/[a-z]+/notes$,^git (status|log|diff)( |$)), an operator with commit access could introduce a pathological regex that causes CPU DoS during policy checks. The comment mentions "Rustregexcrate" but the code uses JavaScriptRegExp, which lacks automatic backtracking prevention. Adding a safety check usingsafe-regex2at load time (whencompileConstraintruns) would be defensive hardening without performance cost since compilation happens once at boot.🤖 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-node/src/harness/policy/compile.ts` around lines 57 - 60, compileConstraint currently constructs a RegExp directly from c.matches; add a safety check to reject pathological patterns by validating c.matches with a safe-regex checker (e.g., safe-regex2) before creating the RegExp. Import and call the safe-regex validator inside the branch that handles 'matches' (where c.matches is used), and if the validator returns unsafe, throw or return a clear compile-time error instead of compiling; only call new RegExp(c.matches) when the pattern is reported safe. Ensure the thrown error is handled consistently with the existing try/catch around compileConstraint so boot-time loading fails fast for unsafe patterns.harness-node/tests/harness/policy.test.ts (1)
395-399: ⚡ Quick winResolve the shipped policy fixture relative to the test module.
readFile('./iii-permissions.yaml', 'utf8')depends on the process CWD, which is ambiguous when vitest runs without an explicit root configuration. While a symlink atharness-node/iii-permissions.yamlcurrently masks this issue, usingimport.meta.urlis the robust pattern for ESM.♻️ Proposed fix
const load = async () => { if (!shipped) { const { readFile } = await import('node:fs/promises'); - shipped = Permissions.parse(await readFile('./iii-permissions.yaml', 'utf8')); + const policyFile = new URL('../../iii-permissions.yaml', import.meta.url); + shipped = Permissions.parse(await readFile(policyFile, 'utf8')); } return shipped; };🤖 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-node/tests/harness/policy.test.ts` around lines 395 - 399, The load function uses readFile('./iii-permissions.yaml', 'utf8') which relies on process CWD; change it to resolve the fixture relative to the test module by using import.meta.url. Specifically, update the call inside load so Permissions.parse(await readFile(...)) reads from new URL('./iii-permissions.yaml', import.meta.url) (i.e., Permissions.parse(await readFile(new URL('./iii-permissions.yaml', import.meta.url), 'utf8'))), leaving shipped and Permissions.parse unchanged.harness-node/tests/turn-orchestrator/approval-resume.test.ts (1)
26-34: ⚡ Quick winMake
unregisteractually detach handlers in this test double.Right now
unregisteris a no-op spy, so function IDs remain callable after “unregister,” which can mask lifecycle bugs around resume-handler cleanup.Proposed patch
const iii = { registerFunction: vi.fn((fnId: string, handler: (payload: unknown) => Promise<unknown>) => { - const entry: RegisteredFn = { + const unregister = vi.fn(() => { + registered.delete(fnId); + }); + const entry: RegisteredFn = { fnId, handler, - unregister: vi.fn(), + unregister, }; registered.set(fnId, entry); return { unregister: entry.unregister }; }),🤖 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-node/tests/turn-orchestrator/approval-resume.test.ts` around lines 26 - 34, The test double's registerFunction currently returns a no-op spy as unregister so entries stay in the registered map after "unregister"; change the implementation so unregister actually removes the handler from the registered Map: when creating the RegisteredFn entry in registerFunction, set unregister to a function (wrapped with vi.fn if you need call-tracking) that calls registered.delete(fnId) (and is idempotent), and return that unregister; update any references to RegisteredFn.unregister to use this removal behavior so handlers cannot be invoked after unregister.
🤖 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 `@harness-node/src/runtime/state.ts`:
- Around line 132-139: stateList currently ignores the optional _prefix and
returns the whole scope; preserve the deprecated wrapper contract by calling
stateListValues(iii, { scope }) and then, when _prefix is provided, locally
filter the returned array to only entries whose key begins with that prefix
(e.g., entry.key.startsWith(_prefix) or adapt to the actual entry shape), then
return the filtered list; update stateList (and mention stateListValues) to
perform this conditional filtering so callers get prefix-scoped results until
they are migrated.
- Around line 39-73: The parsers currently treat any object with a "value"
property as a wrapper and unwrap it, corrupting legitimate stored objects like {
value: 1, state: 'x' }; update unwrapStateListEntry, parseStateListValues and
parseStateListKeyedEntries (and use stateListResponseRows as before) to only
unwrap when the object shape is unambiguously an envelope: for the simple value
envelope require the object’s own enumerable keys are exactly ["value"] (or
exactly ["key","value"] for keyed envelope cases), and for the items-envelope
accept only objects whose own keys are exactly ["items"] and whose items are an
array of { key, value } shapes; otherwise return the original object as the
stored value. Ensure these key checks use Object.keys(...) so other properties
prevent unwrapping.
In `@harness-node/src/turn-orchestrator/approval-resume.ts`:
- Around line 83-90: The current read-then-write using pendingKey(session_id,
function_call_id) with stateGet(iii, STATE_SCOPE, key) and stateSet(...) is
racy: multiple callers can observe no decision and both write; change to a
first-writer-wins atomic update or an in-memory per-function-call guard.
Concretely, replace the separated stateGet/stateSet sequence (and the
hasStoredDecision check) with either a conditional/compare-and-set style
operation provided by the state layer (an atomic "set-if-missing" or
"compareAndSwap" using key) so only the first writer persists
parsed.data.decision and parsed.data.reason, or add a short-lived in-process
guard keyed by pendingKey(session_id, function_call_id) (acquire guard before
awaiting stateGet and release after stateSet) to ensure only one in-flight
resolver can perform the write. Ensure the final write only occurs when the
atomic operation succeeds or when the guard owns the key.
- Around line 92-99: The current catch block unregisters the resume handler even
when iii.trigger({ function_id: STEP_FN_ID, payload: { session_id } }) fails,
removing the only retry path; change the flow so unregisterApprovalResume(fnId)
is only called after a successful wake (i.e., move or call
unregisterApprovalResume(fnId) inside the try block immediately after the await)
and do not call it inside the catch so the handler remains registered for
retries; ensure fnId and STEP_FN_ID references remain unchanged.
In `@harness-node/src/turn-orchestrator/hook.ts`:
- Around line 42-65: The switch on reply.decision can fall through for malformed
or unexpected PolicyCheckReply values and return undefined; modify the switch in
consultBefore (the block after iii.trigger<CheckPermissionsPayload,
PolicyCheckReply>) to handle unknown/missing replies by treating them as a
closed gate: add a default/fallback branch that returns a deny outcome with a
denial envelope indicating a gate_unavailable error (use
permissionsDenyEnvelope(function_call.function_id, 'gate_unavailable', null,
function_call.arguments) or equivalent), ensuring you still use reply.rule_id
and reply.matched_constraint when present but fall back to safe defaults when
they are absent.
---
Nitpick comments:
In `@harness-node/src/harness/policy/compile.ts`:
- Around line 57-60: compileConstraint currently constructs a RegExp directly
from c.matches; add a safety check to reject pathological patterns by validating
c.matches with a safe-regex checker (e.g., safe-regex2) before creating the
RegExp. Import and call the safe-regex validator inside the branch that handles
'matches' (where c.matches is used), and if the validator returns unsafe, throw
or return a clear compile-time error instead of compiling; only call new
RegExp(c.matches) when the pattern is reported safe. Ensure the thrown error is
handled consistently with the existing try/catch around compileConstraint so
boot-time loading fails fast for unsafe patterns.
In `@harness-node/tests/harness/policy.test.ts`:
- Around line 395-399: The load function uses readFile('./iii-permissions.yaml',
'utf8') which relies on process CWD; change it to resolve the fixture relative
to the test module by using import.meta.url. Specifically, update the call
inside load so Permissions.parse(await readFile(...)) reads from new
URL('./iii-permissions.yaml', import.meta.url) (i.e., Permissions.parse(await
readFile(new URL('./iii-permissions.yaml', import.meta.url), 'utf8'))), leaving
shipped and Permissions.parse unchanged.
In `@harness-node/tests/turn-orchestrator/approval-resume.test.ts`:
- Around line 26-34: The test double's registerFunction currently returns a
no-op spy as unregister so entries stay in the registered map after
"unregister"; change the implementation so unregister actually removes the
handler from the registered Map: when creating the RegisteredFn entry in
registerFunction, set unregister to a function (wrapped with vi.fn if you need
call-tracking) that calls registered.delete(fnId) (and is idempotent), and
return that unregister; update any references to RegisteredFn.unregister to use
this removal behavior so handlers cannot be invoked after unregister.
🪄 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: 6c4a1657-f049-461a-bc32-f48eb8040e61
📒 Files selected for processing (55)
harness-node/README.mdharness-node/config.yamlharness-node/docs/architecture.mdharness-node/docs/workers/approval-gate.mdharness-node/docs/workers/harness.mdharness-node/docs/workers/turn-orchestrator.mdharness-node/src/approval-gate/config.tsharness-node/src/approval-gate/denial.tsharness-node/src/approval-gate/iii.worker.yamlharness-node/src/approval-gate/main.tsharness-node/src/approval-gate/on-decision-written.tsharness-node/src/approval-gate/pending.tsharness-node/src/approval-gate/policy-consult.tsharness-node/src/approval-gate/redact.tsharness-node/src/approval-gate/register.tsharness-node/src/approval-gate/resolve.tsharness-node/src/approval-gate/schemas.tsharness-node/src/approval-gate/types.tsharness-node/src/harness/policy-fn.tsharness-node/src/harness/policy.tsharness-node/src/harness/policy/check-permissions.tsharness-node/src/harness/policy/compile.tsharness-node/src/harness/policy/handle.tsharness-node/src/harness/policy/permissions.tsharness-node/src/harness/policy/types.tsharness-node/src/harness/register.tsharness-node/src/index.tsharness-node/src/runtime/state.tsharness-node/src/session/tree/store.tsharness-node/src/turn-orchestrator/abort.tsharness-node/src/turn-orchestrator/agent-call.tsharness-node/src/turn-orchestrator/approval-resume.tsharness-node/src/turn-orchestrator/config.tsharness-node/src/turn-orchestrator/hook.tsharness-node/src/turn-orchestrator/on-terminal.tsharness-node/src/turn-orchestrator/register.tsharness-node/src/turn-orchestrator/states/functions.tsharness-node/tests/approval-gate/_helpers/fakeIii.tsharness-node/tests/approval-gate/denial.test.tsharness-node/tests/approval-gate/on-decision-written.test.tsharness-node/tests/approval-gate/pending.test.tsharness-node/tests/approval-gate/policy-consult.test.tsharness-node/tests/approval-gate/redact.test.tsharness-node/tests/approval-gate/resolve.test.tsharness-node/tests/approval-gate/schemas.test.tsharness-node/tests/approval-gate/types.test.tsharness-node/tests/harness/policy.test.tsharness-node/tests/integration/approval-resume.e2e.test.tsharness-node/tests/runtime/state-list.test.tsharness-node/tests/turn-orchestrator/abort.test.tsharness-node/tests/turn-orchestrator/agent-call.test.tsharness-node/tests/turn-orchestrator/approval-resume.test.tsharness-node/tests/turn-orchestrator/config.test.tsharness-node/tests/turn-orchestrator/functions.test.tsharness-node/tests/turn-orchestrator/hook.test.ts
💤 Files with no reviewable changes (14)
- harness-node/tests/approval-gate/policy-consult.test.ts
- harness-node/src/harness/policy-fn.ts
- harness-node/src/approval-gate/register.ts
- harness-node/tests/approval-gate/types.test.ts
- harness-node/src/approval-gate/types.ts
- harness-node/tests/approval-gate/on-decision-written.test.ts
- harness-node/src/approval-gate/on-decision-written.ts
- harness-node/src/approval-gate/policy-consult.ts
- harness-node/tests/approval-gate/pending.test.ts
- harness-node/src/approval-gate/config.ts
- harness-node/src/approval-gate/pending.ts
- harness-node/config.yaml
- harness-node/src/harness/policy.ts
- harness-node/tests/turn-orchestrator/agent-call.test.ts
Comment on lines
+39
to
+73
| function unwrapStateListEntry<T>(entry: unknown): T { | ||
| if (entry && typeof entry === 'object' && 'value' in (entry as Record<string, unknown>)) { | ||
| return (entry as Record<string, unknown>).value as T; | ||
| } | ||
| return entry as T; | ||
| } | ||
|
|
||
| /** | ||
| * Normalizes a `state::list` trigger result to stored values. | ||
| * | ||
| * Official iii returns a flat `T[]` ({@link StateListInput} only). Some | ||
| * deployments also wrap rows as `{ value }` or `{ items: [{ key, value }] }`; | ||
| * we accept those shapes so harness workers stay compatible. | ||
| */ | ||
| export function parseStateListValues<T>(response: unknown): T[] { | ||
| const arr = stateListResponseRows(response); | ||
| if (!arr) return []; | ||
| return arr.map((entry) => unwrapStateListEntry<T>(entry)); | ||
| } | ||
|
|
||
| /** Keyed rows when the list response includes `key` (not returned by stock iii). */ | ||
| export function parseStateListKeyedEntries(response: unknown): StateListKeyedEntry[] { | ||
| const arr = stateListResponseRows(response); | ||
| if (!arr) return []; | ||
| return arr.map((entry) => { | ||
| if (entry && typeof entry === 'object') { | ||
| const row = entry as Record<string, unknown>; | ||
| return { | ||
| key: typeof row.key === 'string' ? row.key : undefined, | ||
| value: row.value !== undefined ? row.value : entry, | ||
| }; | ||
| } | ||
| return { value: entry }; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Disambiguate list envelopes from stored objects.
Both parsers treat any object with a value field as a wrapper row. For the official flat T[] shape, a legitimate stored value like { value: 1, state: 'x' } gets collapsed to 1, which corrupts list results. Only unwrap when the row shape is unambiguously an envelope.
Proposed fix
function unwrapStateListEntry<T>(entry: unknown): T {
- if (entry && typeof entry === 'object' && 'value' in (entry as Record<string, unknown>)) {
- return (entry as Record<string, unknown>).value as T;
+ if (entry && typeof entry === 'object') {
+ const row = entry as Record<string, unknown>;
+ const keys = Object.keys(row);
+ if ('value' in row && keys.every((k) => k === 'value' || k === 'key')) {
+ return row.value as T;
+ }
}
return entry as T;
}📝 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 unwrapStateListEntry<T>(entry: unknown): T { | |
| if (entry && typeof entry === 'object' && 'value' in (entry as Record<string, unknown>)) { | |
| return (entry as Record<string, unknown>).value as T; | |
| } | |
| return entry as T; | |
| } | |
| /** | |
| * Normalizes a `state::list` trigger result to stored values. | |
| * | |
| * Official iii returns a flat `T[]` ({@link StateListInput} only). Some | |
| * deployments also wrap rows as `{ value }` or `{ items: [{ key, value }] }`; | |
| * we accept those shapes so harness workers stay compatible. | |
| */ | |
| export function parseStateListValues<T>(response: unknown): T[] { | |
| const arr = stateListResponseRows(response); | |
| if (!arr) return []; | |
| return arr.map((entry) => unwrapStateListEntry<T>(entry)); | |
| } | |
| /** Keyed rows when the list response includes `key` (not returned by stock iii). */ | |
| export function parseStateListKeyedEntries(response: unknown): StateListKeyedEntry[] { | |
| const arr = stateListResponseRows(response); | |
| if (!arr) return []; | |
| return arr.map((entry) => { | |
| if (entry && typeof entry === 'object') { | |
| const row = entry as Record<string, unknown>; | |
| return { | |
| key: typeof row.key === 'string' ? row.key : undefined, | |
| value: row.value !== undefined ? row.value : entry, | |
| }; | |
| } | |
| return { value: entry }; | |
| }); | |
| } | |
| function unwrapStateListEntry<T>(entry: unknown): T { | |
| if (entry && typeof entry === 'object') { | |
| const row = entry as Record<string, unknown>; | |
| const keys = Object.keys(row); | |
| if ('value' in row && keys.every((k) => k === 'value' || k === 'key')) { | |
| return row.value as T; | |
| } | |
| } | |
| return entry as T; | |
| } | |
| /** | |
| * Normalizes a `state::list` trigger result to stored values. | |
| * | |
| * Official iii returns a flat `T[]` ({`@link` StateListInput} only). Some | |
| * deployments also wrap rows as `{ value }` or `{ items: [{ key, value }] }`; | |
| * we accept those shapes so harness workers stay compatible. | |
| */ | |
| export function parseStateListValues<T>(response: unknown): T[] { | |
| const arr = stateListResponseRows(response); | |
| if (!arr) return []; | |
| return arr.map((entry) => unwrapStateListEntry<T>(entry)); | |
| } | |
| /** Keyed rows when the list response includes `key` (not returned by stock iii). */ | |
| export function parseStateListKeyedEntries(response: unknown): StateListKeyedEntry[] { | |
| const arr = stateListResponseRows(response); | |
| if (!arr) return []; | |
| return arr.map((entry) => { | |
| if (entry && typeof entry === 'object') { | |
| const row = entry as Record<string, unknown>; | |
| return { | |
| key: typeof row.key === 'string' ? row.key : undefined, | |
| value: row.value !== undefined ? row.value : entry, | |
| }; | |
| } | |
| return { value: entry }; | |
| }); | |
| } |
🤖 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-node/src/runtime/state.ts` around lines 39 - 73, The parsers
currently treat any object with a "value" property as a wrapper and unwrap it,
corrupting legitimate stored objects like { value: 1, state: 'x' }; update
unwrapStateListEntry, parseStateListValues and parseStateListKeyedEntries (and
use stateListResponseRows as before) to only unwrap when the object shape is
unambiguously an envelope: for the simple value envelope require the object’s
own enumerable keys are exactly ["value"] (or exactly ["key","value"] for keyed
envelope cases), and for the items-envelope accept only objects whose own keys
are exactly ["items"] and whose items are an array of { key, value } shapes;
otherwise return the original object as the stored value. Ensure these key
checks use Object.keys(...) so other properties prevent unwrapping.
Comment on lines
+132
to
+139
| /** | ||
| * @deprecated Third argument `prefix` is not sent to iii (engine lists the | ||
| * whole scope). Kept for call-site stability; filter returned values locally | ||
| * if you need key-prefix semantics. | ||
| */ | ||
| export async function stateList(iii: ISdk, scope: string, _prefix?: string): Promise<unknown[]> { | ||
| return stateListValues(iii, { scope }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Keep the deprecated wrapper's prefix contract until callers are migrated.
stateList(iii, scope, prefix) still accepts prefix, but this implementation silently widens to the whole scope. That makes unchanged callers return unrelated entries. Either filter locally when keys are available, or remove the parameter/export in a breaking change.
🤖 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-node/src/runtime/state.ts` around lines 132 - 139, stateList
currently ignores the optional _prefix and returns the whole scope; preserve the
deprecated wrapper contract by calling stateListValues(iii, { scope }) and then,
when _prefix is provided, locally filter the returned array to only entries
whose key begins with that prefix (e.g., entry.key.startsWith(_prefix) or adapt
to the actual entry shape), then return the filtered list; update stateList (and
mention stateListValues) to perform this conditional filtering so callers get
prefix-scoped results until they are migrated.
Comment on lines
+83
to
+90
| const key = pendingKey(session_id, function_call_id); | ||
| const existing = await stateGet(iii, STATE_SCOPE, key); | ||
| if (!hasStoredDecision(existing)) { | ||
| await stateSet(iii, STATE_SCOPE, key, { | ||
| decision: parsed.data.decision, | ||
| reason: parsed.data.reason, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Make approval resolution first-writer-wins.
stateGet() and stateSet() are separated by awaits, so an abort and a user approval can both observe “no decision” and then overwrite each other. This path needs an atomic write or a per-fnId in-flight guard before the first await.
🤖 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-node/src/turn-orchestrator/approval-resume.ts` around lines 83 - 90,
The current read-then-write using pendingKey(session_id, function_call_id) with
stateGet(iii, STATE_SCOPE, key) and stateSet(...) is racy: multiple callers can
observe no decision and both write; change to a first-writer-wins atomic update
or an in-memory per-function-call guard. Concretely, replace the separated
stateGet/stateSet sequence (and the hasStoredDecision check) with either a
conditional/compare-and-set style operation provided by the state layer (an
atomic "set-if-missing" or "compareAndSwap" using key) so only the first writer
persists parsed.data.decision and parsed.data.reason, or add a short-lived
in-process guard keyed by pendingKey(session_id, function_call_id) (acquire
guard before awaiting stateGet and release after stateSet) to ensure only one
in-flight resolver can perform the write. Ensure the final write only occurs
when the atomic operation succeeds or when the guard owns the key.
Comment on lines
+92
to
+99
| try { | ||
| await iii.trigger({ function_id: STEP_FN_ID, payload: { session_id } }); | ||
| } catch (err) { | ||
| logger.warn('approval resume: turn::step invoke failed', { session_id, err: String(err) }); | ||
| } | ||
|
|
||
| unregisterApprovalResume(fnId); | ||
| } |
There was a problem hiding this comment.
Don't unregister the resume function after a failed wake-up.
If turn::step throws, Line 98 still removes the only retry path. The decision is already persisted, so the session can stay parked until restart/manual intervention. Keep the handler registered on failure so the wake can be retried.
Proposed fix
try {
await iii.trigger({ function_id: STEP_FN_ID, payload: { session_id } });
+ unregisterApprovalResume(fnId);
} catch (err) {
logger.warn('approval resume: turn::step invoke failed', { session_id, err: String(err) });
}
-
- unregisterApprovalResume(fnId);
}📝 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
| try { | |
| await iii.trigger({ function_id: STEP_FN_ID, payload: { session_id } }); | |
| } catch (err) { | |
| logger.warn('approval resume: turn::step invoke failed', { session_id, err: String(err) }); | |
| } | |
| unregisterApprovalResume(fnId); | |
| } | |
| try { | |
| await iii.trigger({ function_id: STEP_FN_ID, payload: { session_id } }); | |
| unregisterApprovalResume(fnId); | |
| } catch (err) { | |
| logger.warn('approval resume: turn::step invoke failed', { session_id, err: String(err) }); | |
| } | |
| } |
🤖 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-node/src/turn-orchestrator/approval-resume.ts` around lines 92 - 99,
The current catch block unregisters the resume handler even when iii.trigger({
function_id: STEP_FN_ID, payload: { session_id } }) fails, removing the only
retry path; change the flow so unregisterApprovalResume(fnId) is only called
after a successful wake (i.e., move or call unregisterApprovalResume(fnId)
inside the try block immediately after the await) and do not call it inside the
catch so the handler remains registered for retries; ensure fnId and STEP_FN_ID
references remain unchanged.
Comment on lines
+42
to
+65
| const reply = await iii.trigger<CheckPermissionsPayload, PolicyCheckReply>({ | ||
| function_id: 'policy::check_permissions', | ||
| payload: { | ||
| function_id: function_call.function_id, | ||
| args: function_call.arguments as CheckPermissionsPayload['args'], | ||
| }, | ||
| timeoutMs: 5_000, | ||
| }); | ||
| switch (reply.decision) { | ||
| case 'allow': | ||
| return { kind: 'allow' }; | ||
| case 'deny': | ||
| return { | ||
| kind: 'deny', | ||
| denial: permissionsDenyEnvelope( | ||
| function_call.function_id, | ||
| reply.rule_id, | ||
| reply.matched_constraint ?? null, | ||
| function_call.arguments, | ||
| ), | ||
| }; | ||
| case 'needs_approval': | ||
| return { kind: 'pending' }; | ||
| } |
There was a problem hiding this comment.
Fail closed on malformed policy replies.
If policy::check_permissions returns an unexpected payload, this switch falls through and consultBefore() resolves undefined. The next access to outcome.kind then throws instead of denying. Treat an unknown reply shape/value as gate_unavailable here.
Proposed fix
- switch (reply.decision) {
+ const decision =
+ reply && typeof reply === 'object'
+ ? (reply as Record<string, unknown>).decision
+ : undefined;
+ switch (decision) {
case 'allow':
return { kind: 'allow' };
case 'deny':
return {
kind: 'deny',
@@
case 'needs_approval':
return { kind: 'pending' };
+ default:
+ throw new Error('malformed policy reply');
}📝 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 reply = await iii.trigger<CheckPermissionsPayload, PolicyCheckReply>({ | |
| function_id: 'policy::check_permissions', | |
| payload: { | |
| function_id: function_call.function_id, | |
| args: function_call.arguments as CheckPermissionsPayload['args'], | |
| }, | |
| timeoutMs: 5_000, | |
| }); | |
| switch (reply.decision) { | |
| case 'allow': | |
| return { kind: 'allow' }; | |
| case 'deny': | |
| return { | |
| kind: 'deny', | |
| denial: permissionsDenyEnvelope( | |
| function_call.function_id, | |
| reply.rule_id, | |
| reply.matched_constraint ?? null, | |
| function_call.arguments, | |
| ), | |
| }; | |
| case 'needs_approval': | |
| return { kind: 'pending' }; | |
| } | |
| const reply = await iii.trigger<CheckPermissionsPayload, PolicyCheckReply>({ | |
| function_id: 'policy::check_permissions', | |
| payload: { | |
| function_id: function_call.function_id, | |
| args: function_call.arguments as CheckPermissionsPayload['args'], | |
| }, | |
| timeoutMs: 5_000, | |
| }); | |
| const decision = | |
| reply && typeof reply === 'object' | |
| ? (reply as Record<string, unknown>).decision | |
| : undefined; | |
| switch (decision) { | |
| case 'allow': | |
| return { kind: 'allow' }; | |
| case 'deny': | |
| return { | |
| kind: 'deny', | |
| denial: permissionsDenyEnvelope( | |
| function_call.function_id, | |
| reply.rule_id, | |
| reply.matched_constraint ?? null, | |
| function_call.arguments, | |
| ), | |
| }; | |
| case 'needs_approval': | |
| return { kind: 'pending' }; | |
| default: | |
| throw new Error('malformed policy reply'); | |
| } |
🤖 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-node/src/turn-orchestrator/hook.ts` around lines 42 - 65, The switch
on reply.decision can fall through for malformed or unexpected PolicyCheckReply
values and return undefined; modify the switch in consultBefore (the block after
iii.trigger<CheckPermissionsPayload, PolicyCheckReply>) to handle
unknown/missing replies by treating them as a closed gate: add a
default/fallback branch that returns a deny outcome with a denial envelope
indicating a gate_unavailable error (use
permissionsDenyEnvelope(function_call.function_id, 'gate_unavailable', null,
function_call.arguments) or equivalent), ensuring you still use reply.rule_id
and reply.matched_constraint when present but fall back to safe defaults when
they are absent.
sergiofilhowz
approved these changes
May 20, 2026
…ance clarity This commit refactors the iii-permissions.yaml file to streamline the permissions rules for agents. Key changes include: - Consolidated the permissions rules by removing verbose comments and restructuring the format for clarity. - Replaced individual deny rules with a simplified notation using bare strings for allow and quoted strings for deny. - Removed deprecated sections and comments, focusing on essential permissions and their intended use. These changes improve the readability and maintainability of the permissions configuration, ensuring a more efficient setup for agent permissions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@iii-permissions.yaml`:
- Around line 10-27: The deny list currently blocks '!approval::resolve' but not
the new resume handler, so agents can discover and call the internal resume via
'directory::engine::functions::list' and fallback to 'needs_approval'; add a
deny rule for the 'turn::approval_resume' namespace (e.g., add a line like the
other entries: '- '!turn::approval_resume'') to the deny block in
iii-permissions.yaml so that any calls under the turn::approval_resume prefix
are prevented.
🪄 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: 24e69b1a-1a6d-4cda-9c74-301c384749f9
📒 Files selected for processing (1)
iii-permissions.yaml
Comment on lines
+10
to
+27
| # Gate, state, auth, routing — agents must not call these directly. | ||
| - '!approval::resolve' | ||
| - '!policy::check_permissions' | ||
| - '!hook-fanout::publish_collect' | ||
| - '!state::set' | ||
| - '!state::update' | ||
| - '!state::delete' | ||
| - '!stream::set' | ||
| - '!iii::durable::publish' | ||
| - '!auth::set_token' | ||
| - '!auth::delete_token' | ||
| - '!oauth::anthropic::login' | ||
| - '!oauth::openai-codex::login' | ||
| - '!run::start' | ||
| - '!run::start_and_wait' | ||
| - '!router::stream_assistant' | ||
| - '!router::abort' | ||
|
|
There was a problem hiding this comment.
Deny the new turn::approval_resume control-plane functions.
This refactor moved approval completion behind per-call resume functions, but the kernel deny list here still only blocks approval::resolve. Because directory::engine::functions::list is allowed and unmatched calls fall back to needs_approval, agents can discover and request approval on the internal resume handler instead of the original tool call. Add a deny rule that matches the turn::approval_resume namespace/prefix as well.
🤖 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 `@iii-permissions.yaml` around lines 10 - 27, The deny list currently blocks
'!approval::resolve' but not the new resume handler, so agents can discover and
call the internal resume via 'directory::engine::functions::list' and fallback
to 'needs_approval'; add a deny rule for the 'turn::approval_resume' namespace
(e.g., add a line like the other entries: '- '!turn::approval_resume'') to the
deny block in iii-permissions.yaml so that any calls under the
turn::approval_resume prefix are prevented.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
harness-node/src/runtime/state.ts (2)
142-143:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe deprecated
stateListwrapper still dropsprefixsemantics.
_prefixis accepted but ignored, so unchanged callers can now receive the whole scope instead of prefix-scoped results. Preserve behavior until migration (pass prefix through or conditionally filter when keyed rows are available).🤖 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-node/src/runtime/state.ts` around lines 142 - 143, The stateList wrapper currently accepts _prefix but ignores it, causing callers to lose prefix-scoped behavior; update the stateList function to forward the _prefix to stateListValues (e.g., stateListValues(iii, { scope, prefix: _prefix })) or, if stateListValues returns keyed rows only in some cases, apply conditional filtering inside stateList using the _prefix to preserve the original prefix-scoped results; refer to the stateList function signature and the stateListValues call to implement the fix.
35-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly unwrap list envelopes when the row shape is unambiguous.
The current parsing still unwraps any object with a
valueproperty, which can corrupt legitimate stored objects (e.g.{ value: 1, state: 'x' }becomes1). Restrict unwrapping to envelope-only shapes ({value}or{key,value}), and keep all other objects intact.Proposed minimal fix
function unwrapStateListEntry<T>(entry: unknown): T { - if (entry && typeof entry === 'object' && 'value' in (entry as Record<string, unknown>)) { - return (entry as Record<string, unknown>).value as T; + if (entry && typeof entry === 'object') { + const row = entry as Record<string, unknown>; + const keys = Object.keys(row); + if ('value' in row && keys.every((k) => k === 'value' || k === 'key')) { + return row.value as T; + } } return entry as T; }Also applies to: 44-47
🤖 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-node/src/runtime/state.ts` around lines 35 - 39, The code currently unwraps any object with a value property which can corrupt real objects; update stateListResponseRows so it only unwraps when the object shape is an envelope-only form—i.e., when the item is an object whose own property names are exactly ["value"] (unwrap to item.value) or exactly ["key","value"] (unwrap to { key, value }.value? actually keep semantics: return the item.value for value-only envelopes and return items mapped to { key, value } for key/value envelopes as before) — otherwise return the original object intact; apply the same exact envelope-only check and behavior to the other envelope-unwrapping location in this file (the nearby single-item/single-response unwrap block) so only unambiguous {value} or {key,value} shapes are unwrapped.
🤖 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.
Duplicate comments:
In `@harness-node/src/runtime/state.ts`:
- Around line 142-143: The stateList wrapper currently accepts _prefix but
ignores it, causing callers to lose prefix-scoped behavior; update the stateList
function to forward the _prefix to stateListValues (e.g., stateListValues(iii, {
scope, prefix: _prefix })) or, if stateListValues returns keyed rows only in
some cases, apply conditional filtering inside stateList using the _prefix to
preserve the original prefix-scoped results; refer to the stateList function
signature and the stateListValues call to implement the fix.
- Around line 35-39: The code currently unwraps any object with a value property
which can corrupt real objects; update stateListResponseRows so it only unwraps
when the object shape is an envelope-only form—i.e., when the item is an object
whose own property names are exactly ["value"] (unwrap to item.value) or exactly
["key","value"] (unwrap to { key, value }.value? actually keep semantics: return
the item.value for value-only envelopes and return items mapped to { key, value
} for key/value envelopes as before) — otherwise return the original object
intact; apply the same exact envelope-only check and behavior to the other
envelope-unwrapping location in this file (the nearby
single-item/single-response unwrap block) so only unambiguous {value} or
{key,value} shapes are unwrapped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 658ecffb-3f99-4655-bf9c-ceca1f90250d
📒 Files selected for processing (1)
harness-node/src/runtime/state.ts
Merged
5 tasks
This was referenced Jul 3, 2026
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
harness-node's approval-gate was a literal Rust port: hand-rolledtypeofvalidation,as Record<string, unknown>casts at every boundary,Promise<unknown>handler returns, mutable redaction loops, and inline composite-key string slicing. The rest ofharness-nodehad already moved to Zod schemas + inferred types + JSON-schema export. This PR brings the approval-gate and the harness policy module up to that bar, with no change to the on-the-wire contract.approval-gate
schemas.ts(new) — Zod schemas, inferred types, and a derived JSON schema forapproval::resolve. Houses the wire schemas, key helpers (pendingKey/approvalResumeFnId),parsePolicyReply, andSTATE_SCOPE.ResolvePayloadSchemaacceptsfunction_call_idor the legacytool_call_idalias and transforms to a single non-optionalfunction_call_id, so callers never need a non-null assertion; it also rejects/in ids at the boundary (the reserved state-key separator).resolve.ts(replacespending.ts) —handleResolveRequestvalidates withsafeParseand routes the decision to the per-call resume function. Returns{ ok: true }or{ ok: false, error: 'invalid_payload' | 'resume_failed' }.redact.ts(new, extracted fromdenial.ts) — pure recursive redaction viaObject.fromEntries, guarded by a freeze-input test.denial.tsis now just envelope construction.parsePolicyReply— a discriminated-union decoder forpolicy::check_permissions; unknown shapes fall back toneeds_approval.policy-consult.ts(folded intoschemas.ts),config.tsand theapproval_gateblock inconfig.yaml(scope is fixed in code),on-decision-written.ts,register.ts,types.ts.harness policy
harness/policy.ts(314 lines) +policy-fn.tssplit into a focusedharness/policy/module:handle.ts—PermissionsHandle+ chokidar hot-reload ofiii-permissions.yaml.permissions.ts—Permissions.check(function_id, args)(first match wins).compile.ts— rule compilation +equals/matchesconstraint evaluation.check-permissions.ts—policy::check_permissionsregistration.types.ts— rule / decision types.turn-orchestrator
approval-resume.ts(new) — per-callturn::approval_resume::<sid>/<fcid>registration, the resume handler (persist decision → waketurn::step), and startup recovery for sessions parked across a restart.hook.ts—consultBeforeconsultspolicy::check_permissionsdirectly (5 s timeout) and maps the reply viaparsePolicyReply; fails closed with agate_unavailabledenial envelope.agent-call.ts—dispatchWithHookreturns one ofresult/deny/pending;pendingparks the call.states/functions.ts— parks pending calls intoawaiting_approval, registers a resume function per call, and folds resolved decisions back into the prepared snapshot (allow→pre_approved,deny/aborted→blocked).config.tsdrops the now-unusedpolicy_function_id.Wire contract preserved
handleResolveRequest{ ok: true }|{ ok: false; error: 'invalid_payload' | 'resume_failed' }policy::check_permissions{ decision: 'allow' | 'deny' | 'needs_approval', rule_id?, matched_constraint? }session_id,function_call_id,tool_call_id(fallback),rule_id,matched_constraint(snake_case)approvals, key<session_id>/<function_call_id>Also
runtime/state.ts: typedstate::listhelpers;session/tree/store.tsre-sorts entries by(timestamp, id).architecture.mdandworkers/{approval-gate,harness,turn-orchestrator}.md.Verification
tsc -b --noEmit,biome check, fullvitestsuite — 497 / 497 pass.schemas,resolve,redact,approval-resume, plus an expandedpolicy.test.ts. Removed obsoletepending/types/policy-consult/on-decision-writtensuites.Summary by CodeRabbit
Refactor
New Features
Bug Fixes