test(inventory): pin new-value routing for composite identifyWith entries#163
Conversation
…ries The 2026-05-19 script-inventory migration (mr-yum/script-inventory#92) replaced every name-only CSP entry with N per-host entries shaped as: identifyWith: andMatcher: [{ headerNameMatcher }, { hostMatcher }] authoriseWith: [ContentMatcher, ContentMatcher, …] The inventory-diff gate looks at `authoriseWith.matcher` (an OrMatcher from array syntax), not `identifyWith`, so a new unauthorised value for one of these entries should land in that specific entry's authoriseWith without touching siblings or rewriting the composite identifyWith. But no existing test pinned this case down — every header-update test in the suite used a simple headerNameMatcher in identifyWith. This adds a regression test with two per-host entries (meandu, Stripe) that mirror the production shape, sends a KnownHeaderWithUnauthorisedContentFound for the Stripe entry, and asserts: - the new ContentMatcher landed in the Stripe entry's authoriseWith - the meandu entry is untouched - the Stripe entry's composite identifyWith is preserved verbatim through the raw round-trip - diff.appliedResults reports the result as applied Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughA new regression test is added to validate that ChangesHeader Matcher Composite Update Regression Test
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/services/inventory.test.ts (1)
1052-1052: 💤 Low valueClarify whether the
urlfield is necessary.The header object includes a
urlfield that doesn't appear in similar test cases (e.g., lines 594, 648). If this field is intentionally documenting the production data shape where headers carry source URL context, consider adding a brief comment. If it's unused by the comparison result, it could be removed for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/inventory.test.ts` at line 1052, The test includes an extra url property on the header object ({ name: 'content-security-policy', value: newValue, target, workflow: target.workflow, url: 'https://m.stripe.network/something.js' }) which is inconsistent with other header fixtures and may be unused by the comparison; either remove the url field to match the other cases, or keep it but add a brief inline comment beside the header explaining that url is intentionally present to mirror production header source context and ensure any assertion/comparison logic (the test's header equality checks) accounts for it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/services/inventory.test.ts`:
- Line 1052: The test includes an extra url property on the header object ({
name: 'content-security-policy', value: newValue, target, workflow:
target.workflow, url: 'https://m.stripe.network/something.js' }) which is
inconsistent with other header fixtures and may be unused by the comparison;
either remove the url field to match the other cases, or keep it but add a brief
inline comment beside the header explaining that url is intentionally present to
mirror production header source context and ensure any assertion/comparison
logic (the test's header equality checks) accounts for it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d7e83ef-5f9b-4cdd-a0cd-8a113628fa71
📒 Files selected for processing (1)
src/services/inventory.test.ts
Why
The script-inventory migration replaced every name-only CSP entry with N per-host entries shaped:
```json
{
"identifyWith": {
"andMatcher": [
{ "headerNameMatcher": "^content-security-policy$" },
{ "hostMatcher": "^m\\.stripe\\.network$" }
]
},
"authoriseWith": [ ContentMatcher, ContentMatcher, … ]
}
```
When a new CSP value arrives from
m.stripe.networkthat doesn't match any existing content matcher, the inventory diff should append the new matcher to this entry'sauthoriseWithand leave siblings (meandu, hCaptcha, etc.) untouched.The diff gate checks
authoriseWith.matcher, notidentifyWith, so this should work — but no existing unit test exercised it. Every header-update test in the suite used a simpleheaderNameMatcherinidentifyWith. This adds the missing coverage.What the test asserts
ContentMatcherlanded in the Stripe entry'sauthoriseWith(now 2 entries, was 1).identifyWithis preserved verbatim through the raw round-trip:```json
{ "andMatcher": [{ "headerNameMatcher": "^content-security-policy$" }, { "hostMatcher": "^m\\.stripe\\.network$" }] }
```
diff.appliedResultsreports the result as applied (drives the truthful "Inventory updated" Slack message).Test plan
npm run check:formattingnpm run check:lintingnpm run check:typingnpm run test:unit(483 passing, +1 new)🤖 Generated with Claude Code