GitNexus/gitnexus/test/unit/git-ref.test.ts
Ankit Verma 8ddab9aed5
feat(api): honor branch on POST /api/analyze (#3199)
* 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 <noreply@anthropic.com>

* 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/<slug>/`. 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 <noreply@anthropic.com>

* 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 <branch> origin/<branch>`, 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 `<repo>__<branchSlug>`, 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/<slug>/`. 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 <noreply@anthropic.com>

* 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 `<repo>__<slug>` 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/<slug>/` (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 <noreply@anthropic.com>

* 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 `<repo>__<slug>`
   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/<slug>/`; 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* Address PR review feedback (#3199)

Pin a same-branch re-index to `git pull --ff-only origin <branch>` so the job cannot follow an unverified `branch.<name>.merge` while still skipping the dirty-tree refuse.

Co-authored-by: Cursor <cursoragent@cursor.com>

* 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 <cursoragent@cursor.com>

* chore(autofix): apply prettier + eslint fixes via /autofix command

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
2026-09-08 15:15:45 +01:00

15 lines
576 B
TypeScript

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');
}
});
});