From e8d054d20b452e1b50ec20c7a829d95d58ab1606 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Tue, 28 Jul 2026 19:20:15 -0400 Subject: [PATCH 1/7] feat(cli): add `by_ref` opt-in to deploy by git reference (git+https + credential) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit harper deploy by_ref=true (or ref=) resolves the app's GitHub owner/repo + commit (HEAD or GITHUB_SHA) and deploys git+https://github.com//.git# instead of a payload blob — SHA-pinned so peers can't diverge on a moved tag. A numeric ref (buildRequest JSON-parses ref=1234567 to a number) is coerced to a string rather than ignored. credential=github.com (or credential=true) attaches a credentials reference [{host, secret: deploy..git.}] matching the server's deriveGitSecretName convention (the token sealed by harper deploy setup=true, #1778); the cluster resolves it in-memory to authenticate the HTTPS clone (#1799). Public repos need no credential. No-flag default stays payload. by_ref/ref/credential are transport-only; credentials is a real field. Adds unit tests (48 passing). Rebased onto main. Relates to #1777, #1799, #641. Co-Authored-By: Claude Opus 4.8 --- bin/cliOperations.ts | 124 +++++++++++++++++++++++++++- unitTests/bin/cliOperations.test.js | 58 +++++++++++++ 2 files changed, 181 insertions(+), 1 deletion(-) diff --git a/bin/cliOperations.ts b/bin/cliOperations.ts index d3ff0f0edd..2f66f67753 100644 --- a/bin/cliOperations.ts +++ b/bin/cliOperations.ts @@ -10,6 +10,7 @@ import * as path from 'path'; import * as fs from 'fs-extra'; import * as YAML from 'yaml'; import { Readable } from 'node:stream'; +import { execFileSync } from 'node:child_process'; import { streamPackagedDirectory, packageDirectory, scanPackageDirectory } from '../components/packageComponent.ts'; import { encode as encodeCbor } from 'cbor-x'; import { buildMultipartBody } from './multipartBuilder.ts'; @@ -46,6 +47,11 @@ const TRANSPORT_ONLY_FIELDS = new Set([ 'json', 'skip_node_modules', 'skip_symlinks', + // deploy-by-reference opt-in: consumed client-side to build `package` (and derive `credentials`), + // never sent to the server. (`credentials`, plural, IS a real operation field and is sent.) + 'by_ref', + 'ref', + 'credential', ]); // Streaming (multipart upload + SSE progress) deploy was introduced in 5.1.0. A CLI at >= @@ -249,13 +255,129 @@ function redactCredentials(req: any): any { return redacted; } -export { cliOperations, buildRequest, redactCredentials, refreshExpiredOperationToken }; +export { + cliOperations, + buildRequest, + redactCredentials, + refreshExpiredOperationToken, + resolveGitCommittish, + deriveGitSecretName, +}; + +// --- deploy-by-reference (opt-in via `by_ref=true` / `ref=`) ---------------------- +// Resolve the app's GitHub repo + commit from the local working copy (or GitHub Actions env) so +// `harper deploy by_ref=true` deploys a pinned commit by reference instead of uploading a payload +// blob. Client-side: only the runner has the git context. The no-flag default stays the payload deploy. + +function runGit(args: string[]): string { + return execFileSync('git', args, { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }).trim(); +} + +function resolveGitRepo(): string { + if (process.env.GITHUB_REPOSITORY) return process.env.GITHUB_REPOSITORY; + let url: string; + try { + url = runGit(['remote', 'get-url', 'origin']); + } catch { + throw new Error( + 'deploy by_ref: no git `origin` remote found — push this project to GitHub, or pass an explicit package=.' + ); + } + // git@github.com:owner/repo.git | https://github.com/owner/repo(.git) | ssh://… + const match = url.match(/github\.com[:/]+([^/]+\/[^/]+?)(?:\.git)?\/?$/i); + if (!match) throw new Error(`deploy by_ref: could not parse owner/repo from the origin remote: ${url}`); + return match[1]; +} + +function resolveGitCommittish(ref: unknown): string { + // buildRequest JSON-parses CLI args, so a numeric ref (e.g. `ref=1234567`) arrives as a number — + // coerce it back to a string rather than silently ignoring it and falling back to HEAD. + const refStr = typeof ref === 'string' || typeof ref === 'number' ? String(ref).trim() : ''; + if (refStr.length > 0) return refStr; // explicit ref= wins + if (process.env.GITHUB_SHA) return process.env.GITHUB_SHA; + try { + return runGit(['rev-parse', 'HEAD']); + } catch { + throw new Error('deploy by_ref: could not resolve HEAD — make at least one commit, or pass ref=.'); + } +} + +function warnIfWorkingTreeDirty(): void { + try { + if (runGit(['status', '--porcelain'])) { + process.stderr.write( + 'warning: working tree has uncommitted changes — the cluster deploys the committed (and pushed) commit, so those changes are NOT included.\n' + ); + } + } catch { + // Not a git repo; resolveGitRepo/resolveGitCommittish will surface a clearer error. + } +} + +function defaultProjectName(projectPath: string): string { + try { + const pkg = JSON.parse(fs.readFileSync(path.join(projectPath, 'package.json'), 'utf8')); + if (typeof pkg.name === 'string' && pkg.name.length > 0) return pkg.name.replace(/^@[^/]+\//, ''); + } catch { + // No/invalid package.json — fall back to the directory name. + } + return path.basename(projectPath); +} + +// Mirror the server's `deriveGitSecretName` (secretOperations.ts) EXACTLY so the reference this +// attaches matches the row `harper deploy setup=true` sealed and a literal-token deploy would use: +// `deploy..git.` (host via normalizeGitHost). +function deriveGitSecretName(component: string, host: string): string { + const hostPart = host + .trim() + .replace(/^[a-z0-9+.-]+:\/\//i, '') + .replace(/^\/\//, '') + .replace(/\/.*$/, '') + .replace(/^[^@]*@/, '') + .toLowerCase() + .replace(/[^\w.-]+/g, '_'); + const componentPart = String(component).replace(/[^\w.-]+/g, '_'); + return `deploy.${componentPart}.git.${hostPart}`; +} + +// Opt-in deploy-by-reference: resolve the pinned git ref (+ optional sealed credential) onto `req` — +// a `git+https` package pinned by SHA, plus a `credentials` reference when `credential=` is set. +// Exported for unit tests. +export function prepareDeployByRef(req: any): void { + const repo = resolveGitRepo(); + const committish = resolveGitCommittish(req.ref); + warnIfWorkingTreeDirty(); + if (!req.project) req.project = defaultProjectName(process.cwd()); + // git+https (not ssh): a private clone is authenticated by a git-host token credential (#1799), + // which rides over HTTPS. A public repo needs no credential at all. + req.package = `git+https://github.com/${repo}.git#${committish}`; + // `credential=github.com` (or `credential=true`) attaches the sealed-token reference the cluster + // resolves at fetch time — provision it once with `harper deploy setup=true`. + const credentialHost = + req.credential === true + ? 'github.com' + : typeof req.credential === 'string' && req.credential.length > 0 + ? req.credential + : undefined; + if (credentialHost && req.credentials === undefined) { + req.credentials = [{ host: credentialHost, secret: deriveGitSecretName(req.project, credentialHost) }]; + } + process.stderr.write(`Deploying "${req.project}" by reference: ${req.package}\n`); +} + const PREPARE_OPERATION: any = { deploy_component: async (req) => { if (req.package) { return; } + // Opt-in: deploy a pinned git commit by reference instead of packaging the working directory. + // Templates scaffold `by_ref=true`; without the flag the payload path below is unchanged. + if (req.by_ref || req.ref) { + prepareDeployByRef(req); + return; + } + const projectPath = process.cwd(); if (!req.project) req.project = path.basename(projectPath); const packageOptions = { diff --git a/unitTests/bin/cliOperations.test.js b/unitTests/bin/cliOperations.test.js index fc3f1f3018..1d637113fc 100644 --- a/unitTests/bin/cliOperations.test.js +++ b/unitTests/bin/cliOperations.test.js @@ -1159,3 +1159,61 @@ describe('cliOperations', () => { }); }); }); + +describe('deploy by reference (by_ref)', () => { + const { prepareDeployByRef, resolveGitCommittish, deriveGitSecretName } = cliOperationsModule; + let savedRepo, savedSha, savedStderrWrite; + + beforeEach(() => { + savedRepo = process.env.GITHUB_REPOSITORY; + savedSha = process.env.GITHUB_SHA; + // Resolve deterministically from env so the tests never shell out to git. + process.env.GITHUB_REPOSITORY = 'acme/demo'; + process.env.GITHUB_SHA = 'abc123def456'; + // Silence the "Deploying … by reference" line prepareDeployByRef writes to stderr. + savedStderrWrite = process.stderr.write; + process.stderr.write = () => true; + }); + + afterEach(() => { + if (savedRepo === undefined) delete process.env.GITHUB_REPOSITORY; + else process.env.GITHUB_REPOSITORY = savedRepo; + if (savedSha === undefined) delete process.env.GITHUB_SHA; + else process.env.GITHUB_SHA = savedSha; + process.stderr.write = savedStderrWrite; + }); + + it('builds a git+https package pinned to the resolved SHA', () => { + const req = { by_ref: true, project: 'demo' }; + prepareDeployByRef(req); + assert.strictEqual(req.package, 'git+https://github.com/acme/demo.git#abc123def456'); + assert.strictEqual(req.credentials, undefined); // no credential requested + }); + + it('an explicit ref= wins over GITHUB_SHA', () => { + const req = { by_ref: true, ref: 'v9.9.9', project: 'demo' }; + prepareDeployByRef(req); + assert.strictEqual(req.package, 'git+https://github.com/acme/demo.git#v9.9.9'); + }); + + it('resolveGitCommittish coerces a numeric ref to a string (buildRequest JSON-parses it)', () => { + assert.strictEqual(resolveGitCommittish(1234567), '1234567'); + assert.strictEqual(resolveGitCommittish('v1.0.0'), 'v1.0.0'); + }); + + it('credential=true attaches a github.com credential reference', () => { + const req = { by_ref: true, credential: true, project: 'demo' }; + prepareDeployByRef(req); + assert.deepStrictEqual(req.credentials, [{ host: 'github.com', secret: 'deploy.demo.git.github.com' }]); + }); + + it('credential= attaches a credential reference for that host', () => { + const req = { by_ref: true, credential: 'github.com', project: 'my-app' }; + prepareDeployByRef(req); + assert.deepStrictEqual(req.credentials, [{ host: 'github.com', secret: 'deploy.my-app.git.github.com' }]); + }); + + it('deriveGitSecretName matches the server convention (deploy..git.)', () => { + assert.strictEqual(deriveGitSecretName('my-app', 'github.com'), 'deploy.my-app.git.github.com'); + }); +}); From 4118b71d221504cef53ce0445317e49fe44b90b4 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 30 Jul 2026 09:29:24 -0400 Subject: [PATCH 2/7] fix(cli): pin an explicit ref= to a SHA; warn on an unpushed commit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review on #1850. An explicit `ref=` was passed into the package URL verbatim, so `ref=main` — or a tag repointed between one peer fetching and another re-fetching after a restart — could leave cluster peers running different commits. That is exactly the divergence the default HEAD path resolves a full SHA to prevent, so the same pinning now applies to explicit refs. `^{commit}` also dereferences annotated tags. A ref git can't resolve locally (one that exists only on the remote) still passes through for the cluster to resolve: losing the pin beats failing on a ref the user can plainly see. Also warns when the resolved commit is on no remote branch. The dirty-tree warning covers uncommitted work, but the likelier by_ref mistake is committing and forgetting to push — the cluster clones from the remote, so the SHA isn't there and the deploy fails server-side, far from the CLI and much harder to read. The check reads local remote-tracking refs, so it costs no network round-trip, at the price of a false warning on a stale local view — which the message tells the user how to clear. It's skipped under GITHUB_SHA, where CI is on a pushed commit by construction and a shallow checkout may have no remote-tracking branches. Ref-resolution tests now run against a purpose-built temp repo with a known tag and commit, so "resolves to a SHA" is asserted against a real git resolution rather than whichever tags happen to exist in the checkout running the tests. 53 passing. Co-Authored-By: Claude Opus 4.8 --- bin/cliOperations.ts | 37 +++++++++++++- unitTests/bin/cliOperations.test.js | 78 ++++++++++++++++++++++++++++- 2 files changed, 112 insertions(+), 3 deletions(-) diff --git a/bin/cliOperations.ts b/bin/cliOperations.ts index 2f66f67753..5d25c701f0 100644 --- a/bin/cliOperations.ts +++ b/bin/cliOperations.ts @@ -293,7 +293,20 @@ function resolveGitCommittish(ref: unknown): string { // buildRequest JSON-parses CLI args, so a numeric ref (e.g. `ref=1234567`) arrives as a number — // coerce it back to a string rather than silently ignoring it and falling back to HEAD. const refStr = typeof ref === 'string' || typeof ref === 'number' ? String(ref).trim() : ''; - if (refStr.length > 0) return refStr; // explicit ref= wins + if (refStr.length > 0) { + // Resolve an explicit ref= to an immutable SHA too, rather than passing the name through. + // Cluster peers resolve the package independently, so `ref=main` — or a tag that moves + // between one peer fetching and another restarting and re-fetching — would otherwise leave + // nodes running different commits. `^{commit}` also dereferences annotated tags. + 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, but a deploy that works beats a + // deploy that fails on a ref the user can see on the remote. + return refStr; + } + } if (process.env.GITHUB_SHA) return process.env.GITHUB_SHA; try { return runGit(['rev-parse', 'HEAD']); @@ -314,6 +327,25 @@ function warnIfWorkingTreeDirty(): void { } } +// The likelier by_ref mistake isn't a dirty tree, it's committing and forgetting to push: the +// cluster clones from the remote, so the SHA simply isn't there and the deploy fails server-side, +// far from the CLI and with a much less obvious error. Checked against local remote-tracking refs, +// so it costs no network round-trip — at the price of a false warning when the local view is stale, +// which the message accounts for. +function warnIfCommitNotPushed(committish: string): void { + try { + if (!runGit(['branch', '-r', '--contains', committish])) { + process.stderr.write( + `warning: commit ${committish.slice(0, 12)} isn't on any remote branch — push it, or the cluster ` + + "won't be able to clone it. (If you already pushed, run `git fetch` to refresh your remote refs.)\n" + ); + } + } catch { + // Not a git repo, or a committish git can't resolve locally (a remote-only ref is expected + // to be absent here) — nothing useful to say, and the deploy itself surfaces real errors. + } +} + function defaultProjectName(projectPath: string): string { try { const pkg = JSON.parse(fs.readFileSync(path.join(projectPath, 'package.json'), 'utf8')); @@ -347,6 +379,9 @@ export function prepareDeployByRef(req: any): void { const repo = resolveGitRepo(); const committish = resolveGitCommittish(req.ref); warnIfWorkingTreeDirty(); + // Skipped under GITHUB_SHA: CI runs on a commit that is on the remote by construction, and a + // shallow/detached runner checkout has no remote-tracking branches to check against anyway. + if (!process.env.GITHUB_SHA) warnIfCommitNotPushed(committish); if (!req.project) req.project = defaultProjectName(process.cwd()); // git+https (not ssh): a private clone is authenticated by a git-host token credential (#1799), // which rides over HTTPS. A public repo needs no credential at all. diff --git a/unitTests/bin/cliOperations.test.js b/unitTests/bin/cliOperations.test.js index 1d637113fc..bd65dd0eeb 100644 --- a/unitTests/bin/cliOperations.test.js +++ b/unitTests/bin/cliOperations.test.js @@ -1191,9 +1191,10 @@ describe('deploy by reference (by_ref)', () => { }); it('an explicit ref= wins over GITHUB_SHA', () => { - const req = { by_ref: true, ref: 'v9.9.9', project: 'demo' }; + // `no-such-ref-here` doesn't resolve locally, so this also covers the pass-through fallback. + const req = { by_ref: true, ref: 'no-such-ref-here', project: 'demo' }; prepareDeployByRef(req); - assert.strictEqual(req.package, 'git+https://github.com/acme/demo.git#v9.9.9'); + assert.strictEqual(req.package, 'git+https://github.com/acme/demo.git#no-such-ref-here'); }); it('resolveGitCommittish coerces a numeric ref to a string (buildRequest JSON-parses it)', () => { @@ -1201,6 +1202,79 @@ describe('deploy by reference (by_ref)', () => { assert.strictEqual(resolveGitCommittish('v1.0.0'), 'v1.0.0'); }); + // A real repo with known refs, so "resolves to a SHA" is asserted against an actual git + // resolution rather than whatever tags happen to exist in the checkout running the tests. + describe('explicit refs are pinned to an immutable SHA', () => { + const { execFileSync } = require('node:child_process'); + let repoDir, priorCwd, headSha; + + before(() => { + repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'harper-by-ref-')); + const git = (...args) => + execFileSync('git', args, { cwd: repoDir, encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }).trim(); + git('init', '-q'); + git('config', 'user.email', 'test@example.com'); + git('config', 'user.name', 'Test'); + git('commit', '-q', '--allow-empty', '-m', 'first'); + git('tag', 'v1.2.3'); + headSha = git('rev-parse', 'HEAD'); + // runGit shells out against the real process cwd, so mocking process.cwd isn't enough. + priorCwd = process.cwd(); + process.chdir(repoDir); + }); + + after(() => { + process.chdir(priorCwd); + fs.rmSync(repoDir, { recursive: true, force: true }); + }); + + // Peers resolve the package independently, so a name that can move (a branch, or a tag + // repointed between one peer fetching and another re-fetching after a restart) would let + // nodes in the same cluster run different commits. + it('resolves a tag to its commit SHA rather than passing the name through', () => { + assert.strictEqual(resolveGitCommittish('v1.2.3'), headSha); + }); + + it('resolves a branch name to its commit SHA', () => { + assert.strictEqual(resolveGitCommittish('HEAD'), headSha); + }); + + it('passes a ref through unresolved when git cannot resolve it locally', () => { + // e.g. a branch that exists only on the remote — better to let the cluster resolve it + // than to fail on a ref the user can plainly see. + assert.strictEqual(resolveGitCommittish('only-on-the-remote'), 'only-on-the-remote'); + }); + + // The cluster clones from the remote, so an unpushed commit fails server-side with an error + // far from the CLI. This temp repo has no remote at all, so nothing is "pushed". + it('warns when the commit to deploy is on no remote branch', () => { + const savedSha = process.env.GITHUB_SHA; + delete process.env.GITHUB_SHA; // take the local-resolution path, where the check applies + const written = []; + process.stderr.write = (chunk) => { + written.push(String(chunk)); + return true; + }; + try { + prepareDeployByRef({ by_ref: true, project: 'demo' }); + } finally { + if (savedSha === undefined) delete process.env.GITHUB_SHA; + else process.env.GITHUB_SHA = savedSha; + } + assert.match(written.join(''), /isn't on any remote branch/); + }); + + it('skips the push check under GITHUB_SHA, where CI is already on a pushed commit', () => { + const written = []; + process.stderr.write = (chunk) => { + written.push(String(chunk)); + return true; + }; + prepareDeployByRef({ by_ref: true, project: 'demo' }); // GITHUB_SHA set by the outer beforeEach + assert.doesNotMatch(written.join(''), /isn't on any remote branch/); + }); + }); + it('credential=true attaches a github.com credential reference', () => { const req = { by_ref: true, credential: true, project: 'demo' }; prepareDeployByRef(req); From 2dbef50381637c6126cd4bd8650d8c99f61bc31e Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Thu, 30 Jul 2026 09:35:33 -0400 Subject: [PATCH 3/7] refactor(cli): share the server's normalizeGitHost instead of restating it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit deriveGitSecretName re-implemented the host-normalization chain from components/gitCredentialServer.ts, with a comment warning it had to mirror the server EXACTLY — which is precisely the kind of duplication that drifts: a host quirk added on the server would silently produce a different secret name here, and the deploy would look up a credential that was never sealed. It now imports `normalizeGitHost` directly. gitCredentialServer.ts pulls in only node builtins plus the error/logger utilities `bin/` already uses, and has no top-level side effects, so the CLI pays nothing for it (verified: `harper version` still starts, 53 tests pass). Co-Authored-By: Claude Opus 4.8 --- bin/cliOperations.ts | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/bin/cliOperations.ts b/bin/cliOperations.ts index 5d25c701f0..5effbbb8ca 100644 --- a/bin/cliOperations.ts +++ b/bin/cliOperations.ts @@ -12,6 +12,7 @@ import * as YAML from 'yaml'; import { Readable } from 'node:stream'; import { execFileSync } from 'node:child_process'; import { streamPackagedDirectory, packageDirectory, scanPackageDirectory } from '../components/packageComponent.ts'; +import { normalizeGitHost } from '../components/gitCredentialServer.ts'; import { encode as encodeCbor } from 'cbor-x'; import { buildMultipartBody } from './multipartBuilder.ts'; import { parseSSE } from './sseConsumer.ts'; @@ -356,18 +357,14 @@ function defaultProjectName(projectPath: string): string { return path.basename(projectPath); } -// Mirror the server's `deriveGitSecretName` (secretOperations.ts) EXACTLY so the reference this -// attaches matches the row `harper deploy setup=true` sealed and a literal-token deploy would use: -// `deploy..git.` (host via normalizeGitHost). +// Must produce the same name as the server's `deriveGitSecretName` (secretOperations.ts), so the +// reference this attaches matches the row `harper deploy setup=true` sealed and a literal-token +// deploy would use: `deploy..git.`. Rather than restate the host-normalization +// chain, this shares the server's own `normalizeGitHost` — a future host quirk added there then +// can't drift from what the CLI sends. (gitCredentialServer.ts pulls in only node builtins plus +// the error/logger utilities `bin/` already uses, so importing it costs the CLI nothing.) function deriveGitSecretName(component: string, host: string): string { - const hostPart = host - .trim() - .replace(/^[a-z0-9+.-]+:\/\//i, '') - .replace(/^\/\//, '') - .replace(/\/.*$/, '') - .replace(/^[^@]*@/, '') - .toLowerCase() - .replace(/[^\w.-]+/g, '_'); + const hostPart = normalizeGitHost(host).replace(/[^\w.-]+/g, '_'); const componentPart = String(component).replace(/[^\w.-]+/g, '_'); return `deploy.${componentPart}.git.${hostPart}`; } From 168116f95c1d5e7484a3dc93ee514d571bc4d4c4 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Wed, 12 Aug 2026 11:34:08 -0400 Subject: [PATCH 4/7] fix(cli): fail closed on an unpinnable ref, and pin the credential to the package host MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback on deploy-by-reference: - An explicit `ref=` that this checkout can't resolve is no longer passed through by name. Names move, and peers resolve the package independently, so a pass-through preserved exactly the divergence the SHA pin exists to prevent. Resolution now falls back to `git ls-remote` (asking for the `^{}` peel patterns explicitly, so an annotated tag yields its commit rather than the tag object) and errors out when neither the checkout nor the remote can name a commit. A full object ID still passes through — it can't move. - On a GitHub Actions `pull_request` run, GITHUB_SHA is the synthetic refs/pull//merge commit, which a plain clone never fetches. The PR head from the event payload is deployed instead, from the head repo (a fork's commit isn't in GITHUB_REPOSITORY), and a missing payload fails early with the `github.event.pull_request.head.sha` workaround rather than at clone time on the cluster. - The credential host is derived from the package host instead of taken from `credential=`. A mismatched host built a reference the clone never asked for, so a private deploy failed as if unauthenticated; an explicit host is now accepted only when it agrees, and rejected loudly otherwise. Also: abbreviate the unpushed-commit warning to git's 7 characters, reject a ref spelled like a git option (git parses options anywhere in its argv), and run network git non-interactively with a timeout so a credential prompt can't hang the CLI. Co-Authored-By: Claude Opus 5 --- bin/cliOperations.ts | 194 +++++++++++++++++---- unitTests/bin/cliOperations.test.js | 256 ++++++++++++++++++++-------- 2 files changed, 350 insertions(+), 100 deletions(-) diff --git a/bin/cliOperations.ts b/bin/cliOperations.ts index 5effbbb8ca..dd733752f9 100644 --- a/bin/cliOperations.ts +++ b/bin/cliOperations.ts @@ -261,7 +261,8 @@ export { buildRequest, redactCredentials, refreshExpiredOperationToken, - resolveGitCommittish, + resolveGitTarget, + resolveCredentialHost, deriveGitSecretName, }; @@ -270,10 +271,35 @@ export { // `harper deploy by_ref=true` deploys a pinned commit by reference instead of uploading a payload // blob. Client-side: only the runner has the git context. The no-flag default stays the payload deploy. +// resolveGitRepo only recognizes GitHub remotes, so every by_ref package clones from this host. The +// credential host is derived from it rather than taken on the user's word (see resolveCredentialHost). +const GIT_PACKAGE_HOST = 'github.com'; +// SHA-1 (40) or SHA-256 (64) object IDs. A full object ID is already immutable, so it's the one form +// of ref that needs no resolution; every other form can move. +const FULL_OBJECT_ID = /^[0-9a-f]{40}(?:[0-9a-f]{24})?$/i; +// `ls-remote` reaches the network, where git will otherwise block indefinitely on an interactive +// credential prompt the CLI can't render. Fail fast instead, and cap the whole call. +const NON_INTERACTIVE_GIT_ENV = { + GIT_TERMINAL_PROMPT: '0', + GIT_ASKPASS: 'echo', + SSH_ASKPASS: 'echo', + GIT_SSH_COMMAND: 'ssh -oBatchMode=yes', +}; +const GIT_NETWORK_TIMEOUT_MS = 15000; + function runGit(args: string[]): string { return execFileSync('git', args, { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }).trim(); } +function runGitNetwork(args: string[]): string { + return execFileSync('git', args, { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'ignore'], + env: { ...process.env, ...NON_INTERACTIVE_GIT_ENV }, + timeout: GIT_NETWORK_TIMEOUT_MS, + }).trim(); +} + function resolveGitRepo(): string { if (process.env.GITHUB_REPOSITORY) return process.env.GITHUB_REPOSITORY; let url: string; @@ -290,27 +316,118 @@ function resolveGitRepo(): string { return match[1]; } -function resolveGitCommittish(ref: unknown): string { +// Prefer the configured `origin`: it carries whatever credentials, mirrors, and url.insteadOf +// rewriting the user's git is already set up with. The public URL is only for a checkout that has +// no remote at all (e.g. CI that exported GITHUB_REPOSITORY without adding one). +function resolveGitRemote(repo: string): string { + try { + if (runGit(['remote', 'get-url', 'origin'])) return 'origin'; + } catch { + // No origin. + } + return `https://${GIT_PACKAGE_HOST}/${repo}.git`; +} + +// `ls-remote` reports an annotated tag's peeled commit on a trailing `^{}` line — but only when a +// pattern matches that line, so the peel patterns have to be asked for explicitly. Without them a tag +// resolves to the *tag object's* ID, which is not a commit the cluster can check out. Tags outrank +// branches, matching git's own precedence for a bare name (gitrevisions). +function resolveRefOnRemote(remote: string, ref: string): string | undefined { + let output: string; + try { + output = runGitNetwork([ + 'ls-remote', + remote, + ref, + `${ref}^{}`, + `refs/heads/${ref}`, + `refs/tags/${ref}`, + `refs/tags/${ref}^{}`, + ]); + } catch { + return undefined; // unreachable, unauthenticated, or too slow to answer + } + const shaByRef = new Map(); + for (const line of output.split('\n')) { + const [sha, name] = line.split('\t'); + if (sha && name) shaByRef.set(name, sha); + } + if (shaByRef.size === 1) return shaByRef.values().next().value; // an unambiguous match, whatever it matched + return shaByRef.get(`refs/tags/${ref}^{}`) ?? shaByRef.get(`refs/tags/${ref}`) ?? shaByRef.get(`refs/heads/${ref}`); +} + +// An explicit ref= is pinned to an immutable SHA, exactly as HEAD is. Cluster peers resolve the +// package independently, so `ref=main` — or a tag repointed between one peer fetching and another +// re-fetching after a restart — would otherwise leave nodes running different commits. Resolution is +// local where possible (`^{commit}` also dereferences annotated tags), then falls back to the remote +// for a ref this checkout doesn't have (a shallow CI clone has almost none), and fails closed if +// neither can name a commit: a ref that can't be pinned is the divergence the pin exists to prevent. +function resolveExplicitRef(ref: string, repo: string): string { + // git parses options anywhere in its argv, so a ref spelled like one (`--upload-pack=…`) would be + // obeyed as an option instead of resolved. No real ref starts with `-` — git rejects those itself. + if (ref.startsWith('-')) throw new Error(`deploy by_ref: invalid ref=${ref} — a git ref cannot start with "-".`); + try { + return runGit(['rev-parse', '--verify', `${ref}^{commit}`]); + } catch { + // Not in this checkout — try the remote below. + } + if (FULL_OBJECT_ID.test(ref)) return ref; // already immutable; nothing to pin it to + const remote = resolveGitRemote(repo); + const resolved = resolveRefOnRemote(remote, ref); + if (resolved) return resolved; + throw new Error( + `deploy by_ref: could not resolve ref=${ref} to a commit, locally or on ${remote}. Peers resolve the ` + + 'package independently, so a ref that moves would leave them on different commits — run `git fetch` ' + + 'and retry, or pass a full commit SHA.' + ); +} + +// GitHub Actions checks out a *synthetic merge commit* on a `pull_request` run: GITHUB_SHA points at +// refs/pull//merge, which a plain clone never fetches (its default refspec covers refs/heads/* and +// refs/tags/* only), so deploying it fails server-side at clone time. The event payload carries the PR +// head — a real commit on a real branch — so deploy that instead, from the head repo, which for a fork +// isn't GITHUB_REPOSITORY. See the pull_request section of GitHub's events-that-trigger-workflows docs. +function resolveActionsPullRequestHead(): { repo: string; committish: string } | undefined { + if (!/^refs\/pull\//.test(process.env.GITHUB_REF ?? '')) return undefined; + const eventPath = process.env.GITHUB_EVENT_PATH; + let head: any; + try { + if (eventPath) head = JSON.parse(fs.readFileSync(eventPath, 'utf8'))?.pull_request?.head; + } catch { + // Missing or unparseable payload — the error below is the useful outcome either way. + } + const committish = typeof head?.sha === 'string' ? head.sha : undefined; + const repo = typeof head?.repo?.full_name === 'string' ? head.repo.full_name : undefined; + if (!committish || !repo) { + throw new Error( + `deploy by_ref: GITHUB_SHA on a ${process.env.GITHUB_REF} run is a synthetic merge commit that a plain ` + + 'clone cannot fetch, and the pull request head could not be read from GITHUB_EVENT_PATH. Pass the ' + + 'head commit explicitly: ref=${{ github.event.pull_request.head.sha }}.' + ); + } + if (repo !== process.env.GITHUB_REPOSITORY) { + process.stderr.write(`note: deploying the pull request head from ${repo}, not ${process.env.GITHUB_REPOSITORY}.\n`); + } + return { repo, committish }; +} + +// Repo and commit are resolved together because they aren't independent: on a pull_request run both +// come from the PR head, and pairing a head SHA with the base repo would name a commit that repo +// doesn't have. +function resolveGitTarget(ref: unknown): { repo: string; committish: string } { // buildRequest JSON-parses CLI args, so a numeric ref (e.g. `ref=1234567`) arrives as a number — // coerce it back to a string rather than silently ignoring it and falling back to HEAD. const refStr = typeof ref === 'string' || typeof ref === 'number' ? String(ref).trim() : ''; if (refStr.length > 0) { - // Resolve an explicit ref= to an immutable SHA too, rather than passing the name through. - // Cluster peers resolve the package independently, so `ref=main` — or a tag that moves - // between one peer fetching and another restarting and re-fetching — would otherwise leave - // nodes running different commits. `^{commit}` also dereferences annotated tags. - 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, but a deploy that works beats a - // deploy that fails on a ref the user can see on the remote. - return refStr; - } + const repo = resolveGitRepo(); + return { repo, committish: resolveExplicitRef(refStr, repo) }; } - if (process.env.GITHUB_SHA) return process.env.GITHUB_SHA; + const pullRequestHead = resolveActionsPullRequestHead(); + if (pullRequestHead) return pullRequestHead; + const repo = resolveGitRepo(); + if (process.env.GITHUB_SHA) return { repo, committish: process.env.GITHUB_SHA }; try { - return runGit(['rev-parse', 'HEAD']); + return { repo, committish: runGit(['rev-parse', 'HEAD']) }; } catch { throw new Error('deploy by_ref: could not resolve HEAD — make at least one commit, or pass ref=.'); } @@ -324,7 +441,7 @@ function warnIfWorkingTreeDirty(): void { ); } } catch { - // Not a git repo; resolveGitRepo/resolveGitCommittish will surface a clearer error. + // Not a git repo; resolveGitTarget will surface a clearer error. } } @@ -337,7 +454,7 @@ function warnIfCommitNotPushed(committish: string): void { try { if (!runGit(['branch', '-r', '--contains', committish])) { process.stderr.write( - `warning: commit ${committish.slice(0, 12)} isn't on any remote branch — push it, or the cluster ` + + `warning: commit ${committish.slice(0, 7)} isn't on any remote branch — push it, or the cluster ` + "won't be able to clone it. (If you already pushed, run `git fetch` to refresh your remote refs.)\n" ); } @@ -369,28 +486,43 @@ function deriveGitSecretName(component: string, host: string): string { return `deploy.${componentPart}.git.${hostPart}`; } +// The credential only helps if it's for the host the package is cloned from: a `credential=gitlab.com` +// against a github.com package builds a valid-looking reference the clone never asks for, and the +// private deploy then fails as if nothing were configured. So the host comes from the package rather +// than the user — an explicit value is accepted only when it agrees (`credential=github.com` is the +// documented spelling), and rejected loudly rather than silently producing a mismatched pair. +function resolveCredentialHost(credential: unknown, packageHost: string): string | undefined { + if (credential === undefined || credential === false || credential === '') return undefined; + if (credential === true) return packageHost; + const host = normalizeGitHost(String(credential)); + if (host !== packageHost) { + throw new Error( + `deploy by_ref: credential=${credential} doesn't match the package host ${packageHost} — the clone ` + + `authenticates against ${packageHost}, so a credential for another host would never be used. Use ` + + 'credential=true.' + ); + } + return packageHost; +} + // Opt-in deploy-by-reference: resolve the pinned git ref (+ optional sealed credential) onto `req` — // a `git+https` package pinned by SHA, plus a `credentials` reference when `credential=` is set. // Exported for unit tests. export function prepareDeployByRef(req: any): void { - const repo = resolveGitRepo(); - const committish = resolveGitCommittish(req.ref); + const { repo, committish } = resolveGitTarget(req.ref); warnIfWorkingTreeDirty(); - // Skipped under GITHUB_SHA: CI runs on a commit that is on the remote by construction, and a - // shallow/detached runner checkout has no remote-tracking branches to check against anyway. + // Skipped under GITHUB_SHA: on the one GitHub event where the checked-out commit isn't on a + // cloneable branch (pull_request), resolveGitTarget already substitutes the PR head, so what's + // left is pushed by construction — and a shallow/detached runner checkout has no remote-tracking + // branches to check it against anyway. if (!process.env.GITHUB_SHA) warnIfCommitNotPushed(committish); if (!req.project) req.project = defaultProjectName(process.cwd()); // git+https (not ssh): a private clone is authenticated by a git-host token credential (#1799), // which rides over HTTPS. A public repo needs no credential at all. - req.package = `git+https://github.com/${repo}.git#${committish}`; - // `credential=github.com` (or `credential=true`) attaches the sealed-token reference the cluster - // resolves at fetch time — provision it once with `harper deploy setup=true`. - const credentialHost = - req.credential === true - ? 'github.com' - : typeof req.credential === 'string' && req.credential.length > 0 - ? req.credential - : undefined; + req.package = `git+https://${GIT_PACKAGE_HOST}/${repo}.git#${committish}`; + // `credential=true` attaches the sealed-token reference the cluster resolves at fetch time — + // provision it once with `harper deploy setup=true`. + const credentialHost = resolveCredentialHost(req.credential, GIT_PACKAGE_HOST); if (credentialHost && req.credentials === undefined) { req.credentials = [{ host: credentialHost, secret: deriveGitSecretName(req.project, credentialHost) }]; } diff --git a/unitTests/bin/cliOperations.test.js b/unitTests/bin/cliOperations.test.js index bd65dd0eeb..f227b4c236 100644 --- a/unitTests/bin/cliOperations.test.js +++ b/unitTests/bin/cliOperations.test.js @@ -1161,28 +1161,45 @@ describe('cliOperations', () => { }); describe('deploy by reference (by_ref)', () => { - const { prepareDeployByRef, resolveGitCommittish, deriveGitSecretName } = cliOperationsModule; - let savedRepo, savedSha, savedStderrWrite; + const { prepareDeployByRef, resolveGitTarget, resolveCredentialHost, deriveGitSecretName } = cliOperationsModule; + const GITHUB_ENV = ['GITHUB_REPOSITORY', 'GITHUB_SHA', 'GITHUB_REF', 'GITHUB_EVENT_PATH']; + let savedEnv, savedStderrWrite; beforeEach(() => { - savedRepo = process.env.GITHUB_REPOSITORY; - savedSha = process.env.GITHUB_SHA; + savedEnv = new Map(GITHUB_ENV.map((name) => [name, process.env[name]])); // Resolve deterministically from env so the tests never shell out to git. process.env.GITHUB_REPOSITORY = 'acme/demo'; process.env.GITHUB_SHA = 'abc123def456'; + delete process.env.GITHUB_REF; + delete process.env.GITHUB_EVENT_PATH; // Silence the "Deploying … by reference" line prepareDeployByRef writes to stderr. savedStderrWrite = process.stderr.write; process.stderr.write = () => true; }); afterEach(() => { - if (savedRepo === undefined) delete process.env.GITHUB_REPOSITORY; - else process.env.GITHUB_REPOSITORY = savedRepo; - if (savedSha === undefined) delete process.env.GITHUB_SHA; - else process.env.GITHUB_SHA = savedSha; + for (const [name, value] of savedEnv) { + if (value === undefined) delete process.env[name]; + else process.env[name] = value; + } process.stderr.write = savedStderrWrite; }); + // Collects everything written to stderr while fn runs, so warnings/notes can be asserted on. + function captureStderr(fn) { + const written = []; + process.stderr.write = (chunk) => { + written.push(String(chunk)); + return true; + }; + try { + fn(); + } finally { + process.stderr.write = () => true; + } + return written.join(''); + } + it('builds a git+https package pinned to the resolved SHA', () => { const req = { by_ref: true, project: 'demo' }; prepareDeployByRef(req); @@ -1190,34 +1207,46 @@ describe('deploy by reference (by_ref)', () => { assert.strictEqual(req.credentials, undefined); // no credential requested }); - it('an explicit ref= wins over GITHUB_SHA', () => { - // `no-such-ref-here` doesn't resolve locally, so this also covers the pass-through fallback. - const req = { by_ref: true, ref: 'no-such-ref-here', project: 'demo' }; - prepareDeployByRef(req); - assert.strictEqual(req.package, 'git+https://github.com/acme/demo.git#no-such-ref-here'); - }); - - it('resolveGitCommittish coerces a numeric ref to a string (buildRequest JSON-parses it)', () => { - assert.strictEqual(resolveGitCommittish(1234567), '1234567'); - assert.strictEqual(resolveGitCommittish('v1.0.0'), 'v1.0.0'); - }); - - // A real repo with known refs, so "resolves to a SHA" is asserted against an actual git - // resolution rather than whatever tags happen to exist in the checkout running the tests. - describe('explicit refs are pinned to an immutable SHA', () => { + // A real repo with known refs — plus a local bare repo standing in for `origin` — so ref + // resolution is asserted against actual git behavior rather than whatever happens to exist in the + // checkout running the tests, and without reaching the network. + describe('ref resolution against a real repository', () => { const { execFileSync } = require('node:child_process'); - let repoDir, priorCwd, headSha; + let rootDir, repoDir, priorCwd, headSha, remoteOnlySha, tagObjectSha; before(() => { - repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'harper-by-ref-')); - const git = (...args) => - execFileSync('git', args, { cwd: repoDir, encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }).trim(); + rootDir = fs.mkdtempSync(path.join(os.tmpdir(), 'harper-by-ref-')); + repoDir = path.join(rootDir, 'work'); + const remoteDir = path.join(rootDir, 'remote.git'); + fs.mkdirSync(repoDir); + const runIn = (cwd, ...args) => + execFileSync('git', args, { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }).trim(); + const git = (...args) => runIn(repoDir, ...args); + runIn(rootDir, 'init', '-q', '--bare', remoteDir); git('init', '-q'); git('config', 'user.email', 'test@example.com'); git('config', 'user.name', 'Test'); git('commit', '-q', '--allow-empty', '-m', 'first'); git('tag', 'v1.2.3'); + git('branch', '1234567'); // a branch name that JSON.parse turns into a number headSha = git('rev-parse', 'HEAD'); + git('remote', 'add', 'origin', remoteDir); + // A commit + annotated tag that exist only on the remote: pushed from a detached HEAD, then + // every local trace removed. This is the shallow/never-fetched case, where local resolution + // must fail and `ls-remote` has to supply the SHA. + git('checkout', '-q', '--detach'); + git('commit', '-q', '--allow-empty', '-m', 'second'); + remoteOnlySha = git('rev-parse', 'HEAD'); + git('tag', '-a', 'v2.0.0', '-m', 'annotated'); + tagObjectSha = git('rev-parse', 'v2.0.0'); // the tag object, not the commit it points at + git('push', '-q', 'origin', 'HEAD:refs/heads/remote-only', 'v2.0.0'); + git('tag', '-d', 'v2.0.0'); + git('checkout', '-q', headSha); + // Drop the remote-tracking refs push just created, so nothing resolves locally by accident + // (and so the "commit isn't on a remote branch" check has nothing to find). + for (const ref of git('for-each-ref', '--format=%(refname)', 'refs/remotes').split('\n').filter(Boolean)) { + git('update-ref', '-d', ref); + } // runGit shells out against the real process cwd, so mocking process.cwd isn't enough. priorCwd = process.cwd(); process.chdir(repoDir); @@ -1225,69 +1254,158 @@ describe('deploy by reference (by_ref)', () => { after(() => { process.chdir(priorCwd); - fs.rmSync(repoDir, { recursive: true, force: true }); + fs.rmSync(rootDir, { recursive: true, force: true }); }); // Peers resolve the package independently, so a name that can move (a branch, or a tag // repointed between one peer fetching and another re-fetching after a restart) would let // nodes in the same cluster run different commits. - it('resolves a tag to its commit SHA rather than passing the name through', () => { - assert.strictEqual(resolveGitCommittish('v1.2.3'), headSha); + it('resolves a local tag to its commit SHA rather than passing the name through', () => { + assert.strictEqual(resolveGitTarget('v1.2.3').committish, headSha); + }); + + it('resolves a local branch name to its commit SHA', () => { + assert.strictEqual(resolveGitTarget('HEAD').committish, headSha); + }); + + it('coerces a numeric ref to a string (buildRequest JSON-parses `ref=1234567` to a number)', () => { + assert.strictEqual(resolveGitTarget(1234567).committish, headSha); }); - it('resolves a branch name to its commit SHA', () => { - assert.strictEqual(resolveGitCommittish('HEAD'), headSha); + it('an explicit ref= wins over GITHUB_SHA', () => { + const req = { by_ref: true, ref: 'v1.2.3', project: 'demo' }; + prepareDeployByRef(req); + assert.strictEqual(req.package, `git+https://github.com/acme/demo.git#${headSha}`); }); - it('passes a ref through unresolved when git cannot resolve it locally', () => { - // e.g. a branch that exists only on the remote — better to let the cluster resolve it - // than to fail on a ref the user can plainly see. - assert.strictEqual(resolveGitCommittish('only-on-the-remote'), 'only-on-the-remote'); + it('resolves a ref that exists only on the remote via ls-remote', () => { + assert.strictEqual(resolveGitTarget('remote-only').committish, remoteOnlySha); }); - // The cluster clones from the remote, so an unpushed commit fails server-side with an error - // far from the CLI. This temp repo has no remote at all, so nothing is "pushed". + // An annotated tag's own object ID is not a commit, so a checkout of it would fail server-side. + it('peels a remote annotated tag to its commit, not the tag object', () => { + assert.strictEqual(resolveGitTarget('v2.0.0').committish, remoteOnlySha); + assert.notStrictEqual(resolveGitTarget('v2.0.0').committish, tagObjectSha); + }); + + // Failing closed is the point: passing an unresolvable name through preserved exactly the + // divergence the SHA pin exists to prevent. + it('fails closed when a ref resolves neither locally nor on the remote', () => { + assert.throws(() => resolveGitTarget('no-such-ref-anywhere'), /could not resolve ref=no-such-ref-anywhere/); + }); + + // The one ref that needs no resolution — it can't move — so an unfetched full SHA still deploys. + it('passes a full commit SHA through unresolved', () => { + const sha = 'f'.repeat(40); + assert.strictEqual(resolveGitTarget(sha).committish, sha); + }); + + // git parses options anywhere in its argv, so `ref=--upload-pack=` would otherwise reach + // `git ls-remote` as an option and run that command. + it('rejects a ref that would be parsed as a git option', () => { + assert.throws(() => resolveGitTarget('--upload-pack=touch /tmp/pwned'), /cannot start with "-"/); + }); + + // The cluster clones from the remote, so an unpushed commit fails server-side with an error far + // from the CLI. This repo's remote-tracking refs were dropped above, so nothing looks pushed. it('warns when the commit to deploy is on no remote branch', () => { - const savedSha = process.env.GITHUB_SHA; delete process.env.GITHUB_SHA; // take the local-resolution path, where the check applies - const written = []; - process.stderr.write = (chunk) => { - written.push(String(chunk)); - return true; - }; - try { - prepareDeployByRef({ by_ref: true, project: 'demo' }); - } finally { - if (savedSha === undefined) delete process.env.GITHUB_SHA; - else process.env.GITHUB_SHA = savedSha; - } - assert.match(written.join(''), /isn't on any remote branch/); + const written = captureStderr(() => prepareDeployByRef({ by_ref: true, project: 'demo' })); + assert.match(written, /isn't on any remote branch/); + assert.match(written, new RegExp(headSha.slice(0, 7))); // abbreviated to git's 7 characters }); it('skips the push check under GITHUB_SHA, where CI is already on a pushed commit', () => { - const written = []; - process.stderr.write = (chunk) => { - written.push(String(chunk)); - return true; - }; - prepareDeployByRef({ by_ref: true, project: 'demo' }); // GITHUB_SHA set by the outer beforeEach - assert.doesNotMatch(written.join(''), /isn't on any remote branch/); + const written = captureStderr(() => prepareDeployByRef({ by_ref: true, project: 'demo' })); + assert.doesNotMatch(written, /isn't on any remote branch/); }); }); - it('credential=true attaches a github.com credential reference', () => { - const req = { by_ref: true, credential: true, project: 'demo' }; - prepareDeployByRef(req); - assert.deepStrictEqual(req.credentials, [{ host: 'github.com', secret: 'deploy.demo.git.github.com' }]); - }); + // GITHUB_SHA on a pull_request run is the synthetic refs/pull//merge commit, which a plain clone + // never fetches — deploying it fails server-side at clone time. + describe('GitHub Actions pull_request runs', () => { + let eventDir; - it('credential= attaches a credential reference for that host', () => { - const req = { by_ref: true, credential: 'github.com', project: 'my-app' }; - prepareDeployByRef(req); - assert.deepStrictEqual(req.credentials, [{ host: 'github.com', secret: 'deploy.my-app.git.github.com' }]); + beforeEach(() => { + eventDir = fs.mkdtempSync(path.join(os.tmpdir(), 'harper-by-ref-event-')); + process.env.GITHUB_REF = 'refs/pull/42/merge'; + }); + + afterEach(() => { + fs.rmSync(eventDir, { recursive: true, force: true }); + }); + + function writeEvent(payload) { + const eventPath = path.join(eventDir, 'event.json'); + fs.writeFileSync(eventPath, JSON.stringify(payload)); + process.env.GITHUB_EVENT_PATH = eventPath; + } + + it('deploys the pull request head commit instead of the merge commit', () => { + writeEvent({ pull_request: { head: { sha: 'deadbeef'.repeat(5), repo: { full_name: 'acme/demo' } } } }); + const req = { by_ref: true, project: 'demo' }; + prepareDeployByRef(req); + assert.strictEqual(req.package, `git+https://github.com/acme/demo.git#${'deadbeef'.repeat(5)}`); + }); + + // For a fork PR the head commit lives in the fork, not GITHUB_REPOSITORY — pairing the head SHA + // with the base repo would name a commit that repo doesn't have. + it('uses the head repository, and says so, when the pull request comes from a fork', () => { + writeEvent({ pull_request: { head: { sha: 'abc'.repeat(13) + 'd', repo: { full_name: 'forker/demo' } } } }); + const req = { by_ref: true, project: 'demo' }; + const written = captureStderr(() => prepareDeployByRef(req)); + assert.match(req.package, /^git\+https:\/\/github\.com\/forker\/demo\.git#/); + assert.match(written, /pull request head from forker\/demo/); + }); + + it('fails early with actionable guidance when the head cannot be read', () => { + writeEvent({ pull_request: {} }); + assert.throws( + () => prepareDeployByRef({ by_ref: true, project: 'demo' }), + /synthetic merge commit[\s\S]*github\.event\.pull_request\.head\.sha/ + ); + }); + + it('still honors an explicit ref= on a pull_request run', () => { + writeEvent({ pull_request: {} }); // unreadable head, but ref= means it is never consulted + const sha = 'a'.repeat(40); + const req = { by_ref: true, ref: sha, project: 'demo' }; + prepareDeployByRef(req); + assert.strictEqual(req.package, `git+https://github.com/acme/demo.git#${sha}`); + }); }); - it('deriveGitSecretName matches the server convention (deploy..git.)', () => { - assert.strictEqual(deriveGitSecretName('my-app', 'github.com'), 'deploy.my-app.git.github.com'); + describe('credential reference', () => { + it('credential=true attaches a github.com credential reference', () => { + const req = { by_ref: true, credential: true, project: 'demo' }; + prepareDeployByRef(req); + assert.deepStrictEqual(req.credentials, [{ host: 'github.com', secret: 'deploy.demo.git.github.com' }]); + }); + + it('credential=github.com agrees with the package host and is accepted', () => { + const req = { by_ref: true, credential: 'github.com', project: 'my-app' }; + prepareDeployByRef(req); + assert.deepStrictEqual(req.credentials, [{ host: 'github.com', secret: 'deploy.my-app.git.github.com' }]); + }); + + // A credential for another host builds a reference the clone never asks for, so the private + // deploy fails as if none were configured — reject it instead of shipping the mismatch. + it('rejects a credential host that is not the host the package clones from', () => { + assert.throws( + () => prepareDeployByRef({ by_ref: true, credential: 'gitlab.com', project: 'demo' }), + /credential=gitlab\.com doesn't match the package host github\.com/ + ); + }); + + it('normalizes an explicit host before comparing it', () => { + assert.strictEqual(resolveCredentialHost('https://GitHub.com/acme/demo', 'github.com'), 'github.com'); + assert.strictEqual(resolveCredentialHost(true, 'github.com'), 'github.com'); + assert.strictEqual(resolveCredentialHost(undefined, 'github.com'), undefined); + assert.strictEqual(resolveCredentialHost('', 'github.com'), undefined); + }); + + it('deriveGitSecretName matches the server convention (deploy..git.)', () => { + assert.strictEqual(deriveGitSecretName('my-app', 'github.com'), 'deploy.my-app.git.github.com'); + }); }); }); From 2ee765fdc0ab45123190a5d4a99cc5cd1c4fb23e Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Wed, 12 Aug 2026 11:37:52 -0400 Subject: [PATCH 5/7] test(cli): correct a stale comment about shelling out to git Co-Authored-By: Claude Opus 5 --- unitTests/bin/cliOperations.test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/unitTests/bin/cliOperations.test.js b/unitTests/bin/cliOperations.test.js index f227b4c236..3e0223f890 100644 --- a/unitTests/bin/cliOperations.test.js +++ b/unitTests/bin/cliOperations.test.js @@ -1167,7 +1167,8 @@ describe('deploy by reference (by_ref)', () => { beforeEach(() => { savedEnv = new Map(GITHUB_ENV.map((name) => [name, process.env[name]])); - // Resolve deterministically from env so the tests never shell out to git. + // The default target resolves from env, so tests that aren't about git resolution get a fixed + // repo/SHA. The nested repo describe below overrides this to exercise git itself. process.env.GITHUB_REPOSITORY = 'acme/demo'; process.env.GITHUB_SHA = 'abc123def456'; delete process.env.GITHUB_REF; From 36dae1a522c2b313a783e151d7ab870fc4380e88 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Wed, 12 Aug 2026 11:43:57 -0400 Subject: [PATCH 6/7] fix(cli): stop JSON-parsing ref=, which rewrote a numeric-looking tag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `buildRequest` JSON-parses every CLI value, so `ref=1.0` arrived as the number 1 and resolved as the tag "1" — coercing back to a string downstream can't recover what the parse threw away (1.10 -> "1.1", 1e3 -> "1000"). A git ref is an opaque string, so it now skips the parse entirely. The earlier coercion fix covered `ref=1234567` but not the `ref=1.0` case from the same review comment. The coercion stays: prepareDeployByRef is exported and callable with a hand-built req. Co-Authored-By: Claude Opus 5 --- bin/cliOperations.ts | 19 ++++++++++----- unitTests/bin/cliOperations.test.js | 38 ++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 7 deletions(-) diff --git a/bin/cliOperations.ts b/bin/cliOperations.ts index dd733752f9..d1095f0e1f 100644 --- a/bin/cliOperations.ts +++ b/bin/cliOperations.ts @@ -55,6 +55,11 @@ const TRANSPORT_ONLY_FIELDS = new Set([ 'credential', ]); +// Values that are opaque strings, never JSON. buildRequest otherwise JSON-parses every value, which +// silently rewrites a git ref that happens to look numeric: `ref=1.0` becomes the number 1 (and then +// the string "1"), so a tag named "1.0" would be resolved as "1". Refs can't be anything but strings. +const RAW_STRING_FIELDS = new Set(['ref']); + // Streaming (multipart upload + SSE progress) deploy was introduced in 5.1.0. A CLI at >= // 5.1 talking to a server < 5.1 must not use it: the older server has no multipart body // parser (the upload is rejected) and its generic text/event-stream serializer emits a bare @@ -415,8 +420,8 @@ function resolveActionsPullRequestHead(): { repo: string; committish: string } | // come from the PR head, and pairing a head SHA with the base repo would name a commit that repo // doesn't have. function resolveGitTarget(ref: unknown): { repo: string; committish: string } { - // buildRequest JSON-parses CLI args, so a numeric ref (e.g. `ref=1234567`) arrives as a number — - // coerce it back to a string rather than silently ignoring it and falling back to HEAD. + // `ref` reaches here as a raw string from buildRequest (see RAW_STRING_FIELDS), but a number is + // still coerced rather than ignored — prepareDeployByRef is callable with a hand-built req. const refStr = typeof ref === 'string' || typeof ref === 'number' ? String(ref).trim() : ''; if (refStr.length > 0) { const repo = resolveGitRepo(); @@ -582,10 +587,12 @@ function buildRequest(): any { let [first, ...rest] = arg.split('='); let restStr: any = rest.join('='); - try { - restStr = JSON.parse(restStr); - } catch { - /* noop */ + if (!RAW_STRING_FIELDS.has(first)) { + try { + restStr = JSON.parse(restStr); + } catch { + /* noop */ + } } req[first] = restStr; diff --git a/unitTests/bin/cliOperations.test.js b/unitTests/bin/cliOperations.test.js index 3e0223f890..6f8043a0f3 100644 --- a/unitTests/bin/cliOperations.test.js +++ b/unitTests/bin/cliOperations.test.js @@ -1208,6 +1208,40 @@ describe('deploy by reference (by_ref)', () => { assert.strictEqual(req.credentials, undefined); // no credential requested }); + // Every other CLI value is JSON-parsed, which rewrites a ref that happens to look numeric: `1.0` + // parses to the number 1, and no amount of coercion downstream can turn that back into the tag + // the user typed. Refs are opaque strings, so they skip the parse entirely. + describe('ref parsing', () => { + const { buildRequest } = cliOperationsModule; + let savedArgv; + + beforeEach(() => { + savedArgv = process.argv; + }); + + afterEach(() => { + process.argv = savedArgv; + }); + + function refFromArgv(arg) { + process.argv = ['node', 'harper', 'deploy', arg]; + return buildRequest().ref; + } + + it('keeps a ref that looks numeric exactly as typed', () => { + assert.strictEqual(refFromArgv('ref=1.0'), '1.0'); // not the number 1 + assert.strictEqual(refFromArgv('ref=1.10'), '1.10'); // not 1.1 + assert.strictEqual(refFromArgv('ref=1e3'), '1e3'); // not 1000 + assert.strictEqual(refFromArgv('ref=1234567'), '1234567'); + }); + + it('leaves ordinary refs and other fields alone', () => { + assert.strictEqual(refFromArgv('ref=v1.2.3'), 'v1.2.3'); + process.argv = ['node', 'harper', 'deploy', 'by_ref=true']; + assert.strictEqual(buildRequest().by_ref, true); // still JSON-parsed + }); + }); + // A real repo with known refs — plus a local bare repo standing in for `origin` — so ref // resolution is asserted against actual git behavior rather than whatever happens to exist in the // checkout running the tests, and without reaching the network. @@ -1269,7 +1303,9 @@ describe('deploy by reference (by_ref)', () => { assert.strictEqual(resolveGitTarget('HEAD').committish, headSha); }); - it('coerces a numeric ref to a string (buildRequest JSON-parses `ref=1234567` to a number)', () => { + // prepareDeployByRef is exported and callable with a hand-built req, so a number still resolves + // rather than being ignored — buildRequest itself no longer produces one (see below). + it('coerces a numeric ref to a string', () => { assert.strictEqual(resolveGitTarget(1234567).committish, headSha); }); From 2738b40cc9c04a056681260b67ad29f8606adc85 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Fri, 14 Aug 2026 09:44:54 -0400 Subject: [PATCH 7/7] fix(cli): only resolve refs a clone can fetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remote resolution passed the bare `ref` as its own ls-remote pattern, so `ref=refs/pull/42/head` matched a single ref and returned it. That SHA is immutable but outside the default clone refspec, so the cluster resolves it and then can't check it out — the same reachability failure already fixed for GITHUB_SHA on pull_request runs, reached by typing it instead. Every ls-remote pattern is now namespace-qualified, and the "lone match wins" shortcut that let an arbitrary namespace through is gone. The namespace check runs before local resolution, not only on the remote path: a checkout that has fetched refs/pull//head resolves it happily, and the resulting SHA is just as unreachable. It can't catch a bare SHA that happens to be unreachable — an object ID carries no namespace to inspect. Co-Authored-By: Claude Opus 5 --- bin/cliOperations.ts | 39 ++++++++++++++++++++--------- unitTests/bin/cliOperations.test.js | 27 ++++++++++++++++++++ 2 files changed, 54 insertions(+), 12 deletions(-) diff --git a/bin/cliOperations.ts b/bin/cliOperations.ts index d1095f0e1f..2f5b23b639 100644 --- a/bin/cliOperations.ts +++ b/bin/cliOperations.ts @@ -333,22 +333,32 @@ function resolveGitRemote(repo: string): string { return `https://${GIT_PACKAGE_HOST}/${repo}.git`; } +// A plain clone fetches refs/heads/* and refs/tags/* and nothing else, so those are the only +// namespaces a deployable ref can live in. A commit named through any other — refs/pull//head is +// the one people reach for — pins to a perfectly immutable SHA that the cluster then cannot check +// out, failing the clone exactly as the pull_request merge commit would. Rejecting the namespace +// catches that here, where the user can act on it, rather than on the cluster. It can't catch a bare +// SHA that happens to be unreachable: an object ID carries no namespace to inspect. +function assertCloneableRefNamespace(ref: string): void { + if (!ref.startsWith('refs/') || ref.startsWith('refs/heads/') || ref.startsWith('refs/tags/')) return; + throw new Error( + `deploy by_ref: ref=${ref} is outside refs/heads/ and refs/tags/, the only namespaces a clone ` + + 'fetches — the cluster could resolve that commit but never check it out. Pass a branch or tag the ' + + 'commit is on.' + ); +} + // `ls-remote` reports an annotated tag's peeled commit on a trailing `^{}` line — but only when a // pattern matches that line, so the peel patterns have to be asked for explicitly. Without them a tag -// resolves to the *tag object's* ID, which is not a commit the cluster can check out. Tags outrank -// branches, matching git's own precedence for a bare name (gitrevisions). +// resolves to the *tag object's* ID, which is not a commit the cluster can check out. Every pattern is +// namespace-qualified: passing the bare `ref` as its own pattern would match any namespace ls-remote +// happens to serve, which is how an unreachable ref would slip through as a lone "unambiguous" match. function resolveRefOnRemote(remote: string, ref: string): string | undefined { + const qualified = ref.startsWith('refs/'); + const patterns = qualified ? [ref, `${ref}^{}`] : [`refs/tags/${ref}`, `refs/tags/${ref}^{}`, `refs/heads/${ref}`]; let output: string; try { - output = runGitNetwork([ - 'ls-remote', - remote, - ref, - `${ref}^{}`, - `refs/heads/${ref}`, - `refs/tags/${ref}`, - `refs/tags/${ref}^{}`, - ]); + output = runGitNetwork(['ls-remote', remote, ...patterns]); } catch { return undefined; // unreachable, unauthenticated, or too slow to answer } @@ -357,7 +367,9 @@ function resolveRefOnRemote(remote: string, ref: string): string | undefined { const [sha, name] = line.split('\t'); if (sha && name) shaByRef.set(name, sha); } - if (shaByRef.size === 1) return shaByRef.values().next().value; // an unambiguous match, whatever it matched + // Peeled commit first (an annotated tag object isn't checkout-able), then tags over branches, which + // is git's own precedence for a bare name (gitrevisions). + if (qualified) return shaByRef.get(`${ref}^{}`) ?? shaByRef.get(ref); return shaByRef.get(`refs/tags/${ref}^{}`) ?? shaByRef.get(`refs/tags/${ref}`) ?? shaByRef.get(`refs/heads/${ref}`); } @@ -371,6 +383,9 @@ function resolveExplicitRef(ref: string, repo: string): string { // git parses options anywhere in its argv, so a ref spelled like one (`--upload-pack=…`) would be // obeyed as an option instead of resolved. No real ref starts with `-` — git rejects those itself. if (ref.startsWith('-')) throw new Error(`deploy by_ref: invalid ref=${ref} — a git ref cannot start with "-".`); + // Checked before local resolution, not just remote: a checkout that has fetched refs/pull//head + // resolves it happily, and the resulting SHA is just as unreachable for the cluster's clone. + assertCloneableRefNamespace(ref); try { return runGit(['rev-parse', '--verify', `${ref}^{commit}`]); } catch { diff --git a/unitTests/bin/cliOperations.test.js b/unitTests/bin/cliOperations.test.js index 6f8043a0f3..f5a3da664f 100644 --- a/unitTests/bin/cliOperations.test.js +++ b/unitTests/bin/cliOperations.test.js @@ -1276,6 +1276,10 @@ describe('deploy by reference (by_ref)', () => { tagObjectSha = git('rev-parse', 'v2.0.0'); // the tag object, not the commit it points at git('push', '-q', 'origin', 'HEAD:refs/heads/remote-only', 'v2.0.0'); git('tag', '-d', 'v2.0.0'); + // A pull ref that exists both on the remote and locally, so the namespace rejection is + // tested against a ref that would otherwise resolve on either path. + git('push', '-q', 'origin', 'HEAD:refs/pull/42/head'); + git('update-ref', 'refs/pull/42/head', remoteOnlySha); git('checkout', '-q', headSha); // Drop the remote-tracking refs push just created, so nothing resolves locally by accident // (and so the "commit isn't on a remote branch" check has nothing to find). @@ -1343,6 +1347,29 @@ describe('deploy by reference (by_ref)', () => { assert.throws(() => resolveGitTarget('--upload-pack=touch /tmp/pwned'), /cannot start with "-"/); }); + // A plain clone fetches refs/heads/* and refs/tags/* only. A commit named through any other + // namespace pins to an immutable SHA the cluster still can't check out — the same reachability + // failure as the pull_request merge commit, just reached by typing it explicitly. + describe('refs outside the cloneable namespaces', () => { + it('rejects refs/pull//head even though it resolves both locally and on the remote', () => { + // Guards the ordering: the namespace check runs before local resolution, so a checkout + // that has fetched the pull ref can't quietly pin an unreachable commit. + assert.throws( + () => resolveGitTarget('refs/pull/42/head'), + /outside refs\/heads\/ and refs\/tags\/[\s\S]*never check it out/ + ); + }); + + it('accepts a fully-qualified branch ref', () => { + assert.strictEqual(resolveGitTarget('refs/heads/remote-only').committish, remoteOnlySha); + }); + + it('accepts a fully-qualified tag ref, peeled to its commit', () => { + assert.strictEqual(resolveGitTarget('refs/tags/v2.0.0').committish, remoteOnlySha); + assert.notStrictEqual(resolveGitTarget('refs/tags/v2.0.0').committish, tagObjectSha); + }); + }); + // The cluster clones from the remote, so an unpushed commit fails server-side with an error far // from the CLI. This repo's remote-tracking refs were dropped above, so nothing looks pushed. it('warns when the commit to deploy is on no remote branch', () => {