refactor: package code organization wave 20 (self-healing workspace peels) - #3424
refactor: package code organization wave 20 (self-healing workspace peels)#3424gsxdsm wants to merge 3 commits into
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change extracts self-healing workspace logic into shared modules and delegates existing methods through a reconciliation host. It also centralizes archived mailbox-tab checks, updates lifecycle census coverage, forwards ChangesWorkspace self-healing
Lifecycle census classification
Task recommendation options
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🔵 Low · up to This refactor moves workspace reconciliation and proof helpers behind the existing manager wrappers, but sweep counts and summary logs still treat dropped or non-wired enqueue attempts as recoveries. That can misstate self-healing effectiveness and hinder diagnosis, so the PR is mergeable with explicit owner follow-up. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/engine/src/self-healing/workspace-reconcile.ts (1)
140-157: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the per-task workflow IR resolution.
The loop calls
resolveWorkflowIrForTaskWithProvenanceonce per review-lane task, sequentially, before any candidate filter runs. On projects with many review-lane tasks this serializes one resolver round trip per task on every sweep. Two cheaper options exist: apply the store-free parts of the candidate predicate (isWorkspaceTask,mergeConfirmed,ACTIVE_MERGE_STATUSES) first and resolve only survivors, or resolve in a bounded concurrent window asTriageProcessor.discoverReadyPlanningTasksdoes.🤖 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/self-healing/workspace-reconcile.ts` around lines 140 - 157, Bound the per-task calls to resolveWorkflowIrForTaskWithProvenance in the workspace reconciliation loop: filter tasks using the store-free candidate checks (isWorkspaceTask, mergeConfirmed, and ACTIVE_MERGE_STATUSES) before resolving, and process remaining resolutions with bounded concurrency rather than sequentially. Preserve the existing wsPartialLanes fallback behavior for resolver failures.packages/engine/src/self-healing.ts (1)
994-999: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the private wrappers to avoid shadowing the imported predicates.
isWorkspaceTaskLiveandisWorkspaceOwnerLiveare both method names and imported function names. The bodies resolve to the module imports because the methods needthis., so behavior is correct today. A future edit that addsthis.inside either body creates infinite recursion. Import the functions under...Implaliases, as the file already does for the other extracted helpers.♻️ Proposed refactor
-import { isWorkspaceTaskLive, isWorkspaceOwnerLive } from "./self-healing/workspace-liveness.js"; +import { + isWorkspaceTaskLive as isWorkspaceTaskLiveImpl, + isWorkspaceOwnerLive as isWorkspaceOwnerLiveImpl, +} from "./self-healing/workspace-liveness.js";private isWorkspaceTaskLive(task: Task): { live: boolean; livePaths: string[] } { - return isWorkspaceTaskLive(task, this.options.isTaskActive); + return isWorkspaceTaskLiveImpl(task, this.options.isTaskActive); } private isWorkspaceOwnerLive(owner: Task | null | undefined, completeColumns: ReadonlySet<string>): boolean { - return isWorkspaceOwnerLive(owner, completeColumns); + return isWorkspaceOwnerLiveImpl(owner, completeColumns); }🤖 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/self-healing.ts` around lines 994 - 999, Alias the imported isWorkspaceTaskLive and isWorkspaceOwnerLive predicates with Impl-style names, matching the existing helper import convention, and update both private wrapper methods to call those aliases. Keep the wrapper method names and behavior unchanged.
🤖 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/self-healing.ts`:
- Around line 266-270: Remove the unused PRE_EXECUTION_WORKTREE_MAX_IDLE_MS
import from the self-healing.ts import list, leaving
PHANTOM_EXECUTOR_BINDING_AGE_MULTIPLIER and MAX_STARVATION_DROPS unchanged.
In `@packages/engine/src/self-healing/workspace-reconcile.ts`:
- Around line 187-190: Update the active-merge-task guard in the workspace
reconciliation flow to pass the `"merge-pending"` reason to
emitWorkspacePartialLandNoAction instead of `"live-worktree"`, while preserving
the existing host, task, and livePaths arguments.
- Around line 418-432: Replace the raw Git error text stored in reason within
the orphaned-workspace reconciliation catch block with a bounded outcome code,
while preserving the existing success/failure audit metadata structure. Keep the
full error detail in a separate failureDetail value and use it only in the
log.warn call associated with this reconciliation flow; do not include
failureDetail or error prose in createRunAuditor metadata.
- Around line 255-271: Update enqueueWorkspaceMergeBounded to return true only
for re-enqueue and park-failed outcomes, and false for re-enqueue-noop and
re-enqueue-dropped. In both call sites shown, capture the helper result and
increment recovered only when it is true so the returned count and summary log
exclude no-op and dropped tasks.
---
Nitpick comments:
In `@packages/engine/src/self-healing.ts`:
- Around line 994-999: Alias the imported isWorkspaceTaskLive and
isWorkspaceOwnerLive predicates with Impl-style names, matching the existing
helper import convention, and update both private wrapper methods to call those
aliases. Keep the wrapper method names and behavior unchanged.
In `@packages/engine/src/self-healing/workspace-reconcile.ts`:
- Around line 140-157: Bound the per-task calls to
resolveWorkflowIrForTaskWithProvenance in the workspace reconciliation loop:
filter tasks using the store-free candidate checks (isWorkspaceTask,
mergeConfirmed, and ACTIVE_MERGE_STATUSES) before resolving, and process
remaining resolutions with bounded concurrency rather than sequentially.
Preserve the existing wsPartialLanes fallback behavior for resolver failures.
🪄 Autofix
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: dc2df962-d38f-4438-9b62-1d6753712e24
📒 Files selected for processing (6)
packages/engine/src/self-healing.tspackages/engine/src/self-healing/backward-move-proof.tspackages/engine/src/self-healing/sweep-constants.tspackages/engine/src/self-healing/workspace-liveness.tspackages/engine/src/self-healing/workspace-reconcile.tsscripts/line-count-baseline.json
Drop unused peel leftovers so Lint is green, alias workspace liveness helpers to avoid wrapper shadowing, and persist closed git-remove outcomes in run-audit instead of error prose.
Nitpick follow-up from the review body:
The two other inline items that would change recovery audit/metrics ( |
Drop unused peel leftovers so Lint is green, alias workspace liveness helpers to avoid wrapper shadowing, and persist closed git-remove outcomes in run-audit instead of error prose.
1132723 to
c90b75d
Compare
|
Rebased onto current Lint was still red after the unused-import fix because FN-9014's mailbox folder tab Recovery contracts from the last review pass are unchanged ( |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
| Filename | Overview |
|---|---|
| packages/engine/src/self-healing.ts | Replaces inline workspace recovery logic with host-backed calls into extracted modules. |
| packages/engine/src/self-healing/backward-move-proof.ts | Houses the extracted triple-proof gate, recent-activity check, and no-action notification behavior. |
| packages/engine/src/self-healing/workspace-liveness.ts | Centralizes workspace task and owner liveness classification. |
| packages/engine/src/self-healing/workspace-reconcile.ts | Contains the extracted workspace reconcilers. |
| packages/engine/src/self-healing/sweep-constants.ts | Centralizes starvation, idle, phantom-binding, and teardown timing budgets. |
| scripts/lib/lane-wiring-baseline.json | Moves the lane-wiring allowance from the original class file to the extracted reconciler. |
| scripts/line-count-baseline.json | Updates the self-healing source line-count baseline. |
Reviews (12): Last reviewed commit: "Merge origin/main into feature/code-orga..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Drop unused peel leftovers so Lint is green, alias workspace liveness helpers to avoid wrapper shadowing, and persist closed git-remove outcomes in run-audit instead of error prose.
a7fe8c6 to
d3c68da
Compare
|
Rebased onto current Dropped our Mailbox |
Drop unused peel leftovers so Lint is green, alias workspace liveness helpers to avoid wrapper shadowing, and persist closed git-remove outcomes in run-audit instead of error prose.
d3c68da to
37e68e7
Compare
Drop unused peel leftovers so Lint is green (basename/dirname and the base-branch imports that moved with the reconcilers). Move the skipColumnIdentityCheck lane-wiring allowance with getTaskMergeBlocker, and ratchet the self-healing.ts line-count baseline to the post-peel live count.
23c4fc2 to
55c44de
Compare
ThreatCrush Security Scan4610 finding(s) HIGH/CRITICAL: 42 | MEDIUM: 4071 | LOW: 497
…and 4560 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
…eels) U5 Slice B on latest main: peel workspace reconcilers and the backward-move triple-proof gate out of SelfHealingManager. Public class methods stay on the stable path as thin wrappers. - workspace-liveness.ts - backward-move-proof.ts - workspace-reconcile.ts (partial-land, phantom lease, pre-exec, orphan worktrees) - sweep-constants.ts - self-healing.ts 16983 → 16118 Fold post-peel main invariants (workspaceMode triple-proof, confirmed-scope recovery, recorded base-branch resolution, mergeTransientRetryCount, and isFusionDeletableBranch teardown) into the extracted modules.
Drop unused peel leftovers so Lint is green (basename/dirname and the base-branch imports that moved with the reconcilers). Move the skipColumnIdentityCheck lane-wiring allowance with getTaskMergeBlocker, and ratchet the self-healing.ts line-count baseline to the post-peel live count.
bec9200 to
e7754d2
Compare
|
Automated fix: resolved merge conflicts and rebased onto current main. |
Resolve 7 conflicts in packages/engine/src/self-healing.ts: - Imports: union main's newer symbols (basename/dirname retained for inline reconcilers; isStaleContentApprovalBlocker, cleanup helpers). - Drop obsolete classifyAutoArchiveFailure (auto-archive retired); keep TERMINAL_WORKSPACE_WORKTREE_TEARDOWN_MIN_IDLE_MS. - workspaceOwnerTerminalReason/isWorkspaceOwnerLive: take main's 2-arg archive-free versions. - reclaimPhantomWorkspaceLandLeases / reconcileOrphanedWorkspaceWorktrees: take main's newer inline implementations (acquire-lease sweep, failed-lane-only teardown floor); remove stale extracted duplicates from workspace-reconcile.ts and unused imports/host callbacks.
1 participant
Summary
Continues U5 after wave 19 (
#3403) landed.Peels the workspace reconcile cluster and the shared backward-move triple-proof helpers out of
self-healing.ts.SelfHealingManagerkeeps thin wrappers so public methods and sweep wiring stay on the stable class path.Peels
self-healing/workspace-liveness.tsisWorkspaceTaskLive,isWorkspaceOwnerLiveself-healing/backward-move-proof.tsself-healing/workspace-reconcile.tsself-healing/sweep-constants.tsLine count
self-healing.ts: 16024 → 15367 (baseline ratcheted to post-peel live)Test plan
self-healing-workspace.test.tsself-healing-trait-rekey.test.tsself-healing-reclaim-paused-review.test.tsself-healing-reclaim-live-zero-commits.test.tsself-healing-paused-abort-recovery.test.tsFollow-ups
U5 Slice C: startup / in-review / merge-status / dependency reconcile clusters.
Summary by CodeRabbit
Bug Fixes
Tests