From 8ddab9aed5dabbeb5b1e4a400d083cab53a200c5 Mon Sep 17 00:00:00 2001 From: Ankit Verma Date: Tue, 8 Sep 2026 19:45:45 +0530 Subject: [PATCH] feat(api): honor branch on POST /api/analyze (#3199) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(api): honor branch on POST /api/analyze The serve route accepted a `branch` field in the request body, returned 202 and reported the job `complete` — while indexing the remote's default branch. Express drops unknown body fields, so the caller got no error and no warning; the only way to notice was to inspect the checked-out clone. Both ends of the plumbing already existed: CloneOrPullOptions.branch is honored by cloneOrPull, and AnalyzeOptions.branch already drives resolveBranchPlacement. Only the HTTP layer was missing, so this wires `branch` from the route through cloneOrPull and LaunchOptions into the worker's AnalyzeOptions. StartMessage.options is already typed as AnalyzeOptions, so the IPC protocol is unchanged. Validation reuses validateBranchName — the same function backing the CLI's `--branch` — so both entry points accept exactly the same refs and a malformed value is rejected with 400 before it can reach git. Co-Authored-By: Claude Opus 5 * fix(api): make branch part of job identity and complete ref validation Addresses the review on #3199. 1. Job dedup ignored `branch`, so a request for branch B while branch A was in flight was answered with A's job and a 202. The caller would read that as "B is indexed" — the same silent wrong-branch outcome honoring `branch` was meant to remove. Branch is now part of the dedup identity; a different-branch request falls through to the single-slot guard and gets a truthful 409 instead. 2. validateBranchName implemented only a subset of git's ref rules, so `feature.lock`, `/feature`, `feature/`, `feature//next`, `@`, `@{` and dot-prefixed components passed validation and failed later in the git subprocess — a 202 plus a background failure rather than the advertised 400. The remaining `git check-ref-format` rules are now enforced at the same chokepoint, which fixes the CLI and `.gitnexusrc` paths too. No branch git can create is affected. 3. The LaunchOptions doc claimed an explicit branch always pins `branches//`. resolveBranchPlacement keeps the run on the flat slot when that slot has no owner, or when its owner is already this label. Comment and CHANGELOG corrected. Co-Authored-By: Claude Opus 5 * docs(server): describe cloneOrPull's branch path in its contract comment The header comment predated `options.branch` and still said an existing clone is only ever `git pull --ff-only`. The implementation has a second path: with a branch it fetches that ref and runs `checkout -B origin/`, so the requested branch — not the one already checked out — ends up in the working tree. The stale comment is actively misleading: a reviewer reading it concludes that requesting a branch on an existing clone silently analyzes the default branch, which is not what happens. Co-Authored-By: Claude Opus 5 * docs(server): scope the origin check to the existing-clone path My previous comment said remote.origin is verified "in both cases", which is wrong: assertRemoteMatchesRequestedUrl runs inside `if (exists)`, so a fresh clone has no origin to check. Restructured around whether targetDir exists, which is what actually selects the behavior. Co-Authored-By: Claude Opus 5 * test(server): cover branch job identity in JobManager's own suite The branch dedup tests were sitting in analyze-api.test.ts, but they exercise JobManager directly, so they belong beside the existing "returns existing job for same repoUrl when active" case in analyze-job.test.ts. Moved, and extended to cover the callers that omit branch entirely — the upload route, the embed manager and the existing tests — which compare undefined === undefined and are unaffected. Also pins that branch survives the whole clone -> analyze -> terminal update sequence, since it is now part of dedup identity and must not drift mid-flight. Co-Authored-By: Claude Opus 5 * fix(api): give a pinned branch its own clone and settle the slot it wrote Addresses the review on #3199. Per-branch clone directories (review option 2). One checkout per repo made `branch` one-shot: after any analyze the tree is dirty with generated AGENTS.md / CLAUDE.md / .claude/, so a pinned request 202'd and then died on cloneOrPull's porcelain refusal. Worse, a later request that OMITTED `branch` pulled whatever branch the last pin left checked out and indexed it as the default — silent wrong content, the same class as #3198 one request later. A pinned run now clones into `__`, so the two requests no longer share a tree. The pinned clone registers under its directory name, because both dirs share an origin and the inferred name would otherwise collide; that name re-derives through getCloneDir, so DELETE still finds it. Finalization gate. registerRepo always records the flat `.gitnexus`, but a pinned run whose label differs from the flat slot's owner writes `branches//`. The gate probed the flat path regardless, so it never settled, and the worker's normal exit 0 — sent ~500ms after `complete`, while the job is deliberately still non-terminal — was classified as a crash and a successful analysis was retried three times and failed. The gate now follows the placement the worker reports (isPrimaryBranch, added to the IPC allowlist under the rule that module already documents), and an exit after a terminal IPC counts as winding down, not dying. Reported by the maintainer and reproduced independently by @azizur100389. analyzeCloneOptions extracted so the token/branch combination is asserted. Inline, the branch-only case — a public URL with no token — was untested, and dropping it there would silently reindex the default branch while every other test stayed green. CHANGELOG: the previous entry claimed the newly-400'd payloads "would have failed at git", which is true of CLI --branch but wrong for HTTP, where they succeeded on the default branch. Documented as an explicit behavior change. Co-Authored-By: Claude Opus 5 * fix(server): bound the branch clone-dir name to one path component `validateBranchName` allows a 255-character ref and `branchSlug` appends a dash plus 8 hash characters, so `__` reached 267 — past the 255-byte component limit on ext4/APFS/NTFS. The clone would then fail to create its target directory, which the character-only regex could not catch. Only the readable half is trimmed. The hash is a digest of the full ref and is always kept, so two long branches sharing a prefix still resolve to different directories rather than silently sharing an index. `branchSlug` itself is untouched: the per-branch index slots already use those names on disk, and shortening them there would orphan existing indexes. Also corrects two comments: the forwarding test claimed a branch selector always pins `branches//` (it keeps the flat slot when that slot has no owner or already owns the label), and the web client's `branch` doc said omitting it means the remote default — true for a `url` request, but a `path` request is never cloned and indexes whatever that tree has checked out. Co-Authored-By: Claude Opus 5 * fix(server): bound the branch clone-dir name and unblock same-branch re-index Two defects found by testing this branch end to end, plus the review nits. 1. Path length. `validateBranchName` allows a 255-character ref and `branchSlug` appends a dash plus 8 hash characters, so `__` reached 267 — past the 255-byte component limit on ext4/APFS/NTFS, and the clone could not create its directory. Only the readable half is trimmed; the hash is a digest of the full ref and is always kept, so two long branches sharing a prefix still get separate directories. `branchSlug` itself is untouched — the per-branch index slots already use those names on disk and shortening them there would orphan existing indexes. 2. Same-branch re-index. Per-branch clone dirs stopped branches from contaminating each other, but a REPEAT pin still failed: analyze writes AGENTS.md / CLAUDE.md / .claude/ into the clone, so the second pinned run met its own dirt at the porcelain check and asked for `overwrite_local_changes`. When HEAD already matches the requested branch there is nothing to switch, so the run now takes the same `pull --ff-only` path an unpinned request takes — review option (1), alongside (2). The refusal is untouched where it matters: a real switch, or a detached HEAD, still goes through the checkout path and can still refuse. Also: repositions getCloneDir's JSDoc, which an inserted constant had orphaned; gates the pinned `registryName` on the same condition as the clone, so supplying both `url` and `path` no longer renames the operator's local repo; corrects a test comment that claimed a branch selector always pins `branches//`; and corrects the web client's `branch` doc, which said omitting it means the remote default — true for `url`, but a `path` request is never cloned. Co-Authored-By: Claude Opus 5 * docs: drop the CHANGELOG edits from this PR Requested in review. Checking the history, no feat/fix PR here touches gitnexus/CHANGELOG.md — the only recent commit on it is `chore: release v1.6.11`, and CONTRIBUTING says release notes are generated from the merged PR title via .github/release.yml. Hand-editing an [Unreleased] section from a feature branch was my mistake, not the project's convention. The behavior change it documented (branch: null / "" / non-string now 400 where they were previously dropped and the default branch indexed) is stated in the PR description instead, which is what feeds the release notes. Co-Authored-By: Claude Opus 5 * docs(server): align the clone/pull contract with the same-branch fast path Two comments I wrote went stale against my own later change. `cloneOrPull`'s header still said an existing clone with `options.branch` always fetches and runs `checkout -B`. Since the same-branch fast path landed that is only true when the branch actually differs; when it is already checked out the run takes `pull --ff-only` and no dirty-tree check applies. The header now splits on whether the branch differs, which is what the code branches on. The web client's `branch` doc said omitting it on a `url` request clones the remote default. That holds only when there is no clone yet — an existing unpinned clone is pulled on whatever branch it already has checked out. Comments only; no behavior change. Co-Authored-By: Claude Opus 5 * Address PR review feedback (#3199) Pin a same-branch re-index to `git pull --ff-only origin ` so the job cannot follow an unverified `branch..merge` while still skipping the dirty-tree refuse. Co-authored-by: Cursor * fix(server): close the #3199 holes on pinned analyze re-index Same-ref updates were a raw pull dest (force-fetch via +) or a shallow ff-merge that could not move, and a tag pin compared the tag-object SHA so re-index refused a dirty tree. Fetch the mapped remote-tracking ref, stay put on a peeled SHA match, restore only GitNexus overlays, skip the 60s settle on alreadyUpToDate, and keep branch validation in core so the HTTP route does not import the CLI. Co-authored-by: Cursor * chore(autofix): apply prettier + eslint fixes via /autofix command --------- Co-authored-by: Claude Opus 5 Co-authored-by: Gergő Magyar Co-authored-by: Gergo Magyar Co-authored-by: Cursor Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- gitnexus-web/src/services/backend-client.ts | 7 + gitnexus/src/cli/analyze-config.ts | 52 +- gitnexus/src/core/git-ref.ts | 128 +++++ gitnexus/src/server/analyze-job.ts | 26 +- gitnexus/src/server/analyze-launch.ts | 98 +++- gitnexus/src/server/analyze-worker-ipc.ts | 14 +- gitnexus/src/server/api.ts | 57 ++- gitnexus/src/server/git-clone.ts | 277 +++++++++-- .../server-analyze-branch-validation.test.ts | 210 ++++++++ gitnexus/test/unit/analyze-config.test.ts | 41 ++ gitnexus/test/unit/analyze-job.test.ts | 59 +++ .../unit/analyze-launch-branch-settle.test.ts | 238 +++++++++ .../test/unit/analyze-launch-collapse.test.ts | 44 ++ gitnexus/test/unit/git-clone.test.ts | 466 ++++++++++++++++++ gitnexus/test/unit/git-ref.test.ts | 15 + 15 files changed, 1628 insertions(+), 104 deletions(-) create mode 100644 gitnexus/src/core/git-ref.ts create mode 100644 gitnexus/test/integration/server-analyze-branch-validation.test.ts create mode 100644 gitnexus/test/unit/analyze-launch-branch-settle.test.ts create mode 100644 gitnexus/test/unit/git-ref.test.ts 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'); + } + }); +});