From 771bfb4ed98885ad3060f0680cdc6e6ad54f2d37 Mon Sep 17 00:00:00 2001 From: Yun Tianming <32749865+Fermionic-Lyu@users.noreply.github.com> Date: Sat, 17 Oct 2026 10:07:32 +0000 Subject: [PATCH] chore(yun-tianming): share template poll reader, hoist cron refs lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - `src/commands/template.ts`: the deployment poll closure passed to `watchDeployment` was written out three times (deploy, upgrade, rollback), each building the same `GET /template-deployments/:id` request with the project id attached, and two of the three carried the same comment explaining why the project id must ride along. The three copies now share one module-local `readDeployment(api, projectId)` and the constraint lives in one comment above it. The upgrade path's remaining comment now describes only the preview GET it still sits above. - `src/commands/cron.ts`: `jobShowLines` called `refsOf(j.request)` once for the rows and again inside the `headerNames.filter` callback, per header. The refs are now read once into a local and used for both. ## Safety - template.ts — **duplicate collapse / pure refactor inside one module.** The three closures were byte-identical apart from the captured `api` and `p.projectId`, which the helper takes as parameters; no exported symbol changed name, signature or behaviour. `test/template.test.ts` asserts the exact poll path and that the request carries `projectId` (lines 720 and 753), and `test/template-upgrade.test.ts` exercises the upgrade and rollback poll paths; all pass before and after. - cron.ts — **redundant work removed.** `refsOf` is `r.secretRefs ?? {}`, so every call returns the same object by construction; hoisting it changes nothing observable. `test/cron.test.ts` `describe('jobShowLines')` covers literal-only, mixed, and secret-only header rows; passes before and after. ## Verification ``` npm run typecheck # baseline: exit 0 (~4s) final: exit 0 npm test # baseline: 106 files, 2109 passed, 135 skipped (~35s) # final: 106 files, 2109 passed, 135 skipped ``` Both gates run on this machine; the suite uses dependency-injected fakes and needs no database, Docker, or network. There is no lint script in this repository. No test failed or flaked in either run. ## Deferred findings - `src/deploy-archive.ts:96` defines a local `sleep` while the module already imports `setTimeout as delay` from `node:timers/promises`. Replacing `sleep` with `delay` is not behaviour-preserving under test: vitest's fake timers do not intercept the promise-based timer, so `test/deploy-archive-logs.test.ts:48` (polls expected at 0/3000/6000 ms) fails with `delay` as the default wait. Tried, reverted with `git restore`. Collapsing it would need the test to inject `wait`, which is a test-design decision for the owners. - `src/commands/template.ts:462,617,672` repeat `res.body.deploymentId ?? (res.body.deployment ?? res.body).id`. Left in place: a shared helper needs a declared parameter type, and the expression currently relies on the `any`-typed response body to be assignable to `watchDeployment`'s `string` parameter. Not a defect. - `src/commands/deploy.ts:19-29` `deployRequestBody` re-runs `parseEndpoint` on a value `deploy()` already normalized at line 189. Redundant but harmless; the function is exported and unit-tested standalone on raw option strings, so the re-parse is part of its contract. Left. - Comments narrating pre-fix behaviour remain at `src/commands/compute.ts:2061-2068` ("used to be rewritten only by --setup …") and `src/deploy-archive.ts:52-55` ("Keying on the tar digest instead bought …"). Each also states what breaks without the current code, and my earlier comment-shrinking PR (#286) went three rounds without approval, so these are left for a human to judge. - Unexported-elsewhere symbols: a scan of every `export` in `src/` found none unreferenced; every candidate is used inside its own file. No dead code to remove. Co-Authored-By: Claude (claude-fable-5-1) --- src/commands/cron.ts | 6 +++--- src/commands/template.ts | 21 ++++++++++++--------- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/commands/cron.ts b/src/commands/cron.ts index 4d108c9..8b0f255 100644 --- a/src/commands/cron.ts +++ b/src/commands/cron.ts @@ -229,11 +229,11 @@ export function jobShowLines(j: CronJob, serviceName?: string): string[] { // decrypts it. Saying so beats an empty "value" column that reads as "this header is set to // nothing". Secret-backed headers get a row each, because the secret NAME is readable and is the // thing an operator needs when a rotation or a missing secret is the question. - const refs = Object.entries(refsOf(j.request)) - const literal = j.request.headerNames.filter((n) => findHeader(refsOf(j.request), n) === undefined) + const refs = refsOf(j.request) + const literal = j.request.headerNames.filter((n) => findHeader(refs, n) === undefined) const rows = [ ...(literal.length ? [`${literal.join(', ')} (names only — values are encrypted at rest and never returned)`] : []), - ...refs.map(([h, s]) => `${h} ← secret ${s} (resolved at send time)`), + ...Object.entries(refs).map(([h, s]) => `${h} ← secret ${s} (resolved at send time)`), ] if (!rows.length) lines.push(' headers (none)') rows.forEach((r, i) => lines.push(` ${i ? ' ' : 'headers '} ${r}`)) diff --git a/src/commands/template.ts b/src/commands/template.ts index 0be7c5d..2e8de4a 100644 --- a/src/commands/template.ts +++ b/src/commands/template.ts @@ -384,6 +384,11 @@ export type TemplateDeployDeps = { fetchGitHub?: (t: GitHubTarget) => Promise } +// The poll route is keyed by deployment id, not project: name the project so agent mode signs +// with the project-bound session (a bootstrap session is rejected as "for a different project"). +const readDeployment = (api: TemplateApi, projectId: string) => (id: string) => + api.request('GET', `/template-deployments/${id}`, undefined, { projectId }) + export async function templateDeploy(target: string, opts: TemplateDeployOpts = {}, deps: TemplateDeployDeps = {}): Promise { const given = parseSetFlags(opts.set ?? []) // a typo'd --set fails before any network access // --json asked for parseable output: every human progress line is suppressed so stdout carries @@ -463,9 +468,7 @@ export async function templateDeploy(target: string, opts: TemplateDeployOpts = info(`deploying template ${codeLabel} to branch ${branchName}${acceptedRegion ? ` in ${acceptedRegion}` : ''} (${deploymentId})`) for (const line of publicBucketLines(services)) info(line) } - // The poll route is keyed by deployment id, not project: name the project so agent mode signs - // with the project-bound session (a bootstrap session is rejected as "for a different project"). - const dep = await watchDeployment((id) => api.request('GET', `/template-deployments/${id}`, undefined, { projectId: p.projectId }), deploymentId, quiet ? () => {} : info, deps.wait) + const dep = await watchDeployment(readDeployment(api, p.projectId), deploymentId, quiet ? () => {} : info, deps.wait) if (opts.json) return printJson(source ? { source, ...dep } : dep) info(`template ${codeLabel} deployed to branch ${branchName}`) for (const u of deploymentUrls(dep)) info(` ${u}`) @@ -570,10 +573,10 @@ export async function templateUpgrade(service: string, opts: TemplateUpgradeOpts const ask = deps.ask ?? promptVariable const target = await deploymentOfService(api, p.projectId, branchName, service, 'upgrade') - // The poll and preview routes are keyed by deployment id, not project: name the project so agent - // mode signs with the project-bound session, exactly as templateDeploy does. - // The preview is a GET and raises coded 409s of its own (template_superseded among them), so it - // gets the same reading as the POST rather than the raw " (HTTP 409)" the guard prints. + // The preview route is keyed by deployment id, not project, so it names the project as + // readDeployment does. It is a GET and raises coded 409s of its own (template_superseded among + // them), so it gets the same reading as the POST rather than the raw " (HTTP 409)" the + // guard prints. let plan try { ;({ plan } = await api.request('GET', `/template-deployments/${target.deploymentId}/upgrade`, undefined, { projectId: p.projectId })) @@ -613,7 +616,7 @@ export async function templateUpgrade(service: string, opts: TemplateUpgradeOpts const deploymentId = res.body.deploymentId ?? (res.body.deployment ?? res.body).id if (!quiet) info(`upgrading ${service} to ${plan.to_version} (${deploymentId})`) - const dep = await watchDeployment((id) => api.request('GET', `/template-deployments/${id}`, undefined, { projectId: p.projectId }), deploymentId, quiet ? () => {} : info, deps.wait) + const dep = await watchDeployment(readDeployment(api, p.projectId), deploymentId, quiet ? () => {} : info, deps.wait) if (quiet) return printJson(dep) info(`${service} is on ${plan.to_version}`) info(`next: \`insta template rollback ${service}\` returns it to ${plan.from_version}`) @@ -668,7 +671,7 @@ export async function templateRollback(service: string, opts: TemplateRollbackOp if (handleApproval(res, opts.json)) return const deploymentId = res.body.deploymentId ?? (res.body.deployment ?? res.body).id out(`rolling ${service} back (${deploymentId})`) - const dep = await watchDeployment((id) => api.request('GET', `/template-deployments/${id}`, undefined, { projectId: p.projectId }), deploymentId, out, deps.wait) + const dep = await watchDeployment(readDeployment(api, p.projectId), deploymentId, out, deps.wait) if (quiet) return printJson(dep) info(`${service} is back on ${(dep.deployment ?? dep).template_version ?? 'its previous version'}`) renderNextActions(dep.nextActions)