From e8d73728e9ada7aa6a17e2aa2ac03697cfea9aa5 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Thu, 11 Jun 2026 06:58:58 -0400 Subject: [PATCH 1/3] docs: resolve standards-family contradictions, gaps, and stale references Decisions applied (2026-06-11): - working-with-state $5.5: --force (long-only) replaces --yes on nuclear data verbs, aligning with command-surface $3.1's single safety-skip spelling; $8 decision rows annotated, not rewritten - working-with-secrets $1.5.2: set-credential per-class exit codes soften to SHOULD (binary 0/non-0 is the MUST) pending the typed-error layer; scriptability $3.1 lead aligned - output-and-rendering $8/$10: color stance flips to isatty-gated (the recommended libraries' default); per-CLI verification tracked in #55 - ci.md $2 + repo-layout $7: pr-title check also greps PR title and body against the AI-tooling blocklist (squash-merge blind spot); sample workflow comment ref fixed to release.md $1.1 - repo-layout $2.1: new library-repo profile (required-files delta, check=tidy+lint+test+build, manual tagging, identity-check N/A); $4 exemption tightened to include build in check - release.md $2/$6: GITHUB_RUN_NUMBER path-scope sharp edge (rename resets numbering and regresses tags; bump MAJOR.MINOR to recover) - output-and-rendering $4.1: newline-in-cell substitution rule - working-with-secrets $1.5.1/$2.1: snapshot zeroing softened to best-effort (Go cannot guarantee string zeroization) - doc-rot: README $5a/$5b -> $6a/$6b; working-with-state $7 step 7 records statedir.Data DataDir()/DataDirEnsured() as delivered (a9a6987); stale disposition rows annotated - remove docs/data-pillar-primer.md (superseded by working-with-state $5; decisions log is the surviving record) Closes #54 --- README.md | 4 +- docs/ci.md | 12 +++++- docs/data-pillar-primer.md | 75 ------------------------------------ docs/output-and-rendering.md | 11 +++++- docs/release.md | 14 ++++++- docs/repo-layout.md | 39 ++++++++++++++++--- docs/scriptability.md | 2 +- docs/working-with-secrets.md | 6 +-- docs/working-with-state.md | 37 +++++++++++------- 9 files changed, 95 insertions(+), 105 deletions(-) delete mode 100644 docs/data-pillar-primer.md diff --git a/README.md b/README.md index 361216a..ac50398 100644 --- a/README.md +++ b/README.md @@ -42,9 +42,9 @@ grammar (§1.3), the `Store`/`Open` lifecycle, single-key and bundle ops, OS-keyring backends (Keychain / Credential Manager / Secret Service / encrypted file) with Linux fail-closed classification (§1.4), `--backend` flag plumbing, redaction helpers, and legacy-migration helpers (§1.8). The `cache` package -implements the tier-1 universal core from `working-with-state.md` §5b +implements the tier-1 universal core from `working-with-state.md` §6b (envelope + atomic write + freshness `Classify`). The `statedir` package -provides the shared path/dir resolver from `working-with-state.md` §5a. +provides the shared path/dir resolver from `working-with-state.md` §6a. For component-by-component conformance status and the rollout matrix across consumer CLIs, see `docs/working-with-state.md` §6 and §7 and diff --git a/docs/ci.md b/docs/ci.md index 5307b30..a5e2415 100644 --- a/docs/ci.md +++ b/docs/ci.md @@ -53,7 +53,7 @@ CI runs **build / test / lint** as distinct jobs, plus a PR-only title check: | `test` | `make test` (or `make test-cover`) | push + PR | | `lint` | `golangci-lint` per `repo-layout.md` §5 | push + PR | | `identity-check` | assert `packaging/identity.yml` matches `.goreleaser`/winget/choco/nfpm/Homebrew (`distribution.md` §8.2) | push + PR | -| `pr-title` | assert the PR title is a conventional commit (`release.md` §1) | **pull_request only** | +| `pr-title` | assert the PR title is a conventional commit (`release.md` §1) **and** that the PR title and body are free of AI-tooling mentions (`repo-layout.md` §7 blocklist) | **pull_request only** | `build`/`test`/`lint` are separate because **branch protection requires them as independent status checks** (`repo-layout.md` §6). A single combined job exposes @@ -66,6 +66,14 @@ no PR title to check on a `push`). Because it gates release eligibility, optional, the gate is advisory and a non-conventional title can merge and either mis-trigger or silently skip a release. +For the same squash-merge reason, `pr-title` also greps the **PR title and +body** against the `repo-layout.md` §7 AI-tooling blocklist: the landing +commit is built from the title plus — under the "title and description" +squash-message setting — the PR body, neither of which the local `commit-msg` +hook sees. The commit-details squash mode folds in individual commit messages +instead, and those the local hook already gates; between the two enforcement +points, every squash-message mode is covered. + **`build` is an aggregate, not the matrix.** A matrix job surfaces one check *per leg* (`build (ubuntu-latest)`, `build (windows-latest)`, …), and that shifting set is not a stable name branch protection can require. So the OS matrix @@ -212,7 +220,7 @@ jobs: if: github.event_name == 'pull_request' runs-on: ubuntu-latest steps: - - uses: open-cli-collective/.github/actions/pr-title@v1 # §1 grammar + - uses: open-cli-collective/.github/actions/pr-title@v1 # release.md §1.1 grammar + repo-layout.md §7 blocklist ``` The composites pin `go-version` (from `go.mod`) and the golangci version, so a diff --git a/docs/data-pillar-primer.md b/docs/data-pillar-primer.md deleted file mode 100644 index 6c8daa0..0000000 --- a/docs/data-pillar-primer.md +++ /dev/null @@ -1,75 +0,0 @@ -# TL;DR — working-with-state.md needs a fourth pillar - -> **⚠️ Superseded by Round 6 (2026-05-28).** The fourth pillar landed in -> `working-with-state.md` §5; this primer is preserved as the "how the -> decision was reached" companion but its specifics are now stale. Final -> deltas from what's written below: -> -> - **Backing:** Linux `XDG_STATE_HOME` (not `XDG_DATA_HOME`); Windows -> `%LOCALAPPDATA%` (not `%APPDATA%` — Roaming would sync the data dir -> over the network). macOS `~/Library/Application Support//data/` -> unchanged. Rationale: working-state use case matches XDG STATE's -> spec, not XDG DATA's. -> - **`config clear --all`:** strict separation, *no* `--purge-data` -> flag. Data has its own verb subtree (suggested ` data purge` / -> ` data prune`). -> - **Hermetic test helper:** grew from 7 to 8 vars (`XDG_STATE_HOME` -> added) — `cli-common/statedirtest`. -> - **Naming rule:** data is per-binary (matches cache §4.1, not config -> §3); the "deferred until first shared-credential CLI" hedge is gone. -> - **Retention:** SHOULD declare a policy when the dir can grow -> unboundedly; concrete shapes (size/age/count caps) in §5.6. -> - **`--dry-run`:** both nuclear and maintenance verbs support it. -> -> Read this for context on the architectural debate; read -> `working-with-state.md` §5 for the actual policy. - - -## Context - -We ship a family of Go CLIs (`jtk`, `cfl`, `gro`, `nrq`, `slck`, `sfdc`) on shared standards docs in `cli-common/docs/`. State has three pillars today: - -| Pillar | Where | Rule | -|---|---|---| -| **Secrets** | OS keyring (`cli-common/credstore`) | Access secrets only | -| **Config** | `os.UserConfigDir()/` | Authored, durable, non-secret | -| **Cache** | `os.UserCacheDir()/` | Derived, **safe to delete at any moment** | - -`config clear --all` wipes config + cache for the active profile. That's correct for everything we ship today. - -## The gap - -We're designing a new CLI (`codereview` / `cr`) — AI-driven PR reviewer. It needs to persist: - -- A **run ledger** (SQLite): every review run, agents used, token cost, duration, model, findings -- **Run artifacts** kept past the run: findings JSON, log streams, agent outputs - -This is **derived but not safe to delete**: -- Re-derivable in theory (re-run every review for $$$), not in practice -- Token-cost history, "which agent earns its keep," resume metadata — *real* user value -- `config clear --all` blowing it away violates user expectation - -Cache doesn't fit. Config doesn't fit. There isn't a slot. - -## Proposal sketch (not committed) - -Add a fourth pillar — working name **data** — mirroring XDG semantics: - -- **Linux:** `$XDG_DATA_HOME` / `~/.local/share/` -- **macOS / Windows:** XDG data and config collapse to one root, so use a `data/` subdir under that root (`~/Library/Application Support//data`, `%APPDATA%\\data`) -- Survives `config clear --all`; removed only by a dedicated command (`cr data prune` / `--purge-data` opt-in flag for full uninstall) -- CLI-specific retention policy lives in config (not standards-mandated) -- Hermetic test helper must extend the existing 7-var list - -Maps cleanly to existing patterns: tier-1 cache `Envelope[T]` / atomic write / `Locator` indirection from INT-310 transfer directly. - -## Things to pressure-test with the architect - -1. **Is "data" really a fourth pillar, or a strict-mode cache?** Could we instead change cache's contract to "safe to delete, but may be expensive to recompute" and let CLIs declare retention on cache sub-dirs? Probably worse — muddies the existing "cache = disposable" invariant the other CLIs depend on. -2. **macOS/Windows path collision.** `os.UserConfigDir()` and the natural data dir resolve to the same root. `data/` subdir under the tool dir is the obvious answer; any cleaner option? -3. **Test isolation env-var set.** Adding data resolution changes which env vars need overriding in hermetic tests. Worth folding into `cli-common/statedirtest` in lockstep. -4. **`config clear` scope semantics.** Default narrow scope leaves data alone (already correct). `--all` is the question: does `--all` still mean "active-profile factory reset" but exclude data? Or do we introduce `--purge-data` as an explicit opt-in? -5. **Should the standard mandate retention policy shape?** Or leave it per-CLI? Other CLIs may eventually grow data state too; uniformity could matter. -6. **Conformance status across existing CLIs.** None hold data today, but the §6 rollout matrix and §2 conformance table should at least name the new pillar so future audits don't miss it. - -The standards-doc family converged INT-310 via Codex pressure-test (5 rounds, blockers→0). Same process is the right fit here: draft → architect review → Codex convergence → tag in `cli-common`. diff --git a/docs/output-and-rendering.md b/docs/output-and-rendering.md index 385eef8..8aa5ccb 100644 --- a/docs/output-and-rendering.md +++ b/docs/output-and-rendering.md @@ -71,6 +71,7 @@ Reference implementation of the projection registry pattern (the `--fields` mach - `--extended` adds columns; it does not replace default columns. - Sorted most-recent-first where time-ordered (sprints, releases, deploys, etc.). - **Separator collision:** the ` | ` triplet (space-pipe-space) is reserved. When a cell value contains it, the presentation layer MUST replace the embedded ` | ` with a single space (or a similar non-collision substitution) before emission. A naked `|` inside a value, surrounded by other characters, is fine — the triplet is the discriminator. Document the substitution choice per-CLI; do not silently mangle without telling the user. +- **Newlines in cells:** a cell value containing a newline breaks the one-row-per-line grammar entirely. The presentation layer MUST replace embedded newlines and carriage returns in cell values with a single space before emission — same posture as the separator substitution above. ``` KEY | STATUS | TYPE | PTS | ASSIGNEE | SUMMARY @@ -184,7 +185,13 @@ Reference: jtk's `resolve.New(client).Board/Sprint/User` at `atlassian-cli/tools Production **resource output has no color.** New CLIs MUST NOT add color to list/get/search output. -Color in **setup and diagnostic surfaces** — `init` wizards, `me`, `config test`, `config clear`, success/error glyphs on mutation confirmations — is acceptable and consistent with existing CLIs (jtk/cfl/sfdc/nrq use `fatih/color` decorators; gro uses `lipgloss`). Where any color is rendered, a `--no-color` flag MUST be supported and respected; `isatty` is NOT checked. +Color in **setup and diagnostic surfaces** — `init` wizards, `me`, `config test`, `config clear`, success/error glyphs on mutation confirmations — is acceptable and consistent with existing CLIs (jtk/cfl/sfdc/nrq use `fatih/color` decorators; gro uses `lipgloss`). Where any color is rendered: + +- Color MUST auto-disable when the output stream is not a TTY. This is the default behavior of both recommended libraries — do NOT override it to force color onto pipes. ANSI codes must never land in a script or agent capture. +- A `--no-color` flag MUST be supported and respected. +- The `NO_COLOR` env var SHOULD be honored (both recommended libraries do). + +*(Amended 2026-06-11: an earlier revision said "`isatty` is NOT checked" — that contradicted the recommended libraries' default behavior and the agent-capture use case. isatty gating is now the standard.)* Color choice: prefer `fatih/color` (the family default; honors a `--no-color` toggle via a package-level var) or `lipgloss` (honors `NO_COLOR` env natively). Either is fine; pick one per CLI and stick with it. @@ -232,7 +239,7 @@ The new docs are forward-looking. The following current divergences from this st - **Resource-read JSON is widespread.** slck, sfdc, nrq, cfl all expose JSON on resource reads via global `-o json`; gro exposes it via per-command `--json`. Only jtk holds the §2 line (reserves JSON for `automation export`). New CLIs MUST NOT add resource-read JSON. - **gro has no root `--no-color` flag** — `google-readonly/internal/cmd/root/root.go:107-109` registers a global `--verbose` and the credstore backend flag but no color flag. Its lipgloss styling at `google-readonly/internal/view/view.go:34` honors the `NO_COLOR` env natively, which papers over the gap for users who set the env, but the missing flag is a divergence from §8. -- **No CLI gates color on `isatty`.** This is intentional and consistent with §8 — `--no-color` is the documented opt-out. +- **isatty gating is expected via library defaults but unverified per CLI.** §8 (as amended 2026-06-11) requires color to auto-disable on non-TTY output; fatih/color and lipgloss do this by default, so conformance is expected unless a CLI overrides it — a per-CLI verification pass is tracked in cli-common#55. - **No CLI has output goldens.** Per §9.5 this is recommended-not-normative pending the cli-common helper. Command-surface divergences (init flag-skip failures, missing `set-credential`) are catalogued in `command-surface.md` §9. Scriptability divergences (missing `--non-interactive`, `me` not exiting non-zero) are catalogued in `scriptability.md` §9. diff --git a/docs/release.md b/docs/release.md index 7d50881..1c4f6ed 100644 --- a/docs/release.md +++ b/docs/release.md @@ -84,6 +84,16 @@ tradeoff for never hand-bumping the patch. The build stamps the version via `-ldflags` from the tag; `version.txt` holds only `MAJOR.MINOR`, so there is no full-version constant in the repo to drift. +**Run-number scope (sharp edge).** `GITHUB_RUN_NUMBER` is scoped to the +workflow **file path**. Renaming, moving, or deleting-and-recreating +`auto-release.yml` resets the count to 1, so the next tag can sort *below* +already-published versions (`v3.1.150` → `v3.1.2`) — a downgrade package +channels will refuse or mis-resolve. Never rename the auto-release workflow +file; the reusable-workflow migration (§6) keeps the local caller at the same +path for exactly this reason. If numbering ever does reset, bump `MAJOR.MINOR` +in `version.txt` before the next release so every new tag sorts above the old +line. + To release a new `MAJOR.MINOR`, bump `version.txt`. **The path gate (§3) MUST include `version.txt`** so a deliberate `MAJOR.MINOR` bump ships on its own merge. Current repos exclude it (§7) — meaning today a `version.txt`-only PR @@ -225,7 +235,9 @@ jobs: ``` Inputs: `tag-prefix`, `version-file`, `release-paths`, `tool-paths`. Secret: -`tag-token` (the §3.1 dedicated token). Pin the `@v1` ref. +`tag-token` (the §3.1 dedicated token). Pin the `@v1` ref. Keep the caller at +`.github/workflows/auto-release.yml` — run numbers are path-scoped (§2), and a +renamed caller resets the patch sequence. **GitHub App alternative (§3.1 preferred).** A caller job that `uses:` a reusable workflow cannot also run `steps:`, so the App-token mint can't live in diff --git a/docs/repo-layout.md b/docs/repo-layout.md index ed418b7..0417e64 100644 --- a/docs/repo-layout.md +++ b/docs/repo-layout.md @@ -83,6 +83,32 @@ Monorepo (`atlassian-cli`): per-tool files live under `tools//` `go.work`, `LICENSE`, and CI. Tool-local agent entrypoints and `docs/development.md` equivalents are allowed where they reduce navigation cost. +### §2.1 Library repos (no shipped binary) + +A repo that ships no binary and publishes through no package channel — +`cli-common` itself — follows this doc under a reduced profile: + +- **Required files:** `README.md`, `LICENSE`, `AGENTS.md`, `CLAUDE.md`, + `docs/development.md`, `.golangci.yml`, `Makefile`, + `.gitignore`/`.gitattributes`. Agent entrypoints are required in every repo + agents work in, library or not. +- **Not required:** `version.txt`, `.goreleaser.*`, `packaging/identity.yml`, + `CHANGELOG.md` — distribution files exist only where something ships. +- **Makefile:** the §4 library exemption applies, with `check` = + `tidy` + `lint` + `test` + `build` so a green local `check` still predicts a + green CI run. +- **Release/tagging:** semver tags are cut **manually**. `release.md`'s + auto-release and run-number patch scheme do NOT apply — a library with + co-evolving consumers must gate its tags on the consumer matrix being green + against the candidate SHA, and automating the tag would defeat that gate. + (`working-with-state.md` §6 is the concrete instance of this rule for + cli-common's state components.) +- **CI:** `ci.md` applies — build/test/lint across the three OSes, + `go-version-file: go.mod`, and `pr-title` on the same terms as every repo + (i.e., when the shared composite ships in the family rollout, `ci.md` §8). + `identity-check` is N/A (nothing ships). This profile creates no new + CI-restructuring obligation beyond `ci.md`'s existing rules. + --- ## §3 Go version policy @@ -122,10 +148,10 @@ green CI run. **Library/shared-module exemption.** The full target set above applies to **shipped-binary CLI repos**. A library or shared module that produces no binary -and ships through no package channel — e.g. `cli-common` itself, whose `make -check` is just `tidy` + `lint` + `test` — needs only `tidy`/`lint`/`test`/`build` -and is exempt from `install`/`release`/`snapshot`/`test-cover`. Do not force -distribution targets onto a repo that distributes nothing. +and ships through no package channel — e.g. `cli-common` itself (§2.1) — needs +only `tidy`/`lint`/`test`/`build`, with `check` running all four, and is exempt +from `install`/`release`/`snapshot`/`test-cover`. Do not force distribution +targets onto a repo that distributes nothing. --- @@ -184,7 +210,10 @@ defaults (no config file) is non-conformant. - **Conventional commits** drive releases (`release.md` §1). - Commit messages MUST NOT mention AI tooling (Claude, Anthropic, ChatGPT, Copilot, etc.). Enforce with a `commit-msg` hook that greps a blocklist and - rejects on match. Reference implementation (track it as + rejects on match. The hook alone is insufficient under squash merge: the + landing commit is built from the **PR title and body**, which no local hook + ever sees — the CI `pr-title` check therefore greps both against the same + blocklist (`ci.md` §2). Reference implementation (track it as `scripts/hooks/commit-msg` and wire via `git config core.hooksPath scripts/hooks`): ```sh diff --git a/docs/scriptability.md b/docs/scriptability.md index d4d00b1..1f85c80 100644 --- a/docs/scriptability.md +++ b/docs/scriptability.md @@ -76,7 +76,7 @@ New CLIs MUST implement both `init` (for first-time setup of multiple values at ### §3.1 Normative now - **` me` MUST exit non-zero on auth failure or unreachable upstream.** This is the scripted health-check contract. nrq is the only CLI that currently enforces this (`newrelic-cli/internal/cmd/me/me.go:80-89`); slck `me` returns nil even with no tokens configured (`slack-chat-api/internal/cmd/me/me.go:101-105`) — divergence. -- **` set-credential` exits 0 on success and non-zero per failure class** — the specific failure classes are enumerated in `working-with-secrets.md` §1.5.2 (existing key + no `--overwrite`, disallowed key, keyring write error, locked keyring per §1.4). New CLIs SHOULD map these to the §3.2 taxonomy where applicable (existing-without-overwrite ≈ 1/generic, disallowed key ≈ 2/usage error, keyring write/locked ≈ 3/auth-config), but the precise code-per-class mapping is advisory until §3.2 becomes normative; the binary success/failure contract is what scripts can rely on today. +- **` set-credential` exits 0 on success and non-zero on failure** — the specific failure classes are enumerated in `working-with-secrets.md` §1.5.2 (existing key + no `--overwrite`, disallowed key, keyring write error, locked keyring per §1.4). New CLIs SHOULD map these to the §3.2 taxonomy where applicable (existing-without-overwrite ≈ 1/generic, disallowed key ≈ 2/usage error, keyring write/locked ≈ 3/auth-config), but the precise code-per-class mapping is advisory until §3.2 becomes normative; the binary success/failure contract is what scripts can rely on today. ### §3.2 Recommended target (advisory) diff --git a/docs/working-with-secrets.md b/docs/working-with-secrets.md index edc9c56..31d427c 100644 --- a/docs/working-with-secrets.md +++ b/docs/working-with-secrets.md @@ -184,7 +184,7 @@ This is distinct from *runtime token refresh*. A CLI making normal API calls may - Writes secrets to the keyring under the `credential_ref` derived from flags / config / a default. - If `--credential-ref` is not supplied, defaults to `/default`. - **Pre-write check (per-bundle, not per-key).** Before writing, the CLI lists existing keys under the target ref (`Keyring.Keys()` for the service, filtered by the `/` prefix). If *any* expected key for this CLI is already present, `init` fails by default with an actionable message naming the existing ref, the existing keys, and the OS keyring tool the user can inspect them with (`Keychain Access` on macOS, `cmdkey /list` on Windows, `secret-tool search` on Linux). Remediation in the error: re-run with `--overwrite`, pick a different ref with `--credential-ref`, or run ` config clear` first. -- **Atomicity for multi-key bundles.** `init` for a CLI that writes more than one key under the same ref (e.g. `slck` writes `bot_token` and `user_token`) writes all keys or none. The underlying library doesn't expose a transaction primitive, so the wrapper achieves practical atomicity by: (1) validating all inputs and checking pre-write state before any write begins; (2) **when `--overwrite` is in play, reading existing values for every expected key into an in-memory snapshot before any write**, so rollback can restore prior values rather than merely deleting newly-written keys; (3) on a write failure mid-bundle, restoring from snapshot (for `--overwrite` cases) or removing newly-written keys (for first-write cases); (4) reporting a clear error naming what was written, restored, or deleted. The snapshot lives only for the duration of the call and is zeroed before the function returns. A failure to roll back is itself surfaced; the user is never left wondering what is now in the keyring. +- **Atomicity for multi-key bundles.** `init` for a CLI that writes more than one key under the same ref (e.g. `slck` writes `bot_token` and `user_token`) writes all keys or none. The underlying library doesn't expose a transaction primitive, so the wrapper achieves practical atomicity by: (1) validating all inputs and checking pre-write state before any write begins; (2) **when `--overwrite` is in play, reading existing values for every expected key into an in-memory snapshot before any write**, so rollback can restore prior values rather than merely deleting newly-written keys; (3) on a write failure mid-bundle, restoring from snapshot (for `--overwrite` cases) or removing newly-written keys (for first-write cases); (4) reporting a clear error naming what was written, restored, or deleted. The snapshot lives only for the duration of the call and is best-effort cleared before the function returns (Go cannot guarantee zeroization of string memory; the credstore implementation documents this limitation). A failure to roll back is itself surfaced; the user is never left wondering what is now in the keyring. - **Secret-ingress flags for scripted `init`.** When `init` is invoked non-interactively and needs to ingest secrets, it uses one of two patterns, never `--=`: - **Per-key env vars:** `---from-env ` for each expected secret. Scales cleanly to multi-secret CLIs (e.g. `slck init --bot-token-from-env BOT_TOKEN --user-token-from-env USER_TOKEN`). Subject to the env-var caveats in the threat model (process inheritance, transcripts) but acceptable inside an `op run --` invocation where the env scope is bounded. @@ -208,7 +208,7 @@ This is distinct from *runtime token refresh*. A CLI making normal API calls may **Behavior:** - Never echoes the value to stdout or stderr, never logs it, never includes it in the `set-credential` invocation in shell history (the value comes from stdin or env). - On success, prints one line to stderr: `wrote to via `. Exits 0. -- On failure (existing key + no `--overwrite`, disallowed key, keyring write error, locked keyring per §1.4), exits nonzero with a distinct code per failure class. +- On failure (existing key + no `--overwrite`, disallowed key, keyring write error, locked keyring per §1.4), exits nonzero. The binary success/failure contract is the MUST; distinct exit codes per failure class SHOULD be used, and become normative when the shared typed-error layer ships (`scriptability.md` §3.2). - With `--json`: emits `{"ref": "...", "key": "...", "backend": "...", "written": true}` (or `"written": false` with an `"error"` field on failure). Never emits the value. ## §1.6 What `config show` reports @@ -407,7 +407,7 @@ The package centers on a service-scoped `Store` returned by `Open`. Backend sele - **Ref handling (package-level, no store needed).** `ParseRef(string) (service, profile, error)`; inverse `FormatRef(service, profile)`. Enforce the `[A-Za-z0-9_-]` character set per §1.3, reject `/` inside any segment, return a typed error so callers can produce actionable messages. `EscapeRefSegment(raw)` helper for CLIs that need to derive a profile from a richer identifier (e.g. a user email). - **Single-key operations on a `Store`.** `Get(profile, key) (value, error)`, `Set(profile, key, value string, opts ...SetOpt) error`, `Delete(profile, key) error`, `Exists(profile, key) (bool, error)`. `SetOpt` carries `Overwrite` (per §1.5). - **Bundle operations on a `Store`.** `ListBundle(profile) ([]key, error)` — required for `config show`, pre-write checks (§1.5.1), `config clear` (§1.7), and migration conflict detection (§1.8). `DeleteBundle(profile) error` — used by `config clear`. -- **Atomic-ish multi-key write.** `SetBundle(profile string, kv map[string]string, opts ...SetOpt) (Result, error)` implementing the §1.5.1 contract: validate all inputs, snapshot existing values for keys present in the map when `Overwrite` is set, write all, restore from snapshot on partial failure, zero the snapshot before return. `Result` reports which keys were written, which were restored, which were left untouched. +- **Atomic-ish multi-key write.** `SetBundle(profile string, kv map[string]string, opts ...SetOpt) (Result, error)` implementing the §1.5.1 contract: validate all inputs, snapshot existing values for keys present in the map when `Overwrite` is set, write all, restore from snapshot on partial failure, best-effort clear the snapshot before return (per the §1.5.1 caveat: Go cannot guarantee string zeroization). `Result` reports which keys were written, which were restored, which were left untouched. - **Linux backend classification.** Internal to `Open`'s backend-resolution logic. Distinguishes "unavailable" from "denied/locked" per §1.4, with the "fail closed on ambiguous" rule baked in. Exposed as test seams so each CLI's no-leak / fail-closed tests can drive both paths. - **File-backend opt-in plumbing.** `Open` honors `_KEYRING_BACKEND=file` (per §1.4) and the `keyring.backend: file` config-file knob (passed via `Options`); passes through `_KEYRING_PASSPHRASE` (also §1.4). The package does not hardcode service names. - **In-memory backend.** A `Memory` backend implementation used by tests (selected via `Options.Backend = BackendMemory`), identical behavior contract to the real backends, **no disk side effects**. Every CLI's no-leak and atomicity tests run against this. Critical for CI on machines that don't have a usable keyring. diff --git a/docs/working-with-state.md b/docs/working-with-state.md index 7f36a23..f20d3e6 100644 --- a/docs/working-with-state.md +++ b/docs/working-with-state.md @@ -479,8 +479,12 @@ should not repeat the pattern. `purge` vs. `prune` reads more clearly than **Sub-conventions:** -- Nuclear prompts for confirmation by default; `--yes` (or `--force`) opts - out. Matches `terraform destroy`, `kubectl delete` precedent. +- Nuclear prompts for confirmation by default; `--force` opts out + (long-only) — the family-wide safety-confirmation skip per + `command-surface.md` §3.1. `--yes`/`-y` are not used (amended + 2026-06-11: the terraform/kubectl-style `--yes` was considered and + rejected — one skip spelling across the family beats matching + external precedent). - **Both nuclear and maintenance verbs support `--dry-run`.** Nuclear's dry-run reports the exact paths that would be scrubbed (data dir, database artifacts) without removing anything; maintenance's dry-run @@ -698,11 +702,12 @@ act* — done together (not two horizontal sweeps). 7. **Data pillar (§5) is additive / greenfield (2026-05-28).** No existing CLI holds data state, so the data pillar contributes zero port-units to the rollout above. Net-new CLIs (`cr` is the driver) adopt §5 from - inception alongside §3/§4/§6. The `cli-common` resolver gains a `Data()` - method when the first data-holding CLI lands; the §6 release-train - guardrail applies — the addition is additive (no existing caller - changes behavior), so no consumer-matrix repin is required at - extraction time. + inception alongside §3/§4/§6. **Resolver support DELIVERED 2026-05-30 + (`a9a6987`):** the `statedir.Data` type with `DataDir()` / + `DataDirEnsured()` methods implements the §5.2 derivation, shipped ahead + of the first data-holding CLI. The addition was additive (no existing + caller changed behavior), so no consumer-matrix repin was required — + consistent with the §6 release-train guardrail. ### 7.6 INT-310 close-out retrospective (MON-5375, 2026-05-20) @@ -899,7 +904,8 @@ convergence on §5. Disposition table at the end of this section. the program owns lifecycle, the user shouldn't poke at it directly. Defined negatively ("not config, not secret, not cache") with the cache/data tiebreaker "default to cache when on the fence." Resolves - the gap raised in `data-pillar-primer.md`. Driver: `cr` (codereview) + the gap raised in `data-pillar-primer.md` (since deleted — §5 is the + sole record). Driver: `cr` (codereview) needs a SQLite run ledger + persisted artifacts that survive `config clear --all`. - [x] **Data location (§5.2): Path A — STATE-flavored backing.** @@ -937,7 +943,9 @@ convergence on §5. Disposition table at the end of this section. per-CLI.** Nuclear obeys the §7.6 cleanup-command recovery contract (must not require readable state). Suggested verb pair `purge` (nuclear) / `prune` (maintenance); severity encoded in the verb, - not a flag. Confirmation prompt + `--yes` opt-out for nuclear. + not a flag. Confirmation prompt + `--force` opt-out for nuclear + *(amended 2026-06-11: originally `--yes`; aligned to + `command-surface.md` §3.1's single safety-skip spelling)*. **Both nuclear and maintenance verbs support `--dry-run`** — nuclear's dry-run reports paths-that-would-be-scrubbed without removing. Resolved Codex Round 6 Minor m1 (nuclear is the @@ -978,9 +986,10 @@ convergence on §5. Disposition table at the end of this section. the data-pillar env vars it pins. - [x] **Doc home unchanged:** `cli-common/docs/`, versioned + pinned with the code (the data pillar rides the existing infra). -- [x] **`data-pillar-primer.md` retention:** kept for now as the - "how this decision was reached" companion. Revisit deletion when - `cr` lands its first data dir and §5 is stable. +- [x] **`data-pillar-primer.md` retention:** originally kept as the + "how this decision was reached" companion; **deleted 2026-06-11** + now that §5 is converged and the primer's specifics were stale. + This decisions log is the surviving record. ### Codex pressure-test — disposition (session 019e3fe7, gpt-5.5 xhigh) @@ -1076,8 +1085,8 @@ Round 6 apply. All accepted: | Data deletion semantics — §5 lead-in "removed only by an explicit user-invoked verb" contradicts §5.6's automatic-at-write retention | §5 lead-in rewritten as three explicit invariants: dir survives `config clear --all` / whole-dir nuke is explicit / individual records may be retention-pruned by the program | | `working-with-secrets.md` §1.7.2 heading + "machine never having seen the CLI" sticky framing | §1.7.2 renamed to "config + credentials + cache reset"; "machine never having seen the CLI" reframed as "compose `config clear --all` AND ` data purge` for the historical full reset"; §1.10 row #5 already corrected in Round 6 | | 8-var propagation incomplete — §7 step 1 / §7.6 step 6 / §8 decision rows / `statedirtest.go:3` still said 7-var | All forward-looking text now reads "8-var" or "full env set"; the two §8 decision rows from 2026-05-19 preserve "7-var" as the historical decision with an explicit "(grew to 8 on 2026-05-28)" annotation; package doc updated | -| `statedir/resolver.go` package doc overclaims data-dir support — no `Data()` API exists yet | Doc rewritten as future-tense: explicit "NOT YET IMPLEMENTED" status, points at §7 rollout step 7 as the implementation gate (when the first data-holding CLI lands). Also `LegacySource` doc-string §5a → §6a | -| `data-pillar-primer.md` stale enough to mislead — references old XDG_DATA_HOME / %APPDATA% / --purge-data / 7-var helper | Superseded banner at top with bulleted deltas; primer body preserved as "how the decision was reached" companion (user direction to keep it in place) | +| `statedir/resolver.go` package doc overclaims data-dir support — no `Data()` API exists yet | Doc rewritten as future-tense: explicit "NOT YET IMPLEMENTED" status, points at §7 rollout step 7 as the implementation gate (when the first data-holding CLI lands). Also `LegacySource` doc-string §5a → §6a. *(Since superseded: `statedir.Data` shipped 2026-05-30 in `a9a6987` and the package doc now describes the implemented behavior — see §7 step 7.)* | +| `data-pillar-primer.md` stale enough to mislead — references old XDG_DATA_HOME / %APPDATA% / --purge-data / 7-var helper | Superseded banner at top with bulleted deltas; primer body preserved as "how the decision was reached" companion (user direction to keep it in place) *(since deleted 2026-06-11 — see the §8 retention decision row)* | Round 7 (re-reviewed, 2026-05-28): **`blockers=0 majors=0 minors=2` — CONVERGED after applying the two minors.** Codex re-read the working tree after the cleanup pass landed and found no From 37d189ea29404439e10b93c0b1272719f8de5bc5 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Thu, 11 Jun 2026 07:01:12 -0400 Subject: [PATCH 2/3] docs: align stale cross-refs with softened exit-code and squash-mode wording --- docs/repo-layout.md | 9 ++++++--- docs/scriptability.md | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/docs/repo-layout.md b/docs/repo-layout.md index 0417e64..2af2bdd 100644 --- a/docs/repo-layout.md +++ b/docs/repo-layout.md @@ -211,9 +211,12 @@ defaults (no config file) is non-conformant. - Commit messages MUST NOT mention AI tooling (Claude, Anthropic, ChatGPT, Copilot, etc.). Enforce with a `commit-msg` hook that greps a blocklist and rejects on match. The hook alone is insufficient under squash merge: the - landing commit is built from the **PR title and body**, which no local hook - ever sees — the CI `pr-title` check therefore greps both against the same - blocklist (`ci.md` §2). Reference implementation (track it as + landing commit is built from the PR title plus, depending on the + squash-message setting, either the PR description or the branch's commit + messages — and the local hook sees only the last of those. The CI + `pr-title` check therefore greps the PR title and body against the same + blocklist (`ci.md` §2); between the two enforcement points every + squash-message mode is covered. Reference implementation (track it as `scripts/hooks/commit-msg` and wire via `git config core.hooksPath scripts/hooks`): ```sh diff --git a/docs/scriptability.md b/docs/scriptability.md index 1f85c80..9fba67e 100644 --- a/docs/scriptability.md +++ b/docs/scriptability.md @@ -67,7 +67,7 @@ Refer to `working-with-secrets.md` §1.5 — that doc is the source of truth. Su New CLIs MUST implement both `init` (for first-time setup of multiple values at once) and `set-credential` (for credential rotation, `op run`–driven setup, MDM installers, and the multi-secret-stdin avoidance case). `sfdc` is missing `set-credential` today — that is the canonical divergence. -`set-credential` MUST also ship `--json` from the start per `output-and-rendering.md` §2 (the secrets standard target). Cross-ref: `working-with-secrets.md` §1.5.2 specifies the JSON envelope shape (`{"ref":..., "key":..., "backend":..., "written":true}`) and exit-code-per-failure-class contract. +`set-credential` MUST also ship `--json` from the start per `output-and-rendering.md` §2 (the secrets standard target). Cross-ref: `working-with-secrets.md` §1.5.2 specifies the JSON envelope shape (`{"ref":..., "key":..., "backend":..., "written":true}`) and the exit-code contract (binary success/failure is the MUST; per-class codes are advisory per §3.1). --- From edae33cc3b352e1568418ef171860c513774b8f5 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Thu, 11 Jun 2026 07:05:41 -0400 Subject: [PATCH 3/3] docs: cite release.md $1.1 consistently for the pr-title grammar --- docs/ci.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/ci.md b/docs/ci.md index a5e2415..30c3236 100644 --- a/docs/ci.md +++ b/docs/ci.md @@ -53,7 +53,7 @@ CI runs **build / test / lint** as distinct jobs, plus a PR-only title check: | `test` | `make test` (or `make test-cover`) | push + PR | | `lint` | `golangci-lint` per `repo-layout.md` §5 | push + PR | | `identity-check` | assert `packaging/identity.yml` matches `.goreleaser`/winget/choco/nfpm/Homebrew (`distribution.md` §8.2) | push + PR | -| `pr-title` | assert the PR title is a conventional commit (`release.md` §1) **and** that the PR title and body are free of AI-tooling mentions (`repo-layout.md` §7 blocklist) | **pull_request only** | +| `pr-title` | assert the PR title is a conventional commit (`release.md` §1.1 grammar) **and** that the PR title and body are free of AI-tooling mentions (`repo-layout.md` §7 blocklist) | **pull_request only** | `build`/`test`/`lint` are separate because **branch protection requires them as independent status checks** (`repo-layout.md` §6). A single combined job exposes