Skip to content

CLI follow-ups: preview misses knowable confinement refusals; registry lacks accepted_profiles; five copies of _safe_path #12

Description

@OriNachum

Three follow-ups from the CLI groups that landed in 0.14.0 (fs, process,
env describe). None is a defect in the shipped work; each is a seam the next
slice should close, and each was identified and deliberately not fixed by the
task that found it.

1. A preview does not surface a confinement refusal it could already know

$ shell fs write /tmp/outside.txt --content X --json | jq .status
"previewed"

$ shell fs write /tmp/outside.txt --content X --apply --json | jq .status
"failed"        # "path '/tmp/outside.txt' escapes the confined root"

Path confinement is decidable before the handler runs, but the preview branch
in execute() does not evaluate it, so an agent is told previewed for an
operation that cannot succeed and only learns otherwise on the applied call —
a wasted round trip, and mildly misleading.

Counter-argument worth weighing: the repo contract says a preview "describes
what would run; it does not pretend to predict effects." Refusing at preview
time is arguably predicting. But confinement is a property of the request,
not of its effects, so surfacing it early looks consistent with that contract
rather than against it.

2. The registry has no accepted_profiles

HandlerSpec carries intent and default_profile but not the set of
accepted profiles. Building shell process therefore required promoting
_EXEC_PROFILES / _SHELL_PROFILES and their hint strings to module-level
public constants so the CLI could quote the library's own words.

That works and keeps one source of truth for the text, but the rule still lives
in two places structurally. The fix — an accepted_profiles field on
register() / HandlerSpec — touches every handler and the registry contract,
so it was deliberately deferred. Worth doing before a third execution kind
lands
, since each new kind repeats the pattern.

3. Five verbatim copies of the confinement helper

fs.read, fs.list, fs.write, fs.media and now fs.stat each carry their
own _safe_path port, with three different refusal messages between them.
shell/fs/__init__.py documents the duplication as intentional, and it is:
colleague-parity tests pin those exact strings, so consolidating would change
observable messages.

fs.stat was added as a fifth verbatim port using fs.read's message, with a
test asserting the two produce byte-identical refusals. Deduplicating is
genuinely its own task, and it needs a decision first: which message wins,
and whether changing the others is acceptable against parity.

Also worth a reviewer's opinion

shell process's --env defaults to an empty allow-list, matching
build_env's library default — not even PATH. So --env PATH is usually the
first flag a real command needs. This was chosen as honesty over convenience and
is documented in overview, explain process, and the flag help, but a
reviewer may reasonably want the trade-off revisited.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions