Skip to content

fix: address audit findings across pipeline, protocol, store, session - #211

Merged
saucam merged 2 commits into
mainfrom
feat/codeoid-audit-fixes
Jul 20, 2026
Merged

fix: address audit findings across pipeline, protocol, store, session#211
saucam merged 2 commits into
mainfrom
feat/codeoid-audit-fixes

Conversation

@saucam

@saucam saucam commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

A cross-cutting audit (5 adversarial passes over pipeline, session-manager/dispatch, store/DB, protocol+core, and cross-cutting smells). Every finding was verified against source before fixing; false positives and design-level/migration-risky items were deferred (listed at the bottom), so this PR is bug-fixes + safe hardening only.

Security / correctness

  • session.fork schema stripped isolate/workdir/baseBranch — the fields were in the type + handler but absent from the zod schema, so parseClientMessage dropped them over the wire. Net: isolation was always forced, and bind-mode / clean-base-fork were dead. (Tests passed typed objects directly, bypassing the strip.) Added the fields + a regression test.
  • usage.daily had no scope check — billing/cost aggregates were readable with a token holding no session scope. Gated on SESSION_LIST.
  • findByName ignored tenancy — a cross-tenant name probe + a self-detach side effect via Telegram /attach. Now tenant-scoped (auth required).
  • Pack loader: underDir was lexical-only → a symlink inside a pack could escape it (read /etc/* into a prompt). Now realpath-confined. Command gates had no timeout → a hung gate wedged the pipeline forever + leaked the child; now killed on a bounded timeout. Unbounded constitution/role reads → size-capped.

Runtime bugs

  • Gemini provider hung turn — the MCP tool-discovery await sat outside the turn's try, so a discovery failure was an unhandled rejection and left the event queue unclosed (consumer hangs forever). Guarded the fire-and-forget (OpenAI does this inline).
  • Dispatch double event-injection#deliverPendingEvents had no re-entrancy guard, so the periodic tick and the eager emit could send the same batch to the conductor twice (dup token spend + dup summary). Added a guard mirroring #ticking.
  • pipeline/store boot-resume killer — unguarded JSON.parse in listActive() (runs on every boot) meant one corrupt/version-drifted row threw out and sank all pipeline rehydration. Now skips+logs the bad row, like the sibling stores.
  • Pack retry: N off-by-one — mapped to N total attempts (so retry: 1 == abort). Now N retries (max = N+1).

Hardening / hygiene

  • pipeline.create: normalize workdir once (gate cwd + phase turn now agree; a ~/relative path no longer validates-then-breaks).
  • driveResumable: bound boot concurrency (was Promise.all over every interrupted pipeline → a restart storm).
  • Engine: structuredClone instead of a JSON round-trip on every step/gate.
  • Dispatch mark-delivered / touch: wrap the multi-row loop in one transaction (atomic; avoids partial-write duplicate notifications).
  • agent-identity: log (don't swallow) the 3 credential-revocation failures — a failed revoke leaves a token active with no signal.
  • session.ts: named the 5s subagent-identity fence constant + log on timeout/failure (was a magic number + swallowed error).
  • Protocol: enforce pipeline.create phases-XOR-pack and inline-import bundle at the schema layer (.refine() works in a Zod-4 discriminated union — corrected a wrong comment from feat(pipeline): load packs from config + create-from-pack over the wire #209); use LIMITS constants instead of duplicated literals; formatTokens no longer prints 1000k.

Tests

New: fork-field survival, pipeline.create XOR rejection, inline-import bundle required, pack symlink-escape; updated retry expectations. tsc (daemon + protocol + core) + biome clean. Full suite: 1656 pass, the only failures the pre-existing live-ZeroID conductor integration tests (need localhost:8899, unrelated to this change).

Deferred (verified real, but out of scope for a safe bug-fix PR — recommend follow-ups)

  • Scope/ownership guard duplication (~55 sites) — the root cause that let usage.daily ship without a check. A #requireScope / #requireOwnedSession choke-point refactor deserves its own focused, separately-reviewed PR (rewriting 55 authz sites alongside bug fixes is too risky). The two concrete gaps it caused are fixed here.
  • models.list scope gate — low value (catalog isn't secret) and gating risks model-picker UX for minimally-scoped tokens.
  • Exit-gate can't observe phase output — needs a GateCtx interface change (feature, not a bug).
  • abort can't interrupt an in-flight advance — needs a cancellation token threaded into the engine.
  • createSession INSERT OR REPLACE cascade/reset — latent (no code path reuses a session id today); switch to ON CONFLICT DO UPDATE when a reuse path is introduced.
  • dailyUsage date()-on-column full scan + FTS trigger re-index on embedding-only updates + vector-cache mutated in a rollback-able txn — memory-store perf/consistency; the FTS one needs a trigger migration. Batch into a memory-store PR with a real-DB migration check.
  • Resume cursor can lead dropped delta content (packages/core) — reachability uncertain (eviction/paging race); needs a focused repro before changing the cursor invariant.
  • mintConductorToken return discarded — intent unclear (vestigial vs. a dropped delegated credential); needs a maintainer decision, not a guess on auth behavior.
  • Provider binary-resolution / PiRpcProcess transport / atomic-write duplication; dead generatePKCE; Telegram ctx.chat! guards — refactors / needs-confirmation.

A cross-cutting audit turned up real bugs + hardening. Verified each
against source; fixed the high-confidence, safe ones (deferrals below).

Security / correctness:
- session.fork schema stripped isolate/workdir/baseBranch (validated
  fields absent from the schema) → over the wire, isolation was ALWAYS
  forced and bind-mode / clean-base-fork were dead. Added the fields.
- usage.daily had no scope check → billing/cost aggregates readable with
  a token holding no session scope. Gated on SESSION_LIST.
- findByName ignored tenancy → cross-tenant name probe + a self-detach
  side effect via Telegram /attach. Now tenant-scoped (auth required).
- pack loader: underDir was lexical-only → a symlink inside a pack could
  escape it (read /etc/*). Now realpath-confined. Command gates run with
  no timeout → a hung gate wedged the pipeline forever + leaked the
  child. Now killed on a bounded timeout. Unbounded constitution/role
  reads → size-capped (OOM guard).

Runtime bugs:
- Gemini provider: MCP tool-discovery await sat OUTSIDE the turn's try →
  a discovery failure was an unhandled rejection AND left the event
  queue unclosed (hung turn). Guarded the fire-and-forget (mirrors
  OpenAI).
- dispatch #deliverPendingEvents had no re-entrancy guard → the periodic
  tick and the eager emit could send the same batch to the conductor
  twice (dup token spend + dup summary). Added a guard (mirrors #ticking).
- pipeline/store JSON.parse was unguarded → one corrupt/version-drifted
  row threw out of listActive() (boot resume) and sank ALL pipelines.
  Now skips+logs the bad row (like the sibling stores).
- pack retry:N mapped to N total attempts (retry:1 == abort). Now N
  retries (max = N+1).

Hardening / hygiene:
- pipeline.create: normalize workdir once (gate cwd + turn agreed).
- driveResumable: bound boot concurrency (was Promise.all over all).
- engine: structuredClone instead of JSON round-trip per step.
- dispatch mark-delivered / touch: wrap the multi-row loop in one txn.
- agent-identity: log (don't swallow) the 3 credential-revocation
  failures (a failed revoke leaves a token active).
- session.ts: name the 5s subagent-identity fence constant + log on
  timeout/failure (was a magic number + swallowed error).
- protocol: enforce pipeline.create phases-XOR-pack and inline-import
  bundle at the schema layer (.refine works in a Zod-4 discriminated
  union — corrected a wrong comment); use LIMITS constants; formatTokens
  no longer prints "1000k".

Tests: fork-field survival, pipeline.create XOR rejection, import-bundle
required, pack symlink-escape, plus updated retry/expectations. tsc
(daemon+protocol+core) + biome clean; full suite green except the
pre-existing live-ZeroID integration tests (need a local server).
Comment thread src/daemon/pipeline/manager.ts
@github-actions

Copy link
Copy Markdown

🤖 Gemini code review

This PR implements safe hardening and fixes several critical/high bugs across the pipeline, protocol, session manager, and event-dispatching systems. It resolves issues with missing schema-validation fields (restoring isolation settings), a lack of scope/tenancy checks in usage queries and session searches, directory traversal vulnerabilities via symlink escapes in pack loading, and re-entrancy issues in event delivery.

Findings: 🔴 0 · 🟠 0 · 🟡 1 · 🟢 0


Tokens spent · ⬆️ Input: 11,358 · ⬇️ Output: 334 · Σ Total: 17,886
Total may be higher due to thinking token counts.

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.50000% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.96%. Comparing base (e0f2f9e) to head (9ea3d48).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/daemon/agent-identity.ts 0.00% 17 Missing ⚠️
src/daemon/providers/gemini/index.ts 22.22% 7 Missing ⚠️
src/daemon/session-manager.ts 72.72% 6 Missing ⚠️
src/daemon/pipeline/pack.ts 91.17% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #211      +/-   ##
==========================================
- Coverage   88.02%   87.96%   -0.06%     
==========================================
  Files         129      129              
  Lines       22045    22167     +122     
==========================================
+ Hits        19405    19500      +95     
- Misses       2640     2667      +27     
Flag Coverage Δ
daemon 87.96% <83.50%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/core/src/format.ts 87.27% <100.00%> (+0.73%) ⬆️
packages/protocol/src/schemas.ts 100.00% <100.00%> (ø)
src/daemon/dispatch.ts 97.14% <100.00%> (+0.06%) ⬆️
src/daemon/pipeline/engine.ts 100.00% <100.00%> (ø)
src/daemon/pipeline/manager.ts 100.00% <100.00%> (ø)
src/daemon/pipeline/store.ts 100.00% <100.00%> (ø)
src/daemon/session.ts 96.88% <100.00%> (+0.02%) ⬆️
src/daemon/store.ts 92.50% <100.00%> (+0.07%) ⬆️
src/frontends/telegram/index.ts 96.13% <100.00%> (ø)
src/daemon/pipeline/pack.ts 97.98% <91.17%> (-1.44%) ⬇️
... and 3 more
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

driveResumable batching used Promise.all, so one rejecting advance aborted
all remaining batches (regression flagged in review). Wrap each advance so a
failure is logged and skipped — the rest of the resume proceeds.
@saucam
saucam merged commit 2758bf8 into main Jul 20, 2026
4 checks passed
@saucam saucam mentioned this pull request Jul 20, 2026
saucam added a commit that referenced this pull request Jul 20, 2026
Bump package.json 0.3.1 → 0.3.2 and roll the 0.3.2 CHANGELOG section
(the release workflow gates on package.json matching the tag). 0.3.2 carries
the SDLC pipeline primitive (#204#209), the /settings MCP Servers surface
(#203), embedded-handoff ZeroID token consumption (#210), and the cross-cutting
audit fixes (#211).

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant