[PR #3493] [MERGED] fix(browser): reserve the initial scenario step id from action inputs #5776

New issue

Closed

opened 2026-10-31 01:09:22 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:09:22 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3493
Author: @NIU-123370
Created: 10/14/2026
Status: ✅ Merged
Merged: 10/21/2026
Merged by: @morluto

Base: main ← Head: fix/scenario-start-reserved-step-id

📝 Commits (2)

a80da11 fix(browser): reserve the initial scenario step id from action inputs
5265719 Merge remote-tracking branch 'origin/main' into HEAD

📊 Changes

4 files changed (+43 additions, -2 deletions)

View changed files

📝 src/browser/PlaywrightBrowserScenarioProvider.ts (+5 -2)
📝 src/domain/browserScenario.test.ts (+22 -0)
📝 src/domain/browserScenario.ts (+10 -0)
📝 src/domain/browserScenarioValues.ts (+6 -0)

📄 Description

Summary

Fixes #3391.

capture-browser-scenario accepted a caller action whose step_id was
scenario_start, the identity the provider independently gives to the
initial-state step, so one capture carried two steps with the same ID and could
not even be compared with itself.

validateBrowserScenario already requires caller step IDs to be unique among
themselves (src/domain/browserScenario.ts), but nothing reserved the
generated identity. This adds that missing reservation at the accepted-input
boundary and names the identity in one owner instead of leaving it as a bare
literal in the provider.

Problem and expected behavior

Trigger: a scenario whose declared action uses step_id: "scenario_start".
Violated invariant: docs/browser-scenario-contract.md promises an initial
state followed by one record per declared action, and comparison "aligns only
by exact, unique step_id". A colliding identity makes a complete capture
ambiguous (overall_status: "unknown" with alignment failures).
The issue asks that a reserved identity be identified before the scenario
runs rather than accepted and turned into ambiguous Evidence — that is what
this does.

Before/after, executed through the compiled CLI (no browser involved, because
validation now precedes launch):

// step_id "scenario_start" on the second action — rejected, exit 1
{"code":"invalid_request","category":"invalid_input","retryable":true,
 "details":{"operation":"capture_browser_scenario",
   "issues":[{"path":["actions",1,"step_id"],"reason":"invalid_value",
     "message":"Action step IDs must not reuse the reserved initial step identity"}]}}

// control, identical input with step_id "settle" — passes validation and only
// then fails on the deliberately nonexistent executable, exit 1
{"code":"execution_failure","category":"execution_failure","retryable":false, …}

The control proves the rejection is specific to the reserved identity rather
than a general input problem.

Change and scope

src/domain/browserScenarioValues.ts — one exported owner,
BROWSER_SCENARIO_START_STEP_ID, documented as the identity the provider owns.
src/domain/browserScenario.ts — the input refinement rejects any declared
action that claims it and reports the exact offending path
(actions[i].step_id), next to the existing uniqueness rule.
src/browser/PlaywrightBrowserScenarioProvider.ts — the initial-state step
now uses the constant instead of a local literal, so the accepted-input rule
and the producer cannot drift apart.
src/domain/browserScenario.test.ts — the new case asserts three things: the
reserved id is refused, the issue path points at the second action (not just
the first), and a similar-but-distinct id (scenario_start_2) still validates,
so the rule stays exact rather than prefix- or substring-based.

Equivalence class checked: PlaywrightBrowserScenarioProvider.ts is the only
producer of a synthetic step identity in src/browser (stepId: "…" occurs
once), and the Electron active path
(src/browser/PlaywrightBrowserScenarioProvider.ts sibling
PlaywrightElectronActiveActions.ts:104,118) records only caller-supplied ids,
so it has no reserved identity to collide with and is untouched.

Intentionally not included:

No automatic renaming or de-duplication of claimed IDs — silent identity
rewriting would make step Evidence depend on positional luck.
No edit to docs/browser-scenario-contract.md: its existing
"exact, unique step_id" sentence already covers the invariant, and two open
PRs (#2637, #2961) currently touch that file. A one-line "the initial-state ID
is reserved" note is worth adding, but I would rather do it after those land.
No change to the advertised MCP input schema. The MCP tool advertises
browserScenarioInputSchema while src/server/registerBrowserScenarioTool.ts:37
and src/cliBrowserScenarioCommands.ts:39 re-parse accepted input with
browserScenarioSchema; the new rule therefore rides the same path as the
pre-existing uniqueness rule rather than inventing a second gate.

Contract and boundary impact

Semantic owner and earliest changed stage: browserScenarioValues.ts owns the
identity; earliest stage is accepted-input validation, before any browser
launch or Evidence construction.
Contribution scope fit and linked maintainer decision: ordinary fix inside an
existing boundary — not applicable.
CLI and MCP/tool-catalog contract: no schema, flag, or tool-description
change; one previously accepted (and broken) input now returns
invalid_request with a located issue path.
Provider, bridge, target-format, or platform compatibility: none — Playwright
scenario path only.
Evidence, artifact, provenance, or reconstruction contract: captures can no
longer contain two steps with one identity, so compare-web-captures
alignment for complete scenario captures stays well-defined. No existing
retained field changes.
Target selection, authority, side effects, and cleanup details: strictly fewer
launches — a colliding scenario now stops before the browser starts.
Generated metadata (docs/public/product-catalog.json) / package /
installation impact: none.

Expected effects:

Read existing files or captured data.
Launch or attach to a process.
Inject instrumentation or run scripts/code.
Change process memory, registers, files, or configuration.
Contact network services.
Recover protected code or work around protections.
Documentation or usability only; no target interaction.
Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

Tests added or updated: one new case in src/domain/browserScenario.test.ts
(rejection + offending-path precision + non-overbroad control).
Executed CLI pair: quoted above; rea capture-browser-scenario on the built
runtime, base 3b79af28.

Mutation arms, each actually run against
src/domain/browserScenario.test.ts plus
src/browser/PlaywrightBrowserScenarioProvider.test.ts, tree restored and
re-verified between arms:

arm	mutation	observed
C1	none (control)	`22 passed (22)`
M1	remove the reserved-identity rule (the base defect)	`1 failed \| 21 passed (22)`
M2	inspect only the first action	`1 failed \| 21 passed (22)`
M3	compare a different identity (`goto_start`)	`1 failed \| 21 passed (22)`
M4	invert the guard	`20 failed \| 2 passed (22)`
M5	over-broad substring rejection	`1 failed \| 21 passed (22)`
M6	lose the offending action path	`1 failed \| 21 passed (22)`
M7	provider drifts to another start identity	`3 failed \| 19 passed (22)`

M2/M6 are killed only because the reserved id is placed at index 1 and the
path is asserted; M5 is killed by the scenario_start_2 control; M7 is what
binds the producer to the same owner as the rule.

Remaining proof gaps: no end-to-end capture-browser-scenario run against a
real browser in this session (the collision it prevents is now unreachable
through validation, and the browser-dependent suites are quoted below); the
advertised MCP schema still accepts the ID until the handler rejects it, which
matches how the existing uniqueness rule behaves.

Validation performed

npx vitest run src/domain/browserScenario.test.ts before the source change —
1 failed | 4 passed (5); after — green.
npx vitest run src/domain/browserScenario.test.ts src/browser/PlaywrightBrowserScenarioProvider.test.ts src/domain/browserScenarioDiff.test.ts tests/boundary/mcp/browserScenarioMcp.test.ts tests/boundary/cli/browserCli.test.ts — Test Files 5 passed (5),
Tests 51 passed (51) (re-run after formatting).
Entry-surface sweep: npx vitest run tests/boundary/cli/ tests/boundary/mcp/
— Test Files 213 passed | 2 skipped, Tests 1184 passed | 7 skipped; the two
initially failing files (contractPresentation, toolSchemaValidity) failed
only on the gitignored generated .cache/mcp-tool-catalog.json, then passed
(14 tests) after node scripts/generate-mcp-catalog.mjs, so they are not
affected by this patch.
npm run build — exit 0 (Node 24.18.0), needed for the CLI proof.
npm run typecheck — exit 0; npm run lint — exit 0
(Checked resolved module boundaries in 1878 source files.).
./node_modules/.bin/oxfmt --check <the four changed files> — first pass
flagged one file, fixed with oxfmt (not prettier), re-check exit 0,
All matched files use the correct format.
npm run jscpd with the change — Duplication: 2715 files, 2006 clones, 3.22%; the same tree with the change reverted — byte-identical
2715 files, 2006 clones, 3.22%, so this patch contributes zero clones
(.jscpd.json threshold 3.5).
Not run: full check:ci, real browser scenario execution, other platforms.

Compatibility, safety, and release

Breaking changes or migration steps: a previously accepted input is now
rejected. That input could only produce an ambiguous capture, so callers hit
by the rejection were already losing the comparison they asked for; the error
names the offending action index and is retryable: true. No previously
valid, non-colliding call changes behavior.
Real Hopper/Ghidra, browser, or OS coverage: no provider or platform path
touched; CLI validation proof executed on macOS arm64.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no new commands, no new
process launches; strictly an earlier refusal.

Author readiness

Human owner: @NIU-123370

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

Before merge

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/3493 **Author:** [@NIU-123370](https://github.com/NIU-123370) **Created:** 10/14/2026 **Status:** ✅ Merged **Merged:** 10/21/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/scenario-start-reserved-step-id` --- ### 📝 Commits (2) - [`a80da11`](https://github.com/morluto/rea/commit/a80da11a9587141bf52aa61002231bb284622c7e) fix(browser): reserve the initial scenario step id from action inputs - [`5265719`](https://github.com/morluto/rea/commit/52657195e6754366c7215882f38002d521ae65b6) Merge remote-tracking branch 'origin/main' into HEAD ### 📊 Changes **4 files changed** (+43 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `src/browser/PlaywrightBrowserScenarioProvider.ts` (+5 -2) 📝 `src/domain/browserScenario.test.ts` (+22 -0) 📝 `src/domain/browserScenario.ts` (+10 -0) 📝 `src/domain/browserScenarioValues.ts` (+6 -0) </details> ### 📄 Description ## Summary Fixes #3391. `capture-browser-scenario` accepted a caller action whose `step_id` was `scenario_start`, the identity the provider independently gives to the initial-state step, so one capture carried two steps with the same ID and could not even be compared with itself. `validateBrowserScenario` already requires caller step IDs to be unique *among themselves* (`src/domain/browserScenario.ts`), but nothing reserved the generated identity. This adds that missing reservation at the accepted-input boundary and names the identity in one owner instead of leaving it as a bare literal in the provider. ## Problem and expected behavior - Trigger: a scenario whose declared action uses `step_id: "scenario_start"`. - Violated invariant: `docs/browser-scenario-contract.md` promises an initial state followed by one record per declared action, and comparison "aligns only by exact, unique `step_id`". A colliding identity makes a complete capture ambiguous (`overall_status: "unknown"` with alignment failures). - The issue asks that a reserved identity be identified *before* the scenario runs rather than accepted and turned into ambiguous Evidence — that is what this does. Before/after, executed through the compiled CLI (no browser involved, because validation now precedes launch): ```json // step_id "scenario_start" on the second action — rejected, exit 1 {"code":"invalid_request","category":"invalid_input","retryable":true, "details":{"operation":"capture_browser_scenario", "issues":[{"path":["actions",1,"step_id"],"reason":"invalid_value", "message":"Action step IDs must not reuse the reserved initial step identity"}]}} // control, identical input with step_id "settle" — passes validation and only // then fails on the deliberately nonexistent executable, exit 1 {"code":"execution_failure","category":"execution_failure","retryable":false, …} ``` The control proves the rejection is specific to the reserved identity rather than a general input problem. ## Change and scope - `src/domain/browserScenarioValues.ts` — one exported owner, `BROWSER_SCENARIO_START_STEP_ID`, documented as the identity the provider owns. - `src/domain/browserScenario.ts` — the input refinement rejects any declared action that claims it and reports the exact offending path (`actions[i].step_id`), next to the existing uniqueness rule. - `src/browser/PlaywrightBrowserScenarioProvider.ts` — the initial-state step now uses the constant instead of a local literal, so the accepted-input rule and the producer cannot drift apart. - `src/domain/browserScenario.test.ts` — the new case asserts three things: the reserved id is refused, the issue path points at the *second* action (not just the first), and a similar-but-distinct id (`scenario_start_2`) still validates, so the rule stays exact rather than prefix- or substring-based. Equivalence class checked: `PlaywrightBrowserScenarioProvider.ts` is the only producer of a synthetic step identity in `src/browser` (`stepId: "…"` occurs once), and the Electron active path (`src/browser/PlaywrightBrowserScenarioProvider.ts` sibling `PlaywrightElectronActiveActions.ts:104,118`) records only caller-supplied ids, so it has no reserved identity to collide with and is untouched. Intentionally not included: - No automatic renaming or de-duplication of claimed IDs — silent identity rewriting would make step Evidence depend on positional luck. - No edit to `docs/browser-scenario-contract.md`: its existing "exact, unique `step_id`" sentence already covers the invariant, and two open PRs (#2637, #2961) currently touch that file. A one-line "the initial-state ID is reserved" note is worth adding, but I would rather do it after those land. - No change to the advertised MCP input schema. The MCP tool advertises `browserScenarioInputSchema` while `src/server/registerBrowserScenarioTool.ts:37` and `src/cliBrowserScenarioCommands.ts:39` re-parse accepted input with `browserScenarioSchema`; the new rule therefore rides the same path as the pre-existing uniqueness rule rather than inventing a second gate. ## Contract and boundary impact - Semantic owner and earliest changed stage: `browserScenarioValues.ts` owns the identity; earliest stage is accepted-input validation, before any browser launch or Evidence construction. - Contribution scope fit and linked maintainer decision: ordinary fix inside an existing boundary — not applicable. - CLI and MCP/tool-catalog contract: no schema, flag, or tool-description change; one previously accepted (and broken) input now returns `invalid_request` with a located issue path. - Provider, bridge, target-format, or platform compatibility: none — Playwright scenario path only. - Evidence, artifact, provenance, or reconstruction contract: captures can no longer contain two steps with one identity, so `compare-web-captures` alignment for complete scenario captures stays well-defined. No existing retained field changes. - Target selection, authority, side effects, and cleanup details: strictly fewer launches — a colliding scenario now stops before the browser starts. - Generated metadata (`docs/public/product-catalog.json`) / package / installation impact: none. Expected effects: - [x] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: one new case in `src/domain/browserScenario.test.ts` (rejection + offending-path precision + non-overbroad control). - Executed CLI pair: quoted above; `rea capture-browser-scenario` on the built runtime, base `3b79af28`. - Mutation arms, each actually run against `src/domain/browserScenario.test.ts` plus `src/browser/PlaywrightBrowserScenarioProvider.test.ts`, tree restored and re-verified between arms: | arm | mutation | observed | | --- | --- | --- | | C1 | none (control) | `22 passed (22)` | | M1 | remove the reserved-identity rule (the base defect) | `1 failed \| 21 passed (22)` | | M2 | inspect only the first action | `1 failed \| 21 passed (22)` | | M3 | compare a different identity (`goto_start`) | `1 failed \| 21 passed (22)` | | M4 | invert the guard | `20 failed \| 2 passed (22)` | | M5 | over-broad substring rejection | `1 failed \| 21 passed (22)` | | M6 | lose the offending action path | `1 failed \| 21 passed (22)` | | M7 | provider drifts to another start identity | `3 failed \| 19 passed (22)` | M2/M6 are killed only because the reserved id is placed at index 1 and the path is asserted; M5 is killed by the `scenario_start_2` control; M7 is what binds the producer to the same owner as the rule. - Remaining proof gaps: no end-to-end `capture-browser-scenario` run against a real browser in this session (the collision it prevents is now unreachable through validation, and the browser-dependent suites are quoted below); the advertised MCP schema still accepts the ID until the handler rejects it, which matches how the existing uniqueness rule behaves. ## Validation performed - `npx vitest run src/domain/browserScenario.test.ts` before the source change — `1 failed | 4 passed (5)`; after — green. - `npx vitest run src/domain/browserScenario.test.ts src/browser/PlaywrightBrowserScenarioProvider.test.ts src/domain/browserScenarioDiff.test.ts tests/boundary/mcp/browserScenarioMcp.test.ts tests/boundary/cli/browserCli.test.ts` — `Test Files 5 passed (5)`, `Tests 51 passed (51)` (re-run after formatting). - Entry-surface sweep: `npx vitest run tests/boundary/cli/ tests/boundary/mcp/` — `Test Files 213 passed | 2 skipped`, `Tests 1184 passed | 7 skipped`; the two initially failing files (`contractPresentation`, `toolSchemaValidity`) failed only on the gitignored generated `.cache/mcp-tool-catalog.json`, then passed (14 tests) after `node scripts/generate-mcp-catalog.mjs`, so they are not affected by this patch. - `npm run build` — exit 0 (Node 24.18.0), needed for the CLI proof. - `npm run typecheck` — exit 0; `npm run lint` — exit 0 (`Checked resolved module boundaries in 1878 source files.`). - `./node_modules/.bin/oxfmt --check <the four changed files>` — first pass flagged one file, fixed with `oxfmt` (not prettier), re-check exit 0, `All matched files use the correct format.` - `npm run jscpd` with the change — `Duplication: 2715 files, 2006 clones, 3.22%`; the same tree with the change reverted — byte-identical `2715 files, 2006 clones, 3.22%`, so this patch contributes zero clones (`.jscpd.json` threshold 3.5). - Not run: full `check:ci`, real browser scenario execution, other platforms. ## Compatibility, safety, and release - Breaking changes or migration steps: a previously accepted input is now rejected. That input could only produce an ambiguous capture, so callers hit by the rejection were already losing the comparison they asked for; the error names the offending action index and is `retryable: true`. No previously valid, non-colliding call changes behavior. - Real Hopper/Ghidra, browser, or OS coverage: no provider or platform path touched; CLI validation proof executed on macOS arm64. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new commands, no new process launches; strictly an earlier refusal. ## Author readiness Human owner: @NIU-123370 - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [ ] This PR addresses a concrete problem or an agreed enhancement. - [ ] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [ ] The PR has one focused outcome and the title follows `type(scope): outcome`. - [ ] Related issue is linked, or the reason for not linking one is stated above. - [ ] Tests cover changed observable behavior and meaningful failure paths. - [ ] Owning docs, contracts, and generated metadata are updated where needed. - [ ] User-visible CLI/MCP changes include representative output. - [ ] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:09:22 +00:00