Feature/task context planner#60
Merged
Merged
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fallback review plans to primary evidence when only focus paths are available, so review seeds no longer request change evidence without changed files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
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:
📝 WalkthroughWalkthroughThis PR adds a task-intent taxonomy and runtime, context-inventory contracts and normalizers, task-evidence recipes and a deterministic task-context planner, semantic coverage and stable expandable references in context packs with focused expansion tooling, provider/runtime token-source proof tracking across benchmark/compare/review-compare, and extensive test/docs updates. ChangesTask Intent Classification System
Context Inventory System
Task Evidence Recipes and Planner
Context Pack Semantic Coverage & Expandables
Provider / Runtime Proof Tracking
Stdio Tools & Context Expansion
CLI Command & Metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/runtime/context-pack.ts (1)
584-593: 💤 Low valueMinor: Inconsistent indentation in function call.
The indentation within the
coverageEntriesForCandidatescall is inconsistent - some arguments are indented more than others.🔧 Suggested formatting fix
coverage: coverageEntriesForCandidates( - input.task_contract, - materializedNodes, - selectedMaterialized, - selectedCounts, - { + input.task_contract, + materializedNodes, + selectedMaterialized, + selectedCounts, + { available: input.relationships?.length ?? 0, selected: relationships.length, }, ),🤖 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 584 - 593, The call to coverageEntriesForCandidates has inconsistent indentation for its arguments; reformat the invocation so all top-level arguments (input.task_contract, materializedNodes, selectedMaterialized, selectedCounts, and the object literal) are aligned consistently and the object literal keys (available, selected) are indented one level inside the object; adjust the closing parentheses and comma so they line up with the start of the function call. Ensure you modify the coverageEntriesForCandidates(...) invocation and the object literal containing available and selected to use consistent indentation throughout.
🤖 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/retrieve.ts`:
- Around line 454-458: The current fallback uses node.lineNumber > 0 which will
accept Infinity and fractional values; change the guard to ensure a finite
positive integer (e.g., Number.isFinite(node.lineNumber) &&
Number.isInteger(node.lineNumber) && node.lineNumber > 0) before returning
start_line/end_line so expandable_ref.line_range is only emitted for valid
integer line numbers.
---
Nitpick comments:
In `@src/runtime/context-pack.ts`:
- Around line 584-593: The call to coverageEntriesForCandidates has inconsistent
indentation for its arguments; reformat the invocation so all top-level
arguments (input.task_contract, materializedNodes, selectedMaterialized,
selectedCounts, and the object literal) are aligned consistently and the object
literal keys (available, selected) are indented one level inside the object;
adjust the closing parentheses and comma so they line up with the start of the
function call. Ensure you modify the coverageEntriesForCandidates(...)
invocation and the object literal containing available and selected to use
consistent indentation throughout.
🪄 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: 9b450c0f-0e48-4bd7-a07d-701738a61a97
📒 Files selected for processing (23)
src/contracts/context-inventory.tssrc/contracts/context-pack.tssrc/contracts/task-context-plan.tssrc/contracts/task-intent.tssrc/infrastructure/benchmark.tssrc/infrastructure/compare.tssrc/infrastructure/review-compare.tssrc/runtime/context-inventory.tssrc/runtime/context-pack.tssrc/runtime/pr-impact.tssrc/runtime/retrieve.tssrc/runtime/stdio/tools.tssrc/runtime/task-context-planner.tssrc/runtime/task-evidence-recipes.tssrc/runtime/task-intent.tstests/unit/benchmark.test.tstests/unit/compare.test.tstests/unit/context-inventory.test.tstests/unit/context-pack.test.tstests/unit/retrieve.test.tstests/unit/review-compare.test.tstests/unit/task-context-planner.test.tstests/unit/task-intent.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
examples/mcp-tool-examples.md (1)
147-147: ⚡ Quick winConsider clarifying the agent workflow description.
The updated explanation accurately describes the new fields but packs multiple concepts into one long sentence. Consider splitting it for clarity:
-**What the agent does with this:** Uses the compact pack as a coverage contract: answer with the selected evidence now, inspect `semantic_entries` to see whether implementation/structure/tests are sufficiently covered, and use the stable `handle_id` + `follow_up` metadata only when omitted context is still needed. +**What the agent does with this:** Uses the compact pack as a coverage contract: answer with the selected evidence now and inspect `semantic_entries` to see whether implementation/structure/tests are sufficiently covered. When omitted context is still needed (e.g., `status: "missing"` in semantic_entries), use the stable `handle_id` + `follow_up` metadata to expand those specific areas.This makes the conditional logic clearer—agents should expand when they see missing/incomplete 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 `@examples/mcp-tool-examples.md` at line 147, The sentence describing the agent workflow is too long and combines several behaviors; rewrite it into 2–3 clear sentences that map to the symbols in the diff: state that the agent uses the compact pack as a coverage contract and should answer now using the selected evidence, then say the agent must inspect semantic_entries to determine whether implementation/structure/tests are sufficiently covered (and only expand when coverage is missing/incomplete), and finally specify that the stable handle_id and follow_up metadata should be used only when omitted context still needs to be requested.
🤖 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 `@docs/proof-workflows.md`:
- Line 55: The document uses two inconsistent phrases for the same feature:
"provider/runtime proof block" and "provider/runtime proof note"; standardize to
a single term (pick either "provider/runtime proof block" or "provider/runtime
proof note") and replace all occurrences so the language is consistent (search
for the exact phrases "provider/runtime proof block" and "provider/runtime proof
note" and update them), including the mention alongside `compare` and any other
references that describe the provider/runtime proof output. Ensure the chosen
term is used everywhere in the file.
- Line 55: Split the long sentence about compare's behavior into 3–4 concise
sentences: first state that when Gemini emits structured JSON with
usageMetadata, compare records reported input and total tokens in report.json
and the terminal summary; second explain that if the runner returns only answer
text or malformed JSON, compare falls back to labeled local cl100k_base prompt
estimates; third describe that recent reports add a "provider/runtime proof"
block showing whether counts are provider-reported or local estimates plus
session-reuse accounting; and finally clarify the use case by replacing
"customer-proof" with "customer-facing proof" (or "proof for stakeholders") and
note that compare may consume paid model tokens like benchmark and eval but
saves paired answers while benchmark and eval score labeled sets.
---
Nitpick comments:
In `@examples/mcp-tool-examples.md`:
- Line 147: The sentence describing the agent workflow is too long and combines
several behaviors; rewrite it into 2–3 clear sentences that map to the symbols
in the diff: state that the agent uses the compact pack as a coverage contract
and should answer now using the selected evidence, then say the agent must
inspect semantic_entries to determine whether implementation/structure/tests are
sufficiently covered (and only expand when coverage is missing/incomplete), and
finally specify that the stable handle_id and follow_up metadata should be used
only when omitted context still needs to be requested.
🪄 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: b9c4b77a-6814-4b88-809d-568e6f42888f
📒 Files selected for processing (4)
README.mddocs/proof-workflows.mdexamples/mcp-tool-examples.mdexamples/why-graphify.md
✅ Files skipped from review due to trivial changes (2)
- README.md
- examples/why-graphify.md
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/runtime/stdio/tools.ts (1)
760-783: 💤 Low valueConsider adding shape validation for the stored handle.
The validation at lines 770-772 confirms the stored value is a non-null object, but doesn't verify it has the expected
StoredContextPackHandleshape (prompt, task, task_intent, follow_up). If a malformed object were somehow stored,buildFocusedExpansionPayloadcould fail with unclear errors when accessing properties.Since handles are only stored internally via
storeExpandableHandles, this is low risk, but defensive validation would improve robustness.🤖 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 760 - 783, The stored handle returned by getContextPackHandle needs explicit shape validation before calling buildFocusedExpansionPayload: check that stored is an object and has the required StoredContextPackHandle properties (e.g., prompt, task, task_intent, follow_up) with expected types (strings or arrays as appropriate), and if validation fails return helpers.failure(...) with a clear message; update the case 'context_expand' block (referencing getContextPackHandle, StoredContextPackHandle, buildFocusedExpansionPayload and storeExpandableHandles) to perform this defensive field/type checks and only call buildFocusedExpansionPayload when the shape is valid.
🤖 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/infrastructure/context-pack-command.ts`:
- Around line 173-183: plannerBudget is computed via Math.max(options.budget, 3)
but only used to build the plan; calls like analyzePrImpact and other
retrieve/review/impact calls still pass options.budget causing mismatch. Update
all usages of options.budget in this function (e.g., the call to
dependencies.analyzePrImpact and the top-level budget fields around
buildTaskContextPlan, the review/retrieve/impact calls referenced in the diff
ranges ~173-183, 198-223, 227-235) to use plannerBudget instead so the plan and
subsequent requests use the same normalized budget value.
In `@src/runtime/context-pack-target.ts`:
- Around line 4-13: The helper currently validates node.label with
node.label.trim().length but returns the raw label, causing downstream lookups
to fail on whitespace; update the returns to return trimmed strings: when a
directMatch is found return directMatch.label.trim(), and for bestMatch return
bestMatch.label.trim() (falling back to result.question unchanged), ensuring you
locate the logic around result.matched_nodes, directMatch, bestMatch, and
match_score.
In `@src/runtime/impact.ts`:
- Around line 313-315: compactImpactResult() currently omits the newly emitted
target_file_type field from analyzeImpact(), causing the compacted pack to drop
that metadata; update compactImpactResult() to carry target_file_type through
the compaction step by copying impact.target_file_type (when present/non-empty)
into the returned compact object's corresponding property (the pack or compact
payload builder) so the compact output stays in sync with analyzeImpact().
---
Nitpick comments:
In `@src/runtime/stdio/tools.ts`:
- Around line 760-783: The stored handle returned by getContextPackHandle needs
explicit shape validation before calling buildFocusedExpansionPayload: check
that stored is an object and has the required StoredContextPackHandle properties
(e.g., prompt, task, task_intent, follow_up) with expected types (strings or
arrays as appropriate), and if validation fails return helpers.failure(...) with
a clear message; update the case 'context_expand' block (referencing
getContextPackHandle, StoredContextPackHandle, buildFocusedExpansionPayload and
storeExpandableHandles) to perform this defensive field/type checks and only
call buildFocusedExpansionPayload when the shape is valid.
🪄 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: 0971da01-ea31-4fb8-a71b-9770bacff117
📒 Files selected for processing (16)
src/infrastructure/benchmark.tssrc/infrastructure/compare.tssrc/infrastructure/context-pack-command.tssrc/runtime/context-pack-target.tssrc/runtime/impact.tssrc/runtime/pr-impact.tssrc/runtime/retrieve.tssrc/runtime/stdio-server.tssrc/runtime/stdio/definitions.tssrc/runtime/stdio/tools.tstests/unit/benchmark.test.tstests/unit/compare.test.tstests/unit/context-pack-command.test.tstests/unit/context-pack-target.test.tstests/unit/stdio-server.test.tstests/unit/stdio-tool-profile.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit/benchmark.test.ts
- tests/unit/compare.test.ts
- src/infrastructure/benchmark.ts
- src/infrastructure/compare.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/stdio-server.test.ts (1)
919-929: ⚡ Quick winAssert expandable handle presence before dereferencing it
explainPackPayload.expandable[0].handle_idis used before any shape assertion, so regressions can fail with a genericTypeErrorinstead of a clear expectation failure.Suggested change
const explainPackPayload = JSON.parse((explainPack?.result as { content: Array<{ text: string }> }).content[0]!.text) +expect(explainPackPayload.expandable).toEqual( + expect.arrayContaining([expect.objectContaining({ handle_id: expect.any(String) })]), +) +const explainHandleId = explainPackPayload.expandable[0]!.handle_id const expandedPack = await Promise.resolve(handleStdioRequest(graphPath, { id: 12, method: 'tools/call', params: { name: 'context_expand', arguments: { - handle_id: explainPackPayload.expandable[0].handle_id, + handle_id: explainHandleId, }, }, }, sessionState))🤖 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/unit/stdio-server.test.ts` around lines 919 - 929, The test dereferences explainPackPayload.expandable[0].handle_id without asserting the shape; update the test to first assert that explainPackPayload.expandable is an array with length > 0 and that explainPackPayload.expandable[0] has a defined handle_id before calling handleStdioRequest; locate the variables explainPackPayload and the subsequent call to handleStdioRequest(graphPath, { id: 12, method: 'tools/call', params: { name: 'context_expand', arguments: { handle_id: ... } } }, sessionState) and add an explicit expectation (e.g., expect(Array.isArray(...)) and expect(...[0].handle_id).toBeDefined()) so failures produce clear assertion messages instead of a TypeError.src/runtime/retrieve.ts (1)
1096-1104: 💤 Low valueConsider simplifying the IIFE pattern.
The inline IIFE for conditionally spreading
line_rangeworks but is slightly harder to read. A local variable would be cleaner.♻️ Optional simplification
expandable_ref: { node_id: node.id, label: node.label, source_file: relativizeSourceFile(node.sourceFile, rootPath), - ...(() => { - const lineRange = expandableLineRange(node) - return lineRange ? { line_range: lineRange } : {} - })(), + ...(expandableLineRange(node) ? { line_range: expandableLineRange(node) } : {}), },Or compute once:
const lineRange = expandableLineRange(node) // then in expandable_ref: ...(lineRange ? { line_range: lineRange } : {}),🤖 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/retrieve.ts` around lines 1096 - 1104, The inline IIFE inside the expandable_ref object is unnecessary and reduces readability; compute const lineRange = expandableLineRange(node) before constructing the object (near where expandable_ref is built) and then conditionally spread ...(lineRange ? { line_range: lineRange } : {}) into the expandable_ref instead of using the immediate function; keep other fields (node_id, label, source_file using relativizeSourceFile(node.sourceFile, rootPath)) unchanged.
🤖 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.
Nitpick comments:
In `@src/runtime/retrieve.ts`:
- Around line 1096-1104: The inline IIFE inside the expandable_ref object is
unnecessary and reduces readability; compute const lineRange =
expandableLineRange(node) before constructing the object (near where
expandable_ref is built) and then conditionally spread ...(lineRange ? {
line_range: lineRange } : {}) into the expandable_ref instead of using the
immediate function; keep other fields (node_id, label, source_file using
relativizeSourceFile(node.sourceFile, rootPath)) unchanged.
In `@tests/unit/stdio-server.test.ts`:
- Around line 919-929: The test dereferences
explainPackPayload.expandable[0].handle_id without asserting the shape; update
the test to first assert that explainPackPayload.expandable is an array with
length > 0 and that explainPackPayload.expandable[0] has a defined handle_id
before calling handleStdioRequest; locate the variables explainPackPayload and
the subsequent call to handleStdioRequest(graphPath, { id: 12, method:
'tools/call', params: { name: 'context_expand', arguments: { handle_id: ... } }
}, sessionState) and add an explicit expectation (e.g.,
expect(Array.isArray(...)) and expect(...[0].handle_id).toBeDefined()) so
failures produce clear assertion messages instead of a TypeError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d2be88c3-5fe0-42d5-b847-07d327524cfd
📒 Files selected for processing (14)
docs/proof-workflows.mdexamples/mcp-tool-examples.mdsrc/infrastructure/context-pack-command.tssrc/runtime/context-pack-target.tssrc/runtime/context-pack.tssrc/runtime/impact.tssrc/runtime/retrieve.tssrc/runtime/stdio/tools.tstests/unit/context-pack-command.test.tstests/unit/context-pack-target.test.tstests/unit/impact.test.tstests/unit/retrieve.test.tstests/unit/stdio-server.test.tstests/unit/why-graphify-doc.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/unit/impact.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/unit/context-pack-target.test.ts
- tests/unit/retrieve.test.ts
- src/runtime/context-pack-target.ts
- examples/mcp-tool-examples.md
- docs/proof-workflows.md
- src/runtime/stdio/tools.ts
- 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.
Summary
packand MCPcontext_packcontext_expandso expandable handles can be executed within the active sessionfile_typeso semantic coverage does not falsely miss implementationpackoutput with MCP metadata and add a sharedpickImpactTarget()helper so CLI/MCP impact selection stays consistentTesting
npm run test:runnpm run typechecknpm run buildnpm pack --dry-runChecklist
Related issues
Addresses the PR #60 review blockers around planner wiring, executable expandable refs, honest provider proof reporting, impact semantic coverage, and CLI/MCP output alignment.
Summary by CodeRabbit
New Features
Documentation
Tests