feat(server): add native OpenAI execution and durable turn recovery

[dimakis]

The Responses adapter in #451 streams function calls but cannot execute tools or recover native turns. This PR adds a permission-checked native executor and durable Responses runner that preserve application conversation identity and account/model binding.

Read/Write/Edit/Bash reuse the session's skill, worktree and permission policy. The runner persists model output before side effects and each tool outcome before continuing. Explicit follow-ups restore encrypted continuation; concurrent turns are rejected. Interrupted or crashed calls are never automatically replayed. Credentials are never serialized or implicitly inherited by shells.

This is an internal execution foundation, not wired into chat.ts dispatch. Server queues, MCP lifecycle, ContexGin startup wiring, native credential-reference resolution, reconnect/deduplication and physical-phone acceptance remain outstanding. OpenAI is not enabled in the mobile catalog. No deployment or live API validation was performed. Images, compaction, reasoning UI and ChatGPT Pro are not supported by this slice. See docs/features/openai-native-execution.md for the integration contract and remaining gaps.

Review fixes include atomic SQLite binding validation, a compile-time-complete checkpoint allowlist, partial multi-tool recovery, bounded Read/Edit input, labeled stderr, timeout/permission tests and direct persistence-callback tests. The generator begins work only when iterated; its guards execute synchronously before the first await/yield. Full-history cloning remains a documented scaling limit; the shell worktree policy remains a heuristic, not an OS sandbox.

Validation for 00de793: 210 files / 3,267 tests passed, including 28 executor/runner tests and shared loop callback tests. Server/frontend types, lint, formatting and builds passed; Fresh final-head CI is verified before merge. Existing lint/bundle warnings and advisory dependency audit findings remain. Four review rounds are triaged with fixes and evidence. Dependencies #449 (including #450) and #451 are now merged into main. The final native rebase preserves the exact source tree tested locally (6cc64b9e8af83b2a16db51e7ffe6617cc6564d17). Landing status is recorded by GitHub; no production deployment is included.

Final review resolution evidence:

Round 2 addressed in 468d37f.

• Saves now use one conditional SQLite upsert that rejects a mismatched binding without a preliminary full-state SELECT. Tests verify failed writes preserve the original state.
• The checkpoint allowlist remains deliberate. Automatically copying future fields would weaken credential exclusion. A Record<keyof ResponsesCheckpoint, unknown> completeness check now makes a newly added checkpoint field a compile error until its persistence is reviewed.
• A multi-tool recovery test persists A's result, interrupts before B, reopens the database, and verifies A survives, B is marked uncertain, and neither is replayed.
• Shell stderr is returned in a labeled section under the same combined output limit, with a regression test.
• Full-history cloning/persistence is documented as a scaling limitation. Per-tool durability and callback isolation remain required. Differential persistence and bounded history remain explicit follow-up work before broad activation.

Validation: 208 files / 3,242 tests; lint, formatting, server/frontend types and builds passed; CI run 34095672119 passed. This remains an internal foundation, not chat.ts dispatch or mobile OpenAI support.

Round 4 triage at cbe362e:

• Added exact-limit / limit+1 Read tests and explicitly unisolated session coverage. Both use the existing permission policy; canonical file paths remain correct with no worktree entries.
• New-file permissions are already 0600 in the atomic open/create operation. chmod tightens an existing file; O_EXCL would not remove prior access to an already-existing file. The enclosing directory is explicitly required to be private.
• The runner calls abort.signal.throwIfAborted() before examining every result. A cancelled result cannot be classified as successful completion or loop exhaustion. Current onHistory timing is directly tested; speculative future changes do not demonstrate a current bug.
• toolResults is a private local array inside runAgenticLoop. No existing caller can capture its lexical binding; onHistory receives a structured clone. Moving push before yield exposes no shared array reference. The durable-before-yield contract is directly tested.
• Documented history mutation and the runner's intentional error redaction before the generic loop catch.

