Run the Node floor the package promises, instead of naming it - #581
Merged
Conversation
`node: [22]` asks setup-node for the newest 22.x — 22.23 while this was written. The declared floor is 22.12.0, so no job had ever executed it. The installers refuse 22.11 and accept 22.12 in `test/install-script.test.ts`, but a version being admitted by a comparison is not the same as a suite passing on it, and the second is what the floor claims. It is pinned exactly now. The check run is therefore named `check (22.12.0)`, which the release gate and main's required contexts both had to learn. Both were updated with this commit rather than after it: `check (22)` would otherwise be a required context that can never appear again, which blocks every merge rather than gating one. Two things that had drifted into saying something untrue: - `scripts/check-engines.mjs` opened by stating the floor is Node 20. It has read `engines.node` for a while; now the comment says so instead of repeating a number that can go stale, which is the mistake it just made. - `validate --help` listed exit codes 0, 1 and 2. It returns 3 for a missing shipped file, which is the code that tells an unattended caller the message was never examined — the one worth documenting most. - `docs/RELEASE-GATE.md` listed six required checks. There are ten, they must come from the `github-actions` app, and `lint` is excluded for a reason worth writing down. It now points at `REQUIRED_CHECKS` as the source rather than restating it, since restating it is how it went stale. Verified: `node scripts/check-engines.mjs` exits 0 reporting the floor as 22.12.0; the yaml parses with `node: ['22.12.0', 24]`; 55/55 across both release-gate test files, including the two that compare the required list against `ci.yml`; main's protection reads back 11 contexts with the new name Evidence: .github/workflows/ci.yml Evidence: scripts/check-exact-head-ci.mjs Evidence: docs/RELEASE-GATE.md Follows: r-3f6d09 Blast: system Undo: easy Certainty: firm Record-Id: r-1d4c72 Provenance: authored CommitLore-Version: 2.0.0
CommitLore — record lintTrailers: clean — 12 commits in Active constraints for the paths this PR touchesLimits (286)
Ruled out (587)
Truncated: 704 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. |
The two cases added to stop the release gate drifting from CI did not do that.
They asked whether each ci.yml job had *some* entry in `REQUIRED_CHECKS`, by
prefix — `check.startsWith(`${job} (`)`. The parenthesised value was never
compared to anything. So one entry satisfied a job however many legs it had:
revert `check (22.12.0)` to `check (22)` -> both cases passed
replace it with `check (banana)` -> both cases passed
delete three required entries entirely -> both cases passed
Three of ten legs could drop out of the gate with the guard green. That is the
same defect one layer up from the one these cases exist to catch: a name
standing in for the thing it names, which is exactly what #571 was about.
Worse, I cited them as evidence. The `Verified:` trailer on r-1d4c72 says the
rename was checked by "the two that compare the required list against ci.yml".
Those two pass identically with the rename reverted. They verified nothing, and
`docs/RELEASE-GATE.md` repeats the claim.
The expansion is now computed — job name when there is no matrix, `job (value)`
per value when there is — and compared as a set. A multi-axis matrix would be
named `job (a, b)` by GitHub, so rather than guess at that composition the
helper refuses; there is none today, and a wrong guess would be this bug again.
Verified: each of the three mutations above now fails 3, 3 and 2 cases
respectively, and all three pass with them restored — the negative control I
should have run before claiming the first version worked
Evidence: test/release-publish-prerequisites.test.ts
Follows: r-1d4c72
Blast: module
Undo: easy
Certainty: firm
Record-Id: r-6e2a94
Provenance: authored
CommitLore-Version: 2.0.0
`admits` was fixed once for `>=`: reading only the major said `>=22.12.0`
admitted 22.5.0. The caret, tilde and bare-version branch of the same function
still did exactly that — it returned true whenever the majors matched:
^22.13.0 admits 22.12.0 -> true
~22.12.5 admits 22.12.0 -> true
22.13.0 admits 22.12.0 -> true
So a dependency declaring `^22.13.0` passed `check-engines.mjs` against a
22.12.0 floor, which is the check whose entire job is to refuse that. One of
two branches got the fix; the docstring above it described the defect while the
code below kept it.
Each operator denotes a window and both ends are compared now. `^` allows the
rest of the major, `~` the rest of the minor, and a bare version is bounded by
whatever it leaves unstated — `22` is all of 22, `22.13` is all of 22.13,
`22.13.0` is only itself.
The existing table could not have caught this. Every entry was a `>=` form, a
`*`, or `^18.0.0 || >=20.0.0` whose second clause returns first, so nothing
reached the branch. Fifteen cases now do, covering each operator on both sides
of its boundary.
Also corrected `docs/RELEASE-GATE.md`, which cited the drift tests as proof the
required list cannot drift. It said "two tests ... in both directions"; those
matched by prefix and accepted `check (banana)`. It now describes what they
compare and why prefix matching was wrong.
Verified: restoring the major-only comparison fails 6 of the new cases and none
of the old ones; 32/32 with it in place; `check-engines.mjs` still reports all
10 direct dependencies supporting the floor
Evidence: scripts/engine-floor.mjs
Evidence: test/engine-floor.test.ts
Follows: r-6e2a94
Blast: module
Undo: easy
Certainty: firm
Record-Id: r-0b7f31
Provenance: authored
CommitLore-Version: 2.0.0
The dogfooding gate passes on an empty range. `validate --range HEAD..HEAD --json` returns `shape: ok`, `reference: ok`, no violations — byte-identical to a clean run over the whole history, because the report carried no count of what it examined. Reproduced: the assertion exits 0 on it. This matters because the range is derived, not pinned. `scripts/adoption-range.mjs` finds the oldest commit carrying `CommitLore-Version:`, so a history rewrite, a squash, or a filter that moves that boundary to HEAD yields `<HEAD>..HEAD` — and the gate that proves this repository keeps its own protocol reports clean having read nothing. #542 was the same shape one step along: a check that did not run, reported as one that passed. Closing it by refusing `not-checked` left this door open. `validate --json` now reports `examined`, and the gate requires it to be a number greater than zero. A report without the field is refused rather than assumed fine — an older `validate` on a newer gate is precisely the case where guessing would restore the silence. Verified: the assertion exits 1 on `HEAD..HEAD` ("the range was empty") and on a report with no `examined` field, and exits 0 printing `examined: 3 message(s)` on `HEAD~3..HEAD`; removing the requirement fails the three new cases and none of the nine older ones Evidence: src/commands/validate.ts Evidence: scripts/assert-dogfood.mjs Follows: r-0b7f31 Blast: module Undo: easy Certainty: firm Record-Id: r-4f8d13 Provenance: authored CommitLore-Version: 2.0.0
node:sqlite was added in 22.5.0 but stayed behind --experimental-sqlite
until 22.13.0. The 22.12.0 floor let init die at the Index step:
require failed with "No such built-in module: node:sqlite". CI proved
it — pinning the floor job to 22.12.0 failed 134 tests with 85 of that
error, while Node 24 passed everything.
node 22.12.0: require('node:sqlite') -> "No such built-in module: node:sqlite"
node 22.13.0: require('node:sqlite') -> OK
node 22.13.0: zlib.zstdCompressSync -> not a function
node 22.15.0: zlib.zstdCompressSync -> OK
zstdCompressSync is used only in bench/cdeb, never in src/ or dist/, so
it does not raise the package floor. Those tests skip when zstd is
missing and name 22.15.0 as the version they need.
check-engines.mjs read declared dependency ranges and could not see a
bare node: builtin. That is why 22.12.0 shipped. It now compares every
node: specifier under src/ to the version that provides it unflagged,
and fails when the floor is below what an imported builtin needs.
Reverting engines.node to >=22.12.0 fails that case and the script;
restoring 22.13.0 passes both.
ADR-0010 said a later raise needs a superseding ADR and forbade a
package.json-only change. That instruction was ignored at 22.12.0.
ADR-0033 is the superseding document.
Closes #586.
Blast: system
Undo: easy
Certainty: firm
Record-Id: r-floor213
Provenance: authored
CommitLore-Version: 2.0.0
Verified: npx tsc --noEmit and bench tsc exit 0; npm run build updated dist/core/index-db.js; 201/201 across manifest, readme, install-script, engine-floor, compatibility-matrix and release-publish-prerequisites, including install.sh refusing 22.12.0 and accepting 22.13.0; check-engines names the sqlite gap on a reverted 22.12.0 floor and passes at 22.13.0; engine-floor's declared-floor case fails then passes under the same revert; 27/27 cdeb-evaluator and provider-ledger tests still pass on this Node (zstd present)
Evidence: docs/adr/ADR-0033-node-floor-22-13.md
Evidence: scripts/check-engines.mjs
Evidence: test/engine-floor.test.ts
Evidence: package.json
Adding `examined` to `validate --json` broke three tests that assert the whole envelope by deep equality. I ran the gate's own suite and not the ones that pin the shape I had just changed, so CI found it instead of me — on both Node legs, which is what says it was the change and not the runtime. The count is added to each expectation rather than loosened out of them. These three are where the envelope's shape is documented, and `examined` is part of that shape now: a report that does not say how much it read cannot be told from one that read nothing. Verified: 146/146 across test/validate.test.ts and test/hooks.test.ts Evidence: test/validate.test.ts Evidence: test/hooks.test.ts Follows: r-4f8d13 Blast: local Undo: easy Certainty: firm Record-Id: r-7c1e58 Provenance: authored CommitLore-Version: 2.0.0
validate already exits 3 when spec/schema/record.schema.json is
absent: "this installation is missing ... — the commit message was
not examined. Reinstall CommitLore to restore it: ...". doctor is
the command a user runs to learn whether the installation works, and
the release workflow's install-gate treats its exit 0 as evidence
that the tagged tree is intact. On that same tree doctor listed the
usual runtime and history rows, mentioned the missing file nowhere,
and exited 0.
A new non-optional installation-integrity check now reads the files
this installation actually loads through readInstalledFile —
package.json from packageVersion(), spec/schema/record.schema.json
from schema.ts, and spec/SPEC.md from harvest.ts. A missing file is
fail, not warn: every commit against this install is refused with a
message the user cannot act on by editing, and a warning would let
install-gate keep passing. The detail is readInstalledFile's own
error, so validate and doctor cannot drift into two repairs.
Doctor's exit code already becomes 1 when a non-optional check
fails, so no separate exit-path change was required. The check-set
snapshot is updated because a row was added.
Verified: a copied install with spec/ removed now fails
installation-integrity and exits 1; the same case with the check
reverted exits 0 ("expected +0 not to be +0"); restoring it
passes again. An intact checkout still reports the row as ok.
npx tsc --noEmit, npm run build, 163/163 across the doctor
test files.
Evidence: src/commands/doctor/checks/runtime-installation-integrity.ts
Evidence: src/commands/doctor/registry.ts
Evidence: test/doctor.test.ts
Blast: module
Undo: easy
Certainty: firm
Record-Id: r-instint1
Provenance: authored
CommitLore-Version: 2.0.0
The detector asked the neighbourhood of a Ruled-out quote for a refusal phrase — "does not work", "ruled out", "not viable". A measured rejection often never uses those words. It reports what happened when the alternative was tried. During a real capture the transcript said exponential backoff "re-synchronised the in-flight clients and pushed the 429 rate higher than it was without retries at all"; the verifier discarded the trailer twice with ruled-out-no-rejection, and the strongest constraint in the change survived only as a Warn. Closes #585. A second outcome layer now matches past-tense concluded harm — "did not work", "rolled back", "regressed", "made … worse", "higher/slower/worse than it was/without", "caused … to spike" — without putting "caused" or "higher than" into the marker table. Those tokens alone would accept "the rate is already higher than 10, we should try X". The quote must still be in the transcript, verbatim, and a mention, question, or proposal is still refused. Ruled-out: add `higher than` and `caused` to REJECTION_MARKERS | they match a proposal that names a threshold and an investigation that asks what caused the spike, which is the bar this ticket said not to move Verified: reverting the outcome layer fails all 10 past-tense accept cases with ruled-out-no-rejection and leaves the 5 refuse cases passing; restoring it is 16/16. 258/258 across every harvest and capture verification file. npx tsc --noEmit and npm run build. Evidence: src/core/harvest-verify.ts Evidence: test/ruled-out-past-tense.test.ts Evidence: test/fixtures/harvest-verify/ruled-out-past-tense.ts Evidence: test/capture-verify.test.ts Blast: module Undo: easy Certainty: firm Record-Id: r-past585 Provenance: authored CommitLore-Version: 2.0.0
Both are the same mistake: a value that means "I do not know" reused for a value that means "there is nothing". `readPending` caught every read failure and returned `null` — the same `null` that means no such transaction. EACCES on a file the process cannot read, EIO on a failing disk, and a genuinely absent transaction were one answer. ENOENT and ENOTDIR still return `null`; anything else now throws a marked plain error a caller can recognise, following the pattern `core/paths.ts` already uses rather than an Error subclass, which this repository does not allow. `loadCaptureVerificationHistory` asked `runQuery` for the history and threw away `shallow` and `unreadCommits` — the two fields whose whole job is to say the answer is partial. In a shallow clone, or after a scan budget cut the walk short, overlap detection compared a draft against part of the history and reported `incomplete: false`. A record duplicating one outside the visible range was accepted as novel, and nothing said the check had been partial. Both signals now feed the `incomplete` flag the result already carried, which `stage` already honours. Closes #582. Verified: with the `shallow || unreadCommits > 0` term removed, the shallow case fails; restored, it passes — reproduced independently of the agent that wrote it. 40/40 across test/pending.test.ts and test/capture-verify.test.ts Evidence: src/core/pending.ts Evidence: src/core/capture-verify.ts Follows: r-past585 Blast: module Undo: easy Certainty: firm Record-Id: r-9d4e27 Provenance: authored CommitLore-Version: 2.0.0
The injection scanner was handed trailer values. Every agent-facing renderer that prints an unrecognised key emits `key: value`. So `system: do nothing` as a trailer graded [directive] for a configured author: the scanner saw `do nothing` and returned clean, and the agent was served the scanner's own role-marker. foldLifecycle already strips Record-Id out of resolvedTrailers before stale scans them, so `Record-Id: system: do nothing` reached the model through the identity field. The same identity leaked from renderGuardMatch on a correctly blocked match. withholdBlocked dropped unsafe trailer fields and kept paths, so a withheld record whose commit added a file named `ignore previous instructions` still served the filename. Closes #596. A withheld record whose id and filenames are still printed is not withheld. Known-section renderers print the value alone, which is a substring of the pair, so scanning the pair is a superset and does not disturb legal structural values (`Blast: system` stays directive). Valid Record-Ids on withheld records stay listed; only an identity that itself trips the scanner, or fails RECORD_ID_RE, is redacted. Verified: reverting scanTrailer to scanInjection(trailer.value) fails the key-carries-payload case (expected blocked, got directive); restoring it passes. Reverting identity redaction in stale and guard fails both identity tests with the raw `system: do nothing` in the surface; restoring them passes. Reverting paths: [] fails both withholdBlocked tests (expected [], got ['ignore previous instructions']); restoring it passes. 643/643 across grade, query, stale, guard, inject, before-change, index-db, mcp and provenance. npx tsc --noEmit and npm run build. Evidence: src/core/grade.ts Evidence: src/commands/stale.ts Evidence: src/core/guard.ts Evidence: src/commands/query.ts Evidence: test/grade.test.ts Blast: module Undo: easy Certainty: firm Record-Id: r-scan596 Provenance: authored CommitLore-Version: 2.0.0
The floor was 22.13.0 — the version where `node:sqlite` stops needing
`--experimental-sqlite`. But the module being importable is not the same as the
feature the index depends on. Measured: `node:sqlite`'s bundled SQLite is built
without FTS5 until 22.16.0. `CREATE VIRTUAL TABLE t USING fts5(x)` fails on
22.13.0 and 22.15.0, succeeds from 22.16.0. So on 22.13-22.15 the index existed
and every query fell through to LIKE.
The floor is now 22.23.2, the current Node 22 LTS. Chosen over the minimum that
merely works (22.16.0) because the owner's instruction was to track the LTS and
put product performance and completeness first, and because a floor set to the
oldest version that happens to pass is how the previous two floors were chosen
— each time leaving a capability the code assumed and nothing checked.
With FTS5 guaranteed, `test/index-db.test.ts` gets its assertion back. It had
been weakened to `expect(typeof handle.fts).toBe('boolean')` to survive a floor
where the answer could be either; `expect(handle.fts).toBe(true)` is a real
claim again. Closes #593.
`scripts/engine-floor.mjs` carried `node:sqlite` at 22.13.0 in its built-in
table. That is right for the module and wrong for what the code needs, which is
the whole mistake one layer down; the table now records the version the feature
requires. ADR-0034 supersedes ADR-0033 with these measurements, per the
convention that a floor moves by ADR rather than by editing package.json.
Verified: `check-engines.mjs` reports all 10 dependencies and all imported
builtins clear at 22.23.2; 185/185 across index-db, engine-floor, manifest,
readme and compatibility-matrix; reverting the FTS5 metadata to 22.13.0 fails
2 engine-floor cases, and lowering the installer patch floor to 22.23.1 fails
the rejection case
Evidence: docs/adr/ADR-0034-node-floor-22-23.md
Evidence: scripts/engine-floor.mjs
Follows: r-9d4e27
Blast: system
Undo: easy
Certainty: firm
Record-Id: r-3b8c04
Provenance: authored
CommitLore-Version: 2.0.0
…est follow it Raising the floor to 22.23.2 turned four jobs red, and both causes were the same mistake wearing different clothes. `install-macos` and `install-script` ask setup-node for `22`, which resolves to whatever the runner's mirror has newest — 22.23.1 today. So the installer refused the runtime CI handed it, correctly: it enforces the declared floor and CI was executing something else. That is exactly the defect this whole line of work started from, `node: [22]` never running the 22.12.0 it promised. Every `node-version:` in every workflow is now the floor, so a job cannot silently run a version the package does not claim. Fourteen references across ci, release, demo-lint and demo-preserve; none floating. `test/install-ps1.test.ts` pinned the phrase `Node.js $NodeMajorMin or newer is required`. The floor stopped being expressible as a major alone when it grew a minor and a patch, so both installers interpolate `$NodeFloor` / `$NODE_FLOOR` now and the assertion was holding them to a name that no longer exists. The test asserts the current constant, and the comment says why it changed. Verified: 106/106 across install-ps1, install-script and engine-floor; every workflow still parses; `grep -rn "node-version: 22$" .github/workflows/` returns nothing Evidence: .github/workflows/ci.yml Evidence: .github/workflows/release.yml Evidence: test/install-ps1.test.ts Follows: r-3b8c04 Blast: system Undo: easy Certainty: firm Record-Id: r-5e0a91 Provenance: authored CommitLore-Version: 2.0.0
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.
The final review's remaining cosmetic findings, plus one that turned out not to be cosmetic.
The floor was never executed.
node: [22]resolves to the newest 22.x — 22.23 at the time of writing — so the declared floor of>=22.12.0had never been the version any CI job ran.test/install-script.test.tsasserts that the installers refuse 22.11 and accept 22.12, but a version passing a comparison is not the same as the suite passing on it, and the second is what a floor claims. It is pinned exactly now.That renames the check run to
check (22.12.0), so the release gate'sREQUIRED_CHECKSand main's required contexts were updated with this change rather than after it —check (22)would otherwise be a required context that can never appear again, blocking every merge.Three statements that had drifted out of true:
scripts/check-engines.mjsopened by saying the floor is Node 20. It readsengines.nodeand has for a while.validate --helplisted exit codes 0/1/2. It returns 3 for a missing shipped file — the code that tells an unattended caller the message was never examined.docs/RELEASE-GATE.mdlisted six required checks. There are ten, they must come from thegithub-actionsapp, andlintis excluded because its job only runs onpull_request. The doc now points atREQUIRED_CHECKSinstead of restating it, since restating it is how it went stale.Verified:
check-engines.mjsexits 0 reporting 22.12.0; the workflow yaml parses withnode: ['22.12.0', 24]; 55/55 across both release-gate test files including the two that compare the required list againstci.ymlin both directions; main's protection reads back 11 contexts under the new name.