Add remote git skill sources#29
Conversation
github-actions
Bot
added
needs-triage
needs-kind
needs-priority
needs-actor
needs-release-note
labels
kelos-bot
Bot
added
kind/api
release-note
and removed
needs-kind
needs-release-note
labels
github-actions
Bot
added
needs-release-note
release-note
and removed
release-note
needs-release-note
labels
|
/kelos review |
|
/kelos review |
|
🤖 Kelos Task Status Task |
![kelos-bot[bot]](https://avatars.githubusercontent.com/in/2965305?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Kelos Reviewer Agent @gjkim42
Review Summary
Verdict: COMMENT
Overall correctness: patch is incorrect
Scope: Adds git-backed remote skill sources, automatic cache materialization during render paths, validation, tests, and README documentation.
Findings Overview
| Priority | Count | File:Line | Summary |
|---|---|---|---|
| P0 | 0 | — | none |
| P1 | 1 | internal/core/fetch.go:68 | Remote skill symlinks can make render read files outside the cloned source |
| P2 | 0 | — | none |
| P3 | 0 | — | none |
Findings
Security
- [P1] internal/core/fetch.go:68 — Remote skill materialization accepts cloned repositories without rejecting symlinked subdirectories or files.
renderSkillslater walks the cache and usesos.ReadFile, which follows symlinks, so a remote skill can include a symlink such asnotes.txt -> /home/user/.ssh/id_rsaor make the configuredsubdira symlink and causekanon render,diff,status, orapplyto read and potentially print or write local files outside the cloned repository. Reject symlinks during materialization, or otherwise verify every rendered path resolves inside the materialized skill directory before reading it.
Key takeaways
- The schema and documentation direction match the issue scope, but remote sources need a stronger boundary before this is safe to ship.
- I did not run local validation commands, per the review instructions.
|
/kelos pick-up |
|
🤖 Kelos Task Status Task |
|
/kelos review |
|
🤖 Kelos Agent @gjkim42 Addressed the reviewer finding by rejecting symlinked remote skill roots/files during materialization and validating existing caches before reuse. Added regression tests for symlinked subdirs and files. Local |
|
/kelos review |
|
/kelos pick-up |
|
🤖 Kelos Task Status Task |
|
/kelos review |
|
🤖 Kelos Agent @gjkim42 Addressed the latest review feedback: fetch errors no longer include expanded source URLs, and remote skill cache installation reuses an existing valid cache instead of deleting or replacing it. Added regression tests for query-token redaction and concurrent cache winners. Local |
|
/kelos review |
|
🤖 Kelos Task Status Task |
There was a problem hiding this comment.
🤖 Kelos Reviewer Agent @gjkim42
Review Summary
Verdict: COMMENT
Overall correctness: patch is incorrect
Scope: Adds git-backed remote skill sources, lazy cache materialization during render paths, validation, symlink rejection, tests, and README documentation.
Findings Overview
| Priority | Count | File:Line | Summary |
|---|---|---|---|
| P0 | 0 | — | none |
| P1 | 1 | internal/core/fetch.go:115 | Remote materialization not-found errors are swallowed by status |
| P2 | 2 | internal/core/fetch.go:79 | Remote cache install errors omit the skill name |
| P2 | 2 | internal/core/render_test.go:253 | Git redaction tests do not exercise output redaction |
| P3 | 0 | — | none |
Findings
Correctness
- [P1] internal/core/fetch.go:115 — Remote source validation wraps missing materialized files with
%w, so a repo/ref/subdir that checks out but lacks the configured subdir orSKILL.mdreturns an error matchingos.ErrNotExist. Thestatuscommand currently ignores anyopts.planerror whereerrors.Is(err, os.ErrNotExist)is true (internal/cli/root.go:179), sokanon statuscan exit successfully after only printing source git status and never report that the remote skill could not render. That hides invalid source state for one of the render paths documented in this PR. Narrow the status ignore case to the missing Kanon config/home case, or return non-os.ErrNotExistmaterialization validation errors and add a status regression test.
Conventions
- [P2] internal/core/fetch.go:79 —
materializeRemoteSkillreturnsinstallMaterializedSkillerrors directly. On mkdir/rename/cache failures,installMaterializedSkillreturns raw filesystem errors with only temp/cache paths and a hash, not the skill name; in configs with multiple remote skills, CLI output does not identify which skill failed. Wrap install failures withskill %q sourceor make the install helper include the skill name on every returned error.
Tests
- [P2] internal/core/render_test.go:253 — The redaction tests call
git not-a-real-git-command, which never uses or echoes the repo argument, so they pass even ifredactSourceGitOutputreturns output unchanged. Add a direct test with output containing the expanded URL/userinfo/query, or use a failing git command that actually echoes the URL, so the fetch-output redaction path is covered.
Key takeaways
- The earlier symlink rejection and concurrent-cache replacement concerns appear addressed in the current code.
- I could not submit a
REQUEST_CHANGESreview because GitHub reports the authenticated account is the PR author, so this is submitted asCOMMENTwith overall correctness marked incorrect. - I did not run
make verifyormake test, per the review instructions.
|
/kelos pick-up |
|
🤖 Kelos Task Status Task |
|
🤖 Kelos Agent @gjkim42 Addressed the latest reviewer findings: Local |
|
/kelos review |
|
🤖 Kelos Task Status Task |
There was a problem hiding this comment.
🤖 Kelos Reviewer Agent @gjkim42
Review Summary
Verdict: COMMENT
Overall correctness: patch is incorrect
Scope: Adds git-backed remote skill sources, automatic cache materialization during render paths, validation, symlink rejection, tests, and README documentation.
Findings Overview
| Priority | Count | File:Line | Summary |
|---|---|---|---|
| P0 | 0 | — | none |
| P1 | 1 | internal/core/fetch.go:79 | Root remote skill caches can persist expanded URL credentials in .git/config |
| P2 | 0 | — | none |
| P3 | 0 | — | none |
Findings
Security
- [P1] internal/core/fetch.go:79 — When
subdiris omitted,sourcePathis the cloned repo root andinstallMaterializedSkillmoves the whole clone into.kanon/cache/sources. Git stores the supplied HTTPS remote URL in.git/config, so a config usingsource.url: https://user:${TOKEN}@host/repo.gitwrites the expanded token to disk even though fetch output is redacted. Strip.gitor copy only working-tree contents before installing root-level remote skills.
Key takeaways
- The earlier symlink, output-redaction, concurrent-cache, and status-surfacing findings appear addressed in the current diff.
- I did not run
make verifyormake test, per the review instructions. - I am submitting this as
COMMENTbecause the authenticated GitHub App appears to be the PR author and cannot request changes on its own PR.
|
/kelos pick-up |
|
🤖 Kelos Task Status Task |
|
🤖 Kelos Agent @gjkim42 /kelos review |
|
🤖 Kelos Agent @gjkim42 Addressed the latest reviewer finding by stripping Local |
|
/kelos review |
|
🤖 Kelos Task Status Task |
There was a problem hiding this comment.
🤖 Kelos Reviewer Agent @gjkim42
Review Summary
Verdict: COMMENT
Overall correctness: patch is correct
Scope: Adds optional git-backed remote skill sources, automatic cache materialization on render paths, validation, cache hardening, tests, and README docs.
Findings Overview
| Priority | Count | File:Line | Summary |
|---|---|---|---|
| P0 | 0 | — | none |
| P1 | 0 | — | none |
| P2 | 0 | — | none |
| P3 | 0 | — | none |
Findings
None.
Key takeaways
- The current diff addresses the earlier symlink, redaction, concurrent cache, status-surfacing, install-error, and root
.gitmetadata findings. - I resolved the stale
.git/configreview thread after verifying root clone metadata is stripped before cache install. - I did not run
make verifyormake test, per the review instructions. - I am submitting this as
COMMENTbecause the authenticated GitHub App appears to be the PR author and cannot self-approve.
|
/kelos squash-commits |
|
🤖 Kelos Task Status Task |
kelos-bot
Bot
force-pushed
the
kanon-task-21
branch
from
ea2bdaf to
853b7d3
Compare
|
Squash complete. Rebased onto |
1 participant
What type of PR is this?
/kind api
What this PR does / why we need it:
Adds optional git-backed remote sources for skills. Missing remote skill caches are materialized automatically during render paths, remote source shape is validated, symlinked remote skill content is rejected, root-level clone metadata is stripped before caching, expanded source URL details are redacted from fetch output, concurrent cache winners are reused, and remote materialization failures are surfaced by
status.Which issue(s) this PR is related to:
Fixes #21
Special notes for your reviewer:
Remote plugins are intentionally out of scope for this neutral schema change. Remote skill sources are treated as trusted content, but symlinks are rejected during materialization so render paths do not follow remote-controlled links outside the materialized source. Root-level remote skill caches remove
.gitbefore install so expanded clone URLs are not retained in.git/config. Concurrent materialization reuses an existing valid cache instead of replacing it, and cache install errors include the skill name for multi-skill configs.Does this PR introduce a user-facing change?