Repository navigation
feat(admission): enforce record exclusions across indexing and recall - #180
HengYangDS wants to merge 9 commits into
Conversation
|
The upstream CI run for this fork PR is currently |
|
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. |
|
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
|
Follow-up on a24fde1: Search admission failed the whole request with 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 |
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
retrieval_distancealias 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:
npm cisucceeds 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.TMPDIRinside 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.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_requiredawaiting 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 headb9f880a722a44379c4a006851b5ffc3147931c40, not a claim that its earlier receipt covered this change.A real synthetic app-server that ignores
SIGTERMreproduced 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:
npm cireuses the prior exact resolver lock without changing the PR manifest or dependency declarations. The package's originalallowScriptsremains authoritative; ignored unrelated lifecycle scripts and existing dev-tool advisories are retained, not bypassed.npm run buildpasses. The native bundle input graph does not reachsrc/summarizer.ts; all reachable tracked sources are unchanged. For that shutdown-only follow-up, the then-committed MCP bundle was retained byte-exact; nativedist/summarizer.jscarried the repair. The separate source-derived bundle closure below removes the earlier dependency churn.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.jschanges; 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 isv1.6.0; this contribution is not yet merged or released.Original
npm run buildrecreates 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.tsremains outside this graph and its accepted generated module is unchanged.Fresh isolated macOS-arm64 Node 24.19.0/native npm 12.2.0 acceptance:
npm ci, unchangedallowScripts, 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.npm testpasses 451/451 across 67 files, zero skips, against the exact final source-derived bundle.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.