diff --git a/.agents/skills/architecture-review/SKILL.md b/.agents/skills/architecture-review/SKILL.md new file mode 100644 index 0000000..bbd73a6 --- /dev/null +++ b/.agents/skills/architecture-review/SKILL.md @@ -0,0 +1,93 @@ +--- +name: architecture-review +description: Use when reviewing a PR, branch, or staged diff in the openboot repo. Audits the change against project invariants from AGENTS.md — subprocess hygiene, HTTP wrapper usage, error wrapping, dry-run gating, UI helper usage, test placement. Trigger when user says "review this", "check this PR", "audit the diff", or asks about whether a change follows project conventions. +--- + +# Architecture review for openboot + +Run this checklist on any diff before approving it. The checks map 1:1 to +the rules in AGENTS.md → "Project-specific conventions" and the +fitness functions in `internal/archtest`. + +## Step 1 — Diff scope + +Confirm what's changed: + +```bash +git diff --stat main...HEAD # files +git diff main...HEAD -- 'internal/**/*.go' # production Go +``` + +If the diff spans more than ~10 files or mixes feature + refactor + +docs, call that out and suggest splitting — one thing per commit is a +project convention. + +## Step 2 — Run the mechanical checks first + +```bash +go vet ./... +make test-unit # includes archtest +golangci-lint run ./... # if available +``` + +These catch ~80% of what a human reviewer would otherwise comment on. +**If they don't pass, stop reviewing and ask the author to fix first.** + +## Step 3 — Architecture invariants (manual review of the diff) + +For each changed file under `internal/` or `cmd/`: + +| Check | Look for | If violated | +|---|---|---| +| Subprocess | New `exec.Command` / `exec.CommandContext` call | Should be in `internal/system` or wrapped in a Runner. Reject + suggest `internal/system.RunCommand`. | +| HTTP | New `http.NewRequest`, `http.Get`, `http.DefaultClient` | Should go through `internal/httputil.Do`. Reject + suggest wrapper. | +| Home dir | `os.Getenv("HOME")`, hardcoded `/Users/...`, hardcoded `"~/"` | Use `os.UserHomeDir()`. | +| Error wrap | `return err` without context | Should be `fmt.Errorf("doing X: %w", err)` unless the error is already wrapped one frame up. | +| UI output | `fmt.Println`, `fmt.Printf` in non-UI packages | Use `ui.Info`, `ui.Success`, etc. (legacy violations are baselined — only flag NEW ones). | +| Dry run | New destructive operation (mkdir, write, exec, network mutate) | Must check `cfg.DryRun` and print `[DRY-RUN] Would X` instead. | +| Embedded data | New `data/*.yaml` or schema change | Confirm fallback path still loads. | +| State path | New file under `~/.openboot/` | Confirm the path is created via `os.UserHomeDir()`, perms are `0o600` for secrets / `0o755` for dirs. | + +## Step 4 — Test layer check + +For each non-trivial change: + +- **Pure logic** → must have an L1 test in `internal//`. If missing, request it. +- **Subprocess interaction** → fake via Runner in `internal//` OR add a + real-subprocess test under `test/integration/` (still L1, no build tag). +- **CLI flag parsing** → table-driven L1 test. +- **Destructive op** → must run cleanly under `--dry-run` in a test. + +If the change is small enough to land without tests (e.g. comment +rewording, typo fix), say so explicitly rather than letting it pass +unmentioned. + +## Step 5 — Behaviour invariants + +| Check | Why it matters | +|---|---| +| Does the change break the curl\|bash install path? | `scripts/install.sh` is the primary install vector. The smoke test CI job covers this — make sure it still passes. | +| Does the change alter the contract schema (config / snapshot JSON)? | If yes, the schema needs updating in `openboot-contract` repo and bumping. Otherwise the L2 contract job will fail. | +| Does the change add or change a CLI flag? | Confirm `--help` output is updated, and old-cli compat job still passes (previous release × new mock server). | +| Does the change touch `data/presets.yaml`? | Three presets must still parse and resolve. | +| Does the change touch `~/.openboot/*` file format? | Confirm backward compat with old files (snapshot, auth, state) or write a migration. | + +## Step 6 — Risk assessment + +End the review with: + +- **Risk:** low / medium / high (one sentence on blast radius). +- **Rollback:** how a user / operator would revert (CLI flag, file delete, + brew downgrade). +- **Observability:** what would tell us this broke in production + (telemetry, GitHub issue pattern, smoke job). + +## What the reviewer SHOULD NOT do + +- Do not approve a change that adds an archtest violation without an + updated `baseline/` file and a justification in the commit message. +- Do not approve mixed-purpose commits — request a split. +- Do not approve changes to `.github/workflows/` without confirming the + workflow still runs (`act` locally or trigger on a draft PR). +- Do not approve direct edits to `data/packages.yaml` without confirming + the source-of-truth is openboot.dev (see AGENTS.md "Where to look"). diff --git a/.agents/skills/bootstrap-feature/SKILL.md b/.agents/skills/bootstrap-feature/SKILL.md new file mode 100644 index 0000000..167509e --- /dev/null +++ b/.agents/skills/bootstrap-feature/SKILL.md @@ -0,0 +1,128 @@ +--- +name: bootstrap-feature +description: >- + Use when adding a new CLI subcommand or feature to openboot. Walks through + the canonical pattern: cobra command registration, runner wiring, archtest + awareness, and test placement. Trigger when the user says "add a command", + "new subcommand", "implement feature X for the CLI", or starts editing + under internal/cli/. +--- + +# Bootstrap a new openboot feature + +This skill gives the canonical recipe for adding a new CLI command or +feature to openboot without violating project invariants. + +## Step 1 — Where the code goes + +| Kind of thing | Location | +|---|---| +| New CLI subcommand | `internal/cli/.go`, register in `internal/cli/root.go` `init()` | +| Subprocess call (any binary) | `internal/system.RunCommand` / `RunCommandSilent` — do **not** call `exec.Command` directly | +| HTTP call (any URL) | `internal/httputil.Do` — handles 429 + Retry-After | +| Path under `~` | `os.UserHomeDir()` — never `os.Getenv("HOME")`, never hardcode `~` | +| User-visible output | `internal/ui.*` helpers — never raw `fmt.Println` | +| Destructive action | guarded by `cfg.DryRun` check | +| Error returned to caller | wrapped: `fmt.Errorf("context: %w", err)` | + +These are enforced (or planned) by `internal/archtest`. The rule that +covers each invariant is listed in [AGENTS.md](../../../AGENTS.md). + +## Step 2 — Cobra command skeleton + +```go +// internal/cli/myverb.go +package cli + +import "github.com/spf13/cobra" + +func newMyVerbCmd() *cobra.Command { + var flag string + cmd := &cobra.Command{ + Use: "myverb [arg]", + Short: "One-line description", + Long: "Longer description if useful.", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + return runMyVerb(cmd.Context(), args[0], flag) + }, + } + cmd.Flags().StringVar(&flag, "flag", "", "Description of the flag") + return cmd +} +``` + +Register it in `internal/cli/root.go`: + +```go +rootCmd.AddCommand(newMyVerbCmd()) +``` + +## Step 3 — Runner interface for testability + +If your feature shells out, **use the Runner pattern** so L1 tests can fake +subprocess calls. Example from `internal/brew/runner.go`: + +```go +type Runner interface { + Output(args ...string) ([]byte, error) + CombinedOutput(args ...string) ([]byte, error) + // ... +} + +// realRunner uses exec.Command — this file is in execAllowedPaths. +type realRunner struct{} + +func New() *Brew { return &Brew{r: &realRunner{}} } +``` + +Then in tests: + +```go +type fakeRunner struct { ... } +func (f *fakeRunner) Output(args ...string) ([]byte, error) { ... } +``` + +This is the pattern that lets the fake-runner half of L1 stay fast and hermetic. + +## Step 4 — Test placement + +| Scope | Tier | Build tag | Where | +|---|---|---|---| +| Pure logic + fakes | L1 | none | `/_test.go` | +| Real subprocess in temp dir | L1 | none | `test/integration/_integration_test.go` | +| Compiled binary, no installs | L3 | `e2e` | `test/e2e/...` | +| Real installs on macOS | L4 (VM) | `e2e,vm` | `test/e2e/...` | + +Default to faked-runner L1 unless the thing you're testing only exists when a real +brew/git/npm is on the path — then add an integration test under `test/integration/` +(no build tag; it runs as part of L1). + +## Step 5 — Verify before committing + +```bash +go vet ./... +make test-unit # ~75s, includes archtest + integration +``` + +If archtest fails with a new violation, fix the code rather than +baselining — the baseline is for *intentional* exceptions and they +require justification in the commit message. + +## Step 6 — Conventional commit + +`feat: add openboot myverb command for X` + +One thing per commit. If you also fixed an unrelated bug along the way, +split it into a separate commit. + +## Common mistakes + +1. **Calling `exec.Command` from the feature file** — refactor through + `internal/system` or add a Runner. archtest will catch this. +2. **Skipping `cfg.DryRun` check** — destructive ops must be a no-op + under `--dry-run`. Print "[DRY-RUN] Would X" instead of doing X. +3. **Hardcoding `~/`** — always `os.UserHomeDir()` then `filepath.Join`. +4. **Raw `fmt.Println`** — use `ui.Info`, `ui.Success`, `ui.Warn`, `ui.Error`. +5. **Forgetting to register the command in `root.go`** — cobra silently + does nothing if `AddCommand` is missed. diff --git a/.agents/skills/ship-pr/SKILL.md b/.agents/skills/ship-pr/SKILL.md new file mode 100644 index 0000000..896d946 --- /dev/null +++ b/.agents/skills/ship-pr/SKILL.md @@ -0,0 +1,335 @@ +--- +name: ship-pr +description: Use when the user is done editing and wants to ship the current branch via a pull request — phrases like "open a PR", "ship it", "ship this", "submit the PR", "let's send it", "merge it", "land this", "提 PR", "提个 PR", "提个 MR". Trigger whenever the user signals a change is finished and should head to the default branch, even if they don't say the word "PR". Walks the whole branch-to-merge flow for any GitHub repo with a mandatory review gate — the steps and their rules live in the body. Do NOT trigger for `gh pr view` / status checks on an existing PR, for draft / WIP PRs the user wants opened but not merged, or for cutting a release tag. +license: MIT +metadata: + category: git + language: en +--- + +# Ship a PR + +The canonical way to move a finished change from a feature branch to the +repo's default branch through a pull request — for **any** GitHub repo, +without hardcoding one project's build commands or check names. + +There are two gates between your branch and the default branch: + +- **Mechanical** — the repo's required CI checks. GitHub already enforces + these via branch protection; let them run. +- **Inferential** — a human-judgment review of the diff. CI catches what + it knows how to check; this catches behaviour, design, test coverage, + risk, and rollback. This gate is the reason the skill exists. + +**Auto-merge is intentionally not used.** `gh pr merge --auto` merges the +moment CI passes, which skips the review gate entirely — that defeats the +purpose. The merge command is run from this session, *after* the diff has +been reviewed. + +## When NOT to use + +- **Draft / WIP PRs** the user wants opened but not merged → not this + flow; if it's already been invoked, just `gh pr create --draft` and stop. +- **Changes to `.github/workflows/`, branch protection, or repo settings** + → these usually need a human in the GitHub UI too; flag it. +- **Cutting a release** (tags like `v1.2.3`) → releases follow the repo's + own release process, not this flow. + +## The flow + +### Step 1 — Confirm the branch is shippable + +```bash +git status -sb # clean except the expected diff? +git rev-parse --abbrev-ref HEAD # the current branch +git fetch origin --quiet # refresh the remote base ref +git remote set-head origin --auto >/dev/null # fetch never creates origin/HEAD; this does +BASE=$(git symbolic-ref --quiet --short refs/remotes/origin/HEAD 2>/dev/null | sed 's@^origin/@@') +BASE=${BASE:-main} # the repo's default branch +git log --oneline "origin/$BASE..HEAD" # commits actually exist +``` + +`$BASE` is the repo's default branch — usually `main`, sometimes `master` +or `develop`. Don't assume `main`; use what was detected. Comparisons run +against the **remote** ref `origin/$BASE` (hence the `git fetch`), so they +match what GitHub will diff the PR against even when the local base branch +is missing or stale — common in worktrees and feature-only checkouts. + +- The remote isn't GitHub (`git remote get-url origin`) → **stop**; this + flow is `gh`-only, tell the user (a GitLab "MR" needs a different flow). +- On the base branch → **stop**, make a feature branch first. +- No commits ahead of `origin/$BASE` → **stop**, nothing to ship. +- A PR may already exist for this branch (`gh pr view` succeeds) → still + push (Step 2) so the PR has the latest commits, then skip Step 3 and + continue at Step 4. + +### Step 2 — Push + +```bash +git push -u origin "$(git rev-parse --abbrev-ref HEAD)" +``` + +If the repo installs a `pre-push` hook (Husky, `core.hooksPath`, etc.) it +runs here automatically — don't re-run the same lint/test suite by hand. +If no hook is installed, that's the user's setup; CI is still the gate. + +### Step 3 — Open the PR + +Check for a PR template first — using it keeps the PR consistent with the +repo's conventions instead of inventing a different shape: + +```bash +ls .github/pull_request_template.md .github/PULL_REQUEST_TEMPLATE.md \ + PULL_REQUEST_TEMPLATE.md pull_request_template.md \ + docs/PULL_REQUEST_TEMPLATE.md docs/pull_request_template.md \ + .github/PULL_REQUEST_TEMPLATE/ 2>/dev/null +``` + +If a template exists, fill in its sections honestly — including any +cross-repo / checklist items (e.g. "needs a docs update?"). Do **not** +discard it by passing a wholly custom `--body`. `gh pr create` without +`--body` tries to open an interactive editor, so fill the template +offline and pass it back: + +```bash +gh pr create --title "" --body-file +``` + +If there's no template, fall back to a sensible default: + +```bash +gh pr create --title "" --body "$(cat <<'EOF' +## Summary + +- + +## Test plan + +- [ ] +EOF +)" +``` + +Title rules: + +- Conventional Commits prefix: `feat:` / `fix:` / `docs:` / `refactor:` / + `test:` / `chore:` / `ci:` / `perf:` / `style:` / `build:` / `revert:`. +- Optional scope: `fix(api):`, `feat(editor):`. +- Keep under ~70 chars. Detail goes in the body. +- Several commits on the branch → write a fresh subject that sums up the + whole PR (squash merge makes it the final commit subject). + +### Step 4 — Wait for CI + +```bash +gh pr checks --watch +``` + +This blocks until every check finishes. (Run straight after `gh pr create` +it can error with "no checks reported" before the first check registers — +wait a few seconds and re-run; exit code 8 just means checks are still +pending.) If a **required** check fails: + +- **Stop. Do not proceed to review or merge.** +- Read the failure (`gh run view --log-failed`), explain it to the user, + and ask whether to fix it now. +- Checks that aren't required (drift sensors, optional coverage, advisory + scans) are informational — flag them, but they don't block the merge. + +If you can't tell which checks are required, the ones branch protection +enforces are; `gh pr checks` marks the rest, and the merge in Step 7 will +refuse anyway if a required one is red. + +### Step 5 — Get a review + +Once CI is green, the diff needs an inferential review — behaviour, design, +test coverage, risk, rollback. Prefer to have the **`@claude` GitHub bot** do +this first pass *on the PR itself*, so the review lives next to the code where +the whole team can see it. Fall back to a local review only when the bot isn't +available. + +**5a — Is the Claude bot wired up for this repo?** + +The bot is the Claude GitHub App driven by `anthropics/claude-code-action`. The +reliable, permission-free signal is a workflow that uses it: + +```bash +grep -rilE 'anthropics/claude-code-action|@claude' .github/workflows 2>/dev/null +``` + +(With repo-admin scope you could also confirm via +`gh api "repos/{owner}/{repo}/installations" --jq '.[].app.slug'`, but that +403s without admin — don't depend on it.) + +- Match found → the bot can review; go to **5b**. +- No match → the app isn't set up here; skip to **5d (local fallback)**. + +**5b — Request the review (or pick up an automated one)** + +```bash +PR=$(gh pr view --json number -q .number) +``` + +Some repos run `claude-code-action` automatically on every push +(`on: pull_request`), so a `claude[bot]` review for the current commit may +already be on its way — check before posting, to avoid asking twice: + +```bash +gh pr view "$PR" --json comments \ + -q '.comments[] | select(.author.login|test("claude|github-actions")) | "\(.author.login)\t\(.createdAt)"' +``` + +(Comment objects carry `createdAt`, not `updatedAt` — and the sticky +comment is edited in place, so an old timestamp can still be a live +review. When in doubt, just mention the bot again.) + +If there's no current bot comment, summon it with a mention. A focused prompt +gets a more useful review than a bare "review this": + +```bash +gh pr comment "$PR" --body "@claude please review this PR — focus on correctness, design, test coverage for the branches it adds, risk, and how it rolls back." +``` + +(`@claude` is the default trigger; a repo can rename it via `trigger_phrase` in +its workflow. If the mention gets no response at all, check the workflow for a +custom phrase.) + +**5c — Wait for the bot, then read its review** + +The bot answers in a single **sticky comment it edits in place** — it shows +progress first (checkboxes like "Analyzing…") and fills in the real review when +done. Wait for it to *finish*; don't act on a half-written comment. It usually +lands in under a minute, occasionally a few minutes under load. + +```bash +for i in $(seq 1 30); do + body=$(gh pr view "$PR" --json comments \ + -q '[.comments[] | select(.author.login|test("claude|github-actions"))] | last | .body') + if [ -n "$body" ] && ! printf '%s' "$body" | grep -qiE 'analyzing|in progress|- \[ \]'; then + printf '%s\n' "$body" && break + fi + sleep 10 +done +``` + +Re-fetch until the body reads as a *completed* review (no lingering +"Analyzing…/in progress" markers). The author is normally `claude[bot]`; +a repo using a custom `github_token` surfaces it as `github-actions[bot]` +instead — either way it's obviously a Claude review by its content. A +workflow configured to submit a formal PR review rather than a comment +shows up under `gh pr view --json reviews` — glance there before declaring +the bot unavailable. If nothing +substantive shows up after a few minutes, treat the bot as unavailable and fall +back. (If the loop times out but a bot comment *is* visible — e.g. a finished +review whose body happens to contain unchecked checkboxes — read it manually +instead of discarding it.) Carry whatever it found into Step 6. + +**5d — Local fallback** + +When the bot isn't wired up, or never returns a finished review, review the +**full PR diff** yourself so the gate still closes: + +```bash +git diff "origin/$BASE"...HEAD +``` + +- If a `/code-review` command or a repo-specific review skill is available, run + it on the full diff — it's purpose-built for this. +- Otherwise review inline: behaviour, design, test coverage for the branches + this PR adds, risk, and how it rolls back. + +Either way, the findings feed into Step 6. + +### Step 6 — Triage the findings + +Every finding lands in exactly one bucket. Getting this right is what +keeps the gate from failing open. + +**Self-fixable → fix now, then loop back to Step 4.** Mechanical +corrections clearly inside the PR's stated scope: typos, formatting, doc +wording, dead code this PR introduced, missing imports, missing tests for +branches this PR adds, bug fixes that don't change observable behaviour, +or following through on a rule the user already stated this session. Fix +it, push to the same branch, wait for CI again, then re-request the +review: capture the current sticky-comment body first, mention `@claude` +again, and poll (as in 5c) until the body *changes* from what you captured +and reads as complete — the sticky comment is edited in place, so the +pre-fix review stays visible (and satisfies 5c's break condition) until +the new one lands. Then re-triage. Don't +prompt the user — the point is to spend their attention only on real +decisions. + +**Needs user judgment → surface and stop.** Anything with a genuine +choice: design / API shape / behaviour changes, anything touching a +deliberate prior decision or an existing convention, anything that would +expand the PR beyond its stated scope — anything you'd ask a teammate +about before pushing. State the file/line, the option, and the question; +stop the flow. + +**Clean → merge directly.** If CI is green AND nothing is self-fixable +AND nothing needs judgment, go straight to Step 7. Don't ask "want me to +merge?" — asking when there's nothing to decide just burns attention. The +loop is supposed to close itself. + +Rule of thumb: would a thoughtful engineer file this as a question, push +a follow-up commit, or just merge it? Escalate / fix / merge accordingly. + +### Step 7 — Merge + +Reached on a clean review, or after the user approved a merge following an +escalation. + +```bash +gh pr merge --squash --delete-branch +``` + +- **No `--auto`** — it skips the review gate (Step 5). +- **No `--admin`** — it bypasses branch protection. +- `--squash` keeps the default branch at one commit per PR; if the repo + only allows merge commits or rebase, use `--merge` / `--rebase` instead + (`gh` errors and tells you if the method isn't enabled). +- Branch protection still applies — if something flipped red since Step 4, + GitHub refuses and you loop back to Step 4. +- Refused for missing approvals / unresolved conversations → that's an + org-policy gate, not CI; looping back to Step 4 can never clear it. + Surface it to the user instead. + +Report the result as a one-liner ("PR #N merged, branch deleted, local +synced"). On a clean review, don't ask for confirmation *before* merging. + +### Step 8 — Local cleanup + +`--delete-branch` deleted the local *and* remote branch, and `gh` already +switched the checkout back to the base branch — so don't `git branch -d` +afterwards; the branch is gone and the command just errors. The one thing +`gh` doesn't do is pull: + +```bash +git pull --ff-only +``` + +If the merge ran from somewhere unusual (a worktree, detached HEAD) and +the local branch survived, delete it manually with `git branch -d`. This +step is part of the loop, not optional: because the merge is synchronous +(no `--auto`), there's no reason to leave the checkout behind the base. + +## What NOT to do + +- **No `--auto` by default** — it skips the review gate, the whole reason + for this skill. If the user explicitly asks for it: don't arm it + silently — say what it skips, and offer to close the gate now (review + the diff first, then arm auto-merge; at that point `--auto` only skips + the CI wait). If they still want it after being told, do it: their + repo, their call. Caveat: auto-merge stays armed across later pushes, + so disarm or re-review before pushing anything else. +- **No `--admin`** — bypasses branch protection. +- **Don't merge before CI finishes** — even a trivial-looking diff; drift + checks sometimes catch surprising things. +- **Don't amend / force-push after `gh pr create`** unless the user asks — + it invalidates in-flight reviews and re-runs CI from scratch. +- **Don't push directly to the default branch** — branch protection + rejects it, and it skips both gates. +- **Don't auto-fix findings that need judgment** — behaviour changes, + scope expansion, or anything touching a prior deliberate choice gets + surfaced in Step 6, not silently committed. What counts as self-fixable + is exactly Step 6's first bucket — when in doubt, escalate. diff --git a/.agents/skills/ship-pr/agents/openai.yaml b/.agents/skills/ship-pr/agents/openai.yaml new file mode 100644 index 0000000..73e074d --- /dev/null +++ b/.agents/skills/ship-pr/agents/openai.yaml @@ -0,0 +1,4 @@ +interface: + display_name: "Ship PR" + short_description: "Ship a finished branch through a reviewed PR" + default_prompt: "Use $ship-pr to ship this finished branch through CI, review, and merge." diff --git a/.claude/skills/architecture-review/SKILL.md b/.claude/skills/architecture-review/SKILL.md deleted file mode 100644 index 06f0cc4..0000000 --- a/.claude/skills/architecture-review/SKILL.md +++ /dev/null @@ -1,93 +0,0 @@ ---- -name: architecture-review -description: Use when reviewing a PR, branch, or staged diff in the openboot repo. Audits the change against project invariants from CLAUDE.md — subprocess hygiene, HTTP wrapper usage, error wrapping, dry-run gating, UI helper usage, test placement. Trigger when user says "review this", "check this PR", "audit the diff", or asks about whether a change follows project conventions. ---- - -# Architecture review for openboot - -Run this checklist on any diff before approving it. The checks map 1:1 to -the rules in CLAUDE.md → "Project-specific conventions" and the -fitness functions in `internal/archtest`. - -## Step 1 — Diff scope - -Confirm what's changed: - -```bash -git diff --stat main...HEAD # files -git diff main...HEAD -- 'internal/**/*.go' # production Go -``` - -If the diff spans more than ~10 files or mixes feature + refactor + -docs, call that out and suggest splitting — one thing per commit is a -project convention. - -## Step 2 — Run the mechanical checks first - -```bash -go vet ./... -make test-unit # includes archtest -golangci-lint run ./... # if available -``` - -These catch ~80% of what a human reviewer would otherwise comment on. -**If they don't pass, stop reviewing and ask the author to fix first.** - -## Step 3 — Architecture invariants (manual review of the diff) - -For each changed file under `internal/` or `cmd/`: - -| Check | Look for | If violated | -|---|---|---| -| Subprocess | New `exec.Command` / `exec.CommandContext` call | Should be in `internal/system` or wrapped in a Runner. Reject + suggest `internal/system.RunCommand`. | -| HTTP | New `http.NewRequest`, `http.Get`, `http.DefaultClient` | Should go through `internal/httputil.Do`. Reject + suggest wrapper. | -| Home dir | `os.Getenv("HOME")`, hardcoded `/Users/...`, hardcoded `"~/"` | Use `os.UserHomeDir()`. | -| Error wrap | `return err` without context | Should be `fmt.Errorf("doing X: %w", err)` unless the error is already wrapped one frame up. | -| UI output | `fmt.Println`, `fmt.Printf` in non-UI packages | Use `ui.Info`, `ui.Success`, etc. (legacy violations are baselined — only flag NEW ones). | -| Dry run | New destructive operation (mkdir, write, exec, network mutate) | Must check `cfg.DryRun` and print `[DRY-RUN] Would X` instead. | -| Embedded data | New `data/*.yaml` or schema change | Confirm fallback path still loads. | -| State path | New file under `~/.openboot/` | Confirm the path is created via `os.UserHomeDir()`, perms are `0o600` for secrets / `0o755` for dirs. | - -## Step 4 — Test layer check - -For each non-trivial change: - -- **Pure logic** → must have an L1 test in `internal//`. If missing, request it. -- **Subprocess interaction** → fake via Runner in `internal//` OR add a - real-subprocess test under `test/integration/` (still L1, no build tag). -- **CLI flag parsing** → table-driven L1 test. -- **Destructive op** → must run cleanly under `--dry-run` in a test. - -If the change is small enough to land without tests (e.g. comment -rewording, typo fix), say so explicitly rather than letting it pass -unmentioned. - -## Step 5 — Behaviour invariants - -| Check | Why it matters | -|---|---| -| Does the change break the curl\|bash install path? | `scripts/install.sh` is the primary install vector. The smoke test CI job covers this — make sure it still passes. | -| Does the change alter the contract schema (config / snapshot JSON)? | If yes, the schema needs updating in `openboot-contract` repo and bumping. Otherwise the L2 contract job will fail. | -| Does the change add or change a CLI flag? | Confirm `--help` output is updated, and old-cli compat job still passes (previous release × new mock server). | -| Does the change touch `data/presets.yaml`? | Three presets must still parse and resolve. | -| Does the change touch `~/.openboot/*` file format? | Confirm backward compat with old files (snapshot, auth, state) or write a migration. | - -## Step 6 — Risk assessment - -End the review with: - -- **Risk:** low / medium / high (one sentence on blast radius). -- **Rollback:** how a user / operator would revert (CLI flag, file delete, - brew downgrade). -- **Observability:** what would tell us this broke in production - (telemetry, GitHub issue pattern, smoke job). - -## What the reviewer SHOULD NOT do - -- Do not approve a change that adds an archtest violation without an - updated `baseline/` file and a justification in the commit message. -- Do not approve mixed-purpose commits — request a split. -- Do not approve changes to `.github/workflows/` without confirming the - workflow still runs (`act` locally or trigger on a draft PR). -- Do not approve direct edits to `data/packages.yaml` without confirming - the source-of-truth is openboot.dev (see CLAUDE.md "Where to Look"). diff --git a/.claude/skills/architecture-review/SKILL.md b/.claude/skills/architecture-review/SKILL.md new file mode 120000 index 0000000..a6e6a8b --- /dev/null +++ b/.claude/skills/architecture-review/SKILL.md @@ -0,0 +1 @@ +../../../.agents/skills/architecture-review/SKILL.md \ No newline at end of file diff --git a/.claude/skills/bootstrap-feature/SKILL.md b/.claude/skills/bootstrap-feature/SKILL.md deleted file mode 100644 index faa3fc2..0000000 --- a/.claude/skills/bootstrap-feature/SKILL.md +++ /dev/null @@ -1,123 +0,0 @@ ---- -name: bootstrap-feature -description: Use when adding a new CLI subcommand or feature to openboot. Walks through the canonical pattern: cobra command registration, runner wiring, archtest awareness, where to put tests. Trigger when the user says "add a command", "new subcommand", "implement feature X for the CLI", or starts editing under internal/cli/. ---- - -# Bootstrap a new openboot feature - -This skill gives the canonical recipe for adding a new CLI command or -feature to openboot without violating project invariants. - -## Step 1 — Where the code goes - -| Kind of thing | Location | -|---|---| -| New CLI subcommand | `internal/cli/.go`, register in `internal/cli/root.go` `init()` | -| Subprocess call (any binary) | `internal/system.RunCommand` / `RunCommandSilent` — do **not** call `exec.Command` directly | -| HTTP call (any URL) | `internal/httputil.Do` — handles 429 + Retry-After | -| Path under `~` | `os.UserHomeDir()` — never `os.Getenv("HOME")`, never hardcode `~` | -| User-visible output | `internal/ui.*` helpers — never raw `fmt.Println` | -| Destructive action | guarded by `cfg.DryRun` check | -| Error returned to caller | wrapped: `fmt.Errorf("context: %w", err)` | - -These are enforced (or planned) by `internal/archtest`. The rule that -covers each invariant is listed in [AGENTS.md](../../../AGENTS.md). - -## Step 2 — Cobra command skeleton - -```go -// internal/cli/myverb.go -package cli - -import "github.com/spf13/cobra" - -func newMyVerbCmd() *cobra.Command { - var flag string - cmd := &cobra.Command{ - Use: "myverb [arg]", - Short: "One-line description", - Long: "Longer description if useful.", - Args: cobra.ExactArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - return runMyVerb(cmd.Context(), args[0], flag) - }, - } - cmd.Flags().StringVar(&flag, "flag", "", "Description of the flag") - return cmd -} -``` - -Register it in `internal/cli/root.go`: - -```go -rootCmd.AddCommand(newMyVerbCmd()) -``` - -## Step 3 — Runner interface for testability - -If your feature shells out, **use the Runner pattern** so L1 tests can fake -subprocess calls. Example from `internal/brew/runner.go`: - -```go -type Runner interface { - Output(args ...string) ([]byte, error) - CombinedOutput(args ...string) ([]byte, error) - // ... -} - -// realRunner uses exec.Command — this file is in execAllowedPaths. -type realRunner struct{} - -func New() *Brew { return &Brew{r: &realRunner{}} } -``` - -Then in tests: - -```go -type fakeRunner struct { ... } -func (f *fakeRunner) Output(args ...string) ([]byte, error) { ... } -``` - -This is the pattern that lets the fake-runner half of L1 stay fast and hermetic. - -## Step 4 — Test placement - -| Scope | Tier | Build tag | Where | -|---|---|---|---| -| Pure logic + fakes | L1 | none | `/_test.go` | -| Real subprocess in temp dir | L1 | none | `test/integration/_integration_test.go` | -| Compiled binary, no installs | L3 | `e2e` | `test/e2e/...` | -| Real installs on macOS | L4 (VM) | `e2e,vm` | `test/e2e/...` | - -Default to faked-runner L1 unless the thing you're testing only exists when a real -brew/git/npm is on the path — then add an integration test under `test/integration/` -(no build tag; it runs as part of L1). - -## Step 5 — Verify before committing - -```bash -go vet ./... -make test-unit # ~75s, includes archtest + integration -``` - -If archtest fails with a new violation, fix the code rather than -baselining — the baseline is for *intentional* exceptions and they -require justification in the commit message. - -## Step 6 — Conventional commit - -`feat: add openboot myverb command for X` - -One thing per commit. If you also fixed an unrelated bug along the way, -split it into a separate commit. - -## Common mistakes - -1. **Calling `exec.Command` from the feature file** — refactor through - `internal/system` or add a Runner. archtest will catch this. -2. **Skipping `cfg.DryRun` check** — destructive ops must be a no-op - under `--dry-run`. Print "[DRY-RUN] Would X" instead of doing X. -3. **Hardcoding `~/`** — always `os.UserHomeDir()` then `filepath.Join`. -4. **Raw `fmt.Println`** — use `ui.Info`, `ui.Success`, `ui.Warn`, `ui.Error`. -5. **Forgetting to register the command in `root.go`** — cobra silently - does nothing if `AddCommand` is missed. diff --git a/.claude/skills/bootstrap-feature/SKILL.md b/.claude/skills/bootstrap-feature/SKILL.md new file mode 120000 index 0000000..4ea3a47 --- /dev/null +++ b/.claude/skills/bootstrap-feature/SKILL.md @@ -0,0 +1 @@ +../../../.agents/skills/bootstrap-feature/SKILL.md \ No newline at end of file diff --git a/.claude/skills/ship-pr/SKILL.md b/.claude/skills/ship-pr/SKILL.md new file mode 120000 index 0000000..44f0db4 --- /dev/null +++ b/.claude/skills/ship-pr/SKILL.md @@ -0,0 +1 @@ +../../../.agents/skills/ship-pr/SKILL.md \ No newline at end of file diff --git a/AGENTS.md b/AGENTS.md index 95e5fcb..32924dc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,81 +1,196 @@ # AGENTS.md -Canonical pointer for AI coding agents working on this repo. If you're a -human, you probably want [README.md](README.md) or [CONTRIBUTING.md](CONTRIBUTING.md). +Canonical, tool-neutral instructions for coding agents working on this repo. +Codex reads this file directly; Claude Code imports it from `CLAUDE.md`. +If you're a human, you probably want [README.md](README.md) or +[CONTRIBUTING.md](CONTRIBUTING.md). + +## Agent compatibility + +- Keep shared repository guidance in this file. Do not duplicate it in a + tool-specific instruction file. +- `CLAUDE.md` is a thin Claude Code adapter: it imports this file with + `@AGENTS.md` and may contain only Claude-specific additions. +- Shared skills use the open Agent Skills format and live in + `.agents/skills//SKILL.md`. Claude Code discovers the same skills + through relative symlinks under `.claude/skills/`. +- Other tools that implement `AGENTS.md` or Agent Skills can consume the same + canonical paths without another copy of the instructions. +- Tool-specific integration stays tool-specific. Claude Code hooks and + permissions remain in `.claude/settings.json` and `.claude/hooks/`. + +When adding a shared skill, create it under `.agents/skills/`, then expose it +to Claude Code without copying its contents: + +```bash +mkdir -p .claude/skills/ +ln -s ../../../.agents/skills//SKILL.md .claude/skills//SKILL.md +``` + +The `agent-config` architecture test verifies the import and skill adapters. ## Read first -- **[CLAUDE.md](CLAUDE.md)** — project conventions and where-to-look table. - Treat this as authoritative; this file is a summary index for agents. -- **[CONTRIBUTING.md](CONTRIBUTING.md)** — test layering L1–L4, Runner - interface, hook setup. +- **[CONTRIBUTING.md](CONTRIBUTING.md)** — test layering L1-L4, Runner + interface, and hook setup. - **[docs/HARNESS.md](docs/HARNESS.md)** — the steering meta-doc: when a - class of issue recurs, which file do you edit to prevent it next time. + class of issue recurs, which file to edit to prevent it next time. +- **[internal/archtest/README.md](internal/archtest/README.md)** — architecture + fitness functions and baseline workflow. -## Project invariants (the things that must not drift) +## Project -These are enforced by [`internal/archtest`](internal/archtest/README.md) as -fitness functions. New violations fail `make test-unit` (L1). If you're an -agent and a violation is intentional, the test message tells you how to -update the baseline — do not silence the rule. +OpenBoot is a **macOS-only** Go 1.25 CLI that automates dev-environment setup: Homebrew packages/casks, npm globals, Oh-My-Zsh, macOS `defaults`, and dotfiles. Built on **Cobra** (CLI) + **Charmbracelet** (bubbletea / lipgloss / huh for TUI). -| Invariant | Enforced by | -|---|---| -| `exec.Command` only in `internal/system` and documented runner packages | `internal/archtest/exec_test.go` | -| `http.NewRequest` / `http.DefaultClient` only in `internal/httputil` | `internal/archtest/http_test.go` | -| Use `os.UserHomeDir()` — never `os.Getenv("HOME")` | `internal/archtest/envhome_test.go` | -| Error wrapping with `%w`, no bare returns | reviewer + `errcheck` (golangci-lint) | -| UI output via `ui.*` helpers — never raw `fmt.Println` in user-facing paths | `internal/archtest/fmtprint_test.go` | -| Destructive ops check `cfg.DryRun` before acting | `internal/archtest/dryrun_test.go` | +Entry point: `cmd/openboot/main.go` -> `internal/cli.Execute()`. +Core flow: `openboot install` orchestrates plan -> apply in `internal/installer/installer.go`. -The full convention list lives in CLAUDE.md → "Project-specific conventions". +**The wizard plans; the apply is always linear.** On a TTY the planning phase runs as a full-screen TUI in `internal/ui/tui/wizard/` (boot probe -> select -> git -> review), then the wizard *exits* and hands its `InstallPlan` to `installer.ApplyReviewedPlan`, which applies it on the normal terminal with `ConsoleReporter` + `ui.StickyProgress`. This split is deliberate: a TUI is right for browsing a 100+ package catalog, and wrong for the apply — an alt-screen install discards its own output when it exits, so twenty minutes of package results and failures vanish. Streamed into the scrollback they stay where the user can scroll back, copy an error, and pipe it. Don't move the apply back inside the alt-screen. -## Run before committing +Entry points: bare `install` -> `wizard.Run`; `-p ` -> same, loadout preselected; slug/`-u`/`--from`/alias -> `wizard.RunForConfig` (config mode: the config's own packages on the select screen, preselected). Sync-source installs keep their linear diff pre-flight and apply linearly. `--silent`, `--dry-run`, `--update`, `--pick`, and non-TTY runs never enter the wizard. + +## Working in parallel + +This repo is often worked on from multiple concurrent terminals / agent sessions. Default to `git worktree add ../openboot- -b ` rather than juggling branches in a single checkout — two sessions racing on the same working tree corrupts state (mid-edit files, half-staged diffs, hooks firing against the wrong branch). One worktree per concurrent task; remove it with `git worktree remove` when the PR merges. + +## Commands ```bash -go vet ./... # cheap, ~1s -go test ./internal/... # L1, ~15s, includes archtest +# Build — version injected via -ldflags, default is "dev" +make build +make build-release VERSION=0.25.0 # optimized + UPX + +# Test — full tier table in CONTRIBUTING.md +make test-unit # L1 (~75s) — unit + integration + contract; pre-push hook +make test-e2e # L3 compiled binary + # L4 — destructive e2e runs in CI only (vm-e2e-spike.yml on macos-14) +make test-coverage # coverage.out + coverage.html + +# Single test +go test -v -run TestFoo ./internal//... + +# Hooks (opt-in): pre-commit = vet+build, pre-push = L1 +make install-hooks + +go vet ./... +make clean ``` -Both are wired into `scripts/hooks/pre-commit` (vet only) and -`scripts/hooks/pre-push` (full L1). Install once with `make install-hooks`. +## Layout + +```text +cmd/openboot/ # main.go -> cli.Execute() +internal/ + auth/ # OAuth-like login, token in ~/.openboot/auth.json (0600) + brew/ # Homebrew ops, sequential install with retry, uninstall + cli/ # Cobra cmds: install, snapshot, login, logout, version + config/ # Package catalog + presets + remote fetch (embed fallback in data/) + diff/ # Pure-logic system-vs-config comparison + dotfiles/ # Clone + stow with .openboot.bak backup + httputil/ # HTTP Do() with rate-limit + Retry-After + installer/ # 7-step wizard orchestrator + snapshot restore + macos/ # defaults write + app restart + npm/ # Batch install with sequential fallback + permissions/ # macOS screen-recording probe + search/ # openboot.dev API client (8s timeout) + shell/ # Oh-My-Zsh install + .zshrc + snapshot restore + snapshot/ # Capture / match / restore env state + state/ # Reminder state in ~/.openboot/state.json + sync/ # Compute diff + execute plan for remote config + system/ # RunCommand / RunCommandSilent, arch, git config + ui/ # bubbletea Model pattern, lipgloss styling + updater/ # Auto-update: check GitHub -> download -> replace + doctor/ # Diagnostic checks for openboot doctor command + logging/ # Structured rotating file log under ~/.openboot/logs/ +test/{integration,e2e}/ # integration runs as part of L1; e2e gated by build tags (e2e, vm) +testutil/ # shared helpers + MacHost (destructive E2E on real macOS) +scripts/ + install.sh # curl|bash installer + hooks/ # pre-commit, pre-push (install via `make install-hooks`) +``` + +## Where to look + +| Task | Location | Notes | +|------|----------|-------| +| Add CLI command | `internal/cli/` | Register in `root.go init()`, follow cobra pattern | +| Change install flow | `internal/installer/installer.go` | plan -> apply orchestrator; `PlanFromSelection` builds a plan from TUI picks | +| Change interactive install TUI | `internal/ui/tui/wizard/` | Redesign v5: boot/select/install screens; live install streams `internal/progress` events (brew/npm `SetProgressSink`) | +| Change sync behavior | `internal/sync/diff.go`, `internal/sync/plan.go` | Diff -> confirm -> execute | +| Add package category | `openboot.dev/src/lib/package-metadata.ts` | Server is source of truth; CLI fetches `/api/packages` and caches 24h in `~/.openboot/packages-cache.json`. `data/packages.yaml` is fallback only. | +| Modify presets | `internal/config/data/presets.yaml` | 3 presets: minimal, developer, full | +| Change brew behavior | `internal/brew/brew.go` + `brew_install.go` | Parallel workers, StickyProgress, Uninstall/UninstallCask | +| Cask download progress | `internal/brew/cache.go` + `internal/brew/sizecheck.go` | HEAD pre-fetch -> poll `brew --cache` for bytes; consumed by `installCasksWithProgress` | +| Add snapshot data | `internal/snapshot/capture.go` | Extend `CaptureWithProgress` steps | +| Update self-update | `internal/updater/updater.go` | `AutoUpgrade()` called from `root.go` RunE | +| Change publish flow | `internal/cli/snapshot_publish.go` (`publishSnapshot`) | Slug resolution | +| Source resolution (install) | `internal/cli/install.go` (`resolvePositionalArg`) | file / user-slug / preset / alias detection | +| HTTP with retry | `internal/httputil/ratelimit.go` | Use `httputil.Do()` — handles 429 + Retry-After (archtest: `no-raw-http`) | +| Test tier / when to run | `CONTRIBUTING.md` "Test Layering" | L1-L4 table | +| Release process | `.github/workflows/` | Tag-driven, release-notes template | + +## Project-specific conventions + +These cannot be inferred from code alone — everything else is enforced by `go vet` / review. +Bolded rules are enforced mechanically by `internal/archtest` (fitness functions in L1). + +- **Error wrapping**: `fmt.Errorf("context: %w", err)` — never bare returns. +- **UI output** *(archtest: `fmtprint`)*: always through `ui.*` helpers; raw `fmt.Println` is a bug in user-facing paths. +- **Subprocess** *(archtest: `no-direct-exec`)*: `system.RunCommand` (interactive) / `system.RunCommandSilent` (captured). Do not call `exec.Command` directly from feature code — add to `system/` if a wrapper is missing. +- **Destructive ops** *(archtest: `dryrun`)*: check `cfg.DryRun` first. Always. +- **Paths** *(archtest: `no-os-getenv-home`)*: `os.UserHomeDir()` — never hardcode `~` or `/Users/...`, never `os.Getenv("HOME")`. +- **State**: everything user-local goes under `~/.openboot/` (auth, cache, snapshots, state). +- **Concurrency**: bounded `sync.WaitGroup` — brew install is sequential with retry; `GetInstalledPackages` uses 2 goroutines for formula+cask list. No unbounded goroutines. +- **Embedded data**: `//go:embed data/*.yaml` loaded in `init()`. +- **Tests**: table-driven, `testify/require` for fatal, `testify/assert` for non-fatal. L1 uses the `Runner` interface to fake subprocess calls — no real network, no real fork. +- **Commits**: Conventional (`feat:` / `fix:` / `docs:` / `refactor:` / `test:` / `chore:` / `ci:`), one thing per commit. ## When archtest fails -The failure message tells you exactly what to do. Two cases: +The failure message tells you exactly what to do: -1. **You added a new violation by accident** — fix the code (move the call - into the allowed package, use the wrapper, etc.). -2. **The violation is intentional** — append the new `file:line` to - `internal/archtest/baseline/.txt` via: +1. If you added a violation by accident, fix the code by using the allowed + wrapper or moving the call into the appropriate package. +2. If the violation is intentional, update the baseline: ```bash ARCHTEST_UPDATE_BASELINE=1 go test ./internal/archtest/... git add internal/archtest/baseline/ ``` - The diff to the baseline file IS the audit trail — explain in your - commit message why the new call site is justified. - -## Skills +The baseline diff is the audit trail. Explain why the new call site is +justified in the commit message; do not silence the rule. -Project-specific Claude skills live under [`.claude/skills/`](.claude/skills/): +## Run before committing -- `bootstrap-feature` — how to add a CLI command end-to-end. -- `architecture-review` — what to check when reviewing a PR against - project invariants. +```bash +go vet ./... # cheap, ~1s +go test ./internal/... # L1, ~15s, includes archtest +``` -These are loaded automatically when Claude runs in this repo. +Both are wired into `scripts/hooks/pre-commit` (vet only) and +`scripts/hooks/pre-push` (full L1). Install once with `make install-hooks`. -## Tools you may NOT use (without asking) +## Actions that require confirmation - `git push --force` against `main` or release tags. - `git commit --amend` on commits already pushed. -- `git reset --hard` discarding uncommitted work. -- Triggering L4 e2e tests outside CI — they install real packages onto the - current host. L4 runs only in GitHub Actions (`vm-e2e-spike.yml`). -- Anything that modifies the user's `~/.zshrc`, Homebrew install, or - macOS `defaults`. - -Everything else (Edit/Write/Read in repo, `make test-unit`, `go vet`, etc.) -is safe to run without confirmation. +- `git reset --hard` when it would discard uncommitted work. +- Triggering L4 e2e tests outside CI; they install real packages on the host. +- Modifying the user's `~/.zshrc`, Homebrew installation, or macOS `defaults`. + +Repository reads and edits, `make test-unit`, and `go vet` are safe without +extra confirmation. + +## Env vars + +| Var | Purpose | +|-----|---------| +| `OPENBOOT_DISABLE_AUTOUPDATE=1` | Skip auto-update check | +| `OPENBOOT_GIT_NAME` / `OPENBOOT_GIT_EMAIL` | Git identity override (silent mode) | +| `OPENBOOT_PRESET` | Default preset | +| `OPENBOOT_USER` | Config alias/username | +| `OPENBOOT_API_URL` | Override API base URL (testing; https or http://localhost only) | +| `OPENBOOT_DOTFILES` | Override dotfiles repo URL | +| `OPENBOOT_DRY_RUN` | Dry-run mode for `scripts/install.sh` (not the CLI) | +| `OPENBOOT_VERSION` | Pin version in `scripts/install.sh` | diff --git a/CLAUDE.md b/CLAUDE.md index be1f1a0..a685204 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,124 +1,10 @@ # CLAUDE.md -## Project +@AGENTS.md -OpenBoot is a **macOS-only** Go 1.25 CLI that automates dev-environment setup: Homebrew packages/casks, npm globals, Oh-My-Zsh, macOS `defaults`, and dotfiles. Built on **Cobra** (CLI) + **Charmbracelet** (bubbletea / lipgloss / huh for TUI). +## Claude Code integration -Entry point: `cmd/openboot/main.go` → `internal/cli.Execute()`. -Core flow: `openboot install` orchestrates plan → apply in `internal/installer/installer.go`. - -**The wizard plans; the apply is always linear.** On a TTY the planning phase runs as a full-screen TUI in `internal/ui/tui/wizard/` (boot probe → select → git → review), then the wizard *exits* and hands its `InstallPlan` to `installer.ApplyReviewedPlan`, which applies it on the normal terminal with `ConsoleReporter` + `ui.StickyProgress`. This split is deliberate: a TUI is right for browsing a 100+ package catalog, and wrong for the apply — an alt-screen install discards its own output when it exits, so twenty minutes of package results and failures vanish. Streamed into the scrollback they stay where the user can scroll back, copy an error, and pipe it. Don't move the apply back inside the alt-screen. - -Entry points: bare `install` → `wizard.Run`; `-p ` → same, loadout preselected; slug/`-u`/`--from`/alias → `wizard.RunForConfig` (config mode: the config's own packages on the select screen, preselected). Sync-source installs keep their linear diff pre-flight and apply linearly. `--silent`, `--dry-run`, `--update`, `--pick`, and non-TTY runs never enter the wizard. - -For full contribution guide (test layering L1–L4, Runner interface, hook setup) see @CONTRIBUTING.md. -For AI agents: @AGENTS.md indexes invariants enforced by `internal/archtest`; @docs/HARNESS.md is the steering meta-doc for where to encode new rules. - -## Working in parallel - -This repo is often worked on from multiple concurrent terminals / agent sessions. Default to `git worktree add ../openboot- -b ` rather than juggling branches in a single checkout — two sessions racing on the same working tree corrupts state (mid-edit files, half-staged diffs, hooks firing against the wrong branch). One worktree per concurrent task; remove it with `git worktree remove` when the PR merges. - -## Commands - -```bash -# Build — version injected via -ldflags, default is "dev" -make build -make build-release VERSION=0.25.0 # optimized + UPX - -# Test — full tier table in CONTRIBUTING.md -make test-unit # L1 (~75s) — unit + integration + contract; pre-push hook -make test-e2e # L3 compiled binary - # L4 — destructive e2e runs in CI only (vm-e2e-spike.yml on macos-14) -make test-coverage # coverage.out + coverage.html - -# Single test -go test -v -run TestFoo ./internal//... - -# Hooks (opt-in): pre-commit = vet+build, pre-push = L1 -make install-hooks - -go vet ./... -make clean -``` - -## Layout - -``` -cmd/openboot/ # main.go → cli.Execute() -internal/ - auth/ # OAuth-like login, token in ~/.openboot/auth.json (0600) - brew/ # Homebrew ops, sequential install with retry, uninstall - cli/ # Cobra cmds: install, snapshot, login, logout, version - config/ # Package catalog + presets + remote fetch (embed fallback in data/) - diff/ # Pure-logic system-vs-config comparison - dotfiles/ # Clone + stow with .openboot.bak backup - httputil/ # HTTP Do() with rate-limit + Retry-After - installer/ # 7-step wizard orchestrator + snapshot restore - macos/ # defaults write + app restart - npm/ # Batch install with sequential fallback - permissions/ # macOS screen-recording probe - search/ # openboot.dev API client (8s timeout) - shell/ # Oh-My-Zsh install + .zshrc + snapshot restore - snapshot/ # Capture / match / restore env state - state/ # Reminder state in ~/.openboot/state.json - sync/ # Compute diff + execute plan for remote config - system/ # RunCommand / RunCommandSilent, arch, git config - ui/ # bubbletea Model pattern, lipgloss styling - updater/ # Auto-update: check GitHub → download → replace - doctor/ # Diagnostic checks for openboot doctor command - logging/ # Structured rotating file log under ~/.openboot/logs/ -test/{integration,e2e}/ # integration runs as part of L1; e2e gated by build tags (e2e, vm) -testutil/ # shared helpers + MacHost (destructive E2E on real macOS) -scripts/ - install.sh # curl|bash installer - hooks/ # pre-commit, pre-push (install via `make install-hooks`) -``` - -## Where to Look - -| Task | Location | Notes | -|------|----------|-------| -| Add CLI command | `internal/cli/` | Register in `root.go init()`, follow cobra pattern | -| Change install flow | `internal/installer/installer.go` | plan → apply orchestrator; `PlanFromSelection` builds a plan from TUI picks | -| Change interactive install TUI | `internal/ui/tui/wizard/` | Redesign v5: boot/select/install screens; live install streams `internal/progress` events (brew/npm `SetProgressSink`) | -| Change sync behavior | `internal/sync/diff.go`, `internal/sync/plan.go` | Diff → confirm → execute | -| Add package category | `openboot.dev/src/lib/package-metadata.ts` | Server is source of truth; CLI fetches `/api/packages` and caches 24h in `~/.openboot/packages-cache.json`. `data/packages.yaml` is fallback only. | -| Modify presets | `internal/config/data/presets.yaml` | 3 presets: minimal, developer, full | -| Change brew behavior | `internal/brew/brew.go` + `brew_install.go` | Parallel workers, StickyProgress, Uninstall/UninstallCask | -| Cask download progress | `internal/brew/cache.go` + `internal/brew/sizecheck.go` | HEAD pre-fetch → poll `brew --cache` for bytes; consumed by `installCasksWithProgress` | -| Add snapshot data | `internal/snapshot/capture.go` | Extend `CaptureWithProgress` steps | -| Update self-update | `internal/updater/updater.go` | `AutoUpgrade()` called from `root.go` RunE | -| Change publish flow | `internal/cli/snapshot_publish.go` (`publishSnapshot`) | Slug resolution | -| Source resolution (install) | `internal/cli/install.go` (`resolvePositionalArg`) | file / user-slug / preset / alias detection | -| HTTP with retry | `internal/httputil/ratelimit.go` | Use `httputil.Do()` — handles 429 + Retry-After (archtest: `no-raw-http`) | -| Test tier / when to run | `CONTRIBUTING.md` "Test Layering" | L1–L4 table | -| Release process | `.github/workflows/` | Tag-driven, release-notes template | - -## Project-specific conventions - -These cannot be inferred from code alone — everything else is enforced by `go vet` / review. -Bolded rules are enforced mechanically by `internal/archtest` (fitness functions in L1). - -- **Error wrapping**: `fmt.Errorf("context: %w", err)` — never bare returns. -- **UI output** *(archtest: `fmtprint`)*: always through `ui.*` helpers; raw `fmt.Println` is a bug in user-facing paths. -- **Subprocess** *(archtest: `no-direct-exec`)*: `system.RunCommand` (interactive) / `system.RunCommandSilent` (captured). Do not call `exec.Command` directly from feature code — add to `system/` if a wrapper is missing. -- **Destructive ops** *(archtest: `dryrun`)*: check `cfg.DryRun` first. Always. -- **Paths** *(archtest: `no-os-getenv-home`)*: `os.UserHomeDir()` — never hardcode `~` or `/Users/...`, never `os.Getenv("HOME")`. -- **State**: everything user-local goes under `~/.openboot/` (auth, cache, snapshots, state). -- **Concurrency**: bounded `sync.WaitGroup` — brew install is sequential with retry; `GetInstalledPackages` uses 2 goroutines for formula+cask list. No unbounded goroutines. -- **Embedded data**: `//go:embed data/*.yaml` loaded in `init()`. -- **Tests**: table-driven, `testify/require` for fatal, `testify/assert` for non-fatal. L1 uses the `Runner` interface to fake subprocess calls — no real network, no real fork. -- **Commits**: Conventional (`feat:` / `fix:` / `docs:` / `refactor:` / `test:` / `chore:` / `ci:`), one thing per commit. - -## Env vars - -| Var | Purpose | -|-----|---------| -| `OPENBOOT_DISABLE_AUTOUPDATE=1` | Skip auto-update check | -| `OPENBOOT_GIT_NAME` / `OPENBOOT_GIT_EMAIL` | Git identity override (silent mode) | -| `OPENBOOT_PRESET` | Default preset | -| `OPENBOOT_USER` | Config alias/username | -| `OPENBOOT_API_URL` | Override API base URL (testing; https or http://localhost only) | -| `OPENBOOT_DOTFILES` | Override dotfiles repo URL | -| `OPENBOOT_DRY_RUN` | Dry-run mode for `scripts/install.sh` (not the CLI) | -| `OPENBOOT_VERSION` | Pin version in `scripts/install.sh` | +Shared repository instructions belong in `AGENTS.md`. Shared skills belong in +`.agents/skills/`; the entries under `.claude/skills/` are adapters to those +canonical files. Keep only Claude Code-specific hooks, permissions, and +settings under `.claude/`. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f9ae774..3ad8652 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -79,7 +79,7 @@ Integration tests live alongside L1 in `test/integration/` and *may* touch the r ## Architecture -See [CLAUDE.md](CLAUDE.md) for how everything fits together. +See [AGENTS.md](AGENTS.md) for the repository architecture and agent guidance. ## Questions diff --git a/docs/HARNESS.md b/docs/HARNESS.md index b040f85..8e7368e 100644 --- a/docs/HARNESS.md +++ b/docs/HARNESS.md @@ -29,7 +29,7 @@ Three regulation categories: 1. **Maintainability** — code style, complexity, dead code. 2. **Architecture fitness** — project-specific invariants (the "do X, not Y" - rules in CLAUDE.md). + rules in AGENTS.md). 3. **Behaviour** — does it actually do the right thing. ## Where each control lives @@ -57,8 +57,9 @@ Three regulation categories: | Behav. | Auto-release sensor — patch fast lane (`fix:`-only) auto-tags + dispatches `release.yml`; feat threshold opens a `release-ready` issue (check L4 CI green, then tag manually) | push to `main` | `.github/workflows/auto-release.yml` | | Behav. | Release notes — Conventional Commits since previous tag, grouped by type (Features / Bug Fixes / etc) + Full Changelog link, appended to the install-instructions template | tag push or `workflow_dispatch` | `.github/workflows/release.yml` (`Write release notes` step) | | Behav. | Old-CLI compat (previous release × current mock server) | every PR | `.github/workflows/test.yml` `cli-compat` job | -| Feedfwd. | Agent conventions | every AI turn | `CLAUDE.md`, `AGENTS.md` | -| Feedfwd. | Skills | model-loaded | `.claude/skills/*` | +| Feedfwd. | Shared agent conventions | every AI turn | `AGENTS.md` (canonical); `CLAUDE.md` imports it for Claude Code | +| Feedfwd. | Shared skills | model-loaded | `.agents/skills/*` (canonical); `.claude/skills/*/SKILL.md` symlinks for Claude Code | +| Arch. | Multi-agent config must not drift | L1 | `internal/archtest/agentconfig_test.go` | | Feedfwd. | Session-start hook (warm caches, fetch deps) | every Claude session | `.claude/hooks/session-start.sh` | | Feedback (agent) | `go vet` on edited package | after every Edit/Write/MultiEdit | `.claude/hooks/post-tool-use.sh` | | Feedback (agent) | `go vet ./...` + archtest | end of every Claude turn (if .go dirty) | `.claude/hooks/stop.sh` | @@ -76,8 +77,8 @@ When you observe a recurring issue, decide where to encode the fix: | "Agent introduced a new lint failure that golangci-lint should have caught." | Enable the relevant linter in `.golangci.yml`. | | "Agent broke a behaviour that has no test." | Write the test at the right tier — L1 covers both faked-runner units in `internal//` and real-subprocess integration in `test/integration/`. | | "A shipped release reached nobody: `install.sh`'s already-installed branch prompted on stdin, which under `curl \| bash` is the script itself, so it always took the don't-upgrade default." | Already handled by the `installsh` archtest. The wider lesson the sensor does *not* cover: `curl-bash-smoke` is gated `if: github.event_name != 'pull_request'` and only exercises the mock-server path, so the real `scripts/install.sh` brew branch has no behavioural test. Upgrade-over-existing-install is the case to add. | -| "Agent missed a CLAUDE.md rule we keep restating." | Make it a hard or soft archtest rule (a docs rule that doesn't fail is a docs rule that drifts). | -| "Agent did something safe but suboptimal." | Add to CLAUDE.md "Project-specific conventions" and consider whether it's encodable. | +| "Agent missed an AGENTS.md rule we keep restating." | Make it a hard or soft archtest rule (a docs rule that doesn't fail is a docs rule that drifts). | +| "Agent did something safe but suboptimal." | Add to AGENTS.md "Project-specific conventions" and consider whether it's encodable. | | "Agent guessed at an API contract." | Update `openboot-contract` repo + fixtures; CI already runs schema validation. | | "Agent's PR description was off." | Tighten `pull_request_template.md`. | | "Fixes and features piled up on `main` because nobody told an agent to cut a release." | Already handled: `.github/workflows/auto-release.yml` auto-tags patches and opens a `release-ready` issue for feats. Tune thresholds there. | @@ -93,7 +94,7 @@ it survives doc rot. (`codecov.yml` `informational: true`). Hard coverage gates push toward test-shaped code without raising actual quality. - **No fmt.Print/Println archtest rule yet.** The convention exists in - CLAUDE.md but the codebase has ~150 existing call sites and the rule + AGENTS.md but the codebase has ~150 existing call sites and the rule would be mostly noise. Reconsider after the UI helpers cover all the cases currently using raw stdout. - **No agent-driven changes to `main` without human review.** All AI diff --git a/internal/archtest/README.md b/internal/archtest/README.md index 20ada46..7b8fb5e 100644 --- a/internal/archtest/README.md +++ b/internal/archtest/README.md @@ -1,7 +1,7 @@ # archtest Architecture fitness functions for openboot. Each `*_test.go` file enforces one -project-level invariant documented in [CLAUDE.md](../../CLAUDE.md#project-specific-conventions). +project-level invariant documented in [AGENTS.md](../../AGENTS.md#project-specific-conventions). This is the **Architecture Fitness Harness** described in Martin Fowler's [Harness Engineering for Coding Agents](https://martinfowler.com/articles/harness-engineering.html): @@ -29,6 +29,7 @@ from the baseline) are logged but do not fail the test. | `no-raw-http` | `http_test.go` | yes | "Use `httputil.Do()` — handles 429 + Retry-After" | | `no-os-getenv-home` | `envhome_test.go` | no (hard rule) | "Use `os.UserHomeDir()` — never hardcode `~` or `/Users/...`" | | `dryrun` | `dryrun_test.go` | yes | "Destructive ops: check `cfg.DryRun` first. Always." | +| `agent-config` | `agentconfig_test.go` | no (hard rule) | "Keep `AGENTS.md` and shared skills canonical across agent tools" | ## Workflow @@ -54,7 +55,7 @@ the new call site is justified — those baseline diffs are the audit trail. fail immediately on any hit. See `envhome_test.go`. - **Soft rule** (baseline existing call sites) — call `enforce(t, rule, found)`. Generate the baseline once with `ARCHTEST_UPDATE_BASELINE=1`. -4. Document the rule in CLAUDE.md "Project-specific conventions" so agents +4. Document the rule in AGENTS.md "Project-specific conventions" so agents read it before writing code. ## Why not just use `golangci-lint`? diff --git a/internal/archtest/agentconfig_test.go b/internal/archtest/agentconfig_test.go new file mode 100644 index 0000000..1801688 --- /dev/null +++ b/internal/archtest/agentconfig_test.go @@ -0,0 +1,114 @@ +package archtest + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestAgentConfigSharedAcrossTools keeps AGENTS.md and each SKILL.md as a +// single source of truth while exposing them through the native discovery +// paths used by Codex and Claude Code. +func TestAgentConfigSharedAcrossTools(t *testing.T) { + root := repoRoot() + + claudeInstructions := filepath.Join(root, "CLAUDE.md") + src, err := os.ReadFile(claudeInstructions) // #nosec G304 -- path is inside the repo + if err != nil { + t.Fatalf("read CLAUDE.md: %v", err) + } + if !hasExactLine(string(src), "@AGENTS.md") { + t.Errorf("CLAUDE.md must import the canonical instructions with an exact @AGENTS.md line") + } + + canonicalRoot := filepath.Join(root, ".agents", "skills") + claudeRoot := filepath.Join(root, ".claude", "skills") + canonicalNames := skillNames(t, canonicalRoot) + claudeNames := skillNames(t, claudeRoot) + + for name := range canonicalNames { + if !claudeNames[name] { + t.Errorf("shared skill %q is missing its Claude Code adapter", name) + continue + } + + canonical := filepath.Join(canonicalRoot, name, "SKILL.md") + adapter := filepath.Join(claudeRoot, name, "SKILL.md") + info, err := os.Lstat(adapter) + if err != nil { + t.Errorf("inspect Claude Code adapter for skill %q: %v", name, err) + continue + } + if info.Mode()&os.ModeSymlink == 0 { + t.Errorf("%s must be a symlink to %s; do not copy shared skill instructions", + repoRelative(root, adapter), repoRelative(root, canonical)) + continue + } + target, err := os.Readlink(adapter) + if err != nil { + t.Errorf("read Claude Code adapter for skill %q: %v", name, err) + continue + } + if filepath.IsAbs(target) { + t.Errorf("%s must use a relative symlink so it works in every checkout", + repoRelative(root, adapter)) + continue + } + + resolved, err := filepath.EvalSymlinks(adapter) + if err != nil { + t.Errorf("resolve Claude Code adapter for skill %q: %v", name, err) + continue + } + if filepath.Clean(resolved) != filepath.Clean(canonical) { + t.Errorf("%s resolves to %s, want %s", + repoRelative(root, adapter), repoRelative(root, resolved), repoRelative(root, canonical)) + } + } + + for name := range claudeNames { + if !canonicalNames[name] { + t.Errorf("Claude-only skill %q has no canonical .agents/skills entry", name) + } + } +} + +func hasExactLine(src, want string) bool { + for _, line := range strings.Split(src, "\n") { + if strings.TrimSpace(line) == want { + return true + } + } + return false +} + +func skillNames(t *testing.T, root string) map[string]bool { + t.Helper() + entries, err := os.ReadDir(root) + if err != nil { + t.Fatalf("read skill directory %s: %v", root, err) + } + + names := make(map[string]bool) + for _, entry := range entries { + if !entry.IsDir() { + continue + } + name := entry.Name() + if _, err := os.Stat(filepath.Join(root, name, "SKILL.md")); err != nil { + t.Errorf("skill %q under %s has no readable SKILL.md: %v", name, root, err) + continue + } + names[name] = true + } + return names +} + +func repoRelative(root, path string) string { + rel, err := filepath.Rel(root, path) + if err != nil { + return path + } + return filepath.ToSlash(rel) +} diff --git a/internal/archtest/doc.go b/internal/archtest/doc.go index f07ed82..a1f66e6 100644 --- a/internal/archtest/doc.go +++ b/internal/archtest/doc.go @@ -1,6 +1,6 @@ // Package archtest holds architecture fitness functions for the openboot // codebase. Each *_test.go file enforces one project-level invariant -// documented in CLAUDE.md (e.g. "exec.Command must go through internal/system", +// documented in AGENTS.md (e.g. "exec.Command must go through internal/system", // "raw http.NewRequest must go through internal/httputil"). // // Rules are seeded with a baseline of current violations under baseline/. A diff --git a/internal/archtest/dryrun_test.go b/internal/archtest/dryrun_test.go index b8aa191..f795444 100644 --- a/internal/archtest/dryrun_test.go +++ b/internal/archtest/dryrun_test.go @@ -154,7 +154,7 @@ func findDestructiveWithoutDryRun(gf goFile) []callSite { return out } -// TestDryRunGuard enforces the CLAUDE.md rule: +// TestDryRunGuard enforces the AGENTS.md rule: // // Destructive ops: check cfg.DryRun first. Always. // diff --git a/internal/archtest/envhome_test.go b/internal/archtest/envhome_test.go index 6b5f5b6..1a6b565 100644 --- a/internal/archtest/envhome_test.go +++ b/internal/archtest/envhome_test.go @@ -2,7 +2,7 @@ package archtest import "testing" -// TestNoOsGetenvHome enforces the CLAUDE.md rule: +// TestNoOsGetenvHome enforces the AGENTS.md rule: // // Paths — os.UserHomeDir() — never hardcode `~` or `/Users/...`. // diff --git a/internal/archtest/exec_test.go b/internal/archtest/exec_test.go index c39d4ee..f2acedb 100644 --- a/internal/archtest/exec_test.go +++ b/internal/archtest/exec_test.go @@ -5,14 +5,14 @@ import "testing" // execAllowedPaths is the set of files/packages allowed to call os/exec // directly. Everything else must use internal/system or a documented runner. // Adding a path here is an intentional architectural decision — review the -// rule in CLAUDE.md ("Subprocess") before extending. +// rule in AGENTS.md ("Subprocess") before extending. var execAllowedPaths = []string{ "internal/system", // canonical generic runner "internal/brew/runner.go", // brew runner — wrapped, fakeable "internal/npm/runner.go", // npm runner — wrapped, fakeable } -// TestNoDirectExec enforces the CLAUDE.md rule: +// TestNoDirectExec enforces the AGENTS.md rule: // // Do not call exec.Command directly from feature code — // add to system/ if a wrapper is missing. diff --git a/internal/archtest/fmtprint_test.go b/internal/archtest/fmtprint_test.go index 1ddcbbe..8350b89 100644 --- a/internal/archtest/fmtprint_test.go +++ b/internal/archtest/fmtprint_test.go @@ -99,7 +99,7 @@ func findFmtPrint(gf goFile) []callSite { return out } -// TestNoRawFmtPrint enforces the CLAUDE.md rule: +// TestNoRawFmtPrint enforces the AGENTS.md rule: // // UI output must always go through ui.* helpers; raw fmt.Print* calls // are bugs in user-facing paths. diff --git a/internal/archtest/http_test.go b/internal/archtest/http_test.go index f77e9b2..694f9d9 100644 --- a/internal/archtest/http_test.go +++ b/internal/archtest/http_test.go @@ -13,7 +13,7 @@ var httpAllowedPaths = []string{ "internal/httputil", // owns the wrapper } -// TestNoRawHTTPNewRequest enforces the CLAUDE.md rule: +// TestNoRawHTTPNewRequest enforces the AGENTS.md rule: // // HTTP with retry — use httputil.Do() — handles 429 + Retry-After. func TestNoRawHTTPNewRequest(t *testing.T) {