diff options
| author | Peter Stone <thepeterstone@gmail.com> | 2026-07-10 20:17:03 +0000 |
|---|---|---|
| committer | Peter Stone <thepeterstone@gmail.com> | 2026-07-10 20:17:33 +0000 |
| commit | db1f7d9f18142791776a510ab4e8e28974cdcd40 (patch) | |
| tree | 1c8d0be91003e6c92ca147c99ae19b796efcbc81 | |
| parent | 34380da06b1b0f2088f83c1edcb0b506e3196cca (diff) | |
docs: rewrite piece 5 plan as a code-based SeedRoleConfigs (mirrors SeedProjects) instead of a manual API call
| -rw-r--r-- | docs/superpowers/plans/2026-07-10-builder-role-system-prompt.md | 273 |
1 files changed, 273 insertions, 0 deletions
diff --git a/docs/superpowers/plans/2026-07-10-builder-role-system-prompt.md b/docs/superpowers/plans/2026-07-10-builder-role-system-prompt.md new file mode 100644 index 0000000..6ee5042 --- /dev/null +++ b/docs/superpowers/plans/2026-07-10-builder-role-system-prompt.md @@ -0,0 +1,273 @@ +# Builder Role System Prompt — Seed Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Deliver piece 5 (the final piece) of `docs/superpowers/specs/2026-07-09-recursive-arbitrated-review-design.md`: give the `builder` role an actual `RoleConfig.SystemPrompt` that instructs the decompose-or-implement judgment and tells the agent to use `spawn_subtask`'s `role`/`depends_on`/`acceptance_criteria` parameters when it decomposes. Per the spec: "None of this mechanism does anything unless the builder role's `RoleConfig.SystemPrompt` actually instructs [this] ... This is a real deliverable of implementing this design, not a side effect of the code changes above." + +**Architecture:** A new `SeedRoleConfigs()` method on `storage.DB` (in `internal/storage/seed.go`, alongside the existing `SeedProjects()`), called from `internal/cli/serve.go` right next to the existing `store.SeedProjects()` call. Idempotent and non-destructive: it only creates+activates a version for a role that has **no active version at all** (checked via `GetActiveRoleConfig`, which returns `sql.ErrNoRows` when nothing is active) — a role a human later customizes via `POST /api/roles/{role}/versions` + `/activate` is never touched again by this seed on a subsequent restart. + +**Tech Stack:** Go, `internal/storage` + `internal/cli` only. + +## Global Constraints + +- This is piece 5 (the final piece) of `docs/superpowers/specs/2026-07-09-recursive-arbitrated-review-design.md`. Pieces 1, 2a, 2b, 3, 4a, 4b-1, 4b-2, 4b-3 are already shipped. +- Only the `builder` role is seeded by this plan. The other 6 roles this design already dispatches (`evaluator_quality`, `evaluator_security`, `evaluator_correctness`, `evaluator_performance`, `planner`, `retro`) already receive fully specific per-task instructions text from `ensureEvaluators`/`ensureArbitration`/`processRetro`, so they function meaningfully even with no active `role_configs` row. `builder`'s task-level instructions (a story's spec + acceptance criteria alone) contain no guidance at all about the decompose-or-implement judgment — that's the gap this design's own text identifies as blocking, and the only gap this plan closes. +- `Tools`, `SandboxKind`, `DefaultBudgetUSD` are left unset on the seeded `RoleConfig` — per CLAUDE.md's own Design Debt entry, none of these are enforced by the executor yet regardless of what's stored, so setting them would only be misleading decoration. +- `EscalationLadder`'s `Rung.Provider` must be `"claude"` (not `"anthropic"` or any other cloud-provider key) to actually resolve on this deployment — verified by reading `/site/doot.terst.org/claudomator.toml`: only `claude_binary_path` (registers the `"claude"` `ContainerRunner`) and `[local_model]` (registers `"local"`) are configured; no cloud API keys are present. +- Do not add seeds for any role other than `builder` in this plan. Do not touch `internal/api/roles.go`, `internal/role/role.go`, or `internal/executor` — none of them need to change for this piece. + +--- + +### Task 1: Add `SeedRoleConfigs` and wire it into `serve.go` + +**Files:** +- Modify: `internal/storage/seed.go` (add `SeedRoleConfigs`, `builderRoleConfigJSON`, new imports `database/sql` and `errors`) +- Modify: `internal/storage/seed_test.go` (new file — no test file currently exists for `seed.go`) +- Modify: `internal/cli/serve.go` (one new call site) + +**Interfaces:** +- Consumes: `s.GetActiveRoleConfig(roleName string) (*RoleConfigRow, error)`, `s.CreateRoleConfig(roleName, configJSON, proposedBy string) (*RoleConfigRow, error)`, `s.ActivateRoleConfigVersion(roleName string, version int) error` — all pre-existing, unchanged, in `internal/storage/roleconfig.go`. +- Produces: `func (s *DB) SeedRoleConfigs() error` — new, mirrors `SeedProjects`'s signature/contract exactly (safe to call on every startup, logs-only on error at the call site). + +- [ ] **Step 1: Add `SeedRoleConfigs` to `internal/storage/seed.go`** + +Find the top of `internal/storage/seed.go`: + +```go +package storage + +import ( + "os/exec" + "strings" + + "github.com/thepeterstone/claudomator/internal/task" +) +``` + +Replace with: + +```go +package storage + +import ( + "database/sql" + "errors" + "os/exec" + "strings" + + "github.com/thepeterstone/claudomator/internal/task" +) +``` + +Then, at the end of the file (after `localBareRemote`'s closing brace), add: + +```go +// SeedRoleConfigs upserts default role_configs for roles that ship with no +// operator-authored configuration yet. Idempotent and non-destructive: if a +// role already has an active version -- whether seeded here on a prior +// startup, or authored later by a human via POST /api/roles/{role}/versions +// + /activate -- it is left untouched. Mirrors SeedProjects' "safe to call +// on every startup" contract. +func (s *DB) SeedRoleConfigs() error { + seeds := []struct { + role string + configJSON string + }{ + {role: "builder", configJSON: builderRoleConfigJSON}, + } + for _, seed := range seeds { + if _, err := s.GetActiveRoleConfig(seed.role); err == nil { + continue // already active; never overwrite + } else if !errors.Is(err, sql.ErrNoRows) { + return err + } + row, err := s.CreateRoleConfig(seed.role, seed.configJSON, "seed") + if err != nil { + return err + } + if err := s.ActivateRoleConfigVersion(seed.role, row.Version); err != nil { + return err + } + } + return nil +} + +// builderRoleConfigJSON is the default role.RoleConfig for the "builder" +// role -- see docs/superpowers/specs/2026-07-09-recursive-arbitrated-review-design.md, +// Implementation Order item 5: "the builder role's system prompt still +// needs to be written ... a real deliverable of implementing this design." +// Teaches the decompose-or-implement judgment and spawn_subtask's +// role/depends_on/acceptance_criteria parameters -- see +// internal/executor/agentmcp.go's spawnSubtaskInput for the exact tool +// parameter names used here (name, instructions, model, max_budget_usd, +// role, depends_on, acceptance_criteria). EscalationLadder's provider is +// "claude" to match this deployment's registered ContainerRunner (see this +// plan's Global Constraints). +const builderRoleConfigJSON = `{ + "role": "builder", + "system_prompt": "You are operating as a builder in Claudomator's recursive arbitrated-review system. Every builder-role task you touch -- whether it's a story's root task or a subtask several levels deep -- is automatically reviewed by 4 independent evaluators (quality, security, correctness, performance) and an arbitration pass once your work reaches a ready state. You do not need to ask for review, and you do not merge or accept your own work -- the orchestrator only promotes it to COMPLETED after arbitration approves it. If arbitration rejects your work, a fresh fix-attempt task is automatically spawned carrying the rejection's reasoning; whichever agent picks up that fix-attempt will see that reasoning in its own task instructions.\n\n## The decompose-or-implement judgment\n\nBefore writing any code, decide: can this task be done as one cohesive, reviewable unit of work, or does it naturally split into distinct pieces that each deserve their own focused review?\n\n- Implement directly when the task is a single cohesive change: a bug fix, a focused feature, a refactor confined to one area. Do the work, commit it, and call report_summary. Arbitrated review happens automatically once you finish.\n- Decompose when the task is large enough that bundling it into one review would blur together unrelated concerns, or when it naturally breaks into ordered or independent pieces (e.g. \"add the storage column\", then \"wire it into the API\", then \"update the UI\"). Use spawn_subtask for each piece.\n\nThis refines the general \"break work over ~3 minutes into pieces\" guidance above -- that heuristic still applies, but for a builder task specifically, decomposition is also about producing pieces that are independently reviewable, not merely independently executable.\n\n## When you decompose: use role \"builder\", not a fixed model\n\nEvery subtask you spawn that represents more implementation work -- not evaluation, not arbitration -- must be spawned with role set to \"builder\", not a fixed model. This is what makes the recursion actually recurse: a subtask spawned with role \"builder\" gets the exact same decompose-or-implement judgment and the same 4-evaluator arbitrated review your own task is getting right now, at whatever depth it sits. A subtask spawned with a fixed model instead of a role skips all of that -- only do this for something that is definitely not further build/review work; there is rarely a reason to, since builder-role dispatch already resolves an appropriate model via the role's own escalation ladder.\n\n- Use depends_on to express real ordering between subtasks (e.g. \"wire it into the API\" depends on \"add the storage column\"). Independent subtasks should not depend on each other -- false dependencies just slow the story down.\n- Use acceptance_criteria on every subtask you spawn -- a short list of concrete, checkable statements describing what that specific piece of work must satisfy. This is what the 4 evaluators and arbitration will actually judge that subtask against; a subtask with no acceptance criteria falls back to the whole story's criteria, which is usually too broad to be a meaningful check for one piece. Write criteria specific to the piece, not a restatement of the whole story.\n- Give each subtask focused, self-contained instructions -- the agent that picks it up will not see your reasoning for the overall decomposition, only what you put in instructions.\n\n## Keep decomposition shallow and purposeful\n\nDon't decompose for its own sake. A task that's already a small, single-purpose change should be implemented directly, not split into one-line subtasks -- every additional layer of decomposition adds a full evaluator-and-arbitration cycle's worth of latency and cost. Stop decomposing once a piece is small enough to implement, test, and review as one unit.", + "escalation_ladder": [ + { + "candidates": [{"provider": "claude", "model": "sonnet"}], + "selection_mode": "single", + "max_retries": 1 + }, + { + "candidates": [{"provider": "claude", "model": "opus"}], + "selection_mode": "single", + "max_retries": 0 + } + ] +}` +``` + +- [ ] **Step 2: Create `internal/storage/seed_test.go`** + +No test file currently exists for `seed.go`. Create `internal/storage/seed_test.go`: + +```go +package storage + +import ( + "encoding/json" + "testing" + + "github.com/thepeterstone/claudomator/internal/role" +) + +func TestSeedRoleConfigs_CreatesAndActivatesBuilder(t *testing.T) { + db := testDB(t) + + if err := db.SeedRoleConfigs(); err != nil { + t.Fatalf("SeedRoleConfigs: %v", err) + } + + row, err := db.GetActiveRoleConfig("builder") + if err != nil { + t.Fatalf("GetActiveRoleConfig(builder): %v", err) + } + if row.Version != 1 { + t.Errorf("expected version 1, got %d", row.Version) + } + if row.ProposedBy != "seed" { + t.Errorf("expected proposed_by 'seed', got %q", row.ProposedBy) + } + + var cfg role.RoleConfig + if err := json.Unmarshal([]byte(row.ConfigJSON), &cfg); err != nil { + t.Fatalf("unmarshal seeded config: %v", err) + } + if cfg.Role != "builder" { + t.Errorf("cfg.Role = %q, want builder", cfg.Role) + } + if cfg.SystemPrompt == "" { + t.Error("cfg.SystemPrompt is empty, want the decompose-or-implement guidance") + } + if len(cfg.EscalationLadder) != 2 { + t.Fatalf("expected 2 escalation tiers, got %d", len(cfg.EscalationLadder)) + } + if cfg.EscalationLadder[0].Candidates[0].Provider != "claude" { + t.Errorf("tier 0 provider = %q, want claude", cfg.EscalationLadder[0].Candidates[0].Provider) + } +} + +func TestSeedRoleConfigs_Idempotent_DoesNotDuplicateOrOverwrite(t *testing.T) { + db := testDB(t) + + if err := db.SeedRoleConfigs(); err != nil { + t.Fatalf("first SeedRoleConfigs: %v", err) + } + if err := db.SeedRoleConfigs(); err != nil { + t.Fatalf("second SeedRoleConfigs: %v", err) + } + + versions, err := db.ListRoleConfigVersions("builder") + if err != nil { + t.Fatalf("ListRoleConfigVersions: %v", err) + } + if len(versions) != 1 { + t.Fatalf("expected exactly 1 version after calling SeedRoleConfigs twice, got %d", len(versions)) + } +} + +func TestSeedRoleConfigs_DoesNotOverwriteHumanCustomizedVersion(t *testing.T) { + db := testDB(t) + + // A human (or the retro role's propose_role_config tool) already + // created and activated their own version before the seed ever ran. + custom, err := db.CreateRoleConfig("builder", `{"role":"builder","system_prompt":"custom prompt"}`, "human") + if err != nil { + t.Fatalf("CreateRoleConfig: %v", err) + } + if err := db.ActivateRoleConfigVersion("builder", custom.Version); err != nil { + t.Fatalf("ActivateRoleConfigVersion: %v", err) + } + + if err := db.SeedRoleConfigs(); err != nil { + t.Fatalf("SeedRoleConfigs: %v", err) + } + + row, err := db.GetActiveRoleConfig("builder") + if err != nil { + t.Fatalf("GetActiveRoleConfig: %v", err) + } + if row.ProposedBy != "human" { + t.Errorf("active role config proposed_by = %q, want unchanged 'human' -- seed must not overwrite a human-authored active version", row.ProposedBy) + } + + versions, err := db.ListRoleConfigVersions("builder") + if err != nil { + t.Fatalf("ListRoleConfigVersions: %v", err) + } + if len(versions) != 1 { + t.Fatalf("expected exactly 1 version (seed must not create a competing draft), got %d", len(versions)) + } +} +``` + +- [ ] **Step 3: Wire `SeedRoleConfigs` into `serve.go`** + +In `internal/cli/serve.go`, find: + +```go + if err := store.SeedProjects(); err != nil { + logger.Error("failed to seed projects", "error", err) + } +``` + +Replace with: + +```go + if err := store.SeedProjects(); err != nil { + logger.Error("failed to seed projects", "error", err) + } + if err := store.SeedRoleConfigs(); err != nil { + logger.Error("failed to seed role configs", "error", err) + } +``` + +- [ ] **Step 4: Commit locally NOW (before further verification)** + +```bash +git add -A +git commit -m "feat(storage): seed the builder role's system prompt on server startup" +git status +git log --oneline -1 +``` + +Do NOT run `git push`. This repo's own bare git remote is not mounted inside dispatch containers. Per this repo's own CLAUDE.md ("After a successful run, new commits are pushed from the workspace"), claudomator's ContainerRunner pushes your commits externally once your run finishes successfully with a clean working tree. + +- [ ] **Step 5: Run the storage and cli package tests** + +Run: `go test ./internal/storage/... ./internal/cli/... -v -run 'TestSeedRoleConfigs|TestSeedProjects'` +Expected: PASS for all 3 new `TestSeedRoleConfigs_*` tests. Also confirm `json.Unmarshal` of `builderRoleConfigJSON` round-trips cleanly (covered by `TestSeedRoleConfigs_CreatesAndActivatesBuilder`'s unmarshal step — if this fails, the raw JSON string has a syntax error, most likely in the escaped `system_prompt` value; fix the escaping and recommit). + +- [ ] **Step 6: Run the full repo test suite** + +Run: `go build ./...` then `go test ./...` (the entire repo). Both must pass — paste the actual output. If `internal/api` flakes under the race detector on a one-off "sql: database is closed" teardown error, or if you see pre-existing unrelated failures in `TestHandleGetSpendTimeseries_*`/`TestQuerySpendTimeseries_*` (a known pre-existing NULL-scan issue, confirmed unrelated to this change by piece 4b-3's review), note them as pre-existing and unrelated rather than treating them as caused by this task. + +Also run `gofmt -l internal/storage/seed.go internal/storage/seed_test.go internal/cli/serve.go` and `gofmt -w` anything it flags that wasn't already gofmt-dirty before your changes. + +## Mandatory verification disclosure + +When you call report_summary, paste the actual terminal output of every command above — literal pass/fail counts, not a claim of success — including the full-repo `go test ./...` run and the final `git status`/`git log` confirmation showing a clean tree with your commit(s) present. If anything here is ambiguous or conflicts with what you find in the actual repo, call ask_user and describe the specific conflict — don't guess or silently decide. |
