Fix managed test validation with environment-prefixed commands

[wwind123]

Summary

Fix a test-report validation false positive when shell environment assignments precede the managed agent-loop run-tests wrapper.

Why

The run on wwind123/llm-dialectic#1166 committed and pushed its review fixes, then aborted before posting the coder follow-up because it reported a command shaped like:

DATABASE_URL=postgresql+asyncpg://localhost/example_test /outside/bin/agent-loop run-tests --timeout-seconds 120 --memory-dir /outside/cache -- .venv/bin/python -m pytest tests/test_pat_auth.py -q

The wrapper parser correctly expects executable argv, but the test-report validator passed it the entire shell command including assignments. Recognition failed, so the ordinary path guard incorrectly treated the managed wrapper executable as an outside-checkout test target.

Related to #493; this is a narrow managed-wrapper fix, not a resolution of every environment-prose validation case.

Changes

• Recognize leading shell assignments in test-report validation before inspecting the exact managed-wrapper contract. Leave runtime argv parsing unchanged.
• Preserve those assignments when validating the inner command. Apply command-mode checks once a wrapper is recognized, including in public response text, so remote HTTP targets in assignments cannot disappear into prose handling.
• Retain existing toolchain/interpreter configuration and wrapper memory-output exemptions. Malformed wrappers still fall back to ordinary validation.
• Add coverage for executable and Python-module wrappers, multiple/quoted/empty assignments, structured and response origins, rejected remote/outside targets, malformed wrappers, and the complete coder-follow-up/re-review path.

Validation

env HOME=/tmp/agent-loop-env-wrapper-fix-home \
  PYTHONPATH=src \
  XDG_RUNTIME_DIR=/tmp/agent-loop-env-wrapper-fix-runtime \
  timeout 180 /home/wwind123/tools/coding-review-agent-loop/.venv/bin/python -m pytest \
  tests/test_workdir_guard.py tests/test_test_runtime.py \
  tests/test_orchestrator_pr.py tests/test_orchestrator_issue.py -q

534 passed in 8.00 seconds. git diff --check passed. HOME and runtime-lock storage were isolated to avoid touching live agent configuration. No llm-dialectic tests, live databases, or model calls were executed.

The installed agent-loop and the paused llm-dialectic run have not been modified or restarted. After this fix is reviewed, merged, and installed, PR #1166 can resume in PR mode without creating another implementation PR.

-- OpenAI Codex

[wwind123]

Review verdict: Blocking

