env_fault classifier residuals from #324: an unguarded straddler drop, an unpinned evidence clamp, anchor-drift vacuity, and stale docs

[pbean]

Residuals from the #324 review. None blocked the merge — #324 is a large net win (opencode had no env-fault classification at all before it) and all of these are edges in new functionality rather than regressions. Filing so they are not lost.

Each item was ablation-verified during review; the first two were found independently by two reviewers.

1. Fix A's "won't discard the only line" guard is defeated by a trailing newline

src/bmad_loop/adapters/env_fault.py:192

if truncated and len(lines) > 1:
    lines = lines[1:]

lines = text.split("\n") yields a trailing "". So a tail window that is one >64 KiB line terminated by a newline splits to [fragment, ""] → len(lines) > 1 passes → the fragment is dropped → only "" is scanned → no classification. The byte-identical log without the trailing terminator classifies correctly (which is exactly what tests/test_generic_tmux.py:4422 pins — it deliberately writes no terminator, which is why it is green).

Verified against the real adapter:

input	result
single >64K line, no trailing newline	env_fault=True
single >64K line, trailing \n	env_fault=False
single >64K line, trailing \r	env_fault=False

Failure scenario: a CLI's last act before dying is a single >64 KiB payload (a large JSON error body or stack dump) followed by one terminator — the outage goes unclassified, so the run charges a dev attempt and defers the story instead of pausing. That is the outcome fix A's guard exists to prevent.

One-token remedy: if truncated and any(lines[1:]):

Related nit, same block (:192-205): the code cannot distinguish a straddling fragment from a complete line whose first byte happens to land on the seek boundary — it drops lines[0] unconditionally. Narrow, and last-match-wins makes the tail's first line the least likely winner, but the "the straddling fragment is discarded" framing overstates what the code can tell apart.

2. Fix C's "slide left" clamp has no test coverage

src/bmad_loop/adapters/env_fault.py:251

start = max(0, min(match_pos - ENV_FAULT_EVIDENCE_LEAD, usable - budget))

Replacing it with start = max(0, match_pos - ENV_FAULT_EVIDENCE_LEAD) (dropping only the left-slide clamp) leaves the whole affected suite green — 480 passed, 5 xfailed across test_generic_tmux.py, test_env_fault_patterns.py, test_opencode_http.py, test_profile.py, and 933 passed across the other files that mention env_fault. The harness is otherwise live: five sibling ablations each reddened exactly the right tests.

The clamp is what the code's own comment (:246-250) calls the common case, not a corner. Regression it permits: a match sitting within ENV_FAULT_EVIDENCE_LEAD chars of the line end yields a stub excerpt instead of a full 239-char window, silently discarding operator-facing context.

Note tests/test_opencode_http.py:2443 looks like it pins fix C but does not — it asserts only "AI_APICallError" in evidence, which the pre-PR head-truncated excerpt also satisfies. The breadcrumb (:2454) and e2e (:2501) tests are what actually bite.

Suggested pin: in test_classify_env_fault_marks_a_dropped_suffix, add a case with the match near the line end asserting len(ev) == ENV_FAULT_EVIDENCE_MAX.

3. The anchor-vacuity guard checks a hand-maintained literal, not the live pattern

tests/test_env_fault_patterns.py:150 — PROFILE_ANCHOR_LITERAL is a parallel copy of the profile's anchor, never cross-checked against get_profile(name).env_fault_patterns.

Simulating a realistic upstream drift (opencode renames its ai-sdk error class): change the anchor in opencode.toml to error[.]error="ProviderCallError: and update OPENCODE_REAL + INSEPARABLE_VERBATIM_CITATIONS as a maintainer would, leaving PROFILE_ANCHOR_LITERAL and ANCHOR_REACHING_BAIT on the old literal. Result: 130 passed, 5 xfailed — fully green, while zero ANCHOR_REACHING_BAIT lines reach the real anchor.

That is precisely the vacuity failure the test file's own header (:96-98) narrates having hit once and closed. The reachability guard is real against a corpus edit but not against a pattern edit.

Remedy: derive the anchor from the live pattern, or assert containment, e.g.
assert any(PROFILE_ANCHOR_LITERAL[name].replace(".", "[.]") in p for p in get_profile(name).env_fault_patterns)

4. Stale comments contradict the safety argument

Four sites still describe logs/<task_id>.log as the opencode server's own stdout. Since the readable-logs work, .log is the curated [bmad] transcript (model-writable) and the server sink is <task_id>.server.out:

• src/bmad_loop/adapters/env_fault.py:10-13 and :86
• src/bmad_loop/adapters/profile.py:108-110
• tests/test_env_fault_patterns.py:135 — "logs/<task_id>.log is the opencode serve process's own stdout/stderr, which the model cannot write to"

