Fix #73: regression-test.sh could not go red; reports green having run nothing

[laird]

Closes #73

Supersedes #75, which targeted the integration branch. master is the repo default and all recent merged PRs target it, so the fix belongs here. #75 will be closed.

The important one first: the runner could not fail

if $UNIT_TEST_CMD 2>&1 | tee "$UNIT_RESULTS"; then
    UNIT_STATUS="✅ PASSED"

That takes the exit status of tee, not of the suite — and tee essentially always succeeds. This script has no set -o pipefail, so every failing unit and E2E run on master was recorded as ✅ PASSED with exit 0.

Demonstrated:

$ bash -c 'false | tee /dev/null; echo $?'                    -> 0
$ bash -c 'set -o pipefail; false | tee /dev/null; echo $?'   -> 1

And end-to-end against a deliberately failing suite, before the fix: RC=0, ✅ All tests passed!

A quality gate that cannot go red is worse than no gate — /fix and /dev treat a green regression run as licence to move on to enhancements.

Fixed with ${PIPESTATUS[0]}, deliberately not with a global set -o pipefail. This script has many other pipelines (grep|head, grep|sed) whose failures are currently benign; flipping pipefail under set -e would start aborting runs on those. PIPESTATUS is applied at the two places correctness depends on it.

The other four

2. REPORT_DIR glob. Took the first Location: line anywhere in CLAUDE.md — the Unit Tests one (tests/test_*.sh), a glob. mkdir -p created a directory whose literal name contained an asterisk, which then matched tests/test_*.sh expansions and broke callers iterating the shell suites. Now scoped by awk to the **Test Reports**: block, with a guard rejecting glob metacharacters and ...

3. Dead extraction path. The unit-test command was read from ### Unit Tests Only, but gated behind a Working directory: check that no CLAUDE.md satisfies — so extraction never ran and the suite silently skipped. Now reads the header directly, falling back to ### Regression Test Suite, with a self-reference guard: in this repo that entry is this script, so adopting it would recurse without bound. CLAUDE.md gains the ### Unit Tests Only block the script has always documented. The repo's 14 shell suites now actually run.

4. All-skipped counted as passed. Zero tests executed is not a pass; exits 1 and says why.

5. Environment leak. The runner exported its own ISSUE_SOURCE into the suite. issue-config.sh treats a pre-set ISSUE_SOURCE as authoritative and skips .autocoder.json, so tests building a file-backend fixture would talk to the live tracker. Suites now run under env -u ISSUE_SOURCE -u ISSUE_DIR_PATH -u ISSUE_BACKEND.

Extraction uses awk, not a nested sed range: BSD sed (macOS) rejects /a/,/b/{/c/,/d/p} outright, which aborts the run under set -e.

Also adds REGRESSION_TEST_NO_ISSUES=1 so the suite can exercise the runner without touching a real tracker, and makes a failing tracker query (no remote, no auth) non-fatal rather than aborting a run whose tests already passed.

Verification

tests/test_regression_test_config.sh (new): 12 passed, 0 failed
all tests/test_*.sh:                        14 passed, 0 failed
build verification:                         BUILD_OK

The new test file includes an explicit assertion that a failing suite exits non-zero — the property that was silently broken. Confirmed it fails against the pre-fix script and passes after.

Runner on this repo now executes the 14 shell suites, reports the real result, writes to docs/test/regression-reports/, and creates no glob directory. Remote tree OID verified identical to local HEAD tree (d8ff1033).

Known remaining gap (not fixed here)

The summary line prints counts like 7/0 passed — the parser keys off Jest/Playwright phrasing and doesn't understand this repo's === N passed, M failed === format. The exit code is now authoritative and correct, so this is cosmetic, but it is the same family of misleading-confidence bug and worth a follow-up.

🤖 Generated with Claude Code

[laird]

⚠️ Merge-order note: #77 will overwrite this file.

#77 (merge/30-dev-line-v3 → master, open now) replaces plugins/autocoder/scripts/regression-test.sh wholesale — 447 lines — with the /dev-line version. That happens regardless of this PR.

Comparing what each fixes:

