Repository navigation
review-pr: add T5, X8, D13, V7 and V8 from #1797 - #1817
xiaofei-zheng wants to merge 3 commits into
Conversation
Five rules from one PR's review history. #1797 fixed a real defect four times over, and each round was caught by its reviewer rather than by the checks, for a reason the rules did not name. T5 -- the fix moved a projection relative to a resolver, and the tests added with it called the two in the order they wanted, passing before and after. An ordering fix asserts the order itself or drives the real entry point. V7 -- the AST walk recording every os.environ write in _run_optimize never listed TP/CONC/EP: they are written one frame down. A tool scoped to one function and a tree with no writers give the same empty output, so a search is reported with its scope. V8 -- splitting a function and moving its call earlier broke nine tests in three files the author had not opened. Tests are callers; the caller list comes from a grep on the symbol, not from the files already in the diff. D13 -- treating a pin as a third source alongside the flag and the environment took five mechanisms whose only job was to keep them apart. The tree already resolved the same shape in one ladder; reusing it removed all five, 92 lines, and both defects found in between. X8 -- that PR added 56 lines of comment against 28 of code, most of it recounting how the arrangement got there. AGENTS.md already forbids narrating the change; this is the reviewer-side check, measured on the diff's own ratio.
Backtesting the rule against #1797's own history: the whole-diff totals read 6591 code against 1216 comment and cleared at every head, because test code dilutes the ratio and the merge of main swamped it. cli/__init__.py measured on its own read 28 against 56 and fired from the second commit onward. A per-commit reading inverts for the opposite reason -- a docs-only commit adds comment by construction.
…ationale Self-review with the rules in #1817 found two things this PR had missed. T5: the case covering the seeding calls it and the ladder in the order it wants, so it passes with the two swapped in _run_optimize -- verified by swapping them, where the behavioural case still passed and the defect was live. The ordering anchor table gains the pair, resolved against the resume branch's own ladder call rather than the fresh one, and fails on the swap. X3: a tree-wide sweep for the claim this PR's ladder contradicts found one more copy -- the Qwen skill told operators that "CLI defaults can otherwise override the intended workload", which stopped being true when the default became the bottom rung. The advice stands; the reason is now the real one. Two further copies state the advice without a rationale and are left alone.
PR #1817 -- review-pr: add T5, X8, D13, V7 and V8 from #1797What it does: PR #1797 took four rounds, each round introducing the defect the next one found, and Blocking issues: 1
Checked: |
This branch has not been deployed
2 participants
Description: Five
review-prrules learned from fix(cli): export every --extra-env pin so all readers resolve it alike #1797, which fixed one defect across four rounds — each round finding a defect introduced by the round before it, and each one caught by the reviewer rather than by the checks. The rules name the five reasons the checks stayed silent.T5 (tests): the fix moved a projection relative to the ladder that resolves a pinned value, and the tests added with it called the two in the order they wanted — passing identically before and after, while the production path still ran them the wrong way round. An ordering fix asserts the order itself, or drives the real entry point.
V7 (review method): the AST walk used to record every
os.environwrite in_run_optimizenever listedTP/CONC/EP, because they are written one frame down inside a helper. A tool scoped to one function and a tree with no writers produce the same empty output, so a search is reported together with its scope, and a clearance written without the enumeration saysSKIPPEDinstead.V8 (review method): splitting a function and moving its call earlier broke nine tests across three files the author had not opened — a
Namespacethat suddenly needed one more attribute, cases calling the half that no longer did the job, cases that now needed an empty environment. Tests are callers; the caller list comes from a grep on the symbol, not from the files the diff already touches. V7 enumerates the readers of a value, V8 the callers of a symbol.D13 (design): treating an
--extra-envpin as a third source alongside the flag and the environment required five mechanisms whose only job was to keep the pin from colliding with the projection. The tree already resolved the same shape in one ladder; reusing it removed all five, 92 lines, and both defects found in the intervening rounds could not arise in that form. The count on its own is taste — the finding is the count plus the simpler form the tree already contains.X8 (cross-artifact): the comment block that PR added to
cli/__init__.pywas appended to across four rounds of review and recounted how the arrangement reached its final shape, including a docstring describing a call that had been deleted.AGENTS.mdComment below the local average already forbids narrating the change; this is the reviewer-side check. The ratio is measured per non-test file and net of removals -- count a line as comment when it is a whole line leading with#or part of a bare string-expression statement, drop blanks, net the adds against the removes, compare to net code lines -- and it is taken on the head under review rather than on the merged result: fix(cli): export every --extra-env pin so all readers resolve it alike #1797 cut its own commentary back ine8ff4867c, so its final state no longer fires, while7a9752e4d, a head the review actually ran on, does.Linked issue(s): none
Tests: none — rules and skill prose only, no executable code. Backtested against fix(cli): export every --extra-env pin so all readers resolve it alike #1797's own history, which is where they came from: V7's sweep fires on the exact commit that moved the TP projection past
_preflightand clears on the one that moved it back; V8'sgit grepon_resolve_workload_knobslists the two test files the split broke and the author had not opened; D11 (review-pr: add D11 and D12, and make complexity above 20 a gate #1811) fires on the first head with 5 bare readers of the backend in files the diff never touched; T5's anchor assertion fails on the head before the ordering fix. X8 was corrected by the backtest itself. Verified by hand that all 60 rules have unique ids (the first draft collided with the existingD12), that every rule resolves to an index row or is one of the always-onV1-V8/X2, that no row names an undefined id, and that the SKILL.md rule count and always-on list moved with them.Size/complexity triggers crossed: none
If this simplifies or refactors: n/a
Observable effect: a reviewer running
review-pron a diff that reorders calls, splits a function, or defends itself with five interlocking mechanisms now has rules that fire. On fix(cli): export every --extra-env pin so all readers resolve it alike #1797 the first four rounds returned no blocking issues from the author's own review each time.Breaking changes: no
PR addresses single concern: yes
Root cause is upstream: no