feat(security): diff-scope assertion and credential withholding for delegates

[datj9]

Summary

Delegation grants a peer agent write authority that second-opinion never had. This adds
the guardrails that authority needs: an allowlist the delegate's writes are checked against,
and credential withholding so secrets never reach the child process.

This isn't hypothetical. Aider issue #5058 — "Architect mode can turn README prompt
injection into committed backdoored code" — documents the same two-tier delegation pattern
carrying untrusted repository text all the way to a commit. Published work also finds
monitor agents approve injected-instruction-following code markedly more often when they
perceive it as their own. A delegate is best modelled as a partially-untrusted actor
operating on your repo.

Diff-scope assertion

A baseline of the working tree is captured before the delegate runs (including content
hashes), and compared after, so a tree that was already dirty isn't misattributed to the
delegate. Writes landing outside the declared allowlist fail the delegation and the
offending paths are reported.

Matching resolves both sides to real locations before comparing, so neither a ..
segment nor a symlinked directory can make an out-of-scope write appear in scope. Comparison
uses Path.relative_to rather than a string prefix test, so /a/bc does not match a
declared root of /a/b.

Failing closed is the point

A scope check that quietly passes when it cannot determine what changed is worse than no
check — it manufactures assurance. So "couldn't determine" is a distinct undetermined
status, never ok, raised when:

• the cwd isn't a git repository, or the baseline couldn't be captured;
• git status fails while evaluating post-run changes;
• the repo identity changes during the run — if the cwd stops being the same worktree,
  changes can no longer be attributed, so the result is undetermined rather than a pass.

delegation_verdict() treats any non-ok scope status as failed, so both violated
and undetermined block the green path.

Why no fifth verdict state

Scope folds into the existing failed rather than adding a state: work that wrote outside
its allowlist — or whose adherence can't be established — is not trustworthy work, which is
semantically a failed delegation. The pre-existing four-state contract and its tests are
unchanged.

Credential withholding

Credential-bearing environment variables are withheld from the delegate and must not appear
in events.jsonl, the redacted command, or any job record field. The variable name is
recorded (withheld_env) so the behaviour is auditable; the value never is.

Name matching is deliberately curated rather than aggressive: ACCESS_KEY is matched so
AWS_ACCESS_KEY_ID is caught, but a bare KEY is not listed because it would also match
KEYBOARD_LAYOUT. Over-matching would silently strip benign environment from delegates and
produce baffling, hard-to-diagnose failures.

Scope enforcement is opt-in — read this before assuming coverage

When no scope is declared, scope_result is None and the gate is skipped entirely.
An allowlist nobody wrote cannot be enforced, and denying all writes by default would break
every existing invocation. But it does mean a caller who omits the flag gets none of this
protection. Absent remains distinguishable from satisfied on the record (None vs a dict
with status: "ok"), so the state is honest rather than hidden.

Interaction with the analytics merged in #18

analytics.py computes both the verdict distribution and the escalation-rate denominator
from delegation_verdict(), so:

• Scope violations now surface in the dashboard's verdict distribution with no dashboard
  code in this PR — both features route through one verdict authority.
• The escalation rate now means "of all failed delegations (check failure OR scope violation
  OR undetermined scope), how many were re-dispatched." It mixes two failure kinds; this
  needs stating in user-facing docs.

Test plan

