diff --git a/src/commands/cron.ts b/src/commands/cron.ts index 004f6e0..4d108c9 100644 --- a/src/commands/cron.ts +++ b/src/commands/cron.ts @@ -16,7 +16,7 @@ // forward because it can (see buildRequest). import { randomUUID } from 'node:crypto' import { ApiClient, ApiError, requireProject } from '../api.js' -import { die, handleApproval, info, printJson, refuse } from '../util.js' +import { die, handleApproval, info, isWebUrl, printJson, refuse } from '../util.js' import { q, resolveSoleService } from './services.js' // ---- wire shapes (the fields this module reads; the platform sends more) ---- @@ -321,7 +321,7 @@ export function assertTargetFlags(o: TargetOpts, partial = false): void { if (o.url && o.service) throw new Error('--url and --service name two different targets — pass one') if (o.path !== undefined && !o.service) throw new Error('--path applies to --service (an external target carries its path in the --url)') if (!partial && !o.url && !o.service) throw new Error('name a target: --url or --service [--path /api/cron]') - if (o.url !== undefined && !/^https?:\/\//i.test(o.url)) throw new Error(`--url must be an absolute http(s) URL, got: ${o.url}`) + if (o.url !== undefined && !isWebUrl(o.url)) throw new Error(`--url must be an absolute http(s) URL, got: ${o.url}`) if (o.path !== undefined && !o.path.startsWith('/')) throw new Error(`--path must start with /, got: ${o.path}`) } diff --git a/src/commands/postgres.ts b/src/commands/postgres.ts index 84cbe67..d6d42d4 100644 --- a/src/commands/postgres.ts +++ b/src/commands/postgres.ts @@ -205,12 +205,8 @@ export function dbStatsLines(group: string, body: any): string[] { } // Point-in-time stats snapshot for a postgres service: connections vs the server's ceiling, cache -// hit rate, database size. Read-only. insta-db-backed: a suspended instance answers from the -// provider's control plane (shown as "(suspended)" with structural zeros), never dialed. That is -// every environment today — the Neon-backed contrast below is historical: Neon is no longer used -// anywhere, and the code that handled it is retained, not live. Neon-backed: the platform read -// over a direct SQL connection, so a one-shot call could wake a suspended endpoint — acceptable -// for an explicit command, which is why nothing here polls. +// hit rate, database size. Read-only. A suspended instance answers from the provider's control +// plane (shown as "(suspended)" with structural zeros), never dialed. export async function dbStats(service: string | undefined, opts: Opts): Promise { const api = await ApiClient.load() const p = await requireProject() diff --git a/src/commands/secrets.ts b/src/commands/secrets.ts index 3812712..5cef2af 100644 --- a/src/commands/secrets.ts +++ b/src/commands/secrets.ts @@ -195,6 +195,29 @@ export function applyServiceLines(services: ApplyServiceOutcome[]): string[] { }) } +// POST one entry to /apply and settle what `secrets set` and `secrets unset` share: the approval +// gate, the exit code, and the --json document. Returns null when the caller has nothing left to +// print — the platform gated the write, or --json already carried the outcome. +async function applyEntry( + d: SecretsDeps, + name: string, + branch: string | undefined, + entry: { kind: 'set' | 'delete' }, + opts: { service?: string; json?: boolean }, +): Promise<{ verdict: ApplyVerdict; services: ApplyServiceOutcome[] } | null> { + const res = await d.api.rawRequest('POST', `/projects/${d.projectId}/apply`, { branch: branch ?? d.linkedBranch, entries: [entry] }) + if (handleApproval(res, opts.json)) return null + const entries: ApplyEntryOutcome[] = res.body.entries ?? [] + const services: ApplyServiceOutcome[] = res.body.services ?? [] + const verdict = applyVerdict(entries, services) + process.exitCode = applyExitCode(verdict) + if (opts.json) { + printJson({ ok: verdict === 'ok', verdict, name, branch: branch ?? null, service: opts.service ?? null, entries, services }) + return null + } + return { verdict, services } +} + // Set a user secret. Project-wide by default; --branch scopes it to one branch. --service binds // it to a branch service instead, which implies the current branch (binding requires one). Value // comes from the argument, or stdin when omitted (keeps secret values out of shell history). @@ -214,13 +237,9 @@ export async function secretsSet( const branch = opts.service ? (opts.branch ?? d.linkedBranch) : opts.branch // The batch always deploys on ONE branch, so the top level falls back to the linked one even when the entry itself is project-wide. const entry = { kind: 'set' as const, name, value: v, ...(branch ? { branch } : {}), ...(opts.service ? { service: opts.service } : {}) } - const res = await d.api.rawRequest('POST', `/projects/${d.projectId}/apply`, { branch: branch ?? d.linkedBranch, entries: [entry] }) - if (handleApproval(res, opts.json)) return - const entries: ApplyEntryOutcome[] = res.body.entries ?? [] - const services: ApplyServiceOutcome[] = res.body.services ?? [] - const verdict = applyVerdict(entries, services) - process.exitCode = applyExitCode(verdict) - if (opts.json) return printJson({ ok: verdict === 'ok', verdict, name, branch: branch ?? null, service: opts.service ?? null, entries, services }) + const outcome = await applyEntry(d, name, branch, entry, opts) + if (!outcome) return + const { verdict, services } = outcome info(`set ${name}${opts.service ? ` → ${opts.service}` : ''} (${branch ? `branch ${branch}` : 'project-wide'})`) for (const line of applyServiceLines(services)) info(line) // Verdict note on stderr, after the per-service lines: not success, and not the same failure either. @@ -241,15 +260,11 @@ export async function secretsUnset( // pair without one) — so --service defaults to the linked branch, exactly as `secrets set` does. const branch = opts.service ? (opts.branch ?? d.linkedBranch) : opts.branch const entry = { kind: 'delete' as const, name, ...(branch ? { branch } : {}), ...(opts.service ? { service: opts.service } : {}) } - const res = await d.api.rawRequest('POST', `/projects/${d.projectId}/apply`, { branch: branch ?? d.linkedBranch, entries: [entry] }) - if (handleApproval(res, opts.json)) return - const entries: ApplyEntryOutcome[] = res.body.entries ?? [] - const services: ApplyServiceOutcome[] = res.body.services ?? [] - const verdict = applyVerdict(entries, services) - process.exitCode = applyExitCode(verdict) // The EFFECTIVE branch, not the flag: with --service and no --branch the scope that was deleted // is the linked branch's, and the output has to say which scope it actually touched. - if (opts.json) return printJson({ ok: verdict === 'ok', verdict, name, branch: branch ?? null, service: opts.service ?? null, entries, services }) + const outcome = await applyEntry(d, name, branch, entry, opts) + if (!outcome) return + const { verdict, services } = outcome const scope = opts.service ? `${opts.service}, branch ${branch}` : branch ? `branch ${branch}` : 'project-wide' info(`unset ${name} (${scope})`) for (const line of applyServiceLines(services)) info(line) diff --git a/src/commands/upgrade.ts b/src/commands/upgrade.ts index de94708..38d72ad 100644 --- a/src/commands/upgrade.ts +++ b/src/commands/upgrade.ts @@ -384,8 +384,6 @@ export async function autoupdate(mode?: string): Promise { info(`autoupdate: ${enabled ? 'on' : 'off'} (default on while the CLI is pre-1.0 — \`insta config autoupdate off\` to disable)`) } -// Called once at CLI start-up. Never blocks: reads the cache synchronously, prints at most one -// stderr line, and (when due) spawns detached children for the registry check / quiet upgrade. /** Commands that must never nudge about an update or spawn a background one. * * `upgrade`/`autoupdate` are the update machinery itself. Every `__` command @@ -406,6 +404,8 @@ export function skipsUpdateCheck(cmd: string | undefined, sub?: string | undefin return cmd === 'upgrade' || cmd === 'autoupdate' || !!cmd?.startsWith('__') } +// Called once at CLI start-up. Never blocks: reads the cache synchronously, prints at most one +// stderr line, and (when due) spawns detached children for the registry check / quiet upgrade. export function maybeUpdate(current: string, argv: string[]): void { if (skipsUpdateCheck(argv[2], argv[3])) return const channel = detectChannel() diff --git a/src/template-manifest.ts b/src/template-manifest.ts index 4fa0074..3c388f8 100644 --- a/src/template-manifest.ts +++ b/src/template-manifest.ts @@ -174,7 +174,6 @@ export function validateManifest(m: TemplateManifest): string[] { if (authored.spec !== undefined) { problems.push(`${where}: compute size is the platform's to choose — remove spec`) } - // Validate runtime fields: command and mountPath if (svc.command !== undefined && (typeof svc.command !== 'string' || !svc.command.trim())) { problems.push(`${where}.command must be a non-empty string`) } diff --git a/test/compute-domain-flow.test.ts b/test/compute-domain-flow.test.ts index 5e79dbf..c519c33 100644 --- a/test/compute-domain-flow.test.ts +++ b/test/compute-domain-flow.test.ts @@ -1,6 +1,6 @@ -// The COMMAND-level flow of the three domain verbs, through the injected API seam (r2d2 round 1 -// Suggestion — the pure renderers are covered in compute-domain-region.test.ts, this covers what -// the commands actually send and print): the preflight service lookup, the explicit `group` on +// The COMMAND-level flow of the three domain verbs, through the injected API seam (the pure +// renderers are covered in compute-domain-region.test.ts; this covers what the commands actually +// send and print): the preflight service lookup, the explicit `group` on // every call so the platform's `default` fallback is never relied on, --json passing the platform // body through untouched, and 409 → the release-then-rebind message. import { describe, it, expect, vi, afterEach, afterAll } from 'vitest' diff --git a/test/compute-domain-region.test.ts b/test/compute-domain-region.test.ts index b1f7b8f..dd4d27b 100644 --- a/test/compute-domain-region.test.ts +++ b/test/compute-domain-region.test.ts @@ -85,7 +85,7 @@ describe('domainGuidanceLines (after attach: what to do next, from the platform // The printed follow-up must reach the same service on the same branch: without --group it dies // on the very ambiguity error this feature raises, and without --branch it checks the linked - // branch instead (cubic P2). + // branch instead. it('the follow-up check command carries the resolved group and the invoked branch', () => { expect(domainGuidanceLines(bound, { group: 'api' }).at(-1)).toBe('then: insta domain check app.customer.com --group api') expect(domainGuidanceLines(bound, { group: 'api', branch: 'preview' }).at(-1)) @@ -179,7 +179,7 @@ describe('domainStatusLines (check: every stage + where it routes)', () => { // A PLANE answer always carries `ssl`, so a plane answer with no `origin` is a daemon too old to // say where the hostname lands: routing is UNCONFIRMED and must not be reported as serving — - // "the plane said active" is not evidence that traffic reaches this region (r2d2 round 1 Critical). + // "the plane said active" is not evidence that traffic reaches this region. it('failure shape 3 — a plane daemon reports no routing target: UNCONFIRMED, never serving', () => { const { origin: _o, edgeOrigin: _e, originOk: _k, ...noReport } = active const lines = domainStatusLines(noReport as DomainView) @@ -215,7 +215,7 @@ describe('domainStatusLines (check: every stage + where it routes)', () => { }) // An error STATE with no reason attached is still an error state — a row that says `error` has - // not been observed serving, and printing a URL beside it is the blackhole lie (cubic P2). + // not been observed serving, and printing a URL beside it is the blackhole lie. it('error state with NO reason: still blocked, and the missing reason is said out loud', () => { const lines = domainStatusLines({ ...active, status: 'error' }) expect(lines).toContain(' error error (the plane reported an error state with no reason)') @@ -223,7 +223,7 @@ describe('domainStatusLines (check: every stage + where it routes)', () => { }) // configured: true alongside an outstanding record blocker means the verdict and the record set - // disagree — report the disagreement rather than papering over it with a URL (cubic P1). + // disagree — report the disagreement rather than papering over it with a URL. it('configured but a record is still missing: the disagreement wins, not the verdict', () => { const lines = domainStatusLines({ ...active, dns: active.dns.map((d) => (d.type === 'TXT' ? { ...d, status: 'missing' } : d)) }) expect(lines.at(-1)).toBe(' serving not yet (add the ownership TXT)') @@ -231,7 +231,7 @@ describe('domainStatusLines (check: every stage + where it routes)', () => { }) // An unchecked TXT (no per-record status on a not-yet-configured answer) blocks too: "we have not - // looked" is not "it is there" (cubic P3 — this branch was previously untested). + // looked" is not "it is there". it('ownership unchecked: rendered as unchecked AND counted as a blocker', () => { const noStatus = { ...bound, dns: bound.dns.map(({ status: _s, ...d }) => d) } const lines = domainStatusLines(noStatus) @@ -240,7 +240,7 @@ describe('domainStatusLines (check: every stage + where it routes)', () => { }) // No records at all: the stages are still drawn. An omitted stage reads as "not required", when - // the truth is the platform told us nothing to publish (cubic P2). + // the truth is the platform told us nothing to publish. it('no records from the platform: ownership and cname stages are still drawn, as unknown blockers', () => { const lines = domainStatusLines({ ...bound, dns: [] }) expect(lines[1]).toBe(' ownership unknown the platform returned no ownership TXT for this domain — nothing to publish yet; ask an operator') @@ -248,10 +248,10 @@ describe('domainStatusLines (check: every stage + where it routes)', () => { expect(lines.at(-1)).toContain('no ownership TXT from the platform') }) - // The PARTIAL case (r2d2 round 2 Critical): records came back, but not the routing one. Keying - // the "no routing record" stage on an entirely empty set let a payload carrying only the - // ownership TXT skip the stage and add no blocker — so a configured answer with a live cert and - // a confirmed origin printed a URL for a hostname with nothing pointing at us. + // The PARTIAL case: records came back, but not the routing one. Keying the "no routing record" + // stage on an entirely empty set would let a payload carrying only the ownership TXT skip the + // stage and add no blocker — a configured answer with a live cert and a confirmed origin would + // print a URL for a hostname with nothing pointing at us. it('records present but NO routing record: the stage is still drawn, blocks, and never says serving', () => { const txtOnly: DomainView = { ...bound, configured: true, status: 'active', ssl: 'active', @@ -279,9 +279,9 @@ describe('domainStatusLines (check: every stage + where it routes)', () => { expect(lines.join('\n')).not.toContain('add CNAME customer.com') }) - // r2d2 round 3 Critical: the blocker rule reached only the ownership TXT and the FIRST routing - // record. Every other record rendered from `configured` alone and blocked nothing, so a payload - // whose SECOND apex record was bad still ended in `serving https://…`. + // The blocker rule must reach every routing record, not just the ownership TXT and the FIRST + // one: a record rendered from `configured` alone blocks nothing, so a payload whose SECOND apex + // record is bad would still end in `serving https://…`. it('apex A/AAAA pair with the AAAA missing: both routing records are judged, and it does not serve', () => { const apex: DomainView = { // Fly-shaped (an apex is Fly's A/AAAA path): no ownership TXT, no plane ssl/origin fields. diff --git a/test/limits.test.ts b/test/limits.test.ts index 5e4c620..4d82396 100644 --- a/test/limits.test.ts +++ b/test/limits.test.ts @@ -36,8 +36,8 @@ describe('parseMemoryMb', () => { }) }) -// Review round 2: both jwfing and cubic independently flagged the bare Number() on --cpu (NaN -// serializes to null on the wire) and the unvalidated db strings. These pin the new seams. +// A bare Number() on --cpu lets NaN serialize to null on the wire, and the db strings need the +// same local validation. These pin the parsing seams. import { parseCpu, fmtMb } from '../src/commands/compute.js' import { fetchDbInstance } from '../src/commands/postgres.js' import { ApiError } from '../src/api.js' @@ -71,7 +71,7 @@ describe('parseDbCpu / parseDbMemory (provider quantity strings)', () => { describe('fmtMib (display must not claim a ceiling the API did not set)', () => { it('collapses whole and half GiB', () => { expect(fmtMib(2048)).toBe('2 GiB') - expect(fmtMib(1536)).toBe('1.5 GiB') // the review example: was shown as "2 GiB" + expect(fmtMib(1536)).toBe('1.5 GiB') // never rounded up to "2 GiB" }) it('keeps everything else exact in MiB', () => { expect(fmtMib(1300)).toBe('1300 MiB') @@ -79,12 +79,10 @@ describe('fmtMib (display must not claim a ceiling the API did not set)', () => }) }) -// The round-3 Critical: rawRequest THROWS on >=400, so the no-instance soft-path must live in a -// catch — status-branching on its return value was unreachable dead code and the read crashed with -// a raw ApiError. These drive the seam with a stub client, which is exactly the test that would -// have caught it (the 502 branch was never taken by any test). That soft path is the legacy Neon -// path: Neon is no longer used by any environment, so these stay as retained coverage for code -// that is not live. +// rawRequest THROWS on >=400 (api.ts), so the no-instance soft path must live in a catch, not in +// status branching on its return value. These drive the seam with a stub client so the 502 branch +// is actually taken. That soft path is the legacy Neon path: Neon is no longer used by any +// environment, so these stay as retained coverage for code that is not live. describe('fetchDbInstance (the read seam)', () => { const stub = (fn: () => Promise) => ({ rawRequest: fn }) as any @@ -122,8 +120,7 @@ describe('fmtMb (compute display)', () => { }) }) -// Case-exactness (round-3 suggestion): k8s quantities are case-sensitive, so local validation -// must reject what the server would. +// k8s quantities are case-sensitive, so local validation must reject what the server would. describe('parseDbMemory case-exactness', () => { it('rejects lowercase unit variants the backend refuses', () => { for (const raw of ['4gi', '4GI', '8m', '2048mi']) { diff --git a/test/observe-install.test.ts b/test/observe-install.test.ts index e58986d..d20237f 100644 --- a/test/observe-install.test.ts +++ b/test/observe-install.test.ts @@ -1,7 +1,7 @@ // The Claude Code hooks schema executes `command` as ONE shell string ($CLAUDE_PROJECT_DIR is an -// env var the shell expands). The installer used to emit command:'node' + an args array with a -// ${CLAUDE_PROJECT_DIR} template — nothing expands it, so node threw MODULE_NOT_FOUND after EVERY -// tool call in every linked project. Found live (user report, 2026-07-12). +// env var the shell expands). A command:'node' + args array carrying a ${CLAUDE_PROJECT_DIR} +// template is expanded by nothing, so node would throw MODULE_NOT_FOUND after EVERY tool call in +// every linked project. import { test, expect } from 'vitest' import { existsSync, mkdtempSync, readFileSync, mkdirSync, writeFileSync, realpathSync } from 'node:fs' import { spawnSync } from 'node:child_process' @@ -19,9 +19,8 @@ const posixTest = process.platform === 'win32' ? test.skip : test // The install materializes ./.insta/observe (this CLI version's hook, regenerated on every link) // and the hook appends ./.insta/audit.jsonl (this machine's findings: partial fingerprints + -// redacted context). Neither is project source; both used to be left for the user to discover in -// `git status` (user report, 2026-09-04). ./.insta/project.json is the team binding and must NOT -// be caught by these entries. +// redacted context). Neither is project source, so both are gitignored. ./.insta/project.json is +// the team binding and must NOT be caught by these entries. test('install gitignores the machine-local .insta state but not project.json, idempotently', () => { const cwd = mkdtempSync(join(tmpdir(), 'obs-proj-')) writeFileSync(join(cwd, '.gitignore'), 'node_modules\n') @@ -35,15 +34,14 @@ test('install gitignores the machine-local .insta state but not project.json, id expect(installObserve({ cwd, assetDir: fakeAssets() }).ignored).toEqual([]) // re-link adds nothing }) -// Codex has no $CLAUDE_PROJECT_DIR, so the installer used to bake the absolute project path into -// .codex/hooks.json — a file teams commit — which shipped /Users//… to every clone and -// failed there after every tool call. The replacement must ALSO survive two things a first cut -// got wrong (review of #178): a monorepo where the insta project root is below the git root -// (`git rev-parse --show-toplevel` finds the wrong dir → hook silently dead), and Windows, where -// Codex hands `command` verbatim to cmd.exe unless a `commandWindows` override exists (POSIX -// `[ ! -f … ]` → parse error after every tool call). So: one shell-neutral `node -e` that climbs -// from the session cwd. These tests run the command through the platform shell (`shell: true` → -// sh on POSIX, cmd.exe on Windows), so the Windows CI job exercises the real thing. +// Codex has no $CLAUDE_PROJECT_DIR, and .codex/hooks.json is a file teams commit, so the command +// must not bake in an absolute project path. It must also survive a monorepo where the insta +// project root is below the git root (`git rev-parse --show-toplevel` finds the wrong dir → hook +// silently dead), and Windows, where Codex hands `command` verbatim to cmd.exe unless a +// `commandWindows` override exists (POSIX `[ ! -f … ]` → parse error after every tool call). So: +// one shell-neutral `node -e` that climbs from the session cwd. These tests run the command +// through the platform shell (`shell: true` → sh on POSIX, cmd.exe on Windows), so the Windows CI +// job exercises the real thing. const codexCommand = (cwd: string): string => JSON.parse(readFileSync(join(cwd, '.codex', 'hooks.json'), 'utf8')).hooks.PostToolUse.at(-1).hooks[0].command const runHook = (cmd: string, cwd: string, input = '{}') => spawnSync(cmd, { cwd, shell: true, input }) @@ -83,10 +81,10 @@ test('codex hook climbs to the insta root from a nested monorepo project and pas }) // Finding the right hook.js is only half of it: the hook must also WRITE to that project root. -// It used to record into `CLAUDE_PROJECT_DIR || event.cwd`, and Codex's event.cwd is the session -// cwd — so a session started in apps/api/src/routes ran apps/api/.insta/observe/hook.js but left -// an unignored apps/api/src/routes/.insta/audit.jsonl behind (review of #178, round 2). Now the -// hook derives the root from its own entry path (/.insta/observe/hook.js). +// Codex's event.cwd is the session cwd, so a root taken from it for a session started in +// apps/api/src/routes would leave an unignored apps/api/src/routes/.insta/audit.jsonl behind. The +// hook derives the root from its own entry path (/.insta/observe/hook.js); the env and the +// event cwd are only fallbacks. test('projectRootFor: the materialized entry path wins; env / event cwd are only fallbacks', () => { const root = join(tmpdir(), 'proj') expect(projectRootFor(join(root, '.insta', 'observe', 'hook.js'), { CLAUDE_PROJECT_DIR: '/elsewhere' }, '/session')) @@ -126,7 +124,7 @@ test('codex command from a nested session cwd records findings at the linked pro // hook writes to, from any subdirectory. The hook writes at the NEAREST materialized hook above // the session cwd, so that is the anchor; the link file is only where the next install will // land (fallback), then cwd. The fourth case — link file and hook at different depths — is the -// one where precedence is observable and was wrong (post-merge review of #178). +// one where precedence is observable. test('auditRoot: nearest materialized hook, else link file, else cwd — resolved from a subdirectory', async () => { const linked = realpathSync(mkdtempSync(join(tmpdir(), 'obs-linked-'))) mkdirSync(join(linked, '.insta'), { recursive: true }) @@ -154,7 +152,7 @@ test('auditRoot: nearest materialized hook, else link file, else cwd — resolve expect(await auditRoot(mono)).toBe(mono) // above the hook, the link file still anchors }) -// And the installers no longer create that split in the first place: inside a linked project, +// The installers do not create that split in the first place: inside a linked project, // install anchors at the link root (like writeProject), so a re-link or `observe install` from a // subdirectory refreshes the project's hook instead of minting a second one. test('installRoot: the linked project root from a subdirectory, else cwd', async () => {