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 #172] [MERGED] ci(cursor-review): lint the bump PR's caller before counting it delivered #172

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: matt/be-16336-check-workflow-pins-fail-closed ← Head: matt/be-16377-bump-pr-lint-before-delivered

📝 Commits (4)

10d6279 ci(cursor-review): lint the bump PR's caller before counting it delivered
cb4e5b7 ci(cursor-review): measure both delivery facts, and lint the whole workflow dir
f93d443 Merge the fail-closed base into the lint-before-delivered branch
7b85e1c ci(cursor-review): gate lint-fault on a read pin, and harden the bump-head reads

📊 Changes

1 file changed (+355 additions, -57 deletions)

View changed files

📝 .github/workflows/ci.yml (+355 -57)

📄 Description

STACKED — merging lands on matt/be-16336-check-workflow-pins-fail-closed (owned by @mattmillerai), NOT main. This branches off the top of the #153 ← #169 ← #170 stack, because the code it edits exists only there. Land the stack first; do not treat a green check list here as a merge signal (ci.yml is branches: [main]-scoped, so the real gates do not run on a stacked head — everything below was run locally instead).

ELI-5

The daily pin watchdog asks "is anyone moving this pin?" and goes quiet for a week if it sees an open bump PR that carries the commit we owe. It checked that by reading ONE of the bump PR's two pins. If the other pin was left behind, the PR could not actually work — but the watchdog counted it as delivered and shut up anyway. Now it also runs the repo's own pin lint against that PR's file, so it only shuts up for a bump PR that would be green.

What changed

All in .github/workflows/ci.yml, in the bump_delivers block of the cursor-review-pin-freshness job and the two places downstream that report its verdict.

The lint runs against the scratch root, before the --print-pin read and the cat-file -e gate (see round 10 below — it was originally sequenced after them). ROOT in scripts/check-workflow-pins.mjs comes from import.meta.url, so the copy already placed in $scratch/scripts scans $scratch/.github/workflows — which is materialised from refs/bump-head. It judges that PR's workflows, not this branch's.
The verdict now carries a reason. A new bump_no_reason output is lint when the pair is inconsistent, ancestry when the pin is consistent but does not carry the oldest owed commit, and lint-fault when the lint could not be run at all but the pin was read and does carry the owed commit (round 10, narrowed in round 11). A lint fault together with an unreadable pin falls through to the generic unknown. A new bump_lint_first output carries the lint's own first problem line (--stripped, with the problem count appended), and a new bump_carries_note carries the ancestry fact as a sentence. All are emitted alongside bump_pr / bump_age_days / bump_delivers.
Both downstream sites branch on it — the ::error:: in the state=stale branch of assess, and the cause= chain in "Open or update the stale-pin issue". The lint's line is quoted, not paraphrased: exit 1 covers a non-40-hex uses:, a stale trailing version comment, a missing workflows_ref, a workflows_ref that disagrees with uses:, and a caller spelling the scanner cannot read — only one of which is literally a split pair, so asserting "split pin" in prose would be wrong most of the time. $BUMP_LINT_FIRST is passed as a printf argument, never in a format string, and is %-escaped before reaching any ::error:: (round 10).
The block comment above bump_delivers=unknown now says the pin is read through --print-pin and the pair is checked through the lint, so yes means "a caller that would also be green on workflow-pins".

Judgment call: the exit-1 test is not keyed on the exit code alone

The obvious mapping is rc == 1 → no/lint, and it is wrong. Measured here on node 26: node /nonexistent.mjs exits 1, an uncaught throw exits 1, and only a bad CLI flag gives 9. Keyed on the code alone, this job's own crash would publish a stack-trace line as "the lint says your caller is broken" and open a sticky issue blaming the fleet for it — the fault-as-verdict conflation the rest of this step is built to avoid. All three of the script's exit-1 paths (fail(), reportUnreadable(), the lint-error report) open their stderr with check-workflow-pins: , and nothing node emits on its own does, so the classifier requires that marker too. Anything else lands on unknown with the existing "could NOT VERIFY" wording.

Two smaller deviations from the plan as written: the stderr capture now goes to $gwtmp/bump-lint.err (it was /tmp/bump-lint.err until the base branch introduced the per-run $gwtmp directory — see the merge note below); and the lint line is threaded as a new $GITHUB_OUTPUT output rather than re-derived, which the plan allowed either way.

Falsification of the new deny path

