From a64d06b4c2312d627fc1ca510fc50c938c6a40ea Mon Sep 17 00:00:00 2001 From: Mark Scherer Date: Sun, 16 Aug 2026 11:17:09 +0200 Subject: [PATCH] test: prove the engine pin, and fix the drift job the pin turned red PART 9 section 4b's withdrawal of the quote attribution (markup-carve/carve#1213) is already implemented here: the engine pin names a carve-js commit that postdates the withdrawal, `convert.ts` maps a `figure` whose target is a `block_quote` to a pandoc `Figure` around the `BlockQuote` with the caption, and `reverse.ts` reads that shape back. What was missing is the part that says so when it stops being true. THE PIN IS CHECKED AGAINST THE INSTALLED TREE, NOT THE MANIFEST. Three files name the engine - the manifest, the lockfile, and `node_modules/.package-lock.json`, which records what npm actually wrote to disk. Nothing read the third, so a checkout whose `node_modules` predates a pin bump ran every engine-facing test against a different engine and reported the same green. Measured on a long-lived checkout of this repo, which held: ``` package-lock.json git+...carve-js.git#3f5dd8cb node_modules/.package-lock.json registry.npmjs.org/...carve-0.1.3.tgz ``` 0.1.3 is the last released tag and predates the withdrawal, so that tree still carried the `block_quote` `attribution` field. The new check in `test/participants.test.mjs` compares the commit named by all three and fails with "run npm ci" when they disagree. Mutated by bumping the manifest and leaving the lockfile behind: exactly one test goes red, and it is this one. Nothing else in the suite can see that drift. THE ENGINE'S OWN SERIALIZED AST IS ASSERTED, not only the converted pandoc tree. Every other assertion about a captioned quote reads what `convert.ts` produced, which cannot tell a correct engine from a stale one the converter compensated for - the failure class markup-carve/carve#755 collects. The new check in `test/ast-json.test.mjs` reads `carveToCarveAst` directly: a `figure` whose target is a `block_quote`, carrying the caption, with no `attribution` anywhere, and an uncaptioned quote as the control that keeps the caption load-bearing. Mutated by installing a pre-withdrawal engine: 20 tests red, this one among them. THE DRIFT JOB WENT RED THE FIRST MORNING AFTER THE PIN LANDED, and said the opposite of the truth. It resolved the dependency spec with `npm view`, which cannot read a git spec at all - it answers "GitFetcher requires an Arborist constructor to pack a tarball" - so an empty resolution reached the branch that reports "resolves to nothing on the registry, an install here is broken" while `npm ci` was installing the pin perfectly well. It did not even get that far: under `set -e -o pipefail` the failing `npm view` killed the step before its own error message ran, so the job failed with an empty log. The spec's shape now decides which comparison is meaningful. A range is compared against the registry as before; a commit is compared against carve-js `main` directly, which is the same "how far behind the language is this repo" question with the registry taken out of the middle. The one failure the git path owns is a commit carve-js does not have, which is a dependency nobody can install and the real version of the error the range path was reporting falsely. Both `npm view` reads tolerate a non-zero exit so the explaining message is what the log shows. Every path was run locally against the extracted step scripts: the current git pin (warns, exit 0), a semver range (resolves, exit 0), a git spec naming a branch rather than a commit (errors), an unresolvable range (errors, with its message), a pinned commit present upstream (reports 8 commits behind), and a pinned commit that is not (errors). Two comments left over from the section 4a era are corrected. The one in `convert.ts` was not merely stale but false about the code directly beneath it: it told a reader that outside a group a captioned quote never reaches the generic figure arm, which is the arm it has taken since the reroute came out. The one in `reverse.ts` pointed at a "section 4a quote branch above" that no longer exists, and the table collapse it was comparing itself to is now the only collapse of its kind. No CHANGELOG entry: nothing a consumer installs behaves differently. The behavior these checks pin was described when it landed. --- .github/workflows/engine-drift.yml | 121 +++++++++++++++++++++++------ src/convert.ts | 6 +- src/reverse.ts | 11 ++- test/ast-json.test.mjs | 34 ++++++++ test/participants.test.mjs | 45 +++++++++++ 5 files changed, 189 insertions(+), 28 deletions(-) diff --git a/.github/workflows/engine-drift.yml b/.github/workflows/engine-drift.yml index e4a0bd4..b5dccf1 100644 --- a/.github/workflows/engine-drift.yml +++ b/.github/workflows/engine-drift.yml @@ -22,8 +22,27 @@ name: Engine drift # # It WARNS on both rather than failing. Upstream moving is not a defect in # this repo, and a job that goes red for something outside it teaches people -# to ignore the job. It DOES fail when the range resolves to nothing, which -# means the dependency is unreachable and an install here is broken. +# to ignore the job. It DOES fail when the pin is unresolvable, which means +# an install here is broken. +# +# A GIT PIN IS THE THIRD SHAPE, and it is the shape this repo is in whenever +# it needs a node no release carries yet. `npm view` cannot resolve one - it +# answers "GitFetcher requires an Arborist constructor to pack a tarball" - so +# the range half of this job read that as an EMPTY resolution and failed with +# "resolves to nothing on the registry, an install here is broken" while +# `npm ci` was installing the pin perfectly well. The job went red the first +# morning after the pin landed and said the opposite of the truth, which is +# worth more than the drift it was watching for. +# +# So the pin's shape decides which comparison is meaningful: +# +# - a RANGE is compared against the registry, as before; +# - a COMMIT is compared against carve-js `main` directly, which is the same +# "how far behind the language is this repo" question with the registry +# taken out of the middle. What DOES fail there is a commit carve-js does +# not have - a rebased-away or force-pushed sha is an install that cannot +# be reproduced, which is the real version of the error the range path was +# reporting falsely. on: schedule: @@ -41,43 +60,76 @@ jobs: with: node-version: 22 - - name: Compare the range against the registry + - name: Compare the pin against the registry id: registry run: | set -euo pipefail pkg='@markup-carve/carve' - range="$(node -p "const p=require('./package.json');const d=p.dependencies||{},v=p.devDependencies||{};d['${pkg}']||v['${pkg}']||''")" - if [ -z "${range}" ]; then + spec="$(node -p "const p=require('./package.json');const d=p.dependencies||{},v=p.devDependencies||{};d['${pkg}']||v['${pkg}']||''")" + if [ -z "${spec}" ]; then echo "::error::${pkg} is not a dependency of this repo - this workflow is watching nothing" exit 1 fi - # `npm view @ version` prints ONE bare version when the - # range matches a single release, and `@ ''` lines when it - # matches several. Taking the last WHITESPACE-SEPARATED FIELD reads - # both; `tail -1` alone concatenates the spec onto the version the - # moment a second matching release exists, yielding - # `@markup-carve/carve@0.1.20.1.2` rather than a version. - resolved="$(npm view "${pkg}@${range}" version 2>/dev/null | tail -1 | tr -d "'\"" | awk '{print $NF}')" - latest="$(npm view "${pkg}" version 2>/dev/null | tr -d "'\"" | awk '{print $NF}')" - if [ -z "${resolved}" ]; then - echo "::error::${pkg}@${range} resolves to nothing on the registry - an install here is broken" - exit 1 - fi - # Both reads are checked for the SHAPE of a version before anything is + + # WHICH SHAPE IS IT. A dependency npm fetches from git carries the + # commit after a `#`; anything else is a semver range. The two are + # asked different questions, and asking a git pin the range question + # is what made this job report a broken install against a healthy one. + case "${spec}" in + *://*|git+*|github:*) + commit="${spec##*#}" + case "${commit}" in + # A branch or tag after the `#` pins nothing reproducible, and + # no `#` at all leaves `commit` holding the whole url. + ????????????????????????????????????????) ;; + *) echo "::error::${pkg} is fetched from git as \"${spec}\", which names no 40-character commit - an install here is not reproducible"; exit 1 ;; + esac + echo "pinned to carve-js ${commit}" + echo "commit=${commit}" >> "${GITHUB_OUTPUT}" + ;; + *) + # `npm view @ version` prints ONE bare version when + # the range matches a single release, and `@ ''` lines + # when it matches several. Taking the last WHITESPACE-SEPARATED + # FIELD reads both; `tail -1` alone concatenates the spec onto the + # version the moment a second matching release exists, yielding + # `@markup-carve/carve@0.1.20.1.2` rather than a version. + # + # `|| true` because the message below is the point. Under + # `set -e -o pipefail` an `npm view` that exits non-zero - which + # is exactly what an unresolvable spec does - kills the step + # THERE, before the `::error::` explaining it ever runs, and the + # job fails with an empty log. That is how this step failed for + # real: `--log-failed` printed nothing at all. + resolved="$(npm view "${pkg}@${spec}" version 2>/dev/null | tail -1 | tr -d "'\"" | awk '{print $NF}' || true)" + if [ -z "${resolved}" ]; then + echo "::error::${pkg}@${spec} resolves to nothing on the registry - an install here is broken" + exit 1 + fi + echo "resolved=${resolved}" >> "${GITHUB_OUTPUT}" + ;; + esac + + latest="$(npm view "${pkg}" version 2>/dev/null | tr -d "'\"" | awk '{print $NF}' || true)" + # Every read is checked for the SHAPE of a version before anything is # compared. Without this the workflow's failure mode is silence: a # mis-read compares unequal to everything, so the drift warning fires # with a nonsense string in it and reads as a result (carve#755). - for pair in "resolved=${resolved}" "latest=${latest}"; do + for pair in ${resolved:+"resolved=${resolved}"} "latest=${latest}"; do case "${pair#*=}" in [0-9]*.[0-9]*.[0-9]*) ;; *) echo "::error::${pair%%=*} read back as \"${pair#*=}\" from the registry, which is not a version - every comparison below it would be meaningless"; exit 1 ;; esac done - echo "range ${range}" - echo "resolves to ${resolved}" + echo "spec ${spec}" echo "registry latest ${latest}" - if [ "${resolved}" != "${latest}" ]; then - echo "::warning::${pkg} ${latest} is published, but the range ${range} takes ${resolved} - a major-bumped release does not arrive on its own" + if [ -n "${resolved:-}" ]; then + echo "resolves to ${resolved}" + if [ "${resolved}" != "${latest}" ]; then + echo "::warning::${pkg} ${latest} is published, but the range ${spec} takes ${resolved} - a major-bumped release does not arrive on its own" + fi + else + echo "::warning::${pkg} is on a GIT PIN, so the published ${latest} is not what installs here - the pin returns to a range at the next engine release" fi echo "latest=${latest}" >> "${GITHUB_OUTPUT}" @@ -88,7 +140,30 @@ jobs: path: carve-js fetch-depth: 0 + - name: Compare the pinned commit against carve-js main + if: steps.registry.outputs.commit != '' + run: | + set -euo pipefail + commit='${{ steps.registry.outputs.commit }}' + # THE ONE FAILURE THIS PATH OWNS. A pinned commit carve-js does not + # have is a dependency nobody can install - a rebase or a force-push + # upstream, or a sha typed by hand. It is checked before it is + # measured, because `rev-list` on an unknown ref exits non-zero with + # a message about a bad revision, which reads as a workflow bug + # rather than as the finding it is. + if ! git -C carve-js cat-file -e "${commit}^{commit}" 2>/dev/null; then + echo "::error::carve-js has no commit ${commit} - the pin in package.json cannot be installed" + exit 1 + fi + behind="$(git -C carve-js rev-list --count "${commit}..origin/main")" + echo "pinned ${commit} ($(git -C carve-js log -1 --format=%s "${commit}"))" + echo "carve-js main is ${behind} commit(s) ahead of it" + if [ "${behind}" -gt 0 ]; then + echo "::warning::the PINNED engine is ${behind} commit(s) behind carve-js main - a git pin does not move on its own, so nothing here will notice the language moving until someone bumps it" + fi + - name: Compare the published engine against carve-js main + if: steps.registry.outputs.commit == '' run: | set -euo pipefail latest='${{ steps.registry.outputs.latest }}' diff --git a/src/convert.ts b/src/convert.ts index d28e064..0464512 100644 --- a/src/convert.ts +++ b/src/convert.ts @@ -1258,8 +1258,10 @@ function figure(ctx: Ctx, n: CNode): P.Block[] { const img = inline(ctx, target); return [P.Figure(toAttr(n.attrs), caption, [P.Plain(img)], shortCaption)]; } - // any other captionable target (outside a group a quote never reaches - // here - see above; as a PANEL it does, and lowers as a nested Figure) + // Any other captionable target, a QUOTE INCLUDED - this is the arm a + // captioned quote takes, at document level and as a §4c panel alike. It + // used to be unreachable for a quote outside a group, because §4a rerouted + // that case; nothing reroutes it now. return [P.Figure(toAttr(n.attrs), caption, untight(ctx, () => block(ctx, target)), shortCaption)]; } diff --git a/src/reverse.ts b/src/reverse.ts index 607a7c8..67618fc 100644 --- a/src/reverse.ts +++ b/src/reverse.ts @@ -1062,9 +1062,14 @@ function figure(ctx: Ctx, c: never): CNode[] { // The wrapper and the Table collapse into ONE Carve node, so their // attrs merge rather than the inner one silently winning: pandoc's // readers put the label on the Figure, not on the Table it wraps, and - // dropping it took the id a `` resolves against with it. Same - // collapse rule as the §4a quote branch above - the outer id wins, - // classes union, key/values merge with the outer taking precedence. + // dropping it took the id a `` resolves against with it. The + // outer id wins, classes union, key/values merge with the outer taking + // precedence. + // + // A table is the only host that collapses this way, because it is the + // only one Carve gives a caption of its own. Every other host keeps + // both nodes - the wrapper stays a `figure` and the host's own attrs + // ride on the target - which is what the Div arm below sorts out. // // It matters most for a §4c table PANEL, whose id is what resolves as // the group's number plus a letter. diff --git a/test/ast-json.test.mjs b/test/ast-json.test.mjs index 8f5ce1c..24076aa 100644 --- a/test/ast-json.test.mjs +++ b/test/ast-json.test.mjs @@ -111,6 +111,40 @@ test('the serialized document validates against the spec AST schema', () => { assertConforms(carveToCarveAst(SOURCE), 'serialized source'); }); +test('the pinned engine hands a captioned quote over as a figure, not an attribution', () => { + // WHERE THE ENGINE PIN IS ACTUALLY CHECKED. PART 9 section 4a briefly made a + // caption on a quote an `attribution` field on the quote itself; the clause + // is withdrawn (carve#1213) and `figure` is the generic captioned wrapper + // again, so `resources/ast-schema.json` has no `attribution` property and an + // unknown property is rejected on ingest (PART 12 section 11). + // + // Every other assertion about this shape in the suite reads the CONVERTED + // pandoc tree, which cannot tell a correct engine from a stale one that + // `convert.ts` compensated for - the failure mode markup-carve/carve#755 + // collects. This one reads what the engine itself serialized, before the + // converter sees it, so a pin that slipped back behind the withdrawal has + // nowhere downstream to hide. + const ast = carveToCarveAst('> To be, or not to be.\n^ Hamlet\n'); + assertConforms(ast, 'captioned quote'); + + const [figure] = ast.children; + assert.equal(figure.type, 'figure'); + assert.equal(figure.target.type, 'block_quote'); + // The caption is the half a `figure` has and an `attribution` had elsewhere, + // so it is asserted rather than assumed to have come along. Compared by type + // and text because a serialized node also carries its `pos`. + assert.deepEqual( + figure.caption.map((n) => [n.type, n.value]), + [['text', 'Hamlet']], + ); + assert.ok(!JSON.stringify(ast).includes('attribution'), 'no attribution field anywhere'); + + // The control: an UNCAPTIONED quote is a bare `block_quote`, so the `figure` + // above is the caption's doing and not the engine wrapping every quote. + const [bare] = carveToCarveAst('> To be, or not to be.\n').children; + assert.equal(bare.type, 'block_quote'); +}); + // --- The other direction: a tree that arrived already serialized --- const WIRE = { diff --git a/test/participants.test.mjs b/test/participants.test.mjs index 907f9a3..cc07100 100644 --- a/test/participants.test.mjs +++ b/test/participants.test.mjs @@ -87,6 +87,51 @@ test('no test source carries a literal control byte where an escape belongs', () ); }); +test('the engine under test is the engine the manifest pins', () => { + // A THIRD POPULATION THAT CAN GO UNCOMPARED: the dependency itself. + // + // `package.json` names a commit of carve-js, `package-lock.json` resolves it, + // and `node_modules/.package-lock.json` records what npm actually wrote to + // disk. Nothing in this suite reads the last one, so a checkout whose + // `node_modules` predates a pin bump runs every engine-facing test against a + // DIFFERENT engine and reports the same green. + // + // Measured, not hypothesized. A long-lived checkout of this repo held: + // + // package-lock.json git+...carve-js.git#3f5dd8cb + // node_modules/.package-lock.json registry.npmjs.org/...carve-0.1.3.tgz + // + // 0.1.3 is the last released tag and predates the PART 9 section 4b + // withdrawal (carve#1213), so that tree still carried `block_quote`'s + // `attribution` field. `npm test` there would have run the quote-figure + // assertions against an engine that cannot satisfy them - or, worse, passed + // them one day and not the next with no file in the diff to explain it. + // + // The fix is `npm ci`, and this is the line that says so. + const root = join(dirname(fileURLToPath(import.meta.url)), '..'); + const read = (p) => JSON.parse(readFileSync(join(root, p), 'utf8')); + const dep = '@markup-carve/carve'; + + const manifest = read('package.json').dependencies[dep]; + const locked = read('package-lock.json').packages[`node_modules/${dep}`].resolved; + const installed = read('node_modules/.package-lock.json').packages[`node_modules/${dep}`].resolved; + + // A git pin is `#`; the sha is the only part that identifies code. + // The url half legitimately differs between the three (npm rewrites the + // manifest's `git+https:` to `git+ssh:` when it resolves), so comparing whole + // strings would fail on a correct tree and teach everyone to ignore this. + const sha = (spec) => (typeof spec === 'string' ? (spec.split('#')[1] ?? null) : null); + + assert.ok(sha(manifest), `${dep} is not pinned to a commit in package.json: ${manifest}`); + assert.equal(sha(locked), sha(manifest), 'package-lock.json did not follow the manifest bump'); + assert.equal( + sha(installed), + sha(manifest), + `node_modules holds ${installed}, not the pinned commit. Run \`npm ci\`: every ` + + 'engine-facing test in this suite is currently measuring a different engine.', + ); +}); + test('a short count is a finding, and a sufficient one is not', () => { assert.equal(shortfall({ label: 'X', actual: 4, atLeast: 4 }), null); assert.match(