Skip to content

Pause only when pause_after is true, not merely present#172

Merged
mnot merged 1 commit into
mainfrom
fix/pause-after-truthiness
Jun 9, 2026
Merged

Pause only when pause_after is true, not merely present#172
mnot merged 1 commit into
mainfrom
fix/pause-after-truthiness

Conversation

@mnot

@mnot mnot commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Fixes #166: pauseAfter was computed from 'pause_after' in requests[i], so pause_after: false would still trigger a pause. Now compares the value against true.

Latent today (templates only ever set true), but removes the no-op trap for anyone overriding a template to false.

Closes #166


🤖 This PR was generated by an AI agent (Claude Code) under human supervision, as part of a maintainer-directed review of the test suite.

pauseAfter was set from `'pause_after' in requests[i]`, so a request
with `pause_after: false` would still pause. Compare against true.

Closes #166

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@mnot

mnot commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

Review

Verdict: Approve. The fix is correct and resolves #166 with no behavioral regression for any test that exists today.

Correctness against existing behavior

Walking the truth table for the changed expression at test-engine/client/test.mjs:54:

pause_after value old 'pause_after' in requests[i] new requests[i].pause_after === true
true true (pause) true (pause) — preserved
absent false (no pause) false (no pause) — preserved
false true (pauses — the bug) false (no pause) — fixed

The two cases that occur in practice (true and absent) are unchanged, and the false no-op trap is closed. Good.

Downstream consistency

The consumer at line 62 already gates on nextFetchFunction.pauseAfter === true, so the producer now uses the same strict-boolean discipline as the consumer. Previously the producer could emit pauseAfter: true for a request whose pause_after was a non-boolean; the new code makes producer and consumer agree. Consistent.

"Latent" claim — verified

I grepped the whole tree: every one of the 100+ pause_after occurrences (in tests/lib/templates.mjs and across tests/*.mjs) is pause_after: true. Nothing sets it to false or to any non-boolean. So this is indeed latent — no current test changes behavior — and the PR description's framing is accurate.

One edge case worth noting (not blocking)

This is a slight semantic narrowing for truthy non-boolean values: under the old code pause_after: 1 or pause_after: 'yes' would pause (key present); under the new code they would not (=== true is false). No test uses such a value, and the field is only ever a boolean by convention, so this is harmless in practice. Flagging it only because === true is stricter than a general "truthy" check — if anyone ever intends a truthy-but-not-true value to pause, they'd be surprised. Given the field's documented boolean intent and the matching === true consumer, strict equality is the right call here.

Scope

One-line change, no collateral edits, CI passing. Clean.


This is an AI-generated review (Claude Code), produced as part of a maintainer-directed review of the test suite and relayed by the maintainer.

@mnot mnot merged commit 7743f0a into main Jun 9, 2026
2 checks passed
@mnot mnot deleted the fix/pause-after-truthiness branch June 9, 2026 04:12
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.

pause_after checked by key presence, not truthiness

1 participant