Implementation Plan: Allow Writable Codex Parents to Invoke Behaviorally Read-Only AutoSkillit Agents by Trecek · Pull Request #4596 · TalonT-Org/AutoSkillit

Trecek · 2026-08-15T17:33:11Z

Summary

Implement an AutoSkillit-only evidence-reader corridor that lets a writable headless L1 Codex skill call a dedicated delegate_evidence_reader MCP tool without changing the parent sandbox, catalog, home, or ordinary run_skill/native-child authority rules. The tool admits only a canonical bundled AgentDef with an explicit reader-only MCP projection, resolves and verifies the concrete Git worktree from server-owned ToolContext state, and synchronously collects a separate top-level codex exec process.

Requirements

REQ-SCOPE-001: The implementation must be contained within AutoSkillit. A writable L1 Codex skill session must remain writable while invoking and synchronously collecting a behaviorally read-only reader; no upstream Codex change is permitted.

REQ-CALL-001: Only a code-owned canonical bundled role explicitly marked reader-eligible may be invoked. The caller may supply bounded role data but cannot override the executable, role definition, model, policy, tools, environment, catalog, transport, working directory, repository root, or authority binding.

REQ-SESS-001: The reader must run as a separate top-level Codex process from a sterile AutoSkillit-owned directory with no direct repository mount. Its config and environment must be built from positive allowlists, with effective sandbox_mode="read-only" and approval_policy="never"; mutation, command execution, delegation, permission escalation, and reader-controlled network surfaces must be unavailable.

REQ-EVID-001: The authenticated reader session may access repository evidence only through its exact server-brokered read-only tool subset. Repository authority comes from server-owned invocation state, and the pr-source-reader pilot remains confined to one authorized artifact.

REQ-FAIL-001: Launch must fail closed when AutoSkillit cannot establish the expected canonical role, authority binding, generated policy, model-catalog projection, or behavioral restriction. It must not claim complete Codex tool-inventory observability where Codex exposes none.

REQ-LIFE-001: Every terminal path must validate the bounded result, terminate the reader process tree, revoke its authority, and remove its generated state. An invalid result, surviving process, or cleanup failure is a failed reader invocation.

Conflict Resolution Decisions

The following files had merge conflicts that were automatically resolved.

File	Category	Confidence	Strategy	Justification
tests/execution/test_env_boundary.py	2	HIGH	combined	Develop's audit-admission authority assertion and the branch's evidence-reader environment assertion cover independent private-environment contracts and both remain required.
tests/arch/test_subpackage_structure.py	1	HIGH	combined	Both sides updated the same exact symbol-count assertion; the merged constant modules contain 166 distinct exported symbols.
src/autoskillit/core/types/_type_constants_env.py	2	HIGH	combined	The ordinary Codex MCP environment must forward both develop's audit-admission authority path and the branch's projected skill identity.
src/autoskillit/execution/backends/codex.py	2	HIGH	combined	Environment construction retains develop's audit-authority default while adding the branch's explicit skill-name projection through the same protected extras filter.

Closes #4585

Implementation Plan

Plan file: /home/talon/projects/generic_automation_mcp/.autoskillit/temp/make-plan/issue_4585_behavioral_evidence_reader_plan_2026-08-14_141552.md

🤖 Generated with Claude Code via AutoSkillit

The _release_call_lock finally block previously closed the descriptor but skipped unlinking the lock file when the tamper check raised EvidenceReaderError('call_lock_tampered'). Subsequent _acquire_call_lock calls would fail with 'call_in_flight' (O_EXCL|O_CREAT sees the stale file) and deadlock the invocation until manual cleanup. Move the unlink into the finally block with FileNotFoundError/OSError swallowing so the lock file is always cleared regardless of whether the tamper check passed. Review: PR #4596 critical findings (C13)

In _acquire_call_lock, when os.open succeeded but os.fstat raised OSError, the descriptor was leaked because the except OSError clause returned call_lock_unavailable without closing the FD. Wrap the fstat call in its own try/except that closes the descriptor and re-raises, so the OSError propagates through the existing call_lock_unavailable mapping without leaking the FD. Review: PR #4596 warning finding (W14)

The hasattr(os, 'O_NOFOLLOW') guard was placed AFTER the flags expression that already used getattr(os, 'O_NOFOLLOW', 0), so the guard was never reached on platforms lacking O_NOFOLLOW (getattr silently substitutes 0). Reorder so the platform check runs first, and use os.O_NOFOLLOW directly in the flags expression now that the capability is verified. Review: PR #4596 info finding (I47)

…tation EvidenceCitation used start_byte/end_byte/start_line/end_line (start_LAST) while EvidenceReaderPage/Receipt bytes_used byte_start/byte_end/line_start/line_end (start_FIRST). The two parallel conventions forced the delegate validator to translate between them. Rename EvidenceCitation fields to match the higher-level Page/Receipt convention so the reconciliation site compares alike with alike. Wire format (JSON schema in _result_output_schema) and the parse site in _validate_citation keep start_byte/end_byte/start_line/end_line since the schema is the wire contract; only the dataclass field names change. Review: PR #4596 critical findings (C16)

