Repository navigation
fix(*): finish conditional branches in playbooks, the tasks board and the docs - #1064
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
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 --checkgates passed.
|
No new Windows-specific regression found in the inspected changes. Reviewed head Native Windows results (Python 3.13.12, Node 25.9.0):
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 commandsuv 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" |
gloryfromca
left a comment
There was a problem hiding this comment.
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...HEADpassed.
|
Not a blocker (review of 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.
|
gloryfromca
left a comment
There was a problem hiding this comment.
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.
|
Windows review: the existing board-card layout blocker reproduces natively; no additional Windows-specific regression found in the inspected changes. Reviewed head Existing P2 layout issue, with new native Windows evidence: the open route-overflow thread, Native Chromium, using SSR of the actual component and production styles:
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 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 commandsAll 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 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 |
aa1eaff to
b39d89a
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
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 loadlibatk-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.
|
Windows compatibility re-review at head 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):
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 resultsEach 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 0Node 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 0The browser file was initially deselected by the default marker filter; the recorded successful invocation explicitly selects integration tests. |
b39d89a to
4e13602
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
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 localPATH; collection passed.- Ruff check/format, source-language, large-file, and diff checks passed against the current GitHub base.
4e13602 to
1ef7f1a
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
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 --checkpassed against GitHub base2582e7936892.
gloryfromca
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
5c6cc63 to
6881f4a
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
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.
|
Windows verification at head I inspected the complete three-line delta from Native Windows 11, Python 3.13.12 with
The current head's CI is complete: 21 passed and 2 skipped, with no failure or pending job. The skips are This is scoped Windows evidence, not a review verdict. The two open layout threads remain for their openers to resolve. |
|
Merge conflict: @LivXue this PR no longer merges onto main.
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. |
|
Blocking: rebase onto current main and resolve the CHANGELOG.md conflict. I independently verified the new mergeability report after fetching 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. |
6881f4a to
211d226
Compare
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.
211d226 to
499b139
Compare
|
Rebased onto At the new head: the six playbook/DAG files, 790 passed; On the Windows note about the new job: the layout gate is engine-level -- the 196px card width and the in-page @0xKT the head is rebased and only |
gloryfromca
left a comment
There was a problem hiding this comment.
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.
|
Windows compatibility re-review — PR #1064 @ Scope: the head moved from the previously Windows-reviewed Result: the delta is platform-neutral; no new Windows-specific issue.
Verification (native Windows 11, CPython 3.12.13, isolated worktree Those are exactly the inherited Windows runner failures measured at base Limitations: only the two affected suites were run this pass; the earlier |
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.
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.${params.X}only inpromptTemplate, andvalidate_graph_nodeschecked onlypromptTemplateandinputs, so "the deal is above ${params.threshold}" reached the judge as written and${params.nobody}passed validation. Conditions now go through the same_fill_textas 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 onmainnow, 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.yes,no,on,off,true,falseand bare numbers load. The store parsed the spec block withyaml.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.dependsOnthe whole graph language and never namedroutes,selectorwhen; it now describes a decision node and the rule that a node depending on one names it in itswhen, and says a condition may use a param. A new test reads that section on its own.test_judge_without_routes_sends_the_old_request_verbatimcompared 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, againsttests/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.CONTEXT.mdgains 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 (theabandonparagraphs 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
routesdescription in therun_subagent_dagschema and onDagNodeSpec, which the playbook generator's tool carries; the guard-size error inraven/agent/subagent/dag_guard.py; the noteraven/agent/subagent/dag_render.pywrites 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 ini18n/messages.json, the DAG Panel entry ofui-tui/CONTEXT.md(whichtest_the_tui_glossary_says_what_the_turned_in_line_carriesquotes), 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:
tests/test_web_task_card_route_real_browser.pymeasured a card in a real Chromium, so it moved totests/integration/test_web_task_card_real_browser.py, and a newweb-browserCI 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 underCI, where a skip would leave the line unmeasured on every push and so fails instead.raven playbook validatenow 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
mainat471842132(#1114). TheCHANGELOG.mdconflict that rebasing raised is resolved by keeping both sides' entries, and the branch merges clean.Type
Verification
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 fivetest_subagent_dag_core.py::test_a_reference_landing_in_either_root_is_acceptedparameters, andtest_cli_playbook_commands.py::test_run_judges_its_nodes_so_a_branching_playbook_takes_its_branchand::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 tocat. The branch adds the passing cases.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 writableTEMP; with the default one the nine browser cases skip and the path-lookup case still passes.mainat471842132, 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.pyandscripts/check_large_files.pyoverorigin/main..HEAD, andgit 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.pyandruff format --checkover 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..HEADandscripts/check_large_files.py origin/main..HEAD: passed.git merge-treeagainstorigin/main: no conflict, through--write-treeon 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, inCHANGELOG.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 overmainat 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 tomain.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 theweb layout in a real browserjob, 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
promptTemplatealready 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.Related Issues
#1008 (items 1 to 6)