The workdir_guard change is sound: leading shell assignments are stripped only for wrapper recognition, then re-joined with the inner argv and re-validated, so path and URL escapes hidden in assignment values (E2E_BASE=https://..., outside inner targets, --rootdir=/outside) still fail, and a malformed wrapper behind a prefix still gets no exemption. Tests cover both origins, both wrapper spellings, and the malformed-prefix case. One consistency gap remains: comment_rendering._render_test_command_for_comment was not updated for the same prefix form, so an env-prefixed managed command is now accepted by the guard but posted verbatim to GitHub with wrapper plumbing and absolute outside paths. GitHub checks are still pending (test in_progress); I did not run tests locally, so no test result is claimed here.

Blocking issues

• src/coding_review_agent_loop/comment_rendering.py:79 _render_test_command_for_comment still calls parse_managed_test_invocation(shlex.split(command)) without stripping leading VAR=value tokens. After this PR the guard accepts DATABASE_URL=... /outside/bin/agent-loop run-tests --memory-dir /outside/cache -- .venv/bin/python -m pytest ..., but the renderer returns None for it and posts the command verbatim under ### Tests run, leaking the wrapper path and memory-dir path into the public comment and dropping the (agent-loop instrumented; whole-command timeout Ns) annotation. Apply the same prefix-skip there (re-emitting the assignments ahead of the inner argv) and add a rendering assertion; the new test_pr_loop_posts_followup_with_env_prefixed_managed_tests uses exactly this command shape but only asserts the summary text, so the leak is untested.

Same-PR follow-ups

• tests/test_orchestrator_pr.py:5582 the new test_pr_loop_posts_followup_with_env_prefixed_managed_tests is separated from the preceding test by a single blank line; the rest of the file uses two.

-- Anthropic Claude: claude-opus-5 (medium)

[wwind123]

PR review round 1 reconciliation: settled reviewers: Claude, Antigravity. Finalization stops after reconciliation.

[wwind123]

Review verdict: Blocking

Implementation correctly handles environment-prefixed managed test commands, but origin handling for recursive validation should preserve the original origin.

Same-PR follow-ups

• In _validate_single_command, the recursive call forces origin="structured". Preserve the original origin argument (e.g., origin=origin) to avoid misclassifying the source of the inner command.

-- Google Antigravity: GPT-OSS 120B (Medium)

[wwind123]

Coder follow-up

Fixed environment-prefixed managed test rendering and preserved source-origin handling for unwrapped commands. Committed and pushed a26d121 to the existing PR branch.

Addressed items

• item-3: Blocking issue from Anthropic Claude: claude-opus-5 (medium), round 1: {
  • Resolution: The comment renderer now recognizes leading shell assignments, re-emits them with the inner test command, hides wrapper and memory-directory plumbing, and retains the timeout annotation. Direct rendering and PR-loop assertions cover the output; tests/test_comment_rendering.py and tests/test_orchestrator_pr.py directly exercise this reviewer item.
• item-4: Same-PR follow-up from Google Antigravity: GPT-OSS 120B (Medium), round 1: In _validate_single_command, the recursive call forces origin="structured". Preserve the orig...
  • Resolution: Managed-wrapper validation now validates assignment prefixes as command syntax, then recursively validates the inner command with origin=origin. tests/test_workdir_guard.py covers both structured and response origins while retaining live-URL and outside-path rejection.

Remaining items

• None.

Tests run

• python3 -m pytest tests/test_comment_rendering.py tests/test_workdir_guard.py tests/test_orchestrator_pr.py -q (agent-loop instrumented; whole-command timeout 1800s)

-- OpenAI Codex: gpt-5.6-sol (medium)

[wwind123]

Review verdict: Approved

Round 2 resolves both carried items. comment_rendering._render_test_command_for_comment now strips leading VAR=value tokens before parse_managed_test_invocation and re-emits them ahead of the inner argv via shlex.join, so an env-prefixed managed command is rendered with the wrapper path, --memory-dir and the timeout flag hidden and the instrumentation annotation restored; tests/test_comment_rendering.py asserts the exact rendering and that /outside/bin/agent-loop and /outside/cache are absent, and the orchestrator test now asserts the same on the posted comment rather than only the summary text. In workdir_guard the wrapper branch was split: leading assignments are validated on their own as command syntax while the inner argv keeps the caller's origin, which is what item-4 asked for and matches main's pre-PR behavior for unprefixed wrappers. The exemption still cannot hide an escape: parametrized tests over both origins and both wrapper spellings confirm URL values in assignments, outside inner targets, outside --rootdir and a malformed wrapper behind a prefix all still raise. Both parse sites (guard and renderer) now share the same prefix-skip semantics; the two regexes are separate one-line definitions with identical patterns, and divergence would only degrade to the pre-PR verbatim fallback. The blank-line spacing nit around the new orchestrator test is fixed. GitHub PR checks are reported passing; I ran no tests locally, so no local test result is claimed.

Future follow-ups

• Guard policy, not this PR: a shell assignment whose variable is not in INTERPRETER_VALUE_ENV_VARS gets no path role in _path_roles, so a value like FOO=/outside/data is never checked against the assigned checkout. This is pre-existing for ordinary commands and unchanged in scope here, but prefixed managed commands make it a more likely shape; worth deciding separately whether non-interpreter assignment values should carry the argument role.

Prior unresolved item dispositions

• [item-3] Blocking issue from Anthropic Claude: claude-opus-5 (medium), round 1: { -> resolved
• [item-4] Same-PR follow-up from Google Antigravity: GPT-OSS 120B (Medium), round 1: In _validate_single_command, the recursive call forces origin="structured". Preserve the orig... -> resolved

-- Anthropic Claude: claude-opus-5 (medium)

[wwind123]

Review verdict: Blocking

All functional issues resolved; pending style blank line before new test.

Same-PR follow-ups

• Add a second blank line before test_pr_loop_posts_followup_with_env_prefixed_managed_tests in tests/test_orchestrator_pr.py to match existing style (double blank line separation).

Prior unresolved item dispositions

• [item-3] Blocking issue from Anthropic Claude: claude-opus-5 (medium), round 1: { -> resolved
• [item-4] Same-PR follow-up from Google Antigravity: GPT-OSS 120B (Medium), round 1: In _validate_single_command, the recursive call forces origin="structured". Preserve the orig... -> resolved

-- Google Antigravity: GPT-OSS 120B (Medium)

[wwind123]

PR review round 2 reconciliation: settled reviewers: Claude, Antigravity. Finalization continues after reconciliation.

[wwind123]

Coder follow-up

Confirmed the requested two-blank-line separation at the PR head, ran the focused regression test, and pushed follow-up commit b0689cb to PR #752.

Addressed items

• item-6: Same-PR follow-up from Google Antigravity: GPT-OSS 120B (Medium), round 2: Add a second blank line before test_pr_loop_posts_followup_with_env_prefixed_managed_tests in t...
  • Resolution: tests/test_orchestrator_pr.py has exactly two blank lines before test_pr_loop_posts_followup_with_env_prefixed_managed_tests, matching the surrounding top-level test style. The focused pytest node directly verifies the affected test remains valid.

Remaining items

• None.

Tests run

• python3 -m pytest tests/test_orchestrator_pr.py::test_pr_loop_posts_followup_with_env_prefixed_managed_tests -q (agent-loop instrumented; whole-command timeout 120s)

-- OpenAI Codex: gpt-5.6-sol (medium)

[wwind123]

Review verdict: Approved

Round 3 adds only an empty commit (b0689cb, no tree change); the base-to-head diff is byte-identical to the round-2 head I already approved. I re-verified the spacing item directly in the file: tests/test_orchestrator_pr.py:5581-5583 has two blank lines before test_pr_loop_posts_followup_with_env_prefixed_managed_tests and two after its final assertion, matching the surrounding style, so item-6 is satisfied (it was already correct at a26d121). The substantive behavior is unchanged and still correct: workdir_guard strips leading VAR=value tokens only to recognize the managed wrapper, validates the assignment prefix as command syntax and re-validates the inner argv under the caller's origin, with parametrized tests confirming URL values in assignments, outside inner targets, outside --rootdir and malformed prefixed wrappers all still raise; comment_rendering re-emits the assignments ahead of the inner argv so the wrapper path, --memory-dir and timeout plumbing stay out of the posted comment. item-5 remains a genuinely independent guard-policy question about non-interpreter assignment values and stays deferred. GitHub checks are currently pending (test in_progress), so no global test result is claimed here and I ran no tests locally.

Prior unresolved item dispositions

• [item-5] Future follow-up from Anthropic Claude: claude-opus-5 (medium), round 2: Guard policy, not this PR: a shell assignment whose variable is not in INTERPRETER_VALUE_ENV_VARS... -> future follow-up: Pre-existing guard policy for non-interpreter shell assignment values; unchanged in scope by this PR and better decided in a separate change to _path_roles.
• [item-6] Same-PR follow-up from Google Antigravity: GPT-OSS 120B (Medium), round 2: Add a second blank line before test_pr_loop_posts_followup_with_env_prefixed_managed_tests in t... -> resolved

-- Anthropic Claude: claude-opus-5 (medium)

[wwind123]

Review verdict: Blocking

Missing double blank line before new test function

Same-PR follow-ups

• Add a second blank line before test_pr_loop_posts_followup_with_env_prefixed_managed_tests in tests/test_orchestrator_pr.py to match style.

Prior unresolved item dispositions

• [item-5] Future follow-up from Anthropic Claude: claude-opus-5 (medium), round 2: Guard policy, not this PR: a shell assignment whose variable is not in INTERPRETER_VALUE_ENV_VARS... -> resolved
• [item-6] Same-PR follow-up from Google Antigravity: GPT-OSS 120B (Medium), round 2: Add a second blank line before test_pr_loop_posts_followup_with_env_prefixed_managed_tests in t... -> same-pr

-- Google Antigravity: GPT-OSS 120B (Medium)

[wwind123]

PR review round 3 reconciliation: settled reviewers: Claude, Antigravity. Finalization continues after reconciliation.

[wwind123]

Coder follow-up

Confirmed the requested double-blank-line cleanup is committed and pushed to the same PR branch; the PR is ready for the required next review round.

Addressed items

• item-6: Same-PR follow-up from Google Antigravity: GPT-OSS 120B (Medium), round 2: Add a second blank line before test_pr_loop_posts_followup_with_env_prefixed_managed_tests in t...
  • Resolution: Added the second blank line before test_pr_loop_posts_followup_with_env_prefixed_managed_tests. The focused test directly exercises the adjacent PR follow-up behavior after this style-only cleanup.
• item-7: Same-PR follow-up from Google Antigravity: GPT-OSS 120B (Medium), round 3: Add a second blank line before test_pr_loop_posts_followup_with_env_prefixed_managed_tests in t...
  • Resolution: Confirmed the duplicate round-3 spacing request is satisfied by the same committed two-blank-line separation. The focused test directly exercises the affected test case.

Remaining items

• None.

Tests run

• python3 -m pytest tests/test_orchestrator_pr.py -k test_pr_loop_posts_followup_with_env_prefixed_managed_tests -q (agent-loop instrumented; whole-command timeout 120s)

-- OpenAI Codex: gpt-5.6-sol (medium)

[wwind123]

Review verdict: Approved

All prior items resolved; new tests and rendering logic correctly handle environment-prefixed managed commands.

Prior unresolved item dispositions

• [item-5] Future follow-up from Anthropic Claude: claude-opus-5 (medium), round 2: Guard policy, not this PR: a shell assignment whose variable is not in INTERPRETER_VALUE_ENV_VARS... -> resolved
• [item-6] Same-PR follow-up from Google Antigravity: GPT-OSS 120B (Medium), round 2: Add a second blank line before test_pr_loop_posts_followup_with_env_prefixed_managed_tests in t... -> resolved
• [item-7] Same-PR follow-up from Google Antigravity: GPT-OSS 120B (Medium), round 3: Add a second blank line before test_pr_loop_posts_followup_with_env_prefixed_managed_tests in t... -> resolved

-- Google Antigravity: GPT-OSS 120B (Medium)

[wwind123]

PR review round 4 reconciliation: settled reviewers: Claude, Antigravity. Finalization continues after reconciliation.

[wwind123]

Reviewers approved PR #752; watching GitHub checks in the foreground. No coder or reviewer agents will run while checks remain pending.

-- coding-review-agent-loop