feat(review): check cache identity when reviewing workflow PRs

.qwen/skills/verify-pr/SKILL.md

@@ -30,6 +30,19 @@ The workflow (`qwen-triage.yml` `verify` job) guarantees:
 - **You may execute PR code freely.** This job is the designated sandbox
   (container, no credentials) — the opposite of the `/triage` rules. Builds,
   node processes, loopback servers, and scratch `git worktree`s are all fine.
+- **This container is a live sample of the lane's own runtime.** When the
+  diff changes `qwen-triage.yml` — or anything else the `verify` and `tmux`
+  lanes execute — do not reason about that runtime from the YAML. Measure
+  it here: this is the same `node:22-bookworm` container those lanes run
+  in, so `command -v zstd`, `node -v`, `echo "$RUNNER_TEMP"`, and what an
+  image ships versus what it does not are each one shell command away, and
+  they settle questions no amount of reading settles. Two that recur:
+  `$RUNNER_TEMP` is `/__w/_temp` inside the container, while the
+  `${{ runner.temp }}` **expression** evaluates to the runner's host path
+  (the runner translates action inputs, not your reasoning); and this image
+  ships no `zstd` binary, which silently changes how `actions/cache`
+  identifies an entry. Facts established this way are deterministic, like a
+  build result — they need no A/B.
 - **Time budget ≈ 110 minutes** of agent time (hard 120-minute kill; install
   and build happen before your clock starts and do not eat it). Pick scope
   first (below); when time runs out, ship the report with what ran.
@@ -515,6 +528,26 @@ since the merge-base, say so and re-measure there.
   repo (tags, release commits, merge cadence in `git log`), label it as the
   bounded local estimate it is, and name the exact query a maintainer should
   run to confirm.
+- **Performance, caching, and reuse PRs**: the question is not "is it
+  correct" but "**can the mechanism fire at all**", and A/B has no purchase
+  on it — both sides of a cache restore run identical code. The proof is an
+  identity comparison instead. First, find where the matching key is really
+  defined, **in the implementation, not the documentation**: for
+  `actions/cache`, `npm pack @actions/cache@<version>` and read
+  `getCacheVersion` in `lib/internal/cacheUtils.js` — it hashes the literal
+  `path` strings and the compression method, not the key alone, so two jobs
+  that share a `key:` still miss forever when one runs on `ubuntu-latest`
+  and the other in a container (`/home/runner/work/_temp/…` versus
+  `/__w/_temp/…`, zstd versus gzip). Then compare the **environment
+  tuples** of the write side and the read side — `runs-on`, `container`,
+  what each path expression actually expands to, which tools each image
+  ships — never the YAML strings, which are identical in exactly the case
+  that fails. Close on observability: a restore step with no `id:` and
+  nothing written to `$GITHUB_STEP_SUMMARY` cannot report a miss, so the
+  failure is silent and permanent, and _that_ is the finding rather than a
+  nit. Worked example: a lane's npm cache shipped with matching keys,
+  matching `path:` lines, and 152 green YAML-shape assertions, and could
+  never have hit once.
 - **Config knobs**: trace every new input, flag, or option to an observable
   effect — a control that is recorded but never wired to behavior is a
   finding. Probe the **default** path of manual dispatch/config combinations

packages/cli/src/commands/review/lib/path-rules.test.ts

@@ -85,4 +85,21 @@ describe('pathRulesFor — scoped, or it is noise', () => {
     const out = pathRulesFor(['.github/workflows/x.yml']);
     expect(out).toContain('blast radius of the blocker above');
   });
+
+  it('asks whether a cache mechanism can fire at all, not only whether it is safe', () => {
+    // The checklist already covered a cache a fork can *poison*. It said nothing
+    // about one that can never *hit*, and on a real PR that gap held: the producer
+    // and the consumer shared a key and shared the `path:` line, so every
+    // YAML-shape assertion went green while `actions/cache` hashed two different
+    // `version`s — host path vs container path, zstd vs gzip — and no restore
+    // could ever match. Shape parity between the two sides is not identity parity,
+    // and no dimension agent asks which runner each side actually runs on.
+    const out = pathRulesFor(['.github/workflows/x.yml']);
+    expect(out).toContain('never agree on identity');
+    // What settles it is a comparison of environments, not of YAML strings.
+    expect(out).toMatch(/Compare the \*\*environments\*\*/);
+    expect(out).toContain('runs-on');
+    // And a miss nobody can observe is part of the finding, not a separate nit.
+    expect(out).toContain('$GITHUB_STEP_SUMMARY');
+  });
 });

packages/cli/src/commands/review/lib/path-rules.ts

@@ -56,6 +56,7 @@ const GITHUB_ACTIONS: PathRule = {
 - **A fork guard this diff removes or fails to add on a newly-privileged path.** For any trigger a fork can fire, the guard is what makes everything above unreachable: \`if: github.event.pull_request.head.repo.full_name == github.repository\`, an author-association check, or a \`github.repository == '<owner>/<repo>'\` gate on a scheduled job. A diff that adds a privileged trigger without one has added the vulnerability, not inherited it.
 - **\`$GITHUB_OUTPUT\` / \`$GITHUB_ENV\` written from untrusted data.** \`echo "x=$UNTRUSTED" >> "$GITHUB_OUTPUT"\` with a value containing a newline injects a second, arbitrary variable — \`PATH\` or \`NODE_OPTIONS\` among them. Multi-line values need the heredoc form with an unguessable delimiter.
 - **Artifact or cache poisoning across a trigger boundary.** A \`workflow_run\` job that downloads an artifact a \`pull_request\` job uploaded is pulling contributor-controlled bytes into a privileged context. So is a cache key a fork can populate.
+- **A cache or reuse mechanism whose two sides never agree on identity.** \`actions/cache\` matches an entry on \`(key | restore-key)\` **and** a \`version\` that hashes the literal \`path\` strings plus the compression method — not the key alone. So two jobs can share a key, share the \`path:\` line *as written*, and never hit once: \`\${{ runner.temp }}\` expands to a different string on \`ubuntu-latest\` than in a container job or on a self-hosted runner, and an image without the \`zstd\` binary picks gzip where a hosted runner picks zstd. Compare the **environments** — \`runs-on\`, \`container\`, what each path expression expands to, what each image ships — never the YAML strings, which match in exactly the case that fails. The same question governs any restore/skip/reuse mechanism a diff introduces: does the side that writes agree with the side that reads? A mechanism that can never fire is not a slow optimisation, it is a no-op carrying maintenance cost, and no assertion about the YAML's *shape* will ever say so — tests that the two sides' \`key:\` and \`path:\` strings match pass just as happily when neither side can reach the other's cache. If a hit leaves no observable signal either — a restore step with no \`id:\`, nothing written to \`$GITHUB_STEP_SUMMARY\` — the failure is silent and permanent; say that as part of the finding.
 
 **Recommendations (Suggestion) — say the cost, do not block on them:**