Repository navigation
Conversation
Birfy
left a comment
There was a problem hiding this comment.
Not PASS — the tip is red.
Reviewed tip: 8b13b6aed51da69f80ef7dd6c79ba6213b4153ff
CI at this tip (run 39584366644):
tests-ok failure (10:26:30)
ubuntu-latest · Node 24 · DSH failure (10:25:27)
package, plugins (social|timeline|tree) success
The reproduction is real and good: at the test-only commit 67475fdeb7d77d24e2d9d5c6daef6d337dfeb594 the tests-ok, package and ubuntu-latest · Node 24 jobs all failed, so the two new #603 tests do go red on main. But the fix commits did not turn the tip green — the tip still fails tests-ok, and I cannot read the job log through the tools available to me (the logs endpoint needs repo admin), so I cannot name the failing test from here. Please read run 39584366644 and say which command/test failed and why.
Two documentation gaps in the body, both matter now that "the evidence lives in the PR" is the rule:
green: commit <fix sha>is still a placeholder — it should be8b13b6a(or whatever commit next turns it green), with the run link.- The body says the red run is "linked in the comment below", but this PR has zero comments (I checked). Put the run URL in the body.
Also: the PR carries no labels at all — no type, no area:*, no risk:T*. Per the label discipline it needs one type label, one area label, and exactly one risk:T* (by the controller, not by hand) before it can be queued.
On the code itself I found nothing to block on, so this is close:
trim()returns early whenlines.length < KEPTandBuffer.byteLength(text) <= MAX_BYTES; otherwise it keeps the newestKEPT - 1lines so the file settles at exactlyKEPTafter the append.- The
whileloop drops oldest-first until the byte cap holds, so a single oversized entry is dropped rather than kept over the cap — that is what the second test pins. writeFile(tmp)+renameis atomic on POSIX (and Node'srenamereplaces on Windows too), andrecord()serializes writes through its own promise chain, so two appends cannot interleave a trim.- A failed trim fails the append instead of silently appending over the cap — warn-only, as the file already does.
Non-blocking notes for later, not for this PR: trim() re-reads the whole file on every write (bounded at ~1 MB by the cap, so acceptable), and a crash between writeFile(tmp) and rename leaves a decisions.jsonl.trim behind with nothing that cleans it up.
|
Superseded by #606 ( |
Fixes #603
This PR is submitted in two commits on purpose: the test goes up first, CI goes red on it, then the fix lands and CI goes green. The red run is the reproduction evidence — this host has no usable sandbox backend, so nothing could be run locally.
Commit 1 — test only (
67475fd)core/test/decisions.test.jsgains the minimal reproduction, with no production change:#603: the log is trimmed on disk, so it cannot grow without bound— seedKEPT(2000) lines, record one decision, assert the file holds at mostKEPTlines, the newest entry survived and the oldest is gone.#603: one huge entry cannot blow the byte cap— seed a single 1.1 MB entry, record one decision, assert the file stays under 1 MB.Both fail on
maintoday:record()only appends, and the 2000-line cap lives inlist()alone.Commit 2 — the fix
core/src/host/decisions.js: trim on every write (newest 2000 lines, 1 MB byte cap, atomic rewrite), so the read side stays O(KEPT) because the file itself is bounded.patches/ds-bot/files/src/host/decisions.js: the byte-for-byte mirror. It is a whole-file mirror, so the missingnode scripts/ds-bot-patches.mjs generatestep is a straight copy — that is what the second commit does, and CI'sds-bot-patches.mjs checkverifies it.docs/subsystems/core-host.md: the cap as it now behaves.changelog.d/603.fixed.md.Evidence chain
67475fd→ CI run (linked in the comment below) fails on the two#603tests.<fix sha>→ CI run green.Not run
npm run verify,npm test,node scripts/ds-bot-patches.mjs check— all left to CI for the same sandbox reason. No claim of a local green.