mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-11 01:36:35 +00:00
33aee1b7e1
* fix(review): stop rendering a maintainer's blocker as an endorsement The Step 6 re-check exists to stop a review submitting `C=0` while a live blocker still stands on the PR. On #6486 it did exactly that, and the reason was structural rather than a lapse of judgment. `pr-context` quarantined a thread into the mandatory re-check section only if its body contained the literal string `[Critical]` — a marker only /review itself emits. A maintainer built the PR, drove the real CLI, and filed "Finding 1 — Ctrl+F dual-fires ... (blocker)" as an ISSUE comment. Every issue comment settled into "Already discussed — do NOT re-report" as a 240-character snippet, and the first 240 characters of that one were its preamble: "I built this PR from source and drove the real CLI ... to validate the model-toggle hotkey before merge." That reads as an ENDORSEMENT. The blocker began 1143 characters past the cut. Three hours later /review reviewed the same commit — the fix did not land until that evening — and submitted "no blockers". Recognition is now semantic (`carriesBlockerSignal`) and matches assertion patterns rather than word presence, with a negation guard. Blocker-bearing bodies — inline threads and issue comments alike — are promoted into a "Blockers to re-check" section and rendered in full. Word presence was the first cut and it does not survive contact with a real thread: on the live #6486 discussion it promoted 8 of 15 issue comments, of which one was a live blocker. The rest were the triage bot's own template line "No critical blockers." (the word inside its own negation), the author's "### Critical fixes" heading, and a comment quoting `[Critical]` while arguing a finding away. Eight full bodies took the context file from 30 KB to 59 KB and pushed the real blocker to character 43094 — past the 25000 one `read_file` returns. The section held the right blocker and no agent could read it, which is PR #5738's failure reintroduced one section further down. So the section is written FIRST, ahead of the description and the review history: nothing in this file outranks the claims a `C=0` verdict may not be reached without ruling on. On the live thread that moved the heading from character 25961 to 569 and the blocker body from 43094 to 4421. A character budget bounds the section even so; bodies past it degrade to snippets naming their exact fetch, which the re-check already must run before ruling. The fixture is the real comment body, byte for byte, so the regression is pinned against the thread that produced it. * fix(review): make "fixed by this diff" a verdict that has to be earned The Step 6 re-check has three verdicts, and until now only two of them cost anything: still stands REQUEST_CHANGES — blocks the merge cannot tell serialized into the body, caps the event at COMMENT fixed by this diff nothing. Silent, free, unrecorded. An agent choosing among three answers where one is free and two are not drifts toward the free one — and the free one is the only one that can ship a bug. The bar for it also read "you read the lines and the fix is there", which invites reading the diff's lines. That is precisely the reading that fails: a fix's new lines are always in the diff, but whether they WORK routinely depends on code outside it. #6486 is the case. The author answered a Ctrl+F dual-fire blocker by adding a guard to the toggle handler — visible in the diff, and it reads like a fix. It changed nothing. The second handler is text-buffer.ts:2663, in a file the PR never touches, subscribed independently to a KeypressContext.broadcast() with no stop-propagation; returning from one subscriber does not stop the other. Read the diff and you see a guard and rule "fixed". Read text-buffer.ts:2663 and you cannot. Determinism owns the evidence, judgment owns the ruling: - pr-context extracts the evidence. A blocker's body names the code it is about — #6486's named text-buffer.ts:2663 outright — so every promoted blocker now renders a "Referenced code" list. "Go read the untouched code" stops being a hope the agent might have and becomes a list it is handed. - SKILL.md raises the bar on the ruling: name the mechanism, name what now stops it, and when the stopping condition lives outside the diff, read it there — or the verdict is `cannot tell`. No new compose-review input: `cannot tell` already caps the event. The change is to make wrong "fixed" rulings land there instead of passing silently. * fix(review): a skipped CI check is not a passing CI check GitHub reports a skipped job as `status: completed, conclusion: skipped`. The classifier tested for failure conclusions and for pending statuses, and `skipped` matched neither — so it fell through both branches and landed the run in `all_pass`. A job that never ran was scored as a job that passed. This is load-bearing. /review treats green CI as its licence to approve, and the whole design delegates runtime truth to CI precisely because the LLM pipeline reads code statically. If the delegation returns nothing and returns it wearing a green badge, the delegation is worse than not having it. On #6486 the one job that would have exercised the new hotkey — "Integration Tests (CLI, No Sandbox)" — was skipped, as were the macOS and Windows Test legs. all_pass. "Did it run" is a question about the check NAME, not about any single run: this repo's routing workflows (authorize, review-pr, precheck-pr) routinely emit both a skipped and a successful run of the same name, and reporting those as unrun would bury the one skipped check that matters under a dozen that do not. A name counts as executed if any of its runs reached a real conclusion. Two deliberately different consequences: - Some checks skipped -> a disclosure, not a downgrade. A docs-only PR legitimately skips the test matrix, and auto-downgrading on any skip would downgrade every review in this repo, which is how a gate gets ignored. So presubmit names them and Step 7 rules on them — whether a skipped check would have exercised THIS diff is a question about the diff, which presubmit cannot see and the reviewer can. - Every check skipped -> a downgrade. Checks exist, not one ran: there is no green here to approve on, and no judgment is required to say so. A repo with no CI at all is a different claim (totalChecks === 0) and is not downgraded. The check-run shapes in the tests are the real ones from 6486's head commit. * feat(review): add a test-efficacy probe — does the new test gate the new code? Agent 5 asks whether a test EXISTS and whether its assertions look like they check something. Agent 7 runs the suite and reports that it is GREEN. Neither can see a test that protects nothing, and there are two ways to ship one: - unreachable — the project's test command never collects the file. - inert — it runs, it passes, and it would still pass with the change reverted. #6486 shipped both, in one file. The new test lived in integration-tests/, which is not an npm workspace, so `npm test --workspaces` never collected it; its CI job was skipped, so CI never ran it either. The test executed nowhere — not in CI, not in the review — and nothing in the pipeline noticed. Had it run it would have passed anyway: it drove a kitty CSI-u sequence into a PTY that never negotiated the kitty protocol, so the keypress was discarded before reaching the handler under test. It could only ever have caught a startup crash. Agent 5 saw a test file with plausible assertions and called coverage fine. Both questions are decidable without judgment, which is why they are a subcommand and not a prompt. Unreachability needs no execution at all — a path against the root package.json workspace globs. Inertness needs one run: revert the diff's source files to base, keep its tests, re-run them. The classifier is asymmetric on purpose. Reverting source frequently breaks a test's own compile — it imports a symbol the diff introduced — and the runner exits non-zero having collected nothing. Scoring that as "the test caught the revert" would hand back exactly the false assurance this command exists to remove. `gated` therefore requires a real ASSERTION failure; a bare non-zero exit with nothing collected is `inconclusive`, and `inconclusive` is never reported as a finding. Verdicts are per test FILE, not per run. One `vitest run` covers every probe, and a run-level verdict lets one honest test cover for a useless one: the gating test fails, the run reports failures, and the inert test beside it is scored `gated` too — so every inert test with a working sibling would be invisible, which is the exact defect this command exists to find. Found by running it against a real repo; the unit tests for the run-level classifier all passed. Two other limits are deliberate: a test-only diff is never probed (a new test for old code is SUPPOSED to pass with nothing reverted, and flagging it would be a false blocker on exactly the PRs we want people to write), and findings are Suggestions, not Criticals — a test that does not gate is not itself wrong code; what the finding must name is the behaviour now shipping unprotected. Driven against real PRs: #6433 reports GATED (9 assertions fail on revert, no finding); #6486 reports its integration test unreachable and its two unit tests gated. * fix(review): harden CI classification, path safety, and probe robustness Five correctness fixes surfaced by a Codex $qreview pass on this PR. Each was verified against the real code before applying; the review filed 30 Criticals, of which these are the ones that actually reproduce a wrong result. - presubmit: paginate `check-runs`. The single-page `ghApi` call saw only the first 30 runs — this PR's own head has 508 — so a failing or skipped job past the cut was invisible and could let a review approve past it. New `ghApiAllNested` streams `--paginate --jq '.check_runs[]'` as NDJSON (gh has no `--slurp`; the parse is split into a pure `parseNdjson` for testing). - presubmit: treat `startup_failure` as a failure. It was absent from `FAIL_CONCLUSIONS`, so a workflow that could not start counted as an executed run that added no failed name — an `all_pass` on a commit whose CI never ran. - presubmit: `waiting` and `requested` are active check-run statuses; add them to the pending set so a commit whose only check is waiting is not mislabeled `no_checks`. - pr-context: `extractCodeRefs` rendered path tokens from an untrusted comment body into the trusted "read each at the reviewed commit" directive. A blocker citing `../../../../etc/passwd.sh` or `/root/.ssh/id_rsa.key` entered the read list. Drop any absolute, `~`, or `..`-segment path; a real in-repo reference is repository-relative. - test-efficacy: raise the probe's `spawnSync` maxBuffer to 64 MiB (the ceiling the gh wrapper already uses). Vitest's JSON reporter on a large suite exceeds the 1 MiB default, returns ENOBUFS, and turns every probe `inconclusive`. * fix(review): comma-clause negation, RegExp flag preservation, stale comment Third self-review round. No blockers; these are the substantive suggestions. - pr-context: the negation stop-set gained clause separators last round but not the comma, so "No other concerns, but auth is a blocker" let the negation reach across the comma and suppress a real blocker — a false negative, the costly direction, flagged independently by two reviewers. Adding `,，、` leaves recall 2/2 and false positives 6/36 on the 38-comment corpus, and still negates "No blockers found, ship it". - pr-context: `carriesBlockerSignal` rebuilt each pattern with `new RegExp(re. source, 'g')`, dropping any flags the pattern carried. Harmless today (no pattern has flags) but a latent trap the moment one gains `i`/`u`. Preserve the pattern's flags and dedupe `g`. - test-efficacy: the workspace-glob comment still described the old two-pass filter as a present defect; the code is single-pass ordered evaluation. Fixed the comment to match. * fix(review): don't revert data fixtures; guard dirty worktree; CJK non-blocker Fourth review round (Codex $qreview + qwen). No blockers survived verification; these are the confirmed correctness issues. - test-efficacy: `planTestEfficacy` reverted every `kind: source` file, but `classifyPath` labels non-executable data under a src tree `source` too — JSON fixtures, `.md` bodies, snapshots. This PR ships one such fixture that `pr-context.test.ts` loads; reverting it deleted the file and made that probe inconclusive because of the probe itself. Revert now gates on an executable-source extension. - test-efficacy: refuse to run when the `--worktree` has uncommitted changes to a revert-set file. Safe on the pipeline's ephemeral worktree, but this is a public command and the checkout-over-revert would discard a user's staged or unstaged edits with no undo. - pr-context: `非阻塞` / `并非阻塞` is the Chinese "non-blocking" — the CJK twin of the `non-blocking` lookbehind. Without a guard, "非阻塞问题" promoted and consumed the mandatory-review budget. Same class as the bilingual-negation fix two rounds ago; a guard was written for one language and not the other. - DESIGN.md: sync the pattern list (bare `blocking`, not the noun forms) and the Referenced-code claim (only when the blocker names a file) to the code. * fix(review): make the dirty-worktree guard fail closed Fifth review round. One Critical and a vacuous test, both real. - test-efficacy: the dirty-worktree guard added last round called `spawnSync` directly and read `(r.stdout ?? '')`, so a spawn failure produced an empty string, read as "clean", and let the probe proceed — the fail-OPEN outcome in a guard whose whole purpose is to prevent data loss. Route it through a `gitOut` helper that throws on `r.error`/non-zero, and apply the same `r.error` check to `existsAtRev`. Verified: a dirty worktree now refuses to run and the uncommitted change survives. - pr-context.test: the comma-negation test's second assertion (`No blockers found, ship it` -> false) was vacuous — the plural `blockers` matches no pattern, so it proved nothing about the negation window. Replaced with `This is not a blocker`, which actually exercises it. - test-efficacy.test: add the source-only case (`probes: []` with a non-empty revert set), the mirror of the existing test-only case. * fix(review): NDJSON per-line tolerance, dedupe failed checks, GFM nesting Sixth review round — three small confirmed issues, two of them my own from earlier rounds. - gh: `parseNdjson` threw the whole page away if any single line failed to parse. `gh` can print an update/deprecation notice to stdout, so parse line-by-line and skip a non-JSON line instead of losing the records already read. - presubmit: `failedCheckNames` used `.push()` and so listed a matrix job once per failing platform ("Test, Test, Test"), while the `skippedCheckNames` I added dedupes via a Set. Dedupe `failedCheckNames` too. - SKILL.md: a nested `**bold**` inside a `**bold**` span (introduced when I qualified the Referenced-code claim two rounds ago) breaks GFM — the inner `**` closes the outer span, mis-rendering an agent-facing instruction. Drop the inner emphasis. * fix(review): redesign blocker negation; fail-closed parse; fixture-dir revert Seventh review round (Codex). Six confirmed issues, four of them regressions from my own earlier fixes — a sign the patch-on-patch approach to natural language and to the probe's file selection had to be replaced, not extended. - pr-context: replace the pile of per-pattern negation lookbehinds with one negation-window model. Each lookbehind fix had opened a hole in the other direction: `(?<!非)` suppressed `除非` ("unless", a real blocking condition); the adjacency-only guard missed `并非一个阻塞项`; the comma I added to the stop-set broke the coordinated list "No blocking, must-fix, or critical". The window now scans a negation word within ~40 clause chars, RESETS at an adversative (`but`/`但`) but not a bare comma, and breaks at `;`/`:`. Verified on an 11-case matrix and the 38-comment corpus: recall 2/2, false positives 5/36 (down from 6). Patterns are now bare, negation is one mechanism. - gh: `parseNdjson` is strict by default and `ghApiAllNested` uses strict. Last round I made it lenient to tolerate a `gh` update notice, but silently dropping a malformed check-runs line could hide a *failing* run — the fail-open the pagination fix closed. Leniency is now an explicit opt-in. - test-efficacy: the revert set excludes fixture DIRECTORIES, not non-code extensions. The extension whitelist also dropped runtime-loaded sources a test gates — an executable `SKILL.md`, a settings-schema JSON — so a skill-only change produced no probe. Directory is the right discriminator. - test-efficacy: the dirty-worktree guard adds `--ignored`, so a gitignored revert-set path recreated locally is not read as clean and overwritten. * fix(review): ReDoS, un-replied blocker promotion, status pagination, path guard Eighth review round (Codex). Several confirmed defects, and one I got wrong last round. - pr-context: **ReDoS in `CODE_REF_RE`** — I rejected this as a hallucination after testing the wrong input shape. Codex's exact shape (`"(blocker)\n" + "a".repeat(n)`) reproduces: the two overlapping greedy quantifiers `[\w./@-]*[\w-]+\.` backtrack catastrophically when `\.ext` fails, ~7s at 80k chars on an untrusted comment body. Replaced with a single bounded class `[\w./@-]{0,200}[\w-]\.` — 0 ms at 80k, same matches. - pr-context: **an un-replied blocker root was never promoted.** Only *replied* roots ran through `carriesBlockerSignal`; a fresh `[Critical]` with no reply went straight into "Open inline comments" as a 240-char snippet — the exact read-window failure this change exists to close, left open for the un-replied half. Open blocker roots now join the re-check section, rendered first and in full. - pr-context: the negation window resets at a space-surrounded hyphen (` - ` / ` -- `), an informal clause separator, without touching `must-fix` / `non-blocking`. - presubmit: paginate the legacy combined-status endpoint (same first-page-only gap as check-runs — a failing status on page 2 was invisible). - test-efficacy: reject a revert path that escapes the worktree (the report JSON is untrusted and these become git pathspecs / fs targets), and exit non-zero on a restore failure so a caller cannot mistake a base-code tree for a clean run. * fix(review): don't let the efficacy probe delete through a PR-controlled symlink A reviewer reproduced a P0. The efficacy probe reverts the PR's source to base in the shared worktree and restores it afterward, deleting files with `rmSync(join(worktree, p), { force: true })`. `rmSync` follows symlinks in the path prefix, and the revert set is PR-controlled, so: 1. base has a real `dir/victim`; 2. PR head replaces `dir` with a symlink to an outside directory and deletes `dir/victim`; 3. the probe restores HEAD (the `dir` symlink), then deletes the `dir/victim` path — which now resolves through the link and removes the OUTSIDE file. The lexical `escapes the worktree` guard added last round cannot catch this: `dir/victim` is lexically inside the tree; the escape is a runtime symlink traversal. Confirmed by driving the real handler — the outside file was deleted. Both delete sites (reverting an added file, restoring a deleted one) now go through `safeRmWithin`, which walks every path component from the worktree root and refuses when an ANCESTOR is a symlink. The final component being a symlink is still fine — that unlinks the link itself, which is what reverting an added symlink should do. A refusal fails closed: it sets the restore-failure disclosure and the non-zero exit, so the tree is never silently left mutated. Verified: the P0 repro now leaves the outside file intact and exits non-zero; a legit no-symlink PR still restores cleanly (exit 0, worktree back at HEAD). The deeper fix the reviewer suggested — run the probe in a disposable isolated worktree — also addresses the concurrent-read Critical and is tracked as a follow-up; this closes the file-deletion vector now. |
||
|---|---|---|
| .. | ||
| src | ||
| index.ts | ||
| package.json | ||
| test-setup.ts | ||
| tsconfig.json | ||
| vitest.config.ts | ||