Skip to content

[Refactor] centralize safe log formatter tests, Fixes #429 - #440

Open
juanmaguitar wants to merge 1 commit into
juanmaguitar/cleanup-428from
juanmaguitar/cleanup-429
Open

[Refactor] centralize safe log formatter tests, Fixes #429#440
juanmaguitar wants to merge 1 commit into
juanmaguitar/cleanup-428from
juanmaguitar/cleanup-429

Conversation

@juanmaguitar

Copy link
Copy Markdown
Collaborator

Why

external-url duplicated formatter-only coverage through describeRefusedUrl(), even though the shared safe-log module owns the one-line, bounded rendering contract.

What changes

Removes the forwarding export and calls describeRefused() directly at the URL refusal boundary. Formatter coverage now exercises safe-log directly; external-url retains URL normalization and refusal-behavior coverage.

How to test this

Platforms: any — this is a unit-test-only refactor with no visible UI change.

Starting state:

  1. Check out this branch on top of juanmaguitar/cleanup-428.

  2. Install the repository dependencies.

  3. Run node --test tests/unit/external-url.test.cjs tests/unit/safe-log.test.cjs.

  4. Run npm run lint and npm test.

What must not have happened:

Refused file:, custom, malformed, and otherwise non-HTTP(S) URLs must still not reach shell.openExternal; refused values must remain one-line and bounded. The focused tests cover this directly, and the full suite covers the integration boundary.

Risks and limitations

No user-visible behavior changed. This PR is stacked on #428 and must merge after it; the review found 0 [fix here] and 0 [follow-up] findings.

Related

Fixes #429


Design decisions and alternatives considered

The formatter belongs to safe-log, where it is shared by URL, site, and editor refusal paths. Keeping a URL-specific wrapper would preserve a second public surface without adding behavior.

Review outcome (required — see AGENTS.md)

0 [fix here] · 0 [follow-up] — no findings across architecture, security, performance, cross-platform, or tests. npm run lint and npm test passed (1,258 tests).

Implementation notes

The branch base is juanmaguitar/cleanup-428 so #428 owns removal of the test-only external URL predicate. This change only centralizes the safe-log formatter tests and removes the forwarding alias.

@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

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)
  • trunk

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: efca4fee-e2a1-4b1d-ba76-75ece01f6552

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

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.

1 participant