From 525e62a4e19789c3abba731e768ec94bdf26e572 Mon Sep 17 00:00:00 2001 From: Andy Eschbacher Date: Thu, 16 Jul 2026 08:36:54 -0400 Subject: [PATCH] fix: planning path access --- internal/config/config.go | 43 ++++++++--- internal/config/discover_test.go | 47 ++++++++++++ internal/config/links_test.go | 23 +++--- internal/domain/body_fuzz_test.go | 115 ++++++++++++++++++++++++++++++ 4 files changed, 210 insertions(+), 18 deletions(-) create mode 100644 internal/domain/body_fuzz_test.go diff --git a/internal/config/config.go b/internal/config/config.go index 4b9c9c7..a4f2c43 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -177,18 +177,44 @@ func resolveRoot(dir string, cf configFile) (string, error) { // resolvePlanningRepo resolves planning_repo relative to the config dir (an // absolute value is used as-is) and validates it is a real planning root. Unlike // taskflow_root, it is ALLOWED to escape dir's tree — that is the entire point -// of pointing an impl repo at an external planning repo. A target without tasks/ -// is a loud error (the "require + validate" contract), never a clean empty tree. +// of pointing an impl repo at an external planning repo. +// +// The target names a planning REPO, which is not necessarily the planning ROOT: +// when it carries its own config, THAT config's taskflow_root decides the root, so +// a pointer at the repo root resolves the same tree `-C ` would (the +// subdir layout: entities under planning/). Pointing straight at the subdir keeps +// working — both spellings name the same planning repo, which is exactly what +// checkTrackedRepo already accepts. A target that is neither is a loud error (the +// "require + validate" contract), never a clean empty tree. func resolvePlanningRepo(dir, planningRepo string) (string, error) { root := filepath.FromSlash(planningRepo) if !filepath.IsAbs(root) { root = filepath.Join(dir, root) } root = filepath.Clean(root) + if cfgPath := filepath.Join(root, ConfigFile); fileExists(cfgPath) { + cf, err := readConfigFile(cfgPath) + if err != nil { + return "", err + } + // A target that is ITSELF a pointer is a chain: not followed, because it + // would need loop detection and leaves "where does my data live" unanswerable + // from one config. Loud, naming the real target so the fix is mechanical. + if cf.PlanningRepo != "" { + return "", fmt.Errorf( + "%w: planning_repo %q points at %s, which is itself a pointer (planning_repo = %q) — chains aren't followed; point at the planning repo directly", + domain.ErrValidation, planningRepo, root, cf.PlanningRepo) + } + target, err := configuredRoot(root, cf.TaskflowRoot) + if err != nil { + return "", fmt.Errorf("planning_repo %q: %w", planningRepo, err) + } + return evalOr(target), nil + } if !isDir(filepath.Join(root, domain.TasksDir)) { return "", fmt.Errorf( - "%w: planning_repo %q points at %s, which has no tasks/ — run `tskflwctl init` there first", - domain.ErrValidation, planningRepo, root) + "%w: planning_repo %q points at %s, which is not a planning repo (no tasks/ there, and no %s naming one via taskflow_root) — run `tskflwctl init` there first", + domain.ErrValidation, planningRepo, root, ConfigFile) } // Resolve symlinks so Root is PHYSICAL, matching the in-tree branch (Discover // evalOr's dir, which configuredRoot inherits). The linkback work compares @@ -538,10 +564,11 @@ func CheckLinks(cfg *Config) []LinkProblem { // this impl in its tracked_repos. func checkBackLink(cfg *Config) []LinkProblem { me := resolveRepoPath(cfg.Dir, ".") - planningRoot := resolveRepoPath(cfg.Dir, cfg.PlanningRepo) - // Find the planning repo's config wherever it lives (root or a taskflow_root - // ancestor) — NOT just at the root, which a subdir layout would miss. - pdir, pcf, ok := configForRoot(planningRoot) + // cfg.Root is the RESOLVED planning root (resolvePlanningRepo already followed + // the pointer, including the target's own taskflow_root). Re-resolving the raw + // pointer here instead would hand configForRoot a repo root rather than the + // root it governs, and a subdir layout would then falsely report "no config". + pdir, pcf, ok := configForRoot(cfg.Root) if !ok { return []LinkProblem{{cfg.PlanningRepo, fmt.Sprintf( "planning repo %q has no %s, so it can't track this repo back", cfg.PlanningRepo, ConfigFile)}} diff --git a/internal/config/discover_test.go b/internal/config/discover_test.go index f510170..9c78ae0 100644 --- a/internal/config/discover_test.go +++ b/internal/config/discover_test.go @@ -310,6 +310,53 @@ func TestDiscover_PlanningRepoMustBeValid(t *testing.T) { } } +// TestDiscover_PlanningRepoHonorsTargetTaskflowRoot regresses the desirelines +// break: a pointer at the planning REPO ROOT whose entities live under a +// taskflow_root subdir must resolve to that subdir — the same tree `-C ` +// finds. Resolution used to demand tasks/ directly under the target, so every +// command from the impl repo died with "has no tasks/" even though the target's +// own config said where the root was. +func TestDiscover_PlanningRepoHonorsTargetTaskflowRoot(t *testing.T) { + parent := t.TempDir() + impl := filepath.Join(parent, "impl") + plan := filepath.Join(parent, "plan") + mkdirs(t, impl, filepath.Join(plan, "planning", "tasks")) + writeConfig(t, plan, "taskflow_root = \"planning\"\n") + + // Pointing at the repo root: the target's config names the root. + writeConfig(t, impl, "planning_repo = \"../plan\"\n") + cfg, err := Discover(impl) + if err != nil { + t.Fatalf("a pointer at the repo root should follow its taskflow_root, got %v", err) + } + if want := eval(t, filepath.Join(plan, "planning")); cfg.Root != want { + t.Errorf("Root = %q, want the target's taskflow_root subdir %q", cfg.Root, want) + } + // Pointing straight at the subdir names the same repo and must still work. + writeConfig(t, impl, "planning_repo = \"../plan/planning\"\n") + if cfg, err := Discover(impl); err != nil || cfg.Root != eval(t, filepath.Join(plan, "planning")) { + t.Errorf("a pointer at the root subdir should keep resolving, got %v / %v", cfg, err) + } +} + +// TestDiscover_PlanningRepoChainRejected: a pointer at a repo that is ITSELF a +// pointer is a loud error, not a followed chain (no loop detection, and the data's +// location stops being answerable from one config). +func TestDiscover_PlanningRepoChainRejected(t *testing.T) { + parent := t.TempDir() + impl := filepath.Join(parent, "impl") + mid := filepath.Join(parent, "mid") + planning := filepath.Join(parent, "planning") + mkdirs(t, impl, mid, filepath.Join(planning, "tasks")) + writeConfig(t, impl, "planning_repo = \"../mid\"\n") + writeConfig(t, mid, "planning_repo = \"../planning\"\n") + + _, err := Discover(impl) + if err == nil || !strings.Contains(err.Error(), "chains aren't followed") { + t.Errorf("a pointer chain must be a loud error, got %v", err) + } +} + // TestDiscover_PlanningRepoVsTaskflowRoot pins precedence: planning_repo wins // over a default taskflow_root, but a NON-default taskflow_root alongside it is a // loud conflict (two roots), not a silent override. diff --git a/internal/config/links_test.go b/internal/config/links_test.go index 25c2a3e..785ae6d 100644 --- a/internal/config/links_test.go +++ b/internal/config/links_test.go @@ -39,9 +39,11 @@ func TestCheckLinks_SubdirPlanningLayout(t *testing.T) { // TestCheckLinks_SubdirPlanning_ImplPointsAtRepoRoot regresses the desirelines case: a // planning repo with taskflow_root="planning" whose tracked impl points its planning_repo at -// the REPO ROOT (not the taskflow_root subdir) must link cleanly. The planning-side check must -// accept the impl naming either the repo (Dir) or the subdir (Root) — comparing only against -// Root falsely flagged "points its planning_repo elsewhere". +// the REPO ROOT (not the taskflow_root subdir) must link cleanly FROM BOTH SIDES. +// Planning side: accept the impl naming either the repo (Dir) or the subdir (Root) — +// comparing only against Root falsely flagged "points its planning_repo elsewhere". +// Impl side: the back-link lookup must start from the RESOLVED root, not the raw pointer, +// which falsely reported "planning repo has no .tskflwctl.toml". func TestCheckLinks_SubdirPlanning_ImplPointsAtRepoRoot(t *testing.T) { parent := t.TempDir() plan := filepath.Join(parent, "plan") @@ -49,16 +51,17 @@ func TestCheckLinks_SubdirPlanning_ImplPointsAtRepoRoot(t *testing.T) { writeConfig(t, plan, "taskflow_root = \"planning\"\ntracked_repos = [\"../impl\"]\n") impl := filepath.Join(parent, "impl") mustMkdir(t, impl) - // Write the impl config directly: its planning_repo names the planning REPO ROOT - // (../plan). This is a valid LEGACY pointer — created by InitPointer back when tasks/ - // sat at the root, before an isolation moved them under taskflow_root. Discover still - // resolves it (it reads plan's config + taskflow_root); the link check must accept it. - // (InitPointer can't recreate it now — it rejects a target with no tasks/ directly under - // it — so the config is written by hand here to mirror the real desirelines state.) - writeConfig(t, impl, "planning_repo = \"../plan\"\n") + // The impl's planning_repo names the planning REPO ROOT (../plan). Discover resolves + // it by reading plan's config + taskflow_root, so both sides must see one linked pair. + if _, err := InitPointer(impl, "../plan", false); err != nil { + t.Fatalf("a pointer at the repo root of a subdir layout should init, got %v", err) + } if p := linksAt(t, plan); len(p) != 0 { t.Errorf("planning side must accept an impl that points at the repo root, got %v", p) } + if p := linksAt(t, impl); len(p) != 0 { + t.Errorf("impl side must see its back-link through the repo-root pointer, got %v", p) + } } // linksAt discovers cfg at dir and returns CheckLinks(cfg). diff --git a/internal/domain/body_fuzz_test.go b/internal/domain/body_fuzz_test.go new file mode 100644 index 0000000..ee0d0a3 --- /dev/null +++ b/internal/domain/body_fuzz_test.go @@ -0,0 +1,115 @@ +package domain + +import ( + "strings" + "testing" +) + +// These fuzz targets feed random input into the hand-rolled line/index parsers in +// body.go — Section, the acceptance scanner/flip, and the lint guard. That's the +// panic-prone surface (index math on untrusted markdown; a real off-by-one in +// flipCheckbox already slipped through once). Beyond no-panic they assert the +// load-bearing invariants: the tally and the list agree, a flip preserves the +// criterion count and sets the target state and is idempotent, and Section never +// fabricates a line. The seed corpus also runs under a normal `go test`. + +func bodySeeds() []string { + return []string{ + "", + "# Title\n\n## Acceptance criteria\n\n- [ ] a\n- [x] b\n- [X] c\n", + "## Acceptance criteria\r\n\r\n- [x] crlf\r\n- [ ] two\r\n", + "## Acceptance criteria\n\n```\n- [ ] fenced example\n```\n\n- [x] real\n", + "## acceptance CRITERIA\n- [] botched\n- [ x] botched2\n- [-] partial\n- [1] cite\n", + "## Acceptance\n\n- [X] cap only\n", + "#### deep first\n\n## Acceptance criteria\n\n- [ ] x\n\n## Notes\n\n- [ ] not ac\n", + "## A\n## B\n## C\n", + "```go\n## Acceptance criteria\n- [ ] hidden in fence\n```\n", + "## Acceptance criteria\n\n- [ ] uni é \U0001F600 中文\n", + "~~~\n## Acceptance criteria\n~~~\n## Acceptance criteria\n- [x] real after fence\n", + "## Acceptance criteria\n- [ ] a\n## Progress notes on acceptance criteria\n- [ ] b\n", + "## Acceptance criteria", + "- [ ] no heading at all\n", + "#\n##\n###\n", + } +} + +func normLF(s string) string { + return strings.ReplaceAll(strings.ReplaceAll(s, "\r\n", "\n"), "\r", "\n") +} + +func FuzzSection(f *testing.F) { + for _, s := range bodySeeds() { + f.Add(s, "acceptance") + f.Add(s, "") + f.Add(s, "progress") + } + f.Fuzz(func(t *testing.T, body, name string) { + sec, ok := Section(body, name) + if !ok { + if sec != "" { + t.Fatalf("Section not-ok but returned text %q", sec) + } + return + } + // A found section never fabricates content: every line it returns is a line of + // the newline-normalized body. + have := map[string]bool{} + for _, ln := range strings.Split(normLF(body), "\n") { + have[ln] = true + } + for _, ln := range strings.Split(sec, "\n") { + if !have[ln] { + t.Fatalf("Section fabricated a line %q absent from the body", ln) + } + } + }) +} + +func FuzzAcceptanceCriteria(f *testing.F) { + for _, s := range bodySeeds() { + for _, n := range []int{-1, 0, 1, 2, 1 << 20} { + f.Add(s, n) + } + } + f.Fuzz(func(t *testing.T, body string, n int) { + // 1. The tally and the list agree — they share the scanner, so pin it. + count := CountAcceptanceCriteria(body) + list := ListAcceptanceCriteria(body) + if count.Total != len(list) { + t.Fatalf("Count.Total=%d but len(list)=%d", count.Total, len(list)) + } + checked := 0 + for _, c := range list { + if c.Checked { + checked++ + } + } + if count.Checked != checked { + t.Fatalf("Count.Checked=%d but list has %d checked", count.Checked, checked) + } + + // 2. Lint never panics. + _ = LintAcceptanceCriteria(body) + + // 3. A flip either errors cleanly (out-of-range / no section) or preserves the + // criterion count, sets the target state, and is idempotent. + out, err := SetAcceptanceCriterion(body, n, true) + if err != nil { + return + } + got := ListAcceptanceCriteria(out) + if len(got) != len(list) { + t.Fatalf("flip changed the criterion count: %d -> %d", len(list), len(got)) + } + if n >= 1 && n <= len(got) && !got[n-1].Checked { + t.Fatalf("flip-to-checked left criterion %d unchecked", n) + } + out2, err := SetAcceptanceCriterion(out, n, true) + if err != nil { + t.Fatalf("a second identical flip errored: %v", err) + } + if out2 != out { + t.Fatalf("flip to the current state is not idempotent") + } + }) +}