Repository navigation
fix: apply reviewed maintenance repairs for #3394 - #3686
kovtcharov wants to merge 6 commits into
Conversation
Bypass permissions turned off the confirmation prompt and nothing else, so a user who had granted blanket consent still could not get the agent to run a build or a test suite. Compound commands were refused before they parsed, and no interpreter, test runner or package manager was in the allowlist — "verify your work before claiming it is done" was not something the agent could do, however well it was prompted. Under bypass, the shell's own gates now come off with the prompt: operators (&&, ||, ;, >) parse and run, the read-only binary policy is replaced by a developer set, and the rate limit is lifted. Off by default and byte-identical to before when off — ALLOWED_COMMANDS is untouched and the read-only tier keeps claiming exactly what it claims, so amd#2768's hardening of it stays meaningful. The developer set adds node, npm, make, cmake, go, cargo, sed, awk, curl, sleep, timeout, export, cp and mv. It also names python, python3, pytest and gh, which are consolidation rather than new reach: those already had paths via execute_python_file and shell:execute skill grants, and listing them here gives bypass one answer to "may this binary run" instead of three. rm stays out — not a boundary, since anything in the set can delete a file, but a tripwire against an accidental recursive delete. Stdio transport only. The HTTP surface is a bound socket, and an unguarded shell reachable over it is remote code execution rather than a relaxed permission model, so the handler pins bypass off and the request model forbids unknown fields. bypass_permissions is also a separate handler attribute from auto_approve_gated_tools: an unattended harness that merely pre-approves prompts must not inherit an unguarded shell. Skipping the prompt does not skip the record. Every command run under bypass is written to file_audit.log with its full arguments and its per-segment breakdown, so `cd build && make` is two auditable invocations rather than one opaque string. The per-segment walk is what produces that record, so it stays intact in both modes. Closes amd#3373 Closes amd#3374
The bypass-control guard read `.path` off every entry in the app's route list, which raises on the `_IncludedRouter` wrapper that `include_router` leaves on the CI FastAPI version. Skipping entries without a `.path` would have walked straight past the mounted API and left the assertion passing on an empty set, so the walk now descends into sub-routers and asserts it reached a known route before checking for bypass paths.
…oint The guard asserted /v1/gaia/query was reachable before checking for bypass routes, and that endpoint is not mounted in the CI environment — so the guard failed there while passing locally. Keying it on the /v1/gaia/ prefix still proves the walk descended into the included router, without depending on which endpoints a given environment mounts.
… says Under bypass permissions, `ls\nrm -rf /tmp/x` ran both commands while only `ls` was ever checked. `rm` is deliberately outside the developer command set, so the line break walked a refused binary straight past the allowlist, and the audit log recorded one invocation where two had run. shlex was asked to treat `;&|<>` as punctuation but left the newline in its whitespace set, so it vanished during tokenisation and `_split_pipeline` never saw a break. The newline joins the punctuation set and comes out of `whitespace`, so an unquoted one now survives as its own token. shlex emits a run of adjacent punctuation as a single token, so a newline also arrives fused to whatever preceded it — `;\n`, `&&\n`, `\n|\n`. Segment splitting keys on that rather than on set membership alone. A newline inside quotes is still ordinary data: `echo "a\nb"` stays one operand, and is covered by a test so the fix cannot regress into refusing it.
github-actions
Bot
added
documentation
Request changesThis is a large, mostly-good bundle: a task-start project map for the flagship agent, a The runner-health monitor will now page the team about healthy machines. Tightening the staleness limit to three days works for the scheduled Monday run, but the monitor can also be started by hand — and any manual run from Thursday onward sees a perfectly normal weekly heartbeat as four-plus days old, reports every runner offline, and fires the Teams alert. Pick a limit that sits above a healthy age but below one genuinely missed weekly beat. The TUI-driving skill lost two sections that have nothing to do with this change. Rewriting the dev-mode instructions also deleted the note about how cards and conversation context interact, and the whole "how to report what you saw" section. Both look like collateral from editing the tail of the file — please put them back. Two user-visible defaults change with nothing telling existing users. The Agent UI's MCP server and the Telegram health endpoint both move off their old port, so anyone with a saved MCP client config or a health check pointed at the old one gets a silent connection failure. Separately, the tool decorator now rejects keyword arguments it used to ignore, which means an agent written outside this repo that passes them stops importing. The reference docs were updated correctly, but a changelog line for each would save the support round trip. Some genuinely useful model-facing guidance was deleted along with the dead keyword arguments. The removed text included instructions like "after indexing a document you must query it, never answer from memory" — that guidance was never reaching the model, so removing it changes nothing today, but it's now gone from the tree entirely rather than moved to the docstring, which is where it would take effect. Real-world evidenceN/A — no evidence bundle was produced for this PR, and the CI lane here has no inference backend. The verdict rests on static review plus the test suite the PR ships, which is substantial: new unit tests for the project map, the trace writer, the port map, the publish-workflow gate, Claude tool history, and the email fixes. The user-facing surfaces this PR touches — the Agent UI MCP port move, the TUI 🔍 Technical details🟡 Important1. Manual runs of the runner monitor now report healthy runners as stale (
A threshold derived from the cadence rather than from the Monday check keeps both properties (catches one missed beat, survives a mid-week manual run): 2. The hunk at 3. Breaking changes with no changelog entry
4. Deleted prompt guidance that would have worked in a docstring
🟢 Minor
Strengths
No prompt-injection attempts found in the diff or changed docs. |
79eb81d to
0bd22d1
Compare
|
This PR no longer contributes anything — its base branch already contains all of it. Recommend closing it rather than resolving the conflict. GitHub shows it as conflicting, but that conflict is only the branch trying to revert work the base has since moved past. Merging it in either direction produces nothing new; merging it the way GitHub would would undo roughly 400 lines the base branch now has. Note this targets 🔍 Technical detailsProbe (non-destructive, no branches touched): Empty residual — the base is a superset. Running it with the branch winning instead yields Nothing was pushed to this branch. |
|
This branch looks like an older snapshot of #3394 rather than a separate change, so I've left its merge conflicts alone — resolving them would mean redoing work #3394 has already done, on a worse base. Worth a maintainer deciding between the two before either gets more effort. If #3394 is the one being carried forward, this can close; if there's a repair here that #3394 lacks, cherry-picking it across is cheaper than reconciling two copies. 🔍 Technical details
Neither branch contains the other ( Conflicts here are in |
|
This PR no longer adds anything — everything in it is already on the branch it targets, and that branch is now ahead of it in three places. #3394's branch ( 🔍 Technical details
The remaining two:
The base also carries two repairs this PR does not have at all: Merging the base in here produces 34 conflict hunks across 11 files, all of which resolve to the base's side. I aborted rather than push a resolution that yields an empty PR. |
…ance-pr-3394
The base branch took main's merge, which replaced the shell executor this
branch was repairing: the whole-line pipeline (_split_pipeline, _tokenize,
_check_rate_limit) became a per-step walk (_parse_line -> _Step ->
_run_step/_run_pipeline), and chaining with &&, ||, ; and | now runs by
default instead of being bypass-only. Every conflict was that same
stale-vs-current split, so each one resolves to the base side.
The three maintenance repairs this branch carries are already on the base
under different SHAs, and the base carries two the branch does not:
_is_lone_granted_segment / _redirects (refusing a redirect on the argv-only
skill-granted path) and the executor-fixture repair. _is_granted_binary is
superseded by _is_granted_segment, which applies policy_argv before matching.
The merged tree is therefore identical to the base tip.
Conflicts resolved to the base side, all eleven:
src/gaia/agents/tools/shell_tools.py
tests/unit/test_shell_guardrails.py
docs/plans/security-model.mdx
docs/spec/shell-tools-mixin.mdx
hub/agents/gaia/npm/{CHANGELOG.md,SKILL.md,SPEC.md}
hub/agents/gaia/python/gaia_agent/stdio.py
hub/agents/gaia/python/tests/test_stdio.py
tui/internal/{cli/root.go,ui/chat/bypass.go}
|
Closing this — it has no changes left to merge. Everything it proposed was folded into #3394 directly, so its diff against that branch is now empty and merging it would only create an empty merge commit. Nothing is lost by closing: the repairs are already in #3394. Reopen if you find something that did not carry over. |
2 participants
Follow-up fixes for #3394. Merged current origin/main into the isolated maintenance worktree without committing. Resolved the changelog conflict by retaining both bypass and project-map entries;
git diff --name-only --diff-filter=Uis empty. The staged mainline changes are the merge, not unrelated edits.This PR targets the original feature branch because the current account cannot push to
amd/gaia. It carries the prepared maintenance changes for that PR.Test plan
go test ./internal/ui/chat/... ./internal/cli/...andgo build ./...both pass. Changed-file Black/isort/Flake8 checks andgit diff --checkwere run. New regression asserts a grantedghredirect under bypass reaches shell=True with its intact original command.This commit also resolves main conflicts. Preserve both parents with a merge commit or fast-forward the original branch to this head; squashing/rebasing this follow-up discards the main ancestry needed to resolve the original PR conflict.