From a4951f5446c4bfaa1185481297f0b76a00bda0ee Mon Sep 17 00:00:00 2001 From: wenshao Date: Fri, 31 Jul 2026 14:23:27 +0800 Subject: [PATCH] feat(review): check cache identity when reviewing workflow PRs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cache whose producer and consumer never agree on identity is invisible to every lens the review currently has. `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 exactly as written, and never hit once: `${{ runner.temp }}` expands to a different string on a hosted runner 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. The workflow path-rule already covered a cache a fork can poison; it said nothing about one that can never fire. Add that class as a blocker, so every dimension agent whose territory holds a workflow is asked whether the side that writes agrees with the side that reads — a question no assertion about the YAML's shape can answer, because the two sides' strings match in exactly the case that fails. Give verify-pr the method to settle it: read the matching key from the implementation rather than the documentation, compare the two sides' environment tuples rather than their YAML strings, and treat a hit that leaves no observable signal as the finding rather than a nit. Also record that the verify container is a live sample of the lane's own runtime. When the diff changes what those lanes execute, `command -v zstd` or `echo "$RUNNER_TEMP"` settles in one shell command what no amount of YAML reading settles, and needs no GitHub token — which that environment does not have. --- .qwen/skills/verify-pr/SKILL.md | 33 +++++++++++++++++++ .../commands/review/lib/path-rules.test.ts | 17 ++++++++++ .../cli/src/commands/review/lib/path-rules.ts | 1 + 3 files changed, 51 insertions(+) diff --git a/.qwen/skills/verify-pr/SKILL.md b/.qwen/skills/verify-pr/SKILL.md index 2b75bfe1071..bcaab1e3248 100644 --- a/.qwen/skills/verify-pr/SKILL.md +++ b/.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@` 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 diff --git a/packages/cli/src/commands/review/lib/path-rules.test.ts b/packages/cli/src/commands/review/lib/path-rules.test.ts index 91457c387e1..f76b852d0fc 100644 --- a/packages/cli/src/commands/review/lib/path-rules.test.ts +++ b/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'); + }); }); diff --git a/packages/cli/src/commands/review/lib/path-rules.ts b/packages/cli/src/commands/review/lib/path-rules.ts index 3d3fc053802..b51d46b54b2 100644 --- a/packages/cli/src/commands/review/lib/path-rules.ts +++ b/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 == '/'\` 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:**