diff --git a/VERSION b/VERSION index ac2f82c..aef7deb 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.5.28 +1.5.29 diff --git a/assets/skills/continue/SKILL.md b/assets/skills/continue/SKILL.md index d52b6c9..6209989 100644 --- a/assets/skills/continue/SKILL.md +++ b/assets/skills/continue/SKILL.md @@ -2,8 +2,8 @@ name: continue description: | devbrain resume cursor (Stage C — Assemble) that also works the queue. First - folds new prompt-log entries into the project's brain pages AND extracts open - items into the TODO queue, pulls the brain, refreshes the live world + folds new prompt-log entries into the project's brain pages AND extracts committed + open items into the TODO queue (per /distill's gates), pulls the brain, refreshes the live world (git/issues/CI), and gives a short briefing. Then it picks up the highest-priority task, queries gbrain to synthesize that task's context and attaches it to the TODO (shown to you), builds a MINIMAL MVP for it, opens a PR for review, and asks @@ -115,6 +115,8 @@ A few lines, then move straight into the work: - **From the brain:** the 2-4 most relevant in-scope facts/decisions/open items (with page slug pointers, e.g. `/`). - **From the world:** uncommitted changes, ahead/behind, open issues/PRs, CI. +- **Open questions:** decisions the fold-in parked on a brain page because only you can + make them (`/distill` never queues these) — one line each, so they get answered. - **Top of the queue:** the highest-priority task you're about to pick up. Briefing plus pointers — do not dump whole pages. The flusher pushes pages/tasks you diff --git a/assets/skills/distill/SKILL.md b/assets/skills/distill/SKILL.md index 6f551f4..e94936e 100644 --- a/assets/skills/distill/SKILL.md +++ b/assets/skills/distill/SKILL.md @@ -4,8 +4,9 @@ description: | devbrain curation step (Stage B — Brain) — the explicit "save this now" path. This is the design's "/checkpoint" role, named /distill to avoid Claude Code's native /checkpoint rewind alias. Reads new raw prompt-log entries for the current - project, distills them into brain pages, and extracts actionable open items into - the project's TODO queue (the queue's only source). Writes directly (no approval + project, distills them into brain pages, and extracts explicitly committed, untracked + open items into the project's TODO queue (the queue's only source; musings, tracker + issues, dated readouts, and user-only decisions stay on the brain page). Writes directly (no approval gate — review by git diff). /continue runs this same fold-in automatically on an explicit /continue invocation. Never invoke proactively. An explicit /continue or /distill invocation is consent; run immediately without reconfirming. Do not @@ -162,16 +163,44 @@ Step 2 listed by `cksum` — skip the rest; unchanged memory is already folded i dedupe against existing pages, and fold genuinely-new facts into the relevant topic page (or an `operational-memory-recovered.md` page). Skip `MEMORY.md` (just an index). -**Queue tasks.** The brain records *what happened*; the queue records *what's next*. As -you read the new log, also pull out **actionable open items** — anything phrased as work -still to do: "still open", "TODO", "we should…", "next step", a bug noted but not fixed, a -follow-up the user asked for and you haven't done. Turn each into a queue task. This is the +**Queue tasks.** The brain records *what happened*; the queue records *what's next* — +and only the part of what's next that someone actually **committed to**. This is the queue's **only source** — tasks are born here (and `/continue` runs this same fold-in, so -it refreshes the queue on resume). +it refreshes the queue on resume) — so every task admitted is a worker turn spent. Read +loosely ("still open", "we should…", "next step"), this rule minted 321 tasks in one +project in six weeks; a triage found 17 of the 72 still open were real work — the rest +already done, stale, duplicates of tracker issues, dated readouts, or decisions only the +user could make. So the default is **no task**: an item is queued only when it passes +**every** gate below, and anything that fails one goes into the brain page (as a note, +a pointer, or an open question), never the queue. ```bash -devbrain todo list # see what's already queued — DEDUPE against this before adding +devbrain todo list all # every task, any status — DEDUPE against this before adding +devbrain todo list # open tasks — count them for the soft cap below ``` -For each genuinely new open item: +1. **Explicit commitment.** The user asked for it, or the session decided it *and* + named an owner and a done-check. Musings — "we should…", "could later", "worth + looking at", a "next step" thought aloud — are notes on the brain page. +2. **Not already tracked.** Same intent anywhere in `todo list all` (done included) → + skip. Lives in the project's external tracker (Linear, GitHub issues) → a pointer + on the brain page, not a task; the tracker owns it. +3. **Not a dated readout or reminder.** "Check the experiment on the 24th", + "re-measure in two weeks": append one row (date · what to check · where the numbers + live) to the project's single `Experiment readout calendar` task — `todo show` then + `todo edit -b` — creating it once (`-p 20`) if absent. Never one task per date. +4. **Not owned by a runner.** If an automated job already tracks the follow-through + (a scheduled service, an outreach runner's thread states, a cron), it is not + queue work. +5. **Not a decision only the user can make.** Record it as an open question on the + brain page; `/continue` surfaces it in the briefing. A task a worker can't finish + without the user is not a task. + +**Soft cap: 20 open tasks.** Count `devbrain todo list` first. Past 20 the fold-in may +only **merge** a new item into an existing task (`todo edit -b`) or **drop** it to +the brain page — never add — and the report must say so ("queue at 23/20: merged 2, +dropped 3, added 0"). `todo add` also warns on stderr past the cap; treat the warning +as a failed gate, not noise. + +For each item that clears every gate: ```bash devbrain todo add "" -p <0-100> -b "" ``` @@ -181,10 +210,8 @@ devbrain todo add "" -p <0-100> -b " mx { - mx = n - } - } - } slugSrc := title if slugSrc == "" { slugSrc = "task" @@ -288,7 +275,10 @@ func (q *Queue) Create(project, title string, priority int, body string) (*task. if slug == "" { slug = "task" } - tid := fmt.Sprintf("%04d-%s", mx+1, slug) + tid, err := task.AllocID(d, slug) + if err != nil { + return nil, err + } path := filepath.Join(d, tid+".md") prio := priority if prio < 0 { diff --git a/internal/install/install.go b/internal/install/install.go index 57acfab..585f992 100644 --- a/internal/install/install.go +++ b/internal/install/install.go @@ -22,6 +22,7 @@ import ( "github.com/TheWeiHu/devbrain/internal/config" "github.com/TheWeiHu/devbrain/internal/dashboard" "github.com/TheWeiHu/devbrain/internal/jsonedit" + "github.com/TheWeiHu/devbrain/internal/task" "github.com/TheWeiHu/devbrain/internal/version" ) @@ -518,14 +519,14 @@ func (c *ctx) tccGuard(o *options) int { func (c *ctx) ensureDataRepo() error { if exists(filepath.Join(c.data, ".git")) { fmt.Fprintf(c.stdout, " data repo : exists (%s)\n", c.data) - return nil + return c.ignoreTaskLock() } if remote := os.Getenv("DEVBRAIN_DATA_REMOTE"); remote != "" { if err := run("git", "clone", remote, c.data); err != nil { return fmt.Errorf("clone %s failed: %v", remote, err) } fmt.Fprintf(c.stdout, " data repo : cloned %s -> %s\n", remote, c.data) - return nil + return c.ignoreTaskLock() } if err := os.MkdirAll(filepath.Join(c.data, "projects"), 0o755); err != nil { return err @@ -533,7 +534,7 @@ func (c *ctx) ensureDataRepo() error { if err := run("git", "-C", c.data, "init", "-q"); err != nil { return fmt.Errorf("git init %s failed: %v", c.data, err) } - _ = os.WriteFile(filepath.Join(c.data, ".gitignore"), []byte("*.pglite\n.DS_Store\n"), 0o644) + _ = os.WriteFile(filepath.Join(c.data, ".gitignore"), []byte("*.pglite\n.DS_Store\n"+task.LockName+"\n"), 0o644) // Append-only sidecars union-merge instead of conflicting (flush reseeds // this on existing repos too). _ = os.WriteFile(filepath.Join(c.data, ".gitattributes"), @@ -558,6 +559,30 @@ func (c *ctx) ensureDataRepo() error { // offerGbrain: the optional ranked/semantic search engine. Offered only in a // real terminal; non-interactive runs and bun-less machines skip silently // (offline `devbrain brain search/get` works with zero engine). +func (c *ctx) ignoreTaskLock() error { + name := filepath.Join(c.data, ".gitignore") + b, err := os.ReadFile(name) + if err != nil && !os.IsNotExist(err) { + return err + } + for _, line := range strings.Split(string(b), "\n") { + if strings.TrimSpace(line) == task.LockName { + return nil + } + } + f, err := os.OpenFile(name, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0o644) + if err != nil { + return err + } + defer f.Close() + prefix := "" + if len(b) > 0 && b[len(b)-1] != '\n' { + prefix = "\n" + } + _, err = fmt.Fprintln(f, prefix+task.LockName) + return err +} + func (c *ctx) offerGbrain(o *options) { if o.gbrain == "0" { fmt.Fprintln(c.stdout, " gbrain : skipped (opted out) — offline 'devbrain brain search/get' still works") diff --git a/internal/install/install_test.go b/internal/install/install_test.go index bf4bc75..43f0a37 100644 --- a/internal/install/install_test.go +++ b/internal/install/install_test.go @@ -8,8 +8,40 @@ import ( "testing" "github.com/TheWeiHu/devbrain/internal/config" + "github.com/TheWeiHu/devbrain/internal/task" ) +func TestEnsureDataRepoIgnoresTaskLockOnUpgrade(t *testing.T) { + for _, existing := range []string{"", "custom-output/", "custom-output/\n", task.LockName + "\n"} { + t.Run(existing, func(t *testing.T) { + data := t.TempDir() + if err := os.Mkdir(filepath.Join(data, ".git"), 0o755); err != nil { + t.Fatal(err) + } + name := filepath.Join(data, ".gitignore") + if existing != "" { + if err := os.WriteFile(name, []byte(existing), 0o644); err != nil { + t.Fatal(err) + } + } + var out bytes.Buffer + c := &ctx{data: data, stdout: &out} + for range 2 { + if err := c.ensureDataRepo(); err != nil { + t.Fatal(err) + } + } + got := mustRead(t, name) + if !strings.HasPrefix(got, existing) || strings.Count(got, task.LockName) != 1 { + t.Fatalf("gitignore = %q: want preserved rules and one lock exclusion", got) + } + if !strings.Contains("\n"+got, "\n"+task.LockName+"\n") { + t.Fatalf("lock exclusion is not on its own line: %q", got) + } + }) + } +} + // setupHome builds a throwaway HOME with stubbed schedulers on PATH (so no // test can ever touch the host's launchd/systemd/cron) and pins every env // knob the installer reads. Returns the home dir. diff --git a/internal/task/alloc.go b/internal/task/alloc.go new file mode 100644 index 0000000..3d36a16 --- /dev/null +++ b/internal/task/alloc.go @@ -0,0 +1,69 @@ +package task + +import ( + "fmt" + "os" + "path" + "path/filepath" + "strconv" + "syscall" +) + +// LockName is the advisory-lock file AllocID takes inside a todo dir. It is a +// dotfile so the `*.md` listings and the bash-glob-style scans never see it, +// and the data repo's .gitignore (written by install) keeps it out of git. +const LockName = ".alloc.lock" + +// AllocID mints the next `NNNN-` task id under dir and reserves it by +// creating an empty `.md`. It is the ONE allocator for the queue (the todo +// CLI and the dashboard both mint through it), and the NNNN prefix — not the +// full file name — is the unit of exclusivity: the scan of the current max +// and the create run under an exclusive flock on dir/.alloc.lock, so two +// parallel `todo add` calls with different slugs can no longer both read the +// same max and both win their O_EXCL create (which is how one queue ended up +// with two files each for 0240, 0252, 0281 … in production). The O_EXCL create +// stays as belt-and-braces against a writer that bypasses the lock. +// +// Ids are counted across dir AND dir/archive so a moved-away top id can't be +// reissued; only canonical `NNNN-*.md` names take part in the scan, so a stray +// `12-short.md` or a non-task file never shifts the sequence. +func AllocID(dir, slug string) (string, error) { + if err := os.MkdirAll(dir, 0o755); err != nil { + return "", err + } + lock, err := os.OpenFile(filepath.Join(dir, LockName), os.O_CREATE|os.O_RDWR, 0o644) + if err != nil { + return "", err + } + defer lock.Close() + if err := syscall.Flock(int(lock.Fd()), syscall.LOCK_EX); err != nil { + return "", fmt.Errorf("lock %s: %w", lock.Name(), err) + } + defer syscall.Flock(int(lock.Fd()), syscall.LOCK_UN) //nolint:errcheck // Close releases it anyway + + seq := 0 + for _, d := range []string{dir, filepath.Join(dir, "archive")} { + ents, _ := os.ReadDir(d) + for _, e := range ents { + name := e.Name() + if ok, _ := path.Match("[0-9][0-9][0-9][0-9]-*.md", name); !ok { + continue + } + if n, err := strconv.Atoi(name[:4]); err == nil && n > seq { + seq = n + } + } + } + for { + seq++ + id := fmt.Sprintf("%04d-%s", seq, slug) + f, err := os.OpenFile(filepath.Join(dir, id+".md"), os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o644) + if err == nil { + f.Close() + return id, nil + } + if !os.IsExist(err) { + return "", err + } + } +} diff --git a/internal/task/alloc_test.go b/internal/task/alloc_test.go new file mode 100644 index 0000000..157dfab --- /dev/null +++ b/internal/task/alloc_test.go @@ -0,0 +1,95 @@ +package task + +import ( + "os" + "path/filepath" + "strings" + "sync" + "testing" +) + +// prefixes collects the NNNN of every id and fails on a duplicate. +func prefixes(t *testing.T, ids []string) map[string]string { + t.Helper() + seen := map[string]string{} + for _, id := range ids { + n := id[:4] + if prev, dup := seen[n]; dup { + t.Errorf("duplicate prefix %s: %s and %s", n, prev, id) + } + seen[n] = id + } + return seen +} + +func TestAllocIDConcurrentDistinctSlugs(t *testing.T) { + dir := filepath.Join(t.TempDir(), "todo") + // Pre-existing top id, plus an archived one that must stay counted and a + // non-canonical name that must not take part in the scan. + if err := os.MkdirAll(filepath.Join(dir, "archive"), 0o755); err != nil { + t.Fatal(err) + } + for _, f := range []string{"0003-live.md", "archive/0005-moved.md", "archive/12-short.md", "notes.txt"} { + if err := os.WriteFile(filepath.Join(dir, f), []byte("x\n"), 0o644); err != nil { + t.Fatal(err) + } + } + const n = 32 + ids := make([]string, n) + var wg sync.WaitGroup + for i := range n { + wg.Add(1) + go func() { + defer wg.Done() + id, err := AllocID(dir, "slug-"+strings.Repeat("x", i%5)+string(rune('a'+i%26))) + if err != nil { + t.Error(err) + return + } + ids[i] = id + }() + } + wg.Wait() + if t.Failed() { + return + } + seen := prefixes(t, ids) + if len(seen) != n { + t.Fatalf("got %d unique prefixes, want %d: %v", len(seen), n, ids) + } + for _, want := range []string{"0006", "0037"} { + if _, ok := seen[want]; !ok { + t.Errorf("prefix %s missing — sequence should run 0006..0037 after archived 0005: %v", want, ids) + } + } + if _, ok := seen["0004"]; ok { + t.Errorf("0004 was issued: archived 0005 must be counted: %v", ids) + } + for _, id := range ids { + if _, err := os.Stat(filepath.Join(dir, id+".md")); err != nil { + t.Errorf("%s not reserved on disk: %v", id, err) + } + } + if _, err := os.Stat(filepath.Join(dir, LockName)); err != nil { + t.Errorf("lock file missing: %v", err) + } +} + +func TestAllocIDSkipsReservedSlot(t *testing.T) { + dir := t.TempDir() + // A writer that bypassed the lock already holds 0001-taken: the O_EXCL + // create still moves past it rather than clobbering. + if _, err := AllocID(dir, "taken"); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "0002-bypass.md"), nil, 0o644); err != nil { + t.Fatal(err) + } + id, err := AllocID(dir, "next") + if err != nil { + t.Fatal(err) + } + if id != "0003-next" { + t.Errorf("id = %q, want 0003-next", id) + } +} diff --git a/internal/todo/cli_test.go b/internal/todo/cli_test.go index d13bfdb..9579aac 100644 --- a/internal/todo/cli_test.go +++ b/internal/todo/cli_test.go @@ -5,12 +5,14 @@ package todo_test // binary via the shared clitest harness. Reference for the rest of the suite. import ( + "fmt" "os" "os/exec" "path/filepath" "regexp" "strconv" "strings" + "sync" "testing" "github.com/TheWeiHu/devbrain/internal/clitest" @@ -433,3 +435,73 @@ func containsLine(s, line string) bool { } return false } + +// TestTodoAddConcurrentUniquePrefixes reproduces the production duplicate-id +// bug at the process level: N parallel `todo add` calls with DISTINCT slugs +// used to race the max-NNNN scan and each win its own O_EXCL create, leaving +// two files per prefix (0240, 0252, 0281 … in one real queue). The prefix, +// not the file name, must be the unit of exclusivity. +func TestTodoAddConcurrentUniquePrefixes(t *testing.T) { + h := clitest.New(t) + const n = 24 + ids := make([]string, n) + var wg sync.WaitGroup + for i := range n { + wg.Add(1) + go func() { + defer wg.Done() + r := h.Run("todo", "add", fmt.Sprintf("parallel task %c", 'a'+i), "-p", strconv.Itoa(i)) + if r.Code != 0 { + t.Errorf("add %d: exit %d\n%s", i, r.Code, r.Stderr) + return + } + ids[i] = r.Out() + }() + } + wg.Wait() + seen := map[string]string{} + for _, id := range ids { + if len(id) < 5 { + t.Fatalf("malformed id %q in %v", id, ids) + } + if prev, dup := seen[id[:4]]; dup { + t.Errorf("duplicate prefix %s: %s and %s", id[:4], prev, id) + } + seen[id[:4]] = id + } + if len(seen) != n { + t.Errorf("%d unique prefixes, want %d: %v", len(seen), n, ids) + } + if got := listIDs(h.Run("todo", "list").Stdout); len(got) != n { + t.Errorf("list shows %d tasks, want %d", len(got), n) + } +} + +// TestTodoAddSoftCapWarning: past OpenSoftCap open tasks `add` still succeeds +// but says so on stderr, so a fold-in that ignores the /distill cap is at +// least visible in its transcript. +func TestTodoAddSoftCapWarning(t *testing.T) { + h := clitest.New(t) + for i := 1; i <= 20; i++ { + r := h.Run("todo", "add", fmt.Sprintf("task %d", i)) + if r.Code != 0 || r.Stderr != "" { + t.Fatalf("add %d: exit %d stderr %q", i, r.Code, r.Stderr) + } + } + r := h.Run("todo", "add", "one over") + if r.Code != 0 || r.Out() == "" { + t.Fatalf("add over cap must still succeed: exit %d out %q", r.Code, r.Out()) + } + if !strings.Contains(r.Stderr, "21 open tasks (soft cap 20)") { + t.Errorf("stderr = %q, want soft-cap warning", r.Stderr) + } + // done tasks don't count: closing two leaves 19 open, so the next add (20) is quiet + for _, id := range []string{r.Out(), "0001-task-1"} { + if d := h.Run("todo", "done", id, "--force"); d.Code != 0 { + t.Fatalf("done %s: exit %d %s", id, d.Code, d.Stderr) + } + } + if r2 := h.Run("todo", "add", "back under"); r2.Stderr != "" { + t.Errorf("stderr after a done = %q, want none", r2.Stderr) + } +} diff --git a/internal/todo/todo.go b/internal/todo/todo.go index 195e74e..f30ed80 100644 --- a/internal/todo/todo.go +++ b/internal/todo/todo.go @@ -11,7 +11,6 @@ import ( "io" "os" "os/exec" - "path" "path/filepath" "regexp" "slices" @@ -306,38 +305,39 @@ func isFence(line string) bool { return strings.HasPrefix(line, "---") && strings.TrimRight(line[3:], " \t\v\f\r") == "" } -// allocFile allocates the next NNNN-slug id: scan the max NNNN, then an -// O_EXCL create loop beats a parallel writer to the slot. +// allocFile mints the next NNNN-slug id through the shared allocator: the +// scan of the current max and the O_EXCL create run under one advisory lock +// on /.alloc.lock, so parallel adds with different slugs can't share a +// prefix (see task.AllocID). func (c *cli) allocFile(slug string) (string, error) { - if err := os.MkdirAll(c.dir, 0o755); err != nil { - return "", err - } - seq := 0 - // Scan the live dir AND archive/ so a moved-away top id can't be reissued. - for _, dir := range []string{c.dir, filepath.Join(c.dir, "archive")} { - ents, _ := os.ReadDir(dir) - for _, e := range ents { - name := e.Name() - if ok, _ := path.Match("[0-9][0-9][0-9][0-9]-*.md", name); !ok { - continue - } - if n, err := strconv.Atoi(name[:4]); err == nil && n > seq { - seq = n - } - } + return task.AllocID(c.dir, slug) +} + +// OpenSoftCap is the queue size past which `add` warns: /distill's fold-in +// must merge into an existing task or drop instead of adding once a project +// has more open tasks than a human will ever triage (321 tasks minted in six +// weeks; 17 of 72 open ones were real work). +const OpenSoftCap = 20 + +// openCount counts tasks whose STORED status is open — a cheap frontmatter +// read, deliberately not rows("open"), which would prime the git-derive +// cache (a fetch) on every add. +func (c *cli) openCount() int { + ents, err := os.ReadDir(c.dir) + if err != nil { + return 0 } - for { - seq++ - id := fmt.Sprintf("%04d-%s", seq, slug) - f, err := os.OpenFile(c.taskPath(id), os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o644) - if err == nil { - f.Close() - return id, nil + n := 0 + for _, e := range ents { + name := e.Name() + if !strings.HasSuffix(name, ".md") || strings.HasPrefix(name, ".") { + continue } - if !os.IsExist(err) { - return "", err + if t, err := task.Load(filepath.Join(c.dir, name), c.project); err == nil && t.Status == "open" { + n++ } } + return n } // ── derived status (nightshift mode) ───────────────────────────────────────── @@ -607,6 +607,9 @@ func (c *cli) add(args []string) int { if err := os.WriteFile(c.taskPath(id), []byte(content), 0o644); err != nil { return c.die(err.Error()) } + if n := c.openCount(); n > OpenSoftCap { + fmt.Fprintf(c.stderr, "devbrain todo: %d open tasks (soft cap %d) — merge into an existing task or drop before adding more\n", n, OpenSoftCap) + } fmt.Fprintln(c.stdout, id) return 0 }