fix(bench): plugin load path, single target branch, dual-stream failure artifacts

[bborbe]

Spec 004-bench-review-environment-control. Fixes the three defects that live runs exposed but 55 stubbed unit tests could not (D1, D3, D5). Test count 55 → 72; make precommit 30/30 green.

D1 — preflight verified a directory the review never loaded

The plugin-hash preflight hashed the marketplace clone, while Claude Code loads from plugins/cache/<name>/<name>/<version>/. Observed a MATCH across a 13-version gap — a result row could record a rule set the review never used, which silently invalidates every cross-configuration comparison the bench exists to make.

Now the check resolves the load path from the isolated config directory's own install record, and aborts by name when the record is missing, unparseable, points at a missing directory, escapes that config directory's cache tree, or applies to a different working directory. A pass prints the resolved directory, recorded version and content hash.

D5 — runs were non-deterministic

The working clone retained the upstream's real branches plus origin/HEAD, so the reviewer could second-guess the target it was handed and ask a clarifying question instead of reviewing. Both behaviours were observed from identical inputs.

The clone now carries exactly the checked-out head branch and the two synthetic remote-tracking branches for the PR — every upstream branch, tag and the default-branch pointer are removed on every run.

D3 — failure diagnosis used the wrong stream

Failure logs kept stderr only, but Claude Code writes its real errors to stdout (an expired OAuth session surfaced there while stderr carried incidental warnings). That discarded half caused a wrong diagnosis once already. Both streams are now preserved in one labelled artifact under bench/.cache/failures/, with empty streams marked as such.

Verification

• make precommit — 30/30 checks, 72 tests, green
• D1: install-record resolution unit-tested across all six abort conditions
• D5: post-clone ref set asserted by name, plus reachability after git gc
• D3: both streams asserted present and correctly labelled (an earlier draft of this test passed with the labels swapped — caught in prompt audit)

Not yet proven: three consecutive live runs demonstrating D5 determinism (spec AC18). That is operator-executable only and runs after merge.

[ben-s-pull-request-reviewer]

Now I have all the information needed. Let me compile the full review.

---

Step 4c-sel: CLASSIFY

Changed files:

bench/README.md
bench/run.py
bench/test_config.py
bench/test_resolve.py
bench/test_review.py
bench/testsupport.py
bench/testdata/... (prompts/specs)
CHANGELOG.md

Candidates from Step 4b-i:

• python-factory/zero-business-logic-in-factories — owner: python-architecture-assistant — trigger matches bench/testsupport.py
• changelog/conventional-prefix-required — owner: agent-auditor — trigger matches CHANGELOG.md
• changelog/unreleased-entry-required — owner: agent-auditor — trigger matches CHANGELOG.md
• changelog/preamble-frozen — owner: agent-auditor — trigger matches CHANGELOG.md

Classification:

rule_id	decision	reason (≤8 words)
python-factory/zero-business-logic-in-factories	applicable	make_* function has loop + subprocess calls
changelog/conventional-prefix-required	skipped	All four bullets already use fix: / docs:
changelog/unreleased-entry-required	skipped	## Unreleased section has four bullets
changelog/preamble-frozen	skipped	No change to preamble block detected

Applicable set: ["python-factory/zero-business-logic-in-factories"]

---

Step 4d-sel: ADJUDICATE

1. python-factory/zero-business-logic-in-factories (MUST) — python-architecture-assistant

Rule (from docs/python-factory-pattern.md): Factory functions (make_* / create_*) must contain zero business logic — no loops, conditionals, lambdas, or nested function definitions. Factories are pure composition: only constructor calls and object-tree wiring.

Finding at bench/testsupport.py:327:

