feat(v0.20): value-per-token selection (#131) + signature resolution (#132) + SPI default-readiness criteria (#134)#138
Merged
Conversation
…132) + SPI default-readiness criteria (#134) Bundles three v0.20-context-compiler items. #135 (task-conditioned slicing v1) intentionally deferred — it's a multi-PR substrate, not a single-shot slice. ## #131 — value-per-token selection_strategy on compileContextPack - New optional input field: selection_strategy: 'evidence-order' | 'value-per-token' (default unchanged) - When 'value-per-token': required-evidence-class candidates are placed first (must-include), then remaining optional candidates go through selectByValuePerToken with the residual budget - Score for value-per-token uses the candidate's position in orderedNodes as a rank proxy (1/(idx+1)) — earlier candidates rank higher - Required candidates can't be dropped even if expensive; optional candidates compete by density (score / token_cost) ## #132 — 'signature' resolution level - Extended ContextPackResolution from 'detail' | 'summary' | 'mixed' to also accept 'signature' - Signature mode keeps the first 1-2 lines of each snippet (function/class signature) and drops the body - Heuristic: keep lines up through the one ending in '{' or '=>' (max 3 line scan); fallback to first 2 lines if neither found - Middle ground between 'detail' (full snippet) and 'summary' (no snippet body at all) — useful when agent needs param types and return shape but not implementation ## #134 — SPI default-readiness decision framework - New doc: docs/decisions/2026-05-11-spi-default-readiness.md - Codifies the criteria the --spi flag must meet before becoming default: build-time parity within 1.5x, compatibility checklist across 9 commands, project-shape coverage across 6 shapes, retrieval-quality threshold, diagnostics surface parity - Defines the post-flip fallback path: --no-spi flag remains accessible for 3 releases minimum, with deprecation window pause if SPI-specific regressions surface - This is a DECISION FRAMEWORK, not code. The next code change (the default flip itself) has to meet this checklist. ## Tests - 4 new tests in context-pack-value-per-token-131.test.ts: required candidates must-include, density beats expense, evidence-order default still works, omitted candidates reported correctly - 5 new tests in context-pack-resolution-signature-132.test.ts: brace boundary, arrow boundary, fallback when neither found, null snippet handling, bytes_saved scales with body size - 1833/1833 pass, typecheck + build clean ## Not in this bundle (deferred) - #135 task-conditioned slicing v1 — requires new anchor-detection module, slice walker, task-mode-specific traversal rules. Multi-PR effort; doesn't fit a single-shot bundle alongside #131/#132.
📝 WalkthroughWalkthroughAdds a 'value-per-token' selection strategy and placement refactor to context-pack compilation, introduces a 'signature' resolution mode with extraction helpers and updated resolution_map typing, and adds a SPI default-readiness decision document with graduation criteria and a fallback timeline. ChangesValue-Per-Token Selection Strategy
Signature Resolution Mode
SPI Default-Readiness Decision Document
Estimated code review effort🎯 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 docstrings
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/context-pack.ts (1)
628-653:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack placed candidates directly when computing omitted nodes.
omittedNodesis derived fromlabel:source_file:line_numberafter materialization. That can mark a selected candidate as omitted whenbuild_entry()fills missing source metadata, and it also collapses distinct candidates that share the same triple.pack.expandableshould be based on the actual candidates accepted byplaceCandidate().Proposed fix
const selectedLabelsByEvidence = new Map<ContextPackEvidenceClass, string[]>() const selectedCommunities = new Set<number>() + const placedCandidates = new Set<ContextPackNodeCandidate<TNode>>() let tokenCount = 0 let breakIndex = orderedNodes.length const placeCandidate = (candidate: ContextPackNodeCandidate<TNode>, candidateTokens: number): void => { + placedCandidates.add(candidate) const entry = candidate.build_entry() selectedNodes.push(entry) @@ - const placedLabelKey = (c: ContextPackNodeCandidate<TNode>): string => `${c.label}:${c.source_file}:${c.line_number}` - const placedKeys = new Set(selectedNodes.map((n) => `${n.label}:${n.source_file}:${n.line_number}`)) const omittedNodes = input.selection_strategy === 'value-per-token' - ? orderedNodes.filter((c) => !placedKeys.has(placedLabelKey(c))) + ? orderedNodes.filter((c) => !placedCandidates.has(c)) : orderedNodes.slice(breakIndex)Also applies to: 710-717
🤖 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/runtime/context-pack.ts` around lines 628 - 653, The code currently computes omittedNodes from materialized entry keys (label:source_file:line_number) which can misclassify or collapse distinct candidates after build_entry(); to fix, record placed candidates directly inside placeCandidate() (e.g., add candidate or a stable unique id to a new Set like placedCandidates when calling placeCandidate), use that placedCandidates Set when computing omittedNodes and pack.expandable, and update the same logic used around lines 710-717 to consult placedCandidates instead of derived entry triples so expandable reflects the actual candidates accepted by placeCandidate().
🤖 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 `@src/runtime/context-pack-resolution.ts`:
- Around line 24-25: The resolution_map currently maps the 'signature'
ContextPackResolution to 'summary', causing applyContextPackResolution and
downstream diagnostics to treat signature output as summary; update the
resolution_map to include 'signature' as its own entry (e.g., map 'signature' ->
'signature') and adjust any switch/conditional logic in
applyContextPackResolution, ContextPackResolution type usages, and the other
spots referencing resolution_map so nodes are recorded as 'signature' when
resolution === 'signature' instead of 'summary' (check the blocks around
resolution_map, applyContextPackResolution, and the adjacent mapping logic and
mirror the change wherever resolution values 59-61 and 76-90 are handled).
In `@src/runtime/context-pack.ts`:
- Around line 674-680: The loop over requiredCandidates incorrectly uses break
when a required candidate would overflow the budget (checking
candidate.estimate_tokens(), tokenCount, input.task_contract.budget, and
selectedNodes), which stops processing later required candidates; change the
control flow so that when candidateTokens would exceed the budget and
selectedNodes.length > 0 you continue (skip/drop this overflowing candidate)
instead of break, leaving the rest of the loop to evaluate subsequent required
candidates and still call placeCandidate for those that fit.
- Around line 681-694: The value-per-token score is using idx from
optionalCandidates instead of the node's position in orderedNodes, compressing
ranks if required nodes preceded optionals; update the mapping that builds
valueCandidates so score uses the node's index within orderedNodes (e.g., find
the orderedIndex via orderedNodes.findIndex(...) using the same identity/key
used elsewhere) and set score: 1 / (orderedIndex + 1) (fall back to a large
index if not found). Keep other fields (id, payload, token_cost) unchanged and
then call selectByValuePerToken as before.
---
Outside diff comments:
In `@src/runtime/context-pack.ts`:
- Around line 628-653: The code currently computes omittedNodes from
materialized entry keys (label:source_file:line_number) which can misclassify or
collapse distinct candidates after build_entry(); to fix, record placed
candidates directly inside placeCandidate() (e.g., add candidate or a stable
unique id to a new Set like placedCandidates when calling placeCandidate), use
that placedCandidates Set when computing omittedNodes and pack.expandable, and
update the same logic used around lines 710-717 to consult placedCandidates
instead of derived entry triples so expandable reflects the actual candidates
accepted by placeCandidate().
🪄 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: 14020cac-27d5-4924-adf0-c28c01a4f336
📒 Files selected for processing (5)
docs/decisions/2026-05-11-spi-default-readiness.mdsrc/runtime/context-pack-resolution.tssrc/runtime/context-pack.tstests/unit/context-pack-resolution-signature-132.test.tstests/unit/context-pack-value-per-token-131.test.ts
…uired continue + global-rank score + signature label Four valid findings from CodeRabbit on PR #138: 1) MAJOR — omittedNodes was derived from materialized label:source_file:line_number triples, which can mis-attribute when build_entry() fills missing source metadata or two distinct candidates share a triple. Fixed by tracking candidate object identity directly via a placedCandidates Set populated inside placeCandidate(). 2) requiredCandidates loop used 'break' on overflow — one oversize required candidate killed subsequent smaller required ones. Changed to 'continue': skip the overflowing one, try the next. Required candidates each have an independent must-include semantic, so one's failure shouldn't cascade. 3) value-per-token score used the optionalCandidates-local idx, not the orderedNodes-global index. A leading run of required candidates would compress the optional rank space and inflate every optional's score. Now uses a Map<candidate, orderedIndex> for the correct global rank. 4) signature resolution mis-labeled itself as 'summary' in resolution_map. Extended the type to include 'signature' explicitly. Downstream diagnostics can now differentiate 'has signature info' from 'has no body at all'. Updated stdio/tools.ts applyResolutionToNodes return type to match. 1833/1833 still pass, typecheck + build clean.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/stdio/tools.ts (1)
906-910:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
resolution: "signature"is currently unreachable incontext_pack.Lines 907-910 only admit
'summary' | 'mixed', so'signature'falls back to'detail'and never reachesapplyContextPackResolution.Proposed fix
- const resolution: ContextPackResolution = - resolutionParam === 'summary' || resolutionParam === 'mixed' + const resolution: ContextPackResolution = + resolutionParam === 'summary' || resolutionParam === 'mixed' || resolutionParam === 'signature' ? resolutionParam : 'detail'🤖 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/runtime/stdio/tools.ts` around lines 906 - 910, The resolution variable currently only accepts 'summary' or 'mixed', causing 'signature' to fall back to 'detail' and never reach applyContextPackResolution; update the validation logic where resolutionParam is read (helpers.stringParam and the resolution assignment) to treat 'signature' as a valid ContextPackResolution (e.g., include resolutionParam === 'signature' in the conditional or otherwise allow any ContextPackResolution value), so resolution can be 'summary' | 'mixed' | 'signature' and will be passed through to applyContextPackResolution unchanged.
🧹 Nitpick comments (1)
src/runtime/stdio/tools.ts (1)
916-916: ⚡ Quick winAvoid re-declaring the
resolution_mapunion locally.Line 916 duplicates the resolution union type; prefer reusing the exported
ApplyResolutionResulttyping to prevent future drift between modules.Refactor sketch
-import { applyContextPackResolution, type ContextPackResolution } from '../context-pack-resolution.js' +import { + applyContextPackResolution, + type ContextPackResolution, + type ApplyResolutionResult, +} from '../context-pack-resolution.js' ... - resolution_map: Array<{ node_id: string | undefined; resolution: 'detail' | 'summary' | 'signature' }> + resolution_map: ApplyResolutionResult<ContextPackNode>['resolution_map']🤖 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/runtime/stdio/tools.ts` at line 916, Replace the local re-declaration of the resolution union on the resolution_map property with the exported ApplyResolutionResult type: import ApplyResolutionResult and use it (e.g., resolution_map: ApplyResolutionResult[] or the appropriate ApplyResolutionResult property) instead of redeclaring 'resolution: "detail" | "summary" | "signature"'; update the resolution_map declaration in the same location to reference ApplyResolutionResult and add the necessary import for ApplyResolutionResult at the top of the module.
🤖 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.
Outside diff comments:
In `@src/runtime/stdio/tools.ts`:
- Around line 906-910: The resolution variable currently only accepts 'summary'
or 'mixed', causing 'signature' to fall back to 'detail' and never reach
applyContextPackResolution; update the validation logic where resolutionParam is
read (helpers.stringParam and the resolution assignment) to treat 'signature' as
a valid ContextPackResolution (e.g., include resolutionParam === 'signature' in
the conditional or otherwise allow any ContextPackResolution value), so
resolution can be 'summary' | 'mixed' | 'signature' and will be passed through
to applyContextPackResolution unchanged.
---
Nitpick comments:
In `@src/runtime/stdio/tools.ts`:
- Line 916: Replace the local re-declaration of the resolution union on the
resolution_map property with the exported ApplyResolutionResult type: import
ApplyResolutionResult and use it (e.g., resolution_map: ApplyResolutionResult[]
or the appropriate ApplyResolutionResult property) instead of redeclaring
'resolution: "detail" | "summary" | "signature"'; update the resolution_map
declaration in the same location to reference ApplyResolutionResult and add the
necessary import for ApplyResolutionResult at the top of the module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e5737c4-048d-41d7-89ec-7f28a3ca3117
📒 Files selected for processing (3)
src/runtime/context-pack-resolution.tssrc/runtime/context-pack.tssrc/runtime/stdio/tools.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/runtime/context-pack.ts
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.
Closes #131
Closes #132
Closes #134
Summary
Bundles three v0.20-context-compiler items. #135 (task-conditioned slicing v1) intentionally deferred — it's a multi-PR substrate, not a single-shot slice.
What's in the bundle
#131 — value-per-token selection_strategy on compileContextPack
selection_strategy: 'evidence-order' | 'value-per-token'(default = evidence-order, unchanged)value-per-token:selectByValuePerTokenwith residual budgetorderedNodesas rank proxy (1/(idx+1))#132 — 'signature' resolution level
ContextPackResolutionfromdetail | summary | mixedto also acceptsignature{or=>(max 3 line scan); fallback to first 2 linesdetail(full snippet) andsummary(no body) — useful when the agent needs param types and return shape but not implementation#134 — SPI default-readiness decision framework
docs/decisions/2026-05-11-spi-default-readiness.md--spimust achieve before becoming default:--no-spiflag remains accessible for 3 releases minimumNot in this bundle (intentionally)
#135 — task-conditioned slicing v1. Requires new anchor-detection module, slice walker, task-mode-specific traversal rules. Multi-PR effort; doesn't fit a single-shot bundle alongside #131/#132. Worth its own slice train.
Test plan
npm run typecheckcleannpm run buildcleannpm run test:run— 1833/1833 pass (113 files, +9 new tests)Summary by CodeRabbit
New Features
Tests
Documentation
Style/Types