feat(playbook): route DAG nodes through generated harness - #558
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed github/main...HEAD and the prospective merge tree. Coverage included the repository rules and relevant CONTEXT.md Harness/Worker Table contracts; the diff; participant, DAG schema/preflight/runner/control, playbook-generation, and provider callers; branch history; backward compatibility for disabled/default playbook paths; and the test changes for weakening (none found). The participant-roster portion is already byte-identical on current main; the effective new merge delta is the DAG worker delegation, master-switch guard, and Codex Responses tool-choice conversion.
Verification:
uv run pytest tests/test_agent_playbook_delegate.py tests/test_openai_codex_provider.py tests/test_subagent_dag_core.py tests/test_subagent_dag_control_tools.py tests/integration/test_agent_playbook_e2e.py tests/integration/test_participant_roster_e2e.py -q- 293 passed.uv run pytest tests/test_participant_hook.py tests/test_subagent_dag_runner.py tests/test_subagent_dag_mcp_scope.py tests/test_plugin_runtime_binding.py tests/test_agent_loop_playbook_entry.py -q- 325 passed, 3 failed because the optionaleveros-memorydistribution is not installed; all three are the memory-recorder cases and the run reports that missing backend explicitly.uv run ruff checkon the effective changed Python files and tests - passed.uv run ty checkon the effective changed source files - passed.git merge-tree --write-tree github/main HEAD- clean.- GitHub CI - all reported checks pass.
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
This revision only merges current main. I compared it with the previously reviewed 3331bdc2ab20: the five-file PR diff is unchanged, the three authored commits match one-for-one in the range-diff, and the merge contains no conflict-resolution edits in the Harness, DAG, playbook-generation, or Codex provider paths. I rechecked the repository rules and relevant CONTEXT.md contracts, affected callers, history, backward-compatible disabled/default paths, architecture boundaries, and test changes; nothing new weakens coverage or changes the prior conclusion.
Verification on e74c12b71feb:
uv run pytest tests/test_agent_playbook_delegate.py tests/test_openai_codex_provider.py tests/test_subagent_dag_core.py tests/test_subagent_dag_control_tools.py tests/integration/test_agent_playbook_e2e.py tests/integration/test_participant_roster_e2e.py -q- 293 passed.- Completed GitHub checks are green; the four unit shards were still pending when reviewed.
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
This revision only merges the latest documentation-only main commit. I refreshed github/main and confirmed that the effective five-file PR diff and all three authored commits remain identical; the merge adds no conflict-resolution edits or interactions with the Harness, DAG, playbook-generation, or Codex provider paths. For this delta I rechecked repository rules, relevant architecture contracts, callers and history, backward compatibility, and test coverage; no tests were weakened and no prior conclusion changed.
Verification on f0ca4db7d0f3: the focused Harness, playbook delegation, Codex provider, DAG core/control, and integration suite passed 293 tests. Completed GitHub checks are green; four unit shards were still pending when reviewed.
|
Reviewed at head Tests, run against the PR tree: One substantive finding, four small ones, and some coverage gaps. 1. The playbook engine's own DAG dispatch inherits the turn's generated worker charter (CONFIRMED -- please answer before merge)
So the new block at Options: give the private playbook instance a flag that ignores the turn table; pass the table explicitly rather than reading a turn-scoped ContextVar in a seam two consumers share; or bind 2. Lower priority
3. Coverage
4. Process
Merge gate today: Checked and sound, so the list above is the whole of it: the label-resolution contract matches spawn's (unknown names pass through, the roster check is the refusal point); resolution happens before |
|
Blocking: stored playbook DAGs must not inherit a turn-generated worker charter. I independently confirmed the substantive path in comment 5750557822. Using the loop-built private playbook DAG tool under a generated This changes my standing stance from clean to blocking. The required outcome is that the private playbook executor dispatches its authored graph without consulting the turn-generated worker table; the implementation choice remains with the author. Verification: |
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: stored playbook DAGs still inherit the turn-generated worker charter.
This revision only merges the latest main change for on-call campaign billing. I compared the effective PR diff and commit range with f0ca4db7d0f3: all authored Harness/DAG/provider changes are identical, and the private playbook SubAgentDagTool still reaches the shared _preflight path that consults current_delegate(). Therefore the blocker confirmed in comments 5750557822 and 5750576974 remains unchanged on this head.
For this revision I checked the delta, affected call path, history/range-diff, repository and architecture constraints, backward-compatible paths, and tests for weakening. uv run pytest tests/test_playbook_executor.py tests/test_agent_playbook_delegate.py -q passed 94 tests; those tests still do not exercise the cross-path charter contamination.
35b9f17 to
3086ced
Compare
|
@LivXue @gloryfromca The blocking stored-Playbook contamination path is fixed on The public DAG still reads the turn Worker Table, while the private Playbook DAG is now constructed with an explicit empty Worker Table source. The regression test runs a stored Playbook through The lower-priority findings are also addressed: legacy Post-rebase verification: 613 relevant tests passed; repository-wide lint and changed-source type checks passed. The branch is rebased onto current |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The previous blocker is fixed: the stored Playbook's private DAG tool now injects an isolated worker-table source, and the collision regression verifies that neither the turn-only charter nor its payload reaches the stored node. This lifts my blocking stance.
I left one nonblocking compatibility follow-up inline for the production worker/MCP-adapter wrapper order. It is reproducible, but it requires the generated harness, DAG MCP adaptation, and a pre-authored_task third-party backend together, so it does not meet the late-round ordinary-reachability bar.
I covered AGENTS.md, CLAUDE.md, CONTEXT-MAP.md/CONTEXT.md, the effective diff, callers and construction paths, history/range-diff, backward compatibility, test weakening, and the Harness/Worker Table architecture constraints. git diff --check was clean. Focused runs passed 39 playbook-delegation tests and 85 executor/provider tests. The broader DAG/provider/third-party run reported 801 passed and 3 failed; all three failures are in untouched DAG-memory tests and report the unavailable everos-memory plugin in this environment.
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The compatibility fix is correct: _DispatchBackend now consumes authored_task, checks the wrapped backend before forwarding it, and the regression uses the production wrapper order. The former direct reproduction now succeeds.
For this revision I covered the delta and effective diff, the wrapper callers and construction order, history/range-diff, backward compatibility, test weakening, and the previously checked repository and architecture constraints. git diff --check was clean. The focused playbook/provider run passed 66 tests. The broader affected run reported 801 passed and the same 3 failures in untouched DAG-memory tests caused by the unavailable everos-memory plugin in this environment. My previously opened thread is resolved.
|
@LivXue Final head |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
This is a merge-only revision. The range-diff keeps all three authored commits unchanged, the combined merge diff contains no conflict-resolution changes, and the effective PR diff remains the same seven files reviewed at bfb94335.
For this round I checked the head delta, effective diff, merge/history shape, preservation of the previously reviewed callers and backward-compatibility fix, test changes for weakening, repository diff hygiene, and the standing Harness/Worker Table architecture constraints. git diff --check was clean, and uv run pytest tests/test_agent_playbook_delegate.py tests/test_openai_codex_provider.py -q -n 0 passed all 66 tests. My only prior thread remains resolved.
|
Blocking: 1 finding needs correction before this revision can merge; see the inline note. Scope: I reviewed only the This is a re-review. I last reviewed head I ran this review at Closed since the last round
Confirmed finding
Out of scope, disclosed rather than filed
VerificationOn
Checked and deliberately not reported
|
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the outstanding schema-label thread needs its author to confirm the fix.
The code change itself addresses the reported defect: the Worker Table branch now replaces both the enum and its model-facing description, matching the established spawn contract, and the live schema reproduction shows the corrected instruction. I found no new code issue.
For this revision I checked the delta and effective diff, schema consumers (run_subagent_dag and resolve_dag_node), the spawn precedent, history/range-diff, backward compatibility, test weakening, repository diff hygiene, and the Worker Table architecture contract. git diff --check was clean, and the focused playbook/provider run passed all 66 tests.
LivXue's blocking thread remains open. Because it is not my finding, I have left resolution and any severity change to its author or a maintainer.
|
@LivXue The schema-label blocker is fixed on the current head |
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the outstanding schema-label thread still needs its author to confirm the fix.
This is a merge-only revision. The range-diff preserves all four authored commits, the combined merge diff contains no conflict-resolution changes, and the effective PR diff is unchanged from 4dffdf63, where the schema fix was verified.
For this round I checked the head delta, effective diff, merge/history shape, preservation of the schema fix and prior compatibility paths, test weakening, diff hygiene, and the standing Worker Table architecture contract. git diff --check was clean, and the focused playbook/provider run passed all 66 tests.
LivXue's blocking thread remains open with no confirmation on the fixed code. I have not resolved or downgraded someone else's finding.
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the outstanding schema-label thread still needs its author to confirm the fix.
This is a merge-only revision. The range-diff preserves all four authored commits and the effective seven-file PR diff. The merge has one manual resolution in CONTEXT.md; I checked it and it correctly retains the Worker Table contract alongside current main. No product-code conflict resolution altered the reviewed fix.
For this round I checked the head delta, effective and combined diffs, history/range-diff, the schema and compatibility paths, test weakening, repository rules/diff hygiene, and the Worker Table architecture terms. git diff --check was clean. Because current main changed subagent routing and registry code, I included that suite: 121 focused playbook/provider/routing tests passed.
LivXue's blocking thread remains open with no confirmation on the fixed code. I have not resolved or downgraded someone else's finding.
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the outstanding schema-label thread still needs its author to confirm the fix.
This is a merge-only revision. After refreshing github/main, the range-diff preserves all four authored commits and the effective PR diff remains the same seven files. The merged shutdown fix overlaps dag_tool.py outside the Worker Table/schema path and required no conflict resolution, so the verified schema fix remains intact.
For this round I checked the head and effective diffs, merge/history shape, the overlapping DAG lifecycle change, preservation of schema and compatibility paths, test weakening, diff hygiene, and the Worker Table architecture contract. git diff --check was clean. The expanded playbook/provider/DAG-runner/manager run reported 487 passed and the same 3 environment failures in DAG-memory tests caused by the unavailable everos-memory plugin.
LivXue's blocking thread remains open with no confirmation on the fixed code. I have not resolved or downgraded someone else's finding.
LivXue
left a comment
There was a problem hiding this comment.
Approving at bcc2d558.
Scope: I reviewed only the raven/agent/** portion of this PR - 2 of 7 changed files
(raven/agent/subagent/dag_tool.py, raven/agent/loop/wiring.py). The remaining 5 were read
as context but are not covered by this verdict.
This is my third round. Round 1 at f0ca4db7 carried seven findings; round 2, run at
bfb94335 and posted at b9ad5bb4, closed all seven and filed one Blocker. That Blocker is
now closed.
Why this round re-ran rather than inherited its predecessor: the head has taken three merges
of main since the fix commit 4dffdf63, and dag_tool.py differs from the version the fix
was verified on (+20 -6, main's TaskAdopter refusal change), with one manual conflict
resolution in CONTEXT.md. Everything below was re-derived at bcc2d558.
Findings, all closed at this head:
- The schema-label Blocker: fixed and guarded by a regression test. The reproduction and the
detail are in that thread. - Legacy
authored_taskforwarding: fixed in both wrapper orders, plus the case the earlier
rounds did not show - a backend that does declareauthored_taskstill receives it, so the
suppression did not overshoot. __getattr__recursion under copy, deepcopy and pickle: all six round-trips clean._convert_tool_choicededup and theCONTEXT.mdWorker Table definition: both landed.
Verification on bcc2d558:
- Reproductions ran against a tree extracted at this head, with that tree first on
sys.path. - Rendered the live
run_subagent_dagandresolve_dag_nodenode schemas, with and without a
bound Worker Table. - Mutation check: deleting the description branch fails
test_the_workers_replace_the_dag_roster_too(1 failed, 38 passed). - pytest: 66 passed on
test_agent_playbook_delegate.pyandtest_openai_codex_provider.py;
567 passed across the DAG core, runner, control-tools, MCP-scope and playbook-executor set. - CI at this head: 20 pass, 2 skipping, 0 fail.
Carried forward from round 2, unchanged and disclosed rather than filed:
raven/cli/playbook_commands.py:417 is a third SubAgentDagTool construction that does not
pass worker_table_for. It is unreachable today, since delegate_scope is entered at exactly
one place in the tree, but CONTEXT.md now states the isolation as a contract of this class of
tool, and that site satisfies it by environment rather than by construction.
Not a code finding, for the merge gate: four of the eight commits are still unattributed
(author.login null, unverified), so require_extra_approval_for_unattributed_changes applies
and this PR needs a second approval beyond this one.
Summary
spawn,run_subagent_dag, andresolve_dag_node, and resolve each label before roster and capability validation.spawn.playbooks.enabledas the master switch for Harness generation.Type
Verification
Commands and results:
uv run pytest tests/test_agent_playbook_delegate.py tests/test_playbook_executor.py tests/test_openai_codex_provider.py tests/test_subagent_dag_core.py tests/test_subagent_dag_control_tools.py tests/test_subagent_dag_runner.py tests/integration/test_agent_playbook_e2e.py tests/integration/test_participant_roster_e2e.py -q- 613 passed after rebasing onto currentmain.uv run pytest tests/test_agent_playbook_delegate.py tests/test_openai_codex_provider.py -q- 66 passed, including a blocking stored-Playbook DAG execution with a colliding turn worker label and a Codex request-body assertion.uv run ruff check .- passed.uv run ty check raven/agent/subagent/dag_tool.py raven/agent/loop/wiring.py raven/providers/openai_responses_provider.py- passed.uv run pytest -q- 23,523 passed and 122 skipped. Three failures were outside this PR diff; isolated reruns passed the research-tool case, while the CLI theme ANSI expectation and install-script PATH fixture remained reproducible and unrelated.CONTEXT.mdnow documents all live Worker Table surfaces and the stored-Playbook isolation boundary. No screenshots are needed because the public configuration shape and UI are unchanged.Risk
The generated Worker Table still narrows capabilities only; it cannot widen the target agent permissions. Public model-authored DAGs read the current turn table, while the private stored-Playbook DAG receives an explicit empty source. With no generated table, existing roster behavior is unchanged.
Rollback is a revert of this PR. Operators can also disable the generated Harness path with
playbooks.enabled: false.Related Issues
N/A