fix(setup): anchor marker end-search and reject relative config-home#520
Conversation
Two hardening gaps from the #493 review: (1) upsertMarkerBlock searched for the end marker from the start of the file, so a stray end marker in user content above the managed block defeated idempotency (duplicate block); now the end search is anchored after the begin marker. (2) vscodeUserDir/kilocodeConfigDir used XDG_CONFIG_HOME/APPDATA without filepath.IsAbs, reintroducing the write-into-CWD risk; now relative values fall back to the absolute home path. Adds tests for both.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughTwo hardening fixes in ChangesMarker idempotency and relative config-home hardening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Hardens engram setup’s registry/instruction writing to be reliably idempotent in the presence of user content that may contain marker strings, and prevents relative XDG_CONFIG_HOME / APPDATA from causing config writes under the current working directory.
Changes:
- Fix
upsertMarkerBlockto search for the end marker starting at the begin marker location (preventing stray end markers earlier in the file from breaking idempotency). - Guard VS Code + Kilo Code config directory resolution so relative
XDG_CONFIG_HOME/APPDATAvalues are ignored in favor of an absolute home-based fallback. - Add regression tests covering the stray-end-marker scenario and relative config-home environment variables.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/setup/registry.go | Anchors end-marker search to the managed block region so marker-block writes remain idempotent. |
| internal/setup/agents.go | Rejects relative APPDATA / XDG_CONFIG_HOME when computing agent config directories. |
| internal/setup/registry_test.go | Adds tests for stray end markers and for ignoring relative config-home env vars. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if start != -1 { | ||
| if rel := strings.Index(text[start:], end); rel != -1 { | ||
| stop = start + rel + len(end) | ||
| } | ||
| } |
🔗 Linked Issue
Closes #519
🏷️ PR Type
type:bug— Bug fixtype:feature— New featuretype:docs— Documentation onlytype:refactor— Code refactoring (no behavior change)type:chore— Maintenance, dependencies, toolingtype:breaking-change— Breaking change📝 Summary
Two low-severity hardening fixes from the #493 agent-registry review:
registry.go):upsertMarkerBlocksearched for the end marker from the start of the file. A stray end marker in user content above the managed block defeated idempotency (replace skipped → duplicate block appended). The end search is now anchored to after the begin marker.agents.go):vscodeUserDir/kilocodeConfigDirusedXDG_CONFIG_HOME/APPDATAwithout afilepath.IsAbscheck, reintroducing the write-into-CWD risk theuserHome()guard closes. Relative values now fall back to the absolute home path.📂 Changes
internal/setup/registry.gointernal/setup/agents.gofilepath.IsAbsguard onAPPDATA+XDG_CONFIG_HOME(vscode + kilocode)internal/setup/registry_test.go🧪 Test Plan
go test ./internal/setup/...,go vet,go build ./...all pass📌 Out of scope (separate decision needed)
The #493 review also flagged the Cursor global rule path (
~/.cursor/rules/engram.mdc). I verified against Cursor's docs/forum:.mdcrules withalwaysApplyonly work at the project level (<project>/.cursor/rules/). Global "User Rules" are configured in Settings → Rules as plain text with no filesystem path — there's an open feature request for~/.cursor/rulesglobal support. So the global.mdcengram writes is likely never read by Cursor. Fixing that is a design decision (drop the global rule, or document it as a manual Settings step) and is intentionally left out of this hardening PR.✅ Contributor Checklist
Closes #N)type:*label to this PRCo-Authored-BytrailersSummary by CodeRabbit