• python3 -m pytest -q → 453 passed (377 at this branch point, +47 here, +29 from the
  feat(dashboard): analytics rollups, delegation history, and trace_mismatch cue #18 merge). Rebased onto current main; no conflicts.
• python3 -m ruff check . and ruff format --check . → both green, no unrelated files
  touched.
• Coverage includes: writes outside the allowlist fail with paths listed; declared-path-only
  edits pass; .. and symlink escapes don't defeat the matcher; a pre-existing dirty tree
  isn't attributed to the delegate; a non-git cwd yields undetermined rather than a pass;
  no credential value reaches events.jsonl or the redacted command; schema_version 1/2
  records still load.
• Includes end-to-end tests driving a real job through worker_main and reloading state
  from disk — not just direct calls to the state-transition helper. An earlier slice in
  this series had 321 green unit tests while runtime persistence was entirely broken, so
  that distinction is load-bearing rather than ceremonial.

Known gaps

• Enforcement is opt-in, as described above; a warning when a write-capable delegation runs
  unscoped would be a reasonable follow-up.
• Detection is git-based, so writes to .gitignored paths inside a repo are invisible to
  the assertion. Worth documenting rather than implying the check is exhaustive.
• No dashboard UI for scope results yet; the data layer lands here.

[datj9]

Security review — verdict: FAIL (2 HIGH, exploitable now)

Reviewed as a security control, i.e. assuming an adversary is actively trying to defeat it, since the threat model here is a partially-untrusted delegate with write access (Aider #5058: repo prompt-injection → committed backdoor).

Findings below were reproduced empirically against scope.assert_scope in a throwaway git repo with declared = ("allowed",), not inferred from reading.

HIGH — a delegate can write .git/hooks/* and the scope check reports ok

git status --porcelain does not report changes inside .git/, so _dirty_paths never enumerates them and the assertion passes.

Reproduction:

declared = ("allowed",)
baseline = capture_baseline(cwd)
# delegate writes .git/hooks/pre-commit, chmod 0755
assert_scope(baseline, declared, cwd)
→ status="ok", violating_paths=()

Exploitation. A delegate (or repo content steering it) writes .git/hooks/pre-commit, post-checkout, or post-merge. crossagent reports the delegation as in-scope and delegation_verdict() can return verified. The hook then executes arbitrary code as the user at their next git operation — persistent code execution, surviving the job entirely. This is the exact threat class the PR exists to defend against, and it passes the gate with a green verdict.

Remediation. Hash a small fixed set of .git attack surfaces at baseline and compare after: .git/hooks/ contents, .git/config, and .git/info/exclude. These are few and small, so the cost objection that rules out hashing all ignored files doesn't apply. A change there should be violated (they can never legitimately be in a declared allowlist), or at minimum undetermined.

HIGH — writes outside the repository root are entirely undetected

git status only covers the worktree, so anything written outside it is invisible to the control.

Reproduction:

# delegate writes ../outside.txt
assert_scope(baseline, declared, cwd)
→ status="ok", violating_paths=()

Exploitation. A delegate writes ~/.zshrc, ~/.ssh/authorized_keys, ~/.claude/settings.json, or a sibling repo. All report ok. Note ~/.claude/settings.json is particularly sharp here — it can grant tool permissions to future agent sessions, so one unscoped delegation can widen the blast radius of every subsequent one.

Remediation. In-process detection can't enumerate the whole filesystem, so this is a boundary that must be either enforced at the OS level (sandbox / restricted child, which is the real fix) or stated precisely as out of scope. What it must not be is implied away — see the MEDIUM below.

MEDIUM — the module docstring overstates coverage

src/crossagent/scope.py:22: "so neither traversal nor a symlink can smuggle a write outside the allowlist."

That's accurate about the matcher and inaccurate about the control. The limiting factor isn't matching, it's enumeration: out-of-repo and .git/ writes are never enumerated, so no matching is attempted. The "Blind spots (documented, never hidden)" list at :24 is genuinely good practice, and precisely because it establishes that convention, its omissions read as "checked and safe." Add both findings above to that list, and reword :22 to scope the claim to declared-root matching of in-worktree paths.

Confirmed sound (attempted and closed — stated with evidence)

• .. traversal and symlink escape: both sides resolve()d before comparison (_resolve, _is_in_scope), so matching is on real locations. Symlink loops fall back to lexical normalization rather than raising.
• Prefix collision: uses Path.relative_to, which is component-wise — /a/bc does not match declared root /a/b. This is the classic startswith bug and it is not present.
• Pre-existing dirty tree: baseline captured before the run, with content hashes, so an already-dirty file isn't attributed to the delegate. Verified by control case: an out-of-scope tracked write is caught (violated, ('src/main.py',)).
• Fail-closed on indeterminacy: non-repo cwd, git status failure, and repo identity changing mid-run all yield undetermined, never ok — and delegation_verdict() maps any non-ok scope status to failed. The mid-run identity check is a bypass I did not anticipate and it is closed.
• Subprocess safety: argument lists with shell=False, timeouts present, matching check.py.
• Credential name matching is well-calibrated: ACCESS_KEY matched so AWS_ACCESS_KEY_ID is caught, bare KEY deliberately excluded to avoid KEYBOARD_LAYOUT. Over-matching would silently strip benign env and cause baffling failures; that tradeoff was reasoned about rather than stumbled into.
• Zero runtime dependencies preserved — stdlib only, no pyproject.toml dependency additions.

Not tested (declaring rather than implying coverage)

Hardlinks, case-insensitive filesystem collisions (relevant on default macOS), Unicode NFD/NFC path normalization, submodules, and TOCTOU between baseline capture and assertion. The two HIGH findings above were reproducible without needing any of these, so I stopped there.

Design question, not a defect: enforcement is opt-in

With no allowlist declared, scope_result is None and the gate is skipped. That's defensible — an allowlist nobody wrote can't be enforced, and default-deny would break every existing invocation — and absent stays distinguishable from satisfied on the record. But combined with the two HIGH findings, the honest summary of this control's current guarantee is: "when a scope is declared, in-worktree tracked writes outside it are caught." That's a genuinely useful property and much narrower than "the delegate is constrained." A warning when a write-capable delegation runs unscoped would close the largest usability gap in the posture.

---

Recommendation: the architecture is right — fail-closed design, one verdict authority, honest blind-spot documentation, sound matcher. The two HIGH findings are gaps in enumeration, not flaws in the design, and the .git/hooks one is cheap to fix. I'd fix .git/ enumeration before merge, and land the out-of-repo boundary as an explicit documented limitation plus a follow-up for OS-level containment.

Reviewed by Claude Code. Findings reproduced against a scratch repo; the two HIGH cases are runnable as regression tests.

[datj9]

Code review — verdict: NEEDS FIXES (3 HIGH)

Complements the security review above. All three findings independently confirmed in code, not taken on report.

HIGH — escalate.py:118 escalates a cancelled job

maybe_escalate gates on delegation_verdict(failed_job) != "failed", but delegation_verdict returns "failed" for any terminal status other than SUCCEEDED — including CANCELLED and TIMED_OUT. The module docstring scopes escalation to "a failing check, a scope violation, or a failing independent verification"; the code doesn't enforce that.

Impact: crossagent cancel <job_id> on a job with an escalation ladder silently re-dispatches the same task to a larger, costlier peer — the exact opposite of cancelling. cli.py's _failed_reason already makes this distinction (if job.status != JobState.SUCCEEDED: return "delegate did not finish cleanly"), so the concept exists in the codebase but wasn't carried into escalate.py. No test uses CANCELLED/TIMED_OUT as the parent status, so there's no regression coverage.

Fix: only escalate when the parent reached SUCCEEDED (i.e. a declared gate failed). If process-level failures should also escalate, exclude CANCELLED explicitly — it is explicit user intent — and decide TIMED_OUT deliberately. Add a test asserting a cancelled job is never escalated.

HIGH — verify.py tempfile.mkstemp() is outside its own try, breaking the "never raises" contract

Both the module and function docstrings promise a broken verifier never crashes the worker. But schema_fd, schema_path = tempfile.mkstemp(...) runs before the try: block.

Impact: a read-only /tmp, full disk, or restricted TMPDIR (sandbox/CI container) raises OSError out of run_verification, through worker.py's caller (no guard there either), into worker_main — after the check and scope assertion but before transition_to(job, final_state, ...) persists the terminal record. The job is left non-terminal forever and the delegate's actual, possibly successful, work is never surfaced. check.py and scope.py's assert_scope both guard this correctly; verify.py misses this one spot.

Fix: move mkstemp inside the try (or wrap it to degrade to non-structured mode), and add a test simulating mkstemp failure. Extracting the schema tempfile into a small context manager fixes this and the function-length note below at once.

HIGH — credential scrubbing covers the job path but not the synchronous dispatch path

credentials.py is wired into worker.py:build_advisor_env, but cli.py:115 _run_advisor calls runner_mod.run(cmd, cwd=cwd, consumer=parser, max_runtime_seconds=None) with no env=, so that subprocess inherits the full unscrubbed os.environ. The --pass-env opt-out is only registered under if subcommand == "start":, so this path has no escape hatch either — and no test covers it.

Impact: crossagent --agent codex --prompt ... — the original, default invocation mode — passes every credential-bearing variable to the advisor. The PR title says "credential withholding for delegates", not "for delegated jobs", so the security claim is currently broader than the implementation.

Fix: scrub in _run_advisor/_dispatch with the same --pass-env escape hatch, or narrow the PR/doc claim to job-based delegation only.

MEDIUM

• verify.py — obj as a variable/parameter name (_verdict_from_object(obj), obj = _parse_verdict_object(...)) is a forbidden placeholder per the project's naming rules. Rename to verdict_object.
• maybe_escalate (~84 lines) and run_verification (~85 lines) exceed the 50-line guideline. Each has a clean extraction: child-Job construction/persist, and the schema tempfile context manager.
• jobs.py (935→1061) and cli.py (914→995) were already over the 800-line max and grew. Not new, but ScopeResultDict/VerifyResultDict/ScopeStatus/VerifyVerdict live in jobs.py only because scope.py/verify.py import them back — defining them in their own modules would stop growing the oversized file.
• escalate.py's "never raises" is also partial: create_job_dir, prompt_path.write_text, and save_state are unguarded against OSError. Same root cause as the verify finding, lower likelihood.

Confirmed sound

delegation_verdict's S4/S5 gate composition (scope veto → check → verify, any-pass tracking, fail-closed on undetermined) is correct and pre-S4/S5 behaviour is preserved by dedicated tests. scope.py's resolved-path matching defeats ../symlink escape. credentials.py's scrub/withhold predicate agreement is enforced by test so the two can never disagree. No hardcoded secrets, no shell injection (git always argv, shell=False), no Any/bare-dict violations, no 3.10+-only syntax, zero runtime dependencies preserved. Tests are behaviour-focused and cover failure paths well — the gaps above are precisely the paths not yet covered.

Reviewed by Claude Code.

[datj9]

The three HIGH findings from my review above are fixed on feat/delegation-verify (#20) — commits 9f9f01a, 759e6b9, 3e2b5ad. They land here when #20 merges. Details in the #20 comment; 513 passed, ruff green, and each regression test was confirmed to fail against the pre-fix source.