Skip to content

feat(admission): enforce record exclusions across indexing and recall - #180

Open
HengYangDS wants to merge 9 commits into
obra:mainfrom
HengYangDS:feat/record-admission
Open

HengYangDS wants to merge 9 commits into
obra:mainfrom
HengYangDS:feat/record-admission

Conversation

@HengYangDS

@HengYangDS HengYangDS commented Sep 26, 2026 •

Copy link
Copy Markdown

Purpose and boundary

Opt-in native record admission should apply consistently before indexing, archiving, summarization and recall, including previously retained records. A rejected record must not leak through another path or silently switch a Codex summary to Claude. The default path has no scanner requirement.

Change and review focus

  • Define record/tool-call exclusions once at native admission and reuse them across ingress and recall. Safe neighboring records remain usable; historical source transcripts are not rewritten.
  • Allow an operator-supplied, absolute, SHA-256-pinned Gitleaks executable with bounded per-record execution, validated reports and no inherited scanner configuration or secret-bearing diagnostics.
  • Distinguish rejected/unavailable screening from corruption and missing summaries. Repair refuses to start while screening remains unresolved.
  • Route sessionless Codex summaries through an isolated ephemeral Codex thread; preserve session-ID forks and never borrow another harness identity or fall back to Claude. Bound that owned child's termination with a 1024 ms TERM-to-KILL grace and retire its directory only after close.
  • Keep complete ordered search admission beyond the vec0 KNN candidate ceiling. The corrected retrieval_distance alias prevents SQLite from selecting the vec0 distance column.

The complete search scan favors correct admission; large-index performance is not benchmarked. Codex routing tests use a synthetic app server, not real credentials. This does not backfill summaries or enable background sync. Source/tests remain the semantic core. The native bundle closure below removes unrelated generated dependency-layout churn while preserving the complete reachable admission/search/read implementation.

Predecessor verification

Predecessor head: b9f880a722a44379c4a006851b5ffc3147931c40; verified target: 7e06519357777badd7a115d2014a7ef845904310.

Fresh qualification on 2026-10-10, macOS arm64, Node 22.23.3/npm 10.9.9:

  • Independent empty-destination npm ci succeeds against this PR's unchanged manifest. fix(verify): distinguish appended exchanges from archive refreshes #177/feat(admission): enforce record exclusions across indexing and recall #180 manifests are byte-identical, so the same audited local resolver lock is reused, but each dependency installation is independent.
  • npm run build, original bundled-script syntax, 152/152 relevant tests, and the full 448/448 across 67 files, zero skips, pass. The final full run preserves every tracked input, including the original committed bundle.
  • Real Gitleaks 8.30.1 integration executes alongside the bounded synthetic scanner contracts. The initial 18 failures came from caller-owned TMPDIR inside this ESM package: CommonJS fixture executables inherited the wrong package scope. Native temporary storage outside the package fixes that setup without product/test changes, relaxed screening or longer timeouts. Failed receipts are preserved.
  • Resolved inputs include Transformers 4.3.1, better-sqlite3 12.11.1, TypeScript 5.9.3 and Vitest 3.2.7. Local resolver lock SHA-256: f4ac20b63ee3980db1711ee636f8b79b56fdd6664e5c6cd50809d85722af9398. Model data is prepared before the final full run; a cold-model hook guarantee is not claimed.

The declared Vitest 3 range has known dev-tool advisories (GHSA-82fw-gwwq-j7x9, GHSA-5gmw-xhrv-c9v3, GHSA-85c8-ppgw-ccpr). The operator approved only isolated trusted one-shot tests with ephemeral HOME, no credentials and no test server. No source dependency, local policy, live installation or lockfile change is included.

The historical borrowed-tree Node matrix is not exact-input qualification. The predecessor's Node 24, hosted Linux/Windows, coverage and real provider acceptance were not verified. See the exact revised-head Node 24 qualification below. Predecessor-head CI was action_required awaiting maintainer approval; it does not qualify the revised head. At that predecessor, fresh build output differed from the committed generated bundle; both source build and original-byte tests were recorded separately. The native bundle closure below supersedes that artifact divergence without changing its semantic source. Maintainer merge, release and deployment are not claimed.

