Skip to content

feat(cli): apply stays inside the worker's write dirs (#1082) - #1087

Merged
Birfy merged 2 commits into
mainfrom
agent/1082-write-dir-enforcement
Nov 15, 2026
Merged

Birfy merged 2 commits into
mainfrom
agent/1082-write-dir-enforcement

Conversation

@HugoXK

@HugoXK HugoXK commented Nov 14, 2026 •

Copy link
Copy Markdown
Collaborator

Row 1 of #1082 (#963 remainder): apply obeys the operator's write allowlist.

What changed (20a75044, on 0c9e936d)

  • agentdescent/cli.py: _operator_write_guard() builds a check_write from surface_common.ensure_write_path (SurfaceError → RunStoreError, exit 2) and cmd_apply passes it into the one existing apply_with_target call. _check_apply_paths already checks the destination, every written leaf and the .bak-N slot before anything is written, so the refusal lands before disk is touched — file and tree/skill-library, --from, --install-to included.
  • show writes nothing, so --diff still renders; _refuse_plan_outside_write_dirs() makes its apply_plan report the same refusal instead of promising a write that apply would reject.

Semantics — three states, not two

AGENTDESCENT_SURFACE_WRITE_DIRS behaviour
set, non-empty exactly those roots
set, empty every write refused, including inside the cwd
unset, AGENTDESCENT_NESTED=1 defaults to the MCP boundary: cwd + $AGENTDESCENT_HOME
unset, not nested today's behaviour (allow=None)

The nested default is the amendment on the plan: a host worker that is handed no variable must not be free to write anywhere, which is the escape #963 is about. Interactive CLI behaviour is unchanged.

Red at base 0c9e936d

Clean tree at the base this PR was cut from, with only tests/test_mcp_write_confinement.py present: 6 failed, 17 passed -- every new refusal row red, the two positive rows green as they should be. Live main has moved on (c94e9d67); this PR's base is 0c9e936d and the branch is not refreshed.

Verification at the tip

Tip SHA: 20a7504

Ran:

  • pytest -q tests/test_mcp_write_confinement.py at the tip -> pass (23 passed)
  • apply/surface set at the tip, 11 files (test_apply_backup_away_from_skills, test_apply_from, test_apply_noop, test_apply_symlink_dest, test_cli_surface_error_envelope, test_cli_surface_p1, test_public_surface, test_rsi_surface_integration, test_skill_library_apply_install, test_surface_governed_integrity, test_surface_out_link_guard) -> 102 passed, 7 failed; the same 11 files on a detached tree at base 0c9e936d -> 102 passed, 7 failed, the same ids, every one OSError: [Errno 30] Read-only file system: '/home/xuke/.agentdescent' (these tests write ~/.agentdescent behind a read-only HOME), so none of the 7 is this diff
  • pytest -q tests -k "apply or surface" at the tip -> 168 passed, 7 failed, 7237 deselected (the same 7 ids)
  • red row above, measured in a detached worktree at 0c9e936d with only the new test file copied in
  • python -m tools.gen_api_docs --check -> pass (docs/api.md is up to date)
  • python -m tools.changelog_fragments check -> pass (34 fragment(s) OK)

Not run:

  • full pytest -q at this tip -> not run in this pass; CI test (3.9)--(3.13) is green at 20a750447ccf, and the base red above is the fail-on-main evidence
  • mkdocs build --strict -> not run in this pass; CI build is green at the tip
  • the docker sandbox and any live-model leg -> not available on this host

Docs/notes

  • docs/plugins.md gains the worker-default rule; docs/dsh-super-agent.md notes it next to the long-run page; changelog.d/1082-cli-write-dir-enforcement.md.
  • Known consequence, intended: in a nested worker an unset allowlist now also refuses apply --install-to <host dir> for host directories outside cwd + $AGENTDESCENT_HOME.

Partial: #1082 — rows 2 (per-Ticket budget caps) and 3 (child-env scrub test) are still open.

Six rows measured against main 0c9e936 with only this file present:
an outside destination, an outside tree, the .bak-N slot, an empty
allowlist, the nested-worker default, and show --diff reporting the
refused target.
The CLI apply had no write boundary; the MCP surfaces did. Build the same
check from surface_common.ensure_write_path and pass it into the one
apply_with_target call, so a destination, a materialised tree and the
.bak-N slot are all refused before anything is written.

