diff --git a/gitnexus-web/src/services/backend-client.ts b/gitnexus-web/src/services/backend-client.ts index 8378ef494..50b9f2683 100644 --- a/gitnexus-web/src/services/backend-client.ts +++ b/gitnexus-web/src/services/backend-client.ts @@ -1010,6 +1010,13 @@ export const startAnalyze = async (request: { force?: boolean; embeddings?: boolean; token?: string; + /** + * Index-branch selector. Omitted: a `url` with no existing clone takes the + * remote's default branch, an existing clone updates whichever branch it + * already has checked out, and a `path` request is not cloned at all and + * indexes that working tree as it stands. + */ + branch?: string; }): Promise<{ jobId: string; status: string }> => { const response = await fetchWithTimeout( `${_backendUrl}/api/analyze`, diff --git a/gitnexus/src/cli/analyze-config.ts b/gitnexus/src/cli/analyze-config.ts index 3d040fac1..49896538e 100644 --- a/gitnexus/src/cli/analyze-config.ts +++ b/gitnexus/src/cli/analyze-config.ts @@ -31,6 +31,10 @@ import fs from 'node:fs'; import path from 'node:path'; import { readRepoControlFile } from '../config/repo-control-file.js'; +import { + InvalidBranchError, + validateBranchName as validateBranchNameCore, +} from '../core/git-ref.js'; import type { AnalyzeOptions } from './analyze-options.js'; export const GITNEXUS_RC_FILENAME = '.gitnexusrc'; @@ -38,9 +42,6 @@ export const GITNEXUS_RC_FILENAME = '.gitnexusrc'; /** Final fallback when no branch is configured or detectable. */ export const DEFAULT_BRANCH_FALLBACK = 'main'; -/** Git refs longer than this are almost certainly a mistake / injection attempt. */ -const BRANCH_MAX_LENGTH = 255; - /** * Thrown for any `.gitnexusrc` problem (missing-file is NOT an error — it * returns `undefined`). The message is user-facing and names the file so the @@ -157,45 +158,18 @@ const assertNoHiddenChars = (value: string, source: string): void => { /** * Validate a user-supplied branch name (from CLI or `.gitnexusrc`). Returns the - * trimmed name or throws {@link GitNexusRcError}. Conservative but accepts the - * shapes real branches use (`feature/foo-bar`, `release/1.2`, `develop`). + * trimmed name or throws {@link GitNexusRcError}. Rules live in + * `core/git-ref.ts`; this wrapper keeps the CLI / `.gitnexusrc` error type. */ export function validateBranchName(value: string, source: string): string { - const trimmed = value.trim(); - if (!trimmed) { - throw new GitNexusRcError(`${source}: branch name must not be empty.`); + try { + return validateBranchNameCore(value, source); + } catch (err) { + if (err instanceof InvalidBranchError) { + throw new GitNexusRcError(err.message); + } + throw err; } - if (trimmed.length > BRANCH_MAX_LENGTH) { - throw new GitNexusRcError(`${source}: branch name is too long (max ${BRANCH_MAX_LENGTH}).`); - } - assertNoHiddenChars(trimmed, source); - if (/\s/.test(trimmed)) { - throw new GitNexusRcError(`${source}: branch name must not contain whitespace.`); - } - // git ref-name rules (subset): reject characters git itself forbids in refs. - if (/[~^:?*[\\]/.test(trimmed)) { - throw new GitNexusRcError( - `${source}: branch name contains characters not allowed in a git ref (~ ^ : ? * [ \\).`, - ); - } - if (trimmed.startsWith('-')) { - throw new GitNexusRcError(`${source}: branch name must not start with "-".`); - } - if (trimmed.includes('..')) { - throw new GitNexusRcError(`${source}: branch name must not contain "..".`); - } - // Git permits a backtick in a ref, but the branch is embedded inside a - // Markdown inline-code span in the generated AGENTS.md/CLAUDE.md regression - // example, where a backtick would close the span early and let the rest of - // the template render as instruction text. Reject it at this single - // chokepoint so all three tiers (CLI flag, .gitnexusrc, auto-detect via - // sanitizeDetectedBranch) are covered (#1996 tri-review P1). - if (trimmed.includes('`')) { - throw new GitNexusRcError( - `${source}: branch name must not contain a backtick (it would break the generated Markdown).`, - ); - } - return trimmed; } /** diff --git a/gitnexus/src/core/git-ref.ts b/gitnexus/src/core/git-ref.ts new file mode 100644 index 000000000..e7d101eed --- /dev/null +++ b/gitnexus/src/core/git-ref.ts @@ -0,0 +1,128 @@ +/** + * Git ref-name validation used by both the CLI and the HTTP analyze route. + * + * Lives in `core/` so `server/api.ts` does not import `cli/analyze-config` + * (that import closed a cli → server → cli cycle: `cli/serve.ts` already + * imports `createServer`). The CLI keeps a thin wrapper that rethrows + * {@link InvalidBranchError} as `GitNexusRcError`. + */ + +/** Git refs longer than this are almost certainly a mistake / injection attempt. */ +const BRANCH_MAX_LENGTH = 255; + +/** + * Thrown when a user-supplied branch name fails {@link validateBranchName}. + * Callers at a product boundary map this to their own error type (CLI: + * `GitNexusRcError`; HTTP: 400). + */ +export class InvalidBranchError extends Error { + constructor(message: string) { + super(message); + this.name = 'InvalidBranchError'; + } +} + +/** + * Reject control characters and hidden / bidirectional Unicode in a string + * value. These have no legitimate place in a branch name and would otherwise + * let a committed config or HTTP body smuggle invisible controls into + * generated AGENTS.md / CLAUDE.md content. + */ +const isHiddenOrControl = (codePoint: number): boolean => + codePoint < 0x20 || + codePoint === 0x7f || + (codePoint >= 0x200b && codePoint <= 0x200f) || // zero-width + LRM/RLM + (codePoint >= 0x202a && codePoint <= 0x202e) || // bidi embeddings/overrides + (codePoint >= 0x2060 && codePoint <= 0x2064) || // word-joiner + invisible math + (codePoint >= 0x2066 && codePoint <= 0x206f) || // bidi isolates + deprecated + codePoint === 0xfeff; // BOM / zero-width no-break space + +const assertNoHiddenChars = (value: string, source: string): void => { + for (const ch of value) { + const cp = ch.codePointAt(0); + if (cp !== undefined && isHiddenOrControl(cp)) { + throw new InvalidBranchError( + `${source}: value contains control or hidden/bidirectional characters, which are not allowed.`, + ); + } + } +}; + +/** + * Validate a user-supplied branch name. Returns the trimmed name or throws + * {@link InvalidBranchError}. Conservative but accepts the shapes real + * branches use (`feature/foo-bar`, `release/1.2`, `develop`). + */ +export function validateBranchName(value: string, source: string): string { + const trimmed = value.trim(); + if (!trimmed) { + throw new InvalidBranchError(`${source}: branch name must not be empty.`); + } + if (trimmed.length > BRANCH_MAX_LENGTH) { + throw new InvalidBranchError(`${source}: branch name is too long (max ${BRANCH_MAX_LENGTH}).`); + } + assertNoHiddenChars(trimmed, source); + if (/\s/.test(trimmed)) { + throw new InvalidBranchError(`${source}: branch name must not contain whitespace.`); + } + // git ref-name rules (subset): reject characters git itself forbids in refs. + if (/[~^:?*[\\]/.test(trimmed)) { + throw new InvalidBranchError( + `${source}: branch name contains characters not allowed in a git ref (~ ^ : ? * [ \\).`, + ); + } + if (trimmed.startsWith('-')) { + throw new InvalidBranchError(`${source}: branch name must not start with "-".`); + } + // Force-refspec prefix (`git fetch origin +main` / `+refs/heads/main:…`). + // Rejected here so neither the CLI nor HTTP can pass a force-update refspec + // through as a "branch" (#3199 review, defense in depth). + if (trimmed.startsWith('+')) { + throw new InvalidBranchError(`${source}: branch name must not start with "+".`); + } + // The symbolic ref HEAD (case-sensitive). A repo can have a branch named + // `head`; git itself treats only `HEAD` as the current-commit alias. + if (trimmed === 'HEAD') { + throw new InvalidBranchError(`${source}: branch name must not be "HEAD".`); + } + if (trimmed.includes('..')) { + throw new InvalidBranchError(`${source}: branch name must not contain "..".`); + } + // The remaining `git check-ref-format` rules. Without these the validator + // accepted refs git itself refuses (`feature.lock`, `/feature`, `feature/`, + // `feature//next`, `@`, `.hidden`), so the failure surfaced later from the + // git subprocess instead of here. No real branch can violate them — git + // could not have created one — so nothing that works today starts failing. + if (trimmed.endsWith('.lock') || trimmed.split('/').some((part) => part.endsWith('.lock'))) { + throw new InvalidBranchError(`${source}: branch name must not end with ".lock".`); + } + if (trimmed.startsWith('/') || trimmed.endsWith('/')) { + throw new InvalidBranchError(`${source}: branch name must not start or end with "/".`); + } + if (trimmed.includes('//')) { + throw new InvalidBranchError(`${source}: branch name must not contain consecutive slashes.`); + } + if (trimmed === '@') { + throw new InvalidBranchError(`${source}: branch name must not be the single character "@".`); + } + if (trimmed.includes('@{')) { + throw new InvalidBranchError(`${source}: branch name must not contain "@{".`); + } + if (trimmed.endsWith('.') || trimmed.split('/').some((part) => part.startsWith('.'))) { + throw new InvalidBranchError( + `${source}: branch name must not end with "." or have a path component starting with ".".`, + ); + } + // Git permits a backtick in a ref, but the branch is embedded inside a + // Markdown inline-code span in the generated AGENTS.md/CLAUDE.md regression + // example, where a backtick would close the span early and let the rest of + // the template render as instruction text. Reject it at this single + // chokepoint so all three tiers (CLI flag, .gitnexusrc, auto-detect via + // sanitizeDetectedBranch) are covered (#1996 tri-review P1). + if (trimmed.includes('`')) { + throw new InvalidBranchError( + `${source}: branch name must not contain a backtick (it would break the generated Markdown).`, + ); + } + return trimmed; +} diff --git a/gitnexus/src/server/analyze-job.ts b/gitnexus/src/server/analyze-job.ts index a0a4b527d..fcd6af615 100644 --- a/gitnexus/src/server/analyze-job.ts +++ b/gitnexus/src/server/analyze-job.ts @@ -68,6 +68,13 @@ export interface AnalyzeJob { repoUrl?: string; repoPath?: string; repoName?: string; + /** + * Index-branch selector this job was started with, part of the job's dedup + * identity. A repo is not "the same repo" for reuse purposes when a different + * branch was asked for — reusing across branches would hand the caller a 202 + * for a job indexing something else. + */ + branch?: string; progress: AnalyzeJobProgress; error?: string; /** Set only when a terminal `failed` job still persisted usable work. */ @@ -94,15 +101,25 @@ export class JobManager { this.cleanupTimer = setInterval(() => this.cleanup(), CLEANUP_INTERVAL_MS); } - /** Create a new job, or return existing active job for the same repo. */ - createJob(params: { repoUrl?: string; repoPath?: string }): AnalyzeJob { - // Dedup: return existing active job for the same repo (by URL or path) + /** + * Create a new job, or return the existing active job for the same repo AND + * the same branch. + * + * Branch is part of the identity deliberately. Deduping on repo alone would + * return the in-flight job for branch A to a caller that asked for branch B, + * and that caller would read the resulting 202/`complete` as "B is indexed" + * — the same silent wrong-branch outcome that made `branch` worth honoring in + * the first place. Falling through instead lets the single-slot guard below + * reject the request outright, which is a truthful answer. + */ + createJob(params: { repoUrl?: string; repoPath?: string; branch?: string }): AnalyzeJob { + // Dedup: return existing active job for the same repo (by URL or path) and branch for (const job of this.jobs.values()) { if (!this.isTerminal(job.status)) { const isSameRepo = (params.repoUrl && job.repoUrl === params.repoUrl) || (params.repoPath && job.repoPath === params.repoPath); - if (isSameRepo) { + if (isSameRepo && job.branch === params.branch) { return job; } } @@ -120,6 +137,7 @@ export class JobManager { status: 'queued', repoUrl: params.repoUrl, repoPath: params.repoPath, + branch: params.branch, progress: { phase: 'queued', percent: 0, message: 'Waiting to start...' }, startedAt: Date.now(), retryCount: 0, diff --git a/gitnexus/src/server/analyze-launch.ts b/gitnexus/src/server/analyze-launch.ts index 06ddf94c4..901963a87 100644 --- a/gitnexus/src/server/analyze-launch.ts +++ b/gitnexus/src/server/analyze-launch.ts @@ -22,6 +22,7 @@ import { listRegisteredRepos, registryPathEquals, } from '../storage/repo-manager.js'; +import { BRANCHES_DIR, branchSlug } from '../storage/branch-index.js'; import { logger } from '../core/logger.js'; import { autoHeapCapMb } from '../core/ingestion/utils/effective-ram.js'; import { isTerminalJobStatus, type JobManager } from './analyze-job.js'; @@ -49,6 +50,20 @@ export interface LaunchOptions { springActuatorPath?: string; asyncApiSpecPath?: string; registryName?: string; + /** + * Index-branch selector, forwarded to `AnalyzeOptions.branch`. + * + * Setting it does not by itself mean a `branches//` sub-directory: + * `resolveBranchPlacement` (storage/branch-index.ts) keeps the run on the flat + * slot when that slot has no recorded owner, or when its owner already IS this + * label. Only a label that differs from the flat slot's owner gets its own + * sub-directory. + * + * The caller is responsible for having the branch checked out — + * `resolveWriteTarget` in core refuses a label that disagrees with the working + * tree, which is what keeps one branch's content out of another's slot (#2106). + */ + branch?: string; } const MAX_WORKER_RETRIES = 2; @@ -66,17 +81,6 @@ const MAX_WORKER_RETRIES = 2; const FINALIZE_SETTLE_TIMEOUT_MS = 60_000; const FINALIZE_SETTLE_POLL_MS = 200; -/** - * Resolve once the analyzed repo's index is settled at `storagePath`: the - * LadybugDB file and metadata both exist AND were (re)written by THIS job - * (mtime >= jobStartMs — bare existence is not enough, a re-analysis leaves - * the previous index in place while it works), and no transient WAL/shadow/ - * checkpoint sidecars remain (the worker's native close has finished). - * - * Never rejects. Timing out logs and proceeds (pre-gate behavior) rather - * than failing a job whose analysis genuinely succeeded — e.g. a no-op - * non-force analyze legitimately rewrites nothing. - */ /** * Look up the analyzed repo's registered storage path. The request's * user-provided path is used only as a comparison key; the filesystem probes @@ -91,7 +95,47 @@ const registeredStoragePath = async (targetPath: string): Promise return entry?.storagePath ?? null; }; -const waitForSettledIndex = async (targetPath: string, jobStartMs: number): Promise => { +/** + * Resolve the directory this run's index actually landed in. + * + * `registerRepo` always records the FLAT `.gitnexus` as `entry.storagePath`, + * but a pinned `--branch` run whose label differs from the flat slot's owner + * writes `lbug`/`gitnexus.json` under `branches//` instead. Probing the + * flat path for such a run watches files it never rewrote, so the gate below + * would spin to its timeout on a perfectly successful analysis (#3199 review). + * + * `isPrimaryBranch` is the worker's own report of `!placement.branch`, so this + * follows the placement core actually chose rather than recomputing it here + * (the flat slot's recorded owner can be adopted mid-run, which would make a + * recomputation race the thing it is trying to observe). + */ +const settleDirFor = ( + registryStoragePath: string, + branch: string | undefined, + isPrimaryBranch: boolean | undefined, +): string => + branch && isPrimaryBranch === false + ? path.join(registryStoragePath, BRANCHES_DIR, branchSlug(branch)) + : registryStoragePath; + +/** + * Resolve once the analyzed repo's index is settled at `storagePath`: the + * LadybugDB file and metadata both exist AND were (re)written by THIS job + * (mtime >= jobStartMs — bare existence is not enough, a re-analysis leaves + * the previous index in place while it works), and no transient WAL/shadow/ + * checkpoint sidecars remain (the worker's native close has finished). + * + * Never rejects. Timing out logs and proceeds (pre-gate behavior) rather + * than failing a job whose analysis genuinely succeeded. The `alreadyUpToDate` + * fast path never rewrites `lbug` (see `run-analyze.ts`) and skips this wait + * at the `complete` handler so it does not hold the analyze slot for 60s. + */ +const waitForSettledIndex = async ( + targetPath: string, + jobStartMs: number, + branch?: string, + isPrimaryBranch?: boolean, +): Promise => { const settled = (storagePath: string): boolean => { try { const lbugStat = statSync(path.join(storagePath, 'lbug')); @@ -112,7 +156,7 @@ const waitForSettledIndex = async (targetPath: string, jobStartMs: number): Prom // Re-resolved each round: the worker registers the repo as part of the // finalization this gate is waiting out. const storagePath = await registeredStoragePath(targetPath); - if (storagePath && settled(storagePath)) return; + if (storagePath && settled(settleDirFor(storagePath, branch, isPrimaryBranch))) return; if (Date.now() > deadline) { logger.warn( { targetPath }, @@ -173,6 +217,13 @@ export function createLaunchAnalysisWorker(deps: LaunchDeps) { // Capture stderr for crash diagnostics let stderrChunks = ''; + // A terminal IPC message (`complete`/`error`) means the worker finished + // and is now winding down — it calls process.exit(0) ~500ms later. The + // job is deliberately still non-terminal at that point because the + // finalization gate is running, so without this flag the exit handler + // below reads that clean exit as a crash and retries a SUCCESSFUL + // analysis, three times, before failing it (#3199 review). + let terminalIpcSeen = false; child.stderr?.on('data', (chunk: Buffer) => { stderrChunks += chunk.toString(); if (stderrChunks.length > 4096) stderrChunks = stderrChunks.slice(-4096); @@ -186,6 +237,8 @@ export function createLaunchAnalysisWorker(deps: LaunchDeps) { const current = jobManager.getJob(job.id); if (!current || isTerminalJobStatus(current.status)) return; + if (msg.type === 'complete' || msg.type === 'error') terminalIpcSeen = true; + if (msg.type === 'progress') { jobManager.updateJob(job.id, { status: 'analyzing', @@ -202,7 +255,16 @@ export function createLaunchAnalysisWorker(deps: LaunchDeps) { // below true in practice: the repo is actually queryable when the // client receives the SSE complete event, and an index this run knows // to be incomplete is never published at all. - waitForSettledIndex(targetPath, jobStartMs) + // + // alreadyUpToDate never opens LadybugDB and never rewrites `lbug` + // (run-analyze.ts early-return; CLI notes the same). The mtime gate + // would spin the full 60s and hold the single global analyze slot. + // ftsRepairedOnly DOES rewrite `lbug` (initLbug + createSearchFTSIndexes) + // so it still waits. + const settle = msg.result.alreadyUpToDate + ? Promise.resolve() + : waitForSettledIndex(targetPath, jobStartMs, opts.branch, msg.result.isPrimaryBranch); + settle .then(() => closeDbHandle()) .catch(() => {}) // best-effort: eviction failure must not fail the job .then(() => { @@ -296,6 +358,13 @@ export function createLaunchAnalysisWorker(deps: LaunchDeps) { const j = jobManager.getJob(job.id); if (!j || isTerminalJobStatus(j.status)) return; + // The worker already reported a terminal outcome; this exit is it + // winding down, not dying. The job is still non-terminal only because + // the finalization gate above has not resolved yet, and that gate owns + // the outcome — retrying here would fork a second worker over a + // finished, successful analysis. + if (terminalIpcSeen) return; + // Worker crashed — attempt retry if under the limit if (j.retryCount < MAX_WORKER_RETRIES) { j.retryCount++; @@ -339,6 +408,7 @@ export function createLaunchAnalysisWorker(deps: LaunchDeps) { ...(opts.springActuatorPath ? { springActuatorPath: opts.springActuatorPath } : {}), ...(opts.asyncApiSpecPath ? { asyncApiSpecPath: opts.asyncApiSpecPath } : {}), ...(opts.registryName ? { registryName: opts.registryName } : {}), + ...(opts.branch ? { branch: opts.branch } : {}), }, }); }; diff --git a/gitnexus/src/server/analyze-worker-ipc.ts b/gitnexus/src/server/analyze-worker-ipc.ts index 3bb0a4edd..15b8112d6 100644 --- a/gitnexus/src/server/analyze-worker-ipc.ts +++ b/gitnexus/src/server/analyze-worker-ipc.ts @@ -44,10 +44,11 @@ import type { AnalyzeResult } from '../core/run-analyze.js'; * ones (e.g. `isPrimaryBranch?`), so an optional non-serializable field could be * advertised by the type yet silently dropped by the runtime allowlist. * - * `isPrimaryBranch` is intentionally excluded: the parent (`api.ts`) reads only - * `repoName`, and nothing consumes `isPrimaryBranch` across this fork (its CLI - * consumer calls `runFullAnalysis` in-process). Add a field here only when a - * server-side IPC consumer actually needs it — and only if it is JSON-safe. + * `isPrimaryBranch` IS on the wire, under exactly the rule this comment used to + * cite for excluding it: a server-side consumer now needs it. `analyze-launch.ts` + * settles the index the run actually wrote, and only the worker knows whether + * core chose the flat slot or a `branches//` sub-slot. It is a boolean, so + * it is JSON-safe by construction. */ export type AnalyzeResultIpc = Pick< AnalyzeResult, @@ -58,6 +59,7 @@ export type AnalyzeResultIpc = Pick< | 'ftsRepairedOnly' | 'ftsSkipped' | 'graphWriteCollapsed' + | 'isPrimaryBranch' >; /** @@ -78,5 +80,9 @@ export function projectAnalyzeResultForIpc(result: AnalyzeResult): AnalyzeResult // outcome the CLI does; without it the worker reports a clean `complete` // for a run whose edges are mostly missing. graphWriteCollapsed: result.graphWriteCollapsed, + // Tells the parent which slot this run wrote — the flat `.gitnexus` or a + // `branches//` sub-slot — so its finalization gate watches the files + // this job actually rewrote (#3199 review). + isPrimaryBranch: result.isPrimaryBranch, }; } diff --git a/gitnexus/src/server/api.ts b/gitnexus/src/server/api.ts index 0843d0b63..5a731ae0b 100644 --- a/gitnexus/src/server/api.ts +++ b/gitnexus/src/server/api.ts @@ -57,6 +57,7 @@ import { assertString, BadRequestError, createRouteLimiter } from './validation. import { parseGrepQuery, GREP_TIME_BUDGET_MS } from './grep-params.js'; import { runGrepScanInWorker } from './grep-scan.js'; import { + analyzeCloneOptions, extractWebRepoName, getCloneDir, cloneOrPull, @@ -64,6 +65,10 @@ import { GITHUB_TOKEN_HOSTS, } from './git-clone.js'; import { createAnalyzeUploadHandler } from './analyze-upload.js'; +// Shared with the CLI's `--branch` (via the analyze-config wrapper) so both +// entry points accept the same refs. Imported from core — not cli/ — so +// createServer does not close a cycle with cli/serve.ts. +import { InvalidBranchError, validateBranchName } from '../core/git-ref.js'; import { assertServeAuthForPublicOrigin, createPublicOriginMatcher, @@ -1519,6 +1524,7 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => springActuatorPath, asyncApiSpecPath, token: repoToken, + branch: repoBranch, } = req.body; // Input type validation @@ -1550,6 +1556,27 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => return; } + // Branch: optional index-branch selector, validated with the same rules + // as the CLI's `--branch` so both entry points accept the same refs. + // Rejecting here (rather than letting the clone fail) keeps a malformed + // ref from ever reaching `git`. + if (repoBranch !== undefined && typeof repoBranch !== 'string') { + res.status(400).json({ error: '"branch" must be a string' }); + return; + } + let analyzeBranch: string | undefined; + if (repoBranch !== undefined) { + try { + analyzeBranch = validateBranchName(repoBranch, '"branch"'); + } catch (err) { + if (err instanceof InvalidBranchError) { + res.status(400).json({ error: err.message }); + return; + } + throw err; + } + } + // Token: optional, restricted charset to prevent header smuggling // (CRLF), bound length, and bound to github.com (see validateAnalyzeToken). const tokenError = validateAnalyzeToken(repoToken, repoUrl); @@ -1575,7 +1602,11 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => return; } - const job = jobManager.createJob({ repoUrl, repoPath: repoLocalPath }); + const job = jobManager.createJob({ + repoUrl, + repoPath: repoLocalPath, + branch: analyzeBranch, + }); // If job was already running (dedup), just return its id. The token is // not part of the dedup identity and is never stored on the job, so a @@ -1603,11 +1634,15 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => // Clone if URL provided if (repoUrl && !repoLocalPath) { const repoName = extractWebRepoName(repoUrl); - targetPath = getCloneDir(repoName); + // Branch-pinned runs get their own clone dir, so they never share + // a working tree with the unpinned one (see getCloneDir). + targetPath = getCloneDir(repoName, analyzeBranch); jobManager.updateJob(job.id, { status: 'cloning', - repoName, + // url+branch: same value as registryName (dir basename), not + // the extractWebRepoName stem used only as getCloneDir's first arg. + repoName: analyzeBranch ? path.basename(targetPath) : repoName, progress: { phase: 'cloning', percent: 0, message: `Cloning ${repoUrl}...` }, }); @@ -1619,7 +1654,7 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => progress: { phase: progress.phase, percent: 5, message: progress.message }, }); }, - repoToken ? { token: repoToken } : undefined, + analyzeCloneOptions(repoToken, analyzeBranch), ); } @@ -1633,6 +1668,20 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => dropEmbeddings, springActuatorPath, asyncApiSpecPath, + branch: analyzeBranch, + // Both clone dirs share an `origin`, so the name `registerRepo` + // infers from the remote would be identical and the second one + // would fail with RegistryNameCollisionError. Register the pinned + // clone under its directory name instead: unique per branch, and + // it re-derives through getCloneDir for DELETE /api/repo. + // + // Gated on the SAME condition as the clone above: when a caller + // supplies both `url` and `path` nothing is cloned, and renaming + // the operator's own local repo after its directory would be a + // surprise unrelated to branch pinning. + ...(analyzeBranch && repoUrl && !repoLocalPath + ? { registryName: path.basename(targetPath) } + : {}), }); } catch (err: any) { if (targetPath) releaseRepoLock(getStoragePath(targetPath)); diff --git a/gitnexus/src/server/git-clone.ts b/gitnexus/src/server/git-clone.ts index 6fb1e25c9..f5a8d9e22 100644 --- a/gitnexus/src/server/git-clone.ts +++ b/gitnexus/src/server/git-clone.ts @@ -11,6 +11,7 @@ import fs from 'fs/promises'; import os from 'node:os'; import { logger } from '../core/logger.js'; import { getGlobalDir } from '../storage/repo-manager.js'; +import { branchSlug } from '../storage/branch-index.js'; import { sanitizeRepoName, stripUrlCredentials } from '../storage/git.js'; import { validateGitUrl } from '../core/net/url-guard.js'; import { @@ -84,14 +85,65 @@ export function extractWebRepoName(url: string): string { return safeName; } -/** Get the clone target directory for a repo name. */ -export function getCloneDir(repoName: string): string { +/** + * Longest single path component the supported filesystems accept (ext4, APFS, + * NTFS all cap at 255). Compared against `.length`, which equals the byte + * count here because every name this guards is ASCII by construction + * (REPO_NAME_PATTERN and sanitizeRepoName both restrict to `[a-zA-Z0-9._-]`). + */ +const MAX_PATH_COMPONENT_BYTES = 255; + +/** + * `branchSlug` for a clone-directory name, trimmed to fit one path component. + * + * `validateBranchName` allows a ref up to 255 characters and `branchSlug` + * appends `-` plus 8 hash characters, so `__` can reach 267 — past + * the filesystem limit, and the clone would then fail to create its target + * directory (#3199 review). + * + * Only the READABLE half is trimmed; the 8-character hash is always kept, and + * it is a digest of the full ref, so two long branches that share a prefix + * still get different directories. The slug is not trimmed inside + * `branchSlug` itself because the per-branch *index* slots already use those + * names on disk — shortening them there would orphan existing indexes. + */ +const boundedBranchSegment = (repoName: string, branch: string): string => { + const slug = branchSlug(branch); + if (`${repoName}__${slug}`.length <= MAX_PATH_COMPONENT_BYTES) return slug; + + const hash = slug.slice(slug.lastIndexOf('-')); // "-" + 8 hex + const budget = MAX_PATH_COMPONENT_BYTES - repoName.length - '__'.length - hash.length; + // A repo name long enough to leave no budget falls through to the caller's + // length check, which rejects it rather than building an unusable path. + return `${slug.slice(0, Math.max(0, budget))}${hash}`; +}; + +/** Get the clone target directory for a repo name, optionally pinned to a branch. */ +export function getCloneDir(repoName: string, branch?: string): string { // Re-validate at the boundary even though extractRepoName already checked — // callers may pass a repoName from another source (test fixtures, scripts). if (!repoName || repoName === '.' || repoName === '..' || !REPO_NAME_PATTERN.test(repoName)) { throw new Error('Invalid repository name'); } - return path.join(CLONE_ROOT, repoName); + // A branch-pinned analyze gets its OWN working tree. + // + // Sharing one checkout per repo made `branch` unusable in practice: the tree + // is dirty after any analyze (generated AGENTS.md / CLAUDE.md / .claude/), so + // a pinned request hit `cloneOrPull`'s porcelain refusal; and a later request + // that OMITTED `branch` would pull whatever branch the last pin left checked + // out and index it as the default (#3199 review). Separate directories remove + // both, because the two requests no longer share a tree. + // + // `branchSlug` is the same helper the per-branch index slots use, so the two + // layouts agree on how a ref becomes a path segment. It emits only + // `[a-zA-Z0-9._-]`, so the composed name still satisfies REPO_NAME_PATTERN and + // round-trips through this function — which is how DELETE /api/repo re-derives + // the directory from the registry name. + const dirName = branch ? `${repoName}__${boundedBranchSegment(repoName, branch)}` : repoName; + if (!REPO_NAME_PATTERN.test(dirName) || dirName.length > MAX_PATH_COMPONENT_BYTES) { + throw new Error('Invalid repository name'); + } + return path.join(CLONE_ROOT, dirName); } export interface CloneProgress { @@ -99,6 +151,29 @@ export interface CloneProgress { message: string; } +/** + * Build the `cloneOrPull` options for an `/api/analyze` request. + * + * Extracted from the route so the token/branch combination is unit-testable. + * Inline, the branch-only case was the one nothing asserted: every existing + * test still passed if `branch` were dropped whenever no token was supplied — + * i.e. silently cloning the default branch for every public URL, which is the + * exact behavior #3198 is about (#3199 review). + * + * Returns `undefined` rather than `{}` when neither is set, because that is + * what `cloneOrPull` treats as "no options" at its own call sites. + */ +export function analyzeCloneOptions( + token?: string, + branch?: string, +): Pick | undefined { + if (!token && !branch) return undefined; + return { + ...(token ? { token } : {}), + ...(branch ? { branch } : {}), + }; +} + export interface CloneOrPullOptions { token?: string; allowedCloneRoot?: string; @@ -294,11 +369,127 @@ export async function assertRemoteMatchesRequestedUrl( } } +/** + * Fetch refspec that updates `origin/` from `refs/heads/`. + * + * The leading `+` is git's dest-update prefix (`+refs/heads/*:refs/remotes/origin/*` + * is what `git clone` writes into `.git/config`). Without it, `fetch --depth 1` + * refuses to move `origin/` when the shallow history cannot prove a + * fast-forward — so a same-branch re-index stays stuck on the old tip. + * + * The user string is interpolated inside `refs/heads/…`, never as a raw pull + * dest. A branch named `+develop` becomes `+refs/heads/+develop:…`, not a + * force-update of `develop`. + */ +function branchFetchRefspec(branch: string): string { + return `+refs/heads/${branch}:refs/remotes/origin/${branch}`; +} + +/** Overlays `analyze` writes into a clone; they must not block a same-ref update. */ +const GITNEXUS_GENERATED_OVERLAYS = ['./AGENTS.md', './CLAUDE.md', './.claude'] as const; + +/** + * Restore only GitNexus-generated overlays so a same-ref update is not + * blocked by analyze dirt. Path-limited and root-anchored (`./`): tracked + * files are checked out from HEAD; untracked overlays (including gitignored + * ones — `AGENTS.md` / `.claude/` are commonly ignored) are `git clean -fdx`'d. + * A slash-free `AGENTS.md` would also hit `docs/AGENTS.md`. Never a + * whole-clone `git clean --force -d`. + */ +async function restoreGitNexusGeneratedOverlays( + runGitImpl: typeof runGit, + cwd: string, + gitOpts: RunGitOptions, +): Promise { + for (const overlay of GITNEXUS_GENERATED_OVERLAYS) { + const listed = (await runGitImpl(['ls-files', '--', overlay], cwd, gitOpts)).trim(); + if (!listed) continue; + await runGitImpl(['checkout', 'HEAD', '--', overlay], cwd, gitOpts); + } + // Path-limited: untracked analyze output still blocks checkout when the + // incoming tree has the same path, and otherwise leaves a dirty tree to + // index. `-x` is required because these overlays are often gitignored. + // Never a whole-clone `git clean --force -d`. + await runGitImpl(['clean', '-fdx', '--', ...GITNEXUS_GENERATED_OVERLAYS], cwd, gitOpts); +} + +/** + * True when the working tree is already at the requested pin: either HEAD is + * that named branch, or HEAD is detached at the same SHA as `branch` / + * `origin/`. A missing ref falls through to the switch path. + */ +async function matchRequestedRef( + runGitImpl: typeof runGit, + cwd: string, + branch: string, + gitOpts: RunGitOptions, +): Promise<'branch' | 'sha' | undefined> { + const abbrev = (await runGitImpl(['rev-parse', '--abbrev-ref', 'HEAD'], cwd, gitOpts)).trim(); + if (abbrev === branch) return 'branch'; + // Detached HEAD reports `HEAD`; compare SHAs so a tag/SHA pin is not a switch. + if (abbrev !== 'HEAD') return undefined; + + let headSha: string; + try { + headSha = (await runGitImpl(['rev-parse', 'HEAD'], cwd, gitOpts)).trim(); + } catch { + return undefined; + } + + for (const candidate of [branch, `origin/${branch}`] as const) { + try { + // Peel annotated tags (`v1.0` is a tag object; HEAD is the commit). + const requestedSha = ( + await runGitImpl(['rev-parse', `${candidate}^{commit}`], cwd, gitOpts) + ).trim(); + if (requestedSha && requestedSha === headSha) return 'sha'; + } catch { + // Ref missing — try origin/, then the switch path. + } + } + return undefined; +} + +async function fetchAndCheckoutRequestedBranch( + runGitImpl: typeof runGit, + cwd: string, + branch: string, + gitOpts: RunGitOptions, +): Promise { + // Analyze clones are `--depth 1`. `merge --ff-only` cannot walk O→N when + // the remote moved 2+ commits (the merge-base is not in the shallow + // history). `checkout -B` points the local branch at the fetched tip — + // same as the switch path, no ancestry walk. No `--force`: leftover + // non-overlay dirt still refuses. + await runGitImpl(['fetch', '--depth', '1', 'origin', branchFetchRefspec(branch)], cwd, gitOpts); + await runGitImpl(['checkout', '-B', branch, `origin/${branch}`], cwd, gitOpts); +} + /** * Clone or pull a git repository. - * If targetDir doesn't exist: git clone --depth 1 - * If targetDir exists with .git: git pull --ff-only (after verifying the - * existing clone's remote.origin matches the requested URL). + * + * If targetDir doesn't exist: git clone --depth 1, adding `--branch ` + * when one is requested. + * + * If targetDir exists with .git, its remote.origin is verified against the + * requested URL first, and then the branch decides the update: + * - no `options.branch`: git pull --ff-only, which updates the current + * branch in place via its configured upstream. Nothing moves, so no + * dirty-tree check applies. + * - a `options.branch` that is ALREADY the current named branch: restore + * GitNexus overlays, then fetch via + * `+refs/heads/:refs/remotes/origin/` and + * `checkout -B origin/` (shallow clones cannot + * `merge --ff-only` across a 2+ commit move). Never a raw + * `origin ` pull refspec. No porcelain refuse. + * - a detached HEAD whose SHA already matches the requested ref (tag / + * SHA pin): restore overlays only. Do not fetch/merge — a same-named + * branch could otherwise fast-forward the pin past the tag. + * - a `options.branch` that DIFFERS from the current pin: fetch that ref, + * then `checkout -B origin/` — so the requested branch, + * not the one already checked out, is what ends up in the working tree. + * This is the switching case, and it refuses a dirty tree unless + * `overwriteLocalChanges` is set. * * Security: * - targetDir must resolve inside CLONE_ROOT (~/.gitnexus/repos/). The @@ -382,13 +573,36 @@ export async function cloneOrPull( await assertRemoteMatchesRequestedUrl(safeTarget, url, options?.timeoutMs); onProgress?.({ phase: 'pulling', message: 'Pulling latest changes...' }); const runGitImpl = options?.runGitForTest ?? runGit; - if (options?.branch) { + const gitOpts = { + token: options?.token, + url, + timeoutMs: options?.timeoutMs, + }; + // Already at the requested pin? Then there is no switch to make, so do + // not take the checkout path below — that would run the porcelain check + // against a tree ANALYZE ITSELF dirtied (it writes AGENTS.md / CLAUDE.md / + // .claude/ into the clone), which made a pinned RE-index impossible: the + // first pin succeeded and every later one failed asking for + // `overwrite_local_changes`, a flag this route deliberately does not pass + // because it would `git clean --force -d` the directory (#3199 review). + // + // "Already there" is a named-branch match OR a detached HEAD whose SHA + // equals `branch` / `origin/` (tag / SHA pin). A missing ref + // falls through to the switch path, which still refuses a dirty tree. + // + // Same-named-branch update uses the heads/ → remotes/ fetch refspec plus + // `checkout -B origin/`, never `pull origin ` + // (a leading `+` would otherwise be a force-fetch). Only + // `remote.origin.url` is verified above; `branch..remote` / + // `.merge` are not, so an implicit-upstream pull can update a different + // ref while the job still carries this branch (#3199 review). + const requestedRefMatch = options?.branch + ? await matchRequestedRef(runGitImpl, safeTarget, options.branch, gitOpts) + : undefined; + + if (options?.branch && !requestedRefMatch) { if (!options.overwriteLocalChanges) { - const status = await runGitImpl(['status', '--porcelain'], safeTarget, { - token: options?.token, - url, - timeoutMs: options?.timeoutMs, - }); + const status = await runGitImpl(['status', '--porcelain'], safeTarget, gitOpts); if (status.trim()) { throw new Error( `Refusing to update ${safeTarget}: local changes detected. Set overwrite_local_changes: true to overwrite them.`, @@ -396,19 +610,9 @@ export async function cloneOrPull( } } await runGitImpl( - [ - 'fetch', - '--depth', - '1', - 'origin', - `refs/heads/${options.branch}:refs/remotes/origin/${options.branch}`, - ], + ['fetch', '--depth', '1', 'origin', branchFetchRefspec(options.branch)], safeTarget, - { - token: options?.token, - url, - timeoutMs: options?.timeoutMs, - }, + gitOpts, ); await runGitImpl( [ @@ -419,11 +623,7 @@ export async function cloneOrPull( `origin/${options.branch}`, ], safeTarget, - { - token: options?.token, - url, - timeoutMs: options?.timeoutMs, - }, + gitOpts, ); if (options.overwriteLocalChanges) { // `checkout --force` rewrites tracked files only, so untracked sources @@ -432,18 +632,17 @@ export async function cloneOrPull( // ignored paths must survive, and `-e /.gitnexus` is belt-and-braces // because `.git/info/exclude` is skipped on a read-only storage mount // and a freshly cloned repo may not have been analyzed yet at all. - await runGitImpl(['clean', '--force', '-d', '-e', '/.gitnexus'], safeTarget, { - token: options?.token, - url, - timeoutMs: options?.timeoutMs, - }); + await runGitImpl(['clean', '--force', '-d', '-e', '/.gitnexus'], safeTarget, gitOpts); } + } else if (options?.branch && requestedRefMatch === 'branch') { + await restoreGitNexusGeneratedOverlays(runGitImpl, safeTarget, gitOpts); + await fetchAndCheckoutRequestedBranch(runGitImpl, safeTarget, options.branch, gitOpts); + } else if (options?.branch && requestedRefMatch === 'sha') { + // Tag / SHA pin: already at the requested commit. Fetching + // `refs/heads/` would follow a same-named branch past the pin. + await restoreGitNexusGeneratedOverlays(runGitImpl, safeTarget, gitOpts); } else { - await runGitImpl(['pull', '--ff-only'], safeTarget, { - token: options?.token, - url, - timeoutMs: options?.timeoutMs, - }); + await runGitImpl(['pull', '--ff-only'], safeTarget, gitOpts); } } else { if (targetExists && (await fs.readdir(safeTarget)).length > 0) { diff --git a/gitnexus/test/integration/server-analyze-branch-validation.test.ts b/gitnexus/test/integration/server-analyze-branch-validation.test.ts new file mode 100644 index 000000000..3e71ebf02 --- /dev/null +++ b/gitnexus/test/integration/server-analyze-branch-validation.test.ts @@ -0,0 +1,210 @@ +/** + * End-to-end HTTP test of POST /api/analyze `branch` validation. + * + * `branch` is handed to `git` as a ref (`clone --branch`, `checkout -B`), so the + * route validates it with the same `validateBranchName` the CLI's `--branch` + * uses. This proves the REAL production route wires that in — express.json body + * parsing, the requireTrustedOrigin guard, the route handler invoking the + * validator, and the 400 status/error shape on the wire. + * + * Only rejection paths are asserted: each returns 400 BEFORE any clone, so the + * test is hermetic (no network, no background git, no real repo). The accepted + * path would spawn a background clone; the parent→worker half of it is covered + * in test/unit/analyze-launch-collapse.test.ts, which asserts `branch` reaches + * the worker's `AnalyzeOptions`. + * + * This lives in its own file rather than alongside the token cases because + * /api/analyze is rate-limited to 10 requests/minute per IP — one spawned server + * per concern keeps each suite clear of that ceiling. + * + * Mirrors the spawn+health-poll harness in server-analyze-token-validation.test.ts; + * the integration suite always builds dist first (pretest:integration). + */ +import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process'; +import fs from 'node:fs'; +import http from 'node:http'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = path.resolve(__dirname, '..', '..'); +const DIST_CLI = path.join(REPO_ROOT, 'dist', 'cli', 'index.js'); +const STARTUP_BUDGET_MS = process.env.CI ? 30_000 : 15_000; + +const allocateFreePort = (): Promise => + new Promise((resolve, reject) => { + const probe = http.createServer(); + probe.once('error', reject); + probe.listen(0, '127.0.0.1', () => { + const addr = probe.address(); + if (typeof addr !== 'object' || !addr) { + probe.close(); + reject(new Error('could not allocate ephemeral port')); + return; + } + const port = addr.port; + probe.close((err) => (err ? reject(err) : resolve(port))); + }); + }); + +const httpJson = ( + port: number, + method: string, + reqPath: string, + body?: unknown, +): Promise<{ status: number; body: string }> => + new Promise((resolve, reject) => { + const payload = body === undefined ? undefined : JSON.stringify(body); + const req = http.request( + { + host: '127.0.0.1', + port, + path: reqPath, + method, + headers: payload + ? { 'content-type': 'application/json', 'content-length': Buffer.byteLength(payload) } + : {}, + }, + (res) => { + const chunks: Buffer[] = []; + res.on('data', (c) => chunks.push(c)); + res.on('end', () => + resolve({ status: res.statusCode ?? 0, body: Buffer.concat(chunks).toString('utf8') }), + ); + }, + ); + req.on('error', reject); + req.setTimeout(5_000, () => { + req.destroy(); + reject(new Error(`${method} ${reqPath} timed out`)); + }); + if (payload) req.write(payload); + req.end(); + }); + +const postAnalyze = (port: number, body: unknown) => httpJson(port, 'POST', '/api/analyze', body); + +// Spawned `serve` on Windows can report ready before the socket is reachable +// from the parent (see server-http-startup.test.ts); validateBranchName's own +// unit coverage runs on every platform. +const describeBlock = process.platform === 'win32' ? describe.skip : describe; + +describeBlock('POST /api/analyze branch validation (real server)', () => { + let proc: ChildProcessWithoutNullStreams | undefined; + let homeDir: string | undefined; + let port = 0; + + beforeAll(async () => { + if (!fs.existsSync(DIST_CLI)) { + throw new Error(`Missing ${DIST_CLI} — run npm run build before integration tests`); + } + + port = await allocateFreePort(); + homeDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-analyze-branch-')); + + proc = spawn( + process.execPath, + [DIST_CLI, 'serve', '--port', String(port), '--host', '127.0.0.1'], + { + cwd: REPO_ROOT, + env: { ...process.env, GITNEXUS_HOME: homeDir, NODE_OPTIONS: '' }, + stdio: ['ignore', 'pipe', 'pipe'], + }, + ); + + let stderr = ''; + proc.stderr.on('data', (buf) => { + stderr += buf.toString(); + }); + + const startedAt = Date.now(); + while (Date.now() - startedAt < STARTUP_BUDGET_MS) { + if (proc.exitCode !== null) { + throw new Error(`serve exited ${proc.exitCode} before ready.\nstderr:\n${stderr}`); + } + try { + const { status } = await httpJson(port, 'GET', '/api/health'); + if (status === 200) return; + } catch { + // Server still starting — retry until budget expires. + } + await new Promise((r) => setTimeout(r, 100)); + } + throw new Error( + `serve did not become ready within ${STARTUP_BUDGET_MS}ms.\nstderr:\n${stderr}`, + ); + }, 60_000); + + afterAll(async () => { + if (proc && !proc.killed) { + proc.kill('SIGTERM'); + await new Promise((resolve) => { + const timer = setTimeout(() => { + proc?.kill('SIGKILL'); + resolve(); + }, 3_000); + proc?.on('exit', () => { + clearTimeout(timer); + resolve(); + }); + }); + } + proc = undefined; + if (homeDir) { + fs.rmSync(homeDir, { recursive: true, force: true }); + homeDir = undefined; + } + }); + + it('rejects a non-string branch', async () => { + const { status, body } = await postAnalyze(port, { + url: 'https://github.com/owner/repo', + branch: 42, + }); + expect(status).toBe(400); + expect(JSON.parse(body).error).toContain('"branch" must be a string'); + }); + + it('rejects a branch containing whitespace', async () => { + const { status, body } = await postAnalyze(port, { + url: 'https://github.com/owner/repo', + branch: 'feature branch', + }); + expect(status).toBe(400); + expect(JSON.parse(body).error).toContain('whitespace'); + }); + + it('rejects a branch using characters git forbids in a ref', async () => { + const { status, body } = await postAnalyze(port, { + url: 'https://github.com/owner/repo', + branch: 'feature^bad', + }); + expect(status).toBe(400); + expect(JSON.parse(body).error).toContain('not allowed in a git ref'); + }); + + it('rejects a branch that would read as a git option', async () => { + // `git clone --branch --upload-pack=evil` would otherwise let a ref choose + // the subprocess git runs; buildBranchCloneArgs keeps the `--` separator, + // and this closes the same shape one layer earlier. + const { status, body } = await postAnalyze(port, { + url: 'https://github.com/owner/repo', + branch: '--upload-pack=evil', + }); + expect(status).toBe(400); + expect(JSON.parse(body).error).toContain('must not start with "-"'); + }); + + it('rejects a whitespace-only branch rather than silently indexing the default', async () => { + // The bug this feature fixes was a silent fallback to the default branch; + // an unusable selector must fail loudly, never degrade into that behavior. + const { status, body } = await postAnalyze(port, { + url: 'https://github.com/owner/repo', + branch: ' ', + }); + expect(status).toBe(400); + expect(JSON.parse(body).error).toContain('must not be empty'); + }); +}); diff --git a/gitnexus/test/unit/analyze-config.test.ts b/gitnexus/test/unit/analyze-config.test.ts index fb2999cb2..890877835 100644 --- a/gitnexus/test/unit/analyze-config.test.ts +++ b/gitnexus/test/unit/analyze-config.test.ts @@ -299,6 +299,33 @@ describe('analyze-config (.gitnexusrc support, #243)', () => { expect(() => validateBranchName('foo..bar', 'src')).toThrow(/must not contain ".."/); }); + it('validateBranchName rejects the ref shapes git check-ref-format rejects', () => { + // These previously passed validation and failed later in the git subprocess, + // which over HTTP meant a 202 and a background failure instead of a 400. + expect(() => validateBranchName('feature.lock', 'src')).toThrow(/must not end with "\.lock"/); + expect(() => validateBranchName('refs/heads.lock/x', 'src')).toThrow( + /must not end with "\.lock"/, + ); + expect(() => validateBranchName('/feature', 'src')).toThrow(/must not start or end with "\/"/); + expect(() => validateBranchName('feature/', 'src')).toThrow(/must not start or end with "\/"/); + expect(() => validateBranchName('feature//next', 'src')).toThrow(/consecutive slashes/); + expect(() => validateBranchName('@', 'src')).toThrow(/single character "@"/); + expect(() => validateBranchName('feature@{1}', 'src')).toThrow(/must not contain "@\{"/); + expect(() => validateBranchName('.hidden', 'src')).toThrow(/starting with "\."/); + expect(() => validateBranchName('feature/.hidden', 'src')).toThrow(/starting with "\."/); + expect(() => validateBranchName('feature.', 'src')).toThrow(/end with "\."/); + }); + + it('validateBranchName still accepts the real branch shapes those rules must not catch', () => { + // A dot, a slash and an @ are all legal in the middle of a ref — the new + // rules must reject only what git itself would. + expect(validateBranchName('release/1.2.3', 'src')).toBe('release/1.2.3'); + expect(validateBranchName('feature/lockfile-bump', 'src')).toBe('feature/lockfile-bump'); + expect(validateBranchName('user@host', 'src')).toBe('user@host'); + expect(validateBranchName('v1.0', 'src')).toBe('v1.0'); + expect(validateBranchName('a/b/c', 'src')).toBe('a/b/c'); + }); + it('validateBranchName rejects a newline / control character', () => { expect(() => validateBranchName('main\nrm -rf', 'src')).toThrow(/control or hidden|whitespace/); }); @@ -394,6 +421,20 @@ describe('analyze-config (.gitnexusrc support, #243)', () => { expect(() => validateBranchName('a'.repeat(256), 'src')).toThrow(/too long/); }); + it('validateBranchName rejects HEAD (case-sensitive) and accepts head (#3199)', () => { + expect(() => validateBranchName('HEAD', 'src')).toThrow(GitNexusRcError); + expect(() => validateBranchName('HEAD', 'src')).toThrow(/must not be "HEAD"/); + expect(() => validateBranchName(' HEAD ', 'src')).toThrow(GitNexusRcError); + expect(validateBranchName('head', 'src')).toBe('head'); + }); + + it('validateBranchName rejects a force-refspec "+" prefix (#3199)', () => { + expect(() => validateBranchName('+main', 'src')).toThrow(GitNexusRcError); + expect(() => validateBranchName('+main', 'src')).toThrow(/must not start with "\+"/); + expect(() => validateBranchName('+develop', 'src')).toThrow(GitNexusRcError); + expect(() => validateBranchName('+develop', 'src')).toThrow(/must not start with "\+"/); + }); + it('rejects Markdown-significant characters in a config name, allows real names (#1996)', async () => { await writeRc(JSON.stringify({ name: '**evil**' })); expect(() => loadAnalyzeConfig(dir)).toThrow(/Markdown-significant/); diff --git a/gitnexus/test/unit/analyze-job.test.ts b/gitnexus/test/unit/analyze-job.test.ts index 6743a0b73..aadb9b09d 100644 --- a/gitnexus/test/unit/analyze-job.test.ts +++ b/gitnexus/test/unit/analyze-job.test.ts @@ -56,6 +56,65 @@ describe('JobManager', () => { expect(job2.id).toBe(job1.id); }); + it('returns existing job for the same repoUrl AND the same branch', () => { + const job1 = manager.createJob({ + repoUrl: 'https://github.com/user/repo', + branch: 'development', + }); + manager.updateJob(job1.id, { status: 'analyzing' }); + const job2 = manager.createJob({ + repoUrl: 'https://github.com/user/repo', + branch: 'development', + }); + expect(job2.id).toBe(job1.id); + }); + + it('does not return the active job to a caller asking for a different branch', () => { + const job1 = manager.createJob({ + repoUrl: 'https://github.com/user/repo', + branch: 'development', + }); + manager.updateJob(job1.id, { status: 'analyzing' }); + // Handing job1 back would report branch "development" as the work being done + // for a caller that asked for "main". Falling through to the single-slot + // guard is the truthful answer. + expect(() => + manager.createJob({ repoUrl: 'https://github.com/user/repo', branch: 'main' }), + ).toThrow(/already in progress/); + }); + + it('treats an unpinned request as distinct from a branch-pinned one', () => { + const job1 = manager.createJob({ + repoUrl: 'https://github.com/user/repo', + branch: 'development', + }); + manager.updateJob(job1.id, { status: 'analyzing' }); + expect(() => manager.createJob({ repoUrl: 'https://github.com/user/repo' })).toThrow( + /already in progress/, + ); + }); + + it('keeps callers that omit branch deduping exactly as before', () => { + // The parameter is optional, so every pre-existing call site (upload route, + // embed manager, tests) compares undefined === undefined and is unaffected. + const job1 = manager.createJob({ repoPath: '/tmp/repo' }); + manager.updateJob(job1.id, { status: 'analyzing' }); + expect(manager.createJob({ repoPath: '/tmp/repo' }).id).toBe(job1.id); + }); + + it('carries the branch unchanged through the whole job lifecycle', () => { + // `branch` is part of dedup identity, so it must not drift mid-flight. + // `updateJob`'s Pick<> allowlist omits it, so no well-typed caller can + // change it; this pins that none of the updates the server actually + // performs (clone -> analyze -> terminal) disturbs it either. + const job = manager.createJob({ repoUrl: 'https://github.com/user/repo', branch: 'develop' }); + manager.updateJob(job.id, { status: 'cloning' }); + manager.updateJob(job.id, { repoPath: '/tmp/repo', status: 'analyzing' }); + manager.updateJob(job.id, { progress: { phase: 'parsing', percent: 30, message: 'Parsing' } }); + manager.updateJob(job.id, { status: 'complete', repoName: 'repo' }); + expect(manager.getJob(job.id)?.branch).toBe('develop'); + }); + it('updates job progress', () => { const job = manager.createJob({ repoUrl: 'https://github.com/user/repo' }); manager.updateJob(job.id, { diff --git a/gitnexus/test/unit/analyze-launch-branch-settle.test.ts b/gitnexus/test/unit/analyze-launch-branch-settle.test.ts new file mode 100644 index 000000000..0fa3dc6a4 --- /dev/null +++ b/gitnexus/test/unit/analyze-launch-branch-settle.test.ts @@ -0,0 +1,238 @@ +/** + * The finalization gate must watch the index the run actually WROTE. + * + * `registerRepo` always records the flat `.gitnexus` as `entry.storagePath`, but + * a pinned `--branch` run whose label differs from the flat slot's owner writes + * `lbug`/`gitnexus.json` under `branches//`. The gate used to probe the + * flat path unconditionally, so for such a run it watched files this job never + * rewrote: + * + * - the gate never settles, so the job stays non-terminal for the full 60s; + * - meanwhile the worker, having already sent `complete`, calls + * `process.exit(0)` ~500ms later; + * - the exit handler saw a non-terminal job and classified that clean exit as + * a crash — retrying a SUCCESSFUL analysis three times before failing it + * with `Worker crashed 3 times (code 0)`. + * + * Reported twice on #3199 (maintainer review + @azizur100389's repro). These + * tests pin both halves: the gate follows the placement, and a terminal IPC + * makes a subsequent exit 0 settlement rather than a crash. + * + * The filesystem mock is deliberately PATH-SENSITIVE — only the branch sub-slot + * looks freshly written. A gate that probes the flat path therefore cannot pass + * these tests by accident. + */ +import { afterEach, beforeEach, describe, expect, it, vi, type Mock } from 'vitest'; +import { EventEmitter } from 'node:events'; +import path from 'node:path'; + +// `vi.hoisted` is lifted above the imports, so nothing in here may reference +// them — these stay plain literals and `path` is only used below. +const H = vi.hoisted(() => ({ + forkMock: vi.fn(), + STORAGE_PATH: '/tmp/gitnexus-settle-storage', + REPO_PATH: '/tmp/gitnexus-settle-repo', + METADATA_FILE: 'gitnexus.json', + // Set per-test: the only directory the fake filesystem reports as freshly + // written. Anything else looks stale, exactly like a slot this job skipped. + settledDir: '', +})); +const { forkMock, REPO_PATH } = H; + +vi.mock('child_process', async () => { + const actual = await vi.importActual('child_process'); + return { ...actual, fork: H.forkMock }; +}); + +vi.mock('../../src/storage/repo-manager.js', () => ({ + canonicalizePath: (p: string) => p, + getStoragePath: () => H.STORAGE_PATH, + INDEX_METADATA_FILE: H.METADATA_FILE, + listRegisteredRepos: async () => [{ path: H.REPO_PATH, storagePath: H.STORAGE_PATH }], + registryPathEquals: (a: string, b: string) => a === b, +})); + +vi.mock('node:fs', async () => { + const actual = await vi.importActual('node:fs'); + return { + ...actual, + statSync: (p: string) => { + // Fresh only inside the directory this run is pretending to have written. + // Plain string work rather than `path.dirname`: this factory is hoisted + // above the imports too. + const file = String(p); + const dir = file.slice(0, Math.max(file.lastIndexOf('/'), file.lastIndexOf('\\'))); + if (H.settledDir && dir === H.settledDir) { + return { mtimeMs: Number.MAX_SAFE_INTEGER }; + } + return { mtimeMs: 0 }; + }, + existsSync: () => false, // no WAL/shadow/checkpoint sidecars anywhere + }; +}); + +import { createLaunchAnalysisWorker } from '../../src/server/analyze-launch.js'; +import { JobManager } from '../../src/server/analyze-job.js'; +import { projectAnalyzeResultForIpc } from '../../src/server/analyze-worker-ipc.js'; +import { BRANCHES_DIR, branchSlug } from '../../src/storage/branch-index.js'; +import type { AnalyzeResult } from '../../src/core/run-analyze.js'; +import type { CompleteMessage } from '../../src/server/analyze-worker.js'; + +const BRANCH = 'feature/settle'; + +/** The `complete` message the worker really sends, via the production projection. */ +const completeMessage = ( + isPrimaryBranch: boolean, + extras?: { alreadyUpToDate?: boolean }, +): CompleteMessage => { + const result = { + repoName: 'settle-fixture', + repoPath: REPO_PATH, + stats: { files: 3, nodes: 9, edges: 12 }, + isPrimaryBranch, + ...(extras?.alreadyUpToDate ? { alreadyUpToDate: true } : {}), + } satisfies Partial as AnalyzeResult; + return { type: 'complete', result: projectAnalyzeResultForIpc(result) }; +}; + +interface FakeChild extends EventEmitter { + stderr: EventEmitter; + send: Mock<(msg: unknown) => boolean>; + kill: Mock<(signal?: NodeJS.Signals) => boolean>; +} + +const makeChild = (): FakeChild => { + const child = new EventEmitter() as FakeChild; + child.stderr = new EventEmitter(); + child.send = vi.fn(); + child.kill = vi.fn(); + return child; +}; + +describe('finalization gate follows the placement the run chose', () => { + let jobManager: JobManager; + let child: FakeChild; + let backendInit: Mock<() => Promise>; + let closeDbHandle: Mock<() => Promise>; + + const launcher = () => + createLaunchAnalysisWorker({ + jobManager, + backend: { init: backendInit }, + acquireRepoLock: () => null, + releaseRepoLock: () => {}, + closeDbHandle, + }); + + beforeEach(() => { + jobManager = new JobManager(); + child = makeChild(); + forkMock.mockImplementation(() => child); + backendInit = vi.fn(async () => true); + closeDbHandle = vi.fn(async () => {}); + H.settledDir = ''; + }); + + afterEach(() => { + jobManager.dispose(); + vi.restoreAllMocks(); + forkMock.mockReset(); + }); + + it('completes a branch sub-slot run, whose files are NOT in the flat slot', async () => { + // Only `branches//` looks written. The flat slot is stale, so a gate + // probing it would spin to the 60s timeout instead of settling here. + H.settledDir = path.join(H.STORAGE_PATH, BRANCHES_DIR, branchSlug(BRANCH)); + + const job = jobManager.createJob({ repoPath: REPO_PATH, branch: BRANCH }); + launcher()(job, REPO_PATH, { branch: BRANCH }); + + child.emit('message', completeMessage(false)); + + await vi.waitFor(() => expect(jobManager.getJob(job.id)?.status).toBe('complete')); + expect(jobManager.getJob(job.id)?.error).toBeUndefined(); + expect(backendInit).toHaveBeenCalledTimes(1); + }); + + it('still settles a flat-slot run against the flat slot', async () => { + // Control: the primary-branch case must keep watching `entry.storagePath`. + H.settledDir = H.STORAGE_PATH; + + const job = jobManager.createJob({ repoPath: REPO_PATH }); + launcher()(job, REPO_PATH, {}); + + child.emit('message', completeMessage(true)); + + await vi.waitFor(() => expect(jobManager.getJob(job.id)?.status).toBe('complete')); + expect(backendInit).toHaveBeenCalledTimes(1); + }); + + it('settles a first-pin (branch SET, isPrimaryBranch true) against the flat slot', async () => { + // Fresh clone: first pin adopts the flat slot. The slug dir is stale, so a + // gate that does `branch ? slugDir : flat` would spin the 60s timeout here. + H.settledDir = H.STORAGE_PATH; + + const job = jobManager.createJob({ repoPath: REPO_PATH, branch: BRANCH }); + launcher()(job, REPO_PATH, { branch: BRANCH }); + + child.emit('message', completeMessage(true)); + + await vi.waitFor(() => expect(jobManager.getJob(job.id)?.status).toBe('complete')); + expect(jobManager.getJob(job.id)?.error).toBeUndefined(); + expect(backendInit).toHaveBeenCalledTimes(1); + }); + + it('completes alreadyUpToDate quickly even when the slot is stale, without retrying', async () => { + // No directory looks freshly written. Without the alreadyUpToDate skip the + // mtime gate would hold the analyze slot for the full 60s settle timeout. + H.settledDir = ''; + + const job = jobManager.createJob({ repoPath: REPO_PATH }); + launcher()(job, REPO_PATH, {}); + + child.emit('message', completeMessage(true, { alreadyUpToDate: true })); + child.emit('exit', 0); + + await vi.waitFor(() => expect(jobManager.getJob(job.id)?.status).toBe('complete'), { + timeout: 2_000, + }); + expect(jobManager.getJob(job.id)?.error).toBeUndefined(); + expect(backendInit).toHaveBeenCalledTimes(1); + expect(forkMock).toHaveBeenCalledTimes(1); + expect(jobManager.getJob(job.id)?.retryCount).toBe(0); + }); + + it('does not fork a retry when the worker exits 0 after reporting complete', async () => { + // The worker exits ~500ms after the `complete` IPC, while the gate is still + // running and the job is deliberately non-terminal. That exit is the worker + // winding down, not dying. + H.settledDir = path.join(H.STORAGE_PATH, BRANCHES_DIR, branchSlug(BRANCH)); + + const job = jobManager.createJob({ repoPath: REPO_PATH, branch: BRANCH }); + launcher()(job, REPO_PATH, { branch: BRANCH }); + + child.emit('message', completeMessage(false)); + child.emit('exit', 0); + + await vi.waitFor(() => expect(jobManager.getJob(job.id)?.status).toBe('complete')); + expect(jobManager.getJob(job.id)?.error).toBeUndefined(); + // One fork for the run itself; a retry would be a second. + expect(forkMock).toHaveBeenCalledTimes(1); + expect(jobManager.getJob(job.id)?.retryCount).toBe(0); + }); + + it('still treats an exit with no terminal IPC as a crash worth retrying', async () => { + // The guard must not swallow real crashes: no `complete`/`error` was sent. + H.settledDir = H.STORAGE_PATH; + + const job = jobManager.createJob({ repoPath: REPO_PATH }); + launcher()(job, REPO_PATH, {}); + + child.emit('exit', 1); + + // The first retry is scheduled on a 1s backoff, so this needs more than + // vi.waitFor's default budget. + await vi.waitFor(() => expect(forkMock).toHaveBeenCalledTimes(2), { timeout: 4_000 }); + expect(jobManager.getJob(job.id)?.retryCount).toBe(1); + }); +}); diff --git a/gitnexus/test/unit/analyze-launch-collapse.test.ts b/gitnexus/test/unit/analyze-launch-collapse.test.ts index a32cf530e..226cde9cc 100644 --- a/gitnexus/test/unit/analyze-launch-collapse.test.ts +++ b/gitnexus/test/unit/analyze-launch-collapse.test.ts @@ -170,6 +170,50 @@ describe('createLaunchAnalysisWorker — collapsed index is never published', () ); }); + it('forwards the index-branch selector to the worker', () => { + const launch = createLaunchAnalysisWorker({ + jobManager, + backend: { init: backendInit }, + acquireRepoLock: () => null, + releaseRepoLock: () => {}, + closeDbHandle, + }); + const job = jobManager.createJob({ repoPath: REPO_PATH }); + + launch(job, REPO_PATH, { branch: 'development' }); + + // `StartMessage.options` is typed as `AnalyzeOptions`, so this key IS + // `AnalyzeOptions.branch` — the field `resolveWriteTarget` reads to choose + // the run's storage slot. (It does not always mean a `branches//` + // sub-slot: `resolveBranchPlacement` keeps the flat slot when that slot has + // no owner, or when its owner is already this label.) A rename breaks this + // test. + expect(child.send).toHaveBeenCalledWith( + expect.objectContaining({ + options: expect.objectContaining({ branch: 'development' }), + }), + ); + }); + + it('omits branch entirely when the caller did not select one', () => { + const launch = createLaunchAnalysisWorker({ + jobManager, + backend: { init: backendInit }, + acquireRepoLock: () => null, + releaseRepoLock: () => {}, + closeDbHandle, + }); + const job = jobManager.createJob({ repoPath: REPO_PATH }); + + launch(job, REPO_PATH, {}); + + // Not merely undefined: absent. `AnalyzeOptions.branch === undefined` is the + // documented signal for "target the flat workspace slot", so sending the key + // with an undefined value must not become the way that default is expressed. + const sent = child.send.mock.calls.at(0)?.[0] as { options: Record }; + expect(Object.hasOwn(sent.options, 'branch')).toBe(false); + }); + afterEach(() => { jobManager.dispose(); vi.restoreAllMocks(); diff --git a/gitnexus/test/unit/git-clone.test.ts b/gitnexus/test/unit/git-clone.test.ts index 715955d74..5b6ad1701 100644 --- a/gitnexus/test/unit/git-clone.test.ts +++ b/gitnexus/test/unit/git-clone.test.ts @@ -15,6 +15,7 @@ vi.mock('../../src/core/logger.js', () => ({ })); import { + analyzeCloneOptions, extractRepoName, extractWebRepoName, getCloneDir, @@ -671,6 +672,359 @@ describe('git-clone', () => { } }); + // `assertRemoteMatchesRequestedUrl` runs REAL git (it is not injectable), so + // these fixtures are real repositories with a matching origin; only the + // branch logic under test is driven through `runGitForTest`. + const REMOTE = 'https://github.com/owner/repo.git'; + const makeExistingClone = async (root: string, branch: string) => { + const target = path.join(root, 'repo'); + await fs.mkdir(target, { recursive: true }); + await runGit(['init', `--initial-branch=${branch}`], target); + await runGit(['remote', 'add', 'origin', REMOTE], target); + return target; + }; + + it('re-indexing the SAME pinned branch fetches via a safe refspec instead of refusing a dirty tree', async () => { + // Analyze writes AGENTS.md / CLAUDE.md / .claude/ into the clone, so the + // tree is dirty from its own first run. Routing a same-branch request + // through the checkout path made every pinned RE-index fail asking for + // `overwrite_local_changes` — a flag the route will not pass because it + // would `git clean --force -d` the directory (#3199 review). + const root = await mkControlledRoot('gitnexus-controlled-root-'); + const calls: string[][] = []; + try { + const target = await makeExistingClone(root, 'develop'); + const runGitForTest = vi.fn(async (args: string[]) => { + calls.push(args); + if (args[0] === 'rev-parse') return 'develop\n'; + if (args[0] === 'status') return ' M AGENTS.md\n?? .claude/\n'; // dirty, as analyze leaves it + return ''; + }); + await cloneOrPull(REMOTE, target, undefined, { + allowedCloneRoot: root, + expectedRepoName: 'repo', + branch: 'develop', + runGitForTest, + }); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + + const verbs = calls.map((c) => c[0]); + expect(calls).toContainEqual([ + 'fetch', + '--depth', + '1', + 'origin', + '+refs/heads/develop:refs/remotes/origin/develop', + ]); + expect(calls).toContainEqual(['checkout', '-B', 'develop', 'origin/develop']); + // Never a raw `origin ` pull — `+develop` would be a force-fetch. + expect( + calls.some((c) => c[0] === 'pull' && c.includes('origin') && c.includes('develop')), + ).toBe(false); + expect(verbs).not.toContain('status'); // so the dirty check never ran + expect(calls.some((c) => c[0] === 'merge')).toBe(false); + // Must not fall back to a bare `git pull --ff-only` — that follows + // `branch..merge`, which is not verified (only origin.url is). + expect(calls.some((c) => c[0] === 'pull' && c.length === 2)).toBe(false); + }); + + it('same-branch argv uses +refs/heads/develop:refs/remotes/origin/develop, never raw develop as a pull dest', async () => { + const root = await mkControlledRoot('gitnexus-controlled-root-'); + const calls: string[][] = []; + try { + const target = await makeExistingClone(root, 'develop'); + const runGitForTest = vi.fn(async (args: string[]) => { + calls.push(args); + if (args[0] === 'rev-parse' && args[1] === '--abbrev-ref') return 'develop\n'; + return ''; + }); + await cloneOrPull(REMOTE, target, undefined, { + allowedCloneRoot: root, + expectedRepoName: 'repo', + branch: 'develop', + runGitForTest, + }); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + + const fetchCall = calls.find((c) => c[0] === 'fetch'); + expect(fetchCall).toEqual([ + 'fetch', + '--depth', + '1', + 'origin', + '+refs/heads/develop:refs/remotes/origin/develop', + ]); + expect(fetchCall?.[4]).not.toBe('develop'); + expect(calls.some((c) => c[0] === 'pull' && c[3] === 'develop')).toBe(false); + }); + + it('does not treat a leading-plus branch as a force-fetch pull refspec', async () => { + // If `+develop` somehow reached cloneOrPull, the heads/ mapping keeps + // the `+` inside the ref name. It is not git's force-fetch prefix. + const root = await mkControlledRoot('gitnexus-controlled-root-'); + const calls: string[][] = []; + try { + const target = await makeExistingClone(root, 'main'); + const runGitForTest = vi.fn(async (args: string[]) => { + calls.push(args); + if (args[0] === 'rev-parse' && args[1] === '--abbrev-ref') return '+develop\n'; + return ''; + }); + await cloneOrPull(REMOTE, target, undefined, { + allowedCloneRoot: root, + expectedRepoName: 'repo', + branch: '+develop', + runGitForTest, + }); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + + expect(calls).toContainEqual([ + 'fetch', + '--depth', + '1', + 'origin', + '+refs/heads/+develop:refs/remotes/origin/+develop', + ]); + expect( + calls.some((c) => c[0] === 'pull' && c.some((a) => a === '+develop' || a.startsWith('+'))), + ).toBe(false); + // Force prefix is on the mapping, not a force-update of `develop`. + expect( + calls.some( + (c) => c[0] === 'fetch' && c.includes('+refs/heads/develop:refs/remotes/origin/develop'), + ), + ).toBe(false); + }); + + it('restores a dirty AGENTS.md on the same branch without taking the switch refuse path', async () => { + const root = await mkControlledRoot('gitnexus-controlled-root-'); + const calls: string[][] = []; + try { + const target = await makeExistingClone(root, 'develop'); + const runGitForTest = vi.fn(async (args: string[]) => { + calls.push(args); + if (args[0] === 'rev-parse' && args[1] === '--abbrev-ref') return 'develop\n'; + if (args[0] === 'ls-files' && args.includes('./AGENTS.md')) return 'AGENTS.md\n'; + if (args[0] === 'status') return ' M AGENTS.md\n?? .claude/\n'; + return ''; + }); + await expect( + cloneOrPull(REMOTE, target, undefined, { + allowedCloneRoot: root, + expectedRepoName: 'repo', + branch: 'develop', + runGitForTest, + }), + ).resolves.toBe(target); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + + expect(calls).toContainEqual(['ls-files', '--', './AGENTS.md']); + expect(calls).toContainEqual(['checkout', 'HEAD', '--', './AGENTS.md']); + expect(calls).toContainEqual([ + 'clean', + '-fdx', + '--', + './AGENTS.md', + './CLAUDE.md', + './.claude', + ]); + expect(calls.some((c) => c[0] === 'status')).toBe(false); + expect(calls).toContainEqual(['checkout', '-B', 'develop', 'origin/develop']); + expect(calls.some((c) => c[0] === 'clean' && c.includes('/.gitnexus'))).toBe(false); + }); + + it('removes untracked AGENTS.md on the same branch so merge is not blocked', async () => { + const root = await mkControlledRoot('gitnexus-controlled-root-'); + const calls: string[][] = []; + try { + const target = await makeExistingClone(root, 'develop'); + const runGitForTest = vi.fn(async (args: string[]) => { + calls.push(args); + if (args[0] === 'rev-parse' && args[1] === '--abbrev-ref') return 'develop\n'; + if (args[0] === 'ls-files') return ''; // untracked analyze output + return ''; + }); + await cloneOrPull(REMOTE, target, undefined, { + allowedCloneRoot: root, + expectedRepoName: 'repo', + branch: 'develop', + runGitForTest, + }); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + + expect(calls).toContainEqual([ + 'clean', + '-fdx', + '--', + './AGENTS.md', + './CLAUDE.md', + './.claude', + ]); + expect(calls.some((c) => c[0] === 'checkout' && c.includes('HEAD'))).toBe(false); + expect(calls).toContainEqual(['checkout', '-B', 'develop', 'origin/develop']); + }); + + it('still switches — and still refuses a dirty tree — for a DIFFERENT branch', async () => { + // The refusal must survive where it matters: a real switch can discard + // local work, so the fast path above must not weaken it. + const root = await mkControlledRoot('gitnexus-controlled-root-'); + try { + const target = await makeExistingClone(root, 'main'); + const runGitForTest = vi.fn(async (args: string[]) => { + if (args[0] === 'rev-parse') return 'main\n'; // on a different branch + if (args[0] === 'status') return ' M src/index.ts\n'; + return ''; + }); + await expect( + cloneOrPull(REMOTE, target, undefined, { + allowedCloneRoot: root, + expectedRepoName: 'repo', + branch: 'develop', + runGitForTest, + }), + ).rejects.toThrow(/local changes detected/); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + }); + + it('treats a detached HEAD as needing the checkout path', async () => { + // `rev-parse --abbrev-ref HEAD` reports `HEAD` when detached. A SHA that + // does not match the requested ref is a real switch, not "already there". + const root = await mkControlledRoot('gitnexus-controlled-root-'); + const calls: string[][] = []; + try { + const target = await makeExistingClone(root, 'main'); + const runGitForTest = vi.fn(async (args: string[]) => { + calls.push(args); + if (args[0] === 'rev-parse' && args[1] === '--abbrev-ref') return 'HEAD\n'; + if (args[0] === 'rev-parse' && args[1] === 'HEAD') + return 'aaa111aaa111aaa111aaa111aaa111aaa111aaa1\n'; + if (args[0] === 'rev-parse') return 'bbb222bbb222bbb222bbb222bbb222bbb222bbb2\n'; + if (args[0] === 'status') return ''; // clean, so the checkout proceeds + return ''; + }); + await cloneOrPull(REMOTE, target, undefined, { + allowedCloneRoot: root, + expectedRepoName: 'repo', + branch: 'develop', + runGitForTest, + }); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + expect(calls.map((c) => c[0])).toContain('checkout'); + expect(calls.some((c) => c[0] === 'checkout' && c.includes('-B'))).toBe(true); + }); + + it('does not switch or fetch when a detached HEAD SHA matches the requested ref', async () => { + // Tag / SHA pin: already at the commit. Fetching refs/heads/ would + // follow a same-named branch past the pin (#3199 review). + const root = await mkControlledRoot('gitnexus-controlled-root-'); + const calls: string[][] = []; + const sha = 'aaa111aaa111aaa111aaa111aaa111aaa111aaa1'; + try { + const target = await makeExistingClone(root, 'main'); + const runGitForTest = vi.fn(async (args: string[]) => { + calls.push(args); + if (args[0] === 'rev-parse' && args[1] === '--abbrev-ref') return 'HEAD\n'; + if (args[0] === 'rev-parse') return `${sha}\n`; + if (args[0] === 'status') return ' M AGENTS.md\n'; + if (args[0] === 'ls-files' && args.includes('./AGENTS.md')) return 'AGENTS.md\n'; + return ''; + }); + await cloneOrPull(REMOTE, target, undefined, { + allowedCloneRoot: root, + expectedRepoName: 'repo', + branch: 'develop', + runGitForTest, + }); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + + expect(calls.some((c) => c[0] === 'status')).toBe(false); + expect(calls.some((c) => c[0] === 'checkout' && c.includes('-B'))).toBe(false); + expect(calls).toContainEqual(['checkout', 'HEAD', '--', './AGENTS.md']); + expect(calls.map((c) => c[0])).not.toContain('fetch'); + expect(calls.map((c) => c[0])).not.toContain('merge'); + }); + + it('peels an annotated tag so a tag pin is not treated as a switch', async () => { + // `rev-parse v1.0` is the tag object; HEAD is the peeled commit. Without + // `^{commit}` the SHA match misses and re-index porcelain-refuses. + const root = await mkControlledRoot('gitnexus-controlled-root-'); + const calls: string[][] = []; + const commit = 'aaa111aaa111aaa111aaa111aaa111aaa111aaa1'; + const tagObject = 'cccccccccccccccccccccccccccccccccccccccc'; + try { + const target = await makeExistingClone(root, 'main'); + const runGitForTest = vi.fn(async (args: string[]) => { + calls.push(args); + if (args[0] === 'rev-parse' && args[1] === '--abbrev-ref') return 'HEAD\n'; + if (args[0] === 'rev-parse' && args[1] === 'HEAD') return `${commit}\n`; + if (args[0] === 'rev-parse' && args[1] === 'v1.0^{commit}') return `${commit}\n`; + if (args[0] === 'rev-parse' && args[1] === 'v1.0') return `${tagObject}\n`; + if (args[0] === 'rev-parse') throw new Error('unknown ref'); + if (args[0] === 'status') return ' M AGENTS.md\n'; + return ''; + }); + await cloneOrPull(REMOTE, target, undefined, { + allowedCloneRoot: root, + expectedRepoName: 'repo', + branch: 'v1.0', + runGitForTest, + }); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + + expect(calls).toContainEqual(['rev-parse', 'v1.0^{commit}']); + expect(calls.some((c) => c[0] === 'status')).toBe(false); + expect(calls.some((c) => c[0] === 'checkout' && c.includes('-B'))).toBe(false); + expect(calls.map((c) => c[0])).not.toContain('fetch'); + }); + + it('still refuses a dirty tree when a detached HEAD SHA does not match the requested ref', async () => { + const root = await mkControlledRoot('gitnexus-controlled-root-'); + const calls: string[][] = []; + try { + const target = await makeExistingClone(root, 'main'); + const runGitForTest = vi.fn(async (args: string[]) => { + calls.push(args); + if (args[0] === 'rev-parse' && args[1] === '--abbrev-ref') return 'HEAD\n'; + if (args[0] === 'rev-parse' && args[1] === 'HEAD') + return 'aaa111aaa111aaa111aaa111aaa111aaa111aaa1\n'; + if (args[0] === 'rev-parse') return 'bbb222bbb222bbb222bbb222bbb222bbb222bbb2\n'; + if (args[0] === 'status') return ' M src/index.ts\n'; + return ''; + }); + await expect( + cloneOrPull(REMOTE, target, undefined, { + allowedCloneRoot: root, + expectedRepoName: 'repo', + branch: 'develop', + runGitForTest, + }), + ).rejects.toThrow(/local changes detected/); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + + expect(calls.some((c) => c[0] === 'status')).toBe(true); + expect(calls.some((c) => c[0] === 'checkout')).toBe(false); + }); + it('allows auto-sync SSH SCP clone URLs with a per-repo timeout', async () => { const root = await mkControlledRoot('gitnexus-controlled-root-'); const target = path.join(root, 'repo'); @@ -1367,3 +1721,115 @@ describe('git-clone', () => { }); }); }); + +describe('getCloneDir — a branch-pinned analyze gets its own working tree', () => { + it('keeps the historic directory for an unpinned request', () => { + // Backward compatibility: existing installs must keep using the dir they have. + expect(getCloneDir('Hello-World')).toBe(getCloneDir('Hello-World', undefined)); + }); + + it('gives a pinned request a different directory from the unpinned one', () => { + // This separation is what stops (a) a pinned request failing on the dirty + // tree an earlier analyze left, and (b) a later unpinned request pulling on + // the branch a pin left checked out and indexing it as the default. + expect(getCloneDir('Hello-World', 'development')).not.toBe(getCloneDir('Hello-World')); + }); + + it('gives two different branches two different directories', () => { + expect(getCloneDir('Hello-World', 'development')).not.toBe( + getCloneDir('Hello-World', 'release/1.2'), + ); + }); + + it('is stable for the same branch', () => { + expect(getCloneDir('Hello-World', 'release/1.2')).toBe( + getCloneDir('Hello-World', 'release/1.2'), + ); + }); + + it('keeps a slash-bearing ref inside a single path segment under the clone root', () => { + // `release/1.2` must not become a nested directory, or the containment + // guarantees around CLONE_ROOT would be reasoning about a different path. + const dir = getCloneDir('Hello-World', 'release/1.2'); + const base = getCloneDir('Hello-World'); + expect(path.dirname(dir)).toBe(path.dirname(base)); + expect(path.basename(dir)).not.toContain('/'); + }); + + it('round-trips its own directory name, which is how DELETE re-derives it', () => { + // DELETE /api/repo calls getCloneDir(entry.name); the pinned repo is + // registered under this basename, so the two must agree. + const dir = getCloneDir('Hello-World', 'development'); + expect(getCloneDir(path.basename(dir))).toBe(dir); + }); + + it('pinned basename differs from the GitHub stem (mid-job repoName / registerRepo)', () => { + // POST /api/analyze must set job.repoName and registryName to + // path.basename(targetPath), not extractWebRepoName(url). The stem is + // only getCloneDir's first argument (#3199 review). + const stem = 'Hello-World'; + const basename = path.basename(getCloneDir(stem, 'development')); + expect(basename).not.toBe(stem); + expect(basename.startsWith(`${stem}__`)).toBe(true); + }); + + it('keeps the directory name inside the 255-byte filesystem limit', () => { + // validateBranchName allows a 255-char ref and branchSlug appends 9 more, + // so the naive `__` reached 267 and the clone could not create + // its target directory. + const longBranch = 'b'.repeat(255); + const base = path.basename(getCloneDir('Hello-World', longBranch)); + expect(base.length).toBeLessThanOrEqual(255); + }); + + it('still separates two long branches that share a prefix', () => { + // Trimming keeps the hash, which is a digest of the FULL ref — otherwise + // two long branches would collapse onto one directory and silently share + // an index. + const a = 'b'.repeat(250) + 'one'; + const b = 'b'.repeat(250) + 'two'; + expect(getCloneDir('Hello-World', a)).not.toBe(getCloneDir('Hello-World', b)); + expect(path.basename(getCloneDir('Hello-World', a)).length).toBeLessThanOrEqual(255); + }); + + it('round-trips a trimmed directory name too', () => { + const dir = getCloneDir('Hello-World', 'b'.repeat(255)); + expect(getCloneDir(path.basename(dir))).toBe(dir); + }); + + it('still rejects a traversal attempt in the repo name', () => { + expect(() => getCloneDir('..', 'development')).toThrow(/Invalid repository name/); + expect(() => getCloneDir('a/b', 'development')).toThrow(/Invalid repository name/); + }); +}); + +describe('analyzeCloneOptions — the /api/analyze glue for #3198', () => { + // The route passes the result straight to `cloneOrPull`. Inline, a regression + // that dropped `branch` for token-less URLs left every other test green while + // silently reindexing the default branch — so each combination is pinned. + it('returns undefined when neither a token nor a branch is supplied', () => { + expect(analyzeCloneOptions(undefined, undefined)).toBeUndefined(); + }); + + it('carries a token on its own', () => { + expect(analyzeCloneOptions('ghp_token', undefined)).toEqual({ token: 'ghp_token' }); + }); + + it('carries a branch on its own — the public-repo case', () => { + // The regression that would reopen #3198: a branch requested for a public + // URL must still reach cloneOrPull, with no token in play. + expect(analyzeCloneOptions(undefined, 'development')).toEqual({ branch: 'development' }); + }); + + it('carries both together', () => { + expect(analyzeCloneOptions('ghp_token', 'development')).toEqual({ + token: 'ghp_token', + branch: 'development', + }); + }); + + it('treats an empty branch as absent rather than sending an empty ref', () => { + expect(analyzeCloneOptions('ghp_token', '')).toEqual({ token: 'ghp_token' }); + expect(analyzeCloneOptions('', '')).toBeUndefined(); + }); +}); diff --git a/gitnexus/test/unit/git-ref.test.ts b/gitnexus/test/unit/git-ref.test.ts new file mode 100644 index 000000000..af2492926 --- /dev/null +++ b/gitnexus/test/unit/git-ref.test.ts @@ -0,0 +1,15 @@ +import { describe, expect, it } from 'vitest'; +import { InvalidBranchError, validateBranchName } from '../../src/core/git-ref.js'; + +describe('core/git-ref', () => { + it('throws InvalidBranchError with name "InvalidBranchError"', () => { + expect(() => validateBranchName('HEAD', 'src')).toThrow(InvalidBranchError); + try { + validateBranchName('HEAD', 'src'); + throw new Error('expected InvalidBranchError'); + } catch (err) { + expect(err).toBeInstanceOf(InvalidBranchError); + expect((err as Error).name).toBe('InvalidBranchError'); + } + }); +});