Repository navigation
chore(review): gate Xcode-CLT stub paths (REVIEW.md lesson 7 + machine check) - #2474
Conversation
On macOS /usr/bin/{git,python3,swift,swiftc,clang,gcc,make} are not those
tools — they are one inode hardlinked as the Xcode-CLT stub. The file exists
whether or not the developer tools are installed; invoking it without them
raises a modal "install command line developer tools" dialog and returns
nothing.
Three properties make this its own lesson rather than a case of lesson 6
(hardcoded absolute paths). The path is NOT machine-specific — it exists on
every Mac, so it reads as portable. Every existence probe passes against the
stub (test -x, command -v, shutil.which, FileManager.fileExists), so the
obvious guard does not work. And an absolute path cannot be shadowed by a real
install on PATH, so a user who installs git or python still gets the dialog.
Adds lesson 7 (renumbering the verdict lesson to 8) and the six stub paths to
the machine-readable checks: block, so scripts/review-checks.sh gates every PR.
'/usr/bin/swift' covers '/usr/bin/swiftc' by prefix. Non-stub /usr/bin binaries
(env, pgrep, lsof, osascript, open, id, pmset, xcode-select) are deliberately
absent — they are real binaries the codebase addresses absolutely on purpose.
The scanner already skips # and // comment lines, so the many existing comments
discussing /usr/bin/python3 are unaffected. tests/review-checks-clt-shims.test.sh
pins that, plus every stub being caught in executable code and every non-stub
staying clean, by running the real runner against the real REVIEW.md.
MERGE ORDER: land after sonichi#2469 and sonichi#2473. Those PRs introduce the resolver
constants this gate is designed to make unnecessary (SYSTEM_GIT = "/usr/bin/git",
systemPython = "/usr/bin/python3"), and the gate flags them as added lines. Once
merged they are no longer in any diff and the gate is clean — verified.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 74%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
scripts/review-checks.pyLines 244-252 244 j = start + 1
245 while j < len(all_lines):
246 nxt = all_lines[j]
247 if nxt.startswith("@@ ") or nxt.startswith("+++ ") or nxt.startswith("diff "):
! 248 break
249 if nxt.startswith(" ") or nxt.startswith("+"):
250 body.append(nxt[1:])
251 j += 1
252 return body |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed exact head f1e56978. The regression class exists on current main and the guide-only layer is correct. Reviewer-run new suite passes 10/10; the three adjacent review-check suites pass (16 shell, 23 Python, and review-pr), and the stated stub positives/comment/non-stub controls behave as documented.
[P1 machine-gate false positive] /usr/bin/swift is a substring pattern, not an exact executable token. The body intentionally uses it to cover swiftc, but on this current macOS host /usr/bin/swift-inspect is a separate real binary (inode 1152921500312572861, link count 1), unlike /usr/bin/swift/swiftc (shared inode 1152921500312571562, link count 78). An executable line naming it is nevertheless rejected:
+proc.executableURL = URL(fileURLWithPath: "/usr/bin/swift-inspect")
review-checks: FAIL ... hardcoded path (/usr/bin/swift-inspect)
rc=1
This contradicts the PR's narrow-stub claim and its explicit goal of preserving safe absolute /usr/bin tools. Add a negative control for the real swift-inspect binary and scope the pattern/allow so swift and swiftc remain blocked without blocking swift-* real tools (a token-start allow for /usr/bin/swift- is one minimal option under the current scanner semantics). The same substring property also makes /usr/bin/make reject /usr/bin/makeinfo; even though that binary is absent on this host, a robust fix should avoid prefix-family false positives rather than special-case only one current filename.
Worst case is a mandatory review gate rejecting legitimate platform-tool invocations, encouraging broad exemptions or disabling the check—the exact usability failure the PR says its negative controls prevent. The merge-order dependency on #2469 then #2473 remains correctly documented, and clean-install is still running.
Verdict: changes requested; not merge-ready on this head.
Found while wiring the TS-side resolver against this gate. The scanner skipped
`#` and `//` comment lines but had no equivalent for block-comment continuation
lines, so prose inside a /** … */ block was scanned as code:
src/x.ts:2: hardcoded path (/usr/bin/python3):
* On macOS `/usr/bin/python3` is the CLT stub, not python.
Pre-existing (a JSDoc line naming /Users/… was always flagged), but it becomes
acute with the stub paths added in this PR: the modules that RESOLVE these tools
necessarily document the hazard in JSDoc, so every such module would trip its
own gate and the check would get switched off.
Skips ` * text`, a bare ` *`, and ` */`. Deliberately does NOT skip an opening
`/*` — a `/* … */ code` one-liner must stay scannable so a comment cannot smuggle
a path onto a code line — nor a generator method `*name()`, which has no space
after the star. Requiring whitespace, end-of-line, or `/` after the star is what
separates them; both shapes are pinned by tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-reviewed exact head ade6f5fc. The original P1 remains: the current gate still rejects the real link-count-1 /usr/bin/swift-inspect binary (review-checks: FAIL, rc=1), and the new head does not add the required prefix-family negative control or scoping.
The JSDoc follow-up adds a second P1 scanner bypass. _BLOCK_COMMENT_CONT = r"^\*(\s|$|/)" skips every stripped executable continuation line beginning * , not only lines inside a block comment. This is valid JavaScript/TypeScript multiplication syntax:
const n = 2
* "/usr/bin/python3".length;Reviewer control: Node executes it and prints 32, while the exact-head review scanner reports PASS / rc=0 for the two-line added diff. The new tests cover a generator method but not the ordinary multiplication operator. Please track actual /* ... */ comment state across context/addition lines (with the same fail-closed care used for Python docstrings), or otherwise constrain the skip so standalone executable * lines remain scanned; add this multiplication control.
Reviewer-run declared suites still pass: CLT gate 10/10, runner 16/16, scanner 26/26, and review-pr. That green state does not discriminate either failure. Worst case is a mandatory security/review gate that both rejects safe platform binaries and can be bypassed by valid executable formatting.
Verdict: changes requested; not merge-ready on this head. Coverage and clean-install are also rerunning, and the documented #2469 → #2473 → #2474 merge order remains.
Addresses @john-the-dev's P1 on this PR: '/usr/bin/swift' as a SUBSTRING also rejects '/usr/bin/swift-inspect', which is a separate real binary (its own inode, link count 1) — unlike /usr/bin/swift and /usr/bin/swiftc, which share the stub inode with 76 other names. Same shape for /usr/bin/make vs makeinfo. A gate that blocks legitimate platform tools gets disabled rather than fixed, which is the exact usability failure this PR's negative controls exist to prevent. Adds a `flag_exact:` list to the checks block, matched against the WHOLE extracted path token rather than as a substring, and moves the stub paths there. `flag` keeps its substring semantics — '/Users/' must still match '/Users/alice/deep/path' — so existing behaviour is untouched (pinned by a new control). swiftc no longer rides on the '/usr/bin/swift' prefix; it is named explicitly, as are clang++, g++, cc and gnumake, which the prefix never covered. The list is the subset of the 78-name family this repo plausibly invokes; lesson 7 now records how to re-derive the full family by inode. Negative controls added for swift-inspect, swift-frontend, makeinfo and gcc-14. makeinfo is absent on the current host, so it is pinned as a synthetic control rather than left to whatever happens to be installed. Tests: shim suite 10 -> 15, scanner unit suite 26 -> 30. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both findings addressed. @john-the-dev P1 — I took the general fix rather than the Negative controls added for Second finding (mine): JSDoc prose was scanned as code. The scanner skipped Suites: shim gate 10 → 15, scanner unit 23 → 30. Diff coverage on One note on the shellcheck Cross-PR map (same root cause — please read before reviewing in isolation)On macOS one inode is hardlinked 78 ways under
Merge order: #2469 and #2473 before #2474 (the gate flags the resolver constants they add, as added lines; once merged those lines are in no diff and it is clean — verified both ways). #2475 is exempt: its tier 3 uses a bare The clean-VM ask is one session, not three#2469, #2473 and #2475 have each been asked for separate clean-no-CLT-host evidence. It is the same host and the same boot: launch the packaged app once on a toolchain-free VM and all three are observable in that single run — #2459 is the missing context#2459 is closed but not merged, and it matters to all four:
What these four do NOT cover@john-the-dev's inventory on #2459 is still live. Counting executable lines on current
I am not expanding these PRs to cover the rest — single concern — but reviewers should know the class is not closed when these merge, and that the two boot-path callers ( |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Blocking (current head ef1eb249ac677fbba452ed9c4b88082328dddf34):
The exact-token change fixes the earlier /usr/bin/swift-inspect / makeinfo false positive, and the added suites pass (CLT 15/15, runner 16/16, Python 30/30, review-pr PASS). The remaining scanner bypass from my prior review is unchanged, though.
This is valid executable JavaScript:
const n = 2
* "/usr/bin/python3".length;
console.log(n); // 32At this head, feeding that two-line addition to scripts/review-checks.py with RC_FLAGS_EXACT=/usr/bin/python3 produces no finding and exits 0. The cause remains _BLOCK_COMMENT_CONT = re.compile(r"^\\*(\\s|$|/)") plus the unconditional _BLOCK_COMMENT_CONT.match(stripped) skip: any added code line beginning with multiplication * is treated as JSDoc prose even outside a block comment.
Please make JSDoc continuation suppression stateful (only while actually inside a block comment), and add this exact executable multiplication form as a regression that must be flagged.
… bypass) @john-the-dev's second finding on this PR, and it is a bypass rather than a false negative on prose. The previous commit skipped any line whose stripped form began with `* `, on the theory that only JSDoc bodies look like that. This is also valid JavaScript: const n = 2 * "/usr/bin/python3".length; so the path went unflagged. Reproduced on the prior head: that two-line addition scanned clean and exited 0. Now tracks the real `/* … */` span, so a line is exempt because it IS inside a comment, never because of how it happens to start. Consequences, all pinned by tests: - continued multiplication -> FLAGGED (was the bypass) - JSDoc body in a real span -> exempt - `/* … */ code` one-liner -> FLAGGED (opens and closes on the line, so the code after it is outside the block) - `*gen()` generator method -> FLAGGED - code after the block closes -> FLAGGED Block state resets per file and per hunk and advances on context lines, matching the existing docstring tracker. It shares that tracker's known limitation — a `/*` inside a string literal moves the state — which errs toward scanning. Scanner unit suite 30 -> 33. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@john-the-dev — fixed on Reproduced your exact case on the prior head first: The shape-based rule was the error: Same head, after: Five behaviours pinned by tests, including your case:
It inherits the docstring tracker's known limitation — a Scanner unit suite 30 → 33; CLT gate 15/15; runner and review-pr suites pass; ruff clean; diff coverage 100% (26 lines). |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-reviewed exact head b351b1db8ea0858ff7ac676033d440bec7af5bc9. The original executable multiplication bypass is fixed: the exact two-line control is now flagged, while the declared CLT 15/15, runner 16/16, scanner 33/33, review-pr, diff, and path checks pass. (ruff was not available in this review worktree; current-head CI ruff is green.)
[P1 mandatory-gate false positive] Block-comment state resets at every hunk, so a legitimate added JSDoc line is still scanned as code whenever the opening /* is outside the diff context. scripts/review-checks.py explicitly sets in_block = False on each @@, and _block_transition() cannot infer that an unchanged * ... context line is already inside a block. Exact-head repro against the real runner:
diff --git a/src/x.js b/src/x.js
--- a/src/x.js
+++ b/src/x.js
@@ -20,3 +20,4 @@
* earlier prose
+ * On macOS /usr/bin/python3 is a CLT stub.
*/
export function x() {}$ <that-diff> | bash scripts/review-checks.sh
review-checks: FAIL — hardcoded-paths:
src/x.js:21: hardcoded path (/usr/bin/python3): * On macOS /usr/bin/python3 is a CLT stub.
That is the normal unified-diff shape when editing a long JSDoc body more than three lines below its opener—the exact resolver documentation this PR says must remain exempt. An added opening line such as +/** ... /usr/bin/python3 ... is also flagged because only was_in_block, not a comment portion entered on the current line, is exempt.
Please make suppression reflect actual comment spans even when the opener is outside hunk context (or otherwise conservatively recognize a hunk-local body that closes with */) and add the long-JSDoc hunk above as a negative regression. Preserve the now-correct multiplication positive control.
Worst case is the mandatory review gate rejecting legitimate resolver documentation, recreating the broad-exemption/disable pressure this PR is intended to prevent.
Verdict: changes requested; not merge-ready on this head. The documented merge order (#2469, then #2473, then #2474) also remains in force, and current-head clean-install is still running.
|
Cold review at head The check fires correctly (verified, not assumed)Fed the scanner a two-line synthetic diff: One violation, naming the explicit path. So the The edge:
|
…tion @john-the-dev's P1: a legitimate JSDoc body was still scanned as code whenever the `/**` opener sat outside the hunk — which is the NORMAL diff shape, since unified diffs carry three context lines and doc bodies are usually longer. Two distinct causes, both reproduced on the prior head: src/x.js:21: hardcoded path (/usr/bin/python3): * On macOS /usr/bin/python3 … src/y.js:1: hardcoded path (/usr/bin/python3): /** helper for /usr/bin/python3 … 1. Block state reset to False at every `@@`. It is now INFERRED from the hunk's own content: a `*/` appearing before any `/*` can only happen if a comment was opened above the hunk. The inference is one-directional — no closer, or an opener first, means "assume code" — so a hunk containing neither delimiter (the multiplication bypass) is still scanned. 2. Suppression was line-level (`was_in_block`), so a path in the comment portion of the line that OPENS the comment was flagged: the state BEFORE that line is False. Comment regions are now masked by POSITION, blanked with spaces so column indices — and therefore the reported token — stay aligned with the real source. Code after a same-line `*/` is still scanned, so a comment cannot smuggle a path onto a code line. Four regressions added, including both repros and a control asserting the inference does not resurrect the bypass. Scanner unit suite 33 -> 37; ruff clean; diff coverage 97%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@john-the-dev — fixed on You identified two distinct causes and both were real: 1. Hunk reset. Block state is now inferred from the hunk's own content rather than reset to 2. Line-level suppression. Your second observation was the sharper one: for Same head, after: Four regressions added: both of your repros, the same-line-close control, and the bypass control. Scanner unit suite 33 → 37; CLT gate, runner and review-pr suites pass; ruff clean; diff coverage 97%. Merge order unchanged and still in force: #2469, then #2473, then this one. A note on the pattern, since this is the third finding of the same kind across these PRs. Each time my test asserted the shape of the thing rather than the thing — |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-reviewed exact head d326d7ea6aab1f2b8b9a0b0ea89331b73839df45. The prior long-JSDoc false positive and opening-line case are fixed, and the multiplication control remains correctly flagged. Reviewer-run CLT 15/15, runner 16/16, scanner 37/37, review-pr, diff, and production-diff path checks pass.
[P1 mandatory-gate bypass] _hunk_opens_in_block() treats the first */ anywhere in a hunk as proof that the hunk began inside a comment, including a closer token inside a valid string literal. That inference starts _mask_comments() in block state and can mask executable code before the string token. Exact-head repro against the real runner:
diff --git a/src/x.js b/src/x.js
--- a/src/x.js
+++ b/src/x.js
@@ -1,0 +1,1 @@
+const p = "/usr/bin/python3"; const closer = "*/";$ <that-diff> | bash scripts/review-checks.sh
review-checks: PASS (hardcoded-paths clean)
The JavaScript is valid, the explicit CLT-stub path is executable data, and the gate silently passes it. This is the reverse of the documented “string delimiter errs toward scanning” limitation: during initial hunk inference, a string closer errs toward suppression. Please make initial-state inference delimiter-aware enough that a quoted */ cannot establish block state, and pin the exact control above while retaining the now-correct long-JSDoc and multiplication regressions.
Sonichi’s current-head-adjacent documentation request also remains applicable: lesson 7 should explicitly state that the machine gate covers explicit /usr/bin/... tokens but not bare names resolved through PATH, so a passing scan is not mistaken for complete coverage of the CLT-stub class.
Worst case is a mandatory safety gate that can be bypassed by ordinary valid code formatting and creates false confidence around the exact stub path it claims to reject.
Verdict: changes requested; not merge-ready on this head. Current-head CI/coverage are still running, the branch merge-base remains 3e6c882b, and documented merge order remains #2469 → #2473 → #2474.
@john-the-dev's P1, and it is a bypass in the SUPPRESSION direction — the reverse of the string-literal caveat documented on _mask_comments. _hunk_opens_in_block() treated the first `*/` anywhere in a hunk as proof the hunk began inside a comment, so this valid one-liner scanned clean: const p = "/usr/bin/python3"; const closer = "*/"; The quoted closer established block state, and _mask_comments then blanked the executable code before it. Two paired guards, both needed: 1. Delimiter-aware inference — _blank_string_literals() blanks quoted spans (", ', ` with backslash escapes) before looking for delimiters, so a token inside a literal is data, not a comment close. 2. The closer must be the first thing on its line — the canonical JSDoc `*/` shape. A mid-line closer that survived blanking is not evidence a comment was opened ABOVE the hunk. Blanking is single-line, so a multi-line template literal could still hide a delimiter; guard 2 means it cannot establish block state on its own. Stated in the docstring rather than left implicit. All four controls now hold simultaneously, each pinned: quoted "*/" bypass -> FLAGGED continued multiplication -> FLAGGED long-JSDoc body (opener out of hunk) -> exempt path in an opening line's comment portion -> exempt Also addresses @sonichi's documentation request: lesson 7 now states the gate's LIMIT — it matches explicit /usr/bin/… tokens only, so a bare `git` or `python3` resolved through PATH is invisible to it and a green scan is not proof the class is absent. sonichi#2469 fixed exactly such a caller. Scanner unit suite 37 -> 44; ruff clean; diff coverage 100% (69 lines). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-reviewed exact head ef5c9d47aa6cea7515074832677c87d42c5e5f31. The prior single-line quoted-*/ bypass is fixed, Sonichi’s explicit-path-vs-PATH boundary is now documented in lesson 7, and reviewer-run CLT 15/15, runner 16/16, scanner 44/44, review-pr, diff, and path checks pass.
[P1 mandatory-gate bypass] A line-start */ inside a valid multi-line JavaScript template literal still establishes initial block-comment state and masks executable code earlier in the hunk. _blank_string_literals() is deliberately single-line and loses the opening backtick before _hunk_opens_in_block() examines the next line. Exact-head repro against the real runner:
diff --git a/src/x.js b/src/x.js
--- a/src/x.js
+++ b/src/x.js
@@ -1,0 +1,3 @@
+const p = "/usr/bin/python3"; const tpl = `
+*/
+`;$ <that-diff> | bash scripts/review-checks.sh
review-checks: PASS (hardcoded-paths clean)
That JavaScript is valid. The hidden closer on line 2 makes _hunk_opens_in_block() start the whole hunk in block state, so _mask_comments() suppresses the executable stub path on line 1. The current comment claims the paired line-start rule prevents a hidden token from establishing state on its own; this control disproves that claim.
Please carry string/template state across the hunk during initial-state inference, or choose a conservative inference that cannot begin suppression from a delimiter whose lexical role is unproven. Pin the exact multi-line template control while retaining the now-correct single-line string, long-JSDoc, opening-comment, and multiplication regressions.
Worst case remains a mandatory safety gate that silently passes the exact explicit CLT-stub path it claims to reject, creating false confidence.
Verdict: changes requested; not merge-ready on this head. Current-head clean-install/coverage are still running, the branch merge-base remains 3e6c882b, and documented merge order remains #2469 → #2473 → #2474.
Owner feedback: the point of this module is to route interpreter lookup through ONE place, not to withhold an interpreter that works. selectPython collapsed tiers 3 and 4 into `toolsInstalled() ? 'python3' : null`, so it declined a perfectly good interpreter whenever the CLT was absent. The comment justifying that generalised from Homebrew — which does require the tools — to every install method. Not true: a python.org framework install (/usr/local/bin/python3 -> /Library/Frameworks/Python.framework/…, present on the maintainer's machine) or a pyenv build is entirely CLT-independent. It was also inconsistent with select_git in src/git_binary.py (sonichi#2469), which already asks the right question: is what PATH gave me actually the stub? Only that one is gated. Now mirrors it: $SUTANDO_PY -> bundled runtime -> PATH python3 if NOT the stub -> the stub only when xcode-select -p succeeds -> null isSystemStubPath compares the resolved DIRECTORY rather than the exact stub path — that keeps the flagged literal out of this file (so this PR stays clear of sonichi#2474's gate and carries no merge-order dependency) and also covers a versioned sibling in the same system location. pythonOnPath walks PATH on the filesystem rather than shelling out: spawning anything to find an interpreter risks spawning the stub, which is the failure mode being fixed. Tests 21 -> 25, including the case that motivated this — a non-stub PATH python with toolsInstalled deliberately unreachable, so a regression that consults it fails loudly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@john-the-dev's P1, and it disproves a claim the previous commit's docstring made. That commit asserted the paired line-start rule meant "a hidden token cannot establish block state on its own". It can — via a multi-line template literal, whose opening backtick the single-line _blank_string_literals cannot carry to the next line: const p = "/usr/bin/python3"; const tpl = ` */ `; The hidden line-start closer started the whole hunk in block state and masked the executable stub path on line 1. Reproduced on the prior head: PASS, clean. Rather than lexing JS across lines, the inference now requires CORROBORATION: a closer only counts when some EARLIER line in the hunk already looks like comment body (`*` or `/*` leading). Real JSDoc always has that; the template literal does not, because its first line is ordinary code. That is also the conservative reading the review asked for — suppression can no longer begin from a delimiter whose lexical role is unproven. All five controls hold simultaneously, each pinned: */ in a multi-line template -> FLAGGED (this fix) */ in a single-line string -> FLAGGED continued multiplication -> FLAGGED long-JSDoc body, opener outside the hunk -> exempt path in an opening line's comment portion -> exempt Scanner unit suite 44 -> 45; ruff clean; diff coverage 100%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Blocking on exact head 744af65.
scripts/review-checks.py::_hunk_opens_in_block still infers initial block-comment state from lexical shapes without proving those lines are comments. The added corroboration can itself appear inside the same valid multiline template literal:
const p = "/usr/bin/python3"; const tpl = `
* template content
*/
`;Evidence on this head:
$ printf ... | node --check
VALID_JS
$ <the corresponding unified diff> | bash scripts/review-checks.sh
review-checks: PASS (hardcoded-paths clean)
The * template content line sets saw_comment_body; the following line-start */ returns true, so the hunk is treated as already inside a block and the executable path on line 1 is masked. This remains a suppression-direction bypass in the new required gate. Please carry multiline template/string lexical state through the hunk (or use a conservative inference that cannot treat unproven comment-like lines as evidence) and add this exact valid-JS regression.
The declared suites are green on this head (review-checks-clt-shims, review-checks-py 45/45, review-checks, review-pr, git diff --check, and the cumulative self-scan), but they do not cover this control. Stack order remains #2469 → #2473 → #2474; rebase/update after the parents land and rerun the full checks.
@john-the-dev's P1, and it defeats the corroboration rule from the previous commit by putting the EVIDENCE inside the template too: const p = "/usr/bin/python3"; const tpl = ` * template content */ `; `node --check` says VALID_JS. Line-at-a-time string blanking read `* template content` as comment-body evidence and `*/` as a closer, inferred the hunk had opened inside a comment, and masked the executable path on line 1. Reproduced on the prior head: PASS, clean. _blank_string_literals now returns (blanked, carry) and threads the carry through _hunk_opens_in_block. Only a BACKTICK carries: a template literal is the only JS string that survives a newline, so an unterminated ' or " is a single-line syntax error rather than state. With the backtick carried, lines 2-3 are blanked as string content and supply no evidence at all. This is the third bypass in this inference and the pattern is worth naming: each previous fix reasoned about how a line LOOKS (`* ` prefix, then line-start `*/`, then a corroborating `*` above). Lexical shape is not proof of lexical role. The fix that holds is tracking the actual string/comment spans. All six controls hold simultaneously, each pinned: evidence + closer inside a template -> FLAGGED (this fix) lone */ inside a template -> FLAGGED */ in a single-line string -> FLAGGED continued multiplication -> FLAGGED long-JSDoc body, opener out of hunk -> exempt path in an opening line's comment -> exempt Scanner unit suite 45 -> 50, including the carry-over states directly. Two earlier direct assertions on _blank_string_literals were comparing against the old string return and had to be indexed — three of them were passing for the wrong reason (tuple membership), which the new return value exposed. ruff clean; diff coverage 98%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@john-the-dev — fixed on This review sat unaddressed for two hours, and that was my error. I was comparing your review timestamps (UTC) against my commit times ( And you're right about the substance. Putting the evidence line inside the same template defeats the corroboration rule:
Six controls, all holding at once, each pinned:
Worth naming, since this is the third bypass in this one function. Each earlier fix reasoned about how a line looks — Scanner suite 45 → 50, including the carry states directly. Adding the tuple return also exposed that three existing assertions on
|
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-reviewed exact head .\n\nThe prior mandatory-gate bypass is fixed. The exact valid-JavaScript multiline-template control from my blocker still produces / rc 0 on that prior head, while this head reports the executable on line 1 and exits 1. The fix carries only backtick template state across hunk lines during initial block-comment inference, so the and inside the template no longer provide false comment evidence.\n\nReviewer-run exact-head evidence:\n\n- : 15/15\n- Python scanner suite: 50/50, including the new carry-state and exact bypass controls\n- review-checks runner: 16/16\n- review-pr suite: PASS\n- : clean\n- cumulative diff through the real : PASS\n- added-file/path scan: no machine/user-specific production paths\n\nThe prior long-JSDoc, opening-comment, multiplication, quoted-closer, prefix-family, and single-line-template controls remain green. Worst case for this mandatory gate was either silently admitting an explicit CLT-stub path or rejecting legitimate documentation/platform binaries; the current regression set discriminates both directions.\n\nVerdict: the current code layer is approved. The PR is not merge-ready yet: clean-install and diff coverage are still running, and the documented stack order remains #2469 → #2473 → #2474. After both parents land, refresh/rebase this child, rerun the full exact-head checks, and obtain two qualifying current-head maintainer approvals.
|
Correction to my exact-head approval above: shell quoting stripped the inline code spans from that review body. The formal verdict is unchanged; the intact evidence is:
The current code layer is approved, but the PR is not merge-ready: clean-install and diff coverage are still running, and stack order remains #2469 → #2473 → #2474. After both parents land, refresh/rebase, rerun exact-head checks, and obtain two qualifying current-head maintainer approvals. |
|
Re-reviewed at head I verified the load-bearing claim — the inode discrimination — on a real host, because it is the one thing in this PR that decides whether the gate blocks a legitimate tool: So the carve-out is correct in both directions: the five stub names share one inode and are blocked; Worth noting for cross-validation: this is the same stub inode I measured independently while checking #2477's Xcode-CLT claim earlier today (78 files at link-count 78). Two separate PRs resting on the same underlying fact, confirmed twice on the same host — so the premise under lesson 7 is solid, not folklore. I also like that the lesson states its own limit rather than overselling the gate: "the machine gate matches explicit Merge-readiness: approved at head, CI green (0 failing, 0 pending of 15). |
|
Heads-up on a collision with #2369 — not a blocker on this PR, but the merge order is now a decision rather than an accident. #2474 and #2369 are both rewriting the same review-gate machinery. They overlap on 4 of the 5 files each touches: File overlap alone would not be worth a comment — so I checked whether they touch the same regions, and they do: Both insert large blocks into the same file — ~168 lines here at line 52, ~480 there at line 30 — and then both edit the same later regions of Current state: this PR is approved at head with CI green and only needs a second approver (it is Flagging rather than gating: I would not hold this PR for it. Comment only — no approval from me; merge stays the owner's call. |
|
Cold review, and the finding is about #2369, not this diff: the two collide, and the collision is in the file that encodes both PRs' controls. Measured on #2369 ("contextual paired-allow so portable candidate lists pass the hardcoded-path check") restructures the same allow/detect logic this PR extends. They move in opposite directions on one predicate — this one tightens (CLT stub paths become catchable), #2369 loosens (declared I checked the obvious interaction hazard and it does not apply: #2369's allow is an explicit pair list matched by basename, not a general "two paths on a line" rule, so it cannot silently exempt a CLT stub. No blocker there. The real risk is the resolution, not the semantics. Not a change request on this PR — it's clean against main today, and john's latest review here is an approval. Two suggestions for whoever merges second:
Merge order is otherwise free — both are clean against main independently. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head afb4e06e812a31b170193a7196478d9327edd341 and the full REVIEW/scanner/test layer. The CLT-stub gate now distinguishes executable production paths from prose/fixtures, carries multiline JavaScript template state safely, and closes the mandatory-gate bypass without weakening existing host-path checks. Reviewer-run tests/review-checks-py.test.py passes, the patch clears its own path gate, and exact-head CI is green. Approved on this head. Resolve the known overlap/merge order with #2369, then refresh and rerun both scanners cumulatively before merge.
…m, meeting-tools) (#2475) * fix(ts): resolve python instead of hardcoding the Xcode-CLT stub Two TypeScript call sites spawned /usr/bin/python3 directly: skills/zoom/tools.ts:404 execSync(`/usr/bin/python3 -c "…`) Join-button click src/meeting-tools.ts:68 execFileSync('/usr/bin/python3', …) camera toggle On macOS that path is not python — it is the Xcode-CLT stub, one inode hardlinked across python3 / git / swift / swiftc / clang / gcc / make. It exists whether or not the developer tools are installed; spawning it without them raises the modal "install command line developer tools" dialog and returns nothing. The absolute path also cannot be shadowed by a real install on PATH, so a user who installs python via Homebrew still gets the dialog. Adds src/python-binary.ts — the TS twin of src/git_binary.py (#2469) and SutandoConfig.resolvePython (#2473), carrying the same cascade the shell side already uses for $PY: $SUTANDO_PY -> bundle-vendored runtime -> bare `python3` gated on xcode-select -p -> null Tier 3 is deliberately the BARE name, not an absolute path. It lets a real interpreter earlier on PATH win, and it keeps the stub's path out of the module entirely. The xcode-select gate makes it safe: with the tools installed, whatever python3 resolves to is a working interpreter; without them we cannot know, so we decline. (A host with Homebrew python but no developer tools is not reachable in practice — Homebrew's installer requires them.) requirePython() throws PythonUnavailableError. Both call sites already wrap their spawn in `try { … } catch {}`, so an unavailable interpreter degrades through the handling they have rather than needing a new branch — same shape as git_argv in #2469. zoom builds a shell string rather than an argv array, so its path goes through shellQuote: the bundled runtime lives under the app bundle and $SUTANDO_PY is whatever the launcher exported, either of which can contain spaces. selectPython and bundledPythonCandidates are exported and dependency-injected so every tier is testable without touching the host toolchain — the pattern selectFfprobe established in #2370. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(src-map): index the new python-binary module scripts/gen-src-map.py regenerated after adding src/python-binary.ts — the lint-src-map CI check refuses a docs/src-map.md that is stale vs src/, and it also fails tests/src-map.test.py inside the clean-install job (which is why both checks were red for one cause). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(ts): correct the bundled-runtime path; probe both call-site shapes Addresses both P1s from @john-the-dev's review. [P1 bundled-runtime path] bundledPythonCandidates appended the full relative path 'runtime/python/bin/python3' to BOTH dirname(execPath) and dirname(dirname(execPath)), doubling the segment: execPath /Applications/Sutando.app/Contents/Resources/runtime/bin/node emitted …/Resources/runtime/bin/runtime/python/bin/python3 …/Resources/runtime/runtime/python/bin/python3 wanted …/Resources/runtime/python/bin/python3 Neither exists, so on a packaged host WITH a vendored python but no $SUTANDO_PY the resolver fell through to the xcode-select tier and returned null without CLT — silently killing both GUI actions on exactly the install this module exists to serve. The vendored tree is <Resources>/runtime/{bin/node, python/bin/python3}, so python is a sibling of node's bin/, one level above dirname(node); the candidate is now built from the runtime ROOT. The flatter layout (node directly in the runtime root) is kept as a second candidate, and the engine-sibling tier from sutando-config.sh is unchanged. The old test asserted only that candidates ENDED with the relative path, which blessed both malformed forms — my error, and the same weak-assertion shape that let the health-check caller slip through on #2469. Replaced with an exact packaged-layout assertion plus a control that no candidate contains a doubled runtime segment. [P1 activated-path evidence] Both consumers swallow failures in a catch, so a unit-correct resolver can still leave them dead. Added executable probes that run the resolved interpreter through each call site's EXACT shape against a real on-disk packaged layout under a path containing a space: - meeting-tools shape: execFileSync(resolved, ['-c', …]) -> "meet-ok" - zoom shape: execSync(`${shellQuote(resolved)} -c …`) -> "zoom-ok" - the same zoom command WITHOUT shellQuote throws, proving the quoting is load-bearing rather than decorative - selectPython picks the vendored python from that layout with toolsInstalled false — the no-CLT case Tests 15 -> 21. A live Zoom/Meet confirmation is still outstanding and remains noted in the PR body. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(python-binary): make the probe fixture hermetic (no ambient python3) @john-the-dev's P1: the "no-CLT" fixture was `#!/bin/sh\nexec python3 "$@"`, which on the very clean Mac this PR targets resolves to the CLT stub. Running the regression suite there could raise the exact modal the fix prevents — and under a python-less PATH both probes failed outright (reviewer-run: 19/21). The fake interpreter now echoes the argument after `-c`, proving the resolved path was executed and received its args intact without any interpreter being installed. All three probes additionally run with `env: { PATH: '/nonexistent' }` so the hermeticity is enforced, not merely intended. Reviewer's negative control, this head: PATH=/nonexistent node --test --import tsx tests/python-binary.test.ts ℹ tests 21 ℹ pass 21 ℹ fail 0 (was: pass 19, fail 2) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(ts): use a real PATH interpreter without requiring developer tools Owner feedback: the point of this module is to route interpreter lookup through ONE place, not to withhold an interpreter that works. selectPython collapsed tiers 3 and 4 into `toolsInstalled() ? 'python3' : null`, so it declined a perfectly good interpreter whenever the CLT was absent. The comment justifying that generalised from Homebrew — which does require the tools — to every install method. Not true: a python.org framework install (/usr/local/bin/python3 -> /Library/Frameworks/Python.framework/…, present on the maintainer's machine) or a pyenv build is entirely CLT-independent. It was also inconsistent with select_git in src/git_binary.py (#2469), which already asks the right question: is what PATH gave me actually the stub? Only that one is gated. Now mirrors it: $SUTANDO_PY -> bundled runtime -> PATH python3 if NOT the stub -> the stub only when xcode-select -p succeeds -> null isSystemStubPath compares the resolved DIRECTORY rather than the exact stub path — that keeps the flagged literal out of this file (so this PR stays clear of #2474's gate and carries no merge-order dependency) and also covers a versioned sibling in the same system location. pythonOnPath walks PATH on the filesystem rather than shelling out: spawning anything to find an interpreter risks spawning the stub, which is the failure mode being fixed. Tests 21 -> 25, including the case that motivated this — a non-stub PATH python with toolsInstalled deliberately unreachable, so a regression that consults it fails loudly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 612e2edfb21b9ff4fa9c1546b082bad6e42f481b after its current-main merge refresh.
No blocking findings. Range-diff shows the review-check topic commit is unchanged. The exact-token CLT stub scan still flags only the executable tool paths it intends to catch, while comments, docs/tests, and prefix-family siblings remain exempt.
Reviewer-run exact-head checks:
bash tests/review-checks-clt-shims.test.sh— 15/15python3 tests/review-checks-py.test.py— 50/50git diff --check origin/main...HEADgit diff origin/main...HEAD | bash scripts/review-checks.sh
Hosted CI and CLA are green. Worst-case disruption is a false-positive merge block; the paired exact-token negative controls cover that boundary. Code-ready and approved.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved at exact head f71dc622. The current-main refresh preserves all eight reviewed scanner commits exactly; there is no new main/topic file overlap, the full diff and hardcoded-path gate pass, and all hosted checks are green. The failure-mode/false-positive surface is unchanged from the prior exact-token and comment-state verification. Code is ready; normal behind-main merge mechanics remain.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head f71dc6223ed42708b21c9e2b304bacf14e741a4f after the #2540 main merge refresh.
No blocking findings. Range-diff preserves the review-check hardening commits exactly; this head only merges current main. The Xcode-CLT stub gate still matches exact executable tokens, keeps prose/test exemptions narrow, and preserves the scanner's comment/template handling.
Focused local checks passed:
bash tests/review-checks-clt-shims.test.shpython3 tests/review-checks-py.test.py
Hosted checks are green.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head f9b13a24a48069f1cbe13a2aa95f037210d33913.
No blocking findings on this refreshed head. The Xcode CLT stub gate covers the exact stub paths without catching sibling tools such as swift-inspect, swift-frontend, or makeinfo. Hosted tsc + tests and coverage were still in progress on my last poll, so this approval is scoped to the code review and focused checks for this exact head.
Focused checks I ran:
bash tests/review-checks-clt-shims.test.shpython3 tests/review-checks-py.test.pygit diff --check origin/main...HEAD
Reviewed by Qingyun's Personal Codex.
|
@cla-assistant check |
…defining site sonichi#2474 landed /usr/bin/python3 as a `flag_exact` token after this branch was cut, and the first thing it caught was this PR: src/Sutando/SutandoConfig.swift:309: hardcoded path (/usr/bin/python3): static let systemPython = "/usr/bin/python3" review-checks: 1 violation(s). Not a false positive worth an allow entry: REVIEW.md `allow` is matched per TOKEN, not per file, so allowing /usr/bin/python3 would switch lesson 7 off for that token repo-wide -- the gate would stop catching the exec sites it exists for. Compose it from a systemBin constant instead, matching how the landed src/python-binary.ts keeps the same literal out of its source. The value is unchanged; the resolver still returns it only when developerToolsInstalled(). Composition creates a new hole though: the existing assertion only checked that the resolver agrees with the constant, so a typo in either half stayed green. Verified -- sabotaging systemBin to /usr/local/bin passes the old assertion. Added a test that pins the concrete path (tests/ is scanner-exempt, so the literal lives where it is allowed): FAIL: test_system_constant_is_exactly_the_stub_path AssertionError: /usr/local/bin/python3 != /usr/bin/python3 10/10 after restoring.
…defining site sonichi#2474 landed /usr/bin/python3 as a `flag_exact` token after this branch was cut, and the first thing it caught was this PR: src/Sutando/SutandoConfig.swift:309: hardcoded path (/usr/bin/python3): static let systemPython = "/usr/bin/python3" review-checks: 1 violation(s). Not a false positive worth an allow entry: REVIEW.md `allow` is matched per TOKEN, not per file, so allowing /usr/bin/python3 would switch lesson 7 off for that token repo-wide -- the gate would stop catching the exec sites it exists for. Compose it from a systemBin constant instead, matching how the landed src/python-binary.ts keeps the same literal out of its source. The value is unchanged; the resolver still returns it only when developerToolsInstalled(). Composition creates a new hole though: the existing assertion only checked that the resolver agrees with the constant, so a typo in either half stayed green. Verified -- sabotaging systemBin to /usr/local/bin passes the old assertion. Added a test that pins the concrete path (tests/ is scanner-exempt, so the literal lives where it is allowed): FAIL: test_system_constant_is_exactly_the_stub_path AssertionError: /usr/local/bin/python3 != /usr/bin/python3 10/10 after restoring.
…l dialog (from-source installs) (#2473) * fix(app): resolve python via SUTANDO_PY / bundled runtime, never the CLT stub Sutando.app runs src/health-check.py from a 60-second Timer and picked its interpreter like this: let homebrewPython = "/opt/homebrew/opt/python@3.11/libexec/bin/python3" let pythonPath = FileManager.default.fileExists(atPath: homebrewPython) ? homebrewPython : "/usr/bin/env" Both halves are wrong on a current machine. The hardcoded formula path is stale: python@3.11 is not installed on the maintainer's own dev machine (which has @3.10, @3.12, @3.14), so effectively every host already takes the fallback branch. And the fallback resolves to /usr/bin/python3, which on macOS is the Xcode-CLT stub, not python — one inode hardlinked across python3 / git / swiftc / clang / gcc / make. Without developer tools it raises the modal "install command line developer tools" dialog and returns nothing. Because the caller is a Timer, that dialog reappeared indefinitely. Reproduced on a clean macOS VM installing a bundled Sutando; dev machines are masked only because their tools ARE installed, which makes /usr/bin/python3 a real interpreter. Note the app sets no custom environment on this Process, so it inherits the GUI session's minimal PATH — PATH-based resolution cannot be relied on here. Adds SutandoConfig.resolvePython, the cascade the shell side already uses (scripts/sutando-config.sh, src/agent/claude/cli/start-cli.sh): $SUTANDO_PY -> <repoRoot>/../runtime/python/bin/python3 -> /usr/bin/python3 only when xcode-select -p reports installed tools -> nil nil is the point: the caller now logs and skips instead of prompting. A health check must never be able to raise a modal system dialog. Dependencies are injected with defaults so every tier is testable without mutating the host toolchain. tests/sutando-config-python-resolution.test.py compiles SutandoConfig.swift against a probe and drives all six branches, plus source-tied guards that run on hosts without swiftc. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(app-python): compose the stub path so lesson 7 does not flag the defining site #2474 landed /usr/bin/python3 as a `flag_exact` token after this branch was cut, and the first thing it caught was this PR: src/Sutando/SutandoConfig.swift:309: hardcoded path (/usr/bin/python3): static let systemPython = "/usr/bin/python3" review-checks: 1 violation(s). Not a false positive worth an allow entry: REVIEW.md `allow` is matched per TOKEN, not per file, so allowing /usr/bin/python3 would switch lesson 7 off for that token repo-wide -- the gate would stop catching the exec sites it exists for. Compose it from a systemBin constant instead, matching how the landed src/python-binary.ts keeps the same literal out of its source. The value is unchanged; the resolver still returns it only when developerToolsInstalled(). Composition creates a new hole though: the existing assertion only checked that the resolver agrees with the constant, so a typo in either half stayed green. Verified -- sabotaging systemBin to /usr/local/bin passes the old assertion. Added a test that pins the concrete path (tests/ is scanner-exempt, so the literal lives where it is allowed): FAIL: test_system_constant_is_exactly_the_stub_path AssertionError: /usr/local/bin/python3 != /usr/bin/python3 10/10 after restoring. * ci: retrigger the clean-install job cancelled on this head `tsc + tests (clean install)` is recorded as CANCELLED on 552004d, not failed -- the run started 09:35:44 against the previous head and was killed at 09:45:48 when this head was pushed, so the current head has no result for that job. Every other check on 552004d is green (15 success, 0 failure). `gh run rerun` is admin-only on this repo, so an empty commit is the only retrigger available to me. No file changes: `git diff 552004d..HEAD` is empty, so the reviewed tree is byte-identical. * docs(app): trim added comments to the two-line rule The comments added here predate the code-comment policy now on main: multi-paragraph narration, an outdated REVIEW.md lesson label, and incident history. Condensed to the invariant each protects — probe before spawning system python, skip when nothing is runnable. History stays in the PR body. No behaviour change; the app still builds and the resolver tests pass. * docs(app): condense resolvePython doc comment to the two-line rule Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
Important
Merge order: land this AFTER #2469 and #2473.
Those two PRs add the resolver constants that name the stub in order to guard
against it (
SYSTEM_GIT = "/usr/bin/git",systemPython = "/usr/bin/python3").This gate flags them as added lines, so merging it first turns both of them
red. Once they land, those lines are no longer added in any diff and the gate
is clean — both directions verified, evidence in the "Stacked PR?" section at
the bottom.
What changed, and why?
Two live bugs were just fixed where code invoked a developer tool at an absolute
/usr/bin/path (#2469, #2473). This adds the lesson and the machine check so the class does not come back.On macOS
/usr/bin/{git,python3,swift,swiftc,clang,gcc,make}are not those tools. They are one inode, hardlinked 78 ways, holding the Xcode-CLT stub:The file exists whether or not the tools are installed. Invoking it without them raises a modal "install command line developer tools" dialog and returns nothing.
Why this is its own lesson and not a case of lesson 6
Lesson 6 covers machine- or user-specific path literals. Three things make this different, and all three are why it slipped past review originally:
/usr/bin/gitexists on every Mac, so it reads as the portable choice. Lesson 6's framing does not catch it, and theflaglist did not contain it.test -x,command -v,shutil.which,FileManager.fileExistsall succeed with no toolchain installed — so the obvious guard is not a guard. fix(app): the Swift app's python fallback raises the Xcode CLT install dialog (from-source installs) #2473 contained acommand -v swiftcheck that fell straight through for exactly this reason.Plus the failure mode is unusually bad on polling paths: the caller in #2473 was a 60-second Timer, so the dialog returned every minute indefinitely.
What should reviewers look at first?
REVIEW.mdlesson 7 (new; the verdict lesson renumbers 7 → 8). Prose only — no behavior.checks:block — the six added patterns. Note/usr/bin/swiftcovers/usr/bin/swiftcby prefix; the test asserts that so the implicit coverage stays honest.env,pgrep,lsof,osascript,open,id,pmset,xcode-select. Those are real binaries with their own inodes (link count 1), andmain.swiftaddresses several absolutely on purpose. Flagging them would make the gate unusable and it would be turned off.What user behavior / bug does this prove?
It does not change runtime behavior — it gates the diff. The check catches the exact shapes that shipped:
While designing it I ran the patterns over the current tree and found two more live instances that neither #2469 nor #2473 covers:
Both spawn the stub on a clean Mac. Fixed separately in the TypeScript follow-up (new
src/python-binary.ts), which is what surfaced the JSDoc gap addressed in the second commit below. That PR passes this gate cleanly, so it carries no merge-order dependency on this one.Second commit: JSDoc continuation lines (
ade6f5f)Found while wiring the TypeScript resolver against this gate. The scanner skipped
#and//lines but had no equivalent for block-comment continuation lines, so prose inside a/** … */block was scanned as code:Pre-existing — a JSDoc line naming
/Users/…was always flagged — but it becomes acute with the stub paths added here. The modules that resolve these tools necessarily document the hazard in JSDoc, so every such module would trip its own gate and the check would get switched off within a week.Now skips
* text, a bare*, and*/. Deliberately does not skip:* text(JSDoc body)*/, bare*/* inline */ const x = "…"*gen() { … }(generator method)Verified against all four:
Lines 1–4 (the JSDoc block) are correctly silent. Three cases added to
tests/review-checks-py.test.py(26 checks, all pass); diff coverage on the change is 100% (2 lines).Feature / documentation consistency
REVIEW.mdis the canonical document for review criteria, and it is what this PR updates. PerCLAUDE.md— "Review criteria live in REVIEW.md (single source of truth). Don't duplicate the 7 lessons here… Adding or editing a lesson is a PR to REVIEW.md only" —CLAUDE.mdandAGENTS.mdare deliberately not touched.AGENTS.mdis generated fromCLAUDE.mdbyscripts/agents-md-sync.shin any case, so it is never hand-edited.No
docs/catalog.jsonentry covers the review guide.Before / after evidence
Test added:
tests/review-checks-clt-shims.test.sh(auto-discovered by CI'sfind tests -name '*.test.sh'). It runs the realscripts/review-checks.shagainst the realREVIEW.md, so it covers guide parsing → flag list → scanner, not the scanner in isolation.BEFORE — test present,
REVIEW.mdrestored toorigin/main:AFTER — at HEAD:
The two
oklines that pass in both states are the negative cases — they must not regress when the patterns are added, which is the actual risk of a change like this.The false-positive risk, checked explicitly. This repo discusses these paths constantly in prose (
src/dashboard.py:77,src/telegram-bridge.py:9,src/agent-api.py:299,src/install-*-launchd.sh, …). A check that flagged comments would be disabled within a week. The scanner already skips#,//and docstring lines (scripts/review-checks.py), and the test pins it:Tests run
bash tests/review-checks-clt-shims.test.shpython3 tests/review-checks-py.test.pydiff-cover --compare-branch=origin/main --fail-under=95review-checks.py)bash tests/review-checks.test.shbash tests/review-pr.test.shshellcheck tests/review-checks-clt-shims.test.shThe three neighbouring suites matter here:
review-checks.test.shandreview-pr.test.shboth run against the realREVIEW.md, so a malformedchecks:block would break them.Hardcoded-path check
Adds no path literals to production code. The new patterns live in
REVIEW.md(a.mdfile, which the scanner skips bySKIP) and the test fixtures live undertests/, also skipped. The gate passes over this PR's own diff.Stacked PR? — yes, merge order matters
Land this after #2469 and #2473.
Those PRs introduce the two resolver constants that name the stub in order to guard against it, and the new gate flags them as added lines:
Once they merge, those lines are no longer added in any diff and the gate is clean — verified against a subsequent-PR shape:
I chose merge ordering over a scoped allow deliberately. A token-level allow for
/usr/bin/gitwould match at token start and defeat the check entirely, and a whole-file exemption for the resolver modules would violateCLAUDE.md's "keep fixture exclusions token-specific so they cannot hide another real path on the same line". A contextual paired-allow is what #2369 is building; if a future PR legitimately needs to name a stub path in code, that is the mechanism it should use rather than wideningallowhere.If a reviewer would rather not carry a merge-order dependency, the alternative is to land this first and have #2469 / #2473 add a scoped allow — but that means shipping the exemption mechanism before the thing it exempts, which seems worse.