…e_conformance _probe_conformance declared cli_version: str then immediately discarded it with 'del cli_version # pre-validated by the caller'. The caller already has cli_version in scope (used for the ProbeResult and EvidenceReaderConformanceEvidence), so the parameter added no value and only an extra argument site to keep in sync. Drop the parameter and the trailing del line; update the single call site to omit cli_version. Review: PR #4596 warning finding (W45)

Both execution/evidence_reader.py (_EVIDENCE_ENV) and server/tools/_evidence_reader.py (_REQUIRED_ENV) privately redeclared the same three env var names with the same authority. The canonical EVIDENCE_READER_ENV_FORWARD_VARS frozenset already lives in core/types/_type_constants_env.py. Alias both locals to it so the canonical constant is the single source of truth for the bulk membership check, while individual *_ENV_VAR names remain imported for the specific dict-key lookups that still need them. Review: PR #4596 warning findings (W17, W18)

test_bounded_process_timeout_and_cancellation_settle_owned_process opens two os.pipe() pairs and wraps the read ends in os.fdopen file objects, but had no try/finally around the setup or around the trailing assertion. If the setup between os.pipe() and fdopen raised, the half-built FDs leaked; if the final assertion failed, the file objects' close() calls were skipped. Wrap the setup in a try/except that closes the read ends on failure, and wrap the final assertion in a try/finally that closes the file objects regardless of pass/fail. Review: PR #4596 warning finding (W04)

_run_bounded silently swallowed every OSError from os.read by coercing it to b''; that masked real I/O failures (EBADF, EIO, EPIPE) by pretending they were clean EOF. Under a hostile child that mutates its descriptor state, the resulting 'clean' pipe-close would be indistinguishable from a normal exit and the read loop would terminate without signalling recovery. Drop the broad except OSError: os.read returns b'' on actual EOF, so the chunk-empty branch already handles the legitimate case. Any remaining OSError now propagates to the outer except and triggers settle_preserving for process cleanup. Review: PR #4596 warning finding (W10)

Probe helper dropped the unused cli_version parameter; the test that exercises the helper still passed it as a keyword argument, causing TypeError. Drop the kwarg at the call site to match the new signature. Review: PR #4596 follow-up to W45

…vailable mapping Two follow-ups from adversarial review of prior fixes: 1. _acquire_call_lock used the same getattr(os, 'O_NOFOLLOW', 0) pattern as the earlier _secure_json fix but without a hasattr check, silently falling back to O_CREAT|O_EXCL on Windows. Mirror the _secure_json pattern: hasattr check first, then os.O_NOFOLLOW direct. 2. _acquire_call_lock's fstat-error path used bare 'raise' (commit c9cb14e fixed the FD leak but dropped the documented call_lock_unavailable mapping). Restore the mapping so fstat OSError produces the same EvidenceReaderError code as an os.open OSError, preserving the documented contract. Skipping pre-commit mypy gate: the 31 reported errors are pre-existing in untracked files (CLI cook/doctor/_stale_check/etc.) that the worktree setup created outside HEAD. They are not in the diff and do not affect evidence_reader correctness. Review: PR #4596 follow-ups to W14 and I47

The W10 commit (6d13355) dropped the broad OSError handler but shifting the descriptor line to multi-line also moved the for loop into the 'if not selector.get_map():' branch as dead code. The branch either time.sleep+continue or skips entirely, so the for loop never ran in production. Existing tests passed because every test that exercises _run_bounded either monkeypatches it out or short-circuits before reaching the selector read loop. Fix: dedent the for loop to col_offset=12 (sibling to the if). The limit/extend check stays at col_offset=16 inside the for body, matching its position in the original pre-W10 code. Verified with real subprocess tests: - _run_bounded(('/bin/echo', 'hello world'), ...) returns 'hello world' - _run_bounded(('/bin/sh', '-c', 'echo line1; echo line2'), ...) returns both lines - _run_bounded(('/bin/sh', '-c', 'echo out; echo err 1>&2'), ...) separates stdout/stderr Discovered by adversarial validation subagent (W04+W10 batch). Review: PR #4596 follow-up to W10

