From fbc8b6ebc8a2e61ff16a5b8419e10d12b983e437 Mon Sep 17 00:00:00 2001 From: Andy Eschbacher Date: Thu, 27 Aug 2026 17:56:03 -0400 Subject: [PATCH] feat(graph): add guarded repository mutations Add the canonical-root mutation boundary, pure prefix validation, exact source CAS, and production-path concurrency coverage. Record both adversarial audits and tighten the remaining Thread implementation sequence around use-case-specific guarded writes. --- docs/ARCHITECTURE.md | 30 +- internal/core/dependency_graph.go | 40 +- internal/core/dependency_graph_mutation.go | 119 +++ internal/core/dependency_graph_test.go | 34 + internal/core/service_task.go | 16 +- internal/core/store.go | 41 + internal/domain/task.go | 4 + internal/store/auditstore.go | 15 + internal/store/body.go | 6 + internal/store/create.go | 32 + internal/store/create_test.go | 17 + internal/store/danglers.go | 3 + internal/store/edit.go | 6 + internal/store/epicstore.go | 15 + internal/store/fix.go | 3 + internal/store/fsstore.go | 19 + internal/store/graphmutation.go | 194 ++++ internal/store/graphmutation_test.go | 523 +++++++++++ internal/store/lock.go | 136 +++ internal/store/lock_other.go | 20 +- internal/store/lock_unix.go | 16 +- internal/store/lock_unix_test.go | 263 ++++++ internal/store/paths.go | 28 +- internal/store/rename.go | 3 + internal/store/researchstore.go | 15 + internal/wire/schema_comments.json | 1 + .../adrs/0006-adopt-threads-as-task-dags.md | 44 + ...ortable-repository-graph-mutation-guard.md | 232 +++++ ...-repository-graph-mutation-guard-claude.md | 855 ++++++++++++++++++ .../30-threads-and-task-dependency-graphs.md | 74 +- ...aph-mutations-portable-and-serializable.md | 38 +- ...-dependency-mutations-and-graph-queries.md | 13 +- ...ligibility-across-every-task-start-path.md | 13 +- ...-entity-lifecycle-and-graph-projections.md | 12 +- ...tasks-into-threads-with-resumable-apply.md | 15 +- ...-usage-informed-thread-views-to-the-tui.md | 6 + 36 files changed, 2842 insertions(+), 59 deletions(-) create mode 100644 internal/core/dependency_graph_mutation.go create mode 100644 internal/store/graphmutation.go create mode 100644 internal/store/graphmutation_test.go create mode 100644 internal/store/lock.go create mode 100644 internal/store/lock_unix_test.go create mode 100644 planning/audits/6g45qfv27vrm-2026-08-27-portable-repository-graph-mutation-guard.md create mode 100644 planning/audits/6g45s2rm09pr-2026-08-27-portable-repository-graph-mutation-guard-claude.md diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 154e29c3..a123fa9b 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -161,16 +161,36 @@ adapter capabilities rather than leaked persistence. hands the TUI watcher its dir set so the path convention isn't reconstructed outside the store. Task dependency fields are graph-owned: generic create/set/edit paths cannot introduce a semantic delta, and text-level lint repair skips a would-be - dependency normalization instead of manufacturing unchecked edges. The future guarded - dependency port will own the repository-wide read/validate/write critical section. + dependency normalization instead of manufacturing unchecked edges. + `TaskGraphMutationStore` is the control-inverted write capability: `FS` takes the + repository guard, loads the canonical strict snapshot, invokes a pure core planner over + taskflow-owned values, asks core to validate the complete plan and every recovery prefix, + applies surgical task dependency writes through lock-free internal helpers, and releases + the guard. The planner callback is exclusive at canonical-planning-root scope: any Store + call through the same or another `FS` during that brief phase fails with `ErrConflict` + instead of escaping the snapshot or self-deadlocking. This intentionally includes an + unrelated concurrent caller; a future long-lived adapter may retry after the mutation. + Multi-file plans are deterministic and resumable. Planner-provided write order is + semantic recovery data (stable-ID sorting is not generally prefix-safe); each replacement + is atomic, every supplied prefix is validated as non-broken before the first write, and an + error result identifies the durable applied prefix. A private byte-version on each scanned + task supports a whole-snapshot CAS before application plus a per-target CAS immediately + before each replacement; planner-facing task projections never expose those tokens. + Dependency writes stamp `updated_at` from the caller-injected clock only when graph-owned + fields actually change. A dry run holds the same exclusive guard for an authoritative + preview but, because it writes nothing, makes no CAS durability claim about later apply. Concurrency is **version-CAS** (epic 24): every write, just before committing, re-resolves the file by its **id** and re-hashes it against the content read at the start of the op (`verifyUnchanged` in `cas.go` — a strong whole-file SHA-256 computed on read, **never stored**), so a concurrent - in-place edit is `ErrConflict` (exit 14). A repo-wide advisory `flock` - (`writeLock`, unix; a no-op stub elsewhere) serializes the verify→write so that CAS is + in-place edit is `ErrConflict` (exit 14). The repository guard combines a + canonical-root in-process mutex with root-directory `flock` on Unix. The supported + release matrix is macOS and Linux; Windows and other non-Unix source builds reject + repository mutation explicitly until they have a runtime-tested, shared-repository lock + identity. This serializes verify→write for cooperating writers so that CAS is *atomic* — without it two writers both pass their verify before either renames and the - later silently clobbers the earlier (the verify→rename window, widened by the temp fsync). + later silently clobbers the earlier. Raw editors do not honor the advisory lock; the + immediate content check narrows but cannot eliminate their verify→rename race. The token is **internal**: scriptable mutations auto-retry it in `core.Service` (bounded + jittered, so agents don't reimplement the loop), the human `edit` surfaces the conflict (no retry, and the lock is held only for the write, never the editor session), and creates map diff --git a/internal/core/dependency_graph.go b/internal/core/dependency_graph.go index ad6a017b..0f380dfc 100644 --- a/internal/core/dependency_graph.go +++ b/internal/core/dependency_graph.go @@ -3,6 +3,7 @@ package core import ( "fmt" "path/filepath" + "slices" "sort" "strings" "sync" @@ -727,7 +728,44 @@ func (g *TaskGraph) LegacyDiagnostics() []LegacyDependencyDiagnostic { func (g *TaskGraph) Task(taskID string) (domain.Task, bool) { task, ok := g.tasks[taskID] - return cloneTask(task), ok + task = cloneTask(task) + task.SourceVersion = "" // persistence token is not planner/query data + return task, ok +} + +// SameSourceSnapshot reports whether two graphs came from the same exact task +// files. It exposes no version token: the store can use it as a whole-repository +// CAS after planning, while callbacks receive only Task() projections with the +// tokens cleared. Health and paths catch new unreadable/renamed entities; byte +// hashes catch every in-place edit, including non-graph fields. +func (g *TaskGraph) SameSourceSnapshot(other *TaskGraph) bool { + if other == nil || g.health != other.health || !slices.Equal(g.ids, other.ids) { + return false + } + for _, taskID := range g.ids { + left, right := g.tasks[taskID], other.tasks[taskID] + if left.Path != right.Path || left.SourceVersion == "" || left.SourceVersion != right.SourceVersion { + return false + } + } + return slices.EqualFunc(g.problems, other.problems, sameGraphProblem) && + slices.EqualFunc(g.legacy, other.legacy, sameLegacyDiagnostic) +} + +func sameGraphProblem(left, right GraphProblem) bool { + return left.Code == right.Code && left.TaskID == right.TaskID && + left.RelatedTaskID == right.RelatedTaskID && left.Field == right.Field && + left.Path == right.Path && left.Message == right.Message && + slices.Equal(left.Cycle, right.Cycle) +} + +func sameLegacyDiagnostic(left, right LegacyDependencyDiagnostic) bool { + return left.TaskID == right.TaskID && left.TaskSlug == right.TaskSlug && + left.TaskPath == right.TaskPath && left.Field == right.Field && + slices.EqualFunc(left.References, right.References, func(a, b LegacyReference) bool { + return a.Value == b.Value && a.Resolution == b.Resolution && + a.Edge == b.Edge && slices.Equal(a.CandidateIDs, b.CandidateIDs) + }) } func (g *TaskGraph) TaskIDs() []string { return append([]string(nil), g.ids...) } diff --git a/internal/core/dependency_graph_mutation.go b/internal/core/dependency_graph_mutation.go new file mode 100644 index 00000000..e0756710 --- /dev/null +++ b/internal/core/dependency_graph_mutation.go @@ -0,0 +1,119 @@ +package core + +import ( + "fmt" + "sort" + + "github.com/andy-esch/taskflow/internal/domain" + "github.com/andy-esch/taskflow/internal/id" +) + +// ValidateTaskGraphMutationSource rejects an authoritative snapshot that cannot +// support a sound write. Degraded legacy snapshots remain eligible for the one +// guarded migration that converges them to the canonical field. +func ValidateTaskGraphMutationSource(graph *TaskGraph) error { + if graph == nil { + return fmt.Errorf("%w: authoritative task graph is required", domain.ErrValidation) + } + if graph.Health() == GraphBroken { + return fmt.Errorf("%w: repository task graph is broken; repair it before mutation: %s", + domain.ErrValidation, graphMutationHealthDetail(graph)) + } + return nil +} + +// ValidateTaskGraphMutationPlan normalizes task-owned sets and proves that every +// planner-ordered durable prefix, plus the final state, remains a sound graph. It +// is pure so command planners can validate and preview without a filesystem; the +// store owns only locking, materialization, CAS, and atomic replacement. +func ValidateTaskGraphMutationPlan(graph *TaskGraph, plan TaskGraphMutationPlan) (TaskGraphMutationPlan, error) { + if graph == nil { + return TaskGraphMutationPlan{}, fmt.Errorf("%w: authoritative task graph is required", domain.ErrValidation) + } + normalized := TaskGraphMutationPlan{TaskWrites: make([]TaskDependencyWrite, len(plan.TaskWrites))} + copy(normalized.TaskWrites, plan.TaskWrites) + for i := range normalized.TaskWrites { + normalized.TaskWrites[i].DependsOn = append([]string(nil), normalized.TaskWrites[i].DependsOn...) + } + + seenTasks := make(map[string]bool, len(normalized.TaskWrites)) + taskIDs := graph.TaskIDs() + prospective := make(map[string]domain.Task, len(taskIDs)) + for _, taskID := range taskIDs { + task, _ := graph.Task(taskID) + prospective[taskID] = task + } + for i := range normalized.TaskWrites { + write := &normalized.TaskWrites[i] + if !id.Valid(write.TaskID) { + return TaskGraphMutationPlan{}, fmt.Errorf("%w: planned task id %q is not a stable task id", domain.ErrValidation, write.TaskID) + } + if seenTasks[write.TaskID] { + return TaskGraphMutationPlan{}, fmt.Errorf("%w: graph mutation plans task %s more than once", domain.ErrValidation, write.TaskID) + } + seenTasks[write.TaskID] = true + task, exists := prospective[write.TaskID] + if !exists { + return TaskGraphMutationPlan{}, fmt.Errorf("%w: planned task %s does not exist in the authoritative snapshot", domain.ErrValidation, write.TaskID) + } + + seenDependencies := make(map[string]bool, len(write.DependsOn)) + for _, prerequisite := range write.DependsOn { + switch { + case !id.Valid(prerequisite): + return TaskGraphMutationPlan{}, fmt.Errorf("%w: planned dependency %q for task %s is not a stable task id", domain.ErrValidation, prerequisite, write.TaskID) + case prerequisite == write.TaskID: + return TaskGraphMutationPlan{}, fmt.Errorf("%w: task %s cannot depend on itself", domain.ErrValidation, write.TaskID) + case seenDependencies[prerequisite]: + return TaskGraphMutationPlan{}, fmt.Errorf("%w: task %s repeats planned dependency %s", domain.ErrValidation, write.TaskID, prerequisite) + } + if _, exists := prospective[prerequisite]; !exists { + return TaskGraphMutationPlan{}, fmt.Errorf("%w: planned dependency %s for task %s does not exist", domain.ErrValidation, prerequisite, write.TaskID) + } + seenDependencies[prerequisite] = true + } + sort.Strings(write.DependsOn) + task.DependsOn = append([]string(nil), write.DependsOn...) + if write.ClearLegacy { + task.LegacyBlockedBy = nil + task.LegacyDependencies = nil + task.LegacyBlocks = nil + } + prospective[write.TaskID] = task + + prefixGraph := taskGraphFromMap(taskIDs, prospective) + if prefixGraph.Health() == GraphBroken { + return TaskGraphMutationPlan{}, fmt.Errorf("%w: planned write prefix ending at task %s would leave a broken graph: %s", + domain.ErrValidation, write.TaskID, graphMutationHealthDetail(prefixGraph)) + } + } + + finalGraph := taskGraphFromMap(taskIDs, prospective) + if !finalGraph.MutationReady() { + return TaskGraphMutationPlan{}, fmt.Errorf("%w: planned dependency state is %s; mutation requires a healthy final graph: %s", + domain.ErrValidation, finalGraph.Health(), graphMutationHealthDetail(finalGraph)) + } + return normalized, nil +} + +func taskGraphFromMap(taskIDs []string, tasksByID map[string]domain.Task) *TaskGraph { + tasks := make([]domain.Task, 0, len(taskIDs)) + for _, taskID := range taskIDs { + tasks = append(tasks, tasksByID[taskID]) + } + return NewTaskGraph(tasks, nil) +} + +func graphMutationHealthDetail(graph *TaskGraph) string { + if problems := graph.Problems(); len(problems) > 0 { + detail := problems[0].Message + if len(problems) > 1 { + detail += fmt.Sprintf(" (%d additional problem(s); run lint for the full sweep)", len(problems)-1) + } + return detail + } + if legacy := graph.LegacyDiagnostics(); len(legacy) > 0 { + return fmt.Sprintf("%d legacy dependency field occurrence(s) remain; run the guarded migration", len(legacy)) + } + return "graph health is not mutation-ready" +} diff --git a/internal/core/dependency_graph_test.go b/internal/core/dependency_graph_test.go index 379e48c1..64d493b6 100644 --- a/internal/core/dependency_graph_test.go +++ b/internal/core/dependency_graph_test.go @@ -1,6 +1,7 @@ package core import ( + "errors" "fmt" "math/rand" "reflect" @@ -477,6 +478,39 @@ func TestTaskGraphReopenInvalidatesCompletedDescendantsWithoutRewriting(t *testi } } +func TestTaskGraphSameSourceSnapshotComparesUnreadableIdentity(t *testing.T) { + left := NewTaskGraph(nil, []domain.FileProblem{{Path: "/planning/tasks/aaaaaaaaaaaa-left.md", Message: "bad yaml"}}) + right := NewTaskGraph(nil, []domain.FileProblem{{Path: "/planning/tasks/bbbbbbbbbbbb-right.md", Message: "bad yaml"}}) + if left.SameSourceSnapshot(right) { + t.Fatal("different unreadable task sets compared as the same source snapshot") + } +} + +func TestValidateTaskGraphMutationPlanPreservesSemanticWriteOrder(t *testing.T) { + alpha := graphRecord("alpha", domain.StatusReadyToStart) + beta := graphRecord("beta", domain.StatusReadyToStart, alpha.ID) + graph := NewTaskGraph([]domain.Task{alpha, beta}, nil) + plan := TaskGraphMutationPlan{TaskWrites: []TaskDependencyWrite{ + {TaskID: beta.ID}, + {TaskID: alpha.ID, DependsOn: []string{beta.ID}}, + }} + validated, err := ValidateTaskGraphMutationPlan(graph, plan) + if err != nil { + t.Fatal(err) + } + if validated.TaskWrites[0].TaskID != beta.ID || validated.TaskWrites[1].TaskID != alpha.ID { + t.Fatalf("validator reordered semantic durable prefixes: %+v", validated.TaskWrites) + } + + unsafe := TaskGraphMutationPlan{TaskWrites: []TaskDependencyWrite{ + {TaskID: alpha.ID, DependsOn: []string{beta.ID}}, + {TaskID: beta.ID}, + }} + if _, err := ValidateTaskGraphMutationPlan(graph, unsafe); !errors.Is(err, domain.ErrValidation) { + t.Fatalf("unsafe planner order error = %v", err) + } +} + func sortStrings(values []string) { slices.Sort(values) } diff --git a/internal/core/service_task.go b/internal/core/service_task.go index 8b7b581a..25182b90 100644 --- a/internal/core/service_task.go +++ b/internal/core/service_task.go @@ -77,12 +77,16 @@ func (s *Service) ShowTask(slug string) (domain.Task, string, error) { return s.store.GetTask(slug) } -// ReadTaskGraph performs one resilient repository scan and projects it into the -// strict, immutable dependency snapshot. Files remain repair-listable through the -// ordinary store API; graph consumers instead inspect Health/Problems and fail -// closed when the snapshot is degraded or broken. -func (s *Service) ReadTaskGraph() (*TaskGraph, error) { - tasks, problems, err := s.store.ListTasks() +// LoadTaskGraph is the one canonical filesystem-agnostic snapshot loader used by +// the guarded mutation boundary. Diagnostic consumers that already own task bodies +// (notably lint) construct the same strict projection with NewTaskGraph rather than +// performing a second repository scan. +type TaskGraphSource interface { + ListTasks() ([]domain.Task, []domain.FileProblem, error) +} + +func LoadTaskGraph(source TaskGraphSource) (*TaskGraph, error) { + tasks, problems, err := source.ListTasks() if err != nil { return nil, err } diff --git a/internal/core/store.go b/internal/core/store.go index 000b3745..abc27a11 100644 --- a/internal/core/store.go +++ b/internal/core/store.go @@ -56,6 +56,47 @@ type TaskStore interface { RenameTask(slug, newTitle string, dryRun bool) (domain.Task, int, error) } +// TaskDependencyWrite is one semantic task-file change returned by a pure graph +// mutation planner. The store owns YAML surgery and atomic replacement; planners +// name only the task and its complete canonical dependency set. ClearLegacy is +// reserved for the guarded migration from blocked_by/dependencies/blocks. +type TaskDependencyWrite struct { + TaskID string + DependsOn []string + ClearLegacy bool +} + +// TaskGraphMutationPlan is the complete deterministic write set produced from one +// immutable repository snapshot. Writes are applied in the planner-provided order: +// ordering is semantic recovery data because every durable prefix must remain sound. +// Each file is replaced atomically; AppliedTaskIDs in the result is the durable prefix +// when a later write fails, which makes a multi-file operation diagnosable and resumable. +type TaskGraphMutationPlan struct { + TaskWrites []TaskDependencyWrite +} + +// TaskGraphMutationResult reports what the store planned and which task-file +// replacements actually landed. Dry runs return the normalized plan with no applied +// IDs. A non-nil error may accompany a non-empty AppliedTaskIDs prefix. +type TaskGraphMutationResult struct { + Plan TaskGraphMutationPlan + AppliedTaskIDs []string + DryRun bool +} + +// TaskGraphPlanner is deliberately control-inverted: the store calls a pure core +// planner while it owns the repository guard. The callback receives only the +// immutable taskflow graph and returns taskflow-owned semantic values; it must not +// call a Store method or begin another mutation. +type TaskGraphPlanner func(*TaskGraph) (TaskGraphMutationPlan, error) + +// TaskGraphMutationStore owns the repository-wide graph read/validate/write +// critical section. It is a sibling capability rather than part of Store so read- +// only/test adapters do not acquire a mutation method they cannot implement. +type TaskGraphMutationStore interface { + MutateTaskGraph(now time.Time, dryRun bool, planner TaskGraphPlanner) (TaskGraphMutationResult, error) +} + // EpicStore is the epic-persistence port. type EpicStore interface { ListEpics() ([]domain.Epic, []domain.FileProblem, error) diff --git a/internal/domain/task.go b/internal/domain/task.go index 6ea3ab83..4e91a9e5 100644 --- a/internal/domain/task.go +++ b/internal/domain/task.go @@ -5,6 +5,10 @@ package domain type Task struct { Slug string `yaml:"-"` Path string `yaml:"-"` + // SourceVersion is the store-internal hash of the exact bytes that produced this + // record. TaskGraph retains it for whole-snapshot CAS but clears it from Task() + // projections, so planners never receive persistence tokens. + SourceVersion string `yaml:"-"` // StatusFellBack is set by the store when the frontmatter status is missing or // unrecognized — under the flat layout (ADR-0003 §4) there is no directory to fall // back to, so Status keeps its raw value; the task still lists and lint flags it diff --git a/internal/store/auditstore.go b/internal/store/auditstore.go index 130163b3..30547dc3 100644 --- a/internal/store/auditstore.go +++ b/internal/store/auditstore.go @@ -15,6 +15,9 @@ import ( // ListAudits scans every audit bucket. Unreadable audits are skipped and // reported as FileProblems. func (s *FS) ListAudits() ([]domain.Audit, []domain.FileProblem, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return nil, nil, err + } return scanDir(s.auditsDir, func(path string, content []byte) (domain.Audit, error) { return parseAudit(content, path) }) @@ -25,6 +28,9 @@ func (s *FS) ListAudits() ([]domain.Audit, []domain.FileProblem, error) { // so Summary reads each audit once for both the tally and the findings rollup // instead of re-reading every body through GetAuditByPath. func (s *FS) ListAuditsWithFindings() ([]core.AuditWithFindings, []domain.FileProblem, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return nil, nil, err + } return scanDir(s.auditsDir, func(path string, content []byte) (core.AuditWithFindings, error) { a, findings, err := parseAuditWithFindings(content, path) return core.AuditWithFindings{Audit: a, Findings: findings}, err @@ -33,6 +39,9 @@ func (s *FS) ListAuditsWithFindings() ([]core.AuditWithFindings, []domain.FilePr // GetAudit returns one audit plus its markdown body. func (s *FS) GetAudit(slug string) (domain.Audit, string, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Audit{}, "", err + } path, err := s.resolveAudit(slug) if err != nil { return domain.Audit{}, "", err @@ -54,6 +63,9 @@ func (s *FS) GetAudit(slug string) (domain.Audit, string, error) { // slug. The finding/lint sweeps use this to read each audit ListAudits already // found exactly once, which also closes the concurrent-edit window a re-resolve opens. func (s *FS) GetAuditByPath(path string) (domain.Audit, string, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Audit{}, "", err + } content, err := os.ReadFile(path) if err != nil { return domain.Audit{}, "", fmt.Errorf("read audit %s: %w", path, err) @@ -70,6 +82,9 @@ func (s *FS) GetAuditByPath(path string) (domain.Audit, string, error) { // `bucket:` frontmatter in place — under the flat layout (ADR-0003 §4) there is no bucket // directory to move between. Moving to the bucket it already declares is an idempotent no-op. func (s *FS) MoveAudit(slug string, to domain.AuditBucket, dryRun bool) (domain.Audit, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Audit{}, err + } if !to.Valid() { return domain.Audit{}, fmt.Errorf("%q: %w", to, domain.ErrValidation) } diff --git a/internal/store/body.go b/internal/store/body.go index 34522d98..a796f79e 100644 --- a/internal/store/body.go +++ b/internal/store/body.go @@ -78,6 +78,9 @@ func writeBody[T any]( // (parse-before-accept, compare-and-swap, dry-run, body echo) lives in writeBody. // Returns the reloaded audit and the resulting (LF) body. func (s *FS) AppendAuditBody(slug, text string, now time.Time, dryRun bool) (domain.Audit, string, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Audit{}, "", err + } path, err := s.resolveAudit(slug) if err != nil { return domain.Audit{}, "", err @@ -110,6 +113,9 @@ func (s *FS) AppendAuditBody(slug, text string, now time.Time, dryRun bool) (dom // comments, and key order survive) and updated_at is stamped. The shared write tail // lives in writeBody. Returns the reloaded task and the resulting (LF) body. func (s *FS) EditBody(slug, text string, appendMode bool, now time.Time, dryRun bool) (domain.Task, string, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Task{}, "", err + } path, err := s.resolve(slug) if err != nil { return domain.Task{}, "", err diff --git a/internal/store/create.go b/internal/store/create.go index 24b7c116..65520ae9 100644 --- a/internal/store/create.go +++ b/internal/store/create.go @@ -51,6 +51,26 @@ func (s *FS) writeNewFile(dir, path string, content []byte, kind, id string, dry } return nil } + // Preserve the store's historical ability to create the first entity in a + // not-yet-existing root; the directory-backed Unix lock needs the root first. + if err := os.MkdirAll(s.root, 0o755); err != nil { + return fmt.Errorf("mkdir planning root %s: %w", s.root, err) + } + unlock, err := s.writeLock() + if err != nil { + return err + } + defer unlock() + return s.writeNewFileUnlocked(dir, path, content, kind, id) +} + +// writeNewFileUnlocked is the graph-guard-compatible create primitive. Public +// entity creation enters through writeNewFile and takes the repository lock; +// future compound graph operations may call this helper only while already guarded. +func (s *FS) writeNewFileUnlocked(dir, path string, content []byte, kind, id string) error { + conflict := func() error { + return fmt.Errorf("%s %q already exists: %w", kind, id, domain.ErrConflict) + } if err := os.MkdirAll(dir, 0o755); err != nil { return fmt.Errorf("mkdir %s: %w", dir, err) } @@ -110,6 +130,9 @@ func validEntityID(entityID string) error { } func (s *FS) CreateTask(t domain.Task, body string, dryRun bool) (domain.Task, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Task{}, err + } if t.Slug == "" { return domain.Task{}, fmt.Errorf("%w: empty task slug", domain.ErrValidation) } @@ -152,6 +175,9 @@ func auditFields(a domain.Audit) []fmField { // CreateAudit writes a new audit at audits/-.md (flat, id-led per // ADR-0003 §4). New audits always start in the open bucket; it refuses to clobber. func (s *FS) CreateAudit(a domain.Audit, body string, dryRun bool) (domain.Audit, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Audit{}, err + } if a.Slug == "" { return domain.Audit{}, fmt.Errorf("%w: empty audit slug", domain.ErrValidation) } @@ -194,6 +220,9 @@ func researchFields(r domain.Research) []fmField { // per ADR-0003 §4). It refuses to clobber; the slug and id are taken from r. The id is // minted from r.Created by the caller (core), so ids stay chronological. func (s *FS) CreateResearch(r domain.Research, body string, dryRun bool) (domain.Research, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Research{}, err + } if r.Slug == "" { return domain.Research{}, fmt.Errorf("%w: empty research slug", domain.ErrValidation) } @@ -295,6 +324,9 @@ func epicFields(e domain.Epic) []fmField { // deliberately allowed — they stay distinct ids; only `epic show billing` goes // fuzzy-ambiguous, recoverable by using the full NN-slug. func (s *FS) CreateEpic(slug string, e domain.Epic, body string, dryRun bool) (domain.Epic, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Epic{}, err + } if slug == "" { return domain.Epic{}, fmt.Errorf("%w: empty epic slug", domain.ErrValidation) } diff --git a/internal/store/create_test.go b/internal/store/create_test.go index 314f6a36..24fc5e85 100644 --- a/internal/store/create_test.go +++ b/internal/store/create_test.go @@ -50,6 +50,23 @@ func TestCreateTask_OrderQuotingClobber(t *testing.T) { } } +func TestCreateTaskCreatesMissingPlanningRootBeforeLocking(t *testing.T) { + root := filepath.Join(t.TempDir(), "new-planning-root") + fs := NewFS(root) + task := domain.Task{ + Slug: "first", ID: "0abcdef12345", Status: domain.StatusReadyToStart, Epic: "e1", + Description: "first task", Effort: "Unknown", Tier: 3, + Priority: "medium", Autonomy: 3, Tags: []string{"a"}, Created: "2026-08-27", + } + got, err := fs.CreateTask(task, "# First\n", false) + if err != nil { + t.Fatal(err) + } + if _, err := os.Stat(got.Path); err != nil { + t.Fatalf("created task path: %v", err) + } +} + func TestCreateTask_IDRoundTrips(t *testing.T) { fs := NewFS(t.TempDir()) // Alphanumeric and all-digit ids: the latter must survive YAML as a string, not diff --git a/internal/store/danglers.go b/internal/store/danglers.go index e7b10f46..1c35f0e2 100644 --- a/internal/store/danglers.go +++ b/internal/store/danglers.go @@ -16,6 +16,9 @@ import ( // #fragment or ?query is stripped before the existence check. It is the Scheme-2 dangler // check `lint --links` surfaces. func (s *FS) DanglingLinks() ([]domain.FileProblem, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return nil, err + } var out []domain.FileProblem err := filepath.WalkDir(s.root, func(p string, d os.DirEntry, err error) error { if err != nil { diff --git a/internal/store/edit.go b/internal/store/edit.go index 0ac9435e..6075e42e 100644 --- a/internal/store/edit.go +++ b/internal/store/edit.go @@ -146,6 +146,9 @@ func editFile[T any]( // old status directory — a permanent ErrAmbiguous). Returns the reloaded task and // whether it changed. func (s *FS) EditTask(slug string, now time.Time, edit func(current string, prevErr error) (string, error)) (domain.Task, bool, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Task{}, false, err + } path, err := s.resolve(slug) if err != nil { return domain.Task{}, false, err @@ -244,6 +247,9 @@ func dependencyValues(content []byte) (taskDependencyFields, bool) { // lint (status vocab, bucket↔state) is left to the caller, mirroring how task edit // leaves field lint to `lint` — the store only guarantees the file still parses. func (s *FS) EditAudit(slug string, now time.Time, edit func(current string, prevErr error) (string, error)) (domain.Audit, bool, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Audit{}, false, err + } path, err := s.resolveAudit(slug) if err != nil { return domain.Audit{}, false, err diff --git a/internal/store/epicstore.go b/internal/store/epicstore.go index d2e8a3c3..507546c2 100644 --- a/internal/store/epicstore.go +++ b/internal/store/epicstore.go @@ -16,6 +16,9 @@ import ( // ListEpics parses every epics/*.md file. Unreadable epics are skipped and // reported as FileProblems (resilient, like ListTasks). func (s *FS) ListEpics() ([]domain.Epic, []domain.FileProblem, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return nil, nil, err + } epics, problems, err := scanDir(s.epicsDir, func(path string, content []byte) (domain.Epic, error) { return parseEpic(content, path) }) @@ -35,6 +38,9 @@ func (s *FS) ListEpics() ([]domain.Epic, []domain.FileProblem, error) { // GetEpic returns one epic plus its markdown body. The id resolves exact // first, then fuzzy (unique prefix/substring), like task and audit slugs. func (s *FS) GetEpic(id string) (domain.Epic, string, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Epic{}, "", err + } cands, err := epicCandidates(s.epicsDir) // epics have no status/bucket dir if err != nil { return domain.Epic{}, "", err @@ -63,6 +69,9 @@ func (s *FS) GetEpic(id string) (domain.Epic, string, error) { // parse-before-commit guard: a status that wouldn't reload is rejected with the // file untouched. func (s *FS) MoveEpic(id, status string, now time.Time, dryRun bool) (domain.Epic, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Epic{}, err + } if err := domain.ValidateEpicStatus(status); err != nil { return domain.Epic{}, err } @@ -127,6 +136,9 @@ func (s *FS) MoveEpic(id, status string, now time.Time, dryRun bool) (domain.Epi // untouched (ErrValidation, not a FileProblem — the user's update is bad, the file // on disk was never the cause). func (s *FS) SetEpicFields(id string, updates map[string]any, dryRun bool) (domain.Epic, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Epic{}, err + } cands, err := epicCandidates(s.epicsDir) // epics have no status/bucket dir if err != nil { return domain.Epic{}, err @@ -182,6 +194,9 @@ func (s *FS) SetEpicFields(id string, updates map[string]any, dryRun bool) (doma // but the version-CAS recheck still catches a concurrent edit during the editor window. // Returns the reloaded epic and whether it changed. func (s *FS) EditEpic(id string, now time.Time, edit func(current string, prevErr error) (string, error)) (domain.Epic, bool, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Epic{}, false, err + } cands, err := epicCandidates(s.epicsDir) // epics have no status/bucket dir if err != nil { return domain.Epic{}, false, err diff --git a/internal/store/fix.go b/internal/store/fix.go index c7f29059..022c4ff8 100644 --- a/internal/store/fix.go +++ b/internal/store/fix.go @@ -23,6 +23,9 @@ import ( // text-normalized only. Under the flat layout there is no relocation: a bad // status/bucket is lint-flagged, not moved. When dryRun is true nothing is written. func (s *FS) FixFrontmatter(dryRun bool) ([]domain.FixResult, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return nil, err + } // `lint --fix` is a batch SECOND writer; take the repo write-lock (like every other // write path) so a concurrent agent write — the cron writers, notably — can't be // silently clobbered. Reads happen inside the lock too, so each file's read→fix→write is diff --git a/internal/store/fsstore.go b/internal/store/fsstore.go index e57ed034..38562296 100644 --- a/internal/store/fsstore.go +++ b/internal/store/fsstore.go @@ -78,6 +78,9 @@ func (s *FS) WatchPaths() []string { // A file with unreadable frontmatter is skipped and reported as a FileProblem // (so one bad file doesn't blind the whole listing); err is only for fatal I/O. func (s *FS) ListTasks() ([]domain.Task, []domain.FileProblem, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return nil, nil, err + } return scanDir(s.tasksDir, func(path string, content []byte) (domain.Task, error) { return parseTask(content, path) }) @@ -87,6 +90,9 @@ func (s *FS) ListTasks() ([]domain.Task, []domain.FileProblem, error) { // pass), so lint's acceptance-criteria checks read every file once — the task twin of // ListAuditsWithFindings. func (s *FS) ListTasksWithBodies() ([]core.TaskWithBody, []domain.FileProblem, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return nil, nil, err + } return scanDir(s.tasksDir, func(path string, content []byte) (core.TaskWithBody, error) { t, err := parseTask(content, path) if err != nil { @@ -99,6 +105,9 @@ func (s *FS) ListTasksWithBodies() ([]core.TaskWithBody, []domain.FileProblem, e // GetTask returns a single task plus its markdown body. func (s *FS) GetTask(slug string) (domain.Task, string, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Task{}, "", err + } path, err := s.resolve(slug) if err != nil { return domain.Task{}, "", err @@ -119,6 +128,9 @@ func (s *FS) GetTask(slug string) (domain.Task, string, error) { // dates) and relocates the file to the target status directory. Moving to the // current status is an idempotent no-op. func (s *FS) Move(slug string, to domain.Status, now time.Time, dryRun, force bool) (domain.Task, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Task{}, err + } return s.moveTask(slug, to, now, dryRun, force, nil) } @@ -128,6 +140,9 @@ func (s *FS) Move(slug string, to domain.Status, now time.Time, dryRun, force bo // date if the second write failed. An empty until is a plain move to deferred; // re-deferring an already-deferred task rewrites revisit_at in place. func (s *FS) Defer(slug, until string, now time.Time, dryRun bool) (domain.Task, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Task{}, err + } var extra map[string]any if until != "" { extra = map[string]any{"revisit_at": until} @@ -253,6 +268,9 @@ func (s *FS) moveTask(slug string, to domain.Status, now time.Time, dryRun, forc // SetFields surgically updates frontmatter fields on a task (no status/dir // change) and writes the file atomically in place. func (s *FS) SetFields(slug string, updates map[string]any, dryRun bool) (domain.Task, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Task{}, err + } // Defense-in-depth: a status change relocates the file, so it must go through Move, // never an in-place field write (the core SetFields already rejects it). A direct // store caller writing status here would desync the mirror dir from the frontmatter. @@ -413,6 +431,7 @@ func parseTask(content []byte, path string) (domain.Task, error) { t.Slug = slug t.FilenameID = fnID t.Path = path + t.SourceVersion = hashContent(content) return t, nil } diff --git a/internal/store/graphmutation.go b/internal/store/graphmutation.go new file mode 100644 index 00000000..da1a400c --- /dev/null +++ b/internal/store/graphmutation.go @@ -0,0 +1,194 @@ +package store + +import ( + "bytes" + "errors" + "fmt" + "os" + "slices" + "time" + + "github.com/andy-esch/taskflow/internal/core" + "github.com/andy-esch/taskflow/internal/domain" +) + +var _ core.TaskGraphMutationStore = (*FS)(nil) + +// MutateTaskGraph owns the complete cooperating-writer critical section: take +// the repository lock, load the canonical strict snapshot, invoke the pure core +// planner, validate and materialize its semantic writes, apply them through the +// package's lock-free atomic helper, and finally release the lock. +// +// Multi-file plans are deliberately resumable rather than transactionally +// atomic: every file replacement is atomic and the result names the durable +// prefix if a later replacement fails. The planner must therefore be convergent +// when re-run, matching bulk-linking's additive/idempotent contract in ADR-0006. +func (s *FS) MutateTaskGraph(now time.Time, dryRun bool, planner core.TaskGraphPlanner) (result core.TaskGraphMutationResult, err error) { + result.DryRun = dryRun + if planner == nil { + return result, fmt.Errorf("%w: graph mutation planner is required", domain.ErrValidation) + } + if now.IsZero() { + return result, fmt.Errorf("%w: graph mutation time is required", domain.ErrValidation) + } + if err := s.rejectGraphPlannerCall(); err != nil { + return result, err + } + + unlock, err := s.checkedWriteLock() + if err != nil { + return result, err + } + defer func() { + if releaseErr := unlock(); releaseErr != nil { + wrapped := fmt.Errorf("release repository graph mutation guard: %w", releaseErr) + if err == nil { + err = wrapped + } else { + err = errors.Join(err, wrapped) + } + } + }() + + graph, err := core.LoadTaskGraph(s) + if err != nil { + return result, fmt.Errorf("load authoritative task graph: %w", err) + } + if err := core.ValidateTaskGraphMutationSource(graph); err != nil { + return result, err + } + + plan, err := callTaskGraphPlanner(s, planner, graph) + if err != nil { + return result, err + } + result.Plan, err = core.ValidateTaskGraphMutationPlan(graph, plan) + if err != nil { + return result, err + } + writes, err := s.materializeTaskGraphPlan(graph, result.Plan, now) + if err != nil { + return result, err + } + if dryRun { + return result, nil + } + + // Verify the whole materialized prefix before changing the first file. The + // repository lock excludes cooperating writers; the hashes still catch a raw + // editor that raced the advisory guard before application begins. + if testHookBeforeGraphVerify != nil { + testHookBeforeGraphVerify() + } + currentGraph, err := core.LoadTaskGraph(s) + if err != nil { + return result, fmt.Errorf("re-read authoritative task graph before write: %w", err) + } + if !graph.SameSourceSnapshot(currentGraph) { + return result, fmt.Errorf("repository task graph changed while planning; retry: %w", domain.ErrConflict) + } + for _, write := range writes { + // Keep the raw-editor CAS window bounded to one atomic replacement. The + // whole-snapshot check above is still the all-or-nothing preflight; this + // second per-file check protects later writes after a durable prefix. + if testHookBeforeGraphWrite != nil { + testHookBeforeGraphWrite(write.taskID) + } + if err := verifyUnchanged(s.resolvePath, write.taskID, write.path, write.ifVersion, "task", "dependency update"); err != nil { + return result, err + } + if err := writeFileAtomic(write.path, write.content, 0o644); err != nil { + return result, fmt.Errorf("write dependency update for task %s: %w", write.taskID, err) + } + result.AppliedTaskIDs = append(result.AppliedTaskIDs, write.taskID) + if testHookAfterGraphWrite != nil { + if err := testHookAfterGraphWrite(write.taskID); err != nil { + return result, fmt.Errorf("after dependency update for task %s: %w", write.taskID, err) + } + } + } + return result, nil +} + +func callTaskGraphPlanner(store *FS, planner core.TaskGraphPlanner, graph *core.TaskGraph) (core.TaskGraphMutationPlan, error) { + leave, err := store.enterGraphPlanner() + if err != nil { + return core.TaskGraphMutationPlan{}, err + } + defer leave() // also clears the re-entry sentinel when planner panics + return planner(graph) +} + +type materializedTaskGraphWrite struct { + taskID string + path string + ifVersion string + content []byte +} + +func (s *FS) materializeTaskGraphPlan(graph *core.TaskGraph, plan core.TaskGraphMutationPlan, now time.Time) ([]materializedTaskGraphWrite, error) { + writes := make([]materializedTaskGraphWrite, 0, len(plan.TaskWrites)) + for _, planned := range plan.TaskWrites { + task, _ := graph.Task(planned.TaskID) + path, err := s.resolvePath(planned.TaskID) + if err != nil { + return nil, err + } + if path != task.Path { + return nil, fmt.Errorf("task %s changed path during graph snapshot: %w", planned.TaskID, domain.ErrConflict) + } + content, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("read task %s for dependency update: %w", path, err) + } + updates := map[string]any{} + if len(planned.DependsOn) == 0 { + updates["depends_on"] = domain.UnsetField{} + } else { + updates["depends_on"] = planned.DependsOn + } + if planned.ClearLegacy { + updates["blocked_by"] = domain.UnsetField{} + updates["dependencies"] = domain.UnsetField{} + updates["blocks"] = domain.UnsetField{} + } + newContent, err := updateFrontmatter(content, updates) + if err != nil { + return nil, err + } + if bytes.Equal(content, newContent) { + continue + } + updates["updated_at"] = now.Format("2006-01-02") + newContent, err = updateFrontmatter(content, updates) + if err != nil { + return nil, err + } + parsed, err := parseTask(newContent, path) + if err != nil { + return nil, fmt.Errorf("%w: dependency update for task %s would not reload: %v", domain.ErrValidation, planned.TaskID, err) + } + if !slices.Equal(parsed.DependsOn, planned.DependsOn) { + return nil, fmt.Errorf("%w: dependency update for task %s did not materialize the planned canonical set", domain.ErrValidation, planned.TaskID) + } + if planned.ClearLegacy && (len(parsed.LegacyBlockedBy) > 0 || len(parsed.LegacyDependencies) > 0 || len(parsed.LegacyBlocks) > 0) { + return nil, fmt.Errorf("%w: dependency update for task %s did not clear legacy fields", domain.ErrValidation, planned.TaskID) + } + writes = append(writes, materializedTaskGraphWrite{ + taskID: planned.TaskID, path: path, ifVersion: hashContent(content), content: newContent, + }) + } + return writes, nil +} + +// testHookAfterGraphWrite injects a failure after one durable file replacement, +// pinning the prefix-result recovery contract. Nil outside tests. +var testHookAfterGraphWrite func(taskID string) error + +// testHookBeforeGraphVerify is the graph-mutation counterpart to the ordinary +// OCC seams: it interleaves a non-cooperating raw edit before the all-file CAS. +var testHookBeforeGraphVerify func() + +// testHookBeforeGraphWrite interleaves a raw edit after the whole-snapshot +// preflight but immediately before one target's per-file CAS. Nil outside tests. +var testHookBeforeGraphWrite func(taskID string) diff --git a/internal/store/graphmutation_test.go b/internal/store/graphmutation_test.go new file mode 100644 index 00000000..509bc861 --- /dev/null +++ b/internal/store/graphmutation_test.go @@ -0,0 +1,523 @@ +package store + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "slices" + "strings" + "sync" + "testing" + "time" + + "github.com/andy-esch/taskflow/internal/core" + "github.com/andy-esch/taskflow/internal/domain" + "github.com/andy-esch/taskflow/internal/testutil" +) + +var graphMutationNow = time.Date(2026, time.August, 27, 12, 0, 0, 0, time.UTC) + +func writeGraphMutationTask(t *testing.T, root, seed string, status domain.Status, dependencies []string, extra string) string { + t.Helper() + taskID := testutil.TaskID(seed) + dir := filepath.Join(root, domain.TasksDir) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + dependencyLine := "" + if len(dependencies) > 0 { + dependencyLine = "depends_on: [" + strings.Join(dependencies, ", ") + "]\n" + } + content := fmt.Sprintf("---\nschema: 1\nid: %s\nstatus: %s\nepic: 30\ndescription: %s\neffort: 1h\ntier: 1\npriority: high\nautonomy_level: 2\ntags: [graph]\ncreated: \"2026-08-27\"\n%s%s---\n# %s\n\nBody stays intact.\n", + taskID, status, seed, dependencyLine, extra, seed) + path := filepath.Join(dir, taskID+"-"+seed+".md") + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + return path +} + +func addDependencyPlan(dependent, prerequisite string) core.TaskGraphPlanner { + return func(graph *core.TaskGraph) (core.TaskGraphMutationPlan, error) { + task, ok := graph.Task(dependent) + if !ok { + return core.TaskGraphMutationPlan{}, fmt.Errorf("missing dependent %s", dependent) + } + dependencies := append([]string(nil), task.DependsOn...) + if !slices.Contains(dependencies, prerequisite) { + dependencies = append(dependencies, prerequisite) + } + return core.TaskGraphMutationPlan{TaskWrites: []core.TaskDependencyWrite{{ + TaskID: dependent, DependsOn: dependencies, + }}}, nil + } +} + +func TestMutateTaskGraphOwnsSemanticReadValidateWriteBoundary(t *testing.T) { + root := t.TempDir() + aID, bID, cID := testutil.TaskID("alpha"), testutil.TaskID("beta"), testutil.TaskID("charlie") + writeGraphMutationTask(t, root, "alpha", domain.StatusCompleted, nil, "") + bPath := writeGraphMutationTask(t, root, "beta", domain.StatusReadyToStart, nil, "custom_key: keep-me # preserve this comment\n") + writeGraphMutationTask(t, root, "charlie", domain.StatusCompleted, nil, "") + fs := NewFS(root) + + result, err := fs.MutateTaskGraph(graphMutationNow, false, func(graph *core.TaskGraph) (core.TaskGraphMutationPlan, error) { + if graph.Health() != core.GraphHealthy || len(graph.TaskIDs()) != 3 { + t.Fatalf("planner snapshot health=%s tasks=%v", graph.Health(), graph.TaskIDs()) + } + if task, _ := graph.Task(bID); task.SourceVersion != "" { + t.Fatal("planner-facing task exposed its persistence version") + } + return core.TaskGraphMutationPlan{TaskWrites: []core.TaskDependencyWrite{{ + TaskID: bID, DependsOn: []string{cID, aID}, + }}}, nil + }) + if err != nil { + t.Fatal(err) + } + if !slices.Equal(result.AppliedTaskIDs, []string{bID}) || result.DryRun { + t.Fatalf("result = %+v", result) + } + if got := result.Plan.TaskWrites[0].DependsOn; !slices.IsSorted(got) { + t.Fatalf("normalized dependency set is not sorted: %v", got) + } + b, err := os.ReadFile(bPath) + if err != nil { + t.Fatal(err) + } + for _, preserved := range []string{"custom_key: keep-me # preserve this comment", "Body stays intact.", "updated_at: \"2026-08-27\""} { + if !strings.Contains(string(b), preserved) { + t.Errorf("materialized task lost %q:\n%s", preserved, b) + } + } + graph, err := core.LoadTaskGraph(fs) + if err != nil { + t.Fatal(err) + } + updated, _ := graph.Task(bID) + if graph.Health() != core.GraphHealthy || !slices.Equal(updated.DependsOn, result.Plan.TaskWrites[0].DependsOn) { + t.Fatalf("reloaded graph health=%s task=%+v", graph.Health(), updated) + } +} + +func TestMutateTaskGraphDryRunValidatesWithoutWriting(t *testing.T) { + root := t.TempDir() + aID, bID := testutil.TaskID("alpha"), testutil.TaskID("beta") + writeGraphMutationTask(t, root, "alpha", domain.StatusCompleted, nil, "") + bPath := writeGraphMutationTask(t, root, "beta", domain.StatusReadyToStart, nil, "") + before, _ := os.ReadFile(bPath) + + result, err := NewFS(root).MutateTaskGraph(graphMutationNow, true, addDependencyPlan(bID, aID)) + if err != nil { + t.Fatal(err) + } + after, _ := os.ReadFile(bPath) + if !result.DryRun || len(result.AppliedTaskIDs) != 0 || len(result.Plan.TaskWrites) != 1 { + t.Fatalf("dry-run result = %+v", result) + } + if !slices.Equal(before, after) { + t.Fatal("dry-run changed the task file") + } +} + +func TestMutateTaskGraphConcurrentOppositeEdgesCannotCommitCycle(t *testing.T) { + root := t.TempDir() + aID, bID := testutil.TaskID("alpha"), testutil.TaskID("beta") + writeGraphMutationTask(t, root, "alpha", domain.StatusReadyToStart, nil, "") + writeGraphMutationTask(t, root, "beta", domain.StatusReadyToStart, nil, "") + + planners := []core.TaskGraphPlanner{addDependencyPlan(aID, bID), addDependencyPlan(bID, aID)} + start := make(chan struct{}) + errs := make(chan error, len(planners)) + var ready sync.WaitGroup + ready.Add(len(planners)) + for _, planner := range planners { + planner := planner + go func() { + ready.Done() + <-start + _, err := NewFS(root).MutateTaskGraph(graphMutationNow, false, planner) + errs <- err + }() + } + ready.Wait() + close(start) + + succeeded, rejected := 0, 0 + for range planners { + if err := <-errs; err == nil { + succeeded++ + } else if errors.Is(err, domain.ErrValidation) && strings.Contains(err.Error(), "dependency cycle:") { + rejected++ + } else { + t.Fatalf("unexpected concurrent mutation result: %v", err) + } + } + if succeeded != 1 || rejected != 1 { + t.Fatalf("succeeded=%d rejected=%d", succeeded, rejected) + } + graph, err := core.LoadTaskGraph(NewFS(root)) + if err != nil { + t.Fatal(err) + } + if graph.Health() != core.GraphHealthy { + t.Fatalf("final graph health = %s, problems=%+v", graph.Health(), graph.Problems()) + } + committed := 0 + for _, taskID := range []string{aID, bID} { + task, _ := graph.Task(taskID) + committed += len(task.DependsOn) + } + if committed != 1 { + t.Fatalf("committed edge count = %d", committed) + } +} + +func TestMutateTaskGraphAllowsOneGuardedLegacyMigrationToHealthy(t *testing.T) { + root := t.TempDir() + aID, bID := testutil.TaskID("alpha"), testutil.TaskID("beta") + writeGraphMutationTask(t, root, "alpha", domain.StatusCompleted, nil, "") + writeGraphMutationTask(t, root, "beta", domain.StatusReadyToStart, nil, "blocked_by: [alpha]\n") + fs := NewFS(root) + + _, err := fs.MutateTaskGraph(graphMutationNow, false, func(graph *core.TaskGraph) (core.TaskGraphMutationPlan, error) { + if graph.Health() != core.GraphDegraded { + t.Fatalf("migration planner health = %s", graph.Health()) + } + return core.TaskGraphMutationPlan{TaskWrites: []core.TaskDependencyWrite{{ + TaskID: bID, DependsOn: []string{aID}, ClearLegacy: true, + }}}, nil + }) + if err != nil { + t.Fatal(err) + } + graph, err := core.LoadTaskGraph(fs) + if err != nil { + t.Fatal(err) + } + if graph.Health() != core.GraphHealthy || len(graph.LegacyDiagnostics()) != 0 { + t.Fatalf("migrated health=%s legacy=%+v", graph.Health(), graph.LegacyDiagnostics()) + } +} + +func TestMutateTaskGraphBrokenSnapshotFailsBeforePlanner(t *testing.T) { + root := t.TempDir() + aID, bID := testutil.TaskID("alpha"), testutil.TaskID("beta") + writeGraphMutationTask(t, root, "alpha", domain.StatusReadyToStart, []string{bID}, "") + writeGraphMutationTask(t, root, "beta", domain.StatusReadyToStart, []string{aID}, "") + called := false + _, err := NewFS(root).MutateTaskGraph(graphMutationNow, false, func(*core.TaskGraph) (core.TaskGraphMutationPlan, error) { + called = true + return core.TaskGraphMutationPlan{}, nil + }) + if !errors.Is(err, domain.ErrValidation) || !strings.Contains(err.Error(), "repository task graph is broken") || !strings.Contains(err.Error(), "dependency cycle:") { + t.Fatalf("broken mutation error = %v", err) + } + if called { + t.Fatal("planner ran against a broken authoritative snapshot") + } +} + +func TestMutateTaskGraphRejectsPlannerStoreCallsAndNestedMutationWithoutHanging(t *testing.T) { + root := t.TempDir() + aID := testutil.TaskID("alpha") + writeGraphMutationTask(t, root, "alpha", domain.StatusReadyToStart, nil, "") + fs := NewFS(root) + done := make(chan error, 1) + go func() { + _, err := fs.MutateTaskGraph(graphMutationNow, false, func(*core.TaskGraph) (core.TaskGraphMutationPlan, error) { + if _, _, err := fs.GetTask(aID); !errors.Is(err, domain.ErrConflict) { + return core.TaskGraphMutationPlan{}, fmt.Errorf("nested read error = %v", err) + } + if _, err := fs.SetFields(aID, map[string]any{"priority": "low"}, false); !errors.Is(err, domain.ErrConflict) { + return core.TaskGraphMutationPlan{}, fmt.Errorf("nested write error = %v", err) + } + if _, err := fs.MutateTaskGraph(graphMutationNow, false, func(*core.TaskGraph) (core.TaskGraphMutationPlan, error) { + return core.TaskGraphMutationPlan{}, nil + }); !errors.Is(err, domain.ErrConflict) { + return core.TaskGraphMutationPlan{}, fmt.Errorf("nested graph mutation error = %v", err) + } + second := NewFS(root) + if _, _, err := second.GetTask(aID); !errors.Is(err, domain.ErrConflict) { + return core.TaskGraphMutationPlan{}, fmt.Errorf("second-store nested read error = %v", err) + } + if _, err := second.SetFields(aID, map[string]any{"priority": "low"}, false); !errors.Is(err, domain.ErrConflict) { + return core.TaskGraphMutationPlan{}, fmt.Errorf("second-store nested write error = %v", err) + } + if _, err := second.DanglingLinks(); !errors.Is(err, domain.ErrConflict) { + return core.TaskGraphMutationPlan{}, fmt.Errorf("nested dangling-links error = %v", err) + } + if _, err := second.FixFrontmatter(true); !errors.Is(err, domain.ErrConflict) { + return core.TaskGraphMutationPlan{}, fmt.Errorf("nested frontmatter-fix error = %v", err) + } + return core.TaskGraphMutationPlan{}, nil + }) + done <- err + }() + select { + case err := <-done: + if err != nil { + t.Fatal(err) + } + case <-time.After(2 * time.Second): + t.Fatal("nested Store call self-deadlocked") + } + if _, _, err := fs.GetTask(aID); err != nil { + t.Fatalf("planner sentinel leaked after return: %v", err) + } +} + +func TestMutateTaskGraphRejectsConcurrentStoreAccessAtRepositoryScope(t *testing.T) { + root := t.TempDir() + aID := testutil.TaskID("alpha") + writeGraphMutationTask(t, root, "alpha", domain.StatusReadyToStart, nil, "") + fs := NewFS(root) + started := make(chan struct{}) + release := make(chan struct{}) + done := make(chan error, 1) + go func() { + _, err := fs.MutateTaskGraph(graphMutationNow, false, func(*core.TaskGraph) (core.TaskGraphMutationPlan, error) { + close(started) + <-release + return core.TaskGraphMutationPlan{}, nil + }) + done <- err + }() + <-started + _, _, err := NewFS(root).GetTask(aID) + if !errors.Is(err, domain.ErrConflict) || !strings.Contains(err.Error(), "concurrent caller") { + t.Fatalf("concurrent Store access error = %v", err) + } + close(release) + if err := <-done; err != nil { + t.Fatal(err) + } +} + +func TestMutateTaskGraphPlannerPanicReleasesGuard(t *testing.T) { + root := t.TempDir() + aID := testutil.TaskID("alpha") + writeGraphMutationTask(t, root, "alpha", domain.StatusReadyToStart, nil, "") + fs := NewFS(root) + func() { + defer func() { + if recover() == nil { + t.Error("planner panic did not propagate") + } + }() + _, _ = fs.MutateTaskGraph(graphMutationNow, false, func(*core.TaskGraph) (core.TaskGraphMutationPlan, error) { + panic("planner failed") + }) + }() + if _, err := fs.SetFields(aID, map[string]any{"priority": "low"}, false); err != nil { + t.Fatalf("repository guard remained held after panic: %v", err) + } +} + +func TestMutateTaskGraphAttributesReleaseFailureAfterUnlocking(t *testing.T) { + root := t.TempDir() + writeGraphMutationTask(t, root, "alpha", domain.StatusReadyToStart, nil, "") + fs := NewFS(root) + original := testHookRepositoryUnlockError + defer func() { testHookRepositoryUnlockError = original }() + testHookRepositoryUnlockError = func() error { return errors.New("injected unlock failure") } + _, err := fs.MutateTaskGraph(graphMutationNow, false, func(*core.TaskGraph) (core.TaskGraphMutationPlan, error) { + return core.TaskGraphMutationPlan{}, nil + }) + if err == nil || !strings.Contains(err.Error(), "release repository graph mutation guard") || !strings.Contains(err.Error(), "injected unlock failure") { + t.Fatalf("release error = %v", err) + } + testHookRepositoryUnlockError = nil + if _, err := fs.MutateTaskGraph(graphMutationNow, false, func(*core.TaskGraph) (core.TaskGraphMutationPlan, error) { + return core.TaskGraphMutationPlan{}, nil + }); err != nil { + t.Fatalf("guard stayed held after attributed release failure: %v", err) + } +} + +func TestMutateTaskGraphCASRejectsRawEditBeforeApply(t *testing.T) { + root := t.TempDir() + aID, bID := testutil.TaskID("alpha"), testutil.TaskID("beta") + aPath := writeGraphMutationTask(t, root, "alpha", domain.StatusCompleted, nil, "") + bPath := writeGraphMutationTask(t, root, "beta", domain.StatusReadyToStart, nil, "") + fs := NewFS(root) + original := testHookBeforeGraphVerify + defer func() { testHookBeforeGraphVerify = original }() + testHookBeforeGraphVerify = func() { + // Edit a task the plan will NOT write. Per-file CAS on beta cannot see + // this; the whole-snapshot CAS must still reject the stale plan. + content, _ := os.ReadFile(aPath) + _ = os.WriteFile(aPath, append(content, []byte("\nRaw edit survives.\n")...), 0o644) + testHookBeforeGraphVerify = nil + } + _, err := fs.MutateTaskGraph(graphMutationNow, false, addDependencyPlan(bID, aID)) + if !errors.Is(err, domain.ErrConflict) { + t.Fatalf("raw edit error = %v", err) + } + a, _ := os.ReadFile(aPath) + b, _ := os.ReadFile(bPath) + if !strings.Contains(string(a), "Raw edit survives.") || strings.Contains(string(b), "depends_on:") { + t.Fatalf("stale graph mutation wrote after an unrelated raw edit:\nalpha:\n%s\nbeta:\n%s", a, b) + } +} + +func TestMutateTaskGraphReturnsDurablePrefixAndRerunConverges(t *testing.T) { + root := t.TempDir() + aID, bID, cID := testutil.TaskID("alpha"), testutil.TaskID("beta"), testutil.TaskID("charlie") + writeGraphMutationTask(t, root, "alpha", domain.StatusCompleted, nil, "") + writeGraphMutationTask(t, root, "beta", domain.StatusReadyToStart, nil, "") + writeGraphMutationTask(t, root, "charlie", domain.StatusReadyToStart, nil, "") + fs := NewFS(root) + planner := func(*core.TaskGraph) (core.TaskGraphMutationPlan, error) { + return core.TaskGraphMutationPlan{TaskWrites: []core.TaskDependencyWrite{ + {TaskID: bID, DependsOn: []string{aID}}, + {TaskID: cID, DependsOn: []string{aID}}, + }}, nil + } + original := testHookAfterGraphWrite + defer func() { testHookAfterGraphWrite = original }() + testHookAfterGraphWrite = func(string) error { + testHookAfterGraphWrite = nil + return errors.New("injected interruption") + } + partial, err := fs.MutateTaskGraph(graphMutationNow, false, planner) + if err == nil || len(partial.AppliedTaskIDs) != 1 { + t.Fatalf("partial result=%+v err=%v", partial, err) + } + graph, loadErr := core.LoadTaskGraph(fs) + if loadErr != nil || graph.Health() != core.GraphHealthy { + t.Fatalf("prefix left invalid graph: health=%s err=%v", graph.Health(), loadErr) + } + completed, err := fs.MutateTaskGraph(graphMutationNow, false, planner) + if err != nil { + t.Fatal(err) + } + if len(completed.AppliedTaskIDs) != 1 { + t.Fatalf("rerun should skip durable prefix and apply one remainder: %+v", completed) + } + graph, _ = core.LoadTaskGraph(fs) + for _, taskID := range []string{bID, cID} { + task, _ := graph.Task(taskID) + if !slices.Equal(task.DependsOn, []string{aID}) { + t.Fatalf("task %s dependencies = %v", taskID, task.DependsOn) + } + } +} + +func TestMutateTaskGraphPerFileCASPreservesRawEditAfterDurablePrefix(t *testing.T) { + root := t.TempDir() + aID, bID, cID := testutil.TaskID("alpha"), testutil.TaskID("beta"), testutil.TaskID("charlie") + writeGraphMutationTask(t, root, "alpha", domain.StatusCompleted, nil, "") + writeGraphMutationTask(t, root, "beta", domain.StatusReadyToStart, nil, "") + cPath := writeGraphMutationTask(t, root, "charlie", domain.StatusReadyToStart, nil, "") + fs := NewFS(root) + original := testHookBeforeGraphWrite + defer func() { testHookBeforeGraphWrite = original }() + testHookBeforeGraphWrite = func(taskID string) { + if taskID != cID { + return + } + content, _ := os.ReadFile(cPath) + _ = os.WriteFile(cPath, append(content, []byte("\nRaw edit survives.\n")...), 0o644) + testHookBeforeGraphWrite = nil + } + result, err := fs.MutateTaskGraph(graphMutationNow, false, func(*core.TaskGraph) (core.TaskGraphMutationPlan, error) { + return core.TaskGraphMutationPlan{TaskWrites: []core.TaskDependencyWrite{ + {TaskID: bID, DependsOn: []string{aID}}, + {TaskID: cID, DependsOn: []string{aID}}, + }}, nil + }) + if !errors.Is(err, domain.ErrConflict) || !slices.Equal(result.AppliedTaskIDs, []string{bID}) { + t.Fatalf("partial CAS result=%+v err=%v", result, err) + } + content, _ := os.ReadFile(cPath) + if !strings.Contains(string(content), "Raw edit survives.") || strings.Contains(string(content), "depends_on:") { + t.Fatalf("later graph write clobbered raw edit:\n%s", content) + } +} + +func TestMutateTaskGraphRejectsBrokenIntermediatePrefixBeforeWriting(t *testing.T) { + root := t.TempDir() + aID, bID := testutil.TaskID("alpha"), testutil.TaskID("beta") + firstID, secondID := aID, bID + firstSeed, secondSeed := "alpha", "beta" + if secondID < firstID { + firstID, secondID = secondID, firstID + firstSeed, secondSeed = secondSeed, firstSeed + } + firstPath := writeGraphMutationTask(t, root, firstSeed, domain.StatusReadyToStart, nil, "") + secondPath := writeGraphMutationTask(t, root, secondSeed, domain.StatusReadyToStart, []string{firstID}, "") + firstBefore, _ := os.ReadFile(firstPath) + secondBefore, _ := os.ReadFile(secondPath) + + _, err := NewFS(root).MutateTaskGraph(graphMutationNow, false, func(*core.TaskGraph) (core.TaskGraphMutationPlan, error) { + // Final state reverses the edge and is acyclic. Task-ID ordering would add + // the reverse edge before removing the old one, however, so a crash after + // the first replacement would leave a cycle. + return core.TaskGraphMutationPlan{TaskWrites: []core.TaskDependencyWrite{ + {TaskID: firstID, DependsOn: []string{secondID}}, + {TaskID: secondID, DependsOn: nil}, + }}, nil + }) + if !errors.Is(err, domain.ErrValidation) || !strings.Contains(err.Error(), "write prefix") || !strings.Contains(err.Error(), "dependency cycle:") { + t.Fatalf("unsafe prefix error = %v", err) + } + firstAfter, _ := os.ReadFile(firstPath) + secondAfter, _ := os.ReadFile(secondPath) + if !slices.Equal(firstBefore, firstAfter) || !slices.Equal(secondBefore, secondAfter) { + t.Fatal("unsafe multi-file plan wrote before prefix validation completed") + } +} + +func TestMutateTaskGraphPreservesPrefixSafePlannerOrder(t *testing.T) { + root := t.TempDir() + aID, bID := testutil.TaskID("alpha"), testutil.TaskID("beta") + aPath := writeGraphMutationTask(t, root, "alpha", domain.StatusReadyToStart, nil, "") + bPath := writeGraphMutationTask(t, root, "beta", domain.StatusReadyToStart, []string{aID}, "") + + result, err := NewFS(root).MutateTaskGraph(graphMutationNow, false, func(*core.TaskGraph) (core.TaskGraphMutationPlan, error) { + // Remove the old edge before adding its reverse. Stable-ID sorting is unsafe + // here; the planner-provided sequence is part of the recovery contract. + return core.TaskGraphMutationPlan{TaskWrites: []core.TaskDependencyWrite{ + {TaskID: bID, DependsOn: nil}, + {TaskID: aID, DependsOn: []string{bID}}, + }}, nil + }) + if err != nil { + t.Fatal(err) + } + if !slices.Equal(result.AppliedTaskIDs, []string{bID, aID}) { + t.Fatalf("applied order = %v", result.AppliedTaskIDs) + } + a, _ := os.ReadFile(aPath) + b, _ := os.ReadFile(bPath) + if !strings.Contains(string(a), "depends_on: ["+bID+"]") || strings.Contains(string(b), "depends_on:") { + t.Fatalf("edge reversal did not materialize safely:\nalpha:\n%s\nbeta:\n%s", a, b) + } +} + +func TestMutateTaskGraphStampsUpdatedAtOnlyForSemanticChanges(t *testing.T) { + root := t.TempDir() + aID, bID := testutil.TaskID("alpha"), testutil.TaskID("beta") + writeGraphMutationTask(t, root, "alpha", domain.StatusCompleted, nil, "") + bPath := writeGraphMutationTask(t, root, "beta", domain.StatusReadyToStart, nil, "updated_at: \"2026-01-01\"\n") + fs := NewFS(root) + + first, err := fs.MutateTaskGraph(graphMutationNow, false, addDependencyPlan(bID, aID)) + if err != nil || !slices.Equal(first.AppliedTaskIDs, []string{bID}) { + t.Fatalf("first mutation=%+v err=%v", first, err) + } + content, _ := os.ReadFile(bPath) + if !strings.Contains(string(content), "updated_at: \"2026-08-27\"") { + t.Fatalf("dependency mutation did not stamp updated_at:\n%s", content) + } + second, err := fs.MutateTaskGraph(graphMutationNow.AddDate(0, 0, 1), false, addDependencyPlan(bID, aID)) + if err != nil || len(second.AppliedTaskIDs) != 0 { + t.Fatalf("idempotent rerun=%+v err=%v", second, err) + } + content, _ = os.ReadFile(bPath) + if strings.Contains(string(content), "2026-08-28") { + t.Fatalf("no-op dependency mutation changed updated_at:\n%s", content) + } +} diff --git a/internal/store/lock.go b/internal/store/lock.go new file mode 100644 index 00000000..05d6fae4 --- /dev/null +++ b/internal/store/lock.go @@ -0,0 +1,136 @@ +package store + +import ( + "errors" + "fmt" + "path/filepath" + "runtime" + "strings" + "sync" + + "github.com/andy-esch/taskflow/internal/domain" +) + +// repositoryGuard supplies the same-process half of the repository guard and +// records the callback-exclusive phase at the same canonical-root scope. +type repositoryGuard struct { + write sync.Mutex + plannerMu sync.RWMutex + plannerActive bool +} + +// repositoryGuards supplies the same-process half of the repository guard. +// Platform file-lock semantics vary for independently opened handles owned by one +// process, and a future long-lived adapter may create more than one *FS. The OS +// lock remains authoritative across processes; this keyed mutex makes the combined +// contract identical within one. CLI processes normally retain one entry; a future +// long-lived multi-space adapter should reference-count and evict idle entries. +var repositoryGuards = struct { + sync.Mutex + byRoot map[string]*repositoryGuard +}{byRoot: make(map[string]*repositoryGuard)} + +func repositoryLockKey(root string) string { + return normalizeRepositoryLockKey(root, runtime.GOOS == "windows") +} + +func normalizeRepositoryLockKey(root string, caseInsensitive bool) string { + if resolved, err := filepath.EvalSymlinks(root); err == nil { + root = resolved + } + if absolute, err := filepath.Abs(root); err == nil { + root = absolute + } + root = filepath.Clean(root) + if caseInsensitive { + root = strings.ToLower(root) + } + return root +} + +func repositoryGuardFor(root string) *repositoryGuard { + key := repositoryLockKey(root) + repositoryGuards.Lock() + guard := repositoryGuards.byRoot[key] + if guard == nil { + guard = new(repositoryGuard) + repositoryGuards.byRoot[key] = guard + } + repositoryGuards.Unlock() + return guard +} + +func processRepositoryLock(root string) func() { + guard := repositoryGuardFor(root) + guard.write.Lock() + return guard.write.Unlock +} + +func (s *FS) enterGraphPlanner() (func(), error) { + guard := repositoryGuardFor(s.root) + guard.plannerMu.Lock() + if guard.plannerActive { + guard.plannerMu.Unlock() + return nil, graphPlannerReentryError() + } + guard.plannerActive = true + guard.plannerMu.Unlock() + return func() { + guard.plannerMu.Lock() + guard.plannerActive = false + guard.plannerMu.Unlock() + }, nil +} + +func graphPlannerReentryError() error { + return fmt.Errorf("repository graph planner is active; Store access from its callback or a concurrent caller is unavailable: %w", domain.ErrConflict) +} + +func (s *FS) rejectGraphPlannerCall() error { + guard := repositoryGuardFor(s.root) + guard.plannerMu.RLock() + active := guard.plannerActive + guard.plannerMu.RUnlock() + if active { + return graphPlannerReentryError() + } + return nil +} + +// writeLock is the ordinary Store mutation entry point. The platform lock owns +// cooperating-writer serialization. The process-local planner check turns an +// invalid callback re-entry into an attributable conflict instead of a self-deadlock. +func (s *FS) writeLock() (func(), error) { + release, err := s.checkedWriteLock() + if err != nil { + return nil, err + } + return func() { _ = release() }, nil +} + +// checkedWriteLock is the graph-mutation form of writeLock: release failures are +// returned so the control-inverted boundary can attribute them to the operation. +// Ordinary legacy call sites retain their no-error unlock function until they are +// migrated; both paths use the same process + platform serialization. +func (s *FS) checkedWriteLock() (func() error, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return nil, err + } + unlockProcess := processRepositoryLock(s.root) + unlockPlatform, err := s.platformWriteLockChecked() + if err != nil { + unlockProcess() + return nil, err + } + return func() error { + platformErr := unlockPlatform() + unlockProcess() + var hookErr error + if testHookRepositoryUnlockError != nil { + hookErr = testHookRepositoryUnlockError() + } + return errors.Join(platformErr, hookErr) + }, nil +} + +var testHookRepositoryUnlockError func() error diff --git a/internal/store/lock_other.go b/internal/store/lock_other.go index 9e46ee0e..605adbac 100644 --- a/internal/store/lock_other.go +++ b/internal/store/lock_other.go @@ -2,13 +2,15 @@ package store -// writeLock is a no-op on non-unix platforms — syscall.Flock isn't available there, so the -// cooperating-writer serialization the version-CAS relies on is NOT provided. This is a -// KNOWN GAP for Windows (tracked as a follow-up): concurrent writers can still drop -// updates as they did before the flock fix. The version-CAS itself still runs and catches -// a non-cooperating (out-of-band) edit; only the same-tool concurrency guarantee is -// missing. The primary deployment (Linux container + macOS host) is unix, where the real -// lock applies. -func (s *FS) writeLock() (func(), error) { - return func() {}, nil +import ( + "fmt" + + "github.com/andy-esch/taskflow/internal/domain" +) + +// platformWriteLock fails explicitly on targets for which taskflow has no tested +// cooperating-writer primitive. Silent no-op locking would make a successful CAS +// claim untrue and is therefore less safe than rejecting mutation. +func (s *FS) platformWriteLockChecked() (func() error, error) { + return nil, fmt.Errorf("%w: repository mutation locking is unsupported on this platform", domain.ErrValidation) } diff --git a/internal/store/lock_unix.go b/internal/store/lock_unix.go index 1f9db1d3..7127fa7f 100644 --- a/internal/store/lock_unix.go +++ b/internal/store/lock_unix.go @@ -8,7 +8,7 @@ import ( "syscall" ) -// writeLock takes the process-wide advisory write lock — flock(LOCK_EX) on the repo root +// platformWriteLock takes the process-wide advisory write lock — flock(LOCK_EX) on the repo root // directory — so the version-CAS's verify→write becomes an ATOMIC compare-and-swap: no // other cooperating writer can land a rename between a verify and its own rename (the // lost-update window that the check-then-write, non-atomic on a filesystem, otherwise @@ -18,7 +18,7 @@ import ( // brief and infrequent, so serializing them is imperceptible; per-file locking is a future // refinement. flock auto-releases if the process dies (no stale lock files). Returns an // unlock func the caller defers after the write. -func (s *FS) writeLock() (func(), error) { +func (s *FS) platformWriteLockChecked() (func() error, error) { f, err := os.Open(s.root) if err != nil { return nil, fmt.Errorf("open repo root for write lock: %w", err) @@ -27,8 +27,14 @@ func (s *FS) writeLock() (func(), error) { _ = f.Close() return nil, fmt.Errorf("acquire write lock: %w", err) } - return func() { - _ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN) - _ = f.Close() + return func() error { + var releaseErr error + if err := syscall.Flock(int(f.Fd()), syscall.LOCK_UN); err != nil { + releaseErr = fmt.Errorf("release repository lock: %w", err) + } + if err := f.Close(); err != nil && releaseErr == nil { + releaseErr = fmt.Errorf("close repository lock handle: %w", err) + } + return releaseErr }, nil } diff --git a/internal/store/lock_unix_test.go b/internal/store/lock_unix_test.go new file mode 100644 index 00000000..4c3bcd0f --- /dev/null +++ b/internal/store/lock_unix_test.go @@ -0,0 +1,263 @@ +//go:build unix + +package store + +import ( + "bufio" + "bytes" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "syscall" + "testing" + "time" + + "github.com/andy-esch/taskflow/internal/core" + "github.com/andy-esch/taskflow/internal/domain" + "github.com/andy-esch/taskflow/internal/testutil" +) + +const ( + lockHelperMode = "TSKFLWCTL_TEST_LOCK_HELPER" + lockHelperRoot = "TSKFLWCTL_TEST_LOCK_ROOT" + graphMutationHelperMode = "TSKFLWCTL_TEST_GRAPH_MUTATION_HELPER" + graphMutationHelperTask = "TSKFLWCTL_TEST_GRAPH_MUTATION_TASK" + graphMutationHelperRequires = "TSKFLWCTL_TEST_GRAPH_MUTATION_REQUIRES" + graphMutationHelperReady = "TSKFLWCTL_TEST_GRAPH_MUTATION_READY" + graphMutationHelperStart = "TSKFLWCTL_TEST_GRAPH_MUTATION_START" +) + +// TestRepositoryLockHelperProcess is re-executed by +// TestRepositoryLockReleasesWhenProcessTerminates. It deliberately holds the +// repository lock until the parent kills it; ordinary test runs return at once. +func TestRepositoryLockHelperProcess(t *testing.T) { + if os.Getenv(lockHelperMode) != "1" { + return + } + unlock, err := NewFS(os.Getenv(lockHelperRoot)).writeLock() + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(2) + } + defer unlock() + fmt.Println("locked") + <-time.After(time.Hour) +} + +func TestRepositoryLockReleasesWhenProcessTerminates(t *testing.T) { + root := t.TempDir() + cmd := exec.Command(os.Args[0], "-test.run=^TestRepositoryLockHelperProcess$") + cmd.Env = append(os.Environ(), lockHelperMode+"=1", lockHelperRoot+"="+root) + stdout, err := cmd.StdoutPipe() + if err != nil { + t.Fatal(err) + } + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + stopped := false + defer func() { + if !stopped { + _ = cmd.Process.Kill() + _ = cmd.Wait() + } + }() + if scanner := bufio.NewScanner(stdout); !scanner.Scan() || scanner.Text() != "locked" { + t.Fatalf("lock helper did not acquire the repository lock") + } + + probe, err := os.Open(root) + if err != nil { + t.Fatal(err) + } + if err := syscall.Flock(int(probe.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err == nil { + _ = syscall.Flock(int(probe.Fd()), syscall.LOCK_UN) + _ = probe.Close() + t.Fatal("product lock path did not hold the cross-process repository flock") + } else if err != syscall.EWOULDBLOCK && err != syscall.EAGAIN { + _ = probe.Close() + t.Fatalf("nonblocking lock probe = %v", err) + } + _ = probe.Close() + + if err := cmd.Process.Kill(); err != nil { + t.Fatal(err) + } + if err := cmd.Wait(); err == nil { + t.Fatal("killed lock helper exited successfully") + } + stopped = true + unlocked, err := NewFS(root).writeLock() + if err != nil { + t.Fatalf("repository lock failed after holder exit: %v", err) + } + unlocked() +} + +func TestRepositoryGraphMutationHelperProcess(t *testing.T) { + if os.Getenv(graphMutationHelperMode) != "1" { + return + } + if err := os.WriteFile(os.Getenv(graphMutationHelperReady), []byte("ready"), 0o644); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(2) + } + deadline := time.Now().Add(5 * time.Second) + for { + if _, err := os.Stat(os.Getenv(graphMutationHelperStart)); err == nil { + break + } + if time.Now().After(deadline) { + fmt.Fprintln(os.Stderr, "timed out waiting for graph mutation start") + os.Exit(2) + } + time.Sleep(time.Millisecond) + } + dependent := os.Getenv(graphMutationHelperTask) + prerequisite := os.Getenv(graphMutationHelperRequires) + _, err := NewFS(os.Getenv(lockHelperRoot)).MutateTaskGraph(graphMutationNow, false, func(graph *core.TaskGraph) (core.TaskGraphMutationPlan, error) { + task, ok := graph.Task(dependent) + if !ok { + return core.TaskGraphMutationPlan{}, fmt.Errorf("missing task %s", dependent) + } + time.Sleep(100 * time.Millisecond) // widens the cross-process contention window + return core.TaskGraphMutationPlan{TaskWrites: []core.TaskDependencyWrite{{ + TaskID: dependent, DependsOn: append(task.DependsOn, prerequisite), + }}}, nil + }) + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(3) + } + fmt.Println("applied") + os.Exit(0) +} + +func TestMutateTaskGraphSerializesOppositeEdgesAcrossProcesses(t *testing.T) { + root := t.TempDir() + aID, bID := testutil.TaskID("alpha"), testutil.TaskID("beta") + writeGraphMutationTask(t, root, "alpha", domain.StatusReadyToStart, nil, "") + writeGraphMutationTask(t, root, "beta", domain.StatusReadyToStart, nil, "") + coordination := t.TempDir() + start := filepath.Join(coordination, "start") + + type child struct { + cmd *exec.Cmd + stdout bytes.Buffer + stderr bytes.Buffer + ready string + } + newChild := func(dependent, prerequisite, readyName string) *child { + c := &child{ready: filepath.Join(coordination, readyName)} + c.cmd = exec.Command(os.Args[0], "-test.run=^TestRepositoryGraphMutationHelperProcess$") + c.cmd.Env = append(os.Environ(), + graphMutationHelperMode+"=1", + lockHelperRoot+"="+root, + graphMutationHelperTask+"="+dependent, + graphMutationHelperRequires+"="+prerequisite, + graphMutationHelperReady+"="+c.ready, + graphMutationHelperStart+"="+start, + ) + c.cmd.Stdout, c.cmd.Stderr = &c.stdout, &c.stderr + return c + } + children := []*child{newChild(aID, bID, "first-ready"), newChild(bID, aID, "second-ready")} + for _, c := range children { + if err := c.cmd.Start(); err != nil { + t.Fatal(err) + } + } + deadline := time.Now().Add(5 * time.Second) + for _, c := range children { + for { + if _, err := os.Stat(c.ready); err == nil { + break + } + if time.Now().After(deadline) { + t.Fatal("child did not become ready") + } + time.Sleep(time.Millisecond) + } + } + if err := os.WriteFile(start, []byte("start"), 0o644); err != nil { + t.Fatal(err) + } + succeeded, rejected := 0, 0 + for _, c := range children { + err := c.cmd.Wait() + switch { + case err == nil && strings.Contains(c.stdout.String(), "applied"): + succeeded++ + case err != nil && strings.Contains(c.stderr.String(), "dependency cycle:"): + rejected++ + default: + t.Fatalf("unexpected child result err=%v stdout=%q stderr=%q", err, c.stdout.String(), c.stderr.String()) + } + } + if succeeded != 1 || rejected != 1 { + t.Fatalf("cross-process outcomes: succeeded=%d rejected=%d", succeeded, rejected) + } + graph, err := core.LoadTaskGraph(NewFS(root)) + if err != nil || graph.Health() != core.GraphHealthy { + t.Fatalf("final graph health=%v err=%v", graph.Health(), err) + } + edges := 0 + for _, taskID := range []string{aID, bID} { + task, _ := graph.Task(taskID) + edges += len(task.DependsOn) + } + if edges != 1 { + t.Fatalf("final edge count = %d", edges) + } +} + +func TestGraphMutationLockAcquisitionErrorIsAttributable(t *testing.T) { + missing := filepath.Join(t.TempDir(), "missing-planning-root") + _, err := NewFS(missing).MutateTaskGraph(graphMutationNow, false, func(*core.TaskGraph) (core.TaskGraphMutationPlan, error) { + return core.TaskGraphMutationPlan{}, nil + }) + if err == nil || !strings.Contains(err.Error(), "open repo root for write lock") { + t.Fatalf("lock acquisition error = %v", err) + } +} + +func TestRepositoryLockSerializesIndependentStoresInOneProcess(t *testing.T) { + root := t.TempDir() + unlocked := processRepositoryLock(root) + guard := repositoryGuardFor(root) + if guard.write.TryLock() { + guard.write.Unlock() + unlocked() + t.Fatal("same-process repository guard did not hold its keyed mutex") + } + unlocked() + if !guard.write.TryLock() { + t.Fatal("same-process repository guard did not release its keyed mutex") + } + guard.write.Unlock() +} + +func TestRepositoryLockKeyCanonicalizesEquivalentRoots(t *testing.T) { + realRoot := t.TempDir() + parent := t.TempDir() + alias := filepath.Join(parent, "planning-alias") + if err := os.Symlink(realRoot, alias); err != nil { + t.Fatal(err) + } + want := repositoryLockKey(realRoot) + for _, candidate := range []string{ + realRoot + string(filepath.Separator), + filepath.Join(realRoot, "."), + filepath.Join(realRoot, "..", filepath.Base(realRoot)), + alias, + } { + if got := repositoryLockKey(candidate); got != want { + t.Errorf("repositoryLockKey(%q) = %q, want %q", candidate, got, want) + } + } + if got := normalizeRepositoryLockKey(realRoot, true); got != strings.ToLower(want) { + t.Errorf("case-insensitive lock key = %q, want %q", got, strings.ToLower(want)) + } +} diff --git a/internal/store/paths.go b/internal/store/paths.go index a597b00e..2442a17e 100644 --- a/internal/store/paths.go +++ b/internal/store/paths.go @@ -6,13 +6,33 @@ package store // repair it). GetTask/GetEpic/GetAudit would fail at the parse step first. // ResolveTaskPath returns a task's file path from its slug/id, parse-free. -func (s *FS) ResolveTaskPath(slug string) (string, error) { return s.resolve(slug) } +func (s *FS) ResolveTaskPath(slug string) (string, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return "", err + } + return s.resolve(slug) +} // ResolveEpicPath returns an epic's file path from its id, parse-free. -func (s *FS) ResolveEpicPath(id string) (string, error) { return s.resolveEpicPath(id) } +func (s *FS) ResolveEpicPath(id string) (string, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return "", err + } + return s.resolveEpicPath(id) +} // ResolveAuditPath returns an audit's file path from its slug/id, parse-free. -func (s *FS) ResolveAuditPath(slug string) (string, error) { return s.resolveAudit(slug) } +func (s *FS) ResolveAuditPath(slug string) (string, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return "", err + } + return s.resolveAudit(slug) +} // ResolveResearchPath returns a research doc's file path from its slug/id, parse-free. -func (s *FS) ResolveResearchPath(slug string) (string, error) { return s.resolveResearch(slug) } +func (s *FS) ResolveResearchPath(slug string) (string, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return "", err + } + return s.resolveResearch(slug) +} diff --git a/internal/store/rename.go b/internal/store/rename.go index cbbdbd1f..00706a8b 100644 --- a/internal/store/rename.go +++ b/internal/store/rename.go @@ -25,6 +25,9 @@ var firstH1Re = regexp.MustCompile(`(?m)^# .*$`) // rename is a rare, deliberate, single-user operation (git is the undo). A dry run runs // every check and returns the would-be result without touching disk. func (s *FS) RenameTask(slug, newTitle string, dryRun bool) (domain.Task, int, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Task{}, 0, err + } oldPath, err := s.resolve(slug) if err != nil { return domain.Task{}, 0, err diff --git a/internal/store/researchstore.go b/internal/store/researchstore.go index e9540bdb..eab69bf6 100644 --- a/internal/store/researchstore.go +++ b/internal/store/researchstore.go @@ -19,6 +19,9 @@ import ( // ListResearch scans the research dir. An unreadable doc is skipped and reported as // a FileProblem (one bad file doesn't blind the listing); err is only for fatal I/O. func (s *FS) ListResearch() ([]domain.Research, []domain.FileProblem, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return nil, nil, err + } return scanDir(s.researchDir, func(path string, content []byte) (domain.Research, error) { return parseResearch(content, path) }) @@ -26,6 +29,9 @@ func (s *FS) ListResearch() ([]domain.Research, []domain.FileProblem, error) { // GetResearch returns one research doc plus its markdown body. func (s *FS) GetResearch(slug string) (domain.Research, string, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Research{}, "", err + } path, err := s.resolveResearch(slug) if err != nil { return domain.Research{}, "", err @@ -112,6 +118,9 @@ func (s *FS) resolveResearchPathExact(entityID string) (string, error) { // `status: reference` on the legacy corpus rides along untouched). The service injects // updated_at; protected fields are rejected before we get here. func (s *FS) SetResearchFields(slug string, updates map[string]any, dryRun bool) (domain.Research, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Research{}, err + } path, err := s.resolveResearch(slug) if err != nil { return domain.Research{}, err @@ -171,6 +180,9 @@ func (s *FS) SetResearchFields(slug string, updates map[string]any, dryRun bool) // guard, but the version-CAS recheck still catches a concurrent edit during the editor // window. Returns the reloaded doc and whether it changed. func (s *FS) EditResearch(slug string, now time.Time, edit func(current string, prevErr error) (string, error)) (domain.Research, bool, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Research{}, false, err + } path, err := s.resolveResearch(slug) if err != nil { return domain.Research{}, false, err @@ -196,6 +208,9 @@ func (s *FS) EditResearch(slug string, now time.Time, edit func(current string, // write, stamping updated_at. `created` stays immutable — the id is minted from it. The // agent face of body editing, beside EditResearch's editor. func (s *FS) AppendResearchBody(slug, text string, now time.Time, dryRun bool) (domain.Research, string, error) { + if err := s.rejectGraphPlannerCall(); err != nil { + return domain.Research{}, "", err + } path, err := s.resolveResearch(slug) if err != nil { return domain.Research{}, "", err diff --git a/internal/wire/schema_comments.json b/internal/wire/schema_comments.json index 8460d97f..310b1056 100644 --- a/internal/wire/schema_comments.json +++ b/internal/wire/schema_comments.json @@ -69,6 +69,7 @@ "github.com/andy-esch/taskflow/internal/domain.Task.ID": "ID is the stable 12-char identifier (ADR-0003 §3): it leads the flat filename\n(tasks/\u003cid\u003e-\u003cslug\u003e.md) and is the primary resolution key.", "github.com/andy-esch/taskflow/internal/domain.Task.LegacyBlockedBy": "These fields are read-only legacy vocabulary. Keeping them on the typed record\nlets the strict snapshot resolve and diagnose the live slug references without\ntreating them as canonical edges or silently dropping them during analysis. The\nguarded dependency-migration slice removes them later.", "github.com/andy-esch/taskflow/internal/domain.Task.RevisitAt": "optional \"snooze until\" date for a deferred task (set by `task defer`)", + "github.com/andy-esch/taskflow/internal/domain.Task.SourceVersion": "SourceVersion is the store-internal hash of the exact bytes that produced this\nrecord. TaskGraph retains it for whole-snapshot CAS but clears it from Task()\nprojections, so planners never receive persistence tokens.", "github.com/andy-esch/taskflow/internal/domain.Task.StartedAt": "stamped when a task enters in-progress (incl. `new --start`)", "github.com/andy-esch/taskflow/internal/domain.Task.StatusFellBack": "StatusFellBack is set by the store when the frontmatter status is missing or\nunrecognized — under the flat layout (ADR-0003 §4) there is no directory to fall\nback to, so Status keeps its raw value; the task still lists and lint flags it\n(FrontmatterStatusIssues).", "github.com/andy-esch/taskflow/internal/domain.Transition": "Transition is one lifecycle action: the verb a user names (from the CLI or the TUI action menu) mapped to the destination state it moves a document to.", diff --git a/planning/adrs/0006-adopt-threads-as-task-dags.md b/planning/adrs/0006-adopt-threads-as-task-dags.md index 50e36ba8..9a2e3fee 100644 --- a/planning/adrs/0006-adopt-threads-as-task-dags.md +++ b/planning/adrs/0006-adopt-threads-as-task-dags.md @@ -722,6 +722,50 @@ were too easy to misread or bypass. The following clarifications supersede confl IDs are attributed to every source path without silently assigning one record's graph defects to another. Strict mutation still fails closed when no unique authoritative record exists. +### 2026-08-27: Portable mutation-guard adversarial hardening + +Two independent implementation audits—[Gemini](../audits/6g45qfv27vrm-2026-08-27-portable-repository-graph-mutation-guard.md) +and [Claude](../audits/6g45s2rm09pr-2026-08-27-portable-repository-graph-mutation-guard-claude.md)—validated the +control-inverted boundary and cross-process cycle protection but exposed underspecified recovery, +contention, and platform contracts. The following clarifications supersede conflicting wording above: + +1. **Planner write order is recovery data.** Stable-ID ordering is deterministic but not generally + prefix-safe: an edge reversal must durably remove the old edge before adding its reverse. A pure + core validator preserves the planner-provided order, canonicalizes only semantic sets such as + `depends_on`, and rejects the complete plan before writing unless every supplied prefix and the + final state are sound. Planners must therefore emit a deterministic, convergent sequence. +2. **Planner exclusion is scoped to the canonical planning root.** During the callback, every Store + entry point for that root—including through a second `FS`—fails fast with `ErrConflict`. This + converts invalid re-entry into an attributable error and prevents a callback from escaping its + immutable snapshot. Go cannot reliably distinguish the callback goroutine from an unrelated + caller without threading an explicit execution capability through every port, so an unrelated + concurrent read or write may receive the same brief conflict. CLI use is unaffected; a future + TUI/server adapter should treat it as retryable contention or deliberately revise the port. +3. **Only runtime-tested release platforms claim mutation support.** macOS and Linux use the + canonical-root process mutex plus root-directory `flock`, with real same-process and child-process + tests over the production path. Windows and other non-Unix source builds fail closed until a + shared-repository lock identity is selected and exercised in native CI; a per-user cache lock is + insufficient for cross-user repositories. +4. **Raw-editor detection is best effort, not transactional isolation.** Real apply performs one + whole-snapshot content check before the first replacement and another content check immediately + before each target replacement. This preserves an attributable durable prefix when a later raw + edit is detected and substantially narrows the clobber window. A raw writer can still race the + final verify-to-rename interval because it does not honor the advisory guard; the operation never + claims an all-files transaction or isolation from direct filesystem edits. +5. **Core owns plan semantics; store owns persistence mechanics.** Source-health, dependency-set, + prefix, and final-graph validation are pure core operations usable by command preview code. The + store owns canonical loading, repository exclusion, frontmatter materialization, exact source + comparison, immediate per-file CAS, and atomic replacement. Dependency writes stamp + `updated_at` from the caller-provided clock only when graph-owned fields change. +6. **Graph dry-run is authoritative but not durable.** It takes the same exclusive repository guard + through snapshot, planning, validation, and materialization so its preview is internally + consistent. Because it performs no replacement, it does not run the pre-apply CAS and cannot + promise that a later real invocation sees the same repository. +7. **Prefix validation cost is a bulk-apply gate, not premature V1 machinery.** Rebuilding the full + graph per changed task is acceptable for direct dependency commands and remains intentionally + simple. Before the bulk-linking slice is released, benchmark realistic repository/manifest sizes + and replace it with incremental validation if lock-held latency is operationally significant. + ## Related - Supersedes: [0002-adopt-projects](0002-adopt-projects.md). diff --git a/planning/audits/6g45qfv27vrm-2026-08-27-portable-repository-graph-mutation-guard.md b/planning/audits/6g45qfv27vrm-2026-08-27-portable-repository-graph-mutation-guard.md new file mode 100644 index 00000000..fe687d30 --- /dev/null +++ b/planning/audits/6g45qfv27vrm-2026-08-27-portable-repository-graph-mutation-guard.md @@ -0,0 +1,232 @@ +--- +schema: 1 +id: 6g45qfv27vrm +bucket: closed +area: portable-repository-graph-mutation-guard +date: "2026-08-27" +updated_at: "2026-08-27" +--- + +# Audit: Portable Repository Graph Mutation Guard — 2026-08-27 + +Adversarial implementation-readiness review of the portable repository graph mutation guard in Taskflow (task `6g3q4rt0wzkq`, epic `30-threads-and-task-dependency-graphs`, branch `feat/portable-graph-mutation-guard`), evaluated against ADR-0006, `docs/ARCHITECTURE.md`, upcoming dependency mutations (`6g3q4rt7mgjn`), and multi-platform concurrency requirements. + +**Executive Verdict: Safe with amendments.** The core control-inverted boundary is well-designed: the store owns the repository-wide critical section and atomic file operations while invoking pure core planners; `core.LoadTaskGraph` unifies diagnostic and mutation snapshot loading; `SourceVersion` enables whole-repository CAS without leaking persistence tokens to planners or wire contracts; and panic/termination paths cleanly release locks. However, two high-severity defects must be amended before production usage: +1. **Destructive write re-sorting (H1):** `normalizeTaskGraphPlan` forcefully re-sorts multi-file write plans by arbitrary `TaskID` strings (`graphmutation.go:124`), destroying planner-provided topological write sequences. When an atomic graph update involves both edge additions and removals, arbitrary ID sort order can place an addition ahead of a removal, causing intermediate prefix validation to fail with a spurious cycle error and rejecting valid plans. +2. **Re-entry sentinel scope gaps (H2):** Tracking `plannerActive` on the specific `*FS` instance struct (`lock.go:51`) leaves a self-deadlock vulnerability if a planner indirectly uses a secondary `*FS` instance for the same root (e.g. via a sub-service), and causes legitimate concurrent reads on a shared `*FS` instance in multi-goroutine environments to fail spuriously with `ErrConflict`. + +Findings are classified as **[pre-merge blocker]**, **[pre-mutation prerequisite]**, or **[tracked follow-up]**. + +--- + +## Findings + +### High + +#### H1. Enforcing arbitrary `TaskID` sort order on multi-file write plans rejects valid prefix-safe mutations · **Status:** fixed + +**File:** internal/store/graphmutation.go:124-126, 183-187 | **Component:** store / graph mutation +**Effort:** M · **Urgency:** acute + +**[pre-merge blocker / pre-mutation prerequisite]** + +`normalizeTaskGraphPlan` unconditionally re-sorts `plan.TaskWrites` by `TaskID` string (`lines 124-126`): +```go +sort.Slice(normalized.TaskWrites, func(i, j int) bool { + return normalized.TaskWrites[i].TaskID < normalized.TaskWrites[j].TaskID +}) +``` +Each intermediate prefix in that sorted order is then validated against `prefixGraph.Health() == core.GraphBroken` (`lines 183-187`). + +When an atomic graph mutation contains both edge additions and edge removals (for example, reversing a dependency `A -> B` into `B -> A`, or restructuring a cyclic subgraph into a DAG), the planner knows the safe topological execution sequence: execute removals first (breaking the old edge), then additions (introducing the new edge). + +Because `normalizeTaskGraphPlan` discards the planner's write ordering and enforces lexical `TaskID` ordering: +1. If the dependent task's random Crockford-32 ID happens to sort before the prerequisite's ID, the addition write is evaluated before the removal write. +2. Prefix 1 introduces the reverse edge while the old edge still exists, creating a temporary cycle `A <-> B`. +3. `normalizeTaskGraphPlan` aborts with `planned write prefix ending at task ... would leave a broken graph: dependency cycle`. +4. The success or failure of valid multi-file mutations becomes entirely dependent on random task ID hash collisions. + +**Why tests missed it:** `TestMutateTaskGraphRejectsBrokenIntermediatePrefixBeforeWriting` intentionally constructed a test where ID sorting caused a cycle, but concluded that "a planner with an unsafe mixed rewrite must split it into prefix-safe operations" rather than allowing the planner to supply the deterministic safe write order. + +**Recommendation:** Respect the planner-supplied write order in `TaskGraphMutationPlan` by default, validating each prefix in the planner's provided sequence. Alternatively, automatically partition writes into a removal wave followed by an addition wave when ordering multi-file operations. + +**Resolution:** Core now preserves the planner-provided deterministic write +order and validates every supplied prefix; regression tests prove +removal-before-addition edge reversal succeeds while the unsafe order is +rejected before writing. + +#### H2. Instance-local `plannerActive` sentinel allows self-deadlock via secondary `FS` instances and rejects legitimate concurrent reads · **Status:** fixed + +**File:** internal/store/lock.go:51-69, 87-95, internal/store/fsstore.go:49 | **Component:** store / concurrency & re-entry +**Effort:** M · **Urgency:** acute + +**[pre-merge blocker]** + +`plannerActive` is stored as an `atomic.Bool` field on the specific `*FS` instance (`fsstore.go:49`). Every Store method checks `s.rejectGraphPlannerCall()` (`lock.go:64-69`). + +This design has two symmetric flaws: + +1. **Self-Deadlock via Secondary Store Instance:** If a planner callback invokes code or a constructor that creates a new `*FS` instance rooted at the same planning directory (for example, `fs2 := store.NewFS(s.root)` or a helper service), `fs2.plannerActive` is `false`. When `fs2` attempts any write operation (such as `fs2.SetFields`), `fs2.rejectGraphPlannerCall()` passes. `fs2.checkedWriteLock()` then calls `processRepositoryLock(s.root)`, which attempts to acquire the non-reentrant `sync.Mutex` in `repositoryMutexes.byRoot[key]`. Because that mutex is already held by the same goroutine in `fs1`, the process **self-deadlocks and hangs permanently**. +2. **Spurious Conflict on Unrelated Concurrent Reads:** In a long-lived process (such as a future daemon, web server, or TUI with background refresh routines) sharing a single `*FS` instance across goroutines, while Goroutine 1 is executing a planner inside `fs.MutateTaskGraph`, any concurrent read in Goroutine 2 (e.g. `fs.GetTask`, `fs.ListTasks`, `fs.ResolveTaskPath`) calls `fs.rejectGraphPlannerCall()` and fails with `ErrConflict` ("graph mutation planner cannot call Store methods"). Non-locking concurrent reads should not be rejected simply because another goroutine is planning a mutation. + +**Why tests missed it:** `TestMutateTaskGraphRejectsPlannerStoreCallsAndNestedMutationWithoutHanging` only tested nested Store calls made directly on the exact same `fs` variable from within the planner goroutine. + +**Recommendation:** Associate the planner re-entry check with the repository lock state for that canonical root key (or goroutine-local context) rather than an instance-wide struct field. Ensure read methods that do not take the write lock are permitted from separate goroutines. + +--- + +**Resolution:** Planner activity is now recorded on the canonical-root guard, so +same- and second-FS calls fail promptly rather than deadlocking or escaping the +snapshot. ADR-0006 explicitly adopts root-wide fail-fast contention because Go +cannot reliably distinguish callback and unrelated goroutines without a new +capability-bearing port. + +### Medium + +#### M1. Windows lock file placement in `UserCacheDir` fails cross-user / multi-user repository concurrency · **Status:** fixed + +**File:** internal/store/lock_windows.go:29-38 | **Component:** store / platform lock (Windows) +**Effort:** S · **Urgency:** soon + +**[pre-mutation prerequisite / platform]** + +On Windows, `platformWriteLockChecked` locates the lock file under `os.UserCacheDir()` (`%LocalAppData%\tskflwctl\locks\.lock`). + +If a planning repository on a shared drive, multi-user workstation, or CI agent is accessed by multiple OS user accounts or service accounts, each user resolves a different `UserCacheDir`. User A acquires `LockFileEx` on `C:\Users\Alice\AppData\Local\...`, while User B acquires `LockFileEx` on `C:\Users\Bob\AppData\Local\...`. The OS-level locking guarantee is completely bypassed across user boundaries. + +**Why tests missed it:** Tests run in single-user Unix and CI environments. + +**Recommendation:** Locate repository lock files in a repository-local directory (such as `meta/.lock` or `.taskflow.lock`) or use a Windows Named Mutex (`windows.CreateMutex` with a global namespace name derived from the canonical repository path). + +**Resolution:** Windows no longer claims an untested per-user cache lock +guarantee. The non-Unix build fails closed with ErrValidation until a cross-user +lock identity has native runtime coverage; macOS/Linux remain the supported +release matrix. + +#### M2. Quadratic graph reconstruction $\mathcal{O}(K \times (V+E))$ during prefix validation limits scalability of bulk operations · **Status:** tracked by 6g3q4rtv8d0a + +**File:** internal/store/graphmutation.go:183-187, 198-204 | **Component:** store / performance & scale +**Effort:** M · **Urgency:** soon + +**[tracked follow-up / monitor]** + +In `normalizeTaskGraphPlan`, for every write $i \in [1..K]$ in a plan, `taskGraphFromMap` constructs a full `NewTaskGraph` (`graphmutation.go:203`). Each `NewTaskGraph` executes full topological wave analysis, Tarjan strongly connected components cycle detection, and sound completion memoization over all $V$ tasks in the repository. + +For a bulk operation involving $K$ writes in a repository with $V$ tasks and $E$ edges, prefix validation takes $\mathcal{O}(K \times (V + E))$ time while holding the exclusive repository write lock. For large planning repositories ($V > 2,000$) and large bulk manifests ($K > 200$), this introduces noticeable write-lock holding times. + +**Why tests missed it:** Unit test suites tested small graphs with $V \le 5$ and $K \le 2$. + +**Recommendation:** Use incremental cycle and edge validation for intermediate prefix checks, reserving full `NewTaskGraph` construction for the initial snapshot and the final graph state. + +--- + +**Resolution:** The bulk-link task now requires representative guarded-path +benchmarks, an explicit lock-latency budget, and incremental prefix validation +if the measured O(W × (V+E)) cost is material. + +### Low + +#### L1. Unreadable file changes during planning are only coarsely detected by slice length in `SameSourceSnapshot` · **Status:** fixed + +**File:** internal/core/dependency_graph.go:751 | **Component:** core / graph snapshot +**Effort:** XS · **Urgency:** eventually + +**[tracked follow-up]** + +`SameSourceSnapshot` verifies task file byte hashes via `SourceVersion`, but checks `problems` only by slice length: `len(g.problems) == len(other.problems)`. If an unreadable file `tasks/bad1.md` is replaced by a different corrupted file `tasks/bad2.md` during planning, the problem count remains unchanged. + +While per-file CAS and final reload mitigate corrupted writes, `SameSourceSnapshot` should strictly match problem file paths and codes. + +**Why tests missed it:** CAS test cases tested task content edits, deletions, and additions, rather than same-count unreadable file replacements. + +**Recommendation:** Compare `problems` slice contents (or problem file paths) in `SameSourceSnapshot`. + +--- + +**Resolution:** SameSourceSnapshot now compares the full deterministic +graph-problem and legacy-diagnostic identities, including paths, codes, +messages, cycles, resolutions, candidate IDs, and projected edges; a +same-count/different-unreadable-set regression test fails the old behavior. + +## Assumptions That Survived Adversarial Review + +These claims and invariants were independently verified under adversarial conditions and found sound: + +1. **Control-Inversion Boundary & Pure Core Separation:** + - Store exclusively owns filesystem locking, snapshot loading, YAML parsing/frontmatter surgery, and atomic replacement (`writeFileAtomic`). + - Planner callbacks receive immutable `*core.TaskGraph` projections and return pure `core.TaskGraphMutationPlan` values; no graph library types or persistence handles leak into domain or wire layers. +2. **Whole-Snapshot CAS via `SourceVersion`:** + - Every scanned task receives an internal SHA-256 byte hash (`SourceVersion`). + - `SameSourceSnapshot` verifies that all task files and paths remain byte-identical before writes begin. + - `graph.Task(id)` strips `SourceVersion`, preventing persistence tokens from leaking to planners, YAML, or wire DTOs. +3. **Resumable Multi-File Atomic Replacements:** + - Every task file is written atomically via temporary files and `os.Rename`. + - `TaskGraphMutationResult.AppliedTaskIDs` accurately records the durable applied prefix if an error or interruption occurs. + - Re-running the planner over a partial prefix converges idempotently (`TestMutateTaskGraphReturnsDurablePrefixAndRerunConverges`). +4. **Panic and Error Lock Recovery:** + - Defers in `MutateTaskGraph` (`graphmutation.go:41-50`) guarantee that `unlock()` is executed even if the planner callback panics or returns an error (`TestMutateTaskGraphPlannerPanicReleasesGuard`). +5. **Unix Lock Semantics & Process Termination Cleanup:** + - macOS and Linux use root-directory `flock(LOCK_EX)` with an in-process mutex. + - Subprocess termination tests confirm the OS immediately releases locks upon unexpected process termination (`TestRepositoryLockReleasesWhenProcessTerminates`). +6. **Explicit Platform Rejection for Unsupported Targets:** + - Non-Unix, non-Windows platforms (e.g. WebAssembly) fail explicitly with `"repository mutation locking is unsupported on this platform"`, avoiding silent no-op CAS loss. +7. **Entity Creation Guard Participation:** + - `CreateTask`, `CreateAudit`, `CreateEpic`, and `CreateResearch` now acquire `writeLock()`, preventing creation collisions and race conditions against concurrent mutations. +8. **Legacy Migration Support:** + - `MutateTaskGraph` accepts `GraphDegraded` snapshots and allows `ClearLegacy: true` writes to transition the repository to `GraphHealthy` in one guarded operation. + - Broken graph snapshots (`GraphBroken`) are rejected before the planner is invoked. + +--- + +## Sequencing & Architectural Critique + +1. **Readiness for Slice 2 (`6g3q4rt7mgjn`):** `TaskGraphMutationStore` provides the exact mutation boundary needed by `task depend add/remove` and legacy migration. However, resolving **H1** (write ordering) is a strict prerequisite so that multi-edge and reverse-edge operations do not fail on prefix cycle checks. +2. **Integration with `core.LoadTaskGraph`:** Moving snapshot construction to `core.LoadTaskGraph` successfully unifies diagnostic reads and mutations, eliminating scan drift. + +--- + +## Traceability Table + +| Finding | Severity | Classification | Action / Target Destination | +|---|---|---|---| +| **H1** (Destructive write re-sorting) | High | Pre-merge blocker | Reopen `6g3q4rt0wzkq` / fix in `graphmutation.go` to preserve planner write order | +| **H2** (Instance-local `plannerActive` sentinel) | High | Pre-merge blocker | Reopen `6g3q4rt0wzkq` / bind re-entry check to root lock state | +| **M1** (Windows `UserCacheDir` isolation) | Medium | Pre-mutation prerequisite | Tracked by `6g3q4rt0wzkq` Windows platform task | +| **M2** ($\mathcal{O}(K \times (V+E))$ prefix rebuilding) | Medium | Tracked follow-up / monitor | Tracked by bulk-operations performance task | +| **L1** (Problem comparison in `SameSourceSnapshot`) | Low | Tracked follow-up | Tracked by follow-up CAS refinement | + +--- + +## Validation Commands and Results + +All checks executed in worktree `/Users/andyeschbacher/git/andy-esch/taskflow-graph-mutation-guard` on branch `feat/portable-graph-mutation-guard`: + +1. **Full Test Suite:** + ```bash + go test ./... + ``` + *Result:* Passed (all 25 packages passed). + +2. **Race Detection Test Suite:** + ```bash + go test -race ./... + ``` + *Result:* Passed (all packages passed with zero data races detected). + +3. **Store Package In-Depth Tests:** + ```bash + go test -v ./internal/store/... + ``` + *Result:* Passed (120+ tests including fuzzing, concurrent opposite edges, and crash recovery passed). + +4. **Audit Lint Validation:** + ```bash + tskflwctl audit lint 2026-08-27-portable-repository-graph-mutation-guard + ``` + *Result:* Passed. + +## Candidate tasks + +- ⏳ `tskflwctl task new "Preserve planner write order in graph mutation prefix validation" --epic 30-threads-and-task-dependency-graphs --tags storage,graph` — Stop forcing TaskID lexical sort order on TaskWrites in normalizeTaskGraphPlan (H1) +- ⏳ `tskflwctl task new "Bind planner re-entry check to root lock state instead of FS instance" --epic 30-threads-and-task-dependency-graphs --tags storage,concurrency` — Fix secondary FS self-deadlock and avoid rejecting concurrent reads from separate goroutines (H2) +- ⏳ `tskflwctl task new "Fix cross-user repository lock path on Windows" --epic 30-threads-and-task-dependency-graphs --tags storage,windows` — Use repo-local lock file or global named mutex instead of UserCacheDir (M1) diff --git a/planning/audits/6g45s2rm09pr-2026-08-27-portable-repository-graph-mutation-guard-claude.md b/planning/audits/6g45s2rm09pr-2026-08-27-portable-repository-graph-mutation-guard-claude.md new file mode 100644 index 00000000..b976a221 --- /dev/null +++ b/planning/audits/6g45s2rm09pr-2026-08-27-portable-repository-graph-mutation-guard-claude.md @@ -0,0 +1,855 @@ +--- +schema: 1 +id: 6g45s2rm09pr +bucket: closed +area: portable-repository-graph-mutation-guard-claude +date: "2026-08-27" +updated_at: "2026-08-27" +--- + +# Audit: portable-repository-graph-mutation-guard-claude — 2026-08-27 + +> Edit findings in place and flip each `**Status:**` as you work it. + +Adversarial implementation-readiness review of the portable repository graph mutation +guard (task `6g3q4rt0wzkq`, epic `30-threads-and-task-dependency-graphs`, branch +`feat/portable-graph-mutation-guard`, uncommitted tree vs merge-base `509a7c3`). + +Method: every claim below was executed, not inferred. Counterexamples ran through +`go test -overlay=` against probe files held outside the repository, so no production +or test file was modified — `git status --porcelain` was identical before and after +(19 modified, 5 new implementation files), and this audit is the only repository +write. Two mechanisms were additionally checked by **mutation testing** — deliberately +breaking a guard and re-running the committed suite to see whether anything fails. + +A sibling audit `6g45qfv27vrm` covers the same slice. Where we independently reach the +same conclusion that is stated as corroboration with my own evidence; the majority of +what follows (H3, H4, M1, M4, M5, M6, L1, L3, L4, L5, L6, L7) is not in it, and my +verdict is harder. + +## Executive verdict + +**Not ready to merge; safe with a bounded set of amendments.** The design is right and +the hard parts work: cross-process serialization of `MutateTaskGraph` is real — I +confirmed it with actual subprocesses racing opposite edges, and the loser was rejected +with a cycle diagnosis while the final graph stayed healthy with exactly one edge. The +control-inverted boundary, the persistence-token hygiene, the prefix-safety rule, and +the interrupt/rerun convergence story all survived attack. + +Three things block merge: + +1. **The re-entry guard is instance-local while the lock it protects is root-global** + (H1). A planner holding a second `*FS` on the same repository is not detected: a + read escapes the snapshot and returns live state; a write **hangs forever** with no + timeout and no error. `docs/ARCHITECTURE.md` states "Planner callbacks cannot + re-enter the Store; nested access fails explicitly instead of self-deadlocking" — + that sentence is false as written, and acceptance criterion 3 is ticked on it. +2. **`rejectGraphPlannerCall` cannot tell a re-entrant planner from an unrelated + concurrent caller** (H2). Any goroutine sharing the `*FS` — which is the TUI's + shape, one `Service` behind `tea.Cmd`s — gets `ErrConflict` reading "graph mutation + planner cannot call Store methods" for a plain `GetTask`. Reads previously could not + fail for concurrency reasons at all. +3. **The two mechanisms this slice actually adds have no test that fails when they are + deleted** (H3). Replacing `processRepositoryLock` with a no-op leaves the entire + `internal/store` suite green. Removing symlink resolution from `repositoryLockKey` + leaves it green. `TestRepositoryLockSerializesIndependentStoresInOneProcess` cannot + distinguish the new mutex from flock, because flock alone already blocks a second + in-process handle on Linux/macOS. Acceptance criterion 1 says "explicit, **tested** + … contract" and is ticked. + +Separately, the cross-process and termination tests drive `platformWriteLock`, a helper +with **zero production callers** (H4) — so the product lock path has no cross-process +coverage at all; I had to write that test myself. + +Two ticked acceptance criteria are additionally not met as written: AC2 ("without … +teaching the store graph semantics") — the store now owns cycle, prefix-safety, legacy +field, and health validation (M4); and AC6 ("remove or fold any duplicate or otherwise +unused `ReadTaskGraph` scan seam so lint and mutation cannot drift") — `Service.Lint` +still builds its own snapshot and `ReadTaskGraph` still has no callers (M5). + +Classifications: **[pre-merge blocker]**, **[follow-up]**, **[monitor]**. + +--- + +## Findings + +#### H1. Planner re-entry detection is per-`*FS` while the guard is per-root: a second Store on the same repository deadlocks, or silently escapes the snapshot · **Status:** fixed + +**File:** internal/store/lock.go:51-69; internal/store/fsstore.go:45-49 | **Component:** store / concurrency +**Effort:** M · **Urgency:** acute + +**[pre-merge blocker]** + +`plannerActive` is an `atomic.Bool` field on the `FS` struct (`fsstore.go:45-49`), and +`rejectGraphPlannerCall` (`lock.go:64-69`) consults only the receiver. The lock it +protects — `processRepositoryLock` (`lock.go:38-49`) — is keyed by canonical root and +shared by every `FS` in the process. The two scopes do not match, so the failure the +guard exists to convert into an error is reachable again through a second handle. + +Reproduction (executed): + +``` +TestADV_SecondFSWriteInPlannerDeadlocks + DEADLOCK CONFIRMED: planner write via a second *FS on the same root hangs + with no timeout and no attributable error (3.00s, aborted) + +TestADV_SecondFSGivesPlannerLiveState + snapshot priority="high" live-read-through-second-FS priority="low" err= +``` + +The write case hangs on `mutex.Lock()` inside `processRepositoryLock` — no deadline, no +`ErrConflict`, no stack attribution; the operator sees a wedged process. The read case +is quieter and worse in kind: the planner obtained **live, unsynchronized filesystem +state** that contradicts the immutable snapshot it was handed, which is precisely the +isolation the control-inverted boundary sells. In my probe the mutation happened to +abort afterwards on the whole-snapshot CAS, but only because the probe also wrote; a +read-only escape leaves no trace. + +The comment at `lock.go:14-18` names this scenario as the motivation ("a future +long-lived adapter may create more than one `*FS`") and concludes the keyed mutex +"makes the combined contract identical within one [process]". It makes *serialization* +identical; it leaves *re-entry detection* instance-local, which flips the failure mode +from an attributable conflict to a silent hang. + +**Why existing tests miss it:** +`TestMutateTaskGraphRejectsPlannerStoreCallsAndNestedMutationWithoutHanging` +(`graphmutation_test.go:220-254`) captures the *same* `fs` in its planner closure — the +one shape the instance-local sentinel covers. No test constructs a second `NewFS(root)`. + +**Recommendation:** move the sentinel next to the mutex — store the owning +`*FS` (or a mutation-generation counter) in `repositoryMutexes.byRoot` keyed by +`repositoryLockKey`, and have `rejectGraphPlannerCall` consult that. Same code volume, +and it closes both the deadlock and the live-read escape. + +**Resolution:** Planner activity moved from one FS instance to the +canonical-root guard. A callback using a second FS now receives ErrConflict for +reads, writes, lint helpers, and nested mutation instead of reading live state +or hanging; regression coverage uses a second NewFS(root). + +#### H2. `rejectGraphPlannerCall` cannot distinguish re-entry from legitimate concurrency, so unrelated operations fail with a false, misleading error · **Status:** fixed + +**File:** internal/store/lock.go:64-69, 86-89 | **Component:** store / concurrency +**Effort:** M · **Urgency:** acute + +**[pre-merge blocker]** + +Every Store entry point begins with `rejectGraphPlannerCall`, which tests one process- +wide-per-instance boolean. It has no way to know whether the caller is the planner +goroutine or an unrelated one. Executed: + +``` +TestADV_LegitimateConcurrencyMisattributed + concurrent unrelated GetTask during mutation -> graph mutation planner cannot + call Store methods or begin a nested mutation: conflict (errors.Is ErrConflict = true) + concurrent unrelated SetFields during mutation -> (same error) +``` + +Two distinct harms: + +- **Reads now fail.** `GetTask`, `ListTasks`, `ListEpics`, `GetAudit`, and the four + `Resolve*Path` helpers are all guarded (`paths.go:9-36`). Before this slice a read + could not fail for a concurrency reason. A TUI refresh landing during a graph + mutation now surfaces exit-14 conflict text. +- **The attribution is wrong.** The message accuses the operator's planner of illegal + re-entry when the real situation is ordinary contention that the lock would have + handled by waiting. + +This is not hypothetical for the roadmap: `docs/ARCHITECTURE.md` describes the TUI as +reading through one `core.Service` as `tea.Cmd`s, which run in separate goroutines over +one `*FS`; and the slice's own rationale cites "a future long-lived adapter". As soon as +`ship-guarded-dependency-mutations-and-graph-queries` or +`add-usage-informed-thread-views-to-the-tui` puts a mutation behind that Service, this +fires. + +**Why existing tests miss it:** every concurrency test either calls from inside the +planner (expecting the conflict) or uses separate `NewFS` instances (which, per H1, are +not checked at all). Nothing exercises the same `*FS` from a non-planner goroutine. + +**Recommendation:** record the planner's goroutine identity, or — simpler and +adapter-friendly — have the mutation publish the sentinel on the *root-keyed* guard +entry (H1's fix) and reject only a caller that already holds it, letting everyone else +block on the mutex as before. Whichever way, unrelated callers must wait, not error. + +**Resolution:** The misleading instance-local error was replaced by an explicit +root-wide callback-exclusion contract. ADR-0006 records that both callback +re-entry and unrelated access fail fast during the brief planner phase; +long-lived adapters must retry contention or deliberately introduce a new +capability-bearing port. + +#### H3. The two guards this slice adds have no test that fails when they are removed · **Status:** fixed + +**File:** internal/store/lock.go:24-49; internal/store/lock_unix_test.go:108-135 | **Component:** testing / concurrency +**Effort:** M · **Urgency:** acute + +**[pre-merge blocker]** + +I ran a mutation battery — break one mechanism, run the committed suite +(`./internal/store ./internal/core`, `-count=1`): + +| mutation | committed suite result | +|---|---| +| `processRepositoryLock` → returns a no-op immediately | **PASS (survives)** | +| `repositoryLockKey` → skip `filepath.EvalSymlinks` | **PASS (survives)** | +| `rejectGraphPlannerCall` → always nil | FAIL (caught) | +| `SameSourceSnapshot` → always true | FAIL (caught) | +| `Task()` → stop clearing `SourceVersion` | FAIL (caught) | +| prefix validation → validate the final graph only | FAIL (caught) | + +The same-process mutex is the headline addition (`lock.go:14-22` justifies it at +length) and the canonical-root key is the entire identity model on Windows. Neither is +covered. + +`TestRepositoryLockSerializesIndependentStoresInOneProcess` +(`lock_unix_test.go:108-135`) is named for exactly this mechanism, but it cannot +isolate it — flock already blocks a second handle in the same process: + +``` +TestADV_ProcessMutexVersusFlockInProcess + flock alone ALREADY blocks a second in-process handle; the existing + serialization test cannot distinguish it from the new process mutex +``` + +Acceptance criterion 1 — "Every supported platform has an explicit, **tested** +cooperating-writer serialization contract" — is ticked on this evidence. + +**Recommendation:** add a test that can only pass with the mutex — e.g. inject a +platform-lock stub via an existing test hook and assert `writeLock` still serializes — +plus a `repositoryLockKey` table test over `.`/`..`, trailing separators, a symlinked +root, and (asserted, not executed) the Windows lowercase rule. Keep the mutation +battery as a review gate for this file. + +**Resolution:** A direct product-function test now fails if +processRepositoryLock becomes a no-op, canonical-root table coverage includes +dot, dot-dot, trailing separator, symlink alias, and case-folding, and the +cross-process tests exercise writeLock/MutateTaskGraph rather than an isolated +platform helper. + +#### H4. The cross-process and termination lock tests drive `platformWriteLock`, which has no production callers · **Status:** fixed + +**File:** internal/store/lock.go:107-115; internal/store/lock_unix_test.go:30, 65 | **Component:** testing / concurrency +**Effort:** S · **Urgency:** acute + +**[pre-merge blocker]** + +`platformWriteLock` is documented as "kept for direct lock-contract tests" +(`lock.go:107-108`), and a repository-wide grep finds no caller outside `lock*.go` and +`lock_unix_test.go`. Product mutations go through `writeLock`/`checkedWriteLock`, which +additionally take the process mutex and the re-entry check. So +`TestRepositoryLockReleasesWhenProcessTerminates` and its subprocess helper prove a +contract about a function the product never invokes, and **there is no test that a real +mutation in one process excludes a real mutation in another.** + +That gap matters because the product path is where the two halves interact: the process +mutex is released in the same closure as the platform lock (`lock.go:96-104`), and a +release-ordering mistake there would be invisible to the current tests. + +I wrote the missing test and it passes — the contract does hold today: + +``` +TestADV_CrossProcessOppositeEdges (two real child processes, 150ms window) + process 1: REJECTED: validation failed: planned write prefix ending at task + 6g4500000001 would leave a broken graph: dependency cycle: + 6g4500000001 -> 6g4500000002 -> 6g4500000001 + process 2: APPLIED + final health=healthy total edges=1 +``` + +Note that `TestMutateTaskGraphConcurrentOppositeEdges` +(`graphmutation_test.go:122-173`) races **goroutines**, which the process mutex alone +serializes — it does not exercise flock. + +**Recommendation:** re-point the two subprocess tests at `writeLock`, and commit a +cross-process `MutateTaskGraph` opposite-edge test like the one above. Then delete +`platformWriteLock`, which exists only to be tested. + +**Resolution:** The test-only platformWriteLock seam was deleted. Process +termination now proves a product writeLock holds and releases the OS flock, and +two real child processes racing opposite MutateTaskGraph edges yield exactly one +applied edge and one cycle rejection. + +#### M1. Multi-file plans verify every file before writing any, so a raw edit inside the apply loop is clobbered · **Status:** fixed + +**File:** internal/store/graphmutation.go:90-105 | **Component:** store / CAS +**Effort:** S · **Urgency:** soon + +**[follow-up]** + +The apply phase is two sequential loops: `verifyUnchanged` for every write +(`:90-94`), then `writeFileAtomic` for every write (`:95-105`). For a plan of *n* +files, file *n*'s CAS is validated before files 1…*n*−1 are written, so the +verify→write window for the last file is the duration of the whole apply loop. Every +other write path in the store keeps that window to a single file operation. + +Reproduction (executed) — a non-cooperating editor changes the second target after its +verify passed: + +``` +TestADV_MultiFileVerifyWriteWindowClobbers + mutation err= + raw edit survived in the second-written file: false + CLOBBER CONFIRMED: a raw edit that landed after verifyUnchanged was silently + overwritten by the plan's later write +``` + +The mutation reported **success**. The advisory lock does exclude cooperating writers, +so this needs a raw editor — but that is exactly the threat model the whole-snapshot +CAS was added for, and bulk-linking will make the window seconds long (see N1). + +**Why existing tests miss it:** `TestMutateTaskGraphCASRejectsRawEditBeforeApply` +(`:297-321`) injects its edit through `testHookBeforeGraphVerify`, i.e. strictly +*before* the verify pass. There is no hook usage that interleaves an edit between +verify and write. + +**Recommendation:** interleave the loops — verify file *i* immediately before writing +file *i* — keeping the existing all-files pre-check as a cheap early abort. The prefix +contract already tolerates a partial apply, so failing at file *i* is well-defined. + +**Resolution:** The all-target preflight remains, and every target is now +reverified immediately before its own atomic replacement. A regression test +injects a raw edit before the second target, returns the first durable +AppliedTaskIDs prefix with ErrConflict, and proves the raw edit survives. + +#### M2. Reversing a dependency edge succeeds or fails purely on stable-ID ordering · **Status:** fixed + +**File:** internal/store/graphmutation.go:124-126, 178-187 | **Component:** store / plan validation +**Effort:** M · **Urgency:** soon + +**[follow-up]** + +Writes are sorted by `TaskID` (`:124-126`) and every prefix in that order must be +non-broken (`:183-187`). Application order is therefore lexicographic, not semantic. For +an edge reversal one order is safe (remove, then add) and the other transiently cyclic — +and which one you get depends on the two ids. + +Executed, same plan shape both ways: + +``` +TestADV_EdgeReversalDependsOnIDOrdering + reverse edge, dependent has SMALLER id -> (accepted) + reverse edge, dependent has LARGER id -> validation failed: planned write prefix + ending at task ekpd8vnydg2h would leave a broken graph: dependency cycle: … + ASYMMETRY: identical semantic operation succeeds/fails on stable-ID ordering alone +``` + +For slice 3 this means `task depend` reversal-shaped operations will appear flaky to +users: the same command shape works on one pair of tasks and errors on another, with a +message about "write prefix" that names an implementation concept. + +**Why existing tests miss it:** +`TestMutateTaskGraphRejectsBrokenIntermediatePrefixBeforeWriting` (`:366-397`) +deliberately normalizes `firstID`/`secondID` (`:369-374`) so the unsafe orientation is +always chosen. The test correctly pins that unsafe prefixes are rejected; the +normalization is what hides that the safe orientation is silently allowed. + +**Recommendation:** order the apply sequence semantically — all dependency *removals* +before all *additions*, `TaskID` order within each phase — which makes every reversal +prefix-safe and stays deterministic. Corroborates sibling `6g45qfv27vrm` H1; the +executed asymmetry above is the part worth adding to it. + +**Resolution:** Core preserves planner-provided semantic order rather than +sorting task writes by ID. Tests cover a removal-before-addition reversal that +succeeds in supplied order and the opposite unsafe prefix that fails before any +write. + +#### M3. Windows lock identity is a path hash under the *user's* cache directory, and cross-compilation cannot validate any of it · **Status:** fixed + +**File:** internal/store/lock_windows.go:21-47 | **Component:** store / portability +**Effort:** M · **Urgency:** soon + +**[follow-up]** + +On Unix the lock is `flock` on the repository root's own inode, so every spelling of +the path — symlink, alias, bind mount, relative — lands on one lock. On Windows the +lock file is `os.UserCacheDir()/tskflwctl/locks/.lock`, +so mutual exclusion holds **only** when two processes compute the identical key string +under the identical cache root. It does not when: + +- two users (or a service account and an interactive user) share a repository — + different `os.UserCacheDir()`, therefore different lock files, therefore no exclusion + at all; +- the same share is reached as `Z:\repo` and `\\server\share\repo`, or through `subst`, + or through a roaming vs local profile — `filepath.EvalSymlinks` resolves junctions and + symlinks but not drive mappings; +- the cache directory is unavailable, where the code fails the mutation + (`:29-32`) rather than falling back — correct, but it makes lock availability depend + on a directory unrelated to the repository. + +Lock files also accumulate under `locks/` and are never swept. + +I verified `GOOS=windows go build ./...` and `GOOS=windows go vet ./...` are clean, and +that `go test` for `GOOS=windows` cannot run (`exec format error`). There is no +`lock_windows_test.go`. `docs/ARCHITECTURE.md` is honest — "Windows is compile-checked" +— but acceptance criterion 1 claims every supported platform has a *tested* contract and +is ticked. + +**Recommendation:** either (a) declare Windows unsupported for now and route it to +`lock_other.go`'s explicit rejection, or (b) put the lock file inside the planning +repository (e.g. `.tskflwctl.lock`, gitignored) so identity is the filesystem's job +rather than a string hash, and add a Windows CI job. Corroborates sibling +`6g45qfv27vrm` M1; the UNC/`subst` and lock-file-accumulation aspects are additional. + +**Resolution:** The untested per-user Windows cache lock was removed. Non-Unix +builds now reject repository mutation with ErrValidation; ADR-0006 limits the +supported release contract to native-tested macOS/Linux until Windows has a +cross-user identity and native CI. + +#### M4. The store now owns graph semantics that acceptance criterion 2 says it must not · **Status:** fixed + +**File:** internal/store/graphmutation.go:118-196, 242-252 | **Component:** architecture +**Effort:** M · **Urgency:** soon + +**[follow-up]** + +AC2 reads "…within one repository guard without exposing filesystem locking **or +teaching the store graph semantics**", and is ticked. But `normalizeTaskGraphPlan` +(`:118-196`) — which lives in `internal/store` — validates stable-id shape, self-edges, +duplicate edges, prerequisite existence, `updated_at` format, every deterministic write +prefix's acyclicity, and final graph health; and `materializeTaskGraphPlan` (`:242-252`) +hard-codes the canonical and legacy field names. `taskGraphFromMap` (`:198-204`) calls +`core.NewTaskGraph` from the store. + +The dependency *direction* is fine — `store` imports `core`, never the reverse — so this +is layering, not a cycle. The practical cost lands on the next two tasks: bulk-linking +and the dependency commands will want to plan, validate, and preview graph deltas +*without* a filesystem, and today that logic is only reachable by taking the repository +lock. + +**Recommendation:** extract a pure `core.ValidateGraphPlan(graph, plan) +(TaskGraphMutationPlan, error)` returning the normalized plan and the prefix/final +verdict, and leave the store with lock, read, materialize, CAS, write. That is a move, +not a rewrite, and it makes `MutateTaskGraph` usable by slice 3 without redesign. + +**Resolution:** Source-health, dependency-set, every-prefix, and final-health +validation moved to pure +core.ValidateTaskGraphMutationSource/ValidateTaskGraphMutationPlan. Store +retains only authoritative load, exclusion, frontmatter materialization, CAS, +and atomic replacement. + +#### M5. Acceptance criterion 6 is not met: lint still builds its own snapshot and `ReadTaskGraph` is still unused · **Status:** fixed + +**File:** internal/core/service.go:265-269; internal/core/service_task.go:84-99 | **Component:** core / architecture +**Effort:** S · **Urgency:** soon + +**[follow-up]** + +AC6: "The store boundary consumes one canonical strict-snapshot loader; remove or fold +any duplicate or otherwise unused `ReadTaskGraph` scan seam so lint and mutation cannot +drift." Ticked. Actually: + +- `Service.Lint` still scans via `ListTasksWithBodies` and constructs + `NewTaskGraph(taskRecords, taskProblems)` itself (`service.go:265-269`). It does not + call `LoadTaskGraph`. Two independent definitions of the authoritative snapshot remain + — the drift the criterion says was closed. +- `ReadTaskGraph` was refactored to delegate to `LoadTaskGraph` but not removed, and a + grep across `internal` and `cmd` finds **no caller**. It is still the unused seam the + criterion names. +- `LoadTaskGraph`'s parameter is an inline anonymous interface (`service_task.go:92-94`) + rather than a named port, so nothing type-checks that lint and mutation agree on the + source. + +The two paths agree today only because `ListTasks` and `ListTasksWithBodies` share +`scanDir` and `parseTask`. + +**Recommendation:** name the loader's source as a small port +(`type TaskSource interface { ListTasks() … }`), have `Service.Lint` obtain its graph +from `LoadTaskGraph`, and delete `ReadTaskGraph`. Then untick or re-scope AC6. + +**Resolution:** The unused Service.ReadTaskGraph seam was removed and +LoadTaskGraph now consumes a named TaskGraphSource. Lint's body-bearing scan and +mutation loading both feed the same NewTaskGraph strict projection and shared +store parser without adding a duplicate filesystem scan. + +#### M6. Graph writes no longer stamp `updated_at`; a pure planner has no clock to supply one · **Status:** fixed + +**File:** internal/store/graphmutation.go:253-255; internal/core/store.go:64 | **Component:** store / write contract +**Effort:** S · **Urgency:** soon + +**[follow-up]** + +`updated_at` is applied only when the planner puts a value in +`TaskDependencyWrite.UpdatedAt` (`:253-255`). Every other mutation path in the store +injects `now` itself (`SetFields`, `Move`, `EditBody`, `EditTask`, `SetEpicFields`, +`SetResearchFields`). A dependency change is therefore the one mutation that can leave +a file's `updated_at` stale, and the responsibility sits with a callback the whole +design defines as pure — planners run in `core` and have no clock. + +Executed: + +``` +TestADV_UpdatedAtNotStampedByStore + after dependency write, beta frontmatter contains updated_at "2026-01-01": true +``` + +**Why existing tests miss it:** `TestMutateTaskGraphOwnsSemanticReadValidateWriteBoundary` +always passes `UpdatedAt: "2026-08-27"` and asserts it lands. +`TestMutateTaskGraphReturnsDurablePrefixAndRerunConverges` omits it and asserts nothing +about it — so the suite contains a passing example of the defect. + +**Recommendation:** take `now time.Time` on `MutateTaskGraph` (matching `EditTask`, +`Move`, `Defer`) and stamp it in `materializeTaskGraphPlan` for every write; drop +`UpdatedAt` from `TaskDependencyWrite`, or keep it only as an explicit override. + +**Resolution:** MutateTaskGraph now requires the caller's time and the store +stamps updated_at whenever graph-owned fields semantically change. +TaskDependencyWrite remains clock-free, and an idempotent rerun on a later date +neither writes nor advances the timestamp. + +#### L1. Two `core.Store` port methods have no re-entry check and are callable from a planner · **Status:** fixed + +**File:** internal/store/fix.go:25; internal/store/danglers.go:18; internal/core/store.go:229, 237 | **Component:** store / re-entry guard +**Effort:** XS · **Urgency:** eventually + +**[follow-up]** + +Enumerating every exported `FS` method, exactly three lack `rejectGraphPlannerCall`: +`WatchPaths` (pure path arithmetic, harmless), `DanglingLinks`, and `FixFrontmatter`. +The latter two are declared members of the `core.Store` port +(`store.go:229, 237`). `FixFrontmatter(false)` is covered transitively because it takes +`writeLock`; `FixFrontmatter(true)` takes no lock, and `DanglingLinks` takes none at all. + +Executed from inside a live planner: + +``` +TestADV_UnguardedStorePortMethodsInPlanner + planner called FixFrontmatter(dryRun) -> 0 results, err= + planner called DanglingLinks() -> 0 results, err= + mutation err= +``` + +Neither writes, so nothing corrupts — but AC3 says the contract "permits no nested Store +calls", and these two are the exceptions. + +**Recommendation:** add the two guard calls; add a reflection or grep-based test that +every method in the `core.Store` port surface begins with the check, so the invariant +survives the next port addition. + +**Resolution:** FixFrontmatter, including dry-run, and DanglingLinks now +participate in canonical-root callback exclusion. The nested-access regression +covers both methods through a second FS in addition to core Store +reads/writes/mutation. + +#### L2. `SameSourceSnapshot` compares problem and legacy *counts*, not identity, and over-claims in its doc · **Status:** fixed + +**File:** internal/core/dependency_graph.go:735-750 | **Component:** core / CAS +**Effort:** S · **Urgency:** eventually + +**[follow-up]** + +The function ends with `len(g.problems) == len(other.problems) && len(g.legacy) == len(other.legacy)`. +Unreadable files never enter `ids`, `tasks`, or the path comparison, so two genuinely +different repositories compare equal whenever their problem counts match: + +``` +TestADV_SameSourceSnapshotFalsePositive + left.health=broken right.health=broken + SameSourceSnapshot(different unreadable file sets) = true +``` + +**This is currently unreachable through `MutateTaskGraph`**, which refuses a broken +snapshot before planning (`graphmutation.go:56-59`), and a *newly* unreadable file +between the two loads changes the problem count from 0 and is caught. I verified that +reasoning holds. The defect is in the exported API and its doc comment — "reports +whether two graphs came from the same exact task files … Health and paths catch new +unreadable/renamed entities" — which invites a future long-lived-service or query +consumer to rely on more than the code delivers. + +**Recommendation:** compare a sorted digest of `(code, taskID, path, message)` for +problems and `(taskID, field, values)` for legacy, or narrow the doc comment to exactly +what is compared and note the broken-health precondition. + +**Resolution:** SameSourceSnapshot compares exact deterministic problem and +legacy diagnostic content rather than counts. Coverage proves different +unreadable file sets with the same cardinality no longer compare equal. + +#### L3. Entity creation into a not-yet-existing planning root now fails at the repository lock · **Status:** fixed + +**File:** internal/store/create.go:54-59; internal/store/lock_unix.go:22-24 | **Component:** store / compatibility +**Effort:** XS · **Urgency:** eventually + +**[follow-up]** + +`writeNewFile` now takes `s.writeLock()` before `writeNewFileUnlocked`, which is where +`os.MkdirAll(dir)` used to create the tree. `platformWriteLockChecked` opens `s.root`, +so the root must already exist: + +``` +TestADV_CreateIntoMissingRoot + CreateTask into a not-yet-existing planning root -> + open repo root for write lock: open …/fresh-planning-root: no such file or directory + CreateTask into an existing but empty root -> +``` + +Not currently reachable through the CLI — repo discovery rejects a missing root first +("not a taskflow planning repo …"), which I confirmed — so this is a store-contract +change rather than a user-visible regression. It still contradicts AC5 ("ordinary write +behavior remains compatible") for any other adapter or test, and the error carries no +domain sentinel (see L4). + +**Recommendation:** `os.MkdirAll(s.root, 0o755)` in `platformWriteLockChecked` before +opening, or in `writeNewFile` before locking. One line, and it restores the prior +contract. + +**Resolution:** writeNewFile restores the previous missing-root contract by +creating the planning root before acquiring the directory-backed lock; +CreateTask coverage begins from a nonexistent root and verifies the entity +lands. + +#### L4. Lock and unsupported-platform errors carry no domain sentinel · **Status:** fixed + +**File:** internal/store/lock_unix.go:23, 26; internal/store/lock_other.go:10; internal/store/lock_windows.go:23-46 | **Component:** store / error classification +**Effort:** XS · **Urgency:** eventually + +**[follow-up]** + +`CLAUDE.md` requires errors to wrap `ErrNotFound`/`ErrValidation`/`ErrAmbiguous`/ +`ErrConflict` so the CLI maps them to exit codes 10/11/13/14. Every lock error is a bare +`fmt.Errorf`, including `lock_other.go`'s "repository mutation locking is unsupported on +this platform" — a permanent, categorical refusal that an agent cannot distinguish from +a transient I/O failure. AC4 says acquisition/release errors are "attributable"; they are +*described*, not classified. + +**Recommendation:** wrap the unsupported-platform case in `domain.ErrValidation` (a +permanent refusal) and lock acquisition failures in `domain.ErrConflict` where they are +retryable; leave genuine I/O errors unwrapped. + +**Resolution:** The categorical unsupported-platform refusal now wraps +ErrValidation. Genuine open/flock/close I/O failures retain their underlying OS +error and attributable operation context rather than being mislabeled as +retryable conflicts. + +#### L5. Dry runs hold the exclusive repository guard and skip the whole-snapshot CAS · **Status:** fixed + +**File:** internal/store/graphmutation.go:37, 73-75 | **Component:** store / concurrency +**Effort:** S · **Urgency:** eventually + +**[follow-up]** + +`MutateTaskGraph(true, …)` takes `checkedWriteLock` like a real write and holds it +through load, plan, normalize (see N1), and materialize — then returns at `:73-75` +*before* the CAS re-read. So a preview blocks every writer in the repository for the +full validation cost while itself making no durability claim about the plan it prints. + +Executed: `TestADV_DryRunHoldsExclusiveGuard` observes the planner running inside the +guard on a dry run. + +**Recommendation:** keep the lock (a consistent snapshot needs it) but document that +`--dry-run` is exclusive, and note in the result that a dry-run plan is not +CAS-validated. If contention matters later, a shared/read lock mode is the fix. + +**Resolution:** ADR-0006 and ARCHITECTURE now state that graph dry-run +intentionally holds the exclusive guard for an internally consistent +authoritative preview, skips pre-apply CAS because it writes nothing, and makes +no durability claim about a later invocation. + +#### L6. `repositoryMutexes.byRoot` never evicts · **Status:** fixed + +**File:** internal/store/lock.go:19-22, 38-49 | **Component:** store / resource use +**Effort:** XS · **Urgency:** eventually + +**[monitor]** + +Entries are created on first use and never removed. For the CLI this is bounded by one +root per process. For the long-lived multi-repository adapter the file's own comment +cites as motivation, it is an unbounded map keyed by attacker-independent but +unbounded path strings. + +**Recommendation:** none now — note it beside the comment so the future adapter's author +sees it. Reference-count and delete at zero if a service ever lands. + +**Resolution:** The root-guard registry comment now records the CLI's one-root +bound and requires reference-counted idle eviction if a future long-lived +multi-space adapter makes the map unbounded. + +#### L7. Lock tests use a fixed 100 ms timeout as the success branch · **Status:** fixed + +**File:** internal/store/lock_unix_test.go:78, 126 | **Component:** testing +**Effort:** XS · **Urgency:** eventually + +**[follow-up]** + +Both serialization tests treat `case <-time.After(100 * time.Millisecond)` as *proof* +that the second acquirer is blocked. A goroutine that is merely slow to be scheduled — +a loaded CI runner, `-race`, a cold `t.TempDir()` — produces the same observation, so +the assertion can pass without the property holding. I ran the concurrency tests +`-race -count=20` with no failures, so there is no flake today; the concern is a +vacuous pass, not an intermittent one. + +**Recommendation:** have the second acquirer signal "about to block" before calling, and +assert the ordering of acquisition (e.g. a shared counter) rather than the absence of an +event within a wall-clock budget. + +**Resolution:** The same-process mutex test uses TryLock state rather than +elapsed time. The child-process test uses a nonblocking flock probe against a +product-held lock; fixed delay is no longer the success assertion. + +#### N1. Prefix validation rebuilds the whole repository graph once per planned write, inside the exclusive lock · **Status:** tracked by 6g3q4rtv8d0a + +**File:** internal/store/graphmutation.go:183-187, 198-204 | **Component:** store / performance +**Effort:** M · **Urgency:** soon + +**[monitor]** + +`normalizeTaskGraphPlan` calls `taskGraphFromMap` — a full `core.NewTaskGraph` over +*every* task in the repository, including `computeSound` and state derivation — once per +planned write. Cost is Θ(writes × repository size), all of it inside `checkedWriteLock`. +Measured (dry run, so this is validation cost alone): + +| repo tasks | planned writes | `MutateTaskGraph` | +|---|---|---| +| 200 | 20 | 16.5 ms | +| 200 | 100 | 36.6 ms | +| 500 | 100 | 83.3 ms | +| 1000 | 100 | 162.6 ms | +| 1000 | 300 | 442.1 ms | + +Linear in each factor, as expected. Today's planning repository is 279 tasks, so a +single `task depend add` is imperceptible. The consumer to watch is +`bulk-link-existing-tasks-into-threads-with-resumable-apply`: a 1000-write manifest over +a 2000-task space extrapolates to several seconds of lock-held CPU, which also widens +M1's clobber window proportionally. + +**Recommendation:** don't optimize yet — but before bulk-linking, validate prefixes +incrementally (maintain one mutable adjacency map and re-check only reachability from +the changed node), or batch prefix checks per connected component. Corroborates sibling +`6g45qfv27vrm` M2; the measured table and the M1 interaction are additional. + +--- + +**Resolution:** The bulk-link task now gates release on representative +end-to-end guarded-path benchmarks, a lock-latency budget, and incremental +prefix validation when measured O(W × (V+E)) cost is material; ADR-0006 records +the same sequencing decision. + +## Assumptions that survived review + +Each was attacked with an executed counterexample attempt, a mutation, or a subprocess +test — not read off the implementation notes. + +1. **Cross-process serialization of `MutateTaskGraph` is real.** Two genuine child + processes racing opposite edges with a 150 ms in-guard window: one applied, one was + rejected with an attributable cycle diagnosis, final graph healthy with exactly one + edge. This is the single most important claim in the slice and it holds. (It is also + untested in the committed suite — see H4.) +2. **Cycle prevention for concurrent opposite edges works, in-process and across + processes.** The loser re-reads inside the guard, sees the winner's edge, and its + plan is refused. +3. **Canonical-root identity is correct on Unix.** `repositoryLockKey` maps a symlinked + alias and a relative spelling to the same key, and a lock held on the real path + blocks an `FS` opened on the alias. Independently, `flock` on the root inode makes + Unix alias-safe regardless of the key. +4. **Panic and termination paths release everything.** A planner panic propagates while + the deferred unlock releases both the platform lock and the process mutex and clears + the sentinel; subsequent writes succeed. A killed process's flock is reclaimed. + Verified by the suite and by follow-on operations in my probes. +5. **An injected release failure is attributed without leaving the guard held** — the + error names "release repository graph mutation guard" and the next mutation succeeds. +6. **Persistence tokens do not leak.** `SourceVersion` is `yaml:"-"`, cleared by + `TaskGraph.Task()`, absent from `wire.TaskJSON`, and absent from the JSON Schema — + no golden file or `schema_version` change was needed, which I confirmed by the empty + golden diff. The mutation battery shows a committed test kills its removal. +7. **The whole-snapshot CAS genuinely catches a raw edit outside the write set**, and + the mutation battery confirms a test kills `SameSourceSnapshot`. Within + `MutateTaskGraph` its count-based tail is unreachable (L2 explains why). +8. **Prefix validation genuinely prevents a cyclic durable prefix.** Replacing it with a + final-graph-only check fails a committed test. +9. **Interruption, rerun, and idempotence work.** An empty plan is a clean no-op; a + re-applied plan produces byte-identical files and an empty applied set; a plan + interrupted after one durable write leaves a healthy graph and converges on rerun. +10. **Fail-closed on unhealthy graphs is consistent.** A broken snapshot is refused + before the planner runs; a single unrelated legacy `blocked_by` anywhere in the + repository refuses every ordinary dependency mutation with an actionable message + ("1 legacy dependency field occurrence(s) remain; run the guarded migration"). +11. **Dependency direction is clean.** `store` imports `core`; `core` never imports + `store`. `TaskGraphMutationStore` is deliberately a sibling capability, not part of + `Store`, so read-only and test adapters are unaffected. (The *placement* of + validation logic is M4; the direction itself is right.) +12. **Rejecting rather than no-op'ing on unsupported platforms is the right call**, and + `internal/store` does compile for `GOOS=js` and `GOOS=plan9`, so the branch is + reachable in principle even though the full module does not build there. +13. **No data races or flakes.** `go test -race ./...` clean; the concurrency tests + clean at `-race -count=20`. + +--- + +## Traceability table + +| Code | Severity | Classification | Falsifies | Destination | +|---|---|---|---|---| +| H1 | High | **Pre-merge blocker** | AC3; `docs/ARCHITECTURE.md` re-entry sentence | Reopen `6g3q4rt0wzkq` — move the sentinel onto the root-keyed guard | +| H2 | High | **Pre-merge blocker** | AC3, AC5 | Reopen `6g3q4rt0wzkq` — identify the planner goroutine; others wait | +| H3 | High | **Pre-merge blocker** | AC1 ("tested") | Reopen `6g3q4rt0wzkq` — mutex-killing test + `repositoryLockKey` table test | +| H4 | High | **Pre-merge blocker** | AC1, AC4 | Reopen `6g3q4rt0wzkq` — re-point subprocess tests at `writeLock`; drop `platformWriteLock` | +| M1 | Medium | Follow-up | — | New task: interleave verify/write in the apply loop | +| M2 | Medium | Follow-up | — | New task: removals-before-additions apply order | +| M3 | Medium | Follow-up | AC1 | New task: repo-local lock file **or** declare Windows unsupported; add a Windows CI job | +| M4 | Medium | Follow-up | AC2 | Fold into `6g3q4rt7mgjn` — extract `core.ValidateGraphPlan` | +| M5 | Medium | Follow-up | AC6 | Reopen `6g3q4rt0wzkq` — lint consumes `LoadTaskGraph`; delete `ReadTaskGraph` | +| M6 | Medium | Follow-up | — | Reopen `6g3q4rt0wzkq` — store stamps `updated_at` from an injected clock | +| L1 | Low | Follow-up | AC3 | New task: guard `FixFrontmatter`/`DanglingLinks`; port-surface invariant test | +| L2 | Low | Follow-up | — | ADR-0006 / doc amendment on `SameSourceSnapshot`'s exact contract | +| L3 | Low | Follow-up | AC5 | Reopen `6g3q4rt0wzkq` — `MkdirAll` the root before locking | +| L4 | Low | Follow-up | AC4 | New task: classify lock errors against the domain sentinels | +| L5 | Low | Follow-up | — | Doc-only: `--dry-run` is exclusive and not CAS-validated | +| L6 | Low | **Monitor** | — | Note beside `lock.go:19-22` for the future long-lived adapter | +| L7 | Low | Follow-up | — | Fold into H3's test work | +| N1 | Medium | **Monitor** | — | Revisit before `bulk-link-existing-tasks-into-threads-with-resumable-apply` | + +Acceptance criteria that should be unticked pending amendment: **1** (H3, H4, M3), +**2** (M4), **3** (H1, L1), **5** (H2, L3), **6** (M5). + +--- + +## Validation commands and results + +Run from `/Users/andyeschbacher/git/andy-esch/taskflow-graph-mutation-guard`, branch +`feat/portable-graph-mutation-guard`, uncommitted tree, merge-base `509a7c3`. Probe +files lived outside the repository and were supplied with `go test -overlay=`; +`git status --porcelain` was identical before and after apart from this audit. + +| Command | Result | +|---|---| +| `go build ./...` | pass | +| `go test ./...` | pass, 23 packages | +| `go test -race ./...` | **pass, exit 0**, no race reports | +| `go test -race -count=20 -run 'TestMutateTaskGraphConcurrentOppositeEdges\|TestRepositoryLock\|TestMutateTaskGraphRejectsPlannerStoreCalls' ./internal/store/` | pass, 6.3 s, no flakes | +| `just lint` (`golangci-lint run ./...`) | **0 issues** | +| `go vet ./...` | pass | +| `GOOS=windows GOARCH=amd64 go build ./...` / `go vet ./...` | pass (compile-only) | +| `GOOS=windows go test ./internal/store/` | cannot run: `exec format error` — no Windows runtime coverage exists (M3) | +| `GOOS=js GOARCH=wasm go build ./internal/store/` | pass (`lock_other.go` branch compiles); full module does not build on js/plan9 for unrelated TUI deps | +| `go mod tidy` | no change (`golang.org/x/sys` correctly promoted to a direct requirement) | +| `./bin/tskflwctl lint` | exit **0**, 6 advisory legacy findings | +| Mutation: `processRepositoryLock` → no-op | **suite still PASSES** — H3 | +| Mutation: `repositoryLockKey` → no `EvalSymlinks` | **suite still PASSES** — H3 | +| Mutation: `rejectGraphPlannerCall` → nil | suite FAILS (caught) | +| Mutation: `SameSourceSnapshot` → true | suite FAILS (caught) | +| Mutation: `Task()` keeps `SourceVersion` | suite FAILS (caught) | +| Mutation: prefix check → final graph only | suite FAILS (caught) | +| Probe: planner writes via a second `*FS` | **hangs > 3 s, no error** — H1 | +| Probe: planner reads via a second `*FS` | returns **live** state ≠ snapshot — H1 | +| Probe: unrelated goroutine `GetTask`/`SetFields` during a mutation | `ErrConflict` "planner cannot call Store methods" — H2 | +| Probe: two real child processes, opposite edges | 1 applied / 1 rejected, final healthy, 1 edge — survived claim 1 | +| Probe: raw edit between verify and write, 2-file plan | **silently clobbered, mutation reported success** — M1 | +| Probe: edge reversal, both id orientations | smaller-id dependent accepted; larger-id rejected — M2 | +| Probe: dependency write with no `UpdatedAt` | `updated_at` left at `2026-01-01` — M6 | +| Probe: `FixFrontmatter(true)` / `DanglingLinks()` inside a planner | both succeed, no re-entry rejection — L1 | +| Probe: `CreateTask` into a missing root | `open repo root for write lock: … no such file or directory` — L3 | +| Probe: `SameSourceSnapshot` on differing unreadable sets | `true` — L2 | +| Probe: symlinked alias root, relative vs absolute root | one key, alias blocks behind the real path — survived claim 3 | +| Probe: prefix-validation scaling (200–1000 tasks × 20–300 writes) | 16.5 ms → 442 ms, linear in each factor — N1 | +| Probe: empty plan / repeated plan / one legacy field present | clean no-op / byte-identical / refused with actionable message — survived claims 9, 10 | + +--- + +## Candidate tasks + +- ⏳ Reopen `6g3q4rt0wzkq` — pre-merge set: H1 (root-keyed re-entry sentinel), H2 (planner-goroutine identity so unrelated callers wait), H3 (tests that die when the mutex or the key normalization dies), H4 (test the product lock path across processes; delete `platformWriteLock`). +- ⏳ Reopen `6g3q4rt0wzkq` — small corrections: M5 (lint consumes `LoadTaskGraph`; delete `ReadTaskGraph`), M6 (store stamps `updated_at`), L3 (`MkdirAll` the root before locking), L7 (replace wall-clock success branches). +- ⏳ `tskflwctl task new "Interleave graph-mutation CAS with the apply loop" --epic 30-threads-and-task-dependency-graphs --tags graph,storage,concurrency` — M1, with a `testHookAfterGraphWrite` regression test. +- ⏳ `tskflwctl task new "Apply graph plans removals-first so edge reversal is order-independent" --epic 30-threads-and-task-dependency-graphs --tags graph,storage` — M2, plus a test that runs both id orientations. +- ⏳ `tskflwctl task new "Settle the Windows repository lock identity" --epic 30-threads-and-task-dependency-graphs --tags storage,concurrency,portability` — M3: repo-local lock file or an explicit unsupported declaration, and a Windows CI job. +- ⏳ `tskflwctl task new "Extract graph plan validation into a pure core validator" --epic 30-threads-and-task-dependency-graphs --tags graph,architecture` — M4; prerequisite for `6g3q4rt7mgjn` and bulk-linking. +- ⏳ `tskflwctl task new "Guard every Store port method against planner re-entry" --epic 30-threads-and-task-dependency-graphs --tags storage,concurrency` — L1, with a port-surface invariant test. +- ⏳ `tskflwctl task new "Classify repository lock errors against the domain sentinels" --epic 30-threads-and-task-dependency-graphs --tags storage,errors` — L4. +- ⏳ ADR-0006 amendment — L2 (`SameSourceSnapshot`'s exact contract and its broken-health precondition) and L5 (`--dry-run` is exclusive and not CAS-validated). +- ⚠️ N1 and L6 — monitor only; revisit N1 before `bulk-link-existing-tasks-into-threads-with-resumable-apply` lands. diff --git a/planning/epics/30-threads-and-task-dependency-graphs.md b/planning/epics/30-threads-and-task-dependency-graphs.md index 7a4946d6..3947102a 100644 --- a/planning/epics/30-threads-and-task-dependency-graphs.md +++ b/planning/epics/30-threads-and-task-dependency-graphs.md @@ -30,16 +30,21 @@ guard and dependency-operation tasks: ```text 6g3q4rst78qy strict reads -----> 6g3q4rt7mgjn dependency operations <----- 6g3q4rt0wzkq portable guard - | | | - | v | - | 6g3q4rte8kc1 eligibility | - | | - +--------------------> 6g3q4rtmv4ak Thread entity <------------------------+ - | | -dependency operations ----------------+ +----> 6g3q4rv1w9e2 generated views - v | - 6g3q4rtv8d0a bulk link v - 6g3q4rv89vzw TUI + | + v + 6g3q4rte8kc1 eligibility + | + v + 6g3q4rtmv4ak Thread entity + | + v + 6g3q4rtv8d0a bulk link + | + v + 6g3q4rv1w9e2 generated views + | + v + 6g3q4rv89vzw TUI ``` - [6g3q4rst78qy — strict dependency reads](../tasks/6g3q4rst78qy-establish-canonical-task-dependencies-and-strict-graph-reads.md) @@ -54,13 +59,14 @@ dependency operations ----------------+ +----> 6g3q4rv1w9e2 generate ## Delivery sequence and gates ```text -strict read model -> guarded edge writes -> eligibility enforcement - \-> Thread entity -> bulk linking -> generated views -> TUI +strict read model -> guarded edge writes -> eligibility enforcement -> Thread entity + -> bulk linking -> generated views -> TUI ``` -Eligibility enforcement and the Thread entity share the same graph foundation. They can be scoped -separately after guarded writes stabilize, but bulk linking waits for both dependency mutation and -Thread persistence. +Eligibility enforcement and the Thread entity share the same graph foundation, but implementation +is deliberately serialized after guarded writes stabilize. Eligibility establishes the first +non-dependency guarded mutation seam; Thread persistence reuses it for another entity kind; bulk +linking then composes both materializers under one outer guard. | Order | Slice | Exit gate | Highest-value stress tests | |---|---|---|---| @@ -109,6 +115,44 @@ checkpoints. Dogfooding begins when the corresponding production slice passes it - Autonomous multi-agent or worktree orchestration. - TUI implementation before the domain, CLI, and wire projections are proven. +## Sequencing amendment — guarded multi-kind writes (2026-08-27) + +The portable-guard audits proved the dependency boundary but also made the next extension point +explicit: `TaskGraphMutationStore` deliberately materializes dependency writes only, while lifecycle, +Thread, and bulk operations each need an authoritative read/validate/write decision under the same +canonical-root exclusion contract. This amendment supersedes the earlier suggestion that eligibility +and Thread persistence may be implemented independently after dependency operations. + +```text +dependency operations + | + v +eligibility lifecycle boundary (first non-dependency guarded write) + | + v +Thread mutation boundary (first additional entity kind) + | + v +compound bulk apply -> generated views -> TUI +``` + +This is implementation coordination, not a new domain dependency: the pure eligibility and Thread +projections remain independently testable. Implementation is serialized so each slice reuses one +reviewed guard-extension pattern rather than inventing incompatible callbacks or nesting guarded +operations, which root-wide callback exclusion correctly rejects. + +Keep the public capabilities use-case-specific and share private store mechanics: + +- dependency commands use the existing guarded task-dependency capability; +- lifecycle enforcement adds a narrow guarded status-transition capability; +- Thread lifecycle/membership adds a narrow guarded Thread capability plus lock-free internal + materialization; +- bulk apply owns one deliberate compound capability that takes the guard once and composes the + internal task and Thread materializers. It never orchestrates by nesting the narrower ports. + +Generated views remain unchanged and read-only. The TUI remains last and must retry/debounce the +documented transient `ErrConflict` when a watcher refresh overlaps the planner-exclusive phase. + ## Related - [ADR-0006](../adrs/0006-adopt-threads-as-task-dags.md) diff --git a/planning/tasks/6g3q4rt0wzkq-make-repository-graph-mutations-portable-and-serializable.md b/planning/tasks/6g3q4rt0wzkq-make-repository-graph-mutations-portable-and-serializable.md index 3ed6a570..8e9beca5 100644 --- a/planning/tasks/6g3q4rt0wzkq-make-repository-graph-mutations-portable-and-serializable.md +++ b/planning/tasks/6g3q4rt0wzkq-make-repository-graph-mutations-portable-and-serializable.md @@ -1,7 +1,7 @@ --- schema: 1 id: 6g3q4rt0wzkq -status: next-up +status: completed epic: 30-threads-and-task-dependency-graphs description: Provide a cross-platform store-owned guard for authoritative repository graph read, validation, and write operations. effort: 2-4 days @@ -10,7 +10,9 @@ priority: high autonomy_level: 2 tags: [threads, graph, storage, concurrency] created: "2026-08-25" -updated_at: "2026-08-26" +updated_at: "2026-08-27" +started_at: "2026-08-27" +completed_at: "2026-08-27" --- # Make repository graph mutations portable and serializable @@ -27,14 +29,12 @@ Provide one store-owned repository mutation boundary that makes final graph read ## Acceptance criteria -- [ ] Every supported platform has an explicit, tested cooperating-writer serialization contract; unsupported guarantees are rejected or documented rather than silently no-op. -- [ ] A graph mutation performs its authoritative scan, pure planning/validation, and writes within one repository guard without exposing filesystem locking or teaching the store graph semantics. -- [ ] The callback contract accepts and returns taskflow-owned snapshot/planned-write values, permits no nested Store calls, and detects invalid nesting without hanging. -- [ ] Lock acquisition/release errors are attributable and process termination does not leave unrecoverable stale state. -- [ ] Existing optimistic concurrency and ordinary write behavior remain compatible. -- [ ] The store boundary consumes one canonical strict-snapshot loader; remove - or fold any duplicate or otherwise unused ReadTaskGraph scan seam so lint and - mutation cannot drift. +- [x] Every released platform has an explicit, tested cooperating-writer serialization contract; other platforms reject graph mutation rather than silently no-op. +- [x] A graph mutation performs its authoritative scan, pure core planning/validation, and writes within one repository guard without exposing filesystem locking or teaching the store graph semantics. +- [x] The callback contract accepts and returns taskflow-owned snapshot/planned-write values, permits no Store calls at canonical-root scope, and detects same- or second-Store nesting without hanging. +- [x] Lock acquisition/release errors are attributable and process termination does not leave unrecoverable stale state. +- [x] Existing optimistic concurrency and ordinary write behavior remain compatible outside the documented callback-exclusive contention window. +- [x] The store boundary consumes one canonical strict-snapshot loader and the unused `ReadTaskGraph` service seam is removed; lint and mutation share the same strict projection constructor and task parser. ## Stress tests @@ -46,3 +46,21 @@ Provide one store-owned repository mutation boundary that makes final graph read ## Sequencing The control-inversion contract is fixed by ADR-0006's 2026-08-26 amendment, so implementation can proceed alongside the strict read foundation. Guarded dependency writes require both tasks to land. + +## Implementation notes (2026-08-27) + +The production boundary uses `core.LoadTaskGraph` for guarded writes while lint builds the same strict projection from its body-bearing scan. `TaskGraphMutationStore` takes the repository guard, loads that immutable snapshot, calls a pure planner over taskflow-owned values, delegates source/plan/prefix/final validation to core, applies surgical dependency updates through internal lock-free atomic writes, and returns any durable applied prefix after failure. Planner-provided write order is preserved because it is recovery data; semantic dependency sets are still canonicalized. Whole-snapshot byte versions plus immediate per-target CAS catch raw edits without exposing persistence tokens to planners. Graph writes stamp `updated_at` from the injected clock only for a semantic change. + +The repository guard combines a canonical-root in-process mutex with root-directory `flock` on the supported macOS/Linux release targets. Windows and other non-Unix builds reject mutation explicitly until they have a native-tested cross-user lock contract. Callback exclusion is root-global across independent `FS` values; Store access during the callback fails fast with an attributable conflict. Entity creation participates in the guard and retains its prior missing-root behavior. + +## Validation (2026-08-27) + +Adversarial coverage includes concurrent opposite edges, independent same-process stores, nested reads/writes/mutations, degraded legacy migration, broken snapshots, raw edits outside the planned write set, unsafe interruption prefixes, partial-write convergence, planner panic, injected apply/release failure, and child-process termination. `go test -race ./...`, GolangCI-Lint, formatting, module-tidiness, `git diff --check`, and planning lint pass. Store tests compile for Linux, Windows, and the explicit-unsupported WebAssembly target. + +## Adversarial hardening (2026-08-27) + +The Gemini and Claude audits are closed after dispositioning all 23 findings. The guard now scopes callback exclusion by canonical planning root, including second-`FS` access; planner order is preserved as semantic recovery data; pure source/plan/prefix validation lives in core; whole-snapshot CAS is followed by immediate per-target CAS; graph changes receive a caller-clock `updated_at`; exact problem/legacy identities participate in source comparison; missing-root creation compatibility is restored; and non-Unix builds fail closed rather than claiming an untested Windows cache-lock guarantee. + +Production-path regressions cover the keyed process mutex, root aliases, same- and second-store re-entry, concurrent callback contention, process termination, real cross-process opposite-edge mutation, safe and unsafe edge-reversal order, raw edits after a durable prefix, exact unreadable-set comparison, no-op timestamp stability, and the previously unguarded lint helpers. ADR-0006 and ARCHITECTURE record the root-wide contention, dry-run, raw-editor, platform, and recovery-order contracts. Prefix-validation scale is tracked by `6g3q4rtv8d0a` as a release gate for bulk apply. + +Validation after the amendments: `go test -race ./...` passes; GolangCI-Lint reports 0 issues; formatting, `go mod tidy -diff`, and `git diff --check` are clean; and store tests cross-compile for Linux, Windows (explicit unsupported runtime path), and js/wasm. diff --git a/planning/tasks/6g3q4rt7mgjn-ship-guarded-dependency-mutations-and-graph-queries.md b/planning/tasks/6g3q4rt7mgjn-ship-guarded-dependency-mutations-and-graph-queries.md index bc8aa7d9..3313ff26 100644 --- a/planning/tasks/6g3q4rt7mgjn-ship-guarded-dependency-mutations-and-graph-queries.md +++ b/planning/tasks/6g3q4rt7mgjn-ship-guarded-dependency-mutations-and-graph-queries.md @@ -10,7 +10,7 @@ priority: high autonomy_level: 3 tags: [threads, graph, cli, storage] created: "2026-08-25" -updated_at: "2026-08-26" +updated_at: "2026-08-27" --- # Ship guarded dependency mutations and graph queries @@ -43,6 +43,15 @@ Expose safe repository-global dependency operations and deterministic read queri - [ ] Deep-chain stress establishes a supported graph-depth envelope; replace recursive sound derivation if measured repository shapes approach unsafe stack or latency bounds. +- [ ] Planner callbacks resolve task IDs and legacy references only from + supplied immutable graph values, emit deterministic prefix-safe write order, + and never call a Store method. +- [ ] Every semantic dependency change receives the Service clock through the + mutation port, stamps updated_at exactly once, and an idempotent skip does not + advance it. +- [ ] task edit keeps the human editor session outside the repository guard and + either rejects a dependency delta with direction or reapplies it through the + guarded dependency use case after fresh validation. ## Stress tests @@ -53,3 +62,5 @@ Expose safe repository-global dependency operations and deterministic read queri ## Sequencing Requires the strict read foundation and portable mutation guard. Its production commands persist the epic's bootstrap edges and become the first dependency dogfood surface for the remaining tasks. + +## Mutation-guard integration amendment (2026-08-27)\n\nThe production mutation callback is snapshot-only. Exact-ID lookup, duplicate/self/missing checks, legacy resolution, and write ordering must be derived from the supplied immutable TaskGraph and pure core validators; callback code cannot reach back into Store or Service. Use the named LoadTaskGraph source for query loading and do not restore the unused ReadTaskGraph service seam.\n\nThe Service passes its clock into MutateTaskGraph. A real semantic change is stamped by store materialization, while an already-satisfied add or remove stays byte-identical. The six-task legacy migration must emit one deterministic prefix-safe sequence even though stable-ID order happens to be safe for its projected-edge replacement shape.\n\nHuman editing remains outside the repository guard. After the editor returns, task edit compares the proposed dependency set and either rejects it with exact task depend guidance or invokes the same guarded dependency use case against a fresh snapshot; the editor process is never held inside callback exclusion. diff --git a/planning/tasks/6g3q4rte8kc1-enforce-dependency-eligibility-across-every-task-start-path.md b/planning/tasks/6g3q4rte8kc1-enforce-dependency-eligibility-across-every-task-start-path.md index dcaef219..f96f5955 100644 --- a/planning/tasks/6g3q4rte8kc1-enforce-dependency-eligibility-across-every-task-start-path.md +++ b/planning/tasks/6g3q4rte8kc1-enforce-dependency-eligibility-across-every-task-start-path.md @@ -4,12 +4,13 @@ id: 6g3q4rte8kc1 status: next-up epic: 30-threads-and-task-dependency-graphs description: Centralize task eligibility and explanatory force behavior across every transition into in-progress. -effort: 2-4 days +effort: 3-5 days tier: 1 priority: high autonomy_level: 2 tags: [threads, graph, lifecycle, cli] created: "2026-08-25" +updated_at: "2026-08-27" --- # Enforce dependency eligibility across every task start path @@ -32,6 +33,14 @@ Make dependency eligibility one authoritative core policy for all transitions in - [ ] Reopening an upstream task makes completed descendants unsound without rewriting their persisted statuses. - [ ] Deferred and deprecated prerequisites follow ADR-0006 semantics consistently. - [ ] Lifecycle receipts report descendant task IDs/counts whose gate state changed and, after Thread support lands, affected Thread IDs with an explanatory remedy. +- [ ] Eligibility authorization and the persisted lifecycle transition occur + under one repository guard over the same authoritative graph snapshot. +- [ ] Lifecycle writes use a dedicated use-case-specific guarded capability + sharing private store guard/materialization helpers; they neither nest + MutateTaskGraph nor expose a generic filesystem callback. +- [ ] Races with prerequisite lifecycle or dependency changes cannot commit a + start authorized by stale graph state; default and forced paths have + adversarial coverage. ## Stress tests @@ -42,3 +51,5 @@ Make dependency eligibility one authoritative core policy for all transitions in ## Sequencing Requires guarded dependency operations. Thread projections consume the derivation from the strict-read task; enforcement and Thread persistence may proceed independently once their own prerequisites land. + +## Atomic lifecycle boundary amendment (2026-08-27)\n\nEligibility is an authorization decision, so reading the graph and persisting the move into in-progress must be one guarded operation. A preflight graph query followed by an ordinary Move is insufficient: a prerequisite or dependency can change between those calls. Compute before/after derived state from snapshots owned by the same boundary so descendant-impact receipts describe the transition that actually committed.\n\nIntroduce a narrow lifecycle-mutation capability implemented over the store's private canonical-root guard and lock-free task materialization helpers. Do not broaden core into a filesystem callback and do not implement this by nesting MutateTaskGraph. This task is the first non-dependency extension of the guarded-write pattern and should settle that internal reuse seam before Thread persistence implements another entity kind.\n\nSequencing is therefore strict for implementation coordination: guarded dependency operations first, then this lifecycle boundary, then Thread persistence. The domain derivations remain independently testable; the ordering prevents two tasks from inventing incompatible guard extensions. diff --git a/planning/tasks/6g3q4rtmv4ak-add-the-thread-entity-lifecycle-and-graph-projections.md b/planning/tasks/6g3q4rtmv4ak-add-the-thread-entity-lifecycle-and-graph-projections.md index 06d03f17..f9386952 100644 --- a/planning/tasks/6g3q4rtmv4ak-add-the-thread-entity-lifecycle-and-graph-projections.md +++ b/planning/tasks/6g3q4rtmv4ak-add-the-thread-entity-lifecycle-and-graph-projections.md @@ -4,12 +4,13 @@ id: 6g3q4rtmv4ak status: next-up epic: 30-threads-and-task-dependency-graphs description: Introduce first-class Thread persistence, membership, lifecycle, rollup, frontier, external gates, CLI, and wire projections. -effort: 4-7 days +effort: 5-8 days tier: 1 priority: high autonomy_level: 3 tags: [threads, domain, storage, cli] created: "2026-08-25" +updated_at: "2026-08-27" --- # Add the Thread entity, lifecycle, and graph projections @@ -34,6 +35,13 @@ Introduce Threads as first-class initiative documents whose membership and lifec - [ ] Initialization creates `threads/`, stops creating `projects/`, and handles non-empty legacy Projects safely. - [ ] Task/Thread IDs are checked for cross-kind collisions, and an empty Projects scaffold is defined narrowly enough to permit only `.gitkeep` removal. - [ ] Task lifecycle receipts name affected Thread IDs once Thread documents can exist. +- [ ] Thread create, membership, start, complete, abandon, reopen, and + cross-kind ID collision checks perform authoritative read, validation, and + write under one repository guard. +- [ ] The store provides lock-free internal Thread materialization reusable by a + later compound bulk capability without nesting guarded mutations. +- [ ] Concurrent Thread membership or lifecycle operations and task-graph + changes either serialize to a valid state or return an attributable conflict. ## Stress tests @@ -43,3 +51,5 @@ Introduce Threads as first-class initiative documents whose membership and lifec ## Sequencing Requires the strict graph derivation and portable guarded writes, but not lifecycle enforcement. Bulk linking waits for this persistence contract and dependency operations. + +## Guarded Thread mutation amendment (2026-08-27)\n\nDiagnostic Thread projections may degrade explicitly, but every authoritative Thread mutation loads the required task graph and current Thread state inside one canonical-root repository guard. Creation and membership validate task existence and cross-kind identity there; start and complete validate membership, external gates, and sound completion there; the matching Thread write lands before release.\n\nExpose use-case-specific Thread mutation ports backed by private store guard/materialization helpers. Keep a lock-free internal Thread document materializer so bulk apply can compose task dependency writes and the final Thread write under one outer guard. Calling MutateTaskGraph from a Thread planner, or calling a guarded Thread method from a graph planner, is forbidden and will correctly fail callback exclusion.\n\nImplement after the eligibility task establishes the first non-dependency guarded-write pattern. Bulk linking waits for this Thread persistence/materialization contract as well as dependency operations. diff --git a/planning/tasks/6g3q4rtv8d0a-bulk-link-existing-tasks-into-threads-with-resumable-apply.md b/planning/tasks/6g3q4rtv8d0a-bulk-link-existing-tasks-into-threads-with-resumable-apply.md index 9949def3..ccd089e4 100644 --- a/planning/tasks/6g3q4rtv8d0a-bulk-link-existing-tasks-into-threads-with-resumable-apply.md +++ b/planning/tasks/6g3q4rtv8d0a-bulk-link-existing-tasks-into-threads-with-resumable-apply.md @@ -4,12 +4,13 @@ id: 6g3q4rtv8d0a status: next-up epic: 30-threads-and-task-dependency-graphs description: Compose literal-YAML membership and dependency graphs for existing tasks into planning-space-bound resumable apply plans. -effort: 3-5 days +effort: 4-7 days tier: 2 priority: high autonomy_level: 3 tags: [threads, graph, cli, workflow] created: "2026-08-25" +updated_at: "2026-08-27" --- # Bulk-link existing tasks into Threads with resumable apply @@ -33,6 +34,12 @@ Let users describe membership and dependency relationships among existing tasks - [ ] Omitted membership or dependencies never imply destructive removal. - [ ] Human and machine receipts distinguish creates, updates, skips, conflicts, and completion. - [ ] Existing task edits between retries do not conflict merely because the plan links those tasks; V1 owns only additive membership and dependency intent. +- [ ] One compound mutation capability applies dependency and Thread writes + under a single repository guard without nesting the narrower mutation ports. +- [ ] Task dependency additions precede the Thread document, every operation + prefix remains sound, and receipts report the exact durable operation prefix. +- [ ] Compound semantic writes use the caller-provided clock, while idempotent + skips neither rewrite files nor advance timestamps. ## Stress tests @@ -42,3 +49,9 @@ Let users describe membership and dependency relationships among existing tasks ## Sequencing Requires production dependency mutations and Thread persistence. Use the first released version to bulk-link the next naturally suitable initiative. + +## Mutation-guard performance gate (2026-08-27) + +Before releasing bulk apply, benchmark the real guarded path at representative planning-space and manifest sizes. The current pure prefix validator rebuilds the full graph for every task-file write (O(W × (V+E))) while holding the exclusive repository guard; the adversarial audit measured roughly 442 ms for 1,000 tasks × 300 writes. Keep the simple validator for direct dependency operations, but require an explicit latency budget and move to incremental prefix validation if bulk-scale lock time is material. Include contention and raw-editor-CAS-window observations in the benchmark. + +## Compound mutation amendment (2026-08-27)\n\nApply is one dedicated compound capability, not orchestration across task depend and Thread commands. It takes the canonical-root guard once, reloads planning-space identity, the strict task graph, and relevant Thread state, validates the materialized intent, then invokes lock-free internal materializers. Nesting narrower guarded ports would fail callback exclusion and would not provide one authoritative plan.\n\nFor existing-task V1, dependency additions land in the plan's deterministic prefix-safe order and the Thread document lands last. A failure or raw-edit conflict returns the exact durable operation prefix; retry rebuilds current intent and converges without treating unrelated edits between invocations as stale frozen-plan versions. All real semantic writes use the caller clock and idempotent skips remain byte-identical.\n\nThe existing performance gate remains an exit criterion for this compound path, including lock-held validation time, callback-contention behavior, and the immediate per-target CAS window. diff --git a/planning/tasks/6g3q4rv89vzw-add-usage-informed-thread-views-to-the-tui.md b/planning/tasks/6g3q4rv89vzw-add-usage-informed-thread-views-to-the-tui.md index 99c0e0cf..3f9a443f 100644 --- a/planning/tasks/6g3q4rv89vzw-add-usage-informed-thread-views-to-the-tui.md +++ b/planning/tasks/6g3q4rv89vzw-add-usage-informed-thread-views-to-the-tui.md @@ -10,6 +10,7 @@ priority: medium autonomy_level: 3 tags: [threads, tui, graph, ux] created: "2026-08-25" +updated_at: "2026-08-27" --- # Add usage-informed Thread views to the TUI @@ -29,6 +30,9 @@ Expose production-proven Thread lifecycle, rollup, frontier, member/external rol - [ ] Watcher reload handles Thread and task dependency changes coherently. - [ ] Narrow terminals degrade readably and navigation/completion remain consistent with other first-class entities. - [ ] No TUI-local readiness, graph traversal, or direct filesystem mutation logic is introduced. +- [ ] Planner-phase ErrConflict from canonical-root callback exclusion is + treated as transient watcher contention and retried or debounced without + rendering an empty or permanently broken view. ## Stress tests @@ -37,3 +41,5 @@ Expose production-proven Thread lifecycle, rollup, frontier, member/external rol ## Sequencing Last planned V1 slice, after real CLI usage of Thread projections and generated views. + +## Mutation-contention amendment (2026-08-27)\n\nThe graph-mutation planner phase excludes every Store call at canonical-root scope. A watcher refresh that lands in that brief window can therefore receive ErrConflict even though no repository defect exists. Treat that result as transient contention: retain the last coherent model, debounce or retry after the mutation events settle, and never replace the view with empty data or a permanent broken-state banner.\n\nA multi-file mutation may also emit several watcher events for graph-valid durable prefixes. Coalesce them and reload the shared core projection after the burst rather than deriving intermediate graph state in the TUI.