Skip to content

[Remove] site registry formatter alias - #438

Merged
juanmaguitar merged 2 commits into
trunkfrom
juanmaguitar/cleanup-430
Sep 11, 2026
Merged

juanmaguitar merged 2 commits into
trunkfrom
juanmaguitar/cleanup-430

Conversation

@juanmaguitar

Copy link
Copy Markdown
Collaborator

Why

site-registry only forwarded refused paths to the shared safe-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:

  1. Install the repository dependencies.

  2. Run node --test tests/unit/site-registry.test.cjs; all 16 tests pass, including the rejected-path newline-forgery behaviour.

  3. Run npm run lint and npm 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 to src/site-registry.js and its unit test.

Related

Fixes #430


Design decisions and alternatives considered

Inlining preserves the single safe formatter in safe-log.js and 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, and npm test (1,255 tests). The pre-existing uncommitted package-lock.json change was not staged or included.

@juanmaguitar juanmaguitar added the audit-code Follow-up work from the codebase audit label Sep 10, 2026
@coderabbitai

coderabbitai Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 39 minutes.

Check out review usage here.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 0125a221-f8bd-4b0a-acc8-9a280e3a66f0

📥 Commits

Reviewing files that changed from the base of the PR and between f7f6069 and 6bb5c0d.

📒 Files selected for processing (2)
  • src/site-registry.js
  • tests/unit/site-registry.test.cjs

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@juanmaguitar

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 10, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@juanmaguitar

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 11, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@juanmaguitar
juanmaguitar force-pushed the juanmaguitar/cleanup-430 branch 4 times, most recently from 04a53e4 to de3f2fc Compare September 11, 2026 04:11
@juanmaguitar
juanmaguitar force-pushed the juanmaguitar/cleanup-430 branch from de3f2fc to d74dd38 Compare September 11, 2026 04:16
@juanmaguitar

Copy link
Copy Markdown
Collaborator Author

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.

@juanmaguitar
juanmaguitar merged commit 0ce7495 into trunk Sep 11, 2026
8 checks passed
@juanmaguitar
juanmaguitar deleted the juanmaguitar/cleanup-430 branch September 11, 2026 05:28
juanmaguitar added a commit that referenced this pull request Sep 11, 2026
## 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

audit-code Follow-up work from the codebase audit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove the site-registry formatter alias

1 participant