feat: Phase 1 — core add_from_origin + reindex seams, CLI migrated#213
Merged
Conversation
Inject points on FastSkillService (embedding_service, repository_manager + builders/accessors) and stub the two seams: - core/install.rs: AddMode/AddOutcome/UpdatePreflight/Fetched + add_from_origin = commit(fetch(origin), mode) + preflight (bodies TODO). - core/reindex.rs: ReindexProgress/ReindexOutcome + reindex(skills_dir,observer) (skips silently without a provider; real body TODO). Compiles green; seams not yet callable (return not-implemented). Bodies land next. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fill the Phase-1 core seams (built in parallel worktrees, merged): - reindex.rs: real reindex driven by the injected embedding provider (skip silently without one), hash-skip unchanged skills, prune stale index entries, emit per-skill progress. Moved from the CLI execute_reindex. - install.rs: fetch (4-variant dispatch → temp dir + Resolved) + commit (validate → atomic-move/symlink → manifest → lock → best-effort reindex) + preflight. add_from_origin = commit(fetch(origin), mode). - change_detection.rs: fix a test-isolation bug both agents hit — git subprocess calls inherited GIT_DIR/GIT_WORK_TREE from the pre-commit hook and a private CWD lock raced the shared DIR_MUTEX; harden with env_remove + shared mutex. Core green: build + clippy -D warnings + 478 tests. Not yet wired into the CLI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`upsert_manifest_and_lock` resolved the manifest/lock from the process cwd — correct for the CLI, but the `serve`/HTTP path has an arbitrary cwd and would write to the wrong project. Add an edge-injected `project_root` on FastSkillService; the seam uses it when present, else falls back to cwd (CLI behavior unchanged). Makes add_from_origin serve-ready for Phase 2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Edge injection (config.rs/context.rs): build + inject the embedding provider (from the API key; skip → reindex silent) and a RepositoryManager into the service before use. - reindex: thin wrapper over service.reindex(dir, observer); deletes the CLI's duplicate find/embed/index loop. (--force via cache-clear; --max-concurrent now a no-op — core reindex is sequential.) - update: preflight() per skill → add_from_origin(Update) when Updatable; else report the no-op reason. --check/--dry-run use preflight. - add: common path (single skill, project-level) → add_from_origin; --global and --recursive keep the pre-seam path (seam is project-level single-skill). - detect_skill_source → Origin, with the deferred fix: a URL ending in .zip is now Origin::ZipUrl (installs via add_from_origin) instead of mis-detected git (wiremock e2e test proves it). Known seam gaps, worked around non-destructively (follow-ups): groups not plumbed through add_from_origin (reapplied post-call); --global not seam-backed. Full workspace green: build + clippy -D warnings + 684 tests, 0 failures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Implements Phase 1 of spec 003 (browser skill management) — the core install/reindex seams + CLI migration — per ADR-0005.
What this does
Moves the single-skill install and reindex orchestration into
fastskill-core, behind one seam, and rewires the CLI to call it. The HTTPservepath (Phase 2) can now reach the same logic in-process.Core seams (
FastSkillService)add_from_origin(origin, mode) = commit(fetch(origin), mode)(ADR-0005):fetch— match-dispatched perOriginvariant (git clone / local dir-or-zip / remote zip / registry download) into a temp dir, capturing theResolvedfacts (commit SHA, version, checksum).commit— the shared pipeline: validate → atomic move/symlink into the skills dir → upsert Manifest → write Lock (origin+resolved) → best-effort reindex (a reindex error never fails the add). Ordering is skills-dir → manifest → lock → reindex so a partial failure only ever leaves a harmless orphan dir; recovery is idempotent re-run (§Q3).modegoverns only the id-conflict policy:Fresh→ 409-style error on an existing id;Update→ overwrite.preflight(origin)(§Q6) →UpToDate | Immutable | Updatable, gating the update re-fetch (immutable git tag/commit + editable local are no-ops).reindex(skills_dir, observer)— driven by the injected embedding provider; skips silently without one (ADR-0002); emits per-skill progress to an optional observer; hash-skips unchanged skills and prunes stale entries.with_embedding_service/with_repository_manager/with_project_root— the CLI/serve edge supplies the API-key-bearing provider, the resolved repos, and (for serve) the project root. Core never loads secrets or reads cwd for the served project.CLI migration
execute_reindex/execute_updateare now thin wrappers over the seams (the CLI's duplicate reindex loop is deleted);execute_addroutes the common single-skill project case throughadd_from_origin(with--global/--recursivekept on the pre-seam path — see gaps). Includes the Phase-0-deferred fix: a URL ending in.zipnow installs asOrigin::ZipUrlinstead of being mis-detected as git (proven by a wiremock e2e test).Built with parallel sub-agents + opus review
Core seams implemented by two sonnet agents in parallel isolated worktrees (reindex ∥ install), merged (disjoint files). An opus review of the merged core found one blocker —
commitresolved the manifest/lock from the process cwd (fine for the CLI, wrong forserve) — fixed via the injectedproject_root(bab1378). Everything else (commit atomicity/ordering, Fresh/Update, editable-symlink data-safety, best-effort reindex, the preflight approach) reviewed clean. (The agents also caught + fixed a real pre-existing test-isolation bug: git subprocess calls inheritedGIT_DIR/GIT_WORK_TREEfrom the pre-commit hook.)Known gaps (non-blocking, worked around non-destructively → follow-ups)
groupsisn't plumbed throughadd_from_origin/commit(always writes empty); the CLI reapplies--groupafter the seam call. Follow-up: add agroupsparam to the seam.--globalinstalls stay on the pre-seam path (the seam is project-level; no global-lock concept yet).--force(worked around via cache-clear) and--max-concurrent(now a no-op; core reindex is sequential) have no seam params.GitRef::Commit(clone-by-commit) returns a clear error — no clone-by-commit primitive yet (matches the pre-seam CLI limitation).Testing
cargo build --workspace --all-targets✅cargo clippy --workspace --all-targets -- -D warnings✅cargo test --workspace→ 684 passed, 0 failed ✅ (incl. new seam tests: local/git/zip/registry fetch, Fresh/Update, editable symlink, reindex skip/index/prune, the zip-url wiremock e2e).Behavior-preserving for all existing CLI commands. Next: Phase 2 (HTTP endpoints — the seams are now serve-ready).
🤖 Generated with Claude Code