refactor: package code organization wave 18 (executor pure peels) - #3317
Open
gsxdsm wants to merge 261 commits into
Open
refactor: package code organization wave 18 (executor pure peels)#3317gsxdsm wants to merge 261 commits into
gsxdsm wants to merge 261 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe PR extracts executor parsing, eligibility, refusal, prompt, command, and path utilities into dedicated modules. Executor helper modularization
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/engine/src/executor/workflow-feedback-paths.ts (1)
37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the redundant guard.
normalized.includes("/")already rejects an empty string. The!normalizedterm is unreachable. Keep only theincludes("/")check.♻️ Proposed cleanup
- if (!normalized.includes("/") || !normalized) continue; + if (!normalized.includes("/")) continue;🤖 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/engine/src/executor/workflow-feedback-paths.ts` at line 37, Update the guard in the workflow feedback path normalization loop to remove the redundant !normalized condition, keeping only the normalized.includes("/") check before continuing.packages/engine/src/__tests__/executor-task-done-shared-helper.test.ts (1)
24-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting an exact facade call-site count.
The lower bound of 2 permits silent drift in either direction. An exact count detects both a lost call site and an accidental re-introduction of a local path. If the count is expected to change often, keep the lower bound and add a short comment that states why.
🤖 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/engine/src/__tests__/executor-task-done-shared-helper.test.ts` at line 24, Update the assertion in the shared helper test to require the exact expected facade call-site count of 2 instead of using toBeGreaterThanOrEqual, so both missing and extra call sites are detected.packages/engine/src/executor/workflow-step-verdict.ts (1)
146-163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd an overload for
requireVerdict: true, or drop the overload set.The two declared overloads cover the no-options call and
{ requireVerdict: false }. A caller that passes{ requireVerdict: true }matches no overload and fails to typecheck, although the implementation supports it. A single implementation signature with an optionaloptionsparameter is sufficient here.♻️ Proposed simplification
-export function parseWorkflowStepOutput(rawOutput: string): { - output: string; - verdict?: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE"; - notes?: string; - malformed?: boolean; -}; -export function parseWorkflowStepOutput(rawOutput: string, options: { requireVerdict: false }): { - output: string; - verdict?: "APPROVE" | "APPROVE_WITH_NOTES" | "REVISE"; - notes?: string; - malformed?: boolean; -}; export function parseWorkflowStepOutput(rawOutput: string, options: { requireVerdict?: boolean } = {}): {🤖 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/engine/src/executor/workflow-step-verdict.ts` around lines 146 - 163, Update parseWorkflowStepOutput’s overload declarations to also accept options with requireVerdict: true, or remove the overload declarations and expose the implementation signature with its optional options parameter so all supported calls typecheck.
🤖 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/engine/src/executor.ts`:
- Around line 585-590: Trim unused facade imports in
packages/engine/src/executor.ts at lines 585-590, removing
determineRevisionResetStart and formatTaskDoneRefusal; at lines 601-606, remove
extractReferencedPathsFromWorkflowFeedback and delete the
WorkflowRevisionFeedbackPartition type import; and at lines 969-978, remove
parseWorkflowStepVerdict, inferWorkflowStepVerdictFromProse, and the
WorkflowStepResult type import. Leave the corresponding export statements
unchanged.
---
Nitpick comments:
In `@packages/engine/src/__tests__/executor-task-done-shared-helper.test.ts`:
- Line 24: Update the assertion in the shared helper test to require the exact
expected facade call-site count of 2 instead of using toBeGreaterThanOrEqual, so
both missing and extra call sites are detected.
In `@packages/engine/src/executor/workflow-feedback-paths.ts`:
- Line 37: Update the guard in the workflow feedback path normalization loop to
remove the redundant !normalized condition, keeping only the
normalized.includes("/") check before continuing.
In `@packages/engine/src/executor/workflow-step-verdict.ts`:
- Around line 146-163: Update parseWorkflowStepOutput’s overload declarations to
also accept options with requireVerdict: true, or remove the overload
declarations and expose the implementation signature with its optional options
parameter so all supported calls typecheck.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 91f3d2bb-39e7-4479-acd1-5f67d83cea8e
📒 Files selected for processing (7)
packages/engine/src/__tests__/executor-task-done-shared-helper.test.tspackages/engine/src/executor.tspackages/engine/src/executor/await-input-parse.tspackages/engine/src/executor/no-commit-eligibility.tspackages/engine/src/executor/task-done-refusal.tspackages/engine/src/executor/workflow-feedback-paths.tspackages/engine/src/executor/workflow-step-verdict.ts
gsxdsm
added a commit
that referenced
this pull request
Aug 3, 2026
Keep export-from re-exports for public surface; import only symbols executor.ts still references. Addresses CodeRabbit/ESLint unused-vars on wave18 PR #3317.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/engine/src/executor/prompt-derived-eligibility.ts (1)
61-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the redundant final branch in
hasSourceChangingScopeEntry.Line 68 and line 69 both lead to
return false, and the fallback on line 70 also returnsfalse. The condition on line 68 has no effect on the result. Simplify the function to remove the dead branch.♻️ Proposed simplification
if (/\.(ts|tsx|js|jsx|mjs|cjs|swift|kt|java|py|rs|go|rb|md|json|ya?ml|toml|css|scss|html)\b/.test(normalized)) return true; - if (normalized.includes("read-only") || isNoSourceScopeEntry(normalized)) return false; return false; }🤖 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/engine/src/executor/prompt-derived-eligibility.ts` around lines 61 - 70, Remove the redundant read-only/no-source conditional from hasSourceChangingScopeEntry and retain a single fallback return false after the positive source-path and file-extension checks. Preserve the existing early returns and fusion artifact handling.
🤖 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/engine/src/executor.ts`:
- Around line 382-388: Trim the mirrored facade imports in
packages/engine/src/executor.ts:382-388 and
packages/engine/src/executor.ts:545-552 to only the symbols referenced by
executor.ts, removing OPTIONAL_STEP_REVISION_KEY_MARKER and
normalizeOptionalStepRevisionKey from the optional-step-revision import, and
buildSessionWorktreePathRegex and normalizeWorktreePath from the
session-worktree-paths import. Add corresponding export-from entries only where
deep imports or mocks require symbols that are not already re-exported.
In `@packages/engine/src/executor/session-worktree-paths.ts`:
- Around line 20-25: Update formatGitRepositoryDetectionError so the remediation
command does not interpolate the raw rootDir into shell syntax. Replace it with
the established fixed placeholder such as <project-directory>, or apply POSIX
shell-safe escaping while preserving the existing error message and remedy
behavior.
---
Nitpick comments:
In `@packages/engine/src/executor/prompt-derived-eligibility.ts`:
- Around line 61-70: Remove the redundant read-only/no-source conditional from
hasSourceChangingScopeEntry and retain a single fallback return false after the
positive source-path and file-extension checks. Preserve the existing early
returns and fusion artifact handling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ea91f441-0b19-4533-bf05-fcf789b2282c
📒 Files selected for processing (9)
packages/engine/src/__tests__/executor-prompt.test.tspackages/engine/src/executor.tspackages/engine/src/executor/configured-command.tspackages/engine/src/executor/optional-step-revision.tspackages/engine/src/executor/prompt-derived-eligibility.tspackages/engine/src/executor/resume-orphan-delay.tspackages/engine/src/executor/session-worktree-paths.tspackages/engine/src/executor/skill-path-helpers.tspackages/engine/src/executor/system-prompt.ts
gsxdsm
added a commit
that referenced
this pull request
Aug 3, 2026
- Use <project-directory> in safe.directory remedy (no rootDir shell interpolation); mirror in in-process runtime warning - Drop incomplete parseWorkflowStepOutput overloads; simplify hasSourceChangingScopeEntry fallback - Ratchet evaluateTaskDoneRefusal facade call-site assertion to exact count 2
Collaborator
Author
|
Addressed remaining CodeRabbit nitpicks from the review bodies in
Fixed with
Already landed: the loop only keeps
Ratcheted to
Dropped the overload pair; single optional-options implementation signature remains.
Removed the dead read-only/no-source branch; single
Already fixed in |
Move graphFailureValue/recordedNodeValue/error texts and related merge and worktree graph-failure helpers into executor/graph-failure-pure.ts. Facade re-exports keep deep import stability.
Extract token-usage merges, branch-conflict log formatters, agent binding helpers, CLI config resolution, shell quoting, ephemeral delete race classification, review checkout routing logs, and remaining pure graph failure classifiers. Keep facade re-exports from executor.ts.
Move non-continuable completion checks, implicit task_done refusal, skip-bypass taint patches, restart/unpause resume classification, benign in-review pause-abort classification, and workflow failure scope guard builders out of TaskExecutor. Drop thin parseWorkflowStepOutput wrapper.
Implicit evaluateTaskDoneRefusal path now lives in executor/completion-predicates.ts; count facade + peel call sites.
Move preExecutionWorktreeHasWork, resolveContaminationBaseRef, resolveDiffBaseRef, and captureBaseCommitSha into executor/worktree-git-refs.ts. captureBaseCommitSha takes the store as an injected dep so TaskExecutor only wires this.store at call sites.
Extract isRegisteredWorktree, assertWorktreePathNotNested, and getWorktreeBranchMap into executor/worktree-registry-helpers.ts. Move NonRetryableWorktreeError with them and pass rootDir/store at call sites.
U4 Slice B moved captureBaseCommitSha off TaskExecutor; update the engine-core gate test to inject the mock store into the free helper.
Extract ownership/liveness helpers and cleanupStaleBranch into free modules with injected activeWorktrees/store/rootDir. Keep thin TaskExecutor wrappers so existing vi.spyOn surfaces in worktree tests remain valid.
Extract planSquashImportFromDep and reconcileSelfOwnedBeforeRemove into free modules with injected rootDir/store. Keep thin TaskExecutor wrappers for call-site and test spy stability.
…e B) Extract emitStaleLockAudit, recoverIndexLockIfStale, recoverStaleRegistration, normalizeReclaimableWorktreePath, and removeOwnWorktreeWithReconcile into free modules with injected deps. Keep thin TaskExecutor facades for spy stability.
Move sibling-branch fresh worktree allocation after live conflict into executor/worktree-fresh-after-conflict.ts with tryCreateWorktree injected via a thin TaskExecutor facade.
Move the core worktree creation/conflict recovery stack into executor/worktree-create-conflict.ts and worktree-cleanup-conflicting.ts. Wire circular callbacks via worktreeCreateConflictDeps(); keep thin TaskExecutor facades for call-site and spy stability.
… Slice B) Move resolveWorktreeStartPoint, squashImportDepIntoWorktree, rebaseNewWorktreeOntoRemote, and createWorktree into a free module with injected deps; keep thin TaskExecutor facades. Retarget primary-checkout source-scan end marker so the facade slice includes createWorktreeImpl (sourceRegion is exclusive of the end marker).
…Slice B) Move reclaimExistingWorktree/handleBranchConflict and recoverMissingWorktreeSessionStartFailure into free modules with injected deps; keep thin TaskExecutor facades for spy surfaces. Ratchet executor.ts line-count baseline 20489 → 19296.
… (U4 Slice B) Move completion worktree invariant checks and reanchor audit emission into a free module with injected deps; keep thin TaskExecutor facades. Ratchet executor.ts line-count baseline 19296 → 18973.
…leak (U4 Slice B) Move fn_task_done File Scope leak guard (workspace multi-repo + singular path) into a free module with capture* deps injected; keep thin TaskExecutor facade. Ratchet executor.ts line-count baseline 18973 → 18833.
Move captureModifiedFiles, captureWorkspaceModifiedFiles, and captureUncommittedModifiedFiles into worktree-capture-modified-files.ts; keep thin TaskExecutor facades. Ratchet executor.ts baseline 18833 → 18763.
Move executeScriptWorkflowStep and reviewWorkspacePerRepo into free modules with thin TaskExecutor facades. Ratchet executor.ts line-count baseline.
Move workflowInputRepliesAfterWatermark and resolveWorkflowInputMarkerForGraphNode into workflow-input-markers.ts with thin TaskExecutor facades. Ratchet executor.ts line-count baseline.
Move parkCompletedBlockedTask, getCompletedTaskFinalizationDecision, and shouldFinalizeCompletedTask into completion-finalization.ts with thin TaskExecutor facades. Ratchet executor.ts line-count baseline.
Move handleNonContinuableSessionError and handleNonContinuableSessionRetry into non-continuable-session.ts with injected deps; keep thin TaskExecutor facades. Ratchet executor.ts line-count baseline.
Move fn_task_add_dep tool construction into task-add-dep-tool.ts with injected session abort deps; keep thin TaskExecutor facade. Ratchet executor.ts line-count baseline.
Move implicit fn_task_done refusal requeue/fail path into task-done-refusal-handler.ts; re-export MAX_TASK_DONE_REQUEUE_RETRIES from the new module. Ratchet executor.ts line-count baseline.
Move mid-execution dependency-abort cleanup (worktree remove, branch delete, rebound replan) into dep-abort-cleanup.ts with thin TaskExecutor facade. Ratchet executor.ts line-count baseline.
Collaborator
Author
|
Re-trigger PR Checks after bag host.deps fixes (CI not firing on tip). |
Resolve executor.ts conflict by keeping the U4 peeled facade (236 LOC). Keeps host.deps bag fixes (storeRunContextDeps / sharedWorkerToolsDeps).
Collapse public+free re-exports into executor-reexports so executor.ts drops one export line. Baseline 236→235; inert-sync 2.
Move pure.* worktree ownership helpers onto TaskExecutorWorktreePureFacades and lift store/rootDir/options onto TaskExecutorState. Baseline 235→222; inert-sync 2.
Bags import pure.runConfiguredCommand so executor facades drop pure re-passing. parkCompletedBlockedTask uses FacadeRestArgs (impl default for workComplete).
Move active-session/step/subagent/configured-command helpers onto TaskExecutorSessionFacades. Baseline 222→209; inert-sync 2.
Move safeLog/pause-abort markers, task disposal, and ephemeral deletion helpers onto TaskExecutorSessionFacades. Baseline 205→199; inert-sync 2.
Move getExecutingTaskIds/isTaskActive/hasLiveSessionSurface/clearPhantom onto TaskExecutorSessionFacades. Baseline 199→190; inert-sync 2.
Move abort-in-flight, approval suspension, MCP/semaphore, and token-usage helpers onto TaskExecutorSessionFacades. Baseline 190→174; inert-sync 2.
Move signal/watchdog/rerun/completion-finalization and non-continuable session helpers onto TaskExecutorSessionFacades. Baseline 174→162; inert-sync 2.
Move recoverCompleted/pre-merge recovery, resumeTaskForAgent/resumeOrphaned, and agent runtime config helpers onto TaskExecutorSessionFacades. Baseline 162→148; inert-sync 2.
Move tryCreate/handleConflict/cleanup/createWorktree, branch-conflict reclaim, and cleanup/getWorktreePath onto TaskExecutorWorktreePureFacades. Baseline 148→133; inert-sync 2.
Move stuck/loop handlers, child terminate/spawn tools, and modified-file capture helpers onto TaskExecutorSessionFacades. Baseline 133→123; inert-sync 2.
Move task tools, worktree invariants/task-done refusal, verification fix, workflow step, and step-reset helpers onto TaskExecutorSessionFacades. Baseline 123→101; inert-sync 2.
Move executeWorkflowGraph through outer-dispatch gates onto TaskExecutorGraphFacades. TaskExecutor keeps isBackward (inert-sync 2), constructor wire, execute entry, and thin handoff/lease facades. Baseline 101→33.
Move remaining handoff/lease/gating/feature-video facades onto TaskExecutorGraphFacades. executor.ts retains port-4040 allowlist, public reexports, isBackward (inert-sync 2), constructor wire, setters, and execute/executeCore/runImplementation. Baseline 33→16.
TaskExecutor is a 13-line shell: reexports, isBackward (inert-sync 2), constructor lifecycle wire, merge/log setters, and public execute. Baseline 16→13.
One-line barrel import; executor shell is 10 LOC. Baseline 13→10; inert-sync 2.
Resolve packages/engine/src/executor.ts conflict by keeping the U4 10-line TaskExecutor shell. Pseudo-pause and other public symbols already re-export via executor-reexports/public-reexports.
Port FN-8795 findings through peeled executor modules (workflow-step-verdict, execute-workflow-step, run-graph-custom-node) so plan/code review nodes keep structured findings after the U4 peel.
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
1 participant
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
Wave 18 continues the package code-organization program after wave 17 domain folders (U4 Slice A from
docs/plans/2026-07-14-001-refactor-package-code-organization-plan.md).What changed
Peel pure, behavior-preserving helpers out of
packages/engine/src/executor.tsinto domain modules underpackages/engine/src/executor/, with stable re-exports fromexecutor.tsso deep imports andvi.mock("../executor.js")keep working.executor/task-done-refusal.tsevaluateTaskDoneRefusal,determineRevisionResetStart, skip-bypass refusal helperexecutor/workflow-feedback-paths.tsextractReferencedPathsFromWorkflowFeedback,isAlwaysAllowedScopeLeakPath,workflowPathMatchesDeclaredScopeexecutor/workflow-step-verdict.tsFUSION_WORKFLOW_STEP_CONVENTIONS_PREAMBLE,parseWorkflowStepVerdict/parseWorkflowStepOutput, step outcome typesexecutor/await-input-parse.tsparseAwaitInputSentinel,parseAwaitInputQuestionToolCallexecutor/no-commit-eligibility.tsgetNoCommitEligibilityReason(+ prompt heuristics)executor.tslive LOC ~22817 → ~22427 (first pure-peel batch; more peels needed to approach the 2k cap).Shims
old pathexecutor.tspublic exports →new pathexecutor/*.ts→ delete-when consumer deep-imports are re-pointed (not this PR)Test plan
@fusion/enginetypecheckvitest --project=engine-core(merge-gate curated suite)Stack: wave17 (merged) → this PR
Summary by CodeRabbit
New Features
Bug Fixes