Author follow-through: bounded isolated shutdown

Shutdown-qualified predecessor: a8eadb5e35a0524201a75c709ee25c1a3282a029; unchanged target: 7e06519357777badd7a115d2014a7ef845904310. This is a fast-forward follow-up to the independently reviewed submitted head b9f880a722a44379c4a006851b5ffc3147931c40, not a claim that its earlier receipt covered this change.

A real synthetic app-server that ignores SIGTERM reproduced an indefinite isolated summary wait on the submitted parent for success, error and timeout (three REDs). The existing executor now escalates only its still-running isolated child after a 1024 ms grace, clears the grace on close, preserves close-bound temporary-directory retirement and source-suppressed errors, and leaves ordinary session-ID forks/provider selection unchanged. No installed plugin, native session, credentials, background sync or generation budget was changed.

Fresh local qualification on macOS arm64, App-bundled Node 24.19.0/native npm 12.2.0:

  • Independent native npm ci reuses the prior exact resolver lock without changing the PR manifest or dependency declarations. The package's original allowScripts remains authoritative; ignored unrelated lifecycle scripts and existing dev-tool advisories are retained, not bypassed.
  • npm run build passes. The native bundle input graph does not reach src/summarizer.ts; all reachable tracked sources are unchanged. For that shutdown-only follow-up, the then-committed MCP bundle was retained byte-exact; native dist/summarizer.js carried the repair. The separate source-derived bundle closure below removes the earlier dependency churn.
  • 41/41 focused tests, 8/8 native generated-dist routing tests, and final 451/451 across all 67 files, zero skips, pass. The final full suite uses the original committed MCP bundle and unchanged final source/dist/test bytes.
  • An independent genericity/lifecycle/privacy review approves the final source and regression. The regression protects cleanup even on its RED path and does not signal a PID after an already-terminal outcome.

The three TERM-resistant regressions are POSIX-specific and explicitly skip on Windows; no Windows signal behavior or hosted platform result is claimed. Actual external provider acceptance, coverage, maintainer workflow approval, merge, stable release inclusion and installed-consumer adoption remain separate unverified gates. No additional approval ping accompanies this factual revision; hosted checks must actually execute. Shared verifier/admission merge conflicts, including #109, and overlapping Codex routing with #178 require refresh and affected native tests if upstream inputs change.

Author follow-through: native minimal bundle closure

The final artifact-only revision follows the shutdown-qualified predecessor above. Only dist/mcp-server.js changes; every tracked source, test, manifest, mode and other compiled artifact remains byte-equal to that predecessor. The approved record-admission, source-retention, Codex routing/defaults and 1024 ms isolated shutdown behavior are unchanged. Latest stable upstream release is v1.6.0; this contribution is not yet merged or released.

Original npm run build recreates the complete reachable MCP source graph. Build inputs use SDK 1.30.0, Zod 4.5.4, Marked 16.4.0 and fast-uri 3.1.7 within existing declared constraints; the manifest and original Transformers 4.3.1/native-runtime qualification remain unchanged. No third-party source or generated vendor section is hand-edited. This removes 5,985 lines and adds 369 relative to the submitted MCP bundle, eliminating unrelated dependency drift rather than expanding product scope.

All 178 original dependency sections are present. 166 are byte-identical to the upstream base; twelve retain only original-native esbuild identifier suffix changes induced by new owned admission imports/declarations. The original TypeScript JS parser verifies matching token structure and complete section-byte equality after only the exact identifier-position mappings; strings, comments, properties, templates, constants and logic are unchanged. The native graph's independently emitted bundle is byte-equal to the final artifact. src/summarizer.ts remains outside this graph and its accepted generated module is unchanged.

Fresh isolated macOS-arm64 Node 24.19.0/native npm 12.2.0 acceptance:

  • Independent original npm ci, unchanged allowScripts, native build/typecheck and exact preseeded public-model inference pass. The model probe returns finite 384-dimensional embeddings; this is not a cold-download claim.
  • 98/98 focused tests include actual packaged STDIO MCP admission/search and the retained TERM-resistant shutdown regressions. Final native npm test passes 451/451 across 67 files, zero skips, against the exact final source-derived bundle.
  • Original dependency/fixture advisories and failed verification receipts remain retained. No host package, installed plugin, service, session, credential, model selection or background sync is changed.

Exact final head and hosted checks must be read after publication; prior-head CI is not current-head acceptance. Current Node22/26, Ubuntu/Windows, coverage, large-index performance, actual external provider acceptance, maintainer merge and stable/installed adoption remain separately unqualified. Existing workflow approval requests are retained without duplicate pings. Continue actionable feedback and source-conflict refreshes through merge and the relevant stable-release adoption.

@HengYangDS

Copy link
Copy Markdown
Author

The upstream CI run for this fork PR is currently action_required, so the declared Node 22/24 cold-install matrix has not executed. Could a maintainer approve the workflow run when convenient? The related independent PRs #176–#179 are in the same state. I will treat their CI as unverified until checks actually run, and address any failures or rebases that follow.

@HengYangDS

Copy link
Copy Markdown
Author

Current author follow-up on #176-#180 and #184: the PRs remain open/mergeable, but their exact-head fork CI is still action_required, not green. Please approve the pending workflows when possible so actual failures can be addressed. I am also reviewing source-versus-generated scope and declared-toolchain compatibility to keep each contribution generic and independently mergeable; local test counts do not replace hosted acceptance. This is not a request to bypass branch protection.

@HengYangDS

Copy link
Copy Markdown
Author

Follow-up on the new revision a8eadb5:

The source/product-boundary review found one concrete isolated-summary shutdown gap: after a successful result, protocol error or timeout, a Codex child that ignored SIGTERM could leave the parent waiting indefinitely for close. The minimal repair adds an isolated-only 1024 ms TERM-to-KILL grace, cancels that timer on close, and keeps temporary-directory cleanup close-bound. It does not change the normal summary route, session-ID forks, provider/model selection, or record-admission defaults.

Three actual TERM-resistant child regressions failed on the submitted predecessor and pass on this revision. Native Node 24.19.0 acceptance on macOS arm64 includes build/typecheck, 41 focused tests, eight generated-dist routing tests, and 451/451 final tests across 67 files with zero skips. The unrelated MCP bundle is preserved byte-for-byte. This is local evidence, not a hosted Node 22/24, Windows, real-provider, or descendant-held-stdio claim.

Exact revised-head Actions run 38096647228 is still action_required and has not executed. The existing approval request remains sufficient; this comment supplies the new defect, fix and verification facts for review rather than another unchanged approval ping. I will address actionable review/CI findings and maintain the contribution through merge and relevant stable-release adoption.

A retained search row whose transcript has since been removed made the
whole request fail with "Conversation changed during record admission".
Codex replaces archived rollouts with compressed .jsonl.zst files, so a
transcript indexed before compression disappears and every query that
matched one of its rows failed.

A removed transcript has nothing left to verify or expose, so search now
withholds its rows and continues. Symlink substitution, indexed-range
rewrites and other admission failures still fail closed.

Generated with Claude Code
@HengYangDS

Copy link
Copy Markdown
Author

Follow-up on a24fde1:

Search admission failed the whole request with Conversation changed during record admission whenever a matched index row named a transcript that no longer exists. Codex now replaces archived rollouts with compressed .jsonl.zst files, so a transcript indexed before compression disappears and every query that matched one of its rows failed.

A removed transcript has nothing left to verify or expose, so search now withholds those rows and continues. Symlink substitution, indexed-range rewrites and other admission failures still fail closed.

Three regression cases cover the text, vector and both modes; they fail before the change and pass after it. The full local suite passes: 67 files, 454 tests. As in the earlier fix commits, only dist/record-admission.js is regenerated, and the MCP bundle is left unchanged.

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