This diff adds a path that withholds a delivered verdict, so the affirmative capability was exercised first rather than assumed: fixture case real below runs a fully consistent caller through the unchanged --print-pin read, the new lint, and the ancestry test, and still reaches bump_delivers=yes. The denial fires only on a caller the repo's own lint actually rejects at run time — it is decided by running the tool, not by an authored premise.

Round 10 review (commit `cb4e5b7`)

Nine cursor-review findings, all one shape: the lint cause was wired as a short-circuit, so it suppressed facts the job had already paid for and then asserted things it had never measured. All nine are fixed and their threads resolved.

Lint the whole workflow directory, not one file. Every .github/workflows/*.y*ml is now materialised from refs/bump-head (git ls-tree + git show, non-recursive and .y*ml-filtered, matching the scanner's own readdirSync). The real workflow-pins job scans the whole directory, so a bump PR that broke or added a caller in any other file read as delivers=yes here and muted the watchdog for the full bump-PR window while being red on merge — which falsified item 4's "yes means it would also be green on workflow-pins" claim. Demonstrated in the sandbox: with a clean ci-cursor-review.yml and a broken caller in other.yml, the old code answered yes and the new code answers no/lint naming .github/workflows/other.yml:5.
Compute the ancestry verdict unconditionally. It was gated behind a clean lint, so (a) a lint verdict made the job claim the PR "is not counted as a delivery of this backlog" without ever measuring the backlog, and (b) a transient lint fault demoted a genuinely delivering PR to stale, reddening the schedule and refreshing the public issue daily. Both facts are now reported, so the reader learns whether a fixup commit or a whole fresh bump is owed. Precedence (lint > ancestry > lint-fault) is decided in one place with its rationale.
Hoist the lint above the pin gates. A missing workflows_ref, an unreadable caller spelling or a non-40-hex uses: takes --print-pin down too, so those permanent, actionable defects surfaced as the transient "could not verify". The deterministic verdict now wins.
lint-fault is its own cause. A lint that could not run was routed to text saying the job "could NOT VERIFY which commit its caller pin carries" — false on that path, where the pin was read, asserted 40-hex and confirmed upstream. It now points at this job, and its step 1 says to re-run rather than audit the PR.
The issue body's step 1 is conditional (see Residual, now struck).
% escaping. The runner percent-decodes %25/%0A/%0D in workflow-command message data, so a lint line quoting a ref or path containing one forged extra annotation lines. % is escaped first (so it cannot re-encode the escapes that follow), then CR.
$GITHUB_OUTPUT hardening. The two free-text fields now cross in the heredoc/delimiter form and are newline/CR-flattened at the boundary, rather than relying on the producer's current message shapes. In the plain name=value form a newline forges further outputs — including oldest_sha and behind_count, written earlier in the same block and never overwritten.
The problem count travels with the quoted line ((+N more problem(s))), so a caller with several defects is not fixed one round-trip per day.

Round 11 review (commit `7b85e1c`)

Five inline cursor-review findings, all fixed and their threads resolved:

lint-fault is now gated on carries = yes. Both message sites for that cause say "its pin reads fine", and that was false when the same fault also took --print-pin down. That double fault now reports as the generic unknown.
lint-fault gets its own ancestry sentence. It now states the bare fact ("Its pin does carry the oldest owed commit"). Before, it reused the lint-fail note, which bills a fixup commit for a defect nobody had established.
git ls-tree -z with read -r -d ''. Without -z, git C-quotes a name containing non-ASCII bytes, " or \, and that file silently dropped out of the scratch root.
Run-unique $GITHUB_OUTPUT heredoc delimiter instead of the fixed __BUMP_FIELD_EOF__.
CR is flattened as well as LF in both lint-fault stderr captures.

The panel's two unanchored findings (.github/dependabot.yml:97, and the $clone_err / $pr_err / $reason ::error:: sinks) are about code from #153 at the bottom of the stack (the dependabot.yml narrowing is commit a8c02ef on #153's branch), not this diff. There is a comment on this PR saying so; it names #170, which is wrong, and this sentence corrects it. The High finding's premise also does not hold against current dependabot-core: github_actions/lib/dependabot/github_actions/file_parser.rb (github_dependency) names a dependency whose path matches /.github/workflows/*.y(a)ml as owner/repo plus that path, so the Comfy-Org/github-workflows/.github/workflows/* glob does match these callers.

Merge of the base branch (commit `f93d443`)

The base (matt/be-16336-check-workflow-pins-fail-closed) had moved on and conflicted in three ci.yml hunks. Resolved keeping both intents:

Ancestry probe: this branch's carries/carries_why shape, adopting the base's whole-value [[ =~ ]] 40-hex match and its per-run "$gwtmp/gw" clone path.
Stale ::error:: lines: this branch's lint / lint-fault branches, all now using the base's future-dated-aware $landed wording.
Issue checklist step 1: both sides had fixed the same contradiction independently. This branch's step1 (chosen alongside $cause, covering the lint causes, and already naming $OLDEST_SHA on unknown) is kept; the base's later, narrower step1 block is dropped rather than left to overwrite it.

Also moved the lint stderr capture from /tmp/bump-lint.err to "$gwtmp/bump-lint.err", per the base's rule that every scratch path in that step lives in the per-run directory.

Verification

cursor-review-pin-freshness is if: github.event_name == 'schedule', so no PR CI can run it. The bump_delivers block was extracted verbatim into a /tmp fixture (real local git repo for the upstream clone, so cat-file -e and merge-base --is-ancestor run for real; only the branch fetch/show is stubbed to supply the caller), and each case's $GITHUB_OUTPUT was read back:

fixture caller	`bump_delivers`	`bump_no_reason`
consistent pair, carries the oldest owed commit	`yes`	(empty)
`workflows_ref` advanced, `uses:` left behind	`no`	`lint`
pins agree, only the trailing comment stale	`no`	`lint`
consistent pair on a side branch, not carrying the owed commit	`no`	`ancestry`
script file missing	`unknown`	(empty)
script present but throwing in lint mode	`unknown`	`lint-fault` (round 10; was (empty))

The two quoted lint lines came back intact and single-line through $GITHUB_OUTPUT (the workflows_ref vs uses: message and the stale-comment message). Both downstream chains were then exercised across all four reachable states — no+lint, no+ancestry, unknown, no-PR — and each selected its own branch; a % planted in the lint value survived the cause= printf, confirming it never reaches a format position.

Worth stating plainly about the missing-script row: it reaches unknown via the pre-existing --print-pin guard higher up, not via the new classifier, because a missing script takes --print-pin down first. The new fault path is exercised by the throwing-script row instead.

Also run: actionlint (0 issues; confirmed it invokes shellcheck on run: bodies by planting a deliberate SC2086 in a probe workflow), node scripts/check-workflow-pins.mjs (1 caller, pins agree), pnpm lint, pnpm format:check (86 files), pnpm typecheck, pnpm test (27 files, 652 passed, 0 failed; 654 after the base merge), node scripts/check-public-repo-hygiene.mjs, node scripts/check-spec-drift.mjs.

Scope swept, including the part not changed

Callers: 1 of 1 Comfy-Org/github-workflows caller under .github/workflows is now covered by both the --print-pin read and the pair lint. There is no second caller left uncovered.
Verdict consumers: 3 decision sites read bump_delivers. The 2 that branch on = no were threaded with bump_no_reason; the third is the delivered mute, which branches only on = yes and needs no reason.
Upstream premise, checked rather than trusted: Comfy-Org/github-workflows's .github/bump-callers/bump-callers.sh (public, read via the API at 1273 lines) does rewrite both pins in one pass and re-read them in a post-rewrite assertion that fails the repo on a mismatch. So this bites a hand-pushed ci/bump-cursor-review head or a producer regression, not the fleet's normal output — severity low, as scoped.

Residual

~~The stale-pin issue body's step 1 still reads wrong for every no verdict.~~ Fixed in round 10 — reviewers raised it independently, so it was pulled into scope. Step 1 is now chosen in the same branch as cause, so the checklist can no longer contradict the paragraph above it.
workflow-pins is not a required status check on main (only the two Test jobs, build-check and public-repo-hygiene are), so a hand-pushed split-pin bump PR is red but still mergeable. That is a repo-admin setting, not a repo-content change, and is unfixable from this PR.
No automated regression test accompanies this change. The job is schedule-gated and its logic lives in a run: block, so the /tmp fixture above is a one-shot harness and is not committed; nothing in pnpm test will catch a future regression in this block. Extracting the bump_delivers block into a testable script is the durable fix and is its own piece of work.
Unexercised artifacts. The originating investigation's tracker issue is in an internal tracker this PR cannot reach and was never read; only the machine-written plan derived from it was available. The run dashboard and worker transcript it cites are internal hosts and are deliberately not named here (public repo). The upstream reusable cursor-review.yml itself was not run — no scheduled or workflow_dispatch execution of cursor-review-pin-freshness has been performed from this branch, since adding that trigger is an operator change.

Provenance

Authored by: agent-work loop
Verified: (original) /tmp fixture of the extracted bump_delivers block: 6/6 cases matched the expected bump_delivers/bump_no_reason pairs; both downstream message chains: 4/4 branches selected correctly. (round 10, commit cb4e5b7) actionlint .github/workflows/ci.yml: 0 issues; shellcheck -S warning on the extracted assess and issue-step run: bodies: clean; sandbox harness driving the extracted block against real local git repos — clean+carries → yes, clean+behind → no/ancestry, split pin → no/lint, broken sibling caller → no/lint, node fault → unknown/lint-fault, three-problem tree → (+2 more problem(s)); injection probes against $GITHUB_OUTPUT and ::error:: with a hostile value carrying an embedded oldest_sha= line, a CR and a %0A sequence — no forged output keys, no forged annotation lines; issue body rendered under all six causes, each selecting its own step 1; npm test: 27 files, 652 passed, 0 failed; npm run lint, format:check (86 files), typecheck: clean; node scripts/check-workflow-pins.mjs: 1 caller, pins agree; check-public-repo-hygiene.mjs: clean. (base merge, commit f93d443) pnpm lint, format:check, typecheck: clean; pnpm test: 27 files, 654 passed; check-workflow-pins.mjs: 1 caller, pins agree; check-spec-drift, check-public-repo-hygiene.mjs: OK; bash -n on every run: body and shellcheck -S warning on the assess and issue-step bodies: clean. The /tmp fixture harness was not re-run after the merge. (round 11, commit 7b85e1c) The extracted precedence block was run over all 8 combinations of lint (pass/fail/fault) and carries (yes/no/unknown), and each produced the expected bump_delivers/bump_no_reason. Fault with unknown carries now gives unknown with no reason. The ls-tree -z loop was run in a scratch repo holding a"b.yml, é.yaml and README.md: both YAML files were kept unquoted and the README was skipped. bash -n on every run: body and shellcheck -S warning on the pin-freshness job's bodies were clean, as were check-workflow-pins.mjs (1 caller, pins agree), pnpm lint, pnpm format:check and check-public-repo-hygiene.mjs. actionlint is not installed on this host and was not run this round. pnpm test was not re-run: the change touches only ci.yml. (reviews pass, no new commit) Read dependabot-core's file_parser.rb on its default branch to check the round-11 High finding's naming premise (see above); no code changed.
Deviations: stderr capture was written to /tmp/bump-lint.err while $gwtmp did not exist, and moved to $gwtmp/bump-lint.err once the base introduced it (base merge); the exit-1 classifier additionally requires the script's check-workflow-pins: stderr marker, because node's exit code alone cannot separate a lint verdict from a crash (see above). (round 10) Scope was widened beyond the original plan to take all nine review findings, including the issue-body step 1 that the original PR had deliberately left as a Residual — reviewers raised it independently and it is two lines beside the cause chain it contradicts. The lint-fault text is truncated to 300 characters because a node stack trace is bound for a public issue body; the full text stays in the step log.

Note on CI: ci.yml is branches: [main]-scoped and cursor-review-pin-freshness is if: github.event_name == 'schedule', so neither this job nor the repo's gates run on a stacked head. Everything above was run locally; no CI run on this PR exercises the changed code.

_{🔄 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/172 **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-16336-check-workflow-pins-fail-closed` ← **Head:** `matt/be-16377-bump-pr-lint-before-delivered` --- ### 📝 Commits (4) - [`10d6279`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/10d62799e64e135edf467758cf6ffdd2d028d233) ci(cursor-review): lint the bump PR's caller before counting it delivered - [`cb4e5b7`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/cb4e5b73d8f4cfd7aaeb11beb38f4ddef3a23625) ci(cursor-review): measure both delivery facts, and lint the whole workflow dir - [`f93d443`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/f93d443530445decb4061afdbcafba624ef1467d) Merge the fail-closed base into the lint-before-delivered branch - [`7b85e1c`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/7b85e1cbea0f763e99d4ec845304cedc62009e20) ci(cursor-review): gate lint-fault on a read pin, and harden the bump-head reads ### 📊 Changes **1 file changed** (+355 additions, -57 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/ci.yml` (+355 -57) </details> ### 📄 Description **STACKED — merging lands on `matt/be-16336-check-workflow-pins-fail-closed` (owned by @mattmillerai), NOT `main`.** This branches off the top of the #153 ← #169 ← #170 stack, because the code it edits exists only there. Land the stack first; do not treat a green check list here as a merge signal (`ci.yml` is `branches: [main]`-scoped, so the real gates do not run on a stacked head — everything below was run locally instead). ## ELI-5 The daily pin watchdog asks "is anyone moving this pin?" and goes quiet for a week if it sees an open bump PR that carries the commit we owe. It checked that by reading ONE of the bump PR's two pins. If the other pin was left behind, the PR could not actually work — but the watchdog counted it as delivered and shut up anyway. Now it also runs the repo's own pin lint against that PR's file, so it only shuts up for a bump PR that would be green. ## What changed All in `.github/workflows/ci.yml`, in the `bump_delivers` block of the `cursor-review-pin-freshness` job and the two places downstream that report its verdict. 1. **The lint runs against the scratch root**, *before* the `--print-pin` read and the `cat-file -e` gate (see round 10 below — it was originally sequenced after them). `ROOT` in `scripts/check-workflow-pins.mjs` comes from `import.meta.url`, so the copy already placed in `$scratch/scripts` scans `$scratch/.github/workflows` — which is materialised from `refs/bump-head`. It judges that PR's workflows, not this branch's. 2. **The verdict now carries a reason.** A new `bump_no_reason` output is `lint` when the pair is inconsistent, `ancestry` when the pin is consistent but does not carry the oldest owed commit, and `lint-fault` when the lint could not be run at all but the pin was read and does carry the owed commit (round 10, narrowed in round 11). A lint fault together with an unreadable pin falls through to the generic `unknown`. A new `bump_lint_first` output carries the lint's own first problem line (` - `-stripped, with the problem count appended), and a new `bump_carries_note` carries the ancestry fact as a sentence. All are emitted alongside `bump_pr` / `bump_age_days` / `bump_delivers`. 3. **Both downstream sites branch on it** — the `::error::` in the `state=stale` branch of `assess`, and the `cause=` chain in "Open or update the stale-pin issue". The lint's line is **quoted, not paraphrased**: exit 1 covers a non-40-hex `uses:`, a stale trailing version comment, a missing `workflows_ref`, a `workflows_ref` that disagrees with `uses:`, and a caller spelling the scanner cannot read — only one of which is literally a split pair, so asserting "split pin" in prose would be wrong most of the time. `$BUMP_LINT_FIRST` is passed as a `printf` **argument**, never in a format string, and is `%`-escaped before reaching any `::error::` (round 10). 4. **The block comment** above `bump_delivers=unknown` now says the pin is read through `--print-pin` *and* the pair is checked through the lint, so `yes` means "a caller that would also be green on `workflow-pins`". ## Judgment call: the exit-1 test is not keyed on the exit code alone The obvious mapping is `rc == 1 → no/lint`, and it is wrong. Measured here on node 26: `node /nonexistent.mjs` exits **1**, an uncaught `throw` exits **1**, and only a bad CLI flag gives 9. Keyed on the code alone, this job's own crash would publish a stack-trace line as "the lint says your caller is broken" and open a sticky issue blaming the fleet for it — the fault-as-verdict conflation the rest of this step is built to avoid. All three of the script's exit-1 paths (`fail()`, `reportUnreadable()`, the lint-error report) open their stderr with `check-workflow-pins: `, and nothing node emits on its own does, so the classifier requires that marker too. Anything else lands on `unknown` with the existing "could NOT VERIFY" wording. Two smaller deviations from the plan as written: the stderr capture now goes to `$gwtmp/bump-lint.err` (it was `/tmp/bump-lint.err` until the base branch introduced the per-run `$gwtmp` directory — see the merge note below); and the lint line is threaded as a new `$GITHUB_OUTPUT` output rather than re-derived, which the plan allowed either way. ## Falsification of the new deny path This diff adds a path that **withholds** a `delivered` verdict, so the affirmative capability was exercised first rather than assumed: fixture case `real` below runs a fully consistent caller through the unchanged `--print-pin` read, the new lint, and the ancestry test, and still reaches `bump_delivers=yes`. The denial fires only on a caller the repo's own lint actually rejects at run time — it is decided by running the tool, not by an authored premise. ## Round 10 review (commit `cb4e5b7`) Nine cursor-review findings, all one shape: the `lint` cause was wired as a **short-circuit**, so it suppressed facts the job had already paid for and then asserted things it had never measured. All nine are fixed and their threads resolved. - **Lint the whole workflow directory, not one file.** Every `.github/workflows/*.y*ml` is now materialised from `refs/bump-head` (`git ls-tree` + `git show`, non-recursive and `.y*ml`-filtered, matching the scanner's own `readdirSync`). The real `workflow-pins` job scans the whole directory, so a bump PR that broke or added a caller in any *other* file read as `delivers=yes` here and muted the watchdog for the full bump-PR window while being red on merge — which falsified item 4's "`yes` means it would also be green on `workflow-pins`" claim. Demonstrated in the sandbox: with a clean `ci-cursor-review.yml` and a broken caller in `other.yml`, the old code answered `yes` and the new code answers `no`/`lint` naming `.github/workflows/other.yml:5`. - **Compute the ancestry verdict unconditionally.** It was gated behind a clean lint, so (a) a lint verdict made the job claim the PR "is not counted as a delivery of this backlog" without ever measuring the backlog, and (b) a transient lint *fault* demoted a genuinely delivering PR to `stale`, reddening the schedule and refreshing the public issue daily. Both facts are now reported, so the reader learns whether a fixup commit or a whole fresh bump is owed. Precedence (`lint` > `ancestry` > `lint-fault`) is decided in one place with its rationale. - **Hoist the lint above the pin gates.** A missing `workflows_ref`, an unreadable caller spelling or a non-40-hex `uses:` takes `--print-pin` down too, so those permanent, actionable defects surfaced as the transient "could not verify". The deterministic verdict now wins. - **`lint-fault` is its own cause.** A lint that could not *run* was routed to text saying the job "could NOT VERIFY which commit its caller pin carries" — false on that path, where the pin was read, asserted 40-hex and confirmed upstream. It now points at this job, and its step 1 says to re-run rather than audit the PR. - **The issue body's step 1 is conditional** (see Residual, now struck). - **`%` escaping.** The runner percent-decodes `%25`/`%0A`/`%0D` in workflow-command message *data*, so a lint line quoting a ref or path containing one forged extra annotation lines. `%` is escaped first (so it cannot re-encode the escapes that follow), then CR. - **`$GITHUB_OUTPUT` hardening.** The two free-text fields now cross in the heredoc/delimiter form *and* are newline/CR-flattened at the boundary, rather than relying on the producer's current message shapes. In the plain `name=value` form a newline forges further outputs — including `oldest_sha` and `behind_count`, written earlier in the same block and never overwritten. - **The problem count travels with the quoted line** (`(+N more problem(s))`), so a caller with several defects is not fixed one round-trip per day. ## Round 11 review (commit `7b85e1c`) Five inline cursor-review findings, all fixed and their threads resolved: - **`lint-fault` is now gated on `carries = yes`.** Both message sites for that cause say "its pin reads fine", and that was false when the same fault also took `--print-pin` down. That double fault now reports as the generic `unknown`. - **`lint-fault` gets its own ancestry sentence.** It now states the bare fact ("Its pin does carry the oldest owed commit"). Before, it reused the lint-fail note, which bills a fixup commit for a defect nobody had established. - **`git ls-tree -z` with `read -r -d ''`.** Without `-z`, git C-quotes a name containing non-ASCII bytes, `"` or `\`, and that file silently dropped out of the scratch root. - **Run-unique `$GITHUB_OUTPUT` heredoc delimiter** instead of the fixed `__BUMP_FIELD_EOF__`. - **CR is flattened as well as LF** in both lint-fault stderr captures. The panel's two unanchored findings (`.github/dependabot.yml:97`, and the `$clone_err` / `$pr_err` / `$reason` `::error::` sinks) are about code from #153 at the bottom of the stack (the `dependabot.yml` narrowing is commit `a8c02ef` on #153's branch), not this diff. There is a comment on this PR saying so; it names #170, which is wrong, and this sentence corrects it. The High finding's premise also does not hold against current `dependabot-core`: `github_actions/lib/dependabot/github_actions/file_parser.rb` (`github_dependency`) names a dependency whose path matches `/.github/workflows/*.y(a)ml` as `owner/repo` plus that path, so the `Comfy-Org/github-workflows/.github/workflows/*` glob does match these callers. ## Merge of the base branch (commit `f93d443`) The base (`matt/be-16336-check-workflow-pins-fail-closed`) had moved on and conflicted in three `ci.yml` hunks. Resolved keeping both intents: - **Ancestry probe:** this branch's `carries`/`carries_why` shape, adopting the base's whole-value `[[ =~ ]]` 40-hex match and its per-run `"$gwtmp/gw"` clone path. - **Stale `::error::` lines:** this branch's `lint` / `lint-fault` branches, all now using the base's future-dated-aware `$landed` wording. - **Issue checklist step 1:** both sides had fixed the same contradiction independently. This branch's `step1` (chosen alongside `$cause`, covering the lint causes, and already naming `$OLDEST_SHA` on `unknown`) is kept; the base's later, narrower `step1` block is dropped rather than left to overwrite it. Also moved the lint stderr capture from `/tmp/bump-lint.err` to `"$gwtmp/bump-lint.err"`, per the base's rule that every scratch path in that step lives in the per-run directory. ## Verification `cursor-review-pin-freshness` is `if: github.event_name == 'schedule'`, so no PR CI can run it. The `bump_delivers` block was extracted **verbatim** into a `/tmp` fixture (real local git repo for the upstream clone, so `cat-file -e` and `merge-base --is-ancestor` run for real; only the branch fetch/show is stubbed to supply the caller), and each case's `$GITHUB_OUTPUT` was read back: | fixture caller | `bump_delivers` | `bump_no_reason` | | --- | --- | --- | | consistent pair, carries the oldest owed commit | `yes` | *(empty)* | | `workflows_ref` advanced, `uses:` left behind | `no` | `lint` | | pins agree, only the trailing comment stale | `no` | `lint` | | consistent pair on a side branch, not carrying the owed commit | `no` | `ancestry` | | script file missing | `unknown` | *(empty)* | | script present but throwing in lint mode | `unknown` | `lint-fault` (round 10; was *(empty)*) | The two quoted lint lines came back intact and single-line through `$GITHUB_OUTPUT` (the `workflows_ref` vs `uses:` message and the stale-comment message). Both downstream chains were then exercised across all four reachable states — `no`+`lint`, `no`+`ancestry`, `unknown`, no-PR — and each selected its own branch; a `%` planted in the lint value survived the `cause=` `printf`, confirming it never reaches a format position. Worth stating plainly about the missing-script row: it reaches `unknown` via the **pre-existing** `--print-pin` guard higher up, not via the new classifier, because a missing script takes `--print-pin` down first. The new fault path is exercised by the throwing-script row instead. Also run: `actionlint` (0 issues; confirmed it invokes shellcheck on `run:` bodies by planting a deliberate SC2086 in a probe workflow), `node scripts/check-workflow-pins.mjs` (1 caller, pins agree), `pnpm lint`, `pnpm format:check` (86 files), `pnpm typecheck`, `pnpm test` (27 files, 652 passed, 0 failed; 654 after the base merge), `node scripts/check-public-repo-hygiene.mjs`, `node scripts/check-spec-drift.mjs`. ## Scope swept, including the part not changed - **Callers:** 1 of 1 `Comfy-Org/github-workflows` caller under `.github/workflows` is now covered by both the `--print-pin` read and the pair lint. There is no second caller left uncovered. - **Verdict consumers:** 3 decision sites read `bump_delivers`. The 2 that branch on `= no` were threaded with `bump_no_reason`; the third is the `delivered` mute, which branches only on `= yes` and needs no reason. - **Upstream premise, checked rather than trusted:** `Comfy-Org/github-workflows`'s `.github/bump-callers/bump-callers.sh` (public, read via the API at 1273 lines) does rewrite both pins in one pass and re-read them in a post-rewrite assertion that fails the repo on a mismatch. So this bites a hand-pushed `ci/bump-cursor-review` head or a producer regression, not the fleet's normal output — severity low, as scoped. ## Residual - ~~**The stale-pin issue body's step 1 still reads wrong for every `no` verdict.**~~ **Fixed in round 10** — reviewers raised it independently, so it was pulled into scope. Step 1 is now chosen in the same branch as `cause`, so the checklist can no longer contradict the paragraph above it. - **`workflow-pins` is not a required status check on `main`** (only the two Test jobs, `build-check` and `public-repo-hygiene` are), so a hand-pushed split-pin bump PR is red but still mergeable. That is a repo-admin setting, not a repo-content change, and is unfixable from this PR. - **No automated regression test accompanies this change.** The job is schedule-gated and its logic lives in a `run:` block, so the `/tmp` fixture above is a one-shot harness and is not committed; nothing in `pnpm test` will catch a future regression in this block. Extracting the `bump_delivers` block into a testable script is the durable fix and is its own piece of work. - **Unexercised artifacts.** The originating investigation's tracker issue is in an internal tracker this PR cannot reach and was never read; only the machine-written plan derived from it was available. The run dashboard and worker transcript it cites are internal hosts and are deliberately not named here (public repo). The upstream reusable `cursor-review.yml` itself was not run — no scheduled or `workflow_dispatch` execution of `cursor-review-pin-freshness` has been performed from this branch, since adding that trigger is an operator change. ## Provenance - **Authored by:** agent-work loop - **Verified:** *(original)* `/tmp` fixture of the extracted `bump_delivers` block: 6/6 cases matched the expected `bump_delivers`/`bump_no_reason` pairs; both downstream message chains: 4/4 branches selected correctly. *(round 10, commit `cb4e5b7`)* `actionlint .github/workflows/ci.yml`: 0 issues; `shellcheck -S warning` on the extracted `assess` and issue-step `run:` bodies: clean; sandbox harness driving the extracted block against real local git repos — clean+carries → `yes`, clean+behind → `no`/`ancestry`, split pin → `no`/`lint`, broken sibling caller → `no`/`lint`, node fault → `unknown`/`lint-fault`, three-problem tree → `(+2 more problem(s))`; injection probes against `$GITHUB_OUTPUT` and `::error::` with a hostile value carrying an embedded `oldest_sha=` line, a CR and a `%0A` sequence — no forged output keys, no forged annotation lines; issue body rendered under all six causes, each selecting its own step 1; `npm test`: 27 files, 652 passed, 0 failed; `npm run lint`, `format:check` (86 files), `typecheck`: clean; `node scripts/check-workflow-pins.mjs`: 1 caller, pins agree; `check-public-repo-hygiene.mjs`: clean. *(base merge, commit `f93d443`)* `pnpm lint`, `format:check`, `typecheck`: clean; `pnpm test`: 27 files, 654 passed; `check-workflow-pins.mjs`: 1 caller, pins agree; `check-spec-drift`, `check-public-repo-hygiene.mjs`: OK; `bash -n` on every `run:` body and `shellcheck -S warning` on the `assess` and issue-step bodies: clean. The `/tmp` fixture harness was not re-run after the merge. *(round 11, commit `7b85e1c`)* The extracted precedence block was run over all 8 combinations of lint (`pass`/`fail`/`fault`) and carries (`yes`/`no`/`unknown`), and each produced the expected `bump_delivers`/`bump_no_reason`. Fault with unknown carries now gives `unknown` with no reason. The `ls-tree -z` loop was run in a scratch repo holding `a"b.yml`, `é.yaml` and `README.md`: both YAML files were kept unquoted and the README was skipped. `bash -n` on every `run:` body and `shellcheck -S warning` on the pin-freshness job's bodies were clean, as were `check-workflow-pins.mjs` (1 caller, pins agree), `pnpm lint`, `pnpm format:check` and `check-public-repo-hygiene.mjs`. `actionlint` is not installed on this host and was not run this round. `pnpm test` was not re-run: the change touches only `ci.yml`. *(reviews pass, no new commit)* Read `dependabot-core`'s `file_parser.rb` on its default branch to check the round-11 High finding's naming premise (see above); no code changed. - **Deviations:** stderr capture was written to `/tmp/bump-lint.err` while `$gwtmp` did not exist, and moved to `$gwtmp/bump-lint.err` once the base introduced it *(base merge)*; the exit-1 classifier additionally requires the script's `check-workflow-pins: ` stderr marker, because node's exit code alone cannot separate a lint verdict from a crash (see above). *(round 10)* Scope was widened beyond the original plan to take all nine review findings, including the issue-body step 1 that the original PR had deliberately left as a Residual — reviewers raised it independently and it is two lines beside the `cause` chain it contradicts. The `lint-fault` text is truncated to 300 characters because a node stack trace is bound for a public issue body; the full text stays in the step log. *Note on CI: `ci.yml` is `branches: [main]`-scoped and `cursor-review-pin-freshness` is `if: github.event_name == 'schedule'`, so neither this job nor the repo's gates run on a stacked head. Everything above was run locally; no CI run on this PR exercises the changed code.* --- <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:19 +00:00