fix(tasks): bound sedLines with a search deadline and restore the node export surface - #849
Merged
Conversation
…e export surface `sedLines` had no overall search deadline. Its only bounds were the per-batch substituter timeout and the output caps — and under `onlyChangedLines` the cap checks are skipped for unchanged lines, so a run that changes nothing had no stopping condition but EOF. The deadline is checked after the batch is filled and before it is substituted, identical to grep. Because the batch is non-empty and unprocessed at that point, `truncated = true` is unconditionally correct, so sed needs no equivalent of grep's `sawMoreInput` reconciliation. The node and electron entrypoints also dropped eleven grep/sed names that `browser.ts` exports, so an import that resolved under the browser condition failed to resolve under node. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LowBJQsCghLDiHwPN6FgUT
Coverage Report
File CoverageNo changed files found. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two independent defects in the grep/sed pair in
@workglow/tasks.1 —
sedLineshad no overall search deadlinegrepLinescomputesdeadline = Date.now() + DEFAULT_LIMITS.grepMaxSearchMsand breaks when it passes.sedLineshad no equivalent: its only bounds were the per-batch substituter timeout and the output caps. UnderonlyChangedLines: truethe cap checks are skipped for unchanged lines — thecontinuefires before themaxOutputLines/maxOutputCharstests — so a run that changes nothing had no stopping condition but EOF.FileSedTask.server.tsfeedssedLinesalinesFromStream, so this was a real unbounded local-file scan:On a 100 GB file that streams the whole file, holds a task slot for hours, and then reports
truncated: false.The fix reuses
DEFAULT_LIMITS.grepMaxSearchMsrather than adding a limits key —sedLinesalready reusesgrepMaxLineChars,grepMaxOutputLinesandgrepMaxOutputChars, and a new key would drag a@workglow/utilchange into a@workglow/tasksfix.Why the check sits where it does. It is placed after the existing abort check and before the
budgetcomputation — that is, after the batch has been filled from the iterator and before it is substituted, identical to grep. Because the batch is non-empty and entirely unprocessed at that point, something was definitively left unread, sotruncated = trueis unconditionally correct.Why sed needs no
sawMoreInputreconciliation. Grep carries that extra step because its stop reasons include the output caps, which reject the line they stopped on — grep therefore has to look ahead to decide whether the early stop actually dropped anything. The deadline is not such a reason: it stops on a whole unprocessed batch, so the answer is already known and no lookahead is needed.2 — node/electron dropped most of the grep/sed public surface
browser.tsdoesexport * from "./task/FileGrepTask"and"./task/FileSedTask". The server entrypoints onlyexport *from the two.serverfiles (which re-export just the Config/Input/Output types, the class and the helper fn) plus a hand-picked{ grepLines, linesFromText }— and nothing at all from sed.Missing from both
node.tsandelectron.ts:GrepOptions,GrepLineMatcher,createMatcher,SedOptions,SedBatchResult,SedLineSubstituter,createSedRegex,createSedExpander,createSubstituter,expandReplacement,sedLines.So
import type { SedOptions } from "@workglow/tasks"type-checked and ran under the browser condition and failed to resolve under node — a build that works in the web example and breaks in the CLI and Electron shells.The export lists must stay explicit:
export *from both grep modules would collide onFileGrepTask/fileGrep, and both sed modules onFileSedTask/fileSed. The remaining names were checked againstcommon.tsand the.serverfiles and collide with nothing.A structurally better fix — lifting the platform-neutral helpers into
grepLines.ts/sedLines.tsthat both entrypointsexport *— removes the drift permanently but touches every importer, so it was deliberately deferred. The new guard test stands in for it: it fails to type-check and at runtime the moment a name is dropped from an entrypoint again.Tests
packages/test/src/test/task/FileSedTask.test.tsstops at the search deadline under onlyChangedLines— drivessedLinesdirectly with a finite counting generator against a non-matching pattern.vi.useFakeTimers()advances pastDEFAULT_LIMITS.grepMaxSearchMswhile the first batch is being filled, so the deadline trips at a batch boundary. Assertstruncated === trueand that the generator was abandoned atSECURITY_LIMITS.regexMatchBatchLineslines rather than drained. The fixture is finite on purpose, so a regression fails on the assertions instead of wedging CI.does not report truncation for a short unchanged run— a 10-line non-matchingonlyChangedLinesrun still returnstruncated: false,text: "", guarding the new check from firing where nothing was dropped.New
packages/test/src/test/task/TasksNodeExports.test.ts— static named imports of all eleven symbols from@workglow/tasks, plus type-only uses (const grepOptions: GrepOptions = {},const sedOptions: SedOptions = {}). This fails both to type-check and at runtime onmain.Verification
Both new tests were confirmed to fail without the corresponding fix:
stops at the search deadline under onlyChangedLinesfailsexpected false to be truein 38 ms — bounded, not hung — while the short-run test still passes;TasksNodeExports.test.tsfails withcreateMatcher is not a function/typeof createSedExpander === "undefined".With both fixes:
packages/testhas notsconfig.test.json, so that script does not cover the new test files; they are type-checked bypackages/test's owntsconfig.json(include: ["src/**/*"]), run clean viatsgoand as part ofbun run build:packages.bun run formatreportsAll matched files use Prettier code style!.🤖 Generated with Claude Code
https://claude.ai/code/session_01LowBJQsCghLDiHwPN6FgUT
Generated by Claude Code