diff --git a/cli/lib/ship/review/cache/root.mts b/cli/lib/ship/review/cache/root.mts index 2592feda..2ee1bace 100644 --- a/cli/lib/ship/review/cache/root.mts +++ b/cli/lib/ship/review/cache/root.mts @@ -7,6 +7,7 @@ import { homedir } from 'node:os'; import { basename, dirname, isAbsolute, join, relative, resolve, sep } from 'node:path'; import { runDirectReviewCli } from '../run-direct.mts'; import { reviewPathWithin } from '../runtime-paths.mts'; +import { errorMessage, fail, gitEnvironment } from '../shared/common.mts'; const REVIEW_CACHE_NAMESPACE = 'devkit-review-cache-v1'; const OBJECT_ID = /^(?:[a-f0-9]{40}|[a-f0-9]{64})$/; @@ -18,14 +19,6 @@ export interface ReviewCacheRootOptions { platform?: NodeJS.Platform; } -function fail(message: string): never { - throw new Error(`devkit review: ${message}`); -} - -function errorMessage(cause: unknown): string { - return cause instanceof Error ? cause.message : String(cause); -} - function assertPhysicalDirectory(path: string, label: string): string { const requested = resolve(path); try { @@ -66,22 +59,9 @@ function ensurePhysicalDirectory(path: string, label: string): string { return assertPhysicalDirectory(requested, label); } -function gitEnvironment(): NodeJS.ProcessEnv { - const env = { ...process.env }; - for (const name of Object.keys(env)) { - if (name.startsWith('GIT_')) delete env[name]; - } - return { - ...env, - GIT_NO_LAZY_FETCH: '1', - GIT_OPTIONAL_LOCKS: '0', - GIT_TERMINAL_PROMPT: '0', - }; -} - function gitOutput(targetRoot: string, args: string[], label: string): Buffer { const result = spawnSync('git', ['-c', 'core.hooksPath=/dev/null', '-C', targetRoot, ...args], { - env: gitEnvironment(), + env: gitEnvironment({ GIT_NO_LAZY_FETCH: '1', GIT_TERMINAL_PROMPT: '0' }), maxBuffer: 1024 * 1024, }); if (result.status !== 0) { diff --git a/cli/lib/ship/review/repository/manifest.mts b/cli/lib/ship/review/repository/manifest.mts index a0ba6f05..11474846 100644 --- a/cli/lib/ship/review/repository/manifest.mts +++ b/cli/lib/ship/review/repository/manifest.mts @@ -7,6 +7,7 @@ import { hasValidManifestRoots, isSafeManifestAbsolutePath, } from '../manifest/validation.mts'; +import { errorMessage, fail, objectValue } from '../shared/common.mts'; export const REVIEW_REPOSITORY_STATE_VERSION = 1 as const; export const REVIEW_REPOSITORY_OBJECT_ID = /^(?:[a-f0-9]{40}|[a-f0-9]{64})$/; @@ -40,21 +41,6 @@ export interface ReviewRepositoryStateManifest { selfHash: string; } -function fail(message: string): never { - throw new Error(`devkit review: ${message}`); -} - -function errorMessage(cause: unknown): string { - return cause instanceof Error ? cause.message : String(cause); -} - -function objectValue(value: unknown, label: string): Record { - if (!value || typeof value !== 'object' || Array.isArray(value)) { - fail(`repository state manifest ${label} is invalid.`); - } - return value as Record; -} - function canonicalBase64(value: unknown): value is string { if (typeof value !== 'string' || !value) return false; const decoded = Buffer.from(value, 'base64'); @@ -93,8 +79,11 @@ export function reviewRepositoryManifestHash(value: unknown): string { /** Read, authenticate, and deeply validate a repository-state manifest. */ export function parseReviewRepositoryStateManifest(path: string): ReviewRepositoryStateManifest { - const manifest = objectValue(readManifestValue(path), 'shape'); - const state = objectValue(manifest.state, 'state'); + const manifest = objectValue( + readManifestValue(path), + 'repository state manifest shape is invalid.', + ); + const state = objectValue(manifest.state, 'repository state manifest state is invalid.'); if (!hasValidManifestRoots(manifest, MANIFEST_KEYS, REVIEW_REPOSITORY_STATE_VERSION)) { fail('repository state manifest has an invalid shape.'); } diff --git a/cli/lib/ship/review/repository/state.mts b/cli/lib/ship/review/repository/state.mts index 43eb555d..18bf0e1b 100644 --- a/cli/lib/ship/review/repository/state.mts +++ b/cli/lib/ship/review/repository/state.mts @@ -17,6 +17,7 @@ import { canonicalReviewLeaf, reviewPathWithin, } from '../runtime-paths.mts'; +import { errorMessage, fail, gitEnvironment } from '../shared/common.mts'; import { parseReviewRepositoryStateManifest, REVIEW_REPOSITORY_OBJECT_ID, @@ -44,24 +45,6 @@ interface ConfigFileState { parts: Buffer[]; } -function fail(message: string): never { - throw new Error(`devkit review: ${message}`); -} - -function errorMessage(cause: unknown): string { - return cause instanceof Error ? cause.message : String(cause); -} - -function gitEnvironment(): NodeJS.ProcessEnv { - const env = { ...process.env }; - for (const name of Object.keys(env)) { - if (name.startsWith('GIT_')) delete env[name]; - } - // These commands are read-only; forbid opportunistic lock-taking such as index refreshes too. - env.GIT_OPTIONAL_LOCKS = '0'; - return env; -} - function spawnGit(root: string, args: string[]): SpawnSyncReturns { return spawnSync('git', ['-c', 'core.hooksPath=/dev/null', '-C', root, ...args], { env: gitEnvironment(), diff --git a/cli/lib/ship/review/setup-manifest-parse.mts b/cli/lib/ship/review/setup-manifest-parse.mts index 9c0c0cdb..63dd5fa1 100644 --- a/cli/lib/ship/review/setup-manifest-parse.mts +++ b/cli/lib/ship/review/setup-manifest-parse.mts @@ -15,16 +15,7 @@ import { REVIEW_SETUP_VERSION, reviewSetupHash, } from './setup-manifest-format.mts'; - -function fail(message: string): never { - throw new Error(`devkit review: ${message}`); -} - -function objectValue(value: unknown, label: string): Record { - if (!value || typeof value !== 'object' || Array.isArray(value)) - return fail(`${label} must be a JSON object.`); - return value as Record; -} +import { fail, objectValue } from './shared/common.mts'; function manifestString(value: unknown, label: string): string { if (typeof value !== 'string' || !value || value.includes('\0')) @@ -41,7 +32,7 @@ function manifestRelativePath(value: unknown, label: string): string { } function parseProfile(value: unknown): ReviewProfile { - const profile = objectValue(value, 'review setup manifest profile'); + const profile = objectValue(value, 'review setup manifest profile must be a JSON object.'); if ( !exactKeys(profile, ['enabled', 'guards', 'decisionsDir']) || typeof profile.enabled !== 'boolean' || @@ -65,7 +56,7 @@ function parseProfile(value: unknown): ReviewProfile { function parseChain(value: unknown): ReviewSetupState['chain'] { if (value === null) return null; - const chain = objectValue(value, 'review setup manifest chain'); + const chain = objectValue(value, 'review setup manifest chain must be a JSON object.'); if (!exactKeys(chain, ['path', 'sourcePath'])) return fail('review setup manifest chain has an invalid shape.'); const path = manifestRelativePath(chain.path, 'chain path'); @@ -76,7 +67,7 @@ function parseChain(value: unknown): ReviewSetupState['chain'] { } function parsePath(value: unknown, index: number): ReviewSetupPath { - const path = objectValue(value, `review setup manifest path ${index}`); + const path = objectValue(value, `review setup manifest path ${index} must be a JSON object.`); if ( !exactKeys(path, ['id', 'root', 'relativePath', 'fingerprint', 'required', 'executable']) || (path.root !== 'target' && path.root !== 'git') || @@ -99,7 +90,7 @@ function parsePath(value: unknown, index: number): ReviewSetupPath { } function parseSetup(value: unknown): ReviewSetupState { - const setup = objectValue(value, 'review setup manifest setup'); + const setup = objectValue(value, 'review setup manifest setup must be a JSON object.'); if ( !exactKeys(setup, ['overlay', 'hooksPath', 'profile', 'chain', 'paths']) || typeof setup.overlay !== 'boolean' || @@ -132,7 +123,7 @@ export function parseReviewSetupManifest(path: string): ReviewSetupManifest { const message = cause instanceof Error ? cause.message : String(cause); return fail(`could not read review setup manifest (${message}).`); } - const manifest = objectValue(value, 'review setup manifest'); + const manifest = objectValue(value, 'review setup manifest must be a JSON object.'); if ( !hasValidManifestRoots( manifest, diff --git a/cli/lib/ship/review/setup-manifest.mts b/cli/lib/ship/review/setup-manifest.mts index cad14fda..a809d25b 100644 --- a/cli/lib/ship/review/setup-manifest.mts +++ b/cli/lib/ship/review/setup-manifest.mts @@ -27,6 +27,7 @@ import { parseReviewSetupProfile, type RawReviewConfig, } from './setup-profile.mts'; +import { errorMessage, fail } from './shared/common.mts'; import { type ReviewSourceResolution, resolveReviewSource } from './source-projection.mts'; const HUSKY_RUNNER_PATHS = [ @@ -90,14 +91,6 @@ export interface CaptureReviewSetupOptions { afterFirstCapture?: () => void; } -function fail(message: string): never { - throw new Error(`devkit review: ${message}`); -} - -function errorMessage(cause: unknown): string { - return cause instanceof Error ? cause.message : String(cause); -} - function manifestDestination(path: string, gitRoot: string): string { const destination = canonicalReviewLeaf(path, 'setup manifest parent'); if (reviewPathWithin(gitRoot, destination)) diff --git a/cli/lib/ship/review/setup-profile.mts b/cli/lib/ship/review/setup-profile.mts index 711ad412..063394fe 100644 --- a/cli/lib/ship/review/setup-profile.mts +++ b/cli/lib/ship/review/setup-profile.mts @@ -12,6 +12,7 @@ import { } from '../../components.mts'; import { reviewGuardIssues } from '../../install/review-profile.mts'; import { normalizeSafeReviewRelativePath } from './runtime-paths.mts'; +import { fail, objectValue } from './shared/common.mts'; export const REVIEW_SETUP_DOCTOR = "run 'devkit doctor --fix'."; @@ -29,17 +30,6 @@ export interface ParsedReviewSetupProfile { profile: ReviewProfile; } -function fail(message: string): never { - throw new Error(`devkit review: ${message}`); -} - -function objectValue(value: unknown, label: string): Record { - if (!value || typeof value !== 'object' || Array.isArray(value)) { - return fail(`${label} must be a JSON object — ${REVIEW_SETUP_DOCTOR}`); - } - return value as Record; -} - function booleanField(value: unknown, label: string): boolean | undefined { if (value !== undefined && typeof value !== 'boolean') fail(`${label} must be a boolean — ${REVIEW_SETUP_DOCTOR}`); @@ -72,7 +62,10 @@ function parseConfigJson(raw: Buffer): RawReviewConfig { const message = cause instanceof Error ? cause.message : String(cause); return fail(`could not parse .devkit/config.json (${message}) — ${REVIEW_SETUP_DOCTOR}`); } - return objectValue(parsed, '.devkit/config.json') as RawReviewConfig; + return objectValue( + parsed, + `.devkit/config.json must be a JSON object — ${REVIEW_SETUP_DOCTOR}`, + ) as RawReviewConfig; } function parseOverlayMode(config: RawReviewConfig): boolean { @@ -90,7 +83,10 @@ function parseInstalledSelection(config: RawReviewConfig): Selection { const components = config.components === undefined ? {} - : objectValue(config.components, '.devkit/config.json components'); + : objectValue( + config.components, + `.devkit/config.json components must be a JSON object — ${REVIEW_SETUP_DOCTOR}`, + ); const recordedGuards = components.guards === undefined ? undefined @@ -126,7 +122,12 @@ function parseRequestedGuards(settings: Record, installed: stri function parseReviewProfile(config: RawReviewConfig, installed: string[]): ReviewProfile { const settings = - config.review === undefined ? {} : objectValue(config.review, '.devkit/config.json review'); + config.review === undefined + ? {} + : objectValue( + config.review, + `.devkit/config.json review must be a JSON object — ${REVIEW_SETUP_DOCTOR}`, + ); const enabled = booleanField(settings.enabled, '.devkit/config.json review.enabled') ?? true; if (!enabled) fail("disabled by .devkit/config.json — run 'devkit init --review'."); const requested = parseRequestedGuards(settings, installed); diff --git a/cli/lib/ship/review/shared/common.mts b/cli/lib/ship/review/shared/common.mts new file mode 100644 index 00000000..880a1a63 --- /dev/null +++ b/cli/lib/ship/review/shared/common.mts @@ -0,0 +1,36 @@ +/** + * Shared helpers for the review runtime family (cli/lib/ship/review/**). Extracted when the + * clone gate surfaced the same fail/errorMessage/objectValue/gitEnvironment bodies copy-pasted + * across repository/, cache/, and the setup-manifest files (sc-1414). New review modules should + * import from here instead of re-declaring. + */ + +/** Uniform review-runtime failure: every thrown message carries the `devkit review:` prefix. */ +export function fail(message: string): never { + throw new Error(`devkit review: ${message}`); +} + +export function errorMessage(cause: unknown): string { + return cause instanceof Error ? cause.message : String(cause); +} + +/** Assert a parsed JSON value is a plain object; `message` is the caller's exact failure text. */ +export function objectValue(value: unknown, message: string): Record { + if (!value || typeof value !== 'object' || Array.isArray(value)) { + fail(message); + } + return value as Record; +} + +/** + * A GIT_*-stripped environment for spawning read-only git commands: inherited GIT_DIR / + * GIT_INDEX_FILE etc. from an enclosing hook must never leak into children. Always forbids + * opportunistic lock-taking (GIT_OPTIONAL_LOCKS=0); callers layer extra pins on top. + */ +export function gitEnvironment(extra: Record = {}): NodeJS.ProcessEnv { + const env = { ...process.env }; + for (const name of Object.keys(env)) { + if (name.startsWith('GIT_')) delete env[name]; + } + return { ...env, GIT_OPTIONAL_LOCKS: '0', ...extra }; +} diff --git a/cli/lib/ship/review/source-projection.mts b/cli/lib/ship/review/source-projection.mts index 20df6d4e..6d7feb3e 100644 --- a/cli/lib/ship/review/source-projection.mts +++ b/cli/lib/ship/review/source-projection.mts @@ -3,6 +3,7 @@ import { lstatSync, readlinkSync, realpathSync, type Stats } from 'node:fs'; import { join, resolve } from 'node:path'; import { canonicalReviewDirectory, isSafeReviewRelativePath } from './runtime-paths.mts'; +import { reviewSetupStat } from './setup/setup-runtime-copy.mts'; export interface ReviewSourceProjection { /** Lexical path of the one projected link, relative to the captured source root. */ @@ -23,16 +24,6 @@ function fail(message: string): never { throw new Error(`devkit review: ${message}`); } -function safeStat(path: string) { - try { - return lstatSync(path, { throwIfNoEntry: false }); - } catch (cause) { - const code = (cause as NodeJS.ErrnoException).code; - if (code === 'ENOENT' || code === 'ENOTDIR') return undefined; - throw cause; - } -} - interface SourceTraversal { lexical: string; physical: string; @@ -89,7 +80,7 @@ function traverseSegment( lexical: join(traversal.lexical, segment), physical: join(traversal.physical, segment), }; - const stat = safeStat(next.lexical); + const stat = reviewSetupStat(next.lexical); if (stat === undefined) return { traversal: next, exists: false }; if (stat.isSymbolicLink()) { return {