Fix install funnel: prune guard, legacy config, doctor day-one failures - #116
Merged
Conversation
Reviewer's GuideImplements a safer, more user-friendly setup-driven sync flow with per-harness destructive change confirmation, degrades legacy MCP configs targeting unsupported agents instead of hard-failing, relaxes README doctor checks, fixes the tester agent frontmatter, and improves prompts and git init behavior, with tests updated and added to cover the new contracts. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
offerGitInit, the fallbackgit initerror path printserrfrom the first call while using combined output from both calls; this should reporterr2(and probably only the second command’s output) to avoid misleading error diagnostics. - The new MCP legacy-handling in
validateConfigwrites warnings directly toos.Stderr; consider routing these through an injected writer or logger so that callers (and tests) can control or capture diagnostics more cleanly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `offerGitInit`, the fallback `git init` error path prints `err` from the first call while using combined output from both calls; this should report `err2` (and probably only the second command’s output) to avoid misleading error diagnostics.
- The new MCP legacy-handling in `validateConfig` writes warnings directly to `os.Stderr`; consider routing these through an injected writer or logger so that callers (and tests) can control or capture diagnostics more cleanly.
## Individual Comments
### Comment 1
<location path="cmd/dotagents/setup.go" line_range="111-113" />
<code_context>
- if out, err := exec.Command("git", "-C", repoRoot, "init").CombinedOutput(); err != nil {
- fmt.Fprintf(streams.out, "git init failed: %v: %s\n", err, strings.TrimSpace(string(out)))
- return
+ if out, err := exec.Command("git", "-C", repoRoot, "init", "-b", "main").CombinedOutput(); err != nil {
+ // -b needs git >= 2.28; retry without it before giving up.
+ if out2, err2 := exec.Command("git", "-C", repoRoot, "init").CombinedOutput(); err2 != nil {
+ fmt.Fprintf(streams.out, "git init failed: %v: %s\n", err, strings.TrimSpace(string(append(out, out2...))))
+ return
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Error reporting on git init retry uses the first error and concatenated outputs, which can obscure the actual failure.
When the retry `git init` fails, the log still uses `err` from the first attempt and a concatenated `out`/`out2`, so `err2` is never surfaced and the source of the failure is unclear. It would be better to log `err2` and `out2` when the second attempt fails, and optionally note the initial `-b main` failure separately instead of merging the outputs.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+111
to
+113
| if out, err := exec.Command("git", "-C", repoRoot, "init", "-b", "main").CombinedOutput(); err != nil { | ||
| // -b needs git >= 2.28; retry without it before giving up. | ||
| if out2, err2 := exec.Command("git", "-C", repoRoot, "init").CombinedOutput(); err2 != nil { |
There was a problem hiding this comment.
suggestion (bug_risk): Error reporting on git init retry uses the first error and concatenated outputs, which can obscure the actual failure.
When the retry git init fails, the log still uses err from the first attempt and a concatenated out/out2, so err2 is never surfaced and the source of the failure is unclear. It would be better to log err2 and out2 when the second attempt fails, and optionally note the initial -b main failure separately instead of merging the outputs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e006b56af6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| kept = append(kept, agentName) | ||
| } | ||
| cfg.MCPServers[i].Agents = kept |
There was a problem hiding this comment.
When a legacy config scopes an MCP server only to pi, this assignment leaves Agents as an empty slice after the unsupported target is skipped. desiredMCPServersForAgent treats an empty Agents list as “apply to every MCP-capable harness”, so setup/sync will install that pi-only server into Claude/Codex/OMP instead of ignoring it as the warning says. This can unexpectedly add MCP commands/secrets to unrelated harness configs; unsupported-only targets need to be removed without turning into the global default.
Useful? React with 👍 / 👎.
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.
Fixes for all findings from the VPS e2e funnel test (docs/plans/launch-2026-07-14/07-vps-e2e-funnel.md):
F1 (critical, data loss): setup-driven sync now previews per-harness removals and agent-role overwrites and asks per harness (
[y/N], default no). Declining keeps the harness's existing files; adds and skill updates still apply. Steady-statedotagents syncbehavior is unchanged.F5 (critical, upgrade dead end): a config that targets an MCP-unsupported agent (e.g. pre-split
pi) no longer hard-failssetup/status/doctor. The target is dropped with a warning that names the fix (renamepitoomp).F3 (high):
agents/tester.mdfrontmatter fixed (tools:was a comma-separated string; agnix rejected it, sodoctorfailed on every fresh install).F2 (high): prompts announce
skipped (no input; answering no)on stdin EOF instead of silently defaulting;git inituses-b main(with fallback for git < 2.28).F4 (medium):
doctorREADME check passes on config roots withoutREADME.mdor without the generated block (both optional for users), and the remediation hint is the real command (dotagents sync, notdotagents sync render).README updated for the new confirm behavior. New tests cover the prune guard (decline / accept / EOF), legacy-config degradation, EOF prompt announcement, and the doctor README pass cases; two existing tests updated to the new contracts. Full suite passes.
Summary by Sourcery
Add safeguards and UX fixes around setup-driven sync, legacy MCP config handling, prompts, doctor README checks, and the tester agent metadata to harden the install funnel and day-one experience.
New Features:
Bug Fixes:
Enhancements:
Tests: