Bridge product facts v1 to typed v2 - #67
Merged
Merged
Conversation
📝 WalkthroughWalkthroughVersion 3.2.1 updates product facts to schema v2, adds reviewed v1 normalization, removes fixed entitlement claims, introduces reproducible Node 22 containers, and separates verified npm and MCP registry publication with provenance and readback checks. ChangesProduct facts and entitlement surface
Container and release automation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant VerifyJob
participant NpmRegistry
participant NpmPublish
participant MCPRegistry
VerifyJob->>NpmRegistry: verify release metadata and provenance
VerifyJob->>MCPRegistry: validate expected server metadata
VerifyJob->>NpmPublish: provide verified tarball
NpmPublish->>NpmRegistry: publish tarball with provenance
NpmPublish->>NpmRegistry: verify version, integrity, and latest tag
NpmPublish->>MCPRegistry: publish when registry state is stale
MCPRegistry-->>NpmPublish: return public release metadata
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
karlwaldman
marked this pull request as ready for review
August 11, 2026 13:34
karlwaldman
force-pushed
the
codex/product-facts-v2-compat
branch
from
August 11, 2026 14:47
c2453ae to
6144dc5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/__tests__/productFacts.test.ts (1)
216-241: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe injected clock does not control date validation.
The test injects
now: () => Date.parse("2026-08-11T20:00:00Z"), butrequiredIsoDateinsrc/productFacts.tscallsnew Date()directly for the non-future check. The fixture dates are validated against the real wall clock. This test and the fixtures at line 24, line 25, line 38, and line 39 depend on the machine date. See the related comment onsrc/productFacts.tslines 176-192 for the root cause.🤖 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/__tests__/productFacts.test.ts` around lines 216 - 241, Update ProductFactsProvider date validation, specifically requiredIsoDate in productFacts.ts, to use the injected now clock instead of constructing a real-time Date directly. Ensure the test’s now value controls validation of the fixture dates and remove any remaining dependence on the machine’s wall-clock date in the affected fixtures and test.
🧹 Nitpick comments (12)
src/__tests__/productFacts.test.ts (1)
31-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
nativeV2Factsfrom the pinned artifact, not from the v1 downgrade.
nativeV2FactscallslegacyV1Facts, which deletes the typed allowance fields, and then restoresfreeRequestLimit: 50andfreeRequestWindow: "day". The fixture therefore hardcodes the allowance instead of reading it fromPINNED_PRODUCT_FACTS. If the pinned artifact later changes the limit or the window, this fixture keeps the old values and the provider tests stop testing the shipped contract.♻️ Proposed refactor
function nativeV2Facts(): Record<string, unknown> { - const facts = legacyV1Facts(); - const offer = facts.offer as Record<string, unknown>; - delete offer.freeRequestsPerMonth; - offer.freeRequestLimit = 50; - offer.freeRequestWindow = "day"; - facts.schemaVersion = "2.0.0"; - facts.contractVersion = "2026-08-11"; - facts.reviewedAt = "2026-08-11"; - facts.schemaUrl = - "https://api.oilpriceapi.com/schemas/product-facts-v2.schema.json"; - return facts; + return cloneFacts(); }Then build
legacyV1FactsfromnativeV2Factsinstead of the reverse.🤖 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/__tests__/productFacts.test.ts` around lines 31 - 43, Refactor the test fixtures so nativeV2Facts() is derived directly from PINNED_PRODUCT_FACTS rather than legacyV1Facts(), preserving the pinned allowance values without hardcoding them. Then update legacyV1Facts() to derive from nativeV2Facts() and remove the typed allowance fields for the v1 shape.Dockerfile (1)
14-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe length check fails without a message.
Line 15 prints a reason when
SOURCE_COMMITis empty or contains a non-hex character. Line 17 fails silently when the value is hex but not 40 characters long. Fold the length into the pattern so every rejection prints the reason.♻️ Proposed refactor
RUN case "$SOURCE_COMMIT" in \ - (*[!0-9a-f]*|'') echo "SOURCE_COMMIT must be a lowercase 40-character Git SHA" >&2; exit 1;; \ + ([0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]) ;; \ + (*) echo "SOURCE_COMMIT must be a lowercase 40-character Git SHA" >&2; exit 1;; \ esac && \ - test "${`#SOURCE_COMMIT`}" -eq 40 && \ case "$SOURCE_DATE_EPOCH" in \ (*[!0-9]*|'') echo "SOURCE_DATE_EPOCH must be an integer" >&2; exit 1;; \ esac && \A shorter alternative keeps the current structure and adds an explicit message:
- test "${`#SOURCE_COMMIT`}" -eq 40 && \ + { test "${`#SOURCE_COMMIT`}" -eq 40 || \ + { echo "SOURCE_COMMIT must be 40 characters" >&2; exit 1; }; } && \🤖 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 `@Dockerfile` around lines 14 - 20, Update the SOURCE_COMMIT validation in the Dockerfile RUN command so the 40-character length requirement is enforced within the same rejection path as the lowercase hexadecimal check, ensuring invalid lengths print the existing descriptive error before exiting. Remove the silent standalone test while preserving validation of SOURCE_DATE_EPOCH.scripts/verify-mcp-registry-release.mjs (2)
17-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the unexpected field names in the error message.
The current message reports only a count. This runs after publication in the release chain, so the operator needs the field names to diagnose registry drift quickly. The keys derive from a public server projection, so no secret is exposed.
♻️ Proposed improvement
function assertOnlyKeys(record, allowedKeys, name) { const allowed = new Set(allowedKeys); const unexpected = Object.keys(record).filter((key) => !allowed.has(key)); if (unexpected.length > 0) { - throw new Error(`${name} returned unsupported fields (${unexpected.length})`); + throw new Error( + `${name} returned unsupported fields: ${unexpected.sort().join(", ")}`, + ); } }🤖 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 `@scripts/verify-mcp-registry-release.mjs` around lines 17 - 23, Update assertOnlyKeys to include the unexpected field names in the thrown Error message, while preserving the existing count and validation behavior. Format the names from the unexpected array so release operators can identify the unsupported registry fields directly.
146-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the promise resolver to avoid shadowing the
resolveimport.Line 149 names the executor parameter
resolve, which shadows theresolveimport fromnode:pathat line 5. The behavior is correct today because the outerresolveis used only at line 159. The sibling scriptscripts/verify-npm-release.mjsalready usesresolveDelayat line 98. Align the two files.♻️ Proposed change
- if (attempt < attempts) { - await new Promise((resolve) => setTimeout(resolve, delayMs)); - } + if (attempt < attempts) { + await new Promise((resolveDelay) => setTimeout(resolveDelay, delayMs)); + }🤖 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 `@scripts/verify-mcp-registry-release.mjs` around lines 146 - 152, Rename the Promise executor parameter in the retry delay within the catch block of the release verification flow from resolve to resolveDelay, matching verify-npm-release.mjs and avoiding shadowing the node:path resolve import; preserve the existing timeout behavior.src/__tests__/directorySource.test.ts (1)
47-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReject every audit level weaker than
low, not onlymoderate.Line 57 rejects
--audit-level=moderateonly. A change to--audit-level=highor--audit-level=criticalpasses this test while weakening the gate. Assert that no audit level other thanlowappears.♻️ Proposed change
for (const source of [ packageJson.scripts.prepublishOnly, liveWorkflow, publishWorkflow, ]) { - expect(source).not.toContain("--audit-level=moderate"); + for (const match of source.matchAll(/--audit-level=(\w+)/g)) { + expect(match[1]).toBe("low"); + } }🤖 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/__tests__/directorySource.test.ts` around lines 47 - 59, Update the “requires a literal zero-vulnerability audit gate” test to reject every explicit audit level other than low across prepublishOnly, liveWorkflow, and publishWorkflow, while preserving the required low-level assertions. Validate each source so moderate, high, critical, or any other weaker level cannot pass.scripts/verify-npm-release.mjs (1)
110-117: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCross-check
NPM_RELEASE_EXPECTED_VERSIONagainstpackage.json.
scripts/verify-mcp-registry-release.mjsrejects a configured version that disagrees withserver.jsonat lines 165-169. This script trusts the environment value. If the workflow exports a stale version, the verifier confirms the wrong release and reports success. Mirror the sibling check.♻️ Proposed change
+import { readFile } from "node:fs/promises";const expectedName = process.env.NPM_RELEASE_EXPECTED_NAME; const expectedVersion = process.env.NPM_RELEASE_EXPECTED_VERSION; const expectedIntegrity = process.env.NPM_RELEASE_EXPECTED_INTEGRITY; if (!expectedName || !expectedVersion || !expectedIntegrity) { throw new Error( "NPM_RELEASE_EXPECTED_NAME, NPM_RELEASE_EXPECTED_VERSION, and NPM_RELEASE_EXPECTED_INTEGRITY are required", ); } + const packageJson = JSON.parse( + await readFile(new URL("../package.json", import.meta.url), "utf8"), + ); + if ( + expectedName !== packageJson.name || + expectedVersion !== packageJson.version + ) { + throw new Error( + `NPM_RELEASE_EXPECTED_NAME/VERSION=${expectedName}@${expectedVersion} did not match package.json=${packageJson.name}@${packageJson.version}`, + ); + }🤖 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 `@scripts/verify-npm-release.mjs` around lines 110 - 117, Update the validation in verify-npm-release around expectedVersion to load the package version from package.json and reject any configured NPM_RELEASE_EXPECTED_VERSION that differs from it, mirroring the sibling verifier’s cross-check while preserving the existing required-environment validation.scripts/verify-npm-release-smoke.mjs (2)
8-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the package name and version from
package.json.Lines 9-10 hard-code
oilpriceapi-mcpand3.2.1, and line 35 hard-codes the drift value3.2.0. After the next version bump these literals go stale, and the smoke test still passes, so the drift stays invisible. The sibling testscripts/verify-mcp-registry-release-smoke.mjsalready reads../server.jsonat line 10. Use the same approach here.♻️ Proposed change
+import { readFile } from "node:fs/promises"; import { validateNpmVersionDocument, verifyNpmRelease, } from "./verify-npm-release.mjs"; +const packageJson = JSON.parse( + await readFile(new URL("../package.json", import.meta.url), "utf8"), +); +const staleVersion = `${packageJson.version}-stale`; const expected = { - expectedName: "oilpriceapi-mcp", - expectedVersion: "3.2.1", + expectedName: packageJson.name, + expectedVersion: packageJson.version, expectedIntegrity: "sha512-reviewed", };Then replace the two
"3.2.0"literals at lines 35 and 71 withstaleVersion.🤖 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 `@scripts/verify-npm-release-smoke.mjs` around lines 8 - 12, Update the smoke test’s expected package metadata to derive the package name and version from package.json, following the existing sibling test’s file-reading approach. Store the derived version as staleVersion and replace both hard-coded "3.2.0" values in the verification flow, while preserving the expectedIntegrity check.
29-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a mutation for a non-npm
attestations.url.
validateNpmVersionDocumentrejects anattestations.urlthat is not a string or does not start withhttps://registry.npmjs.org/(scripts/verify-npm-release.mjslines 39-40). No mutation here exercises that branch. This is the branch that binds the provenance record to the npm registry origin, so it should be covered.♻️ Proposed change
(document) => { document.dist.attestations.provenance.predicateType = "https://example.com/not-slsa"; }, + (document) => { + document.dist.attestations.url = + "https://registry.npmjs.org.example.com/attestations"; + }, + (document) => { + delete document.dist.attestations.url; + }, ]) {🤖 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 `@scripts/verify-npm-release-smoke.mjs` around lines 29 - 57, Add a mutation case to the array in the npm metadata verification test that changes document.dist.attestations.url to a non-npm URL, then rely on the existing rejection assertion to verify validateNpmVersionDocument rejects it. Keep the mutation isolated from the existing provenance, integrity, and package metadata cases.scripts/verify-mcp-registry-release-smoke.mjs (1)
75-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd mutations for a non-active
statusand for a nested unexpected field.
validateRegistryPayloadrejects onstatus !== "active"as well asisLatest !== true, but only theisLatestbranch is covered here.assertOnlyKeysalso runs onserver.repository,packages[].transport, and each environment variable, and none of those nested branches is exercised. Cover both so a weakened projection fails this smoke test.♻️ Proposed change
+ withServerMutation((server) => { + server.packages[0].transport.unreviewedField = true; + }), + withServerMutation((server) => { + server.repository.unreviewedField = true; + }), { ...valid, _meta: { "io.modelcontextprotocol.registry/official": { status: "active", isLatest: false, }, }, }, + { + ...valid, + _meta: { + "io.modelcontextprotocol.registry/official": { + status: "deleted", + isLatest: true, + }, + }, + }, + { ...valid, _meta: {} }, ]) {🤖 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 `@scripts/verify-mcp-registry-release-smoke.mjs` around lines 75 - 83, Extend the mutations in the smoke-test cases around validateRegistryPayload to include a non-"active" official status and an unexpected nested field in a structure checked by assertOnlyKeys, such as server.repository, packages[].transport, or an environment variable. Assert both mutations are rejected so the status validation and nested-key projection checks are covered..github/workflows/registry-backfill.yml (1)
46-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the package name from
package.jsoninstead of hard-coding it.Line 49 hard-codes
oilpriceapi-mcp. Line 48 already reads the version frompackage.json. If the package is ever renamed or scoped, this step checks a different package and reports a false pass..github/workflows/publish.ymlLine 140 derives the name from the tarball, so the two workflows use different sources of truth.♻️ Proposed refactor
- name: Verify version exists on npm (registry must never lead npm) run: | + NAME=$(jq -r .name package.json) PKG=$(jq -r .version package.json) - PUBLISHED=$(timeout 30s npm view "oilpriceapi-mcp@$PKG" version) + PUBLISHED=$(timeout 30s npm view "$NAME@$PKG" version) if [ "$PUBLISHED" != "$PKG" ]; then - echo "::error::npm returned $PUBLISHED instead of $PKG" + echo "::error::npm returned '$PUBLISHED' for $NAME instead of $PKG" exit 1 fi🤖 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 @.github/workflows/registry-backfill.yml around lines 46 - 53, Update the “Verify version exists on npm” step to read the package name from package.json and use that value in the npm view command, alongside the existing version lookup. Remove the hard-coded oilpriceapi-mcp identifier so registry-backfill tracks the package metadata as its source of truth.scripts/public-claims-smoke.mjs (1)
177-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the scan so importing the module does not run it.
Line 69 exports
findMutableClaims, which signals that unit tests import this file. Any import executesassertDetectorContract(), runsnpm pack, and can throw at module scope. Run the scan only when the file is the entry point.♻️ Proposed refactor
+import { argv } from "node:process"; ... -assertDetectorContract(); - -const surfaces = [ - ... -process.stdout.write( - `Public-claims smoke passed across ${surfaces.length} source and packed surfaces.\n`, -); +export function runPublicClaimsSmoke() { + assertDetectorContract(); + const surfaces = [/* ...unchanged... */]; + // ...unchanged failure collection... + process.stdout.write( + `Public-claims smoke passed across ${surfaces.length} source and packed surfaces.\n`, + ); +} + +if (resolve(argv[1] ?? "") === fileURLToPath(import.meta.url)) { + runPublicClaimsSmoke(); +}🤖 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 `@scripts/public-claims-smoke.mjs` around lines 177 - 205, Wrap the top-level smoke-scan execution, including assertDetectorContract(), surface collection, failure checks, and success output, in an entry-point guard so it runs only when scripts/public-claims-smoke.mjs is invoked directly. Keep findMutableClaims and other exports import-safe for unit tests..github/workflows/publish.yml (1)
73-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe pinned MCP publisher install is copied into three steps. The download, checksum verification, extraction, and
PATHupdate are identical in all three locations. A publisher version or checksum change must be applied to all three, and a missed copy produces a mismatched toolchain between the verify, registry, and backfill jobs. Extract the block into a local composite action, for example.github/actions/install-mcp-publisher/action.yml, withversion,archive, andsha256inputs.
.github/workflows/publish.yml#L73-L81: replace the inline install script withuses: ./.github/actions/install-mcp-publisher..github/workflows/publish.yml#L200-L208: replace the duplicated install script with the same composite action reference..github/workflows/registry-backfill.yml#L55-L63: replace the duplicated install script with the same composite action reference, and move the pinnedMCP_PUBLISHER_*values into the action defaults or keep them as workflow-level inputs.🤖 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 @.github/workflows/publish.yml around lines 73 - 81, Extract the duplicated MCP publisher installation into a local composite action, such as .github/actions/install-mcp-publisher/action.yml, accepting version, archive, and sha256 inputs and performing download, checksum verification, extraction, and PATH setup. Replace the inline blocks at .github/workflows/publish.yml#L73-L81 and .github/workflows/publish.yml#L200-L208 with the action reference, and replace the block at .github/workflows/registry-backfill.yml#L55-L63 likewise; centralize the pinned MCP_PUBLISHER_* values in the action defaults or pass them consistently as workflow-level inputs.
🤖 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.
Inline comments:
In @.github/workflows/live-tests.yml:
- Around line 65-70: The live-test job condition currently allows secret-bearing
execution for same-repository pull requests; restrict the condition around the
smoke-test job so it runs only for post-merge events, not any pull request.
Update the adjacent comment to accurately state that behavior, while preserving
the existing checkout and secret-absence handling.
In @.github/workflows/publish.yml:
- Around line 213-223: Add an npm ci --ignore-scripts step after Node setup and
before the verification command in the registry job at
.github/workflows/publish.yml lines 213-223 and
.github/workflows/registry-backfill.yml lines 39-44; the publish workflow should
install dependencies before Check whether the public registry already matches,
and the backfill workflow before npm run verify:release-provenance.
In `@scripts/live-product-facts-smoke.mjs`:
- Around line 35-59: Require the exposed facts schema version in both canonical
smoke checks: in scripts/live-product-facts-smoke.mjs lines 35-59, update
assertReviewedContract to reject payloads when facts?.schemaVersion is not
"2.0.0"; in scripts/docker-smoke.mjs lines 181-193, add the equivalent
livePayload?.facts?.schemaVersion validation to the existing release-check
condition. Preserve the current allowance and delivery metadata checks.
In `@scripts/public-claims-smoke.mjs`:
- Around line 102-110: Update the source filter callback around `walkFiles` so
the test-directory and test/spec-file exclusions apply to the entire
allowed-extension condition by grouping the extension checks together. Normalize
`relative(repositoryRoot, path)` using the `sep` symbol from `node:path` before
checking for `__tests__`, ensuring the exclusion works on Windows as well as
POSIX systems.
In `@scripts/verify-npm-release.mjs`:
- Around line 108-109: Update the entrypoint guards in
scripts/verify-npm-release.mjs#L108-L109 and
scripts/verify-mcp-registry-release.mjs#L158-L159 to compare
realpathSync(process.argv[1]) with realpathSync(fileURLToPath(import.meta.url)),
preserving the existing release-check execution. Also make each script exit
non-zero when it is neither imported nor recognized as the entrypoint,
preventing silent successful no-ops.
In `@scripts/verify-release-provenance-smoke.mjs`:
- Around line 24-30: Update runVerifier to remove or explicitly neutralize the
inherited MCP_PROVENANCE_MODE before spawning the verifier, while preserving
environment overrides supplied through env. Ensure release assertions execute in
the default release mode regardless of the caller’s shell environment.
In `@src/__tests__/directorySource.test.ts`:
- Around line 89-91: Update the ordering assertion for mcp-publisher publish to
first assert its index is greater than -1, then verify it appears before
verify:mcp-registry-release, matching the existing explicit presence check for
mcp-publisher validate.
- Around line 123-130: Broaden the action-reference regexes in the “pins every
workflow action that participates in release proof” test to inspect every
non-local uses: reference, not only actions/ entries. Continue excluding local
workflow references, require each matched external reference to use a
40-character lowercase hexadecimal SHA, and preserve the existing checks across
publishWorkflow, backfillWorkflow, and liveWorkflow.
- Around line 26-34: Update the README text used by the “does not hard-code a
numeric post-trial Free allowance” test so whitespace is normalized before
applying the allowance regex, allowing matches across Markdown line breaks while
preserving the existing assertion and required URL/text checks.
In `@src/__tests__/index.test.ts`:
- Around line 1499-1536: Ensure the fetch stub created in the pricing test is
always restored, including when opa_get_plans.handler or an assertion fails.
Move vi.unstubAllGlobals into test lifecycle cleanup such as afterEach, or wrap
the stubbed execution and assertions in try/finally so cleanup runs
unconditionally.
In `@src/productFacts.ts`:
- Around line 176-192: Update requiredIsoDate to make the future-date validation
opt-in, and have loadPinnedProductFacts skip that gate when validating the
packaged artifact. In src/productFacts.ts lines 176-192, preserve format and
calendar validity checks while allowing callers to request the non-future check.
In src/__tests__/productFacts.test.ts lines 216-241, make fixture dates
independent of the machine clock and add coverage proving a future reviewedAt
from a remote response still throws.
- Around line 631-657: Update the pinned checksum guards in the validation flow
around stableFactsDigest to check options.pinnedChecksum !== undefined instead
of truthiness. Apply this consistently to the format validation,
projected-checksum comparison, default-checksum override check, and assignment
so an empty string is rejected rather than emitted as the provider checksum.
- Around line 636-659: Update the default branch in the pinned checksum
initialization to derive this contract checksum with
stableFactsDigest(pinnedValidation.facts), matching the algorithm used for
supplied pinned facts and other delivery sources; do not use
PINNED_PRODUCT_FACTS_CHECKSUM for this field, while preserving validation of any
explicitly supplied checksum against the same computed value.
---
Outside diff comments:
In `@src/__tests__/productFacts.test.ts`:
- Around line 216-241: Update ProductFactsProvider date validation, specifically
requiredIsoDate in productFacts.ts, to use the injected now clock instead of
constructing a real-time Date directly. Ensure the test’s now value controls
validation of the fixture dates and remove any remaining dependence on the
machine’s wall-clock date in the affected fixtures and test.
---
Nitpick comments:
In @.github/workflows/publish.yml:
- Around line 73-81: Extract the duplicated MCP publisher installation into a
local composite action, such as
.github/actions/install-mcp-publisher/action.yml, accepting version, archive,
and sha256 inputs and performing download, checksum verification, extraction,
and PATH setup. Replace the inline blocks at
.github/workflows/publish.yml#L73-L81 and
.github/workflows/publish.yml#L200-L208 with the action reference, and replace
the block at .github/workflows/registry-backfill.yml#L55-L63 likewise;
centralize the pinned MCP_PUBLISHER_* values in the action defaults or pass them
consistently as workflow-level inputs.
In @.github/workflows/registry-backfill.yml:
- Around line 46-53: Update the “Verify version exists on npm” step to read the
package name from package.json and use that value in the npm view command,
alongside the existing version lookup. Remove the hard-coded oilpriceapi-mcp
identifier so registry-backfill tracks the package metadata as its source of
truth.
In `@Dockerfile`:
- Around line 14-20: Update the SOURCE_COMMIT validation in the Dockerfile RUN
command so the 40-character length requirement is enforced within the same
rejection path as the lowercase hexadecimal check, ensuring invalid lengths
print the existing descriptive error before exiting. Remove the silent
standalone test while preserving validation of SOURCE_DATE_EPOCH.
In `@scripts/public-claims-smoke.mjs`:
- Around line 177-205: Wrap the top-level smoke-scan execution, including
assertDetectorContract(), surface collection, failure checks, and success
output, in an entry-point guard so it runs only when
scripts/public-claims-smoke.mjs is invoked directly. Keep findMutableClaims and
other exports import-safe for unit tests.
In `@scripts/verify-mcp-registry-release-smoke.mjs`:
- Around line 75-83: Extend the mutations in the smoke-test cases around
validateRegistryPayload to include a non-"active" official status and an
unexpected nested field in a structure checked by assertOnlyKeys, such as
server.repository, packages[].transport, or an environment variable. Assert both
mutations are rejected so the status validation and nested-key projection checks
are covered.
In `@scripts/verify-mcp-registry-release.mjs`:
- Around line 17-23: Update assertOnlyKeys to include the unexpected field names
in the thrown Error message, while preserving the existing count and validation
behavior. Format the names from the unexpected array so release operators can
identify the unsupported registry fields directly.
- Around line 146-152: Rename the Promise executor parameter in the retry delay
within the catch block of the release verification flow from resolve to
resolveDelay, matching verify-npm-release.mjs and avoiding shadowing the
node:path resolve import; preserve the existing timeout behavior.
In `@scripts/verify-npm-release-smoke.mjs`:
- Around line 8-12: Update the smoke test’s expected package metadata to derive
the package name and version from package.json, following the existing sibling
test’s file-reading approach. Store the derived version as staleVersion and
replace both hard-coded "3.2.0" values in the verification flow, while
preserving the expectedIntegrity check.
- Around line 29-57: Add a mutation case to the array in the npm metadata
verification test that changes document.dist.attestations.url to a non-npm URL,
then rely on the existing rejection assertion to verify
validateNpmVersionDocument rejects it. Keep the mutation isolated from the
existing provenance, integrity, and package metadata cases.
In `@scripts/verify-npm-release.mjs`:
- Around line 110-117: Update the validation in verify-npm-release around
expectedVersion to load the package version from package.json and reject any
configured NPM_RELEASE_EXPECTED_VERSION that differs from it, mirroring the
sibling verifier’s cross-check while preserving the existing
required-environment validation.
In `@src/__tests__/directorySource.test.ts`:
- Around line 47-59: Update the “requires a literal zero-vulnerability audit
gate” test to reject every explicit audit level other than low across
prepublishOnly, liveWorkflow, and publishWorkflow, while preserving the required
low-level assertions. Validate each source so moderate, high, critical, or any
other weaker level cannot pass.
In `@src/__tests__/productFacts.test.ts`:
- Around line 31-43: Refactor the test fixtures so nativeV2Facts() is derived
directly from PINNED_PRODUCT_FACTS rather than legacyV1Facts(), preserving the
pinned allowance values without hardcoding them. Then update legacyV1Facts() to
derive from nativeV2Facts() and remove the typed allowance fields for the v1
shape.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ce191838-a6c9-45ae-87cd-78b7dce12152
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
.dockerignore.github/dependabot.yml.github/workflows/live-tests.yml.github/workflows/publish.yml.github/workflows/registry-backfill.yml.mcp.jsonDockerfileREADME.mddocs/DISTRIBUTION_CONVERSION_PRD.mdmanifest.jsonpackage.jsonscripts/clean-build.mjsscripts/copy-product-facts.mjsscripts/docker-smoke.mjsscripts/live-product-facts-smoke.mjsscripts/package-smoke.mjsscripts/product-facts-smoke.mjsscripts/public-claims-smoke.mjsscripts/verify-mcp-registry-release-smoke.mjsscripts/verify-mcp-registry-release.mjsscripts/verify-npm-release-smoke.mjsscripts/verify-npm-release.mjsscripts/verify-release-metadata.mjsscripts/verify-release-provenance-smoke.mjsscripts/verify-release-provenance.mjsserver.jsonsrc/__tests__/directorySource.test.tssrc/__tests__/index.test.tssrc/__tests__/productFacts.test.tssrc/index.tssrc/product-facts.v1.sha256src/product-facts.v2.jsonsrc/product-facts.v2.sha256src/productFacts.tstsconfig.json
💤 Files with no reviewable changes (1)
- src/product-facts.v1.sha256
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
50/daypayload, while accepting the canonical native-v2 contract after the atomic API rolloutas_of, version, customer-cohort, and fixed tool-count claims from authored and packed public surfacesmanifest.jsonremains compatibility metadata only, not an MCPB artifact or MCPB releaseTDD / red-green proof
.mcp.json, README, source inventory, and public distribution-doc claims; recursive authored plus exact-packed scan is greennpm install -g; the replacement receives a SHA-256/SRI-pinned npm CLI tarball from the unprivileged verify job and rejects all install/ci/exec commands/32denominator was wrong for write scope; unit and packed stdio smokes now derive the registered count and verify read32/36plus write36/36tsc@2.0.4; the replacement image passes non-root/provenance/facts/capability/CLI/live-stdio checksRelease safety
origin/mainancestry must agreea06c9096dcb9727c13555b6be26c7effa707b01f06a4c561ba7a3635443cf2cce679850e663b16f5f146ee425d0eb0e3442c1d2bda3d513bbfd7c81f5ee5db38latest, signed Sigstore/SLSA material, package subject, source repository, workflow, release tag, and source commitserver.jsoncontract plus active/latest status; npm and Registry remain honestly sequential and recoverable, not atomicExact verification
2b8f6979c592a2782fe54e518c75329d0a5fb8a1npm ci: clean installnpm test: 7 files / 193 tests passednpm audit --audit-level=low: 0 vulnerabilitiesreviewed-v1-daily-bridge-> 50/daygit diff --check, and clean worktree: passedOILPRICEAPI_TEST_KEY; the secret-bearing live smoke runs only from the repository default branchRollout boundary
Merge and release/cold-smoke MCP 3.2.1 against the current v1 API first. Only then may the atomic API product-facts/quota/auth deployment proceed, followed by a live MCP v2 smoke. No API deployment, customer send, account mutation, or MCPB publication is included here.