[codex] feat(shell): support folder access grants - #398
Closed
ytallo wants to merge 12 commits into
Closed
Conversation
…quest pane (#392) * feat(console): register-trigger + state tool views; fix duplicated request pane - FunctionCallMessage: suppress the top request pane for every completed call, fixing REQUEST rendering twice on generic (no-custom-view) tool cards. - engine::register_trigger: new rich view — trigger_type → function_id, config chips for state triggers, harness::react model/join(id·key·expect·rearm)/task, plus the harness subscribe variant (label, once, no function_id → "notify session"). Falls back to raw request JSON so the terminal pane is never blank. - state::* : new family view for get/set/delete/update/list/list_groups — scope/key chips + the unwrapped result as one highlighted JSON block. - Tests (engine + state parsers) and Storybook fixtures (EngineFamily, new StateFamily gallery). Claude-Session: https://claude.ai/code/session_01SB8sknFhJLojcaBdQazmPH * fix(console): guard state view error detection against denial-shaped values state::* results are arbitrary JSON, so running every result through the shared sandbox error parser flagged a successful get of a value like { status: "denied" } / { denied_by: … } as a red "Denied" error, hiding the real JSON on the default tab. Only run the parser for non-success envelopes; genuine errors ({ error: { kind: "function_error" } }, wire/ denial shapes) still surface since they are never { content, details }. Also dedupe the harness::react allow-list chips so the React key stays unique when the engine-supplied allow list repeats a function id. Adds state view render tests covering both directions.
![workers-ci[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
… call (#388) * fix(harness): stamp the session working_dir on every scoped invocation path The console-picked working directory is guaranteed reachable through the per-call base_dir stamp, but three invocation paths skipped it: - the deferred approval-release path invoked the recovered transcript arguments un-stamped, so an approved shell/coder call lost the session scope and a model-supplied base_dir survived un-stripped - sub-agent turns were seeded with no metadata, so children could never reach the parent session's directory - harness::function::trigger invoked its target without the stamp pre_trigger hooks now receive the stamped arguments (an approver reviews the base_dir the call actually runs under), with an idempotent re-stamp after the chain so a hook rewrite can never widen the scope. TurnOptions::working_dir() is the shared accessor for all stamp sites; children inherit only the parent's working_dir, never its per-turn tracing metadata. * fix(console): validate browsed directory picks against the live shell worker Browsed "use this folder" selections bypassed shell::workspace::validate. Every selection path — pasted, remembered, or browsed — now round-trips through validate, so a stale listing can't select a vanished directory and the worker-echoed canonical path is what gets stored. * refactor(harness): share WORKING_DIR_KEY across read/write sides Extract the working_dir metadata key into a single WORKING_DIR_KEY constant (mirroring workspace_inject::BASE_DIR_FIELD) used by both TurnOptions::working_dir (read) and subagent::inherit_workspace (write), so a rename cannot silently desync the two and drop child scope. Also complete the working_dir() doc enumeration to include function::trigger, the third stamping path.
BREAKING CHANGE: the top-level inherit_env and allowed_env config keys are replaced by env.inherit / env.allow, with no legacy aliases. The old keys are rejected at parse with a migration hint — serde would otherwise ignore them and silently boot with env forwarding off. A stored configuration value carrying the old keys fails closed at boot; rewrite it via configuration::set (id: shell) after deploying the new binary. Also: - --version flag; --url documented in --help including the III_URL env var - pre-connect reachability probe: one loud ERROR with a fix hint when the engine is unreachable, before the SDK's silent 2s-backoff retry loop - schema descriptions on every operator-visible config field (incl. the nested env/fs/sandbox blocks) so the console config UI documents each knob inline; pinned by a unit test - README Running section documenting the binary's operator surface (--config, --url/III_URL, --version, RUST_LOG); ARCHITECTURE CLI table and defaults table updated, code-defaults vs shipped-seed distinction spelled out - from_yaml re-deserializes from text after the removed-key check so unquoted boolean-like strings (allowlist: [false]) keep parsing; regression-tested - e2e fixtures migrated to the nested env block; tests/e2e/config/ (engine- externalized runtime state) gitignored and the tracked fixtures kept self-contained
…prefer host_roots, describe every knob Applies the config.yaml review findings: - Anchor mkfs/dd/shutdown/reboot denylist patterns to argv[0] (^(\S*/)?name) so they fire when the tool IS the command, not when the word appears in an argument — 'grep -rn shutdown src/' no longer rejected. rm -rf /, the fork bomb, and /etc/shadow stay unanchored (argument-shaped by nature). Pinned by new allow/deny test cases. - Reword the denylist rejection to say it is an advisory tripwire and to rephrase, so agents stop retrying verbatim. - Seed uses the preferred fs.host_roots: [/tmp] (legacy host_root form dropped from the shipped example). - Seed default_timeout_ms 10s -> 30s (code default unchanged): the seed already raises max_timeout_ms to 120s for real builds; callers omitting timeout_ms shouldn't be reaped at 10s on the same workload. - fs.max_read/write_bytes schema descriptions explain why the code default is unlimited (streaming; the cap bounds caller cost, not worker memory); seed comments mark fs.denylist_paths as defense in depth and explain the passwd/shadow exec-side asymmetry. - Doc-comment every remaining CoderConfig budget field; the schema description test now sweeps ALL nested definitions.
…base_path(s), coder migration fold Rides the 0.7.0 breaking window (stored values already need a rewrite): - fs.host_root (0.6.x single-root alias) removed. Rejected at parse with a migration hint (fs.host_root -> fs.host_roots, one-entry list), same fail-closed rationale as the env rename: serde would silently ignore the stale key and the worker would see no jail configured. FsConfig::roots() and is_jailed() lose the legacy branch; the both-keys-set config error disappears with the alias. - code.base_path removed and code.base_paths taken off the wire (serde+schemars skip). They were inert: the code resolver has always taken its roots from fs.host_roots via code_resolver_config, so stored values still carrying them are silently ignored — no reject, they never had an effect. - The one-shot coder->shell config migration is retired (migrate_legacy_coder + hidden migrated_from_coder marker). Boot no longer probes configuration::get for 'coder', which also removes the "configuration 'coder' not found" WARN retries from every boot. Stacks that still need the fold should boot 0.6.x once before upgrading. Tests: legacy-alias boot test inverted into a rejection test; new yaml/json fs.host_root rejection tests; golden C210 case regenerated (no-reachable-roots replaces both-root-forms). 1270 unit tests, e2e 171/171 + jailed 2/2.
base_paths is serde-skipped since 0.7.0, so code_resolver_config is the SOLE source of coder::* roots — a regression dropping the fill would silently jail the code surface to the default ['./', '/tmp'], wider than the operator's fs jail.
…ylist hardening, half-migration rejection Applies findings from a specialist + red-team + Claude/Codex adversarial review pass over the env-consolidation branch: - Seed-file parse failures now abort boot instead of silently seeding the permissive built-in default in place of the operator's intended policy — every un-migrated 0.6.x --config file now hits this path. A genuinely missing file still falls through gracefully. - Hot-reload no longer misclassifies a fetched-but-unparseable stored value as a transient fetch error (infinite retry against bytes that can never parse); it is now Rejected — keep last-good, ack, record for shell::config-status — the same treatment as an unbuildable config. reload_serialized's fetch closure now returns the raw fetched Value so parsing happens inside the classification, not before it. - EnvConfig denies unknown fields, and the removed-key checker was unified into one function covering the top-level, fs, and env objects in a single traversal — a half-migration (old key names nested under the new env: block) now gets the same friendly hint as every other removed key, instead of a generic serde error. Consolidating the two separate checkers surfaced a real bug: the original used .any(), which short-circuits, so a config carrying both removed env keys only ever named the first in its error; now collects every hit. - The anchored, wrapper-tolerant denylist patterns are now case-insensitive and handle env's idiomatic KEY=VALUE form (env FOO=bar shutdown bypassed the tripwire; only bare env shutdown was covered before). - The boot-time reachability probe runs on a detached thread so its unbounded DNS resolution and bounded TCP connects can never delay boot, and logs host:port instead of the raw URL (a wss://user:pass@host URL could otherwise leak credentials to the log). - cargo fmt violations, an EnvConfig test-fixture helper to deduplicate four near-identical constructions, and a cross-test mutex to fix a real intermittent flake (two tests mutating process env could race on separate cargo-test threads). 1278 unit tests, e2e 171/171 + jailed 2/2, cargo fmt/clippy clean.
Adds a Fixed section to the 0.7.0 CHANGELOG entry covering the seed/reload fail-closed fixes, the half-migration rejection, the wrapper-tolerant denylist case-insensitivity and env KEY=VALUE coverage, and the flaky-test fix. Strengthens the coder-migration-removal wording in both files to spell out the concrete consequence of skipping the boot-0.6.x-first escape hatch (silent seed to the generic /tmp default) rather than only naming the removal. Updates the Running section's probe description to match the detached-thread, host:port-only logging behavior.
…migration, no legacy tolerance
The previous commits silently ignored two removed 0.7.0 keys instead of
rejecting them: code.base_path/base_paths (never had a runtime effect,
so 'harmless to keep' seemed reasonable) and the migrated_from_coder
marker (pure internal bookkeeping, never operator-set). Per explicit
direction: this branch does a hard migration with zero legacy-support
code, no exceptions for keys that happen to be inert or invisible to
the operator.
check_removed_keys is restructured around a RemovedKey{old, new: Option}
type so pure removals (no replacement — just delete the key) share the
same table-driven scan as renames. Both new cases get the same
hard-fail treatment and migration hint as inherit_env/allowed_env/
fs.host_root: named in the error, pointed at configuration::set.
Corrects the CHANGELOG/README claims accordingly, and is precise about
what this does and doesn't fix: an install that already has a stored
shell entry (went through the 0.6.x coder fold, carries
migrated_from_coder: true) now fails closed at boot instead of silently
parsing past the marker. An install with ONLY a standalone coder entry
and no shell entry at all still seeds the generic /tmp default
silently — there's nothing stored to reject in that case, so the
boot-0.6.x-first escape hatch remains the only fix for that scenario.
1282 unit tests (4 new), e2e 171/171 + jailed 2/2, fmt/clippy clean.
…path values Revises comments in ShellConfig and CoderConfig to specify that stored values carrying removed keys like code.base_path are rejected during parsing, enhancing clarity on migration behavior. Removes outdated test for ignored base_path values, aligning with the hard migration approach established in previous commits.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
Superseded by #397, which is the current review target for the filesystem-scope architecture. This draft branch is conflicting and belongs to the older folder-grant stack. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stack
feat/shell-env-config/ PR feat(shell)!: consolidate env/fs config into nested blocks; hard-reject every removed 0.6.x key #391feat/harness-folder-access-grants->feat/shell-folder-access-grants)Validation
Not rerun in this turn; this PR was opened for the already-pushed stack branch so the existing harness PR has a visible base PR.