feat(workspace): cue the final part back in after staggered returns - #1106
feat(workspace): cue the final part back in after staggered returns#1106seonghobae wants to merge 20 commits into
Conversation
📝 WalkthroughWalkthrough곡의 역할별 활성 상태에서 첫 번째 leftover last-return을 계산합니다. ChangesLeftover last-return 기능
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The PR adds Workspace guidance for identifying the final leftover return, but its dropout behavior is inconsistently documented and tested, the required lint gate has an error, and the English display copy does not match the intended user-facing contract. Merge should wait until these bounded correctness and validation issues are corrected. Sequence Diagram(s)sequenceDiagram
participant Workspace
participant firstLeftoverLastReturn
participant TranslationResources
Workspace->>firstLeftoverLastReturn: 곡과 활성 역할 전달
firstLeftoverLastReturn-->>Workspace: leftover last-return 결과 반환
Workspace->>TranslationResources: 안내 번역 키 선택
TranslationResources-->>Workspace: 안내 문구 반환
Workspace-->>Workspace: 안내 영역 렌더링
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 63.64% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 8 files. (8 skipped: 8 unsupported.)
✨ 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| function namedGraphNodes( | ||
| sectionValue: Record<string, unknown>, | ||
| namedRoles: NamedRoleCatalog | ||
| ): NamedGraphNode[] | null { | ||
| if (!Array.isArray(sectionValue.partGraph)) { | ||
| return null; | ||
| } | ||
|
|
||
| const nodes: NamedGraphNode[] = []; | ||
| const seenRoleIds = new Set<string>(); | ||
| for (const nodeValue of sectionValue.partGraph) { | ||
| if ( | ||
| !isRuntimeObject(nodeValue) || | ||
| !Object.prototype.hasOwnProperty.call(nodeValue, "role_id") | ||
| ) { | ||
| return null; | ||
| } | ||
|
|
||
| const roleId = meaningfulRangeText(nodeValue.role_id); | ||
| if (!roleId || !namedRoles.has(roleId) || seenRoleIds.has(roleId)) { | ||
| return null; | ||
| } | ||
|
|
||
| const active = ownActiveFlag(nodeValue); | ||
| if (active === null) { | ||
| return null; | ||
| } | ||
|
|
||
| seenRoleIds.add(roleId); | ||
| nodes.push({ roleId, active }); | ||
| } | ||
|
|
||
| return seenRoleIds.size === namedRoles.size ? nodes : null; |
There was a problem hiding this comment.
📝 Info: Complete-graph check matches current output
Current analysis emits every known role in each graph, while section roles remain active-only. The strict completeness check accepts that contract.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const sectionLabel = meaningfulRangeText(sectionValue.label); | ||
| if (!sectionLabel) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
📝 Info: Unnamed sections stay outside cue authority
meaningfulRangeText skips blank and none labels before graph evaluation. Such sections cannot become named rehearsal milestones.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
@OpenCode Apply Fresh current-head review findings
TDD/root repair on this existing lane:
|
| function selectedPartBelongs( | ||
| pending: PendingRemainingLeftover, | ||
| lastRoleId: string, | ||
| activeRole: string | null | ||
| ): boolean { | ||
| if (!activeRole) { | ||
| return true; | ||
| } | ||
| return ( | ||
| pending.originalSitOutIds.includes(activeRole) || | ||
| pending.leftoverIds.includes(activeRole) || | ||
| pending.remainingIds.includes(activeRole) || | ||
| lastRoleId === activeRole | ||
| ); | ||
| } |
There was a problem hiding this comment.
📝 Info: Selected roles receive scoped guidance
An always-active selected part gets no unrelated cue. Included parts retain the cue, and only the final returning part receives first-person instructions.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (returningLast.length > 0 && stillRemaining.length === 0) { | ||
| const trackedRemainingIds = new Set(pendingRemaining.remainingIds); | ||
| const concurrentDropout = sittingOut.some( | ||
| (node) => !trackedRemainingIds.has(node.roleId) | ||
| ); | ||
| if (concurrentDropout || returningLast.length !== 1) { | ||
| restartTrackingFromCurrentSection(sectionLabel, sittingOut); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
📝 Info: Ambiguous final returns fail closed
Multiple final returners or a concurrent new dropout restart tracking. The callout never chooses an arbitrary part from an ambiguous final transition.
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@opencode-agent Take over as the sole writer for canonical Two realistic RED regressions are now committed on Then make the narrowest state-machine repair at the first causal boundary: while A second current-head review finding Run focused RED→GREEN for the review-regression file and locale copy test, then relevant Workspace/helper tests, full desktop Vitest with configured coverage, typecheck/lint and repository quickcheck. Commit the narrow causal repairs to this same branch. Refetch the successor exact head/base and report exact RED/GREEN evidence. Resolve only |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| function templateWithAdditionalRole( | ||
| template: RehearsalSong["sections"][number], | ||
| roleId: string, | ||
| roleName: string | ||
| ): RehearsalSong["sections"][number] { | ||
| const sourceRole = template.roles[0]!; | ||
| const sourceNode = template.partGraph[0]!; | ||
| return { | ||
| ...template, | ||
| roles: [ | ||
| ...template.roles, | ||
| { | ||
| ...sourceRole, | ||
| id: roleId, | ||
| name: roleName, | ||
| overlapWarnings: [] | ||
| } | ||
| ], | ||
| partGraph: [ | ||
| ...template.partGraph, | ||
| { | ||
| ...sourceNode, | ||
| role_id: roleId, | ||
| handoff_to: [], | ||
| handoff_from: [] | ||
| } | ||
| ] | ||
| }; | ||
| } |
There was a problem hiding this comment.
📝 Info: Tied-return coverage is genuine
templateWithAdditionalRole adds acoustic guitar to both identity and activity data. The final section now returns two actual parts, exercising the multiple-return rejection.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const knownRoleIds = new Set(template.partGraph.map((node) => node.role_id)); | ||
| for (const inactiveRoleId of inactiveRoleIds) { | ||
| if (!knownRoleIds.has(inactiveRoleId)) { | ||
| throw new Error(`Unknown test role id: ${inactiveRoleId}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: Unknown fixture roles fail loudly
sectionWithInactiveRoles rejects unknown role IDs before building activity data. Misspelled or absent roles can no longer silently weaken a regression scenario.
Was this helpful? React with 👍 or 👎 to provide feedback.
seonghobae
changed the title
| const knownRoleIds = new Set(template.partGraph.map((node) => node.role_id)); | ||
| for (const inactiveRoleId of inactiveRoleIds) { | ||
| if (!knownRoleIds.has(inactiveRoleId)) { | ||
| throw new Error(`Unknown test role id: ${inactiveRoleId}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: Regression fixtures use real roles
sectionWithInactiveRoles now rejects unknown IDs instead of silently ignoring them. The tied-return case adds its fourth role to both required catalogs.
Was this helpful? React with 👍 or 👎 to provide feedback.
| sectionWithInactiveRoles(selectedTemplate, "verse-1", "verse", 0, [ | ||
| "bass-guitar", | ||
| "keys-right", | ||
| "lead-vocal" | ||
| ]), | ||
| sectionWithInactiveRoles(selectedTemplate, "chorus-1", "chorus", 20, [ | ||
| "keys-right", | ||
| "lead-vocal" |
There was a problem hiding this comment.
📝 Info: Selected-role sequence stays unrelated
The first sequence uses valid demo roles while the selected part remains active. selectedPartBelongs rejects it, preserving the later asserted result.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Queued @opencode-agent for PR #1106 at head |
1 participant
Product outcome
When an earlier reduction returns in stages, the rehearsal map names the one unambiguous final part back in and gives either that selected player a direct come-in cue or the band a count-in cue. Ambiguous simultaneous final returns, incomplete activity evidence, and intervening untracked dropouts fail closed.
Exact current identity
develop@749511c3ad4000090048718f685c6bee6b3d2c25fcf201a904586733171d116d76e2295cb96013cbfeat/workspace-first-leftover-last-returnCurrent contract
firstLeftoverLastReturnrequires a reduction, a partial return that leaves a named leftover cohort, another staggered return that still leaves at least one part out, and then a unique final return.final re-entry, who is last back, where the band began returning, and the concrete count-in/come-in action), not internal state-machine jargon.Exact failure repaired this run
Release-preflight on predecessor exact head
09b32bffbde5e5a9c25e7a789f0656d47704658bfailed in the desktop test suite atfirstLeftoverLastReturn.review-regressions.test.tswhile claiming to exercise a simultaneous final return. The fixture usedacoustic-guitar, but the demo song contains onlybass-guitar,keys-right, andlead-vocal; the helper silently ignored the unknown ID. The scenario therefore contained one real final return and correctly produced Lead Vocal instead of exercising the intended tied-return branch.The current test fixture now adds
acoustic-guitarto bothrolesandpartGraphbefore constructing the tied-return sequence. The section builder also rejects unknown fixture IDs instead of silently turning them into no-ops. The selected-role continuation regression was repaired to use three actual demo roles for its earlier unrelated complete return, preserving the intended state-machine path without fabricated IDs.Production tie handling was independently re-read at the predecessor and merge refs: it already rejects
returningLast.length !== 1; no product-code weakening or symptom workaround was introduced to satisfy the test.Security / ownership boundary
This helper consumes already-loaded
RehearsalSongdata only. It gains no filesystem, network, IPC, subprocess, WebView, model, export, dependency, or credential authority. No gate, security threshold, dependency policy, or central.githubcontract is weakened.Verification contract
All predecessor CI/review evidence is historical after
fcf201a904586733171d116d76e2295cb96013cb. Repository CI/build/release/security/SAST/SBOM workflows were freshly dispatched for this exact head and are currently queued, so they are not passing evidence.Merge only after one unchanged exact head has every applicable protected required check terminal-success, current-head coverage/security/SAST/SBOM/supply-chain/release evidence, zero valid unresolved actionable findings, and a qualifying independent non-author approval. Queued, pending, skipped-required, cancelled, neutral, failed, stale, predecessor, protected-base, self/author, status-only, model-only, synthetic, or administrative-bypass evidence is non-passing.