Retire the CI review workflow; keep the standard it applied - #125
Merged
Conversation
juanmaguitar
force-pushed
the
remove-ci-ai-review
branch
from
August 6, 2026 07:49
e052d04 to
10c5d3b
Compare
The workflow needed a personal Claude token stored as a secret in a public repository. Repo secrets are readable by anything running on trunk and by every repo admin, and that is not a reasonable place to keep a credential tied to one person's account. It was also narrower than it looked: fork PRs never receive secrets, so its real coverage was the maintainers' own pull requests — the same ones the pre-PR self-review covers. The review standard itself is unaffected, and moves to .github/instructions/code-review.instructions.md. That path is not prettier than the old one, but Copilot code review reads it natively — selecting it by matching its applyTo glob against the files in a PR — so the standard stays a single copy instead of growing a condensed duplicate for the one reader that cannot follow a link. It now carries the procedure as well as the rules, so any agent can run it without a tool-specific file, and the self-review skill becomes a thin entry point over it. One thing the workflow gave for free was a reviewer that had not just written the code. The procedure asks for that pass to run with fresh context where the tool allows it, and the skill dispatches it to a subagent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
juanmaguitar
force-pushed
the
remove-ci-ai-review
branch
from
August 6, 2026 07:59
10c5d3b to
64c8b36
Compare
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.
Why
The workflow needed a personal Claude token stored as a repository secret in a public repo. Repo secrets are readable by anything running on
trunkand by every repo admin — not a reasonable place for a credential tied to one person's account.It was also narrower than it looked. Fork PRs never receive secrets, so its real coverage was the maintainers' own pull requests: the same ones a pre-PR self-review already covers.
What changes
.github/workflows/ai-review.ymlis deleted..github/ai-review-rules.mdmoves to.github/instructions/code-review.instructions.mdand gainsapplyTo: "**"frontmatter. Content is unchanged apart from the header and the procedure below.npm run lintandnpm test, then the five dimensions — so any agent can run it without a tool-specific file..claude/skills/self-review/SKILL.mdbecomes a thin entry point over it.lint.yml,eslint.config.mjs) referred to the old path or the deleted workflow.Why that path
Copilot code review reads
.github/instructions/*.instructions.mdnatively, selecting files by matching theirapplyToglob against the PR's contents. It does not follow links out of its instruction files.So the choice was: put the standard where Copilot finds it, or keep a nicer filename and maintain a condensed second copy in
.github/copilot-instructions.mdthat nothing keeps in step. The path is uglier and vendor-flavoured; one copy of the standard is worth more than that.Rebased on #119, which cleared the lint backlog and widened the CI check to
eslint .. That retired the changed-files-only lint dance the skill used to perform —npm run lintandnpm testare both clean ontrunk, so any failure belongs to the branch.What this gives up, and what replaces it
The workflow provided one thing for free: a reviewer that had not just written the code. An author reviewing in the same session reviews their own reasoning and finds it sound.
The procedure now asks for that pass to run with fresh context where the tool allows it, and the skill dispatches it to a subagent. It is weaker than an independent job, and it is a directive rather than a gate — that trade is deliberate, not overlooked.
A Copilot review can also be requested on a PR, or enabled per-account for your own PRs. Neither needs a stored credential.
Follow-up not covered here
Deleting the file does not delete the secret.
CLAUDE_CODE_OAUTH_TOKENstill needs removing in repository settings, and the token revoking.AGENTS.mdis referenced here and arrives in the stacked PR that follows.Self-review
Docs and workflow only, no JavaScript changed.
npm run lintclean, 147 tests pass. No findings across the five dimensions.