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(