Skip to content

fix(*): finish conditional branches in playbooks, the tasks board and the docs - #1064

Merged
0xKT merged 19 commits into
mainfrom
fix/dag_branch_followups
Oct 27, 2026
Merged

0xKT merged 19 commits into
mainfrom
fix/dag_branch_followups

Conversation

@LivXue

@LivXue LivXue commented Oct 22, 2026 •

Copy link
Copy Markdown
Member

Summary

Items 1 to 6 of #1008, the follow-ups the review of #1001 left out. Item 7, file references on Windows, is handled separately.

  1. A decision node's route is no longer the first thing cut on a tasks-board card. The agent's name kept its full width (min-width: auto) and the tool-count chip never shrinks, so the selection took all of the line's overflow: with "Raven-Research" and "3 tools" a 196px card showed only the arrow. The selection now renders as the arrow with its first route, which keeps its width, then the routes after it. Those, and the instance handle, start from nothing and grow into the room the line has left (flex: 1 1 0; max-width: max-content), and the agent's name gives way after them, with an ellipsis. The later routes keep room for a letter and an ellipsis: narrower, a line's first character is clipped rather than replaced by one. The 6px spacing separates fields only: the two selection spans have no additional gap, and the comma-space separator stays with the first route, outside the text that can be cut. A card with no selection lays out as before, a long minted handle included.
  2. Route conditions are filled and validated for params. The executor filled ${params.X} only in promptTemplate, and validate_graph_nodes checked only promptTemplate and inputs, so "the deal is above ${params.threshold}" reached the judge as written and ${params.nobody} passed validation. Conditions now go through the same _fill_text as the prompt, a secret withheld with the same log line, naming the route, and a reference to no declared param is an error. The filling and the check are on main now, carried by fix(playbook): fill and declare-check a route condition's params #1058; this branch holds the executor's docstring and the tests that pin both.
  3. Hand-written route names yes, no, on, off, true, false and bare numbers load. The store parsed the spec block with yaml.safe_load; YAML 1.1 read those keys as booleans or integers, validation refused them, and the runtime dropped the playbook from the library. Every mapping is now keyed by a name, and the value keeps its YAML type. The loader is the one fix(playbook): read yaml mapping keys as the text they were written as #1057 drafted, so the store carries one key loader rather than two: a node that is not a mapping is left to PyYAML to answer, which keeps the error the validate paths catch, and a key node is retagged through a copy, so an anchor that shares one scalar between a key and a value leaves the value's type alone. fix(playbook): read yaml mapping keys as the text they were written as #1057 is closed as superseded by this branch.
  4. The generator's dag-mode instructions describe branches. That section called dependsOn the whole graph language and never named routes, select or when; it now describes a decision node and the rule that a node depending on one names it in its when, and says a condition may use a param. A new test reads that section on its own.
  5. An ordinary node's judge request is held to a literal golden. test_judge_without_routes_sends_the_old_request_verbatim compared the request with the module's own constants, so changing the instruction or a tool schema's description passed. It now compares the whole request -- messages, tools, model, tool choice -- with the fence nonce fixed, against tests/data/dag_verdict_plain_request.json. The golden is the request from before feat(*): let a dag decision node choose which of its branches run #1001: the tree before it and this one send byte-identical requests over 32 input combinations.
  6. Domain terms around branches. CONTEXT.md gains three _Avoid_ notes: "decision" alone for a decision node or for what it chose, since the word already names the main agent's answer to an exception report in a DAG run and a choice the host puts to the user; "skipped" for a node a branch left out; and "branch" for any other part of a graph. The uses those notes retire are rewritten in the files this pull request touches: the DAG orchestration skill (the abandon paragraphs and the conditional-branch section), the orchestration and playbook pages in both languages, a runner docstring, and test docstrings, one test helper and one test name; two ui-tui test comments that called a path through a drawn graph a branch now say path and edge. The playbook page's "not a runtime branching language" had read, since feat(*): let a dag decision node choose which of its branches run #1001, as saying a prompt-mode playbook cannot branch; it now says the guidance decides nothing while the graph runs, and a branch is chosen by a decision node in the graph it composes.

Left as they are, outside the files this pull request touches: "decision" alone still names a decision node or its selection in text a model reads -- the routes description in the run_subagent_dag schema and on DagNodeSpec, which the playbook generator's tool carries; the guard-size error in raven/agent/subagent/dag_guard.py; the note raven/agent/subagent/dag_render.py writes into a sub-agent's prompt; the workflow compiler's prompt -- and in the RPC model's field description and both generated clients, the "Decision missing" reason in i18n/messages.json, the DAG Panel entry of ui-tui/CONTEXT.md (which test_the_tui_glossary_says_what_the_turned_in_line_carries quotes), and docstrings, comments and test names across the DAG modules and both UIs. Retiring those reaches the RPC schema, the generated clients, a translated label and pinned tests, so it is left to #1065. Python test function names, which predate the note, were left alone in these files too.

Two things the review of this pull request left open are closed here:

  • The card's layout test now lives where a real-environment test belongs. tests/test_web_task_card_route_real_browser.py measured a card in a real Chromium, so it moved to tests/integration/test_web_task_card_real_browser.py, and a new web-browser CI job runs it on a runner that ships a system Chrome. It is the only place the tasks board card's agent line is measured: happy-dom lays nothing out, so the stylesheet gate beside it can only read the declarations. Without a browser it skips with the reason, except under CI, where a skip would leave the line unmeasured on every push and so fails instead.
  • The store's loader answers a node that is not a mapping the way PyYAML does. raven playbook validate now answers a YAML syntax error in a spec block as a finding, the way the RPC validator already did, instead of a traceback.

The branch is rebased onto main at 471842132 (#1114). The CHANGELOG.md conflict that rebasing raised is resolved by keeping both sides' entries, and the branch merges clean.

Type

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

Verification

  • Relevant tests pass locally
  • Relevant lint / type checks pass locally
  • User-facing docs or screenshots are updated when needed

Native Windows 11, CPython 3.12.13:

  • uv run --frozen --no-sync pytest tests/test_playbook_store.py tests/test_playbook_executor.py tests/test_playbook_validate.py tests/test_subagent_dag_verdict.py tests/test_subagent_dag_core.py tests/test_cli_playbook_commands.py tests/integration/test_web_task_card_real_browser.py -m "integration or not integration" -q -p no:randomly --tb=short: 770 passed, 11 failed, 15 skipped. The same 11 fail at the merge base on this host, with the same IDs: test_playbook_executor.py::test_a_playbook_run_s_node_attempts_nest_under_the_call_that_ran_it, test_playbook_validate.py::test_path_confinement_waits_for_dispatch_roots, test_subagent_dag_core.py::test_replanning_refuses_a_redeclared_id, test_subagent_dag_core.py::test_a_refused_submission_still_records_the_link_as_unstarted, the five test_subagent_dag_core.py::test_a_reference_landing_in_either_root_is_accepted parameters, and test_cli_playbook_commands.py::test_run_judges_its_nodes_so_a_branching_playbook_takes_its_branch and ::test_run_judges_on_the_users_own_subagent_dag_settings. The first group is the file-reference rules fix(agent): resolve DAG file references by the host's path rules #1066 fixes, and the last two shell out to cat. The branch adds the passing cases.
  • The baseline comparison above, run over the same six files at the merge base: 746 passed, 11 failed, 15 skipped -- the same failing IDs.
  • uv run --frozen --no-sync pytest tests/integration/test_web_task_card_real_browser.py -m integration -q -n 0 -p no:randomly --tb=short: 10 passed, the board's real 196px card measured in a real Chromium. Playwright could not create its artifacts directory under this host's default temp path, so the run needed a writable TEMP; with the default one the nine browser cases skip and the path-lookup case still passes.
  • After the rebase onto main at 471842132, with -n 0: the six playbook/DAG files (tests/test_playbook_store.py, tests/test_playbook_executor.py, tests/test_playbook_validate.py, tests/test_subagent_dag_verdict.py, tests/test_subagent_dag_core.py, tests/test_cli_playbook_commands.py): 790 passed; tests/integration/test_web_task_card_real_browser.py -m integration: 10 passed; scripts/check_commit_messages.py, scripts/check_source_language.py and scripts/check_large_files.py over origin/main..HEAD, and git diff --check: passed.
  • uv run --frozen --no-sync ruff check raven/playbook/store.py raven/cli/playbook_commands.py tests/test_playbook_store.py tests/test_cli_playbook_commands.py tests/integration/test_web_task_card_real_browser.py and ruff format --check over the same files: passed.
  • uv run --frozen --no-sync ty check raven/playbook/store.py raven/cli/playbook_commands.py: passed.
  • uv run --frozen --no-sync lint-imports: 10 kept, 0 broken.
  • uv run --frozen --no-sync --extra dev python scripts/check_commit_messages.py origin/main..HEAD, scripts/check_source_language.py origin/main..HEAD and scripts/check_large_files.py origin/main..HEAD: passed.
  • git merge-tree against origin/main: no conflict, through --write-tree on the Windows host and through the three-argument form on the Linux workstation, whose git predates --write-tree. Before the rebase the same check reported one conflict, in CHANGELOG.md.

ui-web, run in ui-web/:

  • node node_modules/vitest/vitest.mjs run src/features/tasks/BoardCard.test.tsx scripts/gates/task-card-selection-css.test.mjs --no-file-parallelism: 2 files, 36 tests passed.
  • node node_modules/vitest/vitest.mjs run src/features/tasks scripts/gates --no-file-parallelism --testTimeout=60000: 62 files, 560 tests passed. The same command over main at the time of the run reported 61 files and 552 tests passed; the branch adds the stylesheet gate and the cases beside it. Without the longer timeout two of the gates time out on a loaded host; they pass on their own and the files they read are byte-identical to main.

At head 211d226d, the branch's CI run: 23 checks passed, 2 skipped (claude, docs deploy), 0 failed. The rebased head re-runs the same workflow, including the web layout in a real browser job, which runs the browser cases on a runner that ships a system Chrome.

Not run here: macOS, a browser other than Chromium, and the full repository suite.

Risk

A stored playbook whose route condition names an undeclared param now fails validation, and so leaves the library, as one doing the same in promptTemplate already did. On the board, a crowded agent line now shortens the agent's name where it used to cut the selection. The store's loader now reads the frontmatter through the same path as the spec block; a frontmatter key that YAML 1.1 would have resolved to a boolean or a number is now read as the text it was written as, which the frontmatter's two string fields never were.

  • Security impact considered
  • Backward compatibility considered
  • Rollback path is clear for risky changes

Related Issues

#1008 (items 1 to 6)

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: multi-route selections now render an extra flex gap between their first and later routes.

I reviewed github/main...HEAD at 37a9a6866590. The inline tasks-board finding is the only blocker I found.

Coverage included the repository rules in AGENTS.md, the canonical runtime and Web UI terminology in CONTEXT-MAP.md / CONTEXT.md / ui-web/CONTEXT.md, the complete diff and commit history, surrounding playbook execution/validation/storage and board-card code, backward compatibility, test-strength changes, and the documented Runtime/Web UI boundary. I found no weakened tests or other architecture issue.

Verification:

  • uv run pytest tests/test_playbook_executor.py tests/test_playbook_store.py tests/test_playbook_validate.py tests/test_subagent_dag_verdict.py tests/test_subagent_dag_core.py -q: 709 passed.
  • Targeted Web UI Vitest run: 34 passed.
  • Targeted TUI Vitest run: 21 passed.
  • Web UI type-check and lint passed; lint reported four warnings on untouched files and no errors.
  • Ruff check and format check passed for the changed Python files.
  • Source-language, large-file, and git diff --check gates passed.

Comment thread ui-web/src/features/tasks/BoardCard.tsx Resolved
@LivXue

LivXue commented Oct 22, 2026

Copy link
Copy Markdown
Member Author

No new Windows-specific regression found in the inspected changes.

Reviewed head 37a9a6866590b6962974be28b918e296dbdb4d20, base/merge base 4315993300e538ec954c79565cbc8585006c8053. Read the complete 24-file local diff, playbook loading/filling/validation callers, DAG skill and judge fixture, board component/CSS, TUI changes, tests, and discussions.

Native Windows results (Python 3.13.12, Node 25.9.0):

  • The four-file Python run stopped at 125 passed, 1 failed: tests/test_playbook_validate.py::test_path_confinement_waits_for_dispatch_roots. The same test also fails on the exact base with the same absolute Windows path being rejected by check_confined. This is inherited, rather than introduced by the route-condition changes; the separate #1066 Windows reference fix addresses it.
  • With only that confirmed baseline failure deselected: 284 passed, 1 deselected. This includes YAML key/merge parsing, parameter substitution/secret withholding, and the UTF-8 judge golden.
  • Board component and stylesheet-gate tests: 34 passed. These DOM/CSS checks do not measure browser layout.

The existing multi-route flex-gap finding remains a separate general review issue; it is not re-filed here. This comment is not a merge verdict.

Limitations: no full graph/repository suite, live model/remote dispatch, or browser visual check. Dependencies and tests ran only in detached temporary checkouts, with isolated profile/Raven/OAuth state.

Exact verification commands
uv sync --frozen --python 3.13 --extra dev
# native; exit 1; cwd C:\Users\livxu\AppData\Local\Temp\raven-windows-review\07e3ab35a8ca4ce8a79558f02fad7b30\pr1064\head
uv run --frozen --no-sync pytest tests/test_playbook_executor.py tests/test_playbook_store.py tests/test_playbook_validate.py tests/test_subagent_dag_verdict.py -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\07e3ab35a8ca4ce8a79558f02fad7b30\checks\1064-native\pytest' --junitxml 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\07e3ab35a8ca4ce8a79558f02fad7b30\checks\1064-native\junit.xml'
# base-reference; exit 1; cwd C:\Users\livxu\AppData\Local\Temp\raven-windows-review\07e3ab35a8ca4ce8a79558f02fad7b30\base431599
uv run --frozen --no-sync pytest tests/test_playbook_validate.py::test_path_confinement_waits_for_dispatch_roots -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\07e3ab35a8ca4ce8a79558f02fad7b30\checks\1064-base-reference\pytest' --junitxml 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\07e3ab35a8ca4ce8a79558f02fad7b30\checks\1064-base-reference\junit.xml'
# native-rest; exit 0; cwd C:\Users\livxu\AppData\Local\Temp\raven-windows-review\07e3ab35a8ca4ce8a79558f02fad7b30\pr1064\head
uv run --frozen --no-sync pytest tests/test_playbook_executor.py tests/test_playbook_store.py tests/test_playbook_validate.py tests/test_subagent_dag_verdict.py --deselect=tests/test_playbook_validate.py::test_path_confinement_waits_for_dispatch_roots -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\07e3ab35a8ca4ce8a79558f02fad7b30\checks\1064-native-rest\pytest' --junitxml 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\07e3ab35a8ca4ce8a79558f02fad7b30\checks\1064-native-rest\junit.xml'
# cwd: isolated head checkout/ui-web
npm.cmd ci --ignore-scripts --no-audit --no-fund
node node_modules/vitest/vitest.mjs run src/features/tasks/BoardCard.test.tsx scripts/gates/task-card-selection-css.test.mjs --reporter=default --reporter=json --outputFile.json="C:/Users/livxu/AppData/Local/Temp/raven-windows-review/07e3ab35a8ca4ce8a79558f02fad7b30/pr1064/web-results.json"

@LivXue
LivXue requested a review from gloryfromca October 22, 2026 16:45

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; this can merge as far as I am concerned.

Reviewed the 37a9a6866590..aa1eaffed389 delta together with the prior full review and discussion record. The new CSS removes the unintended inter-span gap while retaining the intended shrink priorities, and the regression test now checks computed field spacing, zero margin before the remaining routes, and separator preservation. My only prior finding is fixed; I replied and resolved that thread. No new compatibility, architecture, repository-rule, or test-strength concern was introduced.

Verification:

  • npm test --prefix ui-web -- --run src/features/tasks scripts/gates --no-file-parallelism: 61 files, 554 tests passed.
  • Web UI type-check passed.
  • Web UI lint passed with four warnings in untouched files and no errors.
  • git diff --check github/main...HEAD passed.

Comment thread ui-web/src/features/tasks/styles.css Resolved
@0xKT

0xKT commented Oct 22, 2026

Copy link
Copy Markdown
Member

Not a blocker (review of aa1eaffe, main 43159933); the blocking item is in the thread on ui-web/src/features/tasks/styles.css. Two things, re-checked at the head:

Severity of this one finding, not a verdict on the pull request. Any blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text.

  1. _SpecBlockLoader flattens before checking the node kind. A !!map or !!set tag on a non-mapping (routes: !!map [a, b]) now raises a bare TypeError ("cannot unpack non-iterable ScalarNode object") from construct_mapping (raven/playbook/store.py:67-73), where yaml.safe_load raised a located ConstructorError. playbooks.validate catches yaml.YAMLError but not TypeError, so that file gets an internal error instead of ok=False. Returning super().construct_mapping(node, deep=deep) for a node that is not a yaml.MappingNode, before flattening, keeps PyYAML's own message. An unusual input, hence not blocking.
  2. Overlap with fix(playbook): read yaml mapping keys as the text they were written as #1057 and fix(playbook): fill and declare-check a route condition's params #1058. This PR changes the same spec-block read as fix(playbook): read yaml mapping keys as the text they were written as #1057 (a different key loader) and the same route-condition fill and check as fix(playbook): fill and declare-check a route condition's params #1058. Whichever lands first, the others need a rebase, and if both this PR and fix(playbook): read yaml mapping keys as the text they were written as #1057 landed as they are the store would carry two key loaders.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: the first selected route must truncate inside the agent line once the agent name has given way.

The new Chromium measurements in the open styles.css thread invalidate my earlier clean stance. The CSS mechanics support them: .tkrunsel is non-shrinking, and after .tkrunag reaches zero the fixed chip plus first route can still exceed the 166px line. The overflow then belongs to .tkrunl2, so the route span itself does not overflow and cannot render its ellipsis; the parent clips it mid-letter while later routes sit outside the line. I am not re-grading or resolving the other reviewer's finding, only updating my own stance on the whole record.

Verification: npm test --prefix ui-web -- --run src/features/tasks/BoardCard.test.tsx scripts/gates/task-card-selection-css.test.mjs passed 35 tests. Those tests use happy-dom and confirm declarations, not browser layout. A local Chromium reproduction could not launch because this environment lacks libatk-1.0.so.0; the failure occurred before any page loaded.

@LivXue

LivXue commented Oct 22, 2026

Copy link
Copy Markdown
Member Author

Windows review: the existing board-card layout blocker reproduces natively; no additional Windows-specific regression found in the inspected changes.

Reviewed head aa1eaffed389b46fc84fa00830045e1adea55b98, base/merge base 4315993300e538ec954c79565cbc8585006c8053: complete 24-file diff, playbook loading/fill/validation and judge callers, DAG skill/golden, board component/CSS, TUI comment changes, tests and discussions. The delta since the previous reviewed head changes only the board component, its test and CSS.

Existing P2 layout issue, with new native Windows evidence: the open route-overflow thread, ui-web/src/features/tasks/styles.css:743-782. At the actual 196x78 card size (166px agent line), three-tool chips plus ordinary route names exhaust the line. The non-shrinking first route and fixed chip leave the agent name at 0px; the parent clips the route rather than letting the route's own ellipsis render. Later routes can be entirely outside the line. Allow the first route to truncate within the remaining line width after the higher-priority shrinking items are exhausted, and guard this with browser geometry assertions.

Native Chromium, using SSR of the actual component and production styles:

Routes, with 3 tools Base agent width Head agent width Head first-route overflow
feature_request, docs 30.59px 0px 13.875px
needs_clarification 30.59px 0px 26.172px

The short-route separator check passes with a 0px inter-span box gap. The browser probe reports 1 passed, 1 failed on the head; the containment check passes against base component/CSS snapshots. This adds Windows reproduction evidence to the existing finding rather than filing it again. The base control redirects only the component's two helper imports to unchanged helpers and shares the unchanged page stylesheet; it is not a full application run.

Other native results (Python 3.13.12, Node 25.9.0): 284 Python cases passed, 1 deselected; 554 Web cases passed across 61 files. The deselected test_path_confinement_waits_for_dispatch_roots is the already confirmed baseline Windows file-reference failure, tracked by #1066; the Python files and base SHA are unchanged since that review. Green DOM/CSS tests do not contradict the browser failure. The existing unusual-YAML-input note remains a separate general issue.

Limits: no full repository suite, live judge/model/remote dispatch, interactive TUI, other browser engines or display-scaling matrix. This Windows review does not clear the open layout blocker or give a general merge verdict.

Exact verification commands

All work used the detached head checkout and isolated profile/Raven/OAuth state. Browser probes are external scratch files; base component/CSS snapshots were extracted with git show 4315993300e538ec954c79565cbc8585006c8053:<path>.

uv sync --frozen --python 3.13 --extra dev
# native; exit 0
uv run --frozen --no-sync pytest tests/test_playbook_executor.py tests/test_playbook_store.py tests/test_playbook_validate.py tests/test_subagent_dag_verdict.py --deselect=tests/test_playbook_validate.py::test_path_confinement_waits_for_dispatch_roots -n 0 -x --tb=short --basetemp C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\checks\1064-native\pytest --junitxml C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\checks\1064-native\junit.xml

# browser; exit 1
$env:RAVEN_REVIEW_BOARD_HTML="C:/Users/livxu/AppData/Local/Temp/raven-windows-review/76440228003d458eb50cf2d4a3438c5e/probes/board.html"
uv run --frozen --no-sync pytest -c C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\probes\pytest.ini C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\probes\test_board_geometry.py -n 0 -x --tb=short --basetemp C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\checks\1064-browser\pytest --junitxml C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\checks\1064-browser\junit.xml

# browser-base; exit 0
$env:RAVEN_REVIEW_BOARD_HTML="C:/Users/livxu/AppData/Local/Temp/raven-windows-review/76440228003d458eb50cf2d4a3438c5e/probes/board-base.html"
uv run --frozen --no-sync pytest -c C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\probes\pytest.ini C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\probes\test_board_geometry.py::test_ordinary_route_names_with_a_chip_stay_inside_the_line -n 0 -x --tb=short --basetemp C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\checks\1064-browser-base\pytest --junitxml C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\checks\1064-browser-base\junit.xml

# cwd: isolated head checkout/ui-web; exit 0; 554 passed
npm.cmd ci --ignore-scripts --no-audit --no-fund
node node_modules/vitest/vitest.mjs run src/features/tasks scripts/gates --no-file-parallelism --reporter=default --reporter=json --outputFile.json="C:/Users/livxu/AppData/Local/Temp/raven-windows-review/76440228003d458eb50cf2d4a3438c5e/checks/1064-web/results.json"
node C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\probes\render-board.mjs C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\pr1064\head\ui-web C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\probes\board.html
node C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\probes\render-board.mjs C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\pr1064\head\ui-web C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\probes\board-base.html C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\probes\base-card.tsx C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\probes\base-tasks.css

Comment thread ui-web/src/features/tasks/styles.css Resolved
@LivXue
LivXue force-pushed the fix/dag_branch_followups branch from aa1eaff to b39d89a Compare October 23, 2026 06:38

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; this can merge as far as I am concerned.

Reviewed the new tasks-card delta at b39d89a25695 against the prior blocker and the current PR diff. The agent now has a non-shrinking 6ch floor, the first route can shrink to an ellipsis with no intrinsic floor, and handles/later routes retain their lower-priority zero bases. The added Chromium integration test exercises the real 196px card width with chips, long routes, long agent names, handles, overflow boundaries, and the intended shrink order. I found no new compatibility, architecture, repository-rule, or test-strength issue. Threads opened by other reviewers remain for those reviewers to resolve.

Verification:

  • npm test --prefix ui-web -- --run src/features/tasks scripts/gates --no-file-parallelism: 61 files, 555 tests passed.
  • uv run pytest tests/integration/test_web_task_card_route_real_browser.py -q -o addopts="": 8 skipped because local Chromium cannot load libatk-1.0.so.0; collection passed, and this skip behavior matches the repository's existing real-browser test fixture.
  • Web UI type-check passed.
  • Web UI lint passed with four warnings in untouched files and no errors.
  • Ruff check/format, source-language, large-file, and diff checks passed for the PR against GitHub's current base de8891f2df20.

@LivXue

LivXue commented Oct 23, 2026

Copy link
Copy Markdown
Member Author

Windows compatibility re-review at head b39d89a2569599450c8ccad57577dda345a619c3, base/merge base de8891f2df206528b41fe1c5e3fa00872220f5af.

Read the complete 25-file diff, playbook loading/filling/validation and DAG callers, skill/prompt changes, judge golden, board component/CSS, TUI changes, tests, and current discussions. No new Windows-specific regression was found in the inspected changes.

Native Windows results (Python 3.13.12, Node 25.9.0):

  • The previously reproduced card overflow no longer reproduces in the tested cases. All 8 new Chromium layout cases passed, and the original 2 SSR geometry probes passed using the actual current BoardCard and production styles. The agent line remained 166px wide; both long-route cases retained a 30.59px agent name with no first-route overflow. The multi-route separator still has a 0px box gap.
  • 284 Python cases passed, 1 deselected, covering route filling/secret withholding, YAML key and merge handling, validation, and the UTF-8 judge golden. The deselected test_path_confinement_waits_for_dispatch_roots was separately run on both this head and the exact current base: 1 failed on each, with the same Windows absolute-path confinement failure. This remains inherited, as in the prior Windows report, rather than a new finding here.
  • 555 Web tests passed across 61 files (tasks plus stylesheet gates). All 4,667 checkout files and Git modes/tree matched this exact head, all 25 changed files were rechecked, and git diff --check passed. The runner's executable AST is unchanged after removing standalone documentation strings.

Limits: UTF-8 mode was enabled. Chromium checks used the host's default fonts/scale; no other browser/display matrix, interactive TUI, live judge/remote dispatch, Python 3.12, or full repository suite was run. These scoped results are not a general merge verdict.

Exact verification commands and results

Each Python invocation used an isolated uv environment and temporary profile/Raven/OAuth/temp directories. UTF-8 mode was enabled and imports were asserted to belong to the selected checkout.

$run = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88'
$env:UV_PROJECT_ENVIRONMENT = Join-Path $run 'python-env'
$env:UV_PYTHON = '3.13'
$env:PYTHONUTF8 = '1'

# 1064-reference-head: 0 passed, 1 failed, 0 skipped; exit 1
Set-Location -LiteralPath 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\pr1064\head-tree'
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\pr1064\head-tree' + ';' + (Join-Path $run 'probes')
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/test_playbook_validate.py::test_path_confinement_waits_for_dispatch_roots' '-p' 'no:randomly' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\checks\1064-reference-head\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\checks\1064-reference-head\junit.xml'

# 1064-reference-base: 0 passed, 1 failed, 0 skipped; exit 1
Set-Location -LiteralPath 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/00d4296fd93f4a04a8194f0688ba9b88/pr1064/base'
$env:PYTHONPATH = 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/00d4296fd93f4a04a8194f0688ba9b88/pr1064/base' + ';' + (Join-Path $run 'probes')
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/test_playbook_validate.py::test_path_confinement_waits_for_dispatch_roots' '-p' 'no:randomly' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\checks\1064-reference-base\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\checks\1064-reference-base\junit.xml'

# 1064-python: 284 passed, 0 failed, 0 skipped; exit 0
Set-Location -LiteralPath 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\pr1064\head-tree'
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\pr1064\head-tree' + ';' + (Join-Path $run 'probes')
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/test_playbook_executor.py' 'tests/test_playbook_store.py' 'tests/test_playbook_validate.py' 'tests/test_subagent_dag_verdict.py' '--deselect=tests/test_playbook_validate.py::test_path_confinement_waits_for_dispatch_roots' '-p' 'no:randomly' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\checks\1064-python\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\checks\1064-python\junit.xml'

# 1064-browser: 8 passed, 0 failed, 0 skipped; exit 0
Set-Location -LiteralPath 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\pr1064\head-tree'
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\pr1064\head-tree' + ';' + (Join-Path $run 'probes')
uv 'run' '--frozen' '--no-sync' 'pytest' 'tests/integration/test_web_task_card_route_real_browser.py' '-p' 'no:randomly' '-m' 'integration' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\checks\1064-browser\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\checks\1064-browser\junit.xml'

# 1064-geometry: 2 passed, 0 failed, 0 skipped; exit 0
Set-Location -LiteralPath 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\pr1064\head-tree'
$env:PYTHONPATH = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\pr1064\head-tree' + ';' + (Join-Path $run 'probes')
$env:RAVEN_REVIEW_BOARD_HTML = Join-Path $run 'probes/board.html'
$env:RAVEN_REVIEW_GEOMETRY_JSON = Join-Path $run 'checks/1064-geometry/observations.json'
uv 'run' '--frozen' '--no-sync' 'pytest' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\probes\test_board_geometry.py' '-c' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\probes\pytest.ini' '-p' 'no:randomly' '-n' '0' '-x' '--tb=short' '--basetemp' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\checks\1064-geometry\pytest' '--junitxml' 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\checks\1064-geometry\junit.xml'
$bare = Join-Path $run 'objects.git'
git --git-dir=$bare diff --check de8891f2df206528b41fe1c5e3fa00872220f5af..d86f383b95a4d1230fb1de56e2fbe12c455d5a04 # exit 0
Node and source verification commands
# setup-web-1064: exit 0
Set-Location -LiteralPath 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\pr1064\head-tree/ui-web'
npm.cmd 'ci' '--ignore-scripts' '--no-audit' '--no-fund'
# 1064-web-tests: exit 0
Set-Location -LiteralPath 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\00d4296fd93f4a04a8194f0688ba9b88\pr1064\head-tree/ui-web'
npm.cmd 'test' '--' 'src/features/tasks' 'scripts/gates' '--no-file-parallelism'
# 1064-render: exit 0
node 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/00d4296fd93f4a04a8194f0688ba9b88/probes/render-board.mjs' 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/00d4296fd93f4a04a8194f0688ba9b88/pr1064/head-tree/ui-web' 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/00d4296fd93f4a04a8194f0688ba9b88/probes/board.html'
$env:RAVEN_REVIEW_RUN = $run
Set-Location -LiteralPath 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/00d4296fd93f4a04a8194f0688ba9b88/pr1064/head-tree'
uv run --frozen --no-sync python (Join-Path $run 'probes/check_runner_ast.py') # executable AST identical; exit 0

The browser file was initially deselected by the default marker filter; the recorded successful invocation explicitly selects integration tests.

@LivXue
LivXue requested review from 0xKT and gloryfromca October 23, 2026 07:54
@LivXue
LivXue force-pushed the fix/dag_branch_followups branch from b39d89a to 4e13602 Compare October 23, 2026 08:17

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; suggestions only, and they are marked inline.

Reviewed the new test-only delta at 4e13602cce9d after the rebase onto GitHub base 8a7ea1ccf6da. The production tasks-card fix is unchanged. The browser test now runs in the default CI selection, fails rather than silently skips when CI lacks Chrome, and covers the chip's non-shrinking width plus the priority and ellipsis of routes after the first. I found no product, compatibility, architecture, or test-strength regression.

Named nonblocking follow-up: tests/test_web_task_card_route_real_browser.py is a real-browser integration test but now lives under tests/, contrary to AGENTS.md section 5.2's requirement that real-environment tests live under tests/integration/. It should return there once an integration CI job is wired to execute it. I am not holding this late revision on that placement: the move exists because the current unit job is the job with system Chrome, local environments without Chrome have a working skip path, and the production fix remains improved.

Verification:

  • npm test --prefix ui-web -- --run src/features/tasks scripts/gates --no-file-parallelism: 61 files, 555 tests passed.
  • uv run pytest tests/test_web_task_card_route_real_browser.py -q: 9 skipped because neither Chrome nor Chromium is on local PATH; collection passed.
  • Ruff check/format, source-language, large-file, and diff checks passed against the current GitHub base.

@LivXue
LivXue force-pushed the fix/dag_branch_followups branch from 4e13602 to 1ef7f1a Compare October 23, 2026 09:10

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; suggestions only, and they are marked inline.

Reviewed 1ef7f1a079cf after its rebase onto merged #1058. The upstream route-condition implementation and validation remain intact; this branch now contributes complementary documentation and assertions, with no duplicate runtime path or compatibility change. The tasks-board production fix is unchanged. No new findings; the previously named nonblocking real-browser test-placement follow-up remains.

Verification:

  • uv run pytest tests/test_playbook_executor.py tests/test_playbook_validate.py tests/test_playbook_store.py -q: 180 passed.
  • npm test --prefix ui-web -- --run src/features/tasks scripts/gates --no-file-parallelism: 61 files, 555 tests passed.
  • Ruff check/format and git diff --check passed against GitHub base 2582e7936892.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: make the new Windows install-path lookup test portable so the default Linux test run passes.

I reviewed the complete delta from 1ef7f1a079cf to cf9c0398774e: the browser-discovery helper, its fixture caller, compatibility behavior, test coverage, and applicable repository test rules. No production code or architecture changed, and tests were strengthened rather than weakened. I tried to refute the candidate by running the focused file; it fails at the new lookup test with 1 failed and 9 skipped, so this revision cannot merge as-is.

This late-round blocker meets all three required conditions: this revision introduces it, the repository's ordinary Linux test run reaches it, and CI has no working path around a deterministic assertion failure.

Test: uv run pytest tests/test_web_task_card_route_real_browser.py -q -> 1 failed, 9 skipped.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; suggestions only, and they are marked inline.

Reviewed the complete delta from cf9c0398774e to 5c6cc63c1cf0, including the helper's fixture caller, cross-platform path behavior, history, backward compatibility, test strength, and applicable repository rules. The component tuple fix is portable and introduces no architecture or production-caller change. My blocker is fixed and its thread is resolved; both threads I opened on this pull request are now resolved.

The previously named nonblocking follow-up about eventually placing the real-browser test under the repository's integration-test structure remains; it does not hold this revision.

Verification: uv run pytest tests/test_web_task_card_route_real_browser.py -q -> 1 passed, 9 browser-dependent skips; uv run ruff check tests/test_web_task_card_route_real_browser.py -> passed.

@LivXue
LivXue force-pushed the fix/dag_branch_followups branch from 5c6cc63 to 6881f4a Compare October 23, 2026 10:05

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; suggestions only, and they are marked inline.

This revision is a clean rebase onto a7e43a077: git range-diff shows all 16 PR patches unchanged, so there are no conflict-resolution edits to introduce a new regression. I checked the current github/main...HEAD diff, patch history, affected callers, backward compatibility, test strength, applicable repository rules, and architecture boundaries. The three newly based main commits touch separate cron, RPC usage, and Mirasim-test areas; they do not overlap this PR's patch series.

The previously named nonblocking follow-up about eventually placing the real-browser test under the repository's integration-test structure remains; it does not hold this revision. Both threads I opened remain resolved.

Verification: affected Python suites -> 712 passed, 9 browser-dependent skips; Web UI BoardCard and stylesheet gate -> 36 passed; TUI DAG render/layout -> 21 passed.

@LivXue

LivXue commented Oct 23, 2026

Copy link
Copy Markdown
Member Author

Windows verification at head 6881f4a1c4306f8b9fae2502a282ef81e5821783: the portable install-path change and the real-browser layout cases pass natively on Windows.

I inspected the complete three-line delta from b39d89a2 and the current discussions. The changed suffixes are now component tuples joined under the host's temporary root, so the default test run reaches nested Chrome paths on both POSIX and Windows. No further Windows-specific defect remains in this delta.

Native Windows 11, Python 3.13.12 with PYTHONUTF8=1, Playwright Chromium:

  • uv run --frozen pytest tests/test_web_task_card_route_real_browser.py -q -o addopts='': 10 passed, exit 0. This includes the path-discovery branch and the Chromium layout cases at the board's real width.
  • uv run --frozen ruff check tests/test_web_task_card_route_real_browser.py: passed, exit 0.

The current head's CI is complete: 21 passed and 2 skipped, with no failure or pending job. The skips are claude and docs deploy.

This is scoped Windows evidence, not a review verdict. The two open layout threads remain for their openers to resolve.

@0xKT

0xKT commented Oct 24, 2026

Copy link
Copy Markdown
Member

Merge conflict: @LivXue this PR no longer merges onto main. git merge-tree --write-tree of main 28d44f9 with this head (6881f4a) stops with a conflict in:

  • CHANGELOG.md

main moved with #1071 (squash 28d44f9). A rebase onto main that keeps both changelog entries should be enough. The board re-checks it on the new head.

@gloryfromca

Copy link
Copy Markdown
Member

Blocking: rebase onto current main and resolve the CHANGELOG.md conflict.

I independently verified the new mergeability report after fetching github/main at 28d44f9ee38a: git merge-tree --write-tree HEAD github/main exits 1 with a content conflict in CHANGELOG.md (while CONTEXT.md auto-merges). This confirms the other reviewers blocker; I am not re-grading or duplicating it as my own finding.

The head itself is unchanged, so I did not repeat the already reported test runs. My stance changes from nonblocking to blocker solely because this revision cannot currently merge. The previously named code follow-up remains nonblocking.

LivXue added 17 commits October 27, 2026 02:57
The store parsed the spec block with yaml.safe_load, which follows YAML
1.1: route names yes, no, on, off, true, false and bare numbers became
booleans and integers, validation refused them, and the runtime dropped
the playbook from the library. The block is now read with a SafeLoader
that takes every mapping key as the text it is written as, keys a merge
brings in included; values keep their YAML types.
The dag-mode section of the generator's system prompt called dependsOn
the whole graph language and never named routes, select or when, though
the tool schema the generator fills offers all three. It now describes a
decision node, the rule that a node depending on one names it in its
when, and that a condition may use a param. A test reads that section on
its own.
test_judge_without_routes_sends_the_old_request_verbatim compared the
request with the module's own constants, so a change to the instruction
or to a tool schema's description passed. It now compares the whole
request, with the fence nonce fixed, against
tests/data/dag_verdict_plain_request.json: the request the code sent
before a node could select routes.
CONTEXT.md gains three _Avoid_ notes: "decision" alone for a decision
node or for what it chose, since a decision is the main agent's answer
to an exception report; "skipped" for a node a branch left out; and
"branch" for any part of a graph a route does not select. The uses they
retire are rewritten in the DAG orchestration skill, the orchestration
and playbook pages in both languages, a runner docstring, and test
docstrings, names and comments around them. The playbook page no longer
reads as saying a prompt-mode playbook cannot branch.
Since #1050, CONTEXT.md also uses "decision" for a choice the host puts
to the user, which stays open until they make it. The decision node's
_Avoid_ note said a decision is the main agent's answer to an exception
report, which read as the word's only sense; it now names both.
Space the agent-line fields without inserting a flex gap between the
first selected route and the remaining routes. Keep the comma and its
space with the first route so truncating the rest preserves its first
visible letter.

Check the rendered component with its production stylesheet and verify
the spacing in Chromium, including crowded and single-route cards.
The first selected route was flex: none, so on a 196px card with a tool
chip the agent's name was the only thing left to give: it collapsed to
0px and the route ran past the line, clipped mid-letter with no
ellipsis, the later routes wholly outside it.

The name now starts from 6ch (or its own width) and grows ahead of the
handle and the later routes, never shrinking below that floor; the first
route shrinks again, with its ellipsis, once the name is down to it.
The gate asserted the first route's flex: none. It now asserts the
order the line gives way in: the handle and the later routes first, the
agent's name down to a 6ch floor it never shrinks below, and the first
route last, to an ellipsis, with no floor of its own so a route longer
than the line still fits inside it.
happy-dom lays nothing out, so neither the gate nor BoardCard.test.tsx
could see the agent's name collapse to 0px or a route run past the
line. Render BoardCard's markup in the foreignObject DagGraph gives it,
under the real page.css and tasks stylesheet, at the board's 196px
card, and assert the name keeps its floor, the first route ends in an
ellipsis, and nothing is drawn past the line's edge, a route longer
than the whole line included.
Two single-declaration mutants of the agent's rule survived the browser
test: without max-width the name's 6ch floor padded a shorter name and a
roomy line pushed the route to the edge, and with the same grow factor as
the handle the two split the room. Measure the name's text with a Range
rather than scrollWidth, which a widened box hides, and add a case where
the handle must give way while the name is still cut.
The Chromium layout test for a decision node's card lived under
tests/integration, which the default pytest selection skips and no CI job
runs, so the only check CI ran on that line was the declaration gate,
which happy-dom cannot lay out. It now sits in the default selection,
where the unit job runs it against the runner's system Chrome. Without a
browser it still skips locally with the reason, but fails under CI, so
the measurement cannot drop out of CI unnoticed.
Single-declaration mutants of the tasks stylesheet showed the Chromium
test of the agent line passing over three rules it names: a chip allowed
to shrink, routes after the first with no 2ch floor, and those routes
sized from their own width ahead of the first route. Measure the chip's
text against its content box, hold the later routes at their floor with
an ellipsis while the first route is cut, and add a case where a short
first route stays whole while longer later routes give way.
The new Chromium gate looked the browser up as google-chrome or chromium on PATH. Windows ships neither name: the binary is chrome.exe and the installer leaves it under Program Files without adding that directory to PATH. So on a Windows host the gate found nothing and, under CI, failed instead of measuring the line -- the one branch the Linux unit job never runs.

Resolve a PATH name first, then the Windows install paths. The install lookup runs on every host and simply finds no file on Linux, so one code path covers both. A deterministic case stages a chrome.exe under a temporary PROGRAMFILES and asserts each branch, so the Windows-only branch is exercised on Linux too.

Verified natively on Windows 11 with Chrome 155.0.8059.86 from Program Files: the nine Chromium layout cases pass in 8.6s, where before they all errored under CI with no browser on PATH. The three-way branch case passes beside them, and ruff check and format pass.
The browser lookup kept each Windows install path as one backslash string and
joined it with Path(root) / install. Windows splits that string on the
backslashes, but POSIX treats a backslash as an ordinary character, so on Linux
the candidate was a single file literally named
"Google\Chrome\Application\chrome.exe" under the root. The case that stages
chrome.exe under a temporary PROGRAMFILES built real nested directories, so
the lookup never found it and returned None, failing the Linux unit job.

Keep each install path as its parts and join them with Path(root, *install).
Both flavours now build the same nested path, so the Windows branch is
genuinely exercised on Linux and the assertion stays as strict as before.
… value's type

YAML 1.1 resolves a bare `yes`, `on`, `true` or `1` to a boolean or a
number, so a hand-written route name arrived as a non-string key the
spec refuses and the runtime dropped the playbook from the library.

The store now reads every mapping key as the text it was written as, and
a key node is retagged through a copy: an anchor that shares one scalar
between a key and a value leaves the value's type alone. A node that is
not a mapping is left to PyYAML, which keeps the error the validate
paths catch, and `raven playbook validate` now answers a YAML syntax
error as a finding rather than a traceback.
…real-environment tests

The card's agent line is measured in a real Chromium, so the test
belongs with the tests that drive a real environment rather than in the
unit suite. It moves to tests/integration and keeps its name's shape:
scope `web_task_card`, kind `real_browser`.
The unit shards leave tests/integration out, so the card's layout test
needs a job of its own to keep running. The new `web-browser` job runs
it on a runner that ships a system Chrome; the browser extra carries
playwright, and no browser download runs.
@LivXue
LivXue force-pushed the fix/dag_branch_followups branch from 211d226 to 499b139 Compare October 27, 2026 03:03
@LivXue

LivXue commented Oct 27, 2026

Copy link
Copy Markdown
Member Author

Rebased onto main at 471842132 (#1114) and force-pushed as 499b13937161. The only conflict was CHANGELOG.md, in the Fixed section: both sides added entries at the same anchor, and both are kept. No product code changed in this round: the branch's own diff is the same 27 files as at 211d226d, and the only conflict resolution was in CHANGELOG.md. The description now names the rebase target and the verification at the new head.

At the new head: the six playbook/DAG files, 790 passed; tests/integration/test_web_task_card_real_browser.py -m integration, 10 passed; git merge-tree against origin/main, no conflict; the commit-message, source-language and large-file gates over origin/main..HEAD, passed.

On the Windows note about the new job: the layout gate is engine-level -- the 196px card width and the in-page 6ch and 2ch floors are measured inside Chromium -- so web-browser stays on ubuntu-latest by design. windows-upgrade runs with --noconftest and deliberately does not set PYTHONUTF8, so "every Windows job sets it" is not quite the rule. If a Windows leg is added later it will carry env: PYTHONUTF8: "1"; Chrome is already on windows-latest and the test's discovery finds it.

@0xKT the head is rebased and only CHANGELOG.md moved, so by your rule the A grade should carry. @gloryfromca if the rebased head still reads clean to you, an approving review would take this to the merge queue.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; this can merge as far as I am concerned.

Reviewed the rebase delta and current github/main...HEAD diff. I covered repository rules, patch history and conflict resolution, affected callers, backward compatibility, test strength, and architecture boundaries. The branch is based exactly on current github/main at 471842132 and git merge-tree --write-tree succeeds.

The range-diff preserves all PR behavior. Its substantive differences are the expected changelog context and an automatically shifted DAG-verdict test context from newer main; the literal golden assertion itself is unchanged. Both changelog sides are retained, no product-code conflict resolution was introduced, and my previously settled threads remain resolved.

Verification: playbook/CLI/DAG suites -> 790 passed; Web UI BoardCard and stylesheet gate -> 36 passed; TUI DAG render/layout -> 21 passed; local browser integration -> 1 discovery test passed and 9 browser-dependent skips on this browserless host. GitHub's web layout in a real browser job passed, as did all other completed checks visible at review time; four unit shards and Windows ACP stdio were still running.

@LivXue

LivXue commented Oct 27, 2026

Copy link
Copy Markdown
Member Author

Windows compatibility re-review — PR #1064 @ 499b139371612e8b05bab4cf1c322ad4a85aa9f4

Scope: the head moved from the previously Windows-reviewed 211d226d to this one. Across the merge, the PR's own 27 files changed only in .github/workflows/ci.yml, CHANGELOG.md, CONTEXT.md (main's content) and two PR files: raven/agent/subagent/dag_runner.py (stateless-retry prompt now carries every follow-up and records it before dispatch) and tests/test_subagent_dag_verdict.py (+26). The rest of the PR was covered by the 211d226d review; this pass reads the delta and re-runs the affected suites.

Result: the delta is platform-neutral; no new Windows-specific issue.

  • _stateless_retry_prompt, the record_follow_up move, forget_follow_ups and the new describe_failure test are string/prompt logic and store calls; no path, subprocess, signal, encoding, or platform primitive is added.
  • No dependency or CI-matrix change for Windows beyond what the earlier review already reported (the web-browser job remains ubuntu-latest; that non-blocking note still stands).

Verification (native Windows 11, CPython 3.12.13, isolated worktree D:\tmp\rv-1064 at this head, scratch HOME/USERPROFILE, PYTHONUTF8=1):

pytest tests/test_subagent_dag_verdict.py tests/test_subagent_dag_runner.py -q
→ 8 failed, 498 passed, 2 skipped in 23.46s

runner failing IDs (all eight):
  TestBackgroundRun::test_the_failure_path_reaps_a_subagent_child_left_running
  TestBackgroundRun::test_the_failure_path_reaps_the_whole_launcher_process_group
  test_a_missing_file_reads_the_same_for_both_ref_forms[n_ref-...]
  test_a_missing_file_reads_the_same_for_both_ref_forms[n_ref_path-...]
  test_a_reference_under_the_history_root_needs_the_grant
  test_a_run_that_recorded_no_outcome_is_not_addressable_by_id
  test_an_older_runs_output_stays_reachable_by_path_after_id_reuse
  test_the_tool_grants_this_conversations_history_and_nothing_wider

Those are exactly the inherited Windows runner failures measured at base 4718421324 (POSIX-root fixtures, process-group reaping, and the ref-path confinement bug that PR #1066 at e396c4e6 fixes — #1066 is not part of this branch). test_subagent_dag_verdict.py passes in full at this head, including the new parametrised cases.

Limitations: only the two affected suites were run this pass; the earlier 211d226d review's broader Windows verification (playbook store/CLI, real-Chrome card test) was not repeated because neither file changed in the delta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants