Repository navigation
Fork features: ci-lines, skills roots, instructions dir/global, fail-closed, output schema - #22
Conversation
A file-level gate still inherits a file's whole upstream lint debt: a PR editing one line of message_processor.rs was failed for a let_and_return elsewhere in that file. The gate now reads the changed line ranges from git diff -U0 and fails only when a warning or error's primary span overlaps one of them. A pure deletion (count 0) overlaps nothing.
extra_user_roots and extra_project_roots add skill directories beside the built-in ones, which are unchanged when the keys are absent. Each value is relative to its base directory and dropped when it resolves outside it, so the config cannot point skill loading at an arbitrary path. WI-1133
SkillsConfig gained extra_user_roots and extra_project_roots, and the bundled-skills assertion builds one by hand. Both default to empty. WI-1133
extra_user_roots and extra_project_roots are part of SkillsConfig, so the committed config.schema.json fixture has to include them. WI-1133
Set project_root_markers = [] via a User config layer so the test does not depend on ancestor directories of the temp dir (e.g. a real .git).
When claude_compat_instructions is set, the global user-instructions snapshot also includes ~/.claude/CLAUDE.md and ~/.claude/rules/*.md in sorted order. Line-only @path imports expand to depth 5, are deduped by path, symlink and body, and count against project_doc_max_bytes. The switch defaults off, so upstream behaviour is unchanged. WI-1131
expand_imports and expand_imports_all take allowed_roots. None leaves the global user-instructions path unrestricted; a caller that passes project roots gets every file canonicalized and refused, with a warning, when it falls outside them. A symlink or .. climb is caught because the check runs after canonicalization. WI-1131
The fork field is required on Config, and the sample builds a Config literal by hand. Default it off so the workspace check compiles. WI-1131
cargo fmt -p codex-home. No behaviour change. WI-1131
The let binding existed only to be returned, which clippy's let_and_return lint flags, and the gate lints every file this PR touches. WI-1131
`project_doc_mode = "all"` keeps every matching filename in a directory instead of the first, then `.claude/rules/*.md` in sorted order, and expands line-only @path imports. Anything else, including an unset value, keeps the upstream first-match behaviour. Imports and rules are read through the executor filesystem and refused once they canonicalize outside the project root, so a repo cannot pull in an absolute path, a ~/ reference, a .. climb or a symlink that escapes. The untrusted-project skip still runs first, so none of this is read for an untrusted project. WI-1132
cargo fmt -p codex-core. No behaviour change. WI-1132
The fixture lagged the new config field, so config_schema_matches_fixture failed.
failClosed is opt-in and defaults to false. When set, every PreToolUse failure mode (non-0/2 exit, missing status, spawn failure, timeout, signal, invalid JSON, unreadable output) blocks the tool with a reason the model sees. Handlers without the flag still fail open. A non-empty write_stdin is typed into the open shell, so it now runs PreToolUse as tool_name Bash with the stdin text as command. Empty polls stay unreviewed. A block stops the write before it reaches the process.
An async handler that sets failClosed now runs synchronously, with a warning, instead of failing open silently. The block can only be applied before the tool starts, so skipping the hook would leave no guard. A non-empty write_stdin payload now carries the exec process's start cwd as workdir, looked up by session_id, or null when the process is gone. A cd typed into the shell is not tracked, and the hook config documents that MCP tool hooks and hooks that never run cannot fail closed.
The write_stdin hook reads the process store with try_lock, so a busy store reports workdir as null instead of blocking a runtime worker. The previous lookup spawned a task and joined it from another thread, which deadlocked on a current-thread runtime and panicked outside one. Also fills the two HookHandlerConfig literals CI found in the tui and app-server tests.
cargo fmt wants the HookToolName import grouped with the other tool imports and the long json! argument wrapped.
cargo fmt reflows the fail-closed condition, the async warning assertion, and the timeout test call, and orders the test imports.
ConfiguredHookHandler gained failClosed, so the vendored TypeScript, JSON schema, precomputed export and Python SDK type are regenerated from it.
The experimental precomputed export embeds ConfiguredHookHandler and still omitted failClosed.
The config.toml schema embeds the command hook handler, which now carries failClosed.
An optional output_schema on spawn_agent v1 and v2 is copied into the child's TurnStartOptions.final_output_json_schema. The tool schema accepts any JSON value and the child is not re-validated here.
fail_closed belongs to the fail-closed hook change and is not on fork/main, so this branch must build without it.
HookHandlerConfig::Command gained fail_closed in the fail-closed work. The session test built the command variant without it, so cargo check failed with E0063. The field's serde default is false (fail open), so the fixture uses that. Also drop the two unused imports cargo check warned about.
e3b630e to
52badca
Compare
cargo 1.95 --locked rejects fork/main's Cargo.lock as soon as a member manifest depends on a package the base lock already resolves but that member did not list. The comparison only diffs the resolve, and --offline keeps it from moving a version.
The combined branch's spawn_agent/collaboration schema changes the catalog namespace hashes the scenario snapshots render, and adds project_doc_mode to the config schema. Regenerated config.schema.json with codex-write-config-schema and updated the 22 snapshots from the gha-spot-1 nextest output.
expand_imports treated a path canonicalize could not find as "not an import", so the confinement never saw it. On a host without /etc/hostname the @/etc/hostname import in imports_outside_the_allowed_roots_are_refused was dropped instead of refused. A missing path is now resolved through its nearest existing ancestor and refused unless that lands inside an allowed root.
… shares fork-ci run 38089490933 ran unmodified fork/main on gha-spot-1 with codex-cli, codex-core, codex-exec-server and codex-app-server forced, and these 24 tests failed there too. They are the runner, not the branch.
|
Blockers (head f8bd418) Epic lead. These 29 commits on fork/main df2fb72 were reviewed by Fail-closed (#6). All 8 commits are identical (
The new Other features:
Imports fix Generated files: Blockers: known-failures entries checked against run 38089490933That run (fork/main df2fb72, workflow_dispatch) reports 8103 run, 18 failed, 6 flaky. I matched every entry to the run's final outcomes (
Merge condition, besides the above: fork-ci fully green on the fixed head, including the landlock test once gha-spot-1 has bwrap. Non-blocking:
|
The six daemon/remote tests were FLAKY in run 38089490933 and passed on retry, so nextest retries=1 already covers them. The hooks permission test was green on the baseline and this branch changes hook semantics, so it must run. The other three timing-flake lines had no baseline failure behind them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thedancingdeveloper
left a comment
thedancingdeveloper
left a comment
There was a problem hiding this comment.
Re-review of the two blockers from comment 6103808118, at head 222490f (base fork/main).
Diff since the reviewed head f8bd418. One commit, 222490f496 fork-ci: stop ignoring flaky and hook tests without a baseline failure. It touches only .github/fork-ci-known-failures.txt: 10 deletions, 0 insertions, no other file. The branch's diff against fork/main is otherwise unchanged (89 files, same set), so I did not re-review the rest of the branch.
Blocker 1 (six FLAKY-not-failed daemon entries). All six lines are gone:
bedrock_onboarding_leaves_a_running_daemon_untouchedincompatible_daemon_can_run_embeddedstopped_daemon_requires_confirmation_to_disable_persisted_featuresfresh_daemon_requires_confirmation_to_disable_shared_featuresdaemon_worktree_start_and_fork_bind_owner_before_turnremote_process_preserves_empty_workspace_roots
Blocker 2 (unbacked timing-flake entries). All four lines are gone:
hooks::permission_request_hook_allows_exec_command_without_user_approvalpending_input_persistence::background_user_inputpending_input_persistence::synchronous_storeremote_env::future_pending_environment
Baseline kept. 18 test(...) entries remain (28 before): the 14 codex-cli daemon tests and the 4 codex-exec-server shell_snapshot* tests that were final failures in run 38089490933. None of the ten removed names appears anywhere in the file at this head. The header comments are untouched.
fork-ci on this head (run 38193938975) is still in progress at the time of this review. The merge condition from the previous review stands: fork-ci green on this head, with hooks::permission_request_hook_allows_exec_command_without_user_approval now actually running and passing. That is a merge gate, not a blocker on this diff.
Review: no blockers (head 222490f)
1 participant
Combines the six open fork PRs into one branch so fork-ci runs once on the single runner. Commits are cherry-picked in merge order, not squashed. The per-PR scenario snapshot-accept commit from #10 is omitted; snapshots will be re-accepted once from this combined run.
#16 fork/ci-lines
#14 fork/skills-roots
#13 fork/instructions-directory
#12 fork/instructions-global
Shared the instruction commits already brought in by #13. Only its unique commit is added:
#6 fork/wi-1129-fail-closed
#10 fork/wi-1130-output-schema
Cherry-picks applied cleanly; no conflicts. The six original PRs stay open until this merges.