Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Thanks for splitting this so carefully, and for keeping the safeguards from #591 intact. The lock coverage, read-only loading and partial-failure reporting all held up when I traced them. A few things before this can land:

Needs a change

1. Lock file ends up in git. internal/projects/lock.go:21 creates config.yaml.projects.lock next to the config and never removes it. With git_root: root, WriteScopeGitignore only ignores config.yaml, so the next skillshare push (git add -A) commits the lock file. The MCP and hooks services keep their locks in the state dir (internal/mcp/service.go:172, internal/hooks/service.go:364). Could this follow the same pattern?
2. Dashboard save now detaches folders. handleSaveProject → Service.Put → detachDeparting (internal/projects/service.go:172). On main, saving a project only validated and wrote config; only remove detached. Unticking a target in the dashboard editor now removes its manifest and copies links back. That matches the proposal's edit-detach behavior, but the PR describes itself as behavior-preserving apart from two deliberate fixes. Please either list it as a third deliberate change (with a test) or move it to feat(project): manage global registrations from the CLI #603.
3. CLI-only code in the shared layer. At this commit the dashboard calls List, Put and Remove. Service.Sync, Show, Find and StoredKey are only used by feat(project): manage global registrations from the CLI #603. Service.Sync (about 85 lines) also re-implements validation, backup and discovery, while skillshare sync keeps its own path. The scope note on Manage global project registrations from the CLI #591 asked to extract only what both interfaces need and to reuse the existing sync orchestration where practical. Could these move into feat(project): manage global registrations from the CLI #603, ideally reusing the CLI sync flow?
4. SkillsPathKeptBy now uses PathsEqual (internal/config/path_overlap.go:96). target remove also calls it, so on Windows a case-different skills path is now treated as shared and kept. Possibly right, but it is a global change not listed in the PR description.

Please double-check

5. Remove now only detaches targets in enabledSkillTargets (service.go:196-201, 241-250). For an agents-only project (skills: null) whose skills-path targets were synced with exclude: ["*"], the manifest and any leftover links seem to stay behind. main removed them. I have only read this, not reproduced it.

Optional

• save_candidate.go:50-98 adds another temp-file-and-rename writer. writeFileAtomic in internal/server/handler_skill_content.go already does this.
• The oracle in root_fuzz_test.go is the function's own spec, and it skips the risky inputs (~user, drive-relative paths). The table tests may be enough.

Happy to talk through any of these. If one of them is intentional, a short note in the description is fine.

Thanks for tracing it this closely. All five are addressed in the force-pushed commit:

1. Lock file. The project lock now lives in the state dir (<state>/projects/<hash>.lock, keyed by the resolved config path), next to the MCP and hooks locks. Nothing is created beside config.yaml anymore, and a test checks that.
2. Dashboard save. Back to main's behavior: save validates and writes, and only remove detaches. Unticking a target leaves its folder alone. Edit-detach moves to feat(project): manage global registrations from the CLI #603, where it's part of project edit.
3. CLI-only code. Service.Sync, Show, Find and StoredKey are gone from this PR, along with their tests. feat(project): manage global registrations from the CLI #603 adds what it needs back, and project sync there will reuse the existing skillshare sync flow instead of a second orchestration path.
4. SkillsPathKeptBy. I kept PathsEqual on purpose. On Windows a case-different spelling is the same folder, so target remove unlinking it would break the other target still syncing there. Added a Windows test and listed it in the description.
5. Agents-only remove. You were right, that was a regression. Remove now detaches every project target under the root, whether skills are on or not, like main. There's a new test for a skills: null project with a leftover manifest; it fails on the previous commit.

Optional ones: I dropped the fuzz test. I left save_candidate.go's writer separate because it behaves differently from writeFileAtomic. It keeps the existing config's mode and honors the umask on first create (TestPrepareSave_FirstSaveHonorsUmask), and the raw-write audit files them under different domains. Happy to merge them if you'd still prefer one helper.