[PR #7191] [CLOSED] fix(mcp): advertise capture_browser_scenario patterns that compile in v mode #7707

Closed
opened 2026-11-03 13:39:47 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/7191
Author: @zzrs-xxxr
Created: 10/26/2026
Status: ❌ Closed

Base: main ← Head: fix/browser-scenario-vmode-patterns


📝 Commits (1)

  • 901a436 fix(mcp): advertise capture_browser_scenario patterns that compile in v mode

📊 Changes

2 files changed (+99 additions, -2 deletions)

View changed files

📝 src/domain/browserScenarioValues.ts (+2 -2)
➕ tests/boundary/mcp/fullProfilePatternsVMode.test.ts (+97 -0)

📄 Description

Summary

With REA_MCP_INPUT_SCHEMA_PROFILE=full, capture_browser_scenario advertises two patterns that fail to compile in JavaScript's Unicode-set (v) regex mode, so a client that validates advertised patterns rejects the complete tools/list response, not just the affected call. Fixes #7168.

Problem and expected behavior

Trigger: start the MCP server with REA_MCP_INPUT_SCHEMA_PROFILE=full and list tools from a client that compiles advertised format: regex patterns in v mode (or validates them with an RE2/Rust engine).

  • The request step path pattern /^\/(?![/\\])/u uses a lookahead (refused by RE2 and Rust validators) and leaves an unescaped / inside a character class (v mode: Invalid character in character class).
  • The request header name pattern leaves | and the trailing - unescaped inside a character class; v mode treats both as class syntax.

Expected: every advertised pattern compiles in "", u, and v modes and passes RE2-style validators, as the compact-profile boundary already enforces.

Change and scope

Respell both patterns with the hex-escape convention this file already uses (the package-name pattern and HEADER_FIELD_VALUE):

  • path: /^\x2F(?:[^\x2F\x5C]|$)/u — accepts / followed by any character except / or \, or nothing. Equivalent to the old lookahead: both accept / alone and reject //, /\, and a leading \.
  • header name: /^[!#$%&'*+.^_`\x7C~0-9A-Za-z\x2D]+$/u — same character set, with | and - spelled as escapes.

Add tests/boundary/mcp/fullProfilePatternsVMode.test.ts, which builds the server under the full profile and asserts every advertised capture_browser_scenario pattern compiles in all three JS modes and carries no look-around. The win32 host-absolute localPath pattern advertised for browser.executable_path is excluded and left to #7151; a comment records that.

Not included: the #7151 localPath pattern itself, and any change to the compact catalog.

Scope decision: not applicable (ordinary fix within existing boundaries; no new providers, execution, or injection).

Contract and boundary impact

  • Semantic owner and earliest changed stage: src/domain/browserScenarioValues.ts scenario value schemas; consumers are the full-profile MCP input schemas and scenario validation. Accepted language is unchanged.

  • Contribution scope fit and linked maintainer decision: ordinary fix; not applicable.

  • CLI and MCP/tool-catalog contract: two advertised pattern source strings change text; the accepted language is identical, so validation behavior is unchanged. Strict v-mode/RE2 clients now accept the full-profile capture_browser_scenario schema.

  • Provider, bridge, target-format, or platform compatibility: none.

  • Evidence, artifact, provenance, or reconstruction contract: none.

  • Target selection, authority, upstream behavior, and cleanup details: not applicable.

  • Generated metadata, package, or installation impact: none.

  • 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: tests/boundary/mcp/fullProfilePatternsVMode.test.ts (full-profile boundary; the existing advertisedPatternsVMode.test.ts covers only the default compact profile and could not observe these patterns).
  • Base reproduction: on the base revision the new test fails with both patterns reported under v flags:
    • capture_browser_scenario inputSchema/.../path/pattern /^\/(?![/\\])/v: Invalid regular expression: Invalid character in character class
    • capture_browser_scenario inputSchema/.../name/pattern ^[!#$%&'*+.^_|~0-9A-Za-z-]+$v: Invalid regular expression: Invalid character in character class`
      With the fix the test passes.
  • Equivalence: I re-ran the issue author's exhaustive comparison locally — all strings up to length 4 over / \ a NUL for path, and up to length 3 over the full header alphabet plus rejection characters (,, :, space, tab) for name — zero mismatches with the old patterns.
  • User-visible CLI/MCP output: tools/list under the full profile now returns schemas that strict clients accept; no other output changes.
  • Remaining proof gaps: no live RE2/Rust client was exercised here; the lookahead/lookbehind prohibition is asserted in the new boundary instead.

Boundary regressions: new full-profile advertised-pattern boundary above. Lifecycle regressions: not applicable (schema text only).

  • Observed, derived, and inferred claims remain distinguishable.
  • Artifact identity, source provenance, and failed attempts remain preserved.
  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

  • node scripts/run-exact-tests.mjs run tests/boundary/mcp/fullProfilePatternsVMode.test.ts — 1 passed (fails on base with both patterns uncompilable in v mode)
  • node scripts/run-exact-tests.mjs run src/domain/browserScenario.test.ts src/domain/browserScenarioCapture.test.ts — 142 passed
  • npx oxlint + npx prettier --check on both changed files — clean
  • Ran on Windows; the stdio-based browserScenarioMcp/browserScenarioInputDiagnosticsMcp suites fail identically before and after this change (Windows source-checkout limits; the main suite runs in Linux CI), verified by stashing the fix.

Compatibility, safety, and release

  • Breaking changes or migration steps: none. The accepted language of both patterns is unchanged. Any client that pinned the exact old pattern text (none in this repo) would see different .source strings.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable (schema validation only).
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: no behavior change to what the server accepts or executes; header-name and path validation remain identical.

Author readiness

Human owner: @zzrs-xxxr

  • 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/7191 **Author:** [@zzrs-xxxr](https://github.com/zzrs-xxxr) **Created:** 10/26/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/browser-scenario-vmode-patterns` --- ### 📝 Commits (1) - [`901a436`](https://github.com/morluto/rea/commit/901a4368e1f1733887e7c8413846f31f1d29f548) fix(mcp): advertise capture_browser_scenario patterns that compile in v mode ### 📊 Changes **2 files changed** (+99 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/browserScenarioValues.ts` (+2 -2) ➕ `tests/boundary/mcp/fullProfilePatternsVMode.test.ts` (+97 -0) </details> ### 📄 Description ## Summary With `REA_MCP_INPUT_SCHEMA_PROFILE=full`, `capture_browser_scenario` advertises two patterns that fail to compile in JavaScript's Unicode-set (`v`) regex mode, so a client that validates advertised patterns rejects the complete `tools/list` response, not just the affected call. Fixes #7168. ## Problem and expected behavior Trigger: start the MCP server with `REA_MCP_INPUT_SCHEMA_PROFILE=full` and list tools from a client that compiles advertised `format: regex` patterns in `v` mode (or validates them with an RE2/Rust engine). - The `request` step `path` pattern `/^\/(?![/\\])/u` uses a lookahead (refused by RE2 and Rust validators) and leaves an unescaped `/` inside a character class (`v` mode: `Invalid character in character class`). - The request header `name` pattern leaves `|` and the trailing `-` unescaped inside a character class; `v` mode treats both as class syntax. Expected: every advertised pattern compiles in `""`, `u`, and `v` modes and passes RE2-style validators, as the compact-profile boundary already enforces. ## Change and scope Respell both patterns with the hex-escape convention this file already uses (the package-name pattern and `HEADER_FIELD_VALUE`): - `path`: `/^\x2F(?:[^\x2F\x5C]|$)/u` — accepts `/` followed by any character except `/` or `\`, or nothing. Equivalent to the old lookahead: both accept `/` alone and reject `//`, `/\`, and a leading `\`. - header `name`: `` /^[!#$%&'*+.^_`\x7C~0-9A-Za-z\x2D]+$/u `` — same character set, with `|` and `-` spelled as escapes. Add `tests/boundary/mcp/fullProfilePatternsVMode.test.ts`, which builds the server under the full profile and asserts every advertised `capture_browser_scenario` pattern compiles in all three JS modes and carries no look-around. The win32 host-absolute `localPath` pattern advertised for `browser.executable_path` is excluded and left to #7151; a comment records that. Not included: the #7151 localPath pattern itself, and any change to the compact catalog. Scope decision: not applicable (ordinary fix within existing boundaries; no new providers, execution, or injection). ## Contract and boundary impact - Semantic owner and earliest changed stage: `src/domain/browserScenarioValues.ts` scenario value schemas; consumers are the full-profile MCP input schemas and scenario validation. Accepted language is unchanged. - Contribution scope fit and linked maintainer decision: ordinary fix; not applicable. - CLI and MCP/tool-catalog contract: two advertised `pattern` source strings change text; the accepted language is identical, so validation behavior is unchanged. Strict `v`-mode/RE2 clients now accept the full-profile `capture_browser_scenario` schema. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata, package, or installation impact: none. - [ ] 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: `tests/boundary/mcp/fullProfilePatternsVMode.test.ts` (full-profile boundary; the existing `advertisedPatternsVMode.test.ts` covers only the default compact profile and could not observe these patterns). - Base reproduction: on the base revision the new test fails with both patterns reported under `v` flags: - `capture_browser_scenario inputSchema/.../path/pattern /^\/(?![/\\])/v: Invalid regular expression: Invalid character in character class` - `capture_browser_scenario inputSchema/.../name/pattern ^[!#$%&'*+.^_`|~0-9A-Za-z-]+$v: Invalid regular expression: Invalid character in character class` With the fix the test passes. - Equivalence: I re-ran the issue author's exhaustive comparison locally — all strings up to length 4 over `/ \ a NUL` for `path`, and up to length 3 over the full header alphabet plus rejection characters (`,`, `:`, space, tab) for `name` — zero mismatches with the old patterns. - User-visible CLI/MCP output: `tools/list` under the full profile now returns schemas that strict clients accept; no other output changes. - Remaining proof gaps: no live RE2/Rust client was exercised here; the lookahead/lookbehind prohibition is asserted in the new boundary instead. Boundary regressions: new full-profile advertised-pattern boundary above. Lifecycle regressions: not applicable (schema text only). - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `node scripts/run-exact-tests.mjs run tests/boundary/mcp/fullProfilePatternsVMode.test.ts` — 1 passed (fails on base with both patterns uncompilable in `v` mode) - `node scripts/run-exact-tests.mjs run src/domain/browserScenario.test.ts src/domain/browserScenarioCapture.test.ts` — 142 passed - `npx oxlint` + `npx prettier --check` on both changed files — clean - Ran on Windows; the stdio-based `browserScenarioMcp`/`browserScenarioInputDiagnosticsMcp` suites fail identically before and after this change (Windows source-checkout limits; the main suite runs in Linux CI), verified by stashing the fix. ## Compatibility, safety, and release - Breaking changes or migration steps: none. The accepted language of both patterns is unchanged. Any client that pinned the exact old pattern text (none in this repo) would see different `.source` strings. - Real Hopper/Ghidra, browser, or OS coverage: not applicable (schema validation only). - Package or release metadata impact: none. - Security, privacy, process, or containment review: no behavior change to what the server accepts or executes; header-name and path validation remain identical. ## Author readiness Human owner: @zzrs-xxxr - [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 - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] 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-11-03 13:39:47 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
morluto/rea#7707
No description provided.