fix(outlook-addon): fail the build when the manifest URL rewrite matches nothing

[dobby-coder]

Closes #257.

What was wrong

apps/outlook-addon/webpack.config.js rewrote the dev origin out of the shipped manifest with a bare .replace() whose result nothing checked. On a no-match the build exited 0 and shipped a production dist/manifest.xml whose SourceLocation, IconUrl, HighResolutionIconUrl and every <bt:Url> still pointed at https://localhost:3000/.

Reproduced at HEAD before changing anything, with urlDev set to https://localhost:3999/ so the rewrite could not match:

Gate	Result
pnpm build	exit 0, no warning
pnpm validate:dist	"The manifest is valid."
grep -c localhost dist/manifest.xml	18

What changed

The rewrite moves to scripts/rewrite-manifest-urls.mjs so its guards can be tested without a webpack run. webpack.config.js keeps everything else about the transform, including the if (dev) return content; early return, so a development build still keeps its localhost URLs.

Two guards, because neither subsumes the other and the issue's preferred one alone would not have caught the reported bug:

• Replacement count. This is what catches a total no-match. Asserting instead that the output no longer contains urlDev cannot: a global replace already removes every occurrence of its own pattern, so rewritten.includes(urlDev) is false whether or not the rewrite did anything. And when urlDev is the string that is wrong, it is not what the manifest still holds anyway, so the poisoned-port mutation from the issue would sail past it.
• Leftover dev origin. This is what catches a partial rewrite. urlDev carries a trailing slash, so an entry written as the bare origin https://localhost:3000 is not matched by the replace and would survive a rewrite that reported plenty of replacements.

The regex itself, the transform's shape and scopeAppDomains() are untouched. Counting via a replace callback rather than a plain string replacement also stops a $-bearing urlProd from being read as a substitution pattern, which is behaviour-identical for real URLs.

One thing worth a maintainer's opinion: scripts/rewrite-manifest-urls.mjs is ESM rather than CJS because the CI lint and prettier globs enumerate scripts/**/*.mjs, so a .js helper there would have been checked by the pre-commit hook and by nothing in CI. webpack.config.js is CommonJS, so it reaches the helper through await import() inside the async config factory; require() of ESM would work on node 24 in CI but needs node >=22.12, while the root engines only asks for >=22. Changing the workflow globs instead is the alternative, and I cannot push .github/workflows/.

Verification

Acceptance check 1, the bug is caught. With line 8 set to https://localhost:3999/, the production build fails:

ERROR in manifest.xml contains no occurrence of https://localhost:3999/, so the rewrite to
https://addin.postguard.eu/ changed nothing. A production manifest that kept the dev origin
loads nothing in Outlook, and no later gate objects: the build exits 0, the manifest validates,
and scopeAppDomains() drops the unrewritten entry as unused. Make urlDev in webpack.config.js
match how the dev origin is spelled in manifest.xml.

Line 8 restored, everything below run against it:

Check	Result
pnpm build (production origins)	exit 0
grep -c localhost dist/manifest.xml	0
dist/manifest.xml vs. the pre-fix build	byte-identical
pnpm validate:dist	"The manifest is valid."
pnpm build + validate:dist (edge origins)	exit 0, valid
pnpm build:dev	exit 0, keeps its localhost URLs
pnpm typecheck	pass
pnpm test	43 pass, 0 fail
eslint / prettier, CI's own globs	clean
pnpm test and pnpm typecheck at the repo root	pass

The regression test is test/rewrite-manifest-urls.test.ts. Against the pre-fix bare .replace() 3 of its 4 cases fail; against this branch all 4 pass.

apps/outlook-addon/CLAUDE.md records why the rewrite has to assert its own result, the port-config trigger, and the scripts/**/*.mjs glob gotcha.

🤖 Generated with Claude Code

[dobby-coder]

VERDICT: approve

Gatekeeper sign-off, cycle 1. No blocking findings. I re-ran every acceptance check from #257 myself rather than reading the PR body, and mutation-tested the fix in both directions.

Acceptance check 1 — the bug is caught

With line 8 set to https://localhost:3999/, the production build fails (exit 1) and emits no dist/manifest.xml:

ERROR in manifest.xml contains no occurrence of https://localhost:3999/, so the rewrite to
https://addin.postguard.eu/ changed nothing. ...

Line 8 restored with the inverse edit; git status clean.

Acceptance check 2 — nothing else regresses

Each step run separately, read off its own evidence line:

Check	Result
pnpm build (production origins)	exit 0
grep -oE localhost dist/manifest.xml | wc -l	0
dist/manifest.xml vs. a build of webpack.config.js at e6777d6~1	byte-identical (diff clean)
pnpm validate:dist	"The manifest is valid."
pnpm build + validate:dist (edge origins)	exit 0, 0 localhost, valid
pnpm build:dev	exit 0, keeps its 19 localhost occurrences
pnpm test	# pass 43 # fail 0 # cancelled 0, exit 0
pnpm typecheck	exit 0
eslint / prettier, CI's own globs	exit 0 / "All matched files use Prettier code style!"

