-
Notifications
You must be signed in to change notification settings - Fork 24
feat(doctor): ask agy which model it runs; validate permissions.allow #56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
a68773b
9b5a394
7b79376
da29cef
f9451bc
7932b5c
e35f739
8d39412
1574bc7
6fdb0a6
d7b159d
984b41d
1831e03
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,6 +92,88 @@ sys.exit(0 if n else 1) | |
| ' "${files[@]}" 2>/dev/null | ||
| } | ||
|
|
||
| # 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: this docstring says the emitted line shape is |
||
| # | ||
| # The class matters because the CONSEQUENCE is not shared. `zerowords` is the specific | ||
| # thing agy 1.1.11 fixed — a `command(...)` rule naming no command, which used to match | ||
| # every command — and only that class carries the auto-approve-everything history. | ||
| # `unparseable` is everything else: agy cannot use the entry, so the grant is not in | ||
| # effect, and nothing beyond that is known. Reporting the first consequence for the | ||
| # second would put a security claim in front of someone it does not apply to. | ||
| # | ||
| # WHY doctor owns this. The plugin recommends a `permissions.allow` rule in eight | ||
| # places as the NARROW alternative to `--yolo`, and that recommendation carries a | ||
| # placeholder — `write_file(<dir>)`. A rule that agy cannot parse does not announce | ||
| # itself in either direction: before 1.1.11 an entry that tokenizes to zero command | ||
| # words matched EVERY command and silently auto-approved anything the agent ran, which | ||
| # is broader than the `--yolo` it was chosen over; from 1.1.11 it matches nothing, so | ||
| # the grant simply is not there and the write is soft-denied for no visible reason. | ||
| # Same typo, opposite failures, no message either time. | ||
| bad_allow_rules() { | ||
| command -v python3 >/dev/null 2>&1 || return 1 | ||
| python3 -c ' | ||
| import json, re, shlex, sys | ||
|
|
||
| try: | ||
| perm = (json.load(open(sys.argv[1])) or {}).get("permissions") or {} | ||
| allow = perm.get("allow") | ||
| except Exception: | ||
| sys.exit(1) | ||
| if not isinstance(allow, list): | ||
| sys.exit(1) | ||
|
|
||
| # Shell reserved words that may PREFIX a command without being one, so a rule made | ||
| # only of them names no command. agy 1.1.11 describes the class it fixed as an entry | ||
| # that "tokenizes to zero command words" and gives command(time) as its own example. | ||
| PREFIX = {"time", "!", "{", "}", "[[", "]]", "if", "then", "elif", "else", "fi", | ||
| "case", "esac", "for", "select", "while", "until", "do", "done", "in", | ||
| "function", "coproc"} | ||
|
|
||
| # A placeholder is the <...> shape AND placeholder-shaped content: one unbroken token, | ||
| # no whitespace. A bare angle bracket is a literal redirect — command(echo hi > /tmp/f) — | ||
| # 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor false-positive risk: |
||
|
|
||
| bad = [] | ||
| for e in allow: | ||
| if not isinstance(e, str): | ||
| bad.append((repr(e), "not a string", "unparseable")); continue | ||
| t = e.strip() | ||
| if not t: | ||
| bad.append(("(empty string)", "empty entry", "unparseable")); continue | ||
| if PLACEHOLDER.search(t): | ||
| bad.append((t, "unsubstituted placeholder — replace <...> with a real value", | ||
| "unparseable")); continue | ||
| i = t.find("(") | ||
| if i < 0 or not t.endswith(")"): | ||
| continue # not the NAME(...) shape; not ours to judge | ||
| name, inner = t[:i].strip(), t[i + 1:-1] | ||
| if not inner.strip(): | ||
| # Bare `()` is one of the zero-command-word examples upstream gives, and so is | ||
| # command(). An empty body on any OTHER matcher, e.g. write_file(), is merely | ||
| # unusable and carries none of that history. | ||
| bad.append((t, "empty rule body", | ||
| "zerowords" if name in ("", "command") else "unparseable")); continue | ||
| if name != "command": | ||
| continue # write_file(...) etc. are a different matcher | ||
| try: | ||
| words = shlex.split(inner, comments=True) | ||
| except ValueError: | ||
| continue # unbalanced quotes: agy parses these, not us | ||
| while words and words[0] in PREFIX: | ||
| words.pop(0) | ||
| if not words: | ||
| bad.append((t, "tokenizes to zero command words", "zerowords")) | ||
|
|
||
| for t, why, cls in bad: | ||
| print("%s\t%s\t%s" % (t, why, cls)) | ||
| sys.exit(0 if bad else 1) | ||
| ' "$1" 2>/dev/null | ||
| } | ||
|
|
||
| # True when version $1 is strictly older than $2. Pure shell on purpose: `sort -V` is | ||
| # not universal, and when it is missing the command substitution comes back EMPTY, the | ||
| # comparison quietly fails, and the check it guards never fires — a version gate that | ||
|
|
@@ -193,6 +275,42 @@ if command -v agy >/dev/null 2>&1; then | |
| info "agy is multi-model/plan-dependent — remap tiers via CLAUDE_PLUGIN_OPTION_TIER_* (or set _DEFAULT_MODEL), or pass --model <name from \`agy models\`)" | ||
| fi | ||
| done | ||
|
|
||
| # 2c. Does --model actually TAKE EFFECT? Being listed in `agy models` and being | ||
| # honoured are different questions, and for three releases the answer to the second | ||
| # one was no (see the 1.1.10 warning above) while everything here still read green. | ||
| # | ||
| # The check above can only infer that from a version string. 1.1.11 answers the | ||
| # read-only slash commands in print mode without starting an agent turn, so doctor | ||
| # can stop inferring and ASK: request a tier model, see which one comes back. Costs | ||
| # no tokens, no quota, and leaves no conversation behind (`usage.total_tokens: 0`). | ||
| # | ||
| # Gated at 1.1.11 on purpose. Below it the command is not recognised, falls through | ||
| # as literal prompt text, and the model answers as though it had run — so the probe | ||
| # would spend a real turn AND return a made-up answer. Verified on 1.1.11: display | ||
| # names and slugs both work, and all three tiers come back as themselves. | ||
| case "${AGY_VER:-}" in | ||
| ''|*[!0-9.]*) : ;; | ||
| *) | ||
| if ! ver_lt "$AGY_VER" 1.1.11; then | ||
| # cut -f1: the reply is one tab-separated record, `<slug>\t<display name>`. | ||
| EFF="$(agy_guard 20 --model "$FLASH" -p /model 2>/dev/null | head -1 | cut -f1)" | ||
| EFF_N="$(norm_model "$EFF")"; WANT_N="$(norm_model "$FLASH")" | ||
| if [ -z "$EFF_N" ]; then | ||
| # No answer at all: a hang, an older build than the version claims, or a | ||
| # plan that refuses the probe. Not evidence of breakage — stay quiet rather | ||
| # than report a failure doctor cannot actually substantiate. | ||
| : | ||
| elif case "$WANT_N" in *"$EFF_N"*) true ;; *) case "$EFF_N" in *"$WANT_N"*) true ;; *) false ;; esac ;; esac; then | ||
| ok "--model takes effect (asked for '$FLASH', agy reports '$EFF')" | ||
| else | ||
| warn "--model does NOT take effect: asked for '$FLASH', agy reports '$EFF'" | ||
| info "every delegation runs '$EFF' instead, and nothing in the output says so." | ||
| info "--tier / tier_* remaps are therefore inert. Check for a persisted default" | ||
| info "or a profile overriding it, then re-run doctor." | ||
| fi | ||
| fi ;; | ||
| esac | ||
| elif [ "$AGY_TIMED_OUT" -eq 0 ]; then | ||
| # Empty WITHOUT a timeout kill: genuinely no models -> auth/network is the likely cause. | ||
| bad "agy could not list models (not authenticated, or no network)" | ||
|
|
@@ -208,6 +326,38 @@ if [ -f "$SETTINGS" ]; then | |
| LOC="$(sed -n 's/.*"location"[: ]*"\([^"]*\)".*/\1/p' "$SETTINGS" | head -1)" | ||
| ok "agy settings: ${SETTINGS/#$HOME/~}" | ||
| [ -n "$PROJ" ] && info "GCP project: $PROJ location: ${LOC:-?}" | ||
|
|
||
| # 3b. permissions.allow entries agy cannot use as written. | ||
| if BAD_RULES="$(bad_allow_rules "$SETTINGS")"; then | ||
| warn "permissions.allow: $(printf '%s\n' "$BAD_RULES" | grep -c .) entry/entries agy cannot use as written" | ||
| ZEROWORDS=0 | ||
| while IFS="$(printf '\t')" read -r rule why cls; do | ||
|
quorum-code-review[bot] marked this conversation as resolved.
Outdated
|
||
| [ -n "$rule" ] || continue | ||
| info "$rule — $why" | ||
| [ "$cls" = zerowords ] && ZEROWORDS=1 | ||
| done <<EOF | ||
| $BAD_RULES | ||
| EOF | ||
| # Every class above means the grant is not in effect. Only the zero-command-word | ||
| # class ALSO has the pre-1.1.11 history of matching everything, so that sentence is | ||
| # printed only when such an entry is actually present — putting a security claim in | ||
| # front of someone holding a mistyped write_file() would be worse than saying less. | ||
| info "an entry agy cannot use grants nothing, so the tool it was meant to cover is" | ||
| info "still soft-denied (exit 15) with nothing in the message naming the rule." | ||
| if [ "$ZEROWORDS" -eq 1 ]; then | ||
| case "${AGY_VER:-}" in | ||
| ''|*[!0-9.]*) | ||
| info "a rule naming no command is also version-sensitive: before agy 1.1.11 it" | ||
| info "matched EVERY command. Check your version." ;; | ||
| *) | ||
| if ver_lt "$AGY_VER" 1.1.11; then | ||
| info "worse on agy $AGY_VER: a rule naming no command matches EVERY command there" | ||
| info "and silently auto-approves anything the agent runs — broader than the --yolo" | ||
| info "it was chosen instead of. Fix the entry, or \`agy update\` to 1.1.11+." | ||
| fi ;; | ||
| esac | ||
| fi | ||
| fi | ||
| else | ||
| info "no agy settings.json yet (${SETTINGS/#$HOME/~})" | ||
| fi | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph is about the
write_file(<dir>)placeholder specifically, but the "before 1.1.11 ... matched every command" consequence it describes is scoped tocommand(...)-shaped rules with zero words — perscripts/doctor.sh's ownbad_allow_rules()(and the PR's second commit message: "A mistypedwrite_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"(thecommand(time)/comment-only/()examples) carries the match-everything history, anddoctor.shprints that sentence only when a zerowords entry is present. As written, this bullet tells a reader whosewrite_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.mdavoids it and reads correctly.