docs(deploy): by_ref ref pinning, credential=true, and CI behavior - #617
docs(deploy): by_ref ref pinning, credential=true, and CI behavior#617dawsontoth wants to merge 8 commits into
Conversation
…ee one by_ref now warns in both directions (harper#1850): uncommitted changes won't ship, and a commit that's on no remote branch can't be cloned by the cluster. Mentions that the second check reads local remote-tracking refs, so a stale view can warn about a commit that was in fact pushed.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the documentation for by_ref deployments to explain that it now warns when the commit being deployed is not on any remote branch, in addition to warning about a dirty working tree. The reviewer suggested adding a <VersionBadge> to denote this behavior change, as prescribed in the repository's guidelines.
| **A reference is pinned to a SHA, not to the name you typed.** Tags and branches are resolved locally and the full commit SHA is what ships. This matters on a cluster: peers resolve the package independently, so a tag that moves mid-deploy — or a branch that advances — could otherwise leave nodes running different code. | ||
|
|
||
| **Commit and push first.** The cluster clones from the remote, so it only sees commits that have been pushed. `by_ref` warns when the working tree is dirty, since uncommitted changes won't be part of the deploy. | ||
| **Commit and push first.** The cluster clones from the remote, so it only sees commits that have been pushed. `by_ref` warns in both directions: when the working tree is dirty (those changes won't be part of the deploy) and when the commit being deployed isn't on any remote branch (the cluster won't be able to clone it). The second check reads your local remote-tracking refs, so run `git fetch` if you get it for a commit you know you pushed. |
There was a problem hiding this comment.
When documenting behavior changes to an existing surface (such as the new unpushed-commit warning for by_ref), please include a to denote the change, as prescribed in the repository's guidelines.
Example:
Commit and push first. The cluster clones from the remote, so it only sees commits that have been pushed. by_ref warns in both directions: when the working tree is dirty (those changes won't be part of the deploy) and when the commit being deployed isn't on any remote branch (the cluster won't be able to clone it). The second check reads your local remote-tracking refs, so run git fetch if you get it for a commit you know you pushed.
References
- Use the
<VersionBadge type="changed" version="vX.Y.0" />format when documenting behavior changes to existing surface, as prescribed in the repository's guidelines.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-617 This preview will update automatically when you push new commits. |
| **A reference is pinned to a SHA, not to the name you typed.** Tags and branches are resolved locally and the full commit SHA is what ships. This matters on a cluster: peers resolve the package independently, so a tag that moves mid-deploy — or a branch that advances — could otherwise leave nodes running different code. | ||
|
|
||
| **Commit and push first.** The cluster clones from the remote, so it only sees commits that have been pushed. `by_ref` warns when the working tree is dirty, since uncommitted changes won't be part of the deploy. | ||
| **Commit and push first.** The cluster clones from the remote, so it only sees commits that have been pushed. `by_ref` warns in both directions: when the working tree is dirty (those changes won't be part of the deploy) and when the commit being deployed isn't on any remote branch (the cluster won't be able to clone it). The second check reads your local remote-tracking refs, so run `git fetch` if you get it for a commit you know you pushed. |
There was a problem hiding this comment.
Medium: the adjacent SHA-pinning guarantee (line 163) is not absolute, and this PR doesn't flag the gap
The unpushed-commit warning added here is accurate to warnIfCommitNotPushed in harper#1850 (bin/cliOperations.ts) — it checks git branch -r --contains <committish>, i.e. local remote-tracking refs, and the git fetch advice matches the code's own comment.
But the sentence right above this one (line 163) states unconditionally: "A reference is pinned to a SHA, not to the name you typed. Tags and branches are resolved locally and the full commit SHA is what ships." That's only true when resolveGitCommittish() can resolve the ref locally. Its catch branch does the opposite:
try {
return runGit(['rev-parse', `${refStr}^{commit}`]);
} catch {
// Not resolvable locally (e.g. a ref that only exists on the remote). Pass it through
// and let the cluster resolve it — losing the pin, ...
return refStr;
}So a ref=<branch-or-tag-name> that isn't fetched locally (e.g. a teammate's remote-only branch) is deployed unpinned — the raw mutable name, not a SHA — which is exactly the cluster-divergence hazard line 163 says by_ref prevents. This PR is a good place to add that caveat, since it's already revisiting this paragraph for the sibling unpushed-commit warning: e.g. "...full commit SHA is what ships when the ref resolves locally; an unresolvable ref (one that exists only on the remote) is passed through unpinned by name instead."
—
Generated by Barber AI
Ethan-Arrowood
left a comment
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
Holding rather than approving or blocking. This targets claude/two-phase-deploy-docs (#599), which is currently CONFLICTING with main and carries an unresolved CHANGES_REQUESTED, and it documents behavior from harper#1850, still open. Merging the stack as-is publishes docs for unshipped CLI behavior into the live /reference/v5 tree. The content here is fine — the hold is purely sequencing.
The line itself is accurate. I checked the new wording against warnIfCommitNotPushed in harper#1850 (bin/cliOperations.ts): the gate is git branch -r --contains <committish>, so "reads your local remote-tracking refs" is exact, and the git fetch advice mirrors the CLI's own warning text. Nothing to change.
One net-new gap. The unpushed check is skipped under CI:
if (!process.env.GITHUB_SHA) warnIfCommitNotPushed(committish);The dirty-tree warning is not skipped. "Warns in both directions" reads as unconditional, so a GitHub Actions user expecting the second warning silently won't get it. One clause covers it, or call it an implementation detail and leave it — your call.
On @gemini-code-assist's <VersionBadge type="changed" /> suggestion, I'd skip it, on two grounds. Wrong type: by_ref is itself documented as v5.2.0 surface that hasn't shipped (harper's package.json on the 1850 branch is 5.2.0-beta.3), so this warning lands in the same unreleased version as the feature — it isn't a behavior change to existing surface. Wrong placement: CONTRIBUTING.md:232 reserves <VersionBadge> for standalone placement after headings precisely because mid-sentence use is awkward, and the bot's example puts it mid-paragraph.
Agreeing with @cb1kenobi on line 163 — I reached the same read of resolveGitCommittish() independently: the catch returns the raw refStr, so a remote-only ref deploys unpinned by name. Worth folding in while the paragraph is already open, even though the sentence itself belongs to #599.
The plain-text Available since: v5.2.0 line on this section is also #599's, not this diff — I've raised it there rather than here.
sent with Claude Opus 5
… head Follows HarperFast/harper#1850 review changes: - `credential=true` is the documented form. The host is derived from the package, so naming it explicitly is compatibility rather than a choice, and a host that doesn't match the package's is rejected. - Ref resolution now covers the remote (a shallow CI clone rarely has the ref locally) and annotated tags, and an unresolvable ref stops the deploy instead of shipping a name peers would resolve independently. - Document what `by_ref` deploys under GitHub Actions: on a `pull_request` run it's the PR head, not the un-cloneable refs/pull/<n>/merge commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dawsontoth
changed the title
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-617 This preview will update automatically when you push new commits. |
…lone fetches Two gaps, both re-verified against harper#1850 as merged rather than as reviewed. **The unpushed-commit warning is skipped under GitHub Actions** (`bin/cliOperations.ts:538` — `if (!process.env.GITHUB_SHA)`), while the dirty-tree warning is not. @Ethan-Arrowood flagged this on the review: "warns in both directions" reads as unconditional, so a Actions user expecting the second warning silently doesn't get it. Now stated, with the reason — the runner's checkout isn't a branch `git branch -r --contains` can see — and a pointer to the `pull_request` resolution described just below. **A ref outside `refs/heads/` and `refs/tags/` is rejected up front.** This is newer than the review: `assertCloneableRefNamespace` refuses e.g. `ref=refs/pull/123/head` even when the local checkout resolves it happily, because a clone fetches only those two namespaces — the cluster could resolve the commit and still never check it out. Worth documenting because the failure is otherwise puzzling: the ref works locally, so the error looks wrong. Also confirmed against merged `main`, unchanged: `credential=true` derives the host from the package and rejects a mismatched host with a "use credential=true" hint; explicit refs resolve locally, then on the remote, then fail closed; annotated tags resolve to their peeled commit.
@kriszyp's eight findings, re-checked against the current state of harper#1849 and harper-pro#594 rather than the state they were filed against. Four had been fixed at the source in the meantime; four needed the docs changed. **"All-or-nothing at go-live" was wrong, and self-contradictory.** The page claimed it in the opening paragraph and then correctly described a version split twenty lines later. Reframed around what the two phases actually buy: an all-nodes staging barrier, which eliminates the common failure (a node that can't fetch or install) but not a per-node activation failure. The activation-failure paragraph is now its own `#### Activation failures` section instead of sitting mid-parameter-list, and the opening paragraph links to it. **The deploy modes are now documented as a matrix**, which core made possible by adding the validation @kriszyp asked for. `activate: false` and `deployment_id` are rejected outright when combined with `two_phase: false`, `replicated: false`, or a cluster whose `system` database isn't replicated, so a request that says stage-only can no longer activate. `two_phase: true` is likewise rejected without the replication to support it. `revert_on_failure` is refused rather than accepted and ignored. **The `system`-replication precondition is stated** — @Ethan-Arrowood's second question. A plain deploy on a cluster with `system` excluded from replication still silently takes the one-shot path, and that is the case worth warning about, because it also means no rollback target. **`revert_component`'s guarantees are scoped to the two-phase path.** The retained `.deploy-previous` copy is created by activation, so a component deployed one-shot has nothing to revert to however many times it has been deployed — the docs implied repeated deploys build up a rollback target. Also documents that `restart` is optional (a reverted component keeps serving the rolled-away-from code until something restarts it) and adds the `force` parameter the validator accepts. **The SSH-key encryption guarantee is qualified.** `sealSSHKey` in harper-pro still falls back to storing and replicating the private key in plaintext with only a WARN when no secret custody is registered, and that fallback is deliberate — SSH keys predate custody. So encryption at rest is a property of the deployment's configuration, not of the operation, and `generate: true` in particular reads as though the key could never be exposed. Custody is present by default, so the warning says that too rather than implying the risk is likely. Fixed at the source, no doc change needed: - **Explicit refs now resolve to a SHA** (harper 5c85714b8, 26cce986d, 21043f994) — the guarantee the page claimed is now real. The fuller treatment, including remote resolution and failing closed, is in #617. - **`auth_*` vs. ordinary `username=` precedence** — `main` grew a canonical numbered precedence section covering exactly this, and #630 folds the token rules into it. - **Version badges** — previous commit.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-617 This preview will update automatically when you push new commits. |
… claims Cross-model review (codex leg) against 4d1084d raised 13 findings. Three were false positives from reading the wrong tree — it checked `revert_component` against harper `main`, where harper#1849 is unmerged, so the operation looks deleted; it read `add_ssh_key generate` before ab11290 moved keygen in-process; and its claim that activation rebuilds a missing staged tree is the opposite of `activateStagedApplication`, which throws. The other ten were real. The largest is that harper#2173 was rebased and gained eleven commits adding an operation-scoping feature this branch never documented. **`operations` on a trust policy.** 1-100 unique names, validated at write time against the registry `add_role` uses, carried into the minted token as a narrowing-only intersection with the user's role. Documented with the two things that decide whether it is the control an operator thinks it is: - It covers the Operations API and SQL (`verifyPerms`/`verifyPermsAST`) and **not** an application's REST/GraphQL resource path, which authorizes through `checkPermission` — so a scoped token still carries the role's full table CRUD there. It bounds what CI can administer, not what data it can reach, and the policy's user still has to be a least-privilege role. - A runtime-registered operation (`server.registerOperation`) is not recognized, so a policy naming one is rejected. Fails closed, and shared with add_role. Also that a scoped token cannot trade itself for a login session. **`job_workflow_ref` no longer gates the ref**, and the removal is deliberate: for a reusable workflow it names the workflow that ran, not the caller, so its `@ref` is constant however it is invoked and would admit any caller branch. It still pins the workflow. The table said otherwise, which would have produced policies an admin believed were ref-gated. Six claims narrowed to what the code actually guarantees — the same guidance-versus-enforcement distinction applied earlier to the SSH-key text: - Audience: Harper rejects GitHub's shared default shape, but does not compare the value against its own identity, so a typo is accepted at write time and fails at exchange time instead. Not checked at all for a generic issuer. - The one-hour token is a reduced exposure window, not "worthless by the time it reaches a log" — inside the hour it is a live credential. - `drop_oidc_trust` stops future exchanges; it cannot revoke an already-issued stateless JWT. Names the user/role action that actually contains a leak. - A rejected OIDC exchange does not reliably 401: the CLI continues down precedence and a payload `username=`/`password=` pair will authenticate, so a policy mismatch can look like a working deploy under the wrong identity. - A pinned SHA fixes the source revision, not the built artifact — the cluster builds from source per node. - `credential=true` is the git-by-reference path; an npm-registry credential is attached with the printed entry naming `registry`. Release notes gain the replay race (records replicate asynchronously, so simultaneous replays on different nodes can both land) and that auth-event auditing is off by default. Ref resolution in `applications.md` also corrected here rather than only in #617, since #599 would otherwise publish the local-only description on its own.
One conflict, in the `by_ref` SHA-pinning paragraph both branches now touch. Kept this branch's version: it is the superset (it names annotated-tag peeling) and it already carries the fail-closed behavior as its own following paragraph, which is what #599's sentence was adding inline. `git checkout --ours` on that file took the whole file rather than the hunk, so it silently dropped #599's two other edits to the same section — the corrected "same source revision" wording and the new source-versus-artifact paragraph. Re-applied both, then diffed against #599 to confirm nothing else was lost. The remaining differences are only this branch's intended replacements.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-617 This preview will update automatically when you push new commits. |
…aims Round 3 of the cross-model review (codex at 5d4ce0c) re-raised as major something I had rejected as a false positive twice. It was right and I was wrong. **A `deployment_id` activate can re-fetch and re-install.** I checked `activateStagedApplication`, saw it throw on a missing staged tree, and concluded no rebuild path existed. The recovery is at the call site, not in the callee: `deployComponentActivateExisting` tests `hasCompleteStagedApplication` and, when it fails, calls `sourceStagedPayload` and `stageApplication` before activating. So "no new payload is fetched or installed" is only true while every staged tree is intact — a restart or disk repair between staging and activation makes that node rebuild, which is slower and can fail outright if the source or credential is no longer reachable. Documented in both operations.md and the release note. The lesson is narrow and worth stating: verifying a negative claim by reading the function that would perform the action is not enough when a caller can do it first. **Rollback retention is conditional.** `activateStagedApplication` only records a revertable copy when the caller passes `activationSpec` — its own docblock says omitting it leaves the component "not revertable afterwards". The release note claimed every two-phase activation retains its predecessor. Three narrower corrections: - `deploy setup=true` supports exactly two providers, `github` and `npm`, and rejects anything else. The release note implied any git host. - That flow calls `get_secrets_public_key` and `set_secret`, both super_user, so it needs an administrative credential rather than the CI identity it is provisioning. - The SHA-pins-source-not-artifact correction reached applications.md last round but not the release note, which still said "an exact redeploy". Also drops the inline `<VersionBadge>` from the precedence list item added last round. CONTRIBUTING.md reserves the component for standalone placement after a heading, and I had cited that same rule while declining a bot's inline-badge suggestion on #617 — then did it myself. Fourteen further findings from this round are unaddressed and unverified; they are in the PR description rather than silently dropped.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-617 This preview will update automatically when you push new commits. |
4 participants
Its own dependency has landed. harper#1850 merged 2026-08-14 — the previous description here said it was open and approved. Everything this PR documents is now on harper
main, verified against the merged code rather than against the PR. It carries no companion marker for that reason; sequencing is handled by its base branch.It ships in v5.3.0, not v5.2.0:
mainis at 5.2.2 (tagged 2026-08-13) and the merge landed after that tag, soby_refis in no release yet. Badges are corrected in #599.What changed
Refs are pinned or the deploy stops. Resolution covers the remote as well as the local checkout, since a shallow CI clone usually doesn't have the ref, and annotated tags resolve to the commit they point at. If a ref can't be resolved either way the deploy stops rather than sending the name — peers resolve the package independently, so an unresolvable name is exactly the divergence the SHA pin exists to prevent. A full commit SHA is always accepted, since it can't move.
A ref must name something a clone can fetch. New in this pass, and newer than the review:
refs/heads/*,refs/tags/*, or a bare branch or tag name. Anything else —refs/pull/123/headbeing the realistic case — is rejected up front even when the local checkout resolves it happily, because the cluster could resolve that commit and still never check it out. Worth documenting precisely because the failure looks wrong: it works locally, so the error reads as a bug.credential=true. The credential host is derived from the package being deployed, so it always matches the clone it authenticates.credential=github.comstill works but is compatibility rather than a choice, and a host that doesn't match the package's is rejected — the clone would never ask for it, so the deploy would otherwise fail as though no credential were configured.The unpushed-commit warning is skipped under GitHub Actions. @Ethan-Arrowood's finding on the last pass, still valid in merged
main(bin/cliOperations.ts:538): the dirty-tree warning is unconditional, the unpushed-commit one is not. "Warns in both directions" read as unconditional, so an Actions user expecting the second warning silently wouldn't get it. Now stated, with the reason — the runner's checkout isn't a branchgit branch -r --containscan see.What
by_refdeploys under GitHub Actions. On apull_requestrun it's the pull request's head commit, not the merge commit the runner checks out — that one lives underrefs/pull/<n>/merge, which a plain clone can't fetch. For a fork PR the head repository is the fork. Includes theref=${{ github.event.pull_request.head.sha }}escape hatch the CLI points to when the event payload isn't readable.Review notes
mainwould re-show all of docs(deploy): two-phase deploy, revert, by-reference deploys, and OIDC trusted publishing #599's still-unmerged content. GitHub retargets automatically as the stack lands.revert_componentnote was stale and would have reverted docs(deploy): two-phase deploy, revert, by-reference deploys, and OIDC trusted publishing #599's improvement.<VersionBadge type="changed" />suggestion, @Ethan-Arrowood's reasoning to skip it held, but the conclusion has changed with the version:by_refis unreleased surface, so the whole section now carries an addedv5.3.0badge in docs(deploy): two-phase deploy, revert, by-reference deploys, and OIDC trusted publishing #599 rather than a changed one here. Nothing to add mid-sentence.npm run buildandnpm run format:checkclean.Description drafted by Claude Code (Opus 5).