Uh oh!
There was an error while loading. Please reload this page.
Delete the two committed tool transcripts that hold master red - #853
Conversation
`test/core/repo-scratch-hygiene.test.js` has been failing on `master` since it merged: `x/npm-test.log` and `x/typecheck.log` are tracked, and the test's first half asserts no `.log` is. Both came in on `adb448ab` (#785) via the `git add -A` sweep that #786 wrote this test to catch; the files predate the test, so it was red on arrival. Every branch cut since inherits it, currently blocking #833, #849, #850 and #851 for a reason none of them caused. The transcripts are deleted rather than the test relaxed, which is what its message asks for. `.gitignore` needs nothing: `*.log` is already committed and the rule test already passes, since an ignore rule cannot reach a path that is already tracked. That asymmetry is the whole reason the file carries two tests. Scope is exactly the two `.log` paths. The other eight files under `x/` are untouched: the hygiene test forbids tracked transcripts and nothing else, and sweeping up scratch it does not name would be a judgement this fix has no authority to make. Fixes#852 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Neutral review round: |
node --test test/core/repo-scratch-hygiene.test.js | |
|---|---|
origin/master | 3 tests, 2 pass, 1 fail (no tool transcript is tracked in the repo) |
7649c04d | 3 tests, 3 pass, 0 fail |
Nothing outside the test references either deleted path (git grep on the head tree hits only the ignore-rule probes at test/core/repo-scratch-hygiene.test.js:92-94, which are path strings, not file reads).
Non-blocking note (no change requested)
Issue #852's body says the test came from "PR #786". #786 is an issue ("Follow-up: deferred review findings from PR #785"); the test actually landed in PR #796 (bf9e4773). The PR body here writes it as bare #786, which reads fine as the issue that requested the test. The substantive claim - that the test was red from the moment it merged - is accurate in both. Not worth a push on a green unblocker; recording it here instead.
Verdict
Ship it. Minimal, correct, and it is what unblocks the other open PRs inheriting master's red hygiene test.
Corroborated review: two independent passes, zero findings, no contradictions between them. This record was posted after the first pass alone; the second returned afterwards and is folded in here rather than posted as a separate comment. codex is not available in this environment, so the second pass is a separate agent, not a second vendor. Every claim above is direct evidence re-derived in a clean detached worktree at origin/master and at 7649c04d.
What the second pass added, all consistent with the above: after the deletion git ls-tree -r <head> | grep '\.log$' is empty and the tracked-file count is 1185, far above the files.length > 500 floor asserted at the end of the same test file; the neighbouring repo lints (house-style-em-dash.test.js, llp-ref-hygiene.test.js) are 13/13 green on the branch; neither .github/workflows/ci.yml nor llp-check.yml references .log or x/; and the deleted content is regenerable transcript (x/typecheck.log is 5 lines ending TC EXIT=0, x/npm-test.log is raw TAP). It reached the same conclusion on the eight remaining x/ scratch files: outside what this guard forbids, worth a follow-up issue, not a change here.
Uh oh!
There was an error while loading. Please reload this page.
x/npm-test.log and x/typecheck.log are tracked leftovers from the git add -A sweep in #785 (pre-dating the repo-scratch-hygiene test added by #786). master already carries this fix via #853; this branch forked before that landed, so it inherited the red test separately. Same fix, scoped to the same two paths.
Fixes the CI failure on this branch: fix/issue-854 forked before master deleted the two committed tool transcripts (x/npm-test.log, x/typecheck.log) that trip the repo-scratch-hygiene test. Bringing master's fix in resolves the failing 'no tool transcript is tracked in the repo' test.
…sage a failed batch consumed (#851) * Deferred #843 findings: the listener's counters, its refusal log, and the usage a failed batch consumed Six of the ten findings deferred from PR #818, the ones that are a bug with a reproducing test rather than a design call. - A batch whose message write fails no longer loses the usage it claimed. The usage index outlives one POST because an `api_request` and the `assistant_response` that names its `request_id` routinely arrive in different flushes; projection claimed the entry, the write then failed, and the exporter's retry re-projected against a drained index and wrote the same rows with no `attributes.usage` and no `claude.cost_usd`. The catch puts back only what the batch consumed. - `client_attach_stale` no longer fires at an `otel`-attached client. The marker records the gateway port that mode never uses, so a gateway rebind printed a re-attach warning at a client the rebind did not touch; `client_telemetry_stale` already watches the port that decides whether anything is captured. - A refused `body_ref` is logged as a 12-hex digest, not as the raw wire-supplied path (LLP 0257 S23). - `deleteSpooledBodies` counts what it removed. `fs.rm(..., { force: true })` succeeds on a missing path, so `bodies_deleted` and `bodies_dropped` over-reported every already-evicted ref. - A policy drop brings `spool_bytes` down with the files it deleted, instead of leaving the gauge high until the next sweep restates it. - `last_event_at` is maxed by instant, not by string. Mixed `Z` / `.000Z` / offset shapes let the capture-gap baseline run backwards. The other six findings stay open on the issue: the picker's proxy-mode contradiction between two Accepted docs, the `dedupeStoredPartIds` scan cost, `hyp session status` reading only the gateway, the `lastEventAt` stamping order, the telemetry-endpoint prevention half, and the two shapes in finding 10 - each needs a decision this PR is not the place to make. Refs #843 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Delete two committed tool transcripts holding this branch red x/npm-test.log and x/typecheck.log are tracked leftovers from the git add -A sweep in #785 (pre-dating the repo-scratch-hygiene test added by #786). master already carries this fix via #853; this branch forked before that landed, so it inherited the red test separately. Same fix, scoped to the same two paths. * Review fixes: a malformed stamp cannot pin last_event_at, and spool_bytes counts what left the disk Two follow-ups from the review of this branch, both in the code it added. `newerEventTimestamp` fell back to the string compare whenever EITHER side failed to parse. `event.timestamp` is read off the wire unvalidated (`telemetry/events.js` takes whatever string the attribute carried), so a producer that stamps a non-date wins that compare outright - nothing an ISO stamp can begin with sorts above `u` in `unknown` - and because the fallback is symmetric it then beats every genuinely newer event that follows. The baseline is pinned for the life of the daemon, `hyp status` parses it to undefined, falls back to `listener_started_at`, and raises `capture_gap` against a listener that is capturing fine. A value that names an instant now beats one that names none, whichever side it arrives on; the string compare survives only for the pair where neither parses. The projected-body arm still subtracted `spooled.consumedBytes`, the bytes READ, while the PR added `bytesRemoved` precisely so the gauge tracks what left the disk. A body whose unlink fails (EPERM, a read-only spool) is still occupying the cap, and deducting it under-reported `spool_bytes` until the next sweep restated it: the drop arm's bug in the other direction. The new listener case fails on 40e40f9 and passes here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Review fixes: a failed batch's usage restore respects the index cap, and a ref stops citing S18 for something S18 does not say The catch-block restore reinstated every key missing from the pre-projection snapshot, which includes the ones USAGE_INDEX_LIMIT evicted during the batch, not only the ones projection claimed. During a sustained dataset outage nothing is ever claimed and nothing ever shrinks the index, so each failed batch added its `api_request` entries on top of a map that could no longer trim itself and the 512 cap stopped bounding anything. The restore is now `restoreUnclaimedUsage`, which puts the snapshot back and re-applies the cap, with a unit test that drives 40 failed batches through the real projector and asserts the index settles at the cap instead of growing past it (it reaches 576 by round 8 without the trim). The `@ref LLP 0257#failure-modes` glosses on the source and on the accounting test cited "S18 - a retried batch is re-projected from the same inputs, so its inputs have to survive". S18 says delivery is best effort and that a down daemon's behavioral-event loss is accepted; it says nothing about retry inputs surviving. 0257 is Accepted, so the mis-citation is removed rather than the spec edited, and the prose above each already carries the reasoning. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: neutral <neutral@hyperparam.app> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: test <test@test.com>
git rm x/npm-test.log x/typecheck.log. That is the whole change.Why master is red
test/core/repo-scratch-hygiene.test.jshas been failing onmastersince theday it merged:
Both files are tracked (
git ls-tree origin/master -- x/), and both arrived onmerge commit
adb448ab(#785) through the samegit add -Asweep that #786introduced this test to catch. The files predate the test, so it was red on
arrival rather than regressed into. Every branch cut since inherits the failure,
which is currently blocking CI on #833, #849, #850 and #851, none of which touch
x/or the hygiene test.Why nothing else changes
No
.gitignoreedit.*.logis already committed there, and the file'ssecond test (
.gitignore refuses a tool transcript) passes today, including onthe
x/npm-test.logprobe. The rule was never missing; a tracked path is simplyimmune to an ignore rule, which is exactly why the hygiene file carries two
tests instead of one. Adding anything here would be adding a rule that is
already doing its job.
No other file under
x/removed. The test forbids tracked.logfiles andnothing else, so
burst.mjs,invert.mjs,line_asker.fixed.js,line_asker.orig.js,msg.txt,order.mjs,pty_prog.mjsandpty_variants.mjsare outside what this fix is authorized to decide. Whetherthat scratch belongs in the tree is a real question; it is not this PR's, and
answering it here would put an unreviewed judgement inside an unblock.
No change to the test. The failing assertion is correct and its message
names the fix. Weakening it would delete the guard that caught this.
Verification
Both run in a clean detached worktree at
origin/masterwith a tempHYP_HOME:npm testnpm run typecheckThe single before-failure is the hygiene test; nothing else on
masteris red.The pass count rises by one because that same test now passes.
Fixes#852