Prune and refresh managed starter files during sync - #186
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's GuideSync now reconciles only the managed memory hooks and libraries against the running release using a committed hash manifest plus v0.8.0 compatibility hashes: it scaffolds and refreshes recognized content, safely removes retired files with confirmation, preserves and reports edits, and documents the ownership model. Sequence diagram for managed starter reconciliation during syncsequenceDiagram
participant Sync
participant Reconciler as reconcileStarterFiles
participant Manifest as .dotagents-starter.json
participant Disk as ConfigRoot
participant Confirm as ConfirmRemovals
Sync->>Reconciler: reconcileStarterFiles
Reconciler->>Manifest: loadStarterManifest
Reconciler->>Disk: shippedStarterFiles
loop managed memory/hooks and memory/lib files
Reconciler->>Disk: read starter path
alt missing
Reconciler->>Disk: writeStarterFile
else manifest or legacy hash matches
Reconciler->>Disk: writeStarterFile
else unrecognized or user-modified
Reconciler-->>Sync: report kept modified
end
end
opt retired managed files
Reconciler->>Confirm: promptYesNoDefaultNo
alt confirmed and recognized hash
Reconciler->>Disk: remove retired file
else declined or modified
Reconciler-->>Sync: report kept modified
end
end
Reconciler->>Manifest: saveStarterManifest
Reconciler-->>Sync: report changes
Flow diagram for safe managed starter file decisionsflowchart TD
A[dotagents sync] --> B[Load manifest and shipped managed files]
B --> C{File exists?}
C -->|No| D[Scaffold file]
C -->|Yes| E{Disk hash matches shipped or ownership baseline?}
E -->|Yes| F[Refresh file and record shipped hash]
E -->|No| G[Keep file and report modified]
B --> H[Find manifest or legacy files no longer shipped]
H --> I{Retired file hash recognized?}
I -->|No| G
I -->|Yes| J{Removal confirmed?}
J -->|Yes| K[Remove file]
J -->|No| G
D --> L[Save .dotagents-starter.json]
F --> L
K --> L
G --> L
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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 |
![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 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="internal/app/starter_manifest.go" line_range="258-263" />
<code_context>
+ }
+ }
+
+ // Managed files this release no longer ships.
+ for _, path := range sortedKeysNative(manifest.Files) {
+ if _, stillShipped := shipped[path]; stillShipped {
+ continue
+ }
+ removed, kept, err := retireStarterFile(root, path, manifest.Files[path], legacy[path], streams, confirm)
+ if err != nil {
+ return changes, err
</code_context>
<issue_to_address>
**🚨 issue (security):** A committed `.dotagents-starter.json` can contain a path such as `../../some/file`; the manifest removal loop passes it to `retireStarterFile`, whose `filepath.Join` resolves outside the config root and removes the external file when its hash matches the recorded value.
**Triggers:** When a config root contains a malicious or corrupted manifest with a path-traversal entry and a matching hash.
**Suggested fix:** Reject manifest entries unless they are relative, clean, and satisfy `isManagedStarterPath`, and ensure the resolved target remains under `root` before reading or removing it.
</issue_to_address>
### Comment 2
<location path="internal/app/sync.go" line_range="77" />
<code_context>
+ if err != nil {
+ return err
+ }
+ starterChanges.report(os.Stdout)
+
toolInstalls, err := installMemoryTools(repoRoot)
</code_context>
<issue_to_address>
**issue (bug_risk):** The starter reconciliation report is written to `os.Stdout` instead of the sync operation's configured output stream, so callers that pass `Stdout: io.Discard` or a buffer still receive starter-change output on the process stdout.
**Triggers:** When sync is invoked by the TUI, web server, or a test/API caller that supplies a custom `runOptions.Stdout`.
**Suggested fix:** Call `starterChanges.report(setupStreams(opts).out)` or otherwise reuse the configured output writer.
```suggestion
starterChanges.report(setupStreams(opts).out)
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and sync now persists changes to the config root and can delete starter files that a release no longer ships. If the ownership or hash checks are wrong, a file could be removed or stale code could be refreshed; reverting the CLI would not restore a deleted file without another copy, although the scope is bounded and modified files are intended to be protected.
Blocking findings: internal/app/starter_manifest.go:263, internal/app/sync.go:77
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.
Why
setupcopies the starter content once, and nothing ever updates or removes it afterwards. That is why a config root created by v0.8.0 kept the old per-provider digest modules (amp_digest.py,factory_digest.py,hermes_digest.py,omp-memory.ts) after v0.9.0 deleted them upstream, and why itsmemory/hooks/session-end.shkept calling code that no longer exists. Deleting files is not somethingsyncdid at all.What this does
syncnow reconciles the managed starter code layer (memory/hooks/,memory/lib/) against the running release:Ownership is tracked in
.dotagents-starter.jsonat the config root (meant to be committed withdotagents.yaml), so every machine shares the same baseline.Safety rule
dotagents only ever refreshes or removes content it wrote itself. A managed file is touched only when its hash matches the manifest baseline or a known earlier-release hash (
legacyStarterHashes, seeded with the v0.8.0 versions that changed or were retired). Content dotagents does not recognize is never overwritten — it is reported askept (modified by you)and no ownership is recorded for it.AGENTS.md,dotagents.yaml,agents/*.md, andskills/are never managed this way: those stay user content and are still only created when missing.Removals honour the existing setup-driven confirmation flow (
ConfirmRemovals), sodotagents setuppreviews them per item.Example
Upgrading a real v0.8.0 config root:
Verification
go test ./...passes, including 9 new tests: scaffold + manifest recording, idempotent second run, manifest-baseline refresh, legacy-hash refresh, unrecognized content never touched, retired-file removal only when unmodified, manifest-tracked removal, declined confirmation keeps the file, managed-path/legacy-path guards, and an end-to-endrunSynccasego build ./...,go vet ./...,golangci-lint v2.12.2(cold cache, Go 1.24.2) — cleanNotes
legacyStarterHashesis a one-time bridge for roots created before the manifest existed. Every release after this one is covered by the manifest, so the table does not need to grow.Docs updated: README memory section,
docs/setup.md(new "Managed starter files"),memory/README.md.Summary by Sourcery
Keep the managed memory starter code layer synchronized with the running release while preserving user-owned customizations.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: