mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-11-07 06:19:43 -10:00
feat(review): re-anchor on drift, run actionlint, account blockers, refuse retries (#16001)
* feat(review): re-anchor on drift, run actionlint, account blockers, refuse retries Four fixes from the 2026-10-31 audit of seven review run ledgers (#15057, #15377, #15385, #15578, #15853, #15955, #15974): - Head drift posts the completed review at the new head (the re-anchor protocol) instead of abandoning the round — the maintainer's ruling on #15578, never landed in the skill until now. The once-per-review bound and the COMMENT cap survive; the marker withholds its anchor. - script-lint runs actionlint's workflow-level checks (syntax, context availability, action refs) and gates them as [lint] Criticals; only the embedded run: shell stays deferred. #15974's unloadable workflow (runner context in jobs.env) is the regression test, measured against actionlint 1.7.12. - pr-context writes a blocker inventory; compose-review cross-checks it against the round's blockerRulings and caps at COMMENT when the walk skipped a blocker (#15974's missed review-level blocker). - submit documents --dry-run as the only sanctioned probe and refuses a re-post at the commit already landed (already-posted-this-commit) — the retry that replied this round's own roots back to themselves (#15853). The receipt records lastCommitId. Each fix carries its rule text, pin tests, and a DESIGN.md incident. * fix(review): isolate actionlint config and refresh lint on re-anchor Both Criticals from the #16001 review, reproduced against the exact PR source and closed structurally: - The newly active actionlint invocation auto-loaded the reviewed repo's .github/actionlint.yaml, so a PR-added ignore-all rule suppressed every workflow diagnostic — including the unloadable-workflow expression this linter was activated to catch. It now runs against a private mkdtemp neutral config via -config-file (measured to override the discovery), fails closed when no private config can be created, and carries exactly the two trusted compatibility exceptions the repo's own lint script runs (self-hosted runner labels, environment.deployment schema lag). The regression plants the ignore-all config in a real temporary git repository and proves the expression finding survives. - The re-anchor protocol dropped fixed authored findings but left the lint gate regenerating its findings from the old-head report, so a lint blocker the delta fixed was republished against the new commit. The protocol gains a refresh step (scratch worktree at the new head, script-lint --diff rewriting the report bound to the new diff) and a lintDiffPath compose field that makes the gate — body and ledger marker alike — hash the new diff as its freshness basis. Executable regressions pin both directions: fixed blocker gone (COMMENT, absent from body and marker), still-standing blocker keeps gating (REQUEST_CHANGES). A malformed lintDiffPath fails closed. * fix(review): base-tree actionlint config, empty-diff parse, lane provisioning Three findings from the round-3 review of #16001, each reproduced against the exact source and closed: - The private {} config blocked PR-planted suppressions but also dropped the repo's maintainer-merged compatibility exceptions (ECS runner labels, concurrency.queue, the $/ self-repository action reference), turning correct release-workflow changes into bogus [lint] Criticals. The trust boundary is now the MERGE-BASE tree's .github/actionlint.yaml (hex-shape-guarded git show, copied into the private file, {} only when the base has none); the head copy is never read, and the two trusted -ignore constants ride on top exactly as scripts/lint.js runs them. Pinned with a two-commit repo and a real-binary run showing the base exception honored while the planted ignore-all stays blocked. - An empty refresh diff (the new head reverted the PR's last change) was treated as a parse failure: the range selection fell back to the old plan's context-inclusive hunks and promoted a pre-existing finding into a blocker the gate accepted as fresh (the empty diff has a valid hash). diffParsed now separates parse success from range count; empty and deletions-only diffs classify everything pre-existing. The reviewer's exact scenario is an executable regression. - The lanes that run the gate provisioned no linters, so the new uninstalled-capping fired on CI and the real-binary regressions were skipIf'd out of every run. The review job and the unit-test lane now install the pinned, SHA-256-verified actionlint/shellcheck through the repo's own installer and promote the job-scoped dirs onto GITHUB_PATH (unconditional, advisory, Windows excluded). The 335 workflow meta-test pins pass unchanged. * fix(review): lint in the worktree cwd, guard lint.js entry, pin the sha fence Three gaps from the scripted deep verification of #16001 (50/55 assertions, 17 mutants), two of them in the round-3 fixes themselves: - F1: actionlint resolves the config's paths: entries against its cwd, and the runner spawned it in the session checkout, so every path-scoped base exception was inert while the global keys kept working — the split that hid behind the labels-only test. The runner now receives the worktree root and spawns inside it with PWD aligned. The real-binary suite gains the path-scoped fixture it was missing: a benign concurrency.queue edit that must lint clean, and an attack cell where the base exception suppresses the schema-lag finding while the planted head ignore-all still fails to hide the expression. - F2: the provisioning steps imported scripts/lint.js, whose unconditional top-level main() ran a full lint pass on import and never reached the install loop, silently under continue-on-error. lint.js now runs its CLI only when it is the entry point (import.meta.url guard, argv[1]-safe for node -e imports), and both steps assert each installer produced its binary before promoting the dir, so a future no-op install fails visibly instead of degrading to skipped tests. - M15: the option-injection fence was unobservable — git's own rejection of a malformed option matches the guard's refusal, so the guard's mutation survived. A spawn spy now pins the fence directly: non-hex merge base never spawns git; a hex-shaped nonexistent one does (positive control with the exact argv). Also acknowledges the verification's correction: the bot review's $/prepare-release-runner example does not exist at this head (the base config entry is precautionary); the queue exception at line 485 is the one reachable path-scoped case, and F1 is pinned on exactly that shape.
This commit is contained in:
1 parent
edde639992
commit
7bc00a3a08
20 files changed
+2166
-169
No files matched your search
@@ -762,6 +762,41 @@ jobs:
|
||||
echo '::warning::tmux/zip unavailable; real-tmux capture tests will be skipped and the zip-packaging suite will throw on CI.'
|
||||
fi
|
||||
|
||||
- name: 'Provision linters for real-binary tests'
|
||||
# The script-lint real-binary regressions (the #15974 unloadable
|
||||
# workflow, the PR-planted ignore-all config, the merge-base trust
|
||||
# boundary) are skipIf-gated on actionlint/shellcheck being on PATH;
|
||||
# without this step they silently skip on every CI run (PR #16001
|
||||
# review). Same pinned, SHA-256-verified installer the lint lane
|
||||
# uses, promoted onto GITHUB_PATH ahead of anything a shared runner
|
||||
# left there. Advisory: a failed install degrades to the skips, not
|
||||
# a red required check. Windows is excluded — the installer ships
|
||||
# linux/darwin archives only.
|
||||
if: "${{ needs.classify_pr.outputs.skip_ci != 'true' && steps.ci_profile.outputs.ci_profile == 'full' && runner.os != 'Windows' }}"
|
||||
timeout-minutes: 5
|
||||
continue-on-error: true
|
||||
run: |-
|
||||
node --input-type=module -e '
|
||||
import { execSync } from "node:child_process";
|
||||
import { appendFileSync, existsSync, mkdirSync, rmSync } from "node:fs";
|
||||
import { getLinters, getLinterTempDir } from "./scripts/lint.js";
|
||||
const tempDir = getLinterTempDir();
|
||||
rmSync(tempDir, { recursive: true, force: true });
|
||||
mkdirSync(tempDir, { recursive: true });
|
||||
const linters = getLinters();
|
||||
for (const name of ["actionlint", "shellcheck"]) {
|
||||
execSync(linters[name].installer, { stdio: "inherit" });
|
||||
// Verify the install produced its binary BEFORE promoting the
|
||||
// dir: a silent no-op install must fail this step visibly
|
||||
// (continue-on-error tolerates it, the UI shows it), not
|
||||
// degrade to skipped tests nobody notices (PR #16001 F2).
|
||||
const bin = tempDir + "/" + name + "/" + name;
|
||||
if (!existsSync(bin)) throw new Error(name + " installer produced no binary at " + bin);
|
||||
appendFileSync(process.env.GITHUB_PATH, tempDir + "/" + name + "\n");
|
||||
}
|
||||
console.log("PROVISIONED actionlint + shellcheck into " + tempDir);
|
||||
'
|
||||
|
||||
- name: 'Run tests and generate reports'
|
||||
id: 'unit_tests'
|
||||
if: "${{ needs.classify_pr.outputs.skip_ci != 'true' && steps.ci_profile.outputs.ci_profile == 'full' }}"
|
||||
|
||||
@@ -995,6 +995,42 @@ jobs:
|
||||
fi
|
||||
exit 0
|
||||
|
||||
- name: 'Provision review linters (actionlint + shellcheck)'
|
||||
if: "steps.context.outputs.should_run == 'true'"
|
||||
# The review's deterministic script-lint gate spawns actionlint and
|
||||
# shellcheck from PATH; without them every workflow/shell PR caps as
|
||||
# unreviewed and the real-binary regressions never run (PR #16001
|
||||
# review). Install through the repo's own pinned, SHA-256-verified
|
||||
# installer and promote the fresh job-scoped tool dirs onto
|
||||
# GITHUB_PATH — prepended, so the verified bytes shadow anything a
|
||||
# shared runner left on PATH. The install is unconditional (no
|
||||
# `command -v` shortcut): a pre-existing binary is exactly the plant
|
||||
# the capture-tools step refuses to trust. Failure degrades, not
|
||||
# blocks: the gate caps the review as unreviewed, disclosed.
|
||||
continue-on-error: true
|
||||
timeout-minutes: 5
|
||||
run: |-
|
||||
node --input-type=module -e '
|
||||
import { execSync } from "node:child_process";
|
||||
import { appendFileSync, existsSync, mkdirSync, rmSync } from "node:fs";
|
||||
import { getLinters, getLinterTempDir } from "./scripts/lint.js";
|
||||
const tempDir = getLinterTempDir();
|
||||
rmSync(tempDir, { recursive: true, force: true });
|
||||
mkdirSync(tempDir, { recursive: true });
|
||||
const linters = getLinters();
|
||||
for (const name of ["actionlint", "shellcheck"]) {
|
||||
execSync(linters[name].installer, { stdio: "inherit" });
|
||||
// Verify the install produced its binary BEFORE promoting the
|
||||
// dir: a silent no-op install must fail this step visibly
|
||||
// (continue-on-error tolerates it, the UI shows it), not
|
||||
// degrade to a review lane without linters (PR #16001 F2).
|
||||
const bin = tempDir + "/" + name + "/" + name;
|
||||
if (!existsSync(bin)) throw new Error(name + " installer produced no binary at " + bin);
|
||||
appendFileSync(process.env.GITHUB_PATH, tempDir + "/" + name + "\n");
|
||||
}
|
||||
console.log("PROVISIONED actionlint + shellcheck into " + tempDir);
|
||||
'
|
||||
|
||||
- name: 'Prepare review scratch directory'
|
||||
id: 'review_scratch'
|
||||
if: "steps.context.outputs.should_run == 'true'"
|
||||
|
||||
Reference in new issue
Block a user