fix: whole-repository status survives an unrelated change's invalid branch name (0454)

docs/results/2026-09-24-whole-repository-status-must-not-fail-on-an-unrelated-change-results.md

@@ -0,0 +1,41 @@
+<!-- docket:backlink:start (generated — do not hand-edit) -->
+> ↩ **[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)**
+<!-- docket:backlink:end -->
+# 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.

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

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) {

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)
+			}
+		})
+	}
+}

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,

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)
+			}
+		}
+	}
+}

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)
+	}
+}

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