An unset AGENTDESCENT_SURFACE_WRITE_DIRS keeps the terminal behaviour
(write anywhere). Inside a host worker (AGENTDESCENT_NESTED=1) it now
defaults to the MCP boundary, cwd plus $AGENTDESCENT_HOME, because a nested
process writing outside its run is the escape this guards. An explicitly
empty value refuses every write.

show --diff writes nothing, so it still renders; its apply_plan reports
the allowlist error instead of promising a refused write.
@github-actions github-actions Bot added risk:T2 Risk tier T2 (package code or an unnamed path), set by the controller from the diff agent-pr Pull request opened by an agent labels Nov 14, 2026
HugoXK added a commit that referenced this pull request Nov 14, 2026
)

Row 3 of #1082: the run `launch` detaches is our own process, so it inherits
all of os.environ -- including the worker session's DSH_*/CLAUDE*/CODEX*/MCP_*
variables and its MCP server config. Drop those prefixes with
surface_common.scrub_host_env; the API keys and PATH the run needs still pass
through. Row 1 (the CLI write allowlist) already has its own carrier in #1087,
so this change does not touch it.

Refs #1082: the per-Ticket budget-cap row is still open.
@HugoXK
HugoXK marked this pull request as ready for review November 15, 2026 04:04
@Birfy

Birfy commented Nov 15, 2026

Copy link
Copy Markdown
Collaborator

Verdict: PASS

Tip SHA: 20a750447ccfc7dff4eba120b24a65eb8a1b9ec2 · Risk tier: T2 (risk_gate.py from main c94e9d67)

Ran:

  • pytest tests/test_mcp_write_confinement.py on the tip → 23 passed (new CLI cases: outside dest/tree refused with exit 2, inside allowed, unset unchanged, .bak-N slot refused before the destination changes, empty value fails closed, nested default = cwd + $AGENTDESCENT_HOME, show reports the refusal in apply_plan).
  • Read _operator_write_guard: unset + not nested returns None, so terminal apply --to keeps today's behaviour; it reuses the existing check_write hook of apply_with_target, so the refusal lands before any write.
  • Required CI green on this tip.

Not run: a real dsh-superagent worker end-to-end.

