fix(executor): scope legacy empty-branch environment fallback per repository - #3595
Conversation
GitHub Actions
Active
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe change scopes the legacy empty-branch fallback per repository. Branch-scoped rows suppress the fallback even without a worktree ID. New tests and documentation cover canonical matching and reuse validation. ChangesWorkspace reuse matching
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The workspace reuse matching fix is covered by regression tests for both matching and attachment behavior, with no remaining merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description clearly explains the defect, implementation, scope, and validation. However, it does not follow the required template because it omits the required Checklist, uses a Summary heading instead of direct summary prose, uses non-template section names, and retains a prohibited auto-generated attribution footer. Resolution Add the complete required Checklist unchanged, move the summary into direct prose without a Summary heading, use the required Validation heading, remove unnecessary non-template sections if they add no value, and remove the auto-generated attribution footer. Full details: Docstring CoverageExplanation Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files. (3 skipped: 3 unsupported.) ✨ 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. A rabbit checks each branch with care Comment |
|
| Filename | Overview |
|---|---|
| apps/backend/internal/orchestrator/executor/executor_environment_reuse.go | Replaces global legacy-fallback gating with a repository-specific scoped-row check; behavior is sound, but the helper comment violates the production-comment convention. |
| apps/backend/internal/orchestrator/executor/executor_environment_test.go | Adds a focused same-repository regression but omits the mixed-repository case that distinguishes global from per-repository gating. |
| apps/backend/internal/orchestrator/executor/executor_environment_reuse_inventory_test.go | Verifies admission with a local scoped row and stale legacy row, again using only one repository. |
| docs/specs/tasks/system-design/additional-session-workspace-reuse.md | Documents that legacy fallback is suppressed per repository once any scoped branch row exists for that repository. |
| docs/plans/workspace-reuse-empty-branch-fallback/plan.md | Accurately describes the fix but retains draft, unchecked, and pending metadata after implementation. |
| docs/plans/workspace-reuse-empty-branch-fallback/task-01-per-repo-empty-branch-fallback.md | Defines the implemented work order but still records it as pending with no results. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
S[Repository and branch slot] --> R{Same repository has any scoped branch row?}
R -- Yes --> M[Match only the requested scoped branch row]
R -- No --> L{Legacy empty-branch row exists?}
L -- Yes --> F[Allow legacy row as fallback]
L -- No --> N[No canonical match]
M --> C{Exactly one match?}
F --> C
N --> X[Refuse workspace reuse]
C -- Yes --> A[Admit workspace reuse]
C -- No --> X
Reviews (1): Last reviewed commit: "fix(executor): scope legacy empty-branch..." | Re-trigger Greptile
|
Claude finished @WaleWangPW's task in 3m 3s —— View job PR Review
Reviewed head FindingsSuggestion (recommended, doesn't block)
AnalysisThe core logic is correct.
The function's status-agnostic behavior (it does not check The asymmetry between Summary
Verdict: Ready with suggestions — the fix is behaviorally correct. The three suggestions (comment wording, missing cross-repo test, stale plan metadata) can be addressed in a follow-up or in this PR before merge. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4eb2086f06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
GitHub Actions
Active
|
Thanks for the review feedback. Commit 9ba038e adds mixed-repository regression coverage, clarifies the scoped-row invariant, and updates the plan and work order with completed status and verification results. Focused and full executor tests plus specification lint pass. |
This branch was successfully deployed
2 participants
Tip
PR walkthrough: Open the visual walkthrough
Summary
Fixes a residual workspace-reuse defect: the legacy empty-branch environment fallback was gated globally instead of per repository.
canonicalInventoryMatchesusedhasBranchScopedEnvironmentRepoRowsto decide whether a legacy empty-branch row could satisfy a slot. That helper returns true when any repository has a branch-scoped row, so a repository that legitimately published a scoped row (a local executor's scoped row carries an empty worktree ID) still had its legacy empty-branch row accepted. A stale legacy row could then over-match the slot after the scoped row already satisfied it.Change
repositoryHasBranchScopedRepoRow(rows, repositoryID), which checks only the target repository instead of the whole inventory.Scope
apps/backend/internal/orchestrator/executor/executor_environment_reuse.goexecutor_environment_reuse_inventory_test.goandexecutor_environment_test.godocs/specs/tasks/system-design/additional-session-workspace-reuse.mddocs/plans/workspace-reuse-empty-branch-fallback/Testing
go test ./internal/orchestrator/executor/passes.