Skip to content

fix(setup): anchor marker end-search and reject relative config-home#520

Merged
Alan-TheGentleman merged 1 commit into
mainfrom
fix/493-hardening
Jun 20, 2026
Merged

fix(setup): anchor marker end-search and reject relative config-home#520
Alan-TheGentleman merged 1 commit into
mainfrom
fix/493-hardening

Conversation

@Alan-TheGentleman

@Alan-TheGentleman Alan-TheGentleman commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

🔗 Linked Issue

Closes #519


🏷️ PR Type

  • type:bug — Bug fix
  • type:feature — New feature
  • type:docs — Documentation only
  • type:refactor — Code refactoring (no behavior change)
  • type:chore — Maintenance, dependencies, tooling
  • type:breaking-change — Breaking change

📝 Summary

Two low-severity hardening fixes from the #493 agent-registry review:

  • Marker idempotency (registry.go): upsertMarkerBlock searched 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.
  • Relative config-home (agents.go): vscodeUserDir/kilocodeConfigDir used XDG_CONFIG_HOME/APPDATA without a filepath.IsAbs check, reintroducing the write-into-CWD risk the userHome() guard closes. Relative values now fall back to the absolute home path.

📂 Changes

File Change
internal/setup/registry.go Anchor end-marker search after the begin marker
internal/setup/agents.go filepath.IsAbs guard on APPDATA + XDG_CONFIG_HOME (vscode + kilocode)
internal/setup/registry_test.go Tests: stray-end-marker idempotency; relative XDG/APPDATA ignored

🧪 Test Plan

  • go test ./internal/setup/..., go vet, go build ./... all pass
  • New tests fail against the old code and pass with the fix

📌 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: .mdc rules with alwaysApply only 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/rules global support. So the global .mdc engram 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

  • I linked an approved issue above (Closes #N)
  • I added exactly one type:* label to this PR
  • Conventional commit format
  • No Co-Authored-By trailers

Summary by CodeRabbit

  • Bug Fixes
    • Improved validation of configuration directory paths to ensure proper resolution and system compatibility
    • Enhanced marker block management to maintain consistency and reliability across multiple operations

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.
Copilot AI review requested due to automatic review settings June 20, 2026 10:15
@Alan-TheGentleman Alan-TheGentleman added the type:bug Bug fix label Jun 20, 2026
@coderabbitai

coderabbitai Bot commented Jun 20, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d02fb501-c0f2-4edc-9393-bfc40a9182e6

📥 Commits

Reviewing files that changed from the base of the PR and between 13fc976 and f80b299.

📒 Files selected for processing (3)
  • internal/setup/agents.go
  • internal/setup/registry.go
  • internal/setup/registry_test.go

📝 Walkthrough

Walkthrough

Two hardening fixes in internal/setup: upsertMarkerBlock in registry.go now searches for the end marker only in the substring after the begin marker to prevent duplicate managed blocks from stray end markers in user content. vscodeUserDir and kilocodeConfigDir in agents.go add filepath.IsAbs guards to reject relative XDG_CONFIG_HOME and APPDATA values. Two regression tests cover both fixes.

Changes

Marker idempotency and relative config-home hardening

Layer / File(s) Summary
upsertMarkerBlock: anchor end-marker search after begin marker
internal/setup/registry.go, internal/setup/registry_test.go
upsertMarkerBlock now records the begin-marker position first (start), then searches for the end marker only in the substring after it (stop), so a stray end marker in user content above the managed block no longer skips in-place replacement and causes a second block to be appended. The regression test seeds a file with a stray end marker, runs the function twice, and asserts exactly one begin marker and correct managed-block content.
vscodeUserDir/kilocodeConfigDir: reject relative env paths
internal/setup/agents.go, internal/setup/registry_test.go
vscodeUserDir adds filepath.IsAbs checks to both the Windows APPDATA branch and the default XDG_CONFIG_HOME branch; kilocodeConfigDir adds the same guard to its XDG_CONFIG_HOME branch. Relative values now fall through to the home-directory fallback. The regression test sets relative env values and asserts both functions resolve under the absolute test home.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • Gentleman-Programming/engram#493: Introduced the original upsertMarkerBlock registry plumbing and OS-aware vscodeUserDir/kilocodeConfigDir helpers in the same files that this PR hardens.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/493-hardening

Comment @coderabbitai help to get the list of available commands and usage tips.

@Alan-TheGentleman Alan-TheGentleman merged commit 980ebd8 into main Jun 20, 2026
9 of 10 checks passed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 upsertMarkerBlock to 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 / APPDATA values 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.

Comment on lines +204 to +208
if start != -1 {
if rel := strings.Index(text[start:], end); rel != -1 {
stop = start + rel + len(end)
}
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(setup): harden agent-registry marker idempotency and relative config-home paths

2 participants