Repository navigation
fix(graph): reject malformed command parameter shapes - #331
Conversation
Fixes mikamikasuki#323. Assisted-by: OpenAI Codex.
There was a problem hiding this comment.
🟢 Approval recommended
The change is a narrowly scoped, reject-before-mutation validation guard that addresses the reported root cause, preserves existing semantics and internal callers, and is backed by consistent contract updates plus comprehensive unit and real-API tests.
No open issues 🎉
What changed in this PR
This PR hardens the FOREST graph command kernel against malformed params/targets JSON shapes. Previously, wrong-type values (booleans, numbers, nulls, lists/objects where IDs are expected) passed the existing checks and then crashed inside the operation branch with an unhandled AttributeError/TypeError/NoneType fault, returning HTTP 500 instead of an actionable validation error (issues #321, #322, #323). A new _check_shapes guard runs inside _check — before any graph mutation — and raises GraphError("invalid_command", status 422) naming the offending field. Since every command path (preview/simulate/apply/apply_compound_step) funnels through _execute → _check, the guard applies uniformly, including the batch/compound path.
Changes:
- Add
GraphCommandService._check_shapesvalidatingparams/targets, node patches andbranch_id, dependency endpoints, fork/clonenode_ids, and splitparts, returning 422invalid_commandwith the field name before dispatch. - Add kernel unit tests (171 boundary checks across preview/apply/compound) and a real-API integration scenario verifying 422 responses, unchanged graph/revision, zero side effects, preserved legacy
node_idform, and that valid preview/apply still work. - Regenerate error documentation across
contracts.py,openapi.json, andAPI_REFERENCE.mdto noteinvalid_commandon preview/commands.
Independent Merge Assessment (FOREST policy)
Reviewed commit: 3c606e6108835f4d1a911fe6f155b2645841a727
I independently traced the guard against the real _execute operation branches and against internal command producers. The guard addresses the root cause (shape faults reaching .pop()/len()/require(None)/unhashable keys) rather than masking the symptom, and it raises before any node/edge/branch write, so the graph, revision, receipts, interventions, and run state are provably untouched on rejection (asserted by both the unit and API tests). The only behavioral tightening I found is that an explicitly falsy params.source/params.target/node_ids: [] is now rejected instead of silently falling back to targets; this input is malformed, no observed caller depends on the old fallback, and a field-named 422 is a strict improvement — not a merge blocker.
| File | Description |
|---|---|
research/kernel/graph.py |
Adds _check_shapes, invoked from _check, validating command parameter/target shapes before mutation and raising 422 invalid_command. |
tests/test_graph.py |
Adds parametrized kernel boundary tests (preview/apply/compound) and a legacy node_id source-form regression. |
tests/test_api.py |
Adds a real FastAPI scenario asserting 422 responses, no side effects, unchanged graph, and working valid preview/apply. |
services/api/contracts.py |
Documents invalid_command (422) on graph preview/commands operations. |
packages/contracts/openapi.json |
Regenerated OpenAPI with updated descriptions and 422 invalid_command. |
docs/API_REFERENCE.md |
Regenerated reference noting invalid_command for preview/commands. |
Review effort: Balanced
|
CI follow-up for 3c606e6: observation-integration passed. The local job reported two failures: the existing digest-import check in The digest-import failure was already reproduced on base e8819a5 (see PR #330). I reran the upload scenario locally on the unchanged base (1 passed, 14 deselected in 7.60s) and this head (1 passed, 14 deselected in 6.13s). The failing test files and their reported production owners are identical at base and head. These local passes do not establish the cause of the hosted timing failure, which remains unconfirmed. No timeout, safety check, workflow or file-upload behavior has been altered. Investigation performed with OpenAI Codex assistance. |
Malformed graph command parameters can reach collection and node operations as booleans, numbers, nulls or other wrong JSON shapes, producing HTTP 500 instead of an actionable validation error. Validate the affected shapes in the shared kernel before dispatch: node patches and branch IDs, dependency endpoints, fork/clone node-ID lists and split parts. Return HTTP 422
invalid_commandnaming the field. Preserve revision checks, the legacynode_idsource form, normal preview/apply behavior and existing graph semantics.Fixes #323 (including the duplicate reports #321 and #322).
Validation:
INTERNAL_ERRORfor the missing dependency target.3c606e6108835f4d1a911fe6f155b2645841a727: 212 tests pass, comprising all 211 graph tests and one isolated real-API scenario. The scenario checks 54 invalid preview/apply requests, verifies unchanged graph/revision and receipt/intervention/run counts, then exercises valid preview and apply. No HTTP response mocks./usr/bin/bwrapis unavailable; the 9 PostgreSQL variants require a dedicated database. These results are not claimed as a green full suite or as a final-commit full run.git diff --checkpasses. Fixtures use temporary SQLite databases and synthetic data, with no provider calls.Limits: the full backend suite, PostgreSQL and Bubblewrap worker behavior were not validated. No dependencies, workflows, authentication, execution isolation or security checks are changed or disabled. This validates the listed command shapes, not every possible graph parameter schema.
Implemented and tested with OpenAI Codex assistance. No independent human review is claimed.