fix(tasks): correctness fixes in the file and fetch tasks

[sroussey]

Three independent correctness fixes in @workglow/tasks. They share no files; each is one commit with its own test, and every test was verified to fail before its fix and pass after.

Merged with main (41e0233). A fourth finding in this PR — readHttpErrorBody buffering the whole error body — was fixed independently on main while this PR was open, and main's fix is the better one. That section has been dropped and main's implementation taken verbatim. See Merge resolution at the bottom.

1. grepLines re-emitted already-emitted lines (FileGrepTask.ts)

Two matches closer together than beforeContext replayed the lines between them. On a non-matching line the scan dropped currentGroup, which blinded emitLine's de-duplication when the before-buffer was replayed for the next match.

Extracted and executed against ["match one", "filler", "match two"] with beforeContext: 2, pattern /match/:

Before

groups[0] = { 1..1, [ line 1 "match one" match: true ] }
groups[1] = { 1..3, [ line 1 "match one" match: FALSE,
                      line 2 "filler"    match: false,
                      line 3 "match two" match: true ] }
matchCount 2, truncated false

Line 1 appears twice, in overlapping ranges, with contradictory match flags. With maxOutputLines: 3 — enough for the three distinct lines — the replayed copies consumed the budget and the second real match was dropped entirely, reported as truncated: true:

groups[0] = { 1..1, [ line 1 match: true ] }
groups[1] = { 1..2, [ line 1 match: false, line 2 match: false ] }
truncated: TRUE

After (both cases identical, and maxOutputLines: 3 now fits)

groups[0] = { 1..3, [ line 1 "match one" match: true,
                      line 2 "filler"    match: false,
                      line 3 "match two" match: true ] }
matchCount 2, truncated false

The fix has two halves, both needed:

• dropping currentGroup on a non-match was redundant — entry.line <= currentGroup.endLine + 1 already opens a new group across a gap — and retaining it is what lets the de-duplication see the replayed lines;
• the de-duplication now runs before the output caps. A line already in the group emits nothing, so it must not be charged against maxOutputLines/maxOutputChars, nor refused by them.

Group shape changes for a multi-match-within-context run: two overlapping groups become one contiguous group, which is GNU grep -B/-A block semantics. No existing test asserted the old shape. Non-context and onlyMatching runs are byte-identical (re-run as controls).

2. expandReplacement deleted $<name> (FileSedTask.ts)

With no named captures the $< is not a substitution — "abc".replace(/b/, "[$<x>]") yields "a[$<x>]c" — but the expander substituted "" unconditionally, so sedding with a replacement like bar $<x> against a pattern carrying no named group silently deleted the caller's text. The run is now copied through verbatim in that case. An unknown name in a pattern that does have named groups still expands to "", matching the engine.

3. HEAD + a derived response_type silently succeeded with no value (FetchUrlTask.ts)

A HEAD response carries no representation body, so the HEAD branch finishes with metadata alone regardless of response_type. assertResponseType accepts "json", so { method: "HEAD", response_type: "json" } completed successfully with json undefined — the same silent success with no value that making response_type required exists to prevent, reached from the other direction.

assertMethodAllowsResponseType now rejects the pairing before the request is issued (so it spends no network call), and validateInput rejects it at the task layer before anything is enqueued. INVALID_RESPONSE_TYPE is outside FETCH_URL_RETRYABLE_ERROR_CODES, so it burns no retry budget.

Behaviour change. { method: "HEAD", response_type: "text" | "json" | "blob" | "arraybuffer" } previously resolved with metadata and an undefined value port; it now fails with INVALID_RESPONSE_TYPE. Use response_type: "stream" and read metadata, or use GET. HEAD shipped one version ago (0.3.47), so the exposure window is small, and a caller relying on the old behaviour was receiving undefined anyway. The commit message carries this as an explicit line naming the rejected combination rather than a generic note.

Merge resolution

main advanced to 41e0233 ("feat(tasks): improve error body reading in FetchUrlTask") while this PR was open, which fixes the same readHttpErrorBody defect this PR's fourth finding addressed, in the same function. origin/main is merged into this branch (a merge commit, not a rebase — the branch is pushed and may be referenced).

main's implementation wins, taken verbatim. It bounds the read the same way this PR did, but additionally adds HTTP_ERROR_BODY_READ_MS = 100 — a read budget that closes a residual hang this PR's author explicitly flagged as not covered by their version: an origin that returns a small error body and then trickles forever. The two implementations were not merged and this PR's version was not ported over it.

Dropped from this PR:

• the competing readHttpErrorBody rewrite;
• the HTTP_ERROR_BODY_MAX_BYTES → HTTP_ERROR_BODY_MAX_CHARS rename;
• the 16 KiB wire ceiling (main bounds at HTTP_ERROR_BODY_MAX_BYTES = 4096);
• the whole finding section from this description.

Kept:

• The F8 work above (assertMethodAllowsResponseType, its call in executeStream before issueRequest, and the validateInput override). It is in the same file but unrelated to the error-body read, and main did not touch it.
• The discardBody(response) removal after buildHttpError, which is still correct under main's version and still carries a comment saying why. buildHttpError unconditionally calls readHttpErrorBody, which cancels its reader in a finally — on the byte ceiling, on the read budget, or at EOF — so the socket is already released. With a body there is nothing left to cancel and the stream is still reader-locked, so a second cancel() only raises a TypeError for discardBody to swallow; with no body it was a no-op to begin with. The two other discardBody call sites (the 304 branch and the HEAD-OK branch) never reach readHttpErrorBody and are untouched.
• The reads only a bounded prefix of a huge error body test, because main has no test pinning the byte ceiling. Honest caveat: unlike the other tests here it does not go red against main's source — it asserts a property main's implementation also has, so it is additive regression coverage rather than proof of a change in this PR. It would still catch a regression to await response.text() (which pulls all 65 chunks).

Restored to main's version: the test an HTTP error cancels the response body instead of abandoning it. This PR had enlarged its chunk from 3 bytes to 64 KiB to make the cancel observable under its own implementation; under main's that edit is unnecessary, so it was reverted rather than kept. The 3-byte body never closes, the 100 ms read budget fires the cancel, and the test passes unchanged — in 103 ms, which is the budget doing exactly that. packages/test/src/test/task/FetchTask.test.ts is now purely additive against main (80 insertions, 0 deletions).

Net effect on FetchUrlTask.ts versus main: 63 insertions, 1 deletion — the F8 additions plus the single removed discardBody line. The readHttpErrorBody / HTTP_ERROR_BODY_MAX_BYTES / HTTP_ERROR_BODY_READ_MS region is byte-identical to main (verified by SHA-256).

Tests

Five new tests, each verified red before its fix and green after — re-verified after the merge by reverting each source hunk alone against the merged tree:

test	with its source hunk reverted	on this branch
two matches inside beforeContext emit each line once	FAIL	pass
a replayed context line is not charged against maxOutputLines	FAIL	pass
leaves $<name> literal when the pattern has no named groups	FAIL	pass
a persisted HEAD payload with a derived response_type fails before fetching	FAIL	pass
validateInput rejects HEAD paired with a derived response_type	FAIL	pass

Plus two pins that are green both ways by design: (?<foo>…) → [$<foo>] = [foo] and [$<bar>] = [], and the bounded-prefix error-body test described under Merge resolution.

Suites run on the final merged state:

packages/test/src/test/task/                    Test Files 73 passed (73)   Tests 1204 passed | 24 skipped (1228)
  FetchTask.test.ts                             94 passed
  FileGrepTask{,.server,Entitlements}.test.ts   } 116 passed across all six
  FileSedTask{,.server,Entitlements}.test.ts    }
turbo run build-types --filter=@workglow/tasks  5 successful, 5 total
eslint + prettier                               clean on all six touched files

Environment note: this container has Node 22 (/opt/node22), not the Node 24+ the repo asks for. No native SQLite-backed suites are in the affected set, and the whole packages/test/src/test/task/ directory passes.

---

Generated by Claude Code

[github-actions]

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	58.66%	33251 / 56683
🔵	Statements	58.34%	34768 / 59590
🔵	Functions	59.85%	6444 / 10766
🔵	Branches	47.11%	16821 / 35701

File Coverage

No changed files found.

Generated in workflow #3242 for commit a8478ca by the Vitest Coverage Report Action