Comfy-Org/comfy-typescript-sdk

Fork 0

mirror of https://github.com/Comfy-Org/comfy-typescript-sdk.git synced 2026-11-04 23:47:27 +00:00

[PR #169] [MERGED] test(workflow-pins): pin the parser with a subprocess fixture matrix #170

New issue

Closed

opened 2026-10-31 22:05:16 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:05:16 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-typescript-sdk/pull/169
Author: @mattmillerai
Created: 9/21/2026
Status: ✅ Merged
Merged: 10/9/2026
Merged by: @mattmillerai

Base: matt/be-15270-resync-cursor-review-pins ← Head: matt/be-16299-check-workflow-pins-regression-suite

📝 Commits (5)

f852e7a test(workflow-pins): pin the parser with a subprocess fixture matrix
8e5570d test(workflow-pins): make the block-scalar decoys actually bite
471c54b test(workflow-pins): make the round-2 cases pin what their names claim
bb3b2a1 ci(workflow-pins): fail closed on a job-level uses: this scanner cannot read (#170)
7599eee Merge #153's tip into the regression-suite branch

📊 Changes

4 files changed (+1414 additions, -90 deletions)

View changed files

📝 .github/workflows/ci.yml (+373 -66)
➕ scripts/__fixtures__/ci-cursor-review.83c0c87.yml (+75 -0)
📝 scripts/check-workflow-pins.mjs (+187 -24)
➕ scripts/check-workflow-pins.test.ts (+779 -0)

📄 Description

STACKED — merging lands on matt/be-15270-resync-cursor-review-pins (owned by Matt Miller, PR #153), NOT main. scripts/check-workflow-pins.mjs exists only on #153's branch, so this PR is based on it rather than on main. GitHub retargets this PR to main once #153 merges; do not read anything below as "ready to merge to main".

ELI-5

There is a little script that checks the two SHAs in .github/workflows/ci-cursor-review.yml still point at the same upstream commit. It learned a lot of fiddly YAML rules over four review rounds — which uses: lines are real callers, where a workflows_ref: is allowed to sit, which ones are decoys inside a shell script. None of that was written down as a test, so the next person to touch the file had nothing to fail against. This PR writes those rules down as tests (34 cases today) that run the script for real and check what it prints and what it exits with.

What this adds

Originally two new files, listed below. Since then, stacked PR #170 (fail closed on a job-level uses: the scanner cannot read) has been squash-merged into this branch. So the diff against #153 now also changes scripts/check-workflow-pins.mjs and .github/workflows/ci.yml, and adds cases to scripts/check-workflow-pins.test.ts. #170's own PR description explains those changes.

scripts/check-workflow-pins.test.ts — 26 vitest cases originally, 34 now that #170 added its own that drive scripts/check-workflow-pins.mjs as a subprocess.
scripts/__fixtures__/ci-cursor-review.83c0c87.yml — 83c0c87's real ci-cursor-review.yml, copied verbatim (git show 83c0c87:.github/workflows/ci-cursor-review.yml, byte-identical). This is the historical split the lint was written for: uses:@ebfd9e44… with a # … (ffcc3f5) comment and workflows_ref: ffcc3f5b….

Subprocess rather than import, for two reasons: the script's contract is its exit status plus stdout/stderr, and it calls process.exit on every failure path, which an in-process import would turn into a killed test run. The script resolves its repo root from its own file location (new URL("..", import.meta.url)) with no --root flag, so each fixture root gets a copy of the script inside it; the real repo is exercised in place.

Fixture bodies are line arrays joined with \n, not template literals — YAML is indentation-significant, and an array keeps every leading space visible in review instead of tangled with the surrounding TypeScript indent.

Cases

Real repo: lints clean and says "all pins agree"; --print-pin cursor-review.yml writes a bare 40-hex SHA and nothing else.

Historical fixture: exits 1 with 2 problem(s) and both halves named — the stale comment and the stale workflows_ref.

The three pin assertions: split pins; stale parenthesised comment; stale bare comment; a date beside the parenthesised short SHA (passes); a branch-pinned uses: (fails).

Where a workflows_ref input may live: absent with a with: block and with none at all; a sibling of uses:; nested under another mapping; inside a body: | block scalar within with:; only in a preceding job's with:; with: written above uses:; with: # inputs with a flush-left # comment inside the block.

Shapes that must not read as a caller: a uses: decoy inside a run: | under a - name: step and inside the dash-carrying - run: | spelling — both written as heredoc body lines, so uses is the first token after the indent and the line is a real candidate for the caller regex; a - run: | block ending at its shell: sibling; a caller written after a multi-line block scalar; a whole-line # uses: comment above the real caller (the header of ci-cursor-review.yml does this); a quoted "uses": key and value, both in sync and split; a composite-action - uses: step, which pins once and has no input to split — asserted as a caller count beside a real job-level caller, so the case fails both if the step is read as a caller and if nothing parses at all.

--print-pin: with the block-scalar decoy present; with two callers (refuses to choose); with a caller of a different reusable only, asserting the mode-specific wording so a fall-through to lint mode cannot pass it; with a workflows_ref naming a branch rather than a commit, asserting nothing reaches stdout (printPin's FULL_SHA_RE guard, which ci.yml feeds to git cat-file -e "$pin^{commit}"); with no name; and the inline --print-pin=<name> form.

Why no `vitest.config.ts` change

vitest.config.ts sets no test.include, so vitest 4.1.10's default **/*.{test,spec}.?(c|m)[jt]s?(x) already discovers scripts/*.test.ts. Verified rather than assumed: pnpm test went from 26 to 27 test files with no config edit. The src/**/*.ts glob in that file is the coverage include, so a test file outside src/ does not disturb it. tsconfig.json has include: ["src"], so the new file is outside typecheck; oxlint . and oxfmt --check . do cover it.

Verified

Review round 2 (`471c54b`)

Round 2 raised seven findings; five held up and were fixed, one was answered, one was deferred.

Fixed: the composite-action case now counts callers beside a real job-level caller instead of asserting only the zero-caller notice; the --print-pin no-caller case holds a caller of a different reusable and asserts the mode-specific wording; a new case covers printPin's FULL_SHA_RE rejection of a branch-valued workflows_ref; run() reads signal before error and names the args in both, because spawnSync sets both on the timeout path so testing error first meant the signal was never named on exactly the path the cap exists for; and run() now passes a hermetic env (PATH only) so an ambient NODE_OPTIONS or a future Node's deprecation warning cannot red the cases asserting stderr is exactly "".

Each new assertion was checked by MUTATION rather than by reading, and each reds exactly one case: replacing printPin's c.reusable === wanted filter with callers.slice(); deleting its FULL_SHA_RE guard; and widening USES_RE group 1 to ^(\s*(?:-\s+)?) so a step-level - uses: reads as a caller.

One finding was answered, not fixed: a re-raise arguing the block-scalar decoys are rejected by an isJobLevelUses / owningKey / JOB_ID_KEY_RE positional check rather than by the skip, so the skip stays unpinned. Those symbols do not exist in this repo (grep finds none in either file), findCallers pushes every USES_RE hit straight onto callers, and the round-1 mutation still reproduces: deleting the block-scalar handling reds 3 cases. The fixtures and the comment are correct as written.

One finding was corrected on its premise: the report that the composite-action case reds on every run read printPin's zero-match fail() as if it were the lint's. Lint mode's zero-caller branch is console.log on stdout with exit 0, and the plural callers found is in the script. The case was green; it was strengthened for the separate, valid reason above.

Review round 1 (`8e5570d`)

Cursor review found the block-scalar decoys could not reach the behaviour they claimed to pin, and that was confirmed by MUTATING the script rather than by reading it. Against f852e7a's fixtures, deleting the BLOCK_SCALAR_RE handling from findCallers outright — both the blockKeyIndent skip and the opener branch — left all 25 cases green. The decoys were written as echo "uses: …" arguments, and the caller regex anchors uses as the first token after the indent, so they were rejected on spelling and the skip was never exercised.

Rewritten as heredoc body lines, the same mutation now turns 3 cases red, and the suite is green against the unmodified script. Two further mutations are documented as not pinned, honestly rather than silently: flipping BLOCK_SCALAR_RE group 1 to exclude the - sequence marker, and deleting the whole-line # guard, both still leave 25 green — the first because cursor-review: at column 2 ends the block under either column, the second because USES_RE and BLOCK_SCALAR_RE each already reject a leading #, making that guard redundant rather than untested. run() also now throws on spawnSync's error and on a non-null signal, and caps the child with spawnSync's own timeout — it blocks the worker's event loop, so vitest's per-test timeout can never fire on a wedged child.

Original

Every command below was run in this worktree. The suite was run on all three runtimes that matter: node 22.22.0 and 24.12.0 (the Test job's matrix) and node 26.4.0 (this machine's default) — 27 files / 645 tests passed on each.

pnpm test — 27 files, 645 tests passed on node 22.22.0, 24.12.0 and 26.4.0 (26 files / 620 before this change).
pnpm lint (oxlint .), pnpm format:check (oxfmt --check ., 86 files), pnpm typecheck (tsc --noEmit) — all clean, exit 0.
node scripts/check-workflow-pins.mjs and node scripts/check-public-repo-hygiene.mjs — both clean against the tree including the new fixture.
git diff confirms scripts/check-workflow-pins.mjs is byte-identical to 786762c; nothing under .github/ and nothing in vitest.config.ts is touched.

Revert spot-checks

The acceptance asked for two; all three were done, each at the tip so the result is not masked by a later commit. (Reverting a commit in isolation on its own parent is misleading here: at 41fd901 the missing-input allowlist was still opt-in, so six cases fail there for an unrelated reason and hide the delta.)

Parser hunk reverted at tip	Tests that fail
`41fd901` — `with:`-child positional gating	2: `workflows_ref` as a sibling of `uses:`; nested deeper than the `with:` children
`6b02db7` — walk back to the job key so the whole body is scanned	1: `with:` written above its job's `uses:`
`786762c` — fail-closed missing input, quoted `"uses":` key, comment candidates	6: date-in-comment; missing input; sibling; nested; preceding job's `with:`; quoted key

Judgment calls

Stacked, base matt/be-15270-resync-cursor-review-pins rather than main. The plan offered opening against main; that is not viable — the script under test does not exist on main, so the suite would fail there, and the diff would double-count #153's commits. Basing on #153's head kept this PR's original diff to exactly the two new files. #170's squash has since added more (see above).
The real-repo cases assert live repo state. run(REAL_REPO) re-reds the same build the workflow-pins CI job already reds if someone lands a split pin. That duplication is deliberate and was asked for: it is what keeps the suite honest about the script's behaviour on a real caller rather than only on synthetic ones.
No falsification evidence is cited, because nothing here denies a capability. The suite changes no product behaviour and adds no dead-end path; the cases that assert exit 1 pin behaviour 786762c already shipped, and each was confirmed by running the real script rather than by asserting a premise.

Residual

This PR's own CI does not run any of the repo's gates, because it is stacked. .github/workflows/ci.yml triggers on pull_request: branches: [main] only, so while the base is matt/be-15270-resync-cursor-review-pins the Test matrix, lint, typecheck, format:check, public-repo-hygiene and workflow-pins jobs are all skipped here; CodeRabbit likewise reports reviews are disabled for this base branch. Those gates start running when GitHub retargets this PR to main after #153 merges. Until then the evidence for them is the local runs listed above — which is why the node matrix was reproduced locally on 22 and 24 rather than left to CI. Re-check the checks after retarget, before merging.
A dash-less step uses: is misread as a caller — deferred, not fixed here. Reproduced against the real script: a composite-action step written - name: on one line and uses: Comfy-Org/github-workflows/actions/setup@<sha> on the next reaches findCallers as a plain mapping key and reds the lint with "passes no workflows_ref input". That contradicts the script header. The fixture cursor review asked for cannot be added here without either pinning the false positive or changing a lint that gates CI, and the fix needs a design call (require a .github/workflows/ path, or track steps: context). Tracked as a follow-up; see the resolved thread on scripts/check-workflow-pins.test.ts.
A folded or flow-mapping uses: was silently skipped — since fixed by #170. #170 (squash-merged into this branch) makes the scanner fail closed on a job-level uses: it cannot read, with cases for the folded uses: >-, flow-mapping job and continuation-line spellings. The original finding, for the record: Reproduced against the real script: a second job-level caller written uses: >- with the value on the next line is not parsed at all (BLOCK_SCALAR_RE matches the opener and swallows the value), so with one ordinary caller beside it the lint reports 1 caller(s) checked, all pins agree and exit 0 while a second caller on a different commit sits unread — the split-pin condition this lint exists to catch — and --print-pin returns the first pin instead of refusing to choose. A test asserting today's output would pin the bug, and the honest fix (fail closed on a job-level uses: the parser cannot read) changes a lint that gates CI, so it is tracked as a follow-up rather than done here. This is the false-negative twin of the dash-less-step false positive above; a yaml-parser rewrite would likely close both.
The script's documented flow-mapping limit is unpinned. with: { workflows_ref: … } written as a FLOW mapping is not parsed and reads as "input absent" — a deliberate loud-failure limit named in the script header. No case here covers it, so a future parser edit that silently started accepting it (or that changed how loudly it fails) would not be caught. Worth its own case.
NO_WORKFLOWS_REF is untested. The allowlist that exempts a reusable from requiring a workflows_ref is empty today, so no fixture exercises the exempt path. A reusable added to it later has no test proving it is actually skipped.
.yaml-extension workflow files are untested. findCallers accepts both .yml and .yaml; every fixture here is .yml.
The originating spike issue is not linked here. This repo is public, so per the public-repo guardrail no internal tracker reference appears in this body, the title, or any review comment — scripts/check-public-repo-hygiene.mjs would fail the build on one. (The fleet's automated follow-up notice comment does name the tickets it filed; that is tooling output about this PR, not part of the PR's content.) The origin that can be named is PR #153, whose four review rounds established every behaviour pinned above.

Provenance

Authored by: agent-work loop
Verified: at 7599eee:
- pnpm test: 27 files, 654 tests passed. pnpm lint, pnpm format:check and pnpm typecheck all exit 0 (local node only, not the 22/24 matrix).
- node scripts/check-workflow-pins.mjs: 1 caller, pins agree. node scripts/check-public-repo-hygiene.mjs: clean.
- ci.yml parses as YAML, and every run: block passes bash -n.
- Earlier rounds checked the suite by mutation: removing the block-scalar handling, printPin's reusable filter and its FULL_SHA_RE guard each turns cases red. That evidence is from rounds before #170 changed the script and was not re-run at this head.
Deviations:
- Stacked on #153, not opened against main, because the script under test exists only on #153's branch.
- #170's squash brought script and ci.yml changes into this PR. It is no longer test-only.
- The conflict merge with #153's tip (#173) adds $BUMP_NO_REASON to the stale-pin report's dedup key, so a bump PR whose failure reason changes is reported again.
- Two parser defects were deferred to follow-ups: a dash-less step uses: read as a caller (still open), and a folded or flow-mapping uses: silently skipped (since fixed by #170, which fails closed on it).
- One round-2 finding was answered without a change: it cited isJobLevelUses, which did not exist at 471c54b. #170 later added a function of that name for its own fail-closed check, so that answer is accurate only for the round-2 head.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-typescript-sdk/pull/169 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/21/2026 **Status:** ✅ Merged **Merged:** 10/9/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `matt/be-15270-resync-cursor-review-pins` ← **Head:** `matt/be-16299-check-workflow-pins-regression-suite` --- ### 📝 Commits (5) - [`f852e7a`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/f852e7ab13385349ab0c0b60b8452588b1f7a251) test(workflow-pins): pin the parser with a subprocess fixture matrix - [`8e5570d`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/8e5570d38c19a5d72eef0f716cbd3a35ef0c29f4) test(workflow-pins): make the block-scalar decoys actually bite - [`471c54b`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/471c54b63caf0bc710caf790de1a04152f34da4d) test(workflow-pins): make the round-2 cases pin what their names claim - [`bb3b2a1`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/bb3b2a19632c1465bfaacf9925dfc63ed682a3d9) ci(workflow-pins): fail closed on a job-level `uses:` this scanner cannot read (#170) - [`7599eee`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/7599eee61cc5c880c94a2d4cc794562ea04b91b4) Merge #153's tip into the regression-suite branch ### 📊 Changes **4 files changed** (+1414 additions, -90 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/ci.yml` (+373 -66) ➕ `scripts/__fixtures__/ci-cursor-review.83c0c87.yml` (+75 -0) 📝 `scripts/check-workflow-pins.mjs` (+187 -24) ➕ `scripts/check-workflow-pins.test.ts` (+779 -0) </details> ### 📄 Description **STACKED — merging lands on `matt/be-15270-resync-cursor-review-pins` (owned by Matt Miller, PR #153), NOT `main`.** `scripts/check-workflow-pins.mjs` exists only on #153's branch, so this PR is based on it rather than on `main`. GitHub retargets this PR to `main` once #153 merges; do not read anything below as "ready to merge to main". ## ELI-5 There is a little script that checks the two SHAs in `.github/workflows/ci-cursor-review.yml` still point at the same upstream commit. It learned a lot of fiddly YAML rules over four review rounds — which `uses:` lines are real callers, where a `workflows_ref:` is allowed to sit, which ones are decoys inside a shell script. None of that was written down as a test, so the next person to touch the file had nothing to fail against. This PR writes those rules down as tests (34 cases today) that run the script for real and check what it prints and what it exits with. ## What this adds Originally two new files, listed below. Since then, stacked PR #170 (fail closed on a job-level `uses:` the scanner cannot read) has been squash-merged into this branch. So the diff against #153 now also changes `scripts/check-workflow-pins.mjs` and `.github/workflows/ci.yml`, and adds cases to `scripts/check-workflow-pins.test.ts`. #170's own PR description explains those changes. - `scripts/check-workflow-pins.test.ts` — 26 vitest cases originally, 34 now that #170 added its own that drive `scripts/check-workflow-pins.mjs` as a **subprocess**. - `scripts/__fixtures__/ci-cursor-review.83c0c87.yml` — `83c0c87`'s real `ci-cursor-review.yml`, copied verbatim (`git show 83c0c87:.github/workflows/ci-cursor-review.yml`, byte-identical). This is the historical split the lint was written for: `uses:@ebfd9e44…` with a `# … (ffcc3f5)` comment and `workflows_ref: ffcc3f5b…`. Subprocess rather than `import`, for two reasons: the script's contract *is* its exit status plus stdout/stderr, and it calls `process.exit` on every failure path, which an in-process import would turn into a killed test run. The script resolves its repo root from its own file location (`new URL("..", import.meta.url)`) with no `--root` flag, so each fixture root gets a copy of the script inside it; the real repo is exercised in place. Fixture bodies are line **arrays** joined with `\n`, not template literals — YAML is indentation-significant, and an array keeps every leading space visible in review instead of tangled with the surrounding TypeScript indent. ## Cases Real repo: lints clean and says "all pins agree"; `--print-pin cursor-review.yml` writes a bare 40-hex SHA and nothing else. Historical fixture: exits 1 with `2 problem(s)` and **both** halves named — the stale comment and the stale `workflows_ref`. The three pin assertions: split pins; stale parenthesised comment; stale bare comment; a date beside the parenthesised short SHA (passes); a branch-pinned `uses:` (fails). Where a `workflows_ref` input may live: absent with a `with:` block and with none at all; a sibling of `uses:`; nested under another mapping; inside a `body: |` block scalar within `with:`; only in a **preceding** job's `with:`; `with:` written **above** `uses:`; `with: # inputs` with a flush-left `#` comment inside the block. Shapes that must not read as a caller: a `uses:` decoy inside a `run: |` under a `- name:` step and inside the dash-carrying `- run: |` spelling — both written as **heredoc body lines**, so `uses` is the first token after the indent and the line is a real candidate for the caller regex; a `- run: |` block ending at its `shell:` sibling; a caller written after a multi-line block scalar; a whole-line `# uses:` comment above the real caller (the header of `ci-cursor-review.yml` does this); a quoted `"uses":` key and value, both in sync and split; a composite-action `- uses:` step, which pins once and has no input to split — asserted as a caller **count** beside a real job-level caller, so the case fails both if the step is read as a caller and if nothing parses at all. `--print-pin`: with the block-scalar decoy present; with two callers (refuses to choose); with a caller of a **different** reusable only, asserting the mode-specific wording so a fall-through to lint mode cannot pass it; with a `workflows_ref` naming a branch rather than a commit, asserting nothing reaches stdout (`printPin`'s `FULL_SHA_RE` guard, which ci.yml feeds to `git cat-file -e "$pin^{commit}"`); with no name; and the inline `--print-pin=<name>` form. ## Why no `vitest.config.ts` change `vitest.config.ts` sets no `test.include`, so vitest 4.1.10's default `**/*.{test,spec}.?(c|m)[jt]s?(x)` already discovers `scripts/*.test.ts`. Verified rather than assumed: `pnpm test` went from 26 to 27 test files with no config edit. The `src/**/*.ts` glob in that file is the **coverage** include, so a test file outside `src/` does not disturb it. `tsconfig.json` has `include: ["src"]`, so the new file is outside typecheck; `oxlint .` and `oxfmt --check .` do cover it. ## Verified ### Review round 2 (`471c54b`) Round 2 raised seven findings; five held up and were fixed, one was answered, one was deferred. Fixed: the composite-action case now counts callers beside a real job-level caller instead of asserting only the zero-caller notice; the `--print-pin` no-caller case holds a caller of a different reusable and asserts the mode-specific wording; a new case covers `printPin`'s `FULL_SHA_RE` rejection of a branch-valued `workflows_ref`; `run()` reads `signal` **before** `error` and names the args in both, because `spawnSync` sets both on the timeout path so testing `error` first meant the signal was never named on exactly the path the cap exists for; and `run()` now passes a hermetic `env` (`PATH` only) so an ambient `NODE_OPTIONS` or a future Node's deprecation warning cannot red the cases asserting `stderr` is exactly `""`. Each new assertion was checked by MUTATION rather than by reading, and each reds exactly one case: replacing `printPin`'s `c.reusable === wanted` filter with `callers.slice()`; deleting its `FULL_SHA_RE` guard; and widening `USES_RE` group 1 to `^(\s*(?:-\s+)?)` so a step-level `- uses:` reads as a caller. One finding was **answered, not fixed**: a re-raise arguing the block-scalar decoys are rejected by an `isJobLevelUses` / `owningKey` / `JOB_ID_KEY_RE` positional check rather than by the skip, so the skip stays unpinned. Those symbols do not exist in this repo (`grep` finds none in either file), `findCallers` pushes every `USES_RE` hit straight onto `callers`, and the round-1 mutation still reproduces: deleting the block-scalar handling reds 3 cases. The fixtures and the comment are correct as written. One finding was **corrected on its premise**: the report that the composite-action case reds on every run read `printPin`'s zero-match `fail()` as if it were the lint's. Lint mode's zero-caller branch is `console.log` on stdout with exit 0, and the plural `callers found` is in the script. The case was green; it was strengthened for the separate, valid reason above. ### Review round 1 (`8e5570d`) Cursor review found the block-scalar decoys could not reach the behaviour they claimed to pin, and that was confirmed by MUTATING the script rather than by reading it. Against `f852e7a`'s fixtures, deleting the `BLOCK_SCALAR_RE` handling from `findCallers` outright — both the `blockKeyIndent` skip and the opener branch — left **all 25 cases green**. The decoys were written as `echo "uses: …"` arguments, and the caller regex anchors `uses` as the first token after the indent, so they were rejected on spelling and the skip was never exercised. Rewritten as heredoc body lines, the same mutation now turns **3 cases red**, and the suite is green against the unmodified script. Two further mutations are documented as *not* pinned, honestly rather than silently: flipping `BLOCK_SCALAR_RE` group 1 to exclude the `- ` sequence marker, and deleting the whole-line `#` guard, both still leave 25 green — the first because ` cursor-review:` at column 2 ends the block under either column, the second because `USES_RE` and `BLOCK_SCALAR_RE` each already reject a leading `#`, making that guard redundant rather than untested. `run()` also now throws on `spawnSync`'s `error` and on a non-null `signal`, and caps the child with spawnSync's own `timeout` — it blocks the worker's event loop, so vitest's per-test timeout can never fire on a wedged child. ### Original Every command below was run in this worktree. The suite was run on **all three** runtimes that matter: node 22.22.0 and 24.12.0 (the `Test` job's matrix) and node 26.4.0 (this machine's default) — 27 files / 645 tests passed on each. - `pnpm test` — 27 files, 645 tests passed on node 22.22.0, 24.12.0 and 26.4.0 (26 files / 620 before this change). - `pnpm lint` (`oxlint .`), `pnpm format:check` (`oxfmt --check .`, 86 files), `pnpm typecheck` (`tsc --noEmit`) — all clean, exit 0. - `node scripts/check-workflow-pins.mjs` and `node scripts/check-public-repo-hygiene.mjs` — both clean against the tree including the new fixture. - `git diff` confirms `scripts/check-workflow-pins.mjs` is byte-identical to `786762c`; nothing under `.github/` and nothing in `vitest.config.ts` is touched. ### Revert spot-checks The acceptance asked for two; all three were done, each **at the tip** so the result is not masked by a later commit. (Reverting a commit in isolation on its own parent is misleading here: at `41fd901` the missing-input allowlist was still opt-**in**, so six cases fail there for an unrelated reason and hide the delta.) | Parser hunk reverted at tip | Tests that fail | | --- | --- | | `41fd901` — `with:`-child positional gating | 2: `workflows_ref` as a sibling of `uses:`; nested deeper than the `with:` children | | `6b02db7` — walk back to the job key so the whole body is scanned | 1: `with:` written above its job's `uses:` | | `786762c` — fail-closed missing input, quoted `"uses":` key, comment candidates | 6: date-in-comment; missing input; sibling; nested; preceding job's `with:`; quoted key | ## Judgment calls - **Stacked, base `matt/be-15270-resync-cursor-review-pins` rather than `main`.** The plan offered opening against `main`; that is not viable — the script under test does not exist on `main`, so the suite would fail there, and the diff would double-count #153's commits. Basing on #153's head kept this PR's original diff to exactly the two new files. #170's squash has since added more (see above). - **The real-repo cases assert live repo state.** `run(REAL_REPO)` re-reds the same build the `workflow-pins` CI job already reds if someone lands a split pin. That duplication is deliberate and was asked for: it is what keeps the suite honest about the script's behaviour on a real caller rather than only on synthetic ones. - **No falsification evidence is cited, because nothing here denies a capability.** The suite changes no product behaviour and adds no dead-end path; the cases that assert exit 1 pin behaviour `786762c` already shipped, and each was confirmed by running the real script rather than by asserting a premise. ## Residual - **This PR's own CI does not run any of the repo's gates, because it is stacked.** `.github/workflows/ci.yml` triggers on `pull_request: branches: [main]` only, so while the base is `matt/be-15270-resync-cursor-review-pins` the `Test` matrix, `lint`, `typecheck`, `format:check`, `public-repo-hygiene` and `workflow-pins` jobs are all skipped here; CodeRabbit likewise reports `reviews are disabled for this base branch`. Those gates start running when GitHub retargets this PR to `main` after #153 merges. Until then the evidence for them is the local runs listed above — which is why the node matrix was reproduced locally on 22 and 24 rather than left to CI. **Re-check the checks after retarget, before merging.** - **A dash-less step `uses:` is misread as a caller — deferred, not fixed here.** Reproduced against the real script: a composite-action step written `- name:` on one line and `uses: Comfy-Org/github-workflows/actions/setup@<sha>` on the next reaches `findCallers` as a plain mapping key and reds the lint with "passes no `workflows_ref` input". That contradicts the script header. The fixture cursor review asked for cannot be added here without either pinning the false positive or changing a lint that gates CI, and the fix needs a design call (require a `.github/workflows/` path, or track `steps:` context). Tracked as a follow-up; see the resolved thread on `scripts/check-workflow-pins.test.ts`. - **A folded or flow-mapping `uses:` was silently skipped — since fixed by #170.** #170 (squash-merged into this branch) makes the scanner fail closed on a job-level `uses:` it cannot read, with cases for the folded `uses: >-`, flow-mapping job and continuation-line spellings. The original finding, for the record: Reproduced against the real script: a second job-level caller written `uses: >-` with the value on the next line is not parsed at all (`BLOCK_SCALAR_RE` matches the opener and swallows the value), so with one ordinary caller beside it the lint reports `1 caller(s) checked, all pins agree` and exit 0 while a second caller on a different commit sits unread — the split-pin condition this lint exists to catch — and `--print-pin` returns the first pin instead of refusing to choose. A test asserting today's output would pin the bug, and the honest fix (fail closed on a job-level `uses:` the parser cannot read) changes a lint that gates CI, so it is tracked as a follow-up rather than done here. This is the false-**negative** twin of the dash-less-step false positive above; a `yaml`-parser rewrite would likely close both. - **The script's documented flow-mapping limit is unpinned.** `with: { workflows_ref: … }` written as a FLOW mapping is not parsed and reads as "input absent" — a deliberate loud-failure limit named in the script header. No case here covers it, so a future parser edit that silently *started* accepting it (or that changed how loudly it fails) would not be caught. Worth its own case. - **`NO_WORKFLOWS_REF` is untested.** The allowlist that exempts a reusable from requiring a `workflows_ref` is empty today, so no fixture exercises the exempt path. A reusable added to it later has no test proving it is actually skipped. - **`.yaml`-extension workflow files are untested.** `findCallers` accepts both `.yml` and `.yaml`; every fixture here is `.yml`. - **The originating spike issue is not linked here.** This repo is public, so per the public-repo guardrail no internal tracker reference appears in this body, the title, or any review comment — `scripts/check-public-repo-hygiene.mjs` would fail the build on one. (The fleet's automated follow-up *notice* comment does name the tickets it filed; that is tooling output about this PR, not part of the PR's content.) The origin that *can* be named is PR #153, whose four review rounds established every behaviour pinned above. ## Provenance - **Authored by:** agent-work loop - **Verified:** at `7599eee`: - `pnpm test`: 27 files, 654 tests passed. `pnpm lint`, `pnpm format:check` and `pnpm typecheck` all exit 0 (local node only, not the 22/24 matrix). - `node scripts/check-workflow-pins.mjs`: 1 caller, pins agree. `node scripts/check-public-repo-hygiene.mjs`: clean. - `ci.yml` parses as YAML, and every `run:` block passes `bash -n`. - Earlier rounds checked the suite by mutation: removing the block-scalar handling, `printPin`'s reusable filter and its `FULL_SHA_RE` guard each turns cases red. That evidence is from rounds before #170 changed the script and was not re-run at this head. - **Deviations:** - Stacked on #153, not opened against `main`, because the script under test exists only on #153's branch. - #170's squash brought script and `ci.yml` changes into this PR. It is no longer test-only. - The conflict merge with #153's tip (#173) adds `$BUMP_NO_REASON` to the stale-pin report's dedup key, so a bump PR whose failure reason changes is reported again. - Two parser defects were deferred to follow-ups: a dash-less step `uses:` read as a caller (still open), and a folded or flow-mapping `uses:` silently skipped (since fixed by #170, which fails closed on it). - One round-2 finding was answered without a change: it cited `isJobLevelUses`, which did not exist at `471c54b`. #170 later added a function of that name for its own fail-closed check, so that answer is accurate only for the round-2 head. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:05:16 +00:00