The regression test actually pins the fix

I replaced the helper body with the pre-fix bare .replace() and re-ran the suite:

ok 26 - rewrites every occurrence of the dev origin to the production one
not ok 27 - throws when the rewrite matches nothing
not ok 28 - throws when a differently spelled dev URL survives the rewrite
not ok 29 - reports the no-match, not a leftover, when urlDev matches nothing at all
# pass 40  # fail 3

3 of 4 go red, matching the PR body. The helper was restored from a backup copy, not git checkout.

The PR's pushback on the issue's preferred single guard is correct: a global replace removes every occurrence of its own pattern, so rewritten.includes(urlDev) is false whether or not the rewrite did anything — that assertion alone would have sailed past the reported poisoned-port bug. Keeping both guards, count first, is the right call, and test 29 pins which of the two errors a caller gets.

Rule check

Selected the rules that can fire on this diff (JS/TS build tooling, regression test, repo docs, bot-authored draft PR in a pnpm monorepo). Nothing to report:

• Docs accuracy — every claim the CLAUDE.md diff adds is true at this HEAD, checked against the source rather than the prose: the CI lint/prettier globs really do enumerate scripts/**/*.mjs and not .js (outlook-addon.yml:89,91); require() of ESM really does need node >=22.12 while the root engines asks only for >=22; Baked URLs resolve really does read only Dockerfile, webpack.config.js and the workflow env, never dist/manifest.xml; and manifest.xml:21 really does list https://addin.postguard.eu as its own <AppDomain> independently of the localhost entry on line 23, which is what makes scopeAppDomains()'s missing check structurally blind here. No prose describes an unlanded gate as live.
• Single call site — the CopyWebpackPlugin transform is the only producer of dist/manifest.xml in the repo; scripts/sync-version.mjs touches the source manifest's <Version> only, and nothing else rewrites its URLs. The guard is not one of several paths.
• No false-positive path for the leftover guard — it compares against new URL(urlDev).origin, i.e. https://localhost:3000. All 19 dev-origin occurrences in manifest.xml carry the trailing slash, so none survives a correct rewrite, and no build in the repo passes a localhost ADDIN_PUBLIC_URL (the Dockerfile ARG defaults to the production origin, both CI legs pass real hosts, and dev mode returns before the guard).
• Regression test committed, and it is an extract-then-test of exactly the kind that suits a transform inline in a webpack config.
• Repo-scoped lessons went to the repo's own CLAUDE.md, not to shared rules.
• Conventional-commit PR title — pr-title.yml runs here and Conventional Commit passes.
• No changeset — right by this repo's convention: the shipped manifest is byte-identical on a correct build, so there is no user-facing change, matching #226/#228/#229 (chore/docs/ci, no changeset) rather than #148/#157 (behaviour, changeset).
• No advisory — #257 carries no GHSA and repo memory marks nothing here embargoed, so the flip to ready is not held.

CI

19 of the 20 required checks pass. E2E Tests has been in_progress for ~35 minutes, wedged on its Install Playwright chromium step with pnpm test:e2e never started — an infrastructure stall, not a result. I am signing off over it rather than burning another gatekeeper cycle on a wait: it is a website job, and this diff touches only apps/outlook-addon/**, so it cannot be affected by the change. Worth a re-run if it does not settle on its own.

Two non-blocking observations, no change requested

• new RegExp(urlDev, "g") still interpolates urlDev unescaped (pre-existing, and #257 explicitly said not to restructure the transform). It is harmless for the current value — https://localhost:3000/ has no regex metacharacter — and the new guards do not catch the over-matching direction, only the under-matching one. Now that the transform is an importable unit, escaping it is a cheap belt-and-braces follow-up if you want it; it is not this PR's job.
• The Baked URLs resolve job asserts the existence of exactly two files so a rename cannot silently degrade it, and scripts/rewrite-manifest-urls.mjs is not among them. Nothing is lost today — the only https:// strings in the new helper are localhost inside comments, which that job filters out anyway, and urlDev/urlProd stay in webpack.config.js. Flagging it only because a host hardcoded in a scripts/ helper later would escape the check, and the bot cannot edit .github/workflows/ to widen it.

The ESM-vs-CJS question the PR body raises for a maintainer is well reasoned and the alternative is named honestly; it does not block. Flipping this out of draft.

[dobby-coder]

Non-blocking, and explicitly out of scope for #257 ("do not restructure the transform") — noting it for whoever reads this next.

urlDev is still interpolated into the pattern unescaped. Harmless for the current value, which has no regex metacharacter, but it means the two guards below cover only the under-matching direction: a urlDev whose metacharacters made the regex match more than the literal string would still corrupt the manifest with replacements > 0 and no leftover literal dev origin to find. new RegExp(urlDev.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"), "g") would close it, and now that this is an importable unit it is testable — but it is a separate change, not a fix this PR owes.