Make the agent instructions canonical in AGENTS.md - #127
Merged
Conversation
CLAUDE.md was the only place the project's constraints were written down, so every agent other than Claude Code started from nothing — on a codebase whose whole premise (zero host prerequisites, no git binary, one persistence layer) is invisible from the code alone. AGENTS.md is now canonical and CLAUDE.md points at it, keeping only what is genuinely Claude Code specific. AGENTS.md also carries the directive to run the review before opening a pull request, and is explicit that nothing enforces it. No Copilot file: the review standard already sits at a path Copilot reads natively, and everything else here is something it can be pointed at. Adding one would have meant a second copy of the invariants with nothing keeping the two in step. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 6, 2026
juanmaguitar
added a commit
that referenced
this pull request
Aug 6, 2026
Closes #116. `url:open` passed whatever it received to `shell.openExternal`, which hands an address to whatever application the OS has registered for its scheme — a wider action than "show this page in the browser". A `file:` address opens an arbitrary local path in its associated application (on Windows that can mean running it rather than viewing it), and any other registered scheme is reachable the same way. Every caller passes an http/https address today, so nothing misuses it. It matters as the second half of a chain: the renderer displays content the app does not author, and the URL the app auto-opens on server start is parsed out of the Playground server's stdout. This guard is the step that keeps any future influence over that string from becoming an action on the contributor's machine. ## What changed - **`src/external-url.js`** (new) — `ALLOWED_URL_SCHEMES = ['http:', 'https:']`, plus `isAllowedExternalUrl()` and `openExternalUrl()`. The scheme is read off a parsed `new URL()` rather than the raw string, so casing and leading whitespace (`FILE:`, ` file:`) normalize before the comparison instead of being a way around it; an address Node can't parse is refused rather than handed to the OS to interpret. What gets opened is the parser's own `href`, not the caller's string. That second half matters as much as the allow-list: the URL parser strips tabs and newlines from anywhere in the input, including the middle of the scheme, so `ht\ntp://example.com/x` validates as `http:` — and forwarding the raw text would hand the OS an address nothing had checked. For every caller here the two forms differ only by the trailing slash the parser adds to a bare origin. - **`src/main.js`** — the handler delegates to that module, passing `shell.openExternal` and an `onRefused` that logs through the existing `logEvent` (scope `url`). A refusal is visible in the log file people attach to bug reports, not silent, so a future caller that trips the guard is diagnosable. The logged address is truncated, since by hypothesis it is the influenced input. The logic lives in its own module, per the repo convention (`src/bind-loopback.js`), so both sides of the guard are testable without an Electron process. No user-facing behaviour changes: every existing caller passes http/https. ## Testing `test/external-url.test.cjs` — 11 tests, using a stub in place of `shell.openExternal` so "did this reach the OS?" is an assertion rather than something the test takes on trust: - The four address shapes the app actually passes (Trac, the feedback form, the site on an ephemeral loopback port, wp-admin) still open, and reach the stub in their normalized form. - A control-character case (`ht\ntp://example.com/x`) pins the normalization: it opens `http://example.com/x`, not the string carrying the newline. This test fails against the first commit of this PR, which forwarded the raw input. - `file:///etc/passwd` and `file:///C:/Windows/System32/cmd.exe` never reach the stub. The Windows one is the case that matters most: the OS association for a `.exe` is "run it". - `javascript:`, `data:`, `mailto:`, and OS-registered third-party schemes (`ms-msdt:`, `vscode:`) are refused too. - Junk input (`''`, whitespace, `null`, `undefined`, a number, an object, an array, `'not a url'`) is refused rather than thrown. - A refused address cannot forge a log line: control characters are escaped rather than passed through, so an address carrying a newline can't close its entry and open another one in the app's own timestamp-and-scope format. Truncation runs after escaping, since escaping is what decides the final length. - One test pins the allow-list itself, so widening it has to be a deliberate change that shows up in a diff. Suite: **158 passing, 0 failing**, on `node --test` and again on Electron's bundled Node via `npm run test:electron`. `npm run lint` is clean across the repo. ### To verify by hand Nothing in the UI can reach the refusal path by design, so the manual pass is about confirming the allowed side is unaffected. On a signed build from this branch: 1. Start a site — it should still auto-open in the browser as before. 2. Click the site URL and the **wp-admin** link in the site header — both open. 3. Open a site with the database tool and click through to **adminer** — opens. 4. Generate a patch and click the **core.trac.wordpress.org** link in the "Next steps" text — opens. 5. Click the **feedback form** link — opens. 6. Optional, to see the guard fire: from the renderer devtools console, run `window.api.openExternal('file:///etc/passwd')`. It should resolve `false`, nothing should open, and the log file (Help → the log path) should contain a `url` line reading `refused to open file:///etc/passwd — only http:, https: are allowed`. Also updates `.github/instructions/code-review.instructions.md`: it named this handler as its known-open calibration example, which stops being true here. Replaced with what the fix teaches, so the next review looks for both halves rather than re-reporting something already closed. Rebased on trunk after #122/#125/#127; the review standard's calibration example moved with it to `.github/instructions/`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Replaces #126, which GitHub closed when its base branch was deleted on merging #125. Same content, retargeted at
trunk.Why
CLAUDE.mdwas the only place this project's constraints were written down, so every agent other than Claude Code started from nothing. That matters more here than in most repos: the premise — zero host prerequisites, nogitbinary,electron-storeas the only persistence layer — is invisible from the code alone, and an agent that does not know it will reach forspawn('git')and be pleased with the result.What changes
AGENTS.mdis now canonical and tool-neutral. It carries whatCLAUDE.mdheld, plus the directive to run the review before opening a pull request and summarise the outcome in the description.CLAUDE.mdshrinks to a pointer plus the one genuinely Claude Code specific line, so there is no second copy to drift.No Copilot file
An earlier version of this added
.github/copilot-instructions.md, because Copilot reads neitherAGENTS.mdnorCLAUDE.md. It is gone: #125 puts the review standard at.github/instructions/code-review.instructions.md, which Copilot reads natively, and that is the part it actually needs. Everything else here is context an agent can be pointed at.Adding one anyway would have meant a second copy of the invariants with nothing keeping the two in step — the failure mode this PR exists to remove, reintroduced one file over.
Notes
AGENTS.mdsays plainly that nothing enforces the pre-PR review. It is a directive, and describing it as anything firmer would be false — worth keeping that way when the next person edits it.No stubs for Codex or Cursor: both read
AGENTS.mdalready, and per-tool files drift. Worth adding one when a tool is actually in use, not before.Self-review
Documentation only, no JavaScript changed.
npm run lintclean, 147 tests pass. No findings across the five dimensions.