fix(vscode): harden Copilot agent install#738
Open
Snakeblack wants to merge 3 commits into
Open
Conversation
This was referenced Jun 2, 2026
Snakeblack
force-pushed
the
fix/vscode-copilot-agent-hardening
branch
from
d9426b4 to
cbc7686
Compare
📝 WalkthroughWalkthroughAdds VS Code Copilot native multi-agent SDD support: enables sub-agent capability in the VS Code adapter, embeds 10 new ChangesVS Code Copilot SDD Native Multi-Agent Support
Sequence Diagram(s)sequenceDiagram
participant User
participant RunSync
participant componentSyncStep
participant sdd.Inject
participant renderVSCodeAgentModelAssignment
participant hideManagedClaudeInternalAgentsForVSCode
participant RenderSyncReport
User->>RunSync: sync (VSCode selection)
RunSync->>RunSync: restore VSCodeModelAssignments from state.json
RunSync->>componentSyncStep: execute SDD step (warnings ptr)
componentSyncStep->>sdd.Inject: InjectOptions{VSCodeModelAssignments, ...}
sdd.Inject->>renderVSCodeAgentModelAssignment: per .agent.md file
renderVSCodeAgentModelAssignment-->>sdd.Inject: rewritten content + warnings
sdd.Inject->>hideManagedClaudeInternalAgentsForVSCode: homeDir
hideManagedClaudeInternalAgentsForVSCode-->>sdd.Inject: changed files
sdd.Inject-->>componentSyncStep: InjectionResult{Warnings}
componentSyncStep->>componentSyncStep: addWarnings(res.Warnings)
componentSyncStep-->>RunSync: step complete
RunSync->>RunSync: dedupStrings(warnings) → result.Warnings
RunSync->>RenderSyncReport: result with Warnings
RenderSyncReport-->>User: sync report + Warnings section
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 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: 8
🤖 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 `@internal/app/app.go`:
- Around line 749-751: The current code for VSCodeModelAssignments only handles
the case when the map is non-empty, but it lacks handling for the explicit
"reset to defaults" signal when the map is non-nil but empty. Add an else branch
to the existing if condition that checks len(selection.VSCodeModelAssignments) >
0. In this else branch, set current.VSCodeModelAssignments to nil to clear it
when an explicit empty map is provided. This else branch should mirror the
pattern already implemented for other assignment maps like
ClaudeModelAssignments to maintain consistent nil vs empty-map override
semantics throughout the function.
In `@internal/assets/assets_test.go`:
- Around line 577-579: The substring match check using strings.Contains in the
frontmatterKeyLine(frontmatter, "tools") validation will incorrectly flag tool
names containing "agent" as a substring. Parse the tools line to extract
individual tool names and check for an exact match of "agent" rather than using
substring matching. This can be done by splitting the tools string by commas or
spaces and checking if "agent" is one of the resulting exact tool names.
- Around line 693-701: The current implementation in requireAgentsAllowlist uses
a fragile string count check that assumes 2-space YAML indentation. While this
is acceptable for controlled embedded assets as noted in the comment, document
this known limitation by adding a code comment above the Count check explaining
why this specific format assumption is acceptable in this context and what would
need to change if YAML formatting requirements evolve in the future.
- Around line 548-586: The sdd-orchestrator.agent.md file is missing the
required agent tool declaration in its frontmatter. The test
TestVSCodeNativeAgentAssetsFrontmatter validates this requirement via the
requireInlineTool call for the coordinator agent, which checks for the presence
of the agent tool. Add the agent tool to the frontmatter of the orchestrator
agent asset file to match the requirement being validated by the test and ensure
consistency with the phase agents that do include this tool.
In `@internal/cli/sync_test.go`:
- Around line 2092-2119: The
TestRunSyncWithSelectionPropagatesVSCodeModelWarnings test lacks explanatory
comments about why it uses a fresh temp home directory and what behavior is
expected. Add a comment block immediately before the RunSyncWithSelection
function call that explains the test setup: the fresh temp home has no models
cache, which makes the VSCode model assignments unvalidated, and the sync should
emit a cache warning while omitting the model line from the generated agent
file.
In `@internal/cli/sync.go`:
- Around line 803-812: The function stateModelAssignmentsToModel is defined
identically in two separate files, creating unnecessary duplication. Remove the
duplicate definition from one of the locations and either move the function to a
shared package location (such as within the state or model package) or keep it
in one place and update the other location to import and use the centralized
version instead. This ensures a single source of truth and prevents
inconsistencies if the function logic needs to be updated in the future.
In `@internal/components/sdd/vscode_agent_visibility.go`:
- Around line 15-52: The function hideManagedClaudeInternalAgentsForVSCode
modifies user config files without creating backups, violating the backup
requirement for user config modifications. Before calling
filemerge.WriteFileAtomic for each file that gets modified by
hideClaudeAgentFromVSCodePicker, create a backup of the original file content.
Track the backups created (likely in the InjectionResult or a similar structure)
so their paths can be returned or used for restoration. Additionally, create a
companion restore function that reverts the modified files back to their
backed-up versions, similar to how injection functions typically have
corresponding undo/restore counterparts.
In `@internal/state/state.go`:
- Line 148: The mergeExplicitAgentInstallState function currently restores
several model assignment fields from newState when non-nil (ModelAssignments,
ClaudeModelAssignments, KiroModelAssignments, CodexModelAssignments,
CodexCarrilModelAssignments, CodexPhaseModelAssignments, and Persona), but it
fails to restore VSCodeModelAssignments. Add a check in the
mergeExplicitAgentInstallState function that restores VSCodeModelAssignments
from newState when it is non-nil, following the same pattern used for the other
model assignment fields being restored from newState.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e9115036-799f-4966-87be-bcc583bdd147
⛔ Files ignored due to path filters (9)
testdata/golden/sdd-claude-agent-sdd-apply.goldenis excluded by!testdata/**testdata/golden/sdd-claude-agent-sdd-archive.goldenis excluded by!testdata/**testdata/golden/sdd-claude-agent-sdd-design.goldenis excluded by!testdata/**testdata/golden/sdd-claude-agent-sdd-explore.goldenis excluded by!testdata/**testdata/golden/sdd-claude-agent-sdd-propose.goldenis excluded by!testdata/**testdata/golden/sdd-claude-agent-sdd-spec.goldenis excluded by!testdata/**testdata/golden/sdd-claude-agent-sdd-tasks.goldenis excluded by!testdata/**testdata/golden/sdd-claude-agent-sdd-verify.goldenis excluded by!testdata/**testdata/golden/sdd-vscode-instructions.goldenis excluded by!testdata/**
📒 Files selected for processing (51)
README.mddocs/agents.mdgo.modinternal/agents/vscode/adapter.gointernal/agents/vscode/adapter_test.gointernal/app/app.gointernal/app/app_test.gointernal/assets/assets.gointernal/assets/assets_test.gointernal/assets/claude/agents/jd-fix-agent.mdinternal/assets/claude/agents/jd-judge-a.mdinternal/assets/claude/agents/jd-judge-b.mdinternal/assets/claude/agents/sdd-apply.mdinternal/assets/claude/agents/sdd-archive.mdinternal/assets/claude/agents/sdd-design.mdinternal/assets/claude/agents/sdd-explore.mdinternal/assets/claude/agents/sdd-init.mdinternal/assets/claude/agents/sdd-onboard.mdinternal/assets/claude/agents/sdd-propose.mdinternal/assets/claude/agents/sdd-spec.mdinternal/assets/claude/agents/sdd-tasks.mdinternal/assets/claude/agents/sdd-verify.mdinternal/assets/vscode/agents/sdd-apply.agent.mdinternal/assets/vscode/agents/sdd-archive.agent.mdinternal/assets/vscode/agents/sdd-design.agent.mdinternal/assets/vscode/agents/sdd-explore.agent.mdinternal/assets/vscode/agents/sdd-init.agent.mdinternal/assets/vscode/agents/sdd-onboard.agent.mdinternal/assets/vscode/agents/sdd-orchestrator.agent.mdinternal/assets/vscode/agents/sdd-propose.agent.mdinternal/assets/vscode/agents/sdd-spec.agent.mdinternal/assets/vscode/agents/sdd-tasks.agent.mdinternal/assets/vscode/agents/sdd-verify.agent.mdinternal/cli/install_test.gointernal/cli/run.gointernal/cli/sync.gointernal/cli/sync_test.gointernal/components/filemerge/replace_default.gointernal/components/filemerge/replace_windows.gointernal/components/filemerge/writer.gointernal/components/filemerge/writer_test.gointernal/components/sdd/inject.gointernal/components/sdd/inject_test.gointernal/components/sdd/vscode_agent_visibility.gointernal/components/sdd/vscode_models.gointernal/components/sdd/vscode_models_test.gointernal/components/uninstall/service_test.gointernal/model/selection.gointernal/model/selection_test.gointernal/state/state.gointernal/state/state_test.go
Comment on lines
+749
to
+751
| if len(selection.VSCodeModelAssignments) > 0 { | ||
| current.VSCodeModelAssignments = modelAssignmentsToState(selection.VSCodeModelAssignments) | ||
| } |
There was a problem hiding this comment.
Missing else branch to handle explicit clear signal for VSCodeModelAssignments.
When selection.VSCodeModelAssignments is a non-nil empty map (the explicit "reset to defaults" signal per the SyncOverrides contract in context snippet 3), the function should clear current.VSCodeModelAssignments by setting it to nil. Currently, only the non-empty write path is implemented. This breaks the nil vs empty-map override semantics that all other assignment maps follow (e.g. ClaudeModelAssignments at lines 698-703).
🛡️ Proposed fix to add the clear branch
- if len(selection.VSCodeModelAssignments) > 0 {
- current.VSCodeModelAssignments = modelAssignmentsToState(selection.VSCodeModelAssignments)
- }
+ if selection.VSCodeModelAssignments != nil {
+ if len(selection.VSCodeModelAssignments) > 0 {
+ current.VSCodeModelAssignments = modelAssignmentsToState(selection.VSCodeModelAssignments)
+ } else {
+ current.VSCodeModelAssignments = nil
+ }
+ }📝 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 len(selection.VSCodeModelAssignments) > 0 { | |
| current.VSCodeModelAssignments = modelAssignmentsToState(selection.VSCodeModelAssignments) | |
| } | |
| if selection.VSCodeModelAssignments != nil { | |
| if len(selection.VSCodeModelAssignments) > 0 { | |
| current.VSCodeModelAssignments = modelAssignmentsToState(selection.VSCodeModelAssignments) | |
| } else { | |
| current.VSCodeModelAssignments = nil | |
| } | |
| } |
🤖 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 `@internal/app/app.go` around lines 749 - 751, The current code for
VSCodeModelAssignments only handles the case when the map is non-empty, but it
lacks handling for the explicit "reset to defaults" signal when the map is
non-nil but empty. Add an else branch to the existing if condition that checks
len(selection.VSCodeModelAssignments) > 0. In this else branch, set
current.VSCodeModelAssignments to nil to clear it when an explicit empty map is
provided. This else branch should mirror the pattern already implemented for
other assignment maps like ClaudeModelAssignments to maintain consistent nil vs
empty-map override semantics throughout the function.
Comment on lines
+548
to
+586
| var vscodeSDDPhaseAgents = []string{ | ||
| "sdd-init", "sdd-explore", "sdd-propose", "sdd-spec", "sdd-design", | ||
| "sdd-tasks", "sdd-apply", "sdd-verify", "sdd-archive", "sdd-onboard", | ||
| } | ||
|
|
||
| func TestVSCodeNativeAgentAssetsFrontmatter(t *testing.T) { | ||
| coordinatorPath := "vscode/agents/sdd-orchestrator.agent.md" | ||
| coordinator := readFrontmatterBlock(t, coordinatorPath) | ||
| requireFrontmatterLine(t, coordinator, "target: vscode") | ||
| requireFrontmatterLine(t, coordinator, "user-invocable: true") | ||
| requireFrontmatterLine(t, coordinator, "disable-model-invocation: true") | ||
| requireInlineTool(t, coordinator, "agent") | ||
| requireNoDeprecatedVSCodeTools(t, coordinator) | ||
| requireAgentsAllowlist(t, coordinator, vscodeSDDPhaseAgents) | ||
| requireFrontmatterKeyAbsent(t, coordinator, "model") | ||
| requireFrontmatterKeyAbsent(t, coordinator, "infer") | ||
| requireAssetBodyContains(t, coordinatorPath, "## Agent Teams Orchestrator", "## SDD Workflow", "### Review Workload Guard") | ||
| requireAssetBodyNotContains(t, coordinatorPath, "## Model Assignments", "model parameter") | ||
|
|
||
| for _, phase := range vscodeSDDPhaseAgents { | ||
| t.Run(phase, func(t *testing.T) { | ||
| path := "vscode/agents/" + phase + ".agent.md" | ||
| frontmatter := readFrontmatterBlock(t, path) | ||
| requireFrontmatterLine(t, frontmatter, "target: vscode") | ||
| requireFrontmatterLine(t, frontmatter, "user-invocable: false") | ||
| requireFrontmatterKeyAbsent(t, frontmatter, "disable-model-invocation") | ||
| requireFrontmatterKeyAbsent(t, frontmatter, "model") | ||
| requireFrontmatterKeyAbsent(t, frontmatter, "infer") | ||
| requireNoDeprecatedVSCodeTools(t, frontmatter) | ||
| if strings.Contains(frontmatterKeyLine(frontmatter, "tools"), "agent") { | ||
| t.Fatalf("%s must not include coordinator-only agent tool", phase) | ||
| } | ||
| for _, tool := range expectedVSCodePhaseTools(phase) { | ||
| requireInlineTool(t, frontmatter, tool) | ||
| } | ||
| requireAssetBodyContains(t, path, "## Instructions", "## Engram Save", "## Result Contract") | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify VS Code agent assets match test expectations
echo "=== Checking VS Code orchestrator asset ==="
ORCH="internal/assets/vscode/agents/sdd-orchestrator.agent.md"
if [[ ! -f "$ORCH" ]]; then
echo "ERROR: Missing $ORCH"
exit 1
fi
# Check orchestrator frontmatter
grep -q "^target: vscode$" "$ORCH" || echo "ERROR: orchestrator missing 'target: vscode'"
grep -q "^user-invocable: true$" "$ORCH" || echo "ERROR: orchestrator missing 'user-invocable: true'"
grep -q "^disable-model-invocation: true$" "$ORCH" || echo "ERROR: orchestrator missing 'disable-model-invocation: true'"
grep -q "^ - agent$" "$ORCH" || echo "ERROR: orchestrator missing agent tool"
echo "=== Checking VS Code phase agent assets ==="
for phase in sdd-init sdd-explore sdd-propose sdd-spec sdd-design sdd-tasks sdd-apply sdd-verify sdd-archive sdd-onboard; do
FILE="internal/assets/vscode/agents/${phase}.agent.md"
if [[ ! -f "$FILE" ]]; then
echo "ERROR: Missing $FILE"
continue
fi
# Check phase agent frontmatter
grep -q "^target: vscode$" "$FILE" || echo "ERROR: $phase missing 'target: vscode'"
grep -q "^user-invocable: false$" "$FILE" || echo "ERROR: $phase missing 'user-invocable: false'"
# Phase agents must NOT have these keys
if grep -q "^disable-model-invocation:" "$FILE"; then
echo "ERROR: $phase has forbidden 'disable-model-invocation'"
fi
if grep -q "^model:" "$FILE"; then
echo "ERROR: $phase has forbidden 'model:'"
fi
if grep -q "^infer:" "$FILE"; then
echo "ERROR: $phase has forbidden 'infer:'"
fi
done
echo "=== Done ==="Repository: Gentleman-Programming/gentle-ai
Length of output: 215
Fix orchestrator agent asset—missing required tool declaration.
The orchestrator asset at internal/assets/vscode/agents/sdd-orchestrator.agent.md exists but is missing the agent tool in its frontmatter. The test at line 559 explicitly validates this requirement with requireInlineTool(t, coordinator, "agent") and will fail. All 10 phase agents (sdd-init through sdd-onboard) exist and have correct frontmatter.
🤖 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 `@internal/assets/assets_test.go` around lines 548 - 586, The
sdd-orchestrator.agent.md file is missing the required agent tool declaration in
its frontmatter. The test TestVSCodeNativeAgentAssetsFrontmatter validates this
requirement via the requireInlineTool call for the coordinator agent, which
checks for the presence of the agent tool. Add the agent tool to the frontmatter
of the orchestrator agent asset file to match the requirement being validated by
the test and ensure consistency with the phase agents that do include this tool.
Comment on lines
+577
to
+579
| if strings.Contains(frontmatterKeyLine(frontmatter, "tools"), "agent") { | ||
| t.Fatalf("%s must not include coordinator-only agent tool", phase) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Minor: agent tool check uses substring match.
The check strings.Contains(frontmatterKeyLine(frontmatter, "tools"), "agent") will fail if VS Code adds a tool name containing "agent" as a substring (e.g., multiagent, agent_helper). A more robust check would parse the tools line and match exact tool names.
However, this is unlikely given the current VS Code tool set (read, search, edit, execute, web), and the test will fail loudly if it happens, making this a low-priority refinement.
🤖 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 `@internal/assets/assets_test.go` around lines 577 - 579, The substring match
check using strings.Contains in the frontmatterKeyLine(frontmatter, "tools")
validation will incorrectly flag tool names containing "agent" as a substring.
Parse the tools line to extract individual tool names and check for an exact
match of "agent" rather than using substring matching. This can be done by
splitting the tools string by commas or spaces and checking if "agent" is one of
the resulting exact tool names.
Comment on lines
+693
to
+701
| func requireAgentsAllowlist(t *testing.T, frontmatter string, want []string) { | ||
| t.Helper() | ||
| if strings.Count(frontmatter, "\n - ") != len(want) { | ||
| t.Fatalf("coordinator agents allowlist must contain only %v:\n%s", want, frontmatter) | ||
| } | ||
| for _, agent := range want { | ||
| requireFrontmatterLine(t, frontmatter, " - "+agent) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Allowlist count check is fragile but acceptable for embedded assets.
The check strings.Count(frontmatter, "\n - ") != len(want) assumes all YAML list items use exactly 2-space indentation. If the YAML formatting changes (e.g., different indentation or inline array syntax), this will break.
However, since these are embedded assets with controlled formatting, this is acceptable. A more robust check would parse the YAML, but that adds complexity for minimal benefit.
🤖 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 `@internal/assets/assets_test.go` around lines 693 - 701, The current
implementation in requireAgentsAllowlist uses a fragile string count check that
assumes 2-space YAML indentation. While this is acceptable for controlled
embedded assets as noted in the comment, document this known limitation by
adding a code comment above the Count check explaining why this specific format
assumption is acceptable in this context and what would need to change if YAML
formatting requirements evolve in the future.
Comment on lines
+2092
to
2119
| func TestRunSyncWithSelectionPropagatesVSCodeModelWarnings(t *testing.T) { | ||
| home := t.TempDir() | ||
| t.Setenv("HOME", home) | ||
| t.Setenv("USERPROFILE", home) | ||
| selection := model.Selection{ | ||
| Agents: []model.AgentID{model.AgentVSCodeCopilot}, | ||
| Components: []model.ComponentID{model.ComponentSDD}, | ||
| VSCodeModelAssignments: map[string]model.ModelAssignment{ | ||
| "sdd-apply": {ProviderID: "github-copilot", ModelID: "gpt-4.1"}, | ||
| }, | ||
| } | ||
|
|
||
| result, err := RunSyncWithSelection(home, selection) | ||
| if err != nil { | ||
| t.Fatalf("RunSyncWithSelection() error = %v", err) | ||
| } | ||
| if !containsSubstring(result.Warnings, "models cache") { | ||
| t.Fatalf("Warnings = %v, want missing cache warning", result.Warnings) | ||
| } | ||
|
|
||
| content, err := os.ReadFile(filepath.Join(home, ".copilot", "agents", "sdd-apply.agent.md")) | ||
| if err != nil { | ||
| t.Fatalf("ReadFile(sdd-apply.agent.md): %v", err) | ||
| } | ||
| if strings.Contains(string(content), "model:") { | ||
| t.Fatalf("missing cache should omit model line; got:\n%s", content) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider adding a setup comment for clarity.
The test correctly simulates the "unvalidated model assignment" case by running in a fresh temp home (no models cache). A brief comment explaining this setup would help future readers understand the test scenario:
// Fresh temp home → no models cache → assignments are unvalidated.
// Sync should emit a cache warning and omit model: from generated agents.
result, err := RunSyncWithSelection(home, selection)This is optional but would make the test intent clearer at a glance.
🤖 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 `@internal/cli/sync_test.go` around lines 2092 - 2119, The
TestRunSyncWithSelectionPropagatesVSCodeModelWarnings test lacks explanatory
comments about why it uses a fresh temp home directory and what behavior is
expected. Add a comment block immediately before the RunSyncWithSelection
function call that explains the test setup: the fresh temp home has no models
cache, which makes the VSCode model assignments unvalidated, and the sync should
emit a cache warning while omitting the model line from the generated agent
file.
Comment on lines
+803
to
+812
| func stateModelAssignmentsToModel(m map[string]state.ModelAssignmentState) map[string]model.ModelAssignment { | ||
| if len(m) == 0 { | ||
| return nil | ||
| } | ||
| out := make(map[string]model.ModelAssignment, len(m)) | ||
| for k, v := range m { | ||
| out[k] = model.ModelAssignment{ProviderID: v.ProviderID, ModelID: v.ModelID, Effort: v.Effort} | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consolidate duplicated stateModelAssignmentsToModel helper.
This helper is defined identically in both internal/app/app.go (lines 755-764) and here in internal/cli/sync.go. Consider moving it to a shared location (e.g. as a method on state or model package) to avoid duplication and ensure consistency.
🤖 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 `@internal/cli/sync.go` around lines 803 - 812, The function
stateModelAssignmentsToModel is defined identically in two separate files,
creating unnecessary duplication. Remove the duplicate definition from one of
the locations and either move the function to a shared package location (such as
within the state or model package) or keep it in one place and update the other
location to import and use the centralized version instead. This ensures a
single source of truth and prevents inconsistencies if the function logic needs
to be updated in the future.
Comment on lines
+15
to
+52
| func hideManagedClaudeInternalAgentsForVSCode(homeDir string) (InjectionResult, error) { | ||
| agentsDir := filepath.Join(homeDir, ".claude", "agents") | ||
| if info, err := os.Stat(agentsDir); err != nil { | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| return InjectionResult{}, nil | ||
| } | ||
| return InjectionResult{}, fmt.Errorf("inspect Claude agents dir: %w", err) | ||
| } else if !info.IsDir() { | ||
| return InjectionResult{}, nil | ||
| } | ||
|
|
||
| result := InjectionResult{} | ||
| for _, fileName := range managedClaudeInternalAgentFiles() { | ||
| path := filepath.Join(agentsDir, fileName) | ||
| content, err := os.ReadFile(path) | ||
| if err != nil { | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| continue | ||
| } | ||
| return InjectionResult{}, fmt.Errorf("read managed Claude agent %s: %w", fileName, err) | ||
| } | ||
|
|
||
| text := string(content) | ||
| updated := hideClaudeAgentFromVSCodePicker(text) | ||
| if updated == text { | ||
| continue | ||
| } | ||
| writeResult, err := filemerge.WriteFileAtomic(path, []byte(updated), 0o644) | ||
| if err != nil { | ||
| return InjectionResult{}, fmt.Errorf("write managed Claude agent %s: %w", fileName, err) | ||
| } | ||
| if writeResult.Changed { | ||
| result.Changed = true | ||
| result.Files = append(result.Files, path) | ||
| } | ||
| } | ||
| return result, nil | ||
| } |
There was a problem hiding this comment.
Missing backup/restore path for user config writes.
This function modifies user config files in ~/.claude/agents without creating backups. As per coding guidelines, any write to user config must have a backup/restore path. Consider adding backup creation before modification and exposing a restoration function.
🤖 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 `@internal/components/sdd/vscode_agent_visibility.go` around lines 15 - 52, The
function hideManagedClaudeInternalAgentsForVSCode modifies user config files
without creating backups, violating the backup requirement for user config
modifications. Before calling filemerge.WriteFileAtomic for each file that gets
modified by hideClaudeAgentFromVSCodePicker, create a backup of the original
file content. Track the backups created (likely in the InjectionResult or a
similar structure) so their paths can be returned or used for restoration.
Additionally, create a companion restore function that reverts the modified
files back to their backed-up versions, similar to how injection functions
typically have corresponding undo/restore counterparts.
Source: Coding guidelines
| return InstallState{ | ||
| InstalledAgents: merged, | ||
| ModelAssignments: existing.ModelAssignments, | ||
| VSCodeModelAssignments: existing.VSCodeModelAssignments, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if run.go restores newState.VSCodeModelAssignments after MergeAgents call
rg -nP -A 30 'func mergeExplicitAgentInstallState' internal/cli/run.go | \
rg 'VSCodeModelAssignments'Repository: Gentleman-Programming/gentle-ai
Length of output: 57
🏁 Script executed:
# First, verify the files exist and find the exact location
find . -name "run.go" -type f | head -20Repository: Gentleman-Programming/gentle-ai
Length of output: 97
🏁 Script executed:
# Then check for the mergeExplicitAgentInstallState function
rg -l 'mergeExplicitAgentInstallState' --type goRepository: Gentleman-Programming/gentle-ai
Length of output: 126
🏁 Script executed:
# Check state.go for MergeAgents function
rg -l 'MergeAgents' --type goRepository: Gentleman-Programming/gentle-ai
Length of output: 148
🏁 Script executed:
# Read the mergeExplicitAgentInstallState function from run.go
rg -n 'func mergeExplicitAgentInstallState' internal/cli/run.go -A 100 | head -150Repository: Gentleman-Programming/gentle-ai
Length of output: 3909
🏁 Script executed:
# Read the MergeAgents function from state.go
rg -n 'func.*MergeAgents' internal/state/state.go -A 50Repository: Gentleman-Programming/gentle-ai
Length of output: 1923
Missing restoration of VSCodeModelAssignments in incremental installs.
After MergeAgents preserves VSCodeModelAssignments from existing state, mergeExplicitAgentInstallState in internal/cli/run.go restores ModelAssignments, ClaudeModelAssignments, KiroModelAssignments, CodexModelAssignments, CodexCarrilModelAssignments, CodexPhaseModelAssignments, and Persona from newState when non-nil (lines 209–233), but does NOT restore VSCodeModelAssignments. This causes incremental installs with new VS Code model assignments to discard them in favor of persisted values.
Add the following check after line 230 in internal/cli/run.go:
if newState.VSCodeModelAssignments != nil {
merged.VSCodeModelAssignments = newState.VSCodeModelAssignments
}
🤖 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 `@internal/state/state.go` at line 148, The mergeExplicitAgentInstallState
function currently restores several model assignment fields from newState when
non-nil (ModelAssignments, ClaudeModelAssignments, KiroModelAssignments,
CodexModelAssignments, CodexCarrilModelAssignments, CodexPhaseModelAssignments,
and Persona), but it fails to restore VSCodeModelAssignments. Add a check in the
mergeExplicitAgentInstallState function that restores VSCodeModelAssignments
from newState when it is non-nil, following the same pattern used for the other
model assignment fields being restored from newState.
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.
🔗 Linked Issue
Closes #504
Depends on: #731
Related context: #708
🏷️ PR Type
What kind of change does this PR introduce?
type:bug— Bug fix (non-breaking change that fixes an issue)type:feature— New feature (non-breaking change that adds functionality)type:docs— Documentation onlytype:refactor— Code refactoring (no functional changes)type:chore— Build, CI, or tooling changestype:breaking-change— Breaking change (fix or feature that changes existing behavior)🔗 Chain Context
PR 3 of the VS Code Copilot SDD subagents stack.
This PR depends on #731. Because the stack is currently opened from a contributor fork and upstream dependency branches are not available as base branches, this PR targets
main; reviewers should use the focused diff below for this work unit.Focused review diff:
Snakeblack/gentle-ai@feat/vscode-copilot-subagents-pr2...fix/vscode-copilot-agent-hardening
📝 Summary
sdd-orchestratorvisible in VS Code while preserving internal phase agents for delegation.model:frontmatter when VS Code model assignments are missing or invalid, and updates SDD golden fixtures.📂 Changes
internal/components/filemergeMoveFileExon Windowsinternal/components/sddinternal/assets/claude/agentstestdata/golden🧠 Size Exception Rationale
This PR requests
size:exception.The focused hardening diff is slightly above the 400-line budget because the installer fix, visibility contract, stale model cleanup, and golden fixtures must be reviewed together. Splitting them would hide the important invariant: VS Code should expose only the orchestrator to users while internal agents remain available for delegation and safe sync.
🧪 Test Plan
Linux/LF full suite
go test ./... go vet ./...Focused golden/contract checks
Results:
go test ./...) in Linux/LF verification clonecd e2e && ./docker-test.sh)Note: local Windows full-suite runs still have known CRLF/env-sensitive golden noise unrelated to this PR. Final validation was done in a clean Linux/LF runner.
🤖 Automated Checks
size:exception; focused work unit is cohesiveCloses #504status:approvedstatus:approvedtype:*Labeltype:bugrequested; maintainer label neededgo test ./...✅ Contributor Checklist
status:approvedsize:exceptionwith rationale documentedtype:*label to this PR — maintainer label needed (type:bug)go test ./...)cd e2e && ./docker-test.sh)Co-Authored-Bytrailers💬 Notes for Reviewers
The key review contract is VS Code visibility: the native dropdown should expose
sdd-orchestratoronly. Phase agents are intentionally hidden from users but still available for orchestration/delegation.Summary by CodeRabbit
Release Notes
New Features
Documentation
Improvements