refactor: dedup orchestrator setup, split executeTask, share tally - #70
Merged
Conversation
Architect review identified concrete duplication + complexity that
could be reduced without behaviour change. Three focused refactors,
414 tests still passing, 0 source semantics changed, no CACHE_VERSION
bump.
## 1. Extract `prepareRun` from `run` and `planRun`
Both entry points were duplicating ~50 lines: findWorkspaceRoot,
loadWorkspace, loadWorkspaceConfig, listProjects, loadProjectConfig
×N, buildPackageGraph, computeNestedProjectDirs, expandRequested,
buildTaskGraph, computeWorkspaceFingerprint, new Cache +
wrapWithRemoteCache. New `src/orchestrator/prepare.ts` does it once;
the two callers stay thin and handle their own divergence (run
logs + returns NOT-ok; planRun returns empty plan).
The `PreparedRun.empty` field is a small discriminated union
(`null` | `'no-tasks-declared'` | `'empty-graph'`) so callers can
produce their own messaging without re-deriving the reason. The
cache handle is constructed unconditionally so callers always have
a uniform `try { ... } finally { cache.close() }` shape.
## 2. Split `executeTask` into three named functions
The 200-line `executeTask` body contained three orthogonal paths
(group / persistent / cached) behind nested ifs. Split into
`executeGroupTask`, `executePersistentTask`, `executeCachedTask`
behind a 3-line dispatcher. `buildIsolatedEnv` was constructed
identically in two paths; hoisted to a private `taskEnv(node, step)`
helper. Naming `executeCachedTask` (rather than "normal") matches
the architect's note — every path through this function is the
"cached path" when cacheEnabled is true.
## 3. Share `tallyOutcomes` between summary + run-artifacts
`summary.ts` and `run-artifacts.ts` were each computing the same
per-status counts (successful / failed / skipped / cachedLocal /
cachedRemote / total) with subtly different filtering. New
`src/orchestrator/tally.ts:tallyOutcomes(outcomes) -> Tally` is the
single source of truth; group-task exclusion is baked in via the
shared `isGroupTask` predicate. Both surfaces now derive identical
numbers from one place.
## Small follow-ons (same PR)
- `isGroupTask(node)` predicate added to `graph/task-graph.ts`;
applied at six call sites that previously inlined
`node.config.exec === undefined`.
- `expandRequested` moved from `orchestrator.ts` to
`graph/task-graph.ts` next to `buildTaskGraph` — they're paired.
- Dead `taskId` re-export from `orchestrator.ts` removed (no
consumers; was a leftover from an earlier surface).
- `formatBriefDuration` in `framed-output.ts` was byte-identical to
`formatDuration` in `summary.ts`; replaced with an import.
- `OnDiskMeta` in `layered-cache.ts` is now
`Omit<CacheEntry, 'hash' | 'outputFiles' | 'source'>` so the
on-disk meta schema stays in sync with the cache contract
automatically.
- `SaveArgs` marked `@internal` (it's a structural shortcut for
`LayeredCache`, not part of the conceptual cache contract).
## Extension-point notes (for future)
The architect flagged where common future features plug in cleanly
after these refactors:
- **Named inputs / target defaults.** Rewrite each
`ProjectEntry.config` between project-config load and graph build
inside `prepareRun`. The seam exists.
- **OTel telemetry.** `Telemetry` sink slots into `PreparedRun` as a
default no-op; consumers (`recordRun`, the per-task callbacks)
pick it up.
- **`--output-logs` modes.** `defaultLogger(colors)` becomes
`defaultLogger({ colors, mode })`. The dispatcher in (2) makes the
per-status hooks easier.
## Lint / format / test
414 pass / 0 fail. `tests/summary.test.ts:outcome` fixture updated
to populate `node.config.exec` so `isGroupTask` returns false (was
previously implicit because the orchestrator pre-filtered group
tasks; now the shared `tallyOutcomes` does the filter itself).
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
Architect-reviewed refactor pass: dedup the most-duplicated logic, split the most-tangled function, share the most-drift-prone helper. 0 behaviour change, 414 tests still pass, no
CACHE_VERSIONbump.What changed
1. Extract
prepareRunfromrunandplanRunBoth entry points were duplicating ~50 lines: workspace discovery → config load → graph build → cache open. New
src/orchestrator/prepare.tsdoes it once; callers stay thin.PreparedRun.emptyis a small discriminated union (null/'no-tasks-declared'/'empty-graph') so callers handle empty cases their own way without re-deriving the reason.2. Split
executeTaskinto three named functionsThe 200-line
executeTaskbody had three orthogonal paths behind nested ifs. NowexecuteGroupTask,executePersistentTask,executeCachedTaskbehind a 3-line dispatcher.buildIsolatedEnvhoisted to a privatetaskEnv(node, step)helper.3. Share
tallyOutcomesbetween summary + run-artifactssummary.tsandrun-artifacts.tswere each computing the same per-status counts. Neworchestrator/tally.ts:tallyOutcomesis the single source of truth; group-task exclusion baked in.Small follow-ons in the same PR
isGroupTask(node)predicate added tograph/task-graph.ts; applied at 6 inline call sites.expandRequestedmoved fromorchestrator.tstograph/task-graph.ts(paired withbuildTaskGraph).taskIdre-export fromorchestrator.tsremoved.formatBriefDuration(byte-identical toformatDuration) replaced with an import.OnDiskMetainlayered-cache.tsderived asOmit<CacheEntry, 'hash' | 'outputFiles' | 'source'>— stays in sync with the cache contract automatically.SaveArgsmarked@internal.Extension-point notes
For the next obvious features the architect noted clean seams now exist for:
ProjectEntry.configbetween project-config load and graph build insideprepareRun.Telemetrysink slots intoPreparedRunas a no-op default.--output-logsmodes —defaultLogger({ colors, mode })extension; the per-status dispatcher in (2) makes the hooks cleaner.Diff stat
src/orchestrator.tsprepare.ts)src/orchestrator/execute-task.tssrc/orchestrator/prepare.tssrc/orchestrator/tally.tssrc/orchestrator/run-artifacts.tstallyOutcomesremoved, imports sharedsrc/orchestrator/summary.tstallyOutcomessrc/orchestrator/framed-output.tsformatDurationfromsummary.ts(was a byte-identical local copy)src/graph/task-graph.tsisGroupTask, +expandRequestedsrc/cache/layered-cache.tsOnDiskMetaderived fromCacheEntrysrc/cache/cache.tsSaveArgsJSDoc'd@internaltests/summary.test.tsnode.config.exec(was implicit before)Tests: 414 pass, 0 fail (was 414 pass, 0 fail). Net source reduction: ~120 lines.Test plan
bun src/bin.ts run lint— cleanbun src/bin.ts run format-check— cleanbun test— 414 pass, 0 failsrc/index.tsexports identical)CACHE_VERSIONbumpGenerated by Claude Code