{{ message }}
fix: stop opencode.json wipe and install panic on failure paths - #17
Merged
Conversation
writeOpenCodeConfig silently overwrote a corrupt opencode.json with only the mcp key, destroying the user's model/provider/permissions config. readOpenCodeConfig masked a malformed mcp block as empty, and the Install path panicked after printing a false 'Successfully installed' when symlink creation failed (e.g. ~/.agents not writable). All three are silent-failure / false-success paths the test suite did not cover. Regression tests added (written RED-first, watched fail then pass). - mcp: return parse errors instead of wiping or masking user config - installer: nil-check *ComponentCounts before deref (mirrors Update path)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes 3 silent-failure / false-success bugs surfaced by a project quality review. All three are failure paths the test suite did not previously cover.
Problems
1.
writeOpenCodeConfigwiped a corruptopencode.json(Critical, data loss)When the existing
opencode.jsonwas truncated/corrupt,json.Unmarshalwas ignored,fullConfigstayednil, and the file was rewritten with only themcpkey — destroying the user'smodel/provider/permissions/$schemaconfig.2.
readOpenCodeConfigmasked a malformedmcpblock (Important)A
mcpvalue of the wrong JSON type (e.g. a string) was silently treated as empty, so every caller saw "no MCP servers" with no error.3. Install panicked after a false "Successfully installed" (Critical)
installOneResolvedPlugindereferenced the(*ComponentCounts)return without a nil-check; when symlink creation failed (e.g.~/.agentsnot writable) it returnednil, err, the code printed⚠️ Warningthen✓ Successfully installed, then nil-deref panicked. TheUpdatepath already handled this correctly — Install did not.Fix
internal/mcp/manager.go: return parse errors (wrapped with%w) instead of wiping/masking. File-not-found path preserved.internal/plugin/installer.go: nil-checkcountsbefore deref, mirroring the Update path.Testing
Strict TDD — each test written RED-first, watched fail for the right reason, then watched pass:
TestWriteOpenCodeConfig_CorruptFileIsPreservedNotWipednil, would wipe fileTestReadOpenCodeConfig_CorruptMCPBlock_ReturnsErrornil(silent)TestInstall_SymlinkFailureDoesNotPanicnil pointer dereferenceGates:
go build✅ ·go vet✅ ·gofmt -lclean ✅ ·go test ./...all 11 packages pass incl. e2e ✅ ·golangci-lintno new issues (the 2 priorjson.Unmarshalerrcheck items resolved).Caller safety
readOpenCodeConfig/writeOpenCodeConfigare unexported. All 5 internal callers (InstallMCPConfig,UninstallMCPConfig,DisableMCPConfig,EnableMCPConfig,ListMCPServers) already propagate non-NotExistserrors viareturn err— the new corrupt-config errors surface correctly; no behavior change on the happy path.Out of scope (flagged in the review, not addressed here)
Non-atomic config writes, Update Stage-2 atomicity,
Removefalse-success,--forcewiping real dirs, shallow clones, file modes (0644→0600). These are semantic/redesign changes worth a separate pass.