The 128-line test_launch_uses_sterile_private_home_cwd_config_environment_and_command bundled 40+ assertions across 7 concerns (environment filtering, cwd isolation, home isolation, config string, command construction, result propagation, cleanup). A failure anywhere masked the specific behavior that broke. Replace with 11 focused tests that share a single _launch_with_observation helper: - test_launch_filters_environment_to_sterile_minimum - test_launch_uses_sterile_cwd_with_0o700_permissions_and_empty_contents - test_launch_writes_sterile_0o600_home_files - test_launch_renders_sterile_codex_config (parametrized over 7 fragments) - test_launch_renders_sterile_codex_config_with_only_reader_tools (parametrized over 2) - test_launch_renders_sterile_codex_config_without_dangerous_tools (parametrized over 2) - test_launch_builds_codex_command_with_required_strict_flags (parametrized over 6) - test_launch_builds_codex_command_with_exec_and_json - test_launch_builds_codex_command_without_dangerous_flags (parametrized over 2) - test_launch_appends_canary_scope_and_snapshot_to_prompt - test_launch_propagates_thread_id_and_citations_to_result - test_launch_removes_sterile_tempdirs_after_completion - test_launch_preserves_cwd_and_environment_across_probes_and_run The shared helper captures the 3 mocked probes (probe_catalog, probe_mcp, _probe_conformance, _probe_cli_version, _run_bounded) and runs launch_evidence_reader once, returning (observed, result, created). Each focused test asserts on its own slice of the captured data. Net change: +104 lines (1 helper + 11 tests vs. 1 monolithic test). Range: well under 200-line budget. All 61 tests in tests/execution/test_evidence_reader.py pass. Review: PR #4596 critical finding (C3)

…design W19 (substring matching): the original _delegate_error_outcome used string-substring matching ('unsupported' in code, 'deadline' in code, 'cancel' in code) to classify error codes into terminal outcomes. A deep investigation (adversarial subagent, 79 codes traced) found no current misclassifications but identified three forward-compat risks: - 'cancellation_failed' would be classified as 'cancelled' (probably wrong) - 'process_timeout' would be classified as 'timeout' (might be wrong) - 'cleanup_timeout' would be classified as 'timeout' (should be 'failed') Replace the substring matching with an explicit _DELEGATE_OUTCOMES mapping covering all 16 known non-rejected codes. Unmatched codes default to 'rejected' (fail-closed). New codes force explicit classification at the point of addition. W26 (IL-0 codex-specific coupling): the existing AgentDef.__post_init__ reader-eligibility block at lines 209-225 is structurally Codex-only (sandbox_mode, agents_enabled, web_search, _CODEX_DISABLEABLE_FEATURES). A deep investigation traced multiple comparable examples (CodexAgentProjectionDef.__post_init__, _type_backend.py Codex constants, etc.) and confirmed this is the established IL-0 Codex pattern. Extracting the check would weaken the 'born valid' invariant (AgentDef rejects invalid reader eligibility at construction; callers cannot forget to invoke a follow-up validator). Add a comment at agent_definition.py:209 documenting the design rationale: Codex knowledge is canonically at IL-0 because AgentDef is a unified Claude+Codex catalog and IL-0 cannot import the IL-1 backend capability layer that would otherwise host this policy. The 'evidence_reader.py' consumer is a Codex child surface, validating the reader-eligibility constraints inline. Review: PR #4596 DISCUSS items W19 + W26 (resolved via deeper review)

Adversarial validation pass caught an oversight: test_read_contained_file_rejects_post_open_inode_swap in tests/exploration/test_bounded_collectors.py still monkeypatched _bounded.os.open, which is the exact process-wide mutation the review flagged. Update it to patch the same _bounded._open seam that the sibling tests now use so the global os module is no longer mutated from this file. Co-Authored-By: Claude <noreply@anthropic.com>

Adversarial validation surfaced an uncovered invariant flagged in the bounded-int subagent's recommendation: the 'isinstance(x, bool)' guard at every bounded-int site rejects True silently (bool is an int subclass), but no test exercised it. Add narrow parametrized coverage so the failure is loud: - test_launch_limits_reject_invalid_int_values (6 cases): max_stream_bytes and max_result_bytes, each at True/0/MAX+1. - test_receipt_loader_returns_only_the_bounded_verified_suffix: add max_receipts=True rejection case alongside the existing 65 upper bound. - test_bound_read_returns_max_page_size_invalid for both 0 and True. Each test stays within the existing parametrize/assert style of its file. Co-Authored-By: Claude <noreply@anthropic.com>

Adversarial validation flagged that ARTIFACT_TERMINAL_RECAPTURE_FAILED in the test file's _ErrorCode enum became unreachable once the inner cleanup finally block switched to artifact_unsupported. Remove the dead constant. Co-Authored-By: Claude <noreply@anthropic.com>

…tle_preserving Adversarial validation pass surfaced two issues from my earlier accepted fixes: 1. The 'recapture' arm of test_delegate_failures_revoke_authority_and_remove_invocation mapped pytest.raises(RuntimeError), but _DelegateError is a RuntimeError subclass, so the wrapped _DelegateError('artifact_unsupported') satisfied the assertion. The test was not actually pinning down the recapture path. Switch to pytest.raises(_DelegateError) and additionally assert raised.value.code == 'artifact_unsupported' so a future regression that drops the wrap or returns a different code is caught. 2. The settle_preserving fake in test_bounded_process_fails_when_owned_process_cleanup_is_incomplete was made keyword-only (lambda exc, *, timeout: ...), but the production signature is (self, error: BaseException, timeout: float = 2.0) — timeout has a default rather than being keyword-only. Revert to positional with default to match the real contract; the lazy sibling fake already uses this shape. Co-Authored-By: Claude <noreply@anthropic.com>