Repository navigation
[Remove] site registry formatter alias - #438
Conversation
|
Warning Review limit reachedNext included review available in 39 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
|
|
@coderabbitai review |
|
04a53e4 to
de3f2fc
Compare
de3f2fc to
d74dd38
Compare
|
Self-review completed: 0 [fix here], 0 functional [follow-up]. Fresh independent judgement verified shared formatting and site-refusal integration coverage remain. Local lint and all 1,248 tests pass on Node 24.18.0. The stale logging comment is corrected in #440. CodeRabbit has no inline findings; attempts are rate limited. |
## Why The regular journeys use local fixtures, so they cannot catch failures in a real WordPress download, dependency installation or full build. Automate that occasional manual check without adding network setup time to every PR. ## What changes - Add a manual-only GitHub Actions workflow with macOS, Windows or both as the platform choice. - Drive the source app through site creation, its automatic clone/install/build chain, and dev-server startup; verify the WordPress login form over HTTP. - Reuse the isolated Electron session harness, keep logs and reports for seven days, and exclude generated Playwright output from ESLint. - Use a separate Playwright config and explicit environment opt-in. No push, PR or scheduled trigger; no automatic retries or packaged-app changes. ## How to test this **Platforms:** macOS and Windows. This changes test tooling only, with no application UI changes. **Starting state:** this branch checked out with its own dependencies installed (`npm ci`) and renderer built (`npm run build:once`). Allow network access, several GB of disk and potentially tens of minutes. 1. Run `npx --no-install playwright test --list`: the real-setup test must not appear. Run `npx --no-install playwright test --config tests/e2e/real-setup.config.js` without the opt-in: it must report one skipped test. 2. On macOS run `TOOLKIT_REAL_SETUP=1 npx --no-install playwright test --config tests/e2e/real-setup.config.js`. On PowerShell first run `$env:TOOLKIT_REAL_SETUP = '1'`, then the same Playwright command, and remove the variable afterwards with `Remove-Item Env:TOOLKIT_REAL_SETUP`. 3. The app opens with an empty temporary profile, uses **Create WordPress Core site**, and automatically downloads, installs and builds. The test then starts the dev server, verifies HTTP 200 and the WordPress login form, stops the processes and closes the app. Expect one passing test and an HTML report under `playwright-report/real-setup`. 4. Once the workflow is on `trunk`, open **Actions → Real WordPress setup → Run workflow**, select the branch and platform, and run it. Expect one job per selected platform and downloadable `playwright-real-setup-<platform>` artifacts. **What must not have happened:** existing sites or the normal site registry changed; the real-setup job started on a push or PR; the network test joined the regular test suites. ## Risks and limitations The real source-app flow passed locally on macOS in 51.2 seconds; this is not a CI runtime estimate. Windows and hosted workflow execution remain unverified. The workflow becomes manually discoverable after it reaches the default branch. Upstream WordPress, npm and network failures can fail the run independently of Toolkit. The test is limited to 45 minutes and the job to 60 minutes, with no retries. Cleanup is best-effort through the existing fixture; forced cancellation can interrupt it. This does not exercise the signed installer or native folder chooser, which the harness answers automatically. ## Related Follow-up to the audit's real-setup coverage gap and the test guardrails in #443. --- <details> <summary>Review outcome (required — see AGENTS.md)</summary> 0 [fix here] · 0 [follow-up]. No findings across architecture, security, performance, cross-platform and tests. - **Review:** completed — fresh agent `review_manual_setup`, following the repository self-review skill and canonical review standard. Reviewed HEAD `c1533bde92f403ecfd7f097974462baf4065a848` plus all five uncommitted files, against confirmed no-PR target `trunk` at `d06630e471939877201a8941c18548f08705b683`. Inspected relevant harness and application code. The intervening base commit only removes an unused external-URL predicate and does not interfere with this change. - **Since review:** committed the reviewed work unchanged as `ef87625a3ca3cbd52920a524f5e7fbda57342964`; verified the five-file, 171-line diff and clean working tree. No later code changes. - **Validation:** `npm run lint`, all 1,251 `npm test` tests, `git diff --check`, YAML parsing/manual-only trigger check, default/separate Playwright discovery, explicit opt-out skip, and a real macOS setup/HTTP pass. Windows and hosted Actions execution have not run. - **Pre-merge update:** Codex inspected `ef87625a3ca3cbd52920a524f5e7fbda57342964 → dbfaef2` after merging current `trunk` (`f7f6069d0e842342edb5d9f15df6d651994ffa44`) without conflicts. The feature diff has the same stable patch ID before and after; the new base changes only remove unused wrappers and consolidate/remove tests, with no effect on the setup flow. No new findings. Lint and all 1,248 tests pass on the updated head; the count changed because of those upstream test cleanups. Hosted CI is rerunning for this head. - **CodeRabbit:** review did not run due to its usage limit, despite its successful status check. The completed independent review above is the review evidence; the status is not counted as a review. - **Further base updates:** Codex inspected the subsequent merges through head `be75d73fb17359ed2029e7221a0c62c5047233b2`, base `f85e6e28eb7cf0219b92b2ce5f6f5e73923862ed`. Upstream #438 removes a redundant site-log formatter alias; #441 removes unused patch exports. Neither changes this flow. The feature patch remains identical (stable patch ID `657ce39a26f1baeb1f3fd28a681792fdeb6a7960`), with no conflicts or new findings. Lint and all 1,245 current tests pass locally; hosted checks rerun for this final head. The real-setup workflow itself still awaits its first manual hosted run. </details>
Why
site-registryonly forwarded refused paths to the sharedsafe-log.describeRefused()formatter. That alias duplicated the formatter API and its direct formatter tests without providing site-registry-specific behaviour.What changes
Refusal paths in the delete, reveal, and debug-log guards now call
describeRefused()directly. The local alias/export and its formatter-only assertions are removed; behaviour-level refusal coverage, including the newline-forgery case, remains.How to test this
Platforms: any — this is a Node-only refactor with no user-visible surface.
Starting state:
Install the repository dependencies.
Run
node --test tests/unit/site-registry.test.cjs; all 16 tests pass, including the rejected-path newline-forgery behaviour.Run
npm run lintandnpm test; both complete successfully.What must not have happened:
No registry refusal may reach its injected logger with an unescaped newline, and no delete, reveal, or debug-log operation may gain access to an unregistered path.
Risks and limitations
Low risk: the changed call sites retain the same shared formatter and the refusal-behaviour test remains. Independent self-review found 1 [fix here] architecture documentation issue in
src/logging.js: its comment still names the removed alias; this PR intentionally leaves it untouched because #430 is explicitly scoped tosrc/site-registry.jsand its unit test.Related
Fixes #430
Design decisions and alternatives considered
Inlining preserves the single safe formatter in
safe-log.jsand removes an API that added no registry-specific meaning. The formatter module's own tests remain the source of truth for escaping and truncation.Review outcome (required — see AGENTS.md)
1 [fix here] · 0 [follow-up] — no functional findings; the one documentation finding is deferred because the task explicitly restricts the change to two files.
Implementation notes
Validation passed:
node --test tests/unit/site-registry.test.cjs(16 tests),npm run lint, andnpm test(1,255 tests). The pre-existing uncommittedpackage-lock.jsonchange was not staged or included.