defect	this PR (#78)	after #77 lands
cmd | tee masks failure — suite cannot go red	fixed	fixed (it has set -o pipefail)
REPORT_DIR takes the first Location: line (a glob)	fixed	still broken
unit command read from a header no CLAUDE.md supplies	fixed	still broken
runner leaks ISSUE_SOURCE into the suite	fixed	still broken
all-skipped exits 0 with "All tests passed"	fixed	still broken

Verified by running #77's head unmodified: zero tests executed, ✅ All tests passed!, RC=0, and the tests/test_*.sh/ junk directory recreated.

Suggested handling: land #77 first, then re-apply the four remaining fixes on top of its version. Merging this first would have those changes reverted by #77.

If you would rather land this one first, that is fine too — it is correct against current master and the red-path fix is the urgent one — just expect to redo the other four after #77.

The new test file tests/test_regression_test_config.sh (12 assertions, including the red-path one) is version-independent and worth keeping either way.

[laird]

Conflict resolution prepared, plus a correction and one new finding

1. The resolution (what you asked for)

Reproduced the merge locally (master + #77 + #74 + #78) and resolved the 5 hunks in plugins/autocoder/scripts/regression-test.sh as a genuine combination, per your spec:

Kept from #78: extract_fenced_command, the ungated ### Unit Tests Only read, the ### Regression Test Suite fallback, the self-invocation guard, and the env -u ISSUE_SOURCE … scrub on both suite invocations.

Kept from #77: both detect_skip_reason guards (Unit + E2E call sites), is_node_based_command, set -o pipefail, and the raw-vs-zero stat extraction. #77's detect_skip_reason handles the empty-command case, so it fully supersedes #78's own skip branch — that branch is dropped rather than merged.

Acceptance on the combined tree:

detect_skip_reason:                         3 (definition + Unit + E2E)
tests/test_regression_test_config.sh:       16/16
all tests/test_*.sh:                        15/15
pytest:                                     124/124

2. Your diagnosis was right, but the cause was not the dropped guards

Taking #78 wholesale does drop both guards — but that is not what produced exit code when the configured suite passes — want 0, got 1. With the guards restored the assertion still failed.

The real cause: #77's unit stat parser only recognises the Jest Tests: N passed form. #77 alone never exercises it (the suite always skipped, so the parser was dead code). #78 makes the suite actually run — and this repo's own shell tests print Results: 13 passed, 0 failed, no Tests: prefix. A fully green run was therefore reported "no parseable test summary" and exited 1.

That is a false RED — the mirror image of the false green #73 is about, and it would have bitten the moment the combined tree ran its own suite. Fixed with a bare <N> passed fallback, mirroring the two-tier approach #77's own E2E block already uses, with assertions pinning both directions: a non-Jest summary is accepted, and output with no counts at all is still an error.

3. Correction — I broke this PR and have restored it

I pushed the merged branch to feature/issue-73-master, which ballooned this PR from 3 files to 133 files / +10,280. That was my error. api-push.py uses createCommitOnBranch, which cannot create multi-parent commits, so the merges collapsed into a squash onto a newer master (which had moved — #83 landed) and verification failed.

This PR is now restored to 3 files / +460 / −27 against current master, re-verified there: 18/18 shell, 59/59 pytest, BUILD_OK.

The resolution is preserved locally on resolution/73-after-77-74 (86987ae). It is not pushed, because a branch containing merge commits cannot be published through the API path this machine is restricted to. Once #77 and #74 land I can re-run the identical resolution against the real post-merge master and push it as an ordinary single-parent commit — which is also the more honest sequencing, since the resolution should be verified against what actually merged rather than a PR head that may still change.

4. One extra fix included

Both summary lines printed only $PASSED/$TOTAL passed in green, discarding $UNIT_STATUS. On a failed run the reason went to the report file and was never shown — an operator saw a green 0/0 passed with no explanation. Now prints the status and colours the label by outcome.

Nothing merged. #77 and #74 remain the gating decision.

[laird]

Retargeted master → integration per the branch model introduced in #89: feature/* → integration → master, with master accepting PRs from integration only. Verified feature/issue-73-master merges into integration with 0 conflicts.

🤖 Autonomous fix workflow