feat(vscode): add model assignment foundation#731
Open
Snakeblack wants to merge 11 commits into
Open
Conversation
Author
|
Maintainer label request: please apply exactly one type label, \ ype:feature\, and \size:exception\. The size exception is intentional: PR2's persistence + resolver + render + warning propagation must be reviewed as one cohesive safety contract, and the PR body documents the rationale. |
This was referenced Jun 2, 2026
Snakeblack
force-pushed
the
feat/vscode-copilot-subagents-pr2
branch
from
763bbf0 to
3fe7acd
Compare
|
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:
📝 WalkthroughWalkthroughVS Code Copilot sub-agent support is enabled, embedded SDD agent files were added, VS Code model assignments are persisted and restored, and injection/sync now resolve, render, and report VS Code agent model settings and warnings. ChangesVS Code Copilot SDD Native Sub-Agent Support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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. 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: 2
🤖 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 condition in the VSCodeModelAssignments block at the
if statement is checking if the length is greater than zero, which prevents
explicit clearing when a non-nil empty map is passed. Change the condition from
checking len(selection.VSCodeModelAssignments) > 0 to instead checking if
selection.VSCodeModelAssignments is not nil. This will allow the
modelAssignmentsToState function to process empty maps that represent an
explicit clear intent, while still allowing the caller to pass nil if they want
to skip processing assignments entirely.
In `@internal/cli/run.go`:
- Around line 183-184: The mergeExplicitAgentInstallState function is not
copying the VSCodeModelAssignments field when merging state, causing newly
selected VS Code assignments to be lost when running install with an existing
state file. Add VSCodeModelAssignments to the field assignments in the
mergeExplicitAgentInstallState function to ensure the value set by RunInstall is
preserved during the state merge operation.
🪄 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: 415afe5b-2b1d-41d7-883e-cb87ffe57903
📒 Files selected for processing (30)
internal/agents/vscode/adapter.gointernal/agents/vscode/adapter_test.gointernal/app/app.gointernal/app/app_test.gointernal/assets/assets.gointernal/assets/assets_test.gointernal/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/sdd/inject.gointernal/components/sdd/inject_test.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
Snakeblack
force-pushed
the
feat/vscode-copilot-subagents-pr2
branch
from
3fe7acd to
20a3005
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/cli/run.go`:
- Line 183: Preserve explicit clears for VS Code model assignments during
install merges: `modelAssignmentsToState` currently turns a non-nil empty
`VSCodeModelAssignments` map into nil, so `mergeExplicitAgentInstallState`
treats it as “no change” and leaves stale assignments behind. Update the install
path in `internal/cli/run.go` to use a dedicated conversion helper (for example,
around `modelAssignmentsToState` / `installModelAssignmentsToState`) that
returns an empty non-nil map when the input map is empty, while still returning
nil only for a nil input, and apply the same fix at both call sites noted in the
diff.
In `@internal/cli/sync.go`:
- Around line 992-998: In sync.go, the VSCodeModelAssignments merge in the
selection/persistedState handling uses len(...) == 0, which incorrectly treats
an explicitly empty map as missing. Update the check on
selection.VSCodeModelAssignments to use a nil test so only nil means “no
override,” while an empty map remains an intentional reset to defaults. Keep the
existing persistedState.VSCodeModelAssignments fallback logic, but only apply it
when the selection map is nil.
In `@internal/components/sdd/vscode_models_test.go`:
- Around line 14-22: The test for vscodeModelAssignmentKeys() only checks that
required entries exist, so it will miss accidental allowlist expansion. Update
TestVSCodeModelAssignmentKeysIncludeCoordinatorAndPhases to assert the exact set
returned by vscodeModelAssignmentKeys() (or at minimum verify the length
matches), using the existing vscodeModelAssignmentKeys and containsString
helpers so any unexpected extra native .agent.md key fails the test.
🪄 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: 2b6ad4ed-928d-47db-bd5b-fdffe17e961c
📒 Files selected for processing (16)
internal/app/app.gointernal/app/app_test.gointernal/assets/assets_test.gointernal/cli/install_test.gointernal/cli/run.gointernal/cli/run_state_test.gointernal/cli/sync.gointernal/cli/sync_test.gointernal/components/sdd/inject.gointernal/components/sdd/inject_test.gointernal/components/sdd/vscode_models.gointernal/components/sdd/vscode_models_test.gointernal/model/selection.gointernal/model/selection_test.gointernal/state/state.gointernal/state/state_test.go
Comment on lines
+992
to
+998
| if len(selection.VSCodeModelAssignments) == 0 && len(persistedState.VSCodeModelAssignments) > 0 { | ||
| m := make(map[string]model.ModelAssignment, len(persistedState.VSCodeModelAssignments)) | ||
| for k, v := range persistedState.VSCodeModelAssignments { | ||
| m[k] = model.ModelAssignment{ProviderID: v.ProviderID, ModelID: v.ModelID, Effort: v.Effort} | ||
| } | ||
| selection.VSCodeModelAssignments = m | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use a nil-check here, not len(...) == 0.
Line 992 treats a non-nil empty VSCodeModelAssignments map as “missing” and reloads persisted values, so an explicit reset-to-defaults request cannot stick. That breaks the documented nil = no override; empty map = reset to defaults contract for this map.
Suggested fix
- if len(selection.VSCodeModelAssignments) == 0 && len(persistedState.VSCodeModelAssignments) > 0 {
+ if selection.VSCodeModelAssignments == nil && len(persistedState.VSCodeModelAssignments) > 0 {
m := make(map[string]model.ModelAssignment, len(persistedState.VSCodeModelAssignments))
for k, v := range persistedState.VSCodeModelAssignments {
m[k] = model.ModelAssignment{ProviderID: v.ProviderID, ModelID: v.ModelID, Effort: v.Effort}
}
selection.VSCodeModelAssignments = m
}📝 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 && len(persistedState.VSCodeModelAssignments) > 0 { | |
| m := make(map[string]model.ModelAssignment, len(persistedState.VSCodeModelAssignments)) | |
| for k, v := range persistedState.VSCodeModelAssignments { | |
| m[k] = model.ModelAssignment{ProviderID: v.ProviderID, ModelID: v.ModelID, Effort: v.Effort} | |
| } | |
| selection.VSCodeModelAssignments = m | |
| } | |
| if selection.VSCodeModelAssignments == nil && len(persistedState.VSCodeModelAssignments) > 0 { | |
| m := make(map[string]model.ModelAssignment, len(persistedState.VSCodeModelAssignments)) | |
| for k, v := range persistedState.VSCodeModelAssignments { | |
| m[k] = model.ModelAssignment{ProviderID: v.ProviderID, ModelID: v.ModelID, Effort: v.Effort} | |
| } | |
| selection.VSCodeModelAssignments = m | |
| } |
🤖 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 992 - 998, In sync.go, the
VSCodeModelAssignments merge in the selection/persistedState handling uses
len(...) == 0, which incorrectly treats an explicitly empty map as missing.
Update the check on selection.VSCodeModelAssignments to use a nil test so only
nil means “no override,” while an empty map remains an intentional reset to
defaults. Keep the existing persistedState.VSCodeModelAssignments fallback
logic, but only apply it when the selection map is nil.
20a3005 to
066e2d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/cli/sync.go`:
- Around line 445-450: The CodeGraph guidance refresh is currently scoped only
by homeDir, so the sync step can affect every discovered agent instead of just
the selected ones. Update the sync path around codeGraphGuidanceSyncStep and the
downstream guidance/backup helpers to accept and use r.agentIDs (or the selected
adapters) when enumerating targets, and ensure only those agents’ AGENTS.md
files are read, backed up, and rewritten during refresh.
In `@internal/components/sdd/inject.go`:
- Around line 947-1014: The cleanup in
removeLegacyOpenCodePlainChatPreflightLines is overbroad because it removes any
line matching legacyFragments anywhere in the prompt, which can delete
user-authored content during sync. Change the logic in inject.go so it only
strips a known stale preflight block or an exact legacy section managed by this
flow, rather than scanning every line for substrings like “A. Pace” or “Before
continuing with SDD...”. Keep the rewrite idempotent in the opencode.json sync
path, and preserve unrelated prompt text by narrowing the match to unique
section markers handled by removeLegacyOpenCodePlainChatPreflightLines and the
surrounding prompt rewrite functions.
In `@internal/components/sdd/vscode_models.go`:
- Around line 143-158: The cache/path warning text in loadVSCodeModelCatalog is
agent-specific because it interpolates agentKey, which prevents dedupWarnings()
from collapsing repeated shared failures. Update the warning strings returned
from loadVSCodeModelCatalog to be agent-neutral while keeping the cache
path/error details, and only leave agentKey out of the shared warning message so
identical cache failures deduplicate across phases.
🪄 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: 6b403d65-a19d-4691-a4ad-57a916156ef4
📒 Files selected for processing (31)
internal/agents/vscode/adapter.gointernal/agents/vscode/adapter_test.gointernal/app/app.gointernal/app/app_test.gointernal/assets/assets.gointernal/assets/assets_test.gointernal/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/run_state_test.gointernal/cli/sync.gointernal/cli/sync_test.gointernal/components/sdd/inject.gointernal/components/sdd/inject_test.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
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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/cli/sync.go`:
- Around line 445-450: The CodeGraph guidance refresh is currently scoped only
by homeDir, so the sync step can affect every discovered agent instead of just
the selected ones. Update the sync path around codeGraphGuidanceSyncStep and the
downstream guidance/backup helpers to accept and use r.agentIDs (or the selected
adapters) when enumerating targets, and ensure only those agents’ AGENTS.md
files are read, backed up, and rewritten during refresh.
In `@internal/components/sdd/inject.go`:
- Around line 947-1014: The cleanup in
removeLegacyOpenCodePlainChatPreflightLines is overbroad because it removes any
line matching legacyFragments anywhere in the prompt, which can delete
user-authored content during sync. Change the logic in inject.go so it only
strips a known stale preflight block or an exact legacy section managed by this
flow, rather than scanning every line for substrings like “A. Pace” or “Before
continuing with SDD...”. Keep the rewrite idempotent in the opencode.json sync
path, and preserve unrelated prompt text by narrowing the match to unique
section markers handled by removeLegacyOpenCodePlainChatPreflightLines and the
surrounding prompt rewrite functions.
In `@internal/components/sdd/vscode_models.go`:
- Around line 143-158: The cache/path warning text in loadVSCodeModelCatalog is
agent-specific because it interpolates agentKey, which prevents dedupWarnings()
from collapsing repeated shared failures. Update the warning strings returned
from loadVSCodeModelCatalog to be agent-neutral while keeping the cache
path/error details, and only leave agentKey out of the shared warning message so
identical cache failures deduplicate across phases.
🪄 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: 6b403d65-a19d-4691-a4ad-57a916156ef4
📒 Files selected for processing (31)
internal/agents/vscode/adapter.gointernal/agents/vscode/adapter_test.gointernal/app/app.gointernal/app/app_test.gointernal/assets/assets.gointernal/assets/assets_test.gointernal/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/run_state_test.gointernal/cli/sync.gointernal/cli/sync_test.gointernal/components/sdd/inject.gointernal/components/sdd/inject_test.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
🛑 Comments failed to post (3)
internal/cli/sync.go (1)
445-450: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Scope CodeGraph guidance refresh to the selected agents.
This path is keyed only by
homeDir, but the downstream guidance helpers enumerate all discovered installed agents. A targeted sync can therefore rewrite another agent’sAGENTS.mdjust because that agent has CodeGraph configured. Please thread the selectedr.agentIDs/adapters into the refresh and backup-target helpers so sync only touches files for the agents the user asked to manage.Also applies to: 472-476, 594-607
🤖 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 445 - 450, The CodeGraph guidance refresh is currently scoped only by homeDir, so the sync step can affect every discovered agent instead of just the selected ones. Update the sync path around codeGraphGuidanceSyncStep and the downstream guidance/backup helpers to accept and use r.agentIDs (or the selected adapters) when enumerating targets, and ensure only those agents’ AGENTS.md files are read, backed up, and rewritten during refresh.internal/components/sdd/inject.go (1)
947-1014: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don't strip preserved prompt content via global substring matches.
removeLegacyOpenCodePlainChatPreflightLinesdeletes any line containing fragments likeA. PaceorBefore continuing with SDD...anywhere in the preserved prompt. Because this runs before rewritingopencode.json, unrelated user-authored prompt text that happens to reuse those phrases gets silently removed on sync, with no recovery path in this flow. Scope the cleanup to a managed stale block or an exact legacy preflight section instead of scanning the whole prompt line-by-line. As per path instructions, "internal/components/**/*.go: Install/sync operations must be idempotent (running twice equals running once). Any write to user config must have a backup/restore path."🤖 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/inject.go` around lines 947 - 1014, The cleanup in removeLegacyOpenCodePlainChatPreflightLines is overbroad because it removes any line matching legacyFragments anywhere in the prompt, which can delete user-authored content during sync. Change the logic in inject.go so it only strips a known stale preflight block or an exact legacy section managed by this flow, rather than scanning every line for substrings like “A. Pace” or “Before continuing with SDD...”. Keep the rewrite idempotent in the opencode.json sync path, and preserve unrelated prompt text by narrowing the match to unique section markers handled by removeLegacyOpenCodePlainChatPreflightLines and the surrounding prompt rewrite functions.Source: Path instructions
internal/components/sdd/vscode_models.go (1)
143-158: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make shared cache warnings agent-neutral before dedup.
These cache/path failures are shared across all VS Code assignments, but the warning text includes
agentKey.dedupWarnings()only removes exact matches, so one missing or unreadable cache will emit one warning per phase instead of a single deduped warning.💡 Suggested direction
- return nil, []string{fmt.Sprintf("VS Code model assignment for %s skipped: models cache path unavailable", agentKey)} + return nil, []string{"VS Code model assignments skipped: models cache path unavailable"} - return nil, []string{fmt.Sprintf("VS Code model assignment for %s skipped: models cache %q not found", agentKey, path)} + return nil, []string{fmt.Sprintf("VS Code model assignments skipped: models cache %q not found", path)} - return nil, []string{fmt.Sprintf("VS Code model assignment for %s skipped: read models cache %q: %v", agentKey, path, err)} + return nil, []string{fmt.Sprintf("VS Code model assignments skipped: read models cache %q: %v", path, err)}📝 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.func loadVSCodeModelCatalog(agentKey, cachePath, variantsPath string) (map[string]opencode.Provider, []string) { path := cachePath if path == "" { path = opencode.DefaultCachePath() } if path == "" { return nil, []string{"VS Code model assignments skipped: models cache path unavailable"} } providers, err := opencode.LoadModels(path) if err != nil { if errors.Is(err, os.ErrNotExist) { return nil, []string{fmt.Sprintf("VS Code model assignments skipped: models cache %q not found", path)} } return nil, []string{fmt.Sprintf("VS Code model assignments skipped: read models cache %q: %v", path, err)} }🤖 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_models.go` around lines 143 - 158, The cache/path warning text in loadVSCodeModelCatalog is agent-specific because it interpolates agentKey, which prevents dedupWarnings() from collapsing repeated shared failures. Update the warning strings returned from loadVSCodeModelCatalog to be agent-neutral while keeping the cache path/error details, and only leave agentKey out of the shared warning message so identical cache failures deduplicate across phases.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@go.mod`:
- Line 11: Update the golang.org/x/sys dependency in go.mod to move off v0.38.0
and onto a fixed release such as v0.44.0 or later, preferably v0.45.0 if you
want the Go team’s patched version. Make sure any corresponding module metadata
or lockfile state is refreshed so the dependency graph consistently uses the
upgraded version.
In `@internal/assets/assets_test.go`:
- Around line 632-634: The hidden-agent check in isClaudeInternalAgentName is
duplicating the prefix rules instead of using the same logic as the VS Code
picker visibility path. Update this test helper to reuse the production
predicate/output that determines which agent names are hidden, so the test
tracks the real contract and stays aligned if the picker visibility rules
change.
In `@internal/cli/sync.go`:
- Around line 445-451: The CodeGraph sync flow is using different agent ID
sources for planning and backup, so make it consistently use the same resolved
agent IDs everywhere. In the sync setup where the codeGraphGuidanceSyncStep is
appended, use the resolved agent IDs rather than r.agentIDs directly, and in the
backup snapshot path that currently uses selection.Agents, switch to the same
resolved set so both the write and rollback snapshot cover the same files.
Update the related logic in sync.go around codeGraphGuidanceSyncStep and the
backup/snapshot handling to share one normalized agent ID list.
In `@internal/components/communitytool/codegraph_guidance.go`:
- Around line 145-155: HasConfiguredCodeGraphForAgents is leaking non-requested
agent state through its fallback, so keep it strictly scoped to the provided
agentIDs. In HasConfiguredCodeGraphForAgents, remove or replace the final
hasDetectedCodeGraphToolWiring(homeDir) fallback so it only returns true when
one of the matched agents in the status.Agents loop is both detected and
configured. If a fallback is still needed, make it accept agentIDs and validate
the wiring against those agents only.
- Around line 217-225: The file-tracking in the guidance injection flow is
adding every non-empty target file even when `injectCodeGraphGuidanceForAgent`
reports no change, so `result.Files` can include unchanged files once
`result.Changed` becomes true later. Update the loop in
`injectCodeGraphGuidanceForAgent`’s caller to append to `result.Files` only when
`changed` is true, while still preserving the existing error handling and
overall `result.Changed` aggregation.
In `@internal/components/sdd/inject.go`:
- Around line 962-981: The prompt filtering logic in inject.go should not start
discarding lines unless both the opener and a matching trailer are present.
Update the inBlock scanning around the line checks in this legacy prompt cleanup
so that it first verifies a complete block exists before entering discard mode,
and preserve the remaining content when the closing marker is missing or
unrecognized. Use the existing inBlock state and the hard-coded opener/trailer
string matches as the place to add the guard.
In `@internal/components/sdd/vscode_agent_visibility.go`:
- Around line 27-45: The current VSCode visibility update logic in
hideClaudeAgentFromVSCodePicker and the managedClaudeInternalAgentFiles loop
rewrites any reserved-name file under the agents directory based only on
filename. Add a guard that verifies the file is actually Gentle-managed using
the existing install-state or marker record before calling
filemerge.WriteFileAtomic, and skip user-authored files entirely. Route any
allowed mutation through the rollback/backup flow so changes to user config
remain reversible and the operation stays idempotent.
🪄 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: dc074462-65b4-4327-93aa-199766ea1085
⛔ 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 (29)
README.mddocs/agents.mdgo.modinternal/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/cli/sync.gointernal/cli/sync_test.gointernal/components/communitytool/codegraph_guidance.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.go
| github.com/charmbracelet/lipgloss v1.1.0 | ||
| github.com/mattn/go-isatty v0.0.20 | ||
| github.com/rivo/uniseg v0.4.7 | ||
| golang.org/x/sys v0.38.0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== go.mod ==\n'
cat -n go.mod | sed -n '1,80p'
printf '\n== x/sys references ==\n'
rg -n --hidden --glob '!**/vendor/**' 'golang\.org/x/sys|x/sys' .Repository: Gentleman-Programming/gentle-ai
Length of output: 1985
🌐 Web query:
GO-2026-5024 golang.org/x/sys windows vulnerable version advisory
💡 Result:
GO-2026-5024 (also tracked as CVE-2026-39824) is a security vulnerability in the golang.org/x/sys/windows package [1][2]. The issue involves an integer overflow in the NewNTUnicodeString function [3][4]. Specifically, the function failed to properly check for string length overflows; when provided with a string exceeding the maximum size of a NTUnicodeString (a 16-bit number of bytes), it would return a truncated string rather than an error [1][2]. This vulnerability affects versions of golang.org/x/sys prior to v0.44.0 [1][2]. Users are advised to update to a patched version to resolve this issue [3][5]. The Go Security Team officially announced the fix in version v0.45.0, though the underlying patch was included in v0.44.0 [3][5].
Citations:
- 1: https://pkg.go.dev/vuln/GO-2026-5024
- 2: https://nvd.nist.gov/vuln/detail/CVE-2026-39824
- 3: https://groups.google.com/g/golang-announce/c/6MMI8Lj-Atg
- 4: https://test.osv.dev/vulnerability/GO-2026-5024
- 5: x/sys/windows, internal/syscall/windows: integer overflow in NewNTUnicodeString golang/go#78916
Bump golang.org/x/sys off v0.38.0. v0.38.0 is in the affected range for GO-2026-5024 / CVE-2026-39824, so this keeps Windows builds on a vulnerable release. Upgrade to a patched version (v0.44.0+, or v0.45.0 if you want the Go team’s fix).
🧰 Tools
🪛 OSV Scanner (2.4.0)
[LOW] 11-11: golang.org/x/sys 0.38.0: Invoking integer overflow in NewNTUnicodeString in golang.org/x/sys/windows
(GO-2026-5024)
🤖 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 `@go.mod` at line 11, Update the golang.org/x/sys dependency in go.mod to move
off v0.38.0 and onto a fixed release such as v0.44.0 or later, preferably
v0.45.0 if you want the Go team’s patched version. Make sure any corresponding
module metadata or lockfile state is refreshed so the dependency graph
consistently uses the upgraded version.
Source: Linters/SAST tools
Comment on lines
+632
to
+634
| func isClaudeInternalAgentName(name string) bool { | ||
| return strings.HasPrefix(name, "sdd-") || strings.HasPrefix(name, "jd-") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Share the hidden-agent predicate with production code.
This helper hard-codes sdd-/jd- prefixes instead of deriving the set from the actual VS Code picker visibility logic added in this stack. If that logic changes, this test can stay green while picker behavior regresses. Reuse the same predicate/output here so the test exercises the real contract.
🤖 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 632 - 634, The hidden-agent
check in isClaudeInternalAgentName is duplicating the prefix rules instead of
using the same logic as the VS Code picker visibility path. Update this test
helper to reuse the production predicate/output that determines which agent
names are hidden, so the test tracks the real contract and stays aligned if the
picker visibility rules change.
Comment on lines
+145
to
+155
| func HasConfiguredCodeGraphForAgents(homeDir string, agentIDs []model.AgentID, detector Detector) bool { | ||
| status := DetectStatus(model.CommunityToolCodeGraph, homeDir, detector) | ||
| if status.CLI != AvailabilityAvailable { | ||
| return false | ||
| } | ||
| for _, agent := range status.Agents { | ||
| if slices.Contains(agentIDs, agent.Agent) && agent.Detected && agent.Configured { | ||
| return true | ||
| } | ||
| } | ||
| return hasDetectedCodeGraphToolWiring(homeDir) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep HasConfiguredCodeGraphForAgents fully agent-scoped.
After the per-agent loop, Line 155 falls back to hasDetectedCodeGraphToolWiring(homeDir), which is not filtered by agentIDs. That means CodeGraph wiring for some other installed agent can make this return true and trigger guidance writes for the requested agents anyway, defeating the new agent-scoped sync behavior.
🤖 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/communitytool/codegraph_guidance.go` around lines 145 -
155, HasConfiguredCodeGraphForAgents is leaking non-requested agent state
through its fallback, so keep it strictly scoped to the provided agentIDs. In
HasConfiguredCodeGraphForAgents, remove or replace the final
hasDetectedCodeGraphToolWiring(homeDir) fallback so it only returns true when
one of the matched agents in the status.Agents loop is both detected and
configured. If a fallback is still needed, make it accept agentIDs and validate
the wiring against those agents only.
Source: Path instructions
Comment on lines
+27
to
+45
| 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) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Don't rewrite ~/.claude/agents files based only on filename.
Any user-authored sdd-apply.md / jd-judge-a.md / similar reserved-name file under ~/.claude/agents matches this list and gets rewritten in place to user-invocable: false, even if Gentle AI never created it. That is a user-config mutation with no backup/restore path. Please gate this on a Gentle-managed marker/install-state record before rewriting, and route the change through rollback. As per path instructions, "Install/sync operations must be idempotent (running twice equals running once). Any write to user config must have a backup/restore path."
Also applies to: 54-69
🤖 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 27 - 45, The
current VSCode visibility update logic in hideClaudeAgentFromVSCodePicker and
the managedClaudeInternalAgentFiles loop rewrites any reserved-name file under
the agents directory based only on filename. Add a guard that verifies the file
is actually Gentle-managed using the existing install-state or marker record
before calling filemerge.WriteFileAtomic, and skip user-authored files entirely.
Route any allowed mutation through the rollback/backup flow so changes to user
config remain reversible and the operation stays idempotent.
Source: Path instructions
Snakeblack
force-pushed
the
feat/vscode-copilot-subagents-pr2
branch
from
8d43e81 to
cc9f892
Compare
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 (2)
internal/app/app.go (2)
657-666: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the new
stateModelAssignmentsToModelhelper forModelAssignmentstoo.The
ModelAssignmentsblock (lines 657-662) manually repeats the exact conversion thatstateModelAssignmentsToModel(introduced by this PR for VSCode) now encapsulates. Consolidate to avoid divergence between the two nearly-identical conversion paths.♻️ Proposed refactor
if len(selection.ModelAssignments) == 0 && len(s.ModelAssignments) > 0 { - m := make(map[string]model.ModelAssignment, len(s.ModelAssignments)) - for k, v := range s.ModelAssignments { - m[k] = model.ModelAssignment{ProviderID: v.ProviderID, ModelID: v.ModelID, Effort: v.Effort} - } - selection.ModelAssignments = m + selection.ModelAssignments = stateModelAssignmentsToModel(s.ModelAssignments) } if len(selection.VSCodeModelAssignments) == 0 && len(s.VSCodeModelAssignments) > 0 { selection.VSCodeModelAssignments = stateModelAssignmentsToModel(s.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/app/app.go` around lines 657 - 666, The ModelAssignments conversion in app.go is duplicating the same state-to-model mapping already handled by stateModelAssignmentsToModel. Update the selection population logic in app.go to reuse stateModelAssignmentsToModel for ModelAssignments as well, using the existing helper and the ModelAssignments/VSCodeModelAssignments fields in the relevant app state and selection structures to keep both conversion paths consistent.
678-686: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant gating condition.
hasAssignmentSignalalready captures "at least one map is non-nil". Since a nil map always haslen == 0, wheneverhasAssignmentSignalis false all thelen(...) == 0checks are trivially true. The long conjunction on line 686 can be simplified toif !hasAssignmentSignal { return }.♻️ Proposed simplification
- hasAssignmentSignal := selection.ClaudeModelAssignments != nil || - selection.ClaudePhaseAssignments != nil || - selection.KiroModelAssignments != nil || - selection.ModelAssignments != nil || - selection.VSCodeModelAssignments != nil || - selection.CodexModelAssignments != nil || - selection.CodexCarrilModelAssignments != nil || - selection.CodexPhaseModelAssignments != nil - if len(selection.ClaudeModelAssignments) == 0 && len(selection.ClaudePhaseAssignments) == 0 && len(selection.KiroModelAssignments) == 0 && len(selection.ModelAssignments) == 0 && len(selection.VSCodeModelAssignments) == 0 && len(selection.CodexModelAssignments) == 0 && len(selection.CodexCarrilModelAssignments) == 0 && len(selection.CodexPhaseModelAssignments) == 0 && !hasAssignmentSignal { + hasAssignmentSignal := selection.ClaudeModelAssignments != nil || + selection.ClaudePhaseAssignments != nil || + selection.KiroModelAssignments != nil || + selection.ModelAssignments != nil || + selection.VSCodeModelAssignments != nil || + selection.CodexModelAssignments != nil || + selection.CodexCarrilModelAssignments != nil || + selection.CodexPhaseModelAssignments != nil + if !hasAssignmentSignal { return }🤖 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 678 - 686, The conditional in app selection handling is redundant because hasAssignmentSignal already covers whether any assignment map is non-nil. Update the logic around hasAssignmentSignal in the selection checks so the long len(...) conjunction is removed and the guard simply uses hasAssignmentSignal to decide whether to proceed, keeping the behavior in the same app.go selection path.
♻️ Duplicate comments (8)
internal/components/communitytool/codegraph_guidance.go (2)
217-226: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFiles are recorded even when the write reported no change.
result.Filesis appended wheneverfile != "", regardless ofchanged. Once any later agent in the loop flipsresult.Changedto true, earlier unchanged files are misreported as modified in the aggregate result. Same issue as raised previously; still present.🩹 Proposed fix
file, changed, err := injectCodeGraphGuidanceForAgent(homeDir, adapter) if err != nil { return result, fmt.Errorf("inject CodeGraph guidance for %s: %w", id, err) } if file == "" { continue } result.Changed = result.Changed || changed - result.Files = append(result.Files, file) + if changed { + result.Files = append(result.Files, file) + } }🤖 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/communitytool/codegraph_guidance.go` around lines 217 - 226, The aggregation in the loop calling injectCodeGraphGuidanceForAgent is still appending files even when changed is false, which can make unchanged guidance files look modified once result.Changed becomes true later. Update the result.Files append logic so only files from successful writes with changed == true are recorded, while preserving the existing error handling and result.Changed update in codegraph_guidance.go.
145-156: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUnscoped fallback defeats agent-scoping in
HasConfiguredCodeGraphForAgents.The loop correctly scopes to
agentIDs, but the fallbackreturn hasDetectedCodeGraphToolWiring(homeDir)is not filtered byagentIDs, so CodeGraph wiring for an unrelated installed agent can still trigger guidance writes/backups for the requested agents. This is the same issue raised on a prior commit and remains unresolved.🩹 Proposed fix
for _, agent := range status.Agents { if slices.Contains(agentIDs, agent.Agent) && agent.Detected && agent.Configured { return true } } - return hasDetectedCodeGraphToolWiring(homeDir) + return false }🤖 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/communitytool/codegraph_guidance.go` around lines 145 - 156, `HasConfiguredCodeGraphForAgents` is still returning an unscoped fallback via `hasDetectedCodeGraphToolWiring(homeDir)`, which can ignore the requested `agentIDs`. Update the fallback logic so it only returns true when the detected CodeGraph wiring belongs to one of the requested agents, and keep the existing `status.Agents` loop using `slices.Contains(agentIDs, agent.Agent)` as the scoped check. If needed, pass `agentIDs` into `hasDetectedCodeGraphToolWiring` or replace it with an agent-aware helper so unrelated agent wiring cannot trigger a true result.internal/cli/sync.go (2)
1041-1047: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEmpty
VSCodeModelAssignmentsmap is treated as "missing" and gets overwritten by persisted state.Same bug shape as previously flagged for this block (originally at lines 992-998):
len(selection.VSCodeModelAssignments) == 0can't distinguish "caller passed nothing" from "caller explicitly reset to{}", so an intentional reset silently reloads stale persisted assignments.🩹 Proposed fix
- if len(selection.VSCodeModelAssignments) == 0 && len(persistedState.VSCodeModelAssignments) > 0 { + if selection.VSCodeModelAssignments == nil && len(persistedState.VSCodeModelAssignments) > 0 { m := make(map[string]model.ModelAssignment, len(persistedState.VSCodeModelAssignments)) for k, v := range persistedState.VSCodeModelAssignments { m[k] = model.ModelAssignment{ProviderID: v.ProviderID, ModelID: v.ModelID, Effort: v.Effort} } selection.VSCodeModelAssignments = m }🤖 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 1041 - 1047, The VSCodeModelAssignments merge logic in the sync flow is treating an explicitly empty map as absent, so a caller reset to {} gets overwritten by persisted data. Update the conditional around the selection/persisted state merge to distinguish nil from an empty-but-intentional map, and only copy persisted assignments when the caller truly omitted the field. Keep the fix in the same VSCodeModelAssignments handling block so the behavior matches the earlier corrected merge logic.
412-412: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBackup snapshot targets still use
selection.Agentswhile the actual refresh/write step usesr.agentIDs.
stagePlangates the CodeGraph guidance step onr.agentIDs(line 445), butsyncBackupTargets(called withr.selectionat line 412, implemented at lines 466-473) still keys backup-target selection offselection.Agents. If these two agent sets diverge, the sync can write to CodeGraph guidance files that were never snapshotted, so rollback can't restore them. This is unchanged from a prior review finding on this exact split.🩹 Proposed fix
- targets := syncBackupTargets(r.homeDir, r.workspaceDir, r.selection, adapters) + targets := syncBackupTargets(r.homeDir, r.workspaceDir, r.selection, r.agentIDs, adapters) ... -func syncBackupTargets(homeDir, workspaceDir string, selection model.Selection, adapters []agents.Adapter) []string { +func syncBackupTargets(homeDir, workspaceDir string, selection model.Selection, agentIDs []model.AgentID, adapters []agents.Adapter) []string { paths := map[string]struct{}{} for _, component := range selection.Components { for _, path := range syncComponentPathsWithWorkspace(homeDir, workspaceDir, selection, adapters, component) { paths[path] = struct{}{} } } - if shouldRefreshCodeGraphGuidance(homeDir, selection.Agents) { - for _, path := range communitytool.CodeGraphGuidancePathsForAgents(homeDir, selection.Agents) { + if shouldRefreshCodeGraphGuidance(homeDir, agentIDs) { + for _, path := range communitytool.CodeGraphGuidancePathsForAgents(homeDir, agentIDs) { paths[path] = struct{}{} } }Also applies to: 445-459, 466-477
🤖 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` at line 412, Backup snapshot target selection is still based on selection.Agents while the refresh/write path uses r.agentIDs, so they can diverge. Update syncBackupTargets and the stagePlan flow in sync.go to derive CodeGraph guidance backup targets from r.agentIDs (the same set used for guidance refresh/write) instead of r.selection, and make sure the snapshot list and write list are built from the same agent source.internal/cli/run.go (1)
190-192: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExplicit VSCodeModelAssignments clears still lost on
install --agent.
modelAssignmentsToState(line 1763) still collapses a non-nil empty map tonil, somergeExplicitAgentInstallStatetreats an explicit "reset VSCodeModelAssignments" as "no change" and stale assignments survive. This diverges fromapplyOverridesininternal/app/app.go, which honors non-nil empty maps as an explicit clear. This is the same unresolved concern raised previously for this call site.🐛 Proposed fix
+func installModelAssignmentsToState(m map[string]model.ModelAssignment) map[string]state.ModelAssignmentState { + if m == nil { + return nil + } + if len(m) == 0 { + return map[string]state.ModelAssignmentState{} + } + return modelAssignmentsToState(m) +}- VSCodeModelAssignments: modelAssignmentsToState(input.Selection.VSCodeModelAssignments), + VSCodeModelAssignments: installModelAssignmentsToState(input.Selection.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/cli/run.go` around lines 190 - 192, The install state conversion for VSCodeModelAssignments is still dropping an explicit empty map because modelAssignmentsToState collapses non-nil empties to nil, so mergeExplicitAgentInstallState cannot distinguish “clear assignments” from “no change.” Update modelAssignmentsToState and the install path in run.go so a non-nil empty VSCodeModelAssignments is preserved as an explicit clear, matching the behavior already used by applyOverrides in app.go, and make sure the merge logic continues to see that distinction.internal/assets/assets_test.go (1)
610-634: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHidden-agent predicate still duplicated instead of reusing production logic.
isClaudeInternalAgentNamehardcodes thesdd-/jd-prefix rules instead of deriving them from the actual VS Code picker visibility logic. If that logic changes, this test can stay green while picker behavior regresses.🤖 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 610 - 634, The hidden-agent check in TestClaudeInternalAgentAssetsAreHiddenFromVSCodePicker is duplicating prefix rules in isClaudeInternalAgentName instead of using the same production visibility logic. Replace the hardcoded sdd-/jd- predicate with a call into the existing production helper or source of truth used by the VS Code picker so the test tracks the real behavior. Keep the test focused on verifying asset metadata for whatever agents the production logic marks as hidden.internal/components/sdd/vscode_agent_visibility.go (1)
15-52: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRewrites
~/.claude/agents/*.mdbased only on filename, with no ownership check or backup.Any user-authored file matching one of
managedClaudeInternalAgentFiles()(e.g. a hand-writtensdd-apply.mdorjd-judge-a.md) gets its frontmatter mutated in place even if Gentle AI never created it — this is a user-config mutation with no backup/restore path, and this exact concern was already raised in a prior review pass without being resolved in this version of the file.Gate the rewrite on a Gentle-managed marker or install-state record before calling
filemerge.WriteFileAtomic, and route the mutation through a rollback/backup flow so the change stays reversible.As per path instructions, "Install/sync operations must be idempotent (running twice equals running once). Any write to user config must have a backup/restore path."
🤖 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 rewrite in hideManagedClaudeInternalAgentsForVSCode currently targets any matching file from managedClaudeInternalAgentFiles() without proving Gentle ownership, so user-authored agents can be mutated in place. Update the logic to first check a Gentle-managed marker or install-state record before reaching filemerge.WriteFileAtomic, and skip files that are not explicitly managed. When a managed file is changed, route the update through the existing rollback/backup flow so the mutation is reversible and idempotent across repeated syncs.Source: Path instructions
internal/components/sdd/vscode_models_test.go (1)
14-22: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAllowlist test still only checks membership, not exact surface.
This is the same concern raised previously (marked "✅ Addressed" on an earlier commit), but the code shown here still only verifies each expected key is present via
containsString; there is no length or exact-set comparison. IfvscodeModelAssignmentKeys()grows an unexpected extra entry, this test still passes even thoughvscode_models.go'sisVSCodeModelAssignmentKeywould immediately accept another native.agent.mdfile for model assignment.🐛 Proposed fix: assert the exact set
func TestVSCodeModelAssignmentKeysIncludeCoordinatorAndPhases(t *testing.T) { keys := vscodeModelAssignmentKeys() + want := []string{"sdd-orchestrator", "sdd-init", "sdd-explore", "sdd-propose", "sdd-spec", "sdd-design", "sdd-tasks", "sdd-apply", "sdd-verify", "sdd-archive", "sdd-onboard"} + if len(keys) != len(want) { + t.Fatalf("vscodeModelAssignmentKeys() = %v, want exactly %v", keys, want) + } - for _, want := range append([]string{"sdd-orchestrator"}, "sdd-init", "sdd-explore", "sdd-propose", "sdd-spec", "sdd-design", "sdd-tasks", "sdd-apply", "sdd-verify", "sdd-archive", "sdd-onboard") { - if !containsString(keys, want) { - t.Fatalf("vscodeModelAssignmentKeys() missing %q in %v", want, keys) + for _, w := range want { + if !containsString(keys, w) { + t.Fatalf("vscodeModelAssignmentKeys() missing %q in %v", w, keys) } } }🤖 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_models_test.go` around lines 14 - 22, The VSCode model assignment allowlist test only checks that expected keys are present, so unexpected extra keys would still pass; update TestVSCodeModelAssignmentKeysIncludeCoordinatorAndPhases to assert the exact surface returned by vscodeModelAssignmentKeys() instead of just using containsString. Use the existing vscodeModelAssignmentKeys and isVSCodeModelAssignmentKey symbols to compare the full set and verify both membership and no extras.
🤖 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/cli/sync.go`:
- Around line 852-861: The restore paths for ModelAssignments and
VSCodeModelAssignments still duplicate the same map conversion logic instead of
using stateModelAssignmentsToModel. Update those restore branches in
internal/cli/sync.go to call stateModelAssignmentsToModel for the
state.ModelAssignmentState to model.ModelAssignment conversion, keeping the
helper as the single source of truth and removing the repeated loop.
In `@internal/components/communitytool/codegraph_guidance.go`:
- Around line 195-258: Both InjectCodeGraphGuidanceForAgents and
CodeGraphGuidancePathsForAgents repeat the same registry and installed-agent
discovery setup, which risks the two lookups drifting over time. Extract the
shared logic into a small helper such as resolveInstalledCodeGraphAgents that
builds the agents.NewDefaultRegistry result and installedMap from
agents.DiscoverInstalled, then have both functions call it and reuse the
filtered agent resolution with isCodeGraphSupportedAgent and
SupportsSystemPrompt.
In `@internal/components/sdd/inject.go`:
- Around line 959-1009: The removal logic in
removeLegacyOpenCodePlainChatPreflightLines still drops content when a later
legacy fragment has a start marker without a matching end marker. Update the
scan so each detected legacy block is validated independently before removing
anything, rather than only checking the first start/end pair. Keep the fix
localized to removeLegacyOpenCodePlainChatPreflightLines by ensuring every
re-entry into the legacy block has a corresponding terminator before suppressing
lines.
---
Outside diff comments:
In `@internal/app/app.go`:
- Around line 657-666: The ModelAssignments conversion in app.go is duplicating
the same state-to-model mapping already handled by stateModelAssignmentsToModel.
Update the selection population logic in app.go to reuse
stateModelAssignmentsToModel for ModelAssignments as well, using the existing
helper and the ModelAssignments/VSCodeModelAssignments fields in the relevant
app state and selection structures to keep both conversion paths consistent.
- Around line 678-686: The conditional in app selection handling is redundant
because hasAssignmentSignal already covers whether any assignment map is
non-nil. Update the logic around hasAssignmentSignal in the selection checks so
the long len(...) conjunction is removed and the guard simply uses
hasAssignmentSignal to decide whether to proceed, keeping the behavior in the
same app.go selection path.
---
Duplicate comments:
In `@internal/assets/assets_test.go`:
- Around line 610-634: The hidden-agent check in
TestClaudeInternalAgentAssetsAreHiddenFromVSCodePicker is duplicating prefix
rules in isClaudeInternalAgentName instead of using the same production
visibility logic. Replace the hardcoded sdd-/jd- predicate with a call into the
existing production helper or source of truth used by the VS Code picker so the
test tracks the real behavior. Keep the test focused on verifying asset metadata
for whatever agents the production logic marks as hidden.
In `@internal/cli/run.go`:
- Around line 190-192: The install state conversion for VSCodeModelAssignments
is still dropping an explicit empty map because modelAssignmentsToState
collapses non-nil empties to nil, so mergeExplicitAgentInstallState cannot
distinguish “clear assignments” from “no change.” Update modelAssignmentsToState
and the install path in run.go so a non-nil empty VSCodeModelAssignments is
preserved as an explicit clear, matching the behavior already used by
applyOverrides in app.go, and make sure the merge logic continues to see that
distinction.
In `@internal/cli/sync.go`:
- Around line 1041-1047: The VSCodeModelAssignments merge logic in the sync flow
is treating an explicitly empty map as absent, so a caller reset to {} gets
overwritten by persisted data. Update the conditional around the
selection/persisted state merge to distinguish nil from an empty-but-intentional
map, and only copy persisted assignments when the caller truly omitted the
field. Keep the fix in the same VSCodeModelAssignments handling block so the
behavior matches the earlier corrected merge logic.
- Line 412: Backup snapshot target selection is still based on selection.Agents
while the refresh/write path uses r.agentIDs, so they can diverge. Update
syncBackupTargets and the stagePlan flow in sync.go to derive CodeGraph guidance
backup targets from r.agentIDs (the same set used for guidance refresh/write)
instead of r.selection, and make sure the snapshot list and write list are built
from the same agent source.
In `@internal/components/communitytool/codegraph_guidance.go`:
- Around line 217-226: The aggregation in the loop calling
injectCodeGraphGuidanceForAgent is still appending files even when changed is
false, which can make unchanged guidance files look modified once result.Changed
becomes true later. Update the result.Files append logic so only files from
successful writes with changed == true are recorded, while preserving the
existing error handling and result.Changed update in codegraph_guidance.go.
- Around line 145-156: `HasConfiguredCodeGraphForAgents` is still returning an
unscoped fallback via `hasDetectedCodeGraphToolWiring(homeDir)`, which can
ignore the requested `agentIDs`. Update the fallback logic so it only returns
true when the detected CodeGraph wiring belongs to one of the requested agents,
and keep the existing `status.Agents` loop using `slices.Contains(agentIDs,
agent.Agent)` as the scoped check. If needed, pass `agentIDs` into
`hasDetectedCodeGraphToolWiring` or replace it with an agent-aware helper so
unrelated agent wiring cannot trigger a true result.
In `@internal/components/sdd/vscode_agent_visibility.go`:
- Around line 15-52: The rewrite in hideManagedClaudeInternalAgentsForVSCode
currently targets any matching file from managedClaudeInternalAgentFiles()
without proving Gentle ownership, so user-authored agents can be mutated in
place. Update the logic to first check a Gentle-managed marker or install-state
record before reaching filemerge.WriteFileAtomic, and skip files that are not
explicitly managed. When a managed file is changed, route the update through the
existing rollback/backup flow so the mutation is reversible and idempotent
across repeated syncs.
In `@internal/components/sdd/vscode_models_test.go`:
- Around line 14-22: The VSCode model assignment allowlist test only checks that
expected keys are present, so unexpected extra keys would still pass; update
TestVSCodeModelAssignmentKeysIncludeCoordinatorAndPhases to assert the exact
surface returned by vscodeModelAssignmentKeys() instead of just using
containsString. Use the existing vscodeModelAssignmentKeys and
isVSCodeModelAssignmentKey symbols to compare the full set and verify both
membership and no extras.
🪄 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: 7e7d0f73-39eb-4402-a844-4aa8a9f97909
⛔ 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 (39)
README.mddocs/agents.mdgo.modinternal/app/app.gointernal/app/app_test.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/cli/install_test.gointernal/cli/run.gointernal/cli/run_state_test.gointernal/cli/sync.gointernal/cli/sync_test.gointernal/components/communitytool/codegraph_guidance.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
+852
to
+861
| 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.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm stateModelAssignmentsToModel has no other callers in the package.
rg -n 'stateModelAssignmentsToModel' internal/cliRepository: Gentleman-Programming/gentle-ai
Length of output: 304
Use stateModelAssignmentsToModel in the restore paths
stateModelAssignmentsToModel is defined but not called anywhere in internal/cli; the ModelAssignments and VSCodeModelAssignments restore branches still duplicate the same conversion loop. Folding those two sites through the helper keeps the mapping logic in one place.
🤖 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 852 - 861, The restore paths for
ModelAssignments and VSCodeModelAssignments still duplicate the same map
conversion logic instead of using stateModelAssignmentsToModel. Update those
restore branches in internal/cli/sync.go to call stateModelAssignmentsToModel
for the state.ModelAssignmentState to model.ModelAssignment conversion, keeping
the helper as the single source of truth and removing the repeated loop.
Comment on lines
+195
to
+258
| func InjectCodeGraphGuidanceForAgents(homeDir string, agentIDs []model.AgentID) (GuidanceInjectionResult, error) { | ||
| reg, err := agents.NewDefaultRegistry() | ||
| if err != nil { | ||
| return GuidanceInjectionResult{}, err | ||
| } | ||
|
|
||
| installed := agents.DiscoverInstalled(reg, homeDir) | ||
| installedMap := make(map[model.AgentID]struct{}) | ||
| for _, inst := range installed { | ||
| installedMap[inst.ID] = struct{}{} | ||
| } | ||
|
|
||
| result := GuidanceInjectionResult{} | ||
| for _, id := range agentIDs { | ||
| if _, ok := installedMap[id]; !ok { | ||
| continue | ||
| } | ||
| adapter, ok := reg.Get(id) | ||
| if !ok || !isCodeGraphSupportedAgent(id) || !adapter.SupportsSystemPrompt() { | ||
| continue | ||
| } | ||
|
|
||
| file, changed, err := injectCodeGraphGuidanceForAgent(homeDir, adapter) | ||
| if err != nil { | ||
| return result, fmt.Errorf("inject CodeGraph guidance for %s: %w", id, err) | ||
| } | ||
| if file == "" { | ||
| continue | ||
| } | ||
| result.Changed = result.Changed || changed | ||
| result.Files = append(result.Files, file) | ||
| } | ||
|
|
||
| return result, nil | ||
| } | ||
|
|
||
| func CodeGraphGuidancePathsForAgents(homeDir string, agentIDs []model.AgentID) []string { | ||
| reg, err := agents.NewDefaultRegistry() | ||
| if err != nil { | ||
| return nil | ||
| } | ||
|
|
||
| installed := agents.DiscoverInstalled(reg, homeDir) | ||
| installedMap := make(map[model.AgentID]struct{}) | ||
| for _, inst := range installed { | ||
| installedMap[inst.ID] = struct{}{} | ||
| } | ||
|
|
||
| paths := make([]string, 0, len(agentIDs)) | ||
| for _, id := range agentIDs { | ||
| if _, ok := installedMap[id]; !ok { | ||
| continue | ||
| } | ||
| adapter, ok := reg.Get(id) | ||
| if !ok || !isCodeGraphSupportedAgent(id) || !adapter.SupportsSystemPrompt() { | ||
| continue | ||
| } | ||
| path := adapter.SystemPromptFile(homeDir) | ||
| if strings.TrimSpace(path) != "" { | ||
| paths = append(paths, path) | ||
| } | ||
| } | ||
| return paths | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
InjectCodeGraphGuidanceForAgents and CodeGraphGuidancePathsForAgents duplicate registry/installed-map setup.
Both functions independently build reg and installedMap with the same loop before diverging. Consider extracting a small shared helper (e.g. resolveInstalledCodeGraphAgents) to avoid drift between the two lookups.
🤖 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/communitytool/codegraph_guidance.go` around lines 195 -
258, Both InjectCodeGraphGuidanceForAgents and CodeGraphGuidancePathsForAgents
repeat the same registry and installed-agent discovery setup, which risks the
two lookups drifting over time. Extract the shared logic into a small helper
such as resolveInstalledCodeGraphAgents that builds the
agents.NewDefaultRegistry result and installedMap from agents.DiscoverInstalled,
then have both functions call it and reuse the filtered agent resolution with
isCodeGraphSupportedAgent and SupportsSystemPrompt.
Snakeblack
force-pushed
the
feat/vscode-copilot-subagents-pr2
branch
from
8917ac1 to
af6710a
Compare
Snakeblack
force-pushed
the
feat/vscode-copilot-subagents-pr2
branch
from
7a35c51 to
e3eba36
Compare
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
Related context: #677
Depends on: #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 2 of the VS Code Copilot SDD subagents stack.
This PR depends on #708. Because #708 is still open and its branch exists only in the contributor fork, this PR targets
main; once #708 lands, this PR can be rebased/updated and the GitHub diff will collapse to the PR2-only commit.Focused review diff while #708 is open:
Snakeblack/gentle-ai@feat/vscode-copilot-subagents-pr1...feat/vscode-copilot-subagents-pr2
📝 Summary
VSCodeModelAssignmentsseparate from OpenCode, Claude, and Kiro model assignment maps.github-copilotmodel cache data and safely renders optionalmodel:frontmatter for native.agent.mdfiles.📂 Changes
internal/model,internal/stateVSCodeModelAssignmentsto selections, sync overrides, and persisted install stateinternal/app,internal/cliinternal/components/sdd/vscode_models.go.agent.mdfrontmatter renderinginternal/components/sdd/inject.go🧠 Size Exception Rationale
This PR requests
size:exception.The change is larger than the 400-line budget because the behavior is intentionally cohesive: persistence, resolver validation,
.agent.mdrendering, warning propagation, and tests must be reviewed together to understand the safety contract. Splitting these pieces would make the implementation harder to reason about and would hide the important invariant: unresolved assignments must stay persisted but must not render unsafemodel:frontmatter.To reduce reviewer load, the new resolver files include concise behavior comments explaining why each helper exists and how failures degrade safely.
🧪 Test Plan
Focused PR2 tests
Package smoke
go test ./internal/cliStatic checks
Full suite in CI-like Linux runner
env -i HOME=/tmp/opencode/home \ PATH=/tmp/opencode/go/bin:/usr/bin:/bin \ GOCACHE=/tmp/opencode/gocache \ GOMODCACHE=/tmp/opencode/gomodcache \ /tmp/opencode/go/bin/go test ./...Results:
go test ./internal/clipassesgo vet ./...passesgit diff --checkpassesgo test ./...passes in clean WSL Ubuntu LF clone with Go 1.25.10cd e2e && ./docker-test.sh)Note: local Windows
go test ./...still has baseline CRLF/env-sensitive failures unrelated to this PR. The full-suite gate was validated in a clean Linux/LF runner instead.🤖 Automated Checks
size:exception; behavior is cohesive and documentedCloses #504status:approvedstatus:approvedtype:*Labeltype:featurerequested (maintainer label may be needed)go test ./...✅ Contributor Checklist
status:approvedsize:exceptionwith rationale documented — maintainer label needed (size:exception)type:*label to this PR — maintainer label may be needed (type:feature)go test ./...) in Linux/LF runnercd e2e && ./docker-test.sh)Co-Authored-Bytrailers💬 Notes for Reviewers
This is backend/model-assignment foundation only. It deliberately does not include the VS Code install hardening or the TUI/docs updates; those remain split into #738 and #740.
Key safety behavior to review:
.agent.mdfiles are ignored by the runtime assignment allowlist;model:instead of failing sync;Summary by CodeRabbit
model:frontmatter from the Copilot cache, or omits it when unavailable/invalid.