def make_upstream_shaped_repo(path: pathlib.Path) -> dict:
    """Build a merge repo that also carries the refs a real upstream clone carries.
    ...
    """
    info = make_merge_repo(path)

    # Detect default branch name
    default_branch = subprocess.run(          # ← conditional: depends on git output
        ["git", "-C", str(path), "rev-parse", "--abbrev-ref", "HEAD"],
        ...
    ).stdout.strip()

    # Detect the merge commit SHA
    merge_sha = info["merge_sha"]

    # Add the upstream remote-tracking branches pointing at the merge commit
    for ref_name in [                         # ← for loop (business logic)
        "refs/remotes/origin/main",
        "refs/remotes/origin/feature/streaming-playback",
        ...
    ]:
        subprocess.run(...)

    subprocess.run(
        ["git", "-C", str(path), "symbolic-ref", ...],  # ← conditional: only runs
        check=True, capture_output=True, text=True,       #   when default_branch != "main"
    )
    ...

This is a make_* function that contains:

• A for loop iterating over domain data (ref_name list)
• A subprocess.run call whose result is used in a conditional subsequent call

This is business logic, not pure composition. Violation confirmed.

Severity: Must Fix (Critical) — factories with embedded logic are the exact pattern this rule exists to prevent, even in test-support code.

Fix: Extract the imperative ref-setup into a separate helper (e.g., _create_upstream_refs(path, merge_sha)) and call it from the factory. The factory itself should only wire the result of make_merge_repo into a pre-built structure, not drive git subprocesses in a loop.

---

Step 5: Consolidated Report

Must Fix (Critical)

• python-factory/zero-business-logic-in-factories — bench/testsupport.py:327 — make_upstream_shaped_repo is a make_* factory containing a for loop over ref_name list and conditional subprocess calls. Factory functions must be pure wiring — no loops or conditionals. Extract ref-setup to a separate function.

Should Fix (Important)

• None.

Nice to Have (Optional)

• None.

---

Selector Mode Traceability

• Candidates: 4 rules matched by Step 4b-i glob filter
• Applicable: 1 rule selected by Step 4c-sel
• Skipped:
  • changelog/conventional-prefix-required → all four CHANGELOG bullets already use correct fix:/docs: prefix
  • changelog/unreleased-entry-required → ## Unreleased section has four bullets
  • changelog/preamble-frozen → no change to preamble block

---

Verdict

The mechanical funnel found a genuine factory-pattern violation in a test-support file. The PR's three fixes (plugin load path, single-target-branch, dual-stream failure artifacts) are correctly implemented and documented. The changelog is properly formatted. No other rules apply.

{
  "verdict": "request-changes",
  "summary": "One MUST-tier factory violation confirmed in bench/testsupport.py:327 — make_upstream_shaped_repo contains a for-loop and conditional subprocess calls, violating the zero-business-logic rule for factory functions. The three core fixes (plugin load path resolution, single target branch guarantee, dual-stream failure artifacts) are correctly implemented and the CHANGELOG is properly formatted.",
  "comments": [
    {
      "file": "bench/testsupport.py",
      "line": 327,
      "severity": "critical",
      "message": "python-factory/zero-business-logic-in-factories: make_upstream_shaped_repo is a make_* factory containing a for-loop iterating over ref_name list and conditional subprocess calls. Factory functions must contain zero business logic — no loops, conditionals, lambdas, or nested function definitions. Extract the imperative ref-setup (the for loop and the default_branch-dependent symbolic-ref call) into a separate non-factory helper function."
    }
  ],
  "concerns_addressed": [
    "correctness: git clone branch stripping (D5) — prune_refs() correctly removes all upstream branches/tags/HEAD, verified in run.py:694-727",
    "correctness: dual-stream capture (D3) — failure_artifact_text() correctly labels stdout before stderr, verified in run.py:346-377",
    "security: plugin load path validation (D1) — six abort conditions implemented in run.py:769-950, out-of-tree guard uses path_is_under(), verified",
    "correctness: path resolution from install record (D1) — resolve_plugin_load_path() validates path stays under plugin_cache_root, verified"
  ]
}