All remaining runtime claims were investigated; the final update adds tests and documentation, not a new execution path. Full suite: 208 files / 3,249 tests passed, with all required checks.

[dimakis]

Centaur Review

Found 4 issue(s) (2 warning).

server/native-responses-runner.ts

Well-designed durable execution layer with strong crash-recovery semantics. One actionable bug: unprotected save() in the finally block can mask errors and leave the database in an inconsistent state.

• 🟡 bugs (L134): Unprotected save() in finally block can throw (e.g. database full/locked), which replaces the original error and leaves the database status as 'running' instead of 'interrupted'. The caller sees a misleading error and the conversation is stuck until recoverAtStartup(). Wrap in try-catch: try { save(); } catch { /* log but don't mask */ }. [fixable]

server/native-tool-executor.ts

Well-designed durable execution layer with strong crash-recovery semantics. One actionable bug: unprotected save() in the finally block can mask errors and leave the database in an inconsistent state.

• 🟡 unsafe_assumptions (L138): entry.path = await realpath(entry.path) mutates ManagedSession.worktreePaths entries in-place on every tool call. This is shared state owned by SessionRegistry. While realpath is idempotent on resolved paths, if a worktree path is removed between calls, realpath throws and the tool returns a confusing 'Session workspace is unavailable' error instead of a worktree-specific message. Resolve into a local copy instead of mutating the session. [fixable]
• 🔵 style (L145): JSON.stringify comparison for deep equality is fragile — objects with identical keys in different insertion order produce different strings. The current callers produce same-order objects, so this works today, but deepStrictEqual or a shallow key-by-key check would be more robust. [fixable]

server/__tests__/native-responses-runner.test.ts

Well-designed durable execution layer with strong crash-recovery semantics. One actionable bug: unprotected save() in the finally block can mask errors and leave the database in an inconsistent state.

• 🔵 missing_tests (L260): The 'stops before side effects if checkpoint persistence fails' test doesn't verify database state after the error. Because state.checkpoint remains truthy, the save() call in the finally block also throws 'disk full', meaning status stays 'running' rather than 'interrupted'. The test passes by coincidence (same error message). Add an assertion like expect(store.load(...).status).toBe('running') to document this limitation, or fix the runner to guard the finally-block save. [fixable]

[dimakis]

🟡 bugs: Unprotected save() in finally block can throw (e.g. database full/locked), which replaces the original error and leaves the database status as 'running' instead of 'interrupted'. The caller sees a misleading error and the conversation is stuck until recoverAtStartup(). Wrap in try-catch: try { save(); } catch { /* log but don't mask */ }. [fixable]

[dimakis]

🟡 unsafe_assumptions: entry.path = await realpath(entry.path) mutates ManagedSession.worktreePaths entries in-place on every tool call. This is shared state owned by SessionRegistry. While realpath is idempotent on resolved paths, if a worktree path is removed between calls, realpath throws and the tool returns a confusing 'Session workspace is unavailable' error instead of a worktree-specific message. Resolve into a local copy instead of mutating the session. [fixable]

[dimakis]

🔵 style: JSON.stringify comparison for deep equality is fragile — objects with identical keys in different insertion order produce different strings. The current callers produce same-order objects, so this works today, but deepStrictEqual or a shallow key-by-key check would be more robust. [fixable]

[dimakis]

🔵 missing_tests: The 'stops before side effects if checkpoint persistence fails' test doesn't verify database state after the error. Because state.checkpoint remains truthy, the save() call in the finally block also throws 'disk full', meaning status stays 'running' rather than 'interrupted'. The test passes by coincidence (same error message). Add an assertion like expect(store.load(...).status).toBe('running') to document this limitation, or fix the runner to guard the finally-block save. [fixable]

[dimakis]

Centaur Review

Found 5 issue(s) (3 warning).

packages/harness/src/providers/sdk-adapter.ts

Well-engineered crash-safety foundation with correct recovery semantics. The sdk-adapter.ts reordering is verified safe. Main gaps are the per-save SELECT overhead in the store and missing multi-tool-turn test coverage for the incremental persistence path.

