feat(approval-gate): state-as-truth refactor — direct policy consult + reactive turn_state event + reload recovery - #159
Merged
Conversation
Replace the imperative `turn::step_requested` self-publish with a state trigger on `session/<id>/turn_state`. Saving the record is now the wake end-to-end — both the first step (from `run::start`) and every subsequent transition fire through the same reactive path. - Register `on-record-written` state trigger in register.ts (mirrors the abort + terminal blocks; condition excludes `stopped` and `function_awaiting_approval` so we don't loop on parked turns). - Drop the redundant `publishStep` from run-start.ts (the initial `saveRecord(state='provisioning')` now wakes turn::step via the trigger; keeping the publish was a double-step on every new session). Removes the function, its export, `STEP_TOPIC`, and the `logger` import that's now unused. - Drop the self-publish from subscriber.ts after each transition; also drop the dangling `Mirrors turn-orchestrator/src/subscriber.rs` comment (Rust workers were removed in 07311d9). - Refactor on-record-written.ts so the condition and handler share a single `parseStepableWrite` — the handler can no longer fire on a parking-state write even if the condition is bypassed. - Remove unconditional `console.log` in transitions.ts. Tests: - New unit tests in tests/turn-orchestrator/on-record-written.test.ts cover the condition (stepable / parking / terminal / wrong key / malformed shape) and the handler (happy path, no-op on key mismatch, swallow on iii.trigger failure). - New integration test in tests/integration/on-record-written.e2e.test.ts drives a fake iii through state::set → handler → turn::step and verifies stepable wake, multi-transition wake, parking no-wake, terminal no-wake, and non-turn_state no-leak. - Update run-start.test.ts: drop the now-broken `iii::durable::publish` assertion, replace it with a positive assertion on the `turn_state` state::set that triggers the wake, and assert no publish fires.
…tching other orchestrator config
…hHook - Widen consultBefore signature with _policy_function_id (5th param, unused until Task 3) - registerAgentCall now receives orchestratorCfg.policy_function_id - handleExecute takes cfg: TurnOrchestratorConfig and forwards cfg.policy_function_id to dispatchWithHook - transitions.ts passes cfg through to handleExecute - functions.test.ts updated with cfg stub for all handleExecute call sites
…from consultBefore Replace the hook-fanout::publish_collect before-hook round-trip with a direct iii.trigger call to the configured policy_function_id. Maps policy replies (allow/deny/needs_approval) to HookOutcome. Fail-closed on transport error: deny with gate_unavailable, unless function_id is in the legacy approval_required list (then pending). Drops merged field from HookOutcome pending variant. publishAfter still uses hook-fanout (after-hook unchanged).
…ch prior policy timeout - Remove duplicate DeniedBy/DenialEnvelope/DENIAL_SCHEMA_VERSION from hook.ts; import from approval-gate/types and re-export so agent-call.ts consumers are unaffected - Drop timeoutMs from HOOK_TIMEOUT_MS (10s) to 5_000 on the policy trigger, matching the prior gate-subscriber → policy hop budget - Add session_id rationale comment and policy timeout rationale comment - Fix misleading test name: "substring" → "list" to reflect Array.includes semantics
…proval The gate-subscriber that previously emitted this event was deleted in the approval-gate refactor. The web UI still consumes it via translate.ts to flip the function card into pending-approval mode. Emit it from handleExecute directly when the dispatch returns pending.
…ng with on-record-written
…unction_execution_end
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR refactors the approval flow architecture by moving approval checking from a durable gate subscriber that intercepts function calls to direct policy consultation in the turn orchestrator, with approval state now driven by state-trigger adapters and observable to the frontend via turn_state_changed events. ChangesApproval flow refactor: direct policy consult and state-driven pending approvals
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
skill-check — worker0 verified, 10 skipped (no docs/).
Three for three. Nicely done. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
harness-node/src/turn-orchestrator/states/functions.ts (1)
87-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle trigger failures in the pre-approved execution path.
Line 88 awaits
iii.triggerwithout atry/catch. If the tool call throws (timeout, function_not_found, transport error),handleExecuteexits early and the turn can remain stuck infunction_executewithout afunction_execution_endevent for that call.Suggested fix
if (entry.pre_approved === true) { - const value = await iii.trigger<unknown, unknown>({ - function_id: fc.function_id, - payload: fc.arguments ?? {}, - }); - const result = decodeOrPassthroughResult(value); + let result: FunctionResult; + try { + const value = await iii.trigger<unknown, unknown>({ + function_id: fc.function_id, + payload: fc.arguments ?? {}, + }); + result = decodeOrPassthroughResult(value); + } catch (err) { + result = { + content: [text(`pre_approved trigger failed: ${String(err)}`)], + details: { error: 'trigger_failed', function: fc.function_id, message: String(err) }, + terminate: false, + }; + } const is_error = isErrorResult(result); persistence.upsertExecutedCall(results, { function_call: fc, result, is_error }); await persistence.saveExecutedCalls(iii, rec.session_id, results); await emit(iii, rec.session_id, buildFunctionExecutionEnd(fc, result, is_error)); continue;🤖 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/states/functions.ts` around lines 87 - 97, The pre-approved execution block calls iii.trigger without error handling; wrap the iii.trigger(...) await inside a try/catch in the pre_approved branch (where entry.pre_approved === true) so any thrown error (timeout, function_not_found, transport error) is caught, convert the caught error into a function result (use the same shape that decodeOrPassthroughResult/isErrorResult expect, mark is_error = true), then call persistence.upsertExecutedCall(results, { function_call: fc, result, is_error }), await persistence.saveExecutedCalls(iii, rec.session_id, results), and await emit(iii, rec.session_id, buildFunctionExecutionEnd(fc, result, is_error)) before continuing; ensure the normal successful path (calling decodeOrPassthroughResult and isErrorResult) remains unchanged inside the try.
🤖 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/docs/workers/approval-gate.md`:
- Around line 91-92: The documentation incorrectly states that the config key
`policy_function_id` lives under `approval_gate`; update the text and any
examples so `policy_function_id` is shown and described as a top-level key in
`config.yaml` (not nested under `approval_gate`), and move or reword the bullet
that references it so it appears in the root-level config section; update any
sample YAML blocks and the explanatory sentence that mentions
`consultBefore`/orchestrator to reference the top-level `policy_function_id`
instead of an `approval_gate.policy_function_id`.
In `@harness-node/src/approval-gate/pending.ts`:
- Line 31: pendingKey(session_id, function_call_id) can throw for malformed ids
which currently escapes the function's { ok: false, error: ... } contract; wrap
the call to pendingKey(...) in a try/catch, catch any exception, and return the
standard error shape (e.g., { ok: false, error: err.message || String(err) })
instead of letting the exception bubble, keeping the rest of the function flow
intact and using the same variable name (key) as before.
---
Outside diff comments:
In `@harness-node/src/turn-orchestrator/states/functions.ts`:
- Around line 87-97: The pre-approved execution block calls iii.trigger without
error handling; wrap the iii.trigger(...) await inside a try/catch in the
pre_approved branch (where entry.pre_approved === true) so any thrown error
(timeout, function_not_found, transport error) is caught, convert the caught
error into a function result (use the same shape that
decodeOrPassthroughResult/isErrorResult expect, mark is_error = true), then call
persistence.upsertExecutedCall(results, { function_call: fc, result, is_error
}), await persistence.saveExecutedCalls(iii, rec.session_id, results), and await
emit(iii, rec.session_id, buildFunctionExecutionEnd(fc, result, is_error))
before continuing; ensure the normal successful path (calling
decodeOrPassthroughResult and isErrorResult) remains unchanged inside the try.
🪄 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: 0b875988-92e4-4dab-b1aa-cd34bf54699b
📒 Files selected for processing (49)
console/web/src/components/chat/ChatView.tsxconsole/web/src/lib/backend/pending-approvals-store.test.tsconsole/web/src/lib/backend/pending-approvals-store.tsconsole/web/src/lib/backend/real.tsconsole/web/src/lib/backend/translate.test.tsconsole/web/src/lib/backend/translate.tsconsole/web/src/lib/backend/turn-state-mirror.test.tsconsole/web/src/lib/backend/turn-state-mirror.tsconsole/web/src/types/iii-agent-event.tsharness-node/config.yamlharness-node/docs/architecture.mdharness-node/docs/workers/approval-gate.mdharness-node/docs/workers/turn-orchestrator.mdharness-node/src/approval-gate/config.tsharness-node/src/approval-gate/gate-subscriber.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/register.tsharness-node/src/approval-gate/state-bus.tsharness-node/src/approval-gate/types.tsharness-node/src/index.tsharness-node/src/runtime/state.tsharness-node/src/turn-orchestrator/agent-call.tsharness-node/src/turn-orchestrator/config.tsharness-node/src/turn-orchestrator/hook.tsharness-node/src/turn-orchestrator/on-record-written.tsharness-node/src/turn-orchestrator/on-turn-state-changed.tsharness-node/src/turn-orchestrator/register.tsharness-node/src/turn-orchestrator/run-start.tsharness-node/src/turn-orchestrator/states/functions.tsharness-node/src/turn-orchestrator/subscriber.tsharness-node/src/turn-orchestrator/transitions.tsharness-node/src/types/agent-event.tsharness-node/tests/approval-gate/gate-subscriber.test.tsharness-node/tests/approval-gate/on-decision-written.test.tsharness-node/tests/approval-gate/pending.test.tsharness-node/tests/approval-gate/types.test.tsharness-node/tests/integration/approval-resume.e2e.test.tsharness-node/tests/integration/on-record-written.e2e.test.tsharness-node/tests/turn-orchestrator/agent-call.test.tsharness-node/tests/turn-orchestrator/config.test.tsharness-node/tests/turn-orchestrator/functions.test.tsharness-node/tests/turn-orchestrator/hook.test.tsharness-node/tests/turn-orchestrator/on-record-written.test.tsharness-node/tests/turn-orchestrator/on-turn-state-changed.test.tsharness-node/tests/turn-orchestrator/run-start.test.ts
💤 Files with no reviewable changes (7)
- harness-node/src/approval-gate/config.ts
- harness-node/src/approval-gate/state-bus.ts
- harness-node/src/turn-orchestrator/run-start.ts
- console/web/src/components/chat/ChatView.tsx
- harness-node/tests/approval-gate/gate-subscriber.test.ts
- harness-node/src/approval-gate/gate-subscriber.ts
- harness-node/src/approval-gate/types.ts
| @@ -35,50 +31,13 @@ export async function handleResolve( | |||
| const key = pendingKey(session_id, function_call_id); | |||
There was a problem hiding this comment.
Guard pendingKey(...) exceptions to preserve the function’s error contract.
On Line 31, pendingKey(session_id, function_call_id) can throw for malformed ids (e.g., containing /), which bypasses your { ok: false, error: ... } response flow and bubbles as an unhandled error.
Suggested fix
- const key = pendingKey(session_id, function_call_id);
+ let key = '';
+ try {
+ key = pendingKey(session_id, function_call_id);
+ } catch {
+ return { ok: false, error: 'missing_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-node/src/approval-gate/pending.ts` at line 31, pendingKey(session_id,
function_call_id) can throw for malformed ids which currently escapes the
function's { ok: false, error: ... } contract; wrap the call to pendingKey(...)
in a try/catch, catch any exception, and return the standard error shape (e.g.,
{ ok: false, error: err.message || String(err) }) instead of letting the
exception bubble, keeping the rest of the function flow intact and using the
same variable name (key) as before.
…e turn_state wakes Two P1 findings from Codex review: 1. hook.ts:88-89 (permissions bypass): the needs_approval branch returned allow when approval_required was non-empty and the function wasn't in the list. That meant a run with approval_required=['shell::run'] let every other no-rule function execute without approval. The legacy list was only ever meant to be consulted in the fail-closed catch path when the policy worker is unreachable. Remove the bypass: when policy returns needs_approval, always pending. 2. on-record-written.ts:47-53 (same-state wakes): the condition accepted any non-terminal, non-parking turn_state write. handlePrepare calls saveRecord while the record is still in function_prepare to persist normalized calls; that same-state write was waking turn::step and racing the in-flight handler. Filter state:updated writes whose old_value.state equals new_value.state. Both regressions guarded by new tests.
After the policy-direct refactor, approval_required was consulted in exactly one place (consultBefore's catch path when policy is unreachable) and the FE stopped sending it. So the parameter was dead pipeline: parsed in run-start.ts, loaded in functions.ts, threaded through agent-call.ts to consultBefore, used only in a branch that effectively never fires in production. Drop it end-to-end. The fail-closed branch now unconditionally returns deny with a gate_unavailable envelope when policy is unreachable. No caller-list escape valve, no per-run allowlist sneaking around the policy. Net: -82 LOC, simpler dispatchWithHook signature, simpler catch path, simpler run::start payload contract. Tests updated: dropped the 'legacy approval_required list when policy unreachable' and 'non-empty approval_required omits this function' test cases. fails-closed test is the sole guard for the unreachable path. Docs in architecture.md and workers/turn-orchestrator.md no longer mention the legacy list.
1. run-start.ts: save the initial turn record AFTER emitting agent_start and the user-message prelude. on-record-written wakes turn::step reactively, so saving first lets the FSM race ahead of the prelude loop and the subscriber sees provisioning-state events before agent_start. Reordering closes the race. 2. docs/workers/approval-gate.md: stop documenting approval_gate.policy_function_id — the field was removed from ApprovalGateConfig in a prior commit, so an operator setting it would have it silently ignored. The active key is top-level policy_function_id, owned by the orchestrator's config slice. P2 #1 from the re-review (preserve legacy approval_required) is obsoleted by the prior 'drop legacy approval_required parameter' commit, which removes the parameter end-to-end.
This commit cleans up the `hook.ts` and `register.ts` files by removing obsolete comments that no longer provide value. The changes enhance code readability and maintainability, streamlining the overall implementation without affecting functionality.
ytallo
force-pushed
the
feat/approval-gate-direct-policy
branch
from
May 19, 2026 10:48
d397c33 to
18a1f5f
Compare
…ilure Codex P1: replacing the durable publish with a direct iii.trigger to turn::step removed the retry safety net. If the direct invoke times out or throws after the triggering state write already landed, the state-trigger handler succeeds but no turn::step_requested message exists for the durable subscriber to retry — the session sits stuck in that state forever. Restore the safety net by falling back to iii::durable::publish on turn::step_requested when the direct invoke rejects. Direct trigger stays the fast path; the durable bus is the fallback that buffers and retries. If the publish fallback also fails, log error and exit (best effort fail-closed — caller can recover via abort or restart). Also export STEP_TOPIC from subscriber.ts so on-record-written can reuse it without duplicating the string literal. Tests: - New: 'falls back to durable publish when the direct turn::step invoke fails' asserts both the direct attempt and the durable publish payload shape. - Renamed: 'swallows when BOTH the direct invoke and durable publish fallback fail' makes the existing log-only behaviour explicit.
Comment on lines
+117
to
+138
| client | ||
| .call<Record<string, unknown> | null>('state::get', { | ||
| scope: 'agent', | ||
| key: `session/${sessionId}/turn_state`, | ||
| }) | ||
| .then((record) => { | ||
| if (!record) return | ||
| queue.push({ | ||
| type: 'turn_state_changed', | ||
| event_type: 'state:created', | ||
| new_value: record, | ||
| }) | ||
| wake() | ||
| }) | ||
| .catch((err) => { | ||
| if (import.meta.env.DEV) { | ||
| console.warn( | ||
| '[real-backend] state::get turn_state recovery failed', | ||
| err, | ||
| ) | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
i think we shouldn't call state directly from the frontend, it should be abstracted on the harness logic
Contributor
Author
There was a problem hiding this comment.
Good call. Added turn::get_state (harness-node/src/turn-orchestrator/get-state.ts) that wraps persistence.loadRecord, and the console now calls it instead of state::get for reload recovery. Schema and key layout stay inside the orchestrator. Pushed in 8292ad0.
…hook tail latency The pre_approved branch in handleExecute did not catch the inner iii.trigger rejection, so a bad-args dispatch (e.g. shell::fs::write called with a raw string for `content`) propagated out of the state machine, leaving the turn stuck in function_execute. The reactive durable-retry then re-published the same failing transition forever, and the UI never received function_execution_end. Wrap the trigger and synthesize a trigger_failed FunctionResult so the card closes and the LLM sees the error. Also drop HOOK_TIMEOUT_MS from 10_000ms to 500ms. The after-hook fanout on agent::after_function_call has zero subscribers after this PR removed gate-subscriber, so publish_collect always hits the deadline; per-call finalize was eating the full 10s.
sergiofilhowz
approved these changes
May 19, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
harness-node/docs/workers/approval-gate.md (1)
58-59:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix relative markdown links; several targets appear broken from this directory.
From
harness-node/docs/workers/approval-gate.md, links likeharness-node/src/...andworkers/turn-orchestrator.mddon’t resolve correctly relative to this file’s location.Suggested patch pattern
-[config.yaml](harness-node/config.yaml) +[config.yaml](../../config.yaml) -[workers/turn-orchestrator.md](workers/turn-orchestrator.md) +[workers/turn-orchestrator.md](turn-orchestrator.md) -[src/approval-gate/types.ts](harness-node/src/approval-gate/types.ts) +[src/approval-gate/types.ts](../../src/approval-gate/types.ts) -[src/approval-gate/main.ts](harness-node/src/approval-gate/main.ts) +[src/approval-gate/main.ts](../../src/approval-gate/main.ts)Also applies to: 85-86, 93-94, 103-118
🤖 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/docs/workers/approval-gate.md` around lines 58 - 59, The markdown in harness-node/docs/workers/approval-gate.md contains broken relative links (e.g., references to harness-node/src/approval-gate/types.ts, harness-node/src/approval-gate/pending.ts, and workers/turn-orchestrator.md) that do not resolve from this doc's location; update each link to use the correct relative path from docs/workers/approval-gate.md (for example, adjust "harness-node/src/..." to "../../src/approval-gate/types.ts" or the appropriate relative traversal) and fix other referenced targets on the same file (lines around the occurrences such as the types.ts, pending.ts, and turn-orchestrator.md links) so they point to the actual files in the repo tree rather than a top-level harness-node prefix.
🧹 Nitpick comments (2)
console/web/src/lib/backend/real.ts (1)
117-138: 💤 Low valueFire-and-forget recovery call may silently fail in production.
The
state::getcall for turn_state recovery only logs errors in DEV mode. While this is reasonable for a best-effort recovery path, consider whether silent failures in production could make debugging reload-recovery issues difficult.If recovery failures are expected to be rare and non-critical, this is acceptable. Otherwise, consider at least emitting a metric or structured log entry (not just console.warn) for observability.
🤖 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/real.ts` around lines 117 - 138, The fire-and-forget recovery call using client.call('state::get', { key: `session/${sessionId}/turn_state` }) only logs errors under import.meta.env.DEV; update the catch handler on that promise to also emit a production-friendly observability signal (e.g., structured logger or metrics) when the call fails so failures aren’t silent in prod—modify the .catch(err => { ... }) block that currently does console.warn to call your app logger/metrics emitter with context (sessionId, operation: 'state::get', resource: 'turn_state', and the error), while keeping the DEV console.warn and still not throwing from queue.push/wake.harness-node/src/turn-orchestrator/states/functions.ts (1)
150-150: 💤 Low valueStray whitespace on line 150.
Line 150 appears to contain only trailing whitespace. Consider removing it for cleanliness.
🤖 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/states/functions.ts` at line 150, Remove the stray whitespace-only line inside the states functions module (functions.ts) by deleting the empty/trailing-whitespace-only line so the file has no blank line containing only spaces; this is a simple cleanup in the turn-orchestrator states module (functions.ts) to satisfy linting and cleanliness checks.
🤖 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/lib/backend/translate.ts`:
- Around line 170-189: The createTurnStateTranslator function creates a
long-lived mirrors Map that never releases per-session entries; update it so
session data is cleared when a stream ends or when an agent_end event is seen:
modify createTurnStateTranslator to either (A) return an object with a
translate(event, sessionId) method plus a cleanup(sessionId) method (or a
translate signature that accepts an options flag like {cleanup:true}), or (B)
detect agent_end inside the translate function (checking event.new_value for
agent_end) and call mirrors.delete(sessionId). Reference
createTurnStateTranslator, mirrors, pendingApprovalsFromTurnState, and
TurnStateChangedEvent; ensure cleanup is invoked when processing
agent_end/stream termination to avoid unbounded Map growth.
In `@harness-node/src/turn-orchestrator/hook.ts`:
- Around line 58-61: The denial payload currently embeds String(err) (in the
gateUnavailableEnvelope call with function_call.function_id), which can leak
internal transport/runtime details; instead log the raw err using the existing
logger (or console.error) and replace the reason passed into
gateUnavailableEnvelope with a stable public message like "policy engine
unreachable" or "upstream service unavailable" so only non-sensitive information
is returned in denial responses. Ensure you modify the
gateUnavailableEnvelope(...) invocation to use the stable message while adding a
logger.error(err, "...context...") nearby to record full details.
---
Outside diff comments:
In `@harness-node/docs/workers/approval-gate.md`:
- Around line 58-59: The markdown in harness-node/docs/workers/approval-gate.md
contains broken relative links (e.g., references to
harness-node/src/approval-gate/types.ts,
harness-node/src/approval-gate/pending.ts, and workers/turn-orchestrator.md)
that do not resolve from this doc's location; update each link to use the
correct relative path from docs/workers/approval-gate.md (for example, adjust
"harness-node/src/..." to "../../src/approval-gate/types.ts" or the appropriate
relative traversal) and fix other referenced targets on the same file (lines
around the occurrences such as the types.ts, pending.ts, and
turn-orchestrator.md links) so they point to the actual files in the repo tree
rather than a top-level harness-node prefix.
---
Nitpick comments:
In `@console/web/src/lib/backend/real.ts`:
- Around line 117-138: The fire-and-forget recovery call using
client.call('state::get', { key: `session/${sessionId}/turn_state` }) only logs
errors under import.meta.env.DEV; update the catch handler on that promise to
also emit a production-friendly observability signal (e.g., structured logger or
metrics) when the call fails so failures aren’t silent in prod—modify the
.catch(err => { ... }) block that currently does console.warn to call your app
logger/metrics emitter with context (sessionId, operation: 'state::get',
resource: 'turn_state', and the error), while keeping the DEV console.warn and
still not throwing from queue.push/wake.
In `@harness-node/src/turn-orchestrator/states/functions.ts`:
- Line 150: Remove the stray whitespace-only line inside the states functions
module (functions.ts) by deleting the empty/trailing-whitespace-only line so the
file has no blank line containing only spaces; this is a simple cleanup in the
turn-orchestrator states module (functions.ts) to satisfy linting and
cleanliness checks.
🪄 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: 20157e9c-4a6e-4fba-b820-14cd42591cc5
📒 Files selected for processing (16)
console/web/src/lib/backend/real.tsconsole/web/src/lib/backend/translate.tsharness-node/docs/architecture.mdharness-node/docs/workers/approval-gate.mdharness-node/docs/workers/turn-orchestrator.mdharness-node/src/turn-orchestrator/agent-call.tsharness-node/src/turn-orchestrator/hook.tsharness-node/src/turn-orchestrator/on-record-written.tsharness-node/src/turn-orchestrator/register.tsharness-node/src/turn-orchestrator/run-start.tsharness-node/src/turn-orchestrator/states/functions.tsharness-node/src/turn-orchestrator/subscriber.tsharness-node/tests/turn-orchestrator/agent-call.test.tsharness-node/tests/turn-orchestrator/functions.test.tsharness-node/tests/turn-orchestrator/hook.test.tsharness-node/tests/turn-orchestrator/on-record-written.test.ts
✅ Files skipped from review due to trivial changes (1)
- harness-node/docs/architecture.md
🚧 Files skipped from review as they are similar to previous changes (5)
- harness-node/tests/turn-orchestrator/agent-call.test.ts
- harness-node/tests/turn-orchestrator/on-record-written.test.ts
- harness-node/docs/workers/turn-orchestrator.md
- harness-node/src/turn-orchestrator/register.ts
- harness-node/src/turn-orchestrator/run-start.ts
Comment on lines
+170
to
+189
| export function createTurnStateTranslator(): ( | ||
| event: TurnStateChangedEvent, | ||
| sessionId: string, | ||
| ) => StreamEvent[] { | ||
| const mirrors = new Map<string, PendingApproval[]>() | ||
| return (event, sessionId) => { | ||
| const prev = mirrors.get(sessionId) ?? [] | ||
| const next = pendingApprovalsFromTurnState(event.new_value) | ||
| mirrors.set(sessionId, next) | ||
| const { added } = diffPending(prev, next) | ||
| return added.map((entry) => ({ | ||
| kind: 'fcall-start' as const, | ||
| functionId: entry.function_id, | ||
| input: entry.args, | ||
| pendingApproval: true, | ||
| functionCallId: entry.function_call_id, | ||
| sessionId, | ||
| })) | ||
| } | ||
| } |
There was a problem hiding this comment.
Unbounded growth of mirrors Map across sessions.
The mirrors Map accumulates entries for each session but is never cleaned up. In a long-running browser tab with multiple chat sessions, this could cause memory growth.
Consider clearing the session entry when agent_end is received, or implementing a cleanup mechanism when the stream terminates.
🛠️ Suggested approach
Either expose a cleanup method from the translator, or clear entries reactively:
export function createTurnStateTranslator(): (
event: TurnStateChangedEvent,
sessionId: string,
+ opts?: { cleanup?: boolean },
) => StreamEvent[] {
const mirrors = new Map<string, PendingApproval[]>()
- return (event, sessionId) => {
+ return (event, sessionId, opts) => {
+ if (opts?.cleanup) {
+ mirrors.delete(sessionId)
+ return []
+ }
const prev = mirrors.get(sessionId) ?? []Then call with { cleanup: true } when processing agent_end.
🤖 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/translate.ts` around lines 170 - 189, The
createTurnStateTranslator function creates a long-lived mirrors Map that never
releases per-session entries; update it so session data is cleared when a stream
ends or when an agent_end event is seen: modify createTurnStateTranslator to
either (A) return an object with a translate(event, sessionId) method plus a
cleanup(sessionId) method (or a translate signature that accepts an options flag
like {cleanup:true}), or (B) detect agent_end inside the translate function
(checking event.new_value for agent_end) and call mirrors.delete(sessionId).
Reference createTurnStateTranslator, mirrors, pendingApprovalsFromTurnState, and
TurnStateChangedEvent; ensure cleanup is invoked when processing
agent_end/stream termination to avoid unbounded Map growth.
Comment on lines
58
to
61
| denial: gateUnavailableEnvelope( | ||
| function_call.function_id, | ||
| `permission gate did not respond: ${String(err)}`, | ||
| `policy unreachable: ${String(err)}`, | ||
| ), |
There was a problem hiding this comment.
Avoid returning raw transport error details in denial payloads.
reason includes String(err), which can leak internal runtime/network details via blocked results. Log the raw error, but return a stable public message in the envelope.
Suggested patch
return {
kind: 'deny',
denial: gateUnavailableEnvelope(
function_call.function_id,
- `policy unreachable: ${String(err)}`,
+ 'policy unreachable',
),
};📝 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
| denial: gateUnavailableEnvelope( | |
| function_call.function_id, | |
| `permission gate did not respond: ${String(err)}`, | |
| `policy unreachable: ${String(err)}`, | |
| ), | |
| return { | |
| kind: 'deny', | |
| denial: gateUnavailableEnvelope( | |
| function_call.function_id, | |
| 'policy unreachable', | |
| ), | |
| }; |
🤖 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 58 - 61, The denial
payload currently embeds String(err) (in the gateUnavailableEnvelope call with
function_call.function_id), which can leak internal transport/runtime details;
instead log the raw err using the existing logger (or console.error) and replace
the reason passed into gateUnavailableEnvelope with a stable public message like
"policy engine unreachable" or "upstream service unavailable" so only
non-sensitive information is returned in denial responses. Ensure you modify the
gateUnavailableEnvelope(...) invocation to use the stable message while adding a
logger.error(err, "...context...") nearby to record full details.
…t from console
Console previously called `state::get { scope: 'agent', key:
'session/<sid>/turn_state' }` directly from the browser for reload
recovery, which leaks the orchestrator's state schema and key
layout into the frontend. Add a `turn::get_state` orchestrator
function that wraps `persistence.loadRecord` and have the console
call it instead. Schema/key changes now stay inside the worker.
This was referenced May 19, 2026
This was referenced Jun 4, 2026
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
Collapses the approval-gate's three-hop hook chain into a direct
policy::check_permissionscall from the orchestrator, then makes the frontend derive pending-approval modals from the orchestrator'sturn_staterecord via a new reactiveturn_state_changedevent — so reload-mid-approval re-mounts the modal and the legacyapproval_requested/approval_resolvedsignal events can be deleted entirely.Backend production code −143 LOC (gate-subscriber, state-bus, hook plumbing, dead types). Frontend gains three small framework-free modules + a stateful translator. End state: state is the source of truth on both sides; events are just notifications.
What changed
Backend (turn-orchestrator + approval-gate):
consultBeforecallspolicy::check_permissionsdirectly viaiii.trigger(5s timeout). No morehook-fanout::publish_collectfor the before-hook path, noagent::before_function_calltopic, nopolicy::approval_gatedurable subscriber. Fail-closed semantics + legacyapproval_requiredfallback preserved.approval-gate/gate-subscriber.ts,state-bus.ts,IncomingCall/extractCall/blockReplyFor/GateBlockReply/SUBSCRIBER_NAMEwire types,consultPolicydead export.approval::on_decision_writtennow triggersturn::stepdirectly (mirrorson-record-written.ts). Both reactive wake paths use the same primitive.STEP_FN_ID = 'turn::step'consolidated to a single canonical definition insubscriber.ts.on-turn-state-changed.tsstate-trigger adapter emitsturn_state_changedon everyturn_statewrite — carries the full new (and prior) record so the FE can derive pending approvals from state.Frontend (console/web):
pending-approvals-store.ts— framework-free diff helper keyed byfunction_call_id.turn-state-mirror.ts— pure reducer that converts a turn_state record into aPendingApproval[].createTurnStateTranslator()factory intranslate.ts— stateful per-session mirror, emitsfcall-start { pendingApproval: true }exactly once per new pending call (suppresses duplicates on re-broadcast).realStreamfires a one-shotstate::geton subscribe so modals re-mount after page reload.approval_requestedandapproval_resolvedconsumer cases fromtranslate.tsand the matching variants fromiii-agent-event.ts. Denial UX continues to flow throughfunction_execution_end(the orchestrator'sentry.blocked→buildFunctionExecutionEndpath).Docs:
architecture.md,workers/turn-orchestrator.md,workers/approval-gate.mdupdated to describe the new flow, the two distinct state triggers (step-waker vs event-emitter), and the reload-recovery contract.Diff stats
The net +87 is offset entirely by new test coverage for the new derivation chain (6 new test files:
on-turn-state-changed,hook,config,pending-approvals-store,turn-state-mirror,translate).Architectural wins (not visible in LOC)
policy::approval_gategone)agent::before_function_callgone)agent::hook_replystream round-trip per approval consultiii.trigger('turn::step', ...))Test plan
harness-nodesuite — 329 tests, all greenconsole/websuite — 173 tests, all greenpnpm exec tsc --noEmitclean on both packagesturn_state_changed, Approve writes the file (/tmp/qa-final-take2.txt=APPROVAL_WORKS_END_TO_END), card flips toran ƒ shell::exec for 0mswith full responseranwithtext: "Permission denied by user: denied",details.approval_denied: true— flowed throughfunction_execution_endblocked-result branch (noapproval_resolvedevent needed)state::get→ Approve completes → file writtengrep -rn "approval_requested\|approval_resolved\|handleResolveWithEvents\|gate-subscriber\|agent::before_function_call" harness-node/src console/web/srcreturns empty (or only in clear "no longer" docs context)Things to know
feat(harness-node): complete reactive turn-step wake(PR feat(harness-node): reactive approval resume + orchestrator cleanup #156 follow-up), which adds the on-record-written state trigger this PR builds on.handleExecute'spre_approvedbranch doesn't catchiii.triggererrors, so a malformed payload from the LLM (e.g., wrong field name onshell::fs::write) can hang the card indefinitely. Worth a follow-up to wrap in try/catch + emit a syntheticfunction_execution_endwith an error envelope.Summary by CodeRabbit
New Features
Improvements
Documentation