4666479e709ccea4e4a3d6fc64e3c2caeca1cc2e
7 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Teknium
|
f6234d00c5 |
fix(security): close GitSpawn RCE class — malicious repo .git/config no longer executes on context gathering (GHSA-7x36-8jrh-v4pw)
Hermes gathers workspace context by running git against the session directory automatically — the coding-workspace snapshot, gateway project-tree build, /diff, @diff|@staged context refs, goal-gate fingerprint, and -w startup worktree add — before any prompt, tool call, approval, or trust gate. Those probes ran the system git without stripping the repository's own config, so a repo delivered as files with its .git directory intact (a shared zip, sync folder, or USB stick; git clone never transfers .git/config) could set an execution-sink git setting and get arbitrary host code execution as the user with nothing on screen. - core.fsmonitor / core.hooksPath / pager / editor / credential helper: neutralized by routing every automatic probe through noninteractive_git_env(), which pins those keys to inert values via GIT_CONFIG_* and ignores global/system config. bounded_git_probe (the reported sink, coding_context._git + tui_gateway.git_probe) now defaults to that env; worktree-add, working_diff, web_git, context_references, goals, and subagent_worktree route through it too. - Attribute-scoped [diff "x"] command=/textconv= drivers: the attacker names the driver in .gitattributes, so GIT_CONFIG_KEY overrides can't enumerate them. Added harden_git_argv(), which inserts --no-ext-diff --no-textconv on diff-rendering subcommands (diff/show/ log/blame) only — status et al reject the flags. Both flags required (verified empirically; each alone leaves the other live). Builds on the noninteractive_git_env config-scrubbing from the gemini-cli #28792 port. Real-git E2E regression suite arms a malicious repo and asserts every automatic path neutralizes fsmonitor, hooks, external-diff, and textconv; a baseline test proves the repo is armed. |
||
|
kshitij
|
4323c67dcc |
fix(delegate): disclaim only the fields a failed probe actually left unmeasured
/simplify-code residual. The note hard-coded "'commits' and 'dirty' are UNKNOWN", but the two probes fail independently: a bad base_commit fails rev-list while `git status` still succeeds, so `dirty` is a REAL measurement being reported as unknown. Safety was never affected (the worktree is preserved either way), but telling the parent a measured value is untrustworthy is its own kind of misreport — and it would push a human toward re-inspecting something already proven. `mark_worktree_payload_unproven()` now takes an `unmeasured` argument, and finalize tracks which probe actually failed. The raising path still disclaims both, because which probe raised is unknowable there. Validation: 22/22 tests/tools/test_subagent_worktree.py; ruff + ty clean. New guard mutation-checked (hard-coding "commits/dirty" back fails it). |
||
|
kshitij
|
ce93a398e8 |
refactor(delegate): extract the unproven-payload factory; drop the source-reading test
Phase 2c fold. The schema guard added in the previous commit read and
AST-parsed delegate_tool's source, which AGENTS.md:1514 bans outright ("Never
read source code in tests" -- it passes when the implementation is subtly
broken and fails on a correct refactor). Extracting the shared factory the rule
prescribes removes the duplication the AST test was invented to police, so one
change resolves both.
- subagent_worktree: new module-level `mark_worktree_payload_unproven()` +
`unproven_worktree_payload()`. Both producers of this schema now call them,
so the payload cannot drift and the note string exists once.
- delegate_tool: the finalize-raised fallback calls the factory instead of
hand-building the dict (-16 lines). The re-import is guarded: the outer
`except` can be entered because the `from tools import subagent_worktree`
itself failed, in which case the name is unbound -- an inline fallback keeps
the flag rather than raising NameError and losing it.
- Test replaced with a BEHAVIORAL equivalent: it calls the real factory and
compares its key set against live `finalize_subagent_worktree()` output. Same
contract, no source reading, refactor-proof, and it actually executes the
code.
Also folded from the same review:
- Fail-closed on an unmeasurable commit count. With no `base_commit` the
rev-list probe never ran, `commits` kept its unproven 0 default, and a clean
tree still reached `git worktree remove --force` + `git branch -D` -- the
exact bug class #88113 is about, on a public function that takes a
caller-supplied dict. Now returns un-inspected instead, with a test driving a
real child commit.
- Per-probe diagnostics: the note said only "rev-list/status non-zero". It now
names WHICH probe failed, its exit code, and a bounded git stderr tail, so
the parent (and the human) can act on first read.
- Dropped the redundant `inspection_ok` bool for a `failed: list` of reasons;
removed the duplicated index-corruption block in favor of the existing
`_break_git_index()` helper.
Validation: 21/21 tests/tools/test_subagent_worktree.py; ruff clean; ty clean
on subagent_worktree.py and 64-vs-64 unchanged on delegate_tool.py (all
pre-existing, verified against the base commit). All 6 guards mutation-checked
twice -- neutering the flag fails 6, reverting production to pre-fix main fails
the same 6. E2E on real git: clean still prunes; corrupt index keeps the work
and reports the real stderr; empty base_commit keeps a committed child.
|
||
|
kshitij
|
97c4f9eeec |
test(delegate): assert the unproven-state contract, not its prose
Review fold on the #88113 follow-up. The new guards asserted implementation details that a strictly-better future change would break, and the second producer of the payload schema had no coverage at all. - The distinguishability test asserted the failure payload was byte-identical to the genuinely-clean one (`for key in commits/dirty/pruned: assertEqual`). That freezes the AMBIGUITY as a required property: emitting `commits: None` for "unknown" would improve exactly what #88113 is about and fail the test. Now asserts what the parent actually depends on -- both keep the worktree, and only the flag separates them. - `assertNotIn("inspection_failed", ok_payload)` pinned key ABSENCE on the happy path, forbidding an always-present-but-False flag (a legitimately better JSON contract: stable key set for serializers). Now `assertFalse(...get("inspection_failed", False))` -- same coverage, tolerant of that refactor. - `assertIn("UNKNOWN", note)` coupled tests to one word of English prose, and was not even a cross-producer contract: delegate_tool's note said "state unknown" (lowercase), so a copy-edit broke the implied convention. Tests now assert the note names the worktree AND branch -- the actionable part for a human -- and both producers' notes were aligned to read as one contract. - The raises test never proved its patched seam ran (a future short-circuit before any git call would keep it green while proving nothing). Now checks `call_count` and mirrors the branch-survival + note-names-path legs its sibling had. - NEW `WorktreePayloadSchemaTests`: commit 2's whole point is the schema the parent reads, but delegate_tool's fallback -- the second producer -- was verified only by reading. It now AST-parses the real fallback dict literal and compares against live `finalize_subagent_worktree()` output, so the two producers cannot drift and the pre-fix leak (repo_root/base_commit, missing commits/dirty/pruned) cannot come back. - Docs/docstring drift: the flag has a second trigger (finalization itself raising, handled in delegate_tool), and the module docstring listed `inspection_failed` without `note`. Both corrected. - Extracted the duplicated 5-line "corrupt the index" setup into `_break_git_index()` beside the file's other module-level helpers. Validation: 19/19 tests/tools/test_subagent_worktree.py; ruff clean. New schema guard mutation-checked -- reverting delegate_tool's fallback to the pre-fix `dict(_worktree_info)` shape fails it. Restores checksum-verified. |
||
|
kshitij
|
38ea711fd0 |
fix(delegate): tell the parent when a worktree was preserved un-inspected
The preserved worktree is invisible to the only consumer that can act on it. Completes the #88113 fix. That change correctly stops the destructive prune when a git probe fails, but still returns commits=0 / dirty=False -- values that were never measured. Those are the defaults the prune used to delete on, so the failure payload is byte-identical to "inspected fine, child left nothing": inspection FAILED, uncommitted work kept -> {commits: 0, dirty: False, pruned: False} inspected OK, child produced nothing -> {commits: 0, dirty: False, pruned: False} The only failure signal was a logger.warning, and the sole consumer of this payload is the parent agent reading the serialized delegate_task entry -- it cannot read logs (no in-repo code reads the key back). So the parent's rational reading of the failure case is "the child produced no work", which is the exact wrong conclusion: a worktree possibly full of uncommitted work is preserved and then never looked at. The data survives but nobody is told to recover it. Changes: - subagent_worktree: one _unproven() helper stamps inspection_failed + a note naming the worktree/branch, warns, and returns the payload. Both unproven exits route through it, so they cannot drift apart again. - subagent_worktree: the pre-existing exception path (timeout, OSError, a non-numeric rev-list stdout) produced the same unproven payload but logged at DEBUG -- effectively silent. It now takes the same flagged path as a non-zero exit; identical outcomes get identical reporting. - delegate_tool: the caller's finalize-raised fallback assigned the creation-side metadata dict (path/branch/repo_root/base_commit) -- a disjoint schema missing commits/dirty/pruned. It now emits the same flagged shape, and logs at WARNING. - Docs + docstring + module contract now state that pruning requires affirmative proof, so a future cleanup doesn't "fix" the preserved worktree by restoring the unconditional prune and reintroducing this P1. Purely additive: the happy-path payload shape is unchanged, so no existing reader can break. Validation: - 18/18 tests/tools/test_subagent_worktree.py; 127 passed across the delegation suites (test_delegate, batch_validation, control_actions, timeout_diagnostic). - 3 new guards mutation-checked: neutering the flag fails all three; reverting the production file to pre-fix main fails all three. Restores checksum-verified. - E2E on real git: inspection-failure now returns inspection_failed=true with work intact on disk; proven-clean still prunes (pruned=true). |
||
|
liuhao1024
|
2b490a0513 |
fix(delegate): keep the worktree when git inspection fails
finalize_subagent_worktree() treated a non-zero exit from its rev-list or status probes as proof of the payload defaults (commits=0, clean), then pruned on them: git worktree remove --force plus branch -D permanently deleted a child's uncommitted work whenever git could not inspect the tree (e.g. a corrupted index) (#88113). A destructive cleanup now requires affirmative proof of zero commits plus a clean tree. Any non-zero inspection result keeps the worktree and branch for manual review, with a warning naming both. |
||
|
Teknium
|
6ee58f4088 |
Inspired by Muse Code: opt-in git worktree isolation for delegated subagents
Adds delegation.worktree_isolation (default: false). When enabled, each delegate_task child gets its own git worktree branched from the repo's current HEAD under <repo>/.worktrees/subagent-<id>, its terminal session starts there, and its goal message carries the isolation contract (work + commit in the worktree; parent reviews/merges the branch). - tools/subagent_worktree.py: clean-room implementation from Muse Code's documented --subagent-worktree-isolation behavior (create per-child worktree, finalize/inspect after run, auto-prune clean no-commit worktrees, keep anything holding work). - tools/delegate_tool.py: config gate + per-child setup in _run_single_child; result entries gain a "worktree" field (path, branch, commits, dirty, pruned) only when isolation engaged — the default-off wire shape is byte-identical. - Git-only + local-terminal-backend-only; non-git dirs, remote backends, or any worktree failure degrade silently to shared-workspace behavior. - Tests: tests/tools/test_subagent_worktree.py (15 tests, real git repos) + E2E through _run_single_child with a real repo verified parent-checkout isolation, branch reviewability, prune, and default-off shape pinning. - Docs: delegation feature page section + configuration.md key. |