• 🟡 regressions (L276): The onHistory callback receives structuredClone([...messages, { role: 'user', content: toolResults }]) after each individual tool result, deep-cloning the entire conversation history each time. For long conversations with multi-tool turns, this creates O(tools × history_length) cloning work per turn. The per-tool-result granularity is correct for crash safety (persist before the next side effect), but the full-history clone cost should be weighed against a differential approach if conversations grow large.

server/native-responses-store.ts

Well-engineered crash-safety foundation with correct recovery semantics. The sdk-adapter.ts reordering is verified safe. Main gaps are the per-save SELECT overhead in the store and missing multi-tool-turn test coverage for the incremental persistence path.

• 🟡 unsafe_assumptions (L55): save() calls this.load(conversationId, binding) purely for binding validation on every invocation, performing an extra SELECT query before every INSERT/UPDATE. During rapid tool execution, onHistory calls save() after each tool result — doubling DB queries unnecessarily. The binding is immutable after begin(), so the key could be cached in the instance (e.g., a Map<string, string> of conversationId → bindingKey set during begin()) instead of re-querying. [fixable]
• 🟡 bugs (L57): The checkpoint serialization in save() explicitly lists four fields: accountId, model, history, input. If ResponsesCheckpoint gains a new field needed for continuation (e.g., provider configuration or conversation metadata), it would be silently dropped. The explicit whitelist is correct for excluding credentials, but a safer pattern would be to destructure away the excluded fields (const { apiKey: _, ...safe } = state.checkpoint) rather than listing included ones, so new safe fields are preserved automatically. [fixable]

server/__tests__/native-responses-runner.test.ts

Well-engineered crash-safety foundation with correct recovery semantics. The sdk-adapter.ts reordering is verified safe. Main gaps are the per-save SELECT overhead in the store and missing multi-tool-turn test coverage for the incremental persistence path.

