diff --git a/docs/results/2026-09-24-whole-repository-status-must-not-fail-on-an-unrelated-change-results.md b/docs/results/2026-09-24-whole-repository-status-must-not-fail-on-an-unrelated-change-results.md new file mode 100644 index 000000000..fc53c8751 --- /dev/null +++ b/docs/results/2026-09-24-whole-repository-status-must-not-fail-on-an-unrelated-change-results.md @@ -0,0 +1,41 @@ + +> ↩ **[Change 0454 — Whole-repository status must not fail on an unrelated change's invalid branch name](https://github.com/danielhanold/docket/blob/docket/docs/changes/active/0454-whole-repository-status-must-not-fail-on-an-unrelated-change.md)** + +# Whole-repository status must not fail on an unrelated change's invalid branch name — Results + +**Human action:** No human action is needed before merge. The optional walkthrough below shows the new finding in real `docket status` output, if you want to see it. + +## Outcome + +Before this change, `docket status` failed completely with an `external-failed` error when any change that another change stacks on recorded a `branch:` value that is not a valid git branch name, for example `feat/a..parent`. Two other reads failed the same way: implement-next's startup preflight (`maintenance.preflight`) and automatic change selection (`context.implementation` with no id). So one broken record hid the whole backlog, including the broken record itself. + +Now: + +- All three reads complete. A branch name that cannot exist in git is treated as absent and is never fetched. A change stacked on such a parent is not build-ready and shows the existing `stack-base-unresolved` state. +- `docket status` reports one error-severity `branch-malformed` finding on each displayed active change with an invalid branch name. The rest of the backlog still renders. The preflight envelope forwards the finding, and the preflight verdict still depends only on the sweep, as it did before. +- The finding's remedy fits the record's state. If the record has a parseable `pr:`, the remedy is a filled-in `docket change repair-identity --adopt-pr-head` command. Otherwise it says to correct `branch:` on the `docket` branch by hand and then run `docket repository migrate`. +- Docket's local branch-name check now follows git's own `check-ref-format` rules. It also rejects control characters, `~ ^ : ? [`, and a trailing `.`. Names like these used to reach `git fetch` and fail there. The repair path's check (`recordedBranch`) uses the same rule, so `repair-identity` can repair a name like `feat/a:b`. +- A well-formed branch name whose fetch fails for a real reason, such as network or auth, still fails the whole read. Named operations are unchanged. + +The build matches the design with one small difference. The design's example said automatic selection picks change 0007. In the test fixture it picks 0006, which is the first healthy candidate by id, and the test pins that. + +## Human actions and testing + +### Optional — see the finding in real status output + +This covers the same ground as `TestStatusBranchMalformed*`. It just shows the new finding in real `docket status` output. + +Prerequisites: a scratch clone of a docket-managed repo, not your working repo. + +1. On the `docket` branch of the scratch clone, set the `branch:` of an active change to `feat/a..bad`, and set another change's `stacked_on:` to point at it. Commit. + Expected: the commit succeeds. +2. Run `docket status --json`. + Expected: `result` is `applied`. `findings` has an entry with `code: branch-malformed`, `severity: error`, and `field: branch` for that change. The stacked child's readiness is `stack-base-unresolved`. + +Cleanup: delete the scratch clone. + +## Verification performed + +- Full suite (`go run ./cmd/docket development test`) passed on the build head through the build gate. The run printed `BUDGET WATCH` lines only, all on long-running integration and race scripts this change did not touch. +- Every task was mutation-tested: removing the filter, the predicate, the remedy branch, or the `recordedBranch` delegation turns its tests red. +- The whole-branch review (standard rung) found nothing. diff --git a/docs/superpowers/plans/2026-09-24-whole-repository-status-must-not-fail-on-an-unrelated-change.md b/docs/superpowers/plans/2026-09-24-whole-repository-status-must-not-fail-on-an-unrelated-change.md new file mode 100644 index 000000000..8a8e03e21 --- /dev/null +++ b/docs/superpowers/plans/2026-09-24-whole-repository-status-must-not-fail-on-an-unrelated-change.md @@ -0,0 +1,597 @@ + +> ↩ **[Change 0454 — Whole-repository status must not fail on an unrelated change's invalid branch name](https://github.com/danielhanold/docket/blob/docket/docs/changes/active/0454-whole-repository-status-must-not-fail-on-an-unrelated-change.md)** + +# Whole-Repository Status Survives an Unrelated Invalid Branch Name — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make the three whole-repository reads that share one live branch probe (`docket status`, `maintenance.preflight`, automatic `context.implementation`) survive a change whose recorded `branch:` is not a valid git ref name, and report it as a per-change `branch-malformed` error finding instead of failing the whole read. + +**Architecture:** Complete gitcli's local ref-name grammar to git's own `check-ref-format` rules and export one predicate (`gitcli.ValidBranchName`). The whole-corpus probe collector `stackBranches` skips names that fail it (such a name cannot exist on the remote, so skipping states absence accurately); the existing `BaseBranchAbsent`/`stack-base-unresolved` domain path then handles selection with no new logic. Status gains a per-displayed-change `branch-malformed` finding whose remedy is branched on whether the record carries a parseable `pr:`. `recordedBranch` delegates its shape check to the same predicate so the printed PR-case remedy (`change repair-identity --adopt-pr-head`) actually applies to every name status flags. + +**Tech Stack:** Go; packages `internal/gitcli`, `internal/app`, `internal/domain` (read-only). Test style: stdlib `testing`, table tests, the existing `fakeReader` / `poisonBranchReader` scriptable StatusReader fakes in `internal/app`. + +**Spec:** `docs/superpowers/specs/2026-09-24-whole-repository-status-must-not-fail-on-an-unrelated-change-design.md` (synchronized metadata tree; the change record is `docs/changes/active/0454-whole-repository-status-must-not-fail-on-an-unrelated-change.md`). + +## Global Constraints + +- `stackBranchesFor` (named operations) is **unchanged** — 0449's bounding stays exactly as is. +- `gitStatusReader.BranchFacts` (`internal/app/status_git.go`) is **unchanged**: a well-formed name whose probe fails for a real reason (network, auth) must keep failing the whole read as `external-failed`. An observation failure is never reported as proven absence (learning: probe-error-is-not-clean-absence). +- The new finding is **not** a snapshot-validation finding — nothing is added to `repository.BuildSnapshot`'s report. +- Do **not** merge `domain.malformedBranchRef` or `transaction.validRefShape` into the gitcli predicate. Only `recordedBranch` delegates. +- Every finding-code string literal in `internal/app` must be minted in `internal/app/finding_codes.go` (`TestNoInlineFindingCodeLiterals` enforces this by AST shape) and registered in the hand-sorted `AllFindingCodes` (`TestFindingCodeRegistryIntegrity` asserts ordering/dedup). +- No new ADR; no new readiness token, status, or manifest field. +- Every remedy string must be valid in the exact repo state that produced it (learning: printed-remedy-state-validity); status stays offline and fills in only values it already holds. +- The build gate runs the whole suite via the repo's configured `build.test_command` (read from config; the Go runner `internal/suiterunner` is the sole channel). + +## Review Focus + +Spec-implied inputs no single task's happy path covers; each line's pinning test is added to the owning task: + +1. A recorded branch that is well-formed but unprobeable (network failure) must still fail the whole read `external-failed` — the filter must not widen into swallowing probe errors. → Task 2, Step 6. +2. A malformed branch on a change with `pr:` present but unparseable (e.g. `pr: 'broken'`) must get the hand-edit remedy, never a `repair-identity` command with a fabricated number. → Task 3 remedy tests (row 3). +3. A name only the *completed* grammar rejects (`feat/a:b` — the old grammar passed it to git) must be filtered from the probe and flagged, not just the historically-rejected `feat/a..parent` shape. → Tasks 1–4 all carry a `feat/a:b` row. +4. `recordedBranch` delegation must stay fail-closed, not loosened: `refs/`-prefixed and leading-`-` values must still be refused even though the prefixed-ref predicate alone would pass them. → Task 4, Step 1 rows. +5. An *archived* or filtered-out change with a malformed branch must not produce a finding (the finding covers **displayed active** changes only), while a malformed stack *ancestor* must still be skipped by the probe even when the ancestor itself is filtered out of display. → Task 3, Step 4. + +--- + +### Task 1: Complete gitcli's ref-name grammar and export the branch-name predicate + +**Files:** +- Modify: `internal/gitcli/types.go` (function `validateRefName`, and a new exported `ValidBranchName`) +- Test: `internal/gitcli/types_test.go` (extend `TestValidateRefName`; add `TestValidBranchName`) + +**Interfaces:** +- Produces: `func ValidBranchName(short string) bool` in package `gitcli` — reports whether `refs/heads/` passes `validateRefName`, i.e. exactly the question the live probe (`FetchBranch`, which validates `refs/heads/`+name) will ask. Tasks 2–4 consume it. +- `validateRefName` keeps its existing rules and error style (`errors.New("gitcli: …")`); the only behavioural change for gitcli operations is that a name git would reject anyway now fails early as `KindInvalidRequest` instead of reaching git and failing as `KindCommandFailed`. + +- [ ] **Step 1: Write the failing tests** + +In `internal/gitcli/types_test.go`, extend `TestValidateRefName`'s `bad` slice with one row per **added** rule (each row fails exactly one new rule, so deleting any one rule reddens its row — the mutation the spec demands), keeping the existing rows as controls: + +```go + bad := []RefName{"main", "heads/main", "refs/", "refs/heads/", "-refs/heads/x", + "refs/heads/a b", "refs/heads/a..b", "refs/heads/a.lock", "refs/heads/a@{1}", + "refs/heads/*", "refs/heads/a\\z", "refs/heads/.hidden", "refs/heads/a\x00b", + // check-ref-format completion (change 0454): control chars, ~ ^ : ? [, trailing dot. + "refs/heads/a\x01b", "refs/heads/a\x1fb", "refs/heads/a\x7fb", + "refs/heads/a~b", "refs/heads/a^b", "refs/heads/a:b", + "refs/heads/a?b", "refs/heads/a[b", "refs/heads/a."} +``` + +Add below it: + +```go +func TestValidBranchName(t *testing.T) { + good := []string{"main", "feat/x", "fix/whole-repository-status", "a.b/c-d"} + for _, b := range good { + if !ValidBranchName(b) { + t.Errorf("ValidBranchName(%q) = false, want true", b) + } + } + // Includes the two fixture names every later task reuses: feat/a..parent + // (old grammar already rejected) and feat/a:b (only the completed grammar + // rejects it locally; git itself always did). + bad := []string{"", "feat/a..parent", "feat/a:b", "a b", "a~b", "a^b", "a?b", + "a[b", "a.", "a\x01b", "@{x", "a\\b", "a*", ".hidden", "a.lock", "a/", "a//b"} + for _, b := range bad { + if ValidBranchName(b) { + t.Errorf("ValidBranchName(%q) = true, want false", b) + } + } +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `go test ./internal/gitcli/ -run 'TestValidateRefName|TestValidBranchName' -v` +Expected: FAIL — `ValidBranchName` undefined (compile error). Fix compile by adding the `ValidBranchName` stub returning `validateRefName(...) == nil` first if you want to see the grammar rows fail individually: `refs/heads/a~b`, `refs/heads/a:b`, `refs/heads/a.` etc. are accepted by the current grammar. + +- [ ] **Step 3: Implement** + +In `internal/gitcli/types.go`, inside `validateRefName`, after the existing whitespace check, add: + +```go + for i := 0; i < len(s); i++ { + if s[i] < 0x20 || s[i] == 0x7F { + return errors.New("gitcli: ref name contains control character") + } + } + if strings.ContainsAny(s, "~^:?[") { + return errors.New("gitcli: ref name contains ~, ^, :, ? or [") + } + if strings.HasSuffix(s, ".") { + return errors.New("gitcli: ref name ends in dot") + } +``` + +Update the function's doc comment to say it implements git's `check-ref-format` rules (list the additions). Then add, near `validateRefName`: + +```go +// ValidBranchName reports whether short is a name git's check-ref-format +// accepts as refs/heads/ — exactly the question the live branch probe +// asks before fetching (FetchBranch validates the refs/heads/-prefixed name). +// It is the app layer's one sanctioned predicate for deciding that a recorded +// branch: value cannot exist as a ref (change 0454). It deliberately shares +// validateRefName so the caller and the probe can never diverge. +func ValidBranchName(short string) bool { + return validateRefName(RefName("refs/heads/"+short)) == nil +} +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `go test ./internal/gitcli/ -v` +Expected: PASS (whole package — the completed grammar must not break other gitcli tests; if an existing test used a now-rejected name, treat that as a real finding to reconcile against the spec, not a test to weaken). + +- [ ] **Step 5: Commit** + +```bash +git add internal/gitcli/types.go internal/gitcli/types_test.go +git commit -m "feat(gitcli): complete ref-name grammar to check-ref-format and export ValidBranchName (change 0454)" +``` + +--- + +### Task 2: The whole-corpus probe skips names that cannot exist + +**Files:** +- Modify: `internal/app/status.go` (function `stackBranches`; add `gitcli` import) +- Test: `internal/app/status_branch_malformed_test.go` (new file; Tasks 3 and 5 extend it) + +**Interfaces:** +- Consumes: `gitcli.ValidBranchName(string) bool` (Task 1). +- Produces: `stackBranches(snap domain.Snapshot) []string` now excludes any recorded ancestor branch failing the predicate. Signature unchanged; both whole-corpus callers (`Status` step 4 and `implementation_context.go`'s `req.ID <= 0` branch) get the fix from this one edit. `stackBranchesFor` untouched. +- Produces (test helpers this file's later tasks reuse): `malformedStackCorpus(t)` returning a `[]StatusBlob` corpus built with the existing `changeBlob` helper (`status_test.go`): change 0001 `parent-dots` with `branch: 'feat/a..parent'` + `status: in-progress`; 0002 `parent-colon` with `branch: 'feat/a:b'` + `status: in-progress`; 0003 `child-a` with `stacked_on: 1`; 0004 `child-b` with `stacked_on: 2`; 0005 `parent-ok` with `branch: 'feat/ok'` + `status: in-progress`; 0006 `child-ok` with `stacked_on: 5`; 0007 `plain` (no branch, no stack — an ordinary build-ready change). Extra frontmatter goes through `changeBlob`'s `extra` parameter, e.g. `"branch: 'feat/a..parent'\nstatus: in-progress\n"` — note `changeBlob` already writes `status: proposed`, so instead pass status via the extra only if `changeBlob` supports overriding; otherwise write the raw record bytes inline the way `changeBlob` composes them, with the needed `status:`/`branch:`/`stacked_on:` lines. Follow the file's local convention after reading `changeBlob`. + +- [ ] **Step 1: Write the failing unit test** + +Create `internal/app/status_branch_malformed_test.go`. Build the corpus, parse and snapshot it exactly as `Status` does (`parseCorpus`, then `repository.BuildSnapshot` with `Config: testConfig(t)` effective config — copy the two-call sequence from `Status` steps 2–3), then: + +```go +func TestStackBranchesSkipsMalformedNames(t *testing.T) { + snap := buildSnapshot(t, malformedStackCorpus(t)) // local helper wrapping parseCorpus + repository.BuildSnapshot + got := stackBranches(snap) + for _, b := range got { + if b == "feat/a..parent" || b == "feat/a:b" { + t.Errorf("stackBranches includes malformed name %q", b) + } + } + want := []string{"feat/ok"} + if !reflect.DeepEqual(got, want) { + t.Errorf("stackBranches = %v, want %v", got, want) + } +} +``` + +- [ ] **Step 2: Run it to verify it fails** + +Run: `go test ./internal/app/ -run TestStackBranchesSkipsMalformedNames -v` +Expected: FAIL — got includes `feat/a..parent` and `feat/a:b`. + +- [ ] **Step 3: Implement the filter** + +In `internal/app/status.go`, `stackBranches`, change the collection condition: + +```go + if b := ancestor.Branch(); b.State == domain.FieldPresent && b.Value != "" && gitcli.ValidBranchName(b.Value) { + seen[b.Value] = true + } +``` + +Add the `gitcli` import (`github.com/danielhanold/docket/internal/gitcli`). Extend `stackBranches`' doc comment: a name failing `gitcli.ValidBranchName` cannot exist on the remote, so leaving it out of the probe is an accurate statement of absence, not a guess — `BranchFacts.HasBranch` returns false for it and `ResolveEffectiveBase` reports `BaseBranchAbsent` (change 0454); `stackBranchesFor` deliberately does **not** filter (named operations keep failing closed, change 0449). + +- [ ] **Step 4: Write the failing Status-level test (applied, not external-failed; probe never asked)** + +Same file. Use `fakeReader` and the `poisoned` / `poisonBranchReader` helpers (`internal/app/named_branch_facts_test.go`) — poisoning proves the malformed names are never requested (mutation: reverting Step 3 makes this red via the poison error): + +```go +func TestStatusSurvivesMalformedStackBranch(t *testing.T) { + pin := docketPin(t) + inner := &fakeReader{pin: pin, corpus: malformedStackCorpus(t), facts: domain.NewBranchFacts(map[string]bool{"feat/ok": true})} + reader := poisoned(inner, "feat/a..parent", "feat/a:b") + res := Status(context.Background(), reader, StatusOptions{RepoDir: "."}) + if res.Result != ResultApplied { + t.Fatalf("Status over a malformed stack branch = %s (%s: %s), want applied", res.Result, res.Reason, res.Message) + } + rows := map[int]StatusChange{} + for _, c := range res.Changes { + rows[c.ID] = c + } + for _, id := range []int{3, 4} { // children of the malformed parents + if rows[id].Readiness != string(domain.ReadyStackBaseUnresolved) { + t.Errorf("change %04d readiness = %q, want %q", id, rows[id].Readiness, domain.ReadyStackBaseUnresolved) + } + for _, r := range res.Ready { + if r == id { + t.Errorf("change %04d is in ready, want excluded", id) + } + } + } + if rows[7].ID != 7 { + t.Errorf("unrelated change 0007 missing from the rendered backlog") + } +} +``` + +Adjust `domain.NewBranchFacts`'s argument to its real signature (see its uses in `status_test.go` — `domain.NewBranchFacts(nil)`); the intent is: `feat/ok` present, nothing else asked. + +- [ ] **Step 5: Run it, verify it passes; verify Step 4's mutation** + +Run: `go test ./internal/app/ -run TestStatusSurvivesMalformedStackBranch -v` +Expected: PASS. Then temporarily revert the Step 3 filter (`git stash -- internal/app/status.go` or comment the predicate out), rerun, and confirm the test fails with the poison error; restore. + +- [ ] **Step 6: Add the regression test — a real probe failure on a well-formed name still fails the read** + +```go +func TestStatusStillFailsOnUnprobeableWellFormedBranch(t *testing.T) { + pin := docketPin(t) + inner := &fakeReader{pin: pin, corpus: malformedStackCorpus(t), facts: domain.NewBranchFacts(nil)} + reader := poisoned(inner, "feat/ok") // well-formed, but the probe errors + res := Status(context.Background(), reader, StatusOptions{RepoDir: "."}) + if res.Result != ResultExternalFailed { + t.Fatalf("Status with an unprobeable well-formed branch = %s, want external-failed", res.Result) + } +} +``` + +Use the exact `Result*` constant `classifyStatusError` maps `ErrStatusExternal` to (see existing external-failure tests in `status_test.go`). + +Run: `go test ./internal/app/ -run TestStatusStillFailsOnUnprobeableWellFormedBranch -v` — PASS (this pins existing behaviour; if it fails, the filter widened too far — fix the filter, not the test). + +- [ ] **Step 7: Commit** + +```bash +git add internal/app/status.go internal/app/status_branch_malformed_test.go +git commit -m "fix(app): whole-corpus branch probe skips names that cannot exist (change 0454)" +``` + +--- + +### Task 3: Status reports a per-change `branch-malformed` finding with a state-valid remedy + +**Files:** +- Modify: `internal/app/finding_codes.go` (new `FCBranchMalformed` constant + `AllFindingCodes` entry) +- Modify: `internal/app/status.go` (new `branchMalformedCheck`; wire into `Status` step 6 loop) +- Test: `internal/app/status_branch_malformed_test.go` (extend) + +**Interfaces:** +- Consumes: `gitcli.ValidBranchName` (Task 1), `parsePRRef(string) (int, bool)` (`internal/app/finalize_context.go`), `changeIdentity(domain.ChangeID) string`, `blobByPath map[string]StatusBlob` (already built in `Status` step 2). +- Produces: `FCBranchMalformed FindingCode = "branch-malformed"` (reuses the token `errBranchMalformed` / `domain.skipBranchMalformed` already spell); `branchMalformedCheck(c domain.Change, blobByPath map[string]StatusBlob) []StatusFinding`, appended to `artifactFindings` so `assembleFindings` sorts it with the artifact group (identity, then field). + +- [ ] **Step 1: Write the failing finding tests** + +Extend `TestStatusSurvivesMalformedStackBranch` (Task 2) and add remedy-variant coverage. For the corpus, give 0001 a parseable PR (`pr: 'https://github.com/danielhanold/docket/pull/77'`) and 0002 no `pr:`; add 0008 `parent-badpr` with `branch: 'feat/a:c'`, `status: in-progress`, and `pr: 'broken'` (present but unparseable — Review Focus 2): + +```go +func TestStatusBranchMalformedFindings(t *testing.T) { + pin := docketPin(t) + inner := &fakeReader{pin: pin, corpus: malformedStackCorpus(t), facts: domain.NewBranchFacts(nil)} + reader := poisoned(inner, "feat/a..parent", "feat/a:b", "feat/a:c") + res := Status(context.Background(), reader, StatusOptions{RepoDir: "."}) + if res.Result != ResultApplied { + t.Fatalf("Status = %s, want applied", res.Result) + } + byIdentity := map[string][]StatusFinding{} + for _, f := range res.Findings { + if f.Code == string(FCBranchMalformed) { + byIdentity[f.Identity] = append(byIdentity[f.Identity], f) + } + } + // Exactly one finding per malformed displayed change; none for well-formed ones. + for _, id := range []string{"0001", "0002", "0008"} { + fs := byIdentity[id] + if len(fs) != 1 { + t.Fatalf("change %s: %d branch-malformed findings, want 1", id, len(fs)) + } + f := fs[0] + if f.Severity != "error" || f.Entity != string(domain.EntityChange) || f.Field != "branch" { + t.Errorf("change %s finding shape = %+v", id, f) + } + if !strings.Contains(f.Message, "not a valid git branch name") { + t.Errorf("change %s message = %q", id, f.Message) + } + } + if len(byIdentity) != 3 { + t.Errorf("branch-malformed identities = %v, want exactly 0001 0002 0008", byIdentity) + } + // Remedy variants (printed-remedy-state-validity): parseable pr: names the + // typed repair with id, version, and PR number filled in; otherwise the + // hand-edit + repository migrate remedy — including pr: present but unparseable. + prRemedy := byIdentity["0001"][0].Remedy + for _, want := range []string{"change repair-identity", "--id 1", "--expect-version blobchange0001", "--adopt-pr-head", "--expect-pr 77"} { + if !strings.Contains(prRemedy, want) { + t.Errorf("PR-case remedy %q lacks %q", prRemedy, want) + } + } + for _, id := range []string{"0002", "0008"} { + r := byIdentity[id][0].Remedy + if strings.Contains(r, "repair-identity") || !strings.Contains(r, "repository migrate") { + t.Errorf("change %s remedy = %q, want the hand-edit + repository migrate remedy", id, r) + } + } +} +``` + +(`blobchange0001` is the `Version` the `changeBlob` fixture helper stamps; if the corpus helper writes raw blobs, assert against the version string it used.) Mutation for the spec's remedy test: swapping the `pr:`-parses condition makes the 0001 vs 0002 expectations cross — this test is that mutation's tripwire. + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `go test ./internal/app/ -run TestStatusBranchMalformedFindings -v` +Expected: FAIL — `FCBranchMalformed` undefined. + +- [ ] **Step 3: Implement** + +In `internal/app/finding_codes.go`, in the census const block: + +```go + // Whole-repository status branch check (change 0454): a displayed active + // change records a branch: value that is not a valid git branch name. The + // token deliberately reuses the spelling finalize's skip reason and + // recordedBranch's errBranchMalformed already emit. + FCBranchMalformed FindingCode = "branch-malformed" +``` + +In `AllFindingCodes`, insert `FCBranchMalformed,` in sorted position — immediately before `FindingCode("branch-still-exists"),`. + +In `internal/app/status.go`, next to `artifactChecks`: + +```go +// branchMalformedCheck reports one error finding when a displayed active +// change's recorded branch: cannot be a git branch name (gitcli.ValidBranchName, +// the same predicate the whole-corpus probe filters on — change 0454). The +// remedy is branched on the same condition that decides which repair can work +// in this exact state: a parseable pr: names the typed repair-identity +// adopt-pr-head command with the id, record version, and PR number filled in +// (the head branch must be read from the PR itself — status stays offline); +// otherwise no typed operation edits branch:, so the remedy is the hand edit +// plus repository migrate to re-render the board. An absent or empty branch: +// is a distinct, benign state here and produces no finding. +func branchMalformedCheck(c domain.Change, blobByPath map[string]StatusBlob) []StatusFinding { + b := c.Branch() + if b.State != domain.FieldPresent || b.Value == "" || gitcli.ValidBranchName(b.Value) { + return nil + } + remedy := "correct branch: on the change record on the docket branch (the real feature branch, or clear it if no branch was ever created), then run: docket repository migrate to re-render the board" + if pr := c.PR(); pr.State == domain.FieldPresent { + if n, ok := parsePRRef(pr.Value); ok { + remedy = fmt.Sprintf("run: docket change repair-identity --id %d --expect-version %s --adopt-pr-head --expect-pr %d --expect-head ", + int(c.ID()), blobByPath[c.Path()].Version, n, n) + } + } + return []StatusFinding{{ + Code: string(FCBranchMalformed), + Severity: string(domain.SeverityError), + Entity: string(domain.EntityChange), + Identity: changeIdentity(c.ID()), + Field: "branch", + Message: fmt.Sprintf("change %s records branch: %q, which is not a valid git branch name", + changeIdentity(c.ID()), b.Value), + Remedy: remedy, + }} +} +``` + +Wire it into `Status` step 6, inside the existing `for _, c := range displayed` loop, after the `artifactChecks` append: + +```go + artifactFindings = append(artifactFindings, branchMalformedCheck(c, blobByPath)...) +``` + +Update the step-6 comment to name both checks. Human output (`status_human.go`) and the preflight envelope render findings generically — verify no format-specific change is needed by reading how `status_human.go` prints `Findings` (it must print Code/Message/Remedy for this finding as for any other; do not add a bespoke branch). + +- [ ] **Step 4: Add the display-scope test (Review Focus 5)** + +Same file: run `Status` with `StatusOptions{Types: []string{"feature"}}` (or whatever projection excludes the fixture's `fix`-typed malformed parents — pick a filter that keeps 0007 and drops 0001/0002/0008, adjusting fixture types as needed) and assert: result applied, zero `branch-malformed` findings (parents not displayed), while the probe still never asks for the malformed names (poisoned reader stays quiet — the skip is corpus-wide even when the record is filtered from display). + +Run: `go test ./internal/app/ -run 'TestStatusBranchMalformed' -v` — Expected: FAIL until implemented, then PASS. + +- [ ] **Step 5: Run the package's guard tests** + +Run: `go test ./internal/app/ -run 'TestNoInlineFindingCodeLiterals|TestFindingCodeRegistryIntegrity|TestStatus' -v` +Expected: PASS — the constant is registered, sorted, and minted only in `finding_codes.go`. + +- [ ] **Step 6: Commit** + +```bash +git add internal/app/finding_codes.go internal/app/status.go internal/app/status_branch_malformed_test.go +git commit -m "feat(app): status reports branch-malformed per displayed change with a state-valid remedy (change 0454)" +``` + +--- + +### Task 4: `recordedBranch` delegates its shape check, so the PR-case remedy works for every flagged name + +**Files:** +- Modify: `internal/app/branch_identity.go` (function `recordedBranch`) +- Test: `internal/app/change_repair_test.go` (extend) + +**Interfaces:** +- Consumes: `gitcli.ValidBranchName` (Task 1). +- Produces: `recordedBranch(c domain.Change) (string, error)` — signature and error values (`errBranchMissing`, `errBranchMalformed`) unchanged; the malformed set **widens** to everything the gitcli predicate rejects while keeping its own stricter `refs/` and leading-`-` refusals (fail-closed both ways; Review Focus 4). Effect: `repairProveWorkspaceClear` skips the workspace inspection for exactly the names status flags, so `change repair-identity --adopt-pr-head` applies on a `feat/a:b` record instead of failing inside git. + +- [ ] **Step 1: Write the failing test** + +`recordedBranch` is package-private with existing direct coverage? Check for a `branch_identity_test.go`; if none, add the table into `change_repair_test.go` (it lives in package `app`): + +```go +func TestRecordedBranchDelegatesToGitcliPredicate(t *testing.T) { + cases := []struct { + branch string + wantErr error + }{ + {"feat/x", nil}, + {"", errBranchMissing}, + {"refs/heads/x", errBranchMalformed}, // kept: stricter than the prefixed-ref predicate alone + {"-x", errBranchMalformed}, // kept: option smuggling + {"feat/a..parent", errBranchMalformed}, + {"feat/a:b", errBranchMalformed}, // NEW: only the delegated predicate rejects this + {"a~b", errBranchMalformed}, + {"a b", errBranchMalformed}, + {"a.", errBranchMalformed}, + } + for _, c := range cases { + got, err := recordedBranch(changeWithBranch(t, c.branch)) // build via the file's existing change-fixture helper + if !errors.Is(err, c.wantErr) { + t.Errorf("recordedBranch(branch=%q) err = %v, want %v", c.branch, err, c.wantErr) + } + if c.wantErr == nil && got != c.branch { + t.Errorf("recordedBranch(branch=%q) = %q", c.branch, got) + } + } +} +``` + +Build the `domain.Change` fixture the way this test file already builds changes for repair tests (read the file first and reuse its helper; if it only builds via corpus parsing, parse a one-record corpus per row). + +- [ ] **Step 2: Run it to verify it fails** + +Run: `go test ./internal/app/ -run TestRecordedBranchDelegatesToGitcliPredicate -v` +Expected: FAIL on the `feat/a:b` row (currently returns nil error). + +- [ ] **Step 3: Implement** + +In `internal/app/branch_identity.go`: + +```go + if strings.HasPrefix(b.Value, "refs/") || strings.HasPrefix(b.Value, "-") || + !gitcli.ValidBranchName(b.Value) { + return "", errBranchMalformed + } +``` + +Import `gitcli`; drop the now-subsumed hand-listed checks (whitespace, `@{`, `..`, NUL — all rejected by the predicate); keep and comment the two survivors: `refs/` (a short name must not smuggle a full ref — prefixing would still make it a *valid* ref, so the predicate alone cannot catch it) and leading `-` (option smuggling; also invisible to the prefixed-ref question). Update the doc comment: the shape check delegates to `gitcli.ValidBranchName` so it agrees with the probe and with status's `branch-malformed` finding (change 0454) — fail-closed: a name git would reject is refused as `branch-malformed` here rather than failing inside git (duplicated-gate-copies-the-whole-predicate: delegation, not a second enumeration). + +- [ ] **Step 4: Run it to verify it passes** + +Run: `go test ./internal/app/ -run 'TestRecordedBranch|TestRepair' -v` +Expected: PASS, including every existing repair test (the widened malformed set must not break the adopt-pr-head apply path's fixtures — they use well-formed branches). + +- [ ] **Step 5: Write the failing end-to-end remedy test (spec Testing 8)** + +In `internal/app/change_repair_test.go`, find the existing test that drives `repair-identity --adopt-pr-head` to `RepairApplied` (the apply-path test around `TestRepairWritesOnlyTheApprovedField` — read it and reuse its fake GitHub + engine harness verbatim). Add a two-row variant where the record's `branch:` is malformed and the adopted head is present on the remote: + +```go +func TestRepairAdoptPRHeadAppliesOnMalformedRecordedBranch(t *testing.T) { + for _, recorded := range []string{"feat/a..parent", "feat/a:b"} { + t.Run(recorded, func(t *testing.T) { + // Same harness as the existing adopt-pr-head apply test, with the + // change record's branch: set to `recorded`, the fake PR reporting + // head "feat/real", and "feat/real" present on the fake remote. + // Assert the operation result is the applied disposition and the + // record's branch: becomes "feat/real". + }) + } +} +``` + +Fill the body from the neighbouring apply test's real harness (fakes, request construction with `AdoptPRHead: true`, `ExpectPRNumber`, `ExpectHead: "feat/real"`, version threading) — the two rows and the two assertions above are the contract; the plumbing is whatever that file already does. The `feat/a:b` row is the discriminating one: without Step 3 it reaches `repairProveWorkspaceClear`'s workspace inspection path (recordedBranch called it well-formed) and diverges — run the test before Step 3's commit if you want to watch it, or temporarily revert `branch_identity.go` to confirm the row reddens. + +- [ ] **Step 6: Run and verify; confirm the mutation** + +Run: `go test ./internal/app/ -run TestRepairAdoptPRHeadAppliesOnMalformedRecordedBranch -v` +Expected: PASS both rows. Temporarily revert the Step 3 delegation and confirm the `feat/a:b` row fails; restore. + +- [ ] **Step 7: Commit** + +```bash +git add internal/app/branch_identity.go internal/app/change_repair_test.go +git commit -m "fix(app): recordedBranch delegates branch shape to the gitcli predicate (change 0454)" +``` + +--- + +### Task 5: Automatic selection and preflight survive the malformed branch (tests only) + +**Files:** +- Test: `internal/app/implementation_context_test.go` (extend) +- Test: `internal/app/maintenance_preflight_test.go` (extend) + +**Interfaces:** +- Consumes: Task 2's `stackBranches` filter (both behaviours come free from it — these tests pin the two other whole-repository entry points the spec names), `malformedStackCorpus` / fixture idiom from Task 2 (the corpus helper lives in `status_branch_malformed_test.go`, same package, so it is directly callable), `poisoned(...)`. + +- [ ] **Step 1: Write the automatic-selection test (spec Testing 4)** + +In `internal/app/implementation_context_test.go`, following the file's existing harness for `ImplementationContext` (read how it builds `deps`, its reader, and a no-id request): + +```go +func TestImplementationContextAutoSelectionSurvivesMalformedStackBranch(t *testing.T) { + // Corpus: malformedStackCorpus(t) with 0007 build-ready (proposed, no deps). + // Reader: poisoned(inner, "feat/a..parent", "feat/a:b") over a fakeReader + // whose facts carry feat/ok. Request: no explicit id (req.ID == 0). + // Assert: the result is the applied/context disposition, the selected + // change is 0007 (top healthy build-ready), and the result is NOT the + // external-failed classification. +} +``` + +Fill the body from the file's existing automatic-selection test (same deps construction, same result-field assertions); the three assertions above are the contract. Mutation: reverting Task 2's filter makes this test fail with the poison error. + +- [ ] **Step 2: Write the preflight test (spec Testing 5)** + +In `internal/app/maintenance_preflight_test.go`, following its existing harness (preflight's post-sweep read is the `Status(ctx, reader, StatusOptions{…})` call inside `maintenance_preflight.go`): + +```go +func TestPreflightForwardsBranchMalformedFinding(t *testing.T) { + // Same corpus + poisoned reader. Run the preflight operation the way the + // file's other tests do. Assert: the preflight returns its normal verdict + // (not an external failure), and its forwarded status findings include + // exactly one with Code == string(FCBranchMalformed) and Identity "0001". +} +``` + +Fill the body from the neighbouring preflight tests' real harness. + +- [ ] **Step 3: Run both to verify they pass** + +Run: `go test ./internal/app/ -run 'TestImplementationContextAutoSelectionSurvivesMalformedStackBranch|TestPreflightForwardsBranchMalformedFinding' -v` +Expected: PASS (the production code shipped in Tasks 2–3; these are pinning tests — if either fails, the wiring assumption is wrong: debug the production path, never weaken the test). + +- [ ] **Step 4: Commit** + +```bash +git add internal/app/implementation_context_test.go internal/app/maintenance_preflight_test.go +git commit -m "test(app): automatic selection and preflight survive a malformed stack branch (change 0454)" +``` + +--- + +### Task 6: 0449's integration test goes through the real `Status` + +**Files:** +- Modify: `internal/app/named_isolation_integration_test.go` (function `assertUnrelatedBytesIntact` and its doc comment) + +**Interfaces:** +- Consumes: the file's existing integration fixtures (`unrelatedBrokenPath`, `unrelatedBrokenBytes`, `originFile`, the git-backed repo harness) and the real `Status` entry point with the production reader the file's flow already constructs (find how the test invokes other real operations against `repo` and use the same construction for `Status` — if the flow drives operations through a CLI/app facade, call `Status` the same way). + +- [ ] **Step 1: Rework the assertion** + +Replace `assertUnrelatedBytesIntact`'s current in-memory `parseCorpus` re-parse (the spec calls it "routing around" the read) with the real read: after asserting the origin bytes are intact (keep that part verbatim), run the real `Status` over the repo and assert (a) the result is applied — the whole-repository read survives the broken record — and (b) the findings include an error-severity finding with `Path == unrelatedBrokenPath` (the parse finding for the broken record). Update the doc comment to say the proof now goes through the production `Status` read (change 0454) instead of a detached `parseCorpus`. + +Note: the 0449 fixture's broken record is a parse-level defect; if the fixture repo also (or instead) needs the `feat/a..parent` branch-name shape the spec names, check what `unrelatedBrokenBytes` seeds — the spec's requirement is "go through the real `Status` over the `feat/a..parent` fixture and assert the finding". If the seeded record parses but records `branch: feat/a..parent`, assert the `FCBranchMalformed` finding for it instead of the parse finding. Read the fixture first; assert the finding class that fixture actually produces, and if it produces neither, extend the fixture with a second unrelated record carrying `branch: 'feat/a..parent'` and assert both: `Status` applied + its `branch-malformed` finding present. + +- [ ] **Step 2: Run the integration test** + +Run: `go test ./internal/app/ -run TestIntegrationNamedImplementationFlowIsolation -v` (build tags: check the file's header for a required tag and add `-tags` accordingly; `tests/README.md` documents the suite's tag partitions.) +Expected: PASS. Before Task 2's filter this exact call would have failed `external-failed` — that ordering is the point of the rework; you can confirm by stashing the Task 2 edit once, rerunning, and restoring. + +- [ ] **Step 3: Commit** + +```bash +git add internal/app/named_isolation_integration_test.go +git commit -m "test(app): 0449 isolation proof goes through the real Status read (change 0454)" +``` + +--- + +### Final gate + +The build gate (owned by the executing build skill) runs the whole suite via the configured `build.test_command` — never only the tests this plan enumerates — and reads the budget report even on green (`BUDGET WATCH:` / `SERIAL CONFIRMED OVER BUDGET:` lines are findings). + +## Self-Review + +- **Spec coverage:** Design §1 → Task 1; §2 → Task 2; §3 → Task 2 Step 4 (readiness assertions) — no new logic, pinned by test; §4 (finding + remedy + `recordedBranch` delegation) → Tasks 3–4; §5 (no ADR) → no task, correctly. Testing 1→T1, 2→T2 S1, 3→T2 S4/S5, 4→T5 S1, 5→T5 S2, 6→T6, 7→T3 S1, 8→T4 S5, 9→T2 S6. No gaps found. +- **Placeholder scan:** Tasks 4–6 direct the implementer to reuse an existing named harness in the same file for plumbing while stating the full assertion contract inline — deliberate existing-codebase-pattern reuse, with every new behavioural assertion spelled out. No TBDs. +- **Type consistency:** `ValidBranchName(string) bool` used identically in Tasks 2, 3, 4; `FCBranchMalformed` minted once in Task 3 and consumed in Tasks 3, 5, 6; `branchMalformedCheck(c domain.Change, blobByPath map[string]StatusBlob) []StatusFinding` defined and wired in Task 3 only. +- **Review Focus:** all five lines have owning tests (T2 S6, T3 S1 row 0008, T1/T2/T3/T4 `feat/a:b` rows, T4 S1, T3 S4). diff --git a/internal/app/branch_identity.go b/internal/app/branch_identity.go index 6d66a72d6..c046ebe5c 100644 --- a/internal/app/branch_identity.go +++ b/internal/app/branch_identity.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/danielhanold/docket/internal/domain" + "github.com/danielhanold/docket/internal/gitcli" ) // The fail-closed errors recordedBranch returns for an unusable recorded @@ -17,7 +18,11 @@ var ( // recordedBranch returns c's recorded feature branch. It fails closed: an // absent or empty branch: on a post-claim record is errBranchMissing, a value -// that cannot be a branch ref is errBranchMalformed. Callers map the error to +// that cannot be a branch ref is errBranchMalformed. The shape check delegates +// to gitcli.ValidBranchName (change 0454), so it agrees with the whole-corpus +// probe filter and with status's branch-malformed finding: a name git would +// reject is refused here as errBranchMalformed rather than failing inside git +// — delegation, not a second enumeration of the grammar. Callers map the error to // their own invalid-state refusal and perform NO mutation. It never // reconstructs a branch from the slug, type, or prefix — a post-claim operation // consumes the recorded branch, nothing else (mint once, record once, consume @@ -27,9 +32,12 @@ func recordedBranch(c domain.Change) (string, error) { if b.State != domain.FieldPresent || b.Value == "" { return "", errBranchMissing } + // Two refusals survive the delegation because the prefixed-ref question + // cannot see them: a "refs/" value would smuggle a full ref (prefixing it + // still yields a *valid* ref), and a leading "-" is option smuggling + // wherever the short name is used bare. if strings.HasPrefix(b.Value, "refs/") || strings.HasPrefix(b.Value, "-") || - strings.ContainsAny(b.Value, " \t\r\n\v\f") || strings.Contains(b.Value, "@{") || - strings.Contains(b.Value, "..") || strings.IndexByte(b.Value, 0) >= 0 { + !gitcli.ValidBranchName(b.Value) { return "", errBranchMalformed } return b.Value, nil diff --git a/internal/app/branch_identity_test.go b/internal/app/branch_identity_test.go index 00895f12c..5ce73e944 100644 --- a/internal/app/branch_identity_test.go +++ b/internal/app/branch_identity_test.go @@ -40,6 +40,15 @@ func TestRecordedBranch(t *testing.T) { {"dotdot", present("a..b"), "", errBranchMalformed}, {"leading dash", present("-lead"), "", errBranchMalformed}, {"whitespace", present("a b"), "", errBranchMalformed}, + // Change 0454: the shape check delegates to gitcli.ValidBranchName, so + // every name the probe filter and status's branch-malformed finding + // reject is refused here too. feat/a:b is the discriminating row — only + // the delegated (completed) grammar rejects it. + {"stack dotdot", present("feat/a..parent"), "", errBranchMalformed}, + {"colon", present("feat/a:b"), "", errBranchMalformed}, + {"tilde", present("a~b"), "", errBranchMalformed}, + {"trailing dot", present("a."), "", errBranchMalformed}, + {"lock suffix", present("feat/x.lock"), "", errBranchMalformed}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/app/change_repair_test.go b/internal/app/change_repair_test.go index bb6d2257d..5fb905438 100644 --- a/internal/app/change_repair_test.go +++ b/internal/app/change_repair_test.go @@ -472,3 +472,42 @@ func TestRepairIdentityUnrelatedInvalidRecordRefusals(t *testing.T) { }) } } + +// TestRepairAdoptPRHeadAppliesOnMalformedRecordedBranch proves the PR-case +// remedy status prints for a branch-malformed record (change 0454) actually +// applies: adopting the PR head over a recorded branch: git would reject lands +// applied, because recordedBranch refuses the malformed name and the workspace +// gate then skips the branch-keyed inspection instead of failing inside git. +// The workspace seam is the real service, so an inspection of the malformed +// name would reach gitcli and fail there. feat/a:b is the discriminating row — +// only the delegated gitcli predicate rejects it; without the delegation the +// gate inspects it and refuses as workspace-conflict. +func TestRepairAdoptPRHeadAppliesOnMalformedRecordedBranch(t *testing.T) { + requireRealGit(t) + for _, recorded := range []string{"feat/a..parent", "feat/a:b"} { + t.Run(recorded, func(t *testing.T) { + recPath := groomPath(3, "widget") + repo := newWorkingRepo(t, map[string]string{recPath: repairRecord(3, "widget", recorded)}) + repo.writerAdvance(t, "feat/renamed", map[string]string{"impl.go": "package impl\n"}) + + node := planningDepsFor(t, repo.invocation) + svc, err := workspace.NewService(node.deps.Client) + if err != nil { + t.Fatalf("workspace.NewService: %v", err) + } + deps := FinalizeDeps{Planning: node.deps, GitHub: repairGitHub("feat/renamed"), Workspace: svc} + res := RepairIdentity(context.Background(), deps, node.dir, RepairIdentityRequest{ + ID: 3, ExpectVersion: blobVersionAt(t, repo.origin, "docket", recPath), + AdoptPRHead: true, ExpectPRNumber: 7, ExpectHead: "feat/renamed", + }) + if res.Result != ResultApplied || res.Branch != "feat/renamed" { + t.Fatalf("adopt-pr-head over recorded branch %q = %q reason %q branch %q (msg %q, findings %v), want applied feat/renamed", + recorded, res.Result, res.Reason, res.Branch, res.Message, res.Findings) + } + rec, _ := originFile(t, repo.origin, "docket", recPath) + if !strings.Contains(rec, "branch: 'feat/renamed'") { + t.Errorf("repaired record on origin does not carry the adopted branch:\n%s", rec) + } + }) + } +} diff --git a/internal/app/finding_codes.go b/internal/app/finding_codes.go index fab9747c9..94ad13c3c 100644 --- a/internal/app/finding_codes.go +++ b/internal/app/finding_codes.go @@ -83,6 +83,12 @@ const ( FCParseFailed FindingCode = "parse-failed" FCSweepPRFactsUnresolved FindingCode = "sweep-pr-facts-unresolved" + // Whole-repository status branch check (change 0454): a displayed active + // change records a branch: value that is not a valid git branch name. The + // token deliberately reuses the spelling finalize's skip reason and + // recordedBranch's errBranchMalformed already emit. + FCBranchMalformed FindingCode = "branch-malformed" + // Schema-surface request-shape finding (change 0399, Task 7): SchemaFor // returns ok=false for an id absent from the operation-schema registry, and // the cli maps that to ResultInvalidInput carrying this code. @@ -167,6 +173,7 @@ var AllFindingCodes = []FindingCode{ FCArtifactMissing, FCArtifactRenderFailed, FCAuthoredInputTooLarge, + FCBranchMalformed, FindingCode("branch-still-exists"), FCCollectionPending, FCDanglingReference, diff --git a/internal/app/implementation_context_test.go b/internal/app/implementation_context_test.go index 412650179..e334433bf 100644 --- a/internal/app/implementation_context_test.go +++ b/internal/app/implementation_context_test.go @@ -563,3 +563,37 @@ func TestContextImplementationBranchFactsFailure(t *testing.T) { t.Errorf("failed facts read fabricated a bundle: %+v", got.Context) } } + +// TestImplementationContextAutoSelectionSurvivesMalformedStackBranch (change +// 0454, spec Testing 4): automatic selection shares the whole-corpus branch +// probe with status. A change whose recorded branch: is not a valid git branch +// name must not fail selection external-failed: the probe never asks for the +// malformed names (poisoned), their stacked children read +// stack-base-unresolved and are skipped, and the top healthy build-ready +// change is selected. +func TestImplementationContextAutoSelectionSurvivesMalformedStackBranch(t *testing.T) { + pin := docketPin(t) + inner := &fakeReader{pin: pin, corpus: malformedStackCorpus(t), facts: domain.NewBranchFacts(map[string]bool{"feat/ok": true})} + reader := poisoned(inner, "feat/a..parent", "feat/a:b", "feat/a:c") + got := ContextImplementation(context.Background(), PlanningDeps{Reader: reader, Clock: testClock()}, "", ImplementationContextRequest{}) + if got.Result == ResultExternalFailed { + t.Fatalf("automatic selection over a malformed stack branch failed external-failed (%s: %s)", got.Reason, got.Message) + } + if got.Result != ResultApplied || got.Context == nil { + t.Fatalf("result=%q reason=%q message=%q, want applied with a bundle", got.Result, got.Reason, got.Message) + } + // The top healthy build-ready change is 0006 (stacked on the present, + // well-formed feat/ok). The malformed parents' children 0003/0004 rank + // ahead of it by id, so selecting 0006 proves they were skipped as + // stack-base-unresolved rather than failing the read. + if s := got.Context.Change.Summary; s == nil || s.ID != 6 { + t.Fatalf("selected change = %+v, want 0006 (the top healthy build-ready change)", s) + } + for _, ask := range reader.asks { + for _, b := range ask { + if b == "feat/a..parent" || b == "feat/a:b" || b == "feat/a:c" { + t.Errorf("branch probe asked for malformed name %q", b) + } + } + } +} diff --git a/internal/app/maintenance_preflight_test.go b/internal/app/maintenance_preflight_test.go index 86e500729..f7eefe35b 100644 --- a/internal/app/maintenance_preflight_test.go +++ b/internal/app/maintenance_preflight_test.go @@ -5,6 +5,8 @@ import ( "encoding/json" "strings" "testing" + + "github.com/danielhanold/docket/internal/domain" ) // cleanSweep builds an applied implementation-scope sweep whose entries are all @@ -192,3 +194,35 @@ func TestPreflightHumanText(t *testing.T) { t.Fatalf("HumanText missing status one-liner: %q", human) } } + +// TestPreflightForwardsBranchMalformedFinding (change 0454, spec Testing 5): +// preflight's post-sweep read is the real Status bound exactly as +// MaintenancePreflight binds it. Over a corpus carrying malformed recorded +// branches it returns its normal verdict — not an external failure — and +// forwards the per-change branch-malformed finding in its status half. +func TestPreflightForwardsBranchMalformedFinding(t *testing.T) { + pin := docketPin(t) + inner := &fakeReader{pin: pin, corpus: malformedStackCorpus(t), facts: domain.NewBranchFacts(map[string]bool{"feat/ok": true})} + reader := poisoned(inner, "feat/a..parent", "feat/a:b", "feat/a:c") + res := maintenancePreflight(context.Background(), preflightOps{ + sweep: func(context.Context) MaintenanceResult { return cleanSweep() }, + status: func(ctx context.Context) StatusResult { + return Status(ctx, reader, StatusOptions{RepoDir: ".", IncludeRecords: false}) + }, + }) + if res.Result != ResultApplied || res.Preflight != PreflightClean { + t.Fatalf("preflight = %s/%s (%s: %s), want applied/clean", res.Result, res.Preflight, res.Reason, res.Message) + } + if res.Status == nil { + t.Fatal("preflight omitted its status half over a malformed stack branch") + } + var forOne int + for _, f := range res.Status.Findings { + if f.Code == string(FCBranchMalformed) && f.Identity == "0001" { + forOne++ + } + } + if forOne != 1 { + t.Fatalf("forwarded branch-malformed findings for 0001 = %d, want exactly 1 (findings: %+v)", forOne, res.Status.Findings) + } +} diff --git a/internal/app/named_isolation_integration_test.go b/internal/app/named_isolation_integration_test.go index 24aae87f9..81dad428e 100644 --- a/internal/app/named_isolation_integration_test.go +++ b/internal/app/named_isolation_integration_test.go @@ -13,7 +13,6 @@ import ( "github.com/danielhanold/docket/internal/gatedrive" "github.com/danielhanold/docket/internal/githubcli" - "github.com/danielhanold/docket/internal/repository" "github.com/danielhanold/docket/internal/testsupport" "github.com/danielhanold/docket/internal/workspace" ) @@ -116,26 +115,38 @@ func assertRuntimeIntact(t *testing.T, seeded map[string][]byte) { } // assertUnrelatedBytesIntact proves the unrelated broken record is -// byte-identical on the origin metadata ref and that the status pipeline's -// parse step still reports it as an error finding. (The whole-repository status -// read itself refuses here — its unbounded branch-fact probe reaches the -// unrelated invalid branch — which is why this does not go through Status.) +// byte-identical on the origin metadata ref, then runs the production +// whole-repository Status read over the repository (change 0454) and proves it +// survives the unrelated damage: the read applies, the unparseable record A +// still surfaces as an error finding on its path, and the unrelated stack +// parent (change 20) recording the invalid branch namedIsolationInvalidBranch +// surfaces as its own branch-malformed error finding instead of failing the +// whole read on the live branch probe. func assertUnrelatedBytesIntact(t *testing.T, repo *gitRepo, branch string) { t.Helper() got, ok := originFile(t, repo.origin, branch, unrelatedBrokenPath) if !ok || got != unrelatedBrokenBytes { t.Fatalf("unrelated broken record on origin = %q (present %v), want its exact seeded bytes", got, ok) } - _, findings := parseCorpus([]StatusBlob{{ - Kind: repository.KindChange, Location: repository.LocationActive, - Path: unrelatedBrokenPath, Version: "v", Data: []byte(got), - }}) - for _, f := range findings { + res := Status(context.Background(), NewGitStatusReader(newGitClient(t)), StatusOptions{RepoDir: repo.invocation}) + if res.Result != ResultApplied { + t.Fatalf("whole-repository Status beside the unrelated damage = %s (%s: %s), want applied", res.Result, res.Reason, res.Message) + } + parseErr, malformed := false, false + for _, f := range res.Findings { if f.Path == unrelatedBrokenPath && f.Severity == "error" { - return + parseErr = true + } + if f.Code == string(FCBranchMalformed) && f.Identity == "0020" && f.Severity == "error" { + malformed = true } } - t.Errorf("the unrelated record no longer parses to an error finding; findings %+v", findings) + if !parseErr { + t.Errorf("Status no longer reports the unrelated record %s as an error finding; findings %+v", unrelatedBrokenPath, res.Findings) + } + if !malformed { + t.Errorf("Status reports no %s error finding for the unrelated stack parent 0020 (branch %q); findings %+v", FCBranchMalformed, namedIsolationInvalidBranch, res.Findings) + } } // namedIsolationCheck is the per-step oracle: the unrelated broken record's diff --git a/internal/app/status.go b/internal/app/status.go index ffa6ef641..fe97dfc74 100644 --- a/internal/app/status.go +++ b/internal/app/status.go @@ -10,6 +10,7 @@ import ( "github.com/danielhanold/docket/internal/config" "github.com/danielhanold/docket/internal/document" "github.com/danielhanold/docket/internal/domain" + "github.com/danielhanold/docket/internal/gitcli" "github.com/danielhanold/docket/internal/repository" ) @@ -188,7 +189,8 @@ func Status(ctx context.Context, reader StatusReader, opts StatusOptions) Status displayed := activeChanges(snap, opts.Types, priorities) changes := make([]StatusChange, 0, len(displayed)) - // 6. Artifact checks accumulate their findings alongside the change rows. + // 6. Artifact checks and the branch-malformed check (change 0454) + // accumulate their findings alongside the change rows. var artifactFindings []StatusFinding for _, c := range displayed { changes = append(changes, statusChange(snap, c, facts, readySet, blobByPath)) @@ -197,6 +199,7 @@ func Status(ctx context.Context, reader StatusReader, opts StatusOptions) Status return statusFailure(ctx, pin, ferr) } artifactFindings = append(artifactFindings, f...) + artifactFindings = append(artifactFindings, branchMalformedCheck(c, blobByPath)...) } // 7. Assemble findings in their fixed order; the records inventory is @@ -368,6 +371,14 @@ func parseFinding(b StatusBlob, err error) StatusFinding { // stackBranches collects the recorded branch of every stack ancestor of every // change — exactly the branch names ResolveEffectiveBase consults through // BranchFacts. The set is sorted so the reader is asked deterministically. +// +// A recorded name failing gitcli.ValidBranchName cannot exist on the remote, +// so leaving it out of the probe is an accurate statement of absence, not a +// guess: BranchFacts.HasBranch returns false for it and ResolveEffectiveBase +// reports BaseBranchAbsent, so one change's malformed branch: no longer fails +// the whole read (change 0454). A well-formed name whose probe fails for a real +// reason still fails the read. stackBranchesFor deliberately does not filter: +// named operations keep failing closed on their own stack (change 0449). func stackBranches(snap domain.Snapshot) []string { seen := make(map[string]bool) for _, c := range snap.Changes() { @@ -376,7 +387,7 @@ func stackBranches(snap domain.Snapshot) []string { if out != domain.LookupFound { continue } - if b := ancestor.Branch(); b.State == domain.FieldPresent && b.Value != "" { + if b := ancestor.Branch(); b.State == domain.FieldPresent && b.Value != "" && gitcli.ValidBranchName(b.Value) { seen[b.Value] = true } } @@ -543,6 +554,40 @@ func artifactChecks(ctx context.Context, reader StatusReader, pin StatusPin, c d return findings, nil } +// branchMalformedCheck reports one error finding when a displayed active +// change's recorded branch: cannot be a git branch name (gitcli.ValidBranchName, +// the same predicate the whole-corpus probe filters on — change 0454). The +// remedy is branched on the same condition that decides which repair can work +// in this exact state: a parseable pr: names the typed repair-identity +// adopt-pr-head command with the id, record version, and PR number filled in +// (the head branch must be read from the PR itself — status stays offline); +// otherwise no typed operation edits branch:, so the remedy is the hand edit +// plus repository migrate to re-render the board. An absent or empty branch: +// is a distinct, benign state here and produces no finding. +func branchMalformedCheck(c domain.Change, blobByPath map[string]StatusBlob) []StatusFinding { + b := c.Branch() + if b.State != domain.FieldPresent || b.Value == "" || gitcli.ValidBranchName(b.Value) { + return nil + } + remedy := "correct branch: on the change record on the docket branch (the real feature branch, or clear it if no branch was ever created), then run: docket repository migrate to re-render the board" + if pr := c.PR(); pr.State == domain.FieldPresent { + if n, ok := parsePRRef(pr.Value); ok { + remedy = fmt.Sprintf("run: docket change repair-identity --id %d --expect-version %s --adopt-pr-head --expect-pr %d --expect-head ", + int(c.ID()), blobByPath[c.Path()].Version, n, n) + } + } + return []StatusFinding{{ + Code: string(FCBranchMalformed), + Severity: string(domain.SeverityError), + Entity: string(domain.EntityChange), + Identity: changeIdentity(c.ID()), + Field: "branch", + Message: fmt.Sprintf("change %s records branch: %q, which is not a valid git branch name", + changeIdentity(c.ID()), b.Value), + Remedy: remedy, + }} +} + // corpusRecords is the artifact-integrity inventory over the COMPLETE corpus: // changes by ascending ID, then ADRs by ascending ID, then learnings by slug — // kind-then-identity, independent of the filter projection. diff --git a/internal/app/status_branch_malformed_test.go b/internal/app/status_branch_malformed_test.go new file mode 100644 index 000000000..e429dbbc0 --- /dev/null +++ b/internal/app/status_branch_malformed_test.go @@ -0,0 +1,205 @@ +package app + +import ( + "bytes" + "context" + "reflect" + "strings" + "testing" + + "github.com/danielhanold/docket/internal/domain" +) + +// Change 0454: a change whose recorded branch: is not a valid git branch name +// must not fail the whole-repository reads that share the whole-corpus branch +// probe (stackBranches). Such a name cannot exist on the remote, so the probe +// skips it and the domain reports its stacked children stack-base-unresolved. + +// malformedStackCorpus is the shared fixture: two in-progress parents whose +// recorded branches are malformed — feat/a..parent (the historical shape) and +// feat/a:b (rejected only by the completed check-ref-format grammar) — each +// with a stacked child; a well-formed parent/child pair; and an unrelated +// ordinary build-ready change. Change 0454's finding tests also lean on the +// pr: shapes: 0001 carries a parseable PR, 0002 none, and 0008 a third +// malformed parent (feat/a:c) whose pr: is present but unparseable. +func malformedStackCorpus(t *testing.T) []StatusBlob { + t.Helper() + return []StatusBlob{ + stackFixtureBlob(1, "parent-dots", "in-progress", "feat/a..parent", "pr: 'https://github.com/danielhanold/docket/pull/77'\n"), + stackFixtureBlob(2, "parent-colon", "in-progress", "feat/a:b", ""), + stackFixtureBlob(3, "child-a", "proposed", "", "stacked_on: 1\n"), + stackFixtureBlob(4, "child-b", "proposed", "", "stacked_on: 2\n"), + stackFixtureBlob(5, "parent-ok", "in-progress", "feat/ok", ""), + stackFixtureBlob(6, "child-ok", "proposed", "", "stacked_on: 5\n"), + stackFixtureBlob(7, "plain", "proposed", "", ""), + stackFixtureBlob(8, "parent-badpr", "in-progress", "feat/a:c", "pr: 'broken'\n"), + } +} + +// TestStackBranchesSkipsMalformedNames: the whole-corpus probe set leaves out +// every recorded ancestor branch that fails gitcli.ValidBranchName. +func TestStackBranchesSkipsMalformedNames(t *testing.T) { + snap := snapshotOf(t, malformedStackCorpus(t)) + got := stackBranches(snap) + for _, b := range got { + if b == "feat/a..parent" || b == "feat/a:b" || b == "feat/a:c" { + t.Errorf("stackBranches includes malformed name %q", b) + } + } + want := []string{"feat/ok"} + if !reflect.DeepEqual(got, want) { + t.Errorf("stackBranches = %v, want %v", got, want) + } +} + +// TestStatusSurvivesMalformedStackBranch: status over the fixture applies, the +// malformed names are never asked of the probe (poisoned), the malformed +// parents' children read stack-base-unresolved and stay out of ready, and the +// unrelated change still renders. +func TestStatusSurvivesMalformedStackBranch(t *testing.T) { + pin := docketPin(t) + inner := &fakeReader{pin: pin, corpus: malformedStackCorpus(t), facts: domain.NewBranchFacts(map[string]bool{"feat/ok": true})} + reader := poisoned(inner, "feat/a..parent", "feat/a:b", "feat/a:c") + res := Status(context.Background(), reader, StatusOptions{RepoDir: "."}) + if res.Result != ResultApplied { + t.Fatalf("Status over a malformed stack branch = %s (%s: %s), want applied", res.Result, res.Reason, res.Message) + } + rows := map[int]StatusChange{} + for _, c := range res.Changes { + rows[c.ID] = c + } + for _, id := range []int{3, 4} { // children of the malformed parents + if rows[id].Readiness != string(domain.ReadyStackBaseUnresolved) { + t.Errorf("change %04d readiness = %q, want %q", id, rows[id].Readiness, domain.ReadyStackBaseUnresolved) + } + for _, r := range res.Ready { + if r == id { + t.Errorf("change %04d is in ready, want excluded", id) + } + } + } + if rows[7].ID != 7 { + t.Errorf("unrelated change 0007 missing from the rendered backlog") + } +} + +// TestStatusStillFailsOnUnprobeableWellFormedBranch (Review Focus 1): the +// filter must not widen into swallowing probe errors — a well-formed recorded +// branch whose probe fails still fails the whole read external-failed. +func TestStatusStillFailsOnUnprobeableWellFormedBranch(t *testing.T) { + pin := docketPin(t) + inner := &fakeReader{pin: pin, corpus: malformedStackCorpus(t), facts: domain.NewBranchFacts(nil)} + reader := poisoned(inner, "feat/ok") // well-formed, but the probe errors + res := Status(context.Background(), reader, StatusOptions{RepoDir: "."}) + if res.Result != ResultExternalFailed || res.Reason != ReasonStatusExternal { + t.Fatalf("Status with an unprobeable well-formed branch = %s (%s: %s), want external-failed", res.Result, res.Reason, res.Message) + } +} + +// TestStatusBranchMalformedFindings: every displayed active change whose +// recorded branch: is not a valid git branch name gets exactly one error +// finding, and its remedy is valid in the exact state that produced it +// (printed-remedy-state-validity): a parseable pr: names the typed +// repair-identity adopt-pr-head command with id, version, and PR number +// filled in; no pr: or an unparseable one (Review Focus 2) gets the hand-edit +// plus repository migrate remedy, never a fabricated PR number. +func TestStatusBranchMalformedFindings(t *testing.T) { + pin := docketPin(t) + inner := &fakeReader{pin: pin, corpus: malformedStackCorpus(t), facts: domain.NewBranchFacts(map[string]bool{"feat/ok": true})} + reader := poisoned(inner, "feat/a..parent", "feat/a:b", "feat/a:c") + res := Status(context.Background(), reader, StatusOptions{RepoDir: "."}) + if res.Result != ResultApplied { + t.Fatalf("Status = %s (%s: %s), want applied", res.Result, res.Reason, res.Message) + } + byIdentity := map[string][]StatusFinding{} + for _, f := range res.Findings { + if f.Code == string(FCBranchMalformed) { + byIdentity[f.Identity] = append(byIdentity[f.Identity], f) + } + } + // Exactly one finding per malformed displayed change; none for well-formed ones. + for _, id := range []string{"0001", "0002", "0008"} { + fs := byIdentity[id] + if len(fs) != 1 { + t.Fatalf("change %s: %d branch-malformed findings, want 1", id, len(fs)) + } + f := fs[0] + if f.Severity != string(domain.SeverityError) || f.Entity != string(domain.EntityChange) || f.Field != "branch" { + t.Errorf("change %s finding shape = %+v", id, f) + } + if !strings.Contains(f.Message, "not a valid git branch name") { + t.Errorf("change %s message = %q", id, f.Message) + } + } + if len(byIdentity) != 3 { + t.Errorf("branch-malformed identities = %v, want exactly 0001 0002 0008", byIdentity) + } + prRemedy := byIdentity["0001"][0].Remedy + for _, want := range []string{"change repair-identity", "--id 1 ", "--expect-version blobchange0001", "--adopt-pr-head", "--expect-pr 77", "PR #77"} { + if !strings.Contains(prRemedy, want) { + t.Errorf("PR-case remedy %q lacks %q", prRemedy, want) + } + } + for _, id := range []string{"0002", "0008"} { + r := byIdentity[id][0].Remedy + if strings.Contains(r, "repair-identity") || !strings.Contains(r, "repository migrate") { + t.Errorf("change %s remedy = %q, want the hand-edit + repository migrate remedy", id, r) + } + } +} + +// retyped rewrites a stackFixtureBlob record's type: (the helper stamps feat). +func retyped(t *testing.T, b StatusBlob, typ string) StatusBlob { + t.Helper() + const from = "\ntype: feat\n" + if !bytes.Contains(b.Data, []byte(from)) { + t.Fatalf("fixture %s has no %q line", b.Path, from) + } + b.Data = bytes.Replace(b.Data, []byte(from), []byte("\ntype: "+typ+"\n"), 1) + return b +} + +// TestStatusBranchMalformedDisplayScope (Review Focus 5): the finding covers +// displayed active changes only — a --type projection that filters out the +// malformed parents yields no branch-malformed finding — while the probe skip +// stays corpus-wide: the filtered-out parents' malformed names are still never +// asked of the probe, and their displayed children still read +// stack-base-unresolved. +func TestStatusBranchMalformedDisplayScope(t *testing.T) { + corpus := malformedStackCorpus(t) + for i, b := range corpus { + switch { + case strings.Contains(b.Path, "/0001-"), strings.Contains(b.Path, "/0002-"), strings.Contains(b.Path, "/0008-"): + corpus[i] = retyped(t, b, "fix") + } + } + pin := docketPin(t) + inner := &fakeReader{pin: pin, corpus: corpus, facts: domain.NewBranchFacts(map[string]bool{"feat/ok": true})} + reader := poisoned(inner, "feat/a..parent", "feat/a:b", "feat/a:c") + res := Status(context.Background(), reader, StatusOptions{RepoDir: ".", Types: []string{"feat"}}) + if res.Result != ResultApplied { + t.Fatalf("Status = %s (%s: %s), want applied", res.Result, res.Reason, res.Message) + } + rows := map[int]StatusChange{} + for _, c := range res.Changes { + rows[c.ID] = c + } + for _, id := range []int{1, 2, 8} { + if _, shown := rows[id]; shown { + t.Fatalf("change %04d displayed under --type feat; the fixture must filter it out", id) + } + } + if rows[7].ID != 7 { + t.Fatalf("change 0007 missing from the --type feat projection") + } + for _, f := range res.Findings { + if f.Code == string(FCBranchMalformed) { + t.Errorf("branch-malformed finding for a change not displayed: %+v", f) + } + } + for _, id := range []int{3, 4} { + if rows[id].Readiness != string(domain.ReadyStackBaseUnresolved) { + t.Errorf("change %04d readiness = %q, want %q", id, rows[id].Readiness, domain.ReadyStackBaseUnresolved) + } + } +} diff --git a/internal/gitcli/types.go b/internal/gitcli/types.go index 7d33e181a..7a4aa2615 100644 --- a/internal/gitcli/types.go +++ b/internal/gitcli/types.go @@ -111,9 +111,24 @@ func validateRemoteName(r RemoteName) error { return nil } -// validateRefName requires a "refs/"-prefixed name with at least two -// components and rejects NUL, whitespace, a leading "-", empty components, -// "."/".." components, "@{", "\\", a trailing ".lock" component, and "*". +// ValidBranchName reports whether short is a name git's check-ref-format +// accepts as refs/heads/ — exactly the question the live branch probe +// asks before fetching (FetchBranch validates the refs/heads/-prefixed name). +// It is the app layer's one sanctioned predicate for deciding that a recorded +// branch: value cannot exist as a ref (change 0454). It deliberately shares +// validateRefName so the caller and the probe can never diverge. +func ValidBranchName(short string) bool { + return validateRefName(RefName("refs/heads/"+short)) == nil +} + +// validateRefName implements git's check-ref-format rules for a full ref +// name: it requires a "refs/"-prefixed name with at least two components and +// rejects NUL and other ASCII control characters (below 0x20, and DEL), +// whitespace, a leading "-", empty components, "."/".." components, a +// component with a leading dot, "..", "@{", "\\", "*", "~", "^", ":", "?", "[", +// a trailing ".lock" component, and a trailing ".". The control-character, +// "~^:?[" and trailing-dot rules complete the grammar (change 0454), so a name +// git would reject fails here as an invalid request rather than reaching git. func validateRefName(r RefName) error { s := string(r) if s == "" { @@ -128,6 +143,17 @@ func validateRefName(r RefName) error { if strings.ContainsAny(s, " \t\r\n\v\f") { return errors.New("gitcli: ref name contains whitespace") } + for i := 0; i < len(s); i++ { + if s[i] < 0x20 || s[i] == 0x7F { + return errors.New("gitcli: ref name contains control character") + } + } + if strings.ContainsAny(s, "~^:?[") { + return errors.New("gitcli: ref name contains ~, ^, :, ? or [") + } + if strings.HasSuffix(s, ".") { + return errors.New("gitcli: ref name ends in dot") + } if strings.Contains(s, "@{") { return errors.New("gitcli: ref name contains @{") } diff --git a/internal/gitcli/types_test.go b/internal/gitcli/types_test.go index dc570dbaf..25dd2ec4b 100644 --- a/internal/gitcli/types_test.go +++ b/internal/gitcli/types_test.go @@ -47,7 +47,11 @@ func TestValidateRefName(t *testing.T) { } bad := []RefName{"main", "heads/main", "refs/", "refs/heads/", "-refs/heads/x", "refs/heads/a b", "refs/heads/a..b", "refs/heads/a.lock", "refs/heads/a@{1}", - "refs/heads/*", "refs/heads/a\\z", "refs/heads/.hidden", "refs/heads/a\x00b"} + "refs/heads/*", "refs/heads/a\\z", "refs/heads/.hidden", "refs/heads/a\x00b", + // check-ref-format completion (change 0454): control chars, ~ ^ : ? [, trailing dot. + "refs/heads/a\x01b", "refs/heads/a\x1fb", "refs/heads/a\x7fb", + "refs/heads/a~b", "refs/heads/a^b", "refs/heads/a:b", + "refs/heads/a?b", "refs/heads/a[b", "refs/heads/a."} for _, r := range bad { if err := validateRefName(r); err == nil { t.Errorf("validateRefName(%q) accepted", r) @@ -55,6 +59,25 @@ func TestValidateRefName(t *testing.T) { } } +func TestValidBranchName(t *testing.T) { + good := []string{"main", "feat/x", "fix/whole-repository-status", "a.b/c-d"} + for _, b := range good { + if !ValidBranchName(b) { + t.Errorf("ValidBranchName(%q) = false, want true", b) + } + } + // Includes the two fixture names every later task reuses: feat/a..parent + // (old grammar already rejected) and feat/a:b (only the completed grammar + // rejects it locally; git itself always did). + bad := []string{"", "feat/a..parent", "feat/a:b", "a b", "a~b", "a^b", "a?b", + "a[b", "a.", "a\x01b", "@{x", "a\\b", "a*", ".hidden", "a.lock", "a/", "a//b"} + for _, b := range bad { + if ValidBranchName(b) { + t.Errorf("ValidBranchName(%q) = true, want false", b) + } + } +} + func TestValidateObjectID(t *testing.T) { sha1 := ObjectID(strings.Repeat("ab", 20)) sha256 := ObjectID(strings.Repeat("cd", 32))