Skip to content

feat(001): skillrig init + single origin resolver#3

Merged
so0k merged 13 commits into
mainfrom
001-init-origin-resolution
May 26, 2026
Merged

feat(001): skillrig init + single origin resolver#3
so0k merged 13 commits into
mainfrom
001-init-origin-resolution

Conversation

@so0k
Copy link
Copy Markdown
Contributor

@so0k so0k commented May 26, 2026

Summary

First slice of the skillrig CLI: the skillrig init command (Environment pattern) that binds a repo (or a per-user global default) to an OWNER/REPO origin, plus the single config.ResolveOrigin primitive (AP-06) every later command will use. Offline config bootstrap only — no network, no consuming commands.

  • skillrig init — binds the origin into .skillrig/config.toml at the git repo root (offline git rev-parse, cwd fallback); --global, --json, --non-interactive; idempotent re-bind; TTY-gated stdlib prompt; errors-as-navigation (what/why/fix) with load-bearing exit codes.
  • ResolveOrigin — precedence SKILLRIG_ORIGIN > project (walk-up) > global; blank-env-unset; malformed sources skipped and surfaced via ResolutionResult.Diagnostics; genuine I/O errors fatal (typed config.MalformedError).
  • Cobra root, exit codes, output helper, .golangci.yml, README, and the skillrig-init agent skill (Constitution IX).

Tests & gate

  • 11 TestQuickstart_* E2E (build + exec the real binary) + TestResolveOrigin_* precedence matrix (rows 1–7 + FromSubdir + diagnostics/fatal cases) + unit tests.
  • go test ./... green · gofmt clean · go vet clean · golangci-lint 0 issues.

Review trail

Implemented per /specledger.implement; reviewed via /specledger.checkpoint (self-review) and an independent fresh-eyes adversarial pass. Full disposition in sessions/001-init-origin-resolution-checkpoint.md.

Tracker: 26 closed, 0 open, 0 force-closed (47/47 DoD items checked). No CRITICAL/HIGH findings.

All review findings resolved or justified, including post-review fixes in this PR:

  • FR-004 resolver diagnostics (never silently swallow a bad source) + MalformedError.
  • Malformed-SKILLRIG_ORIGIN hard-error test (A1); dead Origin.IsZero() removed (A4); research.md D9 perms reconciled (0o600/0o750).

Conscious decisions

  • Interactive prompt tested in-process (quickstart-sanctioned "shim") — the pty test dep was disallowed by the minimal-deps policy.
  • TOML literal-string output (origin = 'my-org/my-skills', go-toml/v2 default); fixture/docs anchored to real output (review G1).

Deferred to the next feature (no actionable surface in this slice; justified in the log)

  • A2 — wire ResolveOrigin's Source==none render + Diagnostics --verbose to the first consuming command (search/add/verify). The resolver ships as a tested primitive with no production caller, by design (spec Out of Scope).
  • A3 — that consuming command should confirm the "unreadable project config is fatal even with a valid global" semantic (currently contract-aligned + test-pinned) and add the E2E resolve-symmetry assertion.

🤖 Generated with Claude Code

so0k and others added 12 commits May 24, 2026 19:23
First feature spec for the skillrig CLI: `skillrig init` plus the single
origin-resolution primitive (env > project config > global default).
Offline config bootstrap only — search/add/verify/lockfile/network and
origin bootstrapping are explicitly out of scope. Includes prioritized
user stories (bind / precedence / actionable failure), functional
requirements, measurable success criteria, and a Constitution Alignment
section (Quickstart-as-Contract, Ground-Truth Anchoring, Skill-CLI
Co-Evolution). Quality checklist passed on first iteration.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…(001)

Resolve reviewer comments via SpecLedger review:
- Spec stays command-agnostic; concrete command surface decided at plan
  (research + quickstart.md), grounded in architecture.md and cli.md.
- Origin binding is origin-only: prompt-if-missing in interactive
  sessions, non-interactive when supplied (FR-006a); no extra onboarding
  metadata such as repo tags (FR-006b) — deferred to a later version.
