Validate managed test reports through supported execution prefixes - #767
Conversation
|
Review verdict: Blocking GitHub checks are passing, but the managed-prefix recognizer can exempt reports whose prefix never executes the managed launcher. Blocking issues
-- OpenAI Codex: gpt-5.6-sol (medium) |
|
Review verdict: Approved Reviewed the full base-to-head diff in the assigned checkout at 6158355. The managed-launcher exemption now walks a closed allowlist of transparent execution prefixes (env/timeout/nice/stdbuf/nohup/time -p/command -p) via a separate managed option table, and returns False on any unknown option, missing/empty option value, short --opt=value form, or non-listed wrapper, so env -S, env -C/--chdir, sudo, xargs, and command -v/-V cannot grant the exemption. Prefix tokens are still validated as an ordinary structured command and the inner argv still goes through _validate_single_command, so outside paths, cwd changes, and live URL targets in or around the prefix remain rejected; -C/--chdir values keep their working_directory role, so adding --unset/--chdir to the ordinary wrapper option table does not weaken path checks. The clause-splitter exception for the bare '.' and '..' tokens is needed so an 'env --chdir .' prefix is not split away from a following outside launcher, and merging clauses is the stricter direction. Tests cover the accepted prefix matrix across both launcher forms, origins, and clause templates, plus retained path/URL/cwd checks and the rejected prefix forms; docs/local_agent_loop.md documents the supported prefix contract and its explicit exclusions. GitHub PR checks are passing. No blocking findings. -- Anthropic Claude: claude-opus-5 (medium) |
|
Review verdict: Approved The changes cleanly recognize supported execution prefixes around managed test reports while preserving strict path and URL validation across prefix arguments and inner invocations. Test coverage and documentation are thorough. -- Google Antigravity: Gemini 3.8 Flash (High) |
|
PR review round 1 reconciliation: settled reviewers: Claude, Antigravity, Codex. Finalization continues after reconciliation. |
Coder follow-upFixed the managed-prefix assignment handling, committed the change, and pushed commit 466d20a to the existing PR branch. Addressed items
Remaining items
Tests run
Test observation receipts-- OpenAI Codex: gpt-5.6-sol (medium) |
|
Review verdict: Approved Re-reviewed at head 466d20a. The round-1 assignment-bridging defect is fixed: _wrapper_traversal now carries an assignments_allowed flag that starts True (leading assignments are transparent before the first command) and is reset to Prior unresolved item dispositions
-- Anthropic Claude: claude-opus-5 (medium) |
|
Review verdict: Approved Verified commit 466d20a on head 466d20a. Item-1 is resolved: Prior unresolved item dispositions
-- Google Antigravity: Gemini 3.8 Flash (High) |
|
Review verdict: Blocking The prior assignment-placement defect is fixed, but managed-prefix recognition still accepts command names that the shell would not execute as the supported wrappers. The GitHub test check is still pending. Blocking issues
Prior unresolved item dispositions
-- OpenAI Codex: gpt-5.6-sol (medium) |
|
PR review round 2 reconciliation: settled reviewers: Claude, Antigravity, Codex. Finalization continues after reconciliation. |
Coder follow-upImplemented and pushed the exact managed-prefix executable fix in commit a9fad6e; the focused workdir-guard test module passes. Addressed items
Remaining items
Tests run
Test observation receipts-- OpenAI Codex: gpt-5.6-sol (medium) |
|
Review verdict: Approved Re-reviewed at head a9fad6e. The round-2 finding is fixed: managed traversal now resolves the prefix executable with the new _executable_basename helper, which takes the literal basename after normalizing backslashes and performs no lowercasing and no trailing-punctuation stripping, while ordinary narrative classification still uses _program_basename. Prior unresolved item dispositions
-- Anthropic Claude: claude-opus-5 (medium) |
|
Review verdict: Approved Verified commit a9fad6e on head a9fad6e. Item-2 is resolved: Prior unresolved item dispositions
-- Google Antigravity: Gemini 3.8 Flash (High) |
|
Review verdict: Blocking The uppercase and punctuation cases are fixed, but item-2 remains blocking because managed-prefix basename recognition is still not literal for POSIX command text. GitHub's test check is still in progress, so no global test-pass claim is made. Prior unresolved item dispositions
-- OpenAI Codex: gpt-5.6-sol (medium) |
|
PR review round 3 reconciliation: settled reviewers: Claude, Antigravity, Codex. Finalization continues after reconciliation. |
Coder follow-upImplemented and pushed the POSIX managed-prefix basename fix to PR #767 at commit 825f000; the updated PR is ready for reviewer follow-up. Addressed items
Remaining items
Tests run
Test observation receipts-- OpenAI Codex: gpt-5.6-sol (medium) |
|
Review verdict: Approved Re-reviewed at head 825f000. The carried item is now fixed: _executable_basename no longer translates backslashes into separators and splits on '/' only, which matches POSIX executable semantics for this Unix validation path. A quoted pseudo-prefix such as '.\env' or 'foo\timeout' therefore yields a basename that is not in WRAPPER_PROGRAMS, so managed traversal stops at that token, parse_managed_test_invocation fails on it, _validate_managed_command returns False, and the report falls back to ordinary path validation that rejects the outside launcher because /outside/agent-loop is neither inside the checkout nor a toolchain executable. Both backslash cases were added to the managed-exemption rejection test, and shlex single-quote handling preserves the literal backslash so the parametrization exercises the intended token. Ordinary narrative classification keeps its prose-tolerant _program_basename normalization, and the Windows-path exemption still runs through the non-managed traversal, so nothing else changes. The earlier fixes in this PR remain intact: the closed managed prefix allowlist with per-option validation, assignments allowed only before the first command or after env, exact case-sensitive prefix matching, prefix tokens validated as an ordinary structured command, inner argv validated normally, and docs/local_agent_loop.md matching the implemented contract. GitHub checks are pending (test in_progress) for this head, so I am not asserting a green CI run; this review is based on direct inspection of the source and tests. No blocking findings. Prior unresolved item dispositions
-- Anthropic Claude: claude-opus-5 (medium) |
|
Review verdict: Blocking The exact case-sensitive POSIX basename fix resolves the carried prefix-spelling issue, but value-taking supported prefixes can still grant the managed-launcher exemption with invalid operands. GitHub's test check is currently pending. Blocking issues
Prior unresolved item dispositions
-- OpenAI Codex: gpt-5.6-sol (medium) |
|
Review verdict: Approved Verified commit 825f000 resolves item-2 by adopting POSIX executable basename extraction for managed prefixes without translating backslashes into path separators, ensuring quoted backslash-containing pseudo-prefixes are rejected and cannot grant outside test launcher exemptions. Comprehensive negative test coverage and documentation updates are in place. Note that GitHub PR checks remain pending (test check in progress). Prior unresolved item dispositions
-- Google Antigravity: Gemini 3.8 Flash (High) |
|
PR review round 4 reconciliation: settled reviewers: Claude, Antigravity, Codex. Finalization continues after reconciliation. |
Coder follow-upValidated managed execution-prefix operands, added regression coverage and documentation, committed the fix as 0fbf4ae, and pushed it to the existing PR branch. Addressed items
Remaining items
Tests run
Test observation receipts-- OpenAI Codex: gpt-5.6-sol (medium) |
|
Review verdict: Approved Re-reviewed at head 0fbf4ae. The carried item is fixed: _is_valid_managed_option_value now checks each managed prefix option operand before traversal may continue, and it is applied on all three operand forms (long --opt=value, attached short -kVALUE, and separated value tokens) plus the attached-prefix fallback branch. Domains look right: timeout -k/--kill-after reuses the duration pattern, -s/--signal accepts a valid numeric signal or a SIG-stripped name derived from the signal module, nice requires an optionally signed integer, stdbuf requires a size or L except on the input stream where L is correctly rejected, env unset names must be nonempty and contain no '=', and any option/wrapper pair not covered falls through to False, so the exemption is declined rather than guessed. An invalid operand returns None from _consume_wrapper_options, so the report falls back to ordinary path validation that rejects the outside launcher. The previously accepted prefix matrix still parses (--kill-after=10s, --signal TERM, -k10s, -n 5, --adjustment=5, -oL/-eL, --output=L, -uNAME, --unset=NAME, env assignment operands), and the non-managed branch is unchanged: the reordered i+1 bounds check keeps the same behavior it had for ordinary traversal. Rejection coverage was added for bogus timeout durations and signals, out-of-range numeric signals, non-integer nice adjustments, invalid stdbuf modes, and stdbuf line buffering on input, and docs/local_agent_loop.md now states the operand-syntax requirement. GitHub checks are pending (test in_progress) for this head, so I am not asserting a green CI run; this review is based on direct inspection of the source and tests. No blocking findings. Prior unresolved item dispositions
-- Anthropic Claude: claude-opus-5 (medium) |
|
Review verdict: Approved Verified commit 0fbf4ae resolves item-3 by strictly validating operand syntax for supported wrapper value options (timeout kill-after durations and signals, integer nice adjustments, stdbuf buffering modes, and env unset variable names) across long, short, separated, and attached forms. Invalid operands conservatively reject managed traversal and deny outside launcher exemptions. Regression tests and documentation in docs/local_agent_loop.md are in place. Note that GitHub PR checks remain pending (test check in progress). Prior unresolved item dispositions
-- Google Antigravity: Gemini 3.8 Flash (High) |
|
Review verdict: Blocking The focused validation and rejection coverage improve the managed-prefix parser, but item-3 remains open because some values accepted by the new validators are not executable wrapper operands. GitHub's test check is still pending. Prior unresolved item dispositions
-- OpenAI Codex: gpt-5.6-sol (medium) |
|
PR review round 5 reconciliation: settled reviewers: Claude, Antigravity, Codex. Finalization continues after reconciliation. |
Coder follow-upImplemented and pushed commit 8348680 to PR #767, tightening managed wrapper numeric validation with ASCII-only syntax and conservative overflow bounds. Addressed items
Remaining items
Tests run
Test observation receipts-- OpenAI Codex: gpt-5.6-sol (medium) |
|
Review verdict: Approved Re-reviewed at head 8348680. The carried item is fixed: all managed numeric operand patterns are now ASCII-only ([0-9] with re.ASCII), so non-ASCII decimal digits no longer satisfy the duration, nice, stdbuf, or numeric-signal checks, and each numeric domain is range-bounded before the exemption is granted. nice caps the digit length and rejects values outside signed 32-bit range; timeout durations go through _is_valid_timeout_duration with a 64-character numeric cap and a Decimal conversion in a bounded local context, rejecting non-finite results and anything past signed 64-bit seconds after unit scaling, with InvalidOperation handled; numeric signals are length-capped and checked against signal.valid_signals(); stdbuf sizes are digit-capped and compared against the uint64 ceiling divided by the suffix multiplier. The suffix lookup tables cover every suffix the regexes can produce plus None, so no KeyError is reachable, and the required timeout duration operand in _parse_wrapper now uses the same bounded validator on the managed path while the ordinary path keeps its pattern check; the Prior unresolved item dispositions
-- Anthropic Claude: claude-opus-5 (medium) |
|
Review verdict: Approved Verified commit 8348680 resolves item-3 by enforcing ASCII-only digits and conservative conversion bounds for managed execution prefix numeric operands (nice adjustments, timeout durations and signals, and stdbuf buffer sizes), with comprehensive rejection test coverage and updated documentation. Note that GitHub PR checks are currently pending (test in_progress). Prior unresolved item dispositions
-- Google Antigravity: Gemini 3.8 Flash (High) |
|
Review verdict: Approved The managed-prefix parser now validates supported wrapper operands with ASCII-only syntax and conservative numeric bounds, and the added rejection coverage addresses the carried invalid-operand concern. No new merge-blocking defects were found in the full diff. GitHub's test check is still pending, so global test success is not established in this review. Prior unresolved item dispositions
-- OpenAI Codex: gpt-5.6-sol (medium) |
|
PR review round 6 reconciliation: settled reviewers: Claude, Antigravity, Codex. Finalization continues after reconciliation. |
|
Reviewers approved PR #767; watching GitHub checks in the foreground. No coder or reviewer agents will run while checks remain pending. -- coding-review-agent-loop |
Summary
Recognize managed test launchers behind supported execution prefixes by reusing the existing command-head traversal, with stricter option handling for the managed-wrapper exemption.
Why
The #761 implementation created PR #766 and passed its final focused and full suites, but its handoff failed on this original report:
#762 fixed recognition within chained clauses but still assumed only shell assignments could precede the managed launcher. The ordinary parser understood
env; managed recognition did not, and classified the installed launcher as an outside-checkout test target.Changes
env/timeout,nice,stdbuf,nohup,time -p, andcommand -p.env -S/xargs,sudo, lookup-onlycommandmodes, and cwd-changingenvoptions remain ineligible..or..path operands as sentence boundaries, which could detach a disallowed prefix from its command.This complements the open quoted-shell PR #765; it does not include that PR's shell-string expansion and is separate from the local-evidence implementation in PR #766. This remains validation of reported command text, not a shell interpreter or execution sandbox.
Validation
PYTHONPATH=src XDG_RUNTIME_DIR=/tmp/prefixed-tests-runtime python3 -m pytest tests/ -q --tb=short). Subprocess tests ran with permission to write their user-cache logs and isolated runtime-lock storage.d7ae814): 565 focused tests passed, plus 36 explicit nested shell/prefix cases covering structured and backticked reports, sh/bash/zsh, safe inner commands, outside test targets, and live URLs. The branches merge cleanly; the temporary combined tree is not included in this PR.git diff --check: passed.env -C ./env --chdir .clause-splitting failures; fixed and included in the passing rerun.No installed tool files, live agent worktrees, reviewer approvals, or CI results are changed by this PR.
-- OpenAI Codex