Skip to content

Add configurable extra skill roots under [skills] - #14

Closed
thedancingdeveloper wants to merge 4 commits into
fork/mainfrom
fork/skills-roots
Closed

thedancingdeveloper wants to merge 4 commits into
fork/mainfrom
fork/skills-roots

Conversation

@thedancingdeveloper

Copy link
Copy Markdown

Summary

  • [skills] extra_user_roots adds roots under the user config directory, beside the built-in skills and ~/.agents/skills.
  • [skills] extra_project_roots adds roots under each directory from the project root to the working directory, beside .agents/skills.
  • Both are relative and confined to their base directory: a .. climb or an absolute value is dropped. Absent keys change nothing.

WI-1133. Do not merge; the epic lead reviews.

Test plan

  • cargo test -p codex-skills-extension --offline --lib extra_ — the two new tests pass (root added, built-in root kept, escape dropped)
  • cargo fmt -p codex-config -p codex-skills-extension -- --check
  • cargo check -p codex-config -p codex-skills --offline --lib

@thedancingdeveloper thedancingdeveloper left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Review: no blockers (head 36e20d4)

WI-1133, a small additive change (+138, config plus ext/skills):

  • [skills] extra_user_roots and extra_project_roots (relative paths); the built-in roots are always kept.
  • confined_join trims leading separators and joins via AbsolutePathBuf::join. That goes through absolutize_from, which normalizes .. lexically, then requires starts_with(base). So ../outside, an absolute path and ~/… (expanded to home, outside the base) are all dropped.
  • Project roots are only searched between the project root and cwd, and only if they're directories.
  • Tests: the extra project root is added, and ../outside is refused; the extra user root is added beside the built-ins.

Non-blocking: symlinks inside a root aren't resolved before the confinement check. Upstream follows symlinked skill dirs anyway, and skills only load SKILL.md files, so that's acceptable. Say so in the doc comment.

Merge condition: fork-ci green on this head.

@thedancingdeveloper thedancingdeveloper left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Review: no blockers (head 10b2590)

36e20d4 is unchanged (=). The one new commit fills the two new SkillsConfig fields (extra_user_roots, extra_project_roots) in an existing config test's struct literal, which is the workspace-check fix. The earlier review stands. It's based on 219664f, one commit behind fork/main (#15's workflow-only change), so no conflict. Merge condition: fork-ci green on this head.

@thedancingdeveloper thedancingdeveloper left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Review: no blockers (head fefebec)

This is a rebase onto fork/main ef97503, which includes #17's workspace bin build. git range-diff against the previously reviewed head shows every commit identical (=): 2 commits. Merge condition: fork-ci green on this head.

@thedancingdeveloper thedancingdeveloper left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Review: no blockers (head 6bf23d2)

On fork/main ef97503. The two reviewed commits are identical (=). The one new commit, 6bf23d2, regenerates core/config.schema.json for the two new [skills] fields, extra_project_roots and extra_user_roots (string arrays, documented). It's the generated-schema counterpart of the config change and nothing else. Merge condition: fork-ci green on this head, under whatever test step is current when it runs.

extra_user_roots and extra_project_roots add skill directories beside the
built-in ones, which are unchanged when the keys are absent. Each value is
relative to its base directory and dropped when it resolves outside it, so
the config cannot point skill loading at an arbitrary path.

WI-1133
SkillsConfig gained extra_user_roots and extra_project_roots, and the
bundled-skills assertion builds one by hand. Both default to empty.

WI-1133
extra_user_roots and extra_project_roots are part of SkillsConfig, so the
committed config.schema.json fixture has to include them.

WI-1133
Set project_root_markers = [] via a User config layer so the test does
not depend on ancestor directories of the temp dir (e.g. a real .git).
@thedancingdeveloper

Copy link
Copy Markdown
Author

no blockers (head f8ce933)

Epic lead. It's on fork/main b8011f5. git range-diff shows efb09e1, b8324fa and a2d734c identical to the reviewed 8602ce3, fefebec and 6bf23d2. The new commit f8ce933 is test-only: it sets project_root_markers = [] through a User config layer so upstream's repo_ancestry_without_project_marker_does_not_walk_parents doesn't depend on a .git above the temp dir. That's a real hermeticity fix, in a file this PR already touches, and in its own commit. Non-blocking: add it to FORK.md's carried-patch table as a candidate to offer upstream. fork-ci is green on this head (run 38041033191).

@thedancingdeveloper

Copy link
Copy Markdown
Author

Superseded: these commits were cherry-picked into #22 (fork features, merged to fork/main as c2d74d2) and ship in release fork-c2d74d2d9. Closing so the work isn't merged twice. The branch is kept.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant