feat(doctor): ask agy which model it runs; validate permissions.allow - #56
Conversation
0.22.4 warned when agy was below 1.1.10, where --model was ignored in headless -p. That is the best a version comparison can do, and a version comparison is a proxy: right about the releases we know about, silent about every other way the flag can fail to land. agy 1.1.11 answers the read-only slash commands in print mode without starting an agent turn, so doctor requests a tier model with -p /model and reports which one comes back. Zero tokens, no quota, no conversation left behind. Gated at 1.1.11: below it the command falls through as prompt text and the model answers as though it had run, so probing would spend a real turn and then trust its own invention. The stub agy logs every invocation, so "never probes below 1.1.11" is asserted against the log rather than described in a comment. Separately, doctor now validates permissions.allow. The plugin recommends such a rule in eight places as the narrow alternative to --yolo, and ships a placeholder, write_file(<dir>). A rule agy cannot parse is silent in both directions and fails opposite ways either side of 1.1.11: from 1.1.11 it matches nothing, so the grant is absent and the write is soft-denied with the rule sitting visibly in the file; before 1.1.11 an entry tokenizing to zero command words matched EVERY command and auto-approved anything the agent ran, broader than the --yolo it was chosen instead of. doctor names the entry, the reason, and the consequence for the version in front of it. The zero-command-word test follows upstream's own examples, plus our own placeholder. Rules it cannot judge are left alone: unbalanced quotes are agy's parser's business, write_file(...) is a different matcher from command(...). A false positive sends someone to edit a rule that was always fine, so the well-formed case is pinned as hard as the broken ones. Verified against 1.1.11 with no change needed: all three tiers resolve, the exit-14 classifier still fires, and the reworded model-loading errors leave exit 15 intact (its message still carries "auto-denied" and "permissions.allow" verbatim). /usage and /quota were considered for doctor and dropped: they return zero bytes on this account type, which 1.1.11's notes explain rather than a bug. 10 new assertions, all shown capable of failing. 6 fail against the unfixed doctor; the other 4 are negative properties of the new code and were confirmed by mutation. That found a real defect in one of them: the two verdicts read "takes effect" and "does NOT take effect", so a pattern copied from one branch silently stopped guarding the other, and the empty-answer test passed with the branch it guards removed. 224 -> 234 tests.
Quorum reviewReviewing This comment will be replaced by the result. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| t = e.strip() | ||
| if not t: | ||
| bad.append(("(empty string)", "empty entry")); continue | ||
| if "<" in t or ">" in t: |
There was a problem hiding this comment.
Minor false-positive risk: "<" in t or ">" in t flags any occurrence of these characters anywhere in the entry as an "unsubstituted placeholder", not just <...> placeholder syntax. A well-formed command(...) rule that happens to use a shell redirect or comparison (e.g. command(sort < file.txt)) would be misreported as a placeholder rather than left alone like other well-formed rules. Given the "well-formed case is pinned as hard as the broken ones" bar this PR sets for itself, this one path isn't covered by a test — worth a quick check on whether that shape is realistic for permissions.allow entries, and if so a placeholder-specific pattern (e.g. requiring <...> to actually bracket something) would avoid it.
|
Reviewed the diff (doctor.sh, tests, README/TROUBLESHOOTING/SKILL.md, CHANGELOG). This PR does not touch hooks/validate-delegate-bash.sh, agy-delegate.sh, or agy-job.sh, so the security gate and the exit-code/signal contract are unaffected. Shell logic (scripts/doctor.sh): traced the new -p /model probe gate, the bidirectional slug/display-name match, and bad_allow_rules() Python. All check out, including the version-gate arithmetic (ver_lt on 1.1.10/1.1.11) and the empty-answer no-verdict branch the PR says was mutation-tested. Left two review comments inline:
Cost discipline: the -p /model probe reads one line via head -1 | cut -f1 - no bulk output reaches the conductor. Good. Verified claims: the exit 15 still contains auto-denied and permissions.allow verbatim claim checks out against the classifier in agy-delegate.sh line 494 (unchanged by this PR). Version bump is consistent between .claude-plugin/plugin.json and skills/antigravity/SKILL.md (both 0.22.5). No test-passes-for-wrong-reason or silent-no-op issues found in the new assertions - traced through the stub agy scripts and confirmed they exercise the paths they claim to (probe not run below 1.1.11, probe run at 1.1.11, empty-answer branch, well-formed-rules produce no warning, and the version-dependent consequence message). |
![quorum-code-review[bot]](https://avatars.githubusercontent.com/in/4452711?s=60&v=4){kind=small}
…finding Both reviewers on #56 caught the first cut printing the pre-1.1.11 "matches EVERY command and auto-approves anything" consequence for every flagged entry. That history belongs to one class only: a command(...) rule naming no command, which is what agy 1.1.11 fixed. A mistyped write_file(<dir>) never had it. Telling someone holding one that their config auto-approves everything is a security claim about a situation they are not in. bad_allow_rules now emits a class alongside the reason. Every class still reports that the grant is absent, since that is true of all of them; only zerowords adds the version-dependent sentence. Also from review: the placeholder test now matches the <...> SHAPE rather than a bare angle bracket, so command(echo hi > /tmp/f) is not misread as a template nobody filled in. And the write-grant block in POC-PLAYBOOK.md was a verbatim copy of the README one, so the placeholder warning landed in three files and missed the fourth. Updated, framed for that document: either failure mode makes an arm's write behaviour a second uncontrolled variable, which is what its own section 5 says a cost claim cannot carry. 3 more assertions, each confirmed by mutation. The first mutation reproduces the reviewers' finding exactly. 234 -> 237.
ReviewFocused on Traced through both new checks and found them sound:
Not independently verifiable here: the "Verified against agy 1.1.11" changelog claims (tier resolution, exit-14 classifier, exit-15 message stability) and the shape of the real Minor, not blocking: a No high-confidence bugs found. Nice work confining the security-claim wording to the class it's actually true for. |
…e test construct Review finding: <[^<>]*> false-positives on a rule with TWO literal redirects, command(sort < in > out), where everything between the brackets is a filename. A placeholder is now the shape AND placeholder-shaped content — one unbroken token, no whitespace — which still catches <dir> and <path/to/repo>. The bigger find came from chasing the flake that appeared while testing that. `printf '%s' "$x" | grep -q PAT` is the shape fixed in 0.21.1 and explained in a comment in this same file: grep -q exits at the first match, closes the pipe, the writer takes SIGPIPE (141), and pipefail marks the pipeline failed — the assertion reads "not found" while the text is right there. The window is whatever the writer still has to emit AFTER the matched line, so appending output below a match revives it. This release appended output below a match: the 1.1.9 version-gate assertion failed 1-4 times in 8 concurrent runs on this branch and never on master. Diagnosed rather than guessed. Instrumenting the test made it vanish, so the trace went inside doctor: mktemp never failed and AGY_VER was never empty, which left only the assertion misreading output that had in fact been printed. All 25 occurrences now use a case-based has(): same test, no second process, no pipe. The five `sed ... | grep -q` source scans give grep a process substitution, so sed's death is not pipefail's business. Those five failed SILENTLY — a false negative there reads as "the property holds" — and they guard the delegate wrapper's --help probe, agy_guard's pipe rule, and the sort -V dependency. 24 consecutive clean runs at the concurrency that reproduced it. Each converted source scan re-checked by mutation. 237 -> 239.
| echo "FAIL: \$out is empty at the envelope check — the assertion below proves nothing"; FAIL=$((FAIL+1)); | ||
| else echo "ok: \$out still holds the json_ok reply at the envelope check"; PASS=$((PASS+1)); fi | ||
| if printf '%s' "$out" | grep -q 'conversation_id'; then | ||
| if has 'conversation_id' "$out"; then |
There was a problem hiding this comment.
has() is called before it's defined — this assertion always "passes", for the wrong reason.
has() is defined at line 692, but this call executes earlier at top-level (bash doesn't hoist function definitions). At line 211, has is not yet a known command, so this errors with has: command not found (exit 127) and the if takes the else branch unconditionally — printing "ok: json envelope does not leak to stdout" regardless of whether conversation_id is actually present in $out.
This is exactly the "test that passes for the wrong reason" class this PR's own CHANGELOG entry calls out (negative assertion that passes for free while PASS still increments). It defeats the json-envelope-leak regression check this line exists to guard.
Same bug at line 368 (has 'NOT write to your workspace without it' "$out"), which similarly always reports "ok" regardless of the actual warning text.
Fix: move the has() definition (line 692) above its first use, e.g. near the top of the file next to the other helpers (check, etc.).
| out=$(STUB_MODE=args "$DELEGATE" "implement the parser module" 2>&1) | ||
| check "write warning names the permissions.allow route" 0 0 "permissions.allow" "$out" | ||
| if printf '%s' "$out" | grep -q 'NOT write to your workspace without it'; then | ||
| if has 'NOT write to your workspace without it' "$out"; then |
There was a problem hiding this comment.
Same bug as line 211: has() isn't defined until line 692, so this call fails with has: command not found and unconditionally takes the else branch — always printing "ok: warning no longer claims --yolo is the only write grant", even if the old "NOT write to your workspace without it" text came back. See the comment on line 211 for the fix (move the has() definition above its first use).
|
Reviewed the diff (doctor.sh One high-confidence bug, left as inline comments: in Everything else checked out:
I did not independently verify the claims about real |
Both reviewers, independently: has() was defined beside the doctor tests at line 692, and two of the conversions sat at 211 and 368. bash does not hoist, so those two calls were `command not found`, exit 127, else taken unconditionally, "ok" printed regardless of what was being checked. The previous commit's own message claimed to be removing exactly this class of defect. One of the two was the "json envelope leaked to stdout" assertion, which had already been voided once before by a different accident. Mutation confirms it is live again: leaking the envelope now fails it. Helpers moved to the top of the file. tests/check-helper-order.py runs first and fails the suite when any function is called above its definition, naming both lines. bash 4's command_not_found_handle was tried for this and removed. macOS ships bash 3.2, where merely defining it is a silent no-op — a guard that reads as protection and provides none, which is the defect this whole PR is about. Verified by putting the original bug back: the static check catches it, the handler did not. 239 -> 240.
| # A CALL is the bare name in command position. Excluded: the definition itself | ||
| # (`name()`), any prose about it (a comment line), and any other name that merely | ||
| # ends with these characters. | ||
| call = re.compile( |
There was a problem hiding this comment.
🟡 Call-site regex misses common bash contexts and ignores quoting
The new hoisting guard scans raw text for a fixed set of anchors that must immediately precede a function name to count as a "call": start-of-line, ;, &, |, if , then , else , while , until , do , !, $(. This has two concrete gaps that undermine the exact guarantee the tool exists to provide (catching a helper called above its own definition, which the CHANGELOG for this PR says caused command not found/exit 127/silently-taken else branches):
- False negative:
elifis not covered, because\bif\scannot match insideelif(no word boundary betweenelandif). A call written aselif has "$x"; thenabovehas() { ... }would not be detected. Worse, since the outer loop doesbreakon the first regex match found scanning top-to-bottom, if a later, correctly-anchored call to the same function exists further down (even after the definition), the scan stops there and the earlier unmatchedelif-based call is never reported at all. - False positive: the script only excludes whole-line comments (
line.lstrip().startswith('#')); it has no awareness of shell string literals. A test description such ascheck "...; has been broken" ...sitting abovehas()'s definition would trip the;-anchor purely from the string's punctuation, flagging a bug that doesn't exist.
Right now the file happens not to trigger either case, so the suite passes, but the tool is materially weaker than its stated purpose ("the check is static ... and works on any shell" / "it names the call site and the definition line"), and a future ordinary edit (e.g. adding an elif has ... branch above a helper) would silently defeat the guard.
Second opinion from gemini-3.6-flash
claude-sonnet-5 raised this. gemini-3.6-flash was then asked to judge it — without being shown the reasoning, the severity, or who reported it, so that it would assess the code rather than agree with a colleague. Its answer:
The regular expression used to match function call sites misses several standard bash contexts (such as
elif, backticks`fn`, subshells(fn), and group blocks{ fn; }) and ignores shell quoting, causing strings likeecho "foo; fn"to be incorrectly identified as function calls.
reliability · id e0b3f364669cd497
| ways — from 1.1.11 it matches nothing (the grant is absent, exit 15 with the rule | ||
| visibly present in the file), and before 1.1.11 an entry tokenizing to zero command | ||
| words (`command(time)`, comment-only, `()`) matched EVERY command. If a user reports a | ||
| rule that "should" work, have them run `agy-doctor` before changing anything else. |
There was a problem hiding this comment.
This caveat (placeholder must be substituted, malformed rule fails silently in opposite ways either side of 1.1.11, run agy-doctor) lands here, in README.md, POC-PLAYBOOK.md and TROUBLESHOOTING.md — but not in the two places that arguably matter most:
scripts/agy-delegate.sh:497, the exit-15 message shown at the moment a write is soft-denied (the exact symptom this whole feature diagnoses). It still just says "add awrite_file(<dir>)rule" with no mention that an already-present rule could be the cause, and no pointer toagy-doctor. The parallel prose indocs/TROUBLESHOOTING.mdfor this exact same exit-15 case was updated to say "suspect the rule before suspecting agy" and to runagy-doctor— the runtime string wasn't kept in sync.agents/antigravity-delegate.md:88andcommands/delegate.md:15, the operational instructions actually consulted when constructing a delegation, still show the barewrite_file(<dir>)recommendation with no placeholder/version caveat at all.
Given "doc drift across more than the file in the diff" is a recurring failure class here, worth deciding whether these should get at least a short pointer (e.g. "malformed rule? run agy-doctor") rather than silently diverging from the docs that were updated.
|
Reviewed the diff (doctor.sh model-probe + permissions.allow validation, run-tests.sh SIGPIPE/ordering fixes, check-helper-order.py, docs). CI/shellcheck/plugin-validate aren't repeated here. Shell logic — looks correct:
Doc drift (left as an inline comment on
Version/count claims — checked: Nothing else stood out — the mutation-testing discipline in the commit history is visible in the diff (e.g. the |
… rule Review: the checker's regex matched command position with one big alternation of contexts and missed elif, a call inside a case branch, and a brace group. Confirmed against the old pattern — all three returned False. A checker that misses a shape is a false negative, which is the defect it exists to prevent, so it now splits the line into command segments and compares each segment's first word. No list of contexts to keep complete. The checker has its own tests now, including a no-false-positive case for a mention inside a string. It promptly flagged its own test data, which is fair: a fixture written across real lines is not distinguishable from code. Fixtures are single-line. Also review: the placeholder caveat landed in four documents and missed the one place someone actually lands when a write is soft-denied. The exit-15 message recommended permissions.allow without allowing that the rule itself may be the problem, which is exactly what this release added a check for. It now says to run agy-doctor when a rule is already present. Same caveat in agents/antigravity-delegate.md and commands/delegate.md, the two files consulted while building a delegation call. 240 -> 245.
|
|
||
| # Anything that ends one command and begins another. `(` and `)` cover subshells and | ||
| # the `pattern)` opening a case branch; backtick and `$(` cover substitution. | ||
| SEGMENT = re.compile(r"\|\||&&|\$\(|[;&|()`{}]") |
There was a problem hiding this comment.
🟡 Segment splitter treats bare {/} as delimiters, mis-parsing ${name}
The new SEGMENT regex splits on the single characters { and } unconditionally, without checking whether they are preceded by $ (i.e. part of a parameter expansion like ${name} rather than a brace group). For a line containing ${hlp} where hlp is the name of a helper function defined later in the file, the split produces the isolated segment hlp, which calls() then reports as a call in command position — even though it is only a variable reference inside ${...}. This is a false positive of exactly the kind this rewrite was supposed to eliminate (the PR explicitly added a 'no false positive' test for a quoted mention, but missed the ${...} shape). It doesn't currently fire because no such variable name collides with a helper name in tests/run-tests.sh, but the very next edit that introduces ${name} matching an existing helper name before its definition will make the CI helper-order check fail spuriously.
Second opinion from gemini-3.6-flash
claude-sonnet-5 raised this. gemini-3.6-flash was then asked to judge it — without being shown the reasoning, the severity, or who reported it, so that it would assess the code rather than agree with a colleague. Its answer:
In
tests/check-helper-order.py, line 20 includes{and}inside the character set[;&|(){}]ofSEGMENT. WhenSEGMENT.split(line)runs on a line containing a parameter expansion like${fn}(e.g.echo ${fn}),re.splitmatches{and}as segment boundaries and splits the line into["echo $", "fn", ""].calls(line, "fn")evaluates the segment"fn", finds"fn"in command position as the first word of that segment, and returnsTrue`, mis-parsing the parameter expansion as a function call.
correctness · id c003c638bf5c4211
| `--yolo`, which approves every tool. You cannot see that file, so `--yolo` stays the | ||
| `--yolo`, which approves every tool. If they say a rule is in place and the write is | ||
| still soft-denied, have them run `agy-doctor` before anything else: an entry agy cannot | ||
| parse grants nothing, and before agy 1.1.11 it granted everything. You cannot see that file, so `--yolo` stays the |
There was a problem hiding this comment.
This restates the pre-1.1.11 "granted everything" consequence for any unparseable permissions.allow entry, not just the command(...)-naming-no-command (zerowords) class. scripts/doctor.sh's bad_allow_rules deliberately does not do this — per its own comment and the commit history on this PR ("Both reviewers on #56 caught the first cut printing the pre-1.1.11 … consequence for every flagged entry … A mistyped write_file(<dir>) never had it"), only a command(...) rule tokenizing to zero words carries that history. write_file(<dir>) — the exact placeholder shipped throughout the docs — is a different matcher and was never shown to match-everything pre-1.1.11.
README.md, docs/TROUBLESHOOTING.md, docs/POC-PLAYBOOK.md and skills/antigravity/SKILL.md all scope this correctly ("an entry that tokenized to zero command words … matched every command"), but this file still makes the generic claim. Since this is the guidance the delegate subagent itself reads before telling a user their rule "granted everything," it reintroduces the exact overreach the linked commit says was fixed.
Same issue at scripts/agy-delegate.sh:497 (the exit-15 message users actually see).
| shopt -u nocasematch | ||
| [ -s "$ERR" ] && cat "$ERR" >&2 | ||
| echo "agy-delegate: agy soft-denied a tool that needs permission (headless can't prompt) — no work was done. For a FILE WRITE, the narrower fix is a permissions.allow rule covering the target in ~/.gemini/antigravity-cli/settings.json — write_file(<dir>) matches recursively beneath <dir> — which needs no flag; --yolo also works but auto-approves ALL tools. Other tools (web / Vertex AI Search / terminal) need --yolo unless a rule covers them. agy's own message above names the specific permission it wanted. (agy >= 1.1.3)" >&2 | ||
| echo "agy-delegate: agy soft-denied a tool that needs permission (headless can't prompt) — no work was done. For a FILE WRITE, the narrower fix is a permissions.allow rule covering the target in ~/.gemini/antigravity-cli/settings.json — write_file(<dir>) matches recursively beneath <dir> — which needs no flag; --yolo also works but auto-approves ALL tools. Other tools (web / Vertex AI Search / terminal) need --yolo unless a rule covers them. agy's own message above names the specific permission it wanted. If a rule is ALREADY in that file and you are still reading this, suspect the rule: run agy-doctor, because an entry agy cannot parse grants nothing (and before agy 1.1.11 granted everything). (agy >= 1.1.3)" >&2 |
There was a problem hiding this comment.
Same over-generalization as agents/antigravity-delegate.md:92: "an entry agy cannot parse … before agy 1.1.11 granted everything" applies the match-everything history to any unparseable entry, but doctor.sh's bad_allow_rules only attributes it to the zerowords class (a command(...) rule naming no command). A mistyped write_file(<dir>) — the placeholder this very message tells the user to use — is a different matcher and, per the doctor.sh comment, "never had it." This is the exact scoping bug the commit history says both reviewers already caught and fixed in doctor.sh's own output; it just didn't get propagated to this string, which is what a user actually sees at the point of an exit-15 failure.
| shopt -u nocasematch | ||
| [ -s "$ERR" ] && cat "$ERR" >&2 | ||
| echo "agy-delegate: agy soft-denied a tool that needs permission (headless can't prompt) — no work was done. For a FILE WRITE, the narrower fix is a permissions.allow rule covering the target in ~/.gemini/antigravity-cli/settings.json — write_file(<dir>) matches recursively beneath <dir> — which needs no flag; --yolo also works but auto-approves ALL tools. Other tools (web / Vertex AI Search / terminal) need --yolo unless a rule covers them. agy's own message above names the specific permission it wanted. (agy >= 1.1.3)" >&2 | ||
| echo "agy-delegate: agy soft-denied a tool that needs permission (headless can't prompt) — no work was done. For a FILE WRITE, the narrower fix is a permissions.allow rule covering the target in ~/.gemini/antigravity-cli/settings.json — write_file(<dir>) matches recursively beneath <dir> — which needs no flag; --yolo also works but auto-approves ALL tools. Other tools (web / Vertex AI Search / terminal) need --yolo unless a rule covers them. agy's own message above names the specific permission it wanted. If a rule is ALREADY in that file and you are still reading this, suspect the rule: run agy-doctor, because an entry agy cannot parse grants nothing (and before agy 1.1.11 granted everything). (agy >= 1.1.3)" >&2 |
There was a problem hiding this comment.
Good addition here, but it lands in only one of the two write-grant messages in this file. The proactive nudge at line 256 ("this looks like a write task and --yolo is not set...") recommends the exact same write_file(<dir>) rule and is fired before the run — arguably the more natural place to plant "if a rule is already there and it's still not working, run agy-doctor" — but it wasn't touched.
The CHANGELOG entry for this release explicitly scopes the "suspect the rule" caveat to three places (this exit-15 message, agents/antigravity-delegate.md, commands/delegate.md), so this looks like an intentional-but-incomplete propagation rather than an oversight — worth either adding the same one-liner to the line-256 nudge, or confirming the omission is deliberate (e.g. because that message fires pre-emptively and doctor can't have run yet).
|
Reviewed the diff (doctor.sh's new Verified by manual trace (sandbox here couldn't execute
One finding, left inline: the exit-15 soft-deny message in Everything else — the version-gated |
Review: the exit-15 message got the 'run agy-doctor if a rule is already there' caveat, but the proactive nudge at line 256 recommends the same write_file(<dir>) rule and did not — the same 'fix N-1 of N places' pattern this PR spent commits closing. It gets the short half only. That message fires on every write-looking task without --yolo, so it says what prevents the typo at the moment someone would make it: <dir> is a placeholder, agy-doctor will say whether yours parses. The diagnosis stays where the failure is.
| **`<dir>` is a placeholder — substitute a real path.** A rule agy cannot parse says | ||
| nothing in either direction: from **agy 1.1.11** it matches nothing, so the grant you | ||
| think you have is absent and the write is soft-denied for no visible reason; **before | ||
| 1.1.11** an entry that tokenized to zero command words (upstream's examples: | ||
| `command(time)`, a comment-only entry, `()`) matched **every** command and silently | ||
| auto-approved anything the agent ran — broader than the `--yolo` it was chosen instead | ||
| of. `agy-doctor` checks your entries and names which failure applies to your version. |
There was a problem hiding this comment.
This paragraph is about the write_file(<dir>) placeholder specifically, but the "before 1.1.11 ... matched every command" consequence it describes is scoped to command(...)-shaped rules with zero words — per scripts/doctor.sh's own bad_allow_rules() (and the PR's second commit message: "A mistyped write_file(<dir>) never had it. Telling someone holding one that their config auto-approves everything is a security claim about a situation they are not in.").
bad_allow_rules() classifies an unsubstituted <dir> placeholder as "unparseable", not "zerowords" — only "zerowords" (the command(time)/comment-only/() examples) carries the match-everything history, and doctor.sh prints that sentence only when a zerowords entry is present. As written, this bullet tells a reader whose write_file(<dir>) was never substituted that pre-1.1.11 it "matched every command and silently auto-approved anything the agent ran" — which, per the code's own distinction, isn't true for that matcher.
Same overclaim is copy-pasted into docs/TROUBLESHOOTING.md:113-118, docs/POC-PLAYBOOK.md:106-111, skills/antigravity/SKILL.md:220-225, and (shorter form) agents/antigravity-delegate.md:89-92. commands/delegate.md avoids it and reads correctly.
|
Reviewed the full diff (7 commits, doctor.sh probe + permissions.allow validation, plus the test-suite pipefail/hoisting fixes). Scope check:
One finding, left as an inline comment on README.md:205-211: the doc prose in five places (README.md, docs/TROUBLESHOOTING.md, docs/POC-PLAYBOOK.md, skills/antigravity/SKILL.md, agents/antigravity-delegate.md) still makes the pre-1.1.11 "matched every command and silently auto-approved anything" claim while talking specifically about the Everything else — shell quoting/portability in the new doctor.sh code, the |
…thing Review: the overclaim removed from doctor's output in commit 2 was still in the prose it was copied from. Five documents put the placeholder caveat next to the pre-1.1.11 match-everything history, which reads as though a mistyped write_file(<dir>) carried it. Per bad_allow_rules it does not: a placeholder is unparseable, not zerowords, and never had that history. Fixing the output and leaving the sentence that produced it just moves the error somewhere a reviewer will not look again. All five now say the placeholder grants nothing on any version, and name command(...) explicitly for the part that is version-sensitive.
|
Reviewed the diff against origin/master (scripts/doctor.sh, scripts/agy-delegate.sh, tests/run-tests.sh, tests/check-helper-order.py, and the doc files). Focused on shell-logic correctness, the exit-code/signal contract, and doc/version consistency, since CI already covers shellcheck/tests/plugin-validate. No blocking issues found. Specifics checked and no problems found:
One non-blocking observation (pre-existing, not introduced by this PR, so not inline-commentable): run-tests.sh still has roughly 20 remaining positive-match 'printf | grep -q' assertions outside the 25 this PR converted to has() -- e.g. the tier-model-present checks around line 711-714 that read doctor.sh's full multi-section output, structurally the same shape as the 1.1.9 version-gate assertion this PR proved was flaky under load. Not confirmed to actually flake here, but it's the same construct class the PR spent several commits eliminating elsewhere in the same file -- worth a follow-up sweep if the concurrency issue resurfaces. Nice work tightening the scope of the security-relevant claims across commits -- the class/reason separation in bad_allow_rules and the doc caveats are careful and consistent. |
Review flagged these as a follow-up on the grounds that they are positive matches, where SIGPIPE gives a noisy false FAIL. A scan says otherwise: 17 are negative assertions. SIGPIPE is only possible when grep -q matches and exits early, which for a negative assertion is precisely the moment it is meant to fail — so they cannot flake in CI, because they only break when they have a real bug to report. grep ... <<<"$x" instead: a here-string has no writer process to kill, the pipeline is one command, and -F/-i/-E/BRE semantics are untouched, which a rewrite to case would not have preserved. Verified by mutation on the plain and the -qE alternation forms. The first pass truncated a grep pattern at a ) inside it and produced valid shell that searched for the wrong string, hanging a polling loop. Caught because the suite stopped completing.
| } | ||
| # upstream's own example of a rule that tokenizes to zero command words: `time` is a | ||
| # shell reserved word that prefixes a command without being one. | ||
| allow_out="$(allow_doctor 1.1.11 '"command(time)"')" |
There was a problem hiding this comment.
bad_allow_rules() in scripts/doctor.sh (lines 154-159) explicitly handles a bare command() and any empty-body rule ("empty rule body", "zerowords" if name in ("", "command") else "unparseable"), and both the in-code comment and the CHANGELOG entry say the zero-command-word test set "follows upstream's own examples — command(time) ..., a comment-only entry, an empty compound ()". But this test block only exercises command(time), write_file(<dir>), command(echo hi > /tmp/f), command(sort < in > out), write_file(<path/to/repo>), plus a well-formed set. There's no allow_doctor case for a bare "command()"/"()" or a comment-only entry ("command(# nope)").
I traced the code manually and both cases do classify correctly (zerowords), so this isn't a live bug — but it is an untested path that the PR's own description claims is covered ("follows upstream's own examples"), which is exactly the kind of claim-vs-diff gap worth pinning down given this PR's own multiple rounds of catching exactly this class of gap.
| # and so is a PAIR of them, command(sort < in > out), where everything between the two | ||
| # brackets is a filename rather than a template. Requiring an unbroken token separates | ||
| # <dir> and <path/to/repo> from both. | ||
| PLACEHOLDER = re.compile(r"<[A-Za-z0-9_./\-]+>") |
There was a problem hiding this comment.
Minor false-positive risk: PLACEHOLDER = re.compile(r"<[A-Za-z0-9_./\-]+>") is checked against the entire rule string, not just inside a command(...)/write_file(...) argument. It correctly avoids the two tested redirect shapes (echo hi > /tmp/f, sort < in > out) because those have whitespace or two unmatched brackets, but a legitimate single-token angle-bracket literal with no whitespace — e.g. command(grep -F '<TAG>' file.txt) — would still match the placeholder shape and get misclassified as "unsubstituted placeholder" even though it's real, working content. Not tested either direction. Given the PR's own stated bar ("a false positive here sends someone to edit a rule that was always fine"), this seems like a gap worth at least a comment acknowledging the trade-off, if not a test.
|
Reviewed the diff (docs/changelog claims cross-checked against code, not just read). Overall this is careful, well-tested work — the two-directional Two minor things worth a look (posted inline):
Neither blocks — both are narrow, and the second is speculative (no evidence such a rule exists in practice). Didn't find anything in the exit-code/signal contract, the doc version numbers ( I could not execute the test suite in this sandbox (bash execution required approval that wasn't available), so the above is from static/manual trace, not a live run — CI's own run of |
Review: three shapes the CHANGELOG named as covered were exercised by nothing (command(), a bare (), a comment-only rule), and write_file() was described as not carrying the match-everything history with no test saying so. All four pinned. Also review: the placeholder test was a shape match, so a command rule holding a real angle-bracketed literal would be misread. It now applies only outside command(...), where shell syntax lives and where the placeholder is never recommended. The cost is a placeholder inside a command rule going unflagged — a miss, which this file prefers to a false positive that sends someone to edit a working rule. Writing that comment disabled the entire validator. A single quote inside python3 -c '...' closes the shell string; bash parses the rest as arguments and redirections, which stays valid shell, so bash -n and shellcheck both pass and python runs a truncated program. With stderr on /dev/null the caller reads 'nothing to report' — every negative allow-rule test still passed, and only the positive ones caught it. check-embedded-python.py now fails the suite on either signature: a body ending on a comment line, or one that no longer compiles. Both verified by mutation. The first attempt looked for the closing quote at the start of a line and false-positived on nudge-delegation.sh, where it is at the end of one. 245 -> 252.
Quorum reviewReviewing only what changed since
Beyond the diff, the models made 20 read-only lookup(s) into the checkout at Refuted by the other model (1)
No longer reportedNot raised by the last two reviews of these files. Two, rather than one, because models are not perfectly repeatable and a single scan disagreeing with the previous one is ordinary — closing on the first miss made findings flap. Still not a guarantee of a fix: they will reappear if a later review finds them again.
Usage
Reviewed |
|
Reviewed the diff (doctor's One real finding, a doc-drift regression of exactly the kind this PR spent several commits fixing elsewhere:
That parenthetical attaches "granted everything before 1.1.11" to any unparseable entry — but per Nothing else rose to reportable confidence — the rest of the diff looks correct and consistent. |
| shopt -u nocasematch | ||
| [ -s "$ERR" ] && cat "$ERR" >&2 | ||
| echo "agy-delegate: agy soft-denied a tool that needs permission (headless can't prompt) — no work was done. For a FILE WRITE, the narrower fix is a permissions.allow rule covering the target in ~/.gemini/antigravity-cli/settings.json — write_file(<dir>) matches recursively beneath <dir> — which needs no flag; --yolo also works but auto-approves ALL tools. Other tools (web / Vertex AI Search / terminal) need --yolo unless a rule covers them. agy's own message above names the specific permission it wanted. (agy >= 1.1.3)" >&2 | ||
| echo "agy-delegate: agy soft-denied a tool that needs permission (headless can't prompt) — no work was done. For a FILE WRITE, the narrower fix is a permissions.allow rule covering the target in ~/.gemini/antigravity-cli/settings.json — write_file(<dir>) matches recursively beneath <dir> — which needs no flag; --yolo also works but auto-approves ALL tools. Other tools (web / Vertex AI Search / terminal) need --yolo unless a rule covers them. agy's own message above names the specific permission it wanted. If a rule is ALREADY in that file and you are still reading this, suspect the rule: run agy-doctor, because an entry agy cannot parse grants nothing (and before agy 1.1.11 granted everything). (agy >= 1.1.3)" >&2 |
There was a problem hiding this comment.
This exit-15 message still says an unparseable permissions.allow entry "grants nothing (and before agy 1.1.11 granted everything)" — attaching the pre-1.1.11 "matched EVERY command" history to any parse failure, including the write_file(<dir>) placeholder named earlier in the same sentence.
Per bad_allow_rules() in scripts/doctor.sh (added in this PR), that history belongs only to the zerowords class (command(...) naming no command); a mistyped write_file() is unparseable and per the CHANGELOG "never had it." agents/antigravity-delegate.md:92-93 and skills/antigravity/SKILL.md:222-225 both carry the corrected, scoped wording ("do not attach that history to a mistyped write_file()"), and this PR's own fixup commit swept "five documents" to remove exactly this conflation — this .sh file (not a "document") looks like it was missed and still has the pre-fixup phrasing from the first commit.
No test pins this exact string, so scoping the parenthetical to the command(...)-naming-no-command case (like the two docs above) should be safe.
Review: the permissions.allow report is tab-separated and a rule is user-supplied JSON. An entry containing a tab shifted every field after it, and the field that moves is the class — the one that decides whether the security consequence prints. A newline split one finding across two lines; the reader dropped the orphan but the count still counted it, so the header promised more entries than it named. Class goes first now and the entry is escaped. Review: the exit-15 message still said 'an entry agy cannot parse grants nothing (and before agy 1.1.11 granted everything)', handing the command-rule history to the write_file(<dir>) placeholder it names two sentences earlier. The sweep that fixed this in five .md files did not look at .sh. Scoped, and pinned by a test. Three assertions, each verified against the unfixed code. The first newline test asserted an empty reason and mutation showed the reader already guards that, so it was pinning nothing; it now compares the header's count against the findings it lists. 252 -> 255.
| else echo "FAIL: a tab in the rule text dropped the zero-command-word consequence"; FAIL=$((FAIL+1)); fi | ||
| # A newline splits one finding across two lines. The reader drops the orphan, so nothing | ||
| # looks wrong on the findings themselves — but the COUNT is taken from the line count, so | ||
| # the header promises more entries than it goes on to name. Assert the two agree; the |
There was a problem hiding this comment.
🟡 New newline-escaping test can't distinguish fixed from fully-reverted doctor.sh
The test builds a single allow-rule entry containing an embedded literal newline (write_file(<dir>)\nwrite_file(/x)), runs doctor, and asserts the header count (claimed) equals the number of printed finding lines (listed). Tracing through the original (pre-PR) bad_allow_rules/reader pair — i.e. reverting both the field order (rule why cls) and the esc() call together, which is what "the unfixed doctor" means for a plain revert of scripts/doctor.sh — the embedded newline splits Python's single print() call into two physical stdout lines. Under the OLD field order, the first (orphan) line has no tabs, so its entire content lands in $rule (the first read variable), which is non-empty, so the [ -n "$rule" ] || continue guard does NOT skip it: the orphan is still printed as its own finding. This makes grep -c . over BAD_RULES (2 lines) equal the number of printed " — " lines (2), so claimed == listed holds and the test reports "ok" even against the fully-reverted, buggy doctor.sh. The test only becomes red when the field order is fixed (cls why rule) but esc() is removed in isolation — because then the orphan's bare text lands in $cls instead of $rule, making $rule empty and triggering the continue that actually drops it. So this assertion validates a narrower mutation (missing esc() while the field-order fix is present) rather than the count-mismatch bug as it exists in the unmodified base code, weakening its value as a regression guard for a plain revert of doctor.sh.
Second opinion from gemini-3.6-flash
claude-sonnet-5 raised this. gemini-3.6-flash was then asked to judge it — without being shown the reasoning, the severity, or who reported it, so that it would assess the code rather than agree with a colleague. Its answer:
When doctor.sh does not escape newlines, an entry containing a newline splits into two lines in BAD_RULES. The header counts lines with
grep -c .and claims 2 entries, while the output loop formats both lines with—, resulting inlisted=2. Becauseclaimedandlistedare both 2,[ "${claimed:-0}" = "$listed" ]evaluates to true and the test passes even on unescaped/reverted code.
reliability · id adac3e8ad0bb77b1
| } | ||
|
|
||
| # Report `permissions.allow` entries agy cannot use as written. Prints one | ||
| # TAB-separated `<entry>\t<reason>\t<class>` line each and returns 0 when any were found. |
There was a problem hiding this comment.
Nit: this docstring says the emitted line shape is <entry>\t<reason>\t<class>, but the actual print (line ~194) emits "%s\t%s\t%s" % (cls, why, esc(t)) — class first, then reason, then entry. The reader (while IFS=$'\t' read -r cls why rule, line ~356) correctly matches the real order. Worth updating the comment to <class>\t<reason>\t<entry> so it doesn't mislead the next person who touches this format (which the PR itself flags as security-sensitive: a shifted field silently drops the security consequence).
|
Reviewed the diff (docs, Verified, not just read:
One nit (posted inline on Nothing else stood out. This is an unusually well-tested PR for a docs/diagnostics change — the new tests are traceable to specific fixed defects (SIGPIPE false-negative, hoisting false-negative, quote-truncation false-negative) rather than being decorative. |
Reviewers: the newline test could not tell the fixed doctor from a fully reverted one. Correct, and the reason is a mistake I made one commit earlier — I dropped the empty-reason assertion after mutating only the escaping, concluded it pinned nothing, and never mutated the ordering. The two orderings fail differently and one assertion cannot see both. Entry last: the orphan line carries no rule text, the reader drops it, and only the count is wrong. Entry first: the orphan keeps rule text and prints as a finding with no reason at all. Both assertions are in now, each verified against the mutation the other misses. Also fixed the bad_allow_rules docstring, which still described the old entry-first field order that the reader no longer uses — the exact field the function's own comment calls security-sensitive if misread. 255 -> 256.
|
Reviewed the diff (doctor.sh model-probe + permissions.allow validator, the two new static checkers, the delegate.sh message tweaks, and the doc sweep). This sits on top of an already extensive self-review cycle visible in the commit history (helper-order bug, SIGPIPE-under-pipefail, TSV field-order, placeholder-shape false positives, embedded-python truncation) — traced through each fix rather than re-deriving it, and they hold up. bad_allow_rules (doctor.sh): walked the classifier by hand against every shape the tests exercise — command(time), bare (), comment-only, write_file(), the /<path/to/repo>/ placeholders, and the command(echo hi > /tmp/f) / command(sort < in > out) / command(grep -F file.txt) negative cases. The class/reason/entry split and TSV escaping are correct — a rule containing a literal tab or newline can no longer shift the class field or split into an orphan line.ver_lt: real per-segment numeric comparison, so 1.1.9 vs 1.1.11 resolves correctly on the third segment; consistent across all three call sites (--tier gate, --model probe gate, zero-words version-sensitivity gate), all guarded by the same unparseable-version pattern. --model probe (2c): gated on agy >= 1.1.11 before ever invoking -p /model; an empty reply produces no verdict either way, matching the changelog's 'not evidence of breakage' claim. The bidirectional substring match is consistent with the pre-existing model_present helper. Test-suite <<< / <() conversions: here-strings and process substitution are both plain bash builtins, fine for the bash-3.2-compatible target these scripts already assume. Doc sweep: grepped write_file( ) across every .md/.sh that recommends it (README, SKILL.md, POC-PLAYBOOK, TROUBLESHOOTING, agents/antigravity-delegate.md, commands/delegate.md, both agy-delegate.sh nudges) — all 8 sites carry the placeholder caveat now, and none still attach the pre-1.1.11 match-everything history to a mistyped write_file() (that history stays scoped to command(...) naming no command). plugin.json and SKILL.md both read 0.22.5.One caveat: the claims about real agy 1.1.11 CLI behavior (-p /model as a zero-token, no-turn slash command; falling through as literal prompt text below 1.1.11) rest on the PR's own testing against a live agy binary, which I don't have access to here. The description is explicit this was checked rather than assumed, so I'm not flagging it as a defect — just noting I verified the shell/python logic and test coverage, not the upstream CLI itself. No findings to raise. |
1 participant
Follow-ups deferred from #54, now that agy 1.1.11 is out. Three items were listed there as "not in this PR"; this settles all three, one of them by deciding not to do it.
1. Stop inferring
--model, ask#54 warns when agy is below 1.1.10, where
--modelwas ignored in headless-p. That warning is the best a version comparison can do — and a version comparison is a proxy. It is right about the releases we know about and silent about every other way the flag can fail to land.1.1.11 answers the read-only slash commands in print mode without starting an agent turn. So doctor asks:
usage.total_tokens: 0— no quota, no conversation left behind. It reads the tab-separated reply's slug and matches it against a tier configured as a display name: the same either-direction comparisonagy modelsneeded in 0.20.x.Gated at 1.1.11 deliberately. Below that the slash command is not recognised, falls through as literal prompt text, and the model answers as though it had run — probing there would spend a real turn and believe the answer it invented. The stub agy logs every invocation, so "never probes below 1.1.11" is asserted against the log, not described in a comment.
An empty answer draws no conclusion either way. An older build than the version claims, a hang, or a plan that refuses the probe is not evidence that routing is broken.
2.
permissions.allowvalidationWe recommend such a rule in eight places as the narrow alternative to
--yolo, and the recommendation ships a placeholder:write_file(<dir>).A rule agy cannot parse announces itself in neither direction, and which way it fails depends on the version:
--yoloit was chosen instead ofdoctor flags the entry, names the reason, and reports the consequence for your version rather than both.
The zero-command-word test follows upstream's own examples —
command(time)(a shell reserved word that prefixes a command without being one), a comment-only entry, an empty compound()— plus the unsubstituted<...>placeholder, which is ours.Rules it cannot judge are left alone: unbalanced quotes are agy's parser's business, and
write_file(...)is a different matcher fromcommand(...). A false positive sends someone to edit a rule that was always fine, so the well-formed case is pinned as hard as the broken ones. Checked against 12 synthetic configs including the maintainer's real rule set.3. Verified against 1.1.11, no change needed
flash→gemini-3.5-flash-high,flash-lo→-low,pro→gemini-3.1-pro-highauto-deniedandpermissions.allowverbatim, two independent anchors the classifier already matches/usageand/quotawere considered for doctor and dropped on evidence. They return zero bytes here. That is not a bug: 1.1.11's own notes say credits do not apply to accounts signed in through a Google Cloud project or ADC./model,/effortand/skillsall return data on the same setup — which is what made §1 possible.Tests
224 → 234. Every new assertion was shown capable of failing:
scripts/doctor.shalone reverted — reverting both proves nothing, which is how the first attempt at this check read "0 failures")Mutation found a real defect in one of them. The two verdicts read
takes effectanddoes NOT take effect, so a pattern copied from one branch silently stopped guarding the other: the empty-answer test passed with the branch it guards removed. The first mutation attempt also produced a false alarm by not matching the file at all, so the harness now asserts the mutation actually applied before drawing any conclusion.