Repository navigation
feat(playbooks): persist agent-led runs and expose tools - #248
Conversation
github-actions
Bot
added
vouch:trusted
d5c5a44 to
11f4dd2
Compare
bryantderosier
left a comment
bryantderosier
left a comment
There was a problem hiding this comment.
Review of the runtime layer. I ran it through correctness, code quality, security, and docs/fork-policy passes; inline comments cover the specific changes I want.
Security came back clean: routes and MCP tools are scoped to the owner thread, definition paths can't escape .j5/playbooks, and the YAML parse is capped and alias-free. Fork ownership is also in good shape. The work lives on the J5 side, migration 18 goes through the J5 lane, and there are no new upstream RPC methods.
What I need before this merges:
- A way out for runs whose owner thread is gone (inline on
readRun). This is the one I'd hold the stack for. - Put the feature definition back in the shape
docs/j5/process/docs.mdrequires (inline on the doc). - Fix the doc updates that don't show up inline:
docs/j5/glossary.md:28("step content that a Role or Crew follows") andproblems.md(~101-110 and ~125-138, "delivers the next step's instructions to every participant") now contradict the rewritten definition, anddocs/README.mddoesn't link the newdocs/user/playbooks.mdfrom the user-guide index. - Add the FORK.md Path-to-contract ledger rows for contract 38:
T3OrchestrationInstructions.tsstill saysR | 8, andmcp/toolkits/worktree/registration.test.tsstill saysA | 4even though it gains the playbook tool names. #151's ws.ts row is the precedent.
| "run_not_found", | ||
| "No playbook run was found. Use playbook_list and playbook_start.", | ||
| ); | ||
| if (row.owner_thread_id !== owner) |
There was a problem hiding this comment.
A run can't be ended once its owner thread can't act. Only the owner thread's agent gets past this check, nothing on the human side can cancel, and nothing reacts to the thread being deleted.
Repro: start release in a thread, then delete the thread. From then on the run shows as "In progress / Owner unavailable" on the /playbooks Active board, and every library delete of release.yaml fails with in_use (see removeDefinition). The user has no way to clear either.
Please add an operate-scoped /api/j5/playbooks/cancel route with a Cancel control on the web and mobile boards, and/or treat runs whose owner thread is deleted as cancelled (and skip them in the in_use check). Either way it needs a focused store test.
| const info = yield* fs.stat(filename); | ||
| if (info.type !== "File") | ||
| return yield* playbookError("invalid_path", "Only playbook YAML files can be deleted."); | ||
| const active = yield* sql<{ run_id: string }>`SELECT run_id FROM j5_playbook_run |
There was a problem hiding this comment.
This check is also where the orphaned-run problem above shows up: an active run owned by a deleted thread blocks this delete permanently. Whatever fix lands for readRun should cover this query too.
| ) { | ||
| return yield* definition.pipe( | ||
| Effect.map((definition) => present(run, definition, replayed)), | ||
| Effect.catch((issue) => |
There was a problem hiding this comment.
This catch attaches issue no matter what the run's status is. Once a completed run's playbook is deleted (allowed, since nothing is active), every past run of it shows a warning badge, sorts above healthy active runs, and tells the user to "Restore or fix its YAML and retry" for a run that's already finished. The thread board shows the same thing.
Please only attach issue when run.status === "active", or have the clients ignore issues on terminal runs.
| "playbook_back", | ||
| "playbook_reselect", | ||
| "playbook_complete", | ||
| "playbook_cancel", |
There was a problem hiding this comment.
The six playbook mutations are only in J5_CODEX_PREAPPROVED_TOOLS, which j5CodexT3McpServerConfig uses only when the approval policy is never. A normal approval-required or auto-accept-edits Codex thread gets J5_CODEX_COORDINATION_TOOLS, so every playbook_start/next/complete pops an approval, and agent-led advancement doesn't work.
These only move the thread's own run pointer, so I think they belong in the coordination list next to send_message. If prompting is intended, write that down. Please also record an explicit per-adapter decision for Cursor, Grok, OpenCode, and Antigravity.
| const definitionPath = path.resolve(workspaceRoot, ".j5/playbooks", `${stem}.yaml`); | ||
| const request = encodeRequest(["start", definitionPath]); | ||
| return yield* sql | ||
| .withTransaction( |
There was a problem hiding this comment.
start and mutate read and parse the definition (up to 256 KiB) inside sql.withTransaction, so file I/O happens while the write transaction is held on the shared SQLite client. Please read the definition before opening the transaction and re-check state inside it.
| )`; | ||
| yield* sql`CREATE UNIQUE INDEX j5_playbook_active_owner | ||
| ON j5_playbook_run(owner_thread_id) WHERE status = 'active'`; | ||
| yield* sql`CREATE INDEX j5_playbook_owner ON j5_playbook_run(owner_thread_id, created_at)`; |
There was a problem hiding this comment.
listAll does COUNT(*) plus ORDER BY (status = 'active') DESC, updated_at DESC, and the only index is on (owner_thread_id, created_at). That's a full scan and sort on every runs-page poll. Please add an index on (status, updated_at). Also, j5_playbook_request gets a row per tool call and is never pruned; please keep request receipts only as long as replay actually needs them.
| const fs = yield* FileSystem.FileSystem; | ||
| const path = yield* Path.Path; | ||
| const crypto = yield* Crypto.Crypto; | ||
| // ponytail: one permit per environment; split by owner only if tool traffic warrants it. |
There was a problem hiding this comment.
Nit: this ponytail: comment records a limitation / TODO rather than describing how the permit is used. Please drop it or turn it into a plain description.
| export const T3_CODE_ORCHESTRATION_INSTRUCTIONS = ` | ||
|
|
||
| ## T3 Code orchestration | ||
|
|
||
| The \`t3-code\` MCP server provides app-owned orchestration. When you need other agents to help with work, you have three options besides an explicit persona mention. Treat these concepts distinctly: | ||
|
|
||
| ${AGENT_INVOCATION_INSTRUCTIONS} | ||
| ${PLAYBOOK_INSTRUCTIONS} |
There was a problem hiding this comment.
This puts the playbook bullets inside the list introduced as "you have three options besides an explicit persona mention" for getting other agents' help, between the persona and Subagent bullets. Playbooks aren't one of those options, so the framing is now wrong, and every provider's system prompt gets several long bullets. Please append the playbook guidance after the Crew/Peer bullets under its own short lead-in.
|
|
||
| ## History | ||
| This version has no executable steps, transition graphs, scheduler, or agent |
There was a problem hiding this comment.
This rewrite breaks the definition rules in docs/j5/process/docs.md:
- the frontmatter (
title,kind: definition) is gone; every otherfeatures/*.mdhas it; - the required Acceptance criteria, Scenarios, and History sections are deleted, and AC1–AC7 aren't marked retired, so existing "Playbooks AC3" references no longer resolve and the R27 → AC3–AC4 mapping is lost;
- build status and issue narrative now live in the body ("This version has no…", "No old definition or run migration is included", feat(agents): run Playbooks inside a Crew #170, feat(playbooks): agent-led prompt sequences #194);
- the end-state definition (Role/Crew playbooks, per-participant delivery) is narrowed to what this build ships.
Please restore the definition shape, change only the sentences that are no longer true, add a History line, and move the "this build ships single-thread, agent-led runs" scope into #194 or a plan.
| provider restarts. | ||
|
|
||
| Playbooks operate within one thread. Shared Squadron progression, executable steps, | ||
| automatic scheduling, and agent spawning are outside this version. |
There was a problem hiding this comment.
"outside this version" is build-status wording that users can't act on. Please describe what playbooks do today and drop the version framing. Also, docs/README.md doesn't link this page from the user-guide index, and nothing later in the stack adds the link.
7737948 to
2869a1f
Compare
|
Warning Review limit reached
This review includes 37 billable files and costs up to $9.25. Or wait 59 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: Jacksondr5/j5code/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (37)
Comment |
2869a1f to
11c70ab
Compare
11c70ab to
1081580
Compare
Carried from j5/main 4bcca20 onto the upstream V2 candidate. Conflict: FORK.md only; the agent-led playbooks case becomes 45 (inventory 46) after the candidate's cases 40-41 and skills cases 42-44, and its two ledger contract references follow (4, 45 and 8, 45). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Joins fork head 2cf4ad7 (j5/main, including #247, #241, #244, #242, #214, #215, #216, #248, #249, #250, #251) with the reviewed candidate (j5/upstream-sync-20260924-candidate), which descends from frozen upstream 67a2be0. Upstream force-rewrote history, so per FORK.md's rewrite runbook the candidate was built from the upstream tree with pin 62aef85 as the content base, then carried each j5/main PR since 8f56083 onto it and adapted it to upstream V2. This merge's tree equals the candidate tree exactly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 participants
What Changed
Agents need a durable position in a live sequence of prompts without a workflow engine driving their turns. This adds workspace YAML discovery, one active run per owner thread, guarded and idempotent movement, terminal history, MCP tools, and authenticated library/progress reads.
Part 1 of 4 for #194. Base:
j5/main. Review and merge from the runtime slice upward.Why
Run IDs remain independent from owner thread IDs. Live edits preserve stable step identity, missing steps require explicit reselection, and cancellation remains possible when YAML is broken. Playbook migration 18 follows the Crew migrations already on main, and Claude's explicit tool allowlist includes the new tools. Existing Crew registrations and provider guidance are preserved.
Validation
73 focused server tests passed across 13 files, including an upgrade from migration 17 and the production HTTP/MCP registrations. Server and contracts typechecks, scoped lint, formatting, and diff checks passed. Real-provider execution was not exercised during this split.
Checklist
Created with GPT-6 in Codex.