Skip to content

Share parallel-roles/map spec validation across sync and async paths - #485

Draft
lezama wants to merge 1 commit into
Automattic:mainfrom
lezama:fix/workflow-parallel-spec-validation-shared
Draft

Share parallel-roles/map spec validation across sync and async paths#485
lezama wants to merge 1 commit into
Automattic:mainfrom
lezama:fix/workflow-parallel-spec-validation-shared

Conversation

@lezama

@lezama lezama commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

The parallel workflow step handler validated its spec in two independent places that could silently drift:

  • Sync (in-process loops): run_parallel_roles() and run_parallel_map() in src/Workflows/class-wp-agent-workflow-runner.php (~L1310 / ~L1238 on the pre-change file).
  • Async (suspend/resume dispatch-plan builders): build_roles_dispatch_plan() and build_map_dispatch_plan() in the same file (~L983 / ~L1063).

Both pairs re-implemented the identical validation by hand:

  • roles — entries-must-be-arrays (workflow_parallel_branch_invalid), non-empty branches (workflow_parallel_branches_empty), at-most-one aggregator (workflow_parallel_aggregator_invalid).
  • map — items-resolve-to-array (workflow_parallel_items_invalid), non-empty nested steps (workflow_parallel_steps_invalid).

Because the two copies were maintained separately, a rule tightened on one path but not the other would make the substrate accept a spec synchronously that it rejects asynchronously (or vice versa) — a hard-to-diagnose correctness gap that only surfaces once a caller registers a branch executor and the run takes the async path.

Fix

Extract two shared private helpers and route all four entry points through them:

  • validate_parallel_roles_spec( array $step ) — enforces the three roles rules once and returns the parsed split (sibling branches + optional aggregator), or a WP_Error with the shared code on an invalid spec.
  • validate_parallel_map_spec( array $step ) — enforces the two map rules once and returns the resolved items + steps, or a WP_Error.

This is a behavior-preserving refactor: same error codes, same messages, same set of accepted and rejected specs. Only the duplication is removed. Each caller keeps its own path-specific bits (the sync loop's raw shared-context snapshot vs. the async builder's string_keyed_array() normalization and descriptor building) — those were never part of the shared validation and are left untouched.

Testing

  • New tests/workflow-parallel-spec-validation-parity-smoke.php asserts parity by invoking both entry points (via reflection, so it targets the exact drift-prone methods):
    • sync and async reject the same invalid specs with the same error codes (all three roles cases + both map cases), and
    • sync and async accept the same valid spec — a roles spec with an aggregator and a map spec — so the legitimate valid-spec path does not regress.
  • Registered in the composer smoke array.
  • composer smoke — all green (exit 0), including the existing workflow-parallel-smoke and workflow-parallel-async-smoke suites (38 + 38 assertions unchanged).
  • vendor/bin/phpstan analyse --no-progress --memory-limit=2GNo errors (level max).

This came out of an automated tech-debt audit of the workflow runner. Opening as a draft for review — the change is intentionally scoped to removing the duplication with a parity safety net, with no change to which specs are accepted or rejected.

…ync paths

The SYNC in-process loops (`run_parallel_roles()` / `run_parallel_map()`) and
the ASYNC dispatch-plan builders (`build_roles_dispatch_plan()` /
`build_map_dispatch_plan()`) independently re-implemented the SAME parallel
spec validation. Because the two copies were maintained by hand, they could
silently DRIFT — a rule tightened on one path but not the other would make the
substrate accept a spec synchronously that it rejects asynchronously (or vice
versa), a hard-to-diagnose correctness gap.

Extract two shared private helpers — `validate_parallel_roles_spec()` and
`validate_parallel_map_spec()` — that enforce the rules once (roles:
entries-are-arrays / non-empty branches / at-most-one aggregator; map: items
resolve to an array / non-empty nested steps) and return the parsed split
(sibling branches + optional aggregator, or items + steps), or a WP_Error with
the shared error code on an invalid spec. Route all four entry points through
them. Behavior is IDENTICAL: same error codes, same messages, same accepted and
rejected specs — only the duplication is removed.

Adds tests/workflow-parallel-spec-validation-parity-smoke.php, which asserts the
sync and async entry points reject the same invalid specs with the same error
codes AND accept the same valid roles (with aggregator) and map specs, so the
two paths can never drift again. All existing workflow smoke tests still pass;
PHPStan (max) is clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.

1 participant