fix(FN-166): bypass coordination-only overlap leases#1172
Merged
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:
📝 WalkthroughWalkthroughAdds detection for coordination-only/no-commit tasks and bypasses file-scope overlap leases for such tasks: new exported helper ChangesCoordination-only overlap bypass
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 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: 1
🧹 Nitpick comments (3)
packages/engine/src/scheduler.ts (1)
129-132: 💤 Low valueGlob pattern in exact-match set may be intentional but is potentially confusing.
The entry
".changeset/*.md"inCOORDINATION_SAFE_SCOPE_EXACTwill only match if a scope entry is literally the string".changeset/*.md"(e.g., from a PROMPT.md scope declaration), not actual files like.changeset/fn-166.md. SinceCOORDINATION_SAFE_SCOPE_PREFIXESincludes".changeset/"which covers all changeset files, this works correctly.If the intent is to match literal glob scope entries, consider adding a clarifying comment. Otherwise, this entry is redundant given the prefix coverage.
🤖 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/scheduler.ts` around lines 129 - 132, COORDINATION_SAFE_SCOPE_EXACT contains the string ".changeset/*.md" which only matches a literal scope equal to that string and is redundant because COORDINATION_SAFE_SCOPE_PREFIXES already includes ".changeset/"; either remove the ".changeset/*.md" entry from COORDINATION_SAFE_SCOPE_EXACT or add a brief clarifying comment next to COORDINATION_SAFE_SCOPE_EXACT explaining that the entry is intentional only for literal-scope matches (and that prefix coverage is handled by COORDINATION_SAFE_SCOPE_PREFIXES) so future readers understand the difference; reference COORDINATION_SAFE_SCOPE_EXACT, COORDINATION_SAFE_SCOPE_PREFIXES, and the ".changeset/*.md" entry when making the change.packages/engine/src/__tests__/scheduler.test.ts (2)
236-283: 💤 Low valueConsider adding edge case coverage for
isCoordinationOnlyTask.The test suite covers core scenarios well, but a few edge cases could strengthen coverage:
- Empty scope array (should return false per upstream contract)
decisionOnly: truein sourceMetadata (alternative coordination signal per context snippet 1)- Tasks with only direct
noCommitsExpectedproperty vs onlysourceMetadata🤖 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__/scheduler.test.ts` around lines 236 - 283, Add unit tests for isCoordinationOnlyTask using createMockTask to cover missing edge cases: (1) assert false when scope array is empty; (2) assert true when sourceMetadata contains decisionOnly: true (treat as coordination signal); and (3) add explicit comparisons for tasks that set noCommitsExpected directly vs tasks that set sourceMetadata.noCommitsExpected to ensure both code paths behave identically. Place these new it blocks alongside the existing describe("isCoordinationOnlyTask") tests and use the same scope arrays and helper to mirror current test patterns.
285-320: ⚡ Quick winConsider testing empty/undefined
activeScopesedge case.The function explicitly handles
!activeScopes || activeScopes.size === 0by returningtrue(per context snippet 2), but this branch is not explicitly covered. Adding a test case would document this behavior and prevent regression.Suggested test case
+ it("accepts queued overlap candidates when no active scopes exist", () => { + const candidate = createMockTask({ id: "FN-050", status: "queued" }); + + expect(isRunnableQueuedOverlapCandidate(candidate, [candidate], now, undefined, ["src/app.ts"])).toBe(true); + expect(isRunnableQueuedOverlapCandidate(candidate, [candidate], now, new Map(), ["src/app.ts"])).toBe(true); + });🤖 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__/scheduler.test.ts` around lines 285 - 320, Add a test to cover the empty/undefined activeScopes edge case for isRunnableQueuedOverlapCandidate: create a queued runnable task (e.g., via createMockTask with status "queued") and assert that isRunnableQueuedOverlapCandidate(runnable, [runnable], now, undefined, someFileList) returns true, and also assert the same when passing an empty Map (new Map()) as activeScopes; place these assertions alongside the existing "rejects queued overlap candidates blocked by active file-scope leases" tests so the branch where !activeScopes || activeScopes.size === 0 is explicitly exercised.
🤖 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/__tests__/scheduler-overlap-starvation.test.ts`:
- Around line 206-232: The test setup doesn't trigger the coordination-only
bypass because FN-158 has no overlapBlockedBy and the store scopes don't
actually overlap with FN-118; update the test so overlap is detected or pre-seed
the blocked field: either (A) change the scope entries passed to createStore so
FN-118's scope includes a path that will overlap with one of FN-158's entries
(for example make FN-118 include a `.changeset/*` or the
`.fusion/tasks/FN-158/...` pattern) so pathsOverlap between the active lease in
Scheduler and FN-158 is true, or (B) set FN-158's initial task object (via
makeTask) to include overlapBlockedBy: "FN-118" to exercise the
coordination-only clearing branch in Scheduler when you mark (scheduler as
any).running = true and call scheduler.schedule(); ensure references to
makeTask, createStore and Scheduler are updated accordingly.
---
Nitpick comments:
In `@packages/engine/src/__tests__/scheduler.test.ts`:
- Around line 236-283: Add unit tests for isCoordinationOnlyTask using
createMockTask to cover missing edge cases: (1) assert false when scope array is
empty; (2) assert true when sourceMetadata contains decisionOnly: true (treat as
coordination signal); and (3) add explicit comparisons for tasks that set
noCommitsExpected directly vs tasks that set sourceMetadata.noCommitsExpected to
ensure both code paths behave identically. Place these new it blocks alongside
the existing describe("isCoordinationOnlyTask") tests and use the same scope
arrays and helper to mirror current test patterns.
- Around line 285-320: Add a test to cover the empty/undefined activeScopes edge
case for isRunnableQueuedOverlapCandidate: create a queued runnable task (e.g.,
via createMockTask with status "queued") and assert that
isRunnableQueuedOverlapCandidate(runnable, [runnable], now, undefined,
someFileList) returns true, and also assert the same when passing an empty Map
(new Map()) as activeScopes; place these assertions alongside the existing
"rejects queued overlap candidates blocked by active file-scope leases" tests so
the branch where !activeScopes || activeScopes.size === 0 is explicitly
exercised.
In `@packages/engine/src/scheduler.ts`:
- Around line 129-132: COORDINATION_SAFE_SCOPE_EXACT contains the string
".changeset/*.md" which only matches a literal scope equal to that string and is
redundant because COORDINATION_SAFE_SCOPE_PREFIXES already includes
".changeset/"; either remove the ".changeset/*.md" entry from
COORDINATION_SAFE_SCOPE_EXACT or add a brief clarifying comment next to
COORDINATION_SAFE_SCOPE_EXACT explaining that the entry is intentional only for
literal-scope matches (and that prefix coverage is handled by
COORDINATION_SAFE_SCOPE_PREFIXES) so future readers understand the difference;
reference COORDINATION_SAFE_SCOPE_EXACT, COORDINATION_SAFE_SCOPE_PREFIXES, and
the ".changeset/*.md" entry when making the change.
🪄 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: f28a7c81-89e4-4143-81d1-00a7bb55d3be
📒 Files selected for processing (5)
.changeset/fn-166-coordination-overlap-bypass.mdpackages/engine/src/__tests__/reliability-interactions/scheduler-overlap-priority-inversion.test.tspackages/engine/src/__tests__/scheduler-overlap-starvation.test.tspackages/engine/src/__tests__/scheduler.test.tspackages/engine/src/scheduler.ts
Contributor
Greptile SummaryThis PR fixes a scheduler starvation bug (FN-166) where coordination/no-commit tasks (e.g. backlog audits, doc-only tasks) could be indefinitely blocked behind active implementation file-scope leases, even when their scopes only touch safe read-only paths like
Confidence Score: 5/5Safe to merge. The bypass is gated on an explicit opt-in flag and a scope allow-list, so it cannot fire on tasks that haven't declared themselves coordination-only. The coordination bypass requires both an explicit No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Schedule pass: build activeScopes] --> B{groupOverlappingFiles?}
B -- No --> Z[Skip overlap logic]
B -- Yes --> C[For each in-progress / in-review task]
C --> D{isCoordinationOnlyTask?}
D -- Yes --> E[Skip: no scope lease registered]
D -- No --> F[setActiveScopeLease]
F --> G[Build queuedHigherPriorityScopes from todo tasks]
G --> H{isCoordinationOnlyTask todo?}
H -- Yes --> I[Skip: not added to queued scopes]
H -- No --> J[Add to queuedHigherPriorityScopes if runnable]
J --> K[Process each todo task for dispatch]
I --> K
K --> L{taskScope.length > 0 AND NOT coordinationOnly?}
L -- Yes --> M[Run overlap check against activeScopes]
M --> N{Overlap found?}
N -- Yes --> O[Set overlapBlockedBy, continue]
N -- No --> P[Clear stale overlapBlockedBy if present]
P --> Q[Check concurrency → dispatch]
L -- No --> R{coordinationOnly AND overlapBlockedBy set?}
R -- Yes --> S[Clear overlapBlockedBy, log bypass]
S --> Q
R -- No --> Q
Q --> T{Dispatched?}
T -- Yes --> U{isCoordinationOnlyTask?}
U -- Yes --> V[Do NOT register post-dispatch scope lease]
U -- No --> W[setActiveScopeLease in-progress]
Reviews (5): Last reviewed commit: "fix(FN-166): bypass coordination-only ov..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
plarson
force-pushed
the
fix/fn166-scheduler-starvation
branch
3 times, most recently
from
fc70545 to
60a724b
Compare
plarson
force-pushed
the
fix/fn166-scheduler-starvation
branch
from
60a724b to
eb8ad24
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/scheduler.ts`:
- Around line 135-143: isCoordinationOnlyTask currently ignores the scope and
returns true based solely on metadata, allowing tasks with real implementation
files to bypass overlap leases; change the function to use the scope parameter
(rename _scope to scope) and only return true when the explicitNoCommitSignal is
true AND the scope is verified coordination-safe—e.g., call or add a helper like
isScopeCoordinationSafe(scope) (or iterate scope entries and ensure none are
implementation/source files) before allowing the bypass so metadata alone cannot
override unsafe scopes.
🪄 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: 21ea07d9-b090-49ff-b873-8e85ef902b28
📒 Files selected for processing (6)
.changeset/fn-166-coordination-overlap-bypass.mdpackages/desktop/src/__tests__/release-workflow.test.tspackages/engine/src/__tests__/reliability-interactions/scheduler-overlap-priority-inversion.test.tspackages/engine/src/__tests__/scheduler-overlap-starvation.test.tspackages/engine/src/__tests__/scheduler.test.tspackages/engine/src/scheduler.ts
✅ Files skipped from review due to trivial changes (2)
- packages/desktop/src/tests/release-workflow.test.ts
- .changeset/fn-166-coordination-overlap-bypass.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/engine/src/tests/scheduler-overlap-starvation.test.ts
- packages/engine/src/tests/reliability-interactions/scheduler-overlap-priority-inversion.test.ts
Fusion-Task-Id: FN-166
plarson
force-pushed
the
fix/fn166-scheduler-starvation
branch
from
eb8ad24 to
f8bda56
Compare
gsxdsm
approved these changes
May 30, 2026
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
Verification
COREPACK_ENABLE_DOWNLOAD_PROMPT=0 corepack pnpm --filter @fusion/engine exec vitest run src/__tests__/scheduler-overlap-starvation.test.ts src/__tests__/scheduler.test.ts src/__tests__/reliability-interactions/scheduler-overlap-priority-inversion.test.ts --reporter=dotCOREPACK_ENABLE_DOWNLOAD_PROMPT=0 corepack pnpm --filter @fusion/engine typecheckCOREPACK_ENABLE_DOWNLOAD_PROMPT=0 corepack pnpm lintCOREPACK_ENABLE_DOWNLOAD_PROMPT=0 corepack pnpm buildCOREPACK_ENABLE_DOWNLOAD_PROMPT=0 corepack pnpm --filter @fusion/engine test -- --runInBandFusion-Task-Id: FN-166
Summary by CodeRabbit
Bug Fixes
Tests
Chores