• 🔵 missing_tests: No test exercises a model response containing multiple tool_use blocks in a single turn. The response(tool) helper always returns a single function_call. This is the most novel behavior introduced by the onHistory change in sdk-adapter.ts — incremental persistence of partial tool results within a turn — and deserves a dedicated test verifying that a crash mid-batch (after tool A's result is persisted but before tool B executes) recovers correctly with A's result present and B marked uncertain. [fixable]

server/native-tool-executor.ts

Well-engineered crash-safety foundation with correct recovery semantics. The sdk-adapter.ts reordering is verified safe. Main gaps are the per-save SELECT overhead in the store and missing multi-tool-turn test coverage for the incremental persistence path.

• 🔵 style (L86): The shell() function collects stdout and stderr into the same chunks array. When both streams produce output, the consumer receives interleaved content with no way to distinguish error diagnostics from actual command output. Consider collecting stderr separately and appending it as a labeled section (e.g., \n--- stderr ---\n...) in the resolved output, matching how the Claude Code SDK formats shell results. [fixable]

[dimakis]

🟡 regressions: The onHistory callback receives structuredClone([...messages, { role: 'user', content: toolResults }]) after each individual tool result, deep-cloning the entire conversation history each time. For long conversations with multi-tool turns, this creates O(tools × history_length) cloning work per turn. The per-tool-result granularity is correct for crash safety (persist before the next side effect), but the full-history clone cost should be weighed against a differential approach if conversations grow large.

[dimakis]

🟡 unsafe_assumptions: save() calls this.load(conversationId, binding) purely for binding validation on every invocation, performing an extra SELECT query before every INSERT/UPDATE. During rapid tool execution, onHistory calls save() after each tool result — doubling DB queries unnecessarily. The binding is immutable after begin(), so the key could be cached in the instance (e.g., a Map<string, string> of conversationId → bindingKey set during begin()) instead of re-querying. [fixable]

[dimakis]

🟡 bugs: The checkpoint serialization in save() explicitly lists four fields: accountId, model, history, input. If ResponsesCheckpoint gains a new field needed for continuation (e.g., provider configuration or conversation metadata), it would be silently dropped. The explicit whitelist is correct for excluding credentials, but a safer pattern would be to destructure away the excluded fields (const { apiKey: _, ...safe } = state.checkpoint) rather than listing included ones, so new safe fields are preserved automatically. [fixable]

[dimakis]

🔵 style: The shell() function collects stdout and stderr into the same chunks array. When both streams produce output, the consumer receives interleaved content with no way to distinguish error diagnostics from actual command output. Consider collecting stderr separately and appending it as a labeled section (e.g., \n--- stderr ---\n...) in the resolved output, matching how the Claude Code SDK formats shell results. [fixable]

[dimakis]

Centaur Review

Found 5 issue(s) (1 warning).

packages/harness/__tests__/sdk-adapter.test.ts

Solid crash-recovery design with careful durable boundaries; the toolResults.push reorder in the shared runAgenticLoop is safe but the new onHistory callback lacks dedicated unit tests in the adapter test suite.

• 🟡 missing_tests: runAgenticLoop gained an onHistory callback and a reordering of toolResults.push(result) from after yield to before it. These are behavioral changes to a shared function used by both query-loop.ts and the new native runner. The existing sdk-adapter tests don't exercise onHistory at all — call count, argument shape (cloned snapshots with incremental tool results), error propagation, or the push-before-yield ordering. The callback is only tested indirectly through the runner integration tests. A dedicated unit test would catch regressions if the callback contract changes. [fixable]

server/native-responses-runner.ts

Solid crash-recovery design with careful durable boundaries; the toolResults.push reorder in the shared runAgenticLoop is safe but the new onHistory callback lacks dedicated unit tests in the adapter test suite.

• 🔵 unsafe_assumptions (L69): this.active concurrency guard is unreliable because run() is an async generator whose body doesn't execute until the first .next() call. If a caller stores the generator without iterating it, then calls run() again, both pass the this.active check. The store.begin() SQLite transaction is the authoritative guard, and the contract says callers must exhaust the iterator, but the instance-level check gives a false sense of safety. Consider documenting this explicitly or eagerly starting the generator body via an intermediate wrapper. [fixable]
• 🔵 style (L13): export { NativeResponsesStore } from './native-responses-store.js' re-exports the store class from the runner module. This creates two import paths for the same symbol (./native-responses-store.js directly, or ./native-responses-runner.js). The test file already imports both from the runner. Consider whether this convenience re-export is worth the indirection — callers needing only the store shouldn't have to import the runner. [fixable]

server/__tests__/native-tool-executor.test.ts

Solid crash-recovery design with careful durable boundaries; the toolResults.push reorder in the shared runAgenticLoop is safe but the new onHistory callback lacks dedicated unit tests in the adapter test suite.

• 🔵 missing_tests: Several executor branches lack test coverage: (1) Edit with zero matches or multiple matches (the indexOf !== lastIndexOf guard at native-tool-executor.ts:180-182), (2) Read on a file exceeding maxOutputBytes (the 64KB check at line 191), (3) shell timeout (the 60s default). These are defensive checks worth exercising to prevent silent regressions. [fixable]

server/native-tool-executor.ts

Solid crash-recovery design with careful durable boundaries; the toolResults.push reorder in the shared runAgenticLoop is safe but the new onHistory callback lacks dedicated unit tests in the adapter test suite.

• 🔵 unsafe_assumptions (L176): For Edit and Read, the entire file is loaded into memory via readFile before any size check. The maxOutputBytes guard only applies to Read's return value (line 191), not to the read itself. A multi-GB file used as an Edit target would consume that much memory. The SDK tools likely have the same behavior, so this is consistent, but worth noting for a tool that accepts untrusted model input. [fixable]

[dimakis]

🔵 unsafe_assumptions: this.active concurrency guard is unreliable because run() is an async generator whose body doesn't execute until the first .next() call. If a caller stores the generator without iterating it, then calls run() again, both pass the this.active check. The store.begin() SQLite transaction is the authoritative guard, and the contract says callers must exhaust the iterator, but the instance-level check gives a false sense of safety. Consider documenting this explicitly or eagerly starting the generator body via an intermediate wrapper. [fixable]

[dimakis]

🔵 style: export { NativeResponsesStore } from './native-responses-store.js' re-exports the store class from the runner module. This creates two import paths for the same symbol (./native-responses-store.js directly, or ./native-responses-runner.js). The test file already imports both from the runner. Consider whether this convenience re-export is worth the indirection — callers needing only the store shouldn't have to import the runner. [fixable]

[dimakis]

🔵 unsafe_assumptions: For Edit and Read, the entire file is loaded into memory via readFile before any size check. The maxOutputBytes guard only applies to Read's return value (line 191), not to the read itself. A multi-GB file used as an Edit target would consume that much memory. The SDK tools likely have the same behavior, so this is consistent, but worth noting for a tool that accepts untrusted model input. [fixable]

[dimakis]

Centaur Review

Found 7 issue(s) (4 warning).

server/native-tool-executor.ts

Well-structured durable execution foundation with careful crash recovery semantics. The onHistory integration into runAgenticLoop is clean and the store's binding enforcement is solid. Main concerns are minor: a TOCTOU in permission-file creation, fragile coupling between onHistory timing and loop-limit detection, and a missing boundary-condition test for readBounded.

• 🟡 unsafe_assumptions (L169): When worktreePaths is empty (no worktrees configured), canonicalPath resolves the file_path but no root match is found, so the path bypasses the alias rewriting. The downstream canUseTool still enforces policy, but the canonicalized path may not match the permission handler's expectations for sessions without worktree entries. If all native execution sessions are guaranteed to have worktrees, this is fine, but the code doesn't enforce or document that invariant. [fixable]

server/native-responses-runner.ts

Well-structured durable execution foundation with careful crash recovery semantics. The onHistory integration into runAgenticLoop is clean and the store's binding enforcement is solid. Main concerns are minor: a TOCTOU in permission-file creation, fragile coupling between onHistory timing and loop-limit detection, and a missing boundary-condition test for readBounded.

• 🟡 bugs (L112): The loop-limit detection state.history.at(-1)?.role !== 'assistant' after a result event relies on onHistory having been called to update state.history during the final turn. If runAgenticLoop yields result after a turn where onHistory was NOT called (e.g., if the loop breaks on signal.aborted before appending the assistant message), state.history may be stale and the check could misclassify the outcome. Currently this can't happen because the abort-signal check in the loop happens after tool execution, not mid-turn — but the coupling between onHistory timing and this check is fragile.
• 🔵 style (L25): recoverToolResults mutates state.history in place (pushes a new user message). This is intentional but undocumented in the function's JSDoc — the caller depends on the mutation side effect. A brief @mutates state.history note would clarify the contract. [fixable]
• 🔵 style (L103): The executeTool wrapper in the runner catches all errors and returns an error result, including for non-abort failures. This means runAgenticLoop's own catch-and-wrap-error path for executeTool rejections (lines 261-268 of sdk-adapter.ts) is unreachable when called from this runner. This is intentional double-defense but worth a brief comment to prevent future confusion about which error path fires. [fixable]

server/native-responses-store.ts

Well-structured durable execution foundation with careful crash recovery semantics. The onHistory integration into runAgenticLoop is clean and the store's binding enforcement is solid. Main concerns are minor: a TOCTOU in permission-file creation, fragile coupling between onHistory timing and loop-limit detection, and a missing boundary-condition test for readBounded.

• 🟡 unsafe_assumptions (L21): The closeSync(openSync(path, 'a', 0o600)); chmodSync(path, 0o600) two-step has a TOCTOU window: another process could open the file between openSync creating it and chmodSync restricting permissions. On a single-server deployment this is negligible, but openSync with O_CREAT|O_EXCL (flag 'ax') wrapped in a try/catch for EEXIST would be tighter for the sensitive-credential concern the doc calls out. [fixable]

packages/harness/src/providers/sdk-adapter.ts

Well-structured durable execution foundation with careful crash recovery semantics. The onHistory integration into runAgenticLoop is clean and the store's binding enforcement is solid. Main concerns are minor: a TOCTOU in permission-file creation, fragile coupling between onHistory timing and loop-limit detection, and a missing boundary-condition test for readBounded.

• 🟡 regressions (L275): Moving toolResults.push(result) before yield changes observable behavior for existing callers consuming the generator: the user event is now yielded AFTER the result is accumulated into toolResults, whereas previously it was yielded BEFORE. For current consumers (query-loop) this is harmless since the user event carries only the individual result, but any consumer that held a reference to toolResults (unlikely but possible via closure) would see different intermediate state. The change is well-motivated for durability but should be called out as a behavioral change.

server/__tests__/native-tool-executor.test.ts

Well-structured durable execution foundation with careful crash recovery semantics. The onHistory integration into runAgenticLoop is clean and the store's binding enforcement is solid. Main concerns are minor: a TOCTOU in permission-file creation, fragile coupling between onHistory timing and loop-limit detection, and a missing boundary-condition test for readBounded.

• 🔵 missing_tests: No test covers the readBounded behavior when reading a file that is exactly at the size limit (limit bytes, not limit+1). The current tests use a small file with maxOutputBytes: 8 and a 33-byte file, but don't test the boundary condition where the file is exactly limit bytes (should succeed) versus limit+1 bytes (should fail). [fixable]

[dimakis]

🟡 unsafe_assumptions: When worktreePaths is empty (no worktrees configured), canonicalPath resolves the file_path but no root match is found, so the path bypasses the alias rewriting. The downstream canUseTool still enforces policy, but the canonicalized path may not match the permission handler's expectations for sessions without worktree entries. If all native execution sessions are guaranteed to have worktrees, this is fine, but the code doesn't enforce or document that invariant. [fixable]

[dimakis]

🟡 bugs: The loop-limit detection state.history.at(-1)?.role !== 'assistant' after a result event relies on onHistory having been called to update state.history during the final turn. If runAgenticLoop yields result after a turn where onHistory was NOT called (e.g., if the loop breaks on signal.aborted before appending the assistant message), state.history may be stale and the check could misclassify the outcome. Currently this can't happen because the abort-signal check in the loop happens after tool execution, not mid-turn — but the coupling between onHistory timing and this check is fragile.

[dimakis]

🔵 style: recoverToolResults mutates state.history in place (pushes a new user message). This is intentional but undocumented in the function's JSDoc — the caller depends on the mutation side effect. A brief @mutates state.history note would clarify the contract. [fixable]

[dimakis]

🔵 style: The executeTool wrapper in the runner catches all errors and returns an error result, including for non-abort failures. This means runAgenticLoop's own catch-and-wrap-error path for executeTool rejections (lines 261-268 of sdk-adapter.ts) is unreachable when called from this runner. This is intentional double-defense but worth a brief comment to prevent future confusion about which error path fires. [fixable]

[dimakis]

🟡 unsafe_assumptions: The closeSync(openSync(path, 'a', 0o600)); chmodSync(path, 0o600) two-step has a TOCTOU window: another process could open the file between openSync creating it and chmodSync restricting permissions. On a single-server deployment this is negligible, but openSync with O_CREAT|O_EXCL (flag 'ax') wrapped in a try/catch for EEXIST would be tighter for the sensitive-credential concern the doc calls out. [fixable]

[dimakis]

🟡 regressions: Moving toolResults.push(result) before yield changes observable behavior for existing callers consuming the generator: the user event is now yielded AFTER the result is accumulated into toolResults, whereas previously it was yielded BEFORE. For current consumers (query-loop) this is harmless since the user event carries only the individual result, but any consumer that held a reference to toolResults (unlikely but possible via closure) would see different intermediate state. The change is well-motivated for durability but should be called out as a behavioral change.