From b6468e05178bf71e4e888ef4e575f5ba8f77a8f5 Mon Sep 17 00:00:00 2001 From: Vincent De Smet Date: Fri, 29 May 2026 19:13:06 +0800 Subject: [PATCH 1/2] feat(001): support optional branch/ref in origin reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Origin references now accept an optional @REF suffix (OWNER/REPO[@REF]) to track a branch of the skills library; a bare OWNER/REPO is unchanged and tracks the default branch. Validation stays shape-only and offline, and the ref is stored combined in the single `origin` config key, round-tripping through the resolver with no structural change (Origin.Ref + String()). Amendment to spec 001 (FR-018–FR-020): adds the addendum, updates the contracts/data-model/quickstart, cli.md "Origin Reference Grammar" section, README, ARCHITECTURE §2d/R26 notes, and the skillrig-init skill + evals. Also includes the CLAUDE.md pre-release marker and ROADMAP branch-aware/aws notes. All amendment issues closed; make check green. Co-Authored-By: Claude Opus 4.8 (1M context) --- .agents/skills/skillrig-init/SKILL.md | 26 +++-- .agents/skills/skillrig-init/evals/evals.json | 12 +++ .../skillrig-init/evals/trigger-eval-set.json | 2 + CLAUDE.md | 4 +- README.md | 17 +++- docs/ARCHITECTURE-v0.md | 4 +- docs/ROADMAP.md | 3 +- docs/design/cli.md | 23 ++++- internal/cli/init.go | 10 +- internal/config/config_test.go | 43 +++++++++ internal/config/origin.go | 67 +++++++++---- internal/config/origin_test.go | 54 ++++++++++- internal/config/resolve_test.go | 32 ++++++- .../amendments/001-origin-ref-support.md | 95 +++++++++++++++++++ .../contracts/init.md | 12 +-- .../contracts/resolve.md | 1 + .../001-init-origin-resolution/data-model.md | 13 ++- .../001-init-origin-resolution/issues.jsonl | 6 ++ .../001-init-origin-resolution/quickstart.md | 14 +++ .../001-origin-ref-support-checkpoint.md | 69 ++++++++++++++ specledger/001-init-origin-resolution/spec.md | 8 +- test/quickstart_test.go | 54 +++++++++++ 22 files changed, 516 insertions(+), 53 deletions(-) create mode 100644 specledger/001-init-origin-resolution/amendments/001-origin-ref-support.md create mode 100644 specledger/001-init-origin-resolution/sessions/001-origin-ref-support-checkpoint.md diff --git a/.agents/skills/skillrig-init/SKILL.md b/.agents/skills/skillrig-init/SKILL.md index 8966efc..7c66ada 100644 --- a/.agents/skills/skillrig-init/SKILL.md +++ b/.agents/skills/skillrig-init/SKILL.md @@ -5,9 +5,10 @@ description: >- and understand how skillrig resolves the active origin. Use when the user wants to "point this repo at our skills library", "set the origin", configure where skills come from, set up `skillrig` in a repo, choose between a project vs global default origin, - use the `SKILLRIG_ORIGIN` environment variable, or fix a "no origin configured" / - "no origin given" error. Triggers on `skillrig init`, origin binding (OWNER/REPO), - and origin-resolution precedence questions. + track a specific branch of the origin (OWNER/REPO@branch), use the `SKILLRIG_ORIGIN` + environment variable, or fix a "no origin configured" / "no origin given" error. + Triggers on `skillrig init`, origin binding (OWNER/REPO[@REF]), pointing at a branch + of the skills repo, and origin-resolution precedence questions. license: MIT metadata: author: skillrig @@ -29,6 +30,13 @@ metadata: **consume-only** — it never creates or scaffolds an origin, never reaches the network, and binding the same origin twice is a no-op. +The origin reference is `OWNER/REPO[@REF]`. The optional `@REF` tracks a specific +**branch** of the library (e.g. `my-org/my-skills@staging`); omit it to track the +default branch. The `@REF` is validated for shape only (offline) — it is **not** checked +against the remote — and is stored combined in the single `origin` key. (Note: an +origin's `@ref` is a moving branch pointer; pinning an individual *skill* to an immutable +tag/SHA is a separate, later concern.) + It writes one of two config files: - **Project** (default): `.skillrig/config.toml` at the **git repository root** (located @@ -44,7 +52,7 @@ It writes one of two config files: | Flag | Purpose | When to use | |------|---------|-------------| -| `--origin OWNER/REPO` | The origin to bind | Always prefer passing it explicitly (scripts/agents) | +| `--origin OWNER/REPO[@REF]` | The origin to bind; optional `@REF` tracks a branch | Always prefer passing it explicitly (scripts/agents); add `@branch` to track a non-default branch | | `--global` | Write the per-user default instead of the repo config | Setting a fallback used across all your repos | | `--non-interactive` | Never prompt; fail fast if `--origin` is missing | CI/agents that must not block on input | | `--json` | Emit a complete result object on stdout | Machine consumption | @@ -93,16 +101,18 @@ present; branch on `written` to tell a fresh bind from an idempotent no-op: 1. **Bind a repo**: `skillrig init --origin my-org/my-skills` → run from anywhere in the repo; config lands at the repo root. -2. **Personal default**: `skillrig init --origin my-org/my-skills --global`. -3. **CI / agent**: pass `--origin` (or set `SKILLRIG_ORIGIN`) **and** `--non-interactive` +2. **Track a branch**: `skillrig init --origin my-org/my-skills@staging` → records the + origin pinned to the `staging` branch (stored as `origin = 'my-org/my-skills@staging'`). +3. **Personal default**: `skillrig init --origin my-org/my-skills --global`. +4. **CI / agent**: pass `--origin` (or set `SKILLRIG_ORIGIN`) **and** `--non-interactive` so the command never prompts. -4. **One-off override**: `SKILLRIG_ORIGIN=ci-org/ci-skills skillrig ` — no file edit. +5. **One-off override**: `SKILLRIG_ORIGIN=ci-org/ci-skills skillrig ` — no file edit. ## Error Handling | Symptom (stderr) | Cause | Fix | |------------------|-------|-----| -| `invalid origin "": expected OWNER/REPO` | Origin not in `OWNER/REPO` shape | Pass a valid `--origin my-org/my-skills` | +| `invalid origin "": expected OWNER/REPO[@REF]` | Origin (or its `@REF`) not in `OWNER/REPO[@REF]` shape | Pass a valid `--origin my-org/my-skills` (or `--origin my-org/my-skills@main`) | | `no origin given … non-interactive session (no TTY)` | `init` run without `--origin` and stdin is not a terminal | Pass `--origin OWNER/REPO` or set `SKILLRIG_ORIGIN` | | `no origin given … non-interactive mode requested (--non-interactive)` | `--non-interactive` set but no `--origin` | Pass `--origin OWNER/REPO` or set `SKILLRIG_ORIGIN` | | "no origin configured" from a later command | No source supplied an origin | Run `skillrig init --origin OWNER/REPO`, or set `SKILLRIG_ORIGIN`, or add a `--global` default | diff --git a/.agents/skills/skillrig-init/evals/evals.json b/.agents/skills/skillrig-init/evals/evals.json index c013001..d8f8ee7 100644 --- a/.agents/skills/skillrig-init/evals/evals.json +++ b/.agents/skills/skillrig-init/evals/evals.json @@ -33,5 +33,17 @@ { "id": "3.2", "text": "Offers concrete fixes: run `skillrig init --origin OWNER/REPO`, set SKILLRIG_ORIGIN, or set a --global default" }, { "id": "3.3", "text": "Notes the project config is found by walking up from the current directory (works from subdirectories)" } ] + }, + { + "id": 4, + "name": "track-a-branch", + "description": "User wants the repo to follow a specific branch of the skills library — should append @REF to the origin (OWNER/REPO@branch), not invent a separate flag.", + "prompt": "Our skills repo acme/agent-skills has a 'staging' branch we want to try before it merges. How do I point this repo at that branch with skillrig?", + "trap": "Model invents a non-existent flag (e.g. --branch/--ref), claims skillrig verifies the branch exists over the network, or conflates this with pinning an individual skill to a tag/SHA.", + "assertions": [ + { "id": "4.1", "text": "Recommends `skillrig init --origin acme/agent-skills@staging` (the @REF suffix on the single --origin string)" }, + { "id": "4.2", "text": "Does NOT invent a separate --branch/--ref flag; the ref rides inside the origin reference" }, + { "id": "4.3", "text": "Notes the @REF is validated for shape only / offline — existence on the remote is not checked" } + ] } ] diff --git a/.agents/skills/skillrig-init/evals/trigger-eval-set.json b/.agents/skills/skillrig-init/evals/trigger-eval-set.json index 79615bc..50166fc 100644 --- a/.agents/skills/skillrig-init/evals/trigger-eval-set.json +++ b/.agents/skills/skillrig-init/evals/trigger-eval-set.json @@ -5,6 +5,8 @@ { "query": "skillrig says 'no origin configured' — how do I fix that?", "should_trigger": true }, { "query": "What's the precedence between SKILLRIG_ORIGIN and the project config file?", "should_trigger": true }, { "query": "How do I set a personal default skills origin used across all my repos?", "should_trigger": true }, + { "query": "I want this repo to track the staging branch of our skills repo acme/agent-skills with skillrig — how?", "should_trigger": true }, + { "query": "Can skillrig point at a specific branch of the skills library instead of the default branch?", "should_trigger": true }, { "query": "Write a table-driven unit test for a Go parsing function.", "should_trigger": false }, { "query": "How do I rebase my feature branch onto main and resolve conflicts?", "should_trigger": false }, { "query": "Configure golangci-lint to enable the gosec linter.", "should_trigger": false } diff --git a/CLAUDE.md b/CLAUDE.md index 1bbda63..48663b9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2,9 +2,11 @@ This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. +> PRE-RELEASE MARKER: As long as this marker is present we NEVER PLAN BACKWARD COMPATBILITY. We are in rapid iteration and may make breaking changes to the CLI and/or skill contract at any time. ANY PLAN IGNORES BACKWARD COMPATIBILITY. + ## What this is -`skillrig` is a single, generic, **consume-only** Go CLI for pointing a repo (or a per-user default) at an **origin** — the `OWNER/REPO` that hosts an org's agent skills — and managing vendored skills from it. The same binary serves humans, agents, and CI. There is no `publish`/`login` and no write credential in the binary: GitHub is the authority plane ("publishing" = a PR to the origin). +`skillrig` is a single, generic, **consume-only** Go CLI for pointing a repo (or a per-user default) at an **origin** — the `OWNER/REPO[@REF]` that hosts an org's agent skills — and managing vendored skills from it. The same binary serves humans, agents, and CI. There is no `publish`/`login` and no write credential in the binary: GitHub is the authority plane ("publishing" = a PR to the origin). Two design documents are binding and override general instincts: - `.specledger/memory/constitution.md` — development principles (spec-first, quickstart-as-contract, YAGNI, skill–CLI co-evolution). diff --git a/README.md b/README.md index e3ba993..f78f1ac 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,8 @@ `skillrig` is a CLI for pointing a repository (or your per-user default) at an **origin** — the `OWNER/REPO` that hosts your team's agent skills — and resolving -which origin is active for any working directory. +which origin is active for any working directory. An origin reference is +`OWNER/REPO[@REF]`; the optional `@REF` tracks a specific branch. > NOTE: a skillrig **origin** is a GitHub repository with a determined structure, use the template repository provided to create your own origin. @@ -28,6 +29,14 @@ directory if you are not inside a git repo). `init` is **idempotent** and **consume-only**: it records an existing origin, never creates or scaffolds one, and binding the same origin twice is a no-op. +```sh +# Track a specific branch of the origin instead of its default branch. +skillrig init --origin my-org/my-skills@staging +``` + +The optional `@REF` is validated for shape only (offline) — it is not checked +against the remote. + ### Set a personal default ```sh @@ -84,6 +93,12 @@ The v0 `config.toml` holds a single key: origin = 'my-org/my-skills' ``` +To track a branch, the optional `@REF` is stored combined in the same key: + +```toml +origin = 'my-org/my-skills@staging' +``` + Unknown keys are ignored on read, so config added by later versions will not break this one. The full, extended `config.toml` structure is documented on the project docs website; see also [docs/design/cli.md](docs/design/cli.md) for the CLI design contract. diff --git a/docs/ARCHITECTURE-v0.md b/docs/ARCHITECTURE-v0.md index c92b842..96094d9 100644 --- a/docs/ARCHITECTURE-v0.md +++ b/docs/ARCHITECTURE-v0.md @@ -123,6 +123,8 @@ skillrig is a *framework* any org can adopt, not a single-org tool. Three pieces Lower-priority sources fill in only what higher ones omit (like git local > global > system) — so a contractor can target client-A's origin in one repo and client-B's in another, while keeping a personal global default. +**Origin reference shape — `OWNER/REPO[@REF]`.** The origin reference accepts an optional `@REF` suffix that tracks a **branch** of the library (e.g. `my-org/my-skills@staging`); omitted, it tracks the default branch. It is stored combined in the single `origin` key and validated shape-only/offline. This realizes the `@ref` half of the R26 identity grammar (§9b) for origins specifically — note the *origin's* `@ref` is a moving branch pointer, whereas a *skill's* pin (`add --pin`) is an immutable tag/SHA. The `[/path]` portion of the grammar remains future work. + **Config vs. lock — separate files, co-located dir.** Input and tool-output have different natures and must not share a file (you'd get merge conflicts on hand-edited bits whenever the tool rewrites generated bits — the reason npm splits `package.json` from `package-lock.json`). So: ``` .skillrig/ @@ -310,7 +312,7 @@ Verified against mise's GitHub backend docs (current as of early 2026). Three fi ## 9b. External sources, allowlist & audit (R26–R29) — v1+ governance, designed-for in v0 -**Identity grammar (R26) — v0 foundation.** Adopt the ecosystem-standard reference grammar `OWNER/REPO[/path]@ref` + skill name. This is the format both Vercel `npx skills` and GitHub `gh skill` use (`gh skill install github/awesome-copilot documentation-writer@v1.2.0`, with nested-path discovery via the `skills/*/SKILL.md` convention). Use it as the **single key** for: lockfile entries (§4.2), `index.json` rows, and allowlist entries. One grammar, three consumers — an allowlist entry is literally "a source-ref pattern that lock entries are permitted to match." +**Identity grammar (R26) — v0 foundation.** Adopt the ecosystem-standard reference grammar `OWNER/REPO[/path]@ref` + skill name. This is the format both Vercel `npx skills` and GitHub `gh skill` use (`gh skill install github/awesome-copilot documentation-writer@v1.2.0`, with nested-path discovery via the `skills/*/SKILL.md` convention). Use it as the **single key** for: lockfile entries (§4.2), `index.json` rows, and allowlist entries. One grammar, three consumers — an allowlist entry is literally "a source-ref pattern that lock entries are permitted to match." *(Implemented so far: the origin reference realizes `OWNER/REPO[@REF]` with `@REF` as a branch pointer — see §2d; `[/path]` and lock/allowlist consumers are still future work.)* **Allowlist (R27) — policy data in the monorepo.** Author an `allowlist` section consumed by the `index.json` build (or a sibling `policy.toml`), listing permitted external sources at `OWNER/REPO[/path]` granularity, optionally pinned with `@ref`. Because it ships with the org-controlled source of truth, `doctor` evaluates "is this source permitted?" as a **deterministic offline lookup** (N6) — never inference. diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index aa9061e..ba1adc0 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -30,7 +30,7 @@ resolver — AP-04 / AP-06) and layers thin commands on top. |---|----------------|---------|------------|--------| | 001 | **`init` + origin resolution** — `env SKILLRIG_ORIGIN > .skillrig/config.toml > ~/.config/skillrig/config.toml`; `skillrig init [--origin] [--global]` binds an existing origin (never bootstraps) | Environment | — (project skeleton) | 🚧 | | 002 | **`skillcore` + `verify`** — git tree-SHA + `skill.toml` manifest parse; offline label-honesty + orphan check; exit codes 0/2/3 | Verification Gate | 001 | ⬜ | -| 003 | **`search`** — read committed `index.json`, deterministic tag filter, Two-Level Output | Query | 001 | ⬜ | +| 003 | **`search`** — read origin (branch aware) committed `index.json`, deterministic tag filter, Two-Level Output | Query | 001 | ⬜ | | 004 | **`add`** — vendor a skill subtree + write lock entry; `--dry-run`, refuse-clobber-without-`--force` | Vendor Mutation | 002 | ⬜ | | 005 | **backing-CLI prereqs** — `[[requires]]` declare + verify (`--eligible`-style readiness, auth-as-distinct-failure R18); mise consumption via per-CLI tagged releases + template-generated `mise.toml` | (extends verify/doctor) | 002 | ⬜ | | 006 | **`doctor`** — superset health check (integrity + prereqs + auth) | Environment | 002, 005 | ⬜ | @@ -38,6 +38,7 @@ resolver — AP-04 / AP-06) and layers thin commands on top. | 008 | **`global add` / `global verify`** — fetch/restore user-scope skills against the global lock | Global Management | 002 | ⬜ | | 009 | **multi-client materialization** — canonical `.agents/skills` + symlink views, copy-fallback (Windows/CI) | (supports add/global) | 004 | ⬜ | | 010 | **`lint`** — author-side conformance gate, required PR check on the origin | Verification Gate | 002 | ⬜ | +| 011 | **`aws`** — support AWS AgentRegistry hosted skills | Evolution | 002 | ⬜ | **Cross-cutting v0 commitments** (architecture §13): - Two scopes only — project (vendored, verify-only) + global (fetch/restore). **No "shared" middle tier.** diff --git a/docs/design/cli.md b/docs/design/cli.md index 7a32a20..709e55e 100644 --- a/docs/design/cli.md +++ b/docs/design/cli.md @@ -18,7 +18,7 @@ Quick reference for contributors. Every `skillrig` subcommand must satisfy these 4. **JSON output is complete** — full data, pipeable to `jq`, no truncation 5. **Errors to stderr, data to stdout** — enables clean piping 6. **Positional args for simple cases** — reserve flags for optional/complex params -7. **Origin is resolved, never baked in** — env > project config > global default (§2d of architecture.md); `--origin` overrides +7. **Origin is resolved, never baked in** — env > project config > global default (§2d of architecture.md); `--origin` overrides. The origin reference is `OWNER/REPO[@REF]`; `@REF` is optional and tracks a branch (see [Origin Reference Grammar](#origin-reference-grammar)) 8. **Classify against a pattern before merging** — see [Pattern Classification](#pattern-classification) and the [pattern-gate checklist](checklist-template.md) 9. **The CLI is consume-only** — no `publish`, no `login`, no write credential in the binary (architecture §2b). GitHub is the authority plane. 10. **Standard flags everywhere** — see [Standard Flags](#standard-flags): `--json` + `--verbose` on every command; mutating commands also take `--dry-run` and refuse to clobber divergent content without `--force` @@ -230,6 +230,27 @@ A small set of flags carry the same meaning across every command, so an agent ca --- +## Origin Reference Grammar + +The origin reference — the value of `--origin`, `SKILLRIG_ORIGIN`, and the `origin` key in `.skillrig/config.toml` — is `OWNER/REPO[@REF]`. The `@REF` suffix is **optional**: omitted, the origin tracks the library's default branch; supplied, it tracks a specific **branch** (e.g. a staging line of the skills library). Validation is **shape-only and offline** — like the rest of `init`, it never checks that the repo or ref actually exists (that is a future `doctor`/`verify`/`add` concern). + +``` +skillrig init --origin my-org/my-skills # default branch +skillrig init --origin my-org/my-skills@staging # track the 'staging' branch +``` + +This realizes the `@ref` half of the ecosystem-standard identity grammar `OWNER/REPO[/path]@ref` (architecture R26) that `gh skill` (`gh skill install github/awesome-copilot documentation-writer@v1.2.0`) and Vercel `npx skills` use. The `[/path]` portion remains future work. + +**Two meanings of `@ref`, kept distinct.** For an **origin**, `@REF` is a *moving pointer* — a branch you track and re-resolve. For a **skill** vendored via `add` (`skillrig add --pin `), the ref is an *immutable* pin — a tag or commit SHA, recorded in the lock so the vendored content is reproducible. Same grammar, opposite intent: the origin says "where to look (and which line of development)"; the pin says "exactly which reviewed bytes." Docs and help text must not conflate them. + +### Why a single `@ref` string, not a separate flag + +The branch rides *inside* the one origin string rather than in a separate `--branch`/`--ref` flag, and is stored combined in the single `origin` config key (`origin = 'my-org/my-skills@staging'`). Rationale: + +- **One key, three consumers (R26).** The same reference is the key for config, `index.json` rows, and (later) allowlist/lock entries. A single canonical string keeps those aligned; splitting owner/repo from ref into parallel fields invites drift and a "what wins if both are set?" ambiguity. +- **Ecosystem familiarity.** `@ref` matches `gh skill`, `npx skills`, npm (`pkg@version`), and Go modules (`mod@version`) — an agent transfers the form without re-reading `--help`. +- **No new flag surface.** `--origin` stays the single way to name an origin; the grammar carries the optional precision. (A `#`-separator — git/npm git-dep style — was considered and rejected for weaker ecosystem alignment with the R26 grammar already adopted.) + ## Pattern Classification Every `skillrig` subcommand MUST identify which pattern(s) it follows. This classification drives design constraints and review expectations. See the [pattern-gate checklist](checklist-template.md) for the per-command gate used at PR review time. diff --git a/internal/cli/init.go b/internal/cli/init.go index c45ec00..23801fb 100644 --- a/internal/cli/init.go +++ b/internal/cli/init.go @@ -14,10 +14,10 @@ import ( // originPromptLabel is the single interactive prompt (research D5: one stdlib // bufio read on stderr, no retry loop). -const originPromptLabel = "Origin (OWNER/REPO): " +const originPromptLabel = "Origin (OWNER/REPO[@REF]): " // missingOriginFix is the shared fix line for the no-origin error paths. -const missingOriginFix = "fix: pass --origin OWNER/REPO (e.g. --origin my-org/my-skills) or set SKILLRIG_ORIGIN" +const missingOriginFix = "fix: pass --origin OWNER/REPO[@REF] (e.g. --origin my-org/my-skills or --origin my-org/my-skills@main) or set SKILLRIG_ORIGIN" // initCmd holds the init command's flags and its injectable seams. Production // uses the os-backed defaults; tests inject deterministic stubs (interactivity, @@ -52,11 +52,15 @@ func newInitCmd(opts *globalOpts) *cobra.Command { Short: "Bind this repo (or your global default) to an origin", Long: "Bind a repository — or your per-user global default — to an existing origin,\n" + "the OWNER/REPO that hosts your team's agent skills, by recording it in config.\n" + + "Append @REF to track a specific branch (e.g. my-org/my-skills@staging); omit it\n" + + "to track the origin's default branch.\n" + "init is idempotent and consume-only: it does not create or scaffold an origin.\n\n" + "Without --origin, init prompts on an interactive terminal; with --non-interactive\n" + "(or no TTY) it fails fast instead of prompting, so scripts and agents never block.", Example: " # Bind the current repo to an existing origin\n" + " skillrig init --origin my-org/my-skills\n\n" + + " # Track a specific branch of the origin\n" + + " skillrig init --origin my-org/my-skills@staging\n\n" + " # Set your personal default origin (used when a repo has none)\n" + " skillrig init --origin my-org/my-skills --global", Args: cobra.NoArgs, @@ -65,7 +69,7 @@ func newInitCmd(opts *globalOpts) *cobra.Command { }, } - cmd.Flags().StringVar(&ic.origin, "origin", "", "origin to bind, OWNER/REPO (prompted if omitted on a TTY)") + cmd.Flags().StringVar(&ic.origin, "origin", "", "origin to bind, OWNER/REPO[@REF] where @REF tracks a branch (prompted if omitted on a TTY)") cmd.Flags().BoolVar(&ic.global, "global", false, "write the per-user global default instead of the repo config") cmd.Flags().BoolVar(&ic.nonInteractive, "non-interactive", false, "never prompt; fail fast if --origin is missing") diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 84e1232..07cf8c8 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -35,6 +35,49 @@ func TestSaveLoadRoundTrip(t *testing.T) { } } +// TestSaveLoadRoundTripWithRef verifies an origin carrying an @ref round-trips +// through Save/Load inside the single `origin` key — the ref is stored combined +// (origin = 'my-org/my-skills@main'), so no config-schema change is needed +// (amendment 001-origin-ref-support). +func TestSaveLoadRoundTripWithRef(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + path := filepath.Join(dir, configDirName, configFileName) + origin := Origin{Owner: "my-org", Repo: "my-skills", Ref: "main"} + + if err := Save(path, origin); err != nil { + t.Fatalf("Save: %v", err) + } + + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read written file: %v", err) + } + + if string(raw) != "origin = 'my-org/my-skills@main'\n" { + t.Errorf("written config = %q, want origin = 'my-org/my-skills@main'", raw) + } + + got, err := Load(path) + if err != nil { + t.Fatalf("Load: %v", err) + } + + if got.Origin != origin.String() { + t.Errorf("round-trip origin = %q, want %q", got.Origin, origin.String()) + } + + parsed, err := ParseOrigin(got.Origin) + if err != nil { + t.Fatalf("ParseOrigin(%q): %v", got.Origin, err) + } + + if parsed.Ref != "main" { + t.Errorf("parsed ref = %q, want main", parsed.Ref) + } +} + // TestSaveMatchesFixture anchors Save's byte-for-byte output to the committed // ground-truth fixture (Constitution III). go-toml/v2 emits TOML literal // strings (single-quoted) for values needing no escaping; the fixture is the diff --git a/internal/config/origin.go b/internal/config/origin.go index ee548fa..3c2bb3f 100644 --- a/internal/config/origin.go +++ b/internal/config/origin.go @@ -9,41 +9,74 @@ import ( "strings" ) -// originPattern is the offline shape check for an origin: two non-empty, -// slash-separated segments over the GitHub owner/repo charset (research D6). -// Existence/reachability is intentionally not checked here. +// originPattern is the offline shape check for the OWNER/REPO part of an +// origin: two non-empty, slash-separated segments over the GitHub owner/repo +// charset (research D6). Existence/reachability is intentionally not checked +// here. var originPattern = regexp.MustCompile(`^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$`) -// Origin is an org's skill source in OWNER/REPO form. It is the single value -// this feature reads, validates, records, and resolves. +// refPattern is the offline shape check for the optional @REF: a non-empty +// string over the owner/repo charset plus '/', so branch names with slashes +// ("feature/x", "release/1.2") are accepted alongside tags ("v1.4.0") and +// commit SHAs. Like originPattern this is shape-only — whether the ref actually +// exists upstream is deliberately not checked (amendment 001-origin-ref-support, +// FR-019). It is general by design: detecting branch vs tag vs commit would need +// either heuristics that misfire on unusual names or a network lookup, both out +// of scope. +var refPattern = regexp.MustCompile(`^[A-Za-z0-9._/-]+$`) + +// originErrFmt is the shared usage-error format for a malformed origin. It names +// the expected shape (including the optional ref) and echoes the offending value +// (FR-012 / amendment FR-018). +const originErrFmt = "invalid origin %q: expected OWNER/REPO[@REF] (e.g. my-org/my-skills or my-org/my-skills@main)" + +// Origin is an org's skill source in OWNER/REPO[@REF] form. It is the single +// value this feature reads, validates, records, and resolves. Ref is optional: +// when set it pins the origin to a branch (for an origin, a moving pointer the +// consumer tracks — distinct from the immutable tag/SHA a *skill* is pinned to); +// an empty Ref means the origin's default branch. type Origin struct { Owner string Repo string + Ref string } -// String renders the origin as "Owner/Repo". The zero Origin (the SourceNone -// sentinel) renders as "" so a "no origin" result never stringifies to a -// misleading "/" that looks configured. +// String renders the origin as "Owner/Repo", appending "@Ref" when a ref is +// set. The zero Origin (the SourceNone sentinel) renders as "" so a "no origin" +// result never stringifies to a misleading "/" that looks configured. func (o Origin) String() string { if o.Owner == "" && o.Repo == "" { return "" } - return o.Owner + "/" + o.Repo + s := o.Owner + "/" + o.Repo + if o.Ref != "" { + s += "@" + o.Ref + } + + return s } // ParseOrigin trims surrounding whitespace and validates s against the -// OWNER/REPO shape. On failure it returns a usage error that names the expected -// format and echoes the offending value (FR-012). A blank string is rejected; -// callers that treat blank as "unset" (e.g. SKILLRIG_ORIGIN) must check before -// calling. +// OWNER/REPO[@REF] shape. The optional ref is split on the first '@' (the +// owner/repo charset excludes '@', so the split is unambiguous) and validated +// against refPattern; a trailing '@' with no ref is rejected. On failure it +// returns a usage error that names the expected format and echoes the offending +// value (FR-012). A blank string is rejected; callers that treat blank as +// "unset" (e.g. SKILLRIG_ORIGIN) must check before calling. func ParseOrigin(s string) (Origin, error) { trimmed := strings.TrimSpace(s) - if !originPattern.MatchString(trimmed) { - return Origin{}, fmt.Errorf("invalid origin %q: expected OWNER/REPO (e.g. my-org/my-skills)", s) + + ownerRepo, ref, hasRef := strings.Cut(trimmed, "@") + if !originPattern.MatchString(ownerRepo) { + return Origin{}, fmt.Errorf(originErrFmt, s) + } + + if hasRef && !refPattern.MatchString(ref) { + return Origin{}, fmt.Errorf(originErrFmt, s) } - owner, repo, _ := strings.Cut(trimmed, "/") + owner, repo, _ := strings.Cut(ownerRepo, "/") - return Origin{Owner: owner, Repo: repo}, nil + return Origin{Owner: owner, Repo: repo, Ref: ref}, nil } diff --git a/internal/config/origin_test.go b/internal/config/origin_test.go index 9da02fd..fa78352 100644 --- a/internal/config/origin_test.go +++ b/internal/config/origin_test.go @@ -14,10 +14,16 @@ func TestParseOrigin(t *testing.T) { wantErr bool wantOwner string wantRepo string + wantRef string }{ {name: "valid", in: "my-org/my-skills", wantOwner: "my-org", wantRepo: "my-skills"}, {name: "valid with dots and underscores", in: "my.org_1/skills.v2_x", wantOwner: "my.org_1", wantRepo: "skills.v2_x"}, {name: "surrounding whitespace trimmed", in: " my-org/my-skills\n", wantOwner: "my-org", wantRepo: "my-skills"}, + {name: "branch ref", in: "my-org/my-skills@main", wantOwner: "my-org", wantRepo: "my-skills", wantRef: "main"}, + {name: "tag ref", in: "my-org/my-skills@v1.4.0", wantOwner: "my-org", wantRepo: "my-skills", wantRef: "v1.4.0"}, + {name: "commit ref", in: "my-org/my-skills@9f2c1a0", wantOwner: "my-org", wantRepo: "my-skills", wantRef: "9f2c1a0"}, + {name: "branch ref with slash", in: "my-org/my-skills@feature/auth", wantOwner: "my-org", wantRepo: "my-skills", wantRef: "feature/auth"}, + {name: "ref with surrounding whitespace trimmed", in: " my-org/my-skills@staging\n", wantOwner: "my-org", wantRepo: "my-skills", wantRef: "staging"}, {name: "empty", in: "", wantErr: true}, {name: "blank whitespace", in: " ", wantErr: true}, {name: "no slash", in: "my-org-my-skills", wantErr: true}, @@ -25,6 +31,10 @@ func TestParseOrigin(t *testing.T) { {name: "missing owner", in: "/my-skills", wantErr: true}, {name: "too many segments", in: "my-org/team/skills", wantErr: true}, {name: "illegal char", in: "my org/my skills", wantErr: true}, + {name: "trailing at with empty ref", in: "my-org/my-skills@", wantErr: true}, + {name: "ref with space", in: "my-org/my-skills@bad ref", wantErr: true}, + {name: "ref with illegal char", in: "my-org/my-skills@bad:ref", wantErr: true}, + {name: "at without owner repo", in: "@main", wantErr: true}, } for _, tc := range tests { @@ -44,8 +54,8 @@ func TestParseOrigin(t *testing.T) { t.Fatalf("ParseOrigin(%q) unexpected error: %v", tc.in, err) } - if got.Owner != tc.wantOwner || got.Repo != tc.wantRepo { - t.Errorf("ParseOrigin(%q) = %+v, want {Owner:%q Repo:%q}", tc.in, got, tc.wantOwner, tc.wantRepo) + if got.Owner != tc.wantOwner || got.Repo != tc.wantRepo || got.Ref != tc.wantRef { + t.Errorf("ParseOrigin(%q) = %+v, want {Owner:%q Repo:%q Ref:%q}", tc.in, got, tc.wantOwner, tc.wantRepo, tc.wantRef) } }) } @@ -70,9 +80,43 @@ func TestParseOriginErrorEchoesValue(t *testing.T) { func TestOriginString(t *testing.T) { t.Parallel() - o := Origin{Owner: "my-org", Repo: "my-skills"} - if got := o.String(); got != "my-org/my-skills" { - t.Errorf("String() = %q, want %q", got, "my-org/my-skills") + tests := []struct { + name string + in Origin + want string + }{ + {name: "no ref", in: Origin{Owner: "my-org", Repo: "my-skills"}, want: "my-org/my-skills"}, + {name: "with ref", in: Origin{Owner: "my-org", Repo: "my-skills", Ref: "main"}, want: "my-org/my-skills@main"}, + {name: "with slash ref", in: Origin{Owner: "my-org", Repo: "my-skills", Ref: "feature/auth"}, want: "my-org/my-skills@feature/auth"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + if got := tc.in.String(); got != tc.want { + t.Errorf("String() = %q, want %q", got, tc.want) + } + }) + } +} + +// TestParseOriginStringRoundTrip pins that an origin with a ref survives the +// String() → ParseOrigin round-trip unchanged — the property that lets the ref +// be stored inside the single `origin` config key with no resolver change +// (amendment 001-origin-ref-support, FR-020). +func TestParseOriginStringRoundTrip(t *testing.T) { + t.Parallel() + + for _, in := range []string{"my-org/my-skills", "my-org/my-skills@main", "my-org/my-skills@feature/auth"} { + o, err := ParseOrigin(in) + if err != nil { + t.Fatalf("ParseOrigin(%q): %v", in, err) + } + + if got := o.String(); got != in { + t.Errorf("round-trip ParseOrigin(%q).String() = %q, want %q", in, got, in) + } } } diff --git a/internal/config/resolve_test.go b/internal/config/resolve_test.go index 568cf7a..d541c61 100644 --- a/internal/config/resolve_test.go +++ b/internal/config/resolve_test.go @@ -70,7 +70,9 @@ func TestResolveOrigin_Precedence(t *testing.T) { {name: "row4_global", globalOrigin: "personal/skills", wantOrigin: "personal/skills", wantSource: SourceGlobal, wantPathIsGlobal: true}, {name: "row5_project_beats_global", projectOrigin: "client-a/skills", globalOrigin: "personal/skills", wantOrigin: "client-a/skills", wantSource: SourceProject, wantPathIsProj: true}, {name: "row6_blank_env_is_unset", envOrigin: " ", projectOrigin: "my-org/my-skills", wantOrigin: "my-org/my-skills", wantSource: SourceProject, wantPathIsProj: true}, - {name: "row7_malformed_project_skipped", projectOrigin: "ignored", projectMalformed: true, globalOrigin: "personal/skills", wantOrigin: "personal/skills", wantSource: SourceGlobal, wantPathIsGlobal: true}, + {name: "row7_malformed_project_skipped", projectOrigin: "ignored", projectMalformed: true, globalOrigin: "personal/skills", wantOrigin: "personal/skills", wantSource: SourceGlobal, wantPathIsProj: false, wantPathIsGlobal: true}, + {name: "row8_project_ref_survives", projectOrigin: "my-org/my-skills@staging", wantOrigin: "my-org/my-skills@staging", wantSource: SourceProject, wantPathIsProj: true}, + {name: "row9_env_ref_survives", envOrigin: "ci-org/ci-skills@main", projectOrigin: "my-org/my-skills", wantOrigin: "ci-org/ci-skills@main", wantSource: SourceEnv}, } for _, tc := range rows { @@ -208,6 +210,34 @@ func TestResolveOrigin_InvalidShapeRecordsDiagnostic(t *testing.T) { } } +// TestResolveOrigin_MalformedRefRecordsDiagnostic verifies the @REF branch of +// the shape check end-to-end: a config whose OWNER/REPO is valid but whose @REF +// is malformed (here a space) is skipped with a diagnostic — resolution falls +// through to global, non-fatal (FR-004 applied to the ref, amendment +// 001-origin-ref-support). This exercises the refPattern reject path that the +// owner/repo-only invalid-shape test never reaches. +func TestResolveOrigin_MalformedRefRecordsDiagnostic(t *testing.T) { + t.Parallel() + + cwd := t.TempDir() + home := t.TempDir() + projPath := writeProject(t, cwd, "my-org/my-skills@bad ref", false) // valid TOML, bad ref shape + writeGlobal(t, home, "personal/skills") + + got, err := ResolveOrigin(cwd, mapEnv(map[string]string{"HOME": home})) + if err != nil { + t.Fatalf("malformed-ref project must not be fatal, got error: %v", err) + } + + if got.Source != SourceGlobal || got.Origin.String() != "personal/skills" { + t.Errorf("got %+v, want global personal/skills (project skipped)", got) + } + + if len(got.Diagnostics) != 1 || got.Diagnostics[0].Path != projPath { + t.Errorf("Diagnostics = %+v, want 1 entry for %s", got.Diagnostics, projPath) + } +} + // TestResolveOrigin_OriginlessNoDiagnostic verifies a parseable file that // simply lacks an origin key is a quiet fall-through (forward-compat), not a // diagnostic. diff --git a/specledger/001-init-origin-resolution/amendments/001-origin-ref-support.md b/specledger/001-init-origin-resolution/amendments/001-origin-ref-support.md new file mode 100644 index 0000000..48bb967 --- /dev/null +++ b/specledger/001-init-origin-resolution/amendments/001-origin-ref-support.md @@ -0,0 +1,95 @@ +# Amendment 001: Optional branch/ref in the origin reference (`OWNER/REPO[@REF]`) + +**Amends**: [spec.md](../spec.md) — *CLI Initialization & Origin Resolution* (feature `001-init-origin-resolution`) +**Created**: 2026-05-29 +**Status**: Accepted +**Tracking**: `SL-2f13c6` (feature) + child tasks, spec `001-init-origin-resolution` + +## Motivation + +The original feature fixed the origin reference at the bare `OWNER/REPO` shape and listed "branch, ref, tag, version, or commit pinning" under *Out of Scope*. A consumer cannot currently point a repo at a **non-default branch** of the skills library (e.g. a `staging` line under active review before it merges to the default branch). This amendment adds an **optional** branch/ref to the origin so a repo can track a specific branch, while preserving the offline, consume-only character of `init`. + +The chosen shape is the ecosystem-standard identity grammar already adopted as the v0 foundation in [architecture §9b R26](../../../docs/ARCHITECTURE-v0.md) — `OWNER/REPO[/path]@ref` — narrowed here to `OWNER/REPO[@REF]` (the `[/path]` portion remains future work). + +> **Supersedes**: the *Out of Scope* bullet "Collecting any onboarding metadata beyond the origin … no branch/ref/tag/version/commit pinning". Branch tracking on the **origin** is now in scope. Immutable pinning of an individual **skill** (tag/SHA via `add --pin`) remains out of scope for this feature. + +## Clarifications + +### Session 2026-05-29 + +- Q: How is the branch/ref passed — a separate flag, a single string with a separator, or something else? → A: A single string with an `@` separator (`--origin OWNER/REPO@REF`), matching the R26 grammar and the `gh skill` / npm / Go-module conventions. No new flag; stored combined in the single `origin` config key. (A separate `--branch` flag and a `#` separator were considered and rejected — see *Decision* D-A1.) +- Q: What does the ref accept — strictly a branch name, or any git ref? → A: General/shape-only. The ref is validated only for shape (a permissive charset, offline); the CLI does not try to detect branch-vs-tag-vs-commit (heuristics misfire on unusual names) and does not perform any network lookup. Semantically, for an *origin* the ref is intended as a **branch** (a moving pointer the consumer tracks). +- Q: Does `init` verify the origin/branch exists or that the user has access? → A: No — and this amendment does not change that. Origin handling is shape-only and offline (the only git call is a local `git rev-parse --show-toplevel` for the write target). Existence/reachability/auth remain deferred to future commands (`doctor`/`verify`/`add`). + +## Requirements (additions) + +Continuing the FR sequence from spec.md (last was FR-017): + +- **FR-018**: The origin reference MUST accept an optional `@REF` suffix — overall shape `OWNER/REPO[@REF]`. When `@REF` is omitted the origin tracks the library's default branch; when supplied it tracks that ref (intended use: a branch). This applies uniformly to `--origin`, `SKILLRIG_ORIGIN`, and the `origin` key in config. +- **FR-019**: The `@REF` MUST be validated **shape-only and offline**, consistent with FR-012's `OWNER/REPO` validation. The `@`-split is unambiguous (the owner/repo charset excludes `@`); a malformed ref — empty after `@`, or containing characters outside the permitted charset — is rejected as a usage error (exit 1) that echoes the offending value and names the expected `OWNER/REPO[@REF]` shape, **without writing config**. Existence/reachability of the ref is **not** checked. +- **FR-020**: The ref MUST be stored **combined within the single `origin` key** (`origin = 'OWNER/REPO@REF'`) — no new config key or struct field is introduced. The resolver returns the ref as part of the resolved origin unchanged; the value round-trips through `Origin.String()` so write and read stay symmetric and the precedence resolver needs no structural change. + +These extend (do not replace) FR-012: a bare `OWNER/REPO` remains valid and is the common case. + +## Data-model delta + +Amends [data-model.md](../data-model.md) → *Entities* → **Origin**: + +| Field | Type | Rules | +|-------|------|-------| +| `Owner` | string | non-empty, charset `[A-Za-z0-9._-]` | +| `Repo` | string | non-empty, charset `[A-Za-z0-9._-]` | +| `Ref` | string | **optional**; when present, charset `[A-Za-z0-9._/-]` (owner/repo charset plus `/` for branch names like `feature/x`). Empty = default branch. | + +- `ParseOrigin(s)` now: trims whitespace; splits on the first `@` into `OWNER/REPO` and `REF`; matches `OWNER/REPO` against `^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$` and, when an `@` was present, `REF` against `^[A-Za-z0-9._/-]+$`; returns the usage error `invalid origin %q: expected OWNER/REPO[@REF] (e.g. my-org/my-skills or my-org/my-skills@main)` on failure (FR-012/FR-018). +- `String()` renders `Owner/Repo`, appending `@Ref` only when `Ref != ""`. The zero Origin still renders `""` (SourceNone sentinel — unchanged). +- **Storage is unchanged structurally**: `Config{ Origin string }` and the single `origin` TOML key. A ref'd origin serializes as `origin = 'my-org/my-skills@staging'`. + +### Canonical fixtures + +- The base fixture `test/fixtures/config.toml` (`origin = 'my-org/my-skills'`) is **unchanged** — the ref-less form is the canonical default and keeps `TestSaveMatchesFixture` + the resolver matrix stable. +- The ref'd form (`origin = 'my-org/my-skills@staging'`) is anchored in-test by `TestSaveLoadRoundTripWithRef` and `TestQuickstart_BindWithRef` rather than a second committed fixture. + +### Precedence matrix delta + +The recorded matrix (rows 1–7) is unchanged in behavior; two rows are added to prove the ref survives resolution end-to-end: + +| # | `SKILLRIG_ORIGIN` | project config | global | → Resolved | → Source | +|---|---|---|---|---|---| +| 8 | – | ✓ `my-org/my-skills@staging` | – | `my-org/my-skills@staging` | `project` | +| 9 | ✓ `ci-org/ci-skills@main` | ✓ `my-org/my-skills` | – | `ci-org/ci-skills@main` | `env` | + +## Quickstart addition + +Amends [quickstart.md](../quickstart.md) → *Part A*: + +### TestQuickstart_BindWithRef (FR-018, FR-019, FR-020) +``` +$ skillrig init --origin my-org/my-skills@staging +``` +- **exit**: 0 +- **stdout** (human, compact): line 1 contains `bound origin my-org/my-skills@staging` and `project`; line 2 is the `→ resolve order:` footer hint. +- **file** `./.skillrig/config.toml`: + ```toml + origin = 'my-org/my-skills@staging' + ``` +- **`--json`** (variant): `origin == "my-org/my-skills@staging"`. +- **shape assert**: `len(stdoutLines) <= 2`. + +Coverage map adds: `BindWithRef → FR-018, FR-019, FR-020`. Resolver rows 8–9 fold into `TestResolveOrigin_Precedence`. + +## Contract deltas + +- [contracts/init.md](../contracts/init.md): `--origin` synopsis/flag becomes `OWNER/REPO[@REF]`; the malformed-origin error row names the `OWNER/REPO[@REF]` shape. Behavior step 3 (`ParseOrigin`) now also validates the optional ref. +- [contracts/resolve.md](../contracts/resolve.md): a resolved origin MAY carry a `Ref`; the resolver passes it through unchanged (no precedence/structural change — the ref lives inside the origin string). + +## Decision + +- **D-A1 — `@ref` single string over `--branch` flag / `#` separator.** Chosen for: alignment with the already-adopted R26 grammar (`OWNER/REPO[/path]@ref`) and the `gh skill` / `npx skills` / npm / Go conventions an agent already knows; the "one key, three consumers" property (config, future `index.json` rows, future allowlist/lock entries share one canonical key); and avoiding a "what wins if both `@ref` and `--branch` are set?" ambiguity. A `#` separator (git-URL / npm git-dep style) was rejected for weaker alignment with the grammar this project already committed to. +- **D-A2 — general, shape-only ref (no network, no branch-vs-tag-vs-commit detection).** Detection would require heuristics that misfire on unusual but legal ref names, or a network lookup that breaks the offline guarantee. Shape-only keeps `init` deterministic and offline; whether the ref exists is a later command's job. + +## Constitution alignment + +- **II — Quickstart-as-Contract**: `TestQuickstart_BindWithRef` is the executable acceptance test for this amendment; resolver rows 8–9 cover the resolution path. +- **III — Ground-Truth Anchoring**: the ref'd `config.toml` bytes are anchored by `TestSaveLoadRoundTripWithRef` (`origin = 'my-org/my-skills@staging'`), regenerated from `Save`. +- **IX — Skill–CLI Co-Evolution**: the `skillrig-init` agent skill is updated to mention the optional `@REF`/branch, with the eval re-run to confirm trigger accuracy holds. diff --git a/specledger/001-init-origin-resolution/contracts/init.md b/specledger/001-init-origin-resolution/contracts/init.md index 801bf95..903ed98 100644 --- a/specledger/001-init-origin-resolution/contracts/init.md +++ b/specledger/001-init-origin-resolution/contracts/init.md @@ -6,14 +6,14 @@ ## Synopsis ``` -skillrig init [--origin OWNER/REPO] [--global] [--non-interactive] [--json] [--verbose] +skillrig init [--origin OWNER/REPO[@REF]] [--global] [--non-interactive] [--json] [--verbose] ``` ## Flags | Flag | Type | Default | Meaning | |------|------|---------|---------| -| `--origin` | string | "" | Origin to bind, `OWNER/REPO`. If omitted: prompt in an interactive TTY; error in non-interactive. | +| `--origin` | string | "" | Origin to bind, `OWNER/REPO[@REF]`. The optional `@REF` tracks a branch (amendment [001-origin-ref-support](../amendments/001-origin-ref-support.md)). If omitted: prompt in an interactive TTY; error in non-interactive. | | `--global` | bool | false | Write the per-user global default (`$XDG_CONFIG_HOME/skillrig/config.toml` or `~/.config/skillrig/config.toml`) instead of the repo project config. | | `--non-interactive` | bool | false | Force non-interactive mode: never prompt. If required flags such `--origin` are omitted, fail (exit 1) instead of prompting, **even on an interactive TTY** (FR-006c). For scripts/agents that must not block on input. | | `--json` | bool | false | Emit the complete result object on stdout instead of compact human text. | @@ -40,7 +40,7 @@ Examples: 1. Resolve write target: `--global` → global config path; else the **git repo root** via `git rev-parse --show-toplevel` (offline) → `/.skillrig/config.toml`; if not inside a git repo, fall back to `./.skillrig/config.toml` in cwd (create `.skillrig/` if missing — FR-010). 2. Determine origin: `--origin` value; else if `--non-interactive` is set → usage error without prompting (FR-006c); else if interactive TTY → prompt once on stderr; else (no TTY) usage error (FR-006a). -3. `ParseOrigin` → on invalid shape, usage error, no write (FR-012). +3. `ParseOrigin` → splits the optional `@REF` and validates both parts shape-only/offline; on invalid shape, usage error, no write (FR-012 / FR-018, FR-019). A valid `@REF` is carried on the resolved origin and stored combined in the `origin` key (FR-020). 4. Load existing config at target (if any). Compare: - none present → write, `written=true`. - equal origin → no-op, `written=false` (idempotent, FR-008). @@ -69,9 +69,9 @@ Keys always present: `ok, origin, scope, configPath, written`. `scope ∈ {proje | Condition | Exit | Message shape | |-----------|------|---------------| -| `--origin` omitted, non-interactive **session** (no TTY) | 1 | what: no origin given; why: non-interactive session (no TTY); fix: pass `--origin OWNER/REPO` or set `SKILLRIG_ORIGIN`. | -| `--origin` omitted, `--non-interactive` **forced** (even on a TTY) | 1 | what: no origin given; why: non-interactive mode requested (`--non-interactive`); fix: pass `--origin OWNER/REPO` or set `SKILLRIG_ORIGIN`. | -| Malformed origin | 1 | what: invalid origin ``; why: expected `OWNER/REPO`; fix: e.g. `skillrig init --origin my-org/my-skills`. | +| `--origin` omitted, non-interactive **session** (no TTY) | 1 | what: no origin given; why: non-interactive session (no TTY); fix: pass `--origin OWNER/REPO[@REF]` or set `SKILLRIG_ORIGIN`. | +| `--origin` omitted, `--non-interactive` **forced** (even on a TTY) | 1 | what: no origin given; why: non-interactive mode requested (`--non-interactive`); fix: pass `--origin OWNER/REPO[@REF]` or set `SKILLRIG_ORIGIN`. | +| Malformed origin (incl. malformed `@REF`) | 1 | what: invalid origin ``; why: expected `OWNER/REPO[@REF]`; fix: e.g. `skillrig init --origin my-org/my-skills` or `--origin my-org/my-skills@main`. | | Config dir/file not writable | 1 | what: cannot write ``; why: ``; fix: check permissions / path. | Exit `0` on success (including idempotent no-op). Codes `2`/`3` not used by this command. diff --git a/specledger/001-init-origin-resolution/contracts/resolve.md b/specledger/001-init-origin-resolution/contracts/resolve.md index dc0ea0f..3b3945a 100644 --- a/specledger/001-init-origin-resolution/contracts/resolve.md +++ b/specledger/001-init-origin-resolution/contracts/resolve.md @@ -12,6 +12,7 @@ func ResolveOrigin(cwd string, env Env) (ResolutionResult, error) - `env` is an injected accessor (e.g. `func(key string) string`) so tests set `SKILLRIG_ORIGIN` deterministically without mutating process env (golang-testing: table-driven, parallel-safe). - Returns `ResolutionResult{Origin, Source, ConfigPath, Diagnostics}` (see data-model.md). `Diagnostics` carries every source that was present but skipped because it was unusable, so the cause is never silently swallowed. +- The resolved `Origin` MAY carry an optional `Ref` (amendment [001-origin-ref-support](../amendments/001-origin-ref-support.md)). The resolver applies **no** special handling: the ref lives inside the origin string each source supplies, so it passes through `ParseOrigin`/`String()` unchanged — precedence and structure are untouched. ## Precedence (FR-002) diff --git a/specledger/001-init-origin-resolution/data-model.md b/specledger/001-init-origin-resolution/data-model.md index 9c09dd6..3cda7f4 100644 --- a/specledger/001-init-origin-resolution/data-model.md +++ b/specledger/001-init-origin-resolution/data-model.md @@ -5,20 +5,21 @@ ## Entities ### Origin -The org's skill source, in `OWNER/REPO` form. The single value this feature reads, validates, records, and resolves. +The org's skill source, in `OWNER/REPO[@REF]` form. The single value this feature reads, validates, records, and resolves. | Field | Type | Rules | |-------|------|-------| | `Owner` | string | non-empty, charset `[A-Za-z0-9._-]` | | `Repo` | string | non-empty, charset `[A-Za-z0-9._-]` | +| `Ref` | string | **optional** (amendment [001-origin-ref-support](amendments/001-origin-ref-support.md)); charset `[A-Za-z0-9._/-]`; empty = default branch | -- Constructed via `ParseOrigin(s string) (Origin, error)`: trims whitespace, matches `^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$`. On failure returns a usage error naming the expected format and echoing the offending value (FR-012). -- `String()` renders `Owner/Repo`. +- Constructed via `ParseOrigin(s string) (Origin, error)`: trims whitespace, splits on the first `@` into `OWNER/REPO` and `REF`, matches `OWNER/REPO` against `^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$` and (when present) `REF` against `^[A-Za-z0-9._/-]+$`. On failure returns a usage error naming the expected `OWNER/REPO[@REF]` format and echoing the offending value (FR-012 / FR-018). +- `String()` renders `Owner/Repo`, appending `@Ref` when a ref is set. ### ProjectConfig → `.skillrig/config.toml` (committed, hand-editable INPUT) | Field | TOML key | Type | Notes | |-------|----------|------|-------| -| `Origin` | `origin` | string | `OWNER/REPO`; the only field this feature reads/writes | +| `Origin` | `origin` | string | `OWNER/REPO[@REF]`; the only field this feature reads/writes. The optional `@REF` is stored combined in this one key (`origin = 'my-org/my-skills@staging'`) — no separate key (amendment [001-origin-ref-support](amendments/001-origin-ref-support.md), FR-020) | > Forward-compatibility: unknown keys in `config.toml` are **ignored** on read (not an error), so fields added later (client targets, adoption policy — architecture §2d) don't break this version. Detailed/extended config structure is documented on the project docs website, not restated here (spec clarification). @@ -68,6 +69,8 @@ That is the entire v0 file: one `origin` key. The byte-for-byte output of `init` | 5 | – | ✓ `client-a/skills` | ✓ `personal/skills` | `client-a/skills` | `project` | | 6 | ✓ (blank) | ✓ `my-org/my-skills` | – | `my-org/my-skills` | `project` (blank env = unset) | | 7 | – | (malformed/unparseable file) | ✓ `personal/skills` | `personal/skills` | `global` (bad project source skipped, FR-004) | +| 8 | – | ✓ `my-org/my-skills@staging` | – | `my-org/my-skills@staging` | `project` (optional `@REF` survives resolution — amendment [001-origin-ref-support](amendments/001-origin-ref-support.md), FR-020) | +| 9 | ✓ `ci-org/ci-skills@main` | ✓ `my-org/my-skills` | – | `ci-org/ci-skills@main` | `env` (`@REF` survives) | Rows map directly to table-driven resolver unit tests and to quickstart precedence scenarios. @@ -87,7 +90,7 @@ No deletion, no other transitions in scope. | Rule | Where | Failure → | |------|-------|-----------| -| Origin matches `OWNER/REPO` | `ParseOrigin` (init write + resolved value) | usage error, exit 1, no write (FR-012) | +| Origin matches `OWNER/REPO[@REF]` (ref optional, shape-only) | `ParseOrigin` (init write + resolved value) | usage error, exit 1, no write (FR-012 / FR-018, FR-019) | | Blank `SKILLRIG_ORIGIN` = unset | resolver | fall through precedence | | Unparseable/origin-less config = "none from this source" | resolver | skip source, continue; clear diagnostic, not raw dump (FR-004) | | No origin in any source | resolver → caller | actionable "no origin configured" error, exit 1 (US3, FR-003) | diff --git a/specledger/001-init-origin-resolution/issues.jsonl b/specledger/001-init-origin-resolution/issues.jsonl index 9e02fa8..e9d2971 100644 --- a/specledger/001-init-origin-resolution/issues.jsonl +++ b/specledger/001-init-origin-resolution/issues.jsonl @@ -24,3 +24,9 @@ {"id":"SL-1819a2","title":"Full gate green: gofmt/vet/golangci-lint + go test ./...","description":"WHY: Feature DONE only when the whole quickstart suite + lint pass.","status":"closed","priority":3,"issue_type":"task","spec_context":"001-init-origin-resolution","created_at":"2026-05-24T23:30:52.535668+08:00","updated_at":"2026-05-25T14:51:21.707969+08:00","closed_at":"2026-05-25T14:51:21.707969+08:00","definition_of_done":{"items":[{"item":"gofmt/go vet/golangci-lint clean","checked":true,"verified_at":"2026-05-25T14:51:21.677925+08:00"},{"item":"go test ./... all green (quickstart suite passes)","checked":true,"verified_at":"2026-05-25T14:51:21.693237+08:00"}]},"labels":["spec:001-init-origin-resolution","phase:polish","component:ci"],"design":"Run gofmt -l (empty), go vet ./..., golangci-lint run (clean), go test ./... (all TestQuickstart_* + TestResolveOrigin_* green).","acceptance_criteria":"All lints clean; go test ./... green incl. full quickstart suite.","parentId":"SL-60678c"} {"id":"SL-804dc6","title":"Usage docs: README note referencing config docs website","description":"WHY: Document init/origin usage; config structure lives on the docs website (not restated).","status":"closed","priority":3,"issue_type":"task","spec_context":"001-init-origin-resolution","created_at":"2026-05-24T23:30:52.546297+08:00","updated_at":"2026-05-25T14:51:21.743429+08:00","closed_at":"2026-05-25T14:51:21.743429+08:00","definition_of_done":{"items":[{"item":"README usage note added","checked":true,"verified_at":"2026-05-25T14:51:21.719541+08:00"},{"item":"links to docs website for config structure","checked":true,"verified_at":"2026-05-25T14:51:21.73133+08:00"}]},"labels":["spec:001-init-origin-resolution","phase:polish","component:docs"],"design":"README/usage section: skillrig init examples, precedence order, SKILLRIG_ORIGIN; link to the docs website for full config.toml structure (per spec clarification).","acceptance_criteria":"README documents init + precedence + env override; links to docs website for config structure.","parentId":"SL-60678c"} {"id":"SL-3b4985","title":"Git-repo fixtures + git-root write-target integration tests","description":"WHY: init writes the project config at the git repo root (git rev-parse --show-toplevel, offline); git is a required dependency. Tests must prove the git-root write target and the non-git cwd fallback.","status":"closed","priority":1,"issue_type":"task","spec_context":"001-init-origin-resolution","created_at":"2026-05-25T10:29:44.632553+08:00","updated_at":"2026-05-25T14:45:08.096196+08:00","closed_at":"2026-05-25T14:45:08.096196+08:00","blocked_by":["SL-4958e2"],"blocks":["SL-2e4214"],"labels":["spec:001-init-origin-resolution","phase:us1","story:US1","component:test","requirement:FR-005","requirement:FR-010"],"design":"Test harness helpers that create throwaway git repos in t.TempDir() via 'git init -q' + nested subdirs; skip-with-message if git absent. Cover TestQuickstart_BindFromGitSubdir (config lands at repo root, not cwd subdir; resolver walk-up symmetry) and TestQuickstart_BindNonGitCwdFallback (cwd fallback when not in a git repo).","acceptance_criteria":"git fixtures build real repos in tempdirs; TestQuickstart_BindFromGitSubdir + BindNonGitCwdFallback pass; git documented as required dependency.","parentId":"SL-ec18e7"} +{"id":"SL-2f13c6","title":"Origin branch/ref support (OWNER/REPO[@REF])","description":"Add an optional @ref suffix to the origin reference so a repo can track a branch (or tag/commit) of the skills library. Syntax: OWNER/REPO[@REF], stored combined in the single 'origin' config key. Shape-only/offline validation (no network/existence check), consistent with the rest of init.","status":"closed","priority":1,"issue_type":"feature","spec_context":"001-init-origin-resolution","created_at":"2026-05-29T17:50:54.434515+08:00","updated_at":"2026-05-29T18:54:48.832867+08:00","closed_at":"2026-05-29T18:54:48.832866+08:00","design":"Split on '@' in ParseOrigin; validate left with originPattern, right with a permissive refPattern. Add Origin.Ref; String() appends @Ref when set. Resolver + config I/O unchanged (ref round-trips inside the origin string). Amendment to spec 001 (supersedes the 'no branch/ref pinning' Out-of-Scope line)."} +{"id":"SL-d15491","title":"Core: Origin.Ref + ParseOrigin/@ref split + String()","description":"internal/config/origin.go: add Ref field, refPattern, @-split parse, String() renders @Ref; update error message to OWNER/REPO[@REF].","status":"closed","priority":1,"issue_type":"task","spec_context":"001-init-origin-resolution","created_at":"2026-05-29T17:51:09.693225+08:00","updated_at":"2026-05-29T18:54:48.77077+08:00","closed_at":"2026-05-29T18:54:48.77077+08:00","parentId":"SL-2f13c6"} +{"id":"SL-c5093a","title":"Tests: unit + integration for origin @ref","description":"origin_test.go (valid/invalid ref cases, String w/ ref), resolve_test.go (ref survives resolution), config_test.go (ref round-trip), init_test.go (prompt label), quickstart_test.go (TestQuickstart_BindWithRef + JSON).","status":"closed","priority":1,"issue_type":"task","spec_context":"001-init-origin-resolution","created_at":"2026-05-29T17:51:09.705815+08:00","updated_at":"2026-05-29T18:54:48.785401+08:00","closed_at":"2026-05-29T18:54:48.785401+08:00","parentId":"SL-2f13c6"} +{"id":"SL-d488d8","title":"Docs: cli.md syntax rationale + README + ARCHITECTURE note","description":"cli.md: document OWNER/REPO[@REF], rationale for @ref vs --branch/#, branch-tracks vs skill-pins distinction, Rule 7. README + ARCHITECTURE-v0 §2d/R26 notes.","status":"closed","priority":2,"issue_type":"task","spec_context":"001-init-origin-resolution","created_at":"2026-05-29T17:51:09.716731+08:00","updated_at":"2026-05-29T18:54:48.796698+08:00","closed_at":"2026-05-29T18:54:48.796697+08:00","parentId":"SL-2f13c6"} +{"id":"SL-d9a9bd","title":"Addendum spec doc + contract updates (init/resolve)","description":"specledger/001-init-origin-resolution/amendments/001-origin-ref-support.md (new FRs, data-model delta, BindWithRef scenario). Update contracts/init.md + contracts/resolve.md + data-model.md + quickstart.md.","status":"closed","priority":1,"issue_type":"task","spec_context":"001-init-origin-resolution","created_at":"2026-05-29T17:51:09.726555+08:00","updated_at":"2026-05-29T18:54:48.80909+08:00","closed_at":"2026-05-29T18:54:48.80909+08:00","parentId":"SL-2f13c6"} +{"id":"SL-f948db","title":"Skill co-evolution: skillrig-init @ref + eval (sonnet)","description":"Update .agents/skills/skillrig-init to mention optional @REF/branch; run run_eval.py with model sonnet to confirm trigger accuracy.","status":"closed","priority":3,"issue_type":"task","spec_context":"001-init-origin-resolution","created_at":"2026-05-29T17:51:09.736514+08:00","updated_at":"2026-05-29T18:54:48.820643+08:00","closed_at":"2026-05-29T18:54:48.820643+08:00","parentId":"SL-2f13c6"} diff --git a/specledger/001-init-origin-resolution/quickstart.md b/specledger/001-init-origin-resolution/quickstart.md index 3cd6874..b1079cc 100644 --- a/specledger/001-init-origin-resolution/quickstart.md +++ b/specledger/001-init-origin-resolution/quickstart.md @@ -36,6 +36,16 @@ $ skillrig init --origin my-org/my-skills --json - **stdout**: single JSON object. - **shape assert**: `json.Unmarshal` succeeds AND keys `ok, origin, scope, configPath, written` all present; `ok==true`, `origin=="my-org/my-skills"`, `scope=="project"`, `written==true`. +### TestQuickstart_BindWithRef (FR-018, FR-019, FR-020 — amendment [001-origin-ref-support](amendments/001-origin-ref-support.md)) +``` +$ skillrig init --origin my-org/my-skills@staging +``` +- **exit**: 0 +- **stdout** (human, compact): line 1 contains `bound origin my-org/my-skills@staging` and `project`; line 2 is the `→ resolve order:` footer hint. +- **file** `./.skillrig/config.toml` is `origin = 'my-org/my-skills@staging'` (the optional `@REF` stored combined in the single key). +- **`--json`** (variant): `origin == "my-org/my-skills@staging"`. +- **shape assert**: `len(stdoutLines) <= 2`. + ### TestQuickstart_IdempotentRebind (US1 / FR-008) ``` $ skillrig init --origin my-org/my-skills @@ -151,6 +161,9 @@ project `client-a/skills` + global `personal/skills` → `client-a/skills`, `Sou ### TestResolveOrigin_Row7_MalformedProjectSkipped (FR-004) unparseable project `config.toml` + global `personal/skills` → `personal/skills`, `Source==global`; resolution does not error on the bad file. +### TestResolveOrigin_Row8_9_RefSurvives (FR-020 — amendment [001-origin-ref-support](amendments/001-origin-ref-support.md)) +project `my-org/my-skills@staging` → resolves `my-org/my-skills@staging`, `Source==project`; env `ci-org/ci-skills@main` over a ref-less project → resolves `ci-org/ci-skills@main`, `Source==env`. Proves the optional `@REF` round-trips through resolution unchanged. + ### TestResolveOrigin_FromSubdir (US2 / SC-002) project config at `/.skillrig/config.toml`; call `ResolveOrigin` with `cwd=/a/b/c` → resolves `my-org/my-skills` via walk-up, `Source==project`. @@ -161,6 +174,7 @@ project config at `/.skillrig/config.toml`; call `ResolveOrigin` with `cwd= | Scenario | Covers | |----------|--------| | BindProject / BindProjectJSON | US1, FR-005, FR-010, FR-016 | +| BindWithRef | FR-018, FR-019, FR-020 (optional `@REF` branch tracking) | | IdempotentRebind | FR-008, SC-005 | | RebindDifferent | FR-009 | | Global | FR-007 | diff --git a/specledger/001-init-origin-resolution/sessions/001-origin-ref-support-checkpoint.md b/specledger/001-init-origin-resolution/sessions/001-origin-ref-support-checkpoint.md new file mode 100644 index 0000000..f623485 --- /dev/null +++ b/specledger/001-init-origin-resolution/sessions/001-origin-ref-support-checkpoint.md @@ -0,0 +1,69 @@ +# Divergence Review: 2026-05-29 (amendment 001-origin-ref-support) + +> Branch: `main` (amendment to closed feature 001). Session log named after the +> amendment rather than the branch to avoid clobbering the original +> `001-init-origin-resolution-checkpoint.md` and the poor/collision-prone `main`. +> Scope reviewed: the `OWNER/REPO[@REF]` branch-ref amendment (feature `SL-2f13c6` + 5 tasks). + +### Divergences + +| # | Severity | Type | Category | Issue/Artifact | Description | +|---|----------|------|----------|----------------|-------------| +| 1 | MEDIUM | conscious | Unverified DoD intent | SL-f948db | Skill trigger eval was run with `--model sonnet`, but the harness is confounded by the **already-installed** `.claude/skills/skillrig-init`: it watches for the model invoking a uniquely-named temp command, while the model triggers the real skill → 0 triggers for **every** query (positives and negatives alike). "Confirm trigger accuracy" is therefore unmeasured. Mitigated: the description change is **purely additive** (every original trigger phrase preserved verbatim — verified via `git diff`), so regression risk is near-zero. Constitution IX wants this *verified*, not merely argued. | +| 2 | LOW | conscious | Tracking-state drift | SL-2f13c6 + 5 children | All 6 amendment issues remain `open` despite the work being complete and green. `sl issue update`/`show` derive the spec context from the git branch; on `main` they error (`read specledger/: is a directory`), so statuses could not be advanced this session. | +| 3 | LOW | oversight | Missing DoD | SL-2f13c6 + 5 children | The amendment issues were created without `--dod` items, so there is no per-issue acceptance checklist to verify against. This weakens the force-closed signal for this feature (nothing to be "bypassed"), and DoD evidence should be attached before closing. | +| 4 | LOW | conscious | Validation permissiveness | `internal/config/origin.go` `refPattern` | The shape-only ref pattern `^[A-Za-z0-9._/-]+$` accepts refs that git itself rejects: `o/r@..`, `o/r@/x` (leading slash), `o/r@x/` (trailing slash), `o/r@-x` (probe-confirmed accepted). Intentional per decision D-A2 (offline, no "smart" branch detection; existence/validity deferred to a future command) and documented in FR-019. Reviewers should know malformed-but-charset-clean refs pass. | +| 5 | LOW | conscious | Plan file-list drift | SL-c5093a / `internal/cli/init_test.go` | The task listed `init_test.go` among files to touch for the prompt-label change; it was **not** modified. Its assertions reference the `originPromptLabel` *constant* (lines 53, 115), so they auto-adapt to the new value — verified passing. No edit was needed. | + +No CRITICAL or HIGH divergences. No requirement (FR-018/FR-019/FR-020) is unimplemented; no planned file is missing; no production scope creep observed (`docs/ROADMAP.md` change is the user's own in-flight edit, not part of this work). + +### Force-Closed Issues (DoD Bypassed) + +| Issue | Title | Unchecked DoD Items | Risk | +|-------|-------|---------------------|------| +| — | — | — | **None.** All 26 closed issues have every `definition_of_done.items[].checked == true` (verified with nested detection). The 6 amendment issues are `open`, not closed. | + +### Issues Encountered & Resolutions +- Force-closed detection initially returned a false negative because `definition_of_done` is nested (`{items:[{item,checked}]}`), not a flat list → re-ran descending into `.items`; confirmed 0 force-closed. +- `sl issue update`/`show` fail on `main` (spec context derived from branch) → could not advance issue statuses; recorded as divergence #2. +- Manual e2e in a prior step accidentally wrote `.skillrig/config.toml` into the repo root → removed; working tree clean. + +### Items Requiring Action Before Merge +1. **[MEDIUM] Verify skill trigger accuracy (Constitution IX).** Either run a clean eval by temporarily relocating `.claude/skills/skillrig-init` so the harness's temp command isn't shadowed by the installed skill, or explicitly accept the additive-change rationale and record that acceptance. — Without this, "verified trigger accuracy" is asserted, not measured. +2. **[LOW] Advance/close the 6 issues on a feature branch** with DoD evidence attached (add `--dod` items first). — Keeps the tracker honest; currently the work looks "open" while being done. +3. **[LOW] Decide on `refPattern` strictness.** Accept the documented permissiveness, or tighten to reject `..` and leading/trailing `/`. — Low risk (offline shape-only), but a conscious sign-off avoids surprise. + +### Tests & Checks +- Status: **PASS** +- Commands run: `make check` (`gofmt -w .`, `go vet ./...`, `golangci-lint run` → **0 issues**, `go test ./...` → all ok); plus targeted `go test -count=1 ./internal/config/ -run 'TestParseOrigin|TestOriginString|TestSaveLoadRoundTripWithRef|TestResolveOrigin_Precedence'` and `./test/ -run TestQuickstart_BindWithRef` — all PASS. +- Failures: none. + +### Progress Summary +- Closed: 26 issues (all pre-existing 001 work; 0 from this amendment) +- In Progress: 0 +- Open/Remaining: 6 issues (amendment feature `SL-2f13c6` + 5 task children — implementation complete, status not advanced; see divergence #2) +- Force-Closed: 0 (DoD bypassed: none) + +### Post-Review Update (2026-05-29, same session) + +Adversarial cold review (independent agent) confirmed the above and surfaced 3 additional MEDIUM findings, now **resolved**: +- **D1 (contract drift)** — `contracts/init.md` no-origin error rows updated to `--origin OWNER/REPO[@REF]` to match the shipped binary. +- **D2 (ground-truth drift)** — `data-model.md` precedence matrix gained rows 8/9 (ref survives resolution), matching the test table. +- **D3 (untested path)** — added `TestResolveOrigin_MalformedRefRecordsDiagnostic` exercising the `refPattern` reject branch end-to-end through the resolver. +- **D4 (assertion gap)** — `TestQuickstart_BindWithRef` now asserts the `→ resolve order:` footer and full JSON key completeness (`ok/origin/scope/configPath/written`). +- Re-ran `make check` → still green (lint 0 issues, all tests pass). + +Divergence #1 (skill eval) remains a **conscious, recorded** acceptance: closed `SL-f948db` with the caveat in its close reason (additive description diff, harness-confound documented). Divergences #2/#3 (tracking state / missing DoD) resolved by the closures below. + +**Issue closures**: created feature branch `001-init-origin-resolution` (carrying the uncommitted amendment) so `sl` resolves the spec, then closed all 6 amendment issues (`SL-2f13c6` + `SL-d15491`, `SL-c5093a`, `SL-d488d8`, `SL-d9a9bd`, `SL-f948db`) with evidence-bearing reasons. Spec 001 now: **32 closed, 0 open**. (Adversarial finding D5 — the `docs/ROADMAP.md` `aws` row / row-003 edit — is the user's own in-flight edit, left untouched.) + +### Uncommitted Changes +- `internal/config/origin.go`, `internal/cli/init.go` (code) +- `internal/config/origin_test.go`, `internal/config/config_test.go`, `internal/config/resolve_test.go`, `test/quickstart_test.go` (tests) +- `docs/design/cli.md`, `README.md`, `docs/ARCHITECTURE-v0.md` (docs) +- `specledger/001-init-origin-resolution/{spec.md,data-model.md,quickstart.md,contracts/init.md,contracts/resolve.md,issues.jsonl}` (spec artifacts) +- `specledger/001-init-origin-resolution/amendments/001-origin-ref-support.md` (new addendum, untracked) +- `.agents/skills/skillrig-init/{SKILL.md,evals/evals.json,evals/trigger-eval-set.json}` (skill + evals) +- Not mine (user's in-flight edits): `CLAUDE.md`, `docs/ROADMAP.md`; pre-existing untracked: `docs/guides/vcr-cassettes.md` + +--- diff --git a/specledger/001-init-origin-resolution/spec.md b/specledger/001-init-origin-resolution/spec.md index d1dfd2d..6d9786f 100644 --- a/specledger/001-init-origin-resolution/spec.md +++ b/specledger/001-init-origin-resolution/spec.md @@ -5,6 +5,8 @@ **Status**: Draft **Input**: User description: "Build `skillrig init` and the single origin-resolution primitive for the generic skillrig CLI." +> **Amendments**: [amendments/001-origin-ref-support.md](./amendments/001-origin-ref-support.md) (2026-05-29) extends the origin reference from `OWNER/REPO` to `OWNER/REPO[@REF]`, adding an optional branch/ref (FR-018–FR-020). Where this spec says the shape is `OWNER/REPO`, read `OWNER/REPO[@REF]`. + ## Overview skillrig is a single generic binary shared by every org; it carries **no baked-in origin**. The "origin" — the org's private git monorepo that is the source of truth for its skills (e.g. `my-org/my-skills`) — is supplied by the consumer at runtime. This feature delivers the two foundational capabilities every later command depends on: @@ -111,7 +113,7 @@ A developer or agent runs a command that needs an origin in a repo that was neve - **FR-009**: The command MUST update the recorded origin when invoked with a different value, replacing the prior value cleanly. - **FR-010**: The command MUST create any missing config directory/file needed to store the origin. - **FR-011**: The command MUST bind only to an origin the consumer supplies; it MUST NOT create, scaffold, or bootstrap an origin repository. (Standing up an origin is out of scope — see Out of Scope.) -- **FR-012**: The command MUST validate that a supplied origin matches the expected `OWNER/REPO` shape and reject malformed values without writing config. +- **FR-012**: The command MUST validate that a supplied origin matches the expected `OWNER/REPO` shape and reject malformed values without writing config. *(Amended: `OWNER/REPO[@REF]` — see FR-018–FR-020 in [amendments/001-origin-ref-support.md](./amendments/001-origin-ref-support.md).)* **Command experience (baseline conformance, inherited by all later commands)** @@ -123,7 +125,7 @@ A developer or agent runs a command that needs an origin in a repo that was neve ### Key Entities *(include if feature involves data)* -- **Origin reference**: an identifier for the org's skill source in `OWNER/REPO` form (e.g. `my-org/my-skills`). The single value this feature reads, validates, records, and resolves. +- **Origin reference**: an identifier for the org's skill source in `OWNER/REPO[@REF]` form (e.g. `my-org/my-skills` or `my-org/my-skills@staging`). The single value this feature reads, validates, records, and resolves. The optional `@REF` tracks a branch ([amendments/001-origin-ref-support.md](./amendments/001-origin-ref-support.md)). - **Project config**: per-repo, committed configuration that records the repo's origin so the repo is self-describing to any clone/agent/CI. Hand-editable input. - **Global default config**: per-user configuration recording the developer's default origin, used when a repo has no project config. - **Environment override**: a runtime-supplied origin (highest precedence) for ephemeral/CI use that does not touch any file. @@ -164,7 +166,7 @@ This feature is the first to exercise the project constitution; the following mu **Assumptions**: -- The origin identifier shape is `OWNER/REPO` (two non-empty, slash-separated segments); deeper validation (does the repo exist / is it reachable) is deliberately deferred because this feature is offline. +- The origin identifier shape is `OWNER/REPO` (two non-empty, slash-separated segments) — amended to `OWNER/REPO[@REF]` with an optional branch/ref ([amendments/001-origin-ref-support.md](./amendments/001-origin-ref-support.md)); deeper validation (does the repo or ref exist / is it reachable) is deliberately deferred because this feature is offline. - Config is stored as a small, hand-editable file at a project-scoped location and a per-user global location, consistent with the architecture's `config.toml` decision; the input config and any tool-written output (lockfiles) are separate concerns (lockfiles are out of scope here). The canonical, detailed config-file structure is maintained on the project docs website and referenced rather than restated here. - A developer running global mode wants only the per-user default written, never the repo config. diff --git a/test/quickstart_test.go b/test/quickstart_test.go index 6a414cb..3698cac 100644 --- a/test/quickstart_test.go +++ b/test/quickstart_test.go @@ -224,6 +224,60 @@ func TestQuickstart_BindProjectJSON(t *testing.T) { } } +// TestQuickstart_BindWithRef maps to quickstart BindWithRef (amendment +// 001-origin-ref-support): an origin may carry an optional @REF (a branch) and +// it is recorded combined in the single `origin` key. +func TestQuickstart_BindWithRef(t *testing.T) { + t.Parallel() + + cwd := t.TempDir() + res := run(t, runOpts{args: []string{"init", "--origin", "my-org/my-skills@staging"}, cwd: cwd}) + + if res.exit != 0 { + t.Fatalf("exit = %d, want 0 (stderr: %s)", res.exit, res.stderr) + } + + lines := nonEmptyLines(res.stdout) + if len(lines) > 2 { + t.Errorf("stdout has %d lines, want <= 2:\n%s", len(lines), res.stdout) + } + + if !strings.Contains(lines[0], "my-org/my-skills@staging") || !strings.Contains(lines[0], "project") { + t.Errorf("line 1 = %q, want it to mention bound origin (incl. @staging) + project", lines[0]) + } + + if !strings.Contains(res.stdout, "→ resolve order:") { + t.Errorf("stdout missing resolve-order footer hint:\n%s", res.stdout) + } + + got := readFile(t, filepath.Join(cwd, ".skillrig", "config.toml")) + if got != "origin = 'my-org/my-skills@staging'\n" { + t.Errorf("config.toml = %q, want origin = 'my-org/my-skills@staging'", got) + } + + jsonRes := run(t, runOpts{args: []string{"init", "--origin", "my-org/my-skills@staging", "--json"}, cwd: cwd}) + + var obj map[string]any + if err := json.Unmarshal([]byte(jsonRes.stdout), &obj); err != nil { + t.Fatalf("json variant: %v\n%s", err, jsonRes.stdout) + } + + // JSON must be structurally complete (Constitution II), not just carry the ref. + for _, key := range []string{"ok", "origin", "scope", "configPath", "written"} { + if _, ok := obj[key]; !ok { + t.Errorf("JSON missing key %q: %v", key, obj) + } + } + + if obj["origin"] != "my-org/my-skills@staging" { + t.Errorf("origin = %v, want my-org/my-skills@staging", obj["origin"]) + } + + if obj["scope"] != "project" { + t.Errorf("scope = %v, want project", obj["scope"]) + } +} + func TestQuickstart_IdempotentRebind(t *testing.T) { t.Parallel() From 169e24144cbb6715e84e42fd08fdcd48b7a79712 Mon Sep 17 00:00:00 2001 From: Vincent De Smet Date: Fri, 29 May 2026 20:31:00 +0800 Subject: [PATCH 2/2] refactor(001): return typed InvalidOriginError from config, render in cli Addresses Qodo rule 783432 (internal/config must stay presentation-free). ParseOrigin now returns a typed *InvalidOriginError carrying only the offending value; internal/cli renders the OWNER/REPO[@REF] what/why/fix message. Updates the config + cli unit tests and the data-model/contract/amendment wording. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/cli/init.go | 15 +++++++++++ internal/cli/init_test.go | 27 +++++++++++++++++++ internal/config/origin.go | 26 +++++++++++------- internal/config/origin_test.go | 21 ++++++++++----- .../amendments/001-origin-ref-support.md | 2 +- .../contracts/init.md | 2 +- .../001-init-origin-resolution/data-model.md | 2 +- 7 files changed, 77 insertions(+), 18 deletions(-) diff --git a/internal/cli/init.go b/internal/cli/init.go index 23801fb..c57280a 100644 --- a/internal/cli/init.go +++ b/internal/cli/init.go @@ -2,6 +2,7 @@ package cli import ( "bufio" + "errors" "fmt" "os" "path/filepath" @@ -19,6 +20,15 @@ const originPromptLabel = "Origin (OWNER/REPO[@REF]): " // missingOriginFix is the shared fix line for the no-origin error paths. const missingOriginFix = "fix: pass --origin OWNER/REPO[@REF] (e.g. --origin my-org/my-skills or --origin my-org/my-skills@main) or set SKILLRIG_ORIGIN" +// invalidOriginMsg renders the three-part what/why/fix usage message for a +// malformed origin. Presentation lives here in internal/cli, not in +// internal/config, which returns a typed *config.InvalidOriginError (the +// presentation-free boundary — see config.ParseOrigin). +func invalidOriginMsg(value string) string { + return fmt.Sprintf("invalid origin %q\nwhy: expected OWNER/REPO[@REF]\n"+ + "fix: pass --origin OWNER/REPO[@REF] (e.g. --origin my-org/my-skills or --origin my-org/my-skills@main)", value) +} + // initCmd holds the init command's flags and its injectable seams. Production // uses the os-backed defaults; tests inject deterministic stubs (interactivity, // cwd, env) so the prompt and write-target logic are testable without a TTY. @@ -94,6 +104,11 @@ func (ic *initCmd) run(cmd *cobra.Command) error { origin, err := config.ParseOrigin(raw) if err != nil { + var invalid *config.InvalidOriginError + if errors.As(err, &invalid) { + return &UsageError{Msg: invalidOriginMsg(invalid.Value), Cause: err} + } + return &UsageError{Msg: err.Error(), Cause: err} } diff --git a/internal/cli/init_test.go b/internal/cli/init_test.go index 45001e2..8bcf347 100644 --- a/internal/cli/init_test.go +++ b/internal/cli/init_test.go @@ -116,3 +116,30 @@ func TestInit_NonInteractiveFlagOverridesTTY(t *testing.T) { t.Errorf("--non-interactive must not prompt even on a TTY, stderr: %q", errBuf.String()) } } + +// TestInit_MalformedOriginMessage pins that the user-facing expected-format +// guidance for a malformed origin is rendered in the CLI layer — config returns +// a presentation-free *config.InvalidOriginError (Qodo rule 783432). Asserts the +// three-part what/why/fix at the unit level (quickstart covers it end-to-end). +func TestInit_MalformedOriginMessage(t *testing.T) { + t.Parallel() + + ic, cmd, _, _ := newTestInitCmd(t, false, t.TempDir()) + ic.origin = "not-a-valid-origin" + + err := ic.run(cmd) + if err == nil { + t.Fatal("expected usage error for malformed origin") + } + + var usageErr *UsageError + if !errors.As(err, &usageErr) { + t.Fatalf("error %T is not a *UsageError", err) + } + + for _, want := range []string{"not-a-valid-origin", "OWNER/REPO", "my-org/my-skills"} { + if !strings.Contains(usageErr.Msg, want) { + t.Errorf("message %q missing %q (what/why/fix must be rendered in cli)", usageErr.Msg, want) + } + } +} diff --git a/internal/config/origin.go b/internal/config/origin.go index 3c2bb3f..c8a9bd3 100644 --- a/internal/config/origin.go +++ b/internal/config/origin.go @@ -25,10 +25,17 @@ var originPattern = regexp.MustCompile(`^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$`) // of scope. var refPattern = regexp.MustCompile(`^[A-Za-z0-9._/-]+$`) -// originErrFmt is the shared usage-error format for a malformed origin. It names -// the expected shape (including the optional ref) and echoes the offending value -// (FR-012 / amendment FR-018). -const originErrFmt = "invalid origin %q: expected OWNER/REPO[@REF] (e.g. my-org/my-skills or my-org/my-skills@main)" +// InvalidOriginError marks a value that fails the OWNER/REPO[@REF] shape check. +// It carries only the offending value so internal/config stays presentation-free: +// the user-facing "expected …, e.g. …" guidance is rendered by internal/cli +// (FR-012 / amendment FR-018). Callers match it with errors.As. +type InvalidOriginError struct { + Value string +} + +func (e *InvalidOriginError) Error() string { + return fmt.Sprintf("invalid origin %q", e.Value) +} // Origin is an org's skill source in OWNER/REPO[@REF] form. It is the single // value this feature reads, validates, records, and resolves. Ref is optional: @@ -61,19 +68,20 @@ func (o Origin) String() string { // OWNER/REPO[@REF] shape. The optional ref is split on the first '@' (the // owner/repo charset excludes '@', so the split is unambiguous) and validated // against refPattern; a trailing '@' with no ref is rejected. On failure it -// returns a usage error that names the expected format and echoes the offending -// value (FR-012). A blank string is rejected; callers that treat blank as -// "unset" (e.g. SKILLRIG_ORIGIN) must check before calling. +// returns a typed *InvalidOriginError carrying the offending value; the +// user-facing expected-format guidance is rendered by internal/cli (FR-012). A +// blank string is rejected; callers that treat blank as "unset" (e.g. +// SKILLRIG_ORIGIN) must check before calling. func ParseOrigin(s string) (Origin, error) { trimmed := strings.TrimSpace(s) ownerRepo, ref, hasRef := strings.Cut(trimmed, "@") if !originPattern.MatchString(ownerRepo) { - return Origin{}, fmt.Errorf(originErrFmt, s) + return Origin{}, &InvalidOriginError{Value: s} } if hasRef && !refPattern.MatchString(ref) { - return Origin{}, fmt.Errorf(originErrFmt, s) + return Origin{}, &InvalidOriginError{Value: s} } owner, repo, _ := strings.Cut(ownerRepo, "/") diff --git a/internal/config/origin_test.go b/internal/config/origin_test.go index fa78352..33b2338 100644 --- a/internal/config/origin_test.go +++ b/internal/config/origin_test.go @@ -1,6 +1,7 @@ package config import ( + "errors" "strings" "testing" ) @@ -68,12 +69,20 @@ func TestParseOriginErrorEchoesValue(t *testing.T) { if err == nil { t.Fatal("expected error for malformed origin") } - // FR-012: error names the expected format and echoes the offending value. - msg := err.Error() - for _, want := range []string{"not-valid", "OWNER/REPO"} { - if !strings.Contains(msg, want) { - t.Errorf("error %q missing %q", msg, want) - } + // FR-012: config returns a typed *InvalidOriginError carrying the offending + // value (presentation-free); internal/cli renders the expected-format + // guidance. Pin the type + the echoed value here. + var invalid *InvalidOriginError + if !errors.As(err, &invalid) { + t.Fatalf("error %T is not a *InvalidOriginError", err) + } + + if invalid.Value != "not-valid" { + t.Errorf("InvalidOriginError.Value = %q, want %q", invalid.Value, "not-valid") + } + + if !strings.Contains(err.Error(), "not-valid") { + t.Errorf("error %q should echo the offending value", err.Error()) } } diff --git a/specledger/001-init-origin-resolution/amendments/001-origin-ref-support.md b/specledger/001-init-origin-resolution/amendments/001-origin-ref-support.md index 48bb967..b50326e 100644 --- a/specledger/001-init-origin-resolution/amendments/001-origin-ref-support.md +++ b/specledger/001-init-origin-resolution/amendments/001-origin-ref-support.md @@ -41,7 +41,7 @@ Amends [data-model.md](../data-model.md) → *Entities* → **Origin**: | `Repo` | string | non-empty, charset `[A-Za-z0-9._-]` | | `Ref` | string | **optional**; when present, charset `[A-Za-z0-9._/-]` (owner/repo charset plus `/` for branch names like `feature/x`). Empty = default branch. | -- `ParseOrigin(s)` now: trims whitespace; splits on the first `@` into `OWNER/REPO` and `REF`; matches `OWNER/REPO` against `^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$` and, when an `@` was present, `REF` against `^[A-Za-z0-9._/-]+$`; returns the usage error `invalid origin %q: expected OWNER/REPO[@REF] (e.g. my-org/my-skills or my-org/my-skills@main)` on failure (FR-012/FR-018). +- `ParseOrigin(s)` now: trims whitespace; splits on the first `@` into `OWNER/REPO` and `REF`; matches `OWNER/REPO` against `^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$` and, when an `@` was present, `REF` against `^[A-Za-z0-9._/-]+$`; on failure returns a typed `*InvalidOriginError` carrying the offending value. Per the presentation-free `internal/config` rule (CLAUDE.md; Qodo rule 783432), `internal/cli` renders the user-facing `OWNER/REPO[@REF]` what/why/fix message (e.g. `--origin my-org/my-skills` or `--origin my-org/my-skills@main`) (FR-012/FR-018). - `String()` renders `Owner/Repo`, appending `@Ref` only when `Ref != ""`. The zero Origin still renders `""` (SourceNone sentinel — unchanged). - **Storage is unchanged structurally**: `Config{ Origin string }` and the single `origin` TOML key. A ref'd origin serializes as `origin = 'my-org/my-skills@staging'`. diff --git a/specledger/001-init-origin-resolution/contracts/init.md b/specledger/001-init-origin-resolution/contracts/init.md index 903ed98..015efeb 100644 --- a/specledger/001-init-origin-resolution/contracts/init.md +++ b/specledger/001-init-origin-resolution/contracts/init.md @@ -40,7 +40,7 @@ Examples: 1. Resolve write target: `--global` → global config path; else the **git repo root** via `git rev-parse --show-toplevel` (offline) → `/.skillrig/config.toml`; if not inside a git repo, fall back to `./.skillrig/config.toml` in cwd (create `.skillrig/` if missing — FR-010). 2. Determine origin: `--origin` value; else if `--non-interactive` is set → usage error without prompting (FR-006c); else if interactive TTY → prompt once on stderr; else (no TTY) usage error (FR-006a). -3. `ParseOrigin` → splits the optional `@REF` and validates both parts shape-only/offline; on invalid shape, usage error, no write (FR-012 / FR-018, FR-019). A valid `@REF` is carried on the resolved origin and stored combined in the `origin` key (FR-020). +3. `ParseOrigin` → splits the optional `@REF` and validates both parts shape-only/offline; on invalid shape it returns a typed `*config.InvalidOriginError` (presentation-free) which the CLI renders as the user-facing what/why/fix, no write (FR-012 / FR-018, FR-019). A valid `@REF` is carried on the resolved origin and stored combined in the `origin` key (FR-020). 4. Load existing config at target (if any). Compare: - none present → write, `written=true`. - equal origin → no-op, `written=false` (idempotent, FR-008). diff --git a/specledger/001-init-origin-resolution/data-model.md b/specledger/001-init-origin-resolution/data-model.md index 3cda7f4..f009c39 100644 --- a/specledger/001-init-origin-resolution/data-model.md +++ b/specledger/001-init-origin-resolution/data-model.md @@ -13,7 +13,7 @@ The org's skill source, in `OWNER/REPO[@REF]` form. The single value this featur | `Repo` | string | non-empty, charset `[A-Za-z0-9._-]` | | `Ref` | string | **optional** (amendment [001-origin-ref-support](amendments/001-origin-ref-support.md)); charset `[A-Za-z0-9._/-]`; empty = default branch | -- Constructed via `ParseOrigin(s string) (Origin, error)`: trims whitespace, splits on the first `@` into `OWNER/REPO` and `REF`, matches `OWNER/REPO` against `^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$` and (when present) `REF` against `^[A-Za-z0-9._/-]+$`. On failure returns a usage error naming the expected `OWNER/REPO[@REF]` format and echoing the offending value (FR-012 / FR-018). +- Constructed via `ParseOrigin(s string) (Origin, error)`: trims whitespace, splits on the first `@` into `OWNER/REPO` and `REF`, matches `OWNER/REPO` against `^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$` and (when present) `REF` against `^[A-Za-z0-9._/-]+$`. On failure returns a typed `*InvalidOriginError` carrying the offending value (presentation-free); `internal/cli` renders the expected-`OWNER/REPO[@REF]` what/why/fix guidance (FR-012 / FR-018). - `String()` renders `Owner/Repo`, appending `@Ref` when a ref is set. ### ProjectConfig → `.skillrig/config.toml` (committed, hand-editable INPUT)