feat(003): Discover & Acquire — search + remote add + index (manifest→frontmatter)#8
Conversation
Combine roadmap 003 (search) and 004 (remote add) into one consumer-side MVP slice: discover a skill in the org library and vendor it straight from the remote library, no local checkout required. Completes the init -> search -> add -> verify loop. - spec.md: user-facing contract (US1 discover, US2 remote acquire, US3 reproducible --pin, US4 distinct/navigable failures), 24 FRs, 8 SCs, Assumptions A1-A6, edge cases. No transport/auth/git mechanics. - spec-tech.md: technical companion (input to /plan) — the 002 local-checkout seam being replaced, convention-version gate, fingerprint/pin semantics, failure taxonomy, network test tier, and the 7 open decisions deferred to /clarify. - checklists/requirements.md: quality gate, all items pass. Co-evolution deliverables baked in: FR-023 reconcile the origin-template catalog schema (currently drops tags); FR-024 update ROADMAP.md + ARCHITECTURE-v0.md with the 003+004 merge and local-vs-remote seam. Also adds .agents/commands/specledger.verify-workflow.md (workflow-based cross-artifact verification command). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Routed 4 high-uncertainty areas to spikes (S1 manifest format, S2 catalog lifecycle, S3 auth/token, S4 remote-git testing) and settled 2 decisions from reviewer comments: - Local-vs-remote (A1 SETTLED): the tool never caches a remote; origin is either a remote OWNER/REPO (fetched) or an explicit local path. Confirmed against 002 code (it overloaded OWNER/REPO as <consumerRepoRoot>/OWNER/REPO and ran git on a local working tree; no file://, no remote). - Pin recording: lock records commit + treeSha + resolved human-readable version/tag (humans reason about versions; verify uses treeSha). spec.md: added Clarifications/Session 2026-05-31 (6 Q->A), updated FR-008/011/013, Key Entities, Assumptions (A1 settled, A4 pending S3). spec-tech.md: §8 rewritten into firm decisions + spike backlog S1-S4; folded in skill.toml->frontmatter history as S1's working hypothesis (drop skill.toml for agentskills.io frontmatter + x-skillrig.* metadata). Resolved 8 reviewer comments; 12 left open as live spike trackers. Feature is NOT plan-ready until S1-S4 close. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ex in scope All four clarify-phase spikes complete (research/2026-05-31-*.md); findings folded into spec-tech.md §8b and propagated to the user-facing spec. Two outcomes expand scope: - S1 (manifest): migrate to agentskills.io frontmatter + metadata.x-skillrig.*; drop skill.toml. requires lives under x-skillrig (NOT allowed-tools). Small, in-slice (commit 1 of 003). Adds gopkg.in/yaml.v3 — a divergence from the "no new dependencies" note, flagged for FR-024. - S2 (catalog): skillrig index ships IN 003 (single-tip generator, full regenerate from HEAD, GC=YAGNI). Reverses the original consume-only lean — search is meaningless against a drifting hand-maintained catalog, and the generator is thin once S1's parser exists (AP-04 by construction). - S3 (auth): os.exec token order GH_TOKEN > GITHUB_TOKEN > `gh auth token`; failure classes split by stderr (exit 128); inject via git -c http.extraHeader; GHE deferred. Corrects architecture §8b.2's mise precedence claim. - S4 (testing): file:// bare repo for happy/integrity + existing commandContext stub seam for auth/unreachable/transient; no httptest. Add AuthError/ UnreachableError typed in the fetch layer. spec.md: added US5 (origin maintainer regenerates catalog), FR-025..028 (catalog generation), SC-009, A7/A8 (scope), updated FR-023/024, settled the local-vs-remote edge case. All 20 reviewer comments resolved. Feature is now PLAN-READY (/specledger.plan). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
/specledger.plan output, consuming the four completed spikes (no
re-investigation):
- plan.md: tech context (yaml.v3 accepted, shell git, no httptest),
Constitution Check (all gates pass; §III httptest/skill.toml touch-ups
flagged for FR-024), project structure (new pkg/skillcore/{fetch,catalog}.go
+ cli/{search,index}.go), 6-step build sequence, complexity tracking
(yaml.v3 + index-in-consume-only, both justified).
- research.md: D1-D7 consolidated from spikes S1-S4 (Decision/Rationale/
Alternatives) + prior work (001/002).
- data-model.md: SKILL.md frontmatter manifest (metadata.x-skillrig.*),
single-tip catalog, lock entry (no schema change — Version already carries
the resolved tag), typed errors (Auth/Unreachable/NotFound/IncompatibleConvention),
origin form classification; ground-truth samples per boundary.
- contracts/: search.md (Query), add-remote.md (Vendor Mutation + --pin),
index.md (origin-side generator).
- quickstart.md: US1-US5 -> TestQuickstart_* with output-shape asserts +
the two ground-truth oracles (fetched treeSha == raw git; index == committed).
- CLAUDE.md: Active Technologies updated (yaml.v3).
Plan-complete; ready for /specledger.tasks.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ending) Reviewer flagged the tag/git-tag collision (skillrig uses git tags for version pins, name-vSEMVER). Recorded in spec.md Clarifications: - search is query-first: positional [QUERY] matches name+description; topic narrowing is a secondary filter. - label concept renamed away from "tag" → metadata.x-skillrig.topics. - filter flag name open (--topic vs -f/--filter); matching algorithm, git-friendly searchable-index storage, flag surface, and any indexing library being settled in Spike S5 (search-index-architecture). New indexing/search deps pre-approved. Full artifact rename deferred to the S5 fold (one coherent pass). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Search/index architecture settled (research/2026-05-31-search-index-architecture.md): - Query-first: search [QUERY...] is case-insensitive token-AND substring over name+description+topics; deterministic order (relevance bucket then name); no fuzzy/semantic/TF-IDF (N6). --topic is a separate exact-string filter. - tag→topic rename (--topic, topics[], metadata.x-skillrig.topics): agentskills.io defines no such field, GitHub "topics" reinforces it, removes the git-tag/ version-pin collision. Flag --topic not --filter (one dimension, YAGNI). - Flat index.json + in-memory filter is sufficient (index structures YAGNI below ~10k docs). STDLIB ONLY — no search dependency (sahilm/fuzzy is the post-v0 escape hatch if forgiving matching is ever wanted). Swept across spec.md (US1, FR-002/002a, Topic entity, A3, US5 scenario), data-model.md (manifest/catalog topics + new §5b search matcher), contracts/ search.md (query semantics + ordering), quickstart.md (query/ordering tests), research.md (D8), spec-tech.md (S5 resolved + surface), plan.md. Reviewer comment resolved. Historical S1/S2 spike narrative left as recorded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lls) Folds the S5 agent's plan edits + S5 reference cleanup: - step 5 (search) reworded to the S5 matcher: stdlib-only SearchCatalog (token-AND substring over name+description+topics, fixed-bucket score then name) + exact-string --topic filter. - new step 6 "seed the origin with more skills" so search/--topic get real multi-entry TestQuickstart_* coverage: use `npx skills add --copy` (NOT `sl skill add` — errors outside a SpecLedger project, installs to .claude/skills/); commit only skills/<name>/, drop the ~25 dot-dir copies; enrich each SKILL.md with metadata.x-skillrig.* (else index emits no topics/version); regenerate via skillrig index. - co-evolution renumbered 6→7; Input/structure refs updated to S1–S5. Verified: both repos clean (agent tested in a temp dir, no stray artifacts). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Avoids the embedded-string parse fragility hit while authoring the 003 verification (a /* glob, unescaped backtick/apostrophe, or mis-counted paren in a long embedded prompt breaks the whole JS workflow script). - Add .specledger/templates/review-report-template.md (the report format the merge agent fills — single source of truth, inspectable/editable on disk). - Refactor .agents/commands/specledger.verify-workflow.md: the workflow script is now minimal — each reviewer reads an on-disk FEATURE_DIR/reviews/ _reviewer-brief.md (authored per-run, carries the SKILLS line + focus areas); the merge agent reads the report template. New execution step 4 authors the brief. Thunks built with an explicit for-loop (clearer paren balance than the nested parallel(Array.from(...)) one-liner). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
/specledger.verify-workflow (2 Opus reviewers + merge): 0 CRITICAL, 1 HIGH, 5 MEDIUM, 6 LOW, 2 INFO; 5 coverage gaps. Report saved to reviews/003-review.md; all findings remediated (C1 per the exact-match ==1 decision; "fix all now"). - C1 (HIGH): convention-version gate = exact-match ==1 (lower/absent now FAIL, not silently pass). data-model §2/§5, search.md step3, spec-tech.md:51 + TestQuickstart_SearchConventionBoundary. - C2: distinct typed NoSuchVersionError (not a NotFoundError message variant); test asserts the discriminator. - C3: deterministic --pin rule (bare semver→tag_scheme expand; else literal ref/SHA) + AddPinTagFormEquivalent test. - C4: tag→topic stragglers (spec-tech:33,102; index.md:42; research:13). - C5/C6/C8/C9: added AddHelpExamples, AddDryRun, IndexNotInOrigin, IndexMissingVersion quickstart scenarios (close the 5 coverage gaps). - C7: index reads skillrigConvention from .skillrig-origin.toml (not hardcoded). - C10: matcher name unified on Search(); C11: data-model D1–D8; C12: US1/US2 traceability FRs; C13: search.md help header; C14: plan enumerates the constitution §III/§IX touch-ups for the FR-024 sweep. Gate: go build ./... clean; every prior coverage gap now has a TestQuickstart_*. Scaffolding (_reviewer-brief/_report-template) removed. Clear for implement. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Implemented via /specledger.implement-workflow (11-agent pipeline, gated on
make check; the experiment skips the sl issue ledger). make check green:
golangci-lint 0 issues; all tiers pass uncached (integration 3.27s); 60
TestQuickstart_* scenarios.
Manifest migration (commit-1 of the slice):
- pkg/skillcore/manifest.go: SKILL.md YAML frontmatter via gopkg.in/yaml.v3
(drops skill.toml); metadata.x-skillrig.* (topics, version, namespace,
convention-version, requires). Fixtures migrated; skill.toml deleted.
skillcore (single impl, AP-04):
- catalog.go: Catalog/ParseCatalog/GenerateCatalog (reads convention from
.skillrig-origin.toml, C7), CheckConvention exact-match ==1 (C1), stdlib
deterministic Search (token-AND substring + bucket+name order, S5/D8).
- fetch.go: remote fetch (git clone --sparse), stderr→typed errors.
- git.go: Clone/FetchSparse + ResolveGitHubToken (GH_TOKEN>GITHUB_TOKEN>
gh auth token via os.exec; http.extraHeader injection — S3/D4).
- errors.go: AuthError/UnreachableError/NotFoundError/NoSuchVersionError(C2)/
IncompatibleConventionError + ClassifyGitError.
- add.go: remote path + --pin resolution (semver→tag_scheme else literal, C3).
CLI (presentation-only): search.go (Query), index.go (generator), extended
add.go (remote + --pin + navigational error mapping); registered in root.go.
Docs/skill co-evolution (FR-023/024): cli.md (search/index/remote-add surface),
ROADMAP + ARCHITECTURE-v0 (003+004 merge, frontmatter+yaml.v3, on-merge catalog,
constitution touch-up list), skillrig skill references/{search,index}.md + add.md.
(Separate skillrig-origin repo migration remains, noted as the residual FR-023.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0 CRITICAL / 0 HIGH. All 14 verify findings present in code+tests (spot-checked); make check green, 60 TestQuickstart_* pass. 3 tracked items before merge: FR-023 residual (skillrig-origin repo not yet frontmatter), Constitution IX skill evals not run, constitution.md §III/§IX amendment (team-approval, list recorded).
…pable Corrects the prior "0 CRITICAL" checkpoint. Cold-context Opus review found 3 CRITICAL + 1 HIGH (all verified): search never fetches a remote catalog (C1); remote add untestable — cloneURL hardcodes github, originPattern rejects file://, 10 remote tests t.Skip-ped, fetch path 0% (C2); --pin misclassifies missing/private repo as NoSuchVersion (C3); remote add skips the convention gate (H1); index.json/search --json emit PascalCase requires keys behind a circular oracle (M1). Root cause: no file://-or-local-path origin seam. Lesson: a test that exists != runs; grep t.Skip at checkpoint.
…LAUDE.md Per review of docs/ + skill + CLAUDE.md after the 003 manifest migration: - CLAUDE.md: explicit DEPRECATED note — skill metadata is SKILL.md frontmatter (agentskills.io + metadata.x-skillrig.*, yaml.v3); skill.toml removed; the [[requires]] TOML notation now means metadata.x-skillrig.requires; go-toml retained only for .skillrig config TOML. - ARCHITECTURE-v0.md: lint scores SKILL.md frontmatter (not skill.toml validity); OpenClaw nugget + open-Q11 reworded to the frontmatter manifest; §4.1 gains a [[requires]]→metadata.x-skillrig.requires notation note. Remaining skill.toml mentions are deliberate deprecation context (dropped/migrated). ROADMAP/cli.md/README/skill files already used frontmatter.
…+ tested Focused remediation workflow (4 phases, HARD gate: no skipped acceptance tests; 10 remote scenarios MUST run+pass). Independently verified: make check green, go test -count=1 all pass, 11 remote TestQuickstart_* RUN+PASS, 0 skipped acceptance scenarios, fresh `skillrig index` emits lowercase + unescaped requires. - FIX-1 (root): config.Origin gains a LOCAL form (Path + IsLocal()) accepting file:// + filesystem-path origins; pkg/skillcore cloneURL derives the transport from the form. Delivers FR-011 local-path mode AND unblocks the S4 file:// bare-repo test substrate. - C1: search.go fetches index.json remotely per call (skillcore.FetchCatalog), not os.ReadFile of a local checkout. - C3/M2: classifyFetchError only returns NoSuchVersionError on a failed --pin ref checkout (repo+skill exist), distinct from missing/private repo. - H1: acquireRemote gates skillrigConvention (gateRemoteConvention) before vendoring. - M1: Require gains json tags; catalog marshals with SetEscapeHTML(false) (>= no longer >=); committed index.json fixture regenerated lowercase+unescaped; IndexMatchesCommitted de-circularized (asserts lowercase keys + unescaped >=). - L1/L2: commit-SHA pin fetch; Origin populated on Auth/Unreachable. - Un-skipped + wired all 10 remote scenarios to a real file:// bare-repo substrate with the RAW-git treeSha oracle; new fetch_test.go + resolvePin/classify coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cold re-review verdict: the remote keystone is genuinely built + honestly
tested (file:// substrate is a real bare-repo clone, raw-git oracle, no
circular dependency). 4 findings, 0 CRITICAL/HIGH; F1/F2/F3 closed, F4
documented as a conscious won't-fix.
- F1 (MEDIUM): TestQuickstart_AddRemoteConventionMismatch — a convention-2
file:// origin makes remote `add` exit 1 (IncompatibleConvention) writing
nothing; pins gateRemoteConvention (H1) end-to-end. + newRemoteOriginConvention helper.
- F2 (MEDIUM, security): TestAuthConfigArgs (base64 x-access-token header
format) + TestClone_TokenInjectionArgvOrder (the -c http.extraHeader global
flag precedes `clone`; token never in the URL). New pkg/skillcore/auth_test.go.
- F3 (LOW): ClassifyGitError now maps local-git stderr ("Could not read from
remote repository", "does not appear to be a git repository") to
UnreachableError, so a missing/unreachable file:// origin renders the distinct
"could not reach" message. + TestClassifyGitError_LocalUnreachable.
- F4: accepted/won't-fix (asymmetric convention gating on the 002 local-checkout
add path; consistent with 002, operator-controlled, FR-006 remote path IS gated).
Gate: make check 0 lint; go test -count=1 all pass; new tests run+pass; only the
two git-on-PATH t.Skip guards remain. Session log updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review Summary by Qodo(Agentic_describe updated until commit c3e95c7)Discover & Acquire — search + remote add + index (manifest→frontmatter)
WalkthroughsDescription**End-to-end consumer loop implementation: init → search → add → verify** • **Search command** (skillrig search [QUERY] [--topic …]): Query-first discovery with token-AND substring matching over skill name/description/topics, deterministic ordering, reads remote index.json per call • **Remote add with pinning** (skillrig add [--pin ]): Fetches skill subtrees from GitHub origins via sparse git clone with token authentication, reproducible immutable pins (bare semver expansion or literal refs), convention version gating • **Index generator** (skillrig index): Origin-side catalog production from SKILL.md frontmatter, deterministic output for discovery • **Manifest migration**: Skills move from skill.toml to SKILL.md agentskills.io frontmatter with metadata.x-skillrig.* namespacing (YAML via gopkg.in/yaml.v3) • **Local origin support**: File-based origins with file:// URLs and path normalization for testing and local workflows • **Typed error classification**: AuthError, UnreachableError, NotFoundError, NoSuchVersionError, IncompatibleConventionError with origin identity and auth-flag population • **Remote git transport**: Credential injection via environment (not argv), sparse no-checkout clones, token resolution from GH_TOKEN/GITHUB_TOKEN/gh auth token • **Comprehensive test coverage**: 1546-line integration suite (11 remote TestQuickstart_* scenarios, 0 skipped), 691-line unit tests, golden fixture validation, determinism checks, injected git failure scenarios • **Documentation**: Architecture updates for frontmatter format, CLI design patterns, feature specification with 28 functional requirements and 5 research spikes Diagramflowchart LR
A["Consumer: search<br/>Query origin catalog"] -->|"token-AND<br/>deterministic"| B["Catalog<br/>index.json"]
B -->|"fetch remote<br/>or local"| C["Origin<br/>SKILL.md files"]
D["Consumer: add --pin"] -->|"resolve pin<br/>gate convention"| E["Remote fetch<br/>sparse clone"]
E -->|"GitHub token<br/>env injection"| F["Vendored skill<br/>tree-SHA + commit"]
G["Origin: index"] -->|"walk skills<br/>parse frontmatter"| C
G -->|"deterministic<br/>output"| B
H["Manifest migration"] -->|"TOML → YAML<br/>frontmatter"| C
File Changes1. test/searchindex_quickstart_test.go
|
Code Review by Qodo
1. Hardcoded page size stop
|
| require ( | ||
| github.com/pelletier/go-toml/v2 v2.3.1 | ||
| github.com/spf13/cobra v1.10.2 | ||
| gopkg.in/yaml.v3 v3.0.1 |
There was a problem hiding this comment.
1. Unapproved gopkg.in/yaml.v3 dependency 📘 Rule violation § Compliance
go.mod adds gopkg.in/yaml.v3, which is outside the approved dependency allowlist. This can introduce supply-chain and audit risk by expanding the transitive dependency surface.
Agent Prompt
## Issue description
`go.mod` introduces a new module dependency (`gopkg.in/yaml.v3`) that is not in the approved allowlist.
## Issue Context
Compliance rule 831401 allows new dependencies only for `github.com/spf13/cobra` and `github.com/pelletier/go-toml/v2`.
## Fix Focus Areas
- go.mod[5-9]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| func (r FetchRequest) cloneURL() string { | ||
| if r.RepoURL != "" { | ||
| return r.RepoURL | ||
| } | ||
|
|
||
| return "https://" + defaultGitHubHost + "/" + r.Owner + "/" + r.Repo + ".git" | ||
| } | ||
|
|
||
| // classifyFetchError turns a raw fetch failure into the renderable typed error | ||
| // the CLI branches on. It runs the shared ClassifyGitError mapping | ||
| // (Auth/Unreachable/NotFound), then applies the fetch-specific refinements | ||
| // ClassifyGitError cannot know on its own, all anchored on WHICH git phase failed | ||
| // (FIX-3 — the *fetchStepError tag): | ||
| // |
There was a problem hiding this comment.
2. fetchskill uses https clone 📘 Rule violation ⌂ Architecture
The new fetch path derives https://github.com/... clone URLs and runs git clone, which performs runtime network access. This violates the requirement that application code must not attempt any network calls under any condition.
Agent Prompt
## Issue description
Application code introduced in this PR performs runtime network access by cloning over HTTPS.
## Issue Context
Compliance rule 782313 disallows any runtime network access in application code (TCP/UDP/HTTP/DNS), regardless of destination.
## Fix Focus Areas
- pkg/skillcore/fetch.go[90-136]
- pkg/skillcore/git.go[108-136]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // This file holds the TestQuickstart_* integration suite for feature | ||
| // 003-search-remote, the scenarios that are exercisable end-to-end against the | ||
| // real binary today: the discover (search, US1) and catalog-generation (index, | ||
| // US5) groups, plus the add --help shape (US2 SC-008). Each maps 1:1 to a | ||
| // scenario in specledger/003-search-remote/quickstart.md. | ||
| // | ||
| // Like the 001/002 suites it builds the binary once (TestMain in | ||
| // quickstart_test.go) and execs it via run(). It reuses the 002 fixture | ||
| // helpers (git, copyTree, sampleOriginDir, pinnedGitEnv, originRepo, | ||
| // decodeJSON, requireKeys, countExampleLines) and the RAW-git oracle | ||
| // discipline: every fixture is bootstrapped and every expected value computed | ||
| // with raw git, NEVER through skillcore (Constitution III / research D11). | ||
| // | ||
| // SUBSTRATE NOTE (S4 / D6). The remote-acquisition group (US2 remote add, US3 | ||
| // --pin, US4 auth/unreachable failures) runs against a real file:// bare repo | ||
| // for the CLI's origin (FIX-1 gave config.ParseOrigin a local/file:// form and | ||
| // pkg/skillcore.cloneURL a file:// seam, so `add` with no local checkout clones | ||
| // a t.TempDir bare repo over a real git transport — offline, no github.com). | ||
| // newRemoteOrigin git-inits a working tree (committed + a v-tag), clones it | ||
| // --bare, and binds SKILLRIG_ORIGIN=file://<bare>; the RAW-git oracle reads the | ||
| // expected treeSha straight from that bare repo (never skillcore, D11). | ||
| // | ||
| // Injected git failures (US4 auth/unreachable/private-not-found) are produced | ||
| // at the integration tier by a fake `git` on the binary's PATH (fakeGitBin) that | ||
| // passes every command through to the real git EXCEPT `clone`, which it fails | ||
| // with a crafted (exit 128, stderr) — the integration analog of the | ||
| // pkg/skillcore commandContext stub seam (which, being an unexported field, is | ||
| // only reachable from a pkg/skillcore unit test). The clone-phase failure trips | ||
| // the catalog gate before any subtree is fetched, so the CLI renders the | ||
| // auth/unreachable/not-found class distinctly. The typed-class assertions for | ||
| // those classes live as unit tests in pkg/skillcore (TestClassifyFetchError), | ||
| // per the quickstart's own "unit-level via the stub seam" note. | ||
| package quickstart | ||
|
|
||
| import ( |
There was a problem hiding this comment.
3. Integration test lacks build tag 📘 Rule violation ⌂ Architecture
test/searchindex_quickstart_test.go adds integration-style tests (execing the real binary) without the required //go:build integration tag. This causes integration tests to run in default unit-test runs and violates the integration tagging requirement.
Agent Prompt
## Issue description
A new integration test file was added without the required `//go:build integration` build tag.
## Issue Context
These `TestQuickstart_*` tests execute the real built binary and use git substrate setup, which meets the definition of integration tests.
## Fix Focus Areas
- test/searchindex_quickstart_test.go[1-35]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // Materialize an off-tip ref (an arbitrary commit SHA, FIX-6). If ref is | ||
| // already a fetched tip (branch/tag), this fetch fails harmlessly; only the | ||
| // checkout below is authoritative for ref existence, so a fetch failure is | ||
| // classified with the clone phase, never as a missing version. | ||
| fetchArgs := append([]string{}, auth...) | ||
| fetchArgs = append(fetchArgs, "-C", dir, "fetch", "--depth", "1", "origin", ref) | ||
| _, _ = c.run(ctx, fetchArgs...) |
There was a problem hiding this comment.
4. Ignored c.run error return 📘 Rule violation ≡ Correctness
fetchSparseInto discards the error from c.run(...) when running git fetch, which can hide failures and lead to incorrect downstream behavior. Error-returning calls must be checked or explicitly suppressed with a justified //nolint:errcheck.
Agent Prompt
## Issue description
A call that returns an error is ignored via `_, _ = ...` without a justified suppression.
## Issue Context
The code comment says the fetch failure is "classified with the clone phase", but the implementation currently discards the error entirely.
## Fix Focus Areas
- pkg/skillcore/git.go[211-223]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| auth := authConfigArgs(token) | ||
|
|
||
| showArgs := append([]string{}, auth...) | ||
| showArgs = append(showArgs, "-C", tmpDir, "show", ref+":"+file) | ||
|
|
||
| out, err := c.run(ctx, showArgs...) | ||
| if err != nil { | ||
| return nil, &fetchStepError{step: stepCheckout, err: err} | ||
| } |
There was a problem hiding this comment.
6. Catalog fetch ignores @ref 🐞 Bug ≡ Correctness
pkg/skillcore fetches index.json via git show <ref>:index.json without first fetching <ref>, so origins configured as OWNER/REPO@branch can fail even when reachable. This breaks search and remote add convention-gating because FetchCatalog forwards Origin.Ref into FetchFile.
Agent Prompt
## Issue description
`FetchFile` clones and immediately runs `git show <ref>:<file>`; when `ref` is a non-default branch (e.g. origin configured as `OWNER/REPO@develop`), that ref commonly does not exist locally after clone, so the show fails.
## Issue Context
`FetchCatalog` passes the configured origin ref (or HEAD) to `FetchFile`, and origin parsing explicitly supports `OWNER/REPO[@REF]`.
## Fix Focus Areas
- pkg/skillcore/git.go[250-297]
- pkg/skillcore/catalog.go[97-114]
- internal/config/origin.go[111-150]
## Implementation notes
- In `FetchFile`, after `Clone(...)`, run an explicit `git fetch --depth 1 origin <ref>` (or equivalent) before `git show`.
- Prefer showing from a fetched, unambiguous revision (e.g. `FETCH_HEAD:<file>` after fetch) to avoid relying on local branch creation.
- Keep error phase classification consistent (`stepClone` vs `stepCheckout`) so callers still get correct typed errors.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…FIG env Qodo code review (3 bugs + 6 rule violations). Fixed the 4 real/actionable; declined 4 (documented decisions / false positive) — see PR reply. - #8 (security): token no longer passed via argv. authConfigArgs (-c http.extraHeader=…) → authConfigEnv injecting GIT_CONFIG_COUNT/KEY_0/VALUE_0 (git >=2.31) via cmd.Env, so the base64 credential is in the process environ, not ps-visible argv. Matches gh-cli's "token out of argv" posture (gh uses a credential helper; we use GIT_CONFIG env since we resolve the token ourselves). runEnv() added; Clone/fetchSparseInto/FetchFile use it. F2 tests rewritten: TestAuthConfigEnv + TestClone_TokenInjectionViaEnv (assert no token in argv, credential in GIT_CONFIG_VALUE_0 env). - #7: `search` no longer requires a git repo — repoRoot is optional; a non-git cwd / checkout-less remote falls through to FetchCatalog. New TestQuickstart_SearchRemoteFromNonGitDir (file:// origin from a plain tempdir). - #5: search declares Args: cobra.ArbitraryArgs (rule 782342). - #9: human description width 60 → 80 (rule 783450). - #4: investigated — no genuine ignored error (errcheck clean); no change. Gate: make check 0 lint; go test -count=1 all pass; new tests run+pass; only the two git-on-PATH t.Skip guards. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Qodo review triage — fixed
|
…loses #7) Resolves issue #7 (Deprecated Terminology review). Terminology patch, no principle changes (Sync Impact note added; version 2.1.0 → 2.1.1): - §III ground-truth: "skills/*/skill.toml walk" → "skills/*/SKILL.md frontmatter walk (skillrig index)"; mise [[requires]] → metadata.x-skillrig.requires (manifest moved to SKILL.md frontmatter in 003/S1). - §III testing tiers: GitHub "httptest + go-vcr" Unit boundary → the pkg/skillcore git exec-stub seam (skillrig shells git; the boundary is the git exec, not an HTTP API), with the file:// remote substrate — matching 003. - §IX: eval tooling path corrected to .agents/skills/skill-creator/scripts/run_eval.py. Completes the doc-scrub started in 18aebf2 (ARCHITECTURE/ROADMAP/CLAUDE.md). Also deactivated Qodo rule 782685 (integration build-tag; project uses ./test/ convention) and narrowed 782313 (network; carved out the consume-only fetch layer). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Closing without merging (branch State at close: the feature ( Why closing rather than merging now: the planned "seed the origin with more skills" step (plan.md step 6) was omitted from the implementation, so the fixture origin still has a single skill — Reopen / supersede once #9 (multi-skill fixture + end-to-end search-filter coverage) lands. |
|
(The earlier close was a misclick — merging now. Fixture multi-skill seeding tracked in #9 as a follow-up.) |
|
Code review by qodo was updated up to the latest commit c3e95c7 |
| page, total = 1, 0 | ||
| while True: | ||
| d = _request("GET", f"/rules?page={page}") | ||
| for r in d.get("rules", []): | ||
| blob = (r.get("name", "") + " " + (r.get("content") or "")).lower() | ||
| if needle in blob: | ||
| ids.add(_rid(r)) | ||
| total = d.get("totalCount", 0) | ||
| if page * 50 >= total or not d.get("rules"): | ||
| break | ||
| page += 1 |
There was a problem hiding this comment.
1. Hardcoded page size stop 🐞 Bug ≡ Correctness
cmd_find terminates pagination using page * 50 >= total, assuming the API returns exactly 50 rules per page; if the page size differs, find can stop early and silently miss matching rules. This makes find produce incomplete results while list paginates based on observed counts.
Agent Prompt
### Issue description
`cmd_find` uses a hardcoded `50` to decide when to stop paging the `/rules` catalog, which can prematurely stop scanning and miss matches.
### Issue Context
This script is intended to reliably resolve rule IDs from partial text; missing pages defeats that goal.
### Fix Focus Areas
- .agents/skills/qodo-manage-rules/scripts/qodo_rules.py[127-153]
### Suggested fix
Mirror `cmd_list`’s approach: keep a running count of rules seen (or use `len(rules_accumulated)`), and stop when `seen >= totalCount` or when the returned `rules` array is empty—do not assume a fixed page size.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Status flip 🚧→✅; 003 (search + remote add + index) shipped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What
The first end-to-end consumer loop —
init → search → add → verify— plus the origin-sideindexgenerator that keeps discovery honest. Merges roadmap 003 (search) + 004 (remoteadd).search [QUERY] [--topic …]— query-first (token-AND substring over name/description/topics, deterministic order), reads the origin'sindex.json(fetched per call for a remote origin).add <skill> [--pin <ref>]— fetches the skill subtree from a GitHub origin (sparsegitclone, token viaos.exec, no local checkout) with reproducible immutable pins; gates the convention version; writescommit+treeSha+resolvedversion.index— origin-side generator producing the catalog from each skill'sSKILL.mdfrontmatter.skill.tomltoSKILL.mdagentskills.io frontmatter +metadata.x-skillrig.*(gopkg.in/yaml.v3).skill.tomldropped.Process (SpecLedger, fully exercised)
Specify → Clarify (5 research spikes: manifest format, catalog lifecycle, auth/token, remote-git testing, search/index) → Plan → Verify-workflow (2 reviewers → 14 findings, all remediated) → Implement-workflow → Checkpoint → adversarial review (caught a green-but-hollow remote keystone: 10 skipped tests, no remote
search, nofile://seam) → Remediation (hard no-skip gate) → cold re-review ("real, not theater" — 0 CRITICAL/HIGH) → F1/F2/F3 closure.Design artifacts, spikes, and the full review trail live under
specledger/003-search-remote/(spec, spec-tech, plan, research/, data-model, contracts/, quickstart, reviews/003-review.md, sessions/…-checkpoint.md).Testing evidence
make check: golangci-lint 0 issues, all tiers pass.go test -count=1 ./...: pass; all 11 remoteTestQuickstart_*RUN+PASS, 0 skipped acceptance scenarios (only 2 git-on-PATH guards).file://bare-repo substrate (genuinegit clone, RAW-git tree-SHA oracle, no circular oracle, nohttptest).Related / residual (non-blocking)
skill.toml→SKILL.mdconfusion in ARCHITECTURE/ROADMAP/CLAUDE.md (CLAUDE.md gains an explicit deprecation note). The constitution.md §III/§IX terminology touch-ups remain (governance-gated amendment; list recorded in ARCHITECTURE/ROADMAP).skillrig/origin-template) frontmatter migration is committed locally (79d23f4), not yet pushed.skillrigskill not yet run.🤖 Generated with Claude Code