- No dedicated config command; config file is hand-editable input,
  detailed structure documented on the docs website (referenced).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 0/1 planning output: plan.md (Go + spf13/cobra + go-toml/v2, single
ResolveOrigin per AP-06, init = Environment pattern), research.md (10
resolved decisions), data-model.md (config.toml ground-truth fixture +
7-row precedence matrix), contracts/ (init command surface + resolver
contract), and quickstart.md (executable TestQuickstart_*/TestResolveOrigin_*
scenarios with output-shape assertions, per Constitution II).

Also harvest the additive (non-conflicting) ideas from agentic-cli-design
into docs/design/cli.md as a "Borrowed Ideas (vNext)" section — machine
introspection, install-skills, scorecard — noting cli.md stays authoritative
where they conflict (exit codes, JSON-default, prose errors). Bind project
to the SpecLedger web app (project id) and refresh CLAUDE.md agent context.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Install the Go-focused agent skills used to build this CLI (golang-cli,
golang-spf13-cobra, golang-testing, golang-code-style, golang-lint from
samber/cc-skills-golang) and record them in skills-lock.json. The
agentic-cli-design skill was intentionally removed after harvesting its
additive ideas into docs/design/cli.md, to keep cli.md authoritative
during implementation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Empty commit to re-fire the SpecLedger GitHub App webhook after it failed
to parse the newly added plan.md/research.md/data-model.md/contracts/quickstart.md.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… (001)

Revise research.md per crit review: D2 (XDG OS/shell support + os.UserHomeDir,
adrg/xdg deferred), D3 (git is a fair baseline — git-root write, fs walk-up
resolve), D4 (explicit --non-interactive flag + flag/prompt model), D9 (POSIX
same-fs + Windows MoveFileEx atomic-write caveats), D10 (viper evaluated and
rejected for v0; TOML supported; cobra/viper/huh orthogonal).

Add spike S1 (research/2026-05-24-interactive-prompt-library.md): measured
stripped-binary deltas (stdlib +0, promptui +768B, survey +0.23MB/+39 mods,
huh +0.61MB/+39 mods) → use stdlib bufio for v0, defer charmbracelet/huh
(v1.0.0, accessible mode) to a future multi-field flow. D5 resolved.

plan.md + CLAUDE.md: Go 1.24+ (toolchain is 1.24.4) per review.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add the crit review/plan-review/code-review agent skills installed via
`crit setup-claude` (kevindutra/crit, the Go review TUI) so the inline-review
workflow travels with the repo.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Epic SL-227789 + 6 phases (Setup, Foundational, US1, US2, US3, Polish) and
18 tasks in the sl issue store (issues.jsonl), linked into a blocking graph.
Tests are first-class per Constitution II (quickstart scenarios -> failing
Go tests before impl; DoD = scenarios match stories + tests pass); includes
ground-truth fixtures (III), the skillrig-init agent skill (IX), and the
gofmt/vet/golangci-lint gate (V). tasks.md is the navigational index.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…, git-root write target, traceability

Cross-artifact verification of the init & origin-resolution feature surfaced
seams between the init task design and the contract/quickstart. Resolved:

- FR-006c: explicit force-non-interactive mode (flag) — fail fast instead of
  prompting even on a TTY. Added to spec (US3 scenario 4), contract, and
  quickstart (TestQuickstart_NonInteractiveFlag).
- git-root write target: documented offline `git rev-parse --show-toplevel`
  resolution + cwd fallback in contract/data-model; declared `git` a required
  dependency in plan; added TestQuickstart_BindFromGitSubdir /
  BindNonGitCwdFallback and a dedicated git-fixture task (SL-3b4985, with a
  scope-boundary note on the harness task SL-4958e2).
- traceability: backfilled requirement: labels across tasks.
- naming: clarified spec "bind command" -> `skillrig init` (no config command).
- saved analysis to specledger/001-init-origin-resolution/reviews/001-review.md.

Also adds the `kevindutra/crit` review tool to mise.toml.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Deliver the first slice of the skillrig CLI: `skillrig init` (Environment
pattern) binds a repo or per-user global default to an OWNER/REPO origin,
plus the single ResolveOrigin primitive (AP-06) with precedence
SKILLRIG_ORIGIN > project > global.

