PAN-1832#2003
Open
eltmon wants to merge 29 commits into
Open
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Warning Review limit reached
More reviews will be available in 41 minutes and 19 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughImplements PAN-1832: role ChangesWeighted Role Model Distribution (PAN-1832)
Sequence Diagram(s)sequenceDiagram
participant SpawnSite as Spawn Site<br/>(spawnRun/Sequencer/Planning)
participant determineModel
participant resolveModel
participant pickWeightedModelRef
participant representativeModelRef
SpawnSite->>determineModel: { role, model?, spawnKey: "role:issueId" }
determineModel->>resolveModel: role, subRole?, config, spawnKey
resolveModel->>resolveModel: sub-role model present?
alt Sub-role override
resolveModel-->>determineModel: scalar sub-role model
else Parent role check
resolveModel->>resolveModel: role model is WeightedModelRef[]?
alt Distribution form
alt Has spawnKey (spawn path)
resolveModel->>pickWeightedModelRef: entries, spawnKey
pickWeightedModelRef->>pickWeightedModelRef: hash(spawnKey) mod totalWeight
pickWeightedModelRef-->>resolveModel: sampled ModelRef
else No spawnKey (display/router)
resolveModel->>representativeModelRef: entries
representativeModelRef-->>resolveModel: highest-weight ModelRef
end
else Scalar model
resolveModel-->>determineModel: scalar ModelRef
end
end
determineModel-->>SpawnSite: resolved concrete model
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Owner
Author
Review CHANGES REQUESTED for PAN-1832Normal spawnAgent work/strike paths resolve distributions without a spawnKey, so they always pick the representative model instead of sampling. Required actionFix every blocking review finding, commit the fixes, then re-request review with:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/config-yaml.ts (1)
1829-1844:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing effort-level validation for distribution entries in config load.
When
roleConfig.modelis a distribution array, the effort-level validation (lines 1836-1843) is skipped. This means aconfig.yamlfile can specify an effort level incompatible with models in the distribution, and the error will only surface at runtime.The settings API correctly validates effort for each distribution entry (see
settings-api.tslines 564-575), but this config-load path does not.🐛 Proposed fix: validate effort for each distribution entry
if (Array.isArray(roleConfig.model)) { // Validate each distribution entry's model ref is resolvable. for (let i = 0; i < roleConfig.model.length; i++) { - derefWorkhorse(roleConfig.model[i].model, config, `roles.${role}.model[${i}].model`); + const resolvedModel = derefWorkhorse(roleConfig.model[i].model, config, `roles.${role}.model[${i}].model`); + if (roleConfig.effort !== undefined) { + const supported = getModelEffortLevelsSync(resolvedModel); + if (supported !== undefined && !supported.includes(roleConfig.effort)) { + throw new Error( + `config.yaml: roles.${role}.effort '${roleConfig.effort}' is not supported by ${resolvedModel} (supported: ${supported.join(', ')})`, + ); + } + } } - } else if (roleConfig.model) { + } else if (roleConfig.model) {🤖 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 `@src/lib/config-yaml.ts` around lines 1829 - 1844, The effort-level validation is missing for distribution array entries in roleConfig.model. When roleConfig.model is an array, the code iterates through each distribution entry and calls derefWorkhorse, but does not validate the effort level against each model like it does in the single-model branch. Add the same effort-level validation logic (checking if roleConfig.effort is defined and validating it against getModelEffortLevelsSync) inside the array branch after calling derefWorkhorse for each distribution entry, ensuring each model in the distribution supports the specified effort level before allowing the configuration to be loaded.
🧹 Nitpick comments (1)
tests/lib/settings-api.test.ts (1)
883-988: ⚡ Quick winAdd a non-integer weight rejection case in this distribution suite.
This block tests
0and negative weights, but it misses1.5, which is part of the same positive-integer contract forvalidateSettingsApiin PAN-1832.Suggested test addition
it('rejects a distribution entry with negative weight', () => { @@ expect(result.errors.some((e) => /weight must be a positive integer/.test(e))).toBe(true); }); + it('rejects a distribution entry with non-integer weight', () => { + const result = validateSettingsApi({ + ...baseProviders, + roles: { + work: { + model: [{ model: 'claude-opus-4-8', weight: 1.5 }], + }, + }, + } as ApiSettingsConfig); + expect(result.errors.some((e) => /weight must be a positive integer/.test(e))).toBe(true); + }); + it('rejects an empty distribution array', () => {🤖 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 `@tests/lib/settings-api.test.ts` around lines 883 - 988, Add a new test case in the validateSettingsApi distribution model (PAN-1832) describe block that validates the rejection of non-integer weights such as 1.5. The test should follow the same pattern as the existing weight validation tests (the ones rejecting weight 0 and negative weights), calling validateSettingsApi with a distribution entry containing a non-integer weight, and then asserting that result.errors contains an error matching the pattern for invalid weight validation.
🤖 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 @.pan/continue.json:
- Around line 1-90: The PR is committing `.pan/continue.json`, which contains
workspace-specific session state and should never be tracked in git. Remove this
file from the commit by running git rm --cached on it, then add
`.pan/continue.json` to the `.gitignore` file to prevent it from being tracked
in future commits. Ensure other .pan files remain committed:
`.pan/records/pan-1832.json` (structured decisions/hazards),
`.pan/spec.vbrief.json` (the vBRIEF plan), and `.pan/drafts/PAN-1832.md`
(planning notes) should all stay in the tree since they are tracking artifacts
meant for version control.
In `@src/dashboard/frontend/src/components/Settings/RolesPanel.tsx`:
- Around line 466-475: The issue is that scalarSavedModel is set to undefined
when the saved role model is a distribution, which breaks sub-role Parent
options and single-model picker functionality. To fix this, modify the
scalarSavedModel assignment to extract a representative scalar model from the
distribution instead of defaulting to undefined. When savedIsDistribution is
true and savedRoleModel is an array of WeightedModelRef[], extract the model
property from the first element (or use another strategy to select a
representative model from the distribution) and assign it to scalarSavedModel,
rather than leaving it undefined. This ensures the tooltip, sub-role options,
and single-model picker continue to work correctly regardless of whether the
saved role is a distribution or scalar model.
In `@src/dashboard/server/routes/agents.ts`:
- Around line 2981-2984: The spawn key being constructed in the determineModel
call uses the raw issueId from the request body, but issue IDs should be
normalized (likely to lowercase) to ensure consistent routing. Find where the
issueId is already being parsed and normalized earlier in this route handler,
and use that normalized issueId variable in the spawnKey construction instead of
extracting it directly from the request body. This ensures that issue IDs with
different casings like PAN-123 and pan-123 are treated as the same key and route
to the same model.
In `@src/lib/config-yaml.ts`:
- Around line 422-428: The fallback return statement at the end of the loop
returns entries[entries.length - 1].model without verifying the last entry has a
positive weight, which can violate the weight constraint. To fix this, introduce
a variable to track the last entry encountered with positive weight during the
loop iteration (inside the condition checking e.weight <= 0), and return that
tracked entry's model instead of unconditionally returning the last array
element. This ensures the fallback always returns a valid positive-weight entry
when floating-point accumulation doesn't quite reach 1.0.
---
Outside diff comments:
In `@src/lib/config-yaml.ts`:
- Around line 1829-1844: The effort-level validation is missing for distribution
array entries in roleConfig.model. When roleConfig.model is an array, the code
iterates through each distribution entry and calls derefWorkhorse, but does not
validate the effort level against each model like it does in the single-model
branch. Add the same effort-level validation logic (checking if
roleConfig.effort is defined and validating it against getModelEffortLevelsSync)
inside the array branch after calling derefWorkhorse for each distribution
entry, ensuring each model in the distribution supports the specified effort
level before allowing the configuration to be loaded.
---
Nitpick comments:
In `@tests/lib/settings-api.test.ts`:
- Around line 883-988: Add a new test case in the validateSettingsApi
distribution model (PAN-1832) describe block that validates the rejection of
non-integer weights such as 1.5. The test should follow the same pattern as the
existing weight validation tests (the ones rejecting weight 0 and negative
weights), calling validateSettingsApi with a distribution entry containing a
non-integer weight, and then asserting that result.errors contains an error
matching the pattern for invalid weight validation.
🪄 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
Run ID: 114653ff-57b9-4394-9744-70d8263cad5a
📒 Files selected for processing (14)
.pan/continue.json.pan/drafts/PAN-1832.md.pan/records/pan-1832.json.pan/spec.vbrief.jsonsrc/dashboard/frontend/src/components/Settings/RolesPanel.tsxsrc/dashboard/server/routes/agents.tssrc/lib/agents.tssrc/lib/backlog/sequencer-agent.tssrc/lib/cloister/review-agent.tssrc/lib/config-yaml.tssrc/lib/planning/spawn-planning-session.tssrc/lib/settings-api.tstests/lib/settings-api.test.tstests/lib/weighted-model-ref.test.ts
Comment on lines
2981
to
+2984
| spawnModel = determineModel({ | ||
| model: (body as any).model, | ||
| role, | ||
| spawnKey: `${role}:${issueId}`, |
There was a problem hiding this comment.
Normalize the issue ID before building the spawn key.
This route already parses the issue ID, but the weighted sampler key still uses raw request casing. PAN-123 and pan-123 can therefore route the same work agent to different models.
🐛 Proposed fix
spawnModel = determineModel({
model: (body as any).model,
role,
- spawnKey: `${role}:${issueId}`,
+ spawnKey: `${role}:${parsedIssueId.normalized.toUpperCase()}`,
});🤖 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 `@src/dashboard/server/routes/agents.ts` around lines 2981 - 2984, The spawn
key being constructed in the determineModel call uses the raw issueId from the
request body, but issue IDs should be normalized (likely to lowercase) to ensure
consistent routing. Find where the issueId is already being parsed and
normalized earlier in this route handler, and use that normalized issueId
variable in the spawnKey construction instead of extracting it directly from the
request body. This ensures that issue IDs with different casings like PAN-123
and pan-123 are treated as the same key and route to the same model.
Comment on lines
+422
to
+428
| for (const e of entries) { | ||
| if (e.weight <= 0) continue; | ||
| cumFraction += e.weight / totalWeight; | ||
| if (posF < cumFraction) return e.model; | ||
| } | ||
| // Floating-point accumulation may leave the final band slightly below 1.0; fall through. | ||
| return entries[entries.length - 1].model; |
There was a problem hiding this comment.
Fallback may return a non-positive-weight entry.
If the last entry in the array has weight <= 0, the fallback at line 428 returns that entry's model instead of a valid positive-weight entry. This can occur when floating-point accumulation doesn't quite reach 1.0 and the loop exits without matching.
Example: [{model: 'a', weight: 10}, {model: 'b', weight: -5}] — if posF ≈ 0.9999, no entry matches, and 'b' is returned.
🐛 Proposed fix: track last positive-weight entry
export function pickWeightedModelRef(entries: WeightedModelRef[], spawnKey: string): ModelRef {
let totalWeight = 0;
for (const e of entries) {
if (e.weight > 0) totalWeight += e.weight;
}
if (totalWeight <= 0) {
throw new Error('pickWeightedModelRef: all entries have weight <= 0');
}
// Normalize hash to [0, 1) so proportional weight sets produce identical picks.
// (7/10 and 70/100 are the same IEEE-754 double, giving the same bands.)
const posF = fnv1a32(spawnKey) / 0x100000000;
let cumFraction = 0;
+ let lastPositive: ModelRef | undefined;
for (const e of entries) {
if (e.weight <= 0) continue;
+ lastPositive = e.model;
cumFraction += e.weight / totalWeight;
if (posF < cumFraction) return e.model;
}
// Floating-point accumulation may leave the final band slightly below 1.0; fall through.
- return entries[entries.length - 1].model;
+ return lastPositive!;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const e of entries) { | |
| if (e.weight <= 0) continue; | |
| cumFraction += e.weight / totalWeight; | |
| if (posF < cumFraction) return e.model; | |
| } | |
| // Floating-point accumulation may leave the final band slightly below 1.0; fall through. | |
| return entries[entries.length - 1].model; | |
| const posF = fnv1a32(spawnKey) / 0x100000000; | |
| let cumFraction = 0; | |
| let lastPositive: ModelRef | undefined; | |
| for (const e of entries) { | |
| if (e.weight <= 0) continue; | |
| lastPositive = e.model; | |
| cumFraction += e.weight / totalWeight; | |
| if (posF < cumFraction) return e.model; | |
| } | |
| // Floating-point accumulation may leave the final band slightly below 1.0; fall through. | |
| return lastPositive!; |
🤖 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 `@src/lib/config-yaml.ts` around lines 422 - 428, The fallback return statement
at the end of the loop returns entries[entries.length - 1].model without
verifying the last entry has a positive weight, which can violate the weight
constraint. To fix this, introduce a variable to track the last entry
encountered with positive weight during the loop iteration (inside the condition
checking e.weight <= 0), and return that tracked entry's model instead of
unconditionally returning the last array element. This ensures the fallback
always returns a valid positive-weight entry when floating-point accumulation
doesn't quite reach 1.0.
Owner
Author
Review CHANGES REQUESTED for PAN-1832Distributed top-level roles break sub-role Parent model picker: value parent has no Parent option or inherited resolution. Required actionFix every blocking review finding, commit the fixes, then re-request review with:
|
…ic weighted sampler (PAN-1832) - Add WeightedModelRef interface and RoleModelRef union type to config-yaml.ts - Change RoleConfig.model from ModelRef to RoleModelRef (scalar or distribution) - Add fnv1a32: pure 32-bit FNV-1a hash (no Math.random/Date.now) - Add pickWeightedModelRef: proportional fractional sampling via hash/2^32 - Add representativeModelRef: highest-weight entry, first on tie - Fix resolveModel and validateRoleModelRefs to compile with the wider type - Unit tests covering determinism, 70/30 distribution, proportionality, throws Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Key (PAN-1832) - Add optional spawnKey param to resolveModel(role, subRole?, config, spawnKey?) - Array role model: pickWeightedModelRef(entries, spawnKey) when key provided, else representativeModelRef(entries) for display/router paths - Sub-role model always takes precedence (never samples parent distribution) - Scalar role models unchanged (back-compat) - Workhorse refs inside distribution entries resolve correctly via derefWorkhorse - Tests for all five acceptance criteria Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…in roles (PAN-1832) - validateRoleFields rejects empty arrays, non-positive/non-integer weights - validateRoleModelRefs derefWorkhorses each distribution entry's model ref - cloneRoles shallow-clones the array so mutations don't alias the merged config - Tests for all validation error cases, scalar back-compat, and merge wholesale Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n (PAN-1832) - Add allowDistribution param to validateModelRef (false by default) - Array: validate non-empty, positive-integer weights, recurse scalar checks per entry - Reject distribution on sub-role and workhorse slots (allowDistribution=false) - Pass allowDistribution=true only at roles.<role>.model call site - Effort check covers every distribution entry (must ALL support the level) - Import WeightedModelRef; 7 new unit tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…832)
- Add spawnKey?: string to determineModel(), passed to resolveModel()
- Thread \`\${role}:\${issueId}\` at: routes/agents start-agent, spawnRun
- Thread \`plan:\${issue.identifier}\` at spawn-planning-session resolveModel
- Thread \`review:\${issueId}\` at review-agent.ts (orchestrator path only, not sub-roles)
- Thread \`sequencer:global\` at sequencer-agent (no per-issue scope)
- sample-once invariant preserved: concrete model override in options returns unchanged
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… chip (PAN-1832) - add WeightedModelRef/RoleModelRef local types - widen RoleConfig.model and RoleConfigPatch.model to RoleModelRef - getRoleModel returns RoleModelRef; distributionSummaryText computes "X% model / Y% model" - scalar role renders unchanged (ModelPicker + Default chip) - distribution role renders read-only summary chip in place of ModelPicker Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tor to RolesPanel (PAN-1832) - toggle button switches a role between single-model (ModelPicker) and distribution mode - distribution editor: rows of ModelPicker + weight input + live %-per-row + remove button - "Add model" appends a row; "Save distribution" persists as WeightedModelRef[] via PUT - toggling back to single sends the highest-weight entry as a scalar ModelRef - helper text explains relative weights and per-spawn statistical distribution - sub-role pickers remain single-model (no toggle); parentModelRef guarded to scalar Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…AN-1832) Without a spawnKey, direct spawnAgent callers (pan strike, handoff) always resolved a distribution to its representative model instead of sampling. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…istribution (PAN-1832) When a top-level role has a weighted distribution, scalarSavedModel is undefined, which caused the sub-role ModelPicker to drop the "Parent" option entirely. Use the highest-weight entry as the representative parentModelRef so sub-role inheritance remains functional. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
patch.model is typed RoleModelRef (widened for distribution support) but RoleSubConfig.model is always scalar. The cast is safe — sub-role ModelPicker never emits a WeightedModelRef[]. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sync-sources/hooks/record-cost-event.js (1)
29153-29159:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
roleConfig.effortfor every distribution entry.Line 29153 dereferences each distribution model but skips the effort-support check applied in the scalar branch. This creates inconsistent validation and can let invalid role distributions pass here.
Suggested fix
- if (Array.isArray(roleConfig.model)) for (let i = 0; i < roleConfig.model.length; i++) derefWorkhorse(roleConfig.model[i].model, config, `roles.${role}.model[${i}].model`); + if (Array.isArray(roleConfig.model)) { + for (let i = 0; i < roleConfig.model.length; i++) { + const resolvedModel = derefWorkhorse(roleConfig.model[i].model, config, `roles.${role}.model[${i}].model`); + if (roleConfig.effort !== void 0) { + const supported = getModelEffortLevelsSync(resolvedModel); + if (supported !== void 0 && !supported.includes(roleConfig.effort)) { + throw new Error(`config.yaml: roles.${role}.effort '${roleConfig.effort}' is not supported by ${resolvedModel} (supported: ${supported.join(", ")})`); + } + } + } + } else if (roleConfig.model) { const resolvedModel = derefWorkhorse(roleConfig.model, config, `roles.${role}.model`);🤖 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 `@sync-sources/hooks/record-cost-event.js` around lines 29153 - 29159, The effort validation logic in the else-if branch for scalar roleConfig.model is missing from the array branch in the for loop. Add the same effort-support check inside the for loop after calling derefWorkhorse for roleConfig.model[i].model, validating that roleConfig.effort (if defined) is included in the supported levels returned by getModelEffortLevelsSync, throwing an appropriate error if the effort level is not supported by that specific model in the array.
🧹 Nitpick comments (2)
.pan/drafts/PAN-1832.md (2)
28-45: ⚡ Quick winDocument the Parent-picker behavior for distributed parents.
This section defines
parentas a sub-role-only sentinel, but it never states how a sub-role should resolve when the parent role is a distribution. That inheritance contract needs to be explicit here, since it’s one of the blocking edge cases in the PR.🤖 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 @.pan/drafts/PAN-1832.md around lines 28 - 45, Add documentation in the definitions section that explicitly explains the behavior for the inheritance contract when a sub-role uses the `parent` sentinel as its ModelRef and the parent role is a distribution. Clarify how the sub-role resolves its model in this scenario by describing whether it samples from the parent's distribution using the same spawn key, inherits a fixed choice, or follows some other resolution strategy. This should be added near the definition of the `parent` sentinel or in the **Distribution** or **Weighted (statistical) sampling** sections to make the edge case handling explicit.
97-105: ⚡ Quick winAdd the still-blocked spawnAgent paths to the checklist.
This list reads like the role-level sampling coverage is complete, but the PR summary still calls out the
spawnAgentwork/strike paths as unresolved. Please add those sites here, or mark them as open follow-up, so this draft matches the remaining implementation work.🤖 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 @.pan/drafts/PAN-1832.md around lines 97 - 105, The checklist in the diff at lines 97-105 lists role-level sampling resolve sites and display/representative resolve sites, but it does not include the spawnAgent work and strike paths that are still blocked according to the PR summary. Add the spawnAgent blocking sites to this checklist using the same format as the existing entries (file path, line number, function name), or alternatively add a section explicitly noting these as open follow-ups with relevant issue references so the documentation accurately reflects the remaining unresolved implementation work.
🤖 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 `@sync-sources/hooks/record-cost-event.js`:
- Around line 29132-29135: The loop iterating over roleConfig.model directly
accesses entry.model and entry.weight properties without first validating that
entry itself is an object; if entry is null or not an object, this will throw a
raw TypeError instead of a properly scoped config error. Add a guard check at
the beginning of the for loop to ensure entry is an object and is not null
before attempting to access its properties, throwing a descriptive error message
in the same config.yaml format as the existing validation checks if this guard
fails.
---
Outside diff comments:
In `@sync-sources/hooks/record-cost-event.js`:
- Around line 29153-29159: The effort validation logic in the else-if branch for
scalar roleConfig.model is missing from the array branch in the for loop. Add
the same effort-support check inside the for loop after calling derefWorkhorse
for roleConfig.model[i].model, validating that roleConfig.effort (if defined) is
included in the supported levels returned by getModelEffortLevelsSync, throwing
an appropriate error if the effort level is not supported by that specific model
in the array.
---
Nitpick comments:
In @.pan/drafts/PAN-1832.md:
- Around line 28-45: Add documentation in the definitions section that
explicitly explains the behavior for the inheritance contract when a sub-role
uses the `parent` sentinel as its ModelRef and the parent role is a
distribution. Clarify how the sub-role resolves its model in this scenario by
describing whether it samples from the parent's distribution using the same
spawn key, inherits a fixed choice, or follows some other resolution strategy.
This should be added near the definition of the `parent` sentinel or in the
**Distribution** or **Weighted (statistical) sampling** sections to make the
edge case handling explicit.
- Around line 97-105: The checklist in the diff at lines 97-105 lists role-level
sampling resolve sites and display/representative resolve sites, but it does not
include the spawnAgent work and strike paths that are still blocked according to
the PR summary. Add the spawnAgent blocking sites to this checklist using the
same format as the existing entries (file path, line number, function name), or
alternatively add a section explicitly noting these as open follow-ups with
relevant issue references so the documentation accurately reflects the remaining
unresolved implementation work.
🪄 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
Run ID: 5d7880b9-c887-4998-87c9-06664f64069e
⛔ Files ignored due to path filters (1)
sync-sources/hooks/record-cost-event.js.mapis excluded by!**/*.map
📒 Files selected for processing (15)
.pan/continue.json.pan/drafts/PAN-1832.md.pan/records/pan-1832.json.pan/spec.vbrief.jsonsrc/dashboard/frontend/src/components/Settings/RolesPanel.tsxsrc/dashboard/server/routes/agents.tssrc/lib/agents.tssrc/lib/backlog/sequencer-agent.tssrc/lib/cloister/review-agent.tssrc/lib/config-yaml.tssrc/lib/planning/spawn-planning-session.tssrc/lib/settings-api.tssync-sources/hooks/record-cost-event.jstests/lib/settings-api.test.tstests/lib/weighted-model-ref.test.ts
✅ Files skipped from review due to trivial changes (2)
- .pan/records/pan-1832.json
- .pan/continue.json
🚧 Files skipped from review as they are similar to previous changes (10)
- src/lib/backlog/sequencer-agent.ts
- src/dashboard/server/routes/agents.ts
- tests/lib/settings-api.test.ts
- src/lib/cloister/review-agent.ts
- tests/lib/weighted-model-ref.test.ts
- src/lib/planning/spawn-planning-session.ts
- src/lib/settings-api.ts
- .pan/spec.vbrief.json
- src/lib/config-yaml.ts
- src/dashboard/frontend/src/components/Settings/RolesPanel.tsx
Comment on lines
+29132
to
+29135
| for (let i = 0; i < roleConfig.model.length; i++) { | ||
| const entry = roleConfig.model[i]; | ||
| if (!entry.model || typeof entry.model !== "string") throw new Error(`config.yaml: roles.${role}.model[${i}].model must be a non-empty string`); | ||
| if (!Number.isInteger(entry.weight) || entry.weight <= 0) throw new Error(`config.yaml: roles.${role}.model[${i}].weight must be a positive integer`); |
There was a problem hiding this comment.
Guard invalid distribution entry shapes before property access.
Line 29134 reads entry.model without checking entry is an object; null entries throw a raw TypeError instead of a scoped config error.
Suggested fix
for (let i = 0; i < roleConfig.model.length; i++) {
const entry = roleConfig.model[i];
- if (!entry.model || typeof entry.model !== "string") throw new Error(`config.yaml: roles.${role}.model[${i}].model must be a non-empty string`);
+ if (!entry || typeof entry !== "object") throw new Error(`config.yaml: roles.${role}.model[${i}] must be an object`);
+ if (typeof entry.model !== "string" || entry.model.trim().length === 0) throw new Error(`config.yaml: roles.${role}.model[${i}].model must be a non-empty string`);
if (!Number.isInteger(entry.weight) || entry.weight <= 0) throw new Error(`config.yaml: roles.${role}.model[${i}].weight must be a positive integer`);
}🤖 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 `@sync-sources/hooks/record-cost-event.js` around lines 29132 - 29135, The loop
iterating over roleConfig.model directly accesses entry.model and entry.weight
properties without first validating that entry itself is an object; if entry is
null or not an object, this will throw a raw TypeError instead of a properly
scoped config error. Add a guard check at the beginning of the for loop to
ensure entry is an object and is not null before attempting to access its
properties, throwing a descriptive error message in the same config.yaml format
as the existing validation checks if this guard fails.
…x (PAN-1832) The stale result.json from before the `model: patch.model as ModelRef` cast (commit 4e2a43a) was blocking the pipeline. Verified locally: tsc --noEmit exits 0, Vite build clean, 7128 unit tests passed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
eltmon
pushed a commit
that referenced
this pull request
Jun 21, 2026
…(PAN-2000) The "reserves advancing slots up to the ceiling" test mocked config.js + agents.js with vi.doMock and dynamically imported tryReserveAdvancingSlot. Under the parallel run the doMock intermittently didn't apply (real config / running count leaked in), reddening main + PRs (PR #2003, two main runs) non- deterministically while passing in isolation. Give tryReserveAdvancingSlot the same injectable (counts, limits) signature as canDispatchAdvancing — defaults unchanged, so production callers (no-arg) behave identically — and rewrite the test to pass them directly, matching the deterministic "pure math" tests in the same file. No module mocking, no pollution surface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 @.pan/records/pan-1832.json:
- Around line 83-90: The reviewStatus field in the pan-1832.json record is set
to "passed" but conflicts with the actual blocking review findings mentioned in
the content. Update the reviewStatus field from "passed" to a status that
accurately reflects the blocking "CHANGES REQUESTED" state (such as
"changes-requested" or "blocked"), and ensure the readyForMerge field is set to
false to correctly represent the gate state. This will prevent misleading
automation and reporting systems that depend on accurate metadata.
🪄 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
Run ID: 4cd545ed-3f34-4e52-b86a-9df9ca5588f9
📒 Files selected for processing (5)
.pan/continue.json.pan/records/pan-1832.json.pan/test/result.jsonsrc/lib/agents.tssrc/lib/cloister/review-agent.ts
💤 Files with no reviewable changes (1)
- .pan/continue.json
✅ Files skipped from review due to trivial changes (1)
- .pan/test/result.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/cloister/review-agent.ts
- src/lib/agents.ts
…ards) Sequencer was added to main after the test was written. The --changed vitest flag now runs this test whenever RolesPanel.tsx is touched, so the stale assertion (7 cards) fails. Update count and role lists to 8. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Owner
Author
Review CHANGES REQUESTED for PAN-1832specialist-context role-level resolve site still lacks spawnKey Required actionFix every blocking review finding, commit the fixes, then re-request review with:
|
… adapter The claude-code runtime's spawnAgent path called determineModel without spawnKey, so weighted distribution sampling was non-deterministic for the work role in this code path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Owner
Author
Review CHANGES REQUESTED for PAN-1832specialist-context role-level resolve site still lacks spawnKey Required actionFix every blocking review finding, commit the fixes, then re-request review with:
|
specialist-context.ts:getDigestModel called resolveModel without a
spawnKey, so a distributed role model always resolved to the
representative entry instead of a deterministically sampled one.
Pass `${role}:${projectKey}` when subRole is undefined (top-level
digest, project-scoped key). Keep sub-role digests unkeyed — sub-roles
are always scalar. Matches the review-agent.ts pattern.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.
Issue: #1832
Acceptance Criteria
Implementation Tasks
Summary by CodeRabbit