@Birfy
Birfy merged commit f5e9333 into main Nov 15, 2026
9 checks passed
@Birfy
Birfy deleted the agent/1082-write-dir-enforcement branch November 15, 2026 04:21
Birfy pushed a commit that referenced this pull request Nov 15, 2026
…) (#1088)

Partial: `#1082` — row 3. Row 1 (the CLI write allowlist) is `#1087`,
merged as `f5e93335`; row 2 (per-Ticket budget caps) stays open on the
issue.

## What changed

`runstore.launch` built the detached run's environment from all of
`os.environ`, so the worker's own session leaked into the run: its
`DSH_*` markers, its `CLAUDE*`/`CODEX*` host markers, and any `MCP_*`
server configuration that would hand the run a second set of tools. The
run is our own process, so the candidate allowlist
(`runners._child_env`) cannot be reused — it would drop the API keys and
`PATH` the run needs.

- `agentdescent/surface_common.py`: the package's one host-session
denylist `SESSION_MARKERS`, `is_session_marker`, the
`DETACHED_KEEP_VARS` keep set, and `scrub_host_env(environ=None)`.
- `agentdescent/agents.py`: imports the list from `surface_common`
instead of carrying a second copy.
- `agentdescent/runstore.py`: `launch` builds the child env with
`scrub_host_env({**os.environ, ...})`, so `env=` cannot put the session
back.
- `tests/test_host_env_scrub.py` (new): drop, keep, one-list,
four-host-root, isolation and launch rows.
- `docs/dsh-super-agent.md` §5 and `changelog.d/1082-host-env-scrub.md`.

No model or network call. `cli.py` is untouched: the `apply`/`show
--diff` write allowlist is `#1087`'s row, and the budget caps are row 2.

## Review findings

1. **The deny list deleted host config dirs the run reads.**
`integrations._host_root` reads four variables — `DSH_HOME`,
`CODEX_HOME`, `XDG_CONFIG_HOME` (for `_opencode_root`) and
`CLAUDE_CONFIG_DIR` — so `DETACHED_KEEP_VARS` keeps all four by exact
name. Everything else in `SESSION_MARKERS` still goes,
`OPENCODE_CONFIG_DIR` and `OPENCODE_CONFIG_CONTENT` included. The
measurement is under "Verification at the tip": all four survive with
their values, the four roots resolve against them, and the session
markers are gone.
2. **Second, divergent list.** `SESSION_MARKERS` lives in
`surface_common` and `agents` imports it; a row asserts the two names
are the same object, and the parametrised row walks the tuple, so a
marker added for a new host is covered the day it is added.

## Red at base `c94e9d67`

Fresh tree at the base this PR targets, with only
`tests/test_host_env_scrub.py` copied in:

```
tests/test_host_env_scrub.py:16: in <module>
    from agentdescent.surface_common import scrub_host_env
E   ImportError: cannot import name 'scrub_host_env' from 'agentdescent.surface_common'
ERROR tests/test_host_env_scrub.py
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
1 error in 2.10s
```

## Red before the keep sets

At `c5f021e9`, with no keep set: `CLAUDE_CONFIG_DIR survives: False`,
`CODEX_HOME survives: False`, `DSH_HOME survives: False`,
`XDG_CONFIG_HOME survives: False`, and `_opencode_root` returned
`<home>/.config/opencode` while `XDG_CONFIG_HOME` pointed at
`<home>/.config-work`.

At `50bde3c2`, with the keep set holding three and the four-variable
rows in the tree: `pytest -q tests/test_host_env_scrub.py` -> `4 failed,
18 passed`, every failure `KeyError: 'XDG_CONFIG_HOME'`.

Tip SHA: 85b2974

Commits: `b5b66bf8` (the original scrub), `9766bad1` (forward-merge onto
`c94e9d67`), `c5f021e9` (one list, and `launch` re-scrubs the merged
env), `50bde3c2` (keep the three config dirs), `85b2974` (keep
`XDG_CONFIG_HOME`, isolation rows asserted by value). Fast-forward only:
no force, no rebase, no merge-up.

## Verification at the tip

Ran:
- probe over the review variables -> `CLAUDE_CONFIG_DIR`, `CODEX_HOME`,
`DSH_HOME`, `XDG_CONFIG_HOME` survive with their values; `CLAUDECODE`,
`CODEX_SESSION_ID`, `DSH_SESSION_ID`, `MCP_SERVERS`, `OPENAI_AGENT_ID`
dropped
- `integrations._host_root` re-resolved with the scrubbed env in place
-> `claude <home>/.claude-work`, `codex <home>/.codex-work`, `dsh
<home>/.dsh-work`, `opencode <home>/.config-work/opencode`
  - `pytest -q tests/test_host_env_scrub.py` -> pass (22 passed)
- focused set (`test_host_env_scrub`, `test_runstore_cli`,
`test_runstore_tilde`, `test_cli_surface_p1`,
`test_mcp_write_confinement`) -> pass (102 passed)
  - `pytest -q tests/test_plugin_runner.py` -> pass (15 passed)
- `python -m tools.gen_api_docs --check` -> pass (`docs/api.md is up to
date`)
- `python -m tools.changelog_fragments check` -> pass (40 fragments OK)
  - `mkdocs build --strict` -> exit 0 (13.47 s)
- full `pytest -q` at this tip: the 492 files from `ls tests/test_*.py |
sort`, split into 8 even chunks and run in parallel as `pytest -q <chunk
files>` with an `AGENTDESCENT_HOME` per chunk -> 7418 passed, 12 failed,
48 skipped, 10 errors. The red set is the environment set:
`test_era_srbench` 6, `test_genesis_example` 2, `test_sandbox_container`
2 failed + 10 errors (the docker image cannot be pulled on this host),
`test_wheel_smoke` 1, and
`test_integrations::test_the_launcher_install_writes_ignores_a_shadowing_cwd`
1, which fails identically at base `c94e9d67` (a stale console script in
this box's venv). None of them is in `runstore.py`, `surface_common.py`,
`agents.py` or `tests/test_host_env_scrub.py`.

Not run:
- the docker sandbox, `test_era_srbench`, `test_genesis_example`,
wheel-smoke and read-only-`$HOME` legs -> environmental on this host

Size: 6 files, `+397/-27`. T3: over the 250-line cap, and `risk:T3` from
the controller matches.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-pr Pull request opened by an agent risk:T2 Risk tier T2 (package code or an unnamed path), set by the controller from the diff

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants