diff --git a/.abcd/config/reading-presets.json b/.abcd/config/reading-presets.json index 7ad520d81..7bb8c54d4 100644 --- a/.abcd/config/reading-presets.json +++ b/.abcd/config/reading-presets.json @@ -133,9 +133,9 @@ ], "window": { "tokens_est": 400000, - "measured_tokens_est": 387939, - "measured_bytes": 1493566, - "measured_at": "db30f1a10992df69d1253260a88065884a21937d" + "measured_tokens_est": 388426, + "measured_bytes": 1495442, + "measured_at": "3a282f5fee1dfb46da002e41b9193ab8a1ca2ce0" } }, "comparative": { @@ -216,10 +216,10 @@ "test" ], "window": { - "tokens_est": 1370000, - "measured_tokens_est": 1353945, - "measured_bytes": 5212690, - "measured_at": "db30f1a10992df69d1253260a88065884a21937d" + "tokens_est": 1380000, + "measured_tokens_est": 1358267, + "measured_bytes": 5229331, + "measured_at": "24e78506b9e7d4471d9c9110d6217e6e5f2c4b88" } } } diff --git a/.abcd/development/brief/04-surfaces/04-launch.md b/.abcd/development/brief/04-surfaces/04-launch.md index be68c6e67..52f5cbcaa 100644 --- a/.abcd/development/brief/04-surfaces/04-launch.md +++ b/.abcd/development/brief/04-surfaces/04-launch.md @@ -66,6 +66,7 @@ refuses to stage one and the archive render refuses outright. | Verb | Bucket | Status | |---|---|---| | `archive` | gate | shipped | +| `manifests` | gate | shipped | | `receipts` | gate | shipped | | `scaffold` | — | shipped | | `ship` | gate | shipped | @@ -592,8 +593,12 @@ read-only lockstep checker proves this over the path list adr-20 records, and a half-state is drift. The cut's bump step runs it against the staged artefact at the **public** polarity and refuses to publish on drift; the **dev** polarity runs in `dry-run` over the working tree, asserting the committed manifests carry -no version key. The checker has no bypass flag, and adr-20 records that -a dirty-tree override must not bypass manifest consistency. +no version key. The checker also has a front door of its own, read-only, over +any tree a person holds at the polarity they choose (a marketplace install or a +release source archive at public, a checkout at dev), exiting 0 consistent, 1 +drift with a line per field, and 2 when an input cannot be read. The checker +has no bypass flag, and adr-20 records that a dirty-tree override must not +bypass manifest consistency. The release commit message format — carrying the bump tier and its reason — is a **full-cut design target** (itd-72). The shipped cut never @@ -777,7 +782,7 @@ _Generated from the command tree; a drift test fails `go test` when this appendi ### `abcd launch` -Sub-verbs: `abcd launch archive`, `abcd launch receipts`, `abcd launch scaffold`, `abcd launch ship`, `abcd launch smoke-pages`. +Sub-verbs: `abcd launch archive`, `abcd launch manifests`, `abcd launch receipts`, `abcd launch scaffold`, `abcd launch ship`, `abcd launch smoke-pages`. | Flag | Type | |---|---| @@ -797,6 +802,15 @@ Sub-verbs: none. | `--tag` | string | | `--verify` | bool | +### `abcd launch manifests` + +Sub-verbs: none. + +| Flag | Type | +|---|---| +| `--root` | string | +| `--tree` | string | + ### `abcd launch receipts` Sub-verbs: none. diff --git a/.abcd/development/brief/04-surfaces/20-banlist.md b/.abcd/development/brief/04-surfaces/20-banlist.md index 3eb1d9d69..05c56917a 100644 --- a/.abcd/development/brief/04-surfaces/20-banlist.md +++ b/.abcd/development/brief/04-surfaces/20-banlist.md @@ -77,7 +77,10 @@ markdown, with `exempt_paths` excusing a historical tree as it does under blocks, which the rest of the family skips by default: a fenced example is not prose, but a fence is published as readily as prose, so an entry that means to skip fences declares `skip_code_fences: true`. This repository's `name_roots` are `.abcd`, `AGENTS.md`, -`.github/CONTRIBUTING.md` and `scripts`, and its `exempt_paths` excuse the +`.github/CONTRIBUTING.md`, `scripts` and every other surface the shipped artefact +carries (`commands`, `agents`, `hooks`, `.claude-plugin`, `LICENSE` and +`.gitignore`; `docs` and `README.md` are `roots`), a coverage a test holds to +the launch payload's own include list, and its `exempt_paths` excuse the configuration itself (whose entries spell every ban), the research data and the review archive. diff --git a/.abcd/development/brief/04-surfaces/27-implement.md b/.abcd/development/brief/04-surfaces/27-implement.md index 013325a7a..674b9be92 100644 --- a/.abcd/development/brief/04-surfaces/27-implement.md +++ b/.abcd/development/brief/04-surfaces/27-implement.md @@ -124,7 +124,14 @@ check verdict reports the count (`agents_alive`) beside the ceiling outside the log — is invisible to the count, which is why the run's no-fork rule stays the discipline for that half (iss-2609240646542516); a run that went over anyway says so with a `ceiling_overrun` line. On a refused claim the -second session also logs a `backoff` with its reason and minutes. +second session also logs a `backoff` with its reason and minutes, the minutes +being what the attempt spent, measured from its start; a run state locked past +the lock's timeout by another session's change is contention too, and the second +session's `backoff` from it carries `on: run_state` and the minutes it waited. A +second session whose join meets the lock has no record yet, so the role it is +joining with places the line; a session that never joined has no role to place +it by, and the refusal says the backoff went unlogged. The append takes no lock, +so that line reaches the log while the lock is held. Every bound keys on the role in the session's record, which is the session's own statement: the release refusal, like the others, rests on a cooperative, @@ -157,7 +164,11 @@ twin does, so two writers each land whole lines and a log leaf planted as a link onto a claim file appends nothing. The session, window, claim and load events are refused here: they are written by their own sub-verbs, so the log cannot record a claim the run state does not hold, or a load warning -the check did not give. +the check did not give. A hand-logged `backoff` names its `reason` and the +`minutes` it spent (a number no smaller than zero), or it is refused with nothing +written: contention the verb cannot see, such as the merge queue, reaches the +comparison only this way, and a backoff with neither would count as one that +cost nothing for no reason. An event missing a field the report reads is refused when it is written, naming the field, rather than found missing afterwards (iss-2609240646555891): a diff --git a/.abcd/development/brief/04-surfaces/30-inbox.md b/.abcd/development/brief/04-surfaces/30-inbox.md index a4fb385f9..3652ca958 100644 --- a/.abcd/development/brief/04-surfaces/30-inbox.md +++ b/.abcd/development/brief/04-surfaces/30-inbox.md @@ -31,7 +31,11 @@ A waiting file this abcd cannot read is listed as unreadable with its reason, never dropped: a report written to a later template version names the version; a file that is not a report, is over the size bound, is not a regular file, or whose sender key disagrees with its file name says so. Its id and sender key come -from the file name. +from the file name. Its sender's name comes from the envelope alone, which is +abcd's own writing and is read apart from the reporter's block: when the block +names `sender_key` once, equal to the file name's key, and `sender_name` once, +in the shape a filing writes, the listing names the sender beside the key; +otherwise it names the key alone. Every value a report carries is another repository's words, and every front door sanitises it before it reaches the terminal. The list and show both reach diff --git a/.abcd/development/intents/shipped/itd-131-managed-repo-identity-gate.md b/.abcd/development/intents/shipped/itd-131-managed-repo-identity-gate.md index 022853f31..3e1f1fee7 100644 --- a/.abcd/development/intents/shipped/itd-131-managed-repo-identity-gate.md +++ b/.abcd/development/intents/shipped/itd-131-managed-repo-identity-gate.md @@ -196,5 +196,63 @@ None stated. ## Audit Notes - -Fidelity review OWED (receipt rcp-8a6673e9bc8a). + +Fidelity review — receipt rcp-8a6673e9bc8a (verifier intent-auditor claude-fable-5-1). + +Provenance: intent-auditor@claude-fable-5-1 · rubric_hash sha256:effa65b3e9e88ff29433b443ec2be159522a8b0b71cf1434526514aa61edb13e · prompt_hash sha256:8c00566b452e24b3cfccc53a3665ee1d434077813fff058646e8b147a7313dc8 +Input attestations: tree:ceb4b6dbb97622bddf2401c91f9f808ed05060a0 (origin/main; internal/core/identity/{identity,establish,toolidentity}.go, internal/core/ahoy/{detect,identity_establish}.go, internal/surface/cli/cli.go)@-; + +Acceptance rollup: MET 4 · MET_WITH_CONCERNS 1 · NOT_MET 0 · INCONCLUSIVE 0 + +Per-criterion verdicts: +- ac-1 — MET: an author mismatch against the pin is the required git_identity.mismatch gap; the apply step proposes the pin (else the global identity, disk only), asks Confirm at a terminal and writes repo-local user.name/user.email only on a yes; TestStepGitIdentity_ProposesPinAndWritesOnlyOnConfirm and TestStepGitIdentity_DeclineWritesNothing pass at BASE + evidence: internal/core/ahoy/detect.go:336 — "ID: MismatchGapID, Category: ConfigChange, Scope: "repo"," + evidence: internal/core/ahoy/identity_establish.go:76 — "if !a.prompter.Confirm("Commit to this repository as " + who + ", " + prop.From + "? (sets user.name and user.email in this repository's .git/config only)") {" + evidence: internal/core/identity/establish.go:25 — "func Propose(root string) (Proposal, bool, error)" + evidence: internal/core/ahoy/identity_establish_test.go:142 — "func TestStepGitIdentity_ProposesPinAndWritesOnlyOnConfirm" +- ac-2 — MET: Check resolves the committer through EffectiveCommitter (GIT_COMMITTER_* first, then committer.*/user.*), compares it with the author and the pin, and ahoy raises git_identity.committer; TestCheck_CommitterEnvOverrideDiverges, TestCheck_CommitterConfigDiverges and TestDetectGitIdentity_CommitterDiverges pass at BASE + evidence: internal/core/identity/identity.go:371 — "committer, err := EffectiveCommitter(root)" + evidence: internal/core/identity/identity.go:403 — "func committerDivergence(pin Pin, pinned bool, author, committer Effective) (bool, string)" + evidence: internal/core/ahoy/detect.go:364 — "ID: CommitterGapID, Category: ConfigChange, Scope: "repo"," + evidence: internal/core/identity/committer_test.go:60 — "func TestCheck_CommitterEnvOverrideDiverges" +- ac-3 — MET: with no TerminalPrompter reporting a tty the step refuses before asking, writes nothing and records the reason, and --yes is refused the same way; the CLI's stdinPrompter reports its tty; TestStepGitIdentity_NoTerminalFailsClosed asserts no question, no config change and the refusal text, and TestAhoyInstallPipedAnswersNeverRewriteTheIdentity covers the front door + evidence: internal/core/ahoy/identity_establish.go:53 — "if !atTerminal(a.prompter) {" + evidence: internal/core/ahoy/identity_establish.go:49 — "if a.autoYes {" + evidence: internal/surface/cli/cli.go:3782 — "func (p *stdinPrompter) AtTerminal() bool { return p.tty }" + evidence: internal/core/ahoy/identity_establish_test.go:189 — "func TestStepGitIdentity_NoTerminalFailsClosed" + evidence: internal/surface/cli/ahoy_identity_gate_test.go:54 — "func TestAhoyInstallPipedAnswersNeverRewriteTheIdentity" +- ac-4 — MET_WITH_CONCERNS: detection is delivered: IsToolIdentity reads the gate's own tool-identities list plus the structural bot and noreply signals, Check sets AuthorIsTool/CommitterIsTool, and ahoy raises the required git_identity.tool gap whose hint names the runner record; concern: the criterion defers establishment to iss-2608210932052003, which is still in issues/open/, so a routine's identity is established by nothing abcd ships today and the deferral has no delivered counterpart to verify against + evidence: internal/core/identity/toolidentity.go:42 — "func IsToolIdentity(role Role, name, email string) bool {" + evidence: internal/core/identity/identity.go:378 — "res.AuthorIsTool = eff != (Effective{}) && IsToolIdentity(RoleAuthor, eff.Name, eff.Email)" + evidence: internal/core/ahoy/detect.go:374 — "ID: ToolIdentityGapID, Category: ConfigChange, Scope: "repo"," + evidence: internal/core/ahoy/identity_establish_test.go:106 — "func TestDetectGitIdentity_ToolIdentity" + evidence: .abcd/work/issues/open/iss-2608210932052003-abcd-launches-autonomous-routines.md:1 — "open/" +- ac-5 — MET: the only write is git config --local on the repository's own .git/config under a scrubbed environment, reached solely after Confirm; an outranking GIT_*_ override or author.*/committer.* key makes the step name what to unset instead of writing; TestStepGitIdentity_EnvOverrideIsNotRewritten and TestStepGitIdentity_DeclineWritesNothing pass at BASE + evidence: internal/core/identity/establish.go:83 — "cmd := exec.Command("git", "-C", root, "config", "--local", kv[0], kv[1])" + evidence: internal/core/ahoy/identity_establish.go:71 — "if len(over) > 0 {" + evidence: internal/core/ahoy/identity_establish_test.go:248 — "func TestStepGitIdentity_EnvOverrideIsNotRewritten" + evidence: internal/core/ahoy/identity_establish_test.go:169 — "func TestStepGitIdentity_DeclineWritesNothing" + +Gap audit: +- honoured: + - detect here, establish at launch: the gate detects and proposes, and names the runner record for a routine (decision 1) + evidence: internal/core/ahoy/identity_establish.go:54 — "An autonomous routine's runner sets the human identity before its first commit (" + routineRunnerRecord + ")" + - the proposal chain is disk-only, pinned then global, no gh fallback (decision 2, adr-38) + evidence: internal/core/identity/establish.go:20 — "Propose returns the identity to offer: the committed pin, else the global git // identity" + - the committer is resolved env-first then config, not through git var, so StatusUnset survives (open question 1) + evidence: internal/core/identity/identity.go:278 — "func EffectiveCommitter(root string) (Effective, error)" + evidence: internal/core/identity/committer_test.go:143 — "func TestCheck_UnsetSurvivesTheCommitterPath" + - doctor shares the detector: detectGitIdentity runs inside Detect, which every read-only mode renders + evidence: internal/core/ahoy/detect.go:96 — "gaps = append(gaps, detectGitIdentity(abs)...)" + - a forge committer (GitHub < noreply@github.com>) is not read as a tool, matching the attribution gate's role asymmetry + evidence: internal/core/ahoy/identity_establish_test.go:126 — "func TestDetectGitIdentity_ForgeCommitterIsNotATool" + - the plugin page relays the four identity gaps and the person-only answer + evidence: commands/ahoy.md:169 — "(`git_identity.mismatch`, `git_identity.unset`, `git_identity.committer`, `git_identity.tool`)" + - the un-pinned repo's pin adoption stays a prompt-only step and refuses a machine identity + evidence: internal/core/ahoy/apply.go:433 — "if identity.IsToolIdentity(identity.RoleAuthor, eff.Name, eff.Email) {" +- diverged: (none) +- missing: + - establishing a routine's human identity before its first commit is verified against the runner: the runner record iss-2608210932052003 is still open, so the establish half the press release promises for autonomous routines has no shipped counterpart + evidence: .abcd/work/issues/open/iss-2608210932052003-abcd-launches-autonomous-routines.md:1 — "open/" + evidence: internal/core/ahoy/detect.go:377 — "An autonomous routine has no one to ask: whatever launches it sets the human identity before the first commit" + diff --git a/.abcd/development/intents/shipped/itd-2609212137116617-a-new-capture-or-draft-is-matched-against-the-record-before.md b/.abcd/development/intents/shipped/itd-2609212137116617-a-new-capture-or-draft-is-matched-against-the-record-before.md index 4856542f9..1c2925f52 100644 --- a/.abcd/development/intents/shipped/itd-2609212137116617-a-new-capture-or-draft-is-matched-against-the-record-before.md +++ b/.abcd/development/intents/shipped/itd-2609212137116617-a-new-capture-or-draft-is-matched-against-the-record-before.md @@ -69,8 +69,71 @@ _None open._ ## Audit Notes - -Fidelity review OWED (receipt rcp-1887e5f574ef). + +Fidelity review — receipt rcp-1887e5f574ef (verifier intent-auditor claude-fable-5-1). + +Provenance: intent-auditor@claude-fable-5-1 · rubric_hash sha256:effa65b3e9e88ff29433b443ec2be159522a8b0b71cf1434526514aa61edb13e · prompt_hash sha256:55cacd764222cf6126e4455ac627a86ccad5b272fd5e58b31e6d99d69a11c824 +Input attestations: tree:ceb4b6dbb97622bddf2401c91f9f808ed05060a0 (origin/main; internal/core/record/match, internal/core/capture/match.go, internal/core/intent/match.go, internal/surface/cli/match.go)@-; + +Acceptance rollup: MET 5 · MET_WITH_CONCERNS 0 · NOT_MET 0 · INCONCLUSIVE 0 + +Per-criterion verdicts: +- ac-1 — MET: the capture workflow runs matchAndLink under the ledger lock before the write, writes duplicates:/refines: into the record's frontmatter, and the CLI prints each link written; TestCaptureLinksAPlantedDouble and TestCaptureVerbLinksAndPrintsTheMatch exercise both ends and pass at BASE + evidence: internal/core/capture/workflow.go:267 — "content, matched = matchAndLink(repoRoot, issuesRoot, *req.Match, req.Text, content, fm)" + evidence: internal/core/capture/match.go:78 — "func matchAndLink(repoRoot, issuesRoot string, cfg match.Config, text, content string, fm map[string]any) (string, *match.Outcome)" + evidence: internal/surface/cli/match.go:36 — "func renderMatch(w io.Writer, o *match.Outcome)" + evidence: internal/core/capture/match_test.go:66 — "func TestCaptureLinksAPlantedDouble" + evidence: internal/surface/cli/match_cli_test.go:67 — "func TestCaptureVerbLinksAndPrintsTheMatch" +- ac-2 — MET: the quoted-text intent create matches title plus press release under the mint lock against every intent's H1 and press release (and the ledger), seeds the typed links into the draft, and the CLI prints the outcome; TestCreateFromTextMatchedLinksADouble and TestIntentCreateVerbLinksAndPrintsTheMatch pass at BASE + evidence: internal/core/intent/create.go:324 — "outcome = runMatch(opts.Match, opts.Title+"\n"+opts.PressRelease)" + evidence: internal/core/intent/create.go:468 — "for _, rel := range []match.Relation{match.Duplicates, match.Refines} {" + evidence: internal/core/intent/match.go:35 — "func MatchTexts(repoRoot string) ([]MatchText, error)" + evidence: internal/surface/cli/cli.go:2686 — "it, err := intent.CreateFromTextMatched(repoRoot, text, opts, m)" + evidence: internal/surface/cli/match_cli_test.go:120 — "func TestIntentCreateVerbLinksAndPrintsTheMatch" +- ac-3 — MET: an unreadable candidate set, a refused configuration or a link the schema refuses all come back as an outcome with the record filed unlinked, never an error; a record whose link line is removed is listed back as ordinary; TestCaptureIsNeverRefusedByTheMatch, TestCreateFromTextMatchedNeverRefuses, TestCaptureVerbFilesThroughARefusedConfiguration and TestRemovingTheLinkLeavesAnOrdinaryRecord pass at BASE + evidence: internal/core/capture/match.go:113 — "o.Skipped = fmt.Sprintf("the links could not be written (%v), so the record is filed unlinked", err)" + evidence: internal/surface/cli/match.go:26 — "so the record is filed without matching; fix or remove the key" + evidence: internal/core/capture/match_test.go:176 — "func TestRemovingTheLinkLeavesAnOrdinaryRecord" + evidence: internal/core/intent/match_test.go:71 — "func TestCreateFromTextMatchedNeverRefuses" +- ac-4 — MET: a score below the threshold takes no Relation and is never linked, up to five near misses travel on the outcome with score and reverse, the threshold and compared fields come from match.threshold / match.fields through the layered reader with bundled defaults, and --json carries near_misses; TestCaptureBelowTheThresholdWritesNothingAndListsNearMisses, TestCaptureVerbJSONListsNearMisses and TestTheThresholdIsHonoured pass at BASE + evidence: internal/core/record/match/match.go:317 — "if len(o.NearMisses) < NearMissLimit {" + evidence: internal/core/record/match/config.go:67 — "th, err := layered.Get(s, "match.threshold", DefaultThreshold," + evidence: internal/core/capture/match_test.go:120 — "func TestCaptureBelowTheThresholdWritesNothingAndListsNearMisses" + evidence: internal/surface/cli/match_cli_test.go:89 — "func TestCaptureVerbJSONListsNearMisses" +- ac-5 — MET: the itd-84 discipline record carries a Delivered rung paragraph naming this intent as what delivers the capture-time candidate pass, and says what it does not deliver + evidence: .abcd/development/intents/disciplines/itd-84-intent-decomposition.md:63 — "**Delivered rung: the capture-time candidate pass.** The lexical shortlist of rule 2 runs at filing, delivered by" + evidence: .abcd/development/intents/disciplines/itd-84-intent-decomposition.md:65 — "[itd-2609212137116617] (../shipped/itd-2609212137116617-a-new-capture-or-draft-is-matched-against-the-record-before.md)" + +Gap audit: +- honoured: + - file it, link it, say so; never refuse, never drop (decision 1) + evidence: internal/core/intent/match.go:105 — "The match never refuses the create" + evidence: internal/core/capture/match.go:76 — "It never fails the capture" + - a lexical heuristic declared as one on every outcome, threshold as configuration (decision 2) + evidence: internal/core/record/match/match.go:31 — "const Heuristic = "lexical term overlap, weighted by how rare each term is across the candidates; a heuristic, not a judgement"" + evidence: internal/core/record/match/config.go:54 — "LoadConfig resolves match.threshold and match.fields through the one layered" + - candidates are every open and resolved issue and every intent's title and press release; wontfix is excluded + evidence: internal/core/capture/match.go:43 — "for _, st := range []State{StateOpen, StateResolved} {" + - only duplicates and refines are ever proposed; reverses and supersedes stay human (itd-84 rule 3) + evidence: internal/core/record/match/match.go:34 — "a lexical // score can only ever propose the two below" + - the plugin pages document the links and near_misses + evidence: commands/capture.md:117 — "`duplicates: []` when the two hold each other's terms" + evidence: commands/intent.md:80 — "`duplicates:` or `refines:` link for each likely double (itd-2609212137116617)" +- diverged: + - a new issue is matched at filing (press release): the ledger's other filing paths file without matching — inbox promote builds a CaptureRequest with no Match, and the consistency and reading ingests write issue records outside matchAndLink; only the capture verb and the quoted-text intent create match + evidence: internal/core/report/inbox.go:683 — "return capture.CaptureRequest{" + evidence: internal/core/capture/workflow.go:267 — "content, matched = matchAndLink(repoRoot, issuesRoot, *req.Match" + evidence: internal/core/capture/consistency.go:22 — "func IngestConsistency(repoRoot string, payload []byte, date string)" + - the overlap function is the one the embark ranking uses, moved to the record package (spec Scope 1 / Approach): no such function existed, the primitive shipped new as internal/core/record/match; recorded by the spec amendment and iss-2609261631134401 + evidence: .abcd/development/specs/closed/spc-2609212141417782-a-new-capture-or-draft-is-matched-against-the-record-before.md:51 — "The Scope, Approach and Footprint above name a delivery that did not happen" +- missing: (none) + +Scope-condition dispositions: +- cond-2609212141415326 — survived: a text with fewer than MinTerms (8) distinct terms is filed with no candidate gathered and the outcome's Skipped names the declared minimum, which the CLI prints as not matched; TestAShortTextIsFiledWithoutMatchingAndSaysSo passes at BASE + evidence: internal/core/record/match/match.go:283 — "o.Skipped = "the text carries fewer distinct terms than the declared minimum, so it is filed without matching"" + evidence: internal/core/record/match/match.go:263 — "func Short(text string) bool { return len(Terms(text)) < MinTerms }" + evidence: internal/core/record/match/match_test.go:131 — "func TestAShortTextIsFiledWithoutMatchingAndSaysSo" + ## Grounds diff --git a/.abcd/development/intents/shipped/itd-63-setup-wizard-explains-installs.md b/.abcd/development/intents/shipped/itd-63-setup-wizard-explains-installs.md index e92775125..a00b6aa56 100644 --- a/.abcd/development/intents/shipped/itd-63-setup-wizard-explains-installs.md +++ b/.abcd/development/intents/shipped/itd-63-setup-wizard-explains-installs.md @@ -76,8 +76,70 @@ _None open; decisions 1 to 3 settle the three this record carried._ ## Audit Notes - -Fidelity review OWED (receipt rcp-3c9fb4ba9770). + +Fidelity review — receipt rcp-3c9fb4ba9770 (verifier intent-auditor claude-fable-5-1). + +Provenance: intent-auditor@claude-fable-5-1 · rubric_hash sha256:effa65b3e9e88ff29433b443ec2be159522a8b0b71cf1434526514aa61edb13e · prompt_hash sha256:02987ee70399625e2472201d6961c35bff8f37408256c2f78915e8785d534a0e +Input attestations: tree:ceb4b6dbb97622bddf2401c91f9f808ed05060a0 (origin/main; commits 0289455eb, edef56854, 705b8216d, 291410bb5, 09938d677, 7f34670d8 under internal/core/tools)@-; + +Acceptance rollup: MET 2 · MET_WITH_CONCERNS 2 · NOT_MET 1 · INCONCLUSIVE 0 + +Per-criterion verdicts: +- ac-1 — MET: Explain renders name, optional/required per (tool, capability), what works without it, what it does and the platform step from the compiled registry; TestExplainNamesEveryPartTheCriterionAsksFor and the ahoy gap detector both exercise it, and the ahoy gap and the missing-gitleaks/gh refusals carry the lines + evidence: internal/core/tools/explain.go:124 — "func (e Explanation) Lines() []string" + evidence: internal/core/tools/explain.go:133 — "e.Tool + " — " + string(e.Requirement) + " for " + e.CapabilityName" + evidence: internal/core/tools/registry.go:118 — "var registry = map[string]Tool{" + evidence: internal/core/ahoy/detect.go:231 — "e := tools.Explain("gitleaks", capability)" + evidence: internal/core/tools/tools_test.go:17 — "func TestExplainNamesEveryPartTheCriterionAsksFor" +- ac-2 — MET_WITH_CONCERNS: Install runs the step only after Confirm returns yes (nil or no or CI or --yes all decline), Result.Summary reports what ran and whether it verified, and every declined path ends in OnDecline (continuing on the native secret scanner); concern: the install is offered for gitleaks alone (DependencyTools), so for gh the explanation is shown in the refusal but no verb ever asks the question or runs the step + evidence: internal/core/tools/install.go:179 — "ans := confirm(e) if !ans.Yes {" + evidence: internal/core/tools/install.go:61 — "func (r Result) Summary() string" + evidence: internal/core/ahoy/apply.go:465 — "res := newToolInstaller(a.cwd).Install(g.Tool.Tool, g.Tool.Capability, a.confirmTool)" + evidence: internal/surface/cli/cli.go:3711 — "--yes never installs a tool" + evidence: internal/core/ahoy/detect.go:207 — "var DependencyTools = []string{"gitleaks"}" + evidence: internal/core/ahoy/tools_route_test.go:122 — "func TestDependencyNoKeepsTheNativeDefaultAndSaysSo" +- ac-3 — MET_WITH_CONCERNS: an unregistered name gets the generic text, no step (install step: none known to abcd) and a RegistryGap line naming the abcd capture that records it; concern: abcd files no capture itself (the line is printed for whoever meets it), and every production caller passes a literal registry name, so the unknown path is reached only by tests + evidence: internal/core/tools/explain.go:87 — "func unknown(name string, capability Capability) Explanation" + evidence: internal/core/tools/explain.go:104 — "func gapCapture(name string, capability Capability) string" + evidence: internal/core/tools/tools_test.go:67 — "func TestUnknownToolGetsGenericTextAndARegistryGap" + evidence: internal/core/ahoy/tools_route_test.go:183 — "func TestEveryToolAhoyNamesIsRegistered" +- ac-4 — NOT_MET: promised: the safety gate's missing-scanner case routes through this mode; delivered: no safety gate exists on main (itd-62 is still in intents/drafts/), so no gate surfaces the prerequisite at all; the close note substitutes the history store's armed-gitleaks refusal, which does carry tools.Missing, but that is transcript capture, not the safety gate the criterion names + evidence: .abcd/development/intents/drafts/itd-62-pluggable-safety-gate.md:1 — "itd-62-pluggable-safety-gate.md (drafts/)" + evidence: .abcd/development/specs/closed/spc-2609211955339422-setup-wizard-explains-installs.md:62 — "belongs to itd-62, which is still a draft: no gate on the default branch always blocks on a missing scanner" + evidence: internal/core/history/history.go:277 — "err = tools.Missing(err, "gitleaks", tools.TranscriptScanArmed)" +- ac-5 — MET: the package reads no terminal and writes no stdout; the CLI supplies the Confirm and asks on a tty itself, refusing piped or --yes answers, and TestToolConfirmAsksOnlyAtATerminal plus TestAhoyInstallNamedToolReachesTheStep run the whole path with no host present + evidence: internal/core/tools/registry.go:9 — "The package has no transport // knowledge — it never reads a terminal and never writes to stdout" + evidence: internal/surface/cli/cli.go:3705 — "func toolConfirm(p ahoy.Prompter, named map[string]bool, yes bool, w io.Writer) tools.Confirm" + evidence: internal/surface/cli/ahoy_tool_confirm_test.go:25 — "func TestToolConfirmAsksOnlyAtATerminal" + +Gap audit: +- honoured: + - a curated registry abcd ships, with what the tool is, why the capability uses it, the native default and the exact step per platform (decision 2) + evidence: internal/core/tools/registry.go:118 — "var registry = map[string]Tool{" + - the install runs on confirmation and its result is reported (decision 1) + evidence: internal/core/tools/install.go:144 — "func (in *Installer) Install(name string, capability Capability, confirm Confirm) Result" + - a mode other verbs call, not a surface of its own (decision 3): ahoy detect/install, history capture, ahoy remote call it + evidence: internal/core/ahoy/detect.go:231 — "tools.Explain("gitleaks", capability)" + evidence: internal/core/ahoy/remote.go:458 — "return nil, tools.Missing(" + - honesty about what the install does to the machine and the network + evidence: internal/core/tools/registry.go:109 — "func homebrewEffects(program string) string" + - declining never weakens a gate: the armed-gitleaks refusal stands, and the install never runs in CI or inside the repository tree + evidence: internal/core/tools/install.go:159 — "if reason, ci := cienv.Runner(in.Getenv); ci {" + evidence: internal/core/tools/install.go:237 — "func (in *Installer) admit(name string) (string, error)" + - the plugin page relays the explanation and the host-relayed yes through --install-tool + evidence: commands/ahoy.md:179 — "**The tool question.**" +- diverged: + - the install question is asked for every missing tool the mode explains: gh is explained in the ahoy remote / site setup refusal but never offered (DependencyTools holds gitleaks only, and --install-tool gh is refused) + evidence: internal/core/ahoy/detect.go:207 — "var DependencyTools = []string{"gitleaks"}" + evidence: internal/surface/cli/cli.go:3691 — "is not a tool ahoy install checks for" + - a registry gap is captured: abcd prints the capture command for the person to run, it does not file the capture + evidence: internal/core/tools/explain.go:104 — "record the gap in abcd's own ledger with: abcd capture" + - the guard's and the launch's tool checks reroute through the mode (spec scope 3): neither exists, nothing was rerouted + evidence: .abcd/development/specs/closed/spc-2609211955339422-setup-wizard-explains-installs.md:68 — "do not exist: the guard runs no external tool, and the launch scans are native" +- missing: + - the safety gate's missing-scanner path routes through the mode (first consumer, itd-62): the gate is not on main + evidence: .abcd/development/intents/drafts/itd-62-pluggable-safety-gate.md:1 — "drafts/" + ### Linkage note (spc-83.5) diff --git a/.abcd/development/release/surface.json b/.abcd/development/release/surface.json index d1f566e91..649b86d7d 100644 --- a/.abcd/development/release/surface.json +++ b/.abcd/development/release/surface.json @@ -2032,6 +2032,27 @@ } ] }, + { + "path": "abcd launch manifests", + "hidden": false, + "sentence": "Check the release manifests agree on the version, or carry none on a dev tree: Writes nothing; refuses with exit 1 on drift and exit 2 on an unreadable input.", + "flags": [ + { + "name": "root", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, + { + "name": "tree", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + } + ] + }, { "path": "abcd launch receipts", "hidden": false, diff --git a/.abcd/docs-lint.json b/.abcd/docs-lint.json index 2b587a2c1..1240a4c56 100644 --- a/.abcd/docs-lint.json +++ b/.abcd/docs-lint.json @@ -7,7 +7,13 @@ ".abcd", "AGENTS.md", ".github/CONTRIBUTING.md", - "scripts" + "scripts", + "commands", + "agents", + "hooks", + ".claude-plugin", + "LICENSE", + ".gitignore" ], "banned_tokens": [ { diff --git a/.abcd/work/issues/open/iss-127-repo-local-lmw-sites-unguarded.md b/.abcd/work/issues/open/iss-127-repo-local-lmw-sites-unguarded.md deleted file mode 100644 index 5fde99621..000000000 --- a/.abcd/work/issues/open/iss-127-repo-local-lmw-sites-unguarded.md +++ /dev/null @@ -1,12 +0,0 @@ ---- -schema_version: 1 -id: "iss-127" -slug: "repo-local-lmw-sites-unguarded" -severity: "minor" -category: "tech-debt" -source: "agent-finding" -found_during: "iss-101/102 class sweep (2026-07-24 run queue, burst 3)" -found_at: "internal/core/intent/lifecycle.go" ---- - -repo-local load-modify-write sites remain unguarded after the iss-101/102 class fix: intent transitions (review.go, lifecycle.go — strongest candidate), ahoy config.json stepConfigValues, gitignore/marker block rewrites, and the CHANGELOG release path all do load-mutate-write without a lock; they lack the home-global cross-worktree exposure the fixed sites had, so risk is same-worktree concurrency only \ No newline at end of file diff --git a/.abcd/work/issues/open/iss-2609231206190526-itd-2609221656373558-criterion-4-promises-the-second-session.md b/.abcd/work/issues/open/iss-2609231206190526-itd-2609221656373558-criterion-4-promises-the-second-session.md index e3fa5e102..09fe9a2c9 100644 --- a/.abcd/work/issues/open/iss-2609231206190526-itd-2609221656373558-criterion-4-promises-the-second-session.md +++ b/.abcd/work/issues/open/iss-2609231206190526-itd-2609221656373558-criterion-4-promises-the-second-session.md @@ -8,6 +8,8 @@ source: "user-observation" found_during: "autonomous run 2026-09-23 fidelity audit" origin: researcher-authored production_mode: hand-written +deferred_after: "v0.11.0" +deferral_reason: "ruling owed to the product thinker (rulings-owed section D, narrowing a shipped promise; run A 2026-09-28): Should the second session be refused any lane it opens without declaring the paths it will touch (so the reading-corpus bound cannot be sidestepped by saying nothing, at the cost of declaring paths before every claim), or should itd-2609221656373558 criterion 4 be amended to say the refusal holds for the paths the session declares?" --- itd-2609221656373558 criterion 4 promises the second session refuses a lane that recalibrates the reading corpus. Delivered: the corpus bound is applied only to paths the session declares, and a claim or lane check that declares no paths asks no corpus question (internal/core/implement/bounds.go:94-98), so a second session that names no paths opens a corpus lane unrefused. The refusal rests on self-declaration; the record should say so, or the bound should refuse an undeclared lane for the second role. Found by the fidelity audit; not fixed here. diff --git a/.abcd/work/issues/open/iss-2609261423214723-itd-138-ac-1-promises-that-the-template-under-site-src-that.md b/.abcd/work/issues/open/iss-2609261423214723-itd-138-ac-1-promises-that-the-template-under-site-src-that.md index a6a7f495f..5b91fd8ca 100644 --- a/.abcd/work/issues/open/iss-2609261423214723-itd-138-ac-1-promises-that-the-template-under-site-src-that.md +++ b/.abcd/work/issues/open/iss-2609261423214723-itd-138-ac-1-promises-that-the-template-under-site-src-that.md @@ -8,6 +8,8 @@ source: "review-followup" found_during: "autonomous run A resumed 2026-09-25: fidelity audit itd-138" origin: researcher-authored production_mode: hand-written +deferred_after: "v0.11.0" +deferral_reason: "ruling owed to the product thinker (rulings-owed section D, narrowing a shipped promise; run A 2026-09-28): Should README's install one-liner be generated from site-src/install.sh.tmpl (a new writer to a committed file, and the template restructured to hold both forms), or should itd-138 ac-1 be amended to accept the one-liner written by hand and held to the script by TestInstallSurfacesAgree?" --- itd-138 ac-1 promises that the template under site-src/ that generates /install.sh also renders README's one-liner, so agreement between the two is structural. Delivered: README.md:106 carries a hand-written sh -c one-liner that nothing renders; TestInstallSurfacesAgree (internal/core/site/installsurface_test.go:158, reading README at line 308) holds it to the template's release prefix, checksum lookup and verifier by assertion, so a drift fails the build but the one-liner is still authored twice. Wanted: either the README block is rendered from site-src/install.sh.tmpl, or the intent's audit records the by-assertion form as the accepted narrowing. diff --git a/.abcd/work/issues/open/iss-2609261457353277-itd-74-ac-5-promises-the-public-family-present-after-ahoy.md b/.abcd/work/issues/open/iss-2609261457353277-itd-74-ac-5-promises-the-public-family-present-after-ahoy.md index 8bf8d97f1..1931bcfb9 100644 --- a/.abcd/work/issues/open/iss-2609261457353277-itd-74-ac-5-promises-the-public-family-present-after-ahoy.md +++ b/.abcd/work/issues/open/iss-2609261457353277-itd-74-ac-5-promises-the-public-family-present-after-ahoy.md @@ -8,6 +8,8 @@ source: "review-followup" found_during: "autonomous run A resumed 2026-09-25: fidelity audit itd-74" origin: researcher-authored production_mode: hand-written +deferred_after: "v0.11.0" +deferral_reason: "ruling owed to the product thinker (rulings-owed section D, narrowing a shipped promise; run A 2026-09-28): Under visibility public, where no .abcd/ file is committed and the fence ignores the whole namespace, should the public banned-names config move outside .abcd/, should the fence carry one un-ignore for it, or should itd-74 ac-5 be amended so a public-visibility repo relies on the private layer alone?" --- itd-74 ac-5 promises the public family present after ahoy scaffolding; where the docs-lint config path is gitignored (TestPublicFamilyUnderPublicVisibility) install writes no public family and reports the gap as unresolvable, leaving that repo with no CI-enforced banned-names family diff --git a/.abcd/work/issues/open/iss-2609261457365969-itd-28-ac-1-promises-review-of-commit-written-by-the-tool-no.md b/.abcd/work/issues/open/iss-2609261457365969-itd-28-ac-1-promises-review-of-commit-written-by-the-tool-no.md index 88ff3c837..5ab40ef75 100644 --- a/.abcd/work/issues/open/iss-2609261457365969-itd-28-ac-1-promises-review-of-commit-written-by-the-tool-no.md +++ b/.abcd/work/issues/open/iss-2609261457365969-itd-28-ac-1-promises-review-of-commit-written-by-the-tool-no.md @@ -8,6 +8,8 @@ source: "review-followup" found_during: "autonomous run A resumed 2026-09-25: fidelity audit itd-28" origin: researcher-authored production_mode: hand-written +deferred_after: "v0.11.0" +deferral_reason: "ruling owed to the product thinker (rulings-owed section D, narrowing a shipped promise; run A 2026-09-28): Should an abcd verb write the dated review folders so the tool itself stamps review_of_commit, or should itd-28 ac-1 be amended to what shipped (the charter template carries the key and gate RD004 refuses a folder without it), ratifying the implementer's Decision 3?" --- itd-28 ac-1 promises review_of_commit written by the tool; no abcd path writes a dated review folder, so the pin is a charter-template obligation on the author whose absence RD004 refuses, and the narrowing was settled by the implementer in Decision 3 rather than by the product thinker diff --git a/.abcd/work/issues/open/iss-2609261457366568-itd-2609212137128014-ac-5-promises-halt-and-record-on-a-gate.md b/.abcd/work/issues/open/iss-2609261457366568-itd-2609212137128014-ac-5-promises-halt-and-record-on-a-gate.md index 39d7f9fff..c94c615e4 100644 --- a/.abcd/work/issues/open/iss-2609261457366568-itd-2609212137128014-ac-5-promises-halt-and-record-on-a-gate.md +++ b/.abcd/work/issues/open/iss-2609261457366568-itd-2609212137128014-ac-5-promises-halt-and-record-on-a-gate.md @@ -8,6 +8,8 @@ source: "review-followup" found_during: "autonomous run A resumed 2026-09-25: fidelity audit itd-2609212137128014" origin: researcher-authored production_mode: hand-written +deferred_after: "v0.11.0" +deferral_reason: "ruling owed to the product thinker (rulings-owed section D, narrowing a shipped promise; run A 2026-09-28): Should abcd lab gain a verb that records a refusal by the world's own gates or a STOP condition as a halting finding (still reported by the operator, since the verb runs none of those gates), or should itd-2609212137128014 ac-5 be amended to say the verb enforces halt-and-record for its own gates and the rest is the discipline's rule?" --- itd-2609212137128014 ac-5 promises halt-and-record on a gate refusal during a lab as a rule the verb enforces; the verb enforces it only for its own preflight and sweep gates and the harvest's citation gaps, while a refusal by the world's own gates during the mutate stage or a STOP condition is the discipline's rule with nothing mechanical behind it diff --git a/.abcd/work/issues/open/iss-2609281911015826-itd-63-criterion-4-the-safety-gate-s-missing-scanner-case.md b/.abcd/work/issues/open/iss-2609281911015826-itd-63-criterion-4-the-safety-gate-s-missing-scanner-case.md new file mode 100644 index 000000000..b1ca9901b --- /dev/null +++ b/.abcd/work/issues/open/iss-2609281911015826-itd-63-criterion-4-the-safety-gate-s-missing-scanner-case.md @@ -0,0 +1,14 @@ +--- +schema_version: 1 +id: "iss-2609281911015826" +slug: "itd-63-criterion-4-the-safety-gate-s-missing-scanner-case" +severity: "minor" +category: "drift" +source: "review-followup" +found_during: "autonomous run A resumed 2026-09-25: fidelity audit itd-63" +origin: researcher-authored +production_mode: hand-written +found_at: ".abcd/development/intents/shipped/itd-63-setup-wizard-explains-installs.md" +--- + +itd-63 criterion 4 (the safety gate's missing-scanner case routes through the explain-then-install mode) is NOT_MET at ceb4b6dbb: no safety gate exists on main, itd-62 (pluggable-safety-gate) is still in intents/drafts/, so nothing surfaces the prerequisite the criterion names. The spec's close note substitutes the history store's armed-gitleaks refusal (internal/core/history/history.go calls tools.Missing), which is transcript capture, not the gate. The shipped record names a first consumer that does not exist; when itd-62 is planned its missing-scanner path must call tools.Missing/tools.Install rather than print a bare command, and itd-63's References/Why should stop asserting that the safety gate already blocks on a missing scanner. diff --git a/.abcd/work/issues/open/iss-2609281911024185-itd-2609212137116617-s-press-release-says-a-new-issue-is.md b/.abcd/work/issues/open/iss-2609281911024185-itd-2609212137116617-s-press-release-says-a-new-issue-is.md new file mode 100644 index 000000000..1240079a2 --- /dev/null +++ b/.abcd/work/issues/open/iss-2609281911024185-itd-2609212137116617-s-press-release-says-a-new-issue-is.md @@ -0,0 +1,14 @@ +--- +schema_version: 1 +id: "iss-2609281911024185" +slug: "itd-2609212137116617-s-press-release-says-a-new-issue-is" +severity: "minor" +category: "drift" +source: "review-followup" +found_during: "autonomous run A resumed 2026-09-25: fidelity audit itd-2609212137116617" +origin: researcher-authored +production_mode: hand-written +found_at: "internal/core/report/inbox.go" +--- + +itd-2609212137116617's press release says a new issue is matched against the record at filing, but only the capture verb and the quoted-text intent create run the filing-time match. The ledger's other writers file without it: inbox promote builds a capture.CaptureRequest with no Match (internal/core/report/inbox.go captureRequest), and IngestConsistency and IngestReading (internal/core/capture/consistency.go, reading.go) write issue records outside matchAndLink (internal/core/capture/workflow.go). The unattended paths the intent's Grounds name as the reason for the match are exactly the ones that skip it, so a double promoted from a peer report or filed by a consistency pass is never linked at filing. Either these writers pass the layered match config through CaptureRequest.Match, or the record narrows its claim to the two verbs. diff --git a/.abcd/work/issues/open/iss-2609281911024838-itd-63-criterion-2-diverges-for-gh-the-explain-then-install.md b/.abcd/work/issues/open/iss-2609281911024838-itd-63-criterion-2-diverges-for-gh-the-explain-then-install.md new file mode 100644 index 000000000..3f4afcfb1 --- /dev/null +++ b/.abcd/work/issues/open/iss-2609281911024838-itd-63-criterion-2-diverges-for-gh-the-explain-then-install.md @@ -0,0 +1,14 @@ +--- +schema_version: 1 +id: "iss-2609281911024838" +slug: "itd-63-criterion-2-diverges-for-gh-the-explain-then-install" +severity: "minor" +category: "drift" +source: "review-followup" +found_during: "autonomous run A resumed 2026-09-25: fidelity audit itd-63" +origin: researcher-authored +production_mode: hand-written +found_at: "internal/core/ahoy/detect.go" +--- + +itd-63 criterion 2 diverges for gh: the explain-then-install mode's install half is offered for gitleaks alone. ahoy.DependencyTools is {gitleaks} (internal/core/ahoy/detect.go), so ahoy install never puts gh to the question and --install-tool gh is refused by installToolNames (internal/surface/cli/cli.go), while ahoy remote and site setup refuse a missing gh with tools.Missing (internal/core/ahoy/remote.go) and show the Homebrew step the person must then run by hand. A required tool the registry knows how to install is explained but never installed on a yes, so for gh the criterion's 'the install runs only on an explicit yes' never arises. Either a gh dependency gap joins DependencyTools (offered only when a verb that needs gh is in play), or the record states that the mode installs gitleaks only. diff --git a/.abcd/work/issues/resolved/iss-127-repo-local-lmw-sites-unguarded.md b/.abcd/work/issues/resolved/iss-127-repo-local-lmw-sites-unguarded.md new file mode 100644 index 000000000..b03b1713c --- /dev/null +++ b/.abcd/work/issues/resolved/iss-127-repo-local-lmw-sites-unguarded.md @@ -0,0 +1,20 @@ +--- +schema_version: 1 +id: "iss-127" +slug: "repo-local-lmw-sites-unguarded" +severity: "minor" +category: "tech-debt" +source: "agent-finding" +found_during: "iss-101/102 class sweep (2026-07-24 run queue, burst 3)" +found_at: "internal/core/intent/lifecycle.go" +resolution: "ahoy's rewrites of .abcd/config.json, the .gitignore block and the CLAUDE.md/AGENTS.md marker block, and the release cut's CHANGELOG writes and undo, now read and write under fsutil.WithFileLock on a retired lock file beside the guarded file; intent transitions were already under the intent store lock at this base" +impact: fix +resolved_by: + commit: "352b21e19" +--- + +repo-local load-modify-write sites remain unguarded after the iss-101/102 class fix: intent transitions (review.go, lifecycle.go — strongest candidate), ahoy config.json stepConfigValues, gitignore/marker block rewrites, and the CHANGELOG release path all do load-mutate-write without a lock; they lack the home-global cross-worktree exposure the fixed sites had, so risk is same-worktree concurrency only + +## Grounds + +- pursued: two abcd runs in one working tree rewriting the same file keep both changes, or the later cut is refused as a release in flight; a concurrent writer's change missing after a race test (rewritelock_test.go, changeloglock_test.go) would show it wrong diff --git a/.abcd/work/issues/open/iss-2609231206196407-itd-2609221656373558-criterion-6-promises-that-on-contention.md b/.abcd/work/issues/resolved/iss-2609231206196407-itd-2609221656373558-criterion-6-promises-that-on-contention.md similarity index 56% rename from .abcd/work/issues/open/iss-2609231206196407-itd-2609221656373558-criterion-6-promises-that-on-contention.md rename to .abcd/work/issues/resolved/iss-2609231206196407-itd-2609221656373558-criterion-6-promises-that-on-contention.md index 0acd635e4..2bd21c34f 100644 --- a/.abcd/work/issues/open/iss-2609231206196407-itd-2609221656373558-criterion-6-promises-that-on-contention.md +++ b/.abcd/work/issues/resolved/iss-2609231206196407-itd-2609221656373558-criterion-6-promises-that-on-contention.md @@ -8,6 +8,14 @@ source: "user-observation" found_during: "autonomous run 2026-09-23 fidelity audit" origin: researcher-authored production_mode: hand-written +resolution: "Every backoff the run log holds now names its reason and minutes: the second session's backoff from a locked run state is logged with on=run_state and the minutes it waited, the claim-denied and unreadable-claim backoffs carry the minutes the attempt spent, measured rather than a constant 0, and a hand-logged backoff (the path for contention the verb cannot see, such as the merge queue) is refused without a reason or numeric minutes." +impact: fix +resolved_by: + commit: "fbbd0d709" --- itd-2609221656373558 criterion 6 promises that on contention of any kind the second session backs off and the log names the reason and the minutes spent. Delivered: a refused claim writes a backoff line with the reason but minutes fixed at 0 (internal/core/implement/claim.go:232), and a locked run state returns ErrContention from withLock with no log line at all (internal/core/implement/run.go:182-186), so lock contention is never counted in the derived comparison and the verb measures no backed-off minutes; queue contention is logged only when the session hand-writes implement log backoff. Found by the fidelity audit; not fixed here. + +## Grounds + +- pursued: contention of any kind the second session meets leaves a backoff line naming reason and minutes; shown wrong if a lock or claim contention returns exit 3 with no backoff line, or a backoff line lacks either field diff --git a/.abcd/work/issues/open/iss-2609231206207995-itd-43-criterion-1-promises-no-live-reference-to-epic-as-a.md b/.abcd/work/issues/resolved/iss-2609231206207995-itd-43-criterion-1-promises-no-live-reference-to-epic-as-a.md similarity index 52% rename from .abcd/work/issues/open/iss-2609231206207995-itd-43-criterion-1-promises-no-live-reference-to-epic-as-a.md rename to .abcd/work/issues/resolved/iss-2609231206207995-itd-43-criterion-1-promises-no-live-reference-to-epic-as-a.md index faa6511df..86d7aa7b1 100644 --- a/.abcd/work/issues/open/iss-2609231206207995-itd-43-criterion-1-promises-no-live-reference-to-epic-as-a.md +++ b/.abcd/work/issues/resolved/iss-2609231206207995-itd-43-criterion-1-promises-no-live-reference-to-epic-as-a.md @@ -8,6 +8,14 @@ source: "user-observation" found_during: "autonomous run 2026-09-23 fidelity audit" origin: researcher-authored production_mode: hand-written +resolution: "Already met at the tip: the one live 'epic' the audit found, in the fenced lifecycle diagram of .abcd/development/brief/04-surfaces/05-intent.md, was reworded by af1c19d98 (the owed-review drain), and the passage now reads 'Nothing runs the reviewer on its own' (05-intent.md:262-268). A word-bounded grep of the brief outside the glossary's own declarations finds no epic, so itd-43 criterion 1 holds without a change here." +impact: internal +resolved_by: + commit: "af1c19d98" --- itd-43 criterion 1 promises no live reference to epic as a standalone noun in abcd-owned files. One survives on the brief's intent surface page, .abcd/development/brief/04-surfaces/05-intent.md:244 ('no epic currently owns it'), inside the fenced surface diagram opened at line 176, which GL002 skips by design as fenced text and a grep finds. Reword it to spec, or record that fenced diagrams are out of the sweep's scope. Found by the fidelity audit; not fixed here. + +## Grounds + +- pursued: the brief carries no live epic noun outside the glossary's forbidden_synonyms; shown wrong if a word-bounded grep of .abcd/development/brief outside glossary/ finds one diff --git a/.abcd/work/issues/open/iss-2609240133234244-fidelity-audit-of-itd-2609221656361680-ac-4-the-inbox.md b/.abcd/work/issues/resolved/iss-2609240133234244-fidelity-audit-of-itd-2609221656361680-ac-4-the-inbox.md similarity index 69% rename from .abcd/work/issues/open/iss-2609240133234244-fidelity-audit-of-itd-2609221656361680-ac-4-the-inbox.md rename to .abcd/work/issues/resolved/iss-2609240133234244-fidelity-audit-of-itd-2609221656361680-ac-4-the-inbox.md index 0a5ffb077..0659fdc81 100644 --- a/.abcd/work/issues/open/iss-2609240133234244-fidelity-audit-of-itd-2609221656361680-ac-4-the-inbox.md +++ b/.abcd/work/issues/resolved/iss-2609240133234244-fidelity-audit-of-itd-2609221656361680-ac-4-the-inbox.md @@ -8,6 +8,14 @@ source: "user-observation" found_during: "autonomous run 2026-09-23 fidelity audit" origin: researcher-authored production_mode: hand-written +resolution: "An unreadable waiting report now names its sender: report.EnvelopeSender reads the envelope apart from the reporter's block, taking sender_name only when the block names it once in a filing's shape beside a single sender_key equal to the file's key; the inbox listing, show and the text line carry it." +impact: fix +resolved_by: + commit: "efe9aa63a" --- Fidelity audit of itd-2609221656361680, ac-4 (the inbox renders waiting reports naming the sender repository): a waiting report the inbox cannot read — a later template version, or a malformed block — is listed by abcd inbox with a 12-hex prefix of its sender key and no sender name (internal/surface/cli/report.go, the UNREADABLE line), because the name sits in the envelope keys the failed parse never reaches (report.go judges schema_version before any other key). The intent's decision 4 puts identity in the inbox so the reader knows who is asking; for exactly the report that most needs a reply (a sender running a newer abcd), the reader gets a key prefix. The envelope is abcd's own writing, so it could be read independently of the reporter's block; or the unreadable line could name the sender from the promoted log or the other reports sharing that key. Minor; the report is not dropped and show renders the same. + +## Grounds + +- pursued: a report written to a later template, envelope intact, is listed with its sender's name; shown wrong if such a report lists with a key prefix alone diff --git a/.abcd/work/issues/open/iss-2609261423222935-itd-69-ac-1-and-ac-2-promise-a-checker-that-runs-with-tree.md b/.abcd/work/issues/resolved/iss-2609261423222935-itd-69-ac-1-and-ac-2-promise-a-checker-that-runs-with-tree.md similarity index 64% rename from .abcd/work/issues/open/iss-2609261423222935-itd-69-ac-1-and-ac-2-promise-a-checker-that-runs-with-tree.md rename to .abcd/work/issues/resolved/iss-2609261423222935-itd-69-ac-1-and-ac-2-promise-a-checker-that-runs-with-tree.md index ef087d318..bd5f27151 100644 --- a/.abcd/work/issues/open/iss-2609261423222935-itd-69-ac-1-and-ac-2-promise-a-checker-that-runs-with-tree.md +++ b/.abcd/work/issues/resolved/iss-2609261423222935-itd-69-ac-1-and-ac-2-promise-a-checker-that-runs-with-tree.md @@ -8,6 +8,14 @@ source: "review-followup" found_during: "autonomous run A resumed 2026-09-25: fidelity audit itd-69" origin: researcher-authored production_mode: hand-written +resolution: "abcd launch manifests --tree public|dev [--root ] runs the manifest lockstep check over a named tree at the chosen polarity through launch.CheckTree, read-only, exiting 0 consistent, 1 drift and 2 unreadable, with no bypass flag; wired on the CLI and the plugin page." +impact: additive +resolved_by: + commit: "9a33bdcb2" --- itd-69 ac-1 and ac-2 promise a checker that runs with --tree public and --tree dev. Delivered: launch.CheckLockstep (internal/core/launch/lockstep.go:51) takes the tree as a Go parameter and has no front door — dry-run and ship call it with TreeDev over the source tree (internal/core/launch/dryrun.go:114, ship.go:80) and only the payload render calls it with TreePublic over its own output (internal/core/launch/render.go:444). A public checkout — a marketplace install, a release source archive — cannot be checked for manifest lockstep from the CLI or the plugin surface at all. Wanted: a read-only verb or flag that runs CheckLockstep with a chosen tree against a named root and exits with its 0/1/2 code. + +## Grounds + +- pursued: a public checkout is checkable for manifest lockstep from the CLI and the plugin surface; shown wrong if launch manifests --tree public over a tree whose marketplace entry disagrees exits other than 1 diff --git a/.abcd/work/issues/open/iss-2609261457358637-itd-74-ac-1-promises-the-public-banlist-gates-readme-docs.md b/.abcd/work/issues/resolved/iss-2609261457358637-itd-74-ac-1-promises-the-public-banlist-gates-readme-docs.md similarity index 50% rename from .abcd/work/issues/open/iss-2609261457358637-itd-74-ac-1-promises-the-public-banlist-gates-readme-docs.md rename to .abcd/work/issues/resolved/iss-2609261457358637-itd-74-ac-1-promises-the-public-banlist-gates-readme-docs.md index e8130e84b..182a5a356 100644 --- a/.abcd/work/issues/open/iss-2609261457358637-itd-74-ac-1-promises-the-public-banlist-gates-readme-docs.md +++ b/.abcd/work/issues/resolved/iss-2609261457358637-itd-74-ac-1-promises-the-public-banlist-gates-readme-docs.md @@ -8,6 +8,14 @@ source: "review-followup" found_during: "autonomous run A resumed 2026-09-25: fidelity audit itd-74" origin: researcher-authored production_mode: hand-written +resolution: "name_roots in .abcd/docs-lint.json now list commands, agents and hooks, the plugin surfaces the shipped artefact carries, so the public banlist (the names/ family alone) gates them as it gates README and docs/; this repository has no skills/ tree, and TestRepoNameRootsCoverThePublicSurface pins every plugin surface the checkout carries." +impact: fix +resolved_by: + commit: "868b9c375" --- itd-74 ac-1 promises the public banlist gates README, docs/ and the shipped artefact; the banned_tokens family walks only the docs-lint roots (docs, README.md) and the payload render reuses those roots, so a banned public token in commands/, agents/ or skills/ ships ungated + +## Grounds + +- pursued: a names/ token written into commands/, agents/ or hooks/ is now a blocker from abcd lint docs; shown wrong if such a token passes the docs lint diff --git a/.abcd/work/issues/resolved/iss-2609281546130900-every-scanner-new-spends-four-git-execs-reading-the-caller-s.md b/.abcd/work/issues/resolved/iss-2609281546130900-every-scanner-new-spends-four-git-execs-reading-the-caller-s.md new file mode 100644 index 000000000..55a5ee4d8 --- /dev/null +++ b/.abcd/work/issues/resolved/iss-2609281546130900-every-scanner-new-spends-four-git-execs-reading-the-caller-s.md @@ -0,0 +1,22 @@ +--- +schema_version: 1 +id: "iss-2609281546130900" +slug: "every-scanner-new-spends-four-git-execs-reading-the-caller-s" +severity: "minor" +category: "tech-debt" +source: "review-followup" +found_during: "autonomous run A resumed 2026-09-25: ruling AR" +origin: researcher-authored +production_mode: hand-written +found_at: "internal/adapter/scanner/identity.go" +resolution: "ProbeIdentity reads every identity key in one git config -z --get-regexp listing under the same scrubbed env; a second listing runs only when the parent carries git -c configuration, which can move discovery or fail the command. A 37-shape pinned table passed at the base and passes unchanged; a counting git shim holds scanner.New to one exec (two with -c)." +impact: internal +resolved_by: + commit: "1683ed2ce" +--- + +Every scanner.New spends four git execs reading the caller's identity: ProbeIdentity (internal/adapter/scanner/identity.go) runs git config --get-all user.name, --get-all user.email, -z --get-regexp over user/author/committer name and email, and --get remote.origin.url, each a separate process under the scrubbed env, although the first, second and fourth read the same configuration the third lists. The review of the ciFast lane counted about 2,193 such execs per cli test run; the cost lands on every CLI verb and hook that builds a scanner. One NUL-delimited listing under the same env yields everything the four did. + +## Grounds + +- pursued: scanner.New spends one git exec on identity with no -c configuration in the parent and two with it, and ProbeIdentity answers every pinned shape exactly as the four-exec probe did; a counting-shim run above one exec, or any pinned shape answering differently, would show it wrong diff --git a/.abcd/work/issues/resolved/iss-2609281715083637-the-macos-check-spends-time-the-tests-do-not-need-in-two.md b/.abcd/work/issues/resolved/iss-2609281715083637-the-macos-check-spends-time-the-tests-do-not-need-in-two.md new file mode 100644 index 000000000..5e41b5b7a --- /dev/null +++ b/.abcd/work/issues/resolved/iss-2609281715083637-the-macos-check-spends-time-the-tests-do-not-need-in-two.md @@ -0,0 +1,22 @@ +--- +schema_version: 1 +id: "iss-2609281715083637" +slug: "the-macos-check-spends-time-the-tests-do-not-need-in-two" +severity: "minor" +category: "tech-debt" +source: "review-followup" +found_during: "autonomous run A resumed 2026-09-25: ruling AR" +origin: researcher-authored +production_mode: hand-written +found_at: "internal/fsutil/fsutil.go" +resolution: "Both levers built under ruling AR. 7bbf7527a routes every flush through fsutil.Flush, which skips File.Sync only in a test binary that set ABCD_TEST_SKIP_FLUSH=1 (the Makefile's test and preflight targets and ci.yml's test steps set it); a cli plain run made 2,889 in-process flushes costing 18.6s locally. 94d311fcd names the nine slowest race packages ahead of ./internal/..., the same 78 packages by go list; at -p 3 on a scratch snapshot the race lane took 845s and 811s in import-path order and 557s slowest-first." +impact: internal +resolved_by: + commit: "94d311fcd" +--- + +The macOS check spends time the tests do not need in two places. Every atomic write the tests make flushes to stable storage (fsutil's WriteFileAtomic family and the history-index bootstrap call File.Sync, which Go runs as F_FULLFSYNC on macOS, about 8.8 ms per synced write on this disk), though no test asserts anything a flush makes true; and the race step names ./internal/... in import-path order, so go test starts internal/surface/cli, the slowest package under -race, among the last on the three-core runner and the step waits on it as a tail. + +## Grounds + +- pursued: the macOS check job shortens by the flush time of every test write and by the race step's single-package tail; a macOS check on the merged tip no shorter than the last green run (23.7 min, race step 888s) would show it wrong diff --git a/.abcd/work/issues/resolved/iss-2609281931185016-ahoy-s-steprules-internal-core-ahoy-apply-go-steprules.md b/.abcd/work/issues/resolved/iss-2609281931185016-ahoy-s-steprules-internal-core-ahoy-apply-go-steprules.md new file mode 100644 index 000000000..4035bbe94 --- /dev/null +++ b/.abcd/work/issues/resolved/iss-2609281931185016-ahoy-s-steprules-internal-core-ahoy-apply-go-steprules.md @@ -0,0 +1,21 @@ +--- +schema_version: 1 +id: "iss-2609281931185016" +slug: "ahoy-s-steprules-internal-core-ahoy-apply-go-steprules" +severity: "minor" +category: "bug" +source: "review-followup" +found_during: "autonomous run A resumed 2026-09-25: review-lmw127" +origin: researcher-authored +production_mode: hand-written +resolution: "stepRules creates the rules.json skeleton through fsutil.CreateExclusiveIn (createRepoJSON); an existing file is kept and not reported as written" +impact: fix +resolved_by: + commit: "10b9a31cf" +--- + +ahoy's stepRules (internal/core/ahoy/apply.go stepRules) writes the empty .abcd/rules.json skeleton through writeRepoJSON (an atomic rename, no re-check) after the interactive prompt phase, so a rules.json written after detection set rules.missing (detect.go) is replaced by the empty-domains skeleton and the receipt says it wrote rules: a silent loss of a hand-written override, with the whole prompt phase as the window. + +## Grounds + +- pursued: a rules.json written after detection survives ahoy's apply byte for byte and the receipt carries no rules write; TestRulesSkeletonNeverReplacesARulesFileWrittenMeanwhile going red again, or a rules write reported for a kept file, would show it wrong diff --git a/.abcd/work/issues/resolved/iss-2609290518278152-fsutil-readdeclaration-refuses-a-home-scoped-declaration-as.md b/.abcd/work/issues/resolved/iss-2609290518278152-fsutil-readdeclaration-refuses-a-home-scoped-declaration-as.md new file mode 100644 index 000000000..59a714113 --- /dev/null +++ b/.abcd/work/issues/resolved/iss-2609290518278152-fsutil-readdeclaration-refuses-a-home-scoped-declaration-as.md @@ -0,0 +1,22 @@ +--- +schema_version: 1 +id: "iss-2609290518278152" +slug: "fsutil-readdeclaration-refuses-a-home-scoped-declaration-as" +severity: "minor" +category: "bug" +source: "review-followup" +found_during: "autonomous run 2026-09-23" +origin: researcher-authored +production_mode: hand-written +found_at: "internal/fsutil/fsutil.go" +resolution: "fsutil.ReadDeclaration and ReadGuardedInRoot re-vet a file renamed into place between the vetting lstat and the open, up to 8 times, instead of refusing it on sight: a same-owner owner-only-writable regular file (a concurrent WriteFileAtomic) is read, anything a guard refuses is refused by that guard, and a replacement that never settles is still refused as a swap." +impact: fix +resolved_by: + commit: "b342b2b29" +--- + +fsutil.ReadDeclaration refuses a home-scoped declaration as replaced between its vetting and its read when another abcd process of the same user atomically renames a new version into place inside the lstat-to-open window, so two abcd processes on one machine (two sessions, or a hook and a verb) can make one refuse its own ~/.abcd/config.json, rules.json or credentials index; the merge-group macOS leg of PR 744 failed TestConcurrentConnectsKeepEveryKeyAndBlock on it. + +## Grounds + +- pursued: two abcd processes on one machine no longer make one refuse its own ~/.abcd/config.json when the other rewrites it; TestReadDeclarationReadsABenignReplacementAfterRevetting or TestConcurrentConnectsKeepEveryKeyAndBlock failing with ErrDeclarationSwapped would show it wrong, and TestReadDeclarationRefusesANonBenignReplacement reading a symlink, FIFO, directory, group-writable or foreign-owned replacement would show the fix weakened the guard diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 88bd3cbcc..17e717214 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -236,9 +236,10 @@ jobs: # the remedy for a leg near the cap is a faster suite. The ubuntu leg runs # well inside 30 minutes and keeps that ceiling. The race step's -timeout lifts go test's 10m # per-package default, so a slow package runs on to this ceiling instead of - # failing at ten minutes; it does not fit inside the ceiling on the macOS - # leg, which spends about 18m before the slowest package starts, so a hang - # there is cancelled without a goroutine dump. + # failing at ten minutes. The step names its slowest packages first, so they + # start with the step, about 9m into the macOS leg, rather than behind every + # faster package; a hang in one of them fails at the package timeout, with a + # goroutine dump, before this ceiling cancels the job. # TestRaceLaneBudgetIsDeclaredAndFitsItsJob holds every leg at or below the # cap, and TestCheckJobWarnsBeforeTheQueueCap holds the warning step at the # end of the job below it. @@ -305,8 +306,14 @@ jobs: if: matrix.os == 'ubuntu-latest' || needs.changes.outputs.inert != 'true' run: go vet ./... + # ABCD_TEST_SKIP_FLUSH lets the test binaries skip the flush to stable + # storage (internal/fsutil/flush.go), an F_FULLFSYNC of about 8.8 ms per + # synced write on macOS that no test asserts anything about. It needs a + # test binary as well, so nothing built here stops flushing. - name: Test if: matrix.os == 'ubuntu-latest' || needs.changes.outputs.inert != 'true' + env: + ABCD_TEST_SKIP_FLUSH: "1" run: go test ./... # Only a source change can introduce a data race, and the plain lane above @@ -323,9 +330,18 @@ jobs: # header). The Makefile's preflight recipe and release.yml's verify # job carry the same flag; TestRaceLaneBudgetIsDeclaredAndFitsItsJob # holds all three. + # + # The packages named ahead of ./internal/... are the slowest under -race + # (iss-2609281715083637). go test starts packages in the + # order its arguments list them, three at a time on the macOS runner, and + # in import-path order internal/surface/cli started among the last and ran + # alone as the step's tail. go test runs a package named twice once, so the + # set is still exactly ./internal/...; the same test pins that with go list. - name: Test (race, internal) if: needs.changes.outputs.inert != 'true' - run: go test -race -timeout 20m ./internal/... + env: + ABCD_TEST_SKIP_FLUSH: "1" + run: go test -race -timeout 20m ./internal/surface/cli ./internal/core/reading ./internal/core/launch ./internal/core/lifeboat ./internal/core/lint ./internal/adapter/scanner ./internal/core/capture ./internal/core/ahoy ./internal/core/site ./internal/... # Drift gate for the .abcd/development design record. Blocking: a # blocker-severity finding exits non-zero and fails the job; warn-level diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 6869bd7fd..20b8b737a 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -139,7 +139,7 @@ jobs: run: go test ./... - name: Test (race, internal) - run: go test -race -timeout 20m ./internal/... + run: go test -race -timeout 20m ./internal/surface/cli ./internal/core/reading ./internal/core/launch ./internal/core/lifeboat ./internal/core/lint ./internal/adapter/scanner ./internal/core/capture ./internal/core/ahoy ./internal/core/site ./internal/... - name: Record-lint (design-record drift gate) run: go run ./cmd/record-lint diff --git a/Makefile b/Makefile index 42e0bcd63..89f321782 100644 --- a/Makefile +++ b/Makefile @@ -307,7 +307,7 @@ preflight: load-check fmt-check lint-reviews lint-issues lint-decisions record-l go build ./... go vet ./... go test ./... - go test -race -timeout 20m ./internal/... + go test -race -timeout 20m ./internal/surface/cli ./internal/core/reading ./internal/core/launch ./internal/core/lifeboat ./internal/core/lint ./internal/adapter/scanner ./internal/core/capture ./internal/core/ahoy ./internal/core/site ./internal/... @scripts/preflight-receipt.sh mint "$(PREFLIGHT_BEGAN)" # The push receipt (iss-2608290810036869, iss-2608210738378295). The pre-push hook @@ -345,6 +345,13 @@ preflight: export ABCD_LOAD_CHECKED := preflight # the release cannot be fetched, before any gate runs on it. preflight: export GOTOOLCHAIN := go$(GO_TOOLCHAIN_VERSION) +# The test binaries these targets run skip the flush to stable storage +# (internal/fsutil/flush.go): on macOS every synced write is an F_FULLFSYNC of +# about 8.8 ms, and no test asserts what a flush makes true. The skip needs a +# test binary as well as this opt-in, so a binary built or run here, the lint +# gates' `go run` tools included, still flushes. CI's test steps set the same. +test preflight: export ABCD_TEST_SKIP_FLUSH := 1 + # The load check (itd-2609231434459890): reads the machine's load and process # table once and warns about programs left running and extreme load. It exits 0 # on every status, and the leading `-` ignores even a failure to build it: a diff --git a/commands/implement.md b/commands/implement.md index 06aaf58b7..a665dba69 100644 --- a/commands/implement.md +++ b/commands/implement.md @@ -91,8 +91,11 @@ session stops: **Exit 3 means back off.** A record another session holds is refused at exit 3, naming the holder, and logged as `claim_denied` (the second session also logs a -`backoff`). Take other work; do not retry the same record in a loop. A locked -run state is the same exit. +`backoff` naming the reason and the minutes the attempt spent). Take other work; +do not retry the same record in a loop. A locked run state is the same exit, +and the second session's backoff from it is logged the same way, with the +minutes it waited for the lock, a `join` that meets it included. A backoff that +cannot be logged (the session never joined) says so in the refusal. ## The second session's bounds @@ -142,6 +145,7 @@ field, with nothing written: | Event | Required fields | Checked when given | |---|---|---| +| `backoff` | `reason`, `minutes` (a number) | | | `lane_close` | `lane`, `outcome` | | | `agent_start` | `agent` | | | `agent_end` | `agent`, `role`, `model`, and `minutes` (or `wall_minutes`, `wall_min`), a number | | @@ -157,8 +161,10 @@ An intervention's `kind` is one of `session_open`, `account`, `ruling`, alternative not taken. For the comparison to count them: a `lane_close` with `outcome=merged` (or -`landed`) is a lane landed; `backoff` and `ceiling_wait` carry `minutes`; a -`context` line carries `used_pct` (with `role` and `note`), the orchestrator's +`landed`) is a lane landed; `backoff` and `ceiling_wait` carry `minutes` (a +`backoff` must also carry `reason`, or it is refused: contention the verb cannot +see, such as the merge queue, is logged this way with the minutes the +backed-off work cost); a `context` line carries `used_pct` (with `role` and `note`), the orchestrator's share of its context window in use. ## Compare the modes diff --git a/commands/inbox.md b/commands/inbox.md index cfdb34fbd..c214a4883 100644 --- a/commands/inbox.md +++ b/commands/inbox.md @@ -37,7 +37,8 @@ Present the `notice`, the `tally` and each report newest first: its `id`, `recei sender repository (`sender_name`), `kind`, `severity` and `title`. A report in state `unreadable` was written to a template version this abcd does not know, or is not a report at all; relay its `unreadable` reason, which names the -version. It is kept, never dropped. +version, and its `sender_name` when the envelope still names the sender. It is +kept, never dropped. ## Read one diff --git a/commands/launch.md b/commands/launch.md index 841d94278..f0eb7ea72 100644 --- a/commands/launch.md +++ b/commands/launch.md @@ -1,7 +1,7 @@ --- name: launch description: "Preview the public launch bundle, its secret scan, and the release gates: Writes only its pre-flight report, to the local tier; refuses without --dry-run." -argument-hint: "[--dry-run [--deep-smoke] [--baseline ] [--fetch-baseline]] | ship [--changelog-json ] [--payload-dir ] [--allow-dirty] [--fetch-baseline] | archive --out [--tag ] [--verify] [--repository ] | scaffold" +argument-hint: "[--dry-run [--deep-smoke] [--baseline ] [--fetch-baseline]] | ship [--changelog-json ] [--payload-dir ] [--allow-dirty] [--fetch-baseline] | archive --out [--tag ] [--verify] [--repository ] | manifests --tree public|dev [--root ] | scaffold" block: people --- @@ -835,6 +835,30 @@ pins the last release's archive, which its moved-on tree no longer reproduces, s `--verify` there is expected to refuse: it proves a release commit, not a branch tip. +## Manifests — the manifest lockstep check + +`manifests` runs the manifest lockstep check over a tree, at the polarity the +caller names: the check the preview runs at `dev` over the source tree and the +cut's render runs at `public` over the staged payload. It reads and never +writes, and it has no flag that waives a finding. + +```bash +"${CLAUDE_PLUGIN_ROOT}/abcd" launch manifests --tree public|dev [--root ] --json +``` + +- `--tree public` requires the version-location primary present as strict + SemVer and every pinned secondary in `.claude-plugin/marketplace.json` to + agree with it (adr-20). Use it on a public tree: a marketplace install or a + release source archive. +- `--tree dev` requires every version key absent (adr-19), as a development + checkout carries them. +- `--root` names the tree; the default is the working directory. The tree's own + `.abcd/config/version-location.json` and artefact declaration are read. + +Exit codes: **0** consistent; **1** drift, with one `drifts` line per field; +**2** an input that cannot be read (`unreadable` with its `detail`), or a +`--tree` that is neither polarity. Relay `tree`, `ok` and each `drifts` line. + ## Scaffold — the release-gate scaffolder `scaffold` writes the changelog-driven release machinery into a **managed repo that diff --git a/docs/reference/cli/commands.md b/docs/reference/cli/commands.md index 6441bf15d..97abf548a 100644 --- a/docs/reference/cli/commands.md +++ b/docs/reference/cli/commands.md @@ -1312,7 +1312,9 @@ claim is a lease (--lease, default 2h, 1m to 24h). Claiming a record this sessio already holds renews the lease. A claim whose lease has passed is claimable again, and the lapse is logged as claim_lapsed. A record another session holds is refused at exit 3 and logged as claim_denied naming the holder; the second session also -logs a backoff. +logs a backoff with its reason and the minutes the attempt spent. A run state +locked by another session's change is exit 3 too, and the second session's +backoff from it is logged the same way. The second session is refused (exit 2, logged as a refusal) when it already holds a live claim, when the window is split-roles, or when a --path it declares is in the @@ -1454,7 +1456,8 @@ The claim, window and session events are written by their own sub-verbs and are refused here, so the log cannot record a claim the run state does not hold. An event missing a field the report reads is refused, naming it: lane_close (lane, outcome); agent_start (agent); agent_end (agent, role, model, minutes|wall_minutes|wall_min); stop (cause); ceiling_overrun (alive, ceiling, lane, minutes); intervention (kind, by, what, why, autonomy_gap); decision (what, alternative, why). -An intervention's kind is one of session_open, account, ruling, restart, close_session, file_restore, permission, other; an at or +A backoff names its reason and the minutes it spent (reason=, minutes=), +or it is refused. An intervention's kind is one of session_open, account, ruling, restart, close_session, file_restore, permission, other; an at or last_productive is an RFC 3339 time, and a *_min or minutes field a number. An agent_start that would take a session past the ceiling it joined with is refused, and the refusal logged. @@ -2082,6 +2085,31 @@ is written to --out. abcd launch archive --out dist ``` +#### `abcd launch manifests` + +Check the release manifests agree on the version, or carry none on a dev tree: Writes nothing; refuses with exit 1 on drift and exit 2 on an unreadable input. + +**Usage:** `abcd launch manifests --tree public|dev [--root ] [flags]` + +Run the manifest lockstep check over a tree. --tree public requires the +version-location primary present as strict SemVer and every pinned secondary +to agree with it; --tree dev requires every version key absent (adr-19). The +tree is the working directory, or --root. Exit 0 consistent, 1 drift (one +line per field), 2 unreadable. Nothing is written. + +**Flags:** + +``` + --root string the tree to check (default: the working directory) + --tree string the polarity to check: public (versions present and agreeing) or dev (versions absent) +``` + +**Example:** + +``` +abcd launch manifests --tree public +``` + #### `abcd launch receipts` Run the release job's semantic-receipt gate locally, before the merge: Writes nothing; refuses with exit 1 when the release job would refuse the receipts. diff --git a/internal/adapter/scanner/identity.go b/internal/adapter/scanner/identity.go index be30d9952..61db63df1 100644 --- a/internal/adapter/scanner/identity.go +++ b/internal/adapter/scanner/identity.go @@ -70,38 +70,47 @@ func DefaultIdentitySeverities() map[string]Severity { // ADDITION to the caller's other identities, not instead of them: the name and // email fields hold the value git resolves in this repository, and the Other* // fields hold every value another scope configured that it displaced. +// +// Every key is read in ONE git process (iss-2609281546130900): a scanner is +// built by every verb and hook that scans, and four processes per build were +// four reads of the same configuration. A second process runs only when the +// parent carries `git -c` configuration, for the reason given below. func ProbeIdentity(repoRoot string) Identity { var id Identity - gitIn := func(env []string, args ...string) string { - full := append([]string{"-C", repoRoot}, args...) - cmd := exec.Command("git", full...) - cmd.Env = env - out, err := cmd.Output() - if err != nil { - return "" - } - return strings.TrimSpace(string(out)) - } - git := func(args ...string) string { - // Scrub repo-selection and config-injection env vars, but keep global - // config: this probe reads the caller's OWN user.name/user.email to redact - // their identity, and those live in global config, so full IsolatedEnv - // (which neutralises ~/.gitconfig) would blind the identity gate. Scrubbing - // still stops an inherited GIT_DIR pointing the probe at another repo and an - // injected GIT_CONFIG_* forging a fake identity that displaces the real one. - return gitIn(gitutil.ScrubbedEnv(), args...) - } - // --get-all lists every value git resolves for the key, in scope order - // with the effective one last — system, global with its includeIf - // includes evaluated where they sit, repo-local, worktree. --get returned - // only that last value, so a repo-local or includeIf persona displaced the - // caller's global identity from the matcher set and the displaced identity + // Scrub repo-selection and config-injection env vars, but keep global + // config: this probe reads the caller's OWN user.name/user.email to redact + // their identity, and those live in global config, so full IsolatedEnv + // (which neutralises ~/.gitconfig) would blind the identity gate. Scrubbing + // still stops an inherited GIT_DIR pointing the probe at another repo and an + // injected GIT_CONFIG_* forging a fake identity that displaces the real one. + // + // The listing is unscoped and lists every value git resolves for each key, + // in scope order with the effective one last — system, global with its + // includeIf includes evaluated where they sit, repo-local, worktree. Reading + // only the last value let a repo-local or includeIf persona displace the + // caller's global identity from the matcher set, and the displaced identity // was stored in clear text. Neither --local nor --global sees an includeIf // persona for what it is (the former misses it, the latter hides it behind - // the unconditional value), which is why the union comes from ONE - // unscoped listing rather than a scope-by-scope reassembly. - id.GitUserName, id.OtherGitUserNames = splitIdentityValues(git("config", "--get-all", "user.name")) - id.GitUserEmail, id.OtherGitUserEmails = splitIdentityValues(git("config", "--get-all", "user.email")) + // the unconditional value), which is why the union comes from ONE unscoped + // listing rather than a scope-by-scope reassembly. + entries := configListing(repoRoot, gitutil.ScrubbedEnv(), identityKeys) + var names, emails []string + var remote string + for _, e := range entries { + switch e.key { + case "user.name": + names = append(names, e.value) + case "user.email": + emails = append(emails, e.value) + case "remote.origin.url": + remote = e.value // the last one listed is the one git resolves + } + } + // Joined on newlines, as a `git config --get-all` listing prints them, and + // split the same way: a value holding an escaped newline contributes each + // line as a user.* value, and stays whole among the persona values below. + id.GitUserName, id.OtherGitUserNames = splitIdentityValues(strings.Join(names, "\n")) + id.GitUserEmail, id.OtherGitUserEmails = splitIdentityValues(strings.Join(emails, "\n")) // GIT_AUTHOR_* and GIT_COMMITTER_* are an identity scope `git config` never // reports and that outranks every config file when a commit is written: a CI // runner, a direnv profile and a rebase wrapper all set them. The persona @@ -118,22 +127,31 @@ func ProbeIdentity(repoRoot string) Identity { // git also stamps a commit from author.*/committer.*, which it ranks above // user.*, and from a `git -c` persona (GIT_CONFIG_PARAMETERS or the // GIT_CONFIG_COUNT form), which outranks every file and reaches a hook - // running this probe. They are read in ONE extra listing, under the - // scrubbed env plus only those command-line entries, and folded in as - // OTHERS like the environment persona above: the effective identity still - // comes from the scrubbed read, so an injected value can only add something - // to redact (iss-2609261614450166). - cmdline := append(gitutil.ScrubbedEnv(), gitutil.CommandLineConfig()...) - // -z: a value git holds with an embedded newline stays one value. - for _, entry := range strings.Split(gitIn(cmdline, "config", "-z", "--get-regexp", `^(user|author|committer)\.(name|email)$`), "\x00") { - key, value, _ := strings.Cut(entry, "\n") - if strings.HasSuffix(key, ".email") { - id.OtherGitUserEmails = addIdentityValues(id.GitUserEmail, id.OtherGitUserEmails, value) - } else { - id.OtherGitUserNames = addIdentityValues(id.GitUserName, id.OtherGitUserNames, value) + // running this probe. They are folded in as OTHERS like the environment + // persona above: the effective identity still comes from the scrubbed read, + // so an injected value can only add something to redact + // (iss-2609261614450166). + // + // With no -c configuration in the parent, the persona environment IS the + // scrubbed one, so the listing above already holds every persona value. With + // some, the persona listing runs apart, under the scrubbed env plus only + // those command-line entries: a -c entry can change what git reads, not + // only add values (safe.bareRepository or safe.directory move discovery, and + // a malformed entry fails the whole command), so folding it into the one + // listing would let it move or blind the effective identity and the remote. + persona := entries + if cmdline := gitutil.CommandLineConfig(); len(cmdline) > 0 { + persona = configListing(repoRoot, append(gitutil.ScrubbedEnv(), cmdline...), personaKeys) + } + for _, e := range persona { + switch e.key { + case "user.email", "author.email", "committer.email": + id.OtherGitUserEmails = addIdentityValues(id.GitUserEmail, id.OtherGitUserEmails, e.value) + case "user.name", "author.name", "committer.name": + id.OtherGitUserNames = addIdentityValues(id.GitUserName, id.OtherGitUserNames, e.value) } } - if remote := git("config", "--get", "remote.origin.url"); remote != "" { + if remote = strings.TrimSpace(remote); remote != "" { id.GitRemoteUsername, id.GitRemoteRepo = parseGitHubRemote(remote) } if home := CallerHome(); home != "" { @@ -145,6 +163,55 @@ func ProbeIdentity(repoRoot string) Identity { return id } +// identityKeys selects every key the probe reads, in one listing: user.* is +// the identity git resolves, author.*/committer.* the persona it commits +// with, and remote.origin.url the remote the public handle is read from. +// personaKeys is the persona subset, for the listing a -c entry runs under. +// git lists canonical keys (section and variable lower-cased, a subsection +// verbatim), so `[User] Name` matches and `[remote "Origin"]` does not, exactly +// as `git config --get-all user.name` and `--get remote.origin.url` resolve. +const ( + identityKeys = `^((user|author|committer)\.(name|email)|remote\.origin\.url)$` + personaKeys = `^(user|author|committer)\.(name|email)$` +) + +// configEntry is one key/value pair of a git config listing. +type configEntry struct{ key, value string } + +// configListing lists every value of every key matching pattern that git +// resolves for repoRoot under env, in the order git lists them. Any failure — +// git absent, a malformed or unreadable repository config, a malformed -c +// entry, no key matching — is no entries: the probe is best-effort, and a +// failed read leaves its fields empty. Output from a command that exits +// non-zero is discarded whole, never half-read. +func configListing(repoRoot string, env []string, pattern string) []configEntry { + cmd := exec.Command("git", "-C", repoRoot, "config", "-z", "--get-regexp", pattern) + cmd.Env = env + out, err := cmd.Output() + if err != nil { + return nil + } + return parseConfigListing(string(out)) +} + +// parseConfigListing reads a `git config -z` listing. Every entry ends in a +// NUL, which no key or value can hold. The key runs to the entry's FIRST +// newline and the value is every byte after it, verbatim: a key never holds a +// newline, a value may (an escaped \n in the file), and '=' is never a +// delimiter in this form. A key listed with no value at all (`[user] name` +// with no '=') has no newline and reads as the empty value. +func parseConfigListing(out string) []configEntry { + var entries []configEntry + for _, raw := range strings.Split(out, "\x00") { + if raw == "" { + continue + } + key, value, _ := strings.Cut(raw, "\n") + entries = append(entries, configEntry{key: key, value: value}) + } + return entries +} + // splitIdentityValues turns a `git config --get-all` listing into the // effective (last) value and the distinct others it displaced — trimmed, // empties dropped, and de-duplicated case-insensitively, the way every diff --git a/internal/adapter/scanner/identity_exec_count_test.go b/internal/adapter/scanner/identity_exec_count_test.go new file mode 100644 index 000000000..100b57fa6 --- /dev/null +++ b/internal/adapter/scanner/identity_exec_count_test.go @@ -0,0 +1,125 @@ +package scanner + +import ( + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// identity_exec_count_test.go (iss-2609281546130900): building a scanner +// probes the caller's identity, and every CLI verb and hook that scans builds +// one. The probe spent four git processes on reads one listing answers: the +// user.name values, the user.email values, the author/committer/-c persona +// listing and remote.origin.url. It spends one; a second only when the parent +// carries `git -c` configuration, whose persona listing must run under an +// environment the effective identity is never read under. + +// countingGit puts a git on PATH that appends one line to a log per +// invocation and hands the call to the real binary, and returns a function +// reporting how many invocations the log holds. +func countingGit(t *testing.T) func() int { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("the counting shim is a POSIX shell script") + } + real, err := exec.LookPath("git") + if err != nil { + t.Skip("git not available") + } + dir := t.TempDir() + log := filepath.Join(dir, "calls.log") + shim := "#!/bin/sh\nprintf 'call\\n' >> '" + log + "'\nexec '" + real + "' \"$@\"\n" + if err := os.WriteFile(filepath.Join(dir, "git"), []byte(shim), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + return func() int { + b, err := os.ReadFile(log) + if os.IsNotExist(err) { + return 0 + } + if err != nil { + t.Fatal(err) + } + return strings.Count(string(b), "\n") + } +} + +func TestScannerNewSpendsOneGitExecOnIdentity(t *testing.T) { + cases := []struct { + name string + env map[string]string + want int + }{ + {"no -c configuration in the parent", nil, 1}, + {"a -c persona in the parent", map[string]string{ + "GIT_CONFIG_PARAMETERS": `'user.name'='Param Name'`}, 2}, + {"a counted -c persona in the parent", map[string]string{ + "GIT_CONFIG_COUNT": "1", "GIT_CONFIG_KEY_0": "author.email", "GIT_CONFIG_VALUE_0": "param@example.com"}, 2}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := newPinCtx(t, false) + appendFile(t, c.global, "[user]\n\tname = Real Name\n\temail = real@example.com\n[author]\n\tname = Author Key\n") + appendFile(t, c.localConfig(), "[remote \"origin\"]\n\turl = https://github.com/octo/proj.git\n") + for k, v := range tc.env { + t.Setenv(k, v) + } + calls := countingGit(t) + sc, err := New(c.repo) + if err != nil { + t.Fatal(err) + } + if got := calls(); got != tc.want { + t.Errorf("scanner.New spent %d git exec(s) on identity, want %d", got, tc.want) + } + // The count is only worth anything if the one exec still answered. + id := answerOf(sc.identity) + if id.Name != "Real Name" || id.Email != "real@example.com" || id.RemoteUser != "octo" || !containsFold(id.OtherNames, "Author Key") { + t.Errorf("the probe under the shim answered %s", fmtAnswer(id)) + } + }) + } +} + +// TestParseConfigListing pins the -z listing parser on the shapes a hostile +// or unusual configuration produces: the value is every byte after the key's +// first newline, '=' is never a delimiter, a valueless key reads as empty. +func TestParseConfigListing(t *testing.T) { + big := strings.Repeat("x", 1<<20) + cases := []struct { + name string + in string + want []configEntry + }{ + {"empty", "", nil}, + {"only terminators", "\x00\x00", nil}, + {"plain", "user.name\nA Name\x00user.email\na@example.com\x00", + []configEntry{{"user.name", "A Name"}, {"user.email", "a@example.com"}}}, + {"embedded newlines and equals", "user.name\nA=B\nC=D\n\x00", + []configEntry{{"user.name", "A=B\nC=D\n"}}}, + {"valueless key", "user.name\x00remote.origin.url\n\x00", + []configEntry{{"user.name", ""}, {"remote.origin.url", ""}}}, + {"control bytes kept verbatim", "user.name\n\x01\t\r\x7f\x1b[31m\x00", + []configEntry{{"user.name", "\x01\t\r\x7f\x1b[31m"}}}, + {"an unterminated trailing fragment is still an entry", "user.name\nA\x00user.email\nb@example.com", + []configEntry{{"user.name", "A"}, {"user.email", "b@example.com"}}}, + {"a huge value", "user.name\n" + big + "\x00", []configEntry{{"user.name", big}}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := parseConfigListing(tc.in) + if len(got) != len(tc.want) { + t.Fatalf("got %d entries, want %d: %q", len(got), len(tc.want), got) + } + for i := range got { + if got[i] != tc.want[i] { + t.Errorf("entry %d = %q, want %q", i, got[i], tc.want[i]) + } + } + }) + } +} diff --git a/internal/adapter/scanner/identity_probe_pin_test.go b/internal/adapter/scanner/identity_probe_pin_test.go new file mode 100644 index 000000000..745799b7d --- /dev/null +++ b/internal/adapter/scanner/identity_probe_pin_test.go @@ -0,0 +1,405 @@ +package scanner + +import ( + "os" + "os/exec" + "path/filepath" + "reflect" + "strings" + "testing" + + "github.com/intentdriven/abcd/internal/gittest" +) + +// identity_probe_pin_test.go — the semantics ProbeIdentity's git reads have, +// pinned shape by shape so a change to HOW the probe asks git (how many +// processes, which flags) is held to WHAT it answers (iss-2609281546130900). +// Every case builds real configuration files and runs the real git binary; +// the expectations are the answers the four-process probe gave, including the +// awkward ones (a value holding an escaped newline is split by the user.* +// read and kept whole by the persona read). + +// probeAnswer is the git-derived part of an Identity. HomePath/HomeUser come +// from $HOME, not from git, and are pinned elsewhere. +type probeAnswer struct { + Name, Email string + OtherNames, OtherEmails []string + RemoteUser, RemoteRepo string +} + +func answerOf(id Identity) probeAnswer { + return probeAnswer{ + Name: id.GitUserName, Email: id.GitUserEmail, + OtherNames: id.OtherGitUserNames, OtherEmails: id.OtherGitUserEmails, + RemoteUser: id.GitRemoteUsername, RemoteRepo: id.GitRemoteRepo, + } +} + +// pinCtx is one case's hermetic world: a HOME, a global and a system config +// file the probe's scrubbed environment honours, and a repository. +type pinCtx struct { + home, global, system, work, repo string +} + +// unsetenv removes k for the rest of the test and restores it afterwards. +// t.Setenv(k, "") would leave k present, and a present-but-empty +// GIT_CONFIG_PARAMETERS is still a command-line config entry. +func unsetenv(t *testing.T, k string) { + t.Helper() + t.Setenv(k, "") + if err := os.Unsetenv(k); err != nil { + t.Fatal(err) + } +} + +// clearIdentityEnv removes every variable the probe reads from the process +// environment, directly or through the scrubbed subprocess environment, so a +// case starts from nothing and adds only what it names. +func clearIdentityEnv(t *testing.T) { + t.Helper() + for _, kv := range os.Environ() { + k, _, _ := strings.Cut(kv, "=") + if strings.HasPrefix(k, "GIT_") { + unsetenv(t, k) + } + } +} + +func appendFile(t *testing.T, path, text string) { + t.Helper() + f, err := os.OpenFile(path, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0o600) + if err != nil { + t.Fatal(err) + } + if _, err := f.WriteString(text); err != nil { + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } +} + +// newPinCtx builds the world. bare makes the repository a bare one, so the +// config it carries is read through discovery rules a `-c` entry can change. +func newPinCtx(t *testing.T, bare bool) *pinCtx { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + clearIdentityEnv(t) + home, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + t.Setenv("HOME", home) + c := &pinCtx{ + home: home, + global: filepath.Join(home, "global.gitconfig"), + system: filepath.Join(home, "system.gitconfig"), + work: filepath.Join(home, "work"), + } + c.repo = filepath.Join(c.work, "repo") + for _, p := range []string{c.global, c.system} { + appendFile(t, p, "") + } + if err := os.MkdirAll(c.repo, 0o755); err != nil { + t.Fatal(err) + } + args := []string{"init", "-q"} + if bare { + args = append(args, "--bare") + } + c.git(t, append(args, c.repo)...) + t.Setenv("GIT_CONFIG_GLOBAL", c.global) + t.Setenv("GIT_CONFIG_SYSTEM", c.system) + t.Setenv("GIT_CONFIG_NOSYSTEM", "0") + return c +} + +// git runs a setup command under the hermetic test environment (global and +// system config neutralised), so setup never reads what the case configures. +func (c *pinCtx) git(t *testing.T, args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Env = gittest.Env(t) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } +} + +func (c *pinCtx) localConfig() string { + if _, err := os.Stat(filepath.Join(c.repo, ".git")); err == nil { + return filepath.Join(c.repo, ".git", "config") + } + return filepath.Join(c.repo, "config") // bare +} + +type pinCase struct { + name string + bare bool + system, global, local string // raw config text appended to each file + setup func(t *testing.T, c *pinCtx) + env map[string]string + root func(c *pinCtx) string // the repoRoot handed to the probe; default c.repo + want probeAnswer + skipAsRoot, skipAbsent bool +} + +func identityPinCases() []pinCase { + big := strings.Repeat("v", 70000) + " Name" + return []pinCase{ + {name: "no configuration at all", want: probeAnswer{}}, + {name: "user.name only", global: "[user]\n\tname = Solo Name\n", + want: probeAnswer{Name: "Solo Name"}}, + {name: "user.email only", global: "[user]\n\temail = solo@example.com\n", + want: probeAnswer{Email: "solo@example.com"}}, + {name: "multi-valued keys in one file, the last effective", + global: "[user]\n\tname = First Name\n\tname = Second Name\n\temail = one@example.com\n\temail = two@example.com\n", + want: probeAnswer{Name: "Second Name", Email: "two@example.com", + OtherNames: []string{"First Name"}, OtherEmails: []string{"one@example.com"}}}, + {name: "every file scope, the worktree effective", + system: "[user]\n\tname = System Name\n\temail = system@example.com\n", + global: "[user]\n\tname = Global Name\n\temail = global@example.com\n", + local: "[user]\n\tname = Local Name\n\temail = local@example.com\n", + setup: func(t *testing.T, c *pinCtx) { + c.git(t, "-C", c.repo, "config", "extensions.worktreeConfig", "true") + c.git(t, "-C", c.repo, "config", "--worktree", "user.name", "Worktree Name") + c.git(t, "-C", c.repo, "config", "--worktree", "user.email", "worktree@example.com") + }, + want: probeAnswer{Name: "Worktree Name", Email: "worktree@example.com", + OtherNames: []string{"System Name", "Global Name", "Local Name"}, + OtherEmails: []string{"system@example.com", "global@example.com", "local@example.com"}}}, + {name: "an includeIf persona evaluated where it sits", + global: "[user]\n\tname = Global Name\n\temail = global@example.com\n", + setup: func(t *testing.T, c *pinCtx) { + inc := filepath.Join(c.home, "persona.inc") + appendFile(t, inc, "[user]\n\tname = Include Persona\n\temail = persona@example.com\n") + appendFile(t, c.global, "[includeIf \"gitdir:"+c.work+"/\"]\n\tpath = "+inc+"\n[user]\n\tname = After Include\n") + }, + want: probeAnswer{Name: "After Include", Email: "persona@example.com", + OtherNames: []string{"Global Name", "Include Persona"}, OtherEmails: []string{"global@example.com"}}}, + {name: "an includeIf that does not match the repository", + global: "[user]\n\tname = Global Name\n", + setup: func(t *testing.T, c *pinCtx) { + inc := filepath.Join(c.home, "elsewhere.inc") + appendFile(t, inc, "[user]\n\tname = Elsewhere Persona\n") + appendFile(t, c.global, "[includeIf \"gitdir:"+c.home+"/elsewhere/\"]\n\tpath = "+inc+"\n") + }, + want: probeAnswer{Name: "Global Name"}}, + {name: "outside a repository only system and global are read", + global: "[user]\n\tname = Global Name\n", + root: func(c *pinCtx) string { return c.home }, + want: probeAnswer{Name: "Global Name"}}, + {name: "a repository root that does not exist", + global: "[user]\n\tname = Global Name\n", + root: func(c *pinCtx) string { return filepath.Join(c.home, "absent") }, + want: probeAnswer{}}, + {name: "case-variant duplicates collapse", + global: "[user]\n\tname = Alice Example\n\temail = Alice@Example.com\n", + local: "[user]\n\tname = ALICE EXAMPLE\n\temail = alice@example.com\n", + want: probeAnswer{Name: "ALICE EXAMPLE", Email: "alice@example.com"}}, + {name: "a section spelled in another case is the same key", + global: "[User]\n\tName = Upper Section\n", + want: probeAnswer{Name: "Upper Section"}}, + {name: "a valueless key and a blank value are dropped", + global: "[user]\n\tname = Real Name\n\tname\n\temail = \" \"\n", + want: probeAnswer{Name: "Real Name"}}, + {name: "quoted padding is trimmed", + global: "[user]\n\tname = \" Padded Name \"\n", + want: probeAnswer{Name: "Padded Name"}}, + {name: "an escaped newline splits the user read and stays whole in the persona read", + global: "[user]\n\tname = \"Line One\\nLine Two\"\n", + want: probeAnswer{Name: "Line Two", + OtherNames: []string{"Line One", "Line One\nLine Two"}}}, + {name: "control bytes and an equals sign survive verbatim", + global: "[user]\n\tname = \"Tab\\tName\"\n\tname = Ctl\x01Name\n\tname = \"Back\\bspace\"\n\temail = \"key=value@example.com\"\n", + want: probeAnswer{Name: "Back\bspace", Email: "key=value@example.com", + OtherNames: []string{"Tab\tName", "Ctl\x01Name"}}}, + {name: "a very large value", + global: "[user]\n\tname = " + big + "\n", + want: probeAnswer{Name: big}}, + {name: "author and committer keys join the others", + global: "[user]\n\tname = User Name\n\temail = user@example.com\n[author]\n\tname = Author Key\n\temail = author@example.com\n[committer]\n\tname = User Name\n\temail = committer@example.com\n", + want: probeAnswer{Name: "User Name", Email: "user@example.com", + OtherNames: []string{"Author Key"}, OtherEmails: []string{"author@example.com", "committer@example.com"}}}, + {name: "author keys with no user identity", + global: "[author]\n\tname = Author Only\n\temail = author@example.com\n", + want: probeAnswer{OtherNames: []string{"Author Only"}, OtherEmails: []string{"author@example.com"}}}, + {name: "keys that only resemble identity keys are ignored", + global: "[user \"sub\"]\n\tname = Sub Name\n[user]\n\tnames = Plural\n\tusername = Handle\n[github]\n\tuser = gh\n", + want: probeAnswer{}}, + {name: "the environment persona joins the others", + global: "[user]\n\tname = Config Name\n\temail = config@example.com\n", + env: map[string]string{"GIT_AUTHOR_NAME": "Env Author", "GIT_AUTHOR_EMAIL": "env@example.com", + "GIT_COMMITTER_NAME": "config name", "GIT_COMMITTER_EMAIL": " "}, + want: probeAnswer{Name: "Config Name", Email: "config@example.com", + OtherNames: []string{"Env Author"}, OtherEmails: []string{"env@example.com"}}}, + {name: "an injected counted -c identity is an other, never the effective", + global: "[user]\n\tname = Real Name\n\temail = real@example.com\n", + env: map[string]string{"GIT_CONFIG_COUNT": "2", + "GIT_CONFIG_KEY_0": "user.email", "GIT_CONFIG_VALUE_0": "injected@example.com", + "GIT_CONFIG_KEY_1": "remote.origin.url", "GIT_CONFIG_VALUE_1": "https://github.com/evil/repo"}, + want: probeAnswer{Name: "Real Name", Email: "real@example.com", + OtherEmails: []string{"injected@example.com"}}}, + {name: "an injected GIT_CONFIG_PARAMETERS identity is an other, after the environment persona", + global: "[user]\n\tname = Real Name\n", + env: map[string]string{"GIT_CONFIG_PARAMETERS": `'user.name'='Param Name' 'committer.email'='param@example.com'`, + "GIT_AUTHOR_NAME": "Env Author"}, + want: probeAnswer{Name: "Real Name", + OtherNames: []string{"Env Author", "Param Name"}, OtherEmails: []string{"param@example.com"}}}, + {name: "a malformed -c entry blinds the persona read and nothing else", + global: "[user]\n\tname = Real Name\n[author]\n\tname = Author Key\n[remote \"origin\"]\n\turl = git@github.com:octo/proj.git\n", + env: map[string]string{"GIT_CONFIG_PARAMETERS": `'bogus`, "GIT_AUTHOR_EMAIL": "env@example.com"}, + want: probeAnswer{Name: "Real Name", OtherEmails: []string{"env@example.com"}, + RemoteUser: "octo", RemoteRepo: "proj"}}, + {name: "a malformed counted -c entry blinds the persona read and nothing else", + global: "[user]\n\temail = real@example.com\n[committer]\n\temail = committer@example.com\n", + env: map[string]string{"GIT_CONFIG_COUNT": "2", "GIT_CONFIG_KEY_0": "user.name", "GIT_CONFIG_VALUE_0": "Param Name"}, + want: probeAnswer{Email: "real@example.com"}}, + {name: "a -c entry that changes discovery changes only the persona read", + bare: true, + global: "[user]\n\tname = Global Name\n", + local: "[user]\n\tname = Bare Local\n[author]\n\tname = Bare Author\n[remote \"origin\"]\n\turl = https://github.com/octo/bare.git\n", + env: map[string]string{"GIT_CONFIG_PARAMETERS": `'safe.bareRepository'='explicit'`}, + want: probeAnswer{Name: "Bare Local", OtherNames: []string{"Global Name"}, + RemoteUser: "octo", RemoteRepo: "bare"}}, + {name: "the legacy GIT_CONFIG file is never read", + global: "[user]\n\tname = Real Name\n", + setup: func(t *testing.T, c *pinCtx) { + alt := filepath.Join(c.home, "alt.gitconfig") + appendFile(t, alt, "[user]\n\tname = Alt Name\n\temail = alt@example.com\n") + t.Setenv("GIT_CONFIG", alt) + }, + want: probeAnswer{Name: "Real Name"}}, + {name: "an inherited GIT_DIR cannot redirect the probe", + local: "[user]\n\tname = This Repo\n", + setup: func(t *testing.T, c *pinCtx) { + other := filepath.Join(c.home, "other") + c.git(t, "init", "-q", other) + appendFile(t, filepath.Join(other, ".git", "config"), "[user]\n\tname = Other Repo\n") + t.Setenv("GIT_DIR", filepath.Join(other, ".git")) + t.Setenv("GIT_WORK_TREE", other) + t.Setenv("GIT_COMMON_DIR", filepath.Join(other, ".git")) + }, + want: probeAnswer{Name: "This Repo"}}, + {name: "GIT_CONFIG_GLOBAL and the system file are honoured, NOSYSTEM too", + system: "[user]\n\temail = system@example.com\n", + global: "[user]\n\tname = Global Name\n", + setup: func(t *testing.T, c *pinCtx) { t.Setenv("GIT_CONFIG_NOSYSTEM", "1") }, + want: probeAnswer{Name: "Global Name"}}, + {name: "remote: the last url wins, a pushurl and another remote are ignored", + local: "[remote \"origin\"]\n\turl = https://github.com/first/one.git\n\turl = https://GitHub.com/Second/two.git\n\tpushurl = https://github.com/push/p\n[remote \"upstream\"]\n\turl = https://github.com/up/u\n", + want: probeAnswer{RemoteUser: "Second", RemoteRepo: "two"}}, + {name: "remote: a subsection spelled in another case is another remote", + local: "[remote \"Origin\"]\n\turl = https://github.com/cased/c\n", + want: probeAnswer{}}, + {name: "remote: read from global config too", + global: "[remote \"origin\"]\n\turl = git@github.com:globalowner/g.git\n", + want: probeAnswer{RemoteUser: "globalowner", RemoteRepo: "g"}}, + {name: "remote: a last url that is valueless leaves no remote", + local: "[remote \"origin\"]\n\turl = https://github.com/first/one\n\turl\n", + want: probeAnswer{}}, + {name: "remote: a non-GitHub remote names no owner", + local: "[remote \"origin\"]\n\turl = https://git.example.com/owner/repo\n", + want: probeAnswer{}}, + {name: "a malformed config file blinds every read", + global: "[user]\n\tname = Real Name\n[broken\n", + env: map[string]string{"GIT_AUTHOR_NAME": "Env Author"}, + want: probeAnswer{OtherNames: []string{"Env Author"}}}, + {name: "an unreadable global config is skipped, the rest still read", + global: "[user]\n\tname = Real Name\n", + local: "[user]\n\temail = local@example.com\n", + setup: func(t *testing.T, c *pinCtx) { chmod(t, c.global, 0) }, + skipAsRoot: true, + want: probeAnswer{Email: "local@example.com"}}, + {name: "an unreadable repository config blinds every read", + global: "[user]\n\tname = Real Name\n", + local: "[user]\n\temail = local@example.com\n", + setup: func(t *testing.T, c *pinCtx) { chmod(t, c.localConfig(), 0) }, + skipAsRoot: true, + want: probeAnswer{}}, + {name: "git absent", + global: "[user]\n\tname = Real Name\n", + env: map[string]string{"GIT_COMMITTER_EMAIL": "env@example.com"}, + setup: func(t *testing.T, c *pinCtx) { + t.Setenv("PATH", t.TempDir()) + }, + want: probeAnswer{OtherEmails: []string{"env@example.com"}}}, + } +} + +func chmod(t *testing.T, path string, mode os.FileMode) { + t.Helper() + if err := os.Chmod(path, mode); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(path, 0o600) }) +} + +// TestProbeIdentityPinnedSemantics is the table: each shape's full answer. +func TestProbeIdentityPinnedSemantics(t *testing.T) { + for _, tc := range identityPinCases() { + t.Run(tc.name, func(t *testing.T) { + if tc.skipAsRoot && os.Geteuid() == 0 { + t.Skip("root reads a mode-000 file") + } + c := newPinCtx(t, tc.bare) + appendFile(t, c.system, tc.system) + appendFile(t, c.global, tc.global) + if tc.local != "" { + appendFile(t, c.localConfig(), tc.local) + } + if tc.setup != nil { + tc.setup(t, c) + } + for k, v := range tc.env { + t.Setenv(k, v) + } + root := c.repo + if tc.root != nil { + root = tc.root(c) + } + got := answerOf(ProbeIdentity(root)) + if !reflect.DeepEqual(got, tc.want) { + t.Errorf("ProbeIdentity answer drifted:\n got %s\n want %s", fmtAnswer(got), fmtAnswer(tc.want)) + } + }) + } +} + +func fmtAnswer(a probeAnswer) string { + short := func(s string) string { + if len(s) > 40 { + return s[:20] + "…" + s[len(s)-10:] + } + return s + } + shortAll := func(v []string) []string { + if v == nil { + return nil + } + out := make([]string, len(v)) + for i, s := range v { + out[i] = short(s) + } + return out + } + return strings.Join([]string{ + "name=" + quote(short(a.Name)), "email=" + quote(a.Email), + "otherNames=" + quoteAll(shortAll(a.OtherNames)), "otherEmails=" + quoteAll(a.OtherEmails), + "remote=" + quote(a.RemoteUser) + "/" + quote(a.RemoteRepo), + }, " ") +} + +func quote(s string) string { return "\"" + strings.ReplaceAll(s, "\n", `\n`) + "\"" } + +func quoteAll(v []string) string { + if v == nil { + return "nil" + } + out := make([]string, len(v)) + for i, s := range v { + out[i] = quote(s) + } + return "[" + strings.Join(out, ", ") + "]" +} diff --git a/internal/core/ahoy/apply.go b/internal/core/ahoy/apply.go index e00d8be89..f7e58a6e4 100644 --- a/internal/core/ahoy/apply.go +++ b/internal/core/ahoy/apply.go @@ -5,6 +5,7 @@ import ( "encoding/hex" "errors" "fmt" + "io/fs" "os" "path/filepath" @@ -495,16 +496,31 @@ func (a *applyCtx) stepDependencies() { var newToolInstaller = tools.Default // stepSkeleton writes .abcd/config.json seed when the skeleton gap is present. +// Detection saw no file, but that read was before the lock: a config another +// abcd wrote since is kept, never replaced by the seed (iss-127). func (a *applyCtx) stepSkeleton() { if !a.approved[SafeAutocreate] || !a.has("skeleton.config_missing") { return } - cfg := map[string]any{"meta": map[string]any{"schema_version": 1}} - if err := writeConfig(a.cwd, cfg); err != nil { + wrote := false + err := withConfigLock(a.cwd, func() error { + if _, err := os.Lstat(configPath(a.cwd)); !errors.Is(err, fs.ErrNotExist) { + return err + } + cfg := map[string]any{"meta": map[string]any{"schema_version": 1}} + if err := writeConfig(a.cwd, cfg); err != nil { + return err + } + wrote = true + return nil + }) + if err != nil { a.refuse("could not write the starter settings file .abcd/config.json: " + errText(err)) return } - a.note(writeSettings, configPath(a.cwd)) + if wrote { + a.note(writeSettings, configPath(a.cwd)) + } } // stepConfigValues collects and persists the four config values. Returns nil on @@ -591,26 +607,34 @@ func (a *applyCtx) stepConfigValues() *InstallConfig { ic.ScanDeep = &v } - // Persist into the config map (read-modify-write). Re-read defensively; if the - // file turned malformed since the first read, refuse rather than clobber it. - cfgMap, cfgErr := readConfig(a.cwd) - if cfgErr != nil { - a.rollbackForced() - return nil - } - if cfgMap == nil { - cfgMap = map[string]any{} - } - setSub(cfgMap, "repo", "visibility", ic.Visibility) - setSub(cfgMap, "docs", "target", ic.DocsTarget) - setSub(cfgMap, "oracle", "backend", ic.OracleBackend) - if ic.ScanDeep != nil { - setSub(cfgMap, "scan", "deep", *ic.ScanDeep) - } - if err := writeConfig(a.cwd, cfgMap); err != nil { + // Persist into the config map (read-modify-write), under the file's lock and + // only after every prompt above has been answered: the re-read is the one + // the write is made from, so a key another abcd wrote since the first read + // is kept (iss-127). If the file turned malformed since the first read, + // refuse rather than clobber it. + err = withConfigLock(a.cwd, func() error { + cfgMap, cfgErr := readConfig(a.cwd) + if cfgErr != nil { + return cfgErr + } + if cfgMap == nil { + cfgMap = map[string]any{} + } + setSub(cfgMap, "repo", "visibility", ic.Visibility) + setSub(cfgMap, "docs", "target", ic.DocsTarget) + setSub(cfgMap, "oracle", "backend", ic.OracleBackend) + if ic.ScanDeep != nil { + setSub(cfgMap, "scan", "deep", *ic.ScanDeep) + } + return writeConfig(a.cwd, cfgMap) + }) + if err != nil { // The write did not land; do not echo a change or let downstream steps // reconcile .gitignore/markers against a config value that was not saved. a.rollbackForced() + if errors.Is(err, fsutil.ErrLockContention) || errors.Is(err, fsutil.ErrLockPathUnsafe) { + a.refuse("could not save the settings to .abcd/config.json: " + errText(err)) + } return nil } a.note(writeSettings, configPath(a.cwd)) @@ -1508,18 +1532,24 @@ func modeWouldChange(opts InstallOptions, det DetectionResult, target string) bo // defaults: the default domains live once in the abcd binary (itd-3), and this // file only overrides them per-field (one-canonical-primitive). An empty // domains map inherits every bundled default as-is. +// +// Detection saw no file, but that was before the interactive prompts: a +// rules.json written since is a hand-written override and is kept, never +// replaced by the skeleton (iss-2609281931185016). The exclusive create is the +// re-check and the write in one act, so it needs no lock. func (a *applyCtx) stepRules() { if !a.approved[SafeAutocreate] || !a.has("rules.missing") { return } rules := map[string]any{"schema_version": 1, "disabled": false, "domains": map[string]any{}} - // Contained through an os.Root opened at the repo: a committed `.abcd` ancestor - // symlink must not land rules.json outside the working tree (GHSA-xrf8-4432-gw2f). - if err := writeRepoJSON(a.cwd, rulesRelPath, rules); err != nil { + wrote, err := createRepoJSON(a.cwd, rulesRelPath, rules) + if wrote { + a.note(writeRules, filepath.Join(a.cwd, ".abcd", "rules.json")) + } + if err != nil { + // A fault after the create (the mode pin) still reports the file it made. a.refuse("could not write .abcd/rules.json: " + errText(err)) - return } - a.note(writeRules, filepath.Join(a.cwd, ".abcd", "rules.json")) } // stepVersionStamp writes the meta setup block. @@ -1530,23 +1560,32 @@ func (a *applyCtx) stepVersionStamp() { if !a.has("install_meta.missing") && !a.has("version.upgrade") { return } - cfgMap, err := readConfig(a.cwd) - if err != nil { + // Read, stamped and written under the file's lock (iss-127). + var malformed error + err := withConfigLock(a.cwd, func() error { + cfgMap, err := readConfig(a.cwd) + if err != nil { + malformed = err + return nil + } + if cfgMap == nil { + cfgMap = map[string]any{} + } + meta := subMap(cfgMap, "meta") + meta["schema_version"] = 1 + meta["setup_version"] = pluginVersion() + meta["setup_date"] = time.Now().UTC().Format("2006-01-02") + meta["project_name"] = a.det.RepoIdentity.Name + cfgMap["meta"] = meta + return writeConfig(a.cwd, cfgMap) + }) + if malformed != nil { // The same posture as stepConfigValues: a file that cannot be parsed is // never rebuilt from an empty map (GHSA-mchq-gm34-3j34). - a.refuseMalformedConfig(err) + a.refuseMalformedConfig(malformed) return } - if cfgMap == nil { - cfgMap = map[string]any{} - } - meta := subMap(cfgMap, "meta") - meta["schema_version"] = 1 - meta["setup_version"] = pluginVersion() - meta["setup_date"] = time.Now().UTC().Format("2006-01-02") - meta["project_name"] = a.det.RepoIdentity.Name - cfgMap["meta"] = meta - if err := writeConfig(a.cwd, cfgMap); err != nil { + if err != nil { a.refuse("could not record the setup version in .abcd/config.json: " + errText(err)) return } diff --git a/internal/core/ahoy/attribution_hook.go b/internal/core/ahoy/attribution_hook.go index fa7da858b..cd1624332 100644 --- a/internal/core/ahoy/attribution_hook.go +++ b/internal/core/ahoy/attribution_hook.go @@ -171,22 +171,34 @@ func (a *applyCtx) stepAttributionHook() { // as every written path does: the error names the config's absolute path, and // a receipt a user pastes into an issue must name neither the repo nor the // home directory. +// +// The read, the change and the write hold the file's lock (withConfigLock), so +// a key another abcd wrote between them is not erased (iss-127). func (a *applyCtx) recordAttributionOptIn() { - cfgMap, err := readConfig(a.cwd) + wrote := false + err := withConfigLock(a.cwd, func() error { + cfgMap, err := readConfig(a.cwd) + if err != nil { + return err + } + if cfgMap == nil { + cfgMap = map[string]any{} + } + if v, ok := boolVal(subMap(cfgMap, "attribution"), "hook"); ok && v { + return nil // already recorded: writing it again would be a diff with no change in it + } + setSub(cfgMap, "attribution", "hook", true) + if err := writeConfig(a.cwd, cfgMap); err != nil { + return err + } + wrote = true + return nil + }) if err != nil { a.changes = append(a.changes, receiptPath(a.cwd, "attribution opt-in not persisted ("+err.Error()+")")) return } - if cfgMap == nil { - cfgMap = map[string]any{} - } - if v, ok := boolVal(subMap(cfgMap, "attribution"), "hook"); ok && v { - return // already recorded: writing it again would be a diff with no change in it - } - setSub(cfgMap, "attribution", "hook", true) - if err := writeConfig(a.cwd, cfgMap); err != nil { - a.changes = append(a.changes, receiptPath(a.cwd, "attribution opt-in not persisted ("+err.Error()+")")) - return + if wrote { + a.note(writeSettings, configPath(a.cwd)) } - a.note(writeSettings, configPath(a.cwd)) } diff --git a/internal/core/ahoy/gitignore.go b/internal/core/ahoy/gitignore.go index 866424a8d..eacf0efee 100644 --- a/internal/core/ahoy/gitignore.go +++ b/internal/core/ahoy/gitignore.go @@ -187,7 +187,19 @@ func applyVisibilityBlock(cwd, visibility string) (bool, error) { } entries, _ := effectiveVisibilityEntries(cwd, visibility) path := filepath.Join(cwd, ".gitignore") + var wrote bool + err := withRewriteLock(path, func() error { + var err error + wrote, err = rewriteVisibilityBlock(path, entries) + return err + }) + return wrote, err +} +// rewriteVisibilityBlock is applyVisibilityBlock's read, change and write of +// path, run under the file's lock (withRewriteLock) so an edit another abcd +// makes to .gitignore between the read and the write is kept (iss-127). +func rewriteVisibilityBlock(path string, entries []string) (bool, error) { // Keep the symlink pre-check: it yields a DISTINCT refusal that ReadGuarded's // O_NOFOLLOW would otherwise collapse into a raw ELOOP passthrough error. if fi, err := os.Lstat(path); err == nil && fi.Mode()&os.ModeSymlink != 0 { diff --git a/internal/core/ahoy/marker.go b/internal/core/ahoy/marker.go index d74102de5..4eb770550 100644 --- a/internal/core/ahoy/marker.go +++ b/internal/core/ahoy/marker.go @@ -114,7 +114,20 @@ func classifyMarker(targetPath string) markerState { // file untouched, and says why, so the install can tell the person which file // kept no block and for what reason (iss-2609260057127611). Byte-stable: a // current block is not rewritten. +// +// The read and the write hold the file's lock (withRewriteLock), so an edit +// another abcd makes to the file between them is kept (iss-127). func installMarkerFile(targetPath string) (wrote bool, err error) { + err = withRewriteLock(targetPath, func() error { + var ierr error + wrote, ierr = installMarkerFileLocked(targetPath) + return ierr + }) + return wrote, err +} + +// installMarkerFileLocked is installMarkerFile's read, change and write. +func installMarkerFileLocked(targetPath string) (wrote bool, err error) { // Reject a symlinked leaf so a planted symlink cannot redirect the write. if fi, lerr := os.Lstat(targetPath); lerr == nil && fi.Mode()&os.ModeSymlink != 0 { return false, &ahoyError{"it is a symlink, and abcd never writes through one"} @@ -258,7 +271,18 @@ func composeMarkerReplacement(existing []byte, matches [][]int, synth []byte) [] // install introduced so install->uninstall round-trips. Returns (wrote, err): a // symlinked leaf, a non-regular file, or a failed read or write leaves the file // untouched and returns why. +// The read and the write hold the file's lock, as installMarkerFile's do. func removeMarkerFile(targetPath string) (wrote bool, err error) { + err = withRewriteLock(targetPath, func() error { + var rerr error + wrote, rerr = removeMarkerFileLocked(targetPath) + return rerr + }) + return wrote, err +} + +// removeMarkerFileLocked is removeMarkerFile's read, change and write. +func removeMarkerFileLocked(targetPath string) (wrote bool, err error) { fi, lerr := os.Lstat(targetPath) if lerr != nil { if os.IsNotExist(lerr) { diff --git a/internal/core/ahoy/rewritelock.go b/internal/core/ahoy/rewritelock.go new file mode 100644 index 000000000..8547bcc5b --- /dev/null +++ b/internal/core/ahoy/rewritelock.go @@ -0,0 +1,77 @@ +package ahoy + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "time" + + "github.com/intentdriven/abcd/internal/fsutil" +) + +// rewriteLockTimeout bounds the wait for another abcd rewriting the same repo +// file. The rewrites it serialises are a read, an edit in memory and an atomic +// write, so a holder is done in milliseconds; a var so a test can shorten it. +var rewriteLockTimeout = 5 * time.Second + +// rewriteLockPath names the lock that guards the read-modify-write of path: a +// dot-file beside it (.config.json.lock, .gitignore.lock, .CLAUDE.md.lock). +func rewriteLockPath(path string) string { + name := filepath.Base(path) + if !strings.HasPrefix(name, ".") { + name = "." + name + } + return filepath.Join(filepath.Dir(path), name+".lock") +} + +// withRewriteLock runs fn — a read, change and write of the repo file at path — +// holding that file's lock, so two abcd runs in one working tree never write +// from the same stale read and erase each other's change (iss-127). Every ahoy +// writer of .abcd/config.json, of abcd's .gitignore block and of the marker +// block in CLAUDE.md / AGENTS.md takes it, and each re-reads the file INSIDE fn: +// the lock is advisory, and a read taken before it proves nothing. It is never +// held across a prompt; a step that asks first re-reads under the lock after. +// +// The lock is fsutil.WithFileLock, the one inter-process load-modify-write +// primitive, on a lock file beside the guarded file. That file sits in the +// user's tree — beside a committed file, in a directory git lists — so the +// holder removes it before letting go (WithFileLock's revalidation makes that +// safe for a waiter that opened it first), and a rewrite leaves nothing behind. +// +// Each lock is a leaf: no holder takes a second rewrite lock, or any other +// lock, inside fn. +func withRewriteLock(path string, fn func() error) error { + lock := rewriteLockPath(path) + err := fsutil.WithFileLock(lock, rewriteLockTimeout, func() error { + defer os.Remove(lock) + return fn() + }) + switch { + case errors.Is(err, fsutil.ErrLockContention): + return fmt.Errorf("another abcd is rewriting %s (its lock %s was held past %s); nothing was written, retry: %w", + filepath.Base(path), filepath.Base(lock), rewriteLockTimeout, err) + case errors.Is(err, fsutil.ErrLockPathUnsafe): + return fmt.Errorf("the lock %s beside %s is a symlink or not a regular file, so it is refused and nothing was written; remove it: %w", + filepath.Base(lock), filepath.Base(path), err) + } + return err +} + +// withConfigLock is withRewriteLock for .abcd/config.json. The lock sits in +// .abcd/, which a first install has not made yet, so the directory is created +// first — contained, through an os.Root at the repo, the way the config write +// itself creates it (GHSA-xrf8-4432-gw2f). +func withConfigLock(cwd string, fn func() error) error { + root, err := os.OpenRoot(cwd) + if err != nil { + return err + } + err = root.MkdirAll(filepath.Dir(configRelPath), 0o755) + root.Close() + if err != nil { + return err + } + return withRewriteLock(configPath(cwd), fn) +} diff --git a/internal/core/ahoy/rewritelock_test.go b/internal/core/ahoy/rewritelock_test.go new file mode 100644 index 000000000..bbf9db30e --- /dev/null +++ b/internal/core/ahoy/rewritelock_test.go @@ -0,0 +1,355 @@ +package ahoy + +import ( + "bytes" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/intentdriven/abcd/internal/fsutil" +) + +// rivalWindow is how long the simulated concurrent writer holds the rewrite +// lock with the rival started: far longer than any of the rewrites under test +// takes when nothing makes it wait, so an unguarded rival always lands inside +// the window. +const rivalWindow = 300 * time.Millisecond + +// raceRewrite plays a concurrent writer of path against rival, one of ahoy's +// load-modify-writes of the same file (iss-127). The writer takes path's +// rewrite lock the way every writer of the file does, reads the file, starts +// rival, waits rivalWindow and then writes edit applied to the bytes it read. +// A rival that takes the lock waits for the writer and then rewrites from the +// writer's bytes, so both changes survive; a rival that does not lands inside +// the window and the writer's stale write erases its change. +func raceRewrite(t *testing.T, path string, edit func([]byte) []byte, rival func()) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + done := make(chan struct{}) + err := fsutil.WithFileLock(rewriteLockPath(path), 5*time.Second, func() error { + before, err := os.ReadFile(path) + if err != nil && !os.IsNotExist(err) { + return err + } + go func() { + defer close(done) + rival() + }() + time.Sleep(rivalWindow) + return os.WriteFile(path, edit(before), 0o644) + }) + if err != nil { + t.Fatalf("the concurrent writer failed: %v", err) + } + <-done +} + +// setJSON returns an edit that sets section.key to v in a JSON object. +func setJSON(t *testing.T, section, key string, v any) func([]byte) []byte { + return func(raw []byte) []byte { + m := map[string]any{} + if len(raw) > 0 { + if err := json.Unmarshal(raw, &m); err != nil { + t.Errorf("the concurrent writer read unparseable config: %v", err) + } + } + setSub(m, section, key, v) + out, err := json.MarshalIndent(m, "", " ") + if err != nil { + t.Errorf("marshal: %v", err) + } + return append(out, '\n') + } +} + +// appendLine returns an edit that appends line to a text file. +func appendLine(line string) func([]byte) []byte { + return func(raw []byte) []byte { return append(append([]byte{}, raw...), line+"\n"...) } +} + +func readConfigT(t *testing.T, dir string) map[string]any { + t.Helper() + m, err := readConfig(dir) + if err != nil { + t.Fatalf("readConfig: %v", err) + } + return m +} + +// TestConfigRewritesKeepAConcurrentWritersChange is the iss-127 detector for +// .abcd/config.json: every ahoy read-modify-write of the file, raced against a +// writer that changes another key, must keep both changes. +func TestConfigRewritesKeepAConcurrentWritersChange(t *testing.T) { + cases := []struct { + name string + rival func(dir string) + // landed reports whether the rival's own change is on disk. + landed func(m map[string]any) bool + }{ + { + name: "attribution opt-in", + rival: func(dir string) { + (&applyCtx{cwd: dir}).recordAttributionOptIn() + }, + landed: func(m map[string]any) bool { + v, ok := boolVal(subMap(m, "attribution"), "hook") + return ok && v + }, + }, + { + name: "version stamp", + rival: func(dir string) { + (&applyCtx{ + cwd: dir, + approved: map[GapCategory]bool{SafeAutocreate: true}, + gapPresent: map[string]bool{"install_meta.missing": true}, + }).stepVersionStamp() + }, + landed: func(m map[string]any) bool { + _, ok := subMap(m, "meta")["setup_version"] + return ok + }, + }, + { + name: "config values", + rival: func(dir string) { + (&applyCtx{ + cwd: dir, + approved: map[GapCategory]bool{}, + gapPresent: map[string]bool{}, + overrides: map[string]string{"visibility": "public"}, + prompter: RefusingPrompter{}, + autoYes: true, + }).stepConfigValues() + }, + landed: func(m map[string]any) bool { + v, _ := stringVal(subMap(m, "repo"), "visibility") + return v == "public" + }, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + writeValidConfig(t, dir, "private", "both", "host-delegated") + raceRewrite(t, configPath(dir), setJSON(t, "scan", "deep", false), func() { tc.rival(dir) }) + + m := readConfigT(t, dir) + if !tc.landed(m) { + t.Errorf("the rewrite's own change was lost to the concurrent writer: %v", m) + } + if v, ok := boolVal(subMap(m, "scan"), "deep"); !ok || v { + t.Errorf("the concurrent writer's scan.deep=false was lost to the rewrite: %v", m) + } + assertLockRetired(t, configPath(dir)) + }) + } +} + +// TestSkeletonNeverReplacesAConfigWrittenMeanwhile: the starter settings file +// is planted when detection found none, and a config another writer created +// since is kept rather than replaced by the seed. +func TestSkeletonNeverReplacesAConfigWrittenMeanwhile(t *testing.T) { + dir := t.TempDir() + path := configPath(dir) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + done := make(chan struct{}) + err := fsutil.WithFileLock(rewriteLockPath(path), 5*time.Second, func() error { + if err := os.WriteFile(path, []byte(`{"docs": {"target": "agents_md"}}`+"\n"), 0o644); err != nil { + return err + } + go func() { + defer close(done) + (&applyCtx{ + cwd: dir, + approved: map[GapCategory]bool{SafeAutocreate: true}, + gapPresent: map[string]bool{"skeleton.config_missing": true}, + }).stepSkeleton() + }() + time.Sleep(rivalWindow) + return nil + }) + if err != nil { + t.Fatal(err) + } + <-done + if v, _ := stringVal(subMap(readConfigT(t, dir), "docs"), "target"); v != "agents_md" { + t.Errorf("the seed replaced a config written after detection: docs.target = %q", v) + } +} + +// TestRulesSkeletonNeverReplacesARulesFileWrittenMeanwhile: detection sets +// rules.missing before the interactive prompts, and stepRules runs after them. +// A rules.json written in that window holds a hand-written override; the +// skeleton must keep it byte for byte and must not report a write it did not +// make (iss-2609281931185016). +func TestRulesSkeletonNeverReplacesARulesFileWrittenMeanwhile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, filepath.FromSlash(rulesRelPath)) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + a := &applyCtx{ + cwd: dir, + approved: map[GapCategory]bool{SafeAutocreate: true}, + gapPresent: map[string]bool{"rules.missing": true}, + } + // Written after detection saw no file, before the step runs. + written := []byte(`{"schema_version": 1, "domains": {"HOUSE": {"recall": ["house"], "rules": ["keep it"]}}}` + "\n") + if err := os.WriteFile(path, written, 0o600); err != nil { + t.Fatal(err) + } + a.stepRules() + + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(got, written) { + t.Errorf("the skeleton replaced a rules.json written after detection:\n got %s\nwant %s", got, written) + } + for _, k := range a.writeKinds { + if k == writeRules { + t.Errorf("the receipt reports a rules write that did not happen: writes = %v", a.writes) + } + } + if len(a.notes) != 0 { + t.Errorf("a kept rules.json is not a refusal; notes = %q", a.notes) + } + if fi, err := os.Stat(path); err != nil || fi.Mode().Perm() != 0o600 { + t.Errorf("the kept file's mode changed: %v %v", fi.Mode().Perm(), err) + } +} + +// TestRulesSkeletonIsWrittenWhenAbsent is the other half: with no rules.json +// the skeleton is planted, at 0644 whatever the umask, and reported. +func TestRulesSkeletonIsWrittenWhenAbsent(t *testing.T) { + dir := t.TempDir() + a := &applyCtx{ + cwd: dir, + approved: map[GapCategory]bool{SafeAutocreate: true}, + gapPresent: map[string]bool{"rules.missing": true}, + } + a.stepRules() + + path := filepath.Join(dir, filepath.FromSlash(rulesRelPath)) + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("the skeleton was not written: %v (notes %q)", err, a.notes) + } + var doc map[string]any + if err := json.Unmarshal(got, &doc); err != nil { + t.Fatalf("the skeleton is not JSON: %v\n%s", err, got) + } + if d, ok := doc["domains"].(map[string]any); !ok || len(d) != 0 { + t.Errorf("the skeleton is not the empty-domains override: %s", got) + } + if fi, err := os.Stat(path); err != nil || fi.Mode().Perm() != 0o644 { + t.Errorf("the skeleton's mode is not 0644: %v %v", fi.Mode().Perm(), err) + } + if len(a.writeKinds) != 1 || a.writeKinds[0] != writeRules { + t.Errorf("the receipt does not report the rules write: kinds = %v", a.writeKinds) + } +} + +// TestGitignoreBlockKeepsAConcurrentEdit is the iss-127 detector for the +// .gitignore block rewrite. +func TestGitignoreBlockKeepsAConcurrentEdit(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, ".gitignore") + if err := os.WriteFile(path, []byte("node_modules/\n"), 0o644); err != nil { + t.Fatal(err) + } + var rivalErr error + raceRewrite(t, path, appendLine("dist/"), func() { + _, rivalErr = applyVisibilityBlock(dir, "private") + }) + if rivalErr != nil { + t.Fatalf("applyVisibilityBlock: %v", rivalErr) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if !bytes.Contains(got, []byte(gitignoreBegin)) { + t.Errorf("the abcd block was lost to the concurrent edit:\n%s", got) + } + if !bytes.Contains(got, []byte("dist/\n")) { + t.Errorf("the concurrent edit was lost to the block rewrite:\n%s", got) + } + assertLockRetired(t, path) +} + +// TestMarkerRewritesKeepAConcurrentEdit is the iss-127 detector for the marker +// block's install and removal. +func TestMarkerRewritesKeepAConcurrentEdit(t *testing.T) { + t.Run("install", func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "CLAUDE.md") + if err := os.WriteFile(path, []byte("# Project\n\nbody\n"), 0o644); err != nil { + t.Fatal(err) + } + var rivalErr error + raceRewrite(t, path, appendLine("appended meanwhile"), func() { + _, rivalErr = installMarkerFile(path) + }) + if rivalErr != nil { + t.Fatalf("installMarkerFile: %v", rivalErr) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if !markerBlockRe.Match(got) { + t.Errorf("the marker block was lost to the concurrent edit:\n%s", got) + } + if !strings.Contains(string(got), "appended meanwhile") { + t.Errorf("the concurrent edit was lost to the marker install:\n%s", got) + } + assertLockRetired(t, path) + }) + t.Run("remove", func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "AGENTS.md") + if err := os.WriteFile(path, []byte("# Project\n\nbody\n"), 0o644); err != nil { + t.Fatal(err) + } + if _, err := installMarkerFile(path); err != nil { + t.Fatal(err) + } + var rivalErr error + raceRewrite(t, path, appendLine("appended meanwhile"), func() { + _, rivalErr = removeMarkerFile(path) + }) + if rivalErr != nil { + t.Fatalf("removeMarkerFile: %v", rivalErr) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if markerBlockRe.Match(got) { + t.Errorf("the removed marker block came back through the concurrent edit:\n%s", got) + } + if !strings.Contains(string(got), "appended meanwhile") { + t.Errorf("the concurrent edit was lost to the marker removal:\n%s", got) + } + assertLockRetired(t, path) + }) +} + +// assertLockRetired: the rewrite lock beside a file the user owns is removed +// by its last holder, so a rewrite leaves no lock file behind in the tree. +func assertLockRetired(t *testing.T, path string) { + t.Helper() + if _, err := os.Lstat(rewriteLockPath(path)); !os.IsNotExist(err) { + t.Errorf("the rewrite lock %s was left behind (err=%v)", filepath.Base(rewriteLockPath(path)), err) + } +} diff --git a/internal/core/ahoy/store.go b/internal/core/ahoy/store.go index 84aaa2e28..315b09130 100644 --- a/internal/core/ahoy/store.go +++ b/internal/core/ahoy/store.go @@ -6,8 +6,10 @@ import ( "encoding/hex" "encoding/json" "errors" + "io/fs" "os" "os/exec" + "path" "path/filepath" "time" @@ -940,7 +942,7 @@ func bootstrapHistory() (bool, error) { tmp.Close() return false, err } - if err := tmp.Sync(); err != nil { + if err := fsutil.Flush(tmp); err != nil { tmp.Close() return false, err } @@ -1092,6 +1094,41 @@ func writeRepoJSON(cwd, rel string, v any) error { return fsutil.WriteFileAtomicPreserveModeInRoot(root, rel, data) } +// createRepoJSON creates v at a repo-.abcd path only when nothing is there, +// through an os.Root opened at cwd like writeRepoJSON. It reports wrote=false +// with no error when the path already exists: a file that appeared after +// detection is kept, not replaced. The exclusive create resolves every +// component through the Root, so a symlinked `.abcd` ancestor escaping the +// tree is refused rather than followed (GHSA-xrf8-4432-gw2f). The new file's +// mode is pinned to 0644, the mode the atomic writer gives it, whatever the +// umask. +func createRepoJSON(cwd, rel string, v any) (wrote bool, err error) { + data, err := marshalJSON(v) + if err != nil { + return false, err + } + root, err := os.OpenRoot(cwd) + if err != nil { + return false, err + } + defer root.Close() + if dir := path.Dir(rel); dir != "." { + if err := root.MkdirAll(dir, 0o755); err != nil { + return false, err + } + } + if err := fsutil.CreateExclusiveIn(root, rel, data, 0o644); err != nil { + if errors.Is(err, fs.ErrExist) { + return false, nil + } + return false, err + } + if err := root.Chmod(rel, 0o644); err != nil { + return true, err + } + return true, nil +} + // writeConfig persists a config map deterministically, contained under cwd. func writeConfig(cwd string, cfg map[string]any) error { return writeRepoJSON(cwd, configRelPath, cfg) diff --git a/internal/core/banlist/public_test.go b/internal/core/banlist/public_test.go index dcf251cbe..e5b84580d 100644 --- a/internal/core/banlist/public_test.go +++ b/internal/core/banlist/public_test.go @@ -13,6 +13,31 @@ import ( "github.com/intentdriven/abcd/internal/core/lint" ) +// nameRootFixtures names one file per name_roots entry of cfg, and per +// extra_roots entry of any banned token (the role ban's, itd-2609212137129937), +// the entry itself when this repository holds it as a file and a README under it +// when it holds a directory, so a fixture tree resolves every root the real +// config declares without a second list to keep in step with it. +func nameRootFixtures(t *testing.T, cfg lint.Config) []string { + t.Helper() + var out []string + roots := append([]string(nil), cfg.NameRoots...) + for _, bt := range cfg.BannedTokens { + roots = append(roots, bt.ExtraRoots...) + } + for _, r := range roots { + st, err := os.Stat(filepath.Join("..", "..", "..", filepath.FromSlash(r))) + if err != nil { + t.Fatalf("configured root %q does not resolve in this repository: %v", r, err) + } + if st.IsDir() { + r += "/README.md" + } + out = append(out, r) + } + return out +} + // realConfig copies this repo's own committed docs-lint config into a temp repo. // Editing the REAL file's bytes is the point of these tests: a surgical editor // proven only against a synthetic two-entry fixture is not proven at all. @@ -135,8 +160,7 @@ func TestAddPublicEntryGatesUserFacingContent(t *testing.T) { write("README.md", "# readme\n") // Its name_roots must resolve too (iss-279), and the role ban's extra_roots // (itd-2609212137129937). - for _, r := range []string{".abcd/README.md", "AGENTS.md", ".github/CONTRIBUTING.md", "scripts/README.md", - "commands/README.md", ".abcd/rules.json", "internal/core/rules/defaults/rules.json"} { + for _, r := range nameRootFixtures(t, cfg) { write(r, "# t\n") } provisionDocsLintTrees(t, cfg, docs) @@ -442,8 +466,7 @@ func TestAddPublicIsCaseInsensitiveLikeTheCuratedEntries(t *testing.T) { } // Its name_roots must resolve too (iss-279), and the role ban's extra_roots // (itd-2609212137129937). - for _, r := range []string{".abcd/README.md", "AGENTS.md", ".github/CONTRIBUTING.md", "scripts/README.md", - "commands/README.md", ".abcd/rules.json", "internal/core/rules/defaults/rules.json"} { + for _, r := range nameRootFixtures(t, cfg) { p := filepath.Join(docs, filepath.FromSlash(r)) if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { t.Fatal(err) diff --git a/internal/core/implement/backoff_test.go b/internal/core/implement/backoff_test.go new file mode 100644 index 000000000..f5c11cdec --- /dev/null +++ b/internal/core/implement/backoff_test.go @@ -0,0 +1,151 @@ +package implement + +import ( + "errors" + "path/filepath" + "strings" + "testing" + + "github.com/intentdriven/abcd/internal/fsutil" +) + +// TestALockedRunStateIsABackoffWithItsReasonAndMinutes: contention of any kind +// the second session meets is a backoff the log names, with the reason and the +// minutes spent (itd-2609221656373558 criterion 6, iss-2609231206196407). A run +// state locked by another session's change is contention the verb itself sees, +// so the verb writes the line, with the minutes it waited for the lock. +func TestALockedRunStateIsABackoffWithItsReasonAndMinutes(t *testing.T) { + r, _ := newRun(t) + join(t, r, "alpha", RoleFirst) + join(t, r, "beta", RoleSecond) + + held := make(chan struct{}) + release := make(chan struct{}) + done := make(chan error, 1) + go func() { + done <- fsutil.WithFileLock(filepath.Join(r.Dir, lockFileName), lockTimeout, func() error { + close(held) + <-release + return nil + }) + }() + <-held + _, err := r.Claim(ClaimRequest{Session: "beta", Record: "itd-1", Lane: "one"}) + close(release) + if lerr := <-done; lerr != nil { + t.Fatalf("holding the lock: %v", lerr) + } + if !errors.Is(err, ErrContention) { + t.Fatalf("a claim against a locked run state = %v; want contention", err) + } + b := lastEvent(t, r, EventBackoff) + if b.Session != "beta" || b.String("on") != "run_state" || b.String("reason") == "" { + t.Fatalf("backoff line = %+v; want the second session's run_state backoff naming its reason", b.Fields) + } + if m, ok := b.Number("minutes"); !ok || m <= 0 { + t.Fatalf("backoff minutes = %v (present %v); want the measured wait for the lock", m, ok) + } +} + +// TestAJoinAgainstALockedRunStateLogsItsBackoff: a second session whose Join +// meets a locked run state has backed off as surely as one whose claim did, but +// its record does not exist yet, so a backoff that looked the role up in the +// record logged nothing and the lookup's refusal was dropped. The joining +// session's requested role stands in for the record it is about to write. +func TestAJoinAgainstALockedRunStateLogsItsBackoff(t *testing.T) { + r, _ := newRun(t) + join(t, r, "alpha", RoleFirst) + + held := make(chan struct{}) + release := make(chan struct{}) + done := make(chan error, 1) + go func() { + done <- fsutil.WithFileLock(filepath.Join(r.Dir, lockFileName), lockTimeout, func() error { + close(held) + <-release + return nil + }) + }() + <-held + _, err := r.Join("beta", RoleSecond, "", "", 0) + close(release) + if lerr := <-done; lerr != nil { + t.Fatalf("holding the lock: %v", lerr) + } + if !errors.Is(err, ErrContention) { + t.Fatalf("a join against a locked run state = %v; want contention", err) + } + if strings.Contains(err.Error(), "could not be logged") { + t.Fatalf("the join's backoff was not logged: %v", err) + } + b := lastEvent(t, r, EventBackoff) + if b.Session != "beta" || b.String("on") != "run_state" || b.String("reason") == "" { + t.Fatalf("backoff line = %+v; want the joining second session's run_state backoff", b.Fields) + } + if m, ok := b.Number("minutes"); !ok || m <= 0 { + t.Fatalf("backoff minutes = %v (present %v); want the measured wait for the lock", m, ok) + } +} + +// TestAnUnloggableBackoffIsSaid: a backoff whose session cannot be placed (it +// never joined, and no Join is recording it) is not written, and the contention +// error says so rather than the lookup's refusal being dropped. +func TestAnUnloggableBackoffIsSaid(t *testing.T) { + r, _ := newRun(t) + join(t, r, "alpha", RoleFirst) + + held := make(chan struct{}) + release := make(chan struct{}) + done := make(chan error, 1) + go func() { + done <- fsutil.WithFileLock(filepath.Join(r.Dir, lockFileName), lockTimeout, func() error { + close(held) + <-release + return nil + }) + }() + <-held + _, err := r.Claim(ClaimRequest{Session: "gamma", Record: "itd-1", Lane: "one"}) + close(release) + if lerr := <-done; lerr != nil { + t.Fatalf("holding the lock: %v", lerr) + } + if !errors.Is(err, ErrContention) { + t.Fatalf("a claim against a locked run state = %v; want contention", err) + } + if !strings.Contains(err.Error(), "could not be logged") || !strings.Contains(err.Error(), "gamma has not joined") { + t.Fatalf("contention for a session that never joined = %v; want it to say the backoff went unlogged and why", err) + } + for _, n := range eventNames(t, r) { + if n == EventBackoff { + t.Fatal("a backoff was written for a session that never joined") + } + } +} + +// TestAHandLoggedBackoffNamesItsReasonAndMinutes: contention the verb cannot +// see (the merge queue, a peer's worktree) is logged on the session's word, and +// that line must name the reason and the minutes too, or it is refused with +// nothing written (iss-2609231206196407). +func TestAHandLoggedBackoffNamesItsReasonAndMinutes(t *testing.T) { + r, _ := newRun(t) + join(t, r, "beta", RoleSecond) + for _, fields := range []map[string]string{ + {"on": "queue", "minutes": "4"}, + {"on": "queue", "reason": "queue busy"}, + {"on": "queue", "reason": "queue busy", "minutes": "a while"}, + {"on": "queue", "reason": "queue busy", "minutes": "-1"}, + } { + if _, err := r.Log("beta", EventBackoff, fields); !errors.Is(err, ErrRefused) { + t.Errorf("backoff %v = %v; want refused", fields, err) + } + } + for _, n := range eventNames(t, r) { + if n == EventBackoff { + t.Fatal("a refused backoff was written") + } + } + if _, err := r.Log("beta", EventBackoff, map[string]string{"on": "queue", "reason": "queue busy", "minutes": "4"}); err != nil { + t.Fatalf("a backoff naming its reason and minutes: %v", err) + } +} diff --git a/internal/core/implement/bounds.go b/internal/core/implement/bounds.go index 559e14e69..88caf2764 100644 --- a/internal/core/implement/bounds.go +++ b/internal/core/implement/bounds.go @@ -153,7 +153,7 @@ func (r *Run) Check(session string, step Step, paths []string) (Verdict, error) } } var out Verdict - err := r.withLock(func() error { + err := r.withLock(session, func() error { s, err := r.requireSession(session) if err != nil { return err diff --git a/internal/core/implement/claim.go b/internal/core/implement/claim.go index 5aad816f1..b562a66ca 100644 --- a/internal/core/implement/claim.go +++ b/internal/core/implement/claim.go @@ -144,8 +144,9 @@ func (r *Run) Claim(req ClaimRequest) (ClaimResult, error) { return ClaimResult{}, refusal("path %q is not a repository-relative path", p) } } + start := time.Now() var out ClaimResult - err := r.withLock(func() error { + err := r.withLock(req.Session, func() error { s, err := r.requireSession(req.Session) if err != nil { return err @@ -180,6 +181,10 @@ func (r *Run) Claim(req ClaimRequest) (ClaimResult, error) { // Nobody can say who holds it. Within the grace it is contention; // after it the file has lapsed, and the lapse is logged as one. if now.Before(bad.LapsesAt) { + if err := r.logBackoff(req.Session, "", "claim", "unreadable claim file within its grace", + time.Since(start), map[string]any{"record": req.Record}); err != nil { + return err + } return fmt.Errorf("%w: %v; it lapses at %s, so back off and retry after then", ErrContention, bad, bad.LapsesAt.Format(time.RFC3339)) } @@ -226,13 +231,9 @@ func (r *Run) Claim(req ClaimRequest) (ClaimResult, error) { }); err != nil { return err } - if s.Role == RoleSecond { - if _, err := r.append(req.Session, EventBackoff, map[string]any{ - "on": "claim", "record": req.Record, - "reason": "record claimed by session " + held.Session, "minutes": 0, - }); err != nil { - return err - } + if err := r.logBackoff(req.Session, "", "claim", "record claimed by session "+held.Session, + time.Since(start), map[string]any{"record": req.Record}); err != nil { + return err } return &HeldError{Holder: held} } @@ -280,7 +281,7 @@ func (r *Run) Release(session, record string) (Claim, error) { return Claim{}, err } var out Claim - err := r.withLock(func() error { + err := r.withLock(session, func() error { if _, err := r.requireSession(session); err != nil { return err } diff --git a/internal/core/implement/load.go b/internal/core/implement/load.go index 86f84869f..3f5bd5a23 100644 --- a/internal/core/implement/load.go +++ b/internal/core/implement/load.go @@ -380,7 +380,7 @@ func logLoadWarning(req LoadRequest, res LoadResult) LoadRunLog { } } fields := loadEventFields(res) - err = run.withLock(func() error { + err = run.withLock(session, func() error { _, err := run.append(session, EventLoad, fields) return err }) diff --git a/internal/core/implement/log.go b/internal/core/implement/log.go index 9a62e2aa1..32281fdb5 100644 --- a/internal/core/implement/log.go +++ b/internal/core/implement/log.go @@ -413,11 +413,16 @@ func (r *Run) Log(session, event string, fields map[string]string) (Event, error } typed[k] = typedValue(v) } + if event == EventBackoff { + if err := backoffFields(fields); err != nil { + return Event{}, err + } + } if err := checkFields(event, fields); err != nil { return Event{}, err } var out Event - err := r.withLock(func() error { + err := r.withLock(session, func() error { s, err := r.requireSession(session) if err != nil { return err @@ -434,6 +439,22 @@ func (r *Run) Log(session, event string, fields map[string]string) (Event, error return out, err } +// backoffFields holds a hand-logged backoff to what criterion 6 of +// itd-2609221656373558 says the log names: the reason the session backed off and +// the minutes it spent, a number no smaller than zero. A backoff missing either +// counts in the comparison as a backoff that cost nothing for no reason, so it +// is refused rather than written. +func backoffFields(fields map[string]string) error { + if strings.TrimSpace(fields["reason"]) == "" { + return refusal("a backoff names its reason (--field reason=)") + } + m, err := strconv.ParseFloat(fields["minutes"], 64) + if err != nil || math.IsNaN(m) || math.IsInf(m, 0) || m < 0 { + return refusal("a backoff names the minutes it spent as a number no smaller than zero (--field minutes=), not %q", fields["minutes"]) + } + return nil +} + // typedValue reads a hand-given value as the JSON type it spells, and only when // writing that value back gives the same text: `0123456` (a short sha), `-0`, // `+5`, `1.50` and `1e3` stay the strings they were, because a number would diff --git a/internal/core/implement/run.go b/internal/core/implement/run.go index 6c39d5910..df326f0e1 100644 --- a/internal/core/implement/run.go +++ b/internal/core/implement/run.go @@ -174,16 +174,64 @@ func (r *Run) root() (*os.Root, error) { return os.OpenRoot(r.Dir) } -// withLock runs fn holding the run's advisory lock. Every mutation of the claim -// and session state takes it, so a read-decide-write sequence (a lapse and a -// re-claim, a cap count and a claim) is never interleaved with another -// session's. The exclusive create underneath remains the exclusion itself: the -// lock orders the sequences, the create decides the race. -func (r *Run) withLock(fn func() error) error { +// withLock runs fn holding the run's advisory lock on session's behalf. Every +// mutation of the claim and session state takes it, so a read-decide-write +// sequence (a lapse and a re-claim, a cap count and a claim) is never +// interleaved with another session's. The exclusive create underneath remains +// the exclusion itself: the lock orders the sequences, the create decides the +// race. A lock another session's change holds past lockTimeout is contention, +// and the second session's backoff from it is logged with the minutes it waited. +func (r *Run) withLock(session string, fn func() error) error { + return r.withLockAs(session, "", fn) +} + +// withLockAs is withLock for a change that writes session's record under the +// lock (Join): joining is the role that record will carry, so a backoff from +// contention met before the record exists is still attributed to the session. +func (r *Run) withLockAs(session string, joining Role, fn func() error) error { + start := time.Now() err := fsutil.WithFileLock(filepath.Join(r.Dir, lockFileName), lockTimeout, fn) if errors.Is(err, fsutil.ErrLockContention) { - return fmt.Errorf("%w: the run state is locked by another session's change; back off and retry", ErrContention) + const msg = "the run state is locked by another session's change; back off and retry" + if lerr := r.logBackoff(session, joining, "run_state", "run state locked by another session's change", time.Since(start), nil); lerr != nil { + return fmt.Errorf("%w: %s (and the backoff could not be logged: %v)", ErrContention, msg, lerr) + } + return fmt.Errorf("%w: %s", ErrContention, msg) + } + return err +} + +// logBackoff writes the second session's backoff from contention the verb +// itself met: where it backed off (on), the reason, and the minutes the attempt +// spent, measured from its start. The bound is the second session's +// (itd-2609221656373558 criterion 6), so a first session logs nothing. The role +// is the session record's; a session that has not joined yet takes joining, the +// role its Join is about to record, and with none the refusal is returned, as +// is a record that cannot be read, so the caller says the backoff went unlogged +// rather than dropping it. The append takes no lock, so a backoff from the lock +// itself still reaches the log. +func (r *Run) logBackoff(session string, joining Role, on, reason string, spent time.Duration, extra map[string]any) error { + if session == "" { + return nil + } + role := joining + s, err := r.requireSession(session) + switch { + case err == nil: + role = s.Role + case joining != "" && errors.Is(err, ErrRefused): + // Not joined yet: the role stays the one its Join is recording. + default: + return err + } + if role != RoleSecond { + return nil + } + f := map[string]any{"on": on, "reason": reason, "minutes": round2(spent.Minutes())} + for k, v := range extra { + f[k] = v } + _, err = r.append(session, EventBackoff, f) return err } diff --git a/internal/core/implement/session.go b/internal/core/implement/session.go index 58a4808a8..ec2146c2d 100644 --- a/internal/core/implement/session.go +++ b/internal/core/implement/session.go @@ -133,7 +133,7 @@ func (r *Run) Join(id string, role Role, model, reason string, ceiling int) (Joi return JoinResult{}, refusal("ceiling %d is outside 0 (none stated) to %d", ceiling, MaxCeiling) } var out JoinResult - err := r.withLock(func() error { + err := r.withLockAs(id, role, func() error { root, err := r.root() if err != nil { return err @@ -201,7 +201,7 @@ func (r *Run) Leave(id, reason string) (LeaveResult, error) { return LeaveResult{}, err } out := LeaveResult{Released: []Claim{}} - err := r.withLock(func() error { + err := r.withLock(id, func() error { s, err := r.requireSession(id) if err != nil { return err @@ -332,7 +332,7 @@ func (r *Run) SetMode(session string, m Mode, window int) (WindowState, error) { return WindowState{}, refusal("window %d is negative", window) } var out WindowState - err := r.withLock(func() error { + err := r.withLock(session, func() error { s, err := r.requireSession(session) if err != nil { return err diff --git a/internal/core/launch/lockstep.go b/internal/core/launch/lockstep.go index bd987f602..513fa4480 100644 --- a/internal/core/launch/lockstep.go +++ b/internal/core/launch/lockstep.go @@ -21,6 +21,30 @@ const ( TreePublic LockstepTree = "public" ) +// ParseLockstepTree reads a polarity by its name, refusing any other. +func ParseLockstepTree(s string) (LockstepTree, error) { + switch LockstepTree(s) { + case TreeDev, TreePublic: + return LockstepTree(s), nil + } + return "", fmt.Errorf("tree %q is neither %q nor %q", s, TreeDev, TreePublic) +} + +// CheckTree runs the lockstep check the caller chooses over the checkout at +// root, reading that checkout's own version-location contract and artefact +// declaration: the check the preview and the cut run at the dev polarity over +// the source tree, and the payload render at the public polarity over its +// output, reachable for any tree a person holds — a public checkout, such as a +// marketplace install or a release source archive, included (itd-69). An +// artefact declaration that cannot be read is an unreadable input (exit 2). +func CheckTree(tree LockstepTree, root string) LockstepResult { + art, err := LoadArtefactOrPlugin(root) + if err != nil { + return unreadable(LockstepResult{Tree: tree}, "artefact declaration not readable: "+err.Error()) + } + return kindLockstep(tree, root, art) +} + // LockstepResult is the outcome of a manifest lockstep check. type LockstepResult struct { Tree LockstepTree `json:"tree"` diff --git a/internal/core/launch/scaffold/templates/release.yml.tmpl b/internal/core/launch/scaffold/templates/release.yml.tmpl index 114f4b09b..af1ba7979 100644 --- a/internal/core/launch/scaffold/templates/release.yml.tmpl +++ b/internal/core/launch/scaffold/templates/release.yml.tmpl @@ -230,7 +230,7 @@ jobs: <%- if .Abcd %> - name: Test (race, internal) - run: go test -race -timeout 20m ./internal/... + run: go test -race -timeout 20m ./internal/surface/cli ./internal/core/reading ./internal/core/launch ./internal/core/lifeboat ./internal/core/lint ./internal/adapter/scanner ./internal/core/capture ./internal/core/ahoy ./internal/core/site ./internal/... <%- else %> # A generic module may have no internal tree, so the race leg runs over the diff --git a/internal/core/lint/lint_test.go b/internal/core/lint/lint_test.go index d2f7ca98d..e4be79fc7 100644 --- a/internal/core/lint/lint_test.go +++ b/internal/core/lint/lint_test.go @@ -279,9 +279,21 @@ func TestDocsLintHarnessNameGate(t *testing.T) { // now that an unresolvable configured root fails loud (GitHub #360). writeFile(t, root, "README.md", "# readme\n") // Its name_roots must resolve too (iss-279), and the role ban's extra_roots - // (itd-2609212137129937). - for _, r := range []string{".abcd/README.md", "AGENTS.md", ".github/CONTRIBUTING.md", "scripts/README.md", - "commands/README.md", ".abcd/rules.json", "internal/core/rules/defaults/rules.json"} { + // (itd-2609212137129937), each as the kind of path it is in this repository, + // read from the config so a root added there is built here without a second + // list to keep in step. + roots := append([]string(nil), cfg.NameRoots...) + for _, bt := range cfg.BannedTokens { + roots = append(roots, bt.ExtraRoots...) + } + for _, r := range roots { + st, err := os.Stat(filepath.Join("..", "..", "..", r)) + if err != nil { + t.Fatalf("configured root %q does not resolve in this repository: %v", r, err) + } + if st.IsDir() { + r += "/README.md" + } writeFile(t, root, r, "# t\n") } // So must links_resolve's extra roots (iss-46), read from the config so a diff --git a/internal/core/lint/nameroots_test.go b/internal/core/lint/nameroots_test.go index 69ccb691d..7169341b6 100644 --- a/internal/core/lint/nameroots_test.go +++ b/internal/core/lint/nameroots_test.go @@ -6,6 +6,8 @@ import ( "path/filepath" "strings" "testing" + + "github.com/intentdriven/abcd/internal/core/launch" ) func nameToken() BannedToken { @@ -59,9 +61,17 @@ func TestNameRootsCarryTheNamesFamilyOnly(t *testing.T) { } // TestRepoNameRootsCoverThePublicSurface pins this repository's own coverage: -// the name gate reaches .abcd/**, the root prose files and scripts/ (iss-279). +// the name gate reaches .abcd/**, the root prose files and scripts/ (iss-279), +// and every surface the shipped artefact carries, which itd-74's first +// criterion names as gated (iss-2609261457358637). The shipped surfaces are +// read from the payload's own include list, through the reader the launch +// bundler uses, rather than restated here: a hand list covered commands/, +// agents/ and hooks/ while the payload also ships .claude-plugin/, LICENSE and +// .gitignore, so a surface added to the payload would have escaped the gate +// with this test still green. func TestRepoNameRootsCoverThePublicSurface(t *testing.T) { - data, err := os.ReadFile(filepath.Join("..", "..", "..", ".abcd", "docs-lint.json")) + repo := filepath.Join("..", "..", "..") + data, err := os.ReadFile(filepath.Join(repo, ".abcd", "docs-lint.json")) if err != nil { t.Fatal(err) } @@ -69,15 +79,40 @@ func TestRepoNameRootsCoverThePublicSurface(t *testing.T) { if err := json.Unmarshal(data, &cfg); err != nil { t.Fatal(err) } - have := map[string]bool{} - for _, r := range append(append([]string{}, cfg.Roots...), cfg.NameRoots...) { - have[r] = true + roots := append(append([]string{}, cfg.Roots...), cfg.NameRoots...) + covered := func(path string) bool { + for _, r := range roots { + if path == r || strings.HasPrefix(path, r+"/") { + return true + } + } + return false } for _, want := range []string{".abcd", "AGENTS.md", ".github/CONTRIBUTING.md", "scripts", "README.md", "docs"} { - if !have[want] { + if !covered(want) { t.Errorf("the name gate does not reach %s (roots %q, name_roots %q)", want, cfg.Roots, cfg.NameRoots) } } + includes, err := launch.LoadIncludes(repo) + if err != nil { + t.Fatal(err) + } + for _, include := range includes { + // A glob include is covered when a root reaches the literal directory + // it expands under; one with no literal directory cannot be judged by a + // prefix, so it fails here until this test learns to expand it. + path := include + if i := strings.IndexAny(path, "*?["); i >= 0 { + path = strings.TrimRight(path[:strings.LastIndex(path[:i], "/")+1], "/") + if path == "" { + t.Errorf("the payload include %q has no literal directory to hold a name root to; expand it here", include) + continue + } + } + if !covered(path) { + t.Errorf("the payload ships %s but the name gate does not reach it (roots %q, name_roots %q)", include, cfg.Roots, cfg.NameRoots) + } + } } // TestNameRootsRefuseAFileTheyCannotExamine: a file the walk lists but cannot diff --git a/internal/core/lint/racelanebudget_test.go b/internal/core/lint/racelanebudget_test.go index ecf33682d..f64fe5744 100644 --- a/internal/core/lint/racelanebudget_test.go +++ b/internal/core/lint/racelanebudget_test.go @@ -2,8 +2,10 @@ package lint_test import ( "encoding/json" + "os/exec" "path/filepath" "regexp" + "slices" "strconv" "strings" "testing" @@ -45,6 +47,13 @@ import ( // package timeout plus the time its slowest package waits before it starts: // the steps ahead of the race step, and the packages `go test` runs ahead of // it inside the step. +// +// The three commands also name one package list, in one order. go test starts +// packages in the order its arguments list them, a few at a time on a runner +// with few cores, so the slowest packages are named ahead of the pattern and +// start first instead of last (iss-2609281715083637). The order may change; the +// set may not: go list must resolve the command's packages to exactly the ones +// ./internal/... matches. func TestRaceLaneBudgetIsDeclaredAndFitsItsJob(t *testing.T) { root := filepath.Join("..", "..", "..") @@ -53,6 +62,8 @@ func TestRaceLaneBudgetIsDeclaredAndFitsItsJob(t *testing.T) { t.Fatal("Makefile declares no `preflight:` recipe") } local := raceTimeout(t, "Makefile preflight", recipe) + localPkgs := racePackages(t, "Makefile preflight", recipe) + sameRacePackageSet(t, root, "Makefile preflight", localPkgs) // raceJob returns a workflow job's block and the -timeout its race step // carries, holding that timeout to the local gate's. @@ -73,6 +84,10 @@ func TestRaceLaneBudgetIsDeclaredAndFitsItsJob(t *testing.T) { t.Errorf("%s runs the race lane under -timeout %s but make preflight uses %s; "+ "local and CI must judge the lane against one budget", where, pkg, local) } + if pkgs := racePackages(t, where, step); !slices.Equal(pkgs, localPkgs) { + t.Errorf("%s runs the race lane over %q but make preflight runs it over %q; "+ + "local and CI start the same packages in the same order", where, pkgs, localPkgs) + } return block, pkg, true } @@ -270,9 +285,9 @@ func mergeQueueResponseTimeout(t *testing.T, root string) time.Duration { return 0 } -// raceTimeout returns the -timeout the one `go test -race` command in text -// carries, failing the test when there is none. -func raceTimeout(t *testing.T, where, text string) time.Duration { +// raceCommand returns the one `go test -race` command in text, failing the +// test when there is not exactly one. +func raceCommand(t *testing.T, where, text string) string { t.Helper() var cmds []string for _, l := range strings.Split(text, "\n") { @@ -284,10 +299,83 @@ func raceTimeout(t *testing.T, where, text string) time.Duration { if len(cmds) != 1 { t.Fatalf("%s: want one `go test -race` command, found %d", where, len(cmds)) } - m := regexp.MustCompile(`\s-timeout[ =](\S+)`).FindStringSubmatch(cmds[0]) + return cmds[0] +} + +// racePackages returns the package arguments of the one `go test -race` +// command in text, in the order written: every word after `go test` that is +// neither a flag nor a flag's value. +func racePackages(t *testing.T, where, text string) []string { + t.Helper() + var pkgs []string + words := strings.Fields(raceCommand(t, where, text))[2:] + for i := 0; i < len(words); i++ { + switch w := words[i]; { + case w == "-timeout": + i++ // its value is the next word + case strings.HasPrefix(w, "-"): + default: + pkgs = append(pkgs, w) + } + } + if len(pkgs) == 0 { + t.Fatalf("%s: the `go test -race` command names no packages", where) + } + return pkgs +} + +// raceLanePattern is the package set the race lane covers. The command may +// name packages ahead of it, so go test starts the slowest first on a runner +// with few cores, but it may neither add nor drop one. +const raceLanePattern = "./internal/..." + +// sameRacePackageSet holds the race lane's packages to exactly the ones +// raceLanePattern matches, as go list resolves both: the slowest-first order +// (iss-2609281715083637) names packages twice, once explicitly and once in the +// pattern, and go test runs each once, so the order is free while the set is +// not. +func sameRacePackageSet(t *testing.T, root, where string, pkgs []string) { + t.Helper() + if pkgs[len(pkgs)-1] != raceLanePattern { + t.Errorf("%s: the race lane's packages are %q; want %s last, so every package it matches runs", + where, pkgs, raceLanePattern) + } + seen := map[string]bool{} + for _, p := range pkgs { + if seen[p] { + t.Errorf("%s: the race lane names %s twice", where, p) + } + seen[p] = true + } + list := func(args ...string) []string { + cmd := exec.Command("go", append([]string{"list"}, args...)...) + cmd.Dir = root + out, err := cmd.Output() + if err != nil { + t.Fatalf("%s: go list %s: %v", where, strings.Join(args, " "), err) + } + return strings.Fields(string(out)) + } + got, want := list(pkgs...), list(raceLanePattern) + if len(got) != len(want) { + t.Errorf("%s: go list resolves the race lane to %d packages, %s to %d", where, len(got), raceLanePattern, len(want)) + } + slices.Sort(got) + slices.Sort(want) + if !slices.Equal(got, want) { + t.Errorf("%s: the race lane's packages differ from %s's:\n lane: %q\n want: %q", where, raceLanePattern, got, want) + } +} + +// raceTimeout returns the -timeout the one `go test -race` command in text +// carries, failing the test when there is none. +func raceTimeout(t *testing.T, where, text string) time.Duration { + t.Helper() + cmd := raceCommand(t, where, text) + m := regexp.MustCompile(`\s-timeout[ =](\S+)`).FindStringSubmatch(cmd) if m == nil { t.Fatalf("%s: `%s` sets no -timeout, so the lane inherits go test's 10m default "+ - "per package, which internal/surface/cli under -race has already crossed", where, cmds[0]) + "per package, which internal/surface/cli under -race has already crossed", where, cmd) } d, err := time.ParseDuration(m[1]) if err != nil { diff --git a/internal/core/release/changeloglock_test.go b/internal/core/release/changeloglock_test.go new file mode 100644 index 000000000..a55ae723f --- /dev/null +++ b/internal/core/release/changeloglock_test.go @@ -0,0 +1,64 @@ +package release + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/intentdriven/abcd/internal/fsutil" +) + +// TestIngestNeverLosesACutToAConcurrentCut is the iss-127 detector for the +// CHANGELOG release path. A second cut of the same tree writes its dated +// section while this ingest runs. Taking the CHANGELOG's lock, the ingest waits +// for the other cut, re-derives from what it wrote, and is refused as a release +// in flight; without the lock it reports its section written while the other +// cut's stale write erases it. +func TestIngestNeverLosesACutToAConcurrentCut(t *testing.T) { + r := shippableRepo(t) + root := r.Root() + path := filepath.Join(root, changelogFile) + + var ( + res IngestResult + ierr error + ) + done := make(chan struct{}) + err := fsutil.WithFileLock(changelogLockPath(root), 5*time.Second, func() error { + before, err := os.ReadFile(path) + if err != nil { + return err + } + go func() { + defer close(done) + res, ierr = Ingest(root, liveSurface(), marshalPayload(t, "v0.4.1", goodEntries()), cutAt) + }() + time.Sleep(time.Second) + other := strings.Replace(string(before), "## [Unreleased]\n", + "## [Unreleased]\n\n## [0.4.1] - 2026-07-21\n\n### Fixed\n\n- the other cut. (iss-51)\n", 1) + return os.WriteFile(path, []byte(other), 0o644) + }) + if err != nil { + t.Fatalf("the concurrent cut failed: %v", err) + } + <-done + if ierr != nil { + t.Fatalf("Ingest: %v", ierr) + } + + got := readChangelog(t, root) + if res.Written && !strings.Contains(got, "A version is a fact.") { + t.Fatalf("the ingest reported its section written, and a concurrent cut erased it:\n%s", got) + } + if res.Written { + t.Fatalf("the ingest wrote a second release over a cut already in flight:\n%s", got) + } + if kinds := strings.Join(refusalKinds(res.Cut), ","); !strings.Contains(kinds, "release-in-flight") { + t.Errorf("refusals = %q, want the loser refused as a release in flight", kinds) + } + if _, err := os.Lstat(changelogLockPath(root)); !os.IsNotExist(err) { + t.Errorf("the CHANGELOG lock was left behind in the tree (err=%v)", err) + } +} diff --git a/internal/core/release/ingest.go b/internal/core/release/ingest.go index c32a46764..9c4804990 100644 --- a/internal/core/release/ingest.go +++ b/internal/core/release/ingest.go @@ -304,8 +304,19 @@ type IngestResult struct { // unreadable file, a missing anchor, an // archive collision, a failed write that was // rolled back): stop. +// +// The whole ingest — the derivation, the reads the plan is built from, and the +// writes — holds the CHANGELOG's lock (withChangelogLock), so two cuts of one +// working tree never write from the same stale read: the second derives after +// the first has written, and is refused as a release in flight (iss-127). func Ingest(root string, current surface.Snapshot, raw []byte, at time.Time) (IngestResult, error) { - return ingest(root, current, raw, at, osOps{root: root}) + var res IngestResult + err := withChangelogLock(root, func() error { + var err error + res, err = ingest(root, current, raw, at, osOps{root: root}) + return err + }) + return res, err } // ingest is Ingest with the writer seam exposed, so a test can observe the diff --git a/internal/core/release/lock.go b/internal/core/release/lock.go new file mode 100644 index 000000000..fca79dbd2 --- /dev/null +++ b/internal/core/release/lock.go @@ -0,0 +1,50 @@ +package release + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "time" + + "github.com/intentdriven/abcd/internal/fsutil" +) + +// changelogLockTimeout bounds the wait for another cut of the same tree. The +// lock covers the whole ingest, the derivation's git reads included, so it is +// longer than a single file rewrite's; a var so a test can shorten it. +var changelogLockTimeout = 30 * time.Second + +// changelogLockPath names the lock the cut's writes take: .CHANGELOG.md.lock, +// beside the file the tagging workflow reads. +func changelogLockPath(root string) string { + return filepath.Join(root, "."+changelogFile+".lock") +} + +// withChangelogLock runs fn holding the CHANGELOG's lock. It is taken by every +// writer of the release record — Ingest, whose writes are the archive move, the +// release page and the CHANGELOG section, and UndoPlan.Apply, which puts them +// back — so no two of them interleave in one working tree (iss-127). +// +// The lock is fsutil.WithFileLock, the one inter-process load-modify-write +// primitive. Its file sits in the repository root, beside a committed file, so +// the holder removes it before letting go (WithFileLock's revalidation makes +// that safe for a waiter that opened it first), and a cut leaves nothing behind +// for the dirty-tree gate of the next one to find. It is a leaf: no other lock +// is taken inside it. +func withChangelogLock(root string, fn func() error) error { + lock := changelogLockPath(root) + err := fsutil.WithFileLock(lock, changelogLockTimeout, func() error { + defer os.Remove(lock) + return fn() + }) + switch { + case errors.Is(err, fsutil.ErrLockContention): + return fmt.Errorf("another cut of this tree holds %s (past %s), so nothing was written; let it finish and cut again: %w", + filepath.Base(lock), changelogLockTimeout, err) + case errors.Is(err, fsutil.ErrLockPathUnsafe): + return fmt.Errorf("the lock %s is a symlink or not a regular file, so it is refused and nothing was written; remove it: %w", + filepath.Base(lock), err) + } + return err +} diff --git a/internal/core/release/write.go b/internal/core/release/write.go index db98546df..642313bb3 100644 --- a/internal/core/release/write.go +++ b/internal/core/release/write.go @@ -73,8 +73,18 @@ type UndoPlan struct { // Apply undoes every write the cut made, in reverse, and returns a description // of each undo that failed (empty when the tree is restored). +// +// It holds the CHANGELOG's lock, as the cut's writes did, so the restore is not +// interleaved with another cut's read of the files it puts back. func (u UndoPlan) Apply(root string) []string { - return u.apply(osOps{root: root}, root) + var failures []string + if err := withChangelogLock(root, func() error { + failures = u.apply(osOps{root: root}, root) + return nil + }); err != nil { + failures = append(failures, changelogFile+": "+err.Error()) + } + return failures } func (u UndoPlan) apply(ops fileOps, root string) []string { diff --git a/internal/core/report/inbox.go b/internal/core/report/inbox.go index 7dfa7f234..be2fdb811 100644 --- a/internal/core/report/inbox.go +++ b/internal/core/report/inbox.go @@ -319,6 +319,7 @@ func readEntry(root *os.Root, rel, name, state string) Entry { r, err := parseFiled(data) if err != nil { e.State = StateUnreadable + e.SenderName = EnvelopeSender(data, e.SenderKey) var ve *VersionError var fe *FieldError switch { diff --git a/internal/core/report/inbox_test.go b/internal/core/report/inbox_test.go index cf682d6e0..f4c4a7cb2 100644 --- a/internal/core/report/inbox_test.go +++ b/internal/core/report/inbox_test.go @@ -701,3 +701,62 @@ func TestTheInboxReadersRefuseWhatTheWritersRefuse(t *testing.T) { // second is the error of a two-value call. func second[T any](_ T, err error) error { return err } + +// TestAnUnreadableReportStillNamesItsSender: the envelope is abcd's own +// writing, so a report whose reporter block this abcd cannot read (a later +// template, a malformed field) is still listed with the sender's name when the +// envelope names it under the key the file is filed by (itd-2609221656361680 +// criterion 4, iss-2609240133234244). An envelope naming another key, or a name +// no filing could have written, names nobody. +func TestAnUnreadableReportStillNamesItsSender(t *testing.T) { + home := sandbox(t, time.Date(2026, 9, 23, 11, 0, 0, 0, time.UTC)) + dir := filepath.Join(home, ".abcd", "inbox") + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + key := strings.Repeat("c", 40) + other := strings.Repeat("d", 40) + write := func(stamp, fileKey, body string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, stamp+"-"+fileKey+".md"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + } + // A later template, envelope intact. + write("2609231100001234", key, "---\nschema_version: 7\nsomething_new: yes\nreceived_at: \"2026-09-23T11:00:00Z\"\nsender_key: "+key+"\nsender_name: \"widget-repo\"\n---\n\nprose\n") + // A malformed reporter field, envelope intact. + write("2609231100011234", key, "---\nschema_version: 1\nkind: nonsense\nsender_key: "+key+"\nsender_name: widget-repo\n---\n\nprose\n") + // An envelope naming a key other than the one the file is filed by. + write("2609231100021234", other, "---\nschema_version: 7\nsender_key: "+key+"\nsender_name: widget-repo\n---\n\nprose\n") + // A name no filing writes. + write("2609231100031234", other, "---\nschema_version: 7\nsender_key: "+other+"\nsender_name: \"a/../b\"\n---\n\nprose\n") + + list, err := List() + if err != nil { + t.Fatalf("List: %v", err) + } + want := map[string]string{ + "rpt-2609231100001234": "widget-repo", + "rpt-2609231100011234": "widget-repo", + "rpt-2609231100021234": "", + "rpt-2609231100031234": "", + } + if len(list) != len(want) { + t.Fatalf("List = %+v", list) + } + for _, e := range list { + if e.State != StateUnreadable { + t.Errorf("%s state = %q; want unreadable", e.ID, e.State) + } + if e.SenderName != want[e.ID] { + t.Errorf("%s sender name = %q; want %q", e.ID, e.SenderName, want[e.ID]) + } + } + e, err := Show("rpt-2609231100001234") + if err != nil { + t.Fatalf("Show: %v", err) + } + if e.SenderName != "widget-repo" { + t.Errorf("Show sender name = %q; want the envelope's", e.SenderName) + } +} diff --git a/internal/core/report/report.go b/internal/core/report/report.go index 0e096178f..dceb09d67 100644 --- a/internal/core/report/report.go +++ b/internal/core/report/report.go @@ -523,6 +523,44 @@ func refusePath(key, v string) error { return nil } +// EnvelopeSender reads the sender's name from a filed report's envelope alone, +// for a report the parser refuses (a later template, a malformed reporter +// field): the envelope is abcd's own writing and sits in the same block, so the +// inbox can say who is asking even where it cannot read what they ask. The name +// is returned only when the block names it once, in the shape a filing writes, +// beside a sender_key once that is fileKey, the key the file is filed by; any +// other file names nobody, and "" is returned. +func EnvelopeSender(data []byte, fileKey string) string { + if len(data) > maxFiledBytes || !utf8.Valid(data) { + return "" + } + lines := strings.Split(strings.ReplaceAll(frontmatter.TrimBOM(string(data)), "\r\n", "\n"), "\n") + if len(lines) == 0 || !frontmatter.IsDelimiter(lines[0]) { + return "" + } + seen := map[string][]string{} + for _, line := range lines[1:] { + if frontmatter.IsDelimiter(line) { + break + } + key, value, ok := strings.Cut(line, ":") + if !ok || (key != keySenderKey && key != keySenderName) { + continue + } + v, ok := scalar(&blockLine{value: value}) + if !ok { + return "" + } + seen[key] = append(seen[key], v) + } + keys, names := seen[keySenderKey], seen[keySenderName] + if len(keys) != 1 || len(names) != 1 || keys[0] != fileKey || !senderKeyRe.MatchString(keys[0]) || + !senderNameRe.MatchString(names[0]) { + return "" + } + return names[0] +} + // senderKeyRe is a full root-commit SHA, SHA-1 or SHA-256. var senderKeyRe = regexp.MustCompile(`^(?:[0-9a-f]{40}|[0-9a-f]{64})$`) diff --git a/internal/core/surface/examples.go b/internal/core/surface/examples.go index 0e9d743f3..84b8d2a73 100644 --- a/internal/core/surface/examples.go +++ b/internal/core/surface/examples.go @@ -81,7 +81,8 @@ var examples = map[string]string{ "abcd lab record": "abcd lab record lab-260901000000-0123abc bare-status", "abcd lab sweep": "abcd lab sweep lab-260901000000-0123abc", - "abcd launch archive": "abcd launch archive --out dist", + "abcd launch archive": "abcd launch archive --out dist", + "abcd launch manifests": "abcd launch manifests --tree public", "abcd memory ask": `abcd memory ask "why do record ids carry a timestamp?"`, "abcd memory ingest": "abcd memory ingest https://example.com/paper.pdf", diff --git a/internal/core/surface/sentences.go b/internal/core/surface/sentences.go index 4e62b661f..f195adb51 100644 --- a/internal/core/surface/sentences.go +++ b/internal/core/surface/sentences.go @@ -241,6 +241,8 @@ var sentences = map[string]string{ "Writes only its pre-flight report, to the local tier; refuses without --dry-run.", "abcd launch archive": "Render the release's plugin archive: " + "Writes the archive into --out; refuses a dirty tree without --verify, and exits 1 when --verify finds it unpinned.", + "abcd launch manifests": "Check the release manifests agree on the version, or carry none on a dev tree: " + + "Writes nothing; refuses with exit 1 on drift and exit 2 on an unreadable input.", "abcd launch receipts": "Run the release job's semantic-receipt gate locally, before the merge: " + "Writes nothing; refuses with exit 1 when the release job would refuse the receipts.", "abcd launch scaffold": "Scaffold the release gate for the declared artefact kind: " + diff --git a/internal/fsutil/declaration_test.go b/internal/fsutil/declaration_test.go index 49ae25dcc..e99bdded7 100644 --- a/internal/fsutil/declaration_test.go +++ b/internal/fsutil/declaration_test.go @@ -3,7 +3,6 @@ package fsutil import ( - "errors" "os" "path/filepath" "testing" @@ -20,7 +19,8 @@ func writeDeclaration(t *testing.T, dir, name, body string) string { } // A declaration that passes every guard is read unchanged: the control for the -// swap below, so the refusal there is the swap's and not the fixture's. +// replacements in replaced_test.go, so a refusal there is the replacement's and +// not the fixture's. func TestReadDeclarationReadsTheVettedFile(t *testing.T) { path := writeDeclaration(t, t.TempDir(), "decl", "vetted\n") raw, refusal, err := ReadDeclaration(path, 1024) @@ -31,32 +31,3 @@ func TestReadDeclarationReadsTheVettedFile(t *testing.T) { t.Fatalf("read %q, want the vetted bytes", raw) } } - -// The owner and permission guards judge the file an lstat saw; the bytes come -// from a later open. A file renamed into place between the two — by anyone -// with write on the declaration's directory — was never vetted, so it is -// refused rather than read as the caller's word (iss-2609251537550065). The -// swapped-in file here would pass every guard on its own, so only the identity -// check between the lstat and the opened descriptor can refuse it. -func TestReadDeclarationRefusesAFileSwappedInAfterVetting(t *testing.T) { - dir := t.TempDir() - path := writeDeclaration(t, dir, "decl", "vetted\n") - other := writeDeclaration(t, dir, "other", "swapped\n") - prev := declarationVetted - t.Cleanup(func() { declarationVetted = prev }) - declarationVetted = func(p string) { - if err := os.Rename(other, p); err != nil { - t.Fatalf("swap: %v", err) - } - } - raw, refusal, err := ReadDeclaration(path, 1024) - if refusal == DeclarationOK || err == nil { - t.Fatalf("a file swapped in after vetting must be refused; read %q", raw) - } - if raw != nil { - t.Fatalf("a refused declaration returns no bytes; got %q", raw) - } - if refusal != DeclarationUnreadable || !errors.Is(err, ErrDeclarationSwapped) { - t.Fatalf("the swap must be named: refusal %d, err %v", refusal, err) - } -} diff --git a/internal/fsutil/flush.go b/internal/fsutil/flush.go new file mode 100644 index 000000000..8e43c138c --- /dev/null +++ b/internal/fsutil/flush.go @@ -0,0 +1,41 @@ +package fsutil + +import ( + "os" + "testing" +) + +// SkipFlushEnv names the opt-in a test run sets to skip the flush to stable +// storage. The Makefile's test and preflight targets and the test steps of +// ci.yml's check job set it to "1". +// +// It is honoured only inside a test binary. On macOS, File.Sync is F_FULLFSYNC, +// which costs about 8.8 ms per synced write on a developer disk, and the tests +// write thousands of files while asserting nothing a flush makes true: a flush +// is what survives a power cut, and no test cuts the power. A shipped binary +// flushes whatever its environment says, because testing.Testing reports false +// in every binary the go command builds other than a test binary, so neither +// half of the gate alone disables durability. +const SkipFlushEnv = "ABCD_TEST_SKIP_FLUSH" + +// syncFile is the flush itself: fsync, and F_FULLFSYNC on macOS. It is a +// variable so the durability test can count the flushes a write makes. +var syncFile = (*os.File).Sync + +// Flush makes what was written to f durable — its content for a file, its +// entries for a directory — and returns the flush's error. It is the one call +// every durable write in this module flushes through +// (TestEveryFlushGoesThroughTheGate), so the test-only skip lives in one place. +func Flush(f *os.File) error { + if skipFlush(testing.Testing(), os.Getenv(SkipFlushEnv)) { + return nil + } + return syncFile(f) +} + +// skipFlush is the gate: a flush is skipped only in a test binary whose +// environment opted in with exactly "1". Any other value, or any binary that +// is not a test binary, flushes. +func skipFlush(testBinary bool, optIn string) bool { + return testBinary && optIn == "1" +} diff --git a/internal/fsutil/flush_test.go b/internal/fsutil/flush_test.go new file mode 100644 index 000000000..f689eacf7 --- /dev/null +++ b/internal/fsutil/flush_test.go @@ -0,0 +1,223 @@ +package fsutil + +import ( + "errors" + "os" + "os/exec" + "path/filepath" + "regexp" + "strconv" + "strings" + "testing" +) + +// TestSkipFlushGateTruthTable pins the gate: the flush is skipped only when the +// binary is a test binary AND the opt-in is exactly "1". Neither half alone +// disables durability. +func TestSkipFlushGateTruthTable(t *testing.T) { + for _, c := range []struct { + testBinary bool + optIn string + want bool + }{ + {false, "", false}, + {false, "1", false}, + {false, "true", false}, + {true, "", false}, + {true, "0", false}, + {true, "true", false}, + {true, " 1", false}, + {true, "1", true}, + } { + if got := skipFlush(c.testBinary, c.optIn); got != c.want { + t.Errorf("skipFlush(testBinary=%v, %s=%q) = %v, want %v", c.testBinary, SkipFlushEnv, c.optIn, got, c.want) + } + } +} + +// countFlushes swaps the flush seam for a counter for the rest of the test. +func countFlushes(t *testing.T) *int { + t.Helper() + n := 0 + orig := syncFile + syncFile = func(f *os.File) error { + n++ + return orig(f) + } + t.Cleanup(func() { syncFile = orig }) + return &n +} + +// TestEveryDurableWriteFlushesUnlessTheTestOptedIn is the durability test the +// skip must not reach: with the opt-in cleared explicitly, every durable write +// flushes the file and its parent directory; with it set, this test binary +// skips both. +func TestEveryDurableWriteFlushesUnlessTheTestOptedIn(t *testing.T) { + writes := map[string]func(t *testing.T, dir string) error{ + "WriteFileAtomic": func(t *testing.T, dir string) error { + return WriteFileAtomic(filepath.Join(dir, "a"), []byte("x"), 0o644) + }, + "WriteFileAtomicInRoot": func(t *testing.T, dir string) error { + root, err := os.OpenRoot(dir) + if err != nil { + t.Fatal(err) + } + defer root.Close() + return WriteFileAtomicInRoot(root, "a", []byte("x"), 0o644) + }, + "CreateExclusiveIn": func(t *testing.T, dir string) error { + root, err := os.OpenRoot(dir) + if err != nil { + t.Fatal(err) + } + defer root.Close() + return CreateExclusiveIn(root, "a", []byte("x"), 0o644) + }, + } + for name, write := range writes { + for _, c := range []struct { + optIn string + want int + }{{"", 2}, {"1", 0}} { + t.Run(name+"/"+SkipFlushEnv+"="+c.optIn, func(t *testing.T) { + t.Setenv(SkipFlushEnv, c.optIn) + n := countFlushes(t) + if err := write(t, t.TempDir()); err != nil { + t.Fatalf("%s: %v", name, err) + } + if *n != c.want { + t.Errorf("%s with %s=%q flushed %d time(s), want %d (the file and its parent directory, or none)", + name, SkipFlushEnv, c.optIn, *n, c.want) + } + }) + } + } +} + +// flushOutcome reports whether Flush reached the flush, observed by its error: +// a flush of a closed file fails with os.ErrClosed, a skipped one returns nil. +func flushOutcome(t *testing.T) string { + t.Helper() + f, err := os.CreateTemp(t.TempDir(), "flush-*") + if err != nil { + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } + switch err := Flush(f); { + case errors.Is(err, os.ErrClosed): + return "flushed" + case err == nil: + return "skipped" + default: + t.Fatalf("Flush: %v", err) + return "" + } +} + +// TestAShippedBinaryFlushesWithTheOptInSet proves the opt-in cannot reach a +// shipped binary: the probe under testdata is built with go build — a binary +// like the released one, not a test binary — and run with the opt-in set, and +// it still flushes. The same observation inside this test binary skips, so the +// difference is testing.Testing and nothing else. +func TestAShippedBinaryFlushesWithTheOptInSet(t *testing.T) { + t.Setenv(SkipFlushEnv, "1") + if got := flushOutcome(t); got != "skipped" { + t.Fatalf("in this test binary with %s=1, Flush %s; want skipped", SkipFlushEnv, got) + } + + if _, err := exec.LookPath("go"); err != nil { + t.Skip("go unavailable") + } + bin := filepath.Join(t.TempDir(), "flushprobe") + build := exec.Command("go", "build", "-o", bin, "./testdata/flushprobe") + if out, err := build.CombinedOutput(); err != nil { + t.Fatalf("go build ./testdata/flushprobe: %v\n%s", err, out) + } + run := exec.Command(bin) + run.Env = append(os.Environ(), SkipFlushEnv+"=1") + out, err := run.Output() + if err != nil { + t.Fatalf("flushprobe: %v\n%s", err, out) + } + if got := strings.TrimSpace(string(out)); got != "flushed" { + t.Fatalf("a go-built binary with %s=1 in its environment reports %q; want flushed, "+ + "because the opt-in must never disable durability outside a test binary", SkipFlushEnv, got) + } +} + +// flushCallRe matches a direct flush: a Sync() method call, an fsync or +// fdatasync wrapper named anywhere (called, or taken as a value to call later), +// the raw fsync or fdatasync syscall number, or the macOS full-flush fcntl. +var flushCallRe = regexp.MustCompile(`\.Sync\(\)|\bFsync\b|\bFdatasync\b|\bSYS_F(?:DATA)?SYNC\b|F_FULLFSYNC`) + +// TestFlushCallReNamesEveryDirectFlush pins the forms the walk below refuses, +// the spellings a first version of the pattern let through among them: a +// datasync, the raw fsync syscall number and an fsync taken as a function value +// and called under another name. Each would flush in every test run with the +// walk still green. +func TestFlushCallReNamesEveryDirectFlush(t *testing.T) { + for _, line := range []string{ + `if err := f.Sync(); err != nil {`, + `return unix.Fsync(fd)`, + `return unix.Fdatasync(fd)`, + `return syscall.Fdatasync(int(f.Fd()))`, + `_, _, e := syscall.Syscall(syscall.SYS_FSYNC, f.Fd(), 0, 0)`, + `_, _, e := unix.Syscall(unix.SYS_FDATASYNC, f.Fd(), 0, 0)`, + `fs := unix.Fsync`, + `_, err := unix.FcntlInt(f.Fd(), unix.F_FULLFSYNC, 0)`, + } { + if !flushCallRe.MatchString(line) { + t.Errorf("flushCallRe misses a direct flush: %s", line) + } + } + for _, line := range []string{ + `return Flush(f)`, + `return syncFile(f)`, + `var mu sync.Mutex`, + `flags |= os.O_SYNC`, + `fsyncCount++`, + `FsyncCount++`, + } { + if flushCallRe.MatchString(line) { + t.Errorf("flushCallRe refuses a line that does not flush: %s", line) + } + } +} + +// TestEveryFlushGoesThroughTheGate holds the gate to one place: no non-test Go +// file in this module flushes except through Flush, whose only Sync is the +// method value in syncFile. A direct call elsewhere would flush in every test +// run, and a copy of the gate would be a second thing to keep out of shipped +// binaries. +func TestEveryFlushGoesThroughTheGate(t *testing.T) { + root := filepath.Join("..", "..") + var offenders []string + for _, top := range []string{"internal", "cmd"} { + err := filepath.WalkDir(filepath.Join(root, top), func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() || !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + data, err := os.ReadFile(path) + if err != nil { + return err + } + for i, line := range strings.Split(string(data), "\n") { + if !strings.HasPrefix(strings.TrimSpace(line), "//") && flushCallRe.MatchString(line) { + offenders = append(offenders, filepath.ToSlash(path)+":"+strconv.Itoa(i+1)+": "+strings.TrimSpace(line)) + } + } + return nil + }) + if err != nil { + t.Fatalf("walk %s: %v", top, err) + } + } + if len(offenders) > 0 { + t.Fatalf("flushes that bypass fsutil.Flush (route them through it):\n %s", strings.Join(offenders, "\n ")) + } +} diff --git a/internal/fsutil/fsutil.go b/internal/fsutil/fsutil.go index ca18dd1fc..c181c5f46 100644 --- a/internal/fsutil/fsutil.go +++ b/internal/fsutil/fsutil.go @@ -15,6 +15,9 @@ // the transcript store all call them rather than each carrying the sequence // (iss-2609091128479544). // +// Every flush to stable storage goes through Flush, the one place a test binary +// that opted in with SkipFlushEnv skips it; a shipped binary always flushes. +// // An append-only log has its own primitive, AppendLineIn: one line, one // O_APPEND write, so concurrent writers land whole lines (the lifeboat voyage // ledger and the implement run log both write through it). @@ -189,9 +192,28 @@ func SwapOwnerUIDForTest(fn func(string) (uint32, error)) (restore func()) { // declarationVetted runs between ReadDeclaration's vetting lstat and its open. // It does nothing in production; it is a var so a detector can rename a // different file into place inside that window, which a real race cannot be -// relied on to hit, and so prove the read refuses what it did not vet. +// relied on to hit, and so prove the read refuses what it did not vet and +// re-vets what replaced it. var declarationVetted = func(string) {} +// inRootVetted is declarationVetted for ReadGuardedInRoot: it runs between the +// vetting lstat and the open, does nothing in production, and exists for the +// same detectors. +var inRootVetted = func(*os.Root, string) {} + +// declarationAttempts bounds how many times ReadDeclaration and +// ReadGuardedInRoot vet a path whose file was replaced between the vetting and +// the open before they refuse it. A replacement is not refused on sight +// because the ordinary one is benign: another abcd process of the same user +// rewriting the file through WriteFileAtomic, a temp file renamed over it, +// which lands inside that window often enough on a loaded machine to make one +// of two concurrent abcd processes refuse its own configuration +// (iss-2609290518278152). Each attempt judges the file then at the path from +// scratch, so a replacement that fails a guard is refused by that guard; the +// bound only stops a replacement that never settles from holding the read +// forever, and one that outlasts it is still refused, as the swap it is. +const declarationAttempts = 8 + // ReadDeclaration is the guarded read for a HOME-SCOPED DECLARATION FILE — a // record in the caller's own home that re-admits something abcd would otherwise // refuse (~/.abcd/trusted-roots re-admits a marker root, ~/.abcd/path-entry @@ -216,34 +238,50 @@ var declarationVetted = func(string) {} // is judged as itself rather than through its target; the read then re-opens // with O_NOFOLLOW, re-validates on its own descriptor, and confirms with // os.SameFile that the descriptor is the file the lstat vetted, so the -// lstat→open window can promote neither a swapped-in symlink nor a swapped-in -// regular file into a read (a replacement is DeclarationUnreadable with -// ErrDeclarationSwapped). +// lstat→open window can promote no file into a read that the guards did not +// judge. The bytes returned are always those of a descriptor that is the very +// file an Lstat passed through every guard. +// +// A file renamed into place inside that window is not read and not refused on +// sight: the whole judgement runs again on whatever the path names now, up to +// declarationAttempts times. A same-owner, owner-only-writable regular file — +// the rewrite a concurrent abcd makes through WriteFileAtomic — passes and is +// read; a symlink, FIFO, device or directory, a file writable by group or +// other, and a file another uid owns are each refused by the guard that judges +// them, exactly as they would be had they been there before the first Lstat. A +// replacement still unsettled after the last attempt is DeclarationUnreadable +// with ErrDeclarationSwapped. // // The returned error is ALWAYS non-nil when the refusal is not DeclarationOK, so // a caller that inspects only the error still fails closed. Callers that need to // say WHICH guard refused — and to keep "absent" silent while reporting // "unreadable" — switch on the refusal instead. func ReadDeclaration(path string, limit int64) ([]byte, DeclarationRefusal, error) { - fi, err := os.Lstat(path) - if err != nil { - return nil, DeclarationAbsent, err - } - if !fi.Mode().IsRegular() { - return nil, DeclarationNotRegular, ErrNotRegular - } - if err := CallersAlone(path, fi); err != nil { - if errors.Is(err, ErrDeclarationWritable) { - return nil, DeclarationWritableByOthers, err + for attempt := 1; ; attempt++ { + fi, err := os.Lstat(path) + if err != nil { + return nil, DeclarationAbsent, err } - return nil, DeclarationForeignOwner, err - } - declarationVetted(path) - raw, err := readGuarded(path, limit, fi) - if err != nil { - return nil, DeclarationUnreadable, err + if !fi.Mode().IsRegular() { + return nil, DeclarationNotRegular, ErrNotRegular + } + if err := CallersAlone(path, fi); err != nil { + if errors.Is(err, ErrDeclarationWritable) { + return nil, DeclarationWritableByOthers, err + } + return nil, DeclarationForeignOwner, err + } + declarationVetted(path) + raw, err := readGuarded(path, limit, fi) + if errors.Is(err, ErrDeclarationSwapped) && attempt < declarationAttempts { + // Replaced after the vetting: judge the replacement from scratch. + continue + } + if err != nil { + return nil, DeclarationUnreadable, err + } + return raw, DeclarationOK, nil } - return raw, DeclarationOK, nil } // CallersAlone is the half of ReadDeclaration's judgement that makes a path the @@ -281,7 +319,10 @@ func CallersAlone(path string, fi os.FileInfo) error { // // It keeps every guarantee ReadGuarded gives at the leaf: a symlinked leaf is // refused (lstat, never followed), the descriptor is confirmed to be the very -// file that was vetted (os.SameFile closes the lstat→open swap), a non-regular +// file that was vetted (os.SameFile closes the lstat→open swap; a replacement +// is vetted again from scratch, up to declarationAttempts times, so the benign +// rewrite a concurrent WriteFileAtomicInRoot makes is read and anything that +// is not a regular file is still refused), a non-regular // leaf returns ErrNotRegular, O_NONBLOCK stops a FIFO or device blocking the // open, and the caller's byte cap is enforced against both the fstat size and // the bytes actually read. @@ -291,6 +332,27 @@ func CallersAlone(path string, fi os.FileInfo) error { // a caller skipping absent files fails closed on the escape rather than treating // it as "the file is not there". func ReadGuardedInRoot(root *os.Root, rel string, limit int64) ([]byte, error) { + for attempt := 1; ; attempt++ { + data, err := readGuardedInRootOnce(root, rel, limit) + if errors.Is(err, errReplacedInRoot) { + if attempt < declarationAttempts { + // Replaced after the vetting: judge the replacement from scratch. + continue + } + return nil, ErrNotRegular + } + return data, err + } +} + +// errReplacedInRoot is readGuardedInRootOnce's word for a descriptor that is +// not the file its lstat vetted. It never leaves the package: ReadGuardedInRoot +// re-vets on it and, once declarationAttempts is spent, refuses with +// ErrNotRegular, the sentinel its callers already refuse on. +var errReplacedInRoot = errors.New("fsutil: file was replaced between its vetting and its read") + +// readGuardedInRootOnce is one vetting and one read of ReadGuardedInRoot. +func readGuardedInRootOnce(root *os.Root, rel string, limit int64) ([]byte, error) { fi, err := root.Lstat(rel) if err != nil { return nil, err @@ -301,6 +363,7 @@ func ReadGuardedInRoot(root *os.Root, rel string, limit int64) ([]byte, error) { if !fi.Mode().IsRegular() { return nil, ErrNotRegular } + inRootVetted(root, rel) f, err := root.OpenFile(rel, os.O_RDONLY|syscall.O_NONBLOCK, 0) if err != nil { return nil, err @@ -316,8 +379,8 @@ func ReadGuardedInRoot(root *os.Root, rel string, limit int64) ([]byte, error) { if !os.SameFile(fi, st) { // Swapped between the lstat and the open. os.Root already stops the // swap escaping the root, but the descriptor is no longer the file that - // was vetted, so it is refused rather than read. - return nil, ErrNotRegular + // was vetted, so it is not read; the caller vets what replaced it. + return nil, errReplacedInRoot } if st.Size() > limit { return nil, ErrTooBig @@ -366,7 +429,7 @@ func WriteFileAtomic(path string, data []byte, perm os.FileMode) error { os.Remove(tmpName) return err } - if err := tmp.Sync(); err != nil { + if err := Flush(tmp); err != nil { tmp.Close() os.Remove(tmpName) return err @@ -436,7 +499,7 @@ func WriteFileAtomicInRoot(root *os.Root, rel string, data []byte, perm os.FileM if err := tmp.Chmod(perm); err != nil { return abandon(err) } - if err := tmp.Sync(); err != nil { + if err := Flush(tmp); err != nil { return abandon(err) } if err := tmp.Close(); err != nil { @@ -504,7 +567,7 @@ func syncDirInRoot(root *os.Root, dir string) { if err != nil { return } - _ = d.Sync() + _ = Flush(d) _ = d.Close() } @@ -515,7 +578,7 @@ func syncParent(dir string) { if err != nil { return } - _ = d.Sync() + _ = Flush(d) _ = d.Close() } @@ -662,7 +725,7 @@ func CreateExclusiveIn(root *os.Root, rel string, data []byte, perm os.FileMode) _ = root.Remove(rel) return err } - if err := f.Sync(); err != nil { + if err := Flush(f); err != nil { f.Close() _ = root.Remove(rel) return err @@ -678,7 +741,7 @@ func CreateExclusiveIn(root *os.Root, rel string, data []byte, perm os.FileMode) // the exact state such a caller's rollback exists to prevent. Best-effort: // some filesystems refuse a directory fsync. if d, err := root.Open(path.Dir(rel)); err == nil { - _ = d.Sync() + _ = Flush(d) _ = d.Close() } return nil diff --git a/internal/fsutil/replaced_test.go b/internal/fsutil/replaced_test.go new file mode 100644 index 000000000..fbdc547af --- /dev/null +++ b/internal/fsutil/replaced_test.go @@ -0,0 +1,260 @@ +//go:build unix + +package fsutil + +import ( + "errors" + "os" + "path/filepath" + "syscall" + "testing" +) + +// swapIn renames staged over dst. rename(2) cannot replace a file with a +// directory, so a directory is moved in after the file is removed; every other +// kind replaces it atomically, as a real rename-swap would. +func swapIn(t *testing.T, staged, dst string) { + t.Helper() + if fi, err := os.Lstat(staged); err == nil && fi.IsDir() { + if err := os.Remove(dst); err != nil { + t.Fatalf("swap: %v", err) + } + } + if err := os.Rename(staged, dst); err != nil { + t.Fatalf("swap: %v", err) + } +} + +// The benign replacement: another abcd process of the same user rewrites the +// declaration through WriteFileAtomic — a temp file renamed over it — inside +// the window between this read's vetting lstat and its open. The new file +// passes every guard the old one passed, so it is the caller's word as much as +// the old one was; refusing it made one of two concurrent abcd processes +// refuse its own configuration (iss-2609290518278152). It is re-vetted and +// read, and the bytes are the new file's. +func TestReadDeclarationReadsABenignReplacementAfterRevetting(t *testing.T) { + dir := t.TempDir() + path := writeDeclaration(t, dir, "decl", "before\n") + prev := declarationVetted + t.Cleanup(func() { declarationVetted = prev }) + calls := 0 + declarationVetted = func(p string) { + calls++ + if calls == 1 { + if err := WriteFileAtomic(p, []byte("after\n"), 0o600); err != nil { + t.Fatalf("replace: %v", err) + } + } + } + raw, refusal, err := ReadDeclaration(path, 1024) + if err != nil || refusal != DeclarationOK { + t.Fatalf("a same-owner regular file renamed into place must be re-vetted and read: refusal %d, err %v", refusal, err) + } + if string(raw) != "after\n" { + t.Fatalf("read %q, want the replacement's bytes", raw) + } + if calls != 2 { + t.Fatalf("the replacement must be vetted before it is read: %d vetting(s), want 2", calls) + } +} + +// A replacement that keeps happening is not waited out forever: after +// declarationAttempts vettings that each lost the race the read refuses, named +// as the swap it is. +func TestReadDeclarationRefusesAnEndlessReplacement(t *testing.T) { + dir := t.TempDir() + path := writeDeclaration(t, dir, "decl", "before\n") + prev := declarationVetted + t.Cleanup(func() { declarationVetted = prev }) + calls := 0 + declarationVetted = func(p string) { + calls++ + if err := WriteFileAtomic(p, []byte("again\n"), 0o600); err != nil { + t.Fatalf("replace: %v", err) + } + } + raw, refusal, err := ReadDeclaration(path, 1024) + if raw != nil || refusal != DeclarationUnreadable || !errors.Is(err, ErrDeclarationSwapped) { + t.Fatalf("an endless replacement must be refused as a swap: raw %q, refusal %d, err %v", raw, refusal, err) + } + if calls != declarationAttempts { + t.Fatalf("%d vetting(s), want the bound %d", calls, declarationAttempts) + } +} + +// What the re-vetting still refuses: a replacement that is not a same-owner, +// owner-only-writable regular file. Each is renamed into place once, after the +// first vetting, so the only thing that can refuse it is the judgement of the +// replacement itself — the retry must never promote it into a read. +func TestReadDeclarationRefusesANonBenignReplacement(t *testing.T) { + cases := []struct { + name string + // plant creates the replacement at dst (a sibling of the declaration). + plant func(t *testing.T, dir, dst string) + // foreign makes the owner lookup report another uid once swapped. + foreign bool + want DeclarationRefusal + }{ + {name: "symlink to an owned file", plant: func(t *testing.T, dir, dst string) { + target := writeDeclaration(t, dir, "target", "linked\n") + if err := os.Symlink(target, dst); err != nil { + t.Fatal(err) + } + }}, + {name: "fifo", plant: func(t *testing.T, _, dst string) { + if err := syscall.Mkfifo(dst, 0o600); err != nil { + t.Fatal(err) + } + }}, + {name: "directory", plant: func(t *testing.T, _, dst string) { + if err := os.Mkdir(dst, 0o700); err != nil { + t.Fatal(err) + } + }}, + {name: "group-writable file", want: DeclarationWritableByOthers, plant: func(t *testing.T, dir, dst string) { + writeDeclaration(t, dir, filepath.Base(dst), "writable\n") + if err := os.Chmod(dst, 0o664); err != nil { + t.Fatal(err) + } + }}, + {name: "foreign-owned file", foreign: true, want: DeclarationForeignOwner, plant: func(t *testing.T, dir, dst string) { + writeDeclaration(t, dir, filepath.Base(dst), "foreign\n") + }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + path := writeDeclaration(t, dir, "decl", "vetted\n") + staged := filepath.Join(dir, "staged") + tc.plant(t, dir, staged) + swapped := false + restore := SwapOwnerUIDForTest(func(p string) (uint32, error) { + if tc.foreign && swapped { + return uint32(os.Getuid()) + 1, nil + } + return OwnerUID(p) + }) + t.Cleanup(restore) + prev := declarationVetted + t.Cleanup(func() { declarationVetted = prev }) + declarationVetted = func(p string) { + if swapped { + return + } + swapped = true + swapIn(t, staged, p) + } + raw, refusal, err := ReadDeclaration(path, 1024) + if refusal == DeclarationOK || err == nil || raw != nil { + t.Fatalf("a %s swapped in after vetting must be refused: raw %q, refusal %d, err %v", tc.name, raw, refusal, err) + } + if tc.want != 0 && refusal != tc.want { + t.Fatalf("a %s must be refused by the guard that judges it: refusal %d, want %d (err %v)", tc.name, refusal, tc.want, err) + } + }) + } +} + +// ReadGuardedInRoot closes the same lstat->open window, for files inside a +// checkout, and loses the same race to the same benign rewrite +// (WriteFileAtomicInRoot by a concurrent abcd). The replacement is re-vetted +// and read. +func TestReadGuardedInRootReadsABenignReplacementAfterRevetting(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "f.json"), []byte("before"), 0o644); err != nil { + t.Fatal(err) + } + r := openFixtureRoot(t, dir) + prev := inRootVetted + t.Cleanup(func() { inRootVetted = prev }) + calls := 0 + inRootVetted = func(root *os.Root, rel string) { + calls++ + if calls == 1 { + if err := WriteFileAtomicInRoot(root, rel, []byte("after"), 0o644); err != nil { + t.Fatalf("replace: %v", err) + } + } + } + got, err := ReadGuardedInRoot(r, "f.json", 1<<20) + if err != nil { + t.Fatalf("a regular file renamed into place must be re-vetted and read: %v", err) + } + if string(got) != "after" || calls != 2 { + t.Fatalf("read %q after %d vetting(s), want the replacement's bytes after 2", got, calls) + } +} + +// What ReadGuardedInRoot's re-vetting still refuses: a leaf that is not a +// regular file, swapped in after the first vetting, and a replacement that +// never stops. +func TestReadGuardedInRootRefusesANonBenignReplacement(t *testing.T) { + cases := map[string]func(t *testing.T, dir, dst string){ + "symlink to a contained file": func(t *testing.T, dir, dst string) { + if err := os.WriteFile(filepath.Join(dir, "target"), []byte("linked"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Symlink("target", dst); err != nil { + t.Fatal(err) + } + }, + "fifo": func(t *testing.T, _, dst string) { + if err := syscall.Mkfifo(dst, 0o600); err != nil { + t.Fatal(err) + } + }, + "directory": func(t *testing.T, _, dst string) { + if err := os.Mkdir(dst, 0o700); err != nil { + t.Fatal(err) + } + }, + } + for name, plant := range cases { + t.Run(name, func(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "f.json"), []byte("vetted"), 0o644); err != nil { + t.Fatal(err) + } + staged := filepath.Join(dir, "staged") + plant(t, dir, staged) + r := openFixtureRoot(t, dir) + prev := inRootVetted + t.Cleanup(func() { inRootVetted = prev }) + swapped := false + inRootVetted = func(_ *os.Root, _ string) { + if swapped { + return + } + swapped = true + swapIn(t, staged, filepath.Join(dir, "f.json")) + } + got, err := ReadGuardedInRoot(r, "f.json", 1<<20) + if !errors.Is(err, ErrNotRegular) || got != nil { + t.Fatalf("a %s swapped in after vetting must be refused as not regular: read %q, err %v", name, got, err) + } + }) + } + t.Run("endless replacement", func(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "f.json"), []byte("vetted"), 0o644); err != nil { + t.Fatal(err) + } + r := openFixtureRoot(t, dir) + prev := inRootVetted + t.Cleanup(func() { inRootVetted = prev }) + calls := 0 + inRootVetted = func(root *os.Root, rel string) { + calls++ + if err := WriteFileAtomicInRoot(root, rel, []byte("again"), 0o644); err != nil { + t.Fatalf("replace: %v", err) + } + } + got, err := ReadGuardedInRoot(r, "f.json", 1<<20) + if !errors.Is(err, ErrNotRegular) || got != nil { + t.Fatalf("an endless replacement must be refused: read %q, err %v", got, err) + } + if calls != declarationAttempts { + t.Fatalf("%d vetting(s), want the bound %d", calls, declarationAttempts) + } + }) +} diff --git a/internal/fsutil/testdata/flushprobe/main.go b/internal/fsutil/testdata/flushprobe/main.go new file mode 100644 index 000000000..145832d21 --- /dev/null +++ b/internal/fsutil/testdata/flushprobe/main.go @@ -0,0 +1,39 @@ +// Command flushprobe is a non-test binary that reports whether fsutil.Flush +// reaches the flush. TestAShippedBinaryFlushesWithTheOptInSet builds it with +// go build and runs it with the test opt-in set, because a shipped binary is +// the one place the opt-in must do nothing. +// +// The flush is observed by its error: Flush on a closed file returns +// os.ErrClosed when it calls File.Sync, and nil when the gate skipped the call. +package main + +import ( + "errors" + "fmt" + "os" + + "github.com/intentdriven/abcd/internal/fsutil" +) + +func main() { + f, err := os.CreateTemp("", "flushprobe-*") + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(2) + } + closeErr := f.Close() + _ = os.Remove(f.Name()) + if closeErr != nil { + fmt.Fprintln(os.Stderr, closeErr) + os.Exit(2) + } + switch err := fsutil.Flush(f); { + case errors.Is(err, os.ErrClosed): + fmt.Println("flushed") + case err == nil: + fmt.Println("skipped") + default: + fmt.Fprintln(os.Stderr, err) + os.Exit(2) + } +} diff --git a/internal/surface/cli/cli.go b/internal/surface/cli/cli.go index 33b9a9a7d..c265a884f 100644 --- a/internal/surface/cli/cli.go +++ b/internal/surface/cli/cli.go @@ -488,6 +488,10 @@ func NewRootCommand() *cobra.Command { // `receipts` runs the release job's semantic-receipt gate locally, before // the merge, through the same reader the job runs (itd-93 AC7). launchCmd.AddCommand(newLaunchReceiptsCommand(&asJSON)) + // `manifests` is the manifest lockstep checker's own front door (itd-69): + // the check the preview, the cut and the render run, over a named tree at + // the polarity the caller chooses. + launchCmd.AddCommand(newLaunchManifestsCommand(&asJSON)) // `smoke-pages` is the deep installability tier's child process (itd-66): // hidden and operator-internal, re-executed by the preview and the cut. launchCmd.AddCommand(newLaunchSmokePagesCommand()) diff --git a/internal/surface/cli/implement.go b/internal/surface/cli/implement.go index 9b47878f0..8e0cd5df4 100644 --- a/internal/surface/cli/implement.go +++ b/internal/surface/cli/implement.go @@ -360,7 +360,9 @@ func newImplementClaimCommand(asJSON *bool) *cobra.Command { "already holds renews the lease. A claim whose lease has passed is claimable again,\n" + "and the lapse is logged as claim_lapsed. A record another session holds is refused\n" + "at exit 3 and logged as claim_denied naming the holder; the second session also\n" + - "logs a backoff.\n\n" + + "logs a backoff with its reason and the minutes the attempt spent. A run state\n" + + "locked by another session's change is exit 3 too, and the second session's\n" + + "backoff from it is logged the same way.\n\n" + "The second session is refused (exit 2, logged as a refusal) when it already holds\n" + "a live claim, when the window is split-roles, or when a --path it declares is in the\n" + "reading corpus.", @@ -493,7 +495,8 @@ func newImplementLogCommand(asJSON *bool) *cobra.Command { "The claim, window and session events are written by their own sub-verbs and are\n" + "refused here, so the log cannot record a claim the run state does not hold.\n\n" + "An event missing a field the report reads is refused, naming it: " + requiredFieldsHelp() + ".\n" + - "An intervention's kind is one of " + strings.Join(implement.InterventionKinds, ", ") + "; an at or\n" + + "A backoff names its reason and the minutes it spent (reason=, minutes=),\n" + + "or it is refused. An intervention's kind is one of " + strings.Join(implement.InterventionKinds, ", ") + "; an at or\n" + "last_productive is an RFC 3339 time, and a *_min or minutes field a number. An agent_start\n" + "that would take a session past the ceiling it joined with is refused, and the refusal logged.", Args: cobra.ExactArgs(1), diff --git a/internal/surface/cli/launch_manifests.go b/internal/surface/cli/launch_manifests.go new file mode 100644 index 000000000..151bf1426 --- /dev/null +++ b/internal/surface/cli/launch_manifests.go @@ -0,0 +1,84 @@ +package cli + +import ( + "fmt" + "io" + "os" + "path/filepath" + + "github.com/spf13/cobra" + + "github.com/intentdriven/abcd/internal/core/launch" + "github.com/intentdriven/abcd/internal/termsafe" +) + +// newLaunchLockstepCommand builds `abcd launch manifests`, the manifest lockstep +// checker's front door (itd-69): the check the preview and the cut run at the +// dev polarity and the payload render runs at the public one, over a tree the +// caller names, so a public checkout (a marketplace install, a release source +// archive) is checkable too. It reads and never writes, and it has no flag that +// waives a finding: manifest consistency cannot be waved through at its own +// layer. +// +// Exit codes: 0 consistent; 1 drift, one line per field; 2 an input that +// cannot be read (the version-location contract, a manifest, the artefact +// declaration) or an operand it does not know. +func newLaunchManifestsCommand(asJSON *bool) *cobra.Command { + var tree, root string + cmd := &cobra.Command{ + Use: "manifests --tree public|dev [--root ]", + Long: "Run the manifest lockstep check over a tree. --tree public requires the\n" + + "version-location primary present as strict SemVer and every pinned secondary\n" + + "to agree with it; --tree dev requires every version key absent (adr-19). The\n" + + "tree is the working directory, or --root. Exit 0 consistent, 1 drift (one\n" + + "line per field), 2 unreadable. Nothing is written.", + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + if tree == "" { + return &exitError{Code: 2, Msg: "abcd launch manifests: name the polarity with --tree public or --tree dev"} + } + t, err := launch.ParseLockstepTree(tree) + if err != nil { + return &exitError{Code: 2, Msg: "abcd launch manifests: " + err.Error()} + } + dir := root + if dir == "" { + if dir, err = os.Getwd(); err != nil { + return &exitError{Code: 2, Msg: "abcd launch manifests: " + scrubPaths(err)} + } + } + if dir, err = filepath.Abs(dir); err != nil { + return &exitError{Code: 2, Msg: "abcd launch manifests: " + scrubPaths(err)} + } + res := launch.CheckTree(t, dir) + if rerr := render(cmd.OutOrStdout(), *asJSON, res, func(w io.Writer) { + renderLockstep(w, res) + }); rerr != nil { + return rerr + } + if res.ExitCode != 0 { + return &exitError{Code: res.ExitCode} + } + return nil + }, + } + cmd.Flags().StringVar(&tree, "tree", "", "the polarity to check: public (versions present and agreeing) or dev (versions absent)") + cmd.Flags().StringVar(&root, "root", "", "the tree to check (default: the working directory)") + return cmd +} + +// renderLockstep prints one lockstep result for a person. Drift lines and the +// unreadable detail quote manifest values, so each is sanitised. +func renderLockstep(w io.Writer, res launch.LockstepResult) { + switch { + case res.Unreadable: + fmt.Fprintf(w, "abcd launch manifests — tree %s: UNREADABLE\n %s\n", res.Tree, termsafe.Sanitize(scrubMessage(res.Detail))) + case len(res.Drifts) > 0: + fmt.Fprintf(w, "abcd launch manifests — tree %s: DRIFT\n", res.Tree) + for _, d := range res.Drifts { + fmt.Fprintf(w, " %s\n", termsafe.Sanitize(scrubMessage(d))) + } + default: + fmt.Fprintf(w, "abcd launch manifests — tree %s: consistent\n", res.Tree) + } +} diff --git a/internal/surface/cli/launch_manifests_test.go b/internal/surface/cli/launch_manifests_test.go new file mode 100644 index 000000000..df1be3b27 --- /dev/null +++ b/internal/surface/cli/launch_manifests_test.go @@ -0,0 +1,114 @@ +package cli + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// lockstepTree lays a version-location contract, a primary plugin.json and a +// marketplace.json under a fresh root; an empty version omits the key. +func lockstepTree(t *testing.T, primary, market string) string { + t.Helper() + root := t.TempDir() + write := func(rel, body string) { + t.Helper() + p := filepath.Join(root, filepath.FromSlash(rel)) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + write(".abcd/config/version-location.json", `{"manifest_path": ".claude-plugin/plugin.json", "json_pointer": "/version"}`) + pj := `{"name": "abcd"` + if primary != "" { + pj += `, "version": "` + primary + `"` + } + write(".claude-plugin/plugin.json", pj+"}") + mk := `{"plugins": [{"name": "abcd", "source": "./"` + if market != "" { + mk += `, "version": "` + market + `", "changelog": {"version": "` + market + `"}` + } + write(".claude-plugin/marketplace.json", mk+"}]}") + return root +} + +// TestLaunchManifestsRunsTheChosenTreeOverANamedRoot: itd-69's first two +// criteria promise a checker run with `--tree public` and `--tree dev`, and a +// public checkout (a marketplace install, a release source archive) must be +// checkable from the CLI, not only by the payload render (iss-2609261423222935). +// Exit 0 consistent, 1 drift with per-field lines, 2 unreadable; no bypass flag. +func TestLaunchManifestsRunsTheChosenTreeOverANamedRoot(t *testing.T) { + cases := []struct { + name, tree, primary, market string + code int + drift bool + }{ + {"public agreement", "public", "1.2.3", "1.2.3", 0, false}, + {"public disagreement", "public", "1.2.3", "1.2.4", 1, true}, + {"dev with a version key", "dev", "1.2.3", "", 1, true}, + {"dev with the keys absent", "dev", "", "", 0, false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + root := lockstepTree(t, c.primary, c.market) + out, err := runCLIErr(t, "launch", "manifests", "--tree", c.tree, "--root", root, "--json") + if got := exitCodeOf(err); got != c.code { + t.Fatalf("exit = %d (%v); want %d\n%s", got, err, c.code, out) + } + var res struct { + Tree string `json:"tree"` + OK bool `json:"ok"` + Drifts []string `json:"drifts"` + ExitCode int `json:"exit_code"` + } + if err := json.Unmarshal(out, &res); err != nil { + t.Fatalf("output is not the result: %v\n%s", err, out) + } + if res.Tree != c.tree || res.ExitCode != c.code || (len(res.Drifts) > 0) != c.drift { + t.Fatalf("result = %+v", res) + } + }) + } + + t.Run("unreadable contract", func(t *testing.T) { + root := t.TempDir() + out, err := runCLIErr(t, "launch", "manifests", "--tree", "public", "--root", root) + if got := exitCodeOf(err); got != 2 { + t.Fatalf("exit = %d (%v); want 2\n%s", got, err, out) + } + }) + + t.Run("no tree is refused", func(t *testing.T) { + _, err := runCLIErr(t, "launch", "manifests", "--root", t.TempDir()) + if got := exitCodeOf(err); got != 2 { + t.Fatalf("exit = %d (%v); want 2", got, err) + } + }) + + t.Run("a tree it does not know is refused", func(t *testing.T) { + _, err := runCLIErr(t, "launch", "manifests", "--tree", "staging", "--root", t.TempDir()) + if got := exitCodeOf(err); got != 2 { + t.Fatalf("exit = %d (%v); want 2", got, err) + } + }) + + t.Run("no bypass flag", func(t *testing.T) { + cmd, _, err := NewRootCommand().Find([]string{"launch", "manifests"}) + if err != nil || cmd.Name() != "manifests" { + t.Fatalf("no manifests command: %v", err) + } + for _, name := range []string{"allow-dirty", "skip", "force", "dirty", "no-verify"} { + if cmd.Flags().Lookup(name) != nil { + t.Errorf("manifests exposes --%s, a bypass the checker must not have", name) + } + } + if !strings.Contains(cmd.Flags().Lookup("tree").Usage, "public") { + t.Error("--tree does not name its polarities") + } + }) +} diff --git a/internal/surface/cli/report.go b/internal/surface/cli/report.go index fc02fc33e..6358628f2 100644 --- a/internal/surface/cli/report.go +++ b/internal/surface/cli/report.go @@ -307,7 +307,11 @@ func newInboxCommand(asJSON *bool) *cobra.Command { fmt.Fprintf(w, " %s\n", inboxUntrustedNotice) for _, e := range list { if e.State == report.StateUnreadable { - fmt.Fprintf(w, " %s %s key %s UNREADABLE: %s\n", e.ID, e.ReceivedAt, e.SenderKey[:12], termsafe.Sanitize(e.Unreadable)) + from := "key " + e.SenderKey[:12] + if e.SenderName != "" { + from = termsafe.Sanitize(e.SenderName) + " (" + from + ")" + } + fmt.Fprintf(w, " %s %s %s UNREADABLE: %s\n", e.ID, e.ReceivedAt, from, termsafe.Sanitize(e.Unreadable)) continue } fmt.Fprintf(w, " %s %s %s %s/%s %s\n", e.ID, e.ReceivedAt, termsafe.Sanitize(e.SenderName),