refactor: decompose the three largest TypeScript god files into single-responsibility modules#1183
Open
willwashburn wants to merge 6 commits into
Open
refactor: decompose the three largest TypeScript god files into single-responsibility modules#1183willwashburn wants to merge 6 commits into
willwashburn wants to merge 6 commits into
Conversation
…ules Decompose the 1654-line RelaycastMessagingClient file into single- responsibility siblings behind an unchanged public surface: - relaycast-translate.ts: pure wire<->domain translation and record readers - relaycast-placement.ts: RelayPlacementError, placement payload helpers - relaycast-client.ts: structural relaycast adapter types, options, factory relaycast.ts (now 1126 lines) keeps the stateful client and re-exports RelayPlacementError and RelaycastMessagingOptions so consumers are unaffected. Pure move: no behavior change. Verified with build, full test suite (959 pass), and an SDK public-surface smoke test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NPFXDfGA74hrgEBah6KTT2
…modules Drop the export keyword on readRecord/readNumber/readBoolean/readStringArray/ readMention in relaycast-translate.ts — they are only used within the module. asRecord, readStr, and definedOptions stay exported since relaycast.ts and relaycast-client.ts consume them. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NPFXDfGA74hrgEBah6KTT2
…rocess Decompose the 1199-line HarnessDriverClient file into two single-responsibility siblings behind an unchanged public surface (client.ts now 931 lines): - spawn-request.ts: spawn input -> broker /api/spawn request-body builders and the beforeAgentSpawn patch applier - broker-process.ts: managed broker child-process lifecycle (API-URL wait, stdio drain, recent-output buffering, startup-error formatting, shutdown) BrokerExitInfo moved into broker-process.ts and is re-exported from client.ts so the package barrel surface is unchanged. Pure move: no behavior change. Verified with build, full test suite (959 pass), and a harness-driver smoke test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NPFXDfGA74hrgEBah6KTT2
Move the pure, side-effect-free workflow-path parsers out of workflows.ts (1129 -> 893 lines) into a focused workflow-paths.ts module: the YAML path parser, the TypeScript literal scanner, the parseWorkflowPaths dispatcher, and inferWorkflowFileType / shouldSyncCodeByDefault. Validation (validateYamlWorkflow / validateTypeScriptWorkflow, which shells out), the runtime PATH_NAME_RE guard, GitHub-remote helpers, and the cloud execution/API/S3 code stay in workflows.ts, which re-exports the three parser entry points so the package barrel and tests still import them from './workflows.js'. Pure move: no behavior change. Verified with build, full test suite (959 pass), and a parser smoke test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NPFXDfGA74hrgEBah6KTT2
Move the trajectory from active/ to completed/ after wrapping up the SDK/ harness-driver/cloud god-file decompositions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NPFXDfGA74hrgEBah6KTT2
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
📝 WalkthroughWalkthroughThree large TypeScript modules ( ChangesGod-file decomposition across cloud, harness-driver, and sdk packages
Agent trajectory artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cloud/src/workflow-paths.ts`:
- Around line 126-201: The functions extractPathArrayLiterals,
extractObjectLiterals, and readStringProperty scan raw source code without
skipping comments, which causes commented-out paths to be incorrectly extracted
as active workflow paths. Add comment-skipping logic to the scanner by creating
a helper function that identifies and skips single-line comments (starting with
//) and block comments (/* ... */), then integrate this helper into the
extraction functions propertyPattern and methodPattern execution loops in
extractPathArrayLiterals, and in the character-by-character scanning loops of
extractObjectLiterals and readStringProperty to ensure commented content is
never parsed as active declarations.
- Around line 52-67: The parser in the switch statement handling the key-value
pairs does not properly process YAML block scalars (indicated by | or >) for the
pushPrBody field. When a block scalar indicator like pushPrBody: | is
encountered, the code currently stores only the marker character instead of
collecting the subsequent indented lines. Modify the handling for the pushPrBody
case to detect when the value starts with | or > (block scalar indicators) and
if so, collect all following indented lines as the actual multi-line content
rather than storing just the marker. Apply the same fix to any other cases
mentioned at lines 117-119 that also handle multi-line content.
In `@packages/harness-driver/src/broker-process.ts`:
- Around line 176-189: The fast-path check in the waitForExit function only
checks if child.exitCode is not null to determine if the process has already
exited, but this misses processes that exited via signal where exitCode is null
and signalCode is not null. Update the condition to also check for
child.signalCode !== null so that processes that have already exited via signal
are detected immediately and the function can resolve without waiting for the
timeout, avoiding unnecessary SIGKILL attempts and shutdown latency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 46dc6c62-0bb3-4a0e-9463-a9862e05be94
📒 Files selected for processing (12)
.agentworkforce/trajectories/completed/2026-06/traj_q768ixcuyzh7.trace.json.agentworkforce/trajectories/completed/2026-06/traj_q768ixcuyzh7/summary.md.agentworkforce/trajectories/completed/2026-06/traj_q768ixcuyzh7/trajectory.jsonpackages/cloud/src/workflow-paths.tspackages/cloud/src/workflows.tspackages/harness-driver/src/broker-process.tspackages/harness-driver/src/client.tspackages/harness-driver/src/spawn-request.tspackages/sdk/src/messaging/relaycast-client.tspackages/sdk/src/messaging/relaycast-placement.tspackages/sdk/src/messaging/relaycast-translate.tspackages/sdk/src/messaging/relaycast.ts
Comment on lines
+52
to
+67
| const value = stripYamlScalar(text.slice(colonIndex + 1)); | ||
| switch (key) { | ||
| case 'name': | ||
| target.name = value; | ||
| break; | ||
| case 'path': | ||
| target.path = value; | ||
| break; | ||
| case 'pushBranch': | ||
| target.pushBranch = value; | ||
| break; | ||
| case 'pushBase': | ||
| target.pushBase = value; | ||
| break; | ||
| case 'pushPrBody': | ||
| target.pushPrBody = value; |
Contributor
There was a problem hiding this comment.
Handle YAML block scalars before accepting pushPrBody.
pushPrBody: | or pushPrBody: > is valid YAML and the natural way to write a multi-line PR body, but this parser stores only the marker (|/>) and ignores the following indented lines. That submits the wrong PR body instead of the declared text.
Also applies to: 117-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cloud/src/workflow-paths.ts` around lines 52 - 67, The parser in the
switch statement handling the key-value pairs does not properly process YAML
block scalars (indicated by | or >) for the pushPrBody field. When a block
scalar indicator like pushPrBody: | is encountered, the code currently stores
only the marker character instead of collecting the subsequent indented lines.
Modify the handling for the pushPrBody case to detect when the value starts with
| or > (block scalar indicators) and if so, collect all following indented lines
as the actual multi-line content rather than storing just the marker. Apply the
same fix to any other cases mentioned at lines 117-119 that also handle
multi-line content.
Comment on lines
+126
to
+201
| function findMatchingBracket(source: string, startIndex: number, open: string, close: string): number { | ||
| let depth = 0; | ||
| let quote: '"' | "'" | '`' | null = null; | ||
| let escaped = false; | ||
|
|
||
| for (let i = startIndex; i < source.length; i += 1) { | ||
| const ch = source[i] as '"' | "'" | '`' | string; | ||
| if (quote) { | ||
| if (escaped) { | ||
| escaped = false; | ||
| } else if (ch === '\\') { | ||
| escaped = true; | ||
| } else if (ch === quote) { | ||
| quote = null; | ||
| } | ||
| continue; | ||
| } | ||
| if (ch === '"' || ch === "'" || ch === '`') { | ||
| quote = ch; | ||
| continue; | ||
| } | ||
| if (ch === open) { | ||
| depth += 1; | ||
| } else if (ch === close) { | ||
| depth -= 1; | ||
| if (depth === 0) return i; | ||
| } | ||
| } | ||
|
|
||
| return -1; | ||
| } | ||
|
|
||
| function extractPathArrayLiterals(source: string): string[] { | ||
| const literals: string[] = []; | ||
|
|
||
| const propertyPattern = /\bpaths\s*:/g; | ||
| let propertyMatch: RegExpExecArray | null; | ||
| while ((propertyMatch = propertyPattern.exec(source)) !== null) { | ||
| const arrayStart = source.indexOf('[', propertyPattern.lastIndex); | ||
| if (arrayStart === -1) continue; | ||
| const arrayEnd = findMatchingBracket(source, arrayStart, '[', ']'); | ||
| if (arrayEnd !== -1) { | ||
| literals.push(source.slice(arrayStart, arrayEnd + 1)); | ||
| propertyPattern.lastIndex = arrayEnd + 1; | ||
| } | ||
| } | ||
|
|
||
| const methodPattern = /\.paths\s*\(/g; | ||
| let methodMatch: RegExpExecArray | null; | ||
| while ((methodMatch = methodPattern.exec(source)) !== null) { | ||
| const arrayStart = source.indexOf('[', methodPattern.lastIndex); | ||
| if (arrayStart === -1) continue; | ||
| const arrayEnd = findMatchingBracket(source, arrayStart, '[', ']'); | ||
| if (arrayEnd !== -1) { | ||
| literals.push(source.slice(arrayStart, arrayEnd + 1)); | ||
| methodPattern.lastIndex = arrayEnd + 1; | ||
| } | ||
| } | ||
|
|
||
| return literals; | ||
| } | ||
|
|
||
| function extractObjectLiterals(arrayLiteral: string): string[] { | ||
| const objects: string[] = []; | ||
| for (let i = 0; i < arrayLiteral.length; i += 1) { | ||
| if (arrayLiteral[i] !== '{') continue; | ||
| const end = findMatchingBracket(arrayLiteral, i, '{', '}'); | ||
| if (end === -1) break; | ||
| objects.push(arrayLiteral.slice(i, end + 1)); | ||
| i = end; | ||
| } | ||
| return objects; | ||
| } | ||
|
|
||
| function readStringProperty(objectLiteral: string, propertyName: string): string | null { | ||
| const pattern = new RegExp(`\\b${propertyName}\\s*:\\s*(['"])(.*?)\\1`, 's'); |
Contributor
There was a problem hiding this comment.
Make the TypeScript literal scanner comment-aware.
The scanner currently reads raw text, so commented-out declarations such as // { name: 'old', path: '../old' } inside a paths array, or // paths: [...], can still be extracted as active workflow paths. Since runWorkflow tarballs/uploads every parsed path, disabled paths can still be synced. Please skip line/block comments while locating paths, object literals, and properties, or switch this path to a TypeScript AST parse.
🧰 Tools
🪛 ast-grep (0.43.0)
[warning] 200-200: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\b${propertyName}\\s*:\\s*(['"])(.*?)\\1, 's')
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🪛 OpenGrep (1.22.0)
[ERROR] 163-163: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 175-175: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cloud/src/workflow-paths.ts` around lines 126 - 201, The functions
extractPathArrayLiterals, extractObjectLiterals, and readStringProperty scan raw
source code without skipping comments, which causes commented-out paths to be
incorrectly extracted as active workflow paths. Add comment-skipping logic to
the scanner by creating a helper function that identifies and skips single-line
comments (starting with //) and block comments (/* ... */), then integrate this
helper into the extraction functions propertyPattern and methodPattern execution
loops in extractPathArrayLiterals, and in the character-by-character scanning
loops of extractObjectLiterals and readStringProperty to ensure commented
content is never parsed as active declarations.
Comment on lines
+176
to
+189
| export function waitForExit(child: ChildProcess, timeoutMs: number): Promise<void> { | ||
| return new Promise((resolve) => { | ||
| if (child.exitCode !== null) { | ||
| resolve(); | ||
| return; | ||
| } | ||
| const timer = setTimeout(() => { | ||
| child.kill('SIGKILL'); | ||
| resolve(); | ||
| }, timeoutMs); | ||
| child.on('exit', () => { | ||
| clearTimeout(timer); | ||
| resolve(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Handle signaled exits in the waitForExit fast-path.
Line 178 only checks exitCode, but a process that already exited via signal has exitCode === null and signalCode !== null. In that case, this code can wait the full timeout and then attempt SIGKILL, which can add avoidable shutdown latency downstream (e.g., HarnessDriverClient.shutdown()).
Suggested fix
export function waitForExit(child: ChildProcess, timeoutMs: number): Promise<void> {
return new Promise((resolve) => {
- if (child.exitCode !== null) {
+ if (child.exitCode !== null || child.signalCode !== null) {
resolve();
return;
}
const timer = setTimeout(() => {
child.kill('SIGKILL');
resolve();
}, timeoutMs);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/harness-driver/src/broker-process.ts` around lines 176 - 189, The
fast-path check in the waitForExit function only checks if child.exitCode is not
null to determine if the process has already exited, but this misses processes
that exited via signal where exitCode is null and signalCode is not null. Update
the condition to also check for child.signalCode !== null so that processes that
have already exited via signal are detected immediately and the function can
resolve without waiting for the timeout, avoiding unnecessary SIGKILL attempts
and shutdown latency.
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
2 participants
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.
Summary
Continues the god-file decomposition pattern (prior
e7fd375did the CLI) on the three largest TypeScript files — the system's most central client/orchestration surfaces. Each change is a behavior-preserving pure move behind an unchanged public API.sdk/messaging/relaycast.tsrelaycast-translate.ts,relaycast-placement.ts,relaycast-client.tsharness-driver/client.tsspawn-request.ts,broker-process.tscloud/workflows.tsworkflow-paths.tsThe common thread: separate pure logic (wire↔domain translation, spawn-request shaping, workflow-path parsing) from the stateful/IO code (messaging client, broker-process client, cloud S3/API orchestrator). Public surfaces are preserved via re-exports, so consumers and tests are unaffected.
Per-file detail
relaycast.ts→relaycast-translate.ts(pure wire↔domain translation + record readers),relaycast-placement.ts(RelayPlacementError, placement payload helpers),relaycast-client.ts(structural relaycast adapter types, options,createRelaycastClient). Re-exportsRelayPlacementErrorandRelaycastMessagingOptions. A follow-up commit also unexports the translate readers that don't cross a module boundary (review feedback).harness-driver/client.ts→spawn-request.ts(/api/spawnbody builders + patch applier),broker-process.ts(managed broker child-process lifecycle: API-URL wait, stdio drain, recent-output buffering, startup-error formatting, shutdown).BrokerExitInfomoved and re-exported so the package barrel is unchanged.cloud/workflows.ts→workflow-paths.ts(YAML path parser + TS literal scanner +parseWorkflowPaths/inferWorkflowFileType/shouldSyncCodeByDefault). Validation (which shells out), the runtimePATH_NAME_REguard, GitHub-remote helpers, and all execution/API/S3 code stay behind; the three parser entry points are re-exported so the barrel and tests still import from./workflows.js.Verification (every step)
npm run build:coregreen, 0 TS errors.vitest run— 959 passed / 5 skipped, identical to baseline throughout.agents.list; harness-driverisProcessRunning/buffer-cap/spawn-body builders; cloud YAML+TS path parsing + re-export identity).Notes
translate.tsrecord readers withnormalize.ts's. They have genuine semantic divergence (normalize.readStringcoerces numbers→strings;readRecordclones) — consolidating would be a silent behavior change, not a refactor.CHANGELOG.mdentry: these are internal module reorganizations with no user-visible effect.🤖 Generated with Claude Code
Generated by Claude Code