superseded by new automated review

[ben-s-pull-request-reviewer]

Now I have a complete picture of the changes. Let me perform the Step 4c-sel classification and Step 4d-sel adjudication.

Step 4b-i candidate rules (judgment rules triggered by changed files):

The only rule with a matching trigger is changelog/conventional-prefix-required (trigger: CHANGELOG.md). No Go rules fire (changed files are Python/bench, not Go). No agent/command/skill rules fire (those dirs not changed).

Step 4c-sel CLASSIFY:

Rule	Decision	Reason
changelog/conventional-prefix-required	Applicable	CHANGELOG.md modified; bullets fix:, fix:, fix:, docs: all match ^[a-z]+:
All other judgment rules	Skipped	No trigger matches any changed file

Step 4d-sel ADJUDICATE:

• changelog/conventional-prefix-required: CHANGELOG.md ## Unreleased bullets: fix: bench runner (×3), docs: bench — all valid conventional prefixes. No violation.
• Mechanical funnel: 0 findings across 74 YAMLs.
• Manual review of bench/run.py:
  • path_is_under + assert_under correctly enforce path traversal guards (D1 security: install path validated to stay within plugin cache tree)
  • publish_keep_refs + prune_refs correctly anchor manifest SHAs before pruning (D5: branch isolation verified by test_ref_pruning_is_idempotent_and_keeps_manifest_commits_reachable)
  • FAILURE_STDOUT_LABEL / FAILURE_STDERR_LABEL correctly assigned, not swapped (D3 verified by test + testsupport fixture)
• Six abort conditions for install-record resolution: all covered in test_config.py (D1 test coverage verified)
• test_ref_pruning_is_idempotent_and_keeps_manifest_commits_reachable explicitly runs git gc --prune=now and asserts manifest SHAs remain reachable (D5 gc-safety verified)

Selector mode: clean — no adjudication needed.

---

Now emitting the consolidated report and verdict JSON:

Must Fix (Critical): None.

Should Fix (Important): None.

Nice to Have (Optional): None.

Selector Mode Traceability:

• Candidates: 1 rule matched by Step 4b-i glob filter
• Applicable: 1 rule selected by Step 4c-sel
• Skipped: all other judgment rules (no trigger match on changed files)

---

{
  "verdict": "approve",
  "summary": "PR introduces three targeted bugfixes to the bench runner — plugin load path validation with six named abort conditions and path-traversal guards, single-target-branch isolation via synthetic ref pruning with gc-safety verification, and dual-stream failure artifacts with correct stdout/stderr labelling. All changes are covered by unit tests; changelog bullets use correct conventional prefixes; mechanical funnel returned zero findings.",
  "comments": [],
  "concerns_addressed": [
    "security: D1 plugin load path traversal guards verified — path_is_under() and assert_under() correctly constrain install_path to plugin_cache_root(config_dir) tree",
    "correctness: D5 branch isolation complete — publish_keep_refs() anchors manifest SHAs before prune_refs() removes all non-synthetic refs; gc-safety asserted in test_ref_pruning_is_idempotent_and_keeps_manifest_commits_reachable",
    "correctness: D3 stream labelling accurate — FAILURE_STDOUT_LABEL and FAILURE_STDERR_LABEL correctly assigned in failure_artifact_text(); test with swapped streams would fail",
    "tests: D1 six abort conditions covered — test_config.py covers NO_INSTALL_RECORD, UNREADABLE_INSTALL_RECORD, STALE_INSTALL_PATH, OUT_OF_TREE_INSTALL_PATH, SCOPE_MISMATCH, PLUGIN_RESOLUTION_MISMATCH",
    "tests: D5 gc reachability covered — test_ref_pruning_is_idempotent_and_keeps_manifest_commits_reachable runs git gc --prune=now and asserts all manifest SHAs remain reachable"
  ]
}