The last one is load-bearing and self-contradictory: it is the written justification for the positive characterization test test_verbatim_citation_of_the_real_error_is_inseparable, and it contradicts that same file's header at :18-21, which correctly names .server.out. The code is right; only the prose is wrong — but a reader who trusts it concludes the transcript is model-free, which is the exact stale premise #324 exists to correct.

5. Docs are stale after #324

• docs/adapter-authoring-guide.md:411 — "Seeded only for claude; empty = inert."
• docs/FEATURES.md:70 — describes the scan as over a "pane log", "seeded only for claude"

opencode.toml now seeds two patterns, and the opencode adapter scans <task_id>.server.out, not a pane log.

Also: #324 landed with no ## [Unreleased] CHANGELOG entry despite user-visible behavior change (opencode gains env-fault classification; the evidence string format changed).

6. Smaller items

• The isolation property rests on a single e2e test. Reverting ENV_FAULT_LOG_SUFFIX to .log fails only test_e2e_env_fault_classified_through_run; the seven other opencode env-fault unit tests still pass, because the unit helper resolves through _env_fault_log_path by design. Two class-attribute assertions would keep it pinned if that test is ever skipped or quarantined.
• Coverage gap toward false negatives. opencode 1.18.2 ships sibling ai-sdk classes (AI_RetryError with maxRetriesExceeded/errorNotRetryable, AI_LoadAPIKeyError). A retry-exhausted outage surfacing as error.error="AI_RetryError: …" is missed by both shipped patterns. Speculative — the author verified only against a captured AI_APICallError outage — but widening the class token produced zero false positives across the review's negative corpus.
• Eager → lazy pattern compilation (src/bmad_loop/adapters/env_fault.py:124) is a semantic change riding inside the extraction: a regex.error now surfaces during run() teardown rather than at adapter construction, and reassigning adapter.profile post-construction now changes classification. Unreachable via load_profile (patterns are validated at TOML parse time, profile.py:214-219) and nothing in src/ mutates .profile, so benign — noting it because it is not a pure move.
• RecursionError escapes load_profiles() bare (pre-existing, not from Classify provider quota errors as environment faults on opencode-http #324): a pattern nested ≥ ~500 deep makes regex.compile raise RecursionError, which is neither regex.error nor in CONVERSION_FAULTS. Identical in main before Classify provider quota errors as environment faults on opencode-http #324. One-word fix if wanted: except (regex.error, RecursionError).
• No total-scan deadline. env_fault.py:211 uses a per-search timeout=2.0, so an operator pattern taking ~1.9 s/line costs lines × patterns × 1.9 s. Documented as a known bound at :58-65; noting for completeness.

Follow-up to #324. See also #323 and the claude-anchor false-positive issue filed alongside this one.

[coderabbitai]

🔗 Related PRs

#267 - build(ruff): pin lint.select to CI's rule set + stale-noqa audit (#246) [merged]
#272 - fix(#194): pause on coding-CLI transport failures instead of burning story/sweep attempts [merged]
#273 - fix(adapters,engine,escalation): synthesize marker-less terminal specs + review.on_timeout salvage [merged]
#279 - feat(opencode-http): readable run logs + structured event trace [merged]
#324 - Classify provider quota errors as environment faults on opencode-http [open]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

• Create Plan

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[pbean]

Two more residuals, from the automated review on #509

#509 reverted #324 for a process reason (re-landing it behind an @codex review gate). CodeRabbit reviewed the revert as if it were a forward change and surfaced two issues that are pre-existing in both the pre-#324 and post-#324 code — so they are not revert artifacts, and they survive the re-landing. Both were validated by ablation in two worktrees.

7. The regex timeout is per-search, not a scan-wide deadline

src/bmad_loop/adapters/env_fault.py:211 passes timeout=ENV_FAULT_MATCH_TIMEOUT_S to each pat.search(line, ...) call. There is no deadline across the scan, so worst-case cost is lines × patterns × timeout. grep -n "monotonic\|deadline\|time\." over env_fault.py returns nothing.

Measured over 20 lines of a catastrophic pattern, in both code states:

	per-line	total	exceeded the 2.0 s bound?
pre-#324 (generic.py)	0.267 s	5.188 s	yes
post-#324 (env_fault.py)	0.254 s	5.141 s	yes

This is documented, not accidental — env_fault.py:57-65 states "it is per-search, so the worst case for a scan is lines × patterns × this, not this." Shipped patterns are safe (measured 3.7 ms worst case on hostile 64 KiB input), so this only bites an operator-supplied overlay pattern. Filing so the known bound is tracked rather than only commented.

Note the pre-#324 comment at generic.py:110-113 ("a runaway is capped at ~this") reads as a scan-wide claim and is misleading; #324's wording exists specifically to correct it. That correction returns when #324 re-lands.

8. The empty-pattern short-circuit is pinned by nothing

test_generic_tmux.py::test_classify_env_fault_inert_without_patterns does not pin the not self._env_fault_patterns guard. Deleting the guard leaves the entire suite green on both sides:

pre-#324,  guard ablated: 4566 passed, 32 skipped
post-#324, guard ablated: 4707 passed, 32 skipped, 5 xfailed

Scope this carefully — the obvious remedy does not work. The guard is result-equivalent to no guard: with an empty pattern tuple _env_fault_evidence returns None on its own, so the outcome is env_fault=False with no lifecycle line whether or not the guard runs.

_env_fault_evidence() with EMPTY patterns -> None
log file opened by that call?             -> True

So no strengthening of the result assertions (including stubbing _env_fault_evidence, which was the suggested fix) can make the test pin the guard. The guard's only observable effect is skipping one log read — it is a performance short-circuit, not a safety gate. A test that actually pins it must assert the log file is never opened, e.g. by counting opens on _env_fault_log_path.

Low value either way: worth a one-line test or worth deleting the guard as unearned complexity, but not worth a rewrite of the existing test's result assertions.

---

For the record, two other CodeRabbit findings on #509 were declined as re-discoveries of #324's own content: the partial-tail-line defect (which is #324's fix A) and "restore the OpenCode classifier" (which is a description of undoing the revert). Full triage is in the #509 comment thread — including a warning that CodeRabbit's suggested diff for the partial-line fix omits #324's len(lines) > 1 guard and fails test_classify_env_fault_scans_a_truncated_window_with_no_newline if applied verbatim.

[chatgpt-codex-connector]

To use Codex here, create an environment for this repo.

[pbean]

Status update — items 1 and its sibling nit are FIXED; one new trap recorded

#324 was reverted (#509) and is re-landing as #510, which carries two follow-up commits fixing defects found during the re-review. Updating this issue so it stops listing resolved bugs as open.

✅ Item 1 — trailing-newline hole — FIXED in 70369a9 (#510)

len(lines) > 1 → any(lines[1:]), which is what "don't discard if that leaves nothing to scan" actually means.

Fixed together with the sibling nit recorded under item 1 (a complete first line being discarded when the seek lands on a line boundary) — Codex flagged it independently as a P2 on #510. The guard no longer keys on "the read truncated" at all: it reads the byte immediately before the window, on its own so it enters neither the buffer nor the 64 KiB budget, and inspects it raw ahead of the \r→\n normalization — a pane capture is CR-terminated, so a \r there ends a line just as a \n does.

Both are pinned by ablation-verified tests that assert their own byte layout, so they cannot decay into vacuity:

• test_classify_env_fault_keeps_a_boundary_aligned_first_line
• test_classify_env_fault_scans_a_single_oversized_terminated_line[lf|cr]

✅ New, found and fixed during the same pass — d0341d8 (#510)

Not previously in this issue. Recording it because the mechanism is a reusable trap in this file rather than a one-off.

generic.py re-exports ENV_FAULT_MATCH_TIMEOUT_S from env_fault.py, but an import copies the object binding. env_fault.py:230 resolves the constant from its own module at pat.search time, so monkeypatch.setattr(generic, "ENV_FAULT_MATCH_TIMEOUT_S", 0.1) never reached the classifier — a regression introduced by the extraction, since before the split the constant was defined and read in the same module.

case	elapsed
unpatched (2.0s default)	1.075s
patch generic.… = 0.1	1.065s — no effect
patch env_fault.… = 0.1	0.100s — takes effect

The part worth keeping: the affected test was not merely slow, it was fully vacuous. At 2.0s the backtracker completes naturally in ~1.07s and returns no match, so TimeoutError was never raised and except TimeoutError was dead code — the gate could be deleted outright with the test still green, because evidence is None held equally for "timed out" and "found nothing".

That is a general shape to watch for in this file: a test asserting evidence is None cannot distinguish "the gate fired" from "nothing matched". Any such test needs a positive control that only survives if the gate did not fire. The fix adds a second trivially-matching pattern after the backtracker — any scan not cut short reaches it, matches, and reddens every assertion. Deterministic, not a timing assertion.

The other four re-exported names (_ANSI_RE, ENV_FAULT_EVIDENCE_MAX, ENV_FAULT_STATUSES, ENV_FAULT_TAIL_BYTES) were swept for the same trap: the timeout was the only site that writes through an alias; the nine other generic.ENV_FAULT_* uses are reads, which are exact. The re-export block is now commented READ-ONLY.

Still open

Items 2 (fix-C left-slide clamp unpinned), 3 (PROFILE_ANCHOR_LITERAL drift vacuity), 4 (stale comments naming .log as the server sink), 5 (stale docs + no CHANGELOG entry), 6 (single-e2e isolation pin, AI_RetryError coverage gap, lazy compile, RecursionError escape), 7 (per-search timeout is not a scan-wide bound) and 8 (empty-pattern short-circuit pinned by nothing) are unchanged and still apply to #510.

Note item 8 and the new trap above are the same failure mode seen twice: a negative assertion that passes for more than one reason.