Repository navigation
Conversation
2d7ebc2 to
4fe5ca5
Compare
belfordzjeeves
left a comment
There was a problem hiding this comment.
Review
I verified this hands-on: checked out the branch, ran the full suite locally (37/37 pass, schema-contract passes), confirmed all 8 pinned action SHAs in release.yml/deploy.yml resolve to exactly the tags claimed, confirmed no deployment identifiers (ARNs, bucket names, account IDs) appear anywhere in the diff, and confirmed the remaining meta.open-rpc.org strings are intentional (legacy-endpoint docs plus one negative test fixture). queue: max is valid documented GitHub syntax, correctly paired with cancel-in-progress: false. The deploy.yml permission split is sound: upload-pages-artifact uploads through the runner's artifact token rather than GITHUB_TOKEN, so the build job genuinely needs only contents: read, and every called job's permissions are a subset of the caller's grant.
The security posture is genuinely good: AWS credentials exist only in the deploy-schema job, no dependency installation happens after auth, persist-credentials: false everywhere, the fork-PR guard on the release job, mask-aws-account-id, a 20-minute session against a 15-minute job timeout, no caller-identity logging, and the freshness machinery (mayPublish rejecting stale/conflicting metadata, version keys before latest, invalidating every path on retry, the double Pages freshness check) all fails closed. Test coverage of those guards is thorough, including the interrupted-newer-run case.
Findings, in order of importance:
1. ci.yml pinning (suggestion attached, agreed). The new schema-contract job was the one place still on floating tags while the PR pins everything else as a stated supply-chain improvement. Accept the suggestion to bring it in line.
2. The CloudFormation template should be in the repo, in parameterized form. I weighed your reason for removing it (avoid exposing info not needed for automation), and I think the exposure concern is smaller than it looks: the trust subject (repo:open-rpc/spec:environment:schema-production), the role's permissions, and the redirect behavior are already stated verbatim in #426, and ARNs/bucket names/account IDs are all parameterizable. Against that near-zero marginal exposure, a committed template buys real things: reviewers can verify the cache policy forwards x-amz-meta-* response headers (the deploy script's verification depends on this; if it doesn't, the first release fails closed at verification and stalls), and the stack becomes rebuildable from the repo instead of living on one machine. If you'd rather keep it out, the two mitigations are: treat header forwarding as the load-bearing check on the first release, and keep the template plus stack outputs somewhere durable and referenced.
3. merge_commit_sha reliance. It's present in webhook payloads (including squash merges, where it's the squash commit SHA, and despite its removal from the REST API schema), but transient nulls at close time are a known phenomenon. If null, checkout silently falls back to the PR merge ref while RELEASE_COMMIT is empty, and the script fails closed with a confusing error; a re-run won't heal it since the event payload is frozen. Worth documenting "merge the Knope release PR with a merge commit" and possibly adding a fallback.
4. Outstanding manual step: the master ruleset requiring schema-contract still needs configuring after merge, per #426.
5. Behavior change worth stating in the PR description: deploy.yml no longer fires on master pushes at all; docs now publish only through successful releases. That's the intent from #426, but it changes what "merge to master" means for the site and should say so outright.
No blockers from me. Approving.
4fe5ca5 to
778f596
Compare
The bundled 1.3 and 1.4 schemas currently share a legacy identity, and Pages publishes unreleased master changes. Give each schema its canonical
https://schema.open-rpc.org/{major.minor}identity and publish schemas and docs from the same successful release commit.spec/1.3/spec.mdunchanged.schema-contractcheck. Preserve validation constraints and the existing dialect; document the identity migration for consumers. The required master ruleset is already active.Validation: AWS
validate-templateaccepts the restored template; README Markdown, local release-guard cases, diff checks, and a scan of every new reachable Git object for known deployment identifiers pass. Workflow validation passes with actionlint 1.7.12 using only its unsupported documentedqueue: maxdiagnostic suppression and shellcheck disabled. GitHub CI is running on the updated single signed commit.The implementation previously passed all 37 tests, TypeScript, bundle/docs builds, a future-version docs fixture, and live temporary hosting checks. No AWS resources changed in this review follow-up. Actual GitHub OIDC assumption and complete release/Pages execution remain to be verified on the first eligible release; no npm package or production schema has been published by this work. This PR remains a draft for review.
fixes #426