feat(vscode): add TUI model flow and docs#740
Open
Snakeblack wants to merge 23 commits into
Open
Conversation
This was referenced Jun 2, 2026
Snakeblack
force-pushed
the
feat/vscode-copilot-subagents-pr3
branch
from
9aa7eee to
13739b4
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:
📝 WalkthroughWalkthroughAdds VS Code Copilot native sub-agent support, along with assignment persistence, sync warnings, model-picker routing, visibility rewrites, uninstall restoration, and updated agent/documentation assets. ChangesVS Code Copilot SDD Native Support
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/agents.md (1)
16-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistency with README.md: Agent Matrix delegation model descriptor.
Line 16 shows "Full (runSubagent)" while README.md line 37 changed to "Full (native subagents)" for the same agent. These should be consistent across documentation. "runSubagent" is the internal VS Code Copilot API term; "native subagents" is the user-facing feature description. Consider aligning line 16 to match README for clarity.
🤖 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 `@docs/agents.md` at line 16, Change the delegation model descriptor for the VS Code Copilot agent in the agent matrix table from "Full (runSubagent)" to "Full (native subagents)" to match the user-facing terminology used consistently across the documentation. The internal API term runSubagent should be replaced with the clearer native subagents description.internal/cli/run.go (1)
199-234:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
mergeExplicitAgentInstallStateomits VS Code assignment merge branch.The explicit
install --agent ...merge path does not copynewState.VSCodeModelAssignmentsinto merged state. VS Code model assignments built innewStateare therefore not persisted for explicit-agent installs.Suggested fix
if newState.ModelAssignments != nil { merged.ModelAssignments = newState.ModelAssignments } + if newState.VSCodeModelAssignments != nil { + merged.VSCodeModelAssignments = newState.VSCodeModelAssignments + } if newState.ClaudeModelAssignments != nil { merged.ClaudeModelAssignments = newState.ClaudeModelAssignments }🤖 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 199 - 234, The mergeExplicitAgentInstallState function is missing a merge branch for VS Code model assignments. After the existing assignment merge conditionals (ModelAssignments, ClaudeModelAssignments, KiroModelAssignments, CodexModelAssignments, etc.) and before the Persona assignment check, add a new conditional block that checks if newState.VSCodeModelAssignments is not nil and copies it to merged.VSCodeModelAssignments, following the same pattern as the other assignment merges in the function.internal/app/app.go (1)
682-751:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve nil-vs-empty clear semantics for
VSCodeModelAssignmentsin persistence.
persistAssignmentscurrently writes VS Code assignments only for non-empty maps, so an explicit clear (non-nil, empty map) never clearsstate.InstallState.VSCodeModelAssignments. That leaves stale assignments persisted and reloaded on later syncs.Suggested fix
- 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 682 - 751, The VSCodeModelAssignments handling currently only checks if the length is greater than 0 and writes the value, but it does not follow the nil-vs-empty semantics used for other assignment fields like ModelAssignments, ClaudeModelAssignments, etc. Refactor the VSCodeModelAssignments block to first check if selection.VSCodeModelAssignments is not nil, and then if it is non-nil, check the length: if len is greater than 0, write the value via modelAssignmentsToState; if len is 0, explicitly set current.VSCodeModelAssignments to nil to clear stale assignments. This ensures that an explicit empty map clears persisted state instead of leaving stale assignments behind.
🤖 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/assets/vscode/agents/sdd-orchestrator.agent.md`:
- Around line 177-182: Add a language identifier to the fenced code block
containing the dependency graph diagram. Change the opening fence from triple
backticks to specify the language as "mermaid" (i.e., ```mermaid) since this
diagram illustrates a Mermaid-style flowchart showing the proposal -> specs ->
tasks -> apply -> verify -> archive dependency relationship with the design
feedback loop.
In `@internal/cli/sync_test.go`:
- Around line 2092-2119: The test function
TestRunSyncWithSelectionPropagatesVSCodeModelWarnings is missing command
execution stubs that all other RunSyncWithSelection tests include. Add
runCommand and cmdLookPath stubs to this test (similar to those used in other
tests like the ones at lines 1475-1476, 1528-1529, and 1600-1601) to ensure
consistency and prevent test failures if the sync operation attempts to execute
or look up external commands during SDD injection or verification.
In `@internal/components/filemerge/replace_windows.go`:
- Line 5: The import of golang.org/x/sys/windows references a vulnerable version
(v0.38.0 affected by GO-2026-5024). Update the golang.org/x/sys dependency to
v0.45.0 or later in your module dependencies and run the dependency update
command to sync the go.mod and go.sum files with the new version.
In `@internal/components/sdd/vscode_agent_visibility.go`:
- Around line 15-52: The function hideManagedClaudeInternalAgentsForVSCode
modifies files in ~/.claude/agents/ without creating backups, violating the
coding guideline that user config writes must have a backup/restore path. Before
calling filemerge.WriteFileAtomic for each file in the loop (where the updated
content differs from the original text), create a backup of the original file
with a .backup suffix. This ensures users have a recovery mechanism if the write
operation fails or corrupts content.
In `@internal/components/sdd/vscode_models.go`:
- Line 211: The line building the YAML model field uses strconv.Quote to quote
the modelLabel, which produces Go-style quoted strings that are not idiomatic
YAML. Replace the strconv.Quote call on line 211 with a plain YAML-aware
approach: for model labels without special characters, use the modelLabel
directly without quotes, or use a YAML-appropriate quoting mechanism if special
characters are present. This will generate cleaner, more idiomatic YAML output.
- Around line 97-138: The function resolveVSCodeModelAssignment is loading the
model cache from disk via loadVSCodeModelCatalog on every invocation, which
causes redundant disk reads and parsing when the function is called once per
agent in the injection loop. Refactor resolveVSCodeModelAssignment to accept the
pre-loaded cache as a parameter instead of calling loadVSCodeModelCatalog
internally. Then in the Inject function in inject.go, load the cache once before
entering the loop that calls resolveVSCodeModelAssignment, and pass the
pre-loaded cache to each call rather than reloading it each time.
In `@internal/tui/model_test.go`:
- Around line 2470-2474: The TestModelConfig_VSCodeOptionOpensCopilotPicker test
hard-codes m.Cursor = 4 to reference the VS Code Copilot option, which breaks if
the menu order changes. Instead of using the hard-coded index value 4,
dynamically compute the VS Code Copilot option's index by calling
screens.ModelConfigOptions() and finding the position of the VS Code Copilot
entry, following the same resilient pattern already established in the codebase
(as referenced at Line 1889). This ensures the test remains valid regardless of
future changes to the menu order.
- Around line 2553-2555: The current assertion in the ScreenModelPicker block
only checks what the screen should NOT be, which is too weak and doesn't verify
the correct route is taken. Replace this negative assertion with a positive one
that explicitly checks state.Screen equals the exact expected screen for the
Codex + SDD preset non-VSCode flow, or alternatively add a check that if
state.Screen does equal ScreenModelPicker, then verify that
state.ModelPicker.ForVSCode is false to ensure the correct flag is set for this
code path.
---
Outside diff comments:
In `@docs/agents.md`:
- Line 16: Change the delegation model descriptor for the VS Code Copilot agent
in the agent matrix table from "Full (runSubagent)" to "Full (native subagents)"
to match the user-facing terminology used consistently across the documentation.
The internal API term runSubagent should be replaced with the clearer native
subagents description.
In `@internal/app/app.go`:
- Around line 682-751: The VSCodeModelAssignments handling currently only checks
if the length is greater than 0 and writes the value, but it does not follow the
nil-vs-empty semantics used for other assignment fields like ModelAssignments,
ClaudeModelAssignments, etc. Refactor the VSCodeModelAssignments block to first
check if selection.VSCodeModelAssignments is not nil, and then if it is non-nil,
check the length: if len is greater than 0, write the value via
modelAssignmentsToState; if len is 0, explicitly set
current.VSCodeModelAssignments to nil to clear stale assignments. This ensures
that an explicit empty map clears persisted state instead of leaving stale
assignments behind.
In `@internal/cli/run.go`:
- Around line 199-234: The mergeExplicitAgentInstallState function is missing a
merge branch for VS Code model assignments. After the existing assignment merge
conditionals (ModelAssignments, ClaudeModelAssignments, KiroModelAssignments,
CodexModelAssignments, etc.) and before the Persona assignment check, add a new
conditional block that checks if newState.VSCodeModelAssignments is not nil and
copies it to merged.VSCodeModelAssignments, following the same pattern as the
other assignment merges in the function.
🪄 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: 9f4633a5-2db1-468b-a79c-bf4703b68673
⛔ 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 (59)
README.mddocs/agents.mddocs/intended-usage.mddocs/platforms.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.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/screens/model_config.gointernal/tui/screens/model_config_test.gointernal/tui/screens/model_picker.gointernal/tui/screens/vscode_model_picker_test.go
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.
Missing test environment stubs.
All other RunSyncWithSelection tests in this file set runCommand and cmdLookPath stubs (e.g., lines 1475-1476, 1528-1529, 1600-1601), even when the sync operation may not invoke external commands. This test uses t.Setenv for home directory discovery but omits the command stubs, creating an inconsistency that could cause test failures if the sync runtime attempts to execute or look up external commands during SDD injection or verification.
🔧 Add missing test stubs
func TestRunSyncWithSelectionPropagatesVSCodeModelWarnings(t *testing.T) {
home := t.TempDir()
+ restoreCommand := runCommand
+ restoreLookPath := cmdLookPath
+ t.Cleanup(func() {
+ runCommand = restoreCommand
+ cmdLookPath = restoreLookPath
+ })
+ runCommand = func(string, ...string) error { return nil }
+ cmdLookPath = func(name string) (string, error) { return "/usr/local/bin/" + name, nil }
+
t.Setenv("HOME", home)
t.Setenv("USERPROFILE", home)📝 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
| 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) | |
| } | |
| } | |
| func TestRunSyncWithSelectionPropagatesVSCodeModelWarnings(t *testing.T) { | |
| home := t.TempDir() | |
| restoreCommand := runCommand | |
| restoreLookPath := cmdLookPath | |
| t.Cleanup(func() { | |
| runCommand = restoreCommand | |
| cmdLookPath = restoreLookPath | |
| }) | |
| runCommand = func(string, ...string) error { return nil } | |
| cmdLookPath = func(name string) (string, error) { return "/usr/local/bin/" + name, nil } | |
| 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) | |
| } | |
| } |
🤖 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 test function
TestRunSyncWithSelectionPropagatesVSCodeModelWarnings is missing command
execution stubs that all other RunSyncWithSelection tests include. Add
runCommand and cmdLookPath stubs to this test (similar to those used in other
tests like the ones at lines 1475-1476, 1528-1529, and 1600-1601) to ensure
consistency and prevent test failures if the sync operation attempts to execute
or look up external commands during SDD injection or verification.
|
|
||
| package filemerge | ||
|
|
||
| import "golang.org/x/sys/windows" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest patched version of golang.org/x/sys that fixes GO-2026-5024 (NewNTUnicodeString integer overflow)?
💡 Result:
The latest patched version of golang.org/x/sys that fixes the GO-2026-5024 (also tracked as CVE-2026-39824) integer overflow vulnerability is v0.45.0 [1][2][3]. The Go Security Team officially announced this fix on May 21, 2026, noting that version v0.45.0 resolves the issue where the NewNTUnicodeString function failed to correctly check for string length overflows [3]. As of June 14, 2026, later versions of golang.org/x/sys, such as v0.46.0, also contain this fix [4]. Users should update to v0.45.0 or any subsequent release to ensure protection against this vulnerability [1][3].
Citations:
- 1: https://osv.dev/vulnerability/GO-2026-5024
- 2: https://pkg.go.dev/vuln/GO-2026-5024
- 3: https://groups.google.com/g/golang-announce/c/6MMI8Lj-Atg
- 4: https://pkg.go.dev/golang.org/x/sys@v0.32.0
🏁 Script executed:
#!/bin/bash
# Check if UTF16PtrFromString or MoveFileEx code paths invoke NewNTUnicodeString
rg -n "NewNTUnicodeString" --type goRepository: Gentleman-Programming/gentle-ai
Length of output: 57
🏁 Script executed:
# Check the golang.org/x/sys version in go.mod
cat go.mod | grep -A 2 "golang.org/x/sys"Repository: Gentleman-Programming/gentle-ai
Length of output: 104
🏁 Script executed:
# Read the replace_windows.go file to see actual usage
cat -n internal/components/filemerge/replace_windows.goRepository: Gentleman-Programming/gentle-ai
Length of output: 714
🏁 Script executed:
# Search for what golang.org/x/sys package documentation says about UTF16PtrFromString
rg -A 10 "UTF16PtrFromString|MoveFileEx" --type goRepository: Gentleman-Programming/gentle-ai
Length of output: 1545
🌐 Web query:
golang.org/x/sys windows.UTF16PtrFromString NewNTUnicodeString GO-2026-5024 vulnerability affected
💡 Result:
The vulnerability GO-2026-5024 (also tracked as CVE-2026-39824) affects the NewNTUnicodeString function within the golang.org/x/sys/windows package [1][2][3]. The issue is an integer overflow caused by the function failing to validate the input string length against the maximum capacity of a NTUnicodeString, which is limited by a 16-bit number of bytes [1][4]. Previously, if provided with a string exceeding this length, the function would return a silently truncated string instead of an error, potentially leading to data integrity issues or unexpected program logic [1][4]. The vulnerability was addressed in version v0.45.0 of golang.org/x/sys [3]. Although some security advisories initially mentioned v0.44.0 as the fix [2][4], the official Go Security Team announcement confirmed that v0.45.0 was tagged specifically to resolve this issue [3]. To remediate this vulnerability, users should update their golang.org/x/sys dependency to v0.45.0 or later [3]. It is important to note that NewNTUnicodeString is not widely used in typical Go Windows applications; the standard library's primary use of similar functionality is in specific symlink handling, which developers have noted is generally not exploitable due to Windows path length limitations [5]. Despite this, users are advised to update to ensure safe handling of system strings [3].
Citations:
- 1: https://pkg.go.dev/vuln/GO-2026-5024
- 2: https://osv.dev/vulnerability/GO-2026-5024
- 3: https://groups.google.com/g/golang-announce/c/6MMI8Lj-Atg
- 4: https://www.resolvedsecurity.com/vulnerability-catalog/CVE-2026-39824
- 5: x/sys/windows, internal/syscall/windows: integer overflow in NewNTUnicodeString golang/go#78916
🌐 Web query:
golang.org/x/sys v0.38.0 UTF16PtrFromString implementation details
💡 Result:
In the golang.org/x/sys/windows package (including version v0.38.0), the UTF16PtrFromString function is implemented by delegating the conversion logic to UTF16FromString and returning a pointer to the first element of the resulting UTF-16 slice [1]. The implementation follows these steps: 1. It calls UTF16FromString(s) to convert the input UTF-8 string into a null-terminated []uint16 slice [1][2]. 2. If UTF16FromString returns an error—specifically if the input string contains a NUL byte—UTF16PtrFromString propagates that error [1][2]. 3. If successful, it returns the address of the first element of the slice (&a), providing a pointer compatible with Windows APIs that expect a *uint16 (or LPCWSTR) [1][2]. UTF16FromString itself handles the conversion by checking for NUL bytes, pre-allocating a buffer, and using an internal encodeWTF16 function to process the UTF-8 string, ensuring that unpaired surrogates are encoded using WTF-8 [2][3]. It concludes by appending a terminating zero (NUL) to the slice [2]. Note that this function is the modern replacement for the deprecated StringToUTF16Ptr, which would panic rather than return an error if a NUL byte was encountered [1][2][3]. Modern implementations of windows.UTF16PtrFromString in the x/sys package are designed to forward to the standard library's syscall package implementation, ensuring consistency and benefiting from shared improvements and bug fixes [4].
Citations:
- 1: https://github.com/golang/sys/blob/master/windows/syscall_windows.go
- 2: https://go.dev/src/syscall/syscall_windows.go?m=text
- 3: https://github.com/golang/go/blob/go1.21.5/src/syscall/syscall_windows.go
- 4: https://android.googlesource.com/platform/external/golang-x-sys/+/a6f4650873b0647f4a7562d1bc45c27d4da686d7
Update golang.org/x/sys to v0.45.0 or later for security patching.
The codebase uses golang.org/x/sys v0.38.0, which is affected by GO-2026-5024. While the vulnerability is specific to the NewNTUnicodeString function (not the UTF16PtrFromString and MoveFileEx functions used here), updating to v0.45.0 or later is recommended as a general security best practice to ensure all dependencies are current with patches.
🤖 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/filemerge/replace_windows.go` at line 5, The import of
golang.org/x/sys/windows references a vulnerable version (v0.38.0 affected by
GO-2026-5024). Update the golang.org/x/sys dependency to v0.45.0 or later in
your module dependencies and run the dependency update command to sync the
go.mod and go.sum files with the new version.
13739b4 to
b699fa9
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/components/sdd/inject.go (1)
651-703: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAdd backup/restore for the new VS Code user-config writes.
These VS Code branches now overwrite
~/.copilot/agents/*.agent.mdandgentle-ai.instructions.md, but there is still no backup/restore path for those user-owned files. If a user already customized either location, this sync can destroy prior content with no way to restore it later. 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.Also applies to: 1704-1738, 1741-1752
🤖 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 651 - 703, The new VS Code user-config writes in inject.go can overwrite user-owned files without any restore path, so add backup/restore handling around the write flow for both the .agent.md sync and gentle-ai.instructions.md updates. Update the relevant install/sync logic in internal/components/sdd/inject.go and any shared write helpers it uses so existing files are backed up before replacement and can be restored on failure or uninstall, keeping the operation idempotent and safe for repeated runs.Source: Path instructions
🤖 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_state_test.go`:
- Around line 34-36: Update the VS Code model-assignment fixtures in the run
state tests to use the canonical provider ID and assert it explicitly. In the
affected test cases around the VSCodeModelAssignments setup and related
assertions, replace the noncanonical ProviderID value with the expected VS Code
provider identifier used elsewhere in persisted-state coverage, and extend the
checks in the relevant test functions to verify both ProviderID and ModelID for
the assignment entries. This tightening should be applied consistently in the
run_state_test coverage so a merge that changes ProviderID cannot slip through.
In `@internal/components/filemerge/writer.go`:
- Around line 96-97: The switch to replaceFileAtomic in WriteFileAtomic now
allows overwriting existing files without any restore safety, which is unsafe
for user-owned config written through downstream callers like inject.go. Update
the WriteFileAtomic flow to either require a backup/restore contract before
calling replaceFileAtomic, or limit this overwrite path to app-owned files only;
use the existing managed-file patterns from vscode_agent_visibility.go and keep
install/sync behavior idempotent.
In `@internal/components/sdd/vscode_agent_visibility.go`:
- Around line 43-47: The backup in the managed Claude agent write path is being
overwritten on every update, which breaks the original backup/restore contract.
In the logic around os.WriteFile for backupPath inside the visibility/injection
flow, change it so the backup is created only if it does not already exist,
preserving the first user content snapshot for later restore. Keep the fix
localized to the backup creation step used by the managed file write path.
In `@internal/components/sdd/vscode_models.go`:
- Around line 153-159: The cache-read error handling in opencode.LoadModels
currently formats the underlying error with %v, which prevents callers from
using errors.Is/As. Update the error returned from the LoadModels path in the
models cache read flow to wrap err with %w instead, while keeping the existing
context message and the special os.ErrNotExist branch unchanged.
In `@internal/components/uninstall/service.go`:
- Around line 1230-1235: In the uninstall flow, the apply callback is always
returning changed=true after calling
RestoreManagedClaudeInternalAgentsForVSCode, even when no managed .backup files
were restored. Update the logic around
RestoreManagedClaudeInternalAgentsForVSCode in service.go so it can tell whether
any backups were actually restored, and return changed=true only in that case.
If nothing was restored (for example on a second uninstall or after manual
cleanup), return changed=false while keeping the existing error handling intact.
In `@internal/tui/model.go`:
- Around line 1940-1944: The Codex completion path in model.go still bypasses
the VS Code picker when model.AgentCodex is combined with
model.AgentVSCodeCopilot. Update the Codex handling logic (around the Codex
completion branch and ScreenPreset flow) to perform the same VS Code handoff
used by shouldShowVSCodeModelPickerScreen/initVSCodeModelPicker before
proceeding to SDD, StrictTDD, or DependencyTree. Add a regression test covering
the Codex + VS Code Copilot combination to verify it reaches ScreenModelPicker
instead of skipping straight ahead.
- Around line 3929-3931: `initVSCodeModelPicker()` rebuilds the picker without
restoring persisted VS Code assignments, so the picker can show defaults and
overwrite existing selections. Fix this by hydrating
`m.Selection.VSCodeModelAssignments` before opening the picker, either in
`NewModel` alongside `Selection.ModelAssignments` or as a fallback inside
`initVSCodeModelPicker()`. Keep the behavior consistent with the Model Config
initialization path so `screens.NewVSCodeModelPickerState` always sees the saved
VS Code assignment state.
---
Outside diff comments:
In `@internal/components/sdd/inject.go`:
- Around line 651-703: The new VS Code user-config writes in inject.go can
overwrite user-owned files without any restore path, so add backup/restore
handling around the write flow for both the .agent.md sync and
gentle-ai.instructions.md updates. Update the relevant install/sync logic in
internal/components/sdd/inject.go and any shared write helpers it uses so
existing files are backed up before replacement and can be restored on failure
or uninstall, keeping the operation idempotent and safe for repeated runs.
🪄 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: 65284882-e8fd-43f8-98d4-e78e985508de
⛔ 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 (47)
README.mddocs/agents.mddocs/intended-usage.mddocs/platforms.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/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_agent_visibility_test.gointernal/components/sdd/vscode_models.gointernal/components/sdd/vscode_models_test.gointernal/components/uninstall/service.gointernal/model/selection.gointernal/model/selection_test.gointernal/state/state.gointernal/state/state_test.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/screens/model_config.gointernal/tui/screens/model_config_test.gointernal/tui/screens/model_picker.gointernal/tui/screens/vscode_model_picker_test.go
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
internal/tui/model_test.go (1)
5808-5810: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the picker is the VS Code picker, not just
ScreenModelPicker.
ScreenModelPickeris shared, so this can pass even if the Codex confirmation routes to the wrong picker mode. Add astate.ModelPicker.ForVSCodeassertion.💚 Proposed test hardening
if state.Screen != ScreenModelPicker { t.Fatalf("screen = %v, want ScreenModelPicker (VS Code model picker) after Codex model picker confirmation", state.Screen) } + if !state.ModelPicker.ForVSCode { + t.Fatalf("ModelPicker.ForVSCode = false, want true after Codex model picker confirmation") + }🤖 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/tui/model_test.go` around lines 5808 - 5810, The test in the model picker flow is too generic because ScreenModelPicker is shared, so it can still pass even if Codex confirmation routes to the wrong picker mode. Update the assertion in the relevant test around state.Screen to also check state.ModelPicker.ForVSCode, using the existing model picker state fields in internal/tui/model_test.go, so the confirmation is verified specifically as the VS Code picker.
🤖 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/components/sdd/inject.go`:
- Around line 2301-2314: Make writeFileWithBackup fail closed and create the
backup atomically. In writeFileWithBackup, only proceed with the normal write
when the target file truly exists, and treat any unexpected error from
os.Stat(path) or os.Stat(backupPath) as a hard failure instead of falling
through. Replace the direct os.WriteFile backup creation with an atomic
temp-file-and-rename flow so a partial .backup cannot be left behind; use the
existing write path in filemerge.WriteFileAtomic as the model and keep the
backup/restore behavior tied to writeFileWithBackup.
In `@internal/components/sdd/vscode_agent_visibility.go`:
- Around line 71-79: The restore path in vscode_agent_visibility.go ignores
cleanup failures after a successful write, so a stale backup can remain and
break idempotent restores. In the backup restore block that uses os.ReadFile,
filemerge.WriteFileAtomic, and os.Remove, capture and handle the
os.Remove(backupPath) error instead of discarding it, and return a wrapped error
from the restore flow so callers know cleanup failed. Keep the existing
read/write behavior in place, but make the full backup restore path fail when
the backup cannot be removed.
- Around line 43-48: The backup check in vscode_agent_visibility.go only handles
the missing-file case, so permission or I/O errors can fall through and still
allow the user config write. Update the backup logic in the managed Claude agent
write path to fail closed whenever os.Stat on backupPath returns anything other
than os.ErrNotExist, and return an error before proceeding. Make sure the backup
creation in the write flow is atomic and that the InjectionResult path only
continues after a successful backup, using the existing symbols backupPath,
os.Stat, os.WriteFile, and the managed Claude agent write logic.
In `@internal/components/uninstall/service.go`:
- Around line 1253-1277: After a successful backup restore in the uninstall
flow, the next run should not fall through to deleting the restored user file;
update the restore/delete logic in the uninstall service’s file-handling path so
that once a `.backup` is restored and removed, subsequent uninstall passes are a
no-op for that user-owned target unless it is explicitly confirmed to still be
managed/generated content. Use the existing restore branch and the
`removeFileIfExists`/no-backup branch to make the operation idempotent and avoid
deleting restored configs on a second uninstall.
---
Duplicate comments:
In `@internal/tui/model_test.go`:
- Around line 5808-5810: The test in the model picker flow is too generic
because ScreenModelPicker is shared, so it can still pass even if Codex
confirmation routes to the wrong picker mode. Update the assertion in the
relevant test around state.Screen to also check state.ModelPicker.ForVSCode,
using the existing model picker state fields in internal/tui/model_test.go, so
the confirmation is verified specifically as the VS Code picker.
🪄 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: c52fc49a-aef1-4285-9979-3bd7c30a85bf
📒 Files selected for processing (9)
internal/cli/run_state_test.gointernal/components/sdd/inject.gointernal/components/sdd/vscode_agent_visibility.gointernal/components/sdd/vscode_agent_visibility_test.gointernal/components/sdd/vscode_models.gointernal/components/uninstall/service.gointernal/components/uninstall/service_test.gointernal/tui/model.gointernal/tui/model_test.go
Snakeblack
force-pushed
the
feat/vscode-copilot-subagents-pr3
branch
from
3a7b291 to
878e8ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 17
♻️ Duplicate comments (1)
internal/components/filemerge/writer.go (1)
96-97: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDon't make destructive replacement the default for all component writes.
Line 96 now turns every
WriteFileAtomicoverwrite into a replace-on-existing-file path, but the suppliedinternal/components/sdd/inject.gocontext still routes user-home prompt, command, skill, and agent writes through this helper without a general backup/restore contract. That means installs/syncs can now clobber user-edited config on Windows where they previously failed safely. Keep replacement opt-in per managed caller, or require a backup-backed path before using it. 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/filemerge/writer.go` around lines 96 - 97, The overwrite path in WriteFileAtomic is now making destructive replacement the default, which can clobber user-edited config for callers like inject.go that do not have a backup/restore contract. Update replaceFileAtomic usage in writer.go so replacement is only used by explicitly opt-in managed callers, or gate it behind a backup-backed flow before replacing existing files. Keep the behavior idempotent for install/sync paths by preserving the safe failure/restore semantics for user config writes.Source: Path instructions
🤖 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 v0.44.0 or later
to address the Windows-only golang.org/x/sys/windows import issue. Locate the
existing golang.org/x/sys requirement in go.mod and bump that module version to
the patched release, then make sure any resulting dependency metadata remains
consistent.
In `@internal/assets/vscode/agents/sdd-archive.agent.md`:
- Around line 9-39: The archive agent’s declared tool set is missing the
capabilities required by its own contract. Update the frontmatter for
sdd-archive.agent.md so the available tools include the Engram/read-write
actions needed to complete the archive flow end-to-end, specifically the
observation lookup and persistence steps used by the archive phase. Keep the
instruction set aligned with the archive workflow in the agent body so the
declared tools support the required mem_search/mem_get_observation, mem_save,
and move/archive operations.
In `@internal/assets/vscode/agents/sdd-explore.agent.md`:
- Line 9: The SDD exploration agent currently requires a mem_save step even
though its frontmatter tools list only includes read, search, and web, so update
the prompt in sdd-explore.agent.md to make the save step conditional on tool
availability. Use the existing agent instructions around the mandatory save step
and the tools frontmatter as the place to gate the Engram/mem_save action so
sessions without that tool can still proceed without contradiction.
In `@internal/assets/vscode/agents/sdd-init.agent.md`:
- Line 9: The sdd-init agent prompt currently requires mem_save even though the
agent’s declared tools in the tools list do not include it, causing a mismatch
in restricted sessions. Update the prompt text in sdd-init.agent.md so
persistence is only requested when mem_save is actually available, and make the
instruction conditional on the tool being present. Keep the guidance aligned
with the agent’s tool declaration and adjust any related wording in the same
prompt block that assumes mem_save is always callable.
In `@internal/assets/vscode/agents/sdd-propose.agent.md`:
- Around line 9-33: The agent allowlist is missing the Engram MCP tools needed
by the SDD propose flow. Update the `tools:` frontmatter in
`sdd-propose.agent.md` to expose `mem_search`, `mem_get_observation`, and
`mem_save` using the configured Engram server name or a wildcard, so the agent
can read exploration artifacts and persist the proposal directly as required by
the instructions in this file.
In `@internal/cli/run.go`:
- Around line 718-723: The current shouldRefreshWindowsEngram logic is too broad
because it refreshes whenever engramBinaryDirsOnPath(pathEntries, profile.OS)
finds more than one Windows Engram copy, even if cmdLookPath("engram") already
resolved to the intended binary. Update shouldRefreshWindowsEngram in
internal/cli/run.go to compare resolvedPath against the user-managed Engram
location and only return true when the resolved binary is actually shadowed by
an earlier PATH entry; then keep the install/reorder flow in the Engram install
path from running for the already-correct binary.
In `@internal/components/sdd/inject_test.go`:
- Around line 1873-1876: The agent-map coverage checks in the test helpers are
missing explicit verification for sdd-onboard, so a regression could slip
through while the total count still matches. Update the subAgent lists in the
relevant assertions inside inject_test.go and the other matching test block to
include sdd-onboard alongside the existing sdd-* and jd-* entries, keeping the
count check but ensuring the named agent is asserted directly.
In `@internal/components/sdd/inject.go`:
- Around line 747-750: The post-check in the inject flow is too permissive
because it only validates extensions for each phase, which can let a stale
filename like sdd-apply.md pass even when the adapter-specific managed agent
file was never created. Update the verification logic in the inject path to
check the adapter’s expected sub-agent filenames explicitly rather than looping
over generic extensions, using the existing phase handling for sdd-apply and
sdd-verify to match the actual file naming scheme for each adapter.
- Around line 2126-2149: The `writeClaudeLazySDDWorkflow` path writes the Claude
shared workflow directly with `filemerge.WriteFileAtomic`, which can overwrite a
user-edited file without any restore path. Update this function to use the same
backup-aware write flow used by the new VS Code config writes, ensuring the
existing `~/.claude/skills/_shared/sdd-orchestrator-workflow.md` is backed up
before replacement and can be restored on failure. Keep the change localized to
`writeClaudeLazySDDWorkflow` and reuse the existing backup/restore helpers or
pattern already present in the config write logic.
In `@internal/components/sdd/vscode_agent_visibility.go`:
- Around line 75-87: The backup check in the restore flow is swallowing
non-ErrNotExist failures, so the managed-file recovery path can silently skip
real filesystem errors. Update the os.Stat(backupPath) handling in the restore
logic to distinguish ErrNotExist from other errors, and return a wrapped error
for any non-ErrNotExist case before proceeding with
ReadFile/WriteFileAtomic/Remove. Use the restore loop around backupPath in
vscode_agent_visibility.go as the place to fix this.
In `@internal/components/sdd/vscode_models.go`:
- Around line 109-123: The cache-wide warning branches in vscode_models.go are
still keyed by agentKey, so dedupWarnings() cannot collapse repeated catalog
failures across VS Code agents. Update the warning text in the catalog-wide
paths handled around catalogErr, catalog == nil, missing
githubCopilotProviderID, and missing assignment.ModelID to be agent-agnostic,
while keeping agentKey only for per-agent assignment validation. Use the
existing warning construction in the VS Code model assignment flow so shared
catalog issues produce one dedupable message.
- Around line 218-230: The yamlQuote helper only escapes backslashes and double
quotes, so control characters can still break injected YAML scalars. Update
yamlQuote to either reject or safely escape control characters such as newlines,
carriage returns, and tabs before returning the quoted value. Keep the fix
localized to yamlQuote in vscode_models.go so any model label coming from the
dynamic cache is serialized as a literal single-line YAML value.
In `@internal/components/uninstall/service.go`:
- Around line 1280-1285: The managed-file detection in the uninstall flow is too
narrow and misses the shipped VS Code agent file, so a pristine managed
`sdd-tasks.agent.md` can be left behind. Update the fallback in the uninstall
service logic around the `isManaged` check to recognize real managed agent
content as well, using a more reliable marker from the agent asset itself or by
including `sdd-tasks` in the managed पहचान set. Keep the change localized to the
uninstall path that inspects `contentStr` so managed VS Code agents are removed
consistently.
- Around line 1253-1292: The no-backup branch in the uninstall cleanup logic can
still delete files when backup inspection or file reading fails, so tighten the
flow in the restore/remove helper that uses os.Stat, os.ReadFile, and
removeFileIfExists. Treat any backup status other than a definite missing
backup, or any read error on the target path, as a fail-closed case and return
without deleting; only proceed to removeFileIfExists when you have positively
confirmed the file is managed content.
In `@internal/tui/model.go`:
- Around line 2745-2755: The executeExternalCommand helper currently waits on
cmd.CombinedOutput with no timeout or cancellation, so a community-tool install
can hang the TUI indefinitely. Update executeExternalCommand to accept and honor
a context or timeout, wire that through the command creation path (using
commandFn and cmd.Process signaling as needed), and make sure the caller in
ScreenCommunityToolInstalling can abort cleanly instead of blocking forever.
- Around line 4041-4048: The picker flow in pickerFlowSlice currently appends
ScreenModelPicker for both VS Code Copilot and OpenCode multi-mode, which makes
pickerNextScreen and pickerPreviousScreen ambiguous because they match only by
Screen value. Update the flow to give each picker state a distinct identity in
the flow slice, and adjust the next/previous lookup logic to distinguish the two
model picker states instead of treating them as the same ScreenModelPicker
entry. Use the existing pickerFlowSlice, pickerNextScreen, and
pickerPreviousScreen paths to ensure navigation stays linear and does not
collapse or loop when both pickers are present.
- Around line 2027-2042: The VS Code picker’s Back action is handled by the
m.Cursor == 1 branch before the ModelConfigMode check, so in the empty-state
path Enter on the Back row returns to the previous picker screen instead of
ScreenModelConfig. Update the picker navigation logic in the
ModelPicker/ModelConfigMode flow so the Back row honors ModelConfigMode first,
restoring the model configuration screen when opened from model configuration
with no Copilot models available.
---
Duplicate comments:
In `@internal/components/filemerge/writer.go`:
- Around line 96-97: The overwrite path in WriteFileAtomic is now making
destructive replacement the default, which can clobber user-edited config for
callers like inject.go that do not have a backup/restore contract. Update
replaceFileAtomic usage in writer.go so replacement is only used by explicitly
opt-in managed callers, or gate it behind a backup-backed flow before replacing
existing files. Keep the behavior idempotent for install/sync paths by
preserving the safe failure/restore semantics for user config writes.
🪄 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: 584fe330-c1f6-457a-8657-a7c448b31105
⛔ 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 (63)
README.mddocs/agents.mddocs/intended-usage.mddocs/platforms.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/run_engram_download_test.gointernal/cli/run_state_test.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_agent_visibility_test.gointernal/components/sdd/vscode_models.gointernal/components/sdd/vscode_models_test.gointernal/components/uninstall/service.gointernal/components/uninstall/service_test.gointernal/model/selection.gointernal/model/selection_test.gointernal/state/state.gointernal/state/state_test.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/screens/model_config.gointernal/tui/screens/model_config_test.gointernal/tui/screens/model_picker.gointernal/tui/screens/vscode_model_picker_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: 17
♻️ Duplicate comments (1)
internal/components/filemerge/writer.go (1)
96-97: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDon't make destructive replacement the default for all component writes.
Line 96 now turns every
WriteFileAtomicoverwrite into a replace-on-existing-file path, but the suppliedinternal/components/sdd/inject.gocontext still routes user-home prompt, command, skill, and agent writes through this helper without a general backup/restore contract. That means installs/syncs can now clobber user-edited config on Windows where they previously failed safely. Keep replacement opt-in per managed caller, or require a backup-backed path before using it. 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/filemerge/writer.go` around lines 96 - 97, The overwrite path in WriteFileAtomic is now making destructive replacement the default, which can clobber user-edited config for callers like inject.go that do not have a backup/restore contract. Update replaceFileAtomic usage in writer.go so replacement is only used by explicitly opt-in managed callers, or gate it behind a backup-backed flow before replacing existing files. Keep the behavior idempotent for install/sync paths by preserving the safe failure/restore semantics for user config writes.Source: Path instructions
🤖 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 v0.44.0 or later
to address the Windows-only golang.org/x/sys/windows import issue. Locate the
existing golang.org/x/sys requirement in go.mod and bump that module version to
the patched release, then make sure any resulting dependency metadata remains
consistent.
In `@internal/assets/vscode/agents/sdd-archive.agent.md`:
- Around line 9-39: The archive agent’s declared tool set is missing the
capabilities required by its own contract. Update the frontmatter for
sdd-archive.agent.md so the available tools include the Engram/read-write
actions needed to complete the archive flow end-to-end, specifically the
observation lookup and persistence steps used by the archive phase. Keep the
instruction set aligned with the archive workflow in the agent body so the
declared tools support the required mem_search/mem_get_observation, mem_save,
and move/archive operations.
In `@internal/assets/vscode/agents/sdd-explore.agent.md`:
- Line 9: The SDD exploration agent currently requires a mem_save step even
though its frontmatter tools list only includes read, search, and web, so update
the prompt in sdd-explore.agent.md to make the save step conditional on tool
availability. Use the existing agent instructions around the mandatory save step
and the tools frontmatter as the place to gate the Engram/mem_save action so
sessions without that tool can still proceed without contradiction.
In `@internal/assets/vscode/agents/sdd-init.agent.md`:
- Line 9: The sdd-init agent prompt currently requires mem_save even though the
agent’s declared tools in the tools list do not include it, causing a mismatch
in restricted sessions. Update the prompt text in sdd-init.agent.md so
persistence is only requested when mem_save is actually available, and make the
instruction conditional on the tool being present. Keep the guidance aligned
with the agent’s tool declaration and adjust any related wording in the same
prompt block that assumes mem_save is always callable.
In `@internal/assets/vscode/agents/sdd-propose.agent.md`:
- Around line 9-33: The agent allowlist is missing the Engram MCP tools needed
by the SDD propose flow. Update the `tools:` frontmatter in
`sdd-propose.agent.md` to expose `mem_search`, `mem_get_observation`, and
`mem_save` using the configured Engram server name or a wildcard, so the agent
can read exploration artifacts and persist the proposal directly as required by
the instructions in this file.
In `@internal/cli/run.go`:
- Around line 718-723: The current shouldRefreshWindowsEngram logic is too broad
because it refreshes whenever engramBinaryDirsOnPath(pathEntries, profile.OS)
finds more than one Windows Engram copy, even if cmdLookPath("engram") already
resolved to the intended binary. Update shouldRefreshWindowsEngram in
internal/cli/run.go to compare resolvedPath against the user-managed Engram
location and only return true when the resolved binary is actually shadowed by
an earlier PATH entry; then keep the install/reorder flow in the Engram install
path from running for the already-correct binary.
In `@internal/components/sdd/inject_test.go`:
- Around line 1873-1876: The agent-map coverage checks in the test helpers are
missing explicit verification for sdd-onboard, so a regression could slip
through while the total count still matches. Update the subAgent lists in the
relevant assertions inside inject_test.go and the other matching test block to
include sdd-onboard alongside the existing sdd-* and jd-* entries, keeping the
count check but ensuring the named agent is asserted directly.
In `@internal/components/sdd/inject.go`:
- Around line 747-750: The post-check in the inject flow is too permissive
because it only validates extensions for each phase, which can let a stale
filename like sdd-apply.md pass even when the adapter-specific managed agent
file was never created. Update the verification logic in the inject path to
check the adapter’s expected sub-agent filenames explicitly rather than looping
over generic extensions, using the existing phase handling for sdd-apply and
sdd-verify to match the actual file naming scheme for each adapter.
- Around line 2126-2149: The `writeClaudeLazySDDWorkflow` path writes the Claude
shared workflow directly with `filemerge.WriteFileAtomic`, which can overwrite a
user-edited file without any restore path. Update this function to use the same
backup-aware write flow used by the new VS Code config writes, ensuring the
existing `~/.claude/skills/_shared/sdd-orchestrator-workflow.md` is backed up
before replacement and can be restored on failure. Keep the change localized to
`writeClaudeLazySDDWorkflow` and reuse the existing backup/restore helpers or
pattern already present in the config write logic.
In `@internal/components/sdd/vscode_agent_visibility.go`:
- Around line 75-87: The backup check in the restore flow is swallowing
non-ErrNotExist failures, so the managed-file recovery path can silently skip
real filesystem errors. Update the os.Stat(backupPath) handling in the restore
logic to distinguish ErrNotExist from other errors, and return a wrapped error
for any non-ErrNotExist case before proceeding with
ReadFile/WriteFileAtomic/Remove. Use the restore loop around backupPath in
vscode_agent_visibility.go as the place to fix this.
In `@internal/components/sdd/vscode_models.go`:
- Around line 109-123: The cache-wide warning branches in vscode_models.go are
still keyed by agentKey, so dedupWarnings() cannot collapse repeated catalog
failures across VS Code agents. Update the warning text in the catalog-wide
paths handled around catalogErr, catalog == nil, missing
githubCopilotProviderID, and missing assignment.ModelID to be agent-agnostic,
while keeping agentKey only for per-agent assignment validation. Use the
existing warning construction in the VS Code model assignment flow so shared
catalog issues produce one dedupable message.
- Around line 218-230: The yamlQuote helper only escapes backslashes and double
quotes, so control characters can still break injected YAML scalars. Update
yamlQuote to either reject or safely escape control characters such as newlines,
carriage returns, and tabs before returning the quoted value. Keep the fix
localized to yamlQuote in vscode_models.go so any model label coming from the
dynamic cache is serialized as a literal single-line YAML value.
In `@internal/components/uninstall/service.go`:
- Around line 1280-1285: The managed-file detection in the uninstall flow is too
narrow and misses the shipped VS Code agent file, so a pristine managed
`sdd-tasks.agent.md` can be left behind. Update the fallback in the uninstall
service logic around the `isManaged` check to recognize real managed agent
content as well, using a more reliable marker from the agent asset itself or by
including `sdd-tasks` in the managed पहचान set. Keep the change localized to the
uninstall path that inspects `contentStr` so managed VS Code agents are removed
consistently.
- Around line 1253-1292: The no-backup branch in the uninstall cleanup logic can
still delete files when backup inspection or file reading fails, so tighten the
flow in the restore/remove helper that uses os.Stat, os.ReadFile, and
removeFileIfExists. Treat any backup status other than a definite missing
backup, or any read error on the target path, as a fail-closed case and return
without deleting; only proceed to removeFileIfExists when you have positively
confirmed the file is managed content.
In `@internal/tui/model.go`:
- Around line 2745-2755: The executeExternalCommand helper currently waits on
cmd.CombinedOutput with no timeout or cancellation, so a community-tool install
can hang the TUI indefinitely. Update executeExternalCommand to accept and honor
a context or timeout, wire that through the command creation path (using
commandFn and cmd.Process signaling as needed), and make sure the caller in
ScreenCommunityToolInstalling can abort cleanly instead of blocking forever.
- Around line 4041-4048: The picker flow in pickerFlowSlice currently appends
ScreenModelPicker for both VS Code Copilot and OpenCode multi-mode, which makes
pickerNextScreen and pickerPreviousScreen ambiguous because they match only by
Screen value. Update the flow to give each picker state a distinct identity in
the flow slice, and adjust the next/previous lookup logic to distinguish the two
model picker states instead of treating them as the same ScreenModelPicker
entry. Use the existing pickerFlowSlice, pickerNextScreen, and
pickerPreviousScreen paths to ensure navigation stays linear and does not
collapse or loop when both pickers are present.
- Around line 2027-2042: The VS Code picker’s Back action is handled by the
m.Cursor == 1 branch before the ModelConfigMode check, so in the empty-state
path Enter on the Back row returns to the previous picker screen instead of
ScreenModelConfig. Update the picker navigation logic in the
ModelPicker/ModelConfigMode flow so the Back row honors ModelConfigMode first,
restoring the model configuration screen when opened from model configuration
with no Copilot models available.
---
Duplicate comments:
In `@internal/components/filemerge/writer.go`:
- Around line 96-97: The overwrite path in WriteFileAtomic is now making
destructive replacement the default, which can clobber user-edited config for
callers like inject.go that do not have a backup/restore contract. Update
replaceFileAtomic usage in writer.go so replacement is only used by explicitly
opt-in managed callers, or gate it behind a backup-backed flow before replacing
existing files. Keep the behavior idempotent for install/sync paths by
preserving the safe failure/restore semantics for user config writes.
🪄 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: 584fe330-c1f6-457a-8657-a7c448b31105
⛔ 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 (63)
README.mddocs/agents.mddocs/intended-usage.mddocs/platforms.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/run_engram_download_test.gointernal/cli/run_state_test.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_agent_visibility_test.gointernal/components/sdd/vscode_models.gointernal/components/sdd/vscode_models_test.gointernal/components/uninstall/service.gointernal/components/uninstall/service_test.gointernal/model/selection.gointernal/model/selection_test.gointernal/state/state.gointernal/state/state_test.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/screens/model_config.gointernal/tui/screens/model_config_test.gointernal/tui/screens/model_picker.gointernal/tui/screens/vscode_model_picker_test.go
🛑 Comments failed to post (17)
go.mod (1)
11-11: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf 'Repo root: ' pwd printf '\nRelevant files:\n' git ls-files go.mod go.sum internal/components/filemerge/replace_windows.go || true printf '\n--- go.mod excerpt ---\n' nl -ba go.mod | sed -n '1,80p' printf '\n--- replace_windows.go ---\n' nl -ba internal/components/filemerge/replace_windows.go | sed -n '1,220p' 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: 344
🌐 Web query:
golang.org/x/sys v0.38.0 GO-2026-5024 advisory patched version💡 Result:
The security advisory GO-2026-5024 (also tracked as CVE-2026-39824) affects the golang.org/x/sys/windows package [1][2]. It relates to an integer overflow in the NewNTUnicodeString function, which, when provided with a string exceeding the 16-bit maximum size, would previously truncate the string instead of returning an error [1][3]. To address this vulnerability, you should update your dependency to golang.org/x/sys v0.44.0 or later [1][4]. While there was some initial confusion regarding the specific versioning during the coordinated release, v0.44.0 is the officially documented patched version [1][3]. Updating to this version ensures that the function correctly returns an error when provided with an oversized string [3].
Citations:
- 1: https://pkg.go.dev/vuln/GO-2026-5024
- 2: https://nvd.nist.gov/vuln/detail/cve-2026-39824
- 3: golang/go#78916
- 4: sethcarney/mdm@361b6f5
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' import sys, urllib.request, json, re url='https://pkg.go.dev/vuln/GO-2026-5024' try: with urllib.request.urlopen(url, timeout=10) as r: data=r.read().decode('utf-8','replace') print(data[:5000]) except Exception as e: print("ERR", e) PYRepository: Gentleman-Programming/gentle-ai
Length of output: 307
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' import urllib.request, re url='https://go.dev/security/vuln/GO-2026-5024' try: with urllib.request.urlopen(url, timeout=10) as r: data=r.read().decode('utf-8','replace') print(data[:7000]) except Exception as e: print("ERR", e) PYRepository: Gentleman-Programming/gentle-ai
Length of output: 307
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n--- go.mod excerpt ---\n' sed -n '1,80p' go.mod | cat -n printf '\n--- replace_windows.go ---\n' sed -n '1,220p' internal/components/filemerge/replace_windows.go | cat -n 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: 2940
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path p = Path('go.mod') print(p.read_text()) PYRepository: Gentleman-Programming/gentle-ai
Length of output: 1481
Bump
golang.org/x/systov0.44.0+ingo.mod. The Windows-only path importsgolang.org/x/sys/windows, which is affected by GO-2026-5024.🧰 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 v0.44.0 or later to address the Windows-only golang.org/x/sys/windows import issue. Locate the existing golang.org/x/sys requirement in go.mod and bump that module version to the patched release, then make sure any resulting dependency metadata remains consistent.Source: Linters/SAST tools
internal/assets/vscode/agents/sdd-archive.agent.md (1)
9-39: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the declared tool set with the archive contract.
This agent is told to read Engram observations, persist an archive report with
mem_save, and move the change folder, but the frontmatter only exposesread,search, andedit. As written, the archive phase cannot complete its own instructions reliably.🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 12-12: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🤖 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/vscode/agents/sdd-archive.agent.md` around lines 9 - 39, The archive agent’s declared tool set is missing the capabilities required by its own contract. Update the frontmatter for sdd-archive.agent.md so the available tools include the Engram/read-write actions needed to complete the archive flow end-to-end, specifically the observation lookup and persistence steps used by the archive phase. Keep the instruction set aligned with the archive workflow in the agent body so the declared tools support the required mem_search/mem_get_observation, mem_save, and move/archive operations.internal/assets/vscode/agents/sdd-explore.agent.md (1)
9-9: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the Engram save step conditional on tool availability.
This agent requires
mem_save, but the frontmatter only exposesread,search, andweb. That makes the mandatory save step impossible in sessions where the tool list is enforced.Suggested prompt fix
-## Engram Save (mandatory when tied to a named change) +## Engram Save (when available and tied to a named change) -After completing work, call `mem_save` with: +If the `mem_save` tool is available in this session, call it with: - title: `"sdd/{change-name}/explore"` (or `"sdd/explore/{topic-slug}"` if standalone) - topic_key: `"sdd/{change-name}/explore"` - type: `"architecture"` - project: `{project-name from context}` - capture_prompt: `false` when the Engram tool schema supports it; if an older schema rejects or does not expose the field, omit it rather than failing.Also applies to: 29-36
🤖 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/vscode/agents/sdd-explore.agent.md` at line 9, The SDD exploration agent currently requires a mem_save step even though its frontmatter tools list only includes read, search, and web, so update the prompt in sdd-explore.agent.md to make the save step conditional on tool availability. Use the existing agent instructions around the mandatory save step and the tools frontmatter as the place to gate the Engram/mem_save action so sessions without that tool can still proceed without contradiction.internal/assets/vscode/agents/sdd-init.agent.md (1)
9-9: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't require
mem_savewhen the agent can't call it.This prompt makes Engram persistence mandatory, but the declared tools do not include
mem_save. That turns a successfulsdd-initrun into a contract failure in tool-restricted sessions.Suggested prompt fix
-## Engram Save (mandatory) +## Engram Save (when available) -After completing work, call `mem_save` with: +If the `mem_save` tool is available in this session, call it with: - title: `"sdd-init/{project}"` - topic_key: `"sdd-init/{project}"` - type: `"architecture"` - project: `{project-name from context}` - capture_prompt: `false` when the Engram tool schema supports it; if an older schema rejects or does not expose the field, omit it rather than failing.Also applies to: 26-33
🤖 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/vscode/agents/sdd-init.agent.md` at line 9, The sdd-init agent prompt currently requires mem_save even though the agent’s declared tools in the tools list do not include it, causing a mismatch in restricted sessions. Update the prompt text in sdd-init.agent.md so persistence is only requested when mem_save is actually available, and make the instruction conditional on the tool being present. Keep the guidance aligned with the agent’s tool declaration and adjust any related wording in the same prompt block that assumes mem_save is always callable.internal/assets/vscode/agents/sdd-propose.agent.md (1)
9-33: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In GitHub Copilot VS Code custom agent.agent.mdfiles, does thetools:frontmatter restrict MCP tools, and if so how should MCP tools likemem_searchandmem_savebe declared?💡 Result:
Yes, the
tools:property in the.agent.mdfile's YAML frontmatter does restrict which tools a GitHub Copilot custom agent can access, including those provided by Model Context Protocol (MCP) servers [1][2]. If you omit thetoolsproperty, the agent defaults to having access to all available tools [1][3][4]. If you choose to define it, you must explicitly list the tools you want to make available [1][2]. To declare specific MCP tools, use the formatserver-name/tool-name[1][2]. For example, if your MCP server is namedmem-serverand provides the toolsmem_searchandmem_save, you would declare them in your.agent.mdfrontmatter as follows [1][2]: --- name: my-agent description: An agent with memory search and save capabilities tools: ["mem-server/mem_search", "mem-server/mem_save"] --- Alternatively, you can grant access to all tools from a specific MCP server by using the/*suffix [5][1][2]: --- name: my-agent tools: ["mem-server/*"] --- Note that for these tools to be functional, the corresponding MCP server must be properly configured and available to the agent, either via repository-level settings or by using themcp-serversproperty within the.agent.mdfile itself [6][2][7]. You can also use the "Configure Tools" UI button within VS Code to select available tools and automatically generate the correct syntax in your agent profile [6][7].Citations:
- 1: https://docs.github.com/en/copilot/reference/custom-agents-configuration
- 2: https://github.com/github/docs/blob/main/content/copilot/reference/custom-agents-configuration.md
- 3: https://docs.github.com/en/copilot/concepts/agents/copilot-cli/about-custom-agents
- 4: https://docs.github.com/en/copilot/how-tos/copilot-cli/customize-copilot/create-custom-agents-for-cli
- 5: https://code.visualstudio.com/docs/copilot/customization/custom-agents
- 6: https://docs.github.com/en/copilot/how-tos/use-copilot-agents/cloud-agent/create-custom-agents-in-your-ide?tool=vscode
- 7: https://docs.github.com/en/copilot/how-tos/use-copilot-agents/cloud-agent/create-custom-agents-in-your-ide
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== File ==\n' cat -n internal/assets/vscode/agents/sdd-propose.agent.md printf '\n== Search for memory tool names ==\n' rg -n --hidden --glob '!**/.git/**' 'mem_search|mem_get_observation|mem_save|mcp-servers|tools:' internal/assets .github .vscode .copilot 2>/dev/null || true printf '\n== Agent files with tools frontmatter ==\n' rg -n --hidden --glob '*.agent.md' '^tools:' internal/assets 2>/dev/null || trueRepository: Gentleman-Programming/gentle-ai
Length of output: 50387
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== File ==\n' sed -n '1,120p' internal/assets/vscode/agents/sdd-propose.agent.md | cat -n printf '\n== Search for memory tool names ==\n' rg -n --hidden --glob '!**/.git/**' 'mem_search|mem_get_observation|mem_save|mcp-servers|tools:' internal/assets .github .vscode .copilot 2>/dev/null || true printf '\n== Agent files with tools frontmatter ==\n' rg -n --hidden --glob '*.agent.md' '^tools:' internal/assets 2>/dev/null || trueRepository: Gentleman-Programming/gentle-ai
Length of output: 50387
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n --hidden --glob '!**/.git/**' 'server-name/tool-name|Configure Tools|custom agents|\.agent\.md|mcp-servers|allowed-tools|tools:' internal README.md .github .vscode 2>/dev/null | head -n 200Repository: Gentleman-Programming/gentle-ai
Length of output: 16452
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path import re paths = [Path('internal/assets/vscode/agents/sdd-propose.agent.md'), Path('internal/assets/vscode/agents/sdd-spec.agent.md'), Path('internal/assets/vscode/agents/sdd-apply.agent.md'), Path('internal/assets/vscode/agents/sdd-verify.agent.md'), Path('internal/assets/vscode/agents/sdd-tasks.agent.md'), Path('internal/assets/vscode/agents/sdd-archive.agent.md'), Path('internal/assets/vscode/agents/sdd-init.agent.md'), Path('internal/assets/vscode/agents/sdd-onboard.agent.md'), Path('internal/assets/vscode/agents/sdd-explore.agent.md'), Path('internal/assets/vscode/agents/sdd-design.agent.md'), Path('internal/assets/vscode/agents/sdd-orchestrator.agent.md')] for p in paths: text = p.read_text() m = re.search(r'^tools:\s*\[(.*?)\]\s*$', text, re.M) print(f'{p}: {m.group(1) if m else "NO TOOLS"}') PYRepository: Gentleman-Programming/gentle-ai
Length of output: 982
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== sdd inject tests around tools frontmatter ==\n' sed -n '5960,6075p' internal/components/sdd/inject_test.go | cat -n printf '\n== sdd inject code around phase file rendering ==\n' sed -n '4700,5075p' internal/components/sdd/inject_test.go | cat -n printf '\n== search for tools frontmatter handling in injector ==\n' rg -n 'tools:|allowed-tools|mcp__|`@engram`|read, search, edit|enable_mcp_tools|\.agent\.md' internal/components/sdd internal/assets | head -n 200Repository: Gentleman-Programming/gentle-ai
Length of output: 36770
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== relevant injector source slices ==\n' sed -n '1,220p' internal/components/sdd/vscode_models.go | cat -n printf '\n---\n' sed -n '730,840p' internal/components/sdd/inject.go | cat -n printf '\n---\n' sed -n '1900,1965p' internal/components/sdd/inject.go | cat -nRepository: Gentleman-Programming/gentle-ai
Length of output: 17828
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path for p in [ Path('internal/components/sdd/inject.go'), Path('internal/components/sdd/vscode_models.go'), ]: print(f'== {p} ==') text = p.read_text().splitlines() for i, line in enumerate(text, 1): if 'tools' in line or 'agent.md' in line or 'mcp' in line or 'engram' in line or 'vscode' in line: print(f'{i}: {line}') PYRepository: Gentleman-Programming/gentle-ai
Length of output: 2115
Add the Engram MCP tools to this agent allowlist The
tools:frontmatter scopes what this VS Code agent can call, somem_search,mem_get_observation, andmem_saveneed to be exposed here with the configured Engram server name (or a server wildcard).🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 12-12: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🤖 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/vscode/agents/sdd-propose.agent.md` around lines 9 - 33, The agent allowlist is missing the Engram MCP tools needed by the SDD propose flow. Update the `tools:` frontmatter in `sdd-propose.agent.md` to expose `mem_search`, `mem_get_observation`, and `mem_save` using the configured Engram server name or a wildcard, so the agent can read exploration artifacts and persist the proposal directly as required by the instructions in this file.internal/cli/run.go (1)
718-723: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Only repair Windows Engram when the resolved binary is actually shadowed.
shouldRefreshWindowsEngramreturns true for any PATH containing twoengram.execopies, so the install path below re-downloads Engram and reorders user PATH even whencmdLookPath("engram")already resolved to the correct user-managed binary. That breaks repeat-install idempotency and churns user config for a non-problem.Suggested direction
func shouldRefreshWindowsEngram(profile system.PlatformProfile, resolvedPath string, pathEntries []string) bool { if profile.OS != "windows" || profile.PackageManager == "brew" || strings.TrimSpace(resolvedPath) == "" { return false } - return len(engramBinaryDirsOnPath(pathEntries, profile.OS)) > 1 + if len(engramBinaryDirsOnPath(pathEntries, profile.OS)) <= 1 { + return false + } + // Only repair when the resolved engram path is coming from a non-user PATH entry. + userEntries, err := userPathEntries(profile.OS) + if err != nil { + return false + } + return !pathEntriesContainDir(userEntries, filepath.Dir(resolvedPath)) }Also applies to: 825-839
🤖 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 718 - 723, The current shouldRefreshWindowsEngram logic is too broad because it refreshes whenever engramBinaryDirsOnPath(pathEntries, profile.OS) finds more than one Windows Engram copy, even if cmdLookPath("engram") already resolved to the intended binary. Update shouldRefreshWindowsEngram in internal/cli/run.go to compare resolvedPath against the user-managed Engram location and only return true when the resolved binary is actually shadowed by an earlier PATH entry; then keep the install/reorder flow in the Engram install path from running for the already-correct binary.internal/components/sdd/inject_test.go (1)
1873-1876: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert
sdd-onboardexplicitly in these OpenCode agent-map checks.Both tests rely on the total count being 18, but neither list includes
sdd-onboard. A regression that drops only that phase can still pass as long as another agent keeps the count unchanged.Also applies to: 2242-2257
🤖 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_test.go` around lines 1873 - 1876, The agent-map coverage checks in the test helpers are missing explicit verification for sdd-onboard, so a regression could slip through while the total count still matches. Update the subAgent lists in the relevant assertions inside inject_test.go and the other matching test block to include sdd-onboard alongside the existing sdd-* and jd-* entries, keeping the count check but ensuring the named agent is asserted directly.internal/components/sdd/inject.go (2)
747-750: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Verify the adapter’s actual sub-agent filenames, not any known extension.
This post-check now accepts
.md,.agent.md, or.yamlfor every adapter. For VS Code, a stalesdd-apply.mdwould satisfy verification even ifsdd-apply.agent.mdwas never written, so sync can report success while Copilot still cannot load the managed agent.🤖 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 747 - 750, The post-check in the inject flow is too permissive because it only validates extensions for each phase, which can let a stale filename like sdd-apply.md pass even when the adapter-specific managed agent file was never created. Update the verification logic in the inject path to check the adapter’s expected sub-agent filenames explicitly rather than looping over generic extensions, using the existing phase handling for sdd-apply and sdd-verify to match the actual file naming scheme for each adapter.
2126-2149: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Back up the Claude lazy workflow before overwriting it.
writeClaudeLazySDDWorkflowwrites~/.claude/skills/_shared/sdd-orchestrator-workflow.mddirectly withWriteFileAtomic, so any user-edited file is overwritten without a restore path. This should use the same backup-aware write path as the new VS Code config writes.Suggested fix
path := filepath.Join(skillDir, "_shared", "sdd-orchestrator-workflow.md") - writeResult, err := filemerge.WriteFileAtomic(path, []byte(content), 0o644) + writeResult, err := writeFileWithBackup(path, []byte(content), 0o644) if err != nil { return InjectionResult{}, err }As per path instructions,
internal/components/**/*.go: “Any write to user config must have a backup/restore path.”📝 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 writeClaudeLazySDDWorkflow(homeDir string, adapter agents.Adapter, legacyAssignments map[string]model.ClaudeModelAlias, phaseAssignments map[string]model.ClaudePhaseAssignment) (InjectionResult, error) { if adapter.Agent() != model.AgentClaudeCode { return InjectionResult{}, nil } skillDir := adapter.SkillsDir(homeDir) if strings.TrimSpace(skillDir) == "" { return InjectionResult{}, nil } content := assets.MustRead("claude/sdd-orchestrator-workflow.md") if len(legacyAssignments) > 0 || len(phaseAssignments) > 0 { var err error content, err = injectClaudePhaseAssignments(content, legacyAssignments, phaseAssignments) if err != nil { return InjectionResult{}, err } } path := filepath.Join(skillDir, "_shared", "sdd-orchestrator-workflow.md") writeResult, err := writeFileWithBackup(path, []byte(content), 0o644) if err != nil { return InjectionResult{}, err } return InjectionResult{Changed: writeResult.Changed, Files: []string{path}}, 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/components/sdd/inject.go` around lines 2126 - 2149, The `writeClaudeLazySDDWorkflow` path writes the Claude shared workflow directly with `filemerge.WriteFileAtomic`, which can overwrite a user-edited file without any restore path. Update this function to use the same backup-aware write flow used by the new VS Code config writes, ensuring the existing `~/.claude/skills/_shared/sdd-orchestrator-workflow.md` is backed up before replacement and can be restored on failure. Keep the change localized to `writeClaudeLazySDDWorkflow` and reuse the existing backup/restore helpers or pattern already present in the config write logic.Source: Path instructions
internal/components/sdd/vscode_agent_visibility.go (1)
75-87: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don't treat backup inspection failures as "no backup".
Any
os.Stat(backupPath)error besidesErrNotExistis ignored here, so restore can silently skip a managed file and report a clean run. Return a wrapped error for non-ErrNotExistcases instead.🤖 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 75 - 87, The backup check in the restore flow is swallowing non-ErrNotExist failures, so the managed-file recovery path can silently skip real filesystem errors. Update the os.Stat(backupPath) handling in the restore logic to distinguish ErrNotExist from other errors, and return a wrapped error for any non-ErrNotExist case before proceeding with ReadFile/WriteFileAtomic/Remove. Use the restore loop around backupPath in vscode_agent_visibility.go as the place to fix this.internal/components/sdd/vscode_models.go (2)
109-123: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize cache-wide warnings before deduping.
These branches include
agentKeyin the warning text, so one unreadable/missing shared catalog turns into one distinct warning per VS Code agent anddedupWarnings()never collapses them. Keep per-agent wording for assignment-specific validation, but emit agent-agnostic text for catalog-wide failures.Suggested fix
if catalogErr != nil { - return "", []string{fmt.Sprintf("VS Code model assignment for %s skipped: %v", agentKey, catalogErr)} + return "", []string{fmt.Sprintf("VS Code model assignments skipped: %v", catalogErr)} } if catalog == nil { - return "", []string{fmt.Sprintf("VS Code model assignment for %s skipped: models cache unavailable", agentKey)} + return "", []string{"VS Code model assignments skipped: models cache unavailable"} } provider, ok := catalog[githubCopilotProviderID] if !ok { - return "", []string{fmt.Sprintf("VS Code model assignment for %s skipped: provider %q missing from models cache", agentKey, githubCopilotProviderID)} + return "", []string{fmt.Sprintf("VS Code model assignments skipped: provider %q missing from models cache", githubCopilotProviderID)} }Also applies to: 249-267
🤖 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 109 - 123, The cache-wide warning branches in vscode_models.go are still keyed by agentKey, so dedupWarnings() cannot collapse repeated catalog failures across VS Code agents. Update the warning text in the catalog-wide paths handled around catalogErr, catalog == nil, missing githubCopilotProviderID, and missing assignment.ModelID to be agent-agnostic, while keeping agentKey only for per-agent assignment validation. Use the existing warning construction in the VS Code model assignment flow so shared catalog issues produce one dedupable message.
218-230: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Escape control characters in injected YAML values.
yamlQuoteonly escapes\and". If a model label from the dynamic cache contains\n,\r, or\t, the emittedmodel:scalar changes across lines instead of being serialized literally, which makes the generated frontmatter brittle. Escape control characters or reject them before writing.Suggested fix
func yamlQuote(s string) string { needsQuote := false for _, r := range s { @@ } if needsQuote { escaped := strings.ReplaceAll(s, "\\", "\\\\") escaped = strings.ReplaceAll(escaped, "\"", "\\\"") + escaped = strings.ReplaceAll(escaped, "\n", "\\n") + escaped = strings.ReplaceAll(escaped, "\r", "\\r") + escaped = strings.ReplaceAll(escaped, "\t", "\\t") return "\"" + escaped + "\"" } return s }📝 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 yamlQuote(s string) string { needsQuote := false for _, r := range s { if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '-' || r == '_' || r == '/' || r == '.' { continue } needsQuote = true break } if needsQuote { escaped := strings.ReplaceAll(s, "\\", "\\\\") escaped = strings.ReplaceAll(escaped, "\"", "\\\"") escaped = strings.ReplaceAll(escaped, "\n", "\\n") escaped = strings.ReplaceAll(escaped, "\r", "\\r") escaped = strings.ReplaceAll(escaped, "\t", "\\t") return "\"" + escaped + "\"" } return s }🤖 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 218 - 230, The yamlQuote helper only escapes backslashes and double quotes, so control characters can still break injected YAML scalars. Update yamlQuote to either reject or safely escape control characters such as newlines, carriage returns, and tabs before returning the quoted value. Keep the fix localized to yamlQuote in vscode_models.go so any model label coming from the dynamic cache is serialized as a literal single-line YAML value.internal/components/uninstall/service.go (2)
1253-1292: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Fail closed before deleting in the no-backup path.
If
os.Stat(path+".backup")returns anything other thanNotExist, oros.ReadFile(path)fails, this still falls through toremoveFileIfExists(path). A permission/I/O problem on a user-owned file becomes data loss here.🛡️ Minimal fix
apply: func(path string) (bool, bool, error) { backupPath := path + ".backup" if _, err := os.Stat(backupPath); err == nil { content, err := os.ReadFile(backupPath) if err != nil { return false, false, fmt.Errorf("read backup %s: %w", backupPath, err) } if _, err := filemerge.WriteFileAtomic(path, content, 0o644); err != nil { return false, false, fmt.Errorf("restore file %s from backup: %w", path, err) } if err := os.Remove(backupPath); err != nil { return false, false, fmt.Errorf("remove backup file %s: %w", backupPath, err) } return true, false, nil + } else if !os.IsNotExist(err) { + return false, false, fmt.Errorf("inspect backup %s: %w", backupPath, err) } // If no backup exists, check if it's still our managed content. // If not, we skip deleting it (it's user-owned or already restored). _, statErr := os.Stat(path) @@ - content, readErr := os.ReadFile(path) - if readErr == nil { - contentStr := string(content) - isManaged := strings.Contains(contentStr, "sdd-orchestrator") || - strings.Contains(contentStr, "strict-tdd-mode") || - strings.Contains(contentStr, "trigger-rules") || - strings.Contains(contentStr, "gentle-ai") || - strings.Contains(contentStr, "managed") - if !isManaged { - return false, false, nil - } + content, readErr := os.ReadFile(path) + if readErr != nil { + return false, false, fmt.Errorf("read managed candidate %s: %w", path, readErr) + } + contentStr := string(content) + isManaged := strings.Contains(contentStr, "sdd-orchestrator") || + strings.Contains(contentStr, "strict-tdd-mode") || + strings.Contains(contentStr, "trigger-rules") || + strings.Contains(contentStr, "gentle-ai") || + strings.Contains(contentStr, "managed") + if !isManaged { + return false, false, nil } if err := removeFileIfExists(path); err != nil { return false, false, 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.backupPath := path + ".backup" if _, err := os.Stat(backupPath); err == nil { content, err := os.ReadFile(backupPath) if err != nil { return false, false, fmt.Errorf("read backup %s: %w", backupPath, err) } if _, err := filemerge.WriteFileAtomic(path, content, 0o644); err != nil { return false, false, fmt.Errorf("restore file %s from backup: %w", path, err) } if err := os.Remove(backupPath); err != nil { return false, false, fmt.Errorf("remove backup file %s: %w", backupPath, err) } return true, false, nil } else if !os.IsNotExist(err) { return false, false, fmt.Errorf("inspect backup %s: %w", backupPath, err) } // If no backup exists, check if it's still our managed content. // If not, we skip deleting it (it's user-owned or already restored). _, statErr := os.Stat(path) if statErr != nil { if os.IsNotExist(statErr) { return false, false, nil } return false, false, statErr } content, readErr := os.ReadFile(path) if readErr != nil { return false, false, fmt.Errorf("read managed candidate %s: %w", path, readErr) } contentStr := string(content) isManaged := strings.Contains(contentStr, "sdd-orchestrator") || strings.Contains(contentStr, "strict-tdd-mode") || strings.Contains(contentStr, "trigger-rules") || strings.Contains(contentStr, "gentle-ai") || strings.Contains(contentStr, "managed") if !isManaged { return false, false, nil } if err := removeFileIfExists(path); err != nil { return false, false, err } return true, true, 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/components/uninstall/service.go` around lines 1253 - 1292, The no-backup branch in the uninstall cleanup logic can still delete files when backup inspection or file reading fails, so tighten the flow in the restore/remove helper that uses os.Stat, os.ReadFile, and removeFileIfExists. Treat any backup status other than a definite missing backup, or any read error on the target path, as a fail-closed case and return without deleting; only proceed to removeFileIfExists when you have positively confirmed the file is managed content.
1280-1285: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This managed-file check misses real VS Code agents.
The fallback only recognizes
sdd-orchestrator,strict-tdd-mode,trigger-rules,gentle-ai, ormanaged. The shippedinternal/assets/vscode/agents/sdd-tasks.agent.mdcontains none of those strings, so a pristine managedsdd-tasks.agent.mdwith no.backupwill survive uninstall.🤖 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/uninstall/service.go` around lines 1280 - 1285, The managed-file detection in the uninstall flow is too narrow and misses the shipped VS Code agent file, so a pristine managed `sdd-tasks.agent.md` can be left behind. Update the fallback in the uninstall service logic around the `isManaged` check to recognize real managed agent content as well, using a more reliable marker from the agent asset itself or by including `sdd-tasks` in the managed पहचान set. Keep the change localized to the uninstall path that inspects `contentStr` so managed VS Code agents are removed consistently.internal/tui/model.go (3)
2027-2042: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Honor
ModelConfigModeon the empty VS Code picker Back row.When this picker is opened from Model Configuration with no Copilot models available, pressing Enter on
← Backhits them.Cursor == 1branch first and never returns toScreenModelConfig. Esc still works, so the visible Back action regresses only in this empty-state path.Suggested fix
if len(m.ModelPicker.AvailableIDs) == 0 { if m.ModelPicker.ForVSCode { + if m.ModelConfigMode && m.Cursor == 1 { + m.ModelConfigMode = false + m.setScreen(ScreenModelConfig) + return m, nil + } if m.Cursor == 1 { if prev, ok := m.pickerPreviousScreen(); ok { m.applyPickerEntry(prev) } return m, 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.if m.ModelPicker.ForVSCode { if m.ModelConfigMode && m.Cursor == 1 { m.ModelConfigMode = false m.setScreen(ScreenModelConfig) return m, nil } if m.Cursor == 1 { if prev, ok := m.pickerPreviousScreen(); ok { m.applyPickerEntry(prev) } return m, nil } if m.ModelConfigMode { m.ModelConfigMode = false m.PendingSyncOverrides = &model.SyncOverrides{ TargetAgents: []model.AgentID{model.AgentVSCodeCopilot}, VSCodeModelAssignments: map[string]model.ModelAssignment{}, } m = m.withResetSyncState() m.setScreen(ScreenSync) return m, 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/tui/model.go` around lines 2027 - 2042, The VS Code picker’s Back action is handled by the m.Cursor == 1 branch before the ModelConfigMode check, so in the empty-state path Enter on the Back row returns to the previous picker screen instead of ScreenModelConfig. Update the picker navigation logic in the ModelPicker/ModelConfigMode flow so the Back row honors ModelConfigMode first, restoring the model configuration screen when opened from model configuration with no Copilot models available.
2745-2755: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Add a timeout or cancellation path for community-tool commands.
cmd.CombinedOutput()can block indefinitely, andScreenCommunityToolInstallingexplicitly disables Esc. A hungnpm/installer process will therefore wedge the TUI until the child is killed externally.🤖 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/tui/model.go` around lines 2745 - 2755, The executeExternalCommand helper currently waits on cmd.CombinedOutput with no timeout or cancellation, so a community-tool install can hang the TUI indefinitely. Update executeExternalCommand to accept and honor a context or timeout, wire that through the command creation path (using commandFn and cmd.Process signaling as needed), and make sure the caller in ScreenCommunityToolInstalling can abort cleanly instead of blocking forever.
4041-4048: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
This flow slice becomes ambiguous when both model pickers are present.
If the selection includes VS Code Copilot and OpenCode multi-mode with cache present,
pickerFlowSlice()appendsScreenModelPickertwice.pickerNextScreen()/pickerPreviousScreen()match byScreenvalue only, so the second picker collapses onto the first and navigation can loop or jump to the wrong step. These two picker states need distinct flow identities before linear next/previous lookup is safe.🤖 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/tui/model.go` around lines 4041 - 4048, The picker flow in pickerFlowSlice currently appends ScreenModelPicker for both VS Code Copilot and OpenCode multi-mode, which makes pickerNextScreen and pickerPreviousScreen ambiguous because they match only by Screen value. Update the flow to give each picker state a distinct identity in the flow slice, and adjust the next/previous lookup logic to distinguish the two model picker states instead of treating them as the same ScreenModelPicker entry. Use the existing pickerFlowSlice, pickerNextScreen, and pickerPreviousScreen paths to ensure navigation stays linear and does not collapse or loop when both pickers are present.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/run.go (1)
191-191: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve explicit empty VS Code assignment maps in the install merge path (
internal/cli/run.go:1773-1776, 239-241)
modelAssignmentsToStatecollapses an empty selection tonil, somergeExplicitAgentInstallStatetreats it as “no override” and keeps stale persisted assignments. Keep empty maps distinct fromnil, or handle the reset before merging.🤖 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` at line 191, The VS Code assignment merge path is treating an explicit empty selection as nil, so stale persisted assignments are kept instead of being cleared. Update modelAssignmentsToState and the merge flow used by mergeExplicitAgentInstallState so an empty VSCodeModelAssignments map is preserved as an intentional override rather than collapsed to nil, and make sure the reset case is handled before merging in run.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/cli/run.go`:
- Line 191: The VS Code assignment merge path is treating an explicit empty
selection as nil, so stale persisted assignments are kept instead of being
cleared. Update modelAssignmentsToState and the merge flow used by
mergeExplicitAgentInstallState so an empty VSCodeModelAssignments map is
preserved as an intentional override rather than collapsed to nil, and make sure
the reset case is handled before merging in run.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 68818075-76da-4a36-9240-c605a019a0d9
📒 Files selected for processing (4)
README.mdinternal/cli/run.gointernal/components/sdd/inject.gointernal/components/sdd/vscode_agent_visibility.go
Snakeblack
force-pushed
the
feat/vscode-copilot-subagents-pr3
branch
2 times, most recently
from
8e7a218 to
30dbc36
Compare
Snakeblack
force-pushed
the
feat/vscode-copilot-subagents-pr3
branch
from
30dbc36 to
e19cb81
Compare
…ram download test on Windows
…tore contracts, picker hydration, and test mock providers
… flows and test assertions
…cleanup, duplicate paths, and JSONC parsing
… uninstall targets
…tore contracts, picker hydration, and test mock providers
Snakeblack
force-pushed
the
feat/vscode-copilot-subagents-pr3
branch
from
e19cb81 to
acdbd08
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
Depends on: #738
Related context: #731, #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 4 of the VS Code Copilot SDD subagents stack.
This PR depends on #738. Because the stack is currently opened from a contributor fork and upstream dependency branches are not available as base branches, this PR targets
main.Focused review diff for the PR4 work unit before the
mainconflict-resolution merge:Snakeblack/gentle-ai@fix/vscode-copilot-agent-hardening...27e6100
Conflict-resolution merge commit:
9aa7eee(origin/mainv1.34.0).Summary
sdd-orchestratorshould be visible in the native VS Code dropdown, while the TUI can configure phase models.Changes
internal/tui/model.gointernal/tui/screens/model_picker.gointernal/tui/**/*_test.goREADME.md,docs/agents.mddocs/intended-usage.md,docs/platforms.mdSize Exception Rationale
This PR requests
size:exception.The focused PR4 diff is above the 400-line budget because it contains a cohesive user-facing slice: TUI state flow, picker behavior, regression tests, and final docs. Splitting tests or docs away from the TUI change would make review harder because the user contract is the behavior being documented.
Test Plan
Linux/LF full suite
go test ./... go vet ./...Focused review checks
Results:
go test ./...) in Linux/LF verification clone before PR creationgo test ./...) in Linux/LF verification clone after resolving theorigin/mainconflictgo vet ./...) in Linux/LF verification clone before PR creationgo vet ./...) in Linux/LF verification clone after resolving theorigin/mainconflictcd 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:featurerequested; maintainer label neededgo test ./...Contributor Checklist
status:approvedsize:exceptionwith rationale documentedtype:*label to this PR - maintainer label needed (type:feature)go test ./...) before PR creation and after conflict resolutioncd e2e && ./docker-test.sh)Co-Authored-BytrailersNotes for Reviewers
The important UX distinction is intentional: the TUI can configure coordinator and per-phase VS Code models, but the native VS Code agent dropdown should expose only the user-facing
sdd-orchestrator.Summary by CodeRabbit