fix: bound the guard to the prompt's window, and name the runtimes still writing (#884, #885) - #886
Merged
Conversation
`capture` died on a 74,173,844-byte session with `Fatal JavaScript invalid size error 134217728`, exit 133, and no JSON at all. Not a refusal — an engine abort. The command reports outcomes structurally so a caller branches on `staged` / `empty` / `rejected` rather than on the exit code, and this escaped that contract entirely: a wrapper careful enough to read the outcome had nothing to read. Reproduced on 1.2.3 too, so it was a standing limit and not something the last release introduced. 1.2.3 bounded the prompt (#873) and did not bound the guard beside it. `prepareValues` passed the whole transcript to `computeGuardAdvisory` on one line and windowed it to 256 KiB on the next. `guard` normalises its proposal through `normalizeForMatch`, and its `\p{Script=Latin}\p{M}*` global replace collects one match per letter into a single array; past roughly 69 MB that array crosses V8's 2^27 FixedArray ceiling. The frame that dies was measured, not inferred. A static read of this path nominates the `mapped += char` accumulator two lines below, which is the more obvious suspect and is not what fails. The native stack is unambiguous: Builtins_StringPrototypeReplace Builtins_RegExpReplace Runtime_RegExpExecMultiple FixedArrayBuilder::EnsureCapacity NewFixedArrayWithHoles(134217728) A preload wrapping `String.prototype.replace` confirms it from the other side: the last subject over 1 MB before the abort is the whole transcript under `/\p{Script=Latin}\p{M}*/gu`, reached through `tokenize` from `guard`. `computeGuardAdvisory` documents that it never throws and degrades any failure to a recorded gap. That contract could not hold while the input was unbounded, because a fatal engine abort is not catchable — so the input stopped being unbounded rather than the catch growing. The guard now reads the same window the prompt carries. That narrows what the advisory sees, and it is also the more honest alignment: the advisory is rendered beside the prompt, so computing it over a whole session could warn about a decision the prompt does not contain. The window is computed once and handed to both, so a large transcript is split once rather than twice. A truncated scan must not read as a complete one, so the advisory carries a new `proposal-windowed` gap whenever the window dropped part of the session. An empty `matches` array then says something about the window rather than about the transcript. Not fixed here, and deliberate: verification still reads the whole transcript, so a quote from outside the window still verifies, and `scan()` builds per-character structures over the whole file to do it. That survives 74 MB and would fail somewhere past ~134 M characters. Bounding it would break the guarantee that a locator names a line of the file it is checked against, which is the property #873 went out of its way to keep. Verified on a generated transcript matched to the report's profile — 74,450,108 bytes, 49,881 lines, 1,493 bytes/line against the reporter's 74,173,844 / 49,549 / ~1,497. Before: exit 133, zero bytes of stdout. After: exit 0, `outcome: empty` with a nonce in prompt-only mode and `outcome: staged` with a draft, 0.7 s end to end. The new tests assert by where a reviving phrase sits rather than by any byte count, because a size-only test would pass against the old code; the negative control restores `proposal: transcript` and fails the one case that proves the binding while the other three still pass. Closes #884 Record-Id: r-guardwindow884 Provenance: authored Ruled-out: chunk normalizeForMatch so the whole session is still scanned | it is the anti-injection normaliser -- NFKC, invisibles, confusables -- and a fold straddling a chunk boundary is a security regression traded for a performance fix Ruled-out: refuse above a transcript size and report it as a rejection | the reporter offered this as strictly better than a crash and it is, but it leaves large sessions with no advisory at all when a bounded one is available Ruled-out: bound guard() itself against any oversized proposal | the MCP route can hand it text with no ARG_MAX ceiling, so the class is real, but no caller has reported reaching it and the fix belongs where the unbounded input is produced Limit: verification is still unbounded by design and its per-character scan would fail past ~134 M characters; this moves the ceiling for capture, it does not remove it Limit: a decision raised early in a long session is no longer matched at all, because neither the guard nor the model can see it Warn: the advisory and the prompt must keep reading the same bytes; passing a different window to either reintroduces an advisory that describes text the model was never shown Blast: module Undo: easy Certainty: firm Verified: 14/14 in test/capture-prompt-budget.test.ts, full suite 3205 passed 4 skipped 0 failed, tsc exit 0 Verified: the reporter's repro shape run against the built CLI before and after -- exit 133 with 0 bytes, then exit 0 staging a record from the same 74 MB file Unverified: the exact byte threshold between the 69 MB that worked and the 74 MB that did not; it depends on letter density rather than size alone and was not narrowed Co-Authored-By: Claude <noreply@anthropic.com>
…a stale latest `doctor` reported three distinct live CommitLore runtimes answering MCP on a machine where every registration was correct. It was right. The old runtimes were live processes that outlived an upgrade: a host resolves the launcher once at session start and holds that runtime for the life of the session, and the sessions there ran for days. Those runtimes write. Records captured by a session started two days earlier come from the build that session started on, while the operator believes the repository is on the release they installed, and nothing on the commit says which produced it. The row named three versions and stopped, so an operator could not tell whether that was cosmetic or whether half their records came from old code. The scan already knew all five process ids and dropped them on the way to a deduplicated identity string, so the reporter ran `ps` themselves to find the processes behind the names. The row now groups pids by identity and names every one, states that each runtime keeps writing records with the build it started on, and offers an action aimed at the actual remedy: restart the host sessions that own those pids, because an upgrade cannot reach a process that is already running. Reinstalling is not the fix and the row no longer implies it is. It deliberately does not label any runtime the stale one. r-liveruntime660 ruled that out — a copied or stale install can report the same version as a current one, so a version comparison here proves nothing about identity — and a test asserts the row and its action say no such thing. Severity stays `warn` and `needsAttention` stays false: this observes the machine, not the checkout, and #750 is what happens when a repository command fails over another process. `upgrade` had a smaller version of the same blindness. On a machine running 1.2.5 it reported `latest v1.2.3` and added "this is the newest release". The lookup was never wrong: the answer is cached for a day in `~/.cache/commitlore/latest-release.json`, and only `upgrade` acting calls `forgetCachedRelease`, so a release installed any other way — install.sh, the plugin marketplace, a manual checkout — leaves the previous answer standing and the command reports a tag older than the binary printing it. A `latest` older than the version already running cannot be the latest. That case now drops the cache and re-asks. Equality is left alone deliberately: "you are up to date" is the answer the cache exists to hold, and re-asking there would make it decorative. Not addressed, and left for its own decision: #885 also asks that the producing runtime be recorded on the record itself, so a mixed-version repository is legible after the fact rather than only while the processes are alive. That cannot use `CommitLore-Version:` — SPEC §8 defines it as the protocol version a record targets, and every fixture carries 2.0.0, unrelated to the v1.2.x build. It needs a new trailer, which is a protocol surface that would appear on every future record and is a wider decision than this makes. Closes #885 Record-Id: r-runtimevis885 Provenance: authored Ruled-out: stamp the producing runtime on the record now | it needs a new trailer rather than CommitLore-Version, and a permanent addition to what a record looks like should not ride along with a diagnostic fix Ruled-out: name the newest runtime and call the others stale | r-liveruntime660 ruled that out because a copied or stale install can report the same version as a current one, so the ranking would be a guess presented as a fact Ruled-out: raise the row to fail, or have it claim attention | it observes the machine rather than the checkout, and #750 is the measured cost of letting another process decide a repository command's exit code Ruled-out: re-ask whenever the cached latest is not newer than the running version | that includes the equal case, which is every up-to-date machine, and would spawn git ls-remote on every upgrade Limit: only `upgrade` re-asks a provably stale cache; `latestReleaseSync` and its other callers still serve the day-long answer Limit: the row names the processes but cannot restart them, and doctor --fix does not act on this check Warn: the fix text is rendered with newlines collapsed, so it must stay one sentence Blast: module Undo: easy Certainty: firm Verified: 7/7 in test/runtime-identity-action.test.ts, 10/10 in test/update-command.test.ts, 4/4 in test/init-machine-scope.test.ts, full suite 3205 passed 4 skipped 0 failed, tsc exit 0 Verified: negative controls -- dropping the pids from the row fails the pid case, and removing the stale-cache re-ask fails the cache case, each while the rest still pass Unverified: whether a host that respawns the launcher picks up a new runtime, which is #885's third suggestion; the reported symptom is sessions that never respawn, so it was not measured Co-Authored-By: Claude <noreply@anthropic.com>
A long session killed the process outright, and an upgrade nobody could see had already happened: #884, #885. Thirty-six version pins across eleven files, bumped by parsing the JSON manifests rather than replacing text. The dependency that a text replacement would have corrupted moved since the last release — 1.2.4 had `rolldown` at `~1.2.4`, and 1.2.5 has `get-intrinsic` at `^1.2.5`, twice — which is the reason the rule is to parse rather than to remember which dependency to avoid. The script re-reads every file it touched and reports each remaining occurrence of the old version, so the two that are supposed to remain were seen rather than assumed. Record-Id: r-release126 Provenance: authored Ruled-out: text-replacing the version across the manifests | it also matches dependencies genuinely at that version, and which dependency that is changes from release to release Ruled-out: bumping every v1.2.5 in the READMEs | the install shapes are the only pins; README.md's release-boundary prose is a historical statement no test guards Limit: dist/ and installer/canonical-artifact.json are absent by design, so artifact:verify fails on this tree -- canonical-merge.yml rebuilds them on linux/amd64, where a macOS esbuild output would not match Blast: system Undo: easy Certainty: firm Verified: 109/109 across manifest, readme, release-version and check-release-version tests Verified: the only remaining 1.2.5 strings in the eleven touched files are package-lock.json's two get-intrinsic constraints Unverified: the install one-liners cannot be exercised until the tag exists, because install.sh clones a pinned tag Co-Authored-By: Claude <noreply@anthropic.com>
CommitLore — record lintTrailers: clean — 3 commits in Active constraints for the paths this PR touchesLimits (216)
Ruled out (360)
Truncated: 326 lines omitted — the comment hit GitHub's 65000 character limit. Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #884
Closes #885
Source only — no
dist/, noinstaller/canonical-artifact.json. Please runcanonical-merge.ymlagainst this number and merge the canonical PR with a merge commit.#884 —
capturedied with a fatal V8 error on a large transcriptExit 133, zero bytes of stdout, on a 74,173,844-byte session. Not a refusal — an engine abort, which escapes the structural-outcome contract entirely: a wrapper careful enough to branch on
staged/empty/rejectedinstead of$?had nothing to read.1.2.3 bounded the prompt (#873) and did not bound the guard beside it.
prepareValuespassed the whole transcript tocomputeGuardAdvisoryon one line and windowed it to 256 KiB on the next.guardnormalises its proposal throughnormalizeForMatch, whose\p{Script=Latin}\p{M}*global replace collects one match per letter into a single array; past ~69 MB that array crosses V8's 2^27FixedArrayceiling.The failing frame was measured, not inferred. A static read of this path nominates the
mapped += characcumulator two lines below — the more obvious suspect, and not what fails. The native stack:A preload wrapping
String.prototype.replaceconfirms it from the other side: the last subject over 1 MB before the abort is the whole transcript under/\p{Script=Latin}\p{M}*/gu, reached viatokenizefromguard.computeGuardAdvisorydocuments that it never throws and degrades failures to a recorded gap. That contract could not hold while the input was unbounded, because a fatal engine abort is not catchable — so the input stopped being unbounded rather than the catch growing.What changes
proposal-windowedgap whenever the window dropped part of the session. An emptymatchesarray then says something about the window rather than about the transcript.Not fixed, deliberately
Verification still reads the whole transcript on purpose, so a quote from outside the window still verifies, and its
scan()builds per-character structures over the whole file to do it. That survives 74 MB and would fail somewhere past ~134 M characters. Bounding it would break the guarantee that a locator names a line of the file it is checked against — the property #873 went out of its way to keep.#885 — an upgrade does not reach live sessions
doctorreported three distinct live runtimes answering MCP while every registration on the machine was correct. It was right: those were live processes that outlived the upgrade, because a host resolves the launcher once at session start and holds that runtime for the life of the session.Those runtimes write. Records captured by a session started two days earlier come from the build it started on, and nothing on the commit says which. The row named three versions and stopped, so an operator could not tell whether that was cosmetic. The scan already knew all five pids and discarded them on the way to a deduplicated identity string — the reporter had to run
psthemselves.What changes
r-liveruntime660ruled that out — a copied or stale install can report the same version as a current one — and a test asserts the row says no such thing.warnandneedsAttentionstaysfalse. This observes the machine, not the checkout; init.test.ts fails on any machine with a stale CommitLore MCP server running #750 is the measured cost of letting another process decide a repository command's exit code.Incidental, from the same report
upgradesaidlatest v1.2.3on a machine running 1.2.5, plus "this is the newest release". The lookup was never wrong — the answer is cached for a day and onlyupgradeacting clears it, so a release installed any other way leaves yesterday's answer standing and the command reports a tag older than the binary printing it. Alatestolder than the running version cannot be the latest; that case now re-asks. Equality still serves from the cache, or the cache would never serve the case it exists for.Not addressed — needs its own decision
#885 also asks that the producing runtime be recorded on the record. That cannot use
CommitLore-Version:— SPEC §8 defines it as the protocol version a record targets, and every fixture carries2.0.0, unrelated to thev1.2.xbuild. It needs a new trailer, which would appear on every future record and is a wider decision than a patch release should make.Evidence
Reproduced on a generated transcript matched to the report's profile — 74,450,108 bytes / 49,881 lines / 1,493 bytes per line, against the reporter's 74,173,844 / 49,549 / ~1,497.
capture --json --unattendedon 74 MBoutcome: empty+ nonce, 0.7 s--draftoutcome: stagedgaps: ["proposal-windowed"]upgradewith a stale cached latestThe
#884tests assert by where a reviving phrase sits, not by any byte count — a size-only test would pass against the old code. Negative controls: restoringproposal: transcriptfails the one case that proves the binding while the other three still pass; dropping the pids fails the pid case; removing the stale-cache re-ask fails the cache case.Full suite 3205 passed, 4 skipped, 0 failed;
tsc --noEmitexit 0; dogfood green against these three commits.Release
release: 1.2.6is the third commit — 36 pins across 11 files, bumped by parsing the JSON manifests. The dependency a text replacement would have corrupted moved since last release: 1.2.4 hadrolldownat~1.2.4, 1.2.5 hasget-intrinsicat^1.2.5, twice. That is why the rule is to parse rather than to remember which dependency to avoid.