Skip to content

test: surface OpenCode harness output on assertion failure - #2885

Open
karotkriss wants to merge 1 commit into
kunchenguid:mainfrom
karotkriss:fm/opencode-external-healthy-diagnostics
Open

test: surface OpenCode harness output on assertion failure#2885
karotkriss wants to merge 1 commit into
kunchenguid:mainfrom
karotkriss:fm/opencode-external-healthy-diagnostics

Conversation

@karotkriss

Copy link
Copy Markdown
Contributor

Intent

Improve the failure diagnostics of one existing test case in tests/fm-pi-watch-extension.test.sh: test_opencode_healthy_arm_output_does_not_suppress_guard.

Background on why this is being raised at all. This one-line change was originally carried, unrelated, inside a session-lock pull request (#2839) alongside a functional change to .opencode/plugins/fm-primary-watch-arm.js. Reviewers correctly flagged that whole hunk as out of scope for that issue, and it has been removed from that branch. The functional plugin part is deliberately NOT being re-filed: investigation could not substantiate the race it claimed to fix. Instrumenting settleReadiness in the real plugin against this very test fixture showed readiness is always settled by observeArmOutput on the streamed-output path and never by the close handler, across 60 sequential runs and 8 concurrent runs under a load average above 20, which matches Node's contract that a child's close event is emitted only after its stdio streams have ended. The genuine load-sensitive flake in this test file is a PID-recycling issue in the session-transition case, which pull request #2845 already fixes with a per-arm lifetime marker. Only the diagnostic improvement below is worth keeping, so it is being filed on its own.

The problem this change fixes. The case drives a real node harness in a command substitution and captures its output in "$out". That harness prints the specific reason it failed to stderr, for example "turn-end guard was suppressed by an external healthy watcher". The assertion, however, used expect_code, which reports only the exit status. A CI failure therefore read "expected exit 0, got 1" and discarded the one piece of information that explains it, which is precisely what made an earlier CI shard failure in this file hard to diagnose.

The change. Replace the expect_code call with the [ "$status" -eq 0 ] || fail "... (exit $status): $out" form, so the harness output travels into the failure message. A short comment records why this case does not use expect_code.

Decisions and constraints, so the diff is not read as arbitrary:

  • The assertion's pass and fail conditions are completely unchanged. It still requires exit status 0 and nothing else. This is diagnostics only, not a behavioural or coverage change, and it does not weaken the test.
  • expect_code in tests/lib.sh was deliberately NOT modified. Its signature takes only expected status, actual status, and a label, so it has no access to the captured output; giving it that access would mean changing a shared helper used by many callers across the suite for the benefit of one case. The local form chosen here is already this file's own established idiom, used in over thirty places including the assertion on the very next line.
  • The neighbouring [ -z "$out" ] || fail "...: $out" assertion is left exactly as it was.
  • Scope is one test case in one file. No production code, no plugin, and no other test is touched.

Verification performed: the case passes unchanged. The diagnostic was then proven to work rather than assumed, by deliberately sabotaging the plugin so an external healthy watcher settles as an owned failure and re-running both forms against that identical sabotage. The old form reported "expected exit 0, got 1". The new form reported "(exit 1): turn-end guard was suppressed by an external healthy watcher". shellcheck is clean on the changed file, and the change was confirmed conflict-free with open pull request #2845, which edits a different region of the same file.

What Changed

  • Replace the OpenCode external-healthy test’s expect_code assertion with a local zero-exit check that includes captured harness output in failure messages while preserving the existing pass condition.

Risk Assessment

✅ Low: Captain, the change is narrowly scoped and preserves the assertion semantics while correctly including captured harness output in failures.

Testing

The focused case passed normally, then a controlled end-to-end failure showed both assertion forms still exit 1 while the changed form exposes the harness reason; the comparison transcript was saved as reviewer-visible evidence and no worktree artifacts remain.

Evidence: Diagnostic counterfactual transcript

Source: Diagnostic counterfactual transcript

Old: expected exit 0, got 1 New: (exit 1): turn-end guard was suppressed by an external healthy watcher

BASE ASSERTION AGAINST SABOTAGED HEALTHY-WATCHER READINESS
not ok - OpenCode watch plugin must not treat external healthy output as an owned arm: expected exit 0, got 1
observed test exit: 1

CHANGED ASSERTION AGAINST THE SAME SABOTAGE
not ok - OpenCode watch plugin must not treat external healthy output as an owned arm (exit 1): turn-end guard was suppressed by an external healthy watcher
observed test exit: 1

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

✅ **Review** - passed

✅ No issues found.

✅ **Test** - passed

✅ No issues found.

  • Inspected git diff --unified=40 f170cedeb735759e9547a5b9de1a26eca7ea6d71..31a1b825f7f273bc032ea19ab609ea38ce391615 -- tests/fm-pi-watch-extension.test.sh
  • Ran only test_opencode_healthy_arm_output_does_not_suppress_guard through its real Node and OpenCode plugin harness
  • Ran the base and changed assertions against identical disposable plugin sabotage that classifies external healthy readiness as an owned failure
  • Verified the evidence transcript and clean worktree with git status --short
✅ **Document** - passed

✅ No issues found.

⚠️ **Lint** - 1 warning
  • ⚠️ linter found issues (exit code 1)
✅ **Push** - passed

✅ No issues found.

…tion fails

This case drives a real node harness that prints the reason it failed to
stderr, but the assertion reported only the exit status, so a failure read
as "expected exit 0, got 1" with no indication of the cause.

Before:
  not ok - ...owned arm: expected exit 0, got 1

After:
  not ok - ...owned arm (exit 1): turn-end guard was suppressed by an
  external healthy watcher

Diagnostics only - the assertion's pass and fail conditions are unchanged,
and this adopts the `|| fail "...: $out"` form already used throughout
this file.
@greptile-apps

greptile-apps Bot commented Aug 23, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

The PR appears safe to merge because it changes only failure reporting while preserving the test’s existing pass and fail conditions.

The new conditional is equivalent to the prior zero-status assertion, and the captured output is surfaced only on the existing failure path.

Reviews (1): Last reviewed commit: "test: surface harness output when the Op..." | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant