Repository navigation
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: preserve compiled instructions, isolate per-turn resolver state, and reconcile the canonical Playbook architecture.
I found three blocking issues and marked each inline.
Coverage: I reviewed git diff github/feat/harness_generation_capabilities...d8f6492cf3cf, the intervening head update, AGENTS.md, CONTEXT-MAP.md/CONTEXT.md, the new v2 models/compiler/run records, loop and DAG wiring, store/executor projections, CLI/RPC/Web callers, commit/PR history, backward compatibility, and the changed tests. I found no weakened existing assertions.
Verification on this exact head:
uv run pytest tests/test_*playbook*.py tests/integration/test_agent_playbook_e2e.py tests/integration/test_unified_playbook_e2e.py -x- 440 passed.npm test -- --run src/features/playbooks/PlaybooksPage.test.tsx- 43 passed.npm run type-check- passed.npm run gen:check- passed.uv run python scripts/check_source_language.py github/feat/harness_generation_capabilities...HEAD- passed.git diff --check github/feat/harness_generation_capabilities...HEAD- passed.
| tools = sorted((d.get("function", d) or {}).get("name", "") for d in self.tools.get_definitions()) | ||
| table = await WorkerTableGenerator(binding.provider, binding.model).generate( | ||
| getattr(req, "text", "") or "", agents, [t for t in tools if t], notes | ||
| candidates = dict(self._playbooks.listing(getattr(req, "text", "") or "")) if self._playbooks else {} |
There was a problem hiding this comment.
Blocking: reconcile pre-turn Playbook resolution with the canonical architecture contract.
CONTEXT.md currently defines Playbook discovery as having no pre-turn interception or LLM gate and assigns the choice to the model through load_playbook; this code now sends candidates to the pre-turn setup model and may bind the selected Harness before load_playbook runs. The same change introduces durable Workflow and Run Record vocabulary without updating that canonical context, despite AGENTS.md section 6 requiring new domain terms to be defined in the same change. Since this is an intentional architecture change per the PR description, update the Runtime context so the repository no longer gives the opposite ownership rule (or retain the documented discovery path).
There was a problem hiding this comment.
Fixed in a779548. CONTEXT.md now defines the v2 Harness / Workflow / Run Record split and documents both discovery paths: normal main-model load_playbook, and the configured pre-turn LLM resolver that may bind a saved Harness but never auto-runs its Workflow.
There was a problem hiding this comment.
Blocking: the canonical discovery contract and capture-mode result loss still need correction.
The a779548 documentation matched that revision, but 6542e44 subsequently removed saved-candidate selection: TaskPlaybookGenerator explicitly discards playbook_candidates, Persona receives only existing names for collision avoidance, and selected_playbook has no non-None production writer. CONTEXT.md still says the setup model receives ranked saved candidates and may select and bind one. The Harness / Workflow / Run Record definitions are now present, but the discovery contract is again describing a path the implementation does not have, so this thread remains open.
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the three previously reported production issues remain unchanged.
This revision changes tests only: it moves the stubbed unified lifecycle E2E suite into the default test set, uses typed constructor field names, and adds schema/provider-fallback coverage. I found no new defect in that delta and no weakened assertions.
The production files underlying all three open findings, plus CONTEXT.md, are unchanged from d8f6492cf3cf; the prompt-rewrite reproduction still returns preservation_errors=[]. The existing threads therefore remain open and continue to block this revision.
Coverage: I reviewed the revision delta and history, rechecked AGENTS.md/CONTEXT.md implications, the affected test callers, backward-compatibility coverage, and the full standing findings.
Verification on 01e8bc134efb:
uv run pytest tests/test_*playbook*.py -x- 437 passed.uv run python scripts/check_source_language.py github/feat/harness_generation_capabilities...HEAD- passed.git diff --check github/feat/harness_generation_capabilities...HEAD- passed.
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the three open Playbook integrity, turn-isolation, and architecture-contract defects still need to be fixed.
The 01e8bc1..4341582 delta only adds future-facing persona guidance and tests. It does not modify workflow_compiler.py, load_playbook.py, wiring.py, or CONTEXT.md; I rechecked the three open threads against this exact head and each still stands. I found no additional blocker in the new delta, and the new live travel-assistant scenario passed against openai-codex/gpt-5.6-sol.
Verification:
- uv run pytest tests/test_playbook.py tests/test_unified_playbook*.py -x: 437 passed
- uv run pytest tests/integration/test_unified_playbook_real_llm.py::test_live_whole_turn_infers_a_travel_assistant_harness_from_user_needs -v -s: 1 passed
- git diff --check github/feat/harness_generation_capabilities...HEAD: passed
- uv run python scripts/check_source_language.py github/feat/harness_generation_capabilities...HEAD: passed
- An initial command named two nonexistent focused test files and collected 0 tests (exit 5); I corrected the selection above and do not count that empty run as verification.
Coverage this round: the repository rules and canonical Runtime context, the full PR diff plus the new revision delta, affected generator callers and commit history, backward compatibility, test changes/no weakening, and the standing architecture constraints. No prior finding is repeated inline.
a36ed5b to
f49cdef
Compare
4341582 to
a9f27e9
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the three open Playbook integrity, turn-isolation, and architecture-contract defects still need to be fixed.
After refreshing the rebased target ref, I reviewed the full PR diff and range-diffed this revision against 4341582. The genuinely new changes bind a stored composite Workflow to its durable Harness during dispatch and refine worker selection by declared capability ownership. Those paths look sound, and I found no new blocker under the later-round bar. The adjusted live assertion is not test weakening for a green run: it now rejects the exact creation-time instruction while allowing a durable worker to state a legitimate future input gate.
The three earlier blocking paths remain unchanged in this PR revision, and the current canonical Runtime context still states the opposite discovery architecture, so none of those threads is settled or eligible to resolve.
Verification:
- uv run pytest tests/test_playbook.py tests/test_unified_playbook*.py -x: 441 passed
- uv run pytest tests/integration/test_unified_playbook_real_llm.py::test_live_whole_turn_infers_a_travel_assistant_harness_from_user_needs -v -s: 1 passed
- git diff --check github/feat/harness_generation_capabilities...HEAD: passed
- uv run python scripts/check_source_language.py github/feat/harness_generation_capabilities...HEAD: passed
Coverage this round: AGENTS.md and the canonical Runtime context, the refreshed base-to-head diff, the old-to-new range-diff and commit history, affected runtime/DAG callers, backward compatibility, test changes and potential weakening, and the standing architecture constraints.
|
Blocking fixes are now on head 53683a7, including the latest stacked base from #562. The compiler enforces reversible prompt parameterization and preserves node summaries; resolver selection is task-local; and CONTEXT.md now defines the Harness / Workflow / Run Record architecture and both discovery paths. Verification after the base sync: 467 Playbook/turn tests passed; ruff, ty, and diff checks passed. @gloryfromca please re-review the three blocking threads. @LivXue @0xKT please review/approve this head when available. |
| playbook_capture = current_capture() | ||
| capture_workflow = bool(playbook_capture is not None and playbook_capture.capture_workflow) | ||
| if capture_workflow: | ||
| background = False |
There was a problem hiding this comment.
Blocker - do not override the caller's background without letting the caller see it.
execute sets background = False whenever current_capture().capture_workflow is true, discarding whatever the caller passed. This class has a second caller besides the registered tool: the private instance PlaybookExecutor owns, built at raven/agent/loop/wiring.py:1356. That executor is constructed with background=True and renders its reply from self._background (raven/playbook/executor.py:601), not from what actually happened. So on a capture turn a stored Playbook loaded through load_playbook runs to completion in the foreground, _dispatch takes its final event off the outbox and returns the rendered result, and the executor throws that result away and replies started '<name>' (N steps); results will be delivered when the run completes. Nothing can deliver them afterwards: the outbox existed only because not background, and its one event is already consumed.
Reproduced at a59bd43, driving the real SubAgentDagTool.execute and the real PlaybookExecutor, with only _execute stubbed to supply the two receipt shapes:
ORDINARY background=True
reply: DAG run-...: started 'weekly-feedback' (2 steps); results will be delivered when the run completes.
CAPTURE background=False
reply: DAG run-...: started 'weekly-feedback' (2 steps); results will be delivered when the run completes.
run result reached the model? False
At this head the trigger is no longer conditional. capture_workflow is set as self.mode == "task" (raven/playbook/agent_generator.py:935), default_generation_mode is "task" (raven/config/schema.py:2204), and HarnessResolution.selected_playbook no longer has any writer, so the guard not resolution.selected_playbook at raven/agent/loop/wiring.py:801 is always true. Every turn running under playbooks.enabled with agentHarness: generate is therefore a capture turn unless the request or the deployment selects another mode. Reaching this defect then needs only that the model call load_playbook on such a turn, which is an ordinary path rather than a corner; that it does so is still code-reading, while the mechanism above is reproduced.
Forcing the foreground is deliberate and load-bearing here, so the remedy is not to drop it. What is missing is that the effective mode has to be observable by the caller, or the override has to be scoped to the registered tool rather than applied to every holder of this class.
|
|
||
| if workflow_capture_requested(): | ||
| capture_note = ( | ||
| " Playbook capture is active for this turn: put the complete reusable process in " |
There was a problem hiding this comment.
Nonblocking - say in this notice that background has stopped applying.
This paragraph is the only thing added to the model's view of the tool on a capture turn, and it omits the one behavioural change the model can act on. Three resident surfaces still present background as the caller's choice, and this PR changes none of them: the schema still advertises the background property; the description two paragraphs below still says The graph runs in the background and its result is announced to you when it finishes, so do not poll it and do not re-submit it; and the always-injected DAG skill teaches Pass background: false only when you genuinely cannot continue without the outputs (SKILL.md:119) and Don't reach for background: false to "make sure it finishes" (SKILL.md:463). On a capture turn the platform does exactly what that skill tells the model not to do, universally, and says nothing.
This PR's own end-to-end test at tests/test_unified_playbook_e2e.py:127 has the model pass "background": True and passes, because the override makes it false unobserved.
At this head the override is no longer gated on a per-turn model decision: capture_workflow is self.mode == "task" (raven/playbook/agent_generator.py:935) and task is the default default_generation_mode (raven/config/schema.py:2204), so under agentHarness: generate the background argument is ignored on every ordinary turn rather than on the occasional flagged one.
I confirmed the override itself at a59bd43 (background=True in, False at _execute). The four surfaces above are read at this head, not reproduced.
| self._desks.pop(run_id, None) | ||
|
|
||
| # A terminal event carrying the authoritative manifest, so the web UI can | ||
| if capture is not None and _successful_final(Final(result, stopped=cancel.is_set())): |
There was a problem hiding this comment.
Nonblocking - do not split the existing comment around this block.
Line 1990 opens A terminal event carrying the authoritative manifest, so the web UI can and line 1996 completes it with rebuild / finalize the graph (and survive a reload). This block now sits between the two halves, so the first half reads as a comment on the capture code, which it does not describe, and the second half reads as a fragment. Moving the capture block above the comment, or below the emit it belongs to, restores both.
Code-reading.
| origin: _DagOrigin, | ||
| call_id: str | None, | ||
| background: bool, | ||
| capture: tuple[Any, SubAgentDagSpec] | None = None, |
There was a problem hiding this comment.
Question - is a replanned graph meant to forfeit its capture silently?
_execute fills this parameter, but _submit_replan (line 1388) never does: its _dispatch call at line 1411 passes no capture. A replanned run therefore carries none, and the original run cannot supply one either, because _successful_final requires completed == total with no failed, cancelled or skipped, which a graph that was replanned does not satisfy. A capture turn whose graph is replanned records nothing and saves nothing.
At this head that is not an occasional turn: capture_workflow is self.mode == "task" and task is the default generation mode, so any replanned graph under agentHarness: generate forfeits its capture.
That may well be intended, since the capture notice does say the platform saves only after a clean success. What the model is not told is that accepting a replan forfeits the capture for the whole turn, which is a decision it takes while holding the opposite expectation.
Code-reading.
| "description and the node schema alone. " | ||
| ) | ||
| capture_note = "" | ||
| from raven.playbook.run_record import workflow_capture_requested |
There was a problem hiding this comment.
Question - should the shared dispatch tool import raven.playbook at all?
This import, and the two at lines 1173 and 1992, are the first edges from raven/agent/subagent/ into raven.playbook: git show <merge-base>:raven/agent/subagent/dag_tool.py | grep raven.playbook returns nothing. raven/agent/tools/*_playbook.py and raven/agent/loop/ already had that edge, but those are the Playbook feature's own files, whereas dag_tool.py is the dispatch tool behind every shipped product.
This one in particular sits inside the description property, so every render of the DAG tool's description enters the Playbook package, on every install, whether or not Playbooks are enabled.
I ran the import contracts at this head: 10 kept, 0 broken. No contract covers this direction, so it will not surface in CI either way. Raising it as a layering question rather than as a defect.
|
Blocking: 1 finding needs correction before this revision can merge; see the inline notes. Scope: I reviewed only the What this PR does, as I read the diff: it merges the per-turn generated Harness and the stored Workflow into one schema-v2 artifact that may carry either dimension or both, and teaches the loop to create one automatically out of a turn that succeeded. The agent slice is small in volume, 330 of 3936 added lines, but it is the whole mechanism by which a turn is observed and captured. Critical-path sign-off: I examined the in-scope diff at a59bd43, both callers of Confirmed findings:
Verification at a59bd43:
Checked and deliberately not reported:
|
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: correct the stale discovery contract and preserve foreground results when capture overrides background execution.
I verified and closed two of my three original threads: Workflow compilation now rejects instruction rewrites unless declared defaults reconstruct the accepted prompt exactly, and resolver selection state is task-local (and currently has no non-None production writer).
My architecture thread remains open because the later task/persona rewrite removed saved-candidate selection while CONTEXT.md still says the pre-turn setup model receives ranked candidates and may select and bind a saved Harness. The new Harness, Workflow, and Run Record definitions themselves are present.
The separate open blocker from LivXue also still applies on this head: capture forces SubAgentDagTool into foreground mode, but PlaybookExecutor formats the response from its configured background flag, discarding the completed result and promising a later delivery that cannot occur. I confirmed that control flow in both callers; I am not re-grading or duplicating that reviewer’s finding.
Verification:
- Focused Playbook, Harness, charter, and turn suite: 541 passed.
- Broader selection including DAG runner: 838 passed, then 2 failed because this environment lacks the optional everos-memory plugin; the failures are in stacked-base memory-record tests, after which -x stopped collection.
- git diff --check github/feat/harness_generation_capabilities...HEAD: passed.
- source-language gate: failed on newly added Chinese real-model fixtures. This is CI/linter-detectable and therefore not raised as a review finding.
Coverage this round: AGENTS.md and canonical CONTEXT.md rules, refreshed base-to-head diff and range history, the three author responses, affected compiler/runtime/DAG callers, backward compatibility, test changes and potential weakening, and the architecture constraints. I also reviewed the live 59afcad persona-function delta after GitHub advanced during the turn and found no additional defect meeting the later-round blocker bar.
main restructured ui-web while this branch was out: the legacy shell/bridge, demo/ and live/ layers are gone and a page is declared once in state/pages.ts with its domain in features/<domain>/manifest.ts. Twenty-two conflicts, ten of them modify/delete over files main removed. How they were resolved: - the legacy layers and the pages built on them are accepted as deleted; - ui-web/build.py, main.tsx, page.html and features/rail/store.ts take main's side, since the structures this branch changed no longer exist; - rpc/generated.ts is regenerated from the merged contract rather than merged; - agent_generator.py keeps this branch's split of the task selector from the persona one, so main's additions to the task worker schema are not carried over. That divergence is real and is for #568 to reconcile with main; - agent_spec.py keeps the coordinator seat, and raven_loop.py and runtime.py take main's additions. The persona island is not ported here, so this commit does not build on its own; the commit after it moves the island onto main's architecture. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
|
Closing: the work in here is carried by #709, which supersedes it. Every file this branch adds is present there -- This branch is ~300 commits behind main with 14 open review comments against a shape that has since moved, so reviving it would mean resolving that drift to arrive at what #709 already has. The review findings raised here were carried forward: the |
…as (#709) ## Summary The engine half of Personas. A stored Playbook could hold a worker table and a turn could generate one, but nothing let a person describe the assistant they wanted, keep it, and come back to it. **Schema v2** separates three concepts. A reusable artifact carries a **Harness**, a **Workflow**, or both: the Harness is the durable worker table bound into a turn, the Workflow is the DAG compiled from one accepted successful run, and a **Run Record** is the per-execution evidence, kept beside the artifact and never loaded as one. Schema v1 keeps loading through the `dag` and `prompt` modes. **A Harness gains a coordinator.** `_SeatPlaybook` carries the four Harness roles and `CoordinatorPlaybook` / `SubPlaybook` are the two seats that wear them, so the main seat gets the surface a worker already had rather than a parallel one. A workers-only payload is normalized on the way in, so Personas written before this keep loading. Two demands are contract rather than prose: a named missing-input gate must be a generated `intake` on the coordinator, and a boundary depending on tool-call arguments must be a `judge` on the seat making the call. Host Raven is excluded from the persona roster at the candidate layer. **Binding needs no new process.** `bind_charter_for_turn` is the counterpart of `bind_delegate_for_turn`, so `load_playbook` activates a stored Persona's main seat in the same call that activates its workers. What a session binds is a snapshot rather than a name to look up later, carried in the session's own metadata, so editing or deleting the library entry does not rewrite or break a conversation already running on it. **RPC**: `session.create` takes a Harness to open on, `session.set_harness` reports / binds / unbinds, and `playbooks.list` grows a `coordinator` flag. ## Scope change This PR was the whole feature; it is now **the backend only**. The persona page and the shell wiring are being reworked and will follow in their own PR, which is why they are not here. The two `rpc/generated.ts` files stay with this one because the `ui rpc contract` gate reads them against `openrpc.json`. Nothing here has a caller yet: the RPC surface grows, a Playbook gains a shape, and no existing path changes behaviour. This also supersedes #568, now closed -- every file it added is here. ## Type - [ ] Fix - [x] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification ``` ruff check / ruff format clean (1782 files) pytest (full) 27643 passed, 8 failed coverage_gate diff 91.34% (threshold 90) gen-rpc-client --check matches the contract (209 methods) commitlint / commit messages / source language / large files ``` The 8 failures are environmental and fail the same way on plain `main`: five proxy-credential redaction tests, the node-dir resolver, the subagent read test, and one runtime-checkpoint timing test that passes three times out of three when run on its own. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed ## Review history carried over Both blocking threads raised against the earlier combined branch were answered and closed before the split: the refinement poll that accepted the draft it started on, and the staged Persona that outlived the draft it was made for. Both fixes live in the page, so they travel with the frontend PR. LivXue's three non-blocking notes are in this half. One is addressed here: `default_generation_mode`'s `persona` value never reached the guard that mints one, and the field now says so instead of describing itself as a fallback that would. The behaviour is deliberate and unchanged -- read as a default it had a reader typing "hi" spend a model call on a Harness. The other two (the missing `error=` on a cancelled capture's run record, and the `set_preselected` path no producer ever writes) are left for a follow-up; the second one also needs `CONTEXT.md`'s discovery contract corrected with it. ## Risk No behaviour change for a turn that asks for nothing. Rollback is a revert of the merge commit; stored v1 Playbooks keep loading. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes Co-authored-by: yao pengfei <yaopengfei@evermind.ai> Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Summary
off | task | personaPlaybook generation modes, with a server default (playbooks.defaultGenerationMode) and a per-turn RPC override (playbook_mode)spawnandrun_subagent_dag; a successful Task DAG upgrades the same artifact with its reusable WorkflowChinese travel-assistant demo
A natural Chinese request, without internal agent names, generated and reloaded a
travel-conciergeHarness with four components:trip-planneron Raven for routes, budgets, health constraints, and input gatingcurrent-factson Raven-Research for current sourced factsbrief-editoron Raven-Design for an independently formatted six-section deliverablebooking-watchon Raven-OnCall for monitored deadlines and reminder windowsThe model generated an
intakefunction plus twojudgefunctions. Runtime checks proved that missing travel fields stop planning, unapproved or over-CNY-2800 bookings are refused, compliant bookings are allowed, and nonurgent reminders outside 18:00-21:00 are refused. The artifact saved and loaded successfully withworkflow: null; no trip, booking, payment, or Workflow ran during Persona creation.Safety and compatibility
offbypasses Playbook generation entirely and retains the default rosterintake,advise,judge, andsalvage; generated functions pass the AST allow-list before executionas,agent,prompt, andtoolsValidation
/usr/bin/npmLatest implementation commit:
59afcad2.