feat: ship workflow hardening#25
Merged
Merged
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR introduces snippet coverage metric to evaluation, adds four new MCP developer tools (relevant_files, feature_map, risk_map, implementation_checklist), changes MCP payload semantics from opt-in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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. Review rate limit: 0/1 reviews remaining, refill in 33 minutes and 51 seconds.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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/retrieve.ts (1)
990-1001:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve framework metadata in the no-
node_idfallback.
compactRetrieveResultForStdioacceptsRetrieveResult, andRetrieveMatchedNode.node_idis optional. When a caller passes valid nodes without IDs, this fallback rebuild dropsframeworkandframework_roleeven though both fields are still present oncompactResult.matched_nodes, so the helper can silently degrade the payload shape.Suggested fix
return { label: node.label, source_file: node.source_file, line_number: node.line_number, + framework: node.framework, + framework_role: node.framework_role, framework_boost: 0, file_type: node.file_type ?? compactResult.shared_file_type ?? '', snippet: node.snippet, match_score: node.match_score, relevance_band: node.relevance_band,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/retrieve.ts` around lines 990 - 1001, The fallback branch in compactRetrieveResultForStdio (handling RetrieveResult where RetrieveMatchedNode.node_id is missing) currently rebuilds nodes but omits framework and framework_role, losing metadata that exists on compactResult.matched_nodes; update the object construction (the returned literal in compactRetrieveResultForStdio) to preserve node.framework and node.framework_role when present (e.g., spread or conditional properties similar to node.node_kind) so the payload shape matches the original matched_nodes entries.
🧹 Nitpick comments (4)
tests/unit/stdio-server.test.ts (1)
603-638: ⚡ Quick winAdd one regression check for the deprecated
compact: falsealias.This test now covers the new
verbose: truepath, but it no longer exercises the transition alias that the handler still supports. A singlecompact: falseassertion here would lock the backward-compat contract before the next cleanup pass.Suggested addition
+ const retrieveLegacyVerbose = await Promise.resolve(handleStdioRequest(graphPath, { + id: 21, + method: 'tools/call', + params: { + name: 'retrieve', + arguments: { + question: 'which react router route renders dashboard page', + budget: 5000, + file_type: 'code', + compact: false, + }, + }, + })) + const retrieveDefaultPayload = JSON.parse((retrieveDefault?.result as { content: Array<{ text: string }> }).content[0]!.text) const retrieveVerbosePayload = JSON.parse((retrieveVerbose?.result as { content: Array<{ text: string }> }).content[0]!.text) + const retrieveLegacyVerbosePayload = JSON.parse((retrieveLegacyVerbose?.result as { content: Array<{ text: string }> }).content[0]!.text) const impactDefaultPayload = JSON.parse((impactDefault?.result as { content: Array<{ text: string }> }).content[0]!.text) const impactVerbosePayload = JSON.parse((impactVerbose?.result as { content: Array<{ text: string }> }).content[0]!.text) + expect(retrieveLegacyVerbosePayload).toEqual(retrieveVerbosePayload)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/stdio-server.test.ts` around lines 603 - 638, Add a regression call that exercises the deprecated alias by invoking handleStdioRequest with compact: false (instead of verbose: true) so the handler still honors the old option; specifically, add a Promise.resolve(handleStdioRequest(...)) entry next to the existing retrieveVerbose/impactVerbose blocks using the same method name and arguments but replacing verbose: true with compact: false (use a new unique id, e.g., id: 5) and assert that its result matches the corresponding verbose:true behavior; target the same function names in the test (handleStdioRequest, params.name 'retrieve' or 'impact') so the backward-compat contract is locked in.src/runtime/feature-map.ts (1)
55-61: 💤 Low valueDuplicate helper function across files.
pushUniqueis identically implemented in bothfeature-map.tsandrelevant-files.ts. Consider extracting to a shared utility module to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/feature-map.ts` around lines 55 - 61, The helper function pushUnique is duplicated; extract it into a single shared utility (e.g., export pushUnique from a new utility module) and update both locations to import and use that shared implementation (replace the local pushUnique in feature-map.ts and the copy in relevant-files.ts with an import), then remove the duplicate definitions so only the exported function remains; reference pushUnique to locate and replace the duplicates and ensure the new module exports the same signature (values: string[], seen: Set<string>, value: string): void.src/runtime/risk-map.ts (1)
75-83: ⚡ Quick winRedundant
retrieveContextcall.
featureMapon line 77 internally callsretrieveContext, and then lines 78-83 call it again with the same parameters. Consider either:
- Having
featureMapreturn theRetrieveResultit computed, or- Calling
retrieveContextonce and passing the result to a lower-level feature map builderThis doubles the retrieval work for every
riskMapcall.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/risk-map.ts` around lines 75 - 83, The riskMap function is calling retrieveContext twice (once indirectly via featureMap and again directly), doubling retrieval work; to fix, choose one approach: either update featureMap to also return the RetrieveResult it computes (e.g., change featureMap to return { feature, retrieveResult } or similar) and use that inside riskMap, or instead call retrieveContext once in riskMap and pass the resulting RetrieveResult into a modified featureMap that accepts it as an argument; update references to featureMap and retrieveContext accordingly (functions: riskMap, featureMap, retrieveContext) so retrieval happens only once.src/runtime/implementation-checklist.ts (1)
71-73: 🏗️ Heavy liftCascading redundant retrieval calls.
implementationChecklistcalls bothfeatureMapandriskMap, butriskMapinternally callsfeatureMapandretrieveContextagain. This creates up to 5 redundantretrieveContextcalls per checklist request.Consider restructuring to:
- Call
retrieveContextonce at the top level- Pass the result to specialized builders for feature map and risk analysis
This is a higher-effort change but would significantly improve performance for this tool.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/implementation-checklist.ts` around lines 71 - 73, implementationChecklist currently calls featureMap and riskMap which cause redundant retrieveContext calls (riskMap itself calls featureMap and retrieveContext), so refactor to call retrieveContext once in implementationChecklist and pass the retrieved context into the specialized builders; update function signatures for featureMap and riskMap (or create new internal helpers like buildFeatureMapFromContext and buildRiskMapFromContext) to accept the pre-fetched context and the existing KnowledgeGraph/ImplementationChecklistOptions, remove internal retrieveContext calls inside riskMap/featureMap, and ensure implementationChecklist constructs the context once and supplies it to featureMap/riskMap to eliminate duplicate retrievals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pipeline/build.ts`:
- Around line 94-96: The code stores extraction.root_path without normalizing
it, so leading/trailing whitespace can still be persisted; update the assignment
in the block guarding extraction.root_path to store a trimmed and normalized
value (e.g., const normalized = path.normalize(extraction.root_path.trim())) and
assign graph.graph.root_path = normalized (use Node's path.normalize or
equivalent to remove redundant separators/trailing slashes) so downstream
relativization works reliably; reference the extraction.root_path guard and the
graph.graph.root_path assignment when making the change.
---
Outside diff comments:
In `@src/runtime/retrieve.ts`:
- Around line 990-1001: The fallback branch in compactRetrieveResultForStdio
(handling RetrieveResult where RetrieveMatchedNode.node_id is missing) currently
rebuilds nodes but omits framework and framework_role, losing metadata that
exists on compactResult.matched_nodes; update the object construction (the
returned literal in compactRetrieveResultForStdio) to preserve node.framework
and node.framework_role when present (e.g., spread or conditional properties
similar to node.node_kind) so the payload shape matches the original
matched_nodes entries.
---
Nitpick comments:
In `@src/runtime/feature-map.ts`:
- Around line 55-61: The helper function pushUnique is duplicated; extract it
into a single shared utility (e.g., export pushUnique from a new utility module)
and update both locations to import and use that shared implementation (replace
the local pushUnique in feature-map.ts and the copy in relevant-files.ts with an
import), then remove the duplicate definitions so only the exported function
remains; reference pushUnique to locate and replace the duplicates and ensure
the new module exports the same signature (values: string[], seen: Set<string>,
value: string): void.
In `@src/runtime/implementation-checklist.ts`:
- Around line 71-73: implementationChecklist currently calls featureMap and
riskMap which cause redundant retrieveContext calls (riskMap itself calls
featureMap and retrieveContext), so refactor to call retrieveContext once in
implementationChecklist and pass the retrieved context into the specialized
builders; update function signatures for featureMap and riskMap (or create new
internal helpers like buildFeatureMapFromContext and buildRiskMapFromContext) to
accept the pre-fetched context and the existing
KnowledgeGraph/ImplementationChecklistOptions, remove internal retrieveContext
calls inside riskMap/featureMap, and ensure implementationChecklist constructs
the context once and supplies it to featureMap/riskMap to eliminate duplicate
retrievals.
In `@src/runtime/risk-map.ts`:
- Around line 75-83: The riskMap function is calling retrieveContext twice (once
indirectly via featureMap and again directly), doubling retrieval work; to fix,
choose one approach: either update featureMap to also return the RetrieveResult
it computes (e.g., change featureMap to return { feature, retrieveResult } or
similar) and use that inside riskMap, or instead call retrieveContext once in
riskMap and pass the resulting RetrieveResult into a modified featureMap that
accepts it as an argument; update references to featureMap and retrieveContext
accordingly (functions: riskMap, featureMap, retrieveContext) so retrieval
happens only once.
In `@tests/unit/stdio-server.test.ts`:
- Around line 603-638: Add a regression call that exercises the deprecated alias
by invoking handleStdioRequest with compact: false (instead of verbose: true) so
the handler still honors the old option; specifically, add a
Promise.resolve(handleStdioRequest(...)) entry next to the existing
retrieveVerbose/impactVerbose blocks using the same method name and arguments
but replacing verbose: true with compact: false (use a new unique id, e.g., id:
5) and assert that its result matches the corresponding verbose:true behavior;
target the same function names in the test (handleStdioRequest, params.name
'retrieve' or 'impact') so the backward-compat contract is locked in.
🪄 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: 26987a47-d000-4a11-b15f-013dfc8aa0c6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
.github/workflows/ci.ymlCHANGELOG.mdREADME.mddocs/language-capability-matrix.mddocs/proof-workflows.mdexamples/why-graphify.mdpackage.jsonsrc/cli/main.tssrc/infrastructure/benchmark/quality.tssrc/pipeline/build.tssrc/runtime/feature-map.tssrc/runtime/impact.tssrc/runtime/implementation-checklist.tssrc/runtime/relevant-files.tssrc/runtime/retrieve.tssrc/runtime/risk-map.tssrc/runtime/serve.tssrc/runtime/stdio/definitions.tssrc/runtime/stdio/tools.tssrc/shared/source-path.tstests/unit/benchmark-quality.test.tstests/unit/feature-map.test.tstests/unit/impact.test.tstests/unit/implementation-checklist.test.tstests/unit/package-metadata.test.tstests/unit/relevant-files.test.tstests/unit/retrieve.test.tstests/unit/risk-map.test.tstests/unit/stdio-server.test.ts
Comment on lines
+94
to
+96
| if (typeof extraction.root_path === 'string' && extraction.root_path.trim().length > 0) { | ||
| graph.graph.root_path = extraction.root_path | ||
| } |
There was a problem hiding this comment.
Persist a normalized root_path value.
At Line 95, the value stored can still include leading/trailing whitespace even though the guard trims for emptiness. That can break downstream path relativization in edge cases.
Suggested fix
- if (typeof extraction.root_path === 'string' && extraction.root_path.trim().length > 0) {
- graph.graph.root_path = extraction.root_path
- }
+ if (typeof extraction.root_path === 'string') {
+ const rootPath = extraction.root_path.trim()
+ if (rootPath.length > 0) {
+ graph.graph.root_path = rootPath
+ }
+ }📝 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
| if (typeof extraction.root_path === 'string' && extraction.root_path.trim().length > 0) { | |
| graph.graph.root_path = extraction.root_path | |
| } | |
| if (typeof extraction.root_path === 'string') { | |
| const rootPath = extraction.root_path.trim() | |
| if (rootPath.length > 0) { | |
| graph.graph.root_path = rootPath | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pipeline/build.ts` around lines 94 - 96, The code stores
extraction.root_path without normalizing it, so leading/trailing whitespace can
still be persisted; update the assignment in the block guarding
extraction.root_path to store a trimmed and normalized value (e.g., const
normalized = path.normalize(extraction.root_path.trim())) and assign
graph.graph.root_path = normalized (use Node's path.normalize or equivalent to
remove redundant separators/trailing slashes) so downstream relativization works
reliably; reference the extraction.root_path guard and the graph.graph.root_path
assignment when making the change.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Apr 30, 2026
Merged
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
Testing
npm run test:runnpm run typechecknpm run buildnpm pack --dry-run(if packaging or install behavior changed)Checklist
Related issues
Summary by CodeRabbit
Release Notes v0.9.2
New Features
Improvements
verbose: truefor legacy full payloads.