- cobra root with --json/--verbose, errors-as-navigation, load-bearing exit codes
- atomic config.toml read/write (go-toml/v2), git-root write target + cwd fallback
- idempotent re-bind, TTY-gated stdlib prompt, --non-interactive fail-fast (FR-006c)
- 11 TestQuickstart_* E2E (build+exec) + TestResolveOrigin precedence matrix + unit tests
- skillrig-init agent skill (Constitution IX) + trigger eval-set; README usage docs
- full gate green: gofmt/vet/golangci-lint clean, go test ./... passing

All 19 tasks closed under epic SL-227789. Fixture/docs anchored to real
go-toml/v2 output (single-quote TOML literal; review finding G1).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… gaps

Resolve the post-implementation checkpoint findings on the origin resolver so a
future --verbose caller can never mistake a silent skip for intended behavior.

- config.MalformedError: typed, unwrappable. Load returns it for parse failures
  and a plain wrapped error for genuine I/O failures, so callers distinguish a
  skippable malformed file from a fatal read error (FR-004 / contract resolve.md).
- ResolveOrigin: add ResolutionResult.Diagnostics ([]SourceDiagnostic). New
  originFromFile classifies each source into four honest outcomes — usable /
  skippable-with-diagnostic (malformed or invalid origin) / quiet fall-through
  (absent or origin-less) / fatal I/O — replacing usableOrigin which collapsed
  everything (including I/O errors) to (zero, false). Diagnostics accumulate
  regardless of the final Source.
- Tests: malformed-project, invalid-shape, origin-less, unreadable-fatal,
  unreadable-project-fatal-despite-global (A3), and malformed-SKILLRIG_ORIGIN
  hard-error (A1); TestLoadMalformedErrors asserts errors.As(*MalformedError).
- Remove dead Origin.IsZero() (A4 — no callers).
- Reconcile research.md D9 to the shipped 0o600/0o750 perms (self-review #2).
- contracts/resolve.md + data-model.md document Diagnostics and the
  malformed-vs-fatal split, with a "never re-introduce a silent skip" note.

Gate green: go test ./... · gofmt · go vet · golangci-lint (0 issues).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Record the post-implementation divergence review, the independent fresh-eyes
adversarial review, and the disposition of every finding (A1–A6 + self-review
divergences). All resolved or justified-and-deferred; tracker is 0 open /
0 force-closed (47/47 DoD items checked). issues.jsonl carries the A5
clarifying note on SL-db8e96 (interactive-prompt test relocated in-process).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@so0k
Copy link
Copy Markdown
Contributor Author

so0k commented May 26, 2026

/agentic_review

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented May 26, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Checkbox task list in plan.md 📘 Rule violation § Compliance
Description
The PR adds markdown checkbox checklist items (e.g., - [x] ...) under a Constitution Check/spec
checklist context, embedding task/checklist tracking directly in markdown. This violates Compliance
Rule (PR Compliance ID 780918) disallowing markdown TODO/task lists for issue tracking instead of
using the issue system.
Code

specledger/001-init-origin-resolution/plan.md[R29-37]

Evidence
PR Compliance ID 780918 prohibits using markdown checkbox task lists (- [ ] / - [x]) for
tracking work items. The cited sections in specledger/001-init-origin-resolution/plan.md and
specledger/001-init-origin-resolution/checklists/requirements.md contain multiple checkbox list
items in the - [x] ... format (including the Constitution Check checklist), which matches the
prohibited pattern and indicates checklist/task tracking being done in markdown rather than via the
issue system.

Rule 780918: Disallow markdown TODO or task lists for issue tracking
specledger/001-init-origin-resolution/plan.md[29-37]
specledger/001-init-origin-resolution/checklists/requirements.md[9-30]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Remove or refactor the markdown checkbox task list items (`- [ ]` / `- [x]`) that are being used to track checklist/work items in markdown, as this violates PR Compliance ID 780918.

## Issue Context
PR compliance requires that work items/checklists be tracked in the issue system rather than via markdown task lists, unless the checkboxes are clearly historical/changelog-only or explicitly mirroring an `sl issue` reference.

## Fix Focus Areas
- specledger/001-init-origin-resolution/plan.md[29-37]
- specledger/001-init-origin-resolution/checklists/requirements.md[9-30]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Stat errors silently bypassed ✓ Resolved 🐞 Bug ☼ Reliability
Description
FindProjectConfig ignores all os.Stat errors (including permission/I/O failures) and continues
walking, which can cause ResolveOrigin to fall through to global/none instead of failing on a
genuine filesystem error. This contradicts the resolver’s documented “genuine I/O errors are fatal”
behavior because an unreadable/inaccessible project config can be treated as “not found.”
Code

internal/config/config.go[R157-167]

Evidence
FindProjectConfig only returns a found path when os.Stat succeeds; all other errors are dropped,
and ResolveOrigin only attempts to load a project config when FindProjectConfig reports
found==true, so stat failures prevent the intended fatal read path from running.

internal/config/config.go[149-169]
internal/config/resolve.go[55-90]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`FindProjectConfig` currently treats any `os.Stat` error as “candidate not present” and keeps walking upward. If the error is `EACCES`/permission denied (or other I/O failure), the resolver will behave as if there is no project config and may incorrectly resolve to the global config or `SourceNone`.

### Issue Context
- `ResolveOrigin` relies on `FindProjectConfig(cwd)` to decide whether to attempt loading the project config.
- `ResolveOrigin`/`originFromFile` are explicitly designed to treat genuine I/O errors as fatal.

### Fix Focus Areas
- internal/config/config.go[149-169]
- internal/config/resolve.go[77-90]

### Suggested fix
- Change `FindProjectConfig` to surface errors for non-`ErrNotExist` stat failures (e.g., return `(string, bool, error)`), and have `ResolveOrigin` fail fast on that error.
- Alternatively (smaller signature impact), special-case `os.Stat` errors: if `errors.Is(err, fs.ErrNotExist)` continue walking; otherwise return a fatal error via a different API used by `ResolveOrigin`.
- Add a unit test that simulates a permission-denied walk-up scenario (e.g., remove execute bit from `.skillrig` dir or an ancestor) and asserts the resolver returns an error rather than falling through.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

3. Zero origin prints slash ✓ Resolved 🐞 Bug ≡ Correctness
Description
config.Origin.String() renders the zero-value Origin as "/", but ResolveOrigin explicitly
returns the zero Origin when Source==none, making the "no origin" state stringify to a misleading
non-empty value. This will produce confusing output and can break any caller that treats non-empty
Origin.String() as "configured" without also checking Source.
Code

internal/config/origin.go[R25-27]

Evidence
The resolver intentionally returns the zero Origin for SourceNone, but the Origin value type
currently stringifies that sentinel as "/" rather than an empty string, making the sentinel look
like a configured value when rendered.

internal/config/origin.go[17-27]
internal/config/resolve.go[40-63]
internal/config/resolve.go[55-111]
internal/config/resolve_test.go[325-333]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Origin.String()` currently concatenates `Owner + "/" + Repo` unconditionally, so the zero-value prints as "/". But `ResolveOrigin` uses the zero Origin as the sentinel value when `SourceNone`, so stringifying the returned Origin yields a misleading, non-empty string.

### Issue Context
- `ResolveOrigin` documents and returns `SourceNone` with the zero Origin.
- Callers (present or future) may serialize/log/display `Origin.String()`; with the current behavior the "none" state becomes indistinguishable from a real-but-weird-looking value.

### Fix Focus Areas
- internal/config/origin.go[24-27]
- internal/config/resolve.go[40-111]
- internal/config/resolve_test.go[325-333]

### Suggested fix
- Update `Origin.String()` to return `""` when `Owner=="" && Repo==""` (or more strictly when either segment is empty).
- Update tests that currently accept `"/"` for the zero Origin (and the helper/comment in `originString`) to reflect the new empty-string representation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Git root errors masked ✓ Resolved 🐞 Bug ☼ Reliability
Description
gitRoot collapses any git rev-parse failure into “not a repo/git unavailable,” so
ProjectWriteTarget can fall back to writing under the current working directory even when the
directory is inside a repo but git failed for another reason. This can place
.skillrig/config.toml in the wrong directory (e.g., a subdir), diverging from the “write at repo
root” intent.
Code

internal/config/config.go[R192-195]

Evidence
gitRoot treats all git command errors the same (return "", false), and ProjectWriteTarget
uses that boolean to decide between repo root and cwd; init uses ProjectWriteTarget for the
project config path, so a broad false can directly change where config is written.

internal/config/config.go[171-203]
internal/cli/init.go[113-132]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`gitRoot` returns `(“”, false)` for any error from `cmd.Output()`. `ProjectWriteTarget` interprets `ok==false` as “not in repo or git unavailable” and falls back to `cwd`, which can cause `skillrig init` to write the project config in a subdirectory when `git` fails for reasons other than “not a repo.”

### Issue Context
- `init` calls `config.ProjectWriteTarget(ctx, cwd)` for the project scope.
- `ProjectWriteTarget`’s fallback is correct for “not a repo” / “git missing,” but overly broad for other execution failures.

### Fix Focus Areas
- internal/config/config.go[171-203]
- internal/cli/init.go[113-132]

### Suggested fix
- Consider returning `(string, error)` from `gitRoot` (or an additional error) and only falling back to `cwd` for specific, expected cases (e.g., `exec.ErrNotFound` or known “not a git repository” exit status), while propagating unexpected errors.
- If you keep the boolean API, at least detect context cancellation (`errors.Is(err, context.Canceled)` / `DeadlineExceeded`) and propagate rather than falling back.
- Add a test that simulates a `git` execution failure (injectable seam or helper) and asserts behavior (either explicit failure or explicit fallback) so the contract is pinned.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread specledger/001-init-origin-resolution/plan.md
Comment thread internal/config/config.go
…igin String, scope todo rule

Resolve the three real bugs from the Qodo PR review (#2/#3/#4); #1 dismissed as
a false positive with an AGENTS.md clarification.

- #2 FindProjectConfig now returns (string, bool, error): a non-fs.ErrNotExist
  stat failure (e.g. permission denied on an ancestor .skillrig dir) is surfaced
  as fatal instead of masked as "not found"; ResolveOrigin fails fast. Closes
  the discovery-stage gap symmetric to Load's I/O-fatal path.
- #3 Origin.String() returns "" for the zero Origin (the SourceNone sentinel)
  instead of a misleading "/"; precedence test compares directly.
- #4 gitRoot returns (string, error); ProjectWriteTarget falls back to cwd ONLY
  for expected cases (git absent / not a repo) and propagates unexpected errors
  (context cancellation/timeout, exec failures), so init never writes config to
  the wrong directory.
- #1 (markdown checkbox rule violation): false positive — the cited checkboxes
  are /specledger in-document spec/plan checklists, not work tracking. AGENTS.md
  reworded to scope the rule to work-item tracking and explicitly allow them.

Tests (TDD, real fixtures): zero-origin String, permission-denied walk-up fatal,
cancelled-ctx write-target fatal, non-repo cwd fallback preserved. Gate green:
go test ./... · gofmt · go vet · golangci-lint (0 issues).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@vincenthsh
Copy link
Copy Markdown

Review findings addressed (commit d95b830)

# Finding Resolution
1 Markdown checkbox task list (rule) Dismissed — false positive. /specledger in-doc spec/plan checklists, not work tracking. AGENTS.md reworded to scope the rule to work-item tracking and allow them (see thread).
2 Stat errors silently bypassed Fixed. FindProjectConfig(string,bool,error); non-ErrNotExist stat failures are fatal; resolver fails fast. + TestFindProjectConfigUnstattableIsError.
3 Zero origin prints / Fixed. Origin.String() returns "" for the zero (SourceNone) value. + TestOriginStringZeroIsEmpty.
4 Git root errors masked Fixed. gitRoot(string,error); ProjectWriteTarget falls back to cwd only for git-absent / not-a-repo and propagates unexpected errors (context cancellation/timeout, exec failures). + TestProjectWriteTargetCancelledCtxIsFatal / ...NonRepoFallsBackToCwd.

All fixes are TDD with real-filesystem tests (no mocks). Gate green: go test ./... · gofmt · go vet · golangci-lint (0 issues).

@so0k so0k merged commit ba98d06 into main May 26, 2026
@so0k so0k deleted the 001-init-origin-resolution branch May 26, 2026 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants