From 09fc75a8697e88b12830e8b0dc32775c3aecbe7a Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 03:25:40 +0300 Subject: [PATCH 01/20] test(api): capture Signature B's mechanism, and rule out every in-repo cause MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signature B is a whole test file failing with a bare `'test failed'`, no assertion, no stack, and none of that file's own tests reported. It is still not fixed, and this change does not claim to fix it. What it does is replace inference with observation. Node's test runner spawns one child process per test file (confirmed here by distinct PIDs), and that bare message is its own hardcoded fallback for a child that exits non-zero or signaled while no subtest recorded a failure — so the message says nothing about the cause by design. Synthetic fixtures pin the shape: only `process.exit(1)` before test registration reproduces it. A post-test async throw, an `http.Server` emitting `'error'` with its listener removed, a rejection inside a test body, a delayed SIGKILL, and a module-load throw each print a diagnostic line, a stack, or a partial `✔` that the real defect never shows. `signature-b-preload.cjs` then settles it on real occurrences rather than proxies. It hooks `process.exit`, `process.kill`, `uncaughtExceptionMonitor` (passive — unlike `uncaughtException` it cannot alter crash behaviour), `unhandledRejection`, `rejectionHandled`, `warning`, `beforeExit` and Node's unconditional `exit`, writing redacted structured JSONL to disk, off stdout/stderr so the reporter is untouched. A bounded 20-run instrumented pass reproduced the defect 3 times, on three files never previously implicated — seven distinct files now, sharing only `startHarness`. All three captures log `start` and `preload-installed` and NOTHING else: not even `exit`, which fires for every JS-visible shutdown path including `process.exit()`. The child dies before the JS runtime can react. That directly rules out every mechanism this repository's code can cause or catch, and leaves an uncatchable external termination. The trigger is NOT established — the machine had ~2.5GB of 15.7GB free with other agents' work running, but no crash appeared in the Windows event logs, so resource pressure is circumstantial, not proven. No fix is invented. Sleeps, whole-file retries, or lowering concurrency would hide a symptom whose cause is outside this code. The preload is committed so the next occurrence is captured rather than re-derived. An earlier version of the preload patched `EventEmitter.prototype.emit` globally; it crashed the parent runner via node:test's own TestsStream and was removed. The file records why, so it is not reintroduced. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CUPqvu66J5ZVyiv4nJ797r --- docs/ROADMAP.md | 2 +- ...0140-harness-ephemeral-port-acquisition.md | 109 ++++++++++++-- .../test/diagnostics/signature-b-preload.cjs | 138 ++++++++++++++++++ 3 files changed, 235 insertions(+), 14 deletions(-) create mode 100644 packages/api/test/diagnostics/signature-b-preload.cjs diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index b1e2c021..fa1acc2b 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -1349,7 +1349,7 @@ Debt observed during M14. Each states what is known, not what is planned; items - **The published capability document could omit a composed feature, and the client offered controls that could not work (RESOLVED in M15 Increment 24 / ADR-0132).** Left open by Increment 23 above. `capabilitiesView` (`packages/api/src/presenters.ts`) took a hand-written `Pick`, so a feature never added to it was invisible to `GET /v1/capabilities` and nothing complained. ADR-0131 judged that "a narrower failure than a 503 — the feature works for anyone who calls the route directly", and deferred it on the grounds that closing it meant first deciding which optional dependencies are user-facing capabilities. **Both halves of that were wrong.** The failure is narrower only when the client can still reach the feature, and a live instance already existed where it could not: `GET /v1/search` served three modes from two independently-gated dependency sets behind one published `search` flag, so a deployment running the Helm chart’s `search.semanticEnabled: false` advertised search while the client offered two mode buttons whose every request answered 503. And the judgement, while genuinely underivable, can be made **unskippable**, which was the property wanted. `semanticSearch` is now published from both of its dependencies, `Exclude[0] | NotAPublishedCapability>` must be `never` so a new optional dependency cannot compile until someone gives it a flag or records why it has none, and a behavioural guard requires every capability source to change what the document publishes — because a key can sit in that parameter unread, which the compile-time half cannot see. The mutation ledger lives in ADR-0132 and is not restated here. - **The search surface was not gated on `capabilities.search`, so an absolute kill switch still showed a search box (RESOLVED in M15 Increment 24 / ADR-0132 §5).** `SEARCH_ENABLED=0` — the chart's `search.enabled: false`, an absolute kill switch per ADR-0055 — leaves `searchRepository` unconstructed and `GET /v1/search` answering 503 on every mode, keyword included. The entry point was the persistent header form in `packages/web/index.html`, present on every page; being a `
` rather than an `a[data-route]`, `NAV_CAPABILITY_MAP` could not reach it, which is why the first pass at Increment 24 gated the semantic and hybrid *modes* and left keyword ungated — the same defect class one mode over. Raised by the Qodo review of PR #155. **Fixed in the same increment rather than deferred:** the form now ships `hidden` and is revealed by `applySearchCapability` only on an explicit `search: true`; the route renders an honest unavailable notice and issues no request; and keyword search waits for the capability answer, reversing this increment's own earlier latency decision, because knowing whether a request is pointless requires having asked. A markup-contract test pins the `hidden` attribute, since the gate depends on it and every other test passes without it. - **Clicking a search mode discarded text typed since the page loaded (RESOLVED in a follow-up to M15 Increment 24 / ADR-0132).** `createModeInput` in `packages/web/src/app/search-mount.ts` closed over the query captured when the route mounted, so `navigateToSearchMode` navigated with the old term and the remount reset the input to match it. Type a new term into the header field, click **Semantic** without pressing enter, and the typed text was gone with no indication it had been discarded. Pre-existing — the closure predated Increment 24 and was untouched by it — and found by the adversarial review of PR #155 while reviewing the capability gate wrapped around the same control. **Resolved:** the query is now a `() => string` read when a mode is chosen rather than a string captured when the selector renders, matching what `main.ts`'s submit handler already does, and falling back to the mounted query only where the document has no header input. Two regression tests, one of which fails against the exact pre-fix closure. -- **`startHarness` drew ports `fetch` refuses (RESOLVED in ADR-0140); the unexplained whole-file failure is still open.** Two signatures were filed here, deliberately not as one cause, and that judgement held. **Signature A is resolved.** WHATWG Fetch blocks eighty-two ports and undici enforces the list on the port number alone, before opening a socket — so `server.listen(0)` could bind, listen and answer raw TCP while `fetch` still refused, surfacing as `TypeError: fetch failed` / `Error: bad port` at the harness’s first request rather than at the listen that caused it. Whether it can happen at all is a property of the host’s dynamic port range: a typical Linux CI range (32768–60999) contains no blocked port, while the Windows range in use here (1024–15000) contains nineteen, which is the whole of "green on CI, flaky locally". The guard that already existed was incomplete in a way that still failed — its hand-observed set of eighteen ports was the spec list intersected with one machine’s range **minus `6679`** — and it retried unboundedly, had no behaviour on exhaustion, and had been copy-pasted into `auth-signin-schema.integration.test.ts`, so the missing port had to be found twice. `packages/api/test/listen.ts` now owns port acquisition for both sites: the spec-complete eighty-two ports (verified by sweeping all 65535 through the real `fetch` on Node v24.15.0), a bounded twenty attempts, each rejected listener closed before the next is asked for, a guarded `address()` read in place of the `as AddressInfo` cast, and an exhaustion error naming the attempts and rejected ports and nothing else. **Signature B is not resolved and was not folded in.** A file still fails with `'test failed'`, no assertion, no stack and none of its own tests reported. Twenty consecutive full runs gave five failures: on pre-fix code one signature A (`auth.test.js`) and three signature B; on post-fix code one signature B and no signature A. The port fix removes A and leaves B exactly where it was, which is the evidence that they are two defects. Four different files were hit (`move-explanation-route`, `tournament-commentary-route`, `bot-detection-analyze`, `anti-cheat-analysis`), sharing no import beyond `./helpers`; each died in 589–703 ms with no test of its own reporting and no stderr. Refuted with evidence: ephemeral-port exhaustion (113 sockets in TIME_WAIT against a 13977-port range), a `Promise.race` loser becoming an unhandled rejection (`race` subscribes to every promise, confirmed on v24.15.0), a throwing `after`/`afterEach` hook (the affected files use none), and a double `close()` rejecting (awaited in a `finally`, it would be attributed to that test with a stack). A second bounded pass — twelve more full runs under the TAP reporter (which prints the file-level YAML block rather than collapsing it to `'test failed'`) with a preload recording `uncaughtException`, `unhandledRejection` and any non-zero exit — produced twelve clean runs and captured nothing, so the mechanism remains unread. No fix was invented for it. See ADR-0140 §4. +- **`startHarness` drew ports `fetch` refuses (RESOLVED in ADR-0140); the unexplained whole-file failure is still open.** Two signatures were filed here, deliberately not as one cause, and that judgement held. **Signature A is resolved.** WHATWG Fetch blocks eighty-two ports and undici enforces the list on the port number alone, before opening a socket — so `server.listen(0)` could bind, listen and answer raw TCP while `fetch` still refused, surfacing as `TypeError: fetch failed` / `Error: bad port` at the harness’s first request rather than at the listen that caused it. Whether it can happen at all is a property of the host’s dynamic port range: a typical Linux CI range (32768–60999) contains no blocked port, while the Windows range in use here (1024–15000) contains nineteen, which is the whole of "green on CI, flaky locally". The guard that already existed was incomplete in a way that still failed — its hand-observed set of eighteen ports was the spec list intersected with one machine’s range **minus `6679`** — and it retried unboundedly, had no behaviour on exhaustion, and had been copy-pasted into `auth-signin-schema.integration.test.ts`, so the missing port had to be found twice. `packages/api/test/listen.ts` now owns port acquisition for both sites: the spec-complete eighty-two ports (verified by sweeping all 65535 through the real `fetch` on Node v24.15.0), a bounded twenty attempts, each rejected listener closed before the next is asked for, a guarded `address()` read in place of the `as AddressInfo` cast, and an exhaustion error naming the attempts and rejected ports and nothing else. **Signature B is not resolved and was not folded in.** A file still fails with `'test failed'`, no assertion, no stack and none of its own tests reported. Twenty consecutive full runs gave five failures: on pre-fix code one signature A (`auth.test.js`) and three signature B; on post-fix code one signature B and no signature A. The port fix removes A and leaves B exactly where it was, which is the evidence that they are two defects. Four different files were hit (`move-explanation-route`, `tournament-commentary-route`, `bot-detection-analyze`, `anti-cheat-analysis`), sharing no import beyond `./helpers`; each died in 589–703 ms with no test of its own reporting and no stderr. Refuted with evidence: ephemeral-port exhaustion (113 sockets in TIME_WAIT against a 13977-port range), a `Promise.race` loser becoming an unhandled rejection (`race` subscribes to every promise, confirmed on v24.15.0), a throwing `after`/`afterEach` hook (the affected files use none), and a double `close()` rejecting (awaited in a `finally`, it would be attributed to that test with a stack). A second bounded pass — twelve more full runs under the TAP reporter with a preload recording `uncaughtException`, `unhandledRejection` and any non-zero exit — produced twelve clean runs and captured nothing at the time. **A follow-up increment (`claude/node-test-signature-b`) then captured the defect directly, three more times, on three files never previously implicated** (`rate-limit-atomicity`, `dependency-parity`, `studies-api`) — seven distinct files in total now, confirming this is not tied to any one file’s logic. A new instrumented preload (`packages/api/test/diagnostics/signature-b-preload.cjs`) hooking every JS-visible process-level event — `process.exit`, `process.kill`, `uncaughtExceptionMonitor`, `unhandledRejection`, `rejectionHandled`, `warning`, `beforeExit`, and Node’s own unconditional `exit` — showed **none of them fire** on any of the three captures, including `exit` itself, which fires for every JS-visible shutdown path including `process.exit()`. A synthetic `process.exit(1)`-before-registration fixture reproduces the identical silent shape; every other synthetic mechanism tried (a post-test async throw, an emitter `'error'` with its listener removed, a synchronous module-load throw, a delayed `SIGKILL`) prints a visibly different diagnostic line, stack, or partial test output that the real defect never shows. This proves the per-file child process (`node --test` spawns one per file, confirmed by distinct PIDs) is being terminated by something outside the JS/V8 layer entirely — not any exception, rejection, or event this repository’s code can throw or catch. The machine had roughly 2.5 GB of 15.7 GB RAM free at capture time with several other agents’ processes concurrently running, which is circumstantially consistent with resource contention, but no crash was recorded in the Windows Application or System event logs in that window, so the exact external trigger is still not established. No fix was invented — the forbidden responses (sleeps, whole-file retries, lowering concurrency) would only hide a cause that remains outside this repository’s code. See ADR-0140 §4. - **`ApiServer.listen` registered no `'error'` handler, so a failed bind hung and raised an uncaught event (RESOLVED in ADR-0140 §5).** `packages/api/src/server.ts` resolved its promise from the `listening` callback only and built it with no reject path. A bind failing asynchronously (`EADDRINUSE`, `EMFILE`) left the promise pending forever and, with no `'error'` listener on the `http.Server`, was re-raised as an uncaught exception. Found while investigating ADR-0140 and independently raised by the Qodo review of PR #21. **Resolved in the same increment** rather than deferred, because ADR-0140 §2's bounded, diagnosable acquisition is not true without it — the retry can only report a bind error if the listener it is handed rejects. A one-shot `'error'` listener now rejects and is removed once listening, so later server errors keep their previous semantics rather than being swallowed by a `reject` on a settled promise. The regression test fails against the exact pre-fix code, through an uncaught `ERR_UNHANDLED_ERROR`. - **`main.ts`'s controller-disposal list is manual, untested, and silently incomplete when a section is added (RESOLVED in Increment 25 / ADR-0092).** `run()` in `packages/web/src/main.ts` disposes the previous route's controllers by name, and its own comment says doing so "is what makes re-bootstrapping safe" — but adding a section to `bootstrap` and forgetting to add it there compiles, passes every gate, and leaks. Increment 23 shipped exactly that omission for `LearningController` and it was caught in PR review, not by a test. `main.ts` has no test coverage of any kind, so no section's disposal is verified. A structural fix (bootstrap returning its disposables as a collection, or a type-level exhaustiveness check keyed off the result type) would make the next omission a compile error; it is a refactor across ~15 return sites and belongs in its own increment. **Resolved in Increment 25 (ADR-0092):** extracted `createLifecycle` run loop in `lifecycle.ts`, defined `BootstrappedDisposables` and `DisposableKey` driving `DISPOSABLE_TEARDOWN_MAP: Record` for compile-time exhaustiveness, normalised `.dispose()` verb across all disposables, and cascaded `GameController.stop()` to `gameSync.stop()`. - **`stepView` sends the answers to the learner (RESOLVED in Increment 29 / ADR-0095).** `packages/api/src/presenters.ts` emits `expectedSan` on a move step and `correctIndex` on a quiz step, and `GET /v1/lessons/:id/steps` is the route the learner's own lesson page calls. Increment 23 omits both from the client-side types (`packages/web/src/api/models.ts`), so the app cannot render or grade against them and a future edit that tries becomes a compile error — but the fields are still on the wire and readable in devtools. The authoring routes legitimately need them returned to the author, so the fix is a learner-scoped step view (or a caller-dependent projection), not a deletion: an API contract decision with its own ADR. Nothing rated or rewarded depends on step progress today, so this is a wart rather than a breach. **Resolved in Increment 29 (ADR-0095):** added `LearnerStepView` / `learnerStepView` in `packages/api/src/presenters.ts` omitting `expectedSan` and `correctIndex`. `GET /v1/lessons/:id/steps` and `GET /v1/steps/:id` now check course authorship via `repo.getLesson` / `repo.getCourse`, returning full `stepView` to the author and `learnerStepView` to learners and anonymous callers. Updated OpenAPI schema and web model comments. The first attempt resolved authorship with a separate `getLesson` + `getCourse` after the step read, which doubled both routes from 3 SQL queries to 6 because `listSteps` / `getStep` had already made those reads internally and discarded the course; caught in the PR #92 review and fixed by adding `getStepWithCourse` / `listStepsWithCourse` to `LearningRepository`, which return what was already loaded. Both routes now make exactly one repository call, pinned by a counting-proxy test. diff --git a/docs/adr/0140-harness-ephemeral-port-acquisition.md b/docs/adr/0140-harness-ephemeral-port-acquisition.md index d62ed1d2..f5c90001 100644 --- a/docs/adr/0140-harness-ephemeral-port-acquisition.md +++ b/docs/adr/0140-harness-ephemeral-port-acquisition.md @@ -170,19 +170,102 @@ Hypotheses examined and **refuted** with evidence: throw would be attributed to that test with a stack, which is not the observed signature. -The leading unrefuted hypothesis is a process-level fault in the test child -after module load and before the first test reports — a keep-alive socket to a -closed harness erroring inside undici's pool, or an uncaught `'error'` event on -an `http.Server`. **One instance of that second shape has now been fixed** (§5), -and it produces exactly this signature when it fires: with the fix reverted, the -regression test for it fails through an uncaught `ERR_UNHANDLED_ERROR` rather -than an assertion. That makes it a plausible contributor, but not a -demonstrated cause — signature B was reproduced on files doing no failing bind, -and it was never observed to coincide with one. It stays open. - -Nothing above is strong enough to justify a fix, and a fix that cannot be shown -to remove a reproduction is indistinguishable from a coincidence. Signature B -stays open. +One instance of an uncaught `'error'` event on an `http.Server` **has since been +fixed** (§5): with that fix reverted, the regression test for it fails through +an uncaught `ERR_UNHANDLED_ERROR`. That made it a plausible contributor, but a +follow-up investigation (below) has since ruled it out as the mechanism behind +the observed signature specifically, even though it remained a real defect +worth fixing on its own merits. + +### Follow-up investigation: the mechanism is now proven, the trigger is not + +A later increment (`claude/node-test-signature-b`) instrumented every +JS-visible process-level event a child process can raise and captured three +further real occurrences directly, rather than reasoning from symptoms or +synthetic proxies alone. + +**Node's test runner spawns one child process per test file.** Verified +directly: two trivial files run together under +`node --test --test-concurrency=1` report distinct PIDs, neither matching the +parent's. A same-named (currently experimental on this Node version) flag, +`--test-isolation=process`, confirms this is the default rather than an +accident of this run. It is why the failure is confined to exactly one file +per occurrence with no effect on any other file in the same run. + +**The bare `'test failed'` with no stack is what Node's runner reports for a +child that exits non-zero or signaled while none of its subtests recorded a +failure.** This was reproduced directly: a synthetic file that calls +`process.exit(1)` before registering any test produces the identical reported +shape as the real defect — `✖ (Nms)` / `'test failed'`, zero individual +tests in the summary, nothing on stderr. Every OTHER synthetic mechanism tried +produces a visibly different shape: + +- An error thrown after a test's own promise settles (`setImmediate` inside a + passing test) — Node prints an explicit + `ℹ Error: ... generated asynchronous activity after the test ended ...` + diagnostic line, and the triggering test still shows `✔`. +- An `http.Server` emitting `'error'` with its listener already removed — + same diagnostic line, same visible `✔` on the test that created it. +- A promise rejected synchronously inside a test's own body — attributed to + that specific test, with a full stack. +- A process killed by `SIGKILL` after a test completes — that test's `✔` + still prints before the file dies. +- A synchronous throw at module load, before any `test()` call — prints a + full stack trace to stderr. + +None of these match. The real defect's log is completely silent before the +bare file-level failure: no diagnostic line, no stderr, no test names at all +for that file (not even the ones that would have run first). + +**A new diagnostic preload +(`packages/api/test/diagnostics/signature-b-preload.cjs`) proves which of +those mechanisms it is, on real occurrences, not synthetic ones.** It hooks +`process.exit`, `process.kill`, `uncaughtExceptionMonitor` (a passive observer +that, unlike `uncaughtException`, never alters Node's default crash handling), +`unhandledRejection`, `rejectionHandled`, `warning`, `beforeExit`, and Node's +own unconditional `exit` event, writing a structured, redacted, timestamped +line to disk before each fires — bypassing stdout/stderr entirely so the test +reporter's own output is never touched. A bounded 20-run instrumented pass over +the full `packages/api` suite (chosen the same way as the original 20-run +sample: enough for >90% detection odds at the historically observed ~1-in-5 +rate) reproduced the defect 3 times, on **three files never previously +implicated** — `rate-limit-atomicity`, `dependency-parity`, `studies-api` — none +of the original four. Combined with the original four, that is **seven +different files**, sharing nothing but the shared harness's `startHarness` +import, which confirms this is not a defect specific to any one file's logic. + +All three captures show the identical signature: **only the `start` and +`preload-installed` lines are logged. Nothing else fires — including Node's own +`process.on('exit')`, which fires unconditionally for every JS-visible shutdown +path, `process.exit()` included.** That rules out, directly and by observation +rather than inference, every mechanism the preload watches for: `process.exit`, +an uncaught exception, an unhandled rejection, a handled-late rejection, a +runtime warning, and reaching an idle event loop. The child process disappeared +before the JS runtime got to react to anything. + +**This narrows Signature B's cause to a class, not a line of code:** the +per-file child process is being terminated by something outside the JS/V8 +layer entirely — an uncatchable signal or an external kill — not by any defect +this repository's TypeScript can throw, catch, or leak. Two observations are +consistent with an environmental (not code) origin: at the time of capture this +development machine had roughly 2.5 GB of 15.7 GB RAM free, with several +unrelated concurrent processes (other agents' worktrees and dev servers) +running; and `node --test` spawns dozens of child processes across a full +`packages/api` run, each loading the same large cross-package import graph, +which is exactly the pattern most exposed to transient resource contention. +Neither observation is proof of a specific external actor — no crash was +recorded in the Windows Application or System event logs in the capture +window — so the exact trigger (OS scheduler, memory pressure, antivirus, or +something else entirely) is not established. + +**No fix is proposed.** Every mechanism this repository's code could cause and +catch has been directly ruled out on real captures; what remains is external to +the process, and the forbidden responses — sleeps, retries around the whole +file, swallowing errors, lowering concurrency to hide it — would suppress the +symptom without touching whatever the cause turns out to be. Signature B stays +open. The diagnostic preload is committed so the next occurrence — on this +machine, in CI, or elsewhere — can be captured with this same evidence rather +than re-deriving it. ## 5. `ApiServer.listen` rejects on a failed bind diff --git a/packages/api/test/diagnostics/signature-b-preload.cjs b/packages/api/test/diagnostics/signature-b-preload.cjs new file mode 100644 index 00000000..b70ef030 --- /dev/null +++ b/packages/api/test/diagnostics/signature-b-preload.cjs @@ -0,0 +1,138 @@ +'use strict'; +/** + * Diagnostic preload for the "Signature B" node:test failure (docs/adr/0140 + * §4): a test file occasionally fails with a bare `'test failed'`, no + * assertion, no stack, and none of that file's own tests reported. Node's + * test runner spawns one child process per file; that exact message is its + * own hardcoded fallback (`ERR_TEST_FAILURE('test failed', kTestCodeFailure)` + * with `stack: undefined`) for when a child exits non-zero or signaled while + * no subtest recorded a failure — see `internal/test_runner/runner.js`. + * + * This module distinguishes the JS-catchable causes of that shape + * (`process.exit`, an uncaught exception, an unhandled rejection, an + * EventEmitter `'error'` event with no listener) from an external, + * uncatchable termination of the child (an OS-level kill or native crash), + * by hooking every relevant process-level event and writing what fired to a + * structured log. If NONE of these hooks fire before the child disappears — + * including Node's own unconditional `process.on('exit')`, which fires for + * every JS-visible shutdown path including `process.exit()` itself — the + * termination did not go through the JS layer at all. + * + * Usage (manual, not wired into `npm test`; run from packages/api): + * node --require ./test/diagnostics/signature-b-preload.cjs \ + * --test --test-concurrency=1 "dist-test/test/**\/*.test.js" + * + * Set SIGB_LOG_DIR to control where logs land (defaults under the OS temp + * dir). Logs are structured JSONL, one line per event, one file per child + * process. Nothing is written to stdout/stderr, so the test reporter's own + * output is never touched. + * + * Does not log request bodies, Authorization headers, cookie values, raw + * connection strings, or password hashes — see `redact`/`safeErr` below. + * + * Deliberately does NOT patch `EventEmitter.prototype.emit`. An earlier + * version did, to catch an unlistened `'error'` event before Node re-raises + * it; verified empirically to corrupt node:test's own internal `TestsStream` + * (itself an EventEmitter) and crash the PARENT `node --test` process on a + * trivial passing test with no Signature-B-related content at all — + * `--require` loads into every node process spawned with that flag, + * including the top-level runner, not just its per-file children. A global + * prototype patch is not a safe way to observe this system. An unhandled + * `'error'` event still surfaces below via `uncaughtExceptionMonitor`, + * Node's own re-raise of it, just without the emitter's constructor name. + */ + +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + +const LOG_DIR = process.env.SIGB_LOG_DIR || path.join(os.tmpdir(), 'sigb-diag'); +try { fs.mkdirSync(LOG_DIR, { recursive: true }); } catch { /* best-effort; missing dir just drops logging */ } +const LOG_FILE = path.join(LOG_DIR, `run-${process.pid}-${Date.now()}.jsonl`); +let fd; +try { fd = fs.openSync(LOG_FILE, 'a'); } catch { fd = null; } + +const REDACT_PATTERNS = [ + [/sk-[a-zA-Z0-9_-]{10,}/g, '[REDACTED_API_KEY]'], + [/Bearer\s+[A-Za-z0-9._~+/-]+=*/g, 'Bearer [REDACTED_TOKEN]'], + [/postgres:\/\/[^:]+:[^@]+@/g, 'postgres://[REDACTED_CREDS]@'], + [/(password|secret|token|authorization|cookie)\s*[:=]\s*["']?[^"',\s]+["']?/gi, '$1=[REDACTED]'], +]; + +function redact(value) { + if (typeof value !== 'string') return value; + let out = value; + for (const [pattern, replacement] of REDACT_PATTERNS) out = out.replace(pattern, replacement); + return out; +} + +function safeErr(err) { + if (err == null) return err; + if (typeof err !== 'object') return redact(String(err)); + return { + name: err.name, + message: redact(String(err.message ?? '')), + stack: redact(String(err.stack ?? '')), + code: err.code, + syscall: err.syscall, + errno: err.errno, + }; +} + +function activeResourceCounts() { + try { + if (typeof process.getActiveResourcesInfo !== 'function') return null; + const counts = {}; + for (const resource of process.getActiveResourcesInfo()) counts[resource] = (counts[resource] || 0) + 1; + return counts; + } catch { + return null; + } +} + +function write(kind, fields) { + if (!fd) return; + const line = JSON.stringify({ + kind, + pid: process.pid, + ppid: process.ppid, + testFile: process.env.NODE_TEST_CONTEXT === 'child-v8' ? (process.argv[1] || null) : null, + nodeTestContext: process.env.NODE_TEST_CONTEXT || null, + timeIso: new Date().toISOString(), + timeNs: process.hrtime.bigint().toString(), + ...fields, + }); + try { fs.writeSync(fd, line + '\n'); } catch { /* best-effort */ } +} + +write('start', {}); + +const originalExit = process.exit.bind(process); +process.exit = function patchedExit(code) { + write('process.exit', { code, callerStack: redact(new Error('exit-call-site').stack) }); + return originalExit(code); +}; + +const originalKill = process.kill.bind(process); +process.kill = function patchedKill(pid, signal) { + write('process.kill', { targetPid: pid, signal, callerStack: redact(new Error('kill-call-site').stack) }); + return originalKill(pid, signal); +}; + +// Passive observer only: unlike 'uncaughtException', registering this does +// NOT suppress Node's default crash behavior, so it cannot itself change +// whether or how the process exits. +process.on('uncaughtExceptionMonitor', (err, origin) => { + write('uncaughtExceptionMonitor', { err: safeErr(err), origin, activeResources: activeResourceCounts() }); +}); + +process.on('unhandledRejection', (reason) => { + write('unhandledRejection', { reason: safeErr(reason), activeResources: activeResourceCounts() }); +}); + +process.on('rejectionHandled', () => write('rejectionHandled', {})); +process.on('warning', (warning) => write('warning', { err: safeErr(warning) })); +process.on('beforeExit', (code) => write('beforeExit', { code, activeResources: activeResourceCounts() })); +process.on('exit', (code) => write('exit', { code })); + +write('preload-installed', {}); From 082e801f4e56fccb7da7e4cd3e8ddce452e38f44 Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 04:43:45 +0300 Subject: [PATCH 02/20] fix(api): address Qodo and CodeRabbit review findings on PR #23 - Instrument process.abort() synchronously in signature-b-preload.cjs before delegating to native abort. - Restrict diagnostic directory mode to 0700 and log file mode to 0600. - Fix documented usage test glob from dist-test/test/**\/*.test.js to dist-test/test/**/*.test.js using line comments. - Add JSDoc docstrings for helper and wrapper functions in signature-b-preload.cjs. - Qualify historical diagnostic capture claims in ADR-0140 and ROADMAP.md noting process.abort was uninstrumented in those initial runs and Signature B root cause remains unresolved. - Add comprehensive synthetic regression tests in signature-b-preload.test.ts. --- docs/ROADMAP.md | 2 +- ...0140-harness-ephemeral-port-acquisition.md | 71 +++--- .../test/diagnostics/signature-b-preload.cjs | 93 ++++++- .../diagnostics/signature-b-preload.test.ts | 227 ++++++++++++++++++ 4 files changed, 343 insertions(+), 50 deletions(-) create mode 100644 packages/api/test/diagnostics/signature-b-preload.test.ts diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index fa1acc2b..6f7a83a2 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -1349,7 +1349,7 @@ Debt observed during M14. Each states what is known, not what is planned; items - **The published capability document could omit a composed feature, and the client offered controls that could not work (RESOLVED in M15 Increment 24 / ADR-0132).** Left open by Increment 23 above. `capabilitiesView` (`packages/api/src/presenters.ts`) took a hand-written `Pick`, so a feature never added to it was invisible to `GET /v1/capabilities` and nothing complained. ADR-0131 judged that "a narrower failure than a 503 — the feature works for anyone who calls the route directly", and deferred it on the grounds that closing it meant first deciding which optional dependencies are user-facing capabilities. **Both halves of that were wrong.** The failure is narrower only when the client can still reach the feature, and a live instance already existed where it could not: `GET /v1/search` served three modes from two independently-gated dependency sets behind one published `search` flag, so a deployment running the Helm chart’s `search.semanticEnabled: false` advertised search while the client offered two mode buttons whose every request answered 503. And the judgement, while genuinely underivable, can be made **unskippable**, which was the property wanted. `semanticSearch` is now published from both of its dependencies, `Exclude[0] | NotAPublishedCapability>` must be `never` so a new optional dependency cannot compile until someone gives it a flag or records why it has none, and a behavioural guard requires every capability source to change what the document publishes — because a key can sit in that parameter unread, which the compile-time half cannot see. The mutation ledger lives in ADR-0132 and is not restated here. - **The search surface was not gated on `capabilities.search`, so an absolute kill switch still showed a search box (RESOLVED in M15 Increment 24 / ADR-0132 §5).** `SEARCH_ENABLED=0` — the chart's `search.enabled: false`, an absolute kill switch per ADR-0055 — leaves `searchRepository` unconstructed and `GET /v1/search` answering 503 on every mode, keyword included. The entry point was the persistent header form in `packages/web/index.html`, present on every page; being a `` rather than an `a[data-route]`, `NAV_CAPABILITY_MAP` could not reach it, which is why the first pass at Increment 24 gated the semantic and hybrid *modes* and left keyword ungated — the same defect class one mode over. Raised by the Qodo review of PR #155. **Fixed in the same increment rather than deferred:** the form now ships `hidden` and is revealed by `applySearchCapability` only on an explicit `search: true`; the route renders an honest unavailable notice and issues no request; and keyword search waits for the capability answer, reversing this increment's own earlier latency decision, because knowing whether a request is pointless requires having asked. A markup-contract test pins the `hidden` attribute, since the gate depends on it and every other test passes without it. - **Clicking a search mode discarded text typed since the page loaded (RESOLVED in a follow-up to M15 Increment 24 / ADR-0132).** `createModeInput` in `packages/web/src/app/search-mount.ts` closed over the query captured when the route mounted, so `navigateToSearchMode` navigated with the old term and the remount reset the input to match it. Type a new term into the header field, click **Semantic** without pressing enter, and the typed text was gone with no indication it had been discarded. Pre-existing — the closure predated Increment 24 and was untouched by it — and found by the adversarial review of PR #155 while reviewing the capability gate wrapped around the same control. **Resolved:** the query is now a `() => string` read when a mode is chosen rather than a string captured when the selector renders, matching what `main.ts`'s submit handler already does, and falling back to the mounted query only where the document has no header input. Two regression tests, one of which fails against the exact pre-fix closure. -- **`startHarness` drew ports `fetch` refuses (RESOLVED in ADR-0140); the unexplained whole-file failure is still open.** Two signatures were filed here, deliberately not as one cause, and that judgement held. **Signature A is resolved.** WHATWG Fetch blocks eighty-two ports and undici enforces the list on the port number alone, before opening a socket — so `server.listen(0)` could bind, listen and answer raw TCP while `fetch` still refused, surfacing as `TypeError: fetch failed` / `Error: bad port` at the harness’s first request rather than at the listen that caused it. Whether it can happen at all is a property of the host’s dynamic port range: a typical Linux CI range (32768–60999) contains no blocked port, while the Windows range in use here (1024–15000) contains nineteen, which is the whole of "green on CI, flaky locally". The guard that already existed was incomplete in a way that still failed — its hand-observed set of eighteen ports was the spec list intersected with one machine’s range **minus `6679`** — and it retried unboundedly, had no behaviour on exhaustion, and had been copy-pasted into `auth-signin-schema.integration.test.ts`, so the missing port had to be found twice. `packages/api/test/listen.ts` now owns port acquisition for both sites: the spec-complete eighty-two ports (verified by sweeping all 65535 through the real `fetch` on Node v24.15.0), a bounded twenty attempts, each rejected listener closed before the next is asked for, a guarded `address()` read in place of the `as AddressInfo` cast, and an exhaustion error naming the attempts and rejected ports and nothing else. **Signature B is not resolved and was not folded in.** A file still fails with `'test failed'`, no assertion, no stack and none of its own tests reported. Twenty consecutive full runs gave five failures: on pre-fix code one signature A (`auth.test.js`) and three signature B; on post-fix code one signature B and no signature A. The port fix removes A and leaves B exactly where it was, which is the evidence that they are two defects. Four different files were hit (`move-explanation-route`, `tournament-commentary-route`, `bot-detection-analyze`, `anti-cheat-analysis`), sharing no import beyond `./helpers`; each died in 589–703 ms with no test of its own reporting and no stderr. Refuted with evidence: ephemeral-port exhaustion (113 sockets in TIME_WAIT against a 13977-port range), a `Promise.race` loser becoming an unhandled rejection (`race` subscribes to every promise, confirmed on v24.15.0), a throwing `after`/`afterEach` hook (the affected files use none), and a double `close()` rejecting (awaited in a `finally`, it would be attributed to that test with a stack). A second bounded pass — twelve more full runs under the TAP reporter with a preload recording `uncaughtException`, `unhandledRejection` and any non-zero exit — produced twelve clean runs and captured nothing at the time. **A follow-up increment (`claude/node-test-signature-b`) then captured the defect directly, three more times, on three files never previously implicated** (`rate-limit-atomicity`, `dependency-parity`, `studies-api`) — seven distinct files in total now, confirming this is not tied to any one file’s logic. A new instrumented preload (`packages/api/test/diagnostics/signature-b-preload.cjs`) hooking every JS-visible process-level event — `process.exit`, `process.kill`, `uncaughtExceptionMonitor`, `unhandledRejection`, `rejectionHandled`, `warning`, `beforeExit`, and Node’s own unconditional `exit` — showed **none of them fire** on any of the three captures, including `exit` itself, which fires for every JS-visible shutdown path including `process.exit()`. A synthetic `process.exit(1)`-before-registration fixture reproduces the identical silent shape; every other synthetic mechanism tried (a post-test async throw, an emitter `'error'` with its listener removed, a synchronous module-load throw, a delayed `SIGKILL`) prints a visibly different diagnostic line, stack, or partial test output that the real defect never shows. This proves the per-file child process (`node --test` spawns one per file, confirmed by distinct PIDs) is being terminated by something outside the JS/V8 layer entirely — not any exception, rejection, or event this repository’s code can throw or catch. The machine had roughly 2.5 GB of 15.7 GB RAM free at capture time with several other agents’ processes concurrently running, which is circumstantially consistent with resource contention, but no crash was recorded in the Windows Application or System event logs in that window, so the exact external trigger is still not established. No fix was invented — the forbidden responses (sleeps, whole-file retries, lowering concurrency) would only hide a cause that remains outside this repository’s code. See ADR-0140 §4. +- **`startHarness` drew ports `fetch` refuses (RESOLVED in ADR-0140); the unexplained whole-file failure is still open.** Two signatures were filed here, deliberately not as one cause, and that judgement held. **Signature A is resolved.** WHATWG Fetch blocks eighty-two ports and undici enforces the list on the port number alone, before opening a socket — so `server.listen(0)` could bind, listen and answer raw TCP while `fetch` still refused, surfacing as `TypeError: fetch failed` / `Error: bad port` at the harness’s first request rather than at the listen that caused it. Whether it can happen at all is a property of the host’s dynamic port range: a typical Linux CI range (32768–60999) contains no blocked port, while the Windows range in use here (1024–15000) contains nineteen, which is the whole of "green on CI, flaky locally". The guard that already existed was incomplete in a way that still failed — its hand-observed set of eighteen ports was the spec list intersected with one machine’s range **minus `6679`** — and it retried unboundedly, had no behaviour on exhaustion, and had been copy-pasted into `auth-signin-schema.integration.test.ts`, so the missing port had to be found twice. `packages/api/test/listen.ts` now owns port acquisition for both sites: the spec-complete eighty-two ports (verified by sweeping all 65535 through the real `fetch` on Node v24.15.0), a bounded twenty attempts, each rejected listener closed before the next is asked for, a guarded `address()` read in place of the `as AddressInfo` cast, and an exhaustion error naming the attempts and rejected ports and nothing else. **Signature B is not resolved and was not folded in.** A file still fails with `'test failed'`, no assertion, no stack and none of its own tests reported. Twenty consecutive full runs gave five failures: on pre-fix code one signature A (`auth.test.js`) and three signature B; on post-fix code one signature B and no signature A. The port fix removes A and leaves B exactly where it was, which is the evidence that they are two defects. Four different files were hit (`move-explanation-route`, `tournament-commentary-route`, `bot-detection-analyze`, `anti-cheat-analysis`), sharing no import beyond `./helpers`; each died in 589–703 ms with no test of its own reporting and no stderr. Refuted with evidence: ephemeral-port exhaustion (113 sockets in TIME_WAIT against a 13977-port range), a `Promise.race` loser becoming an unhandled rejection (`race` subscribes to every promise, confirmed on v24.15.0), a throwing `after`/`afterEach` hook (the affected files use none), and a double `close()` rejecting (awaited in a `finally`, it would be attributed to that test with a stack). A second bounded pass — twelve more full runs under the TAP reporter with a preload recording `uncaughtException`, `unhandledRejection` and any non-zero exit — produced twelve clean runs and captured nothing at the time. **A follow-up increment (`claude/node-test-signature-b`) then captured the defect directly, three more times, on three files never previously implicated** (`rate-limit-atomicity`, `dependency-parity`, `studies-api`) — seven distinct files in total now, confirming this is not tied to any one file’s logic. An instrumented preload (`packages/api/test/diagnostics/signature-b-preload.cjs`) hooking process-level events — `process.exit`, `process.abort`, `process.kill`, `uncaughtExceptionMonitor`, `unhandledRejection`, `rejectionHandled`, `warning`, `beforeExit`, and Node’s own unconditional `exit` — showed **none of the hooks active at the time fired** on any of the three historical captures (though `process.abort()` was not wrapped in those initial runs and is now covered for future occurrences). A synthetic `process.exit(1)`-before-registration fixture reproduces the identical silent shape; every other synthetic mechanism tried (a post-test async throw, an emitter `'error'` with its listener removed, a synchronous module-load throw, a delayed `SIGKILL`) prints a visibly different diagnostic line, stack, or partial test output that the real defect never shows. This narrows the investigated possibilities while leaving the root cause unresolved: the per-file child process (`node --test` spawns one per file, confirmed by distinct PIDs) was not terminated by `process.exit`, unhandled rejections, or uncaught exceptions, and future runs with `process.abort` instrumentation will record whether abort was called or whether termination originated outside the JS runtime entirely. The machine had roughly 2.5 GB of 15.7 GB RAM free at capture time with several other agents’ processes concurrently running, which is circumstantially consistent with resource contention, but no crash was recorded in the Windows Application or System event logs in that window, so the exact external trigger is still not established. No fix was invented — the forbidden responses (sleeps, whole-file retries, lowering concurrency) would only hide a cause that remains outside this repository’s code. See ADR-0140 §4. - **`ApiServer.listen` registered no `'error'` handler, so a failed bind hung and raised an uncaught event (RESOLVED in ADR-0140 §5).** `packages/api/src/server.ts` resolved its promise from the `listening` callback only and built it with no reject path. A bind failing asynchronously (`EADDRINUSE`, `EMFILE`) left the promise pending forever and, with no `'error'` listener on the `http.Server`, was re-raised as an uncaught exception. Found while investigating ADR-0140 and independently raised by the Qodo review of PR #21. **Resolved in the same increment** rather than deferred, because ADR-0140 §2's bounded, diagnosable acquisition is not true without it — the retry can only report a bind error if the listener it is handed rejects. A one-shot `'error'` listener now rejects and is removed once listening, so later server errors keep their previous semantics rather than being swallowed by a `reject` on a settled promise. The regression test fails against the exact pre-fix code, through an uncaught `ERR_UNHANDLED_ERROR`. - **`main.ts`'s controller-disposal list is manual, untested, and silently incomplete when a section is added (RESOLVED in Increment 25 / ADR-0092).** `run()` in `packages/web/src/main.ts` disposes the previous route's controllers by name, and its own comment says doing so "is what makes re-bootstrapping safe" — but adding a section to `bootstrap` and forgetting to add it there compiles, passes every gate, and leaks. Increment 23 shipped exactly that omission for `LearningController` and it was caught in PR review, not by a test. `main.ts` has no test coverage of any kind, so no section's disposal is verified. A structural fix (bootstrap returning its disposables as a collection, or a type-level exhaustiveness check keyed off the result type) would make the next omission a compile error; it is a refactor across ~15 return sites and belongs in its own increment. **Resolved in Increment 25 (ADR-0092):** extracted `createLifecycle` run loop in `lifecycle.ts`, defined `BootstrappedDisposables` and `DisposableKey` driving `DISPOSABLE_TEARDOWN_MAP: Record` for compile-time exhaustiveness, normalised `.dispose()` verb across all disposables, and cascaded `GameController.stop()` to `gameSync.stop()`. - **`stepView` sends the answers to the learner (RESOLVED in Increment 29 / ADR-0095).** `packages/api/src/presenters.ts` emits `expectedSan` on a move step and `correctIndex` on a quiz step, and `GET /v1/lessons/:id/steps` is the route the learner's own lesson page calls. Increment 23 omits both from the client-side types (`packages/web/src/api/models.ts`), so the app cannot render or grade against them and a future edit that tries becomes a compile error — but the fields are still on the wire and readable in devtools. The authoring routes legitimately need them returned to the author, so the fix is a learner-scoped step view (or a caller-dependent projection), not a deletion: an API contract decision with its own ADR. Nothing rated or rewarded depends on step progress today, so this is a wart rather than a breach. **Resolved in Increment 29 (ADR-0095):** added `LearnerStepView` / `learnerStepView` in `packages/api/src/presenters.ts` omitting `expectedSan` and `correctIndex`. `GET /v1/lessons/:id/steps` and `GET /v1/steps/:id` now check course authorship via `repo.getLesson` / `repo.getCourse`, returning full `stepView` to the author and `learnerStepView` to learners and anonymous callers. Updated OpenAPI schema and web model comments. The first attempt resolved authorship with a separate `getLesson` + `getCourse` after the step read, which doubled both routes from 3 SQL queries to 6 because `listSteps` / `getStep` had already made those reads internally and discarded the course; caught in the PR #92 review and fixed by adding `getStepWithCourse` / `listStepsWithCourse` to `LearningRepository`, which return what was already loaded. Both routes now make exactly one repository call, pinned by a counting-proxy test. diff --git a/docs/adr/0140-harness-ephemeral-port-acquisition.md b/docs/adr/0140-harness-ephemeral-port-acquisition.md index f5c90001..5dedb34d 100644 --- a/docs/adr/0140-harness-ephemeral-port-acquisition.md +++ b/docs/adr/0140-harness-ephemeral-port-acquisition.md @@ -217,10 +217,10 @@ None of these match. The real defect's log is completely silent before the bare file-level failure: no diagnostic line, no stderr, no test names at all for that file (not even the ones that would have run first). -**A new diagnostic preload -(`packages/api/test/diagnostics/signature-b-preload.cjs`) proves which of -those mechanisms it is, on real occurrences, not synthetic ones.** It hooks -`process.exit`, `process.kill`, `uncaughtExceptionMonitor` (a passive observer +**A diagnostic preload +(`packages/api/test/diagnostics/signature-b-preload.cjs`) captures which of +those mechanisms fire on real occurrences, not synthetic ones.** It hooks +`process.exit`, `process.abort`, `process.kill`, `uncaughtExceptionMonitor` (a passive observer that, unlike `uncaughtException`, never alters Node's default crash handling), `unhandledRejection`, `rejectionHandled`, `warning`, `beforeExit`, and Node's own unconditional `exit` event, writing a structured, redacted, timestamped @@ -234,38 +234,37 @@ of the original four. Combined with the original four, that is **seven different files**, sharing nothing but the shared harness's `startHarness` import, which confirms this is not a defect specific to any one file's logic. -All three captures show the identical signature: **only the `start` and -`preload-installed` lines are logged. Nothing else fires — including Node's own -`process.on('exit')`, which fires unconditionally for every JS-visible shutdown -path, `process.exit()` included.** That rules out, directly and by observation -rather than inference, every mechanism the preload watches for: `process.exit`, -an uncaught exception, an unhandled rejection, a handled-late rejection, a -runtime warning, and reaching an idle event loop. The child process disappeared -before the JS runtime got to react to anything. - -**This narrows Signature B's cause to a class, not a line of code:** the -per-file child process is being terminated by something outside the JS/V8 -layer entirely — an uncatchable signal or an external kill — not by any defect -this repository's TypeScript can throw, catch, or leak. Two observations are -consistent with an environmental (not code) origin: at the time of capture this -development machine had roughly 2.5 GB of 15.7 GB RAM free, with several -unrelated concurrent processes (other agents' worktrees and dev servers) -running; and `node --test` spawns dozens of child processes across a full -`packages/api` run, each loading the same large cross-package import graph, -which is exactly the pattern most exposed to transient resource contention. -Neither observation is proof of a specific external actor — no crash was -recorded in the Windows Application or System event logs in the capture -window — so the exact trigger (OS scheduler, memory pressure, antivirus, or -something else entirely) is not established. - -**No fix is proposed.** Every mechanism this repository's code could cause and -catch has been directly ruled out on real captures; what remains is external to -the process, and the forbidden responses — sleeps, retries around the whole -file, swallowing errors, lowering concurrency to hide it — would suppress the -symptom without touching whatever the cause turns out to be. Signature B stays -open. The diagnostic preload is committed so the next occurrence — on this -machine, in CI, or elsewhere — can be captured with this same evidence rather -than re-deriving it. +All three historical captures show the identical signature: **only the `start` and +`preload-installed` lines were logged. None of the hooks active at that time fired — +including `process.exit`, `process.kill`, `uncaughtExceptionMonitor`, `unhandledRejection`, +`rejectionHandled`, `warning`, `beforeExit`, and Node's `exit` event.** +Crucially, however, the diagnostic preload in use during those historical runs did +not yet wrap `process.abort()`. Because `process.abort()` terminates the process +immediately without emitting Node's `exit` event, those historical captures directly +ruled out `process.exit`, unhandled rejections, uncaught exceptions, and reaching an +idle event loop, but could not categorically exclude an uninstrumented `process.abort()` +or an external termination. + +**This narrows Signature B's investigated possibilities while leaving its root cause unresolved:** +`process.abort()` is now instrumented so any future occurrence will record whether an abort +was invoked or whether termination originated outside the JS runtime entirely (e.g. an OS-level +kill, uncatchable signal, or native crash). Two observations remain consistent with an +environmental (not code) origin: at the time of capture this development machine had roughly +2.5 GB of 15.7 GB RAM free, with several unrelated concurrent processes (other agents' worktrees +and dev servers) running; and `node --test` spawns dozens of child processes across a full +`packages/api` run, each loading the same large cross-package import graph, which is exactly the +pattern most exposed to transient resource contention. Neither observation is proof of a specific +external actor — no crash was recorded in the Windows Application or System event logs in the +capture window — so the exact trigger (OS scheduler, memory pressure, antivirus, or something +else entirely) is not established. + +**No fix is proposed.** The investigated JS mechanisms (`process.exit`, exceptions, rejections) +have been ruled out on the historical captures, `process.abort()` instrumentation is in place for +future occurrences, and the forbidden responses — sleeps, retries around the whole file, +swallowing errors, lowering concurrency to hide it — would suppress the symptom without touching +whatever the root cause turns out to be. Signature B stays open and unresolved. The diagnostic +preload is committed so the next occurrence — on this machine, in CI, or elsewhere — can be +captured with complete instrumentation rather than re-deriving it. ## 5. `ApiServer.listen` rejects on a failed bind diff --git a/packages/api/test/diagnostics/signature-b-preload.cjs b/packages/api/test/diagnostics/signature-b-preload.cjs index b70ef030..6f610309 100644 --- a/packages/api/test/diagnostics/signature-b-preload.cjs +++ b/packages/api/test/diagnostics/signature-b-preload.cjs @@ -9,18 +9,11 @@ * no subtest recorded a failure — see `internal/test_runner/runner.js`. * * This module distinguishes the JS-catchable causes of that shape - * (`process.exit`, an uncaught exception, an unhandled rejection, an - * EventEmitter `'error'` event with no listener) from an external, + * (`process.exit`, `process.abort`, an uncaught exception, an unhandled + * rejection, an EventEmitter `'error'` event with no listener) from an external, * uncatchable termination of the child (an OS-level kill or native crash), * by hooking every relevant process-level event and writing what fired to a - * structured log. If NONE of these hooks fire before the child disappears — - * including Node's own unconditional `process.on('exit')`, which fires for - * every JS-visible shutdown path including `process.exit()` itself — the - * termination did not go through the JS layer at all. - * - * Usage (manual, not wired into `npm test`; run from packages/api): - * node --require ./test/diagnostics/signature-b-preload.cjs \ - * --test --test-concurrency=1 "dist-test/test/**\/*.test.js" + * structured log. * * Set SIGB_LOG_DIR to control where logs land (defaults under the OS temp * dir). Logs are structured JSONL, one line per event, one file per child @@ -42,15 +35,27 @@ * Node's own re-raise of it, just without the emitter's constructor name. */ +// Usage (manual, not wired into `npm test`; run from packages/api): +// node --require ./test/diagnostics/signature-b-preload.cjs \ +// --test --test-concurrency=1 "dist-test/test/**/*.test.js" + const fs = require('node:fs'); const os = require('node:os'); const path = require('node:path'); const LOG_DIR = process.env.SIGB_LOG_DIR || path.join(os.tmpdir(), 'sigb-diag'); -try { fs.mkdirSync(LOG_DIR, { recursive: true }); } catch { /* best-effort; missing dir just drops logging */ } +try { + fs.mkdirSync(LOG_DIR, { recursive: true, mode: 0o700 }); +} catch { + /* best-effort; missing dir just drops logging */ +} const LOG_FILE = path.join(LOG_DIR, `run-${process.pid}-${Date.now()}.jsonl`); let fd; -try { fd = fs.openSync(LOG_FILE, 'a'); } catch { fd = null; } +try { + fd = fs.openSync(LOG_FILE, 'a', 0o600); +} catch { + fd = null; +} const REDACT_PATTERNS = [ [/sk-[a-zA-Z0-9_-]{10,}/g, '[REDACTED_API_KEY]'], @@ -59,6 +64,13 @@ const REDACT_PATTERNS = [ [/(password|secret|token|authorization|cookie)\s*[:=]\s*["']?[^"',\s]+["']?/gi, '$1=[REDACTED]'], ]; +/** + * Redacts known sensitive patterns (API keys, bearer tokens, credentials, passwords) + * from strings before writing to diagnostic logs. + * + * @param {unknown} value - The input value to redact. + * @returns {unknown} The sanitized value with sensitive patterns replaced by redaction placeholders. + */ function redact(value) { if (typeof value !== 'string') return value; let out = value; @@ -66,6 +78,13 @@ function redact(value) { return out; } +/** + * Safely serializes an Error or error-like object into a redacted, JSON-safe structure. + * Prevents circular reference crashes and strips credentials from error messages and stacks. + * + * @param {unknown} err - The error or rejection value to serialize. + * @returns {unknown} A serialized error object or redacted primitive. + */ function safeErr(err) { if (err == null) return err; if (typeof err !== 'object') return redact(String(err)); @@ -79,6 +98,12 @@ function safeErr(err) { }; } +/** + * Collects a non-identifying, aggregate count of currently active Node async resources + * using `process.getActiveResourcesInfo()` where available. + * + * @returns {Record | null} Resource counts by type, or null if unsupported. + */ function activeResourceCounts() { try { if (typeof process.getActiveResourcesInfo !== 'function') return null; @@ -90,6 +115,14 @@ function activeResourceCounts() { } } +/** + * Synchronously appends a structured JSONL diagnostic record to the active log file. + * Synchronous writes ensure diagnostic records survive abrupt process termination. + * + * @param {string} kind - The event or lifecycle hook kind (e.g. 'start', 'process.abort'). + * @param {Record} fields - Event-specific payload fields. + * @returns {void} + */ function write(kind, fields) { if (!fd) return; const line = JSON.stringify({ @@ -102,18 +135,52 @@ function write(kind, fields) { timeNs: process.hrtime.bigint().toString(), ...fields, }); - try { fs.writeSync(fd, line + '\n'); } catch { /* best-effort */ } + try { + fs.writeSync(fd, line + '\n'); + } catch { + /* best-effort */ + } } write('start', {}); const originalExit = process.exit.bind(process); +/** + * Patched wrapper around `process.exit` that synchronously records the exit code + * and redacted caller stack before invoking the original `process.exit`. + * + * @param {number} [code] - The process exit code. + * @returns {never} + */ process.exit = function patchedExit(code) { write('process.exit', { code, callerStack: redact(new Error('exit-call-site').stack) }); return originalExit(code); }; +const originalAbort = typeof process.abort === 'function' ? process.abort.bind(process) : null; +if (originalAbort) { + /** + * Patched wrapper around `process.abort` that synchronously records the abort event + * and redacted caller stack before invoking the original `process.abort`. + * + * @param {...unknown} args - Any arguments forwarded to process.abort. + * @returns {never} + */ + process.abort = function patchedAbort(...args) { + write('process.abort', { callerStack: redact(new Error('abort-call-site').stack) }); + return originalAbort(...args); + }; +} + const originalKill = process.kill.bind(process); +/** + * Patched wrapper around `process.kill` that synchronously records target PID and signal + * before delegating to the original `process.kill`. + * + * @param {number} pid - Target process ID. + * @param {string | number} [signal] - Signal to send. + * @returns {boolean} Result of original process.kill. + */ process.kill = function patchedKill(pid, signal) { write('process.kill', { targetPid: pid, signal, callerStack: redact(new Error('kill-call-site').stack) }); return originalKill(pid, signal); diff --git a/packages/api/test/diagnostics/signature-b-preload.test.ts b/packages/api/test/diagnostics/signature-b-preload.test.ts new file mode 100644 index 00000000..66087894 --- /dev/null +++ b/packages/api/test/diagnostics/signature-b-preload.test.ts @@ -0,0 +1,227 @@ +import { test } from 'node:test'; +import * as assert from 'node:assert/strict'; +import { spawnSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; + +const PRELOAD_PATH = path.resolve(__dirname, '../../../test/diagnostics/signature-b-preload.cjs'); + +interface DiagnosticRecord { + readonly kind: string; + readonly pid: number; + readonly ppid: number; + readonly code?: number; + readonly targetPid?: number; + readonly signal?: string | number; + readonly callerStack?: string; + readonly err?: { + readonly name?: string; + readonly message?: string; + readonly stack?: string; + }; + readonly reason?: { + readonly name?: string; + readonly message?: string; + readonly stack?: string; + }; + readonly activeResources?: Record | null; +} + +function runWithPreload( + code: string, + envOverride: Record = {}, +): { + readonly status: number | null; + readonly signal: NodeJS.Signals | null; + readonly logDir: string; + readonly records: readonly DiagnosticRecord[]; +} { + const generatedLogDir = fs.mkdtempSync(path.join(os.tmpdir(), 'sigb-test-')); + const targetLogDir = envOverride.SIGB_LOG_DIR || generatedLogDir; + const scriptPath = path.join(generatedLogDir, 'test-target.cjs'); + fs.writeFileSync(scriptPath, code, 'utf8'); + + const result = spawnSync(process.execPath, ['--require', PRELOAD_PATH, scriptPath], { + env: { + ...process.env, + SIGB_LOG_DIR: targetLogDir, + ...envOverride, + }, + encoding: 'utf8', + windowsHide: true, + }); + + const files = fs.existsSync(targetLogDir) + ? fs.readdirSync(targetLogDir).filter((f) => f.endsWith('.jsonl')) + : []; + const records: DiagnosticRecord[] = []; + for (const file of files) { + const content = fs.readFileSync(path.join(targetLogDir, file), 'utf8'); + const lines = content.split('\n').map((l) => l.trim()).filter(Boolean); + for (const line of lines) { + records.push(JSON.parse(line) as DiagnosticRecord); + } + } + + return { + status: result.status, + signal: result.signal, + logDir: generatedLogDir, + records, + }; +} + +test('diagnostic preload: normal shutdown records lifecycle events', (t) => { + const { status, records, logDir } = runWithPreload(` + // clean normal execution + const x = 1 + 1; + `); + t.after(() => fs.rmSync(logDir, { recursive: true, force: true })); + + assert.equal(status, 0); + const kinds = records.map((r) => r.kind); + assert.ok(kinds.includes('start'), 'records start'); + assert.ok(kinds.includes('preload-installed'), 'records preload-installed'); + assert.ok(kinds.includes('beforeExit'), 'records beforeExit'); + assert.ok(kinds.includes('exit'), 'records exit'); +}); + +test('diagnostic preload: process.exit(42) records exit code and call-site stack before exit', (t) => { + const { status, records, logDir } = runWithPreload(` + process.exit(42); + `); + t.after(() => fs.rmSync(logDir, { recursive: true, force: true })); + + assert.equal(status, 42); + const exitRecord = records.find((r) => r.kind === 'process.exit'); + assert.ok(exitRecord, 'records process.exit'); + assert.equal(exitRecord?.code, 42); + assert.ok(typeof exitRecord?.callerStack === 'string', 'includes callerStack'); + assert.ok(exitRecord?.callerStack?.includes('exit-call-site'), 'stack traces exit invocation site'); +}); + +test('diagnostic preload: process.abort() records abort synchronously without relying on exit hook', (t) => { + const { status, signal, records, logDir } = runWithPreload(` + process.abort(); + `); + t.after(() => fs.rmSync(logDir, { recursive: true, force: true })); + + // On Windows/POSIX, abort causes abnormal termination (non-zero exit or signal) + assert.ok(status !== 0 || signal !== null, 'process terminates abnormally'); + const abortRecord = records.find((r) => r.kind === 'process.abort'); + assert.ok(abortRecord, 'synchronously records process.abort'); + assert.ok(typeof abortRecord?.callerStack === 'string', 'includes callerStack'); + assert.ok(abortRecord?.callerStack?.includes('abort-call-site'), 'stack traces abort call site'); + + // Verify process.on('exit') does NOT run on process.abort() + const exitRecord = records.find((r) => r.kind === 'exit'); + assert.equal(exitRecord, undefined, 'exit event must not fire on process.abort'); +}); + +test('diagnostic preload: uncaughtExceptionMonitor passively captures error without suppressing crash', (t) => { + const { status, records, logDir } = runWithPreload(` + throw new Error('synthetic fatal crash with Bearer secret-tok-12345'); + `); + t.after(() => fs.rmSync(logDir, { recursive: true, force: true })); + + assert.notEqual(status, 0); + const uncaught = records.find((r) => r.kind === 'uncaughtExceptionMonitor'); + assert.ok(uncaught, 'records uncaughtExceptionMonitor'); + assert.ok(uncaught?.err?.message?.includes('Bearer [REDACTED_TOKEN]'), 'redacts token in error message'); +}); + +test('diagnostic preload: unhandledRejection captures rejection reason', (t) => { + const { records, logDir } = runWithPreload(` + Promise.reject(new Error('synthetic unhandled rejection: password="mysecretpassword"')); + `); + t.after(() => fs.rmSync(logDir, { recursive: true, force: true })); + + const rejection = records.find((r) => r.kind === 'unhandledRejection'); + assert.ok(rejection, 'records unhandledRejection'); + assert.ok(rejection?.reason?.message?.includes('password=[REDACTED]'), 'redacts password in rejection reason'); +}); + +test('diagnostic preload: process.kill records target PID and signal', (t) => { + const { records, logDir } = runWithPreload(` + try { + process.kill(process.pid, 0); + } catch {} + `); + t.after(() => fs.rmSync(logDir, { recursive: true, force: true })); + + const killRecord = records.find((r) => r.kind === 'process.kill'); + assert.ok(killRecord, 'records process.kill'); + assert.equal(killRecord?.signal, 0); + assert.ok(killRecord?.callerStack?.includes('kill-call-site'), 'traces kill call site'); +}); + +test('diagnostic preload: redacts known sensitive token, key, password, and database credential patterns', (t) => { + const { records, logDir } = runWithPreload(` + const err = new Error('Connect failed to postgres://app_user:s3cr3tpass@db.internal:5432/chess and sk-1234567890abcdef12345'); + process.on('uncaughtExceptionMonitor', () => {}); + throw err; + `); + t.after(() => fs.rmSync(logDir, { recursive: true, force: true })); + + const uncaught = records.find((r) => r.kind === 'uncaughtExceptionMonitor'); + assert.ok(uncaught, 'uncaught record exists'); + const msg = uncaught?.err?.message ?? ''; + assert.ok(msg.includes('postgres://[REDACTED_CREDS]@db.internal:5432/chess'), 'redacts postgres credentials'); + assert.ok(msg.includes('[REDACTED_API_KEY]'), 'redacts OpenAI-style api key'); + assert.ok(!msg.includes('s3cr3tpass'), 'raw password not present'); + assert.ok(!msg.includes('sk-1234567890abcdef12345'), 'raw key not present'); +}); + +test('diagnostic preload: directory mode 0700 and log file mode 0600', (t) => { + const targetDir = path.join(os.tmpdir(), `sigb-mode-test-${Date.now()}-${Math.random().toString(36).slice(2)}`); + const { logDir } = runWithPreload('const a = 1;', { SIGB_LOG_DIR: targetDir }); + t.after(() => { + fs.rmSync(logDir, { recursive: true, force: true }); + fs.rmSync(targetDir, { recursive: true, force: true }); + }); + + assert.ok(fs.existsSync(targetDir), 'target directory was created'); + const files = fs.readdirSync(targetDir).filter((f) => f.endsWith('.jsonl')); + assert.ok(files.length > 0, 'created log file'); + + const preloadSource = fs.readFileSync(PRELOAD_PATH, 'utf8'); + assert.ok(preloadSource.includes('0o700'), 'mkdirSync specifies 0o700 mode'); + assert.ok(preloadSource.includes('0o600'), 'openSync specifies 0o600 mode'); + + if (process.platform !== 'win32') { + const dirStat = fs.statSync(targetDir); + const fileStat = fs.statSync(path.join(targetDir, files[0])); + // On POSIX, check mode bits + const dirMode = dirStat.mode & 0o777; + const fileMode = fileStat.mode & 0o777; + assert.equal(dirMode, 0o700, `directory mode should be 0700, got ${dirMode.toString(8)}`); + assert.equal(fileMode, 0o600, `file mode should be 0600, got ${fileMode.toString(8)}`); + } +}); + +test('diagnostic preload: pre-existing directory does not fail or crash', (t) => { + const existingDir = fs.mkdtempSync(path.join(os.tmpdir(), 'sigb-existing-')); + const { status, records, logDir } = runWithPreload('const ok = true;', { SIGB_LOG_DIR: existingDir }); + t.after(() => { + fs.rmSync(logDir, { recursive: true, force: true }); + fs.rmSync(existingDir, { recursive: true, force: true }); + }); + + assert.equal(status, 0, 'runs cleanly with pre-existing directory'); + assert.ok(records.length > 0, 'wrote diagnostic records to pre-existing directory'); +}); + +test('diagnostic preload: usage glob pattern matches test files without literal backslash', () => { + const correctGlob = 'dist-test/test/**/*.test.js'; + const badEscapedGlob = 'dist-test/test/**\\/*.test.js'; + + // Bad glob contains a literal backslash before slash + assert.ok(badEscapedGlob.includes('\\/'), 'bad glob contains literal escaped slash'); + assert.ok(!correctGlob.includes('\\/'), 'correct glob contains clean POSIX path separator'); + assert.ok(correctGlob.startsWith('dist-test/test/'), 'correct glob targets compiled test files'); + + const preloadSource = fs.readFileSync(PRELOAD_PATH, 'utf8'); + assert.ok(preloadSource.includes('"dist-test/test/**/*.test.js"'), 'preload usage specifies unescaped glob'); + assert.ok(!preloadSource.includes('**\\/*.test.js'), 'preload usage does not contain escaped backslash in glob'); +}); From 5223a8effaf6433c90c2817ce20d5e63d8171701 Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 04:52:46 +0300 Subject: [PATCH 03/20] test(api): scope child process cwd and disable POSIX core dumps in diagnostic tests --- .../diagnostics/signature-b-preload.test.ts | 40 ++++++++++++++----- 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/packages/api/test/diagnostics/signature-b-preload.test.ts b/packages/api/test/diagnostics/signature-b-preload.test.ts index 66087894..0d46dc42 100644 --- a/packages/api/test/diagnostics/signature-b-preload.test.ts +++ b/packages/api/test/diagnostics/signature-b-preload.test.ts @@ -28,6 +28,14 @@ interface DiagnosticRecord { readonly activeResources?: Record | null; } +/** + * Executes a synthetic test script under the diagnostic preload in an isolated temporary directory. + * Sets `cwd: generatedLogDir` and disables core dumps on POSIX (`ulimit -c 0`) to prevent crash artifacts. + * + * @param {string} code - JavaScript code to execute in the child process. + * @param {Record} [envOverride] - Optional environment variables to override. + * @returns {{ status: number | null, signal: NodeJS.Signals | null, logDir: string, records: readonly DiagnosticRecord[] }} + */ function runWithPreload( code: string, envOverride: Record = {}, @@ -42,15 +50,29 @@ function runWithPreload( const scriptPath = path.join(generatedLogDir, 'test-target.cjs'); fs.writeFileSync(scriptPath, code, 'utf8'); - const result = spawnSync(process.execPath, ['--require', PRELOAD_PATH, scriptPath], { - env: { - ...process.env, - SIGB_LOG_DIR: targetLogDir, - ...envOverride, - }, - encoding: 'utf8', - windowsHide: true, - }); + const args = ['--require', PRELOAD_PATH, scriptPath]; + const env = { + ...process.env, + SIGB_LOG_DIR: targetLogDir, + ...envOverride, + }; + + // Set cwd to generatedLogDir and disable core dumping on POSIX to prevent routine test runs + // from leaving crash files or triggering host crash reporting. + const result = + process.platform !== 'win32' + ? spawnSync('sh', ['-c', 'ulimit -c 0 2>/dev/null; exec "$@"', 'sh', process.execPath, ...args], { + cwd: generatedLogDir, + env, + encoding: 'utf8', + windowsHide: true, + }) + : spawnSync(process.execPath, args, { + cwd: generatedLogDir, + env, + encoding: 'utf8', + windowsHide: true, + }); const files = fs.existsSync(targetLogDir) ? fs.readdirSync(targetLogDir).filter((f) => f.endsWith('.jsonl')) From eca5ed1c8af7e11d4ded5af56fc2bc4971255883 Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 04:59:20 +0300 Subject: [PATCH 04/20] fix(api): support postgresql:// uri redaction and use passive uncaughtExceptionMonitor for unhandled rejections --- .../test/diagnostics/signature-b-preload.cjs | 14 +++++------- .../diagnostics/signature-b-preload.test.ts | 22 +++++++++++-------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/packages/api/test/diagnostics/signature-b-preload.cjs b/packages/api/test/diagnostics/signature-b-preload.cjs index 6f610309..f9065cf0 100644 --- a/packages/api/test/diagnostics/signature-b-preload.cjs +++ b/packages/api/test/diagnostics/signature-b-preload.cjs @@ -60,7 +60,7 @@ try { const REDACT_PATTERNS = [ [/sk-[a-zA-Z0-9_-]{10,}/g, '[REDACTED_API_KEY]'], [/Bearer\s+[A-Za-z0-9._~+/-]+=*/g, 'Bearer [REDACTED_TOKEN]'], - [/postgres:\/\/[^:]+:[^@]+@/g, 'postgres://[REDACTED_CREDS]@'], + [/(?:postgres|postgresql):\/\/[^:]+:[^@]+@/g, 'postgres://[REDACTED_CREDS]@'], [/(password|secret|token|authorization|cookie)\s*[:=]\s*["']?[^"',\s]+["']?/gi, '$1=[REDACTED]'], ]; @@ -186,18 +186,14 @@ process.kill = function patchedKill(pid, signal) { return originalKill(pid, signal); }; -// Passive observer only: unlike 'uncaughtException', registering this does -// NOT suppress Node's default crash behavior, so it cannot itself change -// whether or how the process exits. +// Passive observer only: unlike 'uncaughtException' or 'unhandledRejection', registering +// 'uncaughtExceptionMonitor' does NOT suppress Node's default crash behavior, so it cannot +// itself change whether or how the process exits. Node emits it for both 'uncaughtException' +// and 'unhandledRejection' origins. process.on('uncaughtExceptionMonitor', (err, origin) => { write('uncaughtExceptionMonitor', { err: safeErr(err), origin, activeResources: activeResourceCounts() }); }); -process.on('unhandledRejection', (reason) => { - write('unhandledRejection', { reason: safeErr(reason), activeResources: activeResourceCounts() }); -}); - -process.on('rejectionHandled', () => write('rejectionHandled', {})); process.on('warning', (warning) => write('warning', { err: safeErr(warning) })); process.on('beforeExit', (code) => write('beforeExit', { code, activeResources: activeResourceCounts() })); process.on('exit', (code) => write('exit', { code })); diff --git a/packages/api/test/diagnostics/signature-b-preload.test.ts b/packages/api/test/diagnostics/signature-b-preload.test.ts index 0d46dc42..468c21b6 100644 --- a/packages/api/test/diagnostics/signature-b-preload.test.ts +++ b/packages/api/test/diagnostics/signature-b-preload.test.ts @@ -14,6 +14,7 @@ interface DiagnosticRecord { readonly code?: number; readonly targetPid?: number; readonly signal?: string | number; + readonly origin?: string; readonly callerStack?: string; readonly err?: { readonly name?: string; @@ -150,18 +151,20 @@ test('diagnostic preload: uncaughtExceptionMonitor passively captures error with assert.notEqual(status, 0); const uncaught = records.find((r) => r.kind === 'uncaughtExceptionMonitor'); assert.ok(uncaught, 'records uncaughtExceptionMonitor'); + assert.equal(uncaught?.origin, 'uncaughtException'); assert.ok(uncaught?.err?.message?.includes('Bearer [REDACTED_TOKEN]'), 'redacts token in error message'); }); -test('diagnostic preload: unhandledRejection captures rejection reason', (t) => { - const { records, logDir } = runWithPreload(` +test('diagnostic preload: unhandledRejection passively captured by uncaughtExceptionMonitor with fatal exit', (t) => { + const { status, records, logDir } = runWithPreload(` Promise.reject(new Error('synthetic unhandled rejection: password="mysecretpassword"')); `); t.after(() => fs.rmSync(logDir, { recursive: true, force: true })); - const rejection = records.find((r) => r.kind === 'unhandledRejection'); - assert.ok(rejection, 'records unhandledRejection'); - assert.ok(rejection?.reason?.message?.includes('password=[REDACTED]'), 'redacts password in rejection reason'); + assert.notEqual(status, 0, 'unhandled rejection must cause non-zero exit'); + const rejectionRecord = records.find((r) => r.kind === 'uncaughtExceptionMonitor' && r.origin === 'unhandledRejection'); + assert.ok(rejectionRecord, 'records unhandledRejection origin via uncaughtExceptionMonitor'); + assert.ok(rejectionRecord?.err?.message?.includes('password=[REDACTED]'), 'redacts password in rejection reason'); }); test('diagnostic preload: process.kill records target PID and signal', (t) => { @@ -178,10 +181,9 @@ test('diagnostic preload: process.kill records target PID and signal', (t) => { assert.ok(killRecord?.callerStack?.includes('kill-call-site'), 'traces kill call site'); }); -test('diagnostic preload: redacts known sensitive token, key, password, and database credential patterns', (t) => { +test('diagnostic preload: redacts postgres:// and postgresql:// connection strings, tokens, and passwords', (t) => { const { records, logDir } = runWithPreload(` - const err = new Error('Connect failed to postgres://app_user:s3cr3tpass@db.internal:5432/chess and sk-1234567890abcdef12345'); - process.on('uncaughtExceptionMonitor', () => {}); + const err = new Error('Connect failed to postgresql://app_user:s3cr3tpass@db.internal:5432/chess and postgres://admin:supersecret@10.0.0.1:5432/main and sk-1234567890abcdef12345'); throw err; `); t.after(() => fs.rmSync(logDir, { recursive: true, force: true })); @@ -189,9 +191,11 @@ test('diagnostic preload: redacts known sensitive token, key, password, and data const uncaught = records.find((r) => r.kind === 'uncaughtExceptionMonitor'); assert.ok(uncaught, 'uncaught record exists'); const msg = uncaught?.err?.message ?? ''; - assert.ok(msg.includes('postgres://[REDACTED_CREDS]@db.internal:5432/chess'), 'redacts postgres credentials'); + assert.ok(msg.includes('postgres://[REDACTED_CREDS]@db.internal:5432/chess'), 'redacts postgresql credentials'); + assert.ok(msg.includes('postgres://[REDACTED_CREDS]@10.0.0.1:5432/main'), 'redacts postgres credentials'); assert.ok(msg.includes('[REDACTED_API_KEY]'), 'redacts OpenAI-style api key'); assert.ok(!msg.includes('s3cr3tpass'), 'raw password not present'); + assert.ok(!msg.includes('supersecret'), 'raw second password not present'); assert.ok(!msg.includes('sk-1234567890abcdef12345'), 'raw key not present'); }); From 978bd3d134c4f6ae7da471dffe8f8e7144b7eee2 Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 05:43:27 +0300 Subject: [PATCH 05/20] fix(api): address Qodo and CodeRabbit review findings on diagnostic preload - Harden safeErr and write serialization against BigInt, circular references, and non-primitive metadata - Update redaction regex to sanitize quoted secret values containing spaces - Document and test passive uncaughtExceptionMonitor observation for unhandled rejections under node --test - Isolate real native process.abort execution to explicit diagnostic fixture (signature-b-preload-abort.diag.ts) to prevent core dumps in routine test runs - Align ADR-0140 and ROADMAP.md descriptions with passive monitor behavior and runner reporting --- docs/ROADMAP.md | 2 +- ...0140-harness-ephemeral-port-acquisition.md | 16 +- packages/api/package.json | 1 + .../signature-b-preload-abort.diag.ts | 108 ++++++++++++ .../test/diagnostics/signature-b-preload.cjs | 89 +++++++--- .../diagnostics/signature-b-preload.test.ts | 154 ++++++++++++------ 6 files changed, 287 insertions(+), 83 deletions(-) create mode 100644 packages/api/test/diagnostics/signature-b-preload-abort.diag.ts diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 6f7a83a2..4bfac6b1 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -1349,7 +1349,7 @@ Debt observed during M14. Each states what is known, not what is planned; items - **The published capability document could omit a composed feature, and the client offered controls that could not work (RESOLVED in M15 Increment 24 / ADR-0132).** Left open by Increment 23 above. `capabilitiesView` (`packages/api/src/presenters.ts`) took a hand-written `Pick`, so a feature never added to it was invisible to `GET /v1/capabilities` and nothing complained. ADR-0131 judged that "a narrower failure than a 503 — the feature works for anyone who calls the route directly", and deferred it on the grounds that closing it meant first deciding which optional dependencies are user-facing capabilities. **Both halves of that were wrong.** The failure is narrower only when the client can still reach the feature, and a live instance already existed where it could not: `GET /v1/search` served three modes from two independently-gated dependency sets behind one published `search` flag, so a deployment running the Helm chart’s `search.semanticEnabled: false` advertised search while the client offered two mode buttons whose every request answered 503. And the judgement, while genuinely underivable, can be made **unskippable**, which was the property wanted. `semanticSearch` is now published from both of its dependencies, `Exclude[0] | NotAPublishedCapability>` must be `never` so a new optional dependency cannot compile until someone gives it a flag or records why it has none, and a behavioural guard requires every capability source to change what the document publishes — because a key can sit in that parameter unread, which the compile-time half cannot see. The mutation ledger lives in ADR-0132 and is not restated here. - **The search surface was not gated on `capabilities.search`, so an absolute kill switch still showed a search box (RESOLVED in M15 Increment 24 / ADR-0132 §5).** `SEARCH_ENABLED=0` — the chart's `search.enabled: false`, an absolute kill switch per ADR-0055 — leaves `searchRepository` unconstructed and `GET /v1/search` answering 503 on every mode, keyword included. The entry point was the persistent header form in `packages/web/index.html`, present on every page; being a `` rather than an `a[data-route]`, `NAV_CAPABILITY_MAP` could not reach it, which is why the first pass at Increment 24 gated the semantic and hybrid *modes* and left keyword ungated — the same defect class one mode over. Raised by the Qodo review of PR #155. **Fixed in the same increment rather than deferred:** the form now ships `hidden` and is revealed by `applySearchCapability` only on an explicit `search: true`; the route renders an honest unavailable notice and issues no request; and keyword search waits for the capability answer, reversing this increment's own earlier latency decision, because knowing whether a request is pointless requires having asked. A markup-contract test pins the `hidden` attribute, since the gate depends on it and every other test passes without it. - **Clicking a search mode discarded text typed since the page loaded (RESOLVED in a follow-up to M15 Increment 24 / ADR-0132).** `createModeInput` in `packages/web/src/app/search-mount.ts` closed over the query captured when the route mounted, so `navigateToSearchMode` navigated with the old term and the remount reset the input to match it. Type a new term into the header field, click **Semantic** without pressing enter, and the typed text was gone with no indication it had been discarded. Pre-existing — the closure predated Increment 24 and was untouched by it — and found by the adversarial review of PR #155 while reviewing the capability gate wrapped around the same control. **Resolved:** the query is now a `() => string` read when a mode is chosen rather than a string captured when the selector renders, matching what `main.ts`'s submit handler already does, and falling back to the mounted query only where the document has no header input. Two regression tests, one of which fails against the exact pre-fix closure. -- **`startHarness` drew ports `fetch` refuses (RESOLVED in ADR-0140); the unexplained whole-file failure is still open.** Two signatures were filed here, deliberately not as one cause, and that judgement held. **Signature A is resolved.** WHATWG Fetch blocks eighty-two ports and undici enforces the list on the port number alone, before opening a socket — so `server.listen(0)` could bind, listen and answer raw TCP while `fetch` still refused, surfacing as `TypeError: fetch failed` / `Error: bad port` at the harness’s first request rather than at the listen that caused it. Whether it can happen at all is a property of the host’s dynamic port range: a typical Linux CI range (32768–60999) contains no blocked port, while the Windows range in use here (1024–15000) contains nineteen, which is the whole of "green on CI, flaky locally". The guard that already existed was incomplete in a way that still failed — its hand-observed set of eighteen ports was the spec list intersected with one machine’s range **minus `6679`** — and it retried unboundedly, had no behaviour on exhaustion, and had been copy-pasted into `auth-signin-schema.integration.test.ts`, so the missing port had to be found twice. `packages/api/test/listen.ts` now owns port acquisition for both sites: the spec-complete eighty-two ports (verified by sweeping all 65535 through the real `fetch` on Node v24.15.0), a bounded twenty attempts, each rejected listener closed before the next is asked for, a guarded `address()` read in place of the `as AddressInfo` cast, and an exhaustion error naming the attempts and rejected ports and nothing else. **Signature B is not resolved and was not folded in.** A file still fails with `'test failed'`, no assertion, no stack and none of its own tests reported. Twenty consecutive full runs gave five failures: on pre-fix code one signature A (`auth.test.js`) and three signature B; on post-fix code one signature B and no signature A. The port fix removes A and leaves B exactly where it was, which is the evidence that they are two defects. Four different files were hit (`move-explanation-route`, `tournament-commentary-route`, `bot-detection-analyze`, `anti-cheat-analysis`), sharing no import beyond `./helpers`; each died in 589–703 ms with no test of its own reporting and no stderr. Refuted with evidence: ephemeral-port exhaustion (113 sockets in TIME_WAIT against a 13977-port range), a `Promise.race` loser becoming an unhandled rejection (`race` subscribes to every promise, confirmed on v24.15.0), a throwing `after`/`afterEach` hook (the affected files use none), and a double `close()` rejecting (awaited in a `finally`, it would be attributed to that test with a stack). A second bounded pass — twelve more full runs under the TAP reporter with a preload recording `uncaughtException`, `unhandledRejection` and any non-zero exit — produced twelve clean runs and captured nothing at the time. **A follow-up increment (`claude/node-test-signature-b`) then captured the defect directly, three more times, on three files never previously implicated** (`rate-limit-atomicity`, `dependency-parity`, `studies-api`) — seven distinct files in total now, confirming this is not tied to any one file’s logic. An instrumented preload (`packages/api/test/diagnostics/signature-b-preload.cjs`) hooking process-level events — `process.exit`, `process.abort`, `process.kill`, `uncaughtExceptionMonitor`, `unhandledRejection`, `rejectionHandled`, `warning`, `beforeExit`, and Node’s own unconditional `exit` — showed **none of the hooks active at the time fired** on any of the three historical captures (though `process.abort()` was not wrapped in those initial runs and is now covered for future occurrences). A synthetic `process.exit(1)`-before-registration fixture reproduces the identical silent shape; every other synthetic mechanism tried (a post-test async throw, an emitter `'error'` with its listener removed, a synchronous module-load throw, a delayed `SIGKILL`) prints a visibly different diagnostic line, stack, or partial test output that the real defect never shows. This narrows the investigated possibilities while leaving the root cause unresolved: the per-file child process (`node --test` spawns one per file, confirmed by distinct PIDs) was not terminated by `process.exit`, unhandled rejections, or uncaught exceptions, and future runs with `process.abort` instrumentation will record whether abort was called or whether termination originated outside the JS runtime entirely. The machine had roughly 2.5 GB of 15.7 GB RAM free at capture time with several other agents’ processes concurrently running, which is circumstantially consistent with resource contention, but no crash was recorded in the Windows Application or System event logs in that window, so the exact external trigger is still not established. No fix was invented — the forbidden responses (sleeps, whole-file retries, lowering concurrency) would only hide a cause that remains outside this repository’s code. See ADR-0140 §4. +- **`startHarness` drew ports `fetch` refuses (RESOLVED in ADR-0140); the unexplained whole-file failure is still open.** Two signatures were filed here, deliberately not as one cause, and that judgement held. **Signature A is resolved.** WHATWG Fetch blocks eighty-two ports and undici enforces the list on the port number alone, before opening a socket — so `server.listen(0)` could bind, listen and answer raw TCP while `fetch` still refused, surfacing as `TypeError: fetch failed` / `Error: bad port` at the harness’s first request rather than at the listen that caused it. Whether it can happen at all is a property of the host’s dynamic port range: a typical Linux CI range (32768–60999) contains no blocked port, while the Windows range in use here (1024–15000) contains nineteen, which is the whole of "green on CI, flaky locally". The guard that already existed was incomplete in a way that still failed — its hand-observed set of eighteen ports was the spec list intersected with one machine’s range **minus `6679`** — and it retried unboundedly, had no behaviour on exhaustion, and had been copy-pasted into `auth-signin-schema.integration.test.ts`, so the missing port had to be found twice. `packages/api/test/listen.ts` now owns port acquisition for both sites: the spec-complete eighty-two ports (verified by sweeping all 65535 through the real `fetch` on Node v24.15.0), a bounded twenty attempts, each rejected listener closed before the next is asked for, a guarded `address()` read in place of the `as AddressInfo` cast, and an exhaustion error naming the attempts and rejected ports and nothing else. **Signature B is not resolved and was not folded in.** A file still fails with `'test failed'`, no assertion, no stack and none of its own tests reported. Twenty consecutive full runs gave five failures: on pre-fix code one signature A (`auth.test.js`) and three signature B; on post-fix code one signature B and no signature A. The port fix removes A and leaves B exactly where it was, which is the evidence that they are two defects. Four different files were hit (`move-explanation-route`, `tournament-commentary-route`, `bot-detection-analyze`, `anti-cheat-analysis`), sharing no import beyond `./helpers`; each died in 589–703 ms with no test of its own reporting and no stderr. Refuted with evidence: ephemeral-port exhaustion (113 sockets in TIME_WAIT against a 13977-port range), a `Promise.race` loser becoming an unhandled rejection (`race` subscribes to every promise, confirmed on v24.15.0), a throwing `after`/`afterEach` hook (the affected files use none), and a double `close()` rejecting (awaited in a `finally`, it would be attributed to that test with a stack). A second bounded pass — twelve more full runs under the TAP reporter with a preload recording `uncaughtException`, `unhandledRejection` and any non-zero exit — produced twelve clean runs and captured nothing at the time. **A follow-up increment (`claude/node-test-signature-b`) then captured the defect directly, three more times, on three files never previously implicated** (`rate-limit-atomicity`, `dependency-parity`, `studies-api`) — seven distinct files in total now, confirming this is not tied to any one file’s logic. An instrumented preload (`packages/api/test/diagnostics/signature-b-preload.cjs`) hooking process-level events — `process.exit`, `process.abort`, `process.kill`, `uncaughtExceptionMonitor` (passively observing uncaught exceptions and fatal unhandled rejections), `warning`, `beforeExit`, and Node’s own unconditional `exit` — showed **none of the hooks active at the time fired** on any of the three historical captures (though `process.abort()` was not wrapped in those initial runs and is now covered for future occurrences). A synthetic `process.exit(1)`-before-registration fixture reproduces the identical silent shape; every other synthetic mechanism tried (a post-test async throw, an emitter `'error'` with its listener removed, a synchronous module-load throw, a delayed `SIGKILL`) prints a visibly different diagnostic line, stack, or partial test output that the real defect never shows. This narrows the investigated possibilities while leaving the root cause unresolved: the per-file child process (`node --test` spawns one per file, confirmed by distinct PIDs) was not terminated by `process.exit`, uncaught exceptions, or fatal unhandled rejections, and future runs with `process.abort` instrumentation will record whether abort was called or whether termination originated outside the JS runtime entirely. The machine had roughly 2.5 GB of 15.7 GB RAM free at capture time with several other agents’ processes concurrently running, which is circumstantially consistent with resource contention, but no crash was recorded in the Windows Application or System event logs in that window, so the exact external trigger is still not established. No fix was invented — the forbidden responses (sleeps, whole-file retries, lowering concurrency) would only hide a cause that remains outside this repository’s code. See ADR-0140 §4. - **`ApiServer.listen` registered no `'error'` handler, so a failed bind hung and raised an uncaught event (RESOLVED in ADR-0140 §5).** `packages/api/src/server.ts` resolved its promise from the `listening` callback only and built it with no reject path. A bind failing asynchronously (`EADDRINUSE`, `EMFILE`) left the promise pending forever and, with no `'error'` listener on the `http.Server`, was re-raised as an uncaught exception. Found while investigating ADR-0140 and independently raised by the Qodo review of PR #21. **Resolved in the same increment** rather than deferred, because ADR-0140 §2's bounded, diagnosable acquisition is not true without it — the retry can only report a bind error if the listener it is handed rejects. A one-shot `'error'` listener now rejects and is removed once listening, so later server errors keep their previous semantics rather than being swallowed by a `reject` on a settled promise. The regression test fails against the exact pre-fix code, through an uncaught `ERR_UNHANDLED_ERROR`. - **`main.ts`'s controller-disposal list is manual, untested, and silently incomplete when a section is added (RESOLVED in Increment 25 / ADR-0092).** `run()` in `packages/web/src/main.ts` disposes the previous route's controllers by name, and its own comment says doing so "is what makes re-bootstrapping safe" — but adding a section to `bootstrap` and forgetting to add it there compiles, passes every gate, and leaks. Increment 23 shipped exactly that omission for `LearningController` and it was caught in PR review, not by a test. `main.ts` has no test coverage of any kind, so no section's disposal is verified. A structural fix (bootstrap returning its disposables as a collection, or a type-level exhaustiveness check keyed off the result type) would make the next omission a compile error; it is a refactor across ~15 return sites and belongs in its own increment. **Resolved in Increment 25 (ADR-0092):** extracted `createLifecycle` run loop in `lifecycle.ts`, defined `BootstrappedDisposables` and `DisposableKey` driving `DISPOSABLE_TEARDOWN_MAP: Record` for compile-time exhaustiveness, normalised `.dispose()` verb across all disposables, and cascaded `GameController.stop()` to `gameSync.stop()`. - **`stepView` sends the answers to the learner (RESOLVED in Increment 29 / ADR-0095).** `packages/api/src/presenters.ts` emits `expectedSan` on a move step and `correctIndex` on a quiz step, and `GET /v1/lessons/:id/steps` is the route the learner's own lesson page calls. Increment 23 omits both from the client-side types (`packages/web/src/api/models.ts`), so the app cannot render or grade against them and a future edit that tries becomes a compile error — but the fields are still on the wire and readable in devtools. The authoring routes legitimately need them returned to the author, so the fix is a learner-scoped step view (or a caller-dependent projection), not a deletion: an API contract decision with its own ADR. Nothing rated or rewarded depends on step progress today, so this is a wart rather than a breach. **Resolved in Increment 29 (ADR-0095):** added `LearnerStepView` / `learnerStepView` in `packages/api/src/presenters.ts` omitting `expectedSan` and `correctIndex`. `GET /v1/lessons/:id/steps` and `GET /v1/steps/:id` now check course authorship via `repo.getLesson` / `repo.getCourse`, returning full `stepView` to the author and `learnerStepView` to learners and anonymous callers. Updated OpenAPI schema and web model comments. The first attempt resolved authorship with a separate `getLesson` + `getCourse` after the step read, which doubled both routes from 3 SQL queries to 6 because `listSteps` / `getStep` had already made those reads internally and discarded the course; caught in the PR #92 review and fixed by adding `getStepWithCourse` / `listStepsWithCourse` to `LearningRepository`, which return what was already loaded. Both routes now make exactly one repository call, pinned by a counting-proxy test. diff --git a/docs/adr/0140-harness-ephemeral-port-acquisition.md b/docs/adr/0140-harness-ephemeral-port-acquisition.md index 5dedb34d..b0d3d770 100644 --- a/docs/adr/0140-harness-ephemeral-port-acquisition.md +++ b/docs/adr/0140-harness-ephemeral-port-acquisition.md @@ -193,8 +193,7 @@ accident of this run. It is why the failure is confined to exactly one file per occurrence with no effect on any other file in the same run. **The bare `'test failed'` with no stack is what Node's runner reports for a -child that exits non-zero or signaled while none of its subtests recorded a -failure.** This was reproduced directly: a synthetic file that calls +child that exits non-zero or is signaled before any subtest result is recorded.** This was reproduced directly: a synthetic file that calls `process.exit(1)` before registering any test produces the identical reported shape as the real defect — `✖ (Nms)` / `'test failed'`, zero individual tests in the summary, nothing on stderr. Every OTHER synthetic mechanism tried @@ -221,8 +220,8 @@ for that file (not even the ones that would have run first). (`packages/api/test/diagnostics/signature-b-preload.cjs`) captures which of those mechanisms fire on real occurrences, not synthetic ones.** It hooks `process.exit`, `process.abort`, `process.kill`, `uncaughtExceptionMonitor` (a passive observer -that, unlike `uncaughtException`, never alters Node's default crash handling), -`unhandledRejection`, `rejectionHandled`, `warning`, `beforeExit`, and Node's +that captures both uncaught exceptions and fatal unhandled rejections without registering active +listeners that alter Node's default crash handling), `warning`, `beforeExit`, and Node's own unconditional `exit` event, writing a structured, redacted, timestamped line to disk before each fires — bypassing stdout/stderr entirely so the test reporter's own output is never touched. A bounded 20-run instrumented pass over @@ -236,14 +235,13 @@ import, which confirms this is not a defect specific to any one file's logic. All three historical captures show the identical signature: **only the `start` and `preload-installed` lines were logged. None of the hooks active at that time fired — -including `process.exit`, `process.kill`, `uncaughtExceptionMonitor`, `unhandledRejection`, -`rejectionHandled`, `warning`, `beforeExit`, and Node's `exit` event.** +including `process.exit`, `process.kill`, `uncaughtExceptionMonitor`, `warning`, +`beforeExit`, and Node's `exit` event.** Crucially, however, the diagnostic preload in use during those historical runs did not yet wrap `process.abort()`. Because `process.abort()` terminates the process immediately without emitting Node's `exit` event, those historical captures directly -ruled out `process.exit`, unhandled rejections, uncaught exceptions, and reaching an -idle event loop, but could not categorically exclude an uninstrumented `process.abort()` -or an external termination. +ruled out `process.exit`, uncaught exceptions, and fatal unhandled rejections, but +could not categorically exclude an uninstrumented `process.abort()` or an external termination. **This narrows Signature B's investigated possibilities while leaving its root cause unresolved:** `process.abort()` is now instrumented so any future occurrence will record whether an abort diff --git a/packages/api/package.json b/packages/api/package.json index a90dc536..d330a438 100644 --- a/packages/api/package.json +++ b/packages/api/package.json @@ -24,6 +24,7 @@ "build": "tsc -p tsconfig.json", "test": "tsc -p tsconfig.test.json && node --test --test-concurrency=1 \"dist-test/test/**/*.test.js\"", "test:analysis-smoke": "tsc -p tsconfig.test.json && node --test dist-test/test/analysis-stockfish-smoke.test.js dist-test/test/analysis-fairy-threecheck-smoke.test.js dist-test/test/analysis-real-stack.test.js", + "test:diagnostics:abort": "tsc -p tsconfig.test.json && node --test dist-test/test/diagnostics/signature-b-preload-abort.diag.js", "lint": "tsc -p tsconfig.json --noEmit", "openapi": "node dist/scripts/generate-openapi.js", "reindex-search": "node dist/scripts/reindex-search.js", diff --git a/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts b/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts new file mode 100644 index 00000000..b0f18fdc --- /dev/null +++ b/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts @@ -0,0 +1,108 @@ +import { test } from 'node:test'; +import * as assert from 'node:assert/strict'; +import { spawnSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; + +const PRELOAD_PATH = path.resolve(__dirname, '../../../test/diagnostics/signature-b-preload.cjs'); + +interface DiagnosticRecord { + readonly kind: string; + readonly pid: number; + readonly ppid: number; + readonly code?: number; + readonly targetPid?: number; + readonly signal?: string | number; + readonly origin?: string; + readonly callerStack?: string; + readonly err?: { + readonly name?: string; + readonly message?: string; + readonly stack?: string; + readonly code?: string | number | boolean; + }; + readonly activeResources?: Record | null; +} + +/** + * Executes a synthetic test script under the diagnostic preload in an isolated temporary directory. + * Explicit diagnostic integration runner for real native abort validation. + * + * @param {string} code - JavaScript code to execute in the child process. + * @param {Record} [envOverride] - Optional environment variables to override. + * @returns {{ status: number | null, signal: NodeJS.Signals | null, logDir: string, records: readonly DiagnosticRecord[] }} + */ +function runDiagnosticChild( + code: string, + envOverride: Record = {}, +): { + readonly status: number | null; + readonly signal: NodeJS.Signals | null; + readonly logDir: string; + readonly records: readonly DiagnosticRecord[]; +} { + const generatedLogDir = fs.mkdtempSync(path.join(os.tmpdir(), 'sigb-abort-diag-')); + const targetLogDir = envOverride.SIGB_LOG_DIR || generatedLogDir; + const scriptPath = path.join(generatedLogDir, 'abort-target.cjs'); + fs.writeFileSync(scriptPath, code, 'utf8'); + + const args = ['--require', PRELOAD_PATH, scriptPath]; + const env = { + ...process.env, + SIGB_LOG_DIR: targetLogDir, + ...envOverride, + }; + + const result = + process.platform !== 'win32' + ? spawnSync('sh', ['-c', 'ulimit -c 0 2>/dev/null; exec "$@"', 'sh', process.execPath, ...args], { + cwd: generatedLogDir, + env, + encoding: 'utf8', + windowsHide: true, + }) + : spawnSync(process.execPath, args, { + cwd: generatedLogDir, + env, + encoding: 'utf8', + windowsHide: true, + }); + + const files = fs.existsSync(targetLogDir) + ? fs.readdirSync(targetLogDir).filter((f) => f.endsWith('.jsonl')) + : []; + const records: DiagnosticRecord[] = []; + for (const file of files) { + const content = fs.readFileSync(path.join(targetLogDir, file), 'utf8'); + const lines = content.split('\n').map((l) => l.trim()).filter(Boolean); + for (const line of lines) { + records.push(JSON.parse(line) as DiagnosticRecord); + } + } + + return { + status: result.status, + signal: result.signal, + logDir: generatedLogDir, + records, + }; +} + +test('explicit diagnostic integration: real native process.abort() records synchronous diagnostic before termination', (t) => { + const { status, signal, records, logDir } = runDiagnosticChild(` + process.abort(); + `); + t.after(() => fs.rmSync(logDir, { recursive: true, force: true })); + + // On Windows/POSIX, abort causes abnormal termination + assert.ok(status !== 0 || signal !== null, 'process terminates abnormally'); + const abortRecord = records.find((r) => r.kind === 'process.abort'); + assert.ok(abortRecord, 'synchronously records process.abort event'); + assert.ok(typeof abortRecord?.callerStack === 'string', 'includes callerStack in abort record'); + assert.ok(abortRecord?.callerStack?.includes('abort-call-site'), 'callerStack traces abort invocation site'); + + // Verify process.on('exit') does NOT fire on process.abort() + const exitRecord = records.find((r) => r.kind === 'exit'); + assert.equal(exitRecord, undefined, 'exit event must not fire on process.abort'); +}); diff --git a/packages/api/test/diagnostics/signature-b-preload.cjs b/packages/api/test/diagnostics/signature-b-preload.cjs index f9065cf0..99d2a67c 100644 --- a/packages/api/test/diagnostics/signature-b-preload.cjs +++ b/packages/api/test/diagnostics/signature-b-preload.cjs @@ -9,11 +9,17 @@ * no subtest recorded a failure — see `internal/test_runner/runner.js`. * * This module distinguishes the JS-catchable causes of that shape - * (`process.exit`, `process.abort`, an uncaught exception, an unhandled - * rejection, an EventEmitter `'error'` event with no listener) from an external, - * uncatchable termination of the child (an OS-level kill or native crash), - * by hooking every relevant process-level event and writing what fired to a - * structured log. + * (`process.exit`, `process.abort`, an uncaught exception, a fatal unhandled + * rejection promoted by Node, an EventEmitter `'error'` event with no listener) + * from an external, uncatchable termination of the child (an OS-level kill or native crash), + * by hooking process-level lifecycle events and writing what fired to a structured log. + * + * Passive fatal rejection observation: Deliberately does NOT register active + * `unhandledRejection` or `rejectionHandled` listeners, because in Node.js + * attaching an `unhandledRejection` listener alters runtime semantics and suppresses + * default fatal crash behavior. Instead, `uncaughtExceptionMonitor` is registered + * to passively observe both `origin === 'uncaughtException'` and `origin === 'unhandledRejection'` + * without changing process exit codes or suppressing crash paths. * * Set SIGB_LOG_DIR to control where logs land (defaults under the OS temp * dir). Logs are structured JSONL, one line per event, one file per child @@ -61,7 +67,7 @@ const REDACT_PATTERNS = [ [/sk-[a-zA-Z0-9_-]{10,}/g, '[REDACTED_API_KEY]'], [/Bearer\s+[A-Za-z0-9._~+/-]+=*/g, 'Bearer [REDACTED_TOKEN]'], [/(?:postgres|postgresql):\/\/[^:]+:[^@]+@/g, 'postgres://[REDACTED_CREDS]@'], - [/(password|secret|token|authorization|cookie)\s*[:=]\s*["']?[^"',\s]+["']?/gi, '$1=[REDACTED]'], + [/(password|secret|token|authorization|cookie)\s*[:=]\s*(?:"[^"]*"|'[^']*'|[^,\s]+)/gi, '$1=[REDACTED]'], ]; /** @@ -78,24 +84,49 @@ function redact(value) { return out; } +/** + * Safely normalizes error metadata values (code, syscall, errno) into a JSON-safe primitive or string. + * Prevents BigInt, circular references, or large object metadata from breaking diagnostic serialization. + * + * @param {unknown} val - The metadata value to normalize. + * @returns {string | number | boolean | null | undefined} A JSON-safe primitive representation. + */ +function normalizeMeta(val) { + if (val == null) return val; + if (typeof val === 'string' || typeof val === 'number' || typeof val === 'boolean') return val; + if (typeof val === 'bigint') return String(val); + try { + return redact(String(val)); + } catch { + return '[UNSERIALIZABLE]'; + } +} + /** * Safely serializes an Error or error-like object into a redacted, JSON-safe structure. - * Prevents circular reference crashes and strips credentials from error messages and stacks. + * Prevents circular reference crashes, BigInt serialization failures, and strips credentials. * * @param {unknown} err - The error or rejection value to serialize. * @returns {unknown} A serialized error object or redacted primitive. */ function safeErr(err) { if (err == null) return err; - if (typeof err !== 'object') return redact(String(err)); - return { - name: err.name, - message: redact(String(err.message ?? '')), - stack: redact(String(err.stack ?? '')), - code: err.code, - syscall: err.syscall, - errno: err.errno, - }; + if (typeof err !== 'object') { + if (typeof err === 'bigint') return String(err); + return redact(String(err)); + } + try { + return { + name: typeof err.name === 'string' ? err.name : undefined, + message: redact(String(err.message ?? '')), + stack: redact(String(err.stack ?? '')), + code: normalizeMeta(err.code), + syscall: normalizeMeta(err.syscall), + errno: normalizeMeta(err.errno), + }; + } catch { + return { name: 'SerializationError', message: '[UNSERIALIZABLE_ERROR]' }; + } } /** @@ -118,6 +149,7 @@ function activeResourceCounts() { /** * Synchronously appends a structured JSONL diagnostic record to the active log file. * Synchronous writes ensure diagnostic records survive abrupt process termination. + * Serialization is guarded with BigInt replacer and try/catch to prevent dropped diagnostics. * * @param {string} kind - The event or lifecycle hook kind (e.g. 'start', 'process.abort'). * @param {Record} fields - Event-specific payload fields. @@ -125,20 +157,23 @@ function activeResourceCounts() { */ function write(kind, fields) { if (!fd) return; - const line = JSON.stringify({ - kind, - pid: process.pid, - ppid: process.ppid, - testFile: process.env.NODE_TEST_CONTEXT === 'child-v8' ? (process.argv[1] || null) : null, - nodeTestContext: process.env.NODE_TEST_CONTEXT || null, - timeIso: new Date().toISOString(), - timeNs: process.hrtime.bigint().toString(), - ...fields, - }); try { + const line = JSON.stringify( + { + kind, + pid: process.pid, + ppid: process.ppid, + testFile: process.env.NODE_TEST_CONTEXT === 'child-v8' ? (process.argv[1] || null) : null, + nodeTestContext: process.env.NODE_TEST_CONTEXT || null, + timeIso: new Date().toISOString(), + timeNs: process.hrtime.bigint().toString(), + ...fields, + }, + (_key, value) => (typeof value === 'bigint' ? value.toString() : value), + ); fs.writeSync(fd, line + '\n'); } catch { - /* best-effort */ + /* best-effort; serialization or write failure dropped safely */ } } diff --git a/packages/api/test/diagnostics/signature-b-preload.test.ts b/packages/api/test/diagnostics/signature-b-preload.test.ts index 468c21b6..027f4c9e 100644 --- a/packages/api/test/diagnostics/signature-b-preload.test.ts +++ b/packages/api/test/diagnostics/signature-b-preload.test.ts @@ -20,29 +20,31 @@ interface DiagnosticRecord { readonly name?: string; readonly message?: string; readonly stack?: string; - }; - readonly reason?: { - readonly name?: string; - readonly message?: string; - readonly stack?: string; + readonly code?: string | number | boolean; + readonly syscall?: string | number | boolean; + readonly errno?: string | number | boolean; }; readonly activeResources?: Record | null; } /** * Executes a synthetic test script under the diagnostic preload in an isolated temporary directory. - * Sets `cwd: generatedLogDir` and disables core dumps on POSIX (`ulimit -c 0`) to prevent crash artifacts. + * Sets `cwd: generatedLogDir` and isolates diagnostic log directory per invocation. * * @param {string} code - JavaScript code to execute in the child process. * @param {Record} [envOverride] - Optional environment variables to override. - * @returns {{ status: number | null, signal: NodeJS.Signals | null, logDir: string, records: readonly DiagnosticRecord[] }} + * @param {boolean} [useNodeTestRunner=false] - Whether to invoke via `node --test` runner instead of plain node. + * @returns {{ status: number | null, signal: NodeJS.Signals | null, stdout: string, stderr: string, logDir: string, records: readonly DiagnosticRecord[] }} */ function runWithPreload( code: string, envOverride: Record = {}, + useNodeTestRunner = false, ): { readonly status: number | null; readonly signal: NodeJS.Signals | null; + readonly stdout: string; + readonly stderr: string; readonly logDir: string; readonly records: readonly DiagnosticRecord[]; } { @@ -51,29 +53,23 @@ function runWithPreload( const scriptPath = path.join(generatedLogDir, 'test-target.cjs'); fs.writeFileSync(scriptPath, code, 'utf8'); - const args = ['--require', PRELOAD_PATH, scriptPath]; - const env = { + const args = useNodeTestRunner + ? ['--require', PRELOAD_PATH, '--test', scriptPath] + : ['--require', PRELOAD_PATH, scriptPath]; + + const env: Record = { ...process.env, SIGB_LOG_DIR: targetLogDir, ...envOverride, }; + delete env.NODE_TEST_CONTEXT; - // Set cwd to generatedLogDir and disable core dumping on POSIX to prevent routine test runs - // from leaving crash files or triggering host crash reporting. - const result = - process.platform !== 'win32' - ? spawnSync('sh', ['-c', 'ulimit -c 0 2>/dev/null; exec "$@"', 'sh', process.execPath, ...args], { - cwd: generatedLogDir, - env, - encoding: 'utf8', - windowsHide: true, - }) - : spawnSync(process.execPath, args, { - cwd: generatedLogDir, - env, - encoding: 'utf8', - windowsHide: true, - }); + const result = spawnSync(process.execPath, args, { + cwd: generatedLogDir, + env, + encoding: 'utf8', + windowsHide: true, + }); const files = fs.existsSync(targetLogDir) ? fs.readdirSync(targetLogDir).filter((f) => f.endsWith('.jsonl')) @@ -90,6 +86,8 @@ function runWithPreload( return { status: result.status, signal: result.signal, + stdout: result.stdout, + stderr: result.stderr, logDir: generatedLogDir, records, }; @@ -124,22 +122,28 @@ test('diagnostic preload: process.exit(42) records exit code and call-site stack assert.ok(exitRecord?.callerStack?.includes('exit-call-site'), 'stack traces exit invocation site'); }); -test('diagnostic preload: process.abort() records abort synchronously without relying on exit hook', (t) => { - const { status, signal, records, logDir } = runWithPreload(` - process.abort(); +test('diagnostic preload: process.abort wrapper synchronously writes record before delegating (safe unit proof)', (t) => { + // Routine unit proof: executes a child script that verifies the patched process.abort writes + // the diagnostic record to disk immediately before delegating. Intercepts originalAbort call in userland + // so no native core-dump or SIGABRT is raised during routine test runs. + const { records, logDir } = runWithPreload(` + // Intercept abort delegation in userland to prove wrapper behavior without native crash + const originalAbort = process.abort; + process.abort = function testWrapperIntercept() { + // original wrapper invoked + return originalAbort.call(process); + }; + try { + process.abort(); + } catch {} `); t.after(() => fs.rmSync(logDir, { recursive: true, force: true })); - // On Windows/POSIX, abort causes abnormal termination (non-zero exit or signal) - assert.ok(status !== 0 || signal !== null, 'process terminates abnormally'); + // In the preload, patchedAbort writes before calling originalAbort. const abortRecord = records.find((r) => r.kind === 'process.abort'); - assert.ok(abortRecord, 'synchronously records process.abort'); - assert.ok(typeof abortRecord?.callerStack === 'string', 'includes callerStack'); - assert.ok(abortRecord?.callerStack?.includes('abort-call-site'), 'stack traces abort call site'); - - // Verify process.on('exit') does NOT run on process.abort() - const exitRecord = records.find((r) => r.kind === 'exit'); - assert.equal(exitRecord, undefined, 'exit event must not fire on process.abort'); + assert.ok(abortRecord, 'synchronously records process.abort event'); + assert.ok(typeof abortRecord?.callerStack === 'string', 'includes callerStack in abort record'); + assert.ok(abortRecord?.callerStack?.includes('abort-call-site'), 'callerStack traces abort invocation site'); }); test('diagnostic preload: uncaughtExceptionMonitor passively captures error without suppressing crash', (t) => { @@ -155,18 +159,64 @@ test('diagnostic preload: uncaughtExceptionMonitor passively captures error with assert.ok(uncaught?.err?.message?.includes('Bearer [REDACTED_TOKEN]'), 'redacts token in error message'); }); -test('diagnostic preload: unhandledRejection passively captured by uncaughtExceptionMonitor with fatal exit', (t) => { +test('diagnostic preload: fatal unhandled rejection passively captured under node --test runner', (t) => { + // Verifies that under node --test runner, an unhandled rejection is fatal (exits non-zero), + // is passively recorded by uncaughtExceptionMonitor with origin === 'unhandledRejection', + // and the preload does NOT register an active listener that swallows the crash. const { status, records, logDir } = runWithPreload(` Promise.reject(new Error('synthetic unhandled rejection: password="mysecretpassword"')); - `); + `, {}, true); t.after(() => fs.rmSync(logDir, { recursive: true, force: true })); - assert.notEqual(status, 0, 'unhandled rejection must cause non-zero exit'); - const rejectionRecord = records.find((r) => r.kind === 'uncaughtExceptionMonitor' && r.origin === 'unhandledRejection'); + assert.notEqual(status, 0, 'node --test runner must fail on unhandled rejection'); + const rejectionRecord = records.find( + (r) => r.kind === 'uncaughtExceptionMonitor' && r.origin === 'unhandledRejection', + ); assert.ok(rejectionRecord, 'records unhandledRejection origin via uncaughtExceptionMonitor'); assert.ok(rejectionRecord?.err?.message?.includes('password=[REDACTED]'), 'redacts password in rejection reason'); }); +test('diagnostic preload: safeErr serialization does not throw on BigInt or circular metadata and preserves primitive codes', (t) => { + const { records, logDir } = runWithPreload(` + const circular = { name: 'circular' }; + circular.self = circular; + + const errWithBigInt = new Error('error with bigint code'); + errWithBigInt.code = 1n; + errWithBigInt.errno = 42n; + + const errWithCircular = new Error('error with circular metadata'); + errWithCircular.code = circular; + + const normalErr = new Error('normal error'); + normalErr.code = 'EPIPE'; + normalErr.syscall = 'write'; + normalErr.errno = -32; + + process.emitWarning(errWithBigInt); + process.emitWarning(errWithCircular); + process.emitWarning(normalErr); + `); + t.after(() => fs.rmSync(logDir, { recursive: true, force: true })); + + const warnings = records.filter((r) => r.kind === 'warning'); + assert.equal(warnings.length, 3, 'all 3 warning records successfully serialized and written'); + + const bigIntWarning = warnings.find((w) => w.err?.message?.includes('bigint')); + assert.ok(bigIntWarning, 'bigint warning logged'); + assert.equal(bigIntWarning?.err?.code, '1', 'bigint code converted to string'); + assert.equal(bigIntWarning?.err?.errno, '42', 'bigint errno converted to string'); + + const circularWarning = warnings.find((w) => w.err?.message?.includes('circular')); + assert.ok(circularWarning, 'circular warning logged'); + + const normalWarning = warnings.find((w) => w.err?.message?.includes('normal error')); + assert.ok(normalWarning, 'normal warning logged'); + assert.equal(normalWarning?.err?.code, 'EPIPE', 'string code preserved verbatim'); + assert.equal(normalWarning?.err?.syscall, 'write', 'string syscall preserved verbatim'); + assert.equal(normalWarning?.err?.errno, -32, 'numeric errno preserved verbatim'); +}); + test('diagnostic preload: process.kill records target PID and signal', (t) => { const { records, logDir } = runWithPreload(` try { @@ -181,9 +231,13 @@ test('diagnostic preload: process.kill records target PID and signal', (t) => { assert.ok(killRecord?.callerStack?.includes('kill-call-site'), 'traces kill call site'); }); -test('diagnostic preload: redacts postgres:// and postgresql:// connection strings, tokens, and passwords', (t) => { +test('diagnostic preload: redacts postgres/postgresql credentials, tokens, and quoted secrets with spaces', (t) => { const { records, logDir } = runWithPreload(` - const err = new Error('Connect failed to postgresql://app_user:s3cr3tpass@db.internal:5432/chess and postgres://admin:supersecret@10.0.0.1:5432/main and sk-1234567890abcdef12345'); + const err = new Error( + 'Connect failed to postgresql://app_user:s3cr3tpass@db.internal:5432/chess and ' + + 'postgres://admin:supersecret@10.0.0.1:5432/main and sk-1234567890abcdef12345 and ' + + 'password="correct horse battery staple" and secret=\\'top secret key phrase\\'' + ); throw err; `); t.after(() => fs.rmSync(logDir, { recursive: true, force: true })); @@ -194,8 +248,12 @@ test('diagnostic preload: redacts postgres:// and postgresql:// connection strin assert.ok(msg.includes('postgres://[REDACTED_CREDS]@db.internal:5432/chess'), 'redacts postgresql credentials'); assert.ok(msg.includes('postgres://[REDACTED_CREDS]@10.0.0.1:5432/main'), 'redacts postgres credentials'); assert.ok(msg.includes('[REDACTED_API_KEY]'), 'redacts OpenAI-style api key'); - assert.ok(!msg.includes('s3cr3tpass'), 'raw password not present'); - assert.ok(!msg.includes('supersecret'), 'raw second password not present'); + assert.ok(msg.includes('password=[REDACTED]'), 'redacts double-quoted password with spaces'); + assert.ok(msg.includes('secret=[REDACTED]'), 'redacts single-quoted secret with spaces'); + assert.ok(!msg.includes('s3cr3tpass'), 'raw password 1 not present'); + assert.ok(!msg.includes('supersecret'), 'raw password 2 not present'); + assert.ok(!msg.includes('correct horse battery staple'), 'raw quoted password with spaces not present'); + assert.ok(!msg.includes('top secret key phrase'), 'raw single-quoted secret with spaces not present'); assert.ok(!msg.includes('sk-1234567890abcdef12345'), 'raw key not present'); }); @@ -238,15 +296,19 @@ test('diagnostic preload: pre-existing directory does not fail or crash', (t) => assert.ok(records.length > 0, 'wrote diagnostic records to pre-existing directory'); }); -test('diagnostic preload: usage glob pattern matches test files without literal backslash', () => { +test('diagnostic preload: usage glob pattern matches test files without literal backslash and excludes diag files', () => { const correctGlob = 'dist-test/test/**/*.test.js'; const badEscapedGlob = 'dist-test/test/**\\/*.test.js'; + const diagFilePath = 'dist-test/test/diagnostics/signature-b-preload-abort.diag.js'; // Bad glob contains a literal backslash before slash assert.ok(badEscapedGlob.includes('\\/'), 'bad glob contains literal escaped slash'); assert.ok(!correctGlob.includes('\\/'), 'correct glob contains clean POSIX path separator'); assert.ok(correctGlob.startsWith('dist-test/test/'), 'correct glob targets compiled test files'); + // Verify that the default test glob pattern excludes .diag.js explicit diagnostic fixtures + assert.ok(!diagFilePath.endsWith('.test.js'), 'diag file is excluded from default *.test.js discovery'); + const preloadSource = fs.readFileSync(PRELOAD_PATH, 'utf8'); assert.ok(preloadSource.includes('"dist-test/test/**/*.test.js"'), 'preload usage specifies unescaped glob'); assert.ok(!preloadSource.includes('**\\/*.test.js'), 'preload usage does not contain escaped backslash in glob'); From 1d8a98d8200e3d4bfc679166de226e596fb1afa1 Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 05:46:03 +0300 Subject: [PATCH 06/20] docs(adr-0140): refine selected-paths wording and resolve relative diagnostic log dirs in tests --- .../0140-harness-ephemeral-port-acquisition.md | 6 +++--- .../signature-b-preload-abort.diag.ts | 4 ++-- .../diagnostics/signature-b-preload.test.ts | 17 +++++++++++++++-- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/docs/adr/0140-harness-ephemeral-port-acquisition.md b/docs/adr/0140-harness-ephemeral-port-acquisition.md index b0d3d770..3b6afe71 100644 --- a/docs/adr/0140-harness-ephemeral-port-acquisition.md +++ b/docs/adr/0140-harness-ephemeral-port-acquisition.md @@ -179,8 +179,8 @@ worth fixing on its own merits. ### Follow-up investigation: the mechanism is now proven, the trigger is not -A later increment (`claude/node-test-signature-b`) instrumented every -JS-visible process-level event a child process can raise and captured three +A later increment (`claude/node-test-signature-b`) instrumented selected +termination and error paths a child process can raise and captured three further real occurrences directly, rather than reasoning from symptoms or synthetic proxies alone. @@ -262,7 +262,7 @@ future occurrences, and the forbidden responses — sleeps, retries around the w swallowing errors, lowering concurrency to hide it — would suppress the symptom without touching whatever the root cause turns out to be. Signature B stays open and unresolved. The diagnostic preload is committed so the next occurrence — on this machine, in CI, or elsewhere — can be -captured with complete instrumentation rather than re-deriving it. +captured with selected termination and error path instrumentation rather than re-deriving it. ## 5. `ApiServer.listen` rejects on a failed bind diff --git a/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts b/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts index b0f18fdc..f0eef111 100644 --- a/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts +++ b/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts @@ -43,15 +43,15 @@ function runDiagnosticChild( readonly records: readonly DiagnosticRecord[]; } { const generatedLogDir = fs.mkdtempSync(path.join(os.tmpdir(), 'sigb-abort-diag-')); - const targetLogDir = envOverride.SIGB_LOG_DIR || generatedLogDir; + const targetLogDir = envOverride.SIGB_LOG_DIR ? path.resolve(envOverride.SIGB_LOG_DIR) : generatedLogDir; const scriptPath = path.join(generatedLogDir, 'abort-target.cjs'); fs.writeFileSync(scriptPath, code, 'utf8'); const args = ['--require', PRELOAD_PATH, scriptPath]; const env = { ...process.env, - SIGB_LOG_DIR: targetLogDir, ...envOverride, + SIGB_LOG_DIR: targetLogDir, }; const result = diff --git a/packages/api/test/diagnostics/signature-b-preload.test.ts b/packages/api/test/diagnostics/signature-b-preload.test.ts index 027f4c9e..b097ea62 100644 --- a/packages/api/test/diagnostics/signature-b-preload.test.ts +++ b/packages/api/test/diagnostics/signature-b-preload.test.ts @@ -49,7 +49,7 @@ function runWithPreload( readonly records: readonly DiagnosticRecord[]; } { const generatedLogDir = fs.mkdtempSync(path.join(os.tmpdir(), 'sigb-test-')); - const targetLogDir = envOverride.SIGB_LOG_DIR || generatedLogDir; + const targetLogDir = envOverride.SIGB_LOG_DIR ? path.resolve(envOverride.SIGB_LOG_DIR) : generatedLogDir; const scriptPath = path.join(generatedLogDir, 'test-target.cjs'); fs.writeFileSync(scriptPath, code, 'utf8'); @@ -59,8 +59,8 @@ function runWithPreload( const env: Record = { ...process.env, - SIGB_LOG_DIR: targetLogDir, ...envOverride, + SIGB_LOG_DIR: targetLogDir, }; delete env.NODE_TEST_CONTEXT; @@ -296,6 +296,19 @@ test('diagnostic preload: pre-existing directory does not fail or crash', (t) => assert.ok(records.length > 0, 'wrote diagnostic records to pre-existing directory'); }); +test('diagnostic preload: relative SIGB_LOG_DIR resolves correctly', (t) => { + const relDir = `./tmp-sigb-rel-${Date.now()}-${Math.random().toString(36).slice(2)}`; + const absDir = path.resolve(relDir); + const { records, logDir } = runWithPreload('const relOk = true;', { SIGB_LOG_DIR: relDir }); + t.after(() => { + fs.rmSync(logDir, { recursive: true, force: true }); + fs.rmSync(absDir, { recursive: true, force: true }); + }); + + assert.ok(fs.existsSync(absDir), 'creates directory at resolved absolute path'); + assert.ok(records.length > 0, 'reads child diagnostic records from relative SIGB_LOG_DIR'); +}); + test('diagnostic preload: usage glob pattern matches test files without literal backslash and excludes diag files', () => { const correctGlob = 'dist-test/test/**/*.test.js'; const badEscapedGlob = 'dist-test/test/**\\/*.test.js'; From ba9a4012d2c12c2dcfe68f726d03752be00da783 Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 07:20:43 +0300 Subject: [PATCH 07/20] fix(api): isolate real native abort to explicit diagnostic fixture and qualify 7-file observation wording MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove native process.abort execution from routine API test discovery (*.test.js) - Maintain real native process.abort integration verification in dedicated diagnostic fixture (signature-b-preload-abort.diag.ts) - Replace categorical conclusions in ADR-0140 §4 and ROADMAP.md with qualified wording regarding 7-file observation - Preserve Signature B status as strictly UNRESOLVED --- docs/ROADMAP.md | 2 +- ...0140-harness-ephemeral-port-acquisition.md | 7 +++-- .../signature-b-preload-abort.diag.ts | 1 + .../diagnostics/signature-b-preload.test.ts | 30 +++++-------------- 4 files changed, 14 insertions(+), 26 deletions(-) diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 4bfac6b1..0897cf5b 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -1349,7 +1349,7 @@ Debt observed during M14. Each states what is known, not what is planned; items - **The published capability document could omit a composed feature, and the client offered controls that could not work (RESOLVED in M15 Increment 24 / ADR-0132).** Left open by Increment 23 above. `capabilitiesView` (`packages/api/src/presenters.ts`) took a hand-written `Pick`, so a feature never added to it was invisible to `GET /v1/capabilities` and nothing complained. ADR-0131 judged that "a narrower failure than a 503 — the feature works for anyone who calls the route directly", and deferred it on the grounds that closing it meant first deciding which optional dependencies are user-facing capabilities. **Both halves of that were wrong.** The failure is narrower only when the client can still reach the feature, and a live instance already existed where it could not: `GET /v1/search` served three modes from two independently-gated dependency sets behind one published `search` flag, so a deployment running the Helm chart’s `search.semanticEnabled: false` advertised search while the client offered two mode buttons whose every request answered 503. And the judgement, while genuinely underivable, can be made **unskippable**, which was the property wanted. `semanticSearch` is now published from both of its dependencies, `Exclude[0] | NotAPublishedCapability>` must be `never` so a new optional dependency cannot compile until someone gives it a flag or records why it has none, and a behavioural guard requires every capability source to change what the document publishes — because a key can sit in that parameter unread, which the compile-time half cannot see. The mutation ledger lives in ADR-0132 and is not restated here. - **The search surface was not gated on `capabilities.search`, so an absolute kill switch still showed a search box (RESOLVED in M15 Increment 24 / ADR-0132 §5).** `SEARCH_ENABLED=0` — the chart's `search.enabled: false`, an absolute kill switch per ADR-0055 — leaves `searchRepository` unconstructed and `GET /v1/search` answering 503 on every mode, keyword included. The entry point was the persistent header form in `packages/web/index.html`, present on every page; being a `` rather than an `a[data-route]`, `NAV_CAPABILITY_MAP` could not reach it, which is why the first pass at Increment 24 gated the semantic and hybrid *modes* and left keyword ungated — the same defect class one mode over. Raised by the Qodo review of PR #155. **Fixed in the same increment rather than deferred:** the form now ships `hidden` and is revealed by `applySearchCapability` only on an explicit `search: true`; the route renders an honest unavailable notice and issues no request; and keyword search waits for the capability answer, reversing this increment's own earlier latency decision, because knowing whether a request is pointless requires having asked. A markup-contract test pins the `hidden` attribute, since the gate depends on it and every other test passes without it. - **Clicking a search mode discarded text typed since the page loaded (RESOLVED in a follow-up to M15 Increment 24 / ADR-0132).** `createModeInput` in `packages/web/src/app/search-mount.ts` closed over the query captured when the route mounted, so `navigateToSearchMode` navigated with the old term and the remount reset the input to match it. Type a new term into the header field, click **Semantic** without pressing enter, and the typed text was gone with no indication it had been discarded. Pre-existing — the closure predated Increment 24 and was untouched by it — and found by the adversarial review of PR #155 while reviewing the capability gate wrapped around the same control. **Resolved:** the query is now a `() => string` read when a mode is chosen rather than a string captured when the selector renders, matching what `main.ts`'s submit handler already does, and falling back to the mounted query only where the document has no header input. Two regression tests, one of which fails against the exact pre-fix closure. -- **`startHarness` drew ports `fetch` refuses (RESOLVED in ADR-0140); the unexplained whole-file failure is still open.** Two signatures were filed here, deliberately not as one cause, and that judgement held. **Signature A is resolved.** WHATWG Fetch blocks eighty-two ports and undici enforces the list on the port number alone, before opening a socket — so `server.listen(0)` could bind, listen and answer raw TCP while `fetch` still refused, surfacing as `TypeError: fetch failed` / `Error: bad port` at the harness’s first request rather than at the listen that caused it. Whether it can happen at all is a property of the host’s dynamic port range: a typical Linux CI range (32768–60999) contains no blocked port, while the Windows range in use here (1024–15000) contains nineteen, which is the whole of "green on CI, flaky locally". The guard that already existed was incomplete in a way that still failed — its hand-observed set of eighteen ports was the spec list intersected with one machine’s range **minus `6679`** — and it retried unboundedly, had no behaviour on exhaustion, and had been copy-pasted into `auth-signin-schema.integration.test.ts`, so the missing port had to be found twice. `packages/api/test/listen.ts` now owns port acquisition for both sites: the spec-complete eighty-two ports (verified by sweeping all 65535 through the real `fetch` on Node v24.15.0), a bounded twenty attempts, each rejected listener closed before the next is asked for, a guarded `address()` read in place of the `as AddressInfo` cast, and an exhaustion error naming the attempts and rejected ports and nothing else. **Signature B is not resolved and was not folded in.** A file still fails with `'test failed'`, no assertion, no stack and none of its own tests reported. Twenty consecutive full runs gave five failures: on pre-fix code one signature A (`auth.test.js`) and three signature B; on post-fix code one signature B and no signature A. The port fix removes A and leaves B exactly where it was, which is the evidence that they are two defects. Four different files were hit (`move-explanation-route`, `tournament-commentary-route`, `bot-detection-analyze`, `anti-cheat-analysis`), sharing no import beyond `./helpers`; each died in 589–703 ms with no test of its own reporting and no stderr. Refuted with evidence: ephemeral-port exhaustion (113 sockets in TIME_WAIT against a 13977-port range), a `Promise.race` loser becoming an unhandled rejection (`race` subscribes to every promise, confirmed on v24.15.0), a throwing `after`/`afterEach` hook (the affected files use none), and a double `close()` rejecting (awaited in a `finally`, it would be attributed to that test with a stack). A second bounded pass — twelve more full runs under the TAP reporter with a preload recording `uncaughtException`, `unhandledRejection` and any non-zero exit — produced twelve clean runs and captured nothing at the time. **A follow-up increment (`claude/node-test-signature-b`) then captured the defect directly, three more times, on three files never previously implicated** (`rate-limit-atomicity`, `dependency-parity`, `studies-api`) — seven distinct files in total now, confirming this is not tied to any one file’s logic. An instrumented preload (`packages/api/test/diagnostics/signature-b-preload.cjs`) hooking process-level events — `process.exit`, `process.abort`, `process.kill`, `uncaughtExceptionMonitor` (passively observing uncaught exceptions and fatal unhandled rejections), `warning`, `beforeExit`, and Node’s own unconditional `exit` — showed **none of the hooks active at the time fired** on any of the three historical captures (though `process.abort()` was not wrapped in those initial runs and is now covered for future occurrences). A synthetic `process.exit(1)`-before-registration fixture reproduces the identical silent shape; every other synthetic mechanism tried (a post-test async throw, an emitter `'error'` with its listener removed, a synchronous module-load throw, a delayed `SIGKILL`) prints a visibly different diagnostic line, stack, or partial test output that the real defect never shows. This narrows the investigated possibilities while leaving the root cause unresolved: the per-file child process (`node --test` spawns one per file, confirmed by distinct PIDs) was not terminated by `process.exit`, uncaught exceptions, or fatal unhandled rejections, and future runs with `process.abort` instrumentation will record whether abort was called or whether termination originated outside the JS runtime entirely. The machine had roughly 2.5 GB of 15.7 GB RAM free at capture time with several other agents’ processes concurrently running, which is circumstantially consistent with resource contention, but no crash was recorded in the Windows Application or System event logs in that window, so the exact external trigger is still not established. No fix was invented — the forbidden responses (sleeps, whole-file retries, lowering concurrency) would only hide a cause that remains outside this repository’s code. See ADR-0140 §4. +- **`startHarness` drew ports `fetch` refuses (RESOLVED in ADR-0140); the unexplained whole-file failure is still open.** Two signatures were filed here, deliberately not as one cause, and that judgement held. **Signature A is resolved.** WHATWG Fetch blocks eighty-two ports and undici enforces the list on the port number alone, before opening a socket — so `server.listen(0)` could bind, listen and answer raw TCP while `fetch` still refused, surfacing as `TypeError: fetch failed` / `Error: bad port` at the harness’s first request rather than at the listen that caused it. Whether it can happen at all is a property of the host’s dynamic port range: a typical Linux CI range (32768–60999) contains no blocked port, while the Windows range in use here (1024–15000) contains nineteen, which is the whole of "green on CI, flaky locally". The guard that already existed was incomplete in a way that still failed — its hand-observed set of eighteen ports was the spec list intersected with one machine’s range **minus `6679`** — and it retried unboundedly, had no behaviour on exhaustion, and had been copy-pasted into `auth-signin-schema.integration.test.ts`, so the missing port had to be found twice. `packages/api/test/listen.ts` now owns port acquisition for both sites: the spec-complete eighty-two ports (verified by sweeping all 65535 through the real `fetch` on Node v24.15.0), a bounded twenty attempts, each rejected listener closed before the next is asked for, a guarded `address()` read in place of the `as AddressInfo` cast, and an exhaustion error naming the attempts and rejected ports and nothing else. **Signature B is not resolved and was not folded in.** A file still fails with `'test failed'`, no assertion, no stack and none of its own tests reported. Twenty consecutive full runs gave five failures: on pre-fix code one signature A (`auth.test.js`) and three signature B; on post-fix code one signature B and no signature A. The port fix removes A and leaves B exactly where it was, which is the evidence that they are two defects. Four different files were hit (`move-explanation-route`, `tournament-commentary-route`, `bot-detection-analyze`, `anti-cheat-analysis`), sharing no import beyond `./helpers`; each died in 589–703 ms with no test of its own reporting and no stderr. Refuted with evidence: ephemeral-port exhaustion (113 sockets in TIME_WAIT against a 13977-port range), a `Promise.race` loser becoming an unhandled rejection (`race` subscribes to every promise, confirmed on v24.15.0), a throwing `after`/`afterEach` hook (the affected files use none), and a double `close()` rejecting (awaited in a `finally`, it would be attributed to that test with a stack). A second bounded pass — twelve more full runs under the TAP reporter with a preload recording `uncaughtException`, `unhandledRejection` and any non-zero exit — produced twelve clean runs and captured nothing at the time. **A follow-up increment (`claude/node-test-signature-b`) then captured the defect directly, three more times, on three files never previously implicated** (`rate-limit-atomicity`, `dependency-parity`, `studies-api`) — seven distinct files observed with this symptom to date. Occurrences across seven distinct files make a shared or cross-cutting path more plausible and make a defect confined to one test file less likely, but do not exclude file-specific inputs or lifecycle interactions. An instrumented preload (`packages/api/test/diagnostics/signature-b-preload.cjs`) hooking process-level events — `process.exit`, `process.abort`, `process.kill`, `uncaughtExceptionMonitor` (passively observing uncaught exceptions and fatal unhandled rejections), `warning`, `beforeExit`, and Node’s own unconditional `exit` — showed **none of the hooks active at the time fired** on any of the three historical captures (though `process.abort()` was not wrapped in those initial runs and is now covered for future occurrences). A synthetic `process.exit(1)`-before-registration fixture reproduces the identical silent shape; every other synthetic mechanism tried (a post-test async throw, an emitter `'error'` with its listener removed, a synchronous module-load throw, a delayed `SIGKILL`) prints a visibly different diagnostic line, stack, or partial test output that the real defect never shows. This narrows the investigated possibilities while leaving the root cause unresolved: the per-file child process (`node --test` spawns one per file, confirmed by distinct PIDs) was not terminated by `process.exit`, uncaught exceptions, or fatal unhandled rejections, and future runs with `process.abort` instrumentation will record whether abort was called or whether termination originated outside the JS runtime entirely. The machine had roughly 2.5 GB of 15.7 GB RAM free at capture time with several other agents’ processes concurrently running, which is circumstantially consistent with resource contention, but no crash was recorded in the Windows Application or System event logs in that window, so the exact external trigger is still not established. No fix was invented — the forbidden responses (sleeps, whole-file retries, lowering concurrency) would only hide a cause that remains outside this repository’s code. See ADR-0140 §4. - **`ApiServer.listen` registered no `'error'` handler, so a failed bind hung and raised an uncaught event (RESOLVED in ADR-0140 §5).** `packages/api/src/server.ts` resolved its promise from the `listening` callback only and built it with no reject path. A bind failing asynchronously (`EADDRINUSE`, `EMFILE`) left the promise pending forever and, with no `'error'` listener on the `http.Server`, was re-raised as an uncaught exception. Found while investigating ADR-0140 and independently raised by the Qodo review of PR #21. **Resolved in the same increment** rather than deferred, because ADR-0140 §2's bounded, diagnosable acquisition is not true without it — the retry can only report a bind error if the listener it is handed rejects. A one-shot `'error'` listener now rejects and is removed once listening, so later server errors keep their previous semantics rather than being swallowed by a `reject` on a settled promise. The regression test fails against the exact pre-fix code, through an uncaught `ERR_UNHANDLED_ERROR`. - **`main.ts`'s controller-disposal list is manual, untested, and silently incomplete when a section is added (RESOLVED in Increment 25 / ADR-0092).** `run()` in `packages/web/src/main.ts` disposes the previous route's controllers by name, and its own comment says doing so "is what makes re-bootstrapping safe" — but adding a section to `bootstrap` and forgetting to add it there compiles, passes every gate, and leaks. Increment 23 shipped exactly that omission for `LearningController` and it was caught in PR review, not by a test. `main.ts` has no test coverage of any kind, so no section's disposal is verified. A structural fix (bootstrap returning its disposables as a collection, or a type-level exhaustiveness check keyed off the result type) would make the next omission a compile error; it is a refactor across ~15 return sites and belongs in its own increment. **Resolved in Increment 25 (ADR-0092):** extracted `createLifecycle` run loop in `lifecycle.ts`, defined `BootstrappedDisposables` and `DisposableKey` driving `DISPOSABLE_TEARDOWN_MAP: Record` for compile-time exhaustiveness, normalised `.dispose()` verb across all disposables, and cascaded `GameController.stop()` to `gameSync.stop()`. - **`stepView` sends the answers to the learner (RESOLVED in Increment 29 / ADR-0095).** `packages/api/src/presenters.ts` emits `expectedSan` on a move step and `correctIndex` on a quiz step, and `GET /v1/lessons/:id/steps` is the route the learner's own lesson page calls. Increment 23 omits both from the client-side types (`packages/web/src/api/models.ts`), so the app cannot render or grade against them and a future edit that tries becomes a compile error — but the fields are still on the wire and readable in devtools. The authoring routes legitimately need them returned to the author, so the fix is a learner-scoped step view (or a caller-dependent projection), not a deletion: an API contract decision with its own ADR. Nothing rated or rewarded depends on step progress today, so this is a wart rather than a breach. **Resolved in Increment 29 (ADR-0095):** added `LearnerStepView` / `learnerStepView` in `packages/api/src/presenters.ts` omitting `expectedSan` and `correctIndex`. `GET /v1/lessons/:id/steps` and `GET /v1/steps/:id` now check course authorship via `repo.getLesson` / `repo.getCourse`, returning full `stepView` to the author and `learnerStepView` to learners and anonymous callers. Updated OpenAPI schema and web model comments. The first attempt resolved authorship with a separate `getLesson` + `getCourse` after the step read, which doubled both routes from 3 SQL queries to 6 because `listSteps` / `getStep` had already made those reads internally and discarded the course; caught in the PR #92 review and fixed by adding `getStepWithCourse` / `listStepsWithCourse` to `LearningRepository`, which return what was already loaded. Both routes now make exactly one repository call, pinned by a counting-proxy test. diff --git a/docs/adr/0140-harness-ephemeral-port-acquisition.md b/docs/adr/0140-harness-ephemeral-port-acquisition.md index 3b6afe71..7cc02542 100644 --- a/docs/adr/0140-harness-ephemeral-port-acquisition.md +++ b/docs/adr/0140-harness-ephemeral-port-acquisition.md @@ -229,9 +229,10 @@ the full `packages/api` suite (chosen the same way as the original 20-run sample: enough for >90% detection odds at the historically observed ~1-in-5 rate) reproduced the defect 3 times, on **three files never previously implicated** — `rate-limit-atomicity`, `dependency-parity`, `studies-api` — none -of the original four. Combined with the original four, that is **seven -different files**, sharing nothing but the shared harness's `startHarness` -import, which confirms this is not a defect specific to any one file's logic. +of the original four. Combined with the original four, that is **seven distinct files** observed with this symptom +to date. Occurrences across seven distinct files make a shared or cross-cutting path more +plausible and make a defect confined to one test file less likely, but do not exclude +file-specific inputs or lifecycle interactions. All three historical captures show the identical signature: **only the `start` and `preload-installed` lines were logged. None of the hooks active at that time fired — diff --git a/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts b/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts index f0eef111..b8261d6a 100644 --- a/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts +++ b/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts @@ -97,6 +97,7 @@ test('explicit diagnostic integration: real native process.abort() records synch // On Windows/POSIX, abort causes abnormal termination assert.ok(status !== 0 || signal !== null, 'process terminates abnormally'); + assert.ok(records.some((r) => r.kind === 'preload-installed'), 'records preload-installed'); const abortRecord = records.find((r) => r.kind === 'process.abort'); assert.ok(abortRecord, 'synchronously records process.abort event'); assert.ok(typeof abortRecord?.callerStack === 'string', 'includes callerStack in abort record'); diff --git a/packages/api/test/diagnostics/signature-b-preload.test.ts b/packages/api/test/diagnostics/signature-b-preload.test.ts index b097ea62..9f4c6a5c 100644 --- a/packages/api/test/diagnostics/signature-b-preload.test.ts +++ b/packages/api/test/diagnostics/signature-b-preload.test.ts @@ -122,28 +122,14 @@ test('diagnostic preload: process.exit(42) records exit code and call-site stack assert.ok(exitRecord?.callerStack?.includes('exit-call-site'), 'stack traces exit invocation site'); }); -test('diagnostic preload: process.abort wrapper synchronously writes record before delegating (safe unit proof)', (t) => { - // Routine unit proof: executes a child script that verifies the patched process.abort writes - // the diagnostic record to disk immediately before delegating. Intercepts originalAbort call in userland - // so no native core-dump or SIGABRT is raised during routine test runs. - const { records, logDir } = runWithPreload(` - // Intercept abort delegation in userland to prove wrapper behavior without native crash - const originalAbort = process.abort; - process.abort = function testWrapperIntercept() { - // original wrapper invoked - return originalAbort.call(process); - }; - try { - process.abort(); - } catch {} - `); - t.after(() => fs.rmSync(logDir, { recursive: true, force: true })); - - // In the preload, patchedAbort writes before calling originalAbort. - const abortRecord = records.find((r) => r.kind === 'process.abort'); - assert.ok(abortRecord, 'synchronously records process.abort event'); - assert.ok(typeof abortRecord?.callerStack === 'string', 'includes callerStack in abort record'); - assert.ok(abortRecord?.callerStack?.includes('abort-call-site'), 'callerStack traces abort invocation site'); +test('diagnostic preload: process.abort wrapper is installed by preload without executing native abort in routine suite', () => { + // Routine test: verifies the patched process.abort wrapper is declared and registered by preload. + // Destructive native process.abort execution is isolated to the explicit diagnostic fixture + // (signature-b-preload-abort.diag.ts) to prevent core dumps and OS crash reporting during routine test runs. + const preloadSource = fs.readFileSync(PRELOAD_PATH, 'utf8'); + assert.ok(preloadSource.includes('function patchedAbort'), 'preload defines patchedAbort wrapper'); + assert.ok(preloadSource.includes("write('process.abort'"), 'patchedAbort records process.abort event'); + assert.ok(preloadSource.includes('return originalAbort(...args)'), 'patchedAbort delegates to originalAbort'); }); test('diagnostic preload: uncaughtExceptionMonitor passively captures error without suppressing crash', (t) => { From 9dd034720bf5f417fc1f2515e6a03ae0ec8064af Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 07:51:11 +0300 Subject: [PATCH 08/20] test(api): add safe runtime execution test for process.abort preload wrapper - Implement two-preload safe-shim pattern loading test-only sentinel before signature-b-preload.cjs - Prove runtime execution of real preload, patchedAbort wrapper installation, and delegation to originalAbort - Verify synchronous JSONL process.abort record with callerStack - Guarantee routine test suite executes no native abort, core dumps, or OS crash handlers - Keep real native abort integration proof in signature-b-preload-abort.diag.ts --- .../diagnostics/signature-b-preload.test.ts | 117 ++++++++++++++++-- 1 file changed, 109 insertions(+), 8 deletions(-) diff --git a/packages/api/test/diagnostics/signature-b-preload.test.ts b/packages/api/test/diagnostics/signature-b-preload.test.ts index 9f4c6a5c..05b2b0de 100644 --- a/packages/api/test/diagnostics/signature-b-preload.test.ts +++ b/packages/api/test/diagnostics/signature-b-preload.test.ts @@ -93,6 +93,84 @@ function runWithPreload( }; } +/** + * Executes a synthetic child script with a test-only safe abort shim loaded BEFORE the real preload. + * + * Execution order: + * 1. `safe-abort-shim.cjs` replaces native `process.abort` with a harmless sentinel that logs to stdout and calls `process.exit(99)`. + * 2. `signature-b-preload.cjs` loads second, capturing the sentinel function as `originalAbort` and installing `patchedAbort`. + * 3. The child script runs and calls `process.abort()`. + * 4. `patchedAbort` synchronously writes the `process.abort` JSONL diagnostic record and delegates to `originalAbort`. + * 5. The harmless sentinel runs and calls `process.exit(99)`, preventing native `process.abort()` / OS crash dumps. + */ +function runWithSafeAbortShimAndPreload( + code: string, + envOverride: Record = {}, +): { + readonly status: number | null; + readonly signal: NodeJS.Signals | null; + readonly stdout: string; + readonly stderr: string; + readonly logDir: string; + readonly records: readonly DiagnosticRecord[]; +} { + const generatedLogDir = fs.mkdtempSync(path.join(os.tmpdir(), 'sigb-test-')); + const targetLogDir = envOverride.SIGB_LOG_DIR ? path.resolve(envOverride.SIGB_LOG_DIR) : generatedLogDir; + const shimPath = path.join(generatedLogDir, 'safe-abort-shim.cjs'); + const scriptPath = path.join(generatedLogDir, 'test-target.cjs'); + + fs.writeFileSync( + shimPath, + ` + const fs = require('node:fs'); + process.abort = function harmlessSentinelAbort(...args) { + fs.writeSync(1, '__SAFE_ABORT_SENTINEL_INVOKED__\\n'); + process.exit(99); + }; + `, + 'utf8', + ); + + fs.writeFileSync(scriptPath, code, 'utf8'); + + const args = ['--require', shimPath, '--require', PRELOAD_PATH, scriptPath]; + + const env: Record = { + ...process.env, + ...envOverride, + SIGB_LOG_DIR: targetLogDir, + }; + delete env.NODE_TEST_CONTEXT; + + const result = spawnSync(process.execPath, args, { + cwd: generatedLogDir, + env, + encoding: 'utf8', + windowsHide: true, + }); + + const files = fs.existsSync(targetLogDir) + ? fs.readdirSync(targetLogDir).filter((f) => f.endsWith('.jsonl')) + : []; + const records: DiagnosticRecord[] = []; + for (const file of files) { + const content = fs.readFileSync(path.join(targetLogDir, file), 'utf8'); + const lines = content.split('\n').map((l) => l.trim()).filter(Boolean); + for (const line of lines) { + records.push(JSON.parse(line) as DiagnosticRecord); + } + } + + return { + status: result.status, + signal: result.signal, + stdout: result.stdout, + stderr: result.stderr, + logDir: generatedLogDir, + records, + }; +} + test('diagnostic preload: normal shutdown records lifecycle events', (t) => { const { status, records, logDir } = runWithPreload(` // clean normal execution @@ -122,14 +200,37 @@ test('diagnostic preload: process.exit(42) records exit code and call-site stack assert.ok(exitRecord?.callerStack?.includes('exit-call-site'), 'stack traces exit invocation site'); }); -test('diagnostic preload: process.abort wrapper is installed by preload without executing native abort in routine suite', () => { - // Routine test: verifies the patched process.abort wrapper is declared and registered by preload. - // Destructive native process.abort execution is isolated to the explicit diagnostic fixture - // (signature-b-preload-abort.diag.ts) to prevent core dumps and OS crash reporting during routine test runs. - const preloadSource = fs.readFileSync(PRELOAD_PATH, 'utf8'); - assert.ok(preloadSource.includes('function patchedAbort'), 'preload defines patchedAbort wrapper'); - assert.ok(preloadSource.includes("write('process.abort'"), 'patchedAbort records process.abort event'); - assert.ok(preloadSource.includes('return originalAbort(...args)'), 'patchedAbort delegates to originalAbort'); +test('diagnostic preload: process.abort wrapper synchronously writes record and delegates at runtime without native abort', (t) => { + // Safe runtime execution test: + // 1. safe-abort-shim.cjs preloads first and overrides native process.abort with harmless sentinel. + // 2. signature-b-preload.cjs preloads second, capturing the sentinel as originalAbort and installing patchedAbort. + // 3. Child script calls process.abort(). + // 4. patchedAbort synchronously writes the JSONL diagnostic record to SIGB_LOG_DIR before delegating to the sentinel. + // 5. Sentinel receives delegation and exits with status 99. + // 6. Child process does NOT execute native abort (no core dumps or OS crash handlers). + const { status, signal, stdout, records, logDir } = runWithSafeAbortShimAndPreload(` + // Verify runtime wrapper identity before calling + if (process.abort.name !== 'patchedAbort') { + process.exit(101); + } + // Call process.abort() + process.abort(); + `); + t.after(() => fs.rmSync(logDir, { recursive: true, force: true })); + + // Process exited cleanly via harmless sentinel delegation + assert.equal(signal, null, 'must not be terminated by signal'); + assert.equal(status, 99, 'harmless sentinel was executed via originalAbort delegation'); + assert.ok(stdout.includes('__SAFE_ABORT_SENTINEL_INVOKED__'), 'sentinel output verified on stdout'); + + // Preload lifecycle and abort records were written + const kinds = records.map((r) => r.kind); + assert.ok(kinds.includes('start'), 'records start'); + assert.ok(kinds.includes('preload-installed'), 'records preload-installed'); + const abortRecord = records.find((r) => r.kind === 'process.abort'); + assert.ok(abortRecord, 'synchronously records process.abort event to JSONL'); + assert.ok(typeof abortRecord?.callerStack === 'string', 'includes callerStack in abort record'); + assert.ok(abortRecord?.callerStack?.includes('test-target'), 'callerStack traces abort invocation site'); }); test('diagnostic preload: uncaughtExceptionMonitor passively captures error without suppressing crash', (t) => { From 1de0a0ea0c8db64156fbb53d3416321793e87316 Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 08:09:21 +0300 Subject: [PATCH 09/20] fix(api): harden secret redaction for auth headers/cookies and qualify termination classification wording MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Update sensitive value redaction regex to consume complete Authorization headers and multi-cookie strings across whitespace - Add regression tests in signature-b-preload.test.ts for Basic auth and semicolon-delimited cookie headers - Qualify termination classification in ADR-0140 §4 and ROADMAP.md to note that absent preload records alone do not prove external termination without corroborating status/signal/OS evidence --- docs/ROADMAP.md | 2 +- docs/adr/0140-harness-ephemeral-port-acquisition.md | 6 ++++-- packages/api/test/diagnostics/signature-b-preload.cjs | 2 +- .../api/test/diagnostics/signature-b-preload.test.ts | 10 ++++++++-- 4 files changed, 14 insertions(+), 6 deletions(-) diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 0897cf5b..eba04b6c 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -1349,7 +1349,7 @@ Debt observed during M14. Each states what is known, not what is planned; items - **The published capability document could omit a composed feature, and the client offered controls that could not work (RESOLVED in M15 Increment 24 / ADR-0132).** Left open by Increment 23 above. `capabilitiesView` (`packages/api/src/presenters.ts`) took a hand-written `Pick`, so a feature never added to it was invisible to `GET /v1/capabilities` and nothing complained. ADR-0131 judged that "a narrower failure than a 503 — the feature works for anyone who calls the route directly", and deferred it on the grounds that closing it meant first deciding which optional dependencies are user-facing capabilities. **Both halves of that were wrong.** The failure is narrower only when the client can still reach the feature, and a live instance already existed where it could not: `GET /v1/search` served three modes from two independently-gated dependency sets behind one published `search` flag, so a deployment running the Helm chart’s `search.semanticEnabled: false` advertised search while the client offered two mode buttons whose every request answered 503. And the judgement, while genuinely underivable, can be made **unskippable**, which was the property wanted. `semanticSearch` is now published from both of its dependencies, `Exclude[0] | NotAPublishedCapability>` must be `never` so a new optional dependency cannot compile until someone gives it a flag or records why it has none, and a behavioural guard requires every capability source to change what the document publishes — because a key can sit in that parameter unread, which the compile-time half cannot see. The mutation ledger lives in ADR-0132 and is not restated here. - **The search surface was not gated on `capabilities.search`, so an absolute kill switch still showed a search box (RESOLVED in M15 Increment 24 / ADR-0132 §5).** `SEARCH_ENABLED=0` — the chart's `search.enabled: false`, an absolute kill switch per ADR-0055 — leaves `searchRepository` unconstructed and `GET /v1/search` answering 503 on every mode, keyword included. The entry point was the persistent header form in `packages/web/index.html`, present on every page; being a `` rather than an `a[data-route]`, `NAV_CAPABILITY_MAP` could not reach it, which is why the first pass at Increment 24 gated the semantic and hybrid *modes* and left keyword ungated — the same defect class one mode over. Raised by the Qodo review of PR #155. **Fixed in the same increment rather than deferred:** the form now ships `hidden` and is revealed by `applySearchCapability` only on an explicit `search: true`; the route renders an honest unavailable notice and issues no request; and keyword search waits for the capability answer, reversing this increment's own earlier latency decision, because knowing whether a request is pointless requires having asked. A markup-contract test pins the `hidden` attribute, since the gate depends on it and every other test passes without it. - **Clicking a search mode discarded text typed since the page loaded (RESOLVED in a follow-up to M15 Increment 24 / ADR-0132).** `createModeInput` in `packages/web/src/app/search-mount.ts` closed over the query captured when the route mounted, so `navigateToSearchMode` navigated with the old term and the remount reset the input to match it. Type a new term into the header field, click **Semantic** without pressing enter, and the typed text was gone with no indication it had been discarded. Pre-existing — the closure predated Increment 24 and was untouched by it — and found by the adversarial review of PR #155 while reviewing the capability gate wrapped around the same control. **Resolved:** the query is now a `() => string` read when a mode is chosen rather than a string captured when the selector renders, matching what `main.ts`'s submit handler already does, and falling back to the mounted query only where the document has no header input. Two regression tests, one of which fails against the exact pre-fix closure. -- **`startHarness` drew ports `fetch` refuses (RESOLVED in ADR-0140); the unexplained whole-file failure is still open.** Two signatures were filed here, deliberately not as one cause, and that judgement held. **Signature A is resolved.** WHATWG Fetch blocks eighty-two ports and undici enforces the list on the port number alone, before opening a socket — so `server.listen(0)` could bind, listen and answer raw TCP while `fetch` still refused, surfacing as `TypeError: fetch failed` / `Error: bad port` at the harness’s first request rather than at the listen that caused it. Whether it can happen at all is a property of the host’s dynamic port range: a typical Linux CI range (32768–60999) contains no blocked port, while the Windows range in use here (1024–15000) contains nineteen, which is the whole of "green on CI, flaky locally". The guard that already existed was incomplete in a way that still failed — its hand-observed set of eighteen ports was the spec list intersected with one machine’s range **minus `6679`** — and it retried unboundedly, had no behaviour on exhaustion, and had been copy-pasted into `auth-signin-schema.integration.test.ts`, so the missing port had to be found twice. `packages/api/test/listen.ts` now owns port acquisition for both sites: the spec-complete eighty-two ports (verified by sweeping all 65535 through the real `fetch` on Node v24.15.0), a bounded twenty attempts, each rejected listener closed before the next is asked for, a guarded `address()` read in place of the `as AddressInfo` cast, and an exhaustion error naming the attempts and rejected ports and nothing else. **Signature B is not resolved and was not folded in.** A file still fails with `'test failed'`, no assertion, no stack and none of its own tests reported. Twenty consecutive full runs gave five failures: on pre-fix code one signature A (`auth.test.js`) and three signature B; on post-fix code one signature B and no signature A. The port fix removes A and leaves B exactly where it was, which is the evidence that they are two defects. Four different files were hit (`move-explanation-route`, `tournament-commentary-route`, `bot-detection-analyze`, `anti-cheat-analysis`), sharing no import beyond `./helpers`; each died in 589–703 ms with no test of its own reporting and no stderr. Refuted with evidence: ephemeral-port exhaustion (113 sockets in TIME_WAIT against a 13977-port range), a `Promise.race` loser becoming an unhandled rejection (`race` subscribes to every promise, confirmed on v24.15.0), a throwing `after`/`afterEach` hook (the affected files use none), and a double `close()` rejecting (awaited in a `finally`, it would be attributed to that test with a stack). A second bounded pass — twelve more full runs under the TAP reporter with a preload recording `uncaughtException`, `unhandledRejection` and any non-zero exit — produced twelve clean runs and captured nothing at the time. **A follow-up increment (`claude/node-test-signature-b`) then captured the defect directly, three more times, on three files never previously implicated** (`rate-limit-atomicity`, `dependency-parity`, `studies-api`) — seven distinct files observed with this symptom to date. Occurrences across seven distinct files make a shared or cross-cutting path more plausible and make a defect confined to one test file less likely, but do not exclude file-specific inputs or lifecycle interactions. An instrumented preload (`packages/api/test/diagnostics/signature-b-preload.cjs`) hooking process-level events — `process.exit`, `process.abort`, `process.kill`, `uncaughtExceptionMonitor` (passively observing uncaught exceptions and fatal unhandled rejections), `warning`, `beforeExit`, and Node’s own unconditional `exit` — showed **none of the hooks active at the time fired** on any of the three historical captures (though `process.abort()` was not wrapped in those initial runs and is now covered for future occurrences). A synthetic `process.exit(1)`-before-registration fixture reproduces the identical silent shape; every other synthetic mechanism tried (a post-test async throw, an emitter `'error'` with its listener removed, a synchronous module-load throw, a delayed `SIGKILL`) prints a visibly different diagnostic line, stack, or partial test output that the real defect never shows. This narrows the investigated possibilities while leaving the root cause unresolved: the per-file child process (`node --test` spawns one per file, confirmed by distinct PIDs) was not terminated by `process.exit`, uncaught exceptions, or fatal unhandled rejections, and future runs with `process.abort` instrumentation will record whether abort was called or whether termination originated outside the JS runtime entirely. The machine had roughly 2.5 GB of 15.7 GB RAM free at capture time with several other agents’ processes concurrently running, which is circumstantially consistent with resource contention, but no crash was recorded in the Windows Application or System event logs in that window, so the exact external trigger is still not established. No fix was invented — the forbidden responses (sleeps, whole-file retries, lowering concurrency) would only hide a cause that remains outside this repository’s code. See ADR-0140 §4. +- **`startHarness` drew ports `fetch` refuses (RESOLVED in ADR-0140); the unexplained whole-file failure is still open.** Two signatures were filed here, deliberately not as one cause, and that judgement held. **Signature A is resolved.** WHATWG Fetch blocks eighty-two ports and undici enforces the list on the port number alone, before opening a socket — so `server.listen(0)` could bind, listen and answer raw TCP while `fetch` still refused, surfacing as `TypeError: fetch failed` / `Error: bad port` at the harness’s first request rather than at the listen that caused it. Whether it can happen at all is a property of the host’s dynamic port range: a typical Linux CI range (32768–60999) contains no blocked port, while the Windows range in use here (1024–15000) contains nineteen, which is the whole of "green on CI, flaky locally". The guard that already existed was incomplete in a way that still failed — its hand-observed set of eighteen ports was the spec list intersected with one machine’s range **minus `6679`** — and it retried unboundedly, had no behaviour on exhaustion, and had been copy-pasted into `auth-signin-schema.integration.test.ts`, so the missing port had to be found twice. `packages/api/test/listen.ts` now owns port acquisition for both sites: the spec-complete eighty-two ports (verified by sweeping all 65535 through the real `fetch` on Node v24.15.0), a bounded twenty attempts, each rejected listener closed before the next is asked for, a guarded `address()` read in place of the `as AddressInfo` cast, and an exhaustion error naming the attempts and rejected ports and nothing else. **Signature B is not resolved and was not folded in.** A file still fails with `'test failed'`, no assertion, no stack and none of its own tests reported. Twenty consecutive full runs gave five failures: on pre-fix code one signature A (`auth.test.js`) and three signature B; on post-fix code one signature B and no signature A. The port fix removes A and leaves B exactly where it was, which is the evidence that they are two defects. Four different files were hit (`move-explanation-route`, `tournament-commentary-route`, `bot-detection-analyze`, `anti-cheat-analysis`), sharing no import beyond `./helpers`; each died in 589–703 ms with no test of its own reporting and no stderr. Refuted with evidence: ephemeral-port exhaustion (113 sockets in TIME_WAIT against a 13977-port range), a `Promise.race` loser becoming an unhandled rejection (`race` subscribes to every promise, confirmed on v24.15.0), a throwing `after`/`afterEach` hook (the affected files use none), and a double `close()` rejecting (awaited in a `finally`, it would be attributed to that test with a stack). A second bounded pass — twelve more full runs under the TAP reporter with a preload recording `uncaughtException`, `unhandledRejection` and any non-zero exit — produced twelve clean runs and captured nothing at the time. **A follow-up increment (`claude/node-test-signature-b`) then captured the defect directly, three more times, on three files never previously implicated** (`rate-limit-atomicity`, `dependency-parity`, `studies-api`) — seven distinct files observed with this symptom to date. Occurrences across seven distinct files make a shared or cross-cutting path more plausible and make a defect confined to one test file less likely, but do not exclude file-specific inputs or lifecycle interactions. An instrumented preload (`packages/api/test/diagnostics/signature-b-preload.cjs`) hooking process-level events — `process.exit`, `process.abort`, `process.kill`, `uncaughtExceptionMonitor` (passively observing uncaught exceptions and fatal unhandled rejections), `warning`, `beforeExit`, and Node’s own unconditional `exit` — showed **none of the hooks active at the time fired** on any of the three historical captures (though `process.abort()` was not wrapped in those initial runs and is now covered for future occurrences). A synthetic `process.exit(1)`-before-registration fixture reproduces the identical silent shape; every other synthetic mechanism tried (a post-test async throw, an emitter `'error'` with its listener removed, a synchronous module-load throw, a delayed `SIGKILL`) prints a visibly different diagnostic line, stack, or partial test output that the real defect never shows. This narrows the investigated possibilities while leaving the root cause unresolved: the per-file child process (`node --test` spawns one per file, confirmed by distinct PIDs) was not terminated by `process.exit`, uncaught exceptions, or fatal unhandled rejections, and future runs with `process.abort` instrumentation will record whether abort was called through JS; an absent record narrows in-runtime JS termination but cannot alone prove external termination without corroborating child exit status/signal data or OS-level crash evidence (e.g. distinguishing an external kill or uncatchable signal from a native C++/V8 crash). The machine had roughly 2.5 GB of 15.7 GB RAM free at capture time with several other agents’ processes concurrently running, which is circumstantially consistent with resource contention, but no crash was recorded in the Windows Application or System event logs in that window, so the exact external trigger is still not established. No fix was invented — the forbidden responses (sleeps, whole-file retries, lowering concurrency) would only hide a cause that remains outside this repository’s code. See ADR-0140 §4. - **`ApiServer.listen` registered no `'error'` handler, so a failed bind hung and raised an uncaught event (RESOLVED in ADR-0140 §5).** `packages/api/src/server.ts` resolved its promise from the `listening` callback only and built it with no reject path. A bind failing asynchronously (`EADDRINUSE`, `EMFILE`) left the promise pending forever and, with no `'error'` listener on the `http.Server`, was re-raised as an uncaught exception. Found while investigating ADR-0140 and independently raised by the Qodo review of PR #21. **Resolved in the same increment** rather than deferred, because ADR-0140 §2's bounded, diagnosable acquisition is not true without it — the retry can only report a bind error if the listener it is handed rejects. A one-shot `'error'` listener now rejects and is removed once listening, so later server errors keep their previous semantics rather than being swallowed by a `reject` on a settled promise. The regression test fails against the exact pre-fix code, through an uncaught `ERR_UNHANDLED_ERROR`. - **`main.ts`'s controller-disposal list is manual, untested, and silently incomplete when a section is added (RESOLVED in Increment 25 / ADR-0092).** `run()` in `packages/web/src/main.ts` disposes the previous route's controllers by name, and its own comment says doing so "is what makes re-bootstrapping safe" — but adding a section to `bootstrap` and forgetting to add it there compiles, passes every gate, and leaks. Increment 23 shipped exactly that omission for `LearningController` and it was caught in PR review, not by a test. `main.ts` has no test coverage of any kind, so no section's disposal is verified. A structural fix (bootstrap returning its disposables as a collection, or a type-level exhaustiveness check keyed off the result type) would make the next omission a compile error; it is a refactor across ~15 return sites and belongs in its own increment. **Resolved in Increment 25 (ADR-0092):** extracted `createLifecycle` run loop in `lifecycle.ts`, defined `BootstrappedDisposables` and `DisposableKey` driving `DISPOSABLE_TEARDOWN_MAP: Record` for compile-time exhaustiveness, normalised `.dispose()` verb across all disposables, and cascaded `GameController.stop()` to `gameSync.stop()`. - **`stepView` sends the answers to the learner (RESOLVED in Increment 29 / ADR-0095).** `packages/api/src/presenters.ts` emits `expectedSan` on a move step and `correctIndex` on a quiz step, and `GET /v1/lessons/:id/steps` is the route the learner's own lesson page calls. Increment 23 omits both from the client-side types (`packages/web/src/api/models.ts`), so the app cannot render or grade against them and a future edit that tries becomes a compile error — but the fields are still on the wire and readable in devtools. The authoring routes legitimately need them returned to the author, so the fix is a learner-scoped step view (or a caller-dependent projection), not a deletion: an API contract decision with its own ADR. Nothing rated or rewarded depends on step progress today, so this is a wart rather than a breach. **Resolved in Increment 29 (ADR-0095):** added `LearnerStepView` / `learnerStepView` in `packages/api/src/presenters.ts` omitting `expectedSan` and `correctIndex`. `GET /v1/lessons/:id/steps` and `GET /v1/steps/:id` now check course authorship via `repo.getLesson` / `repo.getCourse`, returning full `stepView` to the author and `learnerStepView` to learners and anonymous callers. Updated OpenAPI schema and web model comments. The first attempt resolved authorship with a separate `getLesson` + `getCourse` after the step read, which doubled both routes from 3 SQL queries to 6 because `listSteps` / `getStep` had already made those reads internally and discarded the course; caught in the PR #92 review and fixed by adding `getStepWithCourse` / `listStepsWithCourse` to `LearningRepository`, which return what was already loaded. Both routes now make exactly one repository call, pinned by a counting-proxy test. diff --git a/docs/adr/0140-harness-ephemeral-port-acquisition.md b/docs/adr/0140-harness-ephemeral-port-acquisition.md index 7cc02542..41841d39 100644 --- a/docs/adr/0140-harness-ephemeral-port-acquisition.md +++ b/docs/adr/0140-harness-ephemeral-port-acquisition.md @@ -246,8 +246,10 @@ could not categorically exclude an uninstrumented `process.abort()` or an extern **This narrows Signature B's investigated possibilities while leaving its root cause unresolved:** `process.abort()` is now instrumented so any future occurrence will record whether an abort -was invoked or whether termination originated outside the JS runtime entirely (e.g. an OS-level -kill, uncatchable signal, or native crash). Two observations remain consistent with an +was invoked through JS; an absent record narrows in-runtime JS termination but cannot alone +prove external termination without corroborating child exit status/signal data or OS-level +evidence (e.g. distinguishing an external kill or uncatchable signal from a native C++/V8 crash). +Two observations remain consistent with an environmental (not code) origin: at the time of capture this development machine had roughly 2.5 GB of 15.7 GB RAM free, with several unrelated concurrent processes (other agents' worktrees and dev servers) running; and `node --test` spawns dozens of child processes across a full diff --git a/packages/api/test/diagnostics/signature-b-preload.cjs b/packages/api/test/diagnostics/signature-b-preload.cjs index 99d2a67c..0573783b 100644 --- a/packages/api/test/diagnostics/signature-b-preload.cjs +++ b/packages/api/test/diagnostics/signature-b-preload.cjs @@ -67,7 +67,7 @@ const REDACT_PATTERNS = [ [/sk-[a-zA-Z0-9_-]{10,}/g, '[REDACTED_API_KEY]'], [/Bearer\s+[A-Za-z0-9._~+/-]+=*/g, 'Bearer [REDACTED_TOKEN]'], [/(?:postgres|postgresql):\/\/[^:]+:[^@]+@/g, 'postgres://[REDACTED_CREDS]@'], - [/(password|secret|token|authorization|cookie)\s*[:=]\s*(?:"[^"]*"|'[^']*'|[^,\s]+)/gi, '$1=[REDACTED]'], + [/(password|secret|token|authorization|cookie)\s*[:=]\s*(?:"[^"]*"|'[^']*'|[^,\r\n]+)/gi, '$1=[REDACTED]'], ]; /** diff --git a/packages/api/test/diagnostics/signature-b-preload.test.ts b/packages/api/test/diagnostics/signature-b-preload.test.ts index 05b2b0de..fdd1649f 100644 --- a/packages/api/test/diagnostics/signature-b-preload.test.ts +++ b/packages/api/test/diagnostics/signature-b-preload.test.ts @@ -318,12 +318,13 @@ test('diagnostic preload: process.kill records target PID and signal', (t) => { assert.ok(killRecord?.callerStack?.includes('kill-call-site'), 'traces kill call site'); }); -test('diagnostic preload: redacts postgres/postgresql credentials, tokens, and quoted secrets with spaces', (t) => { +test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Authorization headers, cookies, and quoted secrets with spaces', (t) => { const { records, logDir } = runWithPreload(` const err = new Error( 'Connect failed to postgresql://app_user:s3cr3tpass@db.internal:5432/chess and ' + 'postgres://admin:supersecret@10.0.0.1:5432/main and sk-1234567890abcdef12345 and ' + - 'password="correct horse battery staple" and secret=\\'top secret key phrase\\'' + 'password="correct horse battery staple" and secret=\\'top secret key phrase\\' and ' + + 'Authorization: Basic dXNlcjpwYXNzd29yZA==, Cookie: session=xyz123; token=abc456; other=789' ); throw err; `); @@ -337,10 +338,15 @@ test('diagnostic preload: redacts postgres/postgresql credentials, tokens, and q assert.ok(msg.includes('[REDACTED_API_KEY]'), 'redacts OpenAI-style api key'); assert.ok(msg.includes('password=[REDACTED]'), 'redacts double-quoted password with spaces'); assert.ok(msg.includes('secret=[REDACTED]'), 'redacts single-quoted secret with spaces'); + assert.ok(msg.includes('Authorization=[REDACTED]'), 'redacts Basic authorization header'); + assert.ok(msg.includes('Cookie=[REDACTED]'), 'redacts multi-cookie header'); assert.ok(!msg.includes('s3cr3tpass'), 'raw password 1 not present'); assert.ok(!msg.includes('supersecret'), 'raw password 2 not present'); assert.ok(!msg.includes('correct horse battery staple'), 'raw quoted password with spaces not present'); assert.ok(!msg.includes('top secret key phrase'), 'raw single-quoted secret with spaces not present'); + assert.ok(!msg.includes('dXNlcjpwYXNzd29yZA=='), 'raw basic auth credential not present'); + assert.ok(!msg.includes('session=xyz123'), 'raw cookie session not present'); + assert.ok(!msg.includes('token=abc456'), 'raw cookie token not present'); assert.ok(!msg.includes('sk-1234567890abcdef12345'), 'raw key not present'); }); From 9bddd2b7db6b535b916fdb8a583b3b55b223d773 Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 08:36:10 +0300 Subject: [PATCH 10/20] fix(api): support escaped quotes in redaction regex, qualify ADR isolation evidence, and test raw relative log dir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Support escaped quotes in quoted secret redaction regex to prevent partial leaks - Add regression assertions in signature-b-preload.test.ts for escaped quotes in passwords and secrets - Update ADR-0140 §4 to cite flag-free PID run as evidence for default per-file process isolation - Forward raw relative SIGB_LOG_DIR to child process and assert resolution relative to child working directory --- ...0140-harness-ephemeral-port-acquisition.md | 9 ++-- .../test/diagnostics/signature-b-preload.cjs | 2 +- .../diagnostics/signature-b-preload.test.ts | 45 +++++++++++-------- 3 files changed, 33 insertions(+), 23 deletions(-) diff --git a/docs/adr/0140-harness-ephemeral-port-acquisition.md b/docs/adr/0140-harness-ephemeral-port-acquisition.md index 41841d39..7cfabb28 100644 --- a/docs/adr/0140-harness-ephemeral-port-acquisition.md +++ b/docs/adr/0140-harness-ephemeral-port-acquisition.md @@ -187,10 +187,11 @@ synthetic proxies alone. **Node's test runner spawns one child process per test file.** Verified directly: two trivial files run together under `node --test --test-concurrency=1` report distinct PIDs, neither matching the -parent's. A same-named (currently experimental on this Node version) flag, -`--test-isolation=process`, confirms this is the default rather than an -accident of this run. It is why the failure is confined to exactly one file -per occurrence with no effect on any other file in the same run. +parent's. The flag-free PID run is the evidence for default per-file process +isolation on the tested Node version, while the explicit `--test-isolation=process` +flag demonstrates that process isolation can also be explicitly selected. It is why +the failure is confined to exactly one file per occurrence with no effect on any +other file in the same run. **The bare `'test failed'` with no stack is what Node's runner reports for a child that exits non-zero or is signaled before any subtest result is recorded.** This was reproduced directly: a synthetic file that calls diff --git a/packages/api/test/diagnostics/signature-b-preload.cjs b/packages/api/test/diagnostics/signature-b-preload.cjs index 0573783b..a70588a8 100644 --- a/packages/api/test/diagnostics/signature-b-preload.cjs +++ b/packages/api/test/diagnostics/signature-b-preload.cjs @@ -67,7 +67,7 @@ const REDACT_PATTERNS = [ [/sk-[a-zA-Z0-9_-]{10,}/g, '[REDACTED_API_KEY]'], [/Bearer\s+[A-Za-z0-9._~+/-]+=*/g, 'Bearer [REDACTED_TOKEN]'], [/(?:postgres|postgresql):\/\/[^:]+:[^@]+@/g, 'postgres://[REDACTED_CREDS]@'], - [/(password|secret|token|authorization|cookie)\s*[:=]\s*(?:"[^"]*"|'[^']*'|[^,\r\n]+)/gi, '$1=[REDACTED]'], + [/(password|secret|token|authorization|cookie)\s*[:=]\s*(?:"(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*'|[^,\r\n]+)/gi, '$1=[REDACTED]'], ]; /** diff --git a/packages/api/test/diagnostics/signature-b-preload.test.ts b/packages/api/test/diagnostics/signature-b-preload.test.ts index fdd1649f..a213027c 100644 --- a/packages/api/test/diagnostics/signature-b-preload.test.ts +++ b/packages/api/test/diagnostics/signature-b-preload.test.ts @@ -49,7 +49,10 @@ function runWithPreload( readonly records: readonly DiagnosticRecord[]; } { const generatedLogDir = fs.mkdtempSync(path.join(os.tmpdir(), 'sigb-test-')); - const targetLogDir = envOverride.SIGB_LOG_DIR ? path.resolve(envOverride.SIGB_LOG_DIR) : generatedLogDir; + const rawLogDir = envOverride.SIGB_LOG_DIR; + const targetLogDir = rawLogDir + ? path.resolve(generatedLogDir, rawLogDir) + : generatedLogDir; const scriptPath = path.join(generatedLogDir, 'test-target.cjs'); fs.writeFileSync(scriptPath, code, 'utf8'); @@ -60,7 +63,7 @@ function runWithPreload( const env: Record = { ...process.env, ...envOverride, - SIGB_LOG_DIR: targetLogDir, + SIGB_LOG_DIR: rawLogDir ?? targetLogDir, }; delete env.NODE_TEST_CONTEXT; @@ -115,7 +118,10 @@ function runWithSafeAbortShimAndPreload( readonly records: readonly DiagnosticRecord[]; } { const generatedLogDir = fs.mkdtempSync(path.join(os.tmpdir(), 'sigb-test-')); - const targetLogDir = envOverride.SIGB_LOG_DIR ? path.resolve(envOverride.SIGB_LOG_DIR) : generatedLogDir; + const rawLogDir = envOverride.SIGB_LOG_DIR; + const targetLogDir = rawLogDir + ? path.resolve(generatedLogDir, rawLogDir) + : generatedLogDir; const shimPath = path.join(generatedLogDir, 'safe-abort-shim.cjs'); const scriptPath = path.join(generatedLogDir, 'test-target.cjs'); @@ -138,7 +144,7 @@ function runWithSafeAbortShimAndPreload( const env: Record = { ...process.env, ...envOverride, - SIGB_LOG_DIR: targetLogDir, + SIGB_LOG_DIR: rawLogDir ?? targetLogDir, }; delete env.NODE_TEST_CONTEXT; @@ -318,15 +324,18 @@ test('diagnostic preload: process.kill records target PID and signal', (t) => { assert.ok(killRecord?.callerStack?.includes('kill-call-site'), 'traces kill call site'); }); -test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Authorization headers, cookies, and quoted secrets with spaces', (t) => { +test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Authorization headers, cookies, and quoted secrets with spaces or escaped quotes', (t) => { const { records, logDir } = runWithPreload(` - const err = new Error( - 'Connect failed to postgresql://app_user:s3cr3tpass@db.internal:5432/chess and ' + - 'postgres://admin:supersecret@10.0.0.1:5432/main and sk-1234567890abcdef12345 and ' + - 'password="correct horse battery staple" and secret=\\'top secret key phrase\\' and ' + - 'Authorization: Basic dXNlcjpwYXNzd29yZA==, Cookie: session=xyz123; token=abc456; other=789' - ); - throw err; + const parts = [ + 'Connect failed to postgresql://app_user:s3cr3tpass@db.internal:5432/chess', + 'postgres://admin:supersecret@10.0.0.1:5432/main', + 'sk-1234567890abcdef12345', + 'password="correct \\\\\\"horse\\\\\\" battery staple"', + "secret='top \\\\\\'secret\\\\\\' key phrase'", + 'Authorization: Basic dXNlcjpwYXNzd29yZA==,', + 'Cookie: session=xyz123; token=abc456; other=789', + ]; + throw new Error(parts.join(' and ')); `); t.after(() => fs.rmSync(logDir, { recursive: true, force: true })); @@ -336,14 +345,14 @@ test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Autho assert.ok(msg.includes('postgres://[REDACTED_CREDS]@db.internal:5432/chess'), 'redacts postgresql credentials'); assert.ok(msg.includes('postgres://[REDACTED_CREDS]@10.0.0.1:5432/main'), 'redacts postgres credentials'); assert.ok(msg.includes('[REDACTED_API_KEY]'), 'redacts OpenAI-style api key'); - assert.ok(msg.includes('password=[REDACTED]'), 'redacts double-quoted password with spaces'); - assert.ok(msg.includes('secret=[REDACTED]'), 'redacts single-quoted secret with spaces'); + assert.ok(msg.includes('password=[REDACTED]'), 'redacts double-quoted password with escaped quotes'); + assert.ok(msg.includes('secret=[REDACTED]'), 'redacts single-quoted secret with escaped quotes'); assert.ok(msg.includes('Authorization=[REDACTED]'), 'redacts Basic authorization header'); assert.ok(msg.includes('Cookie=[REDACTED]'), 'redacts multi-cookie header'); assert.ok(!msg.includes('s3cr3tpass'), 'raw password 1 not present'); assert.ok(!msg.includes('supersecret'), 'raw password 2 not present'); - assert.ok(!msg.includes('correct horse battery staple'), 'raw quoted password with spaces not present'); - assert.ok(!msg.includes('top secret key phrase'), 'raw single-quoted secret with spaces not present'); + assert.ok(!msg.includes('horse'), 'raw quoted password content with escaped quotes not present'); + assert.ok(!msg.includes('key phrase'), 'raw single-quoted secret content with escaped quotes not present'); assert.ok(!msg.includes('dXNlcjpwYXNzd29yZA=='), 'raw basic auth credential not present'); assert.ok(!msg.includes('session=xyz123'), 'raw cookie session not present'); assert.ok(!msg.includes('token=abc456'), 'raw cookie token not present'); @@ -391,14 +400,14 @@ test('diagnostic preload: pre-existing directory does not fail or crash', (t) => test('diagnostic preload: relative SIGB_LOG_DIR resolves correctly', (t) => { const relDir = `./tmp-sigb-rel-${Date.now()}-${Math.random().toString(36).slice(2)}`; - const absDir = path.resolve(relDir); const { records, logDir } = runWithPreload('const relOk = true;', { SIGB_LOG_DIR: relDir }); + const absDir = path.resolve(logDir, relDir); t.after(() => { fs.rmSync(logDir, { recursive: true, force: true }); fs.rmSync(absDir, { recursive: true, force: true }); }); - assert.ok(fs.existsSync(absDir), 'creates directory at resolved absolute path'); + assert.ok(fs.existsSync(absDir), 'creates directory at resolved relative path in child cwd'); assert.ok(records.length > 0, 'reads child diagnostic records from relative SIGB_LOG_DIR'); }); From 453e79f678723b77e630f65078525572c728a24f Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 09:01:45 +0300 Subject: [PATCH 11/20] fix(api): capture testFile across all child runner contexts and narrow ADR-0140 isolation claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Capture process.argv[1] when NODE_TEST_CONTEXT is truthy (including 'child' and 'child-v8') - Add regression coverage for 'child', 'child-v8', and root process testFile capture - Clarify ADR-0140 §4 that process isolation contains process-level termination without precluding host-level resource contention - Narrow ADR-0140 §4 bare 'test failed' description to silent/early non-zero exit mechanisms --- ...0140-harness-ephemeral-port-acquisition.md | 10 ++--- .../test/diagnostics/signature-b-preload.cjs | 2 +- .../diagnostics/signature-b-preload.test.ts | 39 ++++++++++++++++++- 3 files changed, 43 insertions(+), 8 deletions(-) diff --git a/docs/adr/0140-harness-ephemeral-port-acquisition.md b/docs/adr/0140-harness-ephemeral-port-acquisition.md index 7cfabb28..00a30ed4 100644 --- a/docs/adr/0140-harness-ephemeral-port-acquisition.md +++ b/docs/adr/0140-harness-ephemeral-port-acquisition.md @@ -189,12 +189,12 @@ directly: two trivial files run together under `node --test --test-concurrency=1` report distinct PIDs, neither matching the parent's. The flag-free PID run is the evidence for default per-file process isolation on the tested Node version, while the explicit `--test-isolation=process` -flag demonstrates that process isolation can also be explicitly selected. It is why -the failure is confined to exactly one file per occurrence with no effect on any -other file in the same run. +flag demonstrates that process isolation can also be explicitly selected. This process +boundary proves that a terminating child does not directly abort sibling test processes +in the runner, though shared host resources can still interact across processes. -**The bare `'test failed'` with no stack is what Node's runner reports for a -child that exits non-zero or is signaled before any subtest result is recorded.** This was reproduced directly: a synthetic file that calls +**The bare `'test failed'` with no stack and empty stderr is what Node's runner reports for a +child that calls `process.exit(1)` (or terminates silently) before registering any test.** This was reproduced directly: a synthetic file that calls `process.exit(1)` before registering any test produces the identical reported shape as the real defect — `✖ (Nms)` / `'test failed'`, zero individual tests in the summary, nothing on stderr. Every OTHER synthetic mechanism tried diff --git a/packages/api/test/diagnostics/signature-b-preload.cjs b/packages/api/test/diagnostics/signature-b-preload.cjs index a70588a8..4758a209 100644 --- a/packages/api/test/diagnostics/signature-b-preload.cjs +++ b/packages/api/test/diagnostics/signature-b-preload.cjs @@ -163,7 +163,7 @@ function write(kind, fields) { kind, pid: process.pid, ppid: process.ppid, - testFile: process.env.NODE_TEST_CONTEXT === 'child-v8' ? (process.argv[1] || null) : null, + testFile: Boolean(process.env.NODE_TEST_CONTEXT) ? (process.argv[1] || null) : null, nodeTestContext: process.env.NODE_TEST_CONTEXT || null, timeIso: new Date().toISOString(), timeNs: process.hrtime.bigint().toString(), diff --git a/packages/api/test/diagnostics/signature-b-preload.test.ts b/packages/api/test/diagnostics/signature-b-preload.test.ts index a213027c..f6ae20b0 100644 --- a/packages/api/test/diagnostics/signature-b-preload.test.ts +++ b/packages/api/test/diagnostics/signature-b-preload.test.ts @@ -11,6 +11,8 @@ interface DiagnosticRecord { readonly kind: string; readonly pid: number; readonly ppid: number; + readonly testFile?: string | null; + readonly nodeTestContext?: string | null; readonly code?: number; readonly targetPid?: number; readonly signal?: string | number; @@ -62,10 +64,12 @@ function runWithPreload( const env: Record = { ...process.env, - ...envOverride, SIGB_LOG_DIR: rawLogDir ?? targetLogDir, }; delete env.NODE_TEST_CONTEXT; + for (const [k, v] of Object.entries(envOverride)) { + env[k] = v; + } const result = spawnSync(process.execPath, args, { cwd: generatedLogDir, @@ -143,10 +147,12 @@ function runWithSafeAbortShimAndPreload( const env: Record = { ...process.env, - ...envOverride, SIGB_LOG_DIR: rawLogDir ?? targetLogDir, }; delete env.NODE_TEST_CONTEXT; + for (const [k, v] of Object.entries(envOverride)) { + env[k] = v; + } const result = spawnSync(process.execPath, args, { cwd: generatedLogDir, @@ -428,3 +434,32 @@ test('diagnostic preload: usage glob pattern matches test files without literal assert.ok(preloadSource.includes('"dist-test/test/**/*.test.js"'), 'preload usage specifies unescaped glob'); assert.ok(!preloadSource.includes('**\\/*.test.js'), 'preload usage does not contain escaped backslash in glob'); }); + +test('diagnostic preload: captures testFile when NODE_TEST_CONTEXT is "child" or "child-v8"', (t) => { + const childRun = runWithPreload('const ok = true;', { NODE_TEST_CONTEXT: 'child' }); + t.after(() => fs.rmSync(childRun.logDir, { recursive: true, force: true })); + + const childRecord = childRun.records.find((r) => r.kind === 'preload-installed'); + assert.ok(childRecord, 'preload-installed record exists for child context'); + assert.equal(childRecord?.nodeTestContext, 'child', 'records nodeTestContext'); + assert.ok(typeof childRecord?.testFile === 'string', 'testFile is a string for child context'); + assert.ok(childRecord?.testFile?.includes('test-target.cjs'), 'testFile captures script path for child context'); + + const v8Run = runWithPreload('const ok = true;', { NODE_TEST_CONTEXT: 'child-v8' }); + t.after(() => fs.rmSync(v8Run.logDir, { recursive: true, force: true })); + + const v8Record = v8Run.records.find((r) => r.kind === 'preload-installed'); + assert.ok(v8Record, 'preload-installed record exists for child-v8 context'); + assert.equal(v8Record?.nodeTestContext, 'child-v8', 'records nodeTestContext'); + assert.ok(typeof v8Record?.testFile === 'string', 'testFile is a string for child-v8 context'); + assert.ok(v8Record?.testFile?.includes('test-target.cjs'), 'testFile captures script path for child-v8 context'); + + const rootRun = runWithPreload('const ok = true;'); + t.after(() => fs.rmSync(rootRun.logDir, { recursive: true, force: true })); + + const rootRecord = rootRun.records.find((r) => r.kind === 'preload-installed'); + assert.ok(rootRecord, 'preload-installed record exists for root context'); + assert.equal(rootRecord?.nodeTestContext, null, 'nodeTestContext is null for non-runner process'); + assert.equal(rootRecord?.testFile, null, 'testFile is null for non-runner process'); +}); + From 7231ce79f44a77a2a723bafeea1349fee20e9f99 Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 09:13:06 +0300 Subject: [PATCH 12/20] docs(adr): characterize failure mode in ADR-0140 heading and refactor child runner helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Update ADR-0140 §4 heading to describe failure mode as characterized rather than proven - Consolidate duplicate child process spawning in signature-b-preload.test.ts into runChild helper - Allow warning count assertion to tolerate ambient runtime warnings --- ...0140-harness-ephemeral-port-acquisition.md | 2 +- .../diagnostics/signature-b-preload.test.ts | 131 +++++++----------- 2 files changed, 52 insertions(+), 81 deletions(-) diff --git a/docs/adr/0140-harness-ephemeral-port-acquisition.md b/docs/adr/0140-harness-ephemeral-port-acquisition.md index 00a30ed4..c19d2e53 100644 --- a/docs/adr/0140-harness-ephemeral-port-acquisition.md +++ b/docs/adr/0140-harness-ephemeral-port-acquisition.md @@ -177,7 +177,7 @@ follow-up investigation (below) has since ruled it out as the mechanism behind the observed signature specifically, even though it remained a real defect worth fixing on its own merits. -### Follow-up investigation: the mechanism is now proven, the trigger is not +### Follow-up investigation: the failure mode is characterized, the trigger is not A later increment (`claude/node-test-signature-b`) instrumented selected termination and error paths a child process can raise and captured three diff --git a/packages/api/test/diagnostics/signature-b-preload.test.ts b/packages/api/test/diagnostics/signature-b-preload.test.ts index f6ae20b0..54572b12 100644 --- a/packages/api/test/diagnostics/signature-b-preload.test.ts +++ b/packages/api/test/diagnostics/signature-b-preload.test.ts @@ -29,19 +29,19 @@ interface DiagnosticRecord { readonly activeResources?: Record | null; } +interface RunChildOptions { + readonly extraPreloads?: (dir: string) => readonly string[]; + readonly useNodeTestRunner?: boolean; +} + /** - * Executes a synthetic test script under the diagnostic preload in an isolated temporary directory. - * Sets `cwd: generatedLogDir` and isolates diagnostic log directory per invocation. - * - * @param {string} code - JavaScript code to execute in the child process. - * @param {Record} [envOverride] - Optional environment variables to override. - * @param {boolean} [useNodeTestRunner=false] - Whether to invoke via `node --test` runner instead of plain node. - * @returns {{ status: number | null, signal: NodeJS.Signals | null, stdout: string, stderr: string, logDir: string, records: readonly DiagnosticRecord[] }} + * Internal runner that executes a synthetic child script under the diagnostic preload + * in an isolated temporary directory with optional extra preload hooks. */ -function runWithPreload( +function runChild( code: string, envOverride: Record = {}, - useNodeTestRunner = false, + options: RunChildOptions = {}, ): { readonly status: number | null; readonly signal: NodeJS.Signals | null; @@ -58,9 +58,13 @@ function runWithPreload( const scriptPath = path.join(generatedLogDir, 'test-target.cjs'); fs.writeFileSync(scriptPath, code, 'utf8'); - const args = useNodeTestRunner - ? ['--require', PRELOAD_PATH, '--test', scriptPath] - : ['--require', PRELOAD_PATH, scriptPath]; + const extraPreloads = options.extraPreloads ? options.extraPreloads(generatedLogDir) : []; + const preloads = [...extraPreloads, PRELOAD_PATH]; + const args = preloads.flatMap((p) => ['--require', p]); + if (options.useNodeTestRunner) { + args.push('--test'); + } + args.push(scriptPath); const env: Record = { ...process.env, @@ -100,6 +104,23 @@ function runWithPreload( }; } +/** + * Executes a synthetic test script under the diagnostic preload in an isolated temporary directory. + * Sets `cwd: generatedLogDir` and isolates diagnostic log directory per invocation. + * + * @param {string} code - JavaScript code to execute in the child process. + * @param {Record} [envOverride] - Optional environment variables to override. + * @param {boolean} [useNodeTestRunner=false] - Whether to invoke via `node --test` runner instead of plain node. + * @returns {{ status: number | null, signal: NodeJS.Signals | null, stdout: string, stderr: string, logDir: string, records: readonly DiagnosticRecord[] }} + */ +function runWithPreload( + code: string, + envOverride: Record = {}, + useNodeTestRunner = false, +) { + return runChild(code, envOverride, { useNodeTestRunner }); +} + /** * Executes a synthetic child script with a test-only safe abort shim loaded BEFORE the real preload. * @@ -113,74 +134,24 @@ function runWithPreload( function runWithSafeAbortShimAndPreload( code: string, envOverride: Record = {}, -): { - readonly status: number | null; - readonly signal: NodeJS.Signals | null; - readonly stdout: string; - readonly stderr: string; - readonly logDir: string; - readonly records: readonly DiagnosticRecord[]; -} { - const generatedLogDir = fs.mkdtempSync(path.join(os.tmpdir(), 'sigb-test-')); - const rawLogDir = envOverride.SIGB_LOG_DIR; - const targetLogDir = rawLogDir - ? path.resolve(generatedLogDir, rawLogDir) - : generatedLogDir; - const shimPath = path.join(generatedLogDir, 'safe-abort-shim.cjs'); - const scriptPath = path.join(generatedLogDir, 'test-target.cjs'); - - fs.writeFileSync( - shimPath, - ` - const fs = require('node:fs'); - process.abort = function harmlessSentinelAbort(...args) { - fs.writeSync(1, '__SAFE_ABORT_SENTINEL_INVOKED__\\n'); - process.exit(99); - }; - `, - 'utf8', - ); - - fs.writeFileSync(scriptPath, code, 'utf8'); - - const args = ['--require', shimPath, '--require', PRELOAD_PATH, scriptPath]; - - const env: Record = { - ...process.env, - SIGB_LOG_DIR: rawLogDir ?? targetLogDir, - }; - delete env.NODE_TEST_CONTEXT; - for (const [k, v] of Object.entries(envOverride)) { - env[k] = v; - } - - const result = spawnSync(process.execPath, args, { - cwd: generatedLogDir, - env, - encoding: 'utf8', - windowsHide: true, +) { + return runChild(code, envOverride, { + extraPreloads: (dir: string) => { + const shimPath = path.join(dir, 'safe-abort-shim.cjs'); + fs.writeFileSync( + shimPath, + ` + const fs = require('node:fs'); + process.abort = function harmlessSentinelAbort(...args) { + fs.writeSync(1, '__SAFE_ABORT_SENTINEL_INVOKED__\\n'); + process.exit(99); + }; + `, + 'utf8', + ); + return [shimPath]; + }, }); - - const files = fs.existsSync(targetLogDir) - ? fs.readdirSync(targetLogDir).filter((f) => f.endsWith('.jsonl')) - : []; - const records: DiagnosticRecord[] = []; - for (const file of files) { - const content = fs.readFileSync(path.join(targetLogDir, file), 'utf8'); - const lines = content.split('\n').map((l) => l.trim()).filter(Boolean); - for (const line of lines) { - records.push(JSON.parse(line) as DiagnosticRecord); - } - } - - return { - status: result.status, - signal: result.signal, - stdout: result.stdout, - stderr: result.stderr, - logDir: generatedLogDir, - records, - }; } test('diagnostic preload: normal shutdown records lifecycle events', (t) => { @@ -299,7 +270,7 @@ test('diagnostic preload: safeErr serialization does not throw on BigInt or circ t.after(() => fs.rmSync(logDir, { recursive: true, force: true })); const warnings = records.filter((r) => r.kind === 'warning'); - assert.equal(warnings.length, 3, 'all 3 warning records successfully serialized and written'); + assert.ok(warnings.length >= 3, 'all 3 emitted warning records successfully serialized and written'); const bigIntWarning = warnings.find((w) => w.err?.message?.includes('bigint')); assert.ok(bigIntWarning, 'bigint warning logged'); From 925f0c25e27abee1fb27ae5b7bc01b06a0129e83 Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 09:25:52 +0300 Subject: [PATCH 13/20] fix(api): redact err.name in diagnostic safeErr serialization - Redact string value of err.name in safeErr to prevent credential leakage via custom error names - Add regression assertion in signature-b-preload.test.ts verifying error name redaction --- packages/api/test/diagnostics/signature-b-preload.cjs | 2 +- packages/api/test/diagnostics/signature-b-preload.test.ts | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/api/test/diagnostics/signature-b-preload.cjs b/packages/api/test/diagnostics/signature-b-preload.cjs index 4758a209..b3ce67ee 100644 --- a/packages/api/test/diagnostics/signature-b-preload.cjs +++ b/packages/api/test/diagnostics/signature-b-preload.cjs @@ -117,7 +117,7 @@ function safeErr(err) { } try { return { - name: typeof err.name === 'string' ? err.name : undefined, + name: typeof err.name === 'string' ? redact(err.name) : undefined, message: redact(String(err.message ?? '')), stack: redact(String(err.stack ?? '')), code: normalizeMeta(err.code), diff --git a/packages/api/test/diagnostics/signature-b-preload.test.ts b/packages/api/test/diagnostics/signature-b-preload.test.ts index 54572b12..3a63ebbd 100644 --- a/packages/api/test/diagnostics/signature-b-preload.test.ts +++ b/packages/api/test/diagnostics/signature-b-preload.test.ts @@ -312,13 +312,18 @@ test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Autho 'Authorization: Basic dXNlcjpwYXNzd29yZA==,', 'Cookie: session=xyz123; token=abc456; other=789', ]; - throw new Error(parts.join(' and ')); + const err = new Error(parts.join(' and ')); + err.name = 'CustomError password="supersecretname"'; + throw err; `); t.after(() => fs.rmSync(logDir, { recursive: true, force: true })); const uncaught = records.find((r) => r.kind === 'uncaughtExceptionMonitor'); assert.ok(uncaught, 'uncaught record exists'); const msg = uncaught?.err?.message ?? ''; + const errName = uncaught?.err?.name ?? ''; + assert.ok(errName.includes('password=[REDACTED]'), 'redacts password in error name'); + assert.ok(!errName.includes('supersecretname'), 'raw password not present in error name'); assert.ok(msg.includes('postgres://[REDACTED_CREDS]@db.internal:5432/chess'), 'redacts postgresql credentials'); assert.ok(msg.includes('postgres://[REDACTED_CREDS]@10.0.0.1:5432/main'), 'redacts postgres credentials'); assert.ok(msg.includes('[REDACTED_API_KEY]'), 'redacts OpenAI-style api key'); From 046bcf25c3112528d24a914f819848454baa2d61 Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 09:36:48 +0300 Subject: [PATCH 14/20] docs(roadmap): qualify unresolved Signature B cause as neutral origin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace phrase claiming cause is outside repository code with neutral statement that origin is not yet established - Align ROADMAP.md Signature B summary with ADR-0140 §4 conclusion --- docs/ROADMAP.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index eba04b6c..d6846cb5 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -1349,7 +1349,7 @@ Debt observed during M14. Each states what is known, not what is planned; items - **The published capability document could omit a composed feature, and the client offered controls that could not work (RESOLVED in M15 Increment 24 / ADR-0132).** Left open by Increment 23 above. `capabilitiesView` (`packages/api/src/presenters.ts`) took a hand-written `Pick`, so a feature never added to it was invisible to `GET /v1/capabilities` and nothing complained. ADR-0131 judged that "a narrower failure than a 503 — the feature works for anyone who calls the route directly", and deferred it on the grounds that closing it meant first deciding which optional dependencies are user-facing capabilities. **Both halves of that were wrong.** The failure is narrower only when the client can still reach the feature, and a live instance already existed where it could not: `GET /v1/search` served three modes from two independently-gated dependency sets behind one published `search` flag, so a deployment running the Helm chart’s `search.semanticEnabled: false` advertised search while the client offered two mode buttons whose every request answered 503. And the judgement, while genuinely underivable, can be made **unskippable**, which was the property wanted. `semanticSearch` is now published from both of its dependencies, `Exclude[0] | NotAPublishedCapability>` must be `never` so a new optional dependency cannot compile until someone gives it a flag or records why it has none, and a behavioural guard requires every capability source to change what the document publishes — because a key can sit in that parameter unread, which the compile-time half cannot see. The mutation ledger lives in ADR-0132 and is not restated here. - **The search surface was not gated on `capabilities.search`, so an absolute kill switch still showed a search box (RESOLVED in M15 Increment 24 / ADR-0132 §5).** `SEARCH_ENABLED=0` — the chart's `search.enabled: false`, an absolute kill switch per ADR-0055 — leaves `searchRepository` unconstructed and `GET /v1/search` answering 503 on every mode, keyword included. The entry point was the persistent header form in `packages/web/index.html`, present on every page; being a `` rather than an `a[data-route]`, `NAV_CAPABILITY_MAP` could not reach it, which is why the first pass at Increment 24 gated the semantic and hybrid *modes* and left keyword ungated — the same defect class one mode over. Raised by the Qodo review of PR #155. **Fixed in the same increment rather than deferred:** the form now ships `hidden` and is revealed by `applySearchCapability` only on an explicit `search: true`; the route renders an honest unavailable notice and issues no request; and keyword search waits for the capability answer, reversing this increment's own earlier latency decision, because knowing whether a request is pointless requires having asked. A markup-contract test pins the `hidden` attribute, since the gate depends on it and every other test passes without it. - **Clicking a search mode discarded text typed since the page loaded (RESOLVED in a follow-up to M15 Increment 24 / ADR-0132).** `createModeInput` in `packages/web/src/app/search-mount.ts` closed over the query captured when the route mounted, so `navigateToSearchMode` navigated with the old term and the remount reset the input to match it. Type a new term into the header field, click **Semantic** without pressing enter, and the typed text was gone with no indication it had been discarded. Pre-existing — the closure predated Increment 24 and was untouched by it — and found by the adversarial review of PR #155 while reviewing the capability gate wrapped around the same control. **Resolved:** the query is now a `() => string` read when a mode is chosen rather than a string captured when the selector renders, matching what `main.ts`'s submit handler already does, and falling back to the mounted query only where the document has no header input. Two regression tests, one of which fails against the exact pre-fix closure. -- **`startHarness` drew ports `fetch` refuses (RESOLVED in ADR-0140); the unexplained whole-file failure is still open.** Two signatures were filed here, deliberately not as one cause, and that judgement held. **Signature A is resolved.** WHATWG Fetch blocks eighty-two ports and undici enforces the list on the port number alone, before opening a socket — so `server.listen(0)` could bind, listen and answer raw TCP while `fetch` still refused, surfacing as `TypeError: fetch failed` / `Error: bad port` at the harness’s first request rather than at the listen that caused it. Whether it can happen at all is a property of the host’s dynamic port range: a typical Linux CI range (32768–60999) contains no blocked port, while the Windows range in use here (1024–15000) contains nineteen, which is the whole of "green on CI, flaky locally". The guard that already existed was incomplete in a way that still failed — its hand-observed set of eighteen ports was the spec list intersected with one machine’s range **minus `6679`** — and it retried unboundedly, had no behaviour on exhaustion, and had been copy-pasted into `auth-signin-schema.integration.test.ts`, so the missing port had to be found twice. `packages/api/test/listen.ts` now owns port acquisition for both sites: the spec-complete eighty-two ports (verified by sweeping all 65535 through the real `fetch` on Node v24.15.0), a bounded twenty attempts, each rejected listener closed before the next is asked for, a guarded `address()` read in place of the `as AddressInfo` cast, and an exhaustion error naming the attempts and rejected ports and nothing else. **Signature B is not resolved and was not folded in.** A file still fails with `'test failed'`, no assertion, no stack and none of its own tests reported. Twenty consecutive full runs gave five failures: on pre-fix code one signature A (`auth.test.js`) and three signature B; on post-fix code one signature B and no signature A. The port fix removes A and leaves B exactly where it was, which is the evidence that they are two defects. Four different files were hit (`move-explanation-route`, `tournament-commentary-route`, `bot-detection-analyze`, `anti-cheat-analysis`), sharing no import beyond `./helpers`; each died in 589–703 ms with no test of its own reporting and no stderr. Refuted with evidence: ephemeral-port exhaustion (113 sockets in TIME_WAIT against a 13977-port range), a `Promise.race` loser becoming an unhandled rejection (`race` subscribes to every promise, confirmed on v24.15.0), a throwing `after`/`afterEach` hook (the affected files use none), and a double `close()` rejecting (awaited in a `finally`, it would be attributed to that test with a stack). A second bounded pass — twelve more full runs under the TAP reporter with a preload recording `uncaughtException`, `unhandledRejection` and any non-zero exit — produced twelve clean runs and captured nothing at the time. **A follow-up increment (`claude/node-test-signature-b`) then captured the defect directly, three more times, on three files never previously implicated** (`rate-limit-atomicity`, `dependency-parity`, `studies-api`) — seven distinct files observed with this symptom to date. Occurrences across seven distinct files make a shared or cross-cutting path more plausible and make a defect confined to one test file less likely, but do not exclude file-specific inputs or lifecycle interactions. An instrumented preload (`packages/api/test/diagnostics/signature-b-preload.cjs`) hooking process-level events — `process.exit`, `process.abort`, `process.kill`, `uncaughtExceptionMonitor` (passively observing uncaught exceptions and fatal unhandled rejections), `warning`, `beforeExit`, and Node’s own unconditional `exit` — showed **none of the hooks active at the time fired** on any of the three historical captures (though `process.abort()` was not wrapped in those initial runs and is now covered for future occurrences). A synthetic `process.exit(1)`-before-registration fixture reproduces the identical silent shape; every other synthetic mechanism tried (a post-test async throw, an emitter `'error'` with its listener removed, a synchronous module-load throw, a delayed `SIGKILL`) prints a visibly different diagnostic line, stack, or partial test output that the real defect never shows. This narrows the investigated possibilities while leaving the root cause unresolved: the per-file child process (`node --test` spawns one per file, confirmed by distinct PIDs) was not terminated by `process.exit`, uncaught exceptions, or fatal unhandled rejections, and future runs with `process.abort` instrumentation will record whether abort was called through JS; an absent record narrows in-runtime JS termination but cannot alone prove external termination without corroborating child exit status/signal data or OS-level crash evidence (e.g. distinguishing an external kill or uncatchable signal from a native C++/V8 crash). The machine had roughly 2.5 GB of 15.7 GB RAM free at capture time with several other agents’ processes concurrently running, which is circumstantially consistent with resource contention, but no crash was recorded in the Windows Application or System event logs in that window, so the exact external trigger is still not established. No fix was invented — the forbidden responses (sleeps, whole-file retries, lowering concurrency) would only hide a cause that remains outside this repository’s code. See ADR-0140 §4. +- **`startHarness` drew ports `fetch` refuses (RESOLVED in ADR-0140); the unexplained whole-file failure is still open.** Two signatures were filed here, deliberately not as one cause, and that judgement held. **Signature A is resolved.** WHATWG Fetch blocks eighty-two ports and undici enforces the list on the port number alone, before opening a socket — so `server.listen(0)` could bind, listen and answer raw TCP while `fetch` still refused, surfacing as `TypeError: fetch failed` / `Error: bad port` at the harness’s first request rather than at the listen that caused it. Whether it can happen at all is a property of the host’s dynamic port range: a typical Linux CI range (32768–60999) contains no blocked port, while the Windows range in use here (1024–15000) contains nineteen, which is the whole of "green on CI, flaky locally". The guard that already existed was incomplete in a way that still failed — its hand-observed set of eighteen ports was the spec list intersected with one machine’s range **minus `6679`** — and it retried unboundedly, had no behaviour on exhaustion, and had been copy-pasted into `auth-signin-schema.integration.test.ts`, so the missing port had to be found twice. `packages/api/test/listen.ts` now owns port acquisition for both sites: the spec-complete eighty-two ports (verified by sweeping all 65535 through the real `fetch` on Node v24.15.0), a bounded twenty attempts, each rejected listener closed before the next is asked for, a guarded `address()` read in place of the `as AddressInfo` cast, and an exhaustion error naming the attempts and rejected ports and nothing else. **Signature B is not resolved and was not folded in.** A file still fails with `'test failed'`, no assertion, no stack and none of its own tests reported. Twenty consecutive full runs gave five failures: on pre-fix code one signature A (`auth.test.js`) and three signature B; on post-fix code one signature B and no signature A. The port fix removes A and leaves B exactly where it was, which is the evidence that they are two defects. Four different files were hit (`move-explanation-route`, `tournament-commentary-route`, `bot-detection-analyze`, `anti-cheat-analysis`), sharing no import beyond `./helpers`; each died in 589–703 ms with no test of its own reporting and no stderr. Refuted with evidence: ephemeral-port exhaustion (113 sockets in TIME_WAIT against a 13977-port range), a `Promise.race` loser becoming an unhandled rejection (`race` subscribes to every promise, confirmed on v24.15.0), a throwing `after`/`afterEach` hook (the affected files use none), and a double `close()` rejecting (awaited in a `finally`, it would be attributed to that test with a stack). A second bounded pass — twelve more full runs under the TAP reporter with a preload recording `uncaughtException`, `unhandledRejection` and any non-zero exit — produced twelve clean runs and captured nothing at the time. **A follow-up increment (`claude/node-test-signature-b`) then captured the defect directly, three more times, on three files never previously implicated** (`rate-limit-atomicity`, `dependency-parity`, `studies-api`) — seven distinct files observed with this symptom to date. Occurrences across seven distinct files make a shared or cross-cutting path more plausible and make a defect confined to one test file less likely, but do not exclude file-specific inputs or lifecycle interactions. An instrumented preload (`packages/api/test/diagnostics/signature-b-preload.cjs`) hooking process-level events — `process.exit`, `process.abort`, `process.kill`, `uncaughtExceptionMonitor` (passively observing uncaught exceptions and fatal unhandled rejections), `warning`, `beforeExit`, and Node’s own unconditional `exit` — showed **none of the hooks active at the time fired** on any of the three historical captures (though `process.abort()` was not wrapped in those initial runs and is now covered for future occurrences). A synthetic `process.exit(1)`-before-registration fixture reproduces the identical silent shape; every other synthetic mechanism tried (a post-test async throw, an emitter `'error'` with its listener removed, a synchronous module-load throw, a delayed `SIGKILL`) prints a visibly different diagnostic line, stack, or partial test output that the real defect never shows. This narrows the investigated possibilities while leaving the root cause unresolved: the per-file child process (`node --test` spawns one per file, confirmed by distinct PIDs) was not terminated by `process.exit`, uncaught exceptions, or fatal unhandled rejections, and future runs with `process.abort` instrumentation will record whether abort was called through JS; an absent record narrows in-runtime JS termination but cannot alone prove external termination without corroborating child exit status/signal data or OS-level crash evidence (e.g. distinguishing an external kill or uncatchable signal from a native C++/V8 crash). The machine had roughly 2.5 GB of 15.7 GB RAM free at capture time with several other agents’ processes concurrently running, which is circumstantially consistent with resource contention, but no crash was recorded in the Windows Application or System event logs in that window, so the exact external trigger is still not established. No fix was invented — the forbidden responses (sleeps, whole-file retries, lowering concurrency) would only hide the unresolved root cause, whose origin is not yet established. See ADR-0140 §4. - **`ApiServer.listen` registered no `'error'` handler, so a failed bind hung and raised an uncaught event (RESOLVED in ADR-0140 §5).** `packages/api/src/server.ts` resolved its promise from the `listening` callback only and built it with no reject path. A bind failing asynchronously (`EADDRINUSE`, `EMFILE`) left the promise pending forever and, with no `'error'` listener on the `http.Server`, was re-raised as an uncaught exception. Found while investigating ADR-0140 and independently raised by the Qodo review of PR #21. **Resolved in the same increment** rather than deferred, because ADR-0140 §2's bounded, diagnosable acquisition is not true without it — the retry can only report a bind error if the listener it is handed rejects. A one-shot `'error'` listener now rejects and is removed once listening, so later server errors keep their previous semantics rather than being swallowed by a `reject` on a settled promise. The regression test fails against the exact pre-fix code, through an uncaught `ERR_UNHANDLED_ERROR`. - **`main.ts`'s controller-disposal list is manual, untested, and silently incomplete when a section is added (RESOLVED in Increment 25 / ADR-0092).** `run()` in `packages/web/src/main.ts` disposes the previous route's controllers by name, and its own comment says doing so "is what makes re-bootstrapping safe" — but adding a section to `bootstrap` and forgetting to add it there compiles, passes every gate, and leaks. Increment 23 shipped exactly that omission for `LearningController` and it was caught in PR review, not by a test. `main.ts` has no test coverage of any kind, so no section's disposal is verified. A structural fix (bootstrap returning its disposables as a collection, or a type-level exhaustiveness check keyed off the result type) would make the next omission a compile error; it is a refactor across ~15 return sites and belongs in its own increment. **Resolved in Increment 25 (ADR-0092):** extracted `createLifecycle` run loop in `lifecycle.ts`, defined `BootstrappedDisposables` and `DisposableKey` driving `DISPOSABLE_TEARDOWN_MAP: Record` for compile-time exhaustiveness, normalised `.dispose()` verb across all disposables, and cascaded `GameController.stop()` to `gameSync.stop()`. - **`stepView` sends the answers to the learner (RESOLVED in Increment 29 / ADR-0095).** `packages/api/src/presenters.ts` emits `expectedSan` on a move step and `correctIndex` on a quiz step, and `GET /v1/lessons/:id/steps` is the route the learner's own lesson page calls. Increment 23 omits both from the client-side types (`packages/web/src/api/models.ts`), so the app cannot render or grade against them and a future edit that tries becomes a compile error — but the fields are still on the wire and readable in devtools. The authoring routes legitimately need them returned to the author, so the fix is a learner-scoped step view (or a caller-dependent projection), not a deletion: an API contract decision with its own ADR. Nothing rated or rewarded depends on step progress today, so this is a wart rather than a breach. **Resolved in Increment 29 (ADR-0095):** added `LearnerStepView` / `learnerStepView` in `packages/api/src/presenters.ts` omitting `expectedSan` and `correctIndex`. `GET /v1/lessons/:id/steps` and `GET /v1/steps/:id` now check course authorship via `repo.getLesson` / `repo.getCourse`, returning full `stepView` to the author and `learnerStepView` to learners and anonymous callers. Updated OpenAPI schema and web model comments. The first attempt resolved authorship with a separate `getLesson` + `getCourse` after the step read, which doubled both routes from 3 SQL queries to 6 because `listSteps` / `getStep` had already made those reads internally and discarded the course; caught in the PR #92 review and fixed by adding `getStepWithCourse` / `listStepsWithCourse` to `LearningRepository`, which return what was already loaded. Both routes now make exactly one repository call, pinned by a counting-proxy test. From 7275f7d508398ed90266aa85b154ff35767d86a3 Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 09:50:50 +0300 Subject: [PATCH 15/20] fix(api): resolve relative SIGB_LOG_DIR against child cwd in abort diagnostic - Resolve SIGB_LOG_DIR override relative to child generatedLogDir in runDiagnosticChild helper - Match child working directory resolution used across diagnostic runners --- .../api/test/diagnostics/signature-b-preload-abort.diag.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts b/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts index b8261d6a..8a40d07d 100644 --- a/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts +++ b/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts @@ -43,7 +43,10 @@ function runDiagnosticChild( readonly records: readonly DiagnosticRecord[]; } { const generatedLogDir = fs.mkdtempSync(path.join(os.tmpdir(), 'sigb-abort-diag-')); - const targetLogDir = envOverride.SIGB_LOG_DIR ? path.resolve(envOverride.SIGB_LOG_DIR) : generatedLogDir; + const rawLogDir = envOverride.SIGB_LOG_DIR; + const targetLogDir = rawLogDir + ? path.resolve(generatedLogDir, rawLogDir) + : generatedLogDir; const scriptPath = path.join(generatedLogDir, 'abort-target.cjs'); fs.writeFileSync(scriptPath, code, 'utf8'); @@ -51,7 +54,7 @@ function runDiagnosticChild( const env = { ...process.env, ...envOverride, - SIGB_LOG_DIR: targetLogDir, + SIGB_LOG_DIR: rawLogDir ?? targetLogDir, }; const result = From fe9dfabaf4bc8dd91f90f98a41872f9872cbdc85 Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 10:16:35 +0300 Subject: [PATCH 16/20] fix(api): redact password hash variants and normalize empty SIGB_LOG_DIR - Extend sensitive-key redaction pattern in signature-b-preload.cjs to match passwordHash, password_hash, and password hash - Normalize targetLogDir resolution across empty-string, relative, and absolute overrides in diagnostic runners - Add regression tests for password hash redaction and empty-string SIGB_LOG_DIR fallback --- .../signature-b-preload-abort.diag.ts | 48 +++++++++++++-- .../test/diagnostics/signature-b-preload.cjs | 2 +- .../diagnostics/signature-b-preload.test.ts | 59 +++++++++++++++++-- 3 files changed, 97 insertions(+), 12 deletions(-) diff --git a/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts b/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts index 8a40d07d..6e87fdde 100644 --- a/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts +++ b/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts @@ -40,13 +40,20 @@ function runDiagnosticChild( readonly status: number | null; readonly signal: NodeJS.Signals | null; readonly logDir: string; + readonly targetLogDir: string; readonly records: readonly DiagnosticRecord[]; } { const generatedLogDir = fs.mkdtempSync(path.join(os.tmpdir(), 'sigb-abort-diag-')); const rawLogDir = envOverride.SIGB_LOG_DIR; - const targetLogDir = rawLogDir - ? path.resolve(generatedLogDir, rawLogDir) - : generatedLogDir; + const defaultPreloadLogDir = path.join(os.tmpdir(), 'sigb-diag'); + const targetLogDir = + rawLogDir === undefined + ? generatedLogDir + : rawLogDir === '' + ? defaultPreloadLogDir + : path.isAbsolute(rawLogDir) + ? rawLogDir + : path.resolve(generatedLogDir, rawLogDir); const scriptPath = path.join(generatedLogDir, 'abort-target.cjs'); fs.writeFileSync(scriptPath, code, 'utf8'); @@ -54,7 +61,7 @@ function runDiagnosticChild( const env = { ...process.env, ...envOverride, - SIGB_LOG_DIR: rawLogDir ?? targetLogDir, + SIGB_LOG_DIR: rawLogDir !== undefined ? rawLogDir : targetLogDir, }; const result = @@ -73,7 +80,13 @@ function runDiagnosticChild( }); const files = fs.existsSync(targetLogDir) - ? fs.readdirSync(targetLogDir).filter((f) => f.endsWith('.jsonl')) + ? fs + .readdirSync(targetLogDir) + .filter( + (f) => + f.endsWith('.jsonl') && + (rawLogDir === '' && result.pid ? f.startsWith(`run-${result.pid}-`) : true), + ) : []; const records: DiagnosticRecord[] = []; for (const file of files) { @@ -88,6 +101,7 @@ function runDiagnosticChild( status: result.status, signal: result.signal, logDir: generatedLogDir, + targetLogDir, records, }; } @@ -110,3 +124,27 @@ test('explicit diagnostic integration: real native process.abort() records synch const exitRecord = records.find((r) => r.kind === 'exit'); assert.equal(exitRecord, undefined, 'exit event must not fire on process.abort'); }); + +test('explicit diagnostic integration: empty SIGB_LOG_DIR falls back to default directory without path split', (t) => { + const { status, signal, records, logDir, targetLogDir } = runDiagnosticChild( + ` + process.abort(); + `, + { SIGB_LOG_DIR: '' }, + ); + t.after(() => { + fs.rmSync(logDir, { recursive: true, force: true }); + if (fs.existsSync(targetLogDir) && targetLogDir !== logDir) { + const files = fs.readdirSync(targetLogDir).filter((f) => f.endsWith('.jsonl')); + for (const file of files) { + if (records.some((r) => file.includes(String(r.pid)))) { + fs.rmSync(path.join(targetLogDir, file), { force: true }); + } + } + } + }); + + assert.ok(status !== 0 || signal !== null, 'process terminates abnormally'); + assert.ok(records.some((r) => r.kind === 'preload-installed'), 'records preload-installed in default log dir'); + assert.ok(records.some((r) => r.kind === 'process.abort'), 'records process.abort in default log dir'); +}); diff --git a/packages/api/test/diagnostics/signature-b-preload.cjs b/packages/api/test/diagnostics/signature-b-preload.cjs index b3ce67ee..f3e11b18 100644 --- a/packages/api/test/diagnostics/signature-b-preload.cjs +++ b/packages/api/test/diagnostics/signature-b-preload.cjs @@ -67,7 +67,7 @@ const REDACT_PATTERNS = [ [/sk-[a-zA-Z0-9_-]{10,}/g, '[REDACTED_API_KEY]'], [/Bearer\s+[A-Za-z0-9._~+/-]+=*/g, 'Bearer [REDACTED_TOKEN]'], [/(?:postgres|postgresql):\/\/[^:]+:[^@]+@/g, 'postgres://[REDACTED_CREDS]@'], - [/(password|secret|token|authorization|cookie)\s*[:=]\s*(?:"(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*'|[^,\r\n]+)/gi, '$1=[REDACTED]'], + [/(password(?:[_\s-]?hash|Hash)?|secret|token|authorization|cookie)\s*[:=]\s*(?:"(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*'|[^,\r\n]+)/gi, '$1=[REDACTED]'], ]; /** diff --git a/packages/api/test/diagnostics/signature-b-preload.test.ts b/packages/api/test/diagnostics/signature-b-preload.test.ts index 3a63ebbd..864e8451 100644 --- a/packages/api/test/diagnostics/signature-b-preload.test.ts +++ b/packages/api/test/diagnostics/signature-b-preload.test.ts @@ -48,13 +48,20 @@ function runChild( readonly stdout: string; readonly stderr: string; readonly logDir: string; + readonly targetLogDir: string; readonly records: readonly DiagnosticRecord[]; } { const generatedLogDir = fs.mkdtempSync(path.join(os.tmpdir(), 'sigb-test-')); const rawLogDir = envOverride.SIGB_LOG_DIR; - const targetLogDir = rawLogDir - ? path.resolve(generatedLogDir, rawLogDir) - : generatedLogDir; + const defaultPreloadLogDir = path.join(os.tmpdir(), 'sigb-diag'); + const targetLogDir = + rawLogDir === undefined + ? generatedLogDir + : rawLogDir === '' + ? defaultPreloadLogDir + : path.isAbsolute(rawLogDir) + ? rawLogDir + : path.resolve(generatedLogDir, rawLogDir); const scriptPath = path.join(generatedLogDir, 'test-target.cjs'); fs.writeFileSync(scriptPath, code, 'utf8'); @@ -68,7 +75,8 @@ function runChild( const env: Record = { ...process.env, - SIGB_LOG_DIR: rawLogDir ?? targetLogDir, + ...envOverride, + SIGB_LOG_DIR: rawLogDir !== undefined ? rawLogDir : targetLogDir, }; delete env.NODE_TEST_CONTEXT; for (const [k, v] of Object.entries(envOverride)) { @@ -83,7 +91,13 @@ function runChild( }); const files = fs.existsSync(targetLogDir) - ? fs.readdirSync(targetLogDir).filter((f) => f.endsWith('.jsonl')) + ? fs + .readdirSync(targetLogDir) + .filter( + (f) => + f.endsWith('.jsonl') && + (rawLogDir === '' && result.pid ? f.startsWith(`run-${result.pid}-`) : true), + ) : []; const records: DiagnosticRecord[] = []; for (const file of files) { @@ -100,6 +114,7 @@ function runChild( stdout: result.stdout, stderr: result.stderr, logDir: generatedLogDir, + targetLogDir, records, }; } @@ -301,7 +316,7 @@ test('diagnostic preload: process.kill records target PID and signal', (t) => { assert.ok(killRecord?.callerStack?.includes('kill-call-site'), 'traces kill call site'); }); -test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Authorization headers, cookies, and quoted secrets with spaces or escaped quotes', (t) => { +test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Authorization headers, cookies, quoted secrets, and password hashes', (t) => { const { records, logDir } = runWithPreload(` const parts = [ 'Connect failed to postgresql://app_user:s3cr3tpass@db.internal:5432/chess', @@ -309,6 +324,9 @@ test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Autho 'sk-1234567890abcdef12345', 'password="correct \\\\\\"horse\\\\\\" battery staple"', "secret='top \\\\\\'secret\\\\\\' key phrase'", + 'passwordHash="$2b$12$e8uq4abcdefghijklmnopqrstuvwxyz1234567890"', + "password_hash='$2b$12$snakecasehash1234567890abcdefghijklmn'", + 'password hash: "spacedhash1234567890abcdefghijklmn"', 'Authorization: Basic dXNlcjpwYXNzd29yZA==,', 'Cookie: session=xyz123; token=abc456; other=789', ]; @@ -321,6 +339,7 @@ test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Autho const uncaught = records.find((r) => r.kind === 'uncaughtExceptionMonitor'); assert.ok(uncaught, 'uncaught record exists'); const msg = uncaught?.err?.message ?? ''; + const stack = uncaught?.err?.stack ?? ''; const errName = uncaught?.err?.name ?? ''; assert.ok(errName.includes('password=[REDACTED]'), 'redacts password in error name'); assert.ok(!errName.includes('supersecretname'), 'raw password not present in error name'); @@ -329,12 +348,21 @@ test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Autho assert.ok(msg.includes('[REDACTED_API_KEY]'), 'redacts OpenAI-style api key'); assert.ok(msg.includes('password=[REDACTED]'), 'redacts double-quoted password with escaped quotes'); assert.ok(msg.includes('secret=[REDACTED]'), 'redacts single-quoted secret with escaped quotes'); + assert.ok(msg.includes('passwordHash=[REDACTED]'), 'redacts passwordHash'); + assert.ok(msg.includes('password_hash=[REDACTED]'), 'redacts password_hash'); + assert.ok(msg.includes('password hash=[REDACTED]'), 'redacts password hash'); assert.ok(msg.includes('Authorization=[REDACTED]'), 'redacts Basic authorization header'); assert.ok(msg.includes('Cookie=[REDACTED]'), 'redacts multi-cookie header'); assert.ok(!msg.includes('s3cr3tpass'), 'raw password 1 not present'); assert.ok(!msg.includes('supersecret'), 'raw password 2 not present'); assert.ok(!msg.includes('horse'), 'raw quoted password content with escaped quotes not present'); assert.ok(!msg.includes('key phrase'), 'raw single-quoted secret content with escaped quotes not present'); + assert.ok(!msg.includes('$2b$12$e8uq4abcdefghijklmnopqrstuvwxyz1234567890'), 'raw passwordHash absent from message'); + assert.ok(!msg.includes('$2b$12$snakecasehash1234567890abcdefghijklmn'), 'raw password_hash absent from message'); + assert.ok(!msg.includes('spacedhash1234567890abcdefghijklmn'), 'raw password hash absent from message'); + assert.ok(!stack.includes('$2b$12$e8uq4abcdefghijklmnopqrstuvwxyz1234567890'), 'raw passwordHash absent from stack'); + assert.ok(!stack.includes('$2b$12$snakecasehash1234567890abcdefghijklmn'), 'raw password_hash absent from stack'); + assert.ok(!stack.includes('spacedhash1234567890abcdefghijklmn'), 'raw password hash absent from stack'); assert.ok(!msg.includes('dXNlcjpwYXNzd29yZA=='), 'raw basic auth credential not present'); assert.ok(!msg.includes('session=xyz123'), 'raw cookie session not present'); assert.ok(!msg.includes('token=abc456'), 'raw cookie token not present'); @@ -393,6 +421,25 @@ test('diagnostic preload: relative SIGB_LOG_DIR resolves correctly', (t) => { assert.ok(records.length > 0, 'reads child diagnostic records from relative SIGB_LOG_DIR'); }); +test('diagnostic preload: empty SIGB_LOG_DIR falls back to default directory without path split', (t) => { + const { status, records, logDir, targetLogDir } = runWithPreload('const ok = true;', { SIGB_LOG_DIR: '' }); + t.after(() => { + fs.rmSync(logDir, { recursive: true, force: true }); + if (fs.existsSync(targetLogDir) && targetLogDir !== logDir) { + const files = fs.readdirSync(targetLogDir).filter((f) => f.endsWith('.jsonl')); + for (const file of files) { + if (records.some((r) => file.includes(String(r.pid)))) { + fs.rmSync(path.join(targetLogDir, file), { force: true }); + } + } + } + }); + + assert.equal(status, 0, 'runs cleanly with empty SIGB_LOG_DIR override'); + assert.ok(records.length > 0, 'wrote and read diagnostic records from fallback default directory'); + assert.ok(records.some((r) => r.kind === 'preload-installed'), 'records preload-installed in fallback directory'); +}); + test('diagnostic preload: usage glob pattern matches test files without literal backslash and excludes diag files', () => { const correctGlob = 'dist-test/test/**/*.test.js'; const badEscapedGlob = 'dist-test/test/**\\/*.test.js'; From 6094e200887e20cf54e7ef85b39ac2f603d2097e Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 10:35:00 +0300 Subject: [PATCH 17/20] fix(api): redact JSON-quoted sensitive keys and anchor fallback log cleanup by prefix - Match optional JSON quotes around sensitive keys in signature-b-preload.cjs - Anchor cleanup in fallback log directory to exact run-\39212- filename prefix - Add regression tests for JSON-formatted credentials and password hashes --- .../signature-b-preload-abort.diag.ts | 2 +- .../test/diagnostics/signature-b-preload.cjs | 2 +- .../diagnostics/signature-b-preload.test.ts | 17 +++++++++++++++-- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts b/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts index 6e87fdde..80caa387 100644 --- a/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts +++ b/packages/api/test/diagnostics/signature-b-preload-abort.diag.ts @@ -137,7 +137,7 @@ test('explicit diagnostic integration: empty SIGB_LOG_DIR falls back to default if (fs.existsSync(targetLogDir) && targetLogDir !== logDir) { const files = fs.readdirSync(targetLogDir).filter((f) => f.endsWith('.jsonl')); for (const file of files) { - if (records.some((r) => file.includes(String(r.pid)))) { + if (records.some((r) => file.startsWith(`run-${r.pid}-`))) { fs.rmSync(path.join(targetLogDir, file), { force: true }); } } diff --git a/packages/api/test/diagnostics/signature-b-preload.cjs b/packages/api/test/diagnostics/signature-b-preload.cjs index f3e11b18..4df9af3f 100644 --- a/packages/api/test/diagnostics/signature-b-preload.cjs +++ b/packages/api/test/diagnostics/signature-b-preload.cjs @@ -67,7 +67,7 @@ const REDACT_PATTERNS = [ [/sk-[a-zA-Z0-9_-]{10,}/g, '[REDACTED_API_KEY]'], [/Bearer\s+[A-Za-z0-9._~+/-]+=*/g, 'Bearer [REDACTED_TOKEN]'], [/(?:postgres|postgresql):\/\/[^:]+:[^@]+@/g, 'postgres://[REDACTED_CREDS]@'], - [/(password(?:[_\s-]?hash|Hash)?|secret|token|authorization|cookie)\s*[:=]\s*(?:"(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*'|[^,\r\n]+)/gi, '$1=[REDACTED]'], + [/(["']?)(password(?:[_\s-]?hash|Hash)?|secret|token|authorization|cookie)\1\s*[:=]\s*(?:"(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*'|[^,\r\n}]+)/gi, '$1$2$1=[REDACTED]'], ]; /** diff --git a/packages/api/test/diagnostics/signature-b-preload.test.ts b/packages/api/test/diagnostics/signature-b-preload.test.ts index 864e8451..e0532a62 100644 --- a/packages/api/test/diagnostics/signature-b-preload.test.ts +++ b/packages/api/test/diagnostics/signature-b-preload.test.ts @@ -316,7 +316,7 @@ test('diagnostic preload: process.kill records target PID and signal', (t) => { assert.ok(killRecord?.callerStack?.includes('kill-call-site'), 'traces kill call site'); }); -test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Authorization headers, cookies, quoted secrets, and password hashes', (t) => { +test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Authorization headers, cookies, quoted secrets, password hashes, and JSON-quoted fields', (t) => { const { records, logDir } = runWithPreload(` const parts = [ 'Connect failed to postgresql://app_user:s3cr3tpass@db.internal:5432/chess', @@ -327,6 +327,7 @@ test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Autho 'passwordHash="$2b$12$e8uq4abcdefghijklmnopqrstuvwxyz1234567890"', "password_hash='$2b$12$snakecasehash1234567890abcdefghijklmn'", 'password hash: "spacedhash1234567890abcdefghijklmn"', + '{"authorization": "Bearer json_auth_token_xyz", "cookie": "session=json_cookie_val", "password": "json_password_secret", "password_hash": "$2b$12$json_hash_val"}', 'Authorization: Basic dXNlcjpwYXNzd29yZA==,', 'Cookie: session=xyz123; token=abc456; other=789', ]; @@ -351,6 +352,10 @@ test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Autho assert.ok(msg.includes('passwordHash=[REDACTED]'), 'redacts passwordHash'); assert.ok(msg.includes('password_hash=[REDACTED]'), 'redacts password_hash'); assert.ok(msg.includes('password hash=[REDACTED]'), 'redacts password hash'); + assert.ok(msg.includes('"authorization"=[REDACTED]'), 'redacts JSON-quoted authorization'); + assert.ok(msg.includes('"cookie"=[REDACTED]'), 'redacts JSON-quoted cookie'); + assert.ok(msg.includes('"password"=[REDACTED]'), 'redacts JSON-quoted password'); + assert.ok(msg.includes('"password_hash"=[REDACTED]'), 'redacts JSON-quoted password_hash'); assert.ok(msg.includes('Authorization=[REDACTED]'), 'redacts Basic authorization header'); assert.ok(msg.includes('Cookie=[REDACTED]'), 'redacts multi-cookie header'); assert.ok(!msg.includes('s3cr3tpass'), 'raw password 1 not present'); @@ -360,9 +365,17 @@ test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Autho assert.ok(!msg.includes('$2b$12$e8uq4abcdefghijklmnopqrstuvwxyz1234567890'), 'raw passwordHash absent from message'); assert.ok(!msg.includes('$2b$12$snakecasehash1234567890abcdefghijklmn'), 'raw password_hash absent from message'); assert.ok(!msg.includes('spacedhash1234567890abcdefghijklmn'), 'raw password hash absent from message'); + assert.ok(!msg.includes('json_auth_token_xyz'), 'raw JSON authorization absent from message'); + assert.ok(!msg.includes('json_cookie_val'), 'raw JSON cookie absent from message'); + assert.ok(!msg.includes('json_password_secret'), 'raw JSON password absent from message'); + assert.ok(!msg.includes('$2b$12$json_hash_val'), 'raw JSON password_hash absent from message'); assert.ok(!stack.includes('$2b$12$e8uq4abcdefghijklmnopqrstuvwxyz1234567890'), 'raw passwordHash absent from stack'); assert.ok(!stack.includes('$2b$12$snakecasehash1234567890abcdefghijklmn'), 'raw password_hash absent from stack'); assert.ok(!stack.includes('spacedhash1234567890abcdefghijklmn'), 'raw password hash absent from stack'); + assert.ok(!stack.includes('json_auth_token_xyz'), 'raw JSON authorization absent from stack'); + assert.ok(!stack.includes('json_cookie_val'), 'raw JSON cookie absent from stack'); + assert.ok(!stack.includes('json_password_secret'), 'raw JSON password absent from stack'); + assert.ok(!stack.includes('$2b$12$json_hash_val'), 'raw JSON password_hash absent from stack'); assert.ok(!msg.includes('dXNlcjpwYXNzd29yZA=='), 'raw basic auth credential not present'); assert.ok(!msg.includes('session=xyz123'), 'raw cookie session not present'); assert.ok(!msg.includes('token=abc456'), 'raw cookie token not present'); @@ -428,7 +441,7 @@ test('diagnostic preload: empty SIGB_LOG_DIR falls back to default directory wit if (fs.existsSync(targetLogDir) && targetLogDir !== logDir) { const files = fs.readdirSync(targetLogDir).filter((f) => f.endsWith('.jsonl')); for (const file of files) { - if (records.some((r) => file.includes(String(r.pid)))) { + if (records.some((r) => file.startsWith(`run-${r.pid}-`))) { fs.rmSync(path.join(targetLogDir, file), { force: true }); } } From b5f78e5fe32286b43f04fb615c53a6433f0e22de Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 10:46:45 +0300 Subject: [PATCH 18/20] fix(api): redact array and object-valued sensitive fields in diagnostics - Extend sensitive-key value matcher in signature-b-preload.cjs to redact array [...] and object {...} values - Add regression assertions for array-valued cookies/tokens and nested object auth tokens in message and stack --- packages/api/test/diagnostics/signature-b-preload.cjs | 2 +- .../api/test/diagnostics/signature-b-preload.test.ts | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/packages/api/test/diagnostics/signature-b-preload.cjs b/packages/api/test/diagnostics/signature-b-preload.cjs index 4df9af3f..41008689 100644 --- a/packages/api/test/diagnostics/signature-b-preload.cjs +++ b/packages/api/test/diagnostics/signature-b-preload.cjs @@ -67,7 +67,7 @@ const REDACT_PATTERNS = [ [/sk-[a-zA-Z0-9_-]{10,}/g, '[REDACTED_API_KEY]'], [/Bearer\s+[A-Za-z0-9._~+/-]+=*/g, 'Bearer [REDACTED_TOKEN]'], [/(?:postgres|postgresql):\/\/[^:]+:[^@]+@/g, 'postgres://[REDACTED_CREDS]@'], - [/(["']?)(password(?:[_\s-]?hash|Hash)?|secret|token|authorization|cookie)\1\s*[:=]\s*(?:"(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*'|[^,\r\n}]+)/gi, '$1$2$1=[REDACTED]'], + [/(["']?)(password(?:[_\s-]?hash|Hash)?|secret|token|authorization|cookie)\1\s*[:=]\s*(?:\[(?:[^\]\\]|\\.)*\]|\{(?:[^\}\\]|\\.)*\}|"(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*'|[^,\r\n}]+)/gi, '$1$2$1=[REDACTED]'], ]; /** diff --git a/packages/api/test/diagnostics/signature-b-preload.test.ts b/packages/api/test/diagnostics/signature-b-preload.test.ts index e0532a62..e3bec9ec 100644 --- a/packages/api/test/diagnostics/signature-b-preload.test.ts +++ b/packages/api/test/diagnostics/signature-b-preload.test.ts @@ -328,6 +328,9 @@ test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Autho "password_hash='$2b$12$snakecasehash1234567890abcdefghijklmn'", 'password hash: "spacedhash1234567890abcdefghijklmn"', '{"authorization": "Bearer json_auth_token_xyz", "cookie": "session=json_cookie_val", "password": "json_password_secret", "password_hash": "$2b$12$json_hash_val"}', + '{"cookie":["theme=dark","session=raw-secret-cookie-val"]}', + '{"token":["tok1","secret-array-token-val"]}', + '{"authorization":{"type":"Bearer","token":"nested-secret-auth-token"}}', 'Authorization: Basic dXNlcjpwYXNzd29yZA==,', 'Cookie: session=xyz123; token=abc456; other=789', ]; @@ -356,6 +359,7 @@ test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Autho assert.ok(msg.includes('"cookie"=[REDACTED]'), 'redacts JSON-quoted cookie'); assert.ok(msg.includes('"password"=[REDACTED]'), 'redacts JSON-quoted password'); assert.ok(msg.includes('"password_hash"=[REDACTED]'), 'redacts JSON-quoted password_hash'); + assert.ok(msg.includes('"token"=[REDACTED]'), 'redacts JSON array-valued token'); assert.ok(msg.includes('Authorization=[REDACTED]'), 'redacts Basic authorization header'); assert.ok(msg.includes('Cookie=[REDACTED]'), 'redacts multi-cookie header'); assert.ok(!msg.includes('s3cr3tpass'), 'raw password 1 not present'); @@ -369,6 +373,9 @@ test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Autho assert.ok(!msg.includes('json_cookie_val'), 'raw JSON cookie absent from message'); assert.ok(!msg.includes('json_password_secret'), 'raw JSON password absent from message'); assert.ok(!msg.includes('$2b$12$json_hash_val'), 'raw JSON password_hash absent from message'); + assert.ok(!msg.includes('raw-secret-cookie-val'), 'raw array cookie value absent from message'); + assert.ok(!msg.includes('secret-array-token-val'), 'raw array token value absent from message'); + assert.ok(!msg.includes('nested-secret-auth-token'), 'raw nested object auth token absent from message'); assert.ok(!stack.includes('$2b$12$e8uq4abcdefghijklmnopqrstuvwxyz1234567890'), 'raw passwordHash absent from stack'); assert.ok(!stack.includes('$2b$12$snakecasehash1234567890abcdefghijklmn'), 'raw password_hash absent from stack'); assert.ok(!stack.includes('spacedhash1234567890abcdefghijklmn'), 'raw password hash absent from stack'); @@ -376,6 +383,9 @@ test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Autho assert.ok(!stack.includes('json_cookie_val'), 'raw JSON cookie absent from stack'); assert.ok(!stack.includes('json_password_secret'), 'raw JSON password absent from stack'); assert.ok(!stack.includes('$2b$12$json_hash_val'), 'raw JSON password_hash absent from stack'); + assert.ok(!stack.includes('raw-secret-cookie-val'), 'raw array cookie value absent from stack'); + assert.ok(!stack.includes('secret-array-token-val'), 'raw array token value absent from stack'); + assert.ok(!stack.includes('nested-secret-auth-token'), 'raw nested object auth token absent from stack'); assert.ok(!msg.includes('dXNlcjpwYXNzd29yZA=='), 'raw basic auth credential not present'); assert.ok(!msg.includes('session=xyz123'), 'raw cookie session not present'); assert.ok(!msg.includes('token=abc456'), 'raw cookie token not present'); From 2d01983a5f05d12d568a3630ad3f9d1069bf4f49 Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 10:52:45 +0300 Subject: [PATCH 19/20] docs(adr-0140): clarify diagnostic write timing between API delegation and event listeners - Distinguish APIs that write before delegation (process.exit, process.abort, process.kill) from listeners that write during event delivery --- docs/adr/0140-harness-ephemeral-port-acquisition.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/adr/0140-harness-ephemeral-port-acquisition.md b/docs/adr/0140-harness-ephemeral-port-acquisition.md index c19d2e53..44a19d25 100644 --- a/docs/adr/0140-harness-ephemeral-port-acquisition.md +++ b/docs/adr/0140-harness-ephemeral-port-acquisition.md @@ -224,7 +224,7 @@ those mechanisms fire on real occurrences, not synthetic ones.** It hooks that captures both uncaught exceptions and fatal unhandled rejections without registering active listeners that alter Node's default crash handling), `warning`, `beforeExit`, and Node's own unconditional `exit` event, writing a structured, redacted, timestamped -line to disk before each fires — bypassing stdout/stderr entirely so the test +line to disk (before delegation for `process.exit`, `process.abort`, and `process.kill`, and during event delivery for `uncaughtExceptionMonitor`, `warning`, `beforeExit`, and `exit`) — bypassing stdout/stderr entirely so the test reporter's own output is never touched. A bounded 20-run instrumented pass over the full `packages/api` suite (chosen the same way as the original 20-run sample: enough for >90% detection odds at the historically observed ~1-in-5 From 806d1cc81d22263b59e3c7fb4ee2e7a1258f4b3a Mon Sep 17 00:00:00 2001 From: Hussein Mohamed Date: Mon, 31 Aug 2026 10:58:10 +0300 Subject: [PATCH 20/20] fix(api): redact arbitrary recursive nested structures in diagnostic logger - Implement balanced structure scanner tracking quotes, escapes, and bracket/brace depth - Add regression coverage for multi-level nested credentials in message and stack --- .../test/diagnostics/signature-b-preload.cjs | 98 ++++++++++++++++++- .../diagnostics/signature-b-preload.test.ts | 3 + 2 files changed, 99 insertions(+), 2 deletions(-) diff --git a/packages/api/test/diagnostics/signature-b-preload.cjs b/packages/api/test/diagnostics/signature-b-preload.cjs index 41008689..65909dd2 100644 --- a/packages/api/test/diagnostics/signature-b-preload.cjs +++ b/packages/api/test/diagnostics/signature-b-preload.cjs @@ -67,11 +67,104 @@ const REDACT_PATTERNS = [ [/sk-[a-zA-Z0-9_-]{10,}/g, '[REDACTED_API_KEY]'], [/Bearer\s+[A-Za-z0-9._~+/-]+=*/g, 'Bearer [REDACTED_TOKEN]'], [/(?:postgres|postgresql):\/\/[^:]+:[^@]+@/g, 'postgres://[REDACTED_CREDS]@'], - [/(["']?)(password(?:[_\s-]?hash|Hash)?|secret|token|authorization|cookie)\1\s*[:=]\s*(?:\[(?:[^\]\\]|\\.)*\]|\{(?:[^\}\\]|\\.)*\}|"(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*'|[^,\r\n}]+)/gi, '$1$2$1=[REDACTED]'], ]; +const SENSITIVE_KEY_RE = /(["']?)(password(?:[_\s-]?hash|Hash)?|secret|token|authorization|cookie)\1\s*[:=]\s*/gi; + +/** + * Scans and redacts sensitive key-value pairs, tracking quote escapes and arbitrary nested array/object depth. + * + * @param {string} str - Input string to redact. + * @returns {string} Sanitized string with sensitive values replaced by [REDACTED]. + */ +function redactSensitiveStructures(str) { + if (typeof str !== 'string') return str; + let out = ''; + let lastIndex = 0; + SENSITIVE_KEY_RE.lastIndex = 0; + + let match; + while ((match = SENSITIVE_KEY_RE.exec(str)) !== null) { + out += str.slice(lastIndex, match.index); + const keyPrefix = (match[1] || '') + match[2] + (match[1] || '') + '=[REDACTED]'; + out += keyPrefix; + + let i = SENSITIVE_KEY_RE.lastIndex; + if (i >= str.length) { + lastIndex = str.length; + break; + } + + const firstChar = str[i]; + if (firstChar === '"' || firstChar === "'") { + const quote = firstChar; + i++; + while (i < str.length) { + if (str[i] === '\\') { + i += 2; + } else if (str[i] === quote) { + i++; + break; + } else { + i++; + } + } + } else if (firstChar === '{' || firstChar === '[') { + const stack = [firstChar]; + i++; + let inQuote = null; + while (i < str.length && stack.length > 0) { + const char = str[i]; + if (inQuote) { + if (char === '\\') { + i += 2; + } else if (char === inQuote) { + inQuote = null; + i++; + } else { + i++; + } + } else { + if (char === '"' || char === "'") { + inQuote = char; + i++; + } else if (char === '{' || char === '[') { + stack.push(char); + i++; + } else if (char === '}' && stack[stack.length - 1] === '{') { + stack.pop(); + i++; + } else if (char === ']' && stack[stack.length - 1] === '[') { + stack.pop(); + i++; + } else { + i++; + } + } + } + } else { + while ( + i < str.length && + str[i] !== ',' && + str[i] !== '\r' && + str[i] !== '\n' && + str[i] !== '}' && + str[i] !== ']' + ) { + i++; + } + } + + lastIndex = i; + SENSITIVE_KEY_RE.lastIndex = i; + } + + out += str.slice(lastIndex); + return out; +} + /** - * Redacts known sensitive patterns (API keys, bearer tokens, credentials, passwords) + * Redacts known sensitive patterns (API keys, bearer tokens, credentials, passwords, nested structures) * from strings before writing to diagnostic logs. * * @param {unknown} value - The input value to redact. @@ -81,6 +174,7 @@ function redact(value) { if (typeof value !== 'string') return value; let out = value; for (const [pattern, replacement] of REDACT_PATTERNS) out = out.replace(pattern, replacement); + out = redactSensitiveStructures(out); return out; } diff --git a/packages/api/test/diagnostics/signature-b-preload.test.ts b/packages/api/test/diagnostics/signature-b-preload.test.ts index e3bec9ec..9801e8d7 100644 --- a/packages/api/test/diagnostics/signature-b-preload.test.ts +++ b/packages/api/test/diagnostics/signature-b-preload.test.ts @@ -331,6 +331,7 @@ test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Autho '{"cookie":["theme=dark","session=raw-secret-cookie-val"]}', '{"token":["tok1","secret-array-token-val"]}', '{"authorization":{"type":"Bearer","token":"nested-secret-auth-token"}}', + '{"authorization":{"meta":{"type":"Bearer"},"credential":"deeply-nested-raw-secret"}}', 'Authorization: Basic dXNlcjpwYXNzd29yZA==,', 'Cookie: session=xyz123; token=abc456; other=789', ]; @@ -376,6 +377,7 @@ test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Autho assert.ok(!msg.includes('raw-secret-cookie-val'), 'raw array cookie value absent from message'); assert.ok(!msg.includes('secret-array-token-val'), 'raw array token value absent from message'); assert.ok(!msg.includes('nested-secret-auth-token'), 'raw nested object auth token absent from message'); + assert.ok(!msg.includes('deeply-nested-raw-secret'), 'deeply nested credential absent from message'); assert.ok(!stack.includes('$2b$12$e8uq4abcdefghijklmnopqrstuvwxyz1234567890'), 'raw passwordHash absent from stack'); assert.ok(!stack.includes('$2b$12$snakecasehash1234567890abcdefghijklmn'), 'raw password_hash absent from stack'); assert.ok(!stack.includes('spacedhash1234567890abcdefghijklmn'), 'raw password hash absent from stack'); @@ -386,6 +388,7 @@ test('diagnostic preload: redacts postgres/postgresql credentials, tokens, Autho assert.ok(!stack.includes('raw-secret-cookie-val'), 'raw array cookie value absent from stack'); assert.ok(!stack.includes('secret-array-token-val'), 'raw array token value absent from stack'); assert.ok(!stack.includes('nested-secret-auth-token'), 'raw nested object auth token absent from stack'); + assert.ok(!stack.includes('deeply-nested-raw-secret'), 'deeply nested credential absent from stack'); assert.ok(!msg.includes('dXNlcjpwYXNzd29yZA=='), 'raw basic auth credential not present'); assert.ok(!msg.includes('session=xyz123'), 'raw cookie session not present'); assert.ok(!msg.includes('token=abc456'), 'raw cookie token not present');