Repository navigation
Enable PR previews from the cluster: enable_previews and disable_previews, per-app preview limits, and harper preview setup - #3617
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements cluster-enabled PR previews, allowing users to set up, deploy, list, drop, and teardown isolated, branched preview environments for their applications. It introduces new operations (enable_previews, disable_previews, get_preview_policy), CLI commands (preview setup and preview teardown), a preview reachability probe, and updates component deployment and grant validation to enforce constraints such as maximum previews and maximum time-to-live (TTL). Comprehensive integration and unit tests are also added to verify these behaviors. All of the reviewer's comments were filtered out as they suggested defensive try-catch blocks for standard error message extraction, which is discouraged by the repository's style guide; therefore, there is no additional feedback to provide.
280302c to
bd4e860
Compare
bd4e860 to
fb066d8
Compare
…, per-app bounds, harper preview setup Adds three super-user operations that create and remove one application's PR-preview identity on the cluster, so the CLI and Studio's preview panel share one implementation: - enable_previews writes, create-only, the role and user <app>-ci-preview (a component_deploy grant for <app>-pr-*: isolated, hostSuffix, branchedDatabases required by default, preview required, no table permissions), the trust policy github-actions-<app>-preview pinning repository_id, the default branch's workflow_ref, event_name pull_request_target and the preview environment, and a cleanup policy scoped to drop_component in the preview-cleanup environment. - disable_previews revokes (policies, then user) before it drops each of the application's own previews with the replicated drop_component, then removes the user and role; a failed drop stops it with the identity revoked, and a rerun resumes. - get_preview_policy reads the identity back. The per-app policy is the grant's constraints: maxPreviews replaces deployment.previews.maxPerApplication for deploys under the grant, maxTtlDays clamps preview.expiresAt, and preview: required binds every deploy to preview metadata of an app the grant previews. The CLI gains harper preview setup (repository lookup, workflow-file safety checks, the HARPER_PREVIEW_TARGET variable, an optional starter workflow and private-repository credential) and harper preview teardown, and harper preview deploy now warns when the preview's URL does not answer from the preview. Part of #3042 Part of #3055 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… the review's gaps - disable_previews asks every node for its live releases and drops each preview release of the app on the node that holds it, only while it is that node's live release; a node that cannot answer stops it with the identity revoked. A non-preview redeploy under the name is never dropped, and expiry no longer hides a release a peer still holds. - A deploy admitted before the revocation is refused at its activation once its deployer's records changed; an activation by deployment id is checked against the build's recorded preview first, the already-live path included. - Under preview: required the grant covers only <app>-pr-<n>, for drops and reads too; enable caps maxTtlDays at 7 unless told otherwise. - One enable or disable at a time per node, and each policy's id and overlap are rechecked just before it is written. - A user named like the identity in another role stops disable; get_preview_policy reports a role that is not enable's; a branch given as refs/heads/ is refused. - The fork guard counts only a whole && term that admits same-repository pull requests, and checks every job holding the identity token. - The starter workflow drops what it deployed if the PR closed meanwhile, and its cleanup retries while a deploy holds the preview. - The reachability check counts the preview's own 5xx as an answer and names the grant when its maxPreviews refuses a deploy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… second review's minors - A peer's live <app>-pr-<n> release this node has no deployment row for yet stops disable_previews with the identity revoked, instead of being taken for a non-preview; the release census has a timeout, and a peer's drop answer is unwrapped as replicated answers are. - get_preview_policy reports a role whose grant is invalid or whose operations no longer allow the preview commands; a preview grant's names must each be <app>-pr-*. - The starter workflow drops after a deploy only when the PR reads as closed, and still checks when the deploy itself failed. - The reachability check no longer takes a proxy-labelled 404 or 500 as a routing failure: behind nginx the application's own answer carries nginx's Server header. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ose the closing review's minors - harper preview setup follows a job's needs into the job it waits on, and refuses, unless allow_unverified=true, a fork guard or PR-head checkout it cannot rule out; branch-pattern notes stay notes. - The deployer is checked again inside the preview admission lock, just before the swap. - A peer that does not answer the release census, or answers without a release list, counts as unreachable; list_releases reports the latter as an error rather than an empty node. - get_preview_policy reports extra operations; a preview grant's names need a non-empty app; a failed drop's error names the manual drop for branched storage it left. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…stripped preview grant - A job whose condition calls success() runs past a skipped needed job, so it no longer inherits that job's fork guard. - get_preview_policy reports a preview grant that no longer requires isolated previews under a host suffix. - A failed drop's error also names a worker it left running; the design note states that a swap already under way, or a peer's replicated deploy, can still go live after a revocation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nd read status functions in any case - harper preview setup refuses, unless allow_unverified=true, when there is no workflow file to check and none is written: the workflow is what keeps forks and PR code away from the preview identity. - The starter workflow's cleanup checks that the PR is still closed before each retry, so a PR reopened meanwhile keeps its new preview. - The fork guard reads always(), success() and the rest in any case, as GitHub does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A failed read of the pull request used to read as "not closed", so cleanup exited green having dropped nothing. The deploy job now fails on a failed read, and cleanup retries the read with the drop. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…epository head.repo.fork describes the head repository alone: it is false for a PR from the root of the base's own fork network, and a deleted fork's null head passes !fork. The fork guard now accepts only a full_name comparison with the base, and reports the fork spellings as unverified. get_preview_policy also reports a trust policy that no longer pins event_name pull_request_target and an environment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…hat drop_component records it #3600 records a branched database removal a drop could not finish and retries it on main, so disable_previews no longer tells the operator to drop that storage by hand; a manual drop_component is for whatever else the drop's error names. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fb066d8 to
3063184
Compare
|
|
||
| const ROLE_FLAGS = new Set(['super_user', 'structure_user', 'cluster_user', 'operations']); | ||
| const PULL_REQUEST_HEAD = /head\.sha|head\.ref|head\.repo|head_ref|merge_commit_sha|refs\/pull\//; | ||
| const PR_HEAD_SHELL_CHECKOUT = |
There was a problem hiding this comment.
[major] A token-bearing job with env.SHA set to the PR head and git checkout "$SHA" passes without problems or an unsafe warning, allowing PR code to use the preview identity; classify unresolved checkout variables as unverified.
| const failed = [...unreachable]; | ||
| for (const release of releases) { | ||
| try { | ||
| const answer: any = await dropPreviewRelease(release, req.hdb_user); |
There was a problem hiding this comment.
[major] If release Y replaces censused release X, the conditional drop returns componentRemoved:false, yet teardown removes the identity and reports completion while Y remains live; re-census or report incomplete cleanup.
| for await (const row of deployments.search([ | ||
| { attribute: 'project', comparator: 'starts_with', value: `${app}-pr-` }, | ||
| ])) { | ||
| if (row?.preview?.app === app && !ENDED_DEPLOYMENT_STATUSES.has(row.status)) projects.add(row.project); |
There was a problem hiding this comment.
[minor] An activation past final admission can miss both the census and the in-progress report: its row lacks preview metadata and its source is terminal; follow activated_from when identifying pending previews.
| if (!user) problems.push(`user "${names.user}" is missing`); | ||
| else if (user.active === false) problems.push(`user "${names.user}" is inactive`); | ||
| else if (user.role?.role !== names.role) problems.push(`user "${names.user}" is in role "${user.role?.role}"`); | ||
| for (const id of [names.policy, names.cleanupPolicy]) { |
There was a problem hiding this comment.
[minor] Replacing the deploy trust policy’s scope with only drop_component still yields enabled:true, although its tokens cannot deploy; validate policy capabilities alongside role capabilities.
| differences.push( | ||
| `operations are ${JSON.stringify(existing?.operations ?? null)}, not ${JSON.stringify(desired.operations)}` | ||
| ); | ||
| checkEnvironmentJobs(workflow, environment, problems, unverified); |
There was a problem hiding this comment.
[minor] A valid deploy workflow with no cleanup job passes without warning, so closed previews remain until expiry; report the absent cleanup path.
| // error, or a 503 a proxy names itself as sending, says the request never reached the preview. | ||
| const server = response.headers.get('server') ?? ''; | ||
| const fromProxy = PROXY_SERVERS.test(server); | ||
| if (response.status === 502 || response.status === 504 || (response.status === 503 && fromProxy)) { |
There was a problem hiding this comment.
[minor] An application’s own 502/504, or proxy-forwarded 503, is reported as a routing failure and retried; report an ambiguous gateway response unless its origin is established.
| // error, or a 503 a proxy names itself as sending, says the request never reached the preview. | ||
| const server = response.headers.get('server') ?? ''; | ||
| const fromProxy = PROXY_SERVERS.test(server); | ||
| if (response.status === 502 || response.status === 504 || (response.status === 503 && fromProxy)) { |
There was a problem hiding this comment.
[minor] An application's own 502 or 504 is classified as a routing failure, causing retries and a misleading TLS-routing warning. Report an ambiguous gateway response unless its origin is established.
⊙ Problem
Turning on previews takes a hand-written role, user, policy and workflow, which Studio would repeat. Nothing bounds how many previews CI keeps or how long one lives.
Part of #3042. Part of #3055.
🤔 Should the cluster own preview setup?
Yes: Studio and the CLI then share one implementation, and Central Manager needs no per-app setup (HarperFast/central-manager#904). Production deploy setup (#3127) stays client-side, as three existing operations.
💡 Solution
enable_previews,disable_previewsandget_preview_policy(super user only) manage an app's preview identity on the cluster. Disable revokes it, then drops the app's previews.maxPreviews,maxTtlDays(7 by default), and branching by default.harper preview setupchecks the workflow, calls enable once, then sets up the GitHub side.harper preview teardowndisables.harper preview deploywarns when the preview URL does not answer.⚖️ Alternatives
hdb_preview_policytable — a second record to keep in step with the grant, and a new system table on every node.🤔 Disable drops previews, not only the identity
The planning review preferred letting previews expire; the brief asks disable to drop them. It drops only what each node reports as the app's preview release, on that node, at that release.
🤔 Cleanup gets its own policy
Required reviewers on
previewalso gate the close-event job. Cleanup runs inpreview-cleanupwith a drop-only token, so it need not wait forpreview's approval.🤔 Limits are per node, and expiry is capped
maxPreviewscounts previews on the node taking the deploy, likemaxPerApplication.maxTtlDayslowers a laterexpiresAt; a staged build's activation is refused instead.🔧 Changes
components/previewOperations.ts: enable_previews builds the records from one validated request and creates only what is missing, under a node-wide lock, rechecking each policy just before it is written. get_preview_policy reports drift. disable_previews refuses records not shaped as enable writes them, revokes, takes a census of live preview releases and drops each on its node. Registered inutility/hdbTerms.ts(names),server/serverHelpers/serverUtilities.ts(handlers) andutility/operation_authorization.ts(super user only); kept off the default MCP surface incomponents/mcp/tools/operations.ts(excluded, destructive).components/releaseOperations.tsgiveslist_releasesa timeout and reports a peer that sent no list;components/deploymentOperations.tsadds deploymentRecord.utility/previewIdentity.ts: derivePreviewNames, CI_PREVIEW_OPERATIONS, the 7-day default and isPreviewProjectOf.security/ciIdentity.tsholds the create-only comparison moved from Set up GitHub Actions deploys withharper deploy setup=true provider=github-actions#3127's setup: planIdentityReconcile, grant comparison and policiesOverlap.security/authn/oidc/trustPolicyOperations.tsexposes add_oidc_trust's checks so enable runs them before writing.security/componentGrant.tsvalidates the new constraints, binds preview metadata to the grant, narrows coverage to -pr- and bounds expiry.components/operations.tsclamps expiresAt, checks an activation against its build's record, refuses a revoked deployer in admitGrantPreview, and passes the grant's cap to admission.components/previews.tsrechecks inside the admission lock and names the limit that refused.bin/previewSetup.ts: harper preview setup refuses a workflow it cannot check or cannot verify as fork-safe, then makes one enable call; teardown; the starter workflow drops what it deployed if the PR closed meanwhile.bin/deploySetupGithubActions.ts: checkPreviewWorkflowFile, the fork guard with needs chains, PR-head checkouts, and the GitHub helpers (target, variable, production setup adapter).bin/deploySetup.tsexposes sealDeployCredential.bin/previewReachability.tsprobes the preview URL, called frombin/preview.tsafter a deploy, which also routes setup and teardown;bin/help.tslists them.security/DESIGN.md: PR previews are enabled by the cluster, and the grant note; indexed inDESIGN.md.✅ Verification
shop-pr-12withttl=30capped at 2 days; the grant refuses other apps, a foreign host and a second preview. Teardown removes it all.🤔 Review stopped after four full passes
Majors fell from three to none. The last pass's one new minor (
head.repo.fork) was fixed afterward, 24 lines, not re-reviewed. Planning deferred #3590.Docs this needs (no docs PR)
enable_previews,disable_previews,get_preview_policy; thepreview,maxPreviews,maxTtlDaysgrant constraints; that the audience must be the exact target URL with port and trailing slash.harper preview setup(repo=,repository_id=,branch=,domain=,dataMode=,maxPreviews=,maxTtlDays=,credential=true,write_workflow=true,allow_unverified=true),harper preview teardown, andcheck=falseonharper preview deploy.harper preview setup; the starter workflow;HARPER_PREVIEW_TARGET; thepreview-cleanupenvironment needs no reviewers.🤖 Generated with Claude Code
Release target: v5.4 — completes the unreleased 5.4 preview work from #3505; new API surface
Related PRs: #3360 overlaps (registers operations in the same files), #3600 overlaps (merged into main; it records and retries the branch removal a failed preview drop leaves behind), 3 others independent
Complexity: complicated
Deferred findings
48127c9b16f6ffe5minor concurrency —components/previewOperations.ts— trust-policy admission not atomic against other nodes or ordinary add_oidc_trust writers — filed Trust-policy writes from enable_previews are not exclusive across nodes or against add_oidc_trust #36089c769dbc24955da7minor concurrency —components/previews.ts— revocation not fenced through the activation swap or a peer's replicated apply — filed A preview deploy already swapping, or applying on a peer, can go live after disable_previews #36137a224c6b7df89085nit reliability —bin/previewSetup.ts— enable_previews runs before credential, workflow and variable writes — filed harper preview setup enables the cluster side before the GitHub-side writes that can still fail #3609Review-Coverage: authored=claude; ran=gemini,codex,cursor-composer; adjudicated=domain; blocked=cursor-grok(no-receipt),cursor-muse(no-receipt); declined=cursor-kimi,muse; rounds=7; full=4 @ 3063184
Review-Attention: deep ~40m (sensitive: trustPolicyOperations.ts, ciIdentity.ts +1; decisions: cluster-owned-preview-operations, bounds-live-in-component-grant, clamp-deploy-refuse-activation, revocation-fence-any-record-change, unreviewed-cleanup-environment, census-driven-teardown, client-side-workflow-gate, reachability-check-default-on, do-less-alternative) @ 3063184