fix: retrospective review fixes for merged PR #64#111
Merged
Conversation
PR #64 added a "## Source instincts" / "Source instinct:" provenance line to evolved SKILL.md / agent / command bodies, using homunculus.Instinct.Path verbatim. That Path is whatever directory string ReadInstinctFile's scan started from (homunculus.Layout.Root, via BOUGH_HOMUNCULUS_DIR / XDG_DATA_HOME), which is relative whenever either env var is set to a relative path. This is the exact hazard commit c9cece5 ("fix(cli): enforce absolute symlink target") already fixed for the project-skill symlink target the same day PR #64 merged: "Evolved-skill sources can be relative when BOUGH_HOMUNCULUS_DIR is set to a relative path; a raw os.Symlink of a relative target would resolve against linkPath's dir and dangle." emit.go's new provenance lines never got the same treatment, so under that same environment they render a path that resolves against the READER's cwd instead of the scan's — silently pointing at the wrong file (or nothing) the moment a SKILL.md is read from a different directory, which is precisely the "worktree symlink hop away from the homunculus store" scenario this feature exists for. Add absInstinctPath (mirrors ensureSymlink's filepath.Abs + fallback pattern) and route both emit sites (sourceInstinctsBlock, RenderCommand) through it.
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.
Retrospective
/code-reviewsweep of merged PR #64 that shipped without pre-merge review.PR #64 added a "## Source instincts" / "Source instinct:" provenance block to evolved SKILL.md / agent / command bodies, rendering
homunculus.Instinct.Pathverbatim as "the member's absolute on-disk location." That claim doesn't hold:Pathis set byReadInstinctFileto whatever directory string the scan started from (homunculus.Layout.Root, resolved fromBOUGH_HOMUNCULUS_DIR/XDG_DATA_HOMEwhen either is set), which is relative whenever that env var is a relative path.This is the exact hazard commit c9cece5 ("fix(cli): enforce absolute symlink target") already fixed for the project-skill symlink target the same day PR #64 merged — its comment states plainly: "Evolved-skill sources can be relative when BOUGH_HOMUNCULUS_DIR is set to a relative path; a raw os.Symlink of a relative target would resolve against linkPath's dir and dangle."
emit.go's new provenance lines never got the same treatment. Under that same environment, the emitted "Source instinct" path resolves against the reader's cwd instead of the scan's cwd — silently pointing at the wrong file (or nothing) the instant a SKILL.md is opened from a different directory, which is precisely the "worktree symlink hop away from the homunculus store" scenario the feature exists to serve.Fix: add
absInstinctPath(mirrorsensureSymlink'sfilepath.Abs+ fallback-on-error pattern) and route both emit sites —sourceInstinctsBlock(skills + agents) andRenderCommand(commands) — through it, so the emitted path is always resolved to absolute regardless of howBOUGH_HOMUNCULUS_DIR/XDG_DATA_HOMEis configured.Test plan
New tests:
TestSourceInstinctsBlock_RelativePathResolvedAbsolute,TestAbsInstinctPath, plus a relative-path case added toTestRenderCommand.