Ship: Playwright MCP — first-class browser harness#1711
Open
rapiercraft-forge[bot] wants to merge 32 commits into
Open
Ship: Playwright MCP — first-class browser harness#1711rapiercraft-forge[bot] wants to merge 32 commits into
rapiercraft-forge[bot] wants to merge 32 commits into
Conversation
…ify-lane.sh (#852) classify-lane.sh is executed (not sourced), so exports to FORGEDOCK_SCRIPTS and FORGEDOCK_HOME cannot propagate to the parent shell. The script also does not call any per-repo adaptive scripts, so child-process inheritance is unused. The 6-line export block (introduced in PR #670) is unreachable dead code — remove it.
…oot (#849) Add realpath -m canonicalization and a bounds check to the ADAPTIVE_DIR initialization block. If the resolved path does not begin with REPO_PATH/, ADAPTIVE_ENABLED is set to false, silently disabling the adaptive tier. This prevents path traversal via malicious forge.yaml directory values.
…alidate-848 Fix: validate FORGE_LABEL_MAP value before gh --add-label
…d-exports-852 chore(scripts): remove dead FORGEDOCK_SCRIPTS/HOME exports from classify-lane.sh
…-traversal-849 Fix: validate adaptive_scripts.directory stays within repo root
…844) Add else-if content-match branch in the commands removal loop of uninstall() so regular files created by the EPERM/EACCES copyFile fallback on Windows are cleaned up. Mirrors the pattern already used in the scripts removal loop. Also update the catch block to re-throw non-ENOENT errors instead of swallowing them silently.
…handler (#845) Add bounds check before unconditional index advance past block comment terminator. When a block comment is unterminated, the while loop exits with i at len-1; the previous unconditional i += 2 set i to len+1, causing the outer loop to exit and silently discard all content after the comment. Fix applied to both the main handler and the trailing-comma lookahead copy.
…de-commands-844 fix(installer): remove Windows copy-mode command files on uninstall
…inated-block-comment-845 Fix: guard post-loop i += 2 in stripJsonc() block comment handler
…ForgeHomeFromProfile fails (#846) Check the return value of removeForgeHomeFromProfile() and skip the subsequent appendFileSync() call when it returns 'failed'. Previously the return value was ignored, causing a duplicate export FORGE_HOME= line to be appended to the shell profile when the removal failed.
#847) Replace `[^"]*` with `(?:[^"\\]|\\.)*` in the Step 1 regex so the removal pattern correctly handles backslash-escape sequences written by shellEscapeDoubleQuotedPath() — e.g. \" for paths containing a double-quote character. The previous pattern stopped at the literal " inside \", causing removal to silently no-op and duplicate export blocks to accumulate on reinstall.
…-failed-guard-846 Fix: guard against duplicate FORGE_HOME export when removeForgeHomeFromProfile fails
…egex-escaped-quotes-847 Fix: use escape-aware regex in removeForgeHomeFromProfile() to handle paths with special chars
…851) String.prototype.replace() returns the original string for both "content already current" and "regex did not match". The prior comment claimed the regex always matched, but this was implicit — relying on the outer hasBegin/hasEnd/indexOf guards without asserting it. Add an explicit blockRegex.test() check that throws when the regex fails to match, so a regression in the outer guards cannot silently return 'unchanged' for a no-match case. Update comment on replaced === existing to accurately describe content-already-identical semantics.
Pre-compute safe* escaped variants of owner, repo, projectName, defaultBranch, and stagingBranch before embedding them into the forge.yaml template string. Consistent with the existing escaping already applied to description, cwd, and worktreeBase.
…k-no-match-guard-851 fix(installer): add explicit no-match guard in injectManagedBlock() (#851)
…ape-owner-repo-850 fix(installer): escape owner/repo/branch in forge.yaml template
Document that Playwright MCP (MIT) runs as a separate process over the MCP protocol — data contract only, no import/vendor/link — so it introduces no AGPL contamination. Adds a general decision rule for third-party MCP dependencies and notes platform observability angle.
…ity signals (#875) Add Phase V3.6 (Browser Signal Check) to work-on/build/validate.md — triggers when TS/TSX files change, runs Playwright MCP tools (browser_navigate, browser_console_messages, browser_network_requests, browser_evaluate) against services.app_url to surface console errors, network failures, and load time regressions. Skips gracefully when app_url absent. Extend Frontend Quality Auditor in review-pr-agents.md with a Browser Signal Check block that performs the same 4-step live browser check during PR review. Updates the coverage matrix to mark console/network/perf signals as COVERED. Closes #875
#873) Add Playwright MCP registration to the ForgeDock installer following the established SessionStart hook patterns: - CLAUDE_MCP_PATH constant pointing to ~/.claude/mcp_servers.json - readMcpServers() / writeMcpServers() helpers with atomic write (ref: #813) - detectNpx() — cross-platform npx availability check (execFileSync args array) - installPlaywrightMcp() — idempotent registration of playwright key - removePlaywrightMcp() — idempotent removal (symmetric with uninstall) - New install() step: "Registering Playwright MCP" — skips gracefully if npx absent, registers entry, runs lightweight version verification (non-fatal) - New uninstall() step: removes playwright key from mcp_servers.json - doctor() Check 9: verifies playwright key present in mcp_servers.json All exec calls use execFileSync with hardcoded args arrays (no shell injection). All JSON writes use atomicWriteFile() (tmp + rename, no partial writes).
…pen-core-boundary-876 docs(architecture): add Playwright MCP open-core boundary note
…egister-873 feat(installer): register + verify Playwright MCP in bin/forgedock.mjs
…ty-signals-875 Feat: expose Playwright perf/console/network as quality signals in validate and review
…yleft-scope-895 Fix: scope MCP AGPL clearance to linking-based contamination
…ection in MCP boundary section (#896)
…ation-direction-mcp-boundary-896 Docs: distinguish inbound vs outbound contamination direction in MCP boundary section
…n MCP section (#925) The phrase 'the same process-isolation reasoning resolves both' mis- characterized the outbound Boundary Rules mechanism. Boundary Rules (this repo's AGPL → proprietary Platform) rely on a no-cross-linking/ data-contract discipline between two first-party repos — not on a process boundary. Only the inbound (third-party MCP) direction relies on the process boundary. 'boundary-isolation' accurately covers both: code-boundary (outbound) and process-boundary (inbound), without over-specifying either.
…-reasoning-925 fix(devdocs): replace 'process-isolation' with 'boundary-isolation' in MCP boundary section
…ht-mcp-first-class-browser-harness # Conflicts: # bin/forgedock.mjs # commands/review-pr-agents.md # commands/work-on.md # devdocs/project/architecture.md
Contributor
Author
PR Review Summary: #1711 — Ship: Playwright MCP — first-class browser harnessVerdict: APPROVEDChanges ReviewedSingle file: Adds Phase V3.6: Browser Signal Check — a new advisory validation phase that uses Playwright MCP to capture console errors, network failures, and performance metrics for UI changes. Well-structured:
No security concerns. No code execution — this is a spec document. FindingsNone. Clean review. Context-aware review complete. 0 findings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Milestone: Playwright MCP — first-class browser harness
Make Playwright MCP a first-class, installer-guaranteed dependency. Installer registers/verifies it; specs document it.
Issues Included
Hunk-Loss Audit
✓ Clean — no at-risk staging-branch hunks detected. Safe to merge.
Diff Summary
commands/work-on/build/validate.md | 62 +++++++++++++++++++++++++++++++++++++-
1 file changed, 61 insertions(+), 1 deletion(-)
Commits
27b4f07 Merge remote-tracking branch 'origin/staging' into milestone/playwright-mcp-first-class-browser-harness
c17d876 Merge pull request #966 from RapierCraftStudios/fix/process-isolation-reasoning-925
ec4366a fix(devdocs): replace 'process-isolation' with 'boundary-isolation' in MCP section (#925)
a5c9382 Merge pull request #921 from RapierCraftStudios/docs/clarify-contamination-direction-mcp-boundary-896
eef0396 docs(architecture): distinguish inbound vs outbound contamination direction in MCP boundary section (#896)
75e860c Merge pull request #914 from RapierCraftStudios/fix/decision-rule-copyleft-scope-895
22a106a fix(docs): scope MCP AGPL clearance to linking-based contamination (#895)
3362925 Merge pull request #891 from RapierCraftStudios/feat/playwright-quality-signals-875
bfc3647 Merge pull request #892 from RapierCraftStudios/feat/playwright-mcp-register-873
da6b89e Merge pull request #889 from RapierCraftStudios/docs/playwright-mcp-open-core-boundary-876
340d3fc docs(architecture): correct Playwright MCP license to Apache-2.0 (#893, #894)
e16357a feat(installer): register + verify Playwright MCP in bin/forgedock.mjs (#873)
ec45879 feat(validate/review): expose Playwright perf/console/network as quality signals (#875)
5b2447e docs(architecture): add Playwright MCP open-core boundary note (#876)
c6dfedf Merge pull request #862 from RapierCraftStudios/fix/yaml-template-escape-owner-repo-850
f609dff Merge pull request #861 from RapierCraftStudios/fix/injectmanagedblock-no-match-guard-851
5f21955 fix(installer): escape owner/repo/branch in forge.yaml template (#850)
55d3b7c fix(installer): add explicit no-match guard in injectManagedBlock() (#851)
0f2fdc1 Merge pull request #860 from RapierCraftStudios/fix/removeforgehome-regex-escaped-quotes-847
20670ee Merge pull request #859 from RapierCraftStudios/fix/remove-forge-home-failed-guard-846
575656e fix(installer): use escape-aware regex in removeForgeHomeFromProfile() (#847)
8732bfb fix(installer): guard against duplicate FORGE_HOME export when removeForgeHomeFromProfile fails (#846)
0dc4426 Merge pull request #858 from RapierCraftStudios/fix/stripjsonc-unterminated-block-comment-845
c816bf1 Merge pull request #857 from RapierCraftStudios/fix/uninstall-copy-mode-commands-844
f737826 fix(installer): guard post-loop i += 2 in stripJsonc() block comment handler (#845)
503b1c0 fix(installer): remove Windows copy-mode command files on uninstall (#844)
85641ad Merge pull request #855 from RapierCraftStudios/fix/adaptive-dir-path-traversal-849
6716802 Merge pull request #854 from RapierCraftStudios/fix/classify-lane-dead-exports-852
b4a894f Merge pull request #853 from RapierCraftStudios/fix/forge-label-map-validate-848
88134e6 fix(work-on): validate adaptive_scripts.directory stays within repo root (#849)
bf1d23e chore(scripts): remove dead FORGEDOCK_SCRIPTS/HOME exports from classify-lane.sh (#852)
3eadd2f fix(scripts): validate FORGE_LABEL_MAP value before gh --add-label (#848)