Feature/profile messaging#64
Merged
Merged
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR adds selectable MCP tool profiles (core vs full) via CLI flags and environment configuration, extends candidate metadata throughout the context-pack pipeline with source location and snippet information, and implements focused snippet retrieval during context expansion. ChangesMCP Tool Profiles & CLI System
Candidate Metadata & Coverage Refactoring
Documentation & Tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 Generate unit tests (beta)
Comment |
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/main.ts (1)
853-863:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCopilot uninstall leaves MCP server config behind
copilot installnow registers MCP config, butcopilot uninstallonly removes the skill. Users end up with a partial uninstall and stale.vscode/mcp.jsonGraphify entry.Suggested fix direction
if (command === 'copilot') { const options = parsePlatformActionArgs(command, args) if (options.action === 'install') { io.log(dependencies.installSkill('copilot')) io.log(dependencies.installCopilotMcp('.', options.profile ? { profile: options.profile } : {})) } else { io.log(dependencies.uninstallSkill('copilot')) + io.log(dependencies.uninstallCopilotMcp('.')) } return 0 }Also add
uninstallCopilotMcptosrc/infrastructure/install.tsandCliDependencieswith tests.🤖 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/cli/main.ts` around lines 853 - 863, The uninstall path for the 'copilot' command only removes the skill but leaves MCP server config (e.g. .vscode/mcp.json / Graphify entry) behind; add a new uninstallCopilotMcp method to the install module and to the CliDependencies interface, implement it in src/infrastructure/install.ts to remove MCP config/Graphify entries (mirror installCopilotMcp behavior in reverse), call dependencies.uninstallCopilotMcp('.') inside the else branch in src/cli/main.ts when action !== 'install', and add unit tests that verify uninstallCopilotMcp is invoked and that .vscode/mcp.json and Graphify entries are removed.
🧹 Nitpick comments (1)
tests/unit/cli.test.ts (1)
1532-1546: ⚡ Quick winAdd Copilot profile pass-through coverage in this test
This test locks Claude/Cursor profile forwarding but not Copilot, even though Copilot is now profile-aware too. Please assert
installCopilotMcp('.', { profile: 'full' })forcopilot install --profile full.🤖 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/cli.test.ts` around lines 1532 - 1546, The test currently verifies profile forwarding for Claude and Cursor but misses Copilot; add a call to executeCli(['copilot', 'install', '--profile', 'full'], io, dependencies) and assert the mock installCopilotMcp was called with the same args (installCopilotMcp('.', { profile: 'full' })). Create a mock for installCopilotMcp on the dependencies object (similar to claudeInstall/cursorInstall), hook it into dependencies before invoking executeCli, and add an expectation using toHaveBeenCalledWith to verify profile pass-through for Copilot; reference executeCli, createDependencies/createIo, and installCopilotMcp to locate the changes.
🤖 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.ts`:
- Around line 529-554: coverageEntryForCandidate currently returns early when
candidate.source_file exists, skipping candidate.build_entry() and leaving
entry.snippet null; that causes isConfigurationEntry (which checks entry.snippet
for inline env detection) to misclassify nodes. Fix by not short-circuiting
solely on source_file — call candidate.build_entry() (or at least materialize
snippet) when snippet is missing or when isConfigurationEntry needs the snippet;
i.e., in coverageEntryForCandidate, if candidate.source_file exists but
candidate.snippet is falsy, invoke candidate.build_entry() and use its values
for entry.snippet/file_type/node_kind so isConfigurationEntry sees the real
snippet.
In `@src/runtime/stdio/tools.ts`:
- Around line 425-430: The expanded-node construction in context_expand (in
src/runtime/stdio/tools.ts) currently always uses
lineNumberFromSourceLocation(...) to set line_number and snippet, which
overrides an explicitly provided line_number and yields line_number: 0/snippet:
null for graphs that supply line_number without source_location; change the
logic to prefer the explicit stored.follow_up.line_number when it is present and
non-zero (i.e., use stored.follow_up.line_number as the line_number and use the
corresponding snippet lookup/read routine) and only fall back to
lineNumberFromSourceLocation(...) when the explicit line_number is absent/zero;
apply the same fix to the other similar block around the 449-467 range so both
expansions honor an explicit line_number and produce the correct snippet.
- Around line 245-276: snippetSourcePathCandidates/readFocusedSnippet allow
unsafe paths: change the graphDir detection to a platform-safe check (e.g.
compare path.basename(graphDir) === 'graphify-out' instead of endsWith), remove
or stop returning a raw resolve(sourceFile) that can escape the workspace, and
ensure every candidate is resolved and then gated to stay inside the computed
workspace (projectDir or graphDir) by using path.resolve + path.relative to
reject candidates that traverse outside the workspace; update
snippetSourcePathCandidates to only return unique, workspace-contained absolute
paths and leave readFocusedSnippet to iterate those safe candidates.
---
Outside diff comments:
In `@src/cli/main.ts`:
- Around line 853-863: The uninstall path for the 'copilot' command only removes
the skill but leaves MCP server config (e.g. .vscode/mcp.json / Graphify entry)
behind; add a new uninstallCopilotMcp method to the install module and to the
CliDependencies interface, implement it in src/infrastructure/install.ts to
remove MCP config/Graphify entries (mirror installCopilotMcp behavior in
reverse), call dependencies.uninstallCopilotMcp('.') inside the else branch in
src/cli/main.ts when action !== 'install', and add unit tests that verify
uninstallCopilotMcp is invoked and that .vscode/mcp.json and Graphify entries
are removed.
---
Nitpick comments:
In `@tests/unit/cli.test.ts`:
- Around line 1532-1546: The test currently verifies profile forwarding for
Claude and Cursor but misses Copilot; add a call to executeCli(['copilot',
'install', '--profile', 'full'], io, dependencies) and assert the mock
installCopilotMcp was called with the same args (installCopilotMcp('.', {
profile: 'full' })). Create a mock for installCopilotMcp on the dependencies
object (similar to claudeInstall/cursorInstall), hook it into dependencies
before invoking executeCli, and add an expectation using toHaveBeenCalledWith to
verify profile pass-through for Copilot; reference executeCli,
createDependencies/createIo, and installCopilotMcp to locate the changes.
🪄 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: 7bdf6ca3-4dd3-43a3-9d39-298ca8a6d963
📒 Files selected for processing (14)
README.mdexamples/why-graphify.mdsrc/cli/main.tssrc/cli/parser.tssrc/infrastructure/install.tssrc/runtime/context-pack.tssrc/runtime/pr-impact.tssrc/runtime/retrieve.tssrc/runtime/stdio/tools.tstests/unit/cli.test.tstests/unit/context-pack.test.tstests/unit/install.test.tstests/unit/stdio-server.test.tstests/unit/why-graphify-doc.test.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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/cli/main.ts (1)
858-867:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent partial Copilot state when one of the two mutations fails.
At Line 862–Line 866, install/uninstall executes two side-effecting operations in sequence with no rollback/compensation. If the second step fails, the workspace is left half-configured.
💡 Suggested hardening
if (command === 'copilot') { const options = parsePlatformActionArgs(command, args) if (options.action === 'install') { if (!existsSync('graphify-out/graph.json')) { io.log("Warning: graphify-out/graph.json not found. Run 'graphify-ts generate .' first, then re-run this command.") } io.log(dependencies.installSkill('copilot')) - io.log(dependencies.installCopilotMcp('.', options.profile ? { profile: options.profile } : {})) + try { + io.log(dependencies.installCopilotMcp('.', options.profile ? { profile: options.profile } : {})) + } catch (error) { + io.log('Copilot MCP install failed; rolling back Copilot skill install.') + io.log(dependencies.uninstallSkill('copilot')) + throw error + } } else { - io.log(dependencies.uninstallCopilotMcp('.')) - io.log(dependencies.uninstallSkill('copilot')) + let uninstallMcpError: unknown + try { + io.log(dependencies.uninstallCopilotMcp('.')) + } catch (error) { + uninstallMcpError = error + } + io.log(dependencies.uninstallSkill('copilot')) + if (uninstallMcpError) { + throw uninstallMcpError + } } return 0 }🤖 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/cli/main.ts` around lines 858 - 867, The sequence calling dependencies.installSkill('copilot') and dependencies.installCopilotMcp('.', ...) (and the reverse for uninstall) can leave the workspace half-configured if the second call fails; wrap the pair in a try/catch so the second failure triggers a compensating rollback (call uninstallCopilotMcp/uninstallSkill) for any prior successful step, or if rollback fails surface a clear error; use the existing symbols dependencies.installSkill, dependencies.installCopilotMcp, dependencies.uninstallCopilotMcp, dependencies.uninstallSkill and io.log to perform the actions and log successes/errors, and ensure existsSync('graphify-out/graph.json') check remains before attempting install.
🧹 Nitpick comments (2)
tests/unit/stdio-server.test.ts (2)
1209-1213: ⚡ Quick winUse a unique outside-path filename to avoid cross-test collisions.
Using a fixed
outside-secret.tsunder a shared parent can become flaky under parallel/sharded runs.Proposed stability diff
- const outsidePath = resolve(root, '..', 'outside-secret.ts') + const outsidePath = resolve(root, '..', `outside-secret-${process.pid}-${Date.now()}.ts`)🤖 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 1209 - 1213, The test currently writes to a fixed file via the outsidePath variable (const outsidePath = resolve(root, '..', 'outside-secret.ts') and writeFileSync) which can collide between parallel/sharded runs; change the filename generation to be unique per test (e.g., append a random/unique suffix or use crypto.randomUUID()/Date.now()/tmp utilities) when constructing outsidePath so each test writes to its own distinct file and avoid cross-test collisions while still using the same resolve/join/writeFileSync flow.
1147-1201: ⚡ Quick winStrengthen the
line_numberprecedence test with a conflicting location.This test currently validates snippet extraction with
line_number, but it doesn’t prove precedence because the node has no competingsource_location/range. Add a conflicting location so the test fails if precedence regresses.Proposed test-tightening diff
graph.nodes.push({ id: 'line-node', label: 'LineNode', source_file: sourcePath, + source_location: 'L1-L1', line_number: 2, file_type: 'code', community: 0, })🤖 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 1147 - 1201, The test currently only sets a line_number on the pushed node ('line-node') so it doesn't prove that line_number takes precedence over a source_location; update the test around the graph.nodes.push entry for 'line-node' (the same block used by handleStdioRequest and the context_pack handle 'line-number-handle') to also supply a conflicting source location/range (or add another node with the same label and a different source_location/source_file + range), ensuring that the expected snippet still comes from line_number 2; this will fail if precedence regresses.
🤖 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/cli/main.ts`:
- Around line 858-867: The sequence calling dependencies.installSkill('copilot')
and dependencies.installCopilotMcp('.', ...) (and the reverse for uninstall) can
leave the workspace half-configured if the second call fails; wrap the pair in a
try/catch so the second failure triggers a compensating rollback (call
uninstallCopilotMcp/uninstallSkill) for any prior successful step, or if
rollback fails surface a clear error; use the existing symbols
dependencies.installSkill, dependencies.installCopilotMcp,
dependencies.uninstallCopilotMcp, dependencies.uninstallSkill and io.log to
perform the actions and log successes/errors, and ensure
existsSync('graphify-out/graph.json') check remains before attempting install.
---
Nitpick comments:
In `@tests/unit/stdio-server.test.ts`:
- Around line 1209-1213: The test currently writes to a fixed file via the
outsidePath variable (const outsidePath = resolve(root, '..',
'outside-secret.ts') and writeFileSync) which can collide between
parallel/sharded runs; change the filename generation to be unique per test
(e.g., append a random/unique suffix or use crypto.randomUUID()/Date.now()/tmp
utilities) when constructing outsidePath so each test writes to its own distinct
file and avoid cross-test collisions while still using the same
resolve/join/writeFileSync flow.
- Around line 1147-1201: The test currently only sets a line_number on the
pushed node ('line-node') so it doesn't prove that line_number takes precedence
over a source_location; update the test around the graph.nodes.push entry for
'line-node' (the same block used by handleStdioRequest and the context_pack
handle 'line-number-handle') to also supply a conflicting source location/range
(or add another node with the same label and a different
source_location/source_file + range), ensuring that the expected snippet still
comes from line_number 2; this will fail if precedence regresses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d8afa7cb-c459-4e6d-9268-0044543855df
📒 Files selected for processing (8)
src/cli/main.tssrc/infrastructure/install.tssrc/runtime/context-pack.tssrc/runtime/stdio/tools.tstests/unit/cli.test.tstests/unit/context-pack.test.tstests/unit/install.test.tstests/unit/stdio-server.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/infrastructure/install.ts
- tests/unit/cli.test.ts
- src/runtime/context-pack.ts
- tests/unit/install.test.ts
- src/runtime/stdio/tools.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
Testing
npm run test:runnpm run typechecknpm run buildnpm pack --dry-run(if packaging or install behavior changed)Checklist
Related issues
Summary by CodeRabbit
New Features
Documentation