Repository navigation
Commit 085e98c
feat(review): audit the applied --fix for unpinned new assumptions (QwenLM#10169)
* feat(review): audit the applied --fix for unpinned new assumptions
Step 6B applied findings to the working tree and forbade re-running the
review over the result, so `--fix` output shipped with no independent
check at all — while the fix round is the loop's largest single source of
its own next round (a third of post-first-round findings, #9578), and
PR #9793's two fix-introduced Criticals survived a mutation probe of the
intended fix sites.
Add a scoped fix audit, not a re-review: `review fix-delta` records the
working tree before the first edit (a tree object through a throwaway
index — the user's index and the stash stack are never touched) and
diffs the tree against it afterwards, with the review's own side files
excluded; `agent-prompt --role fix-audit` renders the `fixed` findings
above those hunks into one digest-keyed list file and prints the launch
block for a single agent whose brief asks one question per hunk — what
does this edit newly assume, and does anything in the tree pin it — and
reports only the unpinned. The role reads no diff, carries no finder
machinery and no project rules, produces no verdict, and its output is a
disclosure (the affected finding's outcomeNote plus a terminal block),
never a finding. The command refuses an artifact without outcomes, one
with no `fixed` finding, and empty hunks beside a ledger that claims a fix.
Closes #10154
* fix(review): close round-1 fix-audit holes (#10169)
Co-authored-by: Qwen-Coder <[email protected]>
* fix(review): close round-2 fix-delta holes (#10169)
* fix(review): close round-3 fix-delta holes (#10169)
* fix(review): close round-4 fix-delta holes (#10169)
* fix(review): close round-5 fix-delta holes (#10169)
* fix(review): fail closed on fix-delta blind spots and capture notes (#10169)
Close the round-6 Criticals structurally instead of per entrance:
- The blind-spot probe now answers in two states — confirmed dirt and
unresolved paths — instead of one folded flag. Unopenable directories,
failed inner probes and over-budget walks are disclosed at every
comparison and never enter the snapshot baseline, the exclusion
families are applied to what the ignored-directory walk discovers,
and the empty-diff all-clear is hedged to what `git add -A` captures.
- The `add -A` capture is ruled on the child's own stderr line-by-line
(spawnSync keeps stderr on exit-0 paths too): only the zero-commit
nested-repository error, the embedded-repository notice with its hints,
and the autocrlf normalisation warnings are tolerated — any other note
refuses the snapshot and names the gap.
- The hunks artifact is written as git's raw patch bytes; `-z` names are
decoded one by one, so non-UTF-8 fix content and filenames survive.
* fix(review): close round-7 and round-8 fix-delta holes (#10169)
- Refuse a `git add` child that did not exit normally (killed by timeout
or buffer overflow): its notes are a partial capture, and ruling on
them recorded HEAD's tree as the snapshot baseline (R5-2).
- Raise the `add -A` spawnSync maxBuffer to the 512 MiB ceiling every
other git spawn in this tree uses, so a large autocrlf tree's warnings
cannot kill the child mid-capture (R7-7).
- Pin LANG/LC_ALL to C for the review git children: the tolerated-note
patterns match git's English rendering, and translated catalogs turned
every tolerated shape into a hard refusal (R7-5).
- Render git-facing paths in git's `/` separator: the in-tree git-dir
exclusion pathspec (R7-8), the exclusion matching and walk-discovered
disclosure names, which compare against git-originated `/`-separated
names in reporting and baseline identity (R7-6, R8-1).
- Fix the suite's absence assertions to name the all-clear the command
actually prints; the old phrase appears in no output, leaving the
never-all-clear-beside-a-blind-spot contract unpinned (R7-9).
- Skip the two POSIX-only fixtures on the Windows lane (R7-1, R7-2) and
normalize the recorded snapshot root before comparing it (R7-3).
- Carve the empty-hunks refusal out of the fix-audit blanket refusal
rule in the skill: it is a ledger/tree mismatch whose message directs
a correction, not an audit failure to move past (R7-10).
* fix(review): close round-9 and round-10 fix-delta holes (#10169)
Resolve the outstanding Criticals on `fix-delta` and the fix-audit
refusal, and merge origin/main (the one conflict — the `review`
demandCommand message — is resolved by keeping BOTH new subcommands,
main's `emit-workflow` and this PR's `fix-delta`).
R9-1: pin `-c core.quotePath=false` on the hunks `diff-tree` so a
non-ASCII file name arrives raw in the hunks headers exactly as it does
in the summary; the default quoted form gave the fix audit two
spellings of one file.
R9-2: exclude the command's own `--out`/`--since` paths from capture,
comparison and probe whenever they resolve inside the repository — the
name families only cover what the REVIEW flow writes, and a
non-canonical in-repo side file was entering the hunks as bookkeeping.
R5-1 (structural): discover nested repositories from the index's own
mode-160000 gitlinks, not status entries alone — assume-unchanged /
skip-worktree bits hide dirt from BOTH status runs; confirm an empty
inner status against the index's tags instead of reading it as clean;
treat unknown-type (DT_UNKNOWN) dirents in the ignored-directory walk
as unreadable instead of silently skipping them. Uninitialized gitlinks
still stay quiet.
R8-2: resolve a symlink's TARGET against the family / in-tree-git-dir
exclusion at both probe sites, so a link planted at a non-family name
cannot reach the review's own worktrees past the exclusion.
R8-3: reassemble multi-line zero-commit `git add` notes at line
boundaries before matching; a nested repo whose name contains a newline
is tolerated as promised, not refused.
R7-10: replace the empty-hunks refusal's impossible "re-take the
snapshot before the first edit" remedy with the no-recovery disclosure
— at refusal time the edits are already in the tree — in both the skill
and the fix-audit prompt, and re-pin the tests to the new wording.
Also harden the suite teardown against the detached auto-gc race the
11k-file tree triggers (rmSync retry).
Every fix lands with its own witness; mutation probes confirm each
guard turns its test red when removed.
* fix(review): refuse side-path symlink writes, close blind-spot entrances (#10169)
* fix(review): close round-11 fix-delta holes (#10169)
R11-1: pass --binary to the hunks diff-tree. Without it a binary-content
edit entered the hunks — the command's sole output — as a bare
"Binary files ... differ" stub: no patch data, not git apply-replayable,
while the summary still reported the file. Witness: a NUL-byte fixture
edit must arrive as a GIT binary patch.
R5-1: key the blind-spot identity on the raw path bytes (the latin1
byte<->char bijection) instead of the lossy display decode — decodePath
is not injective, and a clean nested repo named bytes C3 A9 swallowed its
dirty sibling named byte E9 (both render 'é') through the `seen` mark,
printing the bare all-clear beside the landed edit. The persisted
baseline compares the same keys. Walk-discovered rels now join with
git's '/' on every platform so both discovery routes key identically.
R5-1 (round-7 entrance): refuse the spawn UTF-8 coercion in the inner
probe — a name the bytes cannot represent would reach the child mangled
to U+FFFD and probe a planted decoy in place of the real repository.
The probe fails for it instead; the failure direction over-warns.
R11-2: failure-couple the Step 6B fix-audit block in the skill — `&&`
between the hunks producer and the auditor prompt, plus the branch for
the producer's failure: fix-delta writes its hunks only as its final
act, so a failed producer leaves whatever an interrupted earlier run
wrote at the same deterministic path, and the auditor must not run over
it. Re-pinned in the skill test beside the producer/consumer pins.
Every fix lands with its own witness; mutation probes confirm each guard
turns its test red when removed.
* fix(review): close the fix-audit's blind spots and its claim-vs-edit gaps
Round-15 review findings on the Step 6B fix audit, addressed at the
mechanism rather than entrance by entrance.
fix-delta — the probe no longer trusts the tree it measures:
- Every probe child carries `-c core.fsmonitor=` and `--work-tree <path>`.
A repository discovered in the working tree owns a `.git/config` that is
writable as this user, and `core.fsmonitor` runs a command on `status`
AND on `ls-files -v` — the measurement became the execution. A repo-local
`core.worktree` redirected the same status at a pristine decoy, so the
probe answered clean over an edit on disk; the work-tree pin outranks it,
and a probe that cannot honour the pins answers `failed`, never `clean`.
- The baseline records a DIGEST of the state inside each dirty path, not a
boolean. "Already dirty at snapshot time" is now a checkable claim: a
level-2 gitlink that merely moved used to stamp its parent, after which a
fix's real edit inside that parent was filed as dirt that was already
there and the blind-spot warning went silent.
- The probe's status runs with the review's name families left IN, and
prunes what it discovers by name instead. A nested repository planted at
`sub/.qwen/tmp/qwen-review-*` fell out of the capture pathspec and out of
the probe's at once; the families hold review FILES, never a repository,
so only the worktree family (`review-pr-*`) prunes one.
- A symlink whose target merely CONTAINS a repository is walked, not
returned from silently; the git-dir exclusion compares bytes rather than
the non-injective display decode.
- Repo-local config that shapes the capture and the rendering cannot be
overridden, so it is disclosed: `filter.<name>.clean`, `info/attributes`,
`core.attributesFile` and an active `info/exclude` at both moments, and
any changed path whose `diff` attribute resolves to `-diff` or a driver —
the surface that turns a text fix's hunks into an unreadable binary patch.
agent-prompt — the ledger and the tree are cross-checked:
- Zero `fixed` outcomes no longer asserts "nothing was applied" without
reading `--hunks`. Beside hunks that landed it is a ledger/tree mismatch,
the mirror of the empty-hunks refusal, and the audit is not what to skip.
- Each `fixed` finding is reconciled against the hunk headers: one that no
hunk corroborates is marked in the rendered entry (a fix may legitimately
land elsewhere), and an input where none of them is corroborated is
refused. The brief says what the marker means.
SKILL.md — the two claims that were false as written: the skip clause now
requires an empty hunks file as well as an empty `fixed` set, and the
interactive path rules per target instead of asserting a sweep that never
reaches a file review's plan — `file-review-…` is outside every cleanup
prefix by contract, so when that plan survives the audit runs there.
* fix(review): ask the attribute probe about the raw name bytes
`renderSteeringPaths` was fed `filesBetweenTrees`' DISPLAY names. A name
whose bytes are not UTF-8 renders through latin1, and re-encoding that
string as UTF-8 asks `check-attr` about a different path — one no rule
matches — so it resolved `unspecified` and the run answered "no steering"
for exactly the path it could not name. Fail-open, in the one direction
the disclosure exists to close.
`filesBetweenTrees` now hands back the raw name buffers and the display
decode happens at the reporting edge, where it belonged: the probe asks
about the bytes, the summary prints the rendering. Pinned by a
`bad<0xE9>.txt` fixture whose `-diff` rule is planted with the same bytes
— feeding the decode back in reds it.
* chore(review): smooth the claim-vs-edit refusal wording
* fix(review): derive the fix-delta prune from git's registry and pin the outer spawns
Round-16 findings on the fix audit, closed at the model rather than
entrance by entrance:
- R5-1: nothing prunes by name any more. The probe excludes the in-tree
git dir and the review's own worktrees — repositories under the
worktree family whose gitfile points into the audited repository's
`worktrees/` registry — and walks or probes everything else, so a
repository planted under a worktree family name, or one level under a
side-file family directory, is discovered. The digest carries the
repository's identity (`status --branch --show-stash`) and is kept for
every probed path, so content committed or stashed inside a nested
repository the trees record nothing of is disclosed.
- R16-1: every outer spawn against the audited repository carries the
probe's pins (`--work-tree <root> -c core.fsmonitor=`) — `diff-tree`
with a pathspec included, which loads the index — and root derivation
is gated: the tree git names must contain the cwd, or a repo-local
`core.worktree` redirect is refused.
- R16-2 / R16-3: `captureSteeringSurfaces` names `core.excludesFile`
(when its file carries an active rule) and `filter.<name>.process`.
- R16-5: the tracked half of a family match is recorded after the
family exclusion (`add -u` over the paths the throwaway index tracks,
byte-exact through `--pathspec-from-file`), and the tree comparison
runs under the literal excludes only.
- R13-2: `hunkHeaderPaths` admits git's C-quoted header productions,
contributing nothing on a decode it cannot complete.
- R16-4 / R16-6: Step 6B enumerates the three ledger/tree-mismatch
carve-outs and relays fix-delta's stderr qualifications under the
Fix audit heading; DESIGN.md follows.
Also tolerates the "unable to index file" note newer gits (2.55) pair
with the zero-commit note, for the same path only.
* fix(review): anchor the fix-delta baseline and close the round-17 blind-spot entrances
Round-17 findings on the fix audit:
- R5-1: the registry check binds back — a review worktree is one whose
`<common>/worktrees/<entry>/gitdir` names the probed path, so a gitfile
merely pointing into a genuine entry is probed. A review worktree is
walked rather than pruned on every discovery route, so a repository
planted inside it is found. Two new transitions, `appeared` (answered
now, no baseline) and `vanished` (baseline, no answer now), join the
all-clear gate; the six transitions now cover every key of either
digest map. The interior of a git directory is ruled out of scope and
the all-clear line says so.
- R17-1: both digest maps are null-prototyped, so a repository named
`__proto__` is an ordinary key on the probe side and the loaded record.
- R17-2: `hunkHeaderPaths` is hunk-aware — after `@@ -l,s +l,s @@` the
counted body lines are consumed before header scanning resumes, so a
deleted `-- a/<path>` content line can no longer corroborate a finding.
- R17-3: with a clean/process filter configured, `add`'s notes are ruled
on their own lines (no multi-line reassembly a forged opener could
absorb); the single-line zero-commit pairing stays, which keeps the
audit for Git-LFS users on git >= 2.55.
- R17-4: `--snapshot` prints the full tree sha and a SHA-256 fingerprint
of the record; `--since` requires `--fingerprint` and refuses a file
whose bytes no longer match. Step 6B carries the printed hex into the
`--since` call and never recomputes it from the file.
Two rounds of independent reverse audit before this commit: round 1
caught the LFS regression of the first cut (pairing disabled under a
filter) and the fingerprint-recompute loophole; round 2 found no
silence-direction defect and a DESIGN.md sentence that contradicted the
code, fixed here, plus the two route pins for the review-worktree walk.
* fix(review): classify review worktrees by the orchestrator's naming and close the round-18 entrances
Round-18 findings on the fix audit:
- R5-1: nothing in the tree classifies a review worktree any more — the
registry lookup, its back-link and the family-name match are gone.
`fix-delta --review-worktree <path>` (repeatable) names the worktrees
this flow created; those are walked for planted repositories and never
probed, everything else is content and its dirt disclosed. A `--fix`
run names none. The digest's status is pinned to `core.quotePath=true`
so a non-UTF-8 name cannot collapse two interior states into one hash.
- R18-1: the mode guard counts presence; an empty `--since` is refused.
- R18-2: a link resolving to the audited root, or a walk re-entering it,
is recognised by filesystem identity (dev/ino) and never probed as its
own nested repository.
- R18-3: the side-path redirect check runs again before each write,
covers the ancestor chain of an in-repository side path (bounded by
the checkout, from the nearest existing component), and the writes
open with O_NOFOLLOW where the platform has it.
- R18-4: `check-attr diff` is asked about a rename's source names too.
- R18-5: root derivation also requires the git dir discovered from the
cwd to be the git dir the derived root belongs to.
- R18-6: the capture pins `core.autocrlf=false` (and `core.safecrlf=false`
beside it); the nested-repository probes keep git's own view of
"modified", for the reason stated in DESIGN.md.
Found by the reverse audits before this commit rather than by the
review: in a repository that ignores `.qwen/` — this one does — the
skill's own in-tree `--out` made `add -A` exit 1 on the negative pathspec
item, so every fix audit refused there. Ignored side paths now stay out
of the capture's pathspec (`check-ignore`), pinned by a test in the exact
skill shape.
Two rounds of independent reverse audit: round 1 found the ignored side
path refusal, a resolved-spelling root check that a differently-cased
link defeats on APFS, and a `safecrlf=true` refusal under the autocrlf
pin — all fixed here; round 2 found no silence-direction defect and a
DESIGN.md sentence describing the rejected root check, corrected.
* fix(review): close the round-20 fix-delta and fix-audit entrances
- fix-delta: key the blind-spot walk on filesystem identity — one
physical directory walked, one physical repository probed, one entry
budget shared by every walk of the run — so a link cycle inside an
ignored directory cannot mint tens of thousands of synthetic probes
(R20-14).
- fix-delta: close the blind-spot state matrix. `--ignored=matching`'s
`!` entries ride the digest but never count as dirt; discovery stat
failures are ruled by errno so only ENOENT skips silently (ELOOP and
its kin disclose); and the baseline persists the unresolved paths, so
an unresolved-then-gone path is its own transition that gates the
bare all-clear (R5-1).
- fix-delta: derive the honest toplevel config-blind — walk the cwd up
to the first `.git` entry (directory or gitfile) and refuse when it
is not the root git named — plus refuse the git-dir re-entry
signature, closing the subtree and planted-gitfile-ancestor
`core.worktree` redirects the containment+identity gate passed
(R18-5). The nested probe's status is likewise refused when its
toplevel is not the path it was asked about.
- fix-delta: disclose a tracked family-named path the fix renamed away
— the hunks can only certify the bare deletion (R19-1) — and take the
house 512 MiB maxBuffer at the shared git opts so the steering
enumeration cannot overflow into silence (R19-2).
- agent-prompt: decode C-quoted path tokens by code point, so an
astral character in a quoted name no longer decodes to U+FFFD pairs
(R19-3).
- agent-prompt: drop the wholesale "no hunk corroborates any fixed
finding" refusal — a fix can legitimately land in files no finding
names, and the two states are indistinguishable to the command, so
the all-unmatched case is annotated and built; SKILL.md, the brief,
and the user docs rule it the same (R20-26).
- skill/docs: state the fix audit's reach precisely — the local/file
`--fix` path where `fix.effective` runs; the #9793 incident happened
on the posted-comment path #10153 covers (R20-1).
Each new guard carries a pin that reds when the guard is removed
(proven per guard); the full review suite passes apart from the eight
pre-existing macOS-only failures that fail identically on the base.
* fix(review): close the round-19/20 fix-audit findings with a state matrix and config-blind gates
fix-delta:
- The comparison is a closed 4×4 matrix (dirty/clean/unresolved/absent at
both moments); unresolved paths are persisted for disclosure so U→A is
reachable; a status-discovered path whose interior probe fails is
unresolved, never "dirty without a digest".
- Ignored (`! `) entries of a nested repository ride its digest but are not
dirt; its collapsed entries take the top level's routing entry for entry
(a directory that IS a repository is probed, one that holds repositories
is walked, a slashless entry may be a link); an ignored entry appearing or
vanishing is disclosed by the interior-moved note.
- Nested probe: `.git` must resolve (through a link) to the git dir git
discovers from inside; a clean/process filter in any non-user scope of
the merged config (own config, include.path/includeIf, per-worktree; read
from `-z` fields so a newline in a value cannot forge a scope, and an
answer that does not pair off is unresolved, not "no filter") makes the
repository unresolved; errno-keyed catches (only ENOENT silent);
FIFO/socket/device dirents classified.
- Walks are keyed on filesystem identity (dev:ino; no identity on ino 0),
with a per-walk budget and a per-run cap.
- Root gate is config-blind: the derived root must be the directory of the
first `.git` git's own discovery accepts at or above the cwd (a gitfile,
or a directory whose HEAD validates).
- Tracked family-named paths deleted between the trees are disclosed.
- Every git wrapper carries the 512 MiB maxBuffer; an unreadable filter
enumeration is disclosed as such.
agent-prompt: C-quoted header names decode by code point; the wholesale
"no fixed finding corroborated" refusal is replaced by per-entry annotation;
only a hunks file with no header line is refused.
SKILL.md/DESIGN.md/docs: reach stated exactly (local/file --fix path; the
posted-comment path is #10153's); carve-outs are the two ledger/tree
refusals plus the annotation rule; core.ignoreCase named as a residual.
Verified: fix-delta 122/129 locally (7 non-UTF-8-name fixtures are EILSEQ on
APFS) and all 129 in node:22 (git 2.39.5); agent-prompt 324; SKILL 63;
review-directory suite 5898 passed; 24 mutants each killed by one pin; two
rounds of independent reverse audit before commit, the second closing a
nested collapsed entry that IS a repository, the global-filter scope ruling
and the HEAD validation.
* fix(review): close the round-21/22 fix-audit findings — hooks, discovery bounds and invented deletions
fix-delta:
- Hooks are steering the tree carries: every spawn now pins
`core.hooksPath` at a path that cannot exist, and the capture's
`check-ignore` — the one working-tree-measuring spawn without them —
takes the probe pins, so neither `core.fsmonitor` nor a
`post-index-change` hook runs inside a measurement (R21-1, R21-2).
- A nested repository's collapsed entries take the top level's routing
entry for entry, so a repository under an ignored path OF a nested
repository is discovered rather than all-cleared over; identity dedupe
goes through the house `hasVerifiableInode` predicate, so a filesystem
reporting no inode walks and probes rather than collapsing every
directory into one; and the walk is bounded to the audited repository,
so a directory link out of the tree is named as scope and never
baselined — by bytes and identity, since Node's portable realpathSync
cannot read a path byte no UTF-8 decode accepts (R5-1, three
entrances).
- A `.git` entry planted inside the working tree narrowed every reading to
a subtree: the run is refused when an enclosing checkout answers for the
same git dir, which no submodule or linked worktree produces (R18-5).
- The walk budget is charged per directory OPENED, not per dirent: this
checkout's own node_modules (8,174 directories, 84,566 entries) spent
the ceiling on every run, so `node_modules` was disclosed as
unresolvable every time and the all-clear could never print (R22-1).
- A family-named path the user staged without committing is captured from
the user's index too — the throwaway index seeded from HEAD cannot see
it, so a fix's edit to it produced an empty delta (R22-2).
- A deletion the capture invented — a path git now reports ignored whose
file is still on disk — is dropped from the hunks and disclosed, so the
auditor's sole input never asserts an edit the fix did not make; the
ignore test is what keeps a file the fix replaced with a directory of
the same name in the hunks (R22-3).
SKILL.md: the no-`fixed`-beside-landed-hunks refusal names its out-of-band
cause and gets an exit that is not inventing a `fixed` outcome (R21-3).
Verified: fix-delta 131/138 locally (7 non-UTF-8-name fixtures are EILSEQ
on APFS) and all of them in node:22 (git 2.39.5); agent-prompt 331; SKILL
64; 38 mutants each killed by one pin; measured in this checkout that the
walk no longer discloses node_modules on every run; reverse-audited before
commit, which closed the invented-deletion classification, the byte-safe
root bound and the out-of-root scope bucket.
* fix(review): close the round-23 fix-audit findings
- fix-delta: the staged-family re-inclusion and the ghost classifier
test the ENTRY with `lstatSync` (drop only on ENOENT), so a staged or
untracked DANGLING symlink is no longer dropped from both trees /
skipped by the classifier — git records a link (mode 120000) whether
or not its target exists (R22-2, R23-3).
- fix-delta: `ghostDeletions` asks `check-ignore` with `--no-index`:
the question is whether the RULES hide the path, and without it the
user's index outranked them, so a staged-but-uncommitted file was
never recognised as a capture-invented deletion (R22-3).
- fix-delta: `assertCompleteCapture` splits the shared stderr on the
note boundary too (`/\n|(?=(?:error|fatal|warning|hint): )/`): a
filter child that omits its trailing newline merged its bytes with
git's next note, and the prefix-anchored tolerance absorbed the note
that proves a path was skipped (R23-2).
- fix-delta: `isGitEntry` asks git (`rev-parse --git-dir`) instead of
approximating `validate_headref` — `ref:` with zero whitespace and
uppercase SHAs are git's to rule, not a regex's — and gate 3 walks
every strict ancestor holding a `.git` path UNVALIDATED, comparing
git dirs, because a validating predicate's miss stood the gate down
(R18-5).
- fix-delta: one classifier at every walk/probe entrance (R5-1):
`escapesRoot` is tri-state (inside / outside / unresolvable — an
unresolvable link rides `unresolved`, never `outOfRoot`), consulted
by `probeLinkedRepo`, `probeNestedInterior` and `walkAndProbe` alike;
a link to an out-of-root REPOSITORY is still probed (its uncommitted
dirt gates) but rides the outOfRoot bucket, so a commit inside a
foreign store reads as scope rather than a move inside the audited
tree; the status-entry route registers the physical repository in
`state.probed`; the nested interior sweeps its own index for tracked
symlinks; the in-tree git-dir comparison is byte-exact; and where the
inode cannot answer, identity falls back to the resolved spelling.
- skill/cli: the out-of-band exception is keyed on OWNERSHIP, not
path/finding overlap — a foreign write can land in a path a finding
names, and the overlap-keyed exception instructed falsifying the
ledger there — and the empty-hunks refusal names the third cause
(the edit landed where the capture cannot see it) in both the ruling
and the message (R21-3).
Each new guard carries a pin that reds when the guard is removed
(proven per guard; the non-UTF-8 byte-shadow case runs on Linux, like
its siblings). Suites: fix-delta 142 passed on macOS (the 9 failures
are the pre-existing EILSEQ/teardown class, identical on the base),
the rest of the review directory 5706 passed, agent-prompt 331, SKILL
64, tsc/eslint/prettier clean.
* fix(review): carry the capture's pathspec as bytes through stdin
The capture's exclusion reached `add -A` as argv: spawn args coerce a
name that is not valid UTF-8 to U+FFFD, so an in-tree git dir named with
such bytes (a `--separate-git-dir` the user chose) fell out of the
exclusion, and the capture recorded the audited repository's own objects
as edits — caught on Linux CI by the round-23 byte-shadow pin
(`probes a repository whose name decodes to the in-tree git dir name`,
whose planted repo sits exactly at the mangled name). The `add -A` call
now reads the pathspec with `--pathspec-from-file=- --pathspec-file-nul`,
the same channel the tracked re-inclusion already uses, with the git-dir
entry carried in `inTreeGitDirBytes`' raw form.
* fix(review): stop excluding the in-tree git dir by its decoded name in the probe pathspec
The probe's status/diff pathspec carried the in-tree git dir through
argv, which coerces a non-UTF-8 name to U+FFFD: beside a
`--separate-git-dir` named `gd-<0xE9>`, the exclusion matched a
lookalike directory (`gd-<EF BF BD>`, valid UTF-8) and hid exactly the
content the probe exists to see — the Linux run of the round-23
byte-shadow pin caught it. The probe pathspec now drops the git dir
entirely: `probeExcluded` prunes it byte-exactly on every discovery
route, and the trees never contain it (the capture's exclusion is the
raw-bytes stdin pathspec). The byte form becomes the one exported
pathspec (`capturePathspecBytes`), and the test pins move with it.
* fix(review): close the round-24/25 blind-spot and capture-invention entrances
The fix audit's measurement mis-answered a shape per route, each route
assuming another had ruled it:
- probeNestedRepo — the funnel every discovery route git-spawns, digests
and baselines through — carried no root-containment ruling of its own,
so an index gitlink whose worktree is a link out of the tree (and a
repository one level inside an out-of-root link target) was baselined
as this tree's content. The funnel rules the reach first now: the
audited root returns, an unresolvable path rides unresolved, an outside
one is recorded as scope — and probed all the same, keeping the link
route's deliberate probe of out-of-root dirt.
- A discovered link whose target is not a directory was dropped
unclassified; the reach ruling now precedes the directory test on the
link route and inside the ignored-directory walk alike.
- A tracked file whose worktree copy is a symlink carries an N... sub
token and an unchanged index mode, so neither the S-token gate nor the
index sweep routed it anywhere; the record's worktree mode (120000)
routes it through the link classifier now.
- The nested status spawn pins core.trustctime (a nested repository's own
config answered clean over a same-size, mtime-restored edit), and is
ruled on stderr as well as exit code: an exit-0 "could not open
directory" warning certified clean over a subtree nobody read. The
index-bit confirmation recurses to the depth the status was taken at,
bounded by the probe run's visited/budget state.
- isAuditedRoot's lexical fallback compared a latin1 decode of the path
bytes and failed open on a non-ASCII root; it compares bytes.
- inTreeGitDirBytes accepted '\' as the root/git-dir boundary byte on
every platform; on POSIX a sibling '<root>\packages' git dir then read
as the in-tree 'packages/', dropping that subtree from capture and
probe. The byte is win32's separator spelling only.
- The T2-side transitions (appeared/movedInside/vanished) keyed the scope
exemption on the union of both moments — a path that was a link at
snapshot time and holds an in-tree repository now is content of this
tree and is no longer exempted on the baseline's say-so. The scope note
keys on freshDirt, so pre-existing dirt on an out-of-root path falls
back to the note instead of cancelling into a bare all-clear.
- The capture-invention classifier was deletion-only: an ignore rule
REMOVED between the moments admitted pre-existing untracked content as
full-file additions. The snapshot records what the rules hid
(ls-files --others --ignored), and the addition side is ruled against
that record — the first moment's rule set is gone by --since. The
deletion side now refuses a ghost when the path holds a directory: a
real replacement the fix made, not the capture's invention.
- The file-target plan family (file-review-*) joins the excluded side
families, so a Step 6B re-run on a file target stops capturing the
audit's own brief/input/launch records as the hunks it audits.
- recover-findings filters the fix-audit key from the sections channel
too: its disclosures are not findings, and a resumed run re-audits from
a fresh snapshot, so nothing is lost.
Every entrance carries a red-proven test (guard removed, witness red,
guard restored); the pins the findings named stay green — the all-clear
gate's freshOutOfRoot keying, the literal pathspec's glob-name case, the
three neighbouring ghost cases, and the byte-shadow pathspec pin.
* test(review): skip the unreadable-interior witness where permissions cannot bite
win32 has no POSIX permission bits to make the directory unreadable and
root bypasses them everywhere, so the fixture's precondition cannot be
constructed there — the same guard the suite's other
unreadable-directory witnesses carry.
* docs(review): name the channel the fix-delta snapshot fingerprint prints on
The Step 6B contract said "prints one line" without saying where; the
line goes to stderr (writeStderrLine), and an orchestrator capturing
only stdout loses the fingerprint and walks into the "Never recompute
it from the file" prohibition with no recovery path. Pinned in
SKILL.test.ts.
* fix(review): close round 26's capture, probe and protocol entrances
The R26 review of the fix audit's measurement found one leak per layer:
- The family re-inclusion trusted "the index holds it, so it is user
content" — and Step 6B's own artifacts (the outcomes/findings rebuild
between the two moments) arrive exactly that way when the user staged
`.qwen` in a checkout that does not ignore it. The re-inclusion now
drops the flow's own bookkeeping by name, and every path it still
admits is disclosed on its own line.
- inRepoSidePaths decided containment lexically against the canonical
root, so a side path spelled through a symlinked prefix (every macOS
/tmp path) read as outside and the side file was never excluded — and
the redirect guard's ancestor walk stood down over the same spelling.
Containment now goes through repoRelativeOf (canonical, with the
not-yet-existing leaf resolved through its nearest ancestor), while the
redirect guard judges each symlink component of the typed spelling by
where its parent canonically sits: a link inside the checkout must
resolve inside it, a link whose parent is outside is the user's own
layout.
- The capture's note ruling gated its strict per-line form on a filter
being configured, but the tolerated notes embed raw path bytes — a
nested repository named `A'error: 'B` forges the unterminated opener
with no filter involved, and the reassembly absorbed a real failure
behind it. The ruling is unconditional now, and the filtersConfigured
plumbing is gone. (A zero-commit note split by a newline in the repo's
own name now refuses — the failure direction over-warns.)
- The capture claimed content filters had no equivalent override;
filterBlankEnv is exactly that. Repo-local filter commands are screened
and blanked through GIT_CONFIG_* pairs on the capture's spawns and the
discovery status (a planted `filter.*.clean` no longer executes as the
reviewing user), while global filters (the user's own, e.g. git-lfs)
stay.
- The blind-spot probe's discovery status ran through a wrapper that
discarded stderr, so an exit-0 `could not open directory` over a
subtree certified a partial enumeration as clean. The spawn now rides
gitRawReport (raw stdout kept, stderr captured, status kept): a warning
lands the named path (or the root, when the note's shape is unknown) in
unresolved, never clean.
- probePins gains core.checkStat=default beside core.trustctime — a
nested repository's own core.checkStat=minimal reopens the same
stat-cache cell one knob over.
- The index-bit recursion treated an UNREADABLE level-2 checkout like an
absent one; only ENOENT means "nothing there" now, and anything else is
unconfirmable — never clean.
- The snapshot's hid-set unions ls-files --cached --ignored with
--others: a staged-but-uncommitted ignored file was in the user's index
and invisible to both, so a removed rule admitted it as a fabricated
addition the classifier could not see.
- Tree-controlled strings (path names, config values) interpolate into
the single-line protocol notes escaped — a config value or directory
name carrying `\nfix-delta: …` forged a standalone protocol line,
including a fingerprint receipt the orchestrator would keep over the
real one. Escaping happens at the render boundary only; identity keys
stay bytes.
- The subtree-narrowing gate keyed on an enclosing checkout answering for
the SAME git dir; a planted `.git` gitfile naming ANY OTHER git dir
narrowed every reading identically. A gitfile narrowing now requires
reciprocal registration — a gitlink in the enclosing index, or a
worktrees/<name>/gitdir entry naming this checkout's `.git` — and a
planted gitfile is registered from nowhere. The same-git-dir refusal
and the copied-worktree carve-out stand.
Each fix carries a red-proven test (guard removed, witness red, restored
green), including the two-directional ones: the newline-named zero-commit
repository test inverts from tolerated to refused, and the two redirect
re-check fixtures move their swap onto a GLOBAL filter — the one channel
the blanking deliberately leaves executing.
* fix(review): narrow the file-target family's own records out of the re-inclusion too
R26-1's narrowing keyed on the qwen-review-{target}-* suffixes the
finding enumerated; the file-target plan family carries the same flow
bookkeeping — the plan report and the role-keyed prompt records under
its -prompts directory — and a staged copy of either rode the capture
identically. Same contract: the names the flow writes are dropped by
name, user content under the family still re-includes, and the
disclosure line still names everything admitted. Test extended with
both file-review shapes; removing the narrowing reddens it.
* test(review): pin the fix-audit wave emit-workflow builds for Step 6B
Step 6B now dispatches the auditor as a one-agent batch manifest through
emit-workflow, so pin that the batch path builds it verbatim and without
a worktree pin — a local or file plan carries no worktreePath, and the
audit must read the working tree the fix was applied in, not a review
tree.
* fix(review): close the round-10 capture entrances and anchor the hunks hand-off
R10-1: the redirect check lstat'd only the prefixes of the excluded
directories, so a symlink planted at a family-matching NAME under
.qwen/tmp redirected every side file the flow writes under it; lstat the
family entries themselves and refuse one that resolves to a directory.
R5-1: two new entrances to the false bare all-clear. A TRACKED symlink
reaching a git repository emits no status entry, so the walk never found
the repo behind it — probe an out-of-root link target that holds one
instead of only naming the scope. A nested repository's own mode-160000
gitlinks take the root's index-sweep ruling, so a dead interior gitlink
is unresolved rather than silently skipped.
Also from the deferred list: fix-delta --since now fingerprints the hunks
file it writes, and agent-prompt --role fix-audit reads the hunks only
against --hunks-fingerprint, closing the tree-writable rewrite window
between the two processes. Flow bookkeeping is derived from the Step 1
plan path (the plan file and its prompt-record directory) instead of a
hand-listed suffix set, and the staged half of the family re-inclusion is
gated fail-closed on the snapshot's own record of what it re-included.
A filter screen that cannot be read to the bottom now refuses the capture
rather than blanking nothing, and the capture pins core.fileMode=true so
an exec-bit edit survives a repo-local fileMode=false.
* fix(review): seed the second fix-delta capture from the snapshot tree and keep the capture out of nested repositories
The `--since` capture is seeded from the snapshot's own tree instead of
from HEAD. Seeded from HEAD at both moments, the comparison also carried
every path the tracked set gained or lost between the moments (a commit
in the window) and every path an ignore rule written in the window hid
from `add -A`, and a deletion classifier had to guess, per record, which
of the two had made it — one entrance per round (a rule hiding an
embedded repository's gitlink; a HEAD gate asked after the move). With
the first tree as the second seed an entry the first capture recorded is
an entry the second one holds whatever the rules say now, so a deletion
in the hunks is a path gone from the disk and nothing else: the deletion
classifier and its note are removed. What the seed cannot carry — a
HEAD-tracked path absent at the snapshot and on disk again — is
re-admitted off the snapshot's recorded HEAD. A commit in the window is
disclosed as a moved HEAD and withholds the bare all-clear; so is a
record that names no HEAD, or one whose HEAD the repository no longer
holds. The addition-side classifier records what the rules hid by kind
— files, links, nested repositories (a `sub/` listing, or a staged
gitlink typed off its index mode) — and reads an addition as a rule's
removal only when its kind matches what stood there; a file or a link
the fix put in a hidden repository's or file's place rides the hunks.
The re-admission skips a path that now sits behind a link, inside a
repository the capture recorded as a gitlink, or under a component that
became a plain file, instead of dying on the pathspec.
`git add -A` decides whether a gitlink is modified by running a status
inside the checkout, and that child executes the checkout's own
repo-local filters, fsmonitor command and hooks — measured, and no `-c`
on the parent reaches it. The seed's gitlinks are now excluded from
`add -A` by pathspec and refreshed by hand off `rev-parse HEAD` with
`update-index --index-info`, entry for entry as git rules them; both
discovery statuses run `--ignore-submodules=all`; every gitlink is
probed by the index sweep after its own config has been screened, with
gitlinks ordered before symlinks so a repository answers under its
registered name. A checkout whose `.git` git does not accept as its own
— a hollow directory, a gitfile naming an enclosing checkout's git dir —
stands rather than being refreshed off the HEAD discovery would find by
walking up.
Also: an empty gitlink checkout (a clone's, or a non-recursive
`submodule update --init`'s at level 2) is skipped like an absent one at
both levels, read with readdirSync rather than existsSync; a nested
repository's skip-worktree bits take the sparse-checkout exemption
`local-anchor.ts` already pays for; every name that reaches a probed
repository is recorded against its filesystem identity and `--since`
reads the baseline by identity too — only where both names resolve to
the same place now, so a rename or an inode reused after a delete stays
disclosed — and a route that changed is not a vanished-plus-appeared
pair; the discovery-status and `add` refusals are
escaped at the render boundary; and the fix auditor's input renders the
finding's path through `inertPath`.
* fix(review): close the round-29 fix-delta findings and the audit-found siblings
The capture's gitlink refresh writes a null OID as long as the
repository's object names (64 hex under sha256), and leaves an entry
standing when the checkout's HEAD is of the other length. The scratch
directory's parent is resolved from `rev-parse --absolute-git-dir` as
bytes, with only git's one trailing newline removed; a git dir whose
name cannot be spelled into `GIT_INDEX_FILE` falls back to the system
temp directory when that is outside the tree, and refuses otherwise.
`core.ignoreCase=false` is pinned on every spawn that measures a working
tree — the capture's and the nested-repository probes' — wherever that
tree's filesystem is case-sensitive: a `true` there is never git's own,
and under it `add -A` and a nested `status` fold a case-colliding sibling
into silence. The probe is read-only, once per path, on the tree's own
volume: its `.git` entry lstat'd under `.GIT`, with ENOENT, a different
inode, an unverifiable inode, or a hard-linked gitfile read as sensitive.
The hidden set's `--cached` half probes the entry before typing it, so a
staged gitlink whose checkout is gone is recorded nowhere. Both family
listings read their recorded mode: a mode-160000 entry is never handed
to `add -u` or `add -A -f` (an explicit gitlink pathspec runs a status
inside the checkout, measured), and is named on stderr once; the staged
half hands `add -f` only a path that lstats and names what it left out.
A baseline recorded through a link out of the tree is withdrawn when the
probe no longer reaches the name as scope, so a removed or replaced
external link routes as vanished or appeared, and the identity alias
reads the withdrawn baseline rather than the snapshot.
The nested-repository probe asks about skip-worktree bits before it
rules on dirt, so a hand-set bit cannot hide an edit behind pre-existing
dirt; the sparse-checkout exemption is granted only where the rules
govern the worktree (every unflagged tracked path in-rules, with the
index's gitlinks kept out of both sets), and the ungoverned or unaskable
paths are named on stderr beside `git sparse-checkout reapply`.
SKILL.md routes "edits no outcome owns" one way — a foreign write leaves
the ledger alone; only an unrecorded fix of a finding is a ledger to
correct — with pins. Six new tests use a spawn-only nested-repo helper
so they run on the Windows lane; the newline-name and exec-bit cases are
gated `win32`; case-sensitivity cases skip on an insensitive volume.
* docs(review): tell the fix-audit agent apart from the fix-audit round
With #10136 on the branch two things answer to "fix audit": the PR
re-review's narrowed round, read off the plan's `incremental.posture`,
and Step 6B's one agent over the hunks a local `--fix` applied. Step 6B
now says which one it is and why the two never meet in one run (the
round needs a pull-request target, where `fix.effective` is false), so
a reader cannot carry the round's convergence carve-out into a local
`--fix` review. DESIGN.md records the same; SKILL.test.ts pins the
sentence.
* refactor(review): state the fix-delta scope instead of certifying it
Thirty review rounds went into making `fix-delta` certify that its hunks
were the whole edit: nested-repository digests, ignored-path and symlink
classification, sparse/skip-worktree bits, redirected excludes, filter
screening, record fingerprints. fix-delta.ts grew from 222 to 5,680
lines and held 142 of the loop's 185 Criticals, each round's fixes
opening the next round's entrances — for a check whose output is a
disclosure that changes no verdict.
Replace the certification with a fixed scope printed on every `--since`
run: the hunks hold what `git add -A` records in this repository, and an
edit inside a submodule or nested repository, or to a gitignored file,
is named as outside it. What stays is what the audit needs: the
throwaway-index capture, the review's own side files excluded (both the
name and the directory form, at any depth, plus the command's own
--out/--since), the second capture seeded from the snapshot tree, and a
moved-HEAD disclosure. HEAD is now read once and the capture is seeded
from that sha, so the record and the tree describe one moment (R30-1).
agent-prompt drops --hunks-fingerprint and the C-quoted patch-path
parser; whether a fixed finding's edit is among the hunks is now the
auditor's check, which has both in front of it. SKILL.md Step 6B,
DESIGN.md and the user docs follow; lib/worktree.ts is back to main and
lib/git.ts keeps only gitWithEnv.
* fix(review): keep fix-delta working under .qwen/ ignore rules
Five everyday shapes the shrink regressed, each measured in a scratch
repository:
- A literal exclude for --out/--since under an ignored directory made
`add` refuse the whole capture ("The following paths are ignored"),
so wherever `.qwen/tmp` is ignored — qwen-code's own `.qwen/*`, the
`.qwen/` rule /setup-github writes, a bare `tmp/` — the audit never
ran. A side file an ignore rule already hides is no longer excluded
(add -A cannot capture it anyway).
- The throwaway index lived under os.tmpdir(); a TMPDIR inside the
working tree captured the index itself. It now lives under the
absolute git dir, which also holds in a linked worktree or submodule.
- `core.autocrlf` printed one conversion warning per file into the
stderr the orchestrator relays, and past Node's 1 MiB default the add
was killed (ENOBUFS). The add pins core.safecrlf=false (the stored
blob is unchanged) and gitWithEnv takes gitRaw's 512 MiB ceiling.
- In a cone-mode sparse checkout an untracked file outside the cone made
`add` exit 1; the capture passes --sparse. Its snapshot cost in very
large sparse checkouts is measured and stated in DESIGN.md.
- The fix-audit input showed a finding's first location only, so a fix
landing at its second location read as unattested. Every location is
listed and the brief (and the ledger-note template) ask about any.
Names on stderr now go through inertPath, the header check requires the
patch to open with `diff --git`, the user docs tell this agent apart from
the fix-audit-shaped review round, and every guard has a witness that
goes red when it is removed (43 mutants), including runs from a
subdirectory and from a linked worktree.
* fix(review): make the fix-delta scope line say what the capture does
R31-1: the scope line claimed coverage the capture does not deliver. It
now describes the capture as it behaves: the hunks hold the files HEAD
tracks and every other file no ignore rule hides; an edit to a gitignored
file HEAD does not track, or to any path in the review's .qwen/tmp name
families (tracked or not), is out of scope; and a hunk shows a file as
git stores it (a binary file as `Binary files … differ`, a Git LFS file
as its pointer). SKILL.md, DESIGN.md and the user docs relay the same
wording, and tests pin each qualification against the behaviour.
The command's own --out/--since files are now excluded from the diff
range only, never from a capture. `add` refuses a pathspec whose literal
prefix names an ignored path, which is why the previous version asked
`check-ignore` first — and asked it of the user's index, so a file HEAD
tracks but the user's index dropped leaked into the hunks (triage
finding). `diff-tree` has no such refusal, so the probe is gone.
The unreachable `(no location)` branch in renderFixAuditInput is gone;
validateFindings guarantees a location.
---------
Co-authored-by: Qwen Code Autofix <[email protected]>
Co-authored-by: Qwen-Coder <[email protected]>
Co-authored-by: qwen-code-dev-bot <[email protected]>
Co-authored-by: probe <probe@local>
Co-authored-by: wenshao <[email protected]>1 parent ca8146a commit 085e98c
17 files changed
Lines changed: 2247 additions & 31 deletions
File tree
- docs/users/features
- packages
- cli/src/commands
- review
- lib
- core/src/skills/bundled/review
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
93 | 93 | | |
94 | 94 | | |
95 | 95 | | |
| 96 | + | |
| 97 | + | |
96 | 98 | | |
97 | 99 | | |
98 | 100 | | |
| |||
232 | 234 | | |
233 | 235 | | |
234 | 236 | | |
| 237 | + | |
| 238 | + | |
235 | 239 | | |
236 | 240 | | |
237 | 241 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
59 | 59 | | |
60 | 60 | | |
61 | 61 | | |
| 62 | + | |
62 | 63 | | |
63 | 64 | | |
64 | 65 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
31 | 31 | | |
32 | 32 | | |
33 | 33 | | |
| 34 | + | |
34 | 35 | | |
35 | 36 | | |
36 | 37 | | |
| |||
76 | 77 | | |
77 | 78 | | |
78 | 79 | | |
| 80 | + | |
79 | 81 | | |
80 | 82 | | |
81 | 83 | | |
| |||
98 | 100 | | |
99 | 101 | | |
100 | 102 | | |
101 | | - | |
| 103 | + | |
102 | 104 | | |
103 | 105 | | |
104 | 106 | | |
| |||
0 commit comments