Repository navigation
fix(playbook): read yaml mapping keys as the text they were written as - #1057
yudongyouqing wants to merge 3 commits into
Conversation
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the full github/main...HEAD diff and the surrounding store, playbook schema/validation, route consumers, writer, relevant history, #1008 contract, and Runtime context terminology. I also checked backward compatibility and safety: the loader remains a SafeLoader subclass, values retain normal YAML coercion, existing string keys and YAML merge keys retain their behavior, and downstream keyed fields require strings. The tests add coverage without weakening existing assertions, and the change follows the applicable AGENTS.md rules; it introduces no architecture, asset, dependency, naming, or source-language issue.
Verification: after uv sync --all-packages supplied the workspace plugins required by the shared test fixture, uv run pytest tests/test_playbook_store.py tests/test_playbook_validate.py tests/test_unified_playbook.py tests/test_playbook_executor.py tests/test_cli_playbook_commands.py -q passed (274 passed). uv run ruff check raven/playbook/store.py tests/test_playbook_store.py, uv run ruff format --check raven/playbook/store.py tests/test_playbook_store.py, and git diff --check github/main...HEAD also passed. A direct load probe confirmed all six YAML 1.1 boolean spellings plus a bare-digit route remain written strings while confirm: yes remains boolean true. The initial test attempt failed only during fixture setup because the fresh environment lacked raven_everos; no test body ran until the workspace sync, after which the exact suite was green.
There was a problem hiding this comment.
Blocking: the custom construct_mapping drops PyYAML's mapping checks, so playbooks.validate now raises on a file it promises to answer for -- including the unquoted {{ params.X }} placeholder.
_KeyAsStringLoader.construct_mapping (raven/playbook/store.py) rebuilds the mapping loop itself instead of delegating, so it loses BaseConstructor.construct_mapping's isinstance(key, Hashable) check (a ConstructorError, i.e. a yaml.YAMLError, with the line and column). An unhashable key now reaches mapping[key] = ... and raises a bare TypeError. The commonest way to write one by hand is the playbook's own placeholder left unquoted: promptTemplate: {{ params.topic }} is a YAML flow mapping whose key is a mapping.
playbooks_validate (raven/rpc/methods/playbooks.py) says it "Answers rather than raises", and its except (ValidationError, ValueError, yaml.YAMLError) was widened for exactly the hand-edited case ("Left out, the one method whose whole point is to answer with findings turned that case into an internal error with a traceback"). TypeError is not in that tuple, so the call ends in the dispatcher's generic handler as internal_error.
Measured by calling the real playbooks_validate on a hand-written dag playbook in a scratch library (HOME and RAVEN_HOME in scratch):
promptTemplate: |
main dcc019a | this PR |
|---|---|---|
{{ params.topic }} (unquoted) |
answers ok=False: "while constructing a mapping ... line 10, column 21" |
raises TypeError: unhashable type: 'dict' |
'{{ params.topic }}' (control) |
answers ok=False (semantic finding) |
same |
The paths that catch Exception (the runtime's library refresh, playbooks.list, raven playbook list) still skip the file, but now report "unhashable type: 'dict'" with no location. The # noqa: S506 ... only mapping keys change notes are no longer accurate either.
A direction I tested: keep PyYAML's own construction and only retag the keys -- in construct_mapping, for a MappingNode, call self.flatten_mapping(node) first, then set key_node.tag = "tag:yaml.org,2002:str" on each ScalarNode key whose tag is bool/int/float, then return super().construct_mapping(node, deep=deep). (Flattening first matters: without it a key that arrives through a <<: merge, e.g. <<: {on: 1}, still becomes True.) With that, yes/no/1/1.5 route keys load as strings, merged keys too, confirm: yes stays True, unhashable keys raise ConstructorError again, the validate case above answers as on main, and tests/test_playbook_store.py + tests/test_rpc_playbooks.py pass (159). A test with an unquoted placeholder, and one for an int/float key (only the bool path is tested now), would pin it.
There was a problem hiding this comment.
Fixed on the delegation direction you tested, and one step broader: every scalar key of a non-string tag is retagged through a copy now, not a bool/int/float whitelist -- a route named 2024-01-01 was still arriving as a datetime key and being dropped. The copy, rather than an in-place retag, is because anchors can share one scalar between a key and a value: retagging the shared node turned the value into text or was eaten by the constructed-objects cache. The unquoted placeholder answers ConstructorError again (pinned by a test that fails with the bare TypeError on the previous head), the digit arm pins 007, and a self-review round in the same push answers a non-mapping spec block with a ValueError instead of a TypeError from the name assignment, and adds yaml.YAMLError to the CLI validate catches the RPC twin always had. Ten-form sweep of the loader (bool/date/null/merge/alias/placeholder/sequence-key/list-document/value-coercion/leading-zero) all behave; 412 across the playbook suites, ruff, format and the type gate clean.
YAML 1.1 resolves yes, no, on, off, true, false and bare digits to booleans and numbers, so a hand-written playbook whose decision named its routes that way arrived at the spec with non-string keys and the runtime dropped the whole file from the library as unloadable. The store's reads now use a SafeLoader subclass that keeps mapping keys in their written form -- the same shape its own writer emits by quoting them -- while values are untouched, so confirm: yes still means true.
The custom construct_mapping rebuilt the mapping loop, which dropped
PyYAML's own answer for an unhashable key: a ConstructorError with a
location, the yaml.YAMLError playbooks_validate catches to answer with a
finding. The commonest hand-written unhashable key is the placeholder
itself left unquoted (promptTemplate: {{ params.topic }} is a mapping
keyed by a mapping), and it escaped as a bare TypeError instead. The
loader now flattens, retags the bool/int/float scalar key nodes to
strings -- including one arriving through a <<: merge -- and hands the
mapping back to PyYAML's own loop, so yes/no/digit route names still
load as written, confirm: yes still means true, and an unquoted
placeholder answers yaml again. Pins the placeholder case and the
leading-zero digit key.
…mappings Two escapes the key-retag left, and two older ones in the same contract. The retag whitelist knew bool, int and float; a bare date is a timestamp and ~ is null, so a route named 2024-01-01 still arrived as a datetime key and the playbook was dropped as unloadable -- every scalar key of a non-string tag is retagged now, not three tags. Retagging the node in place also reached through anchors: one scalar shared between a key and a value turned the value into text or was eaten by the constructed- objects cache; the key is replaced with a retagged copy instead, so the value keeps its type. A spec block that parses to a list, and a frontmatter that does, answered a bare TypeError from the assignment below; both are ValueErrors now, the family playbooks_validate turns into findings -- as does yaml.YAMLError, which the CLI validate command never caught and the RPC twin always did. The load is one _load_yaml helper carrying its single suppression, and the comments that still named safe_load follow the loader they describe.
60d8d53 to
24dc9b7
Compare
|
No new Windows compatibility issue found in the inspected YAML-loading and CLI-validation changes. Reviewed head Native Windows verification (Python 3.13.12, UTF-8 mode):
Code reading: the change retains PyYAML's own mapping construction/errors, copies scalar keys before retagging, and keeps values typed. It adds no shell command, executable discovery, filesystem operation or dependency. Limitations: the CLI checks use CliRunner, and full CLI/RPC/repository suites, live graph/model execution, UNC/non-ASCII directory and code-page combinations were not verified. This is a scoped Windows assessment, not a merge verdict. Exact verification commandsEach run used an isolated detached head/base checkout, frozen dev dependencies, source imports pinned to that checkout, and isolated profile/Raven/OAuth state. # native; exit 1; cwd C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\pr1057\head
uv run --frozen --no-sync pytest tests/test_playbook_store.py tests/test_cli_playbook_commands.py tests/test_rpc_playbooks.py -n 0 -x --tb=short --basetemp C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\checks\1057-native\pytest --junitxml C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\checks\1057-native\junit.xml
# focused; exit 1; cwd C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\pr1057\head
uv run --frozen --no-sync pytest tests/test_playbook_store.py tests/test_cli_playbook_commands.py::test_validate_accepts_a_good_file_by_path_and_by_name tests/test_cli_playbook_commands.py::test_validate_reports_errors_with_the_file_position tests/test_cli_playbook_commands.py::test_validate_answers_a_yaml_syntax_error_as_a_finding tests/test_rpc_playbooks.py -n 0 -x --tb=short --basetemp C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\checks\1057-focused\pytest --junitxml C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\checks\1057-focused\junit.xml
# rpc-validation; exit 0; cwd C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\pr1057\head
uv run --frozen --no-sync pytest tests/test_rpc_playbooks.py -k validate -n 0 -x --tb=short --basetemp C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\checks\1057-rpc-validation\pytest --junitxml C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\checks\1057-rpc-validation\junit.xml
# base-cat; exit 1; cwd C:/Users/livxu/AppData/Local/Temp/raven-windows-review/76440228003d458eb50cf2d4a3438c5e/base431599
uv run --frozen --no-sync pytest tests/test_cli_playbook_commands.py::test_run_judges_its_nodes_so_a_branching_playbook_takes_its_branch -n 0 -x --tb=short --basetemp C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\checks\1057-base-cat\pytest --junitxml C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\checks\1057-base-cat\junit.xml
# base-path; exit 1; cwd C:/Users/livxu/AppData/Local/Temp/raven-windows-review/76440228003d458eb50cf2d4a3438c5e/base431599
uv run --frozen --no-sync pytest tests/test_rpc_playbooks.py::test_get_answers_one_whole_spec -n 0 -x --tb=short --basetemp C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\checks\1057-base-path\pytest --junitxml C:\Users\livxu\AppData\Local\Temp\raven-windows-review\76440228003d458eb50cf2d4a3438c5e\checks\1057-base-path\junit.xml |
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the complete current four-file diff and the changes since 60d8d5307eb8, including the existing constructor discussion. The new implementation delegates mapping construction back to PyYAML, preserves ConstructorError and source locations for unhashable keys, copies retagged scalar key nodes so aliased values retain their types, handles merged and timestamp/null-like keys, and keeps value coercion unchanged. The new mapping-shape checks and CLI yaml.YAMLError handling turn malformed hand-edited files into validation findings rather than internal exceptions.
I also covered the store/RPC/CLI callers, writer and schema constraints, relevant history, backward compatibility, SafeLoader boundary, tests for weakening, AGENTS.md and CONTEXT.md terminology, and architecture/dependency/asset constraints. No new issue remained after checking those paths.
Verification: after syncing all workspace packages, uv run --no-sync pytest tests/test_playbook_store.py tests/test_cli_playbook_commands.py tests/test_rpc_playbooks.py tests/test_playbook_validate.py tests/test_unified_playbook.py tests/test_playbook_executor.py -q passed (400 passed). Ruff check, Ruff format check, git diff --check github/main...HEAD, and direct probes for scalar keys, merges, aliases, and the unquoted-placeholder ConstructorError also passed. The type-gate wrapper was unavailable because make is not installed; invoking its underlying command directly could not build the unchanged optional datrie dependency because this environment lacks x86_64-linux-gnu-gcc, so no type-check result was produced.
|
Closing this in favour of #1064, which now carries the same loader. The key-preserving loader drafted here landed in #1064 as well, so the store would otherwise end up with two of them. #1064 holds the loader this branch wrote, plus the two cases that keep it honest:
#1064 also wires the store's Nothing here is lost: the branch is open for reference, and its commits are readable in #1064's history. |
|
Superseded by #1064, which carries the same key-preserving loader. |
… the docs (#1064) ## 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 #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 #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. #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 #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 #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 - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [x] 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 #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. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues #1008 (items 1 to 6)
Summary
yes,no,on,off,true,falseand bare digits to booleans and numbers, so a hand-written playbook whose decision named its routes that way arrived at the spec with non-string keys (Input should be a valid string), and the runtime's answer to a load error dropped the whole file from the library with only a log line -- item 3 of DAG conditional branches: seven follow-ups left out of #1001 #1008.confirm: yesstill means true. The retag covers every scalar key of a non-string tag (a bare date was still a datetime key on the first revision) and goes through a copy, so an anchor sharing one scalar between a key and a value keeps the value's type; the mapping itself is built by PyYAML's own loop, whose ConstructorError for an unhashable key -- the unquoted{{ params.X }}placeholder among them -- is the yaml.YAMLError the validators answer with. A spec block or frontmatter that parses to a non-mapping answers a ValueError, and the CLI validate command catches yaml.YAMLError like its RPC twin always did.Type
Verification
uv run pytest tests/test_playbook_store.py tests/test_playbook_validate.py tests/test_unified_playbook.py tests/test_playbook_executor.py tests/test_cli_playbook_commands.py -q-> 412 passedyes/no-routes playbook raised pydantic'sInput should be a valid stringon the base; on this head it loads with both routes and thewhen: triage.yesguard intactruff checkandruff format --checkon both files,make lint-types-> clean; the two S506 findings carry the repo's noqa-with-reason form (the loader is a SafeLoader subclass that only changes key handling, no constructors added)Risk
Related Issues
Item 3 of #1008