diff --git a/CHANGELOG.md b/CHANGELOG.md index 367f0aa..15ab487 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,32 @@ version). ## [Unreleased] +### Added + +- `log`, `show`, and `check` results now include each commit's hash: + `hash` and `shortHash` fields in `--json` output, and the short hash + leading each entry in text output. Previously a result could be found + but not acted on, since nothing identified which commit it was. Parse + warnings on stderr name the commit's short hash too. +- `log --touches `: filters by the `touches` field, matching whole + path segments in both directions, so `src/auth` finds commits listing + `src/auth`, `src/auth/session.ts`, or the broader `src`, but never + `src/authz`. Leading `./` or `/` and trailing `/` are ignored. +- `agent-commit blame [-L [,]]`: runs `git blame` on + the given lines (the whole file without `-L`) and shows the + `Agent-Context` of each commit responsible, with the line ranges each + one owns, in pretty or `--json` form. Lines not committed yet are + counted on stderr instead of being attributed. + +### Fixed + +- A commit with an empty message (`git commit --allow-empty-message`) + no longer swallows the commit next to it. Records were split on a + double-NUL boundary, and an empty message produced two adjacent empty + fields that looked identical to it, so `log` dropped the neighboring + commit and misreported what remained as `MISPLACED`. The parser now + reads a fixed number of NUL-terminated fields per commit. + ## [0.1.0] - 2026-09-11 First published release. diff --git a/README.md b/README.md index 771e14c..366710f 100644 --- a/README.md +++ b/README.md @@ -56,7 +56,7 @@ Prose is exactly what agents already do for free, and it's genuinely fine for a ## What the CLI does today -Reading a single repo's history back already works today with plain git, no tool required (see `SPEC.md`, "Reading it back"). The CLI in this repo (`cli/`, published to npm) adds structured filtering on top: `agent-commit log --breaking`, `--grep` (case-insensitive substring match against `why`), `--refs` (exact match against an entry in `refs`, so it's only as reliable as the canonical form guidance in `SPEC.md`; a repo that mixes `INFRA-441` and `#441` for the same ticket will find neither with a query for the other, silently, no error), `--since`/`--until`, `agent-commit show ` for a single commit, both with a pretty-printed default view and a `--json` mode for machine consumption. No `--type` filter, on purpose, `type` isn't a field this format uses (see SPEC.md). `agent-commit check` reports, read-only, whether recent commits on the current branch have a usable `Agent-Context` trailer, distinguishing a trailer that was never written from one that exists in the message but wasn't recognized as a trailer (`MISPLACED`, see SPEC.md's squash-merge section for why that happens), useful right after a squash-merge to confirm the merge commit actually got one. `agent-commit merge-draft ` deterministically unions a branch's `touches`/`refs`/`breaking` and lists per-commit `why` candidates for a squash-merge trailer. `agent-commit init` writes the `AGENTS.md` block (see "How to use it" below) instead of an agent hand-transcribing it. `agent-commit --version` prints the installed version. Try it now with no install: `npx agent-commit log`. Building from source instead: `cd cli && npm install && npm run build && node dist/index.js log`. +Reading a single repo's history back already works today with plain git, no tool required (see `SPEC.md`, "Reading it back"). The CLI in this repo (`cli/`, published to npm) adds structured filtering on top: `agent-commit log --breaking`, `--grep` (case-insensitive substring match against `why`), `--refs` (exact match against an entry in `refs`, so it's only as reliable as the canonical form guidance in `SPEC.md`; a repo that mixes `INFRA-441` and `#441` for the same ticket will find neither with a query for the other, silently, no error), `--touches` (path match against `touches` on whole segments, in both directions: `--touches src/auth` finds commits listing `src/auth`, `src/auth/session.ts`, or the broader `src`, never `src/authz`), `--since`/`--until`, `agent-commit show ` for a single commit, both with a pretty-printed default view and a `--json` mode for machine consumption. Every result carries its commit hash (`hash` and `shortHash` in `--json`, the short hash leading each entry in pretty output), so anything found can be handed straight to `git show`. `agent-commit blame [-L [,]]` is the blame-time lookup described under "The problem" in one step: it runs `git blame` on those lines and prints the `Agent-Context` of each commit responsible, with the line ranges each one owns. No `--type` filter, on purpose, `type` isn't a field this format uses (see SPEC.md). `agent-commit check` reports, read-only, whether recent commits on the current branch have a usable `Agent-Context` trailer, distinguishing a trailer that was never written from one that exists in the message but wasn't recognized as a trailer (`MISPLACED`, see SPEC.md's squash-merge section for why that happens), useful right after a squash-merge to confirm the merge commit actually got one. `agent-commit merge-draft ` deterministically unions a branch's `touches`/`refs`/`breaking` and lists per-commit `why` candidates for a squash-merge trailer. `agent-commit init` writes the `AGENTS.md` block (see "How to use it" below) instead of an agent hand-transcribing it. `agent-commit --version` prints the installed version. Try it now with no install: `npx agent-commit log`. Building from source instead: `cd cli && npm install && npm run build && node dist/index.js log`. ## What's not built diff --git a/SPEC.md b/SPEC.md index 54741ba..6e7e686 100644 --- a/SPEC.md +++ b/SPEC.md @@ -128,4 +128,4 @@ Split the output on the NUL byte (`\0` in most languages, `tr '\0' '\n'` at a sh This already works today, verified against a real commit, not just described; see `README.md`, "What the CLI does today," for the working tool built on top of it. -The CLI's own internal format goes one step further than the two commands above: it also captures each commit's raw message body, specifically so it can tell a genuinely absent trailer apart from one that exists in the message but landed outside the position git recognizes as a trailer (see "The real trap: placement, not the merge strategy itself" above). That's a CLI capability, not a requirement of the format itself; the plain two-field commands here remain everything a reader needs with nothing installed. +The CLI's own internal format goes one step further than the two commands above: it also captures each commit's hash, so every result can be acted on, and its raw message body, specifically so it can tell a genuinely absent trailer apart from one that exists in the message but landed outside the position git recognizes as a trailer (see "The real trap: placement, not the merge strategy itself" above). That's a CLI capability, not a requirement of the format itself; the plain two-field commands here remain everything a reader needs with nothing installed. diff --git a/cli/README.md b/cli/README.md index 2c4e592..76d7978 100644 --- a/cli/README.md +++ b/cli/README.md @@ -38,15 +38,28 @@ target directory (the current directory if none is given), and links an existing, unrelated commit-message convention or an already-set-up repo without `--force`/`--update`. -`agent-commit log [--breaking] [--grep ] [--refs ] [--since ] [--until ] [--json]` +`agent-commit log [--breaking] [--grep ] [--refs ] [--touches ] [--since ] [--until ] [--json]` Filters commit history by `Agent-Context` fields. `--grep` is a case-insensitive substring match against `why`, not semantic search. `--refs` is an exact match, see the main repo's `SPEC.md` for why ref -form consistency matters. +form consistency matters. `--touches` matches whole path segments in +both directions: `src/auth` finds `src/auth`, `src/auth/session.ts`, +and the broader `src`, but never `src/authz`. `agent-commit show [--json]` Shows a single commit's `Agent-Context` trailer. +Every result from `log`, `show`, `blame`, and `check` includes the commit's hash: +`hash` (full) and `shortHash` (git's own abbreviation) in `--json` +output, and the short hash at the start of each entry in text output. + +`agent-commit blame [-L [,]] [--json]` +Shows the `Agent-Context` of each commit that last changed the given +lines (the whole file without `-L`), via `git blame`, with the line +ranges each commit owns. `-L` takes git's numeric form, 1-based and +inclusive: `-L 42` or `-L 40,60`. Lines not committed yet are counted +on stderr rather than attributed to anything. + `agent-commit check [--range ] [--count ] [--json]` Read-only, advisory report on whether recent commits have a usable trailer. Distinguishes a commit with no trailer at all from one where an diff --git a/cli/src/blame.ts b/cli/src/blame.ts new file mode 100644 index 0000000..036501d --- /dev/null +++ b/cli/src/blame.ts @@ -0,0 +1,139 @@ +import { formatPretty } from "./format.js"; +import { parseLog, runGit, runGitLog } from "./parser.js"; +import type { AgentCommitRecord, ParseWarning } from "./types.js"; + +/** An inclusive range of line numbers in the blamed file, as it currently stands. */ +export type LineRange = [start: number, end: number]; + +export interface BlameEntry { + /** Full hash of the commit that last changed these lines. */ + hash: string; + /** Every line attributed to this commit, compressed into ranges, in file order. */ + lines: LineRange[]; +} + +export interface BlameParseResult { + /** One entry per distinct commit, ordered by where its first line appears in the file. */ + entries: BlameEntry[]; + /** Lines changed in the working tree but not committed yet; git attributes them to an all-zero hash. */ + uncommittedLines: number; +} + +export interface BlameRecord extends AgentCommitRecord { + lines: LineRange[]; +} + +export interface BlameResult { + records: BlameRecord[]; + uncommittedLines: number; + warnings: ParseWarning[]; +} + +/** + * Header lines in `git blame --porcelain` output: the commit hash, the + * line number in that commit's version, the line number in the file as it + * stands now, and (on the first line of a group only) the group's size. + * Verified against real git output. Every other line is either commit + * metadata starting with a lowercase keyword (`author`, `summary`, + * `previous`, `boundary`, ...) or the line's own content prefixed with a + * tab, so neither can be mistaken for a header. Accepts SHA-1 and SHA-256 + * object names. + */ +const PORCELAIN_HEADER = /^([0-9a-f]{40}|[0-9a-f]{64}) \d+ (\d+)(?: \d+)?$/; +const UNCOMMITTED_HASH = /^0+$/; + +function toRanges(lineNumbers: number[]): LineRange[] { + const sorted = [...lineNumbers].sort((a, b) => a - b); + const ranges: LineRange[] = []; + for (const line of sorted) { + const last = ranges[ranges.length - 1]; + if (last && line === last[1] + 1) { + last[1] = line; + } else { + ranges.push([line, line]); + } + } + return ranges; +} + +/** Parses `git blame --porcelain` output into the distinct commits responsible for the blamed lines. */ +export function parseBlamePorcelain(raw: string): BlameParseResult { + const linesByHash = new Map(); + let uncommittedLines = 0; + + for (const line of raw.split("\n")) { + const match = PORCELAIN_HEADER.exec(line); + if (!match) continue; + const [, hash, finalLine] = match; + if (UNCOMMITTED_HASH.test(hash)) { + uncommittedLines += 1; + continue; + } + const existing = linesByHash.get(hash); + if (existing) { + existing.push(Number(finalLine)); + } else { + linesByHash.set(hash, [Number(finalLine)]); + } + } + + const entries = [...linesByHash].map(([hash, lineNumbers]) => ({ hash, lines: toRanges(lineNumbers) })); + entries.sort((a, b) => a.lines[0][0] - b.lines[0][0]); + return { entries, uncommittedLines }; +} + +/** + * Parses a `-L` value: `` or `,`, 1-based and + * inclusive, the numeric subset of git blame's own `-L` syntax. git's + * regex and function-name forms are deliberately not accepted, so every + * value that reaches git here is known to be plain numbers. + */ +export function parseLineRange(value: string): LineRange | undefined { + const match = /^(\d+)(?:,(\d+))?$/.exec(value.trim()); + if (!match) return undefined; + const start = Number(match[1]); + const end = match[2] === undefined ? start : Number(match[2]); + if (start < 1 || end < start) return undefined; + return [start, end]; +} + +/** + * Finds the commits that last changed the given lines of a file (the + * whole file if no range is given), then reads each one's Agent-Context. + * Blames the working tree, like plain `git blame`, so lines not committed + * yet are counted rather than attributed to anything. + */ +export function blame(file: string, range?: LineRange, cwd?: string): BlameResult { + const blameArgs = ["blame", "--porcelain"]; + if (range) blameArgs.push("-L", `${range[0]},${range[1]}`); + // `--` ends option parsing, so a path starting with "-" is read as a path. + blameArgs.push("--", file); + + const { entries, uncommittedLines } = parseBlamePorcelain(runGit(blameArgs, cwd)); + if (entries.length === 0) return { records: [], uncommittedLines, warnings: [] }; + + // --no-walk=unsorted reads exactly these commits, in the order given. + const { records, warnings } = parseLog( + runGitLog(["--no-walk=unsorted", ...entries.map((entry) => entry.hash)], cwd), + ); + const linesByHash = new Map(entries.map((entry) => [entry.hash, entry.lines])); + return { + records: records.map((record) => ({ ...record, lines: linesByHash.get(record.hash) ?? [] })), + uncommittedLines, + warnings, + }; +} + +function formatRanges(ranges: LineRange[]): string { + return ranges.map(([start, end]) => (start === end ? `${start}` : `${start}-${end}`)).join(", "); +} + +/** Renders blame results like `log`'s pretty output, with each commit's line ranges right under its subject. */ +export function formatBlamePretty(records: BlameRecord[]): string { + return records + .map((record) => { + const [head, ...rest] = formatPretty(record).split("\n"); + return [head, ` lines: ${formatRanges(record.lines)}`, ...rest].join("\n"); + }) + .join("\n\n"); +} diff --git a/cli/src/check.ts b/cli/src/check.ts index 972ac68..9576359 100644 --- a/cli/src/check.ts +++ b/cli/src/check.ts @@ -4,6 +4,8 @@ import type { AgentCommitRecord } from "./types.js"; export type CheckStatus = "ok" | "miss" | "misplaced"; export interface CheckResult { + hash: string; + shortHash: string; subject: string; status: CheckStatus; } @@ -20,8 +22,9 @@ export interface CheckResult { */ export function checkRecords(records: AgentCommitRecord[]): CheckResult[] { return records.map((record) => { + const base = { hash: record.hash, shortHash: record.shortHash, subject: record.subject }; if (record.misplaced) { - return { subject: record.subject, status: "misplaced" }; + return { ...base, status: "misplaced" }; } // "ok" requires an object-shaped trailer, the same bar query.ts's // structured filters use (isAgentContextObject). SPEC.md allows any @@ -32,6 +35,6 @@ export function checkRecords(records: AgentCommitRecord[]): CheckResult[] { // a false green light query.ts could never act on. Malformed and // ambiguous trailers already fall through to miss below. const ok = isAgentContextObject(record.context) && !record.malformed && !record.ambiguous; - return { subject: record.subject, status: ok ? "ok" : "miss" }; + return { ...base, status: ok ? "ok" : "miss" }; }); } diff --git a/cli/src/format.ts b/cli/src/format.ts index 611e179..0a8d071 100644 --- a/cli/src/format.ts +++ b/cli/src/format.ts @@ -115,12 +115,16 @@ function formatContextFields(context: Record): string[] { } /** - * Renders one record as human-readable text. Field order favors SPEC.md's + * Renders one record as human-readable text: short hash and subject, then + * the trailer's fields. Field order favors SPEC.md's * common fields but never drops an unrecognized key, the format has no * required schema, so the renderer can't assume one either. */ export function formatPretty(record: AgentCommitRecord): string { - const lines = [sanitizeForTerminal(record.subject)]; + // Hash first, `git log --oneline` style, so every result can be handed + // straight to `git show` or `agent-commit show`. Hashes are git's own + // hex output, never commit content, so they need no sanitizing. + const lines = [`${record.shortHash} ${sanitizeForTerminal(record.subject)}`]; if (record.malformed) { lines.push(" (malformed Agent-Context trailer, could not parse as JSON)"); diff --git a/cli/src/index.ts b/cli/src/index.ts index 6100330..2ada10e 100644 --- a/cli/src/index.ts +++ b/cli/src/index.ts @@ -2,6 +2,7 @@ import { existsSync, readFileSync, statSync } from "node:fs"; import { fileURLToPath } from "node:url"; import { Command } from "commander"; +import { blame, formatBlamePretty, parseLineRange } from "./blame.js"; import { checkRecords } from "./check.js"; import { formatJson, formatPrettyList, sanitizeForTerminal } from "./format.js"; import { formatInitResult, needsAction, runInit } from "./init.js"; @@ -59,7 +60,7 @@ function runFatal(fn: () => T): T { function printWarnings(warnings: ParseWarning[]): void { for (const warning of warnings) { process.stderr.write( - `agent-commit: warning: ${warning.reason} on commit "${sanitizeForTerminal(warning.subject)}", skipped\n`, + `agent-commit: warning: ${warning.reason} on commit ${warning.shortHash} "${sanitizeForTerminal(warning.subject)}", skipped\n`, ); } } @@ -91,6 +92,10 @@ program .option("--breaking", "filter to breaking: true only") .option("--grep ", "case-insensitive substring match against why") .option("--refs ", "filter to commits whose refs array contains this exact value") + .option( + "--touches ", + "filter to commits whose touches overlaps this path, on whole path segments (src/auth matches src/auth/session.ts and src)", + ) .option("--since ", "passed through to git log --since") .option("--until ", "passed through to git log --until") .option("--json", "output machine-readable JSON instead of pretty text") @@ -98,6 +103,7 @@ program breaking?: boolean; grep?: string; refs?: string; + touches?: string; since?: string; until?: string; json?: boolean; @@ -116,6 +122,7 @@ program if (opts.breaking) queryOptions.breaking = true; if (opts.grep) queryOptions.grep = opts.grep; if (opts.refs) queryOptions.refs = opts.refs; + if (opts.touches !== undefined) queryOptions.touches = opts.touches; const filtered = queryRecords(records, queryOptions); @@ -142,6 +149,37 @@ program process.stdout.write((opts.json ? formatJson(records) : formatPrettyList(records)) + "\n"); }); +program + .command("blame") + .argument("", "a file in the repository, as it currently stands in the working tree") + .description( + "shows the Agent-Context of each commit that last changed the given lines (the whole file " + + "without -L), via git blame. One entry per commit, ordered by where its lines first " + + "appear in the file. Lines not committed yet are counted on stderr, not attributed.", + ) + .option("-L ", "only these lines, 1-based and inclusive, e.g. -L 42 or -L 40,60") + .option("--json", "output machine-readable JSON instead of pretty text") + .action((file: string, opts: { L?: string; json?: boolean }) => { + let range: ReturnType; + if (opts.L !== undefined) { + range = parseLineRange(opts.L); + if (!range) { + fail(`-L must be or , with 1 <= start <= end, got "${opts.L}"`); + } + } + runFatal(() => checkGitVersion()); + + const { records, uncommittedLines, warnings } = runFatal(() => blame(file, range)); + printWarnings(warnings); + if (uncommittedLines > 0) { + process.stderr.write( + `agent-commit: ${uncommittedLines} line(s) not committed yet, so no commit to show for them\n`, + ); + } + + process.stdout.write((opts.json ? formatJson(records) : formatBlamePretty(records)) + "\n"); + }); + program .command("check") .description( @@ -186,7 +224,9 @@ program misplaced: "MISPLACED".padEnd(labelWidth), }; for (const result of results) { - process.stdout.write(`${labels[result.status]} ${sanitizeForTerminal(result.subject)}\n`); + process.stdout.write( + `${labels[result.status]} ${result.shortHash} ${sanitizeForTerminal(result.subject)}\n`, + ); } } diff --git a/cli/src/parser.ts b/cli/src/parser.ts index aff176e..e0771d6 100644 --- a/cli/src/parser.ts +++ b/cli/src/parser.ts @@ -2,39 +2,48 @@ import { execFileSync } from "node:child_process"; import type { AgentCommitRecord, ParseResult, ParseWarning } from "./types.js"; /** - * A single NUL byte, as a field separator between a commit's trailer - * extraction and its raw body. A NUL byte is the one thing guaranteed to - * never appear inside a git commit message, git itself won't let one - * through, so it can't collide with real content the way an earlier, - * printable-string version of this separator once did (a commit whose - * body happened to contain that literal string, for example one - * documenting this very format, silently split into phantom records; see - * SPEC.md, "Reading it back," for the incident this replaced). + * A single NUL byte, terminating every field of every record. A NUL byte + * is the one thing guaranteed to never appear inside a git commit message + * (git itself won't let one through), so it can't collide with real + * content the way an earlier, printable-string separator once did (a + * commit whose body happened to contain that literal string, for example + * one documenting this very format, silently split into phantom records; + * see SPEC.md, "Reading it back," for the incident this replaced). */ export const FIELD_SEP = "\0"; -/** Two NUL bytes in a row, as the boundary between one commit's record and the next. Unambiguous for the same reason FIELD_SEP is: content can never contain even one NUL, let alone two adjacent ones. */ -export const SENTINEL = "\0\0"; +/** + * Fields emitted per commit by LOG_FORMAT, in order: full hash, + * abbreviated hash, trailer extraction, raw body. Records are parsed + * positionally, LOG_FORMAT_FIELD_COUNT fields at a time, rather than split + * on a multi-byte record boundary: any field can legitimately be empty (no + * trailer, or a commit made with --allow-empty-message), and two adjacent + * empty-field terminators are byte-identical to a "\0\0" boundary, which + * is exactly how an empty-message commit used to swallow the record next + * to it. + */ +export const LOG_FORMAT_FIELD_COUNT = 4; /** * Internal to the CLI, deliberately richer than the plain-git example in * SPEC.md's "Reading it back" section. That example stays a minimal - * two-field command anyone can run with no tool; this format adds the raw - * commit body so the CLI can detect a trailer that's present in the - * message but sitting outside git's recognized trailer block (see the - * MISPLACED detection in parseLog below), which the plain two-field - * command cannot see at all. The subject is derived from the body's first - * line rather than fetched separately, one fewer field, one fewer thing - * that could disagree with itself. + * two-field command anyone can run with no tool; this format adds the + * commit hash (so every result can be acted on: `git show`, `blame`, + * `agent-commit show`) and the raw commit body (so the CLI can detect a + * trailer that's present in the message but sitting outside git's + * recognized trailer block, see the MISPLACED detection in parseLog + * below). The subject is derived from the body's first line rather than + * fetched separately, one fewer field that could disagree with itself. + * The abbreviated hash comes from git (%h) rather than slicing %H, so it + * honors core.abbrev and stays unique in large repos. * * Uses `%x00`, git's own escape syntax for emitting a NUL byte, not a * literal NUL character in this string: this format string is passed as a * command-line argument, and a real embedded NUL there is invalid at the * OS/argv level (Node rejects it outright). Git converts `%x00` into an * actual NUL byte only in its captured *output*, a different code path - * with no such restriction, that output byte is what FIELD_SEP/SENTINEL - * above are matching against in parseLog. Verified by inspecting git's - * raw output byte-for-byte before writing this. + * with no such restriction, that output byte is what FIELD_SEP above + * matches against in parseLog. */ -export const LOG_FORMAT = "%(trailers:key=Agent-Context,valueonly)%x00%B%x00%x00"; +export const LOG_FORMAT = "%H%x00%h%x00%(trailers:key=Agent-Context,valueonly)%x00%B%x00"; // Tolerates optional whitespace before the colon (git itself does: `Agent-Context : {...}` // is extracted as a trailer just as readily as `Agent-Context: {...}`, verified against real // git) and is case-insensitive to match git's own case-insensitive trailer key matching. @@ -118,10 +127,20 @@ export function checkGitVersion(cwd?: string): void { } } -/** Runs `git log` with the sentinel-delimited format from SPEC.md. Extra args are passed through (revision ranges, --since, path filters, etc). */ +/** Runs `git log` with LOG_FORMAT, the NUL-delimited format parseLog expects. Extra args are passed through (revision ranges, --since, path filters, etc). */ export function runGitLog(extraArgs: string[] = [], cwd?: string): string { + return runGit(["log", `--format=${LOG_FORMAT}`, ...extraArgs], cwd); +} + +/** + * Runs any git subcommand and returns its stdout, translating failures + * into this module's typed errors (git missing, not a repository, or + * git's own non-zero exit with its stderr preserved) rather than leaking + * a raw execFileSync stack trace. + */ +export function runGit(args: string[], cwd?: string): string { try { - return execFileSync("git", ["log", `--format=${LOG_FORMAT}`, ...extraArgs], { + return execFileSync("git", args, { cwd, encoding: "utf8", maxBuffer: 1024 * 1024 * 64, @@ -146,53 +165,74 @@ export function runGitLog(extraArgs: string[] = [], cwd?: string): string { } } +/** Raised when git log output doesn't have the shape LOG_FORMAT produces. Indicates a bug, not bad commit content. */ +export class LogFormatError extends Error { + constructor(fieldCount: number) { + super( + `unexpected git log output: ${fieldCount} fields is not a multiple of ${LOG_FORMAT_FIELD_COUNT}`, + ); + this.name = "LogFormatError"; + } +} + /** - * Parses raw `git log --format=` output into records. Each - * commit's record (between SENTINELs, two adjacent NULs) has two fields - * split on a single NUL: the trailer extraction (0, 1, or 2+ non-empty - * lines: 0 means no trailer, 1 means a normal trailer, 2+ means a - * duplicate, treated as ambiguous per SPEC.md rather than guessing which - * is authoritative), and the raw commit body, whose first line is the - * subject and whose full text is used only to detect a trailer that's - * present in the message but not in the position git recognizes as a - * trailer, exactly what GitHub's squash-merge prefill can silently - * produce (see SPEC.md, "Squash merges"). + * Parses raw `git log --format=` output into records. The + * output is split on NUL into a flat field list and consumed + * LOG_FORMAT_FIELD_COUNT fields per commit (see that constant for why + * positional parsing replaced a record boundary). Per commit: + * + * - the trailer extraction holds 0, 1, or 2+ non-empty lines: 0 means no + * trailer, 1 means a normal trailer, 2+ means a duplicate, treated as + * ambiguous per SPEC.md rather than guessing which is authoritative; + * - the raw body's first line is the subject, and its full text is used + * only to detect a trailer that's present in the message but not in the + * position git recognizes as a trailer, exactly what GitHub's + * squash-merge prefill can silently produce (see SPEC.md, "Squash + * merges"). */ export function parseLog(raw: string): ParseResult { const records: AgentCommitRecord[] = []; const warnings: ParseWarning[] = []; - const chunks = raw.split(SENTINEL); - - for (const chunk of chunks) { - const trimmed = chunk.replace(/^\n+/, "").replace(/\n+$/, ""); - if (trimmed === "") continue; + // Every field ends in FIELD_SEP, so the final element is whatever + // follows the last commit's last field: git's own record-terminating + // newline, or nothing. + const fields = raw.split(FIELD_SEP); + const trailing = fields.pop() ?? ""; + if (trailing.trim() !== "" || fields.length % LOG_FORMAT_FIELD_COUNT !== 0) { + throw new LogFormatError(fields.length); + } - const sepIndex = trimmed.indexOf(FIELD_SEP); - const trailerSection = sepIndex === -1 ? "" : trimmed.slice(0, sepIndex); - const rawBody = sepIndex === -1 ? trimmed : trimmed.slice(sepIndex + FIELD_SEP.length); + for (let i = 0; i < fields.length; i += LOG_FORMAT_FIELD_COUNT) { + // git separates records with a newline, which lands at the start of + // the next record's first field; hashes never contain whitespace. + const hash = fields[i].trim(); + const shortHash = fields[i + 1]; + const trailerSection = fields[i + 2]; + const rawBody = fields[i + 3].replace(/\n+$/, ""); const subject = rawBody.split("\n")[0] ?? ""; const trailerLines = trailerSection.split("\n").filter((line) => line.trim() !== ""); + const base = { hash, shortHash, subject }; if (trailerLines.length === 0) { const misplaced = AGENT_CONTEXT_LINE.test(rawBody); - if (misplaced) warnings.push({ subject, reason: "misplaced-trailer" }); - records.push({ subject, context: null, malformed: false, ambiguous: false, misplaced }); + if (misplaced) warnings.push({ ...base, reason: "misplaced-trailer" }); + records.push({ ...base, context: null, malformed: false, ambiguous: false, misplaced }); continue; } if (trailerLines.length > 1) { - warnings.push({ subject, reason: "ambiguous-trailer" }); - records.push({ subject, context: null, malformed: false, ambiguous: true, misplaced: false }); + warnings.push({ ...base, reason: "ambiguous-trailer" }); + records.push({ ...base, context: null, malformed: false, ambiguous: true, misplaced: false }); continue; } try { const context: unknown = JSON.parse(trailerLines[0]); - records.push({ subject, context, malformed: false, ambiguous: false, misplaced: false }); + records.push({ ...base, context, malformed: false, ambiguous: false, misplaced: false }); } catch { - warnings.push({ subject, reason: "malformed-json" }); - records.push({ subject, context: null, malformed: true, ambiguous: false, misplaced: false }); + warnings.push({ ...base, reason: "malformed-json" }); + records.push({ ...base, context: null, malformed: true, ambiguous: false, misplaced: false }); } } diff --git a/cli/src/query.ts b/cli/src/query.ts index 31b25f0..f3a6fc6 100644 --- a/cli/src/query.ts +++ b/cli/src/query.ts @@ -8,10 +8,51 @@ export interface QueryOptions { grep?: string; /** Exact match against an entry in context.refs. */ refs?: string; + /** Path match against context.touches, on whole path segments; see touchesPath. */ + touches?: string; } function hasAnyFilter(options: QueryOptions): boolean { - return options.breaking !== undefined || options.grep !== undefined || options.refs !== undefined; + return ( + options.breaking !== undefined || + options.grep !== undefined || + options.refs !== undefined || + options.touches !== undefined + ); +} + +/** + * Reduces a path to the canonical form SPEC.md asks `touches` to use + * (repo-relative, no leading `./` or `/`), plus no trailing `/`, and + * splits it into segments. Applied to both the query and each entry, + * since an entry written slightly off-spec is still a real claim about + * what the commit touched. + */ +function pathSegments(path: string): string[] { + return path + .trim() + .replace(/^(\.\/|\/)+/, "") + .split("/") + .filter((segment) => segment !== ""); +} + +function isSegmentPrefix(prefix: string[], of: string[]): boolean { + return prefix.length <= of.length && prefix.every((segment, i) => segment === of[i]); +} + +/** + * True if a `touches` entry and the queried path overlap, on whole path + * segments, in either direction: an entry inside the queried path + * (`src/auth/session.ts` for `src/auth`) and an entry containing it + * (`src` for `src/auth/session.ts`, a commit claiming it touched the + * broader area) both match. A partial segment never does: `src/au` + * matches nothing under `src/auth`. Case-sensitive, like git paths. + */ +export function touchesPath(entry: string, query: string): boolean { + const entrySegments = pathSegments(entry); + const querySegments = pathSegments(query); + if (entrySegments.length === 0 || querySegments.length === 0) return false; + return isSegmentPrefix(entrySegments, querySegments) || isSegmentPrefix(querySegments, entrySegments); } /** @@ -48,6 +89,13 @@ export function queryRecords( if (!ctx.refs.includes(options.refs)) return false; } + if (options.touches !== undefined) { + const query = options.touches; + if (!Array.isArray(ctx.touches)) return false; + const matches = ctx.touches.some((entry) => typeof entry === "string" && touchesPath(entry, query)); + if (!matches) return false; + } + return true; }); } diff --git a/cli/src/types.ts b/cli/src/types.ts index bccea7f..46a4435 100644 --- a/cli/src/types.ts +++ b/cli/src/types.ts @@ -2,6 +2,10 @@ export type AgentContextValue = Record; export interface AgentCommitRecord { + /** The commit's full object hash. */ + hash: string; + /** git's own abbreviated hash (%h), unique within the repo at the time of reading. For display; use `hash` for anything machine-facing. */ + shortHash: string; /** The commit's subject line. */ subject: string; /** @@ -33,6 +37,8 @@ export function isAgentContextObject(value: unknown): value is AgentContextValue } export interface ParseWarning { + hash: string; + shortHash: string; subject: string; reason: "malformed-json" | "ambiguous-trailer" | "misplaced-trailer"; } diff --git a/cli/tests/blame.test.ts b/cli/tests/blame.test.ts new file mode 100644 index 0000000..d757e18 --- /dev/null +++ b/cli/tests/blame.test.ts @@ -0,0 +1,147 @@ +import { afterEach, describe, expect, it } from "vitest"; +import { blame, formatBlamePretty, parseBlamePorcelain, parseLineRange } from "../src/blame.js"; +import { GitCommandError } from "../src/parser.js"; +import { record } from "./fixtures/record.js"; +import { TempRepo } from "./fixtures/temp-repo.js"; + +const A = "a".repeat(40); +const B = "b".repeat(40); +const ZERO = "0".repeat(40); + +describe("parseBlamePorcelain", () => { + it("groups lines by commit, compresses them into ranges, and orders commits by first line", () => { + // Shape copied from real `git blame --porcelain` output: a header per + // line (with a group size only on a group's first line), metadata once + // per commit, and tab-prefixed content. + const raw = [ + `${B} 1 1 2`, + "author t", + "summary second", + "filename f.txt", + "\tline one", + `${B} 2 2`, + "\tline two", + `${A} 3 3 1`, + "author t", + "summary first", + "boundary", + "filename f.txt", + "\tline three", + `${B} 5 4 1`, + "\tline four", + ].join("\n"); + expect(parseBlamePorcelain(raw)).toEqual({ + entries: [ + { hash: B, lines: [[1, 2], [4, 4]] }, + { hash: A, lines: [[3, 3]] }, + ], + uncommittedLines: 0, + }); + }); + + it("counts uncommitted lines (the all-zero hash) instead of attributing them", () => { + const raw = [`${A} 1 1 1`, "\tkept", `${ZERO} 2 2 2`, "\tnew", `${ZERO} 3 3`, "\tnew too"].join("\n"); + expect(parseBlamePorcelain(raw)).toEqual({ entries: [{ hash: A, lines: [[1, 1]] }], uncommittedLines: 2 }); + }); + + it("never mistakes line content or metadata for a header, even when it looks like one", () => { + const raw = [ + `${A} 1 1 1`, + `summary ${B} 1 1`, + `previous ${B} f.txt`, + `\t${B} 9 9 9`, + ].join("\n"); + expect(parseBlamePorcelain(raw).entries).toEqual([{ hash: A, lines: [[1, 1]] }]); + }); + + it("accepts SHA-256 object names", () => { + const sha256 = "c".repeat(64); + expect(parseBlamePorcelain(`${sha256} 1 1 1\n\tx`).entries).toEqual([{ hash: sha256, lines: [[1, 1]] }]); + }); +}); + +describe("parseLineRange", () => { + it("accepts a single line or an inclusive start,end pair", () => { + expect(parseLineRange("42")).toEqual([42, 42]); + expect(parseLineRange("40,60")).toEqual([40, 60]); + expect(parseLineRange("7,7")).toEqual([7, 7]); + }); + + it("rejects zero, reversed ranges, and git's non-numeric -L forms", () => { + for (const bad of ["0", "5,2", "abc", "", "1,", ",5", "-3", "1,+5", "/regex/", ":funcname"]) { + expect(parseLineRange(bad)).toBeUndefined(); + } + }); +}); + +describe("formatBlamePretty", () => { + it("puts each commit's line ranges directly under its subject", () => { + const out = formatBlamePretty([ + { + ...record({ shortHash: "aaaaaaa", subject: "fix: a", context: { why: "because" } }), + lines: [[1, 3], [7, 7]], + }, + { ...record({ shortHash: "bbbbbbb", subject: "chore: b" }), lines: [[4, 4]] }, + ]); + expect(out).toBe("aaaaaaa fix: a\n lines: 1-3, 7\n why: because\n\nbbbbbbb chore: b\n lines: 4"); + }); +}); + +describe("blame, against a real temp git repo", () => { + let repo: TempRepo; + afterEach(() => repo?.cleanup()); + + function seed(): void { + repo = new TempRepo(); + repo.commitFile("f.txt", "a\nb\nc\n", 'feat: first\n\nAgent-Context: {"why":"original reason"}'); + repo.commitFile("f.txt", "a\nB\nc\nd\n", 'fix: second\n\nAgent-Context: {"why":"second reason"}'); + } + + it("returns the commits behind the requested lines with their trailers and line ranges", () => { + seed(); + const { records, uncommittedLines, warnings } = blame("f.txt", [2, 3], repo.dir); + expect(records.map((r) => [r.subject, r.context, r.lines])).toEqual([ + ["fix: second", { why: "second reason" }, [[2, 2]]], + ["feat: first", { why: "original reason" }, [[3, 3]]], + ]); + expect(records[0].hash).toMatch(/^[0-9a-f]{40}$/); + expect(uncommittedLines).toBe(0); + expect(warnings).toHaveLength(0); + }); + + it("blames the whole file when no range is given", () => { + seed(); + const { records } = blame("f.txt", undefined, repo.dir); + expect(records.map((r) => [r.subject, r.lines])).toEqual([ + ["feat: first", [[1, 1], [3, 3]]], + ["fix: second", [[2, 2], [4, 4]]], + ]); + }); + + it("counts working-tree changes as uncommitted rather than attributing them", () => { + seed(); + repo.writeFile("f.txt", "a\nB\nc\nd\nnew\n"); + const { records, uncommittedLines } = blame("f.txt", [4, 5], repo.dir); + expect(records.map((r) => r.subject)).toEqual(["fix: second"]); + expect(uncommittedLines).toBe(1); + }); + + it("returns no records, without running git log, when every line is uncommitted", () => { + seed(); + repo.writeFile("f.txt", "a\nB\nc\nd\nnew\n"); + expect(blame("f.txt", [5, 5], repo.dir)).toEqual({ records: [], uncommittedLines: 1, warnings: [] }); + }); + + it("surfaces git's own error for a range past the end of the file or an unknown path", () => { + seed(); + expect(() => blame("f.txt", [9, 9], repo.dir)).toThrow(GitCommandError); + expect(() => blame("f.txt", [9, 9], repo.dir)).toThrow(/has only 4 lines/); + expect(() => blame("missing.txt", undefined, repo.dir)).toThrow(/no such path/); + }); + + it("treats a path starting with - as a path, never as a git option", () => { + repo = new TempRepo(); + repo.commitFile("-L", "x\n", "chore: oddly named file"); + expect(blame("-L", undefined, repo.dir).records.map((r) => r.subject)).toEqual(["chore: oddly named file"]); + }); +}); diff --git a/cli/tests/check.test.ts b/cli/tests/check.test.ts index 7ecade1..aeb13a8 100644 --- a/cli/tests/check.test.ts +++ b/cli/tests/check.test.ts @@ -1,22 +1,13 @@ import { describe, expect, it } from "vitest"; import { checkRecords } from "../src/check.js"; -import type { AgentCommitRecord } from "../src/types.js"; +import { FAKE_HASH, FAKE_SHORT_HASH, record } from "./fixtures/record.js"; -function record(overrides: Partial = {}): AgentCommitRecord { - return { - subject: "subject", - context: null, - malformed: false, - ambiguous: false, - misplaced: false, - ...overrides, - }; -} +const fakeHashes = { hash: FAKE_HASH, shortHash: FAKE_SHORT_HASH }; describe("checkRecords", () => { it("marks a commit with a valid object trailer as ok", () => { const [result] = checkRecords([record({ subject: "fix: x", context: { why: "y" } })]); - expect(result).toEqual({ subject: "fix: x", status: "ok" }); + expect(result).toEqual({ ...fakeHashes, subject: "fix: x", status: "ok" }); }); it("marks a commit with valid-but-non-object JSON as miss, ok requires an object shape", () => { @@ -26,7 +17,7 @@ describe("checkRecords", () => { it("marks a commit with no trailer at all as miss", () => { const [result] = checkRecords([record({ subject: "chore: bump", context: null })]); - expect(result).toEqual({ subject: "chore: bump", status: "miss" }); + expect(result).toEqual({ ...fakeHashes, subject: "chore: bump", status: "miss" }); }); it("marks a malformed trailer as miss, it's not usable even though something was written", () => { diff --git a/cli/tests/cli.test.ts b/cli/tests/cli.test.ts index b6c7249..2aa4760 100644 --- a/cli/tests/cli.test.ts +++ b/cli/tests/cli.test.ts @@ -26,6 +26,41 @@ function runCli(args: string[], cwd: string) { return { stdout: result.stdout, stderr: result.stderr, status: result.status }; } +describe("agent-commit blame, against the compiled binary", () => { + let repo: TempRepo; + afterEach(() => repo?.cleanup()); + + it("prints each commit's lines and trailer, and notes uncommitted lines on stderr only", () => { + repo = new TempRepo(); + repo.commitFile("f.txt", "a\nb\n", 'feat: first\n\nAgent-Context: {"why":"the reason"}'); + repo.writeFile("f.txt", "a\nb\nnew\n"); + const { stdout, stderr, status } = runCli(["blame", "f.txt", "-L", "1,3"], repo.dir); + expect(status).toBe(0); + expect(stdout).toMatch(/^[0-9a-f]{7,} feat: first\n lines: 1-2\n why: the reason\n$/); + expect(stderr).toContain("1 line(s) not committed yet"); + }); + + it("emits records with a lines field under --json", () => { + repo = new TempRepo(); + repo.commitFile("f.txt", "a\n", "chore: one"); + const { stdout, status } = runCli(["blame", "f.txt", "--json"], repo.dir); + expect(status).toBe(0); + expect(JSON.parse(stdout)).toMatchObject([{ subject: "chore: one", lines: [[1, 1]] }]); + }); + + it("rejects a malformed -L before calling git, and reports git's own errors cleanly", () => { + repo = new TempRepo(); + repo.commitFile("f.txt", "a\n", "chore: one"); + const bad = runCli(["blame", "f.txt", "-L", "5,2"], repo.dir); + expect(bad.status).toBe(1); + expect(bad.stderr).toContain("-L must be or ,"); + const missing = runCli(["blame", "nope.txt"], repo.dir); + expect(missing.status).toBe(1); + expect(missing.stderr).toMatch(/^agent-commit: fatal: no such path/); + expect(missing.stdout).toBe(""); + }); +}); + describe("agent-commit log, against the compiled binary", () => { let repo: TempRepo; afterEach(() => repo?.cleanup()); @@ -35,7 +70,7 @@ describe("agent-commit log, against the compiled binary", () => { repo.commit('fix: real one\n\nAgent-Context: {"why":"a real reason"}'); const { stdout, status } = runCli(["log"], repo.dir); expect(status).toBe(0); - expect(stdout).toContain("fix: real one"); + expect(stdout).toMatch(/^[0-9a-f]{7,} fix: real one$/m); expect(stdout).toContain("why: a real reason"); }); @@ -46,9 +81,19 @@ describe("agent-commit log, against the compiled binary", () => { expect(status).toBe(0); const parsed = JSON.parse(stdout); expect(parsed[0].subject).toBe("fix: json one"); + expect(parsed[0].hash).toMatch(/^[0-9a-f]{40}$/); expect(parsed[0].context).toEqual({ why: "x" }); }); + it("filters by --touches on path segments", () => { + repo = new TempRepo(); + repo.commit('feat: auth\n\nAgent-Context: {"why":"a","touches":["src/auth/session.ts"]}'); + repo.commit('feat: authz\n\nAgent-Context: {"why":"b","touches":["src/authz"]}'); + const { stdout, status } = runCli(["log", "--touches", "src/auth", "--json"], repo.dir); + expect(status).toBe(0); + expect(JSON.parse(stdout).map((r: { subject: string }) => r.subject)).toEqual(["feat: auth"]); + }); + it("routes parse warnings to stderr, never mixed into stdout's structured output", () => { repo = new TempRepo(); repo.commit('fix: broken\n\nAgent-Context: {not valid json}'); @@ -166,7 +211,9 @@ describe("agent-commit check, against the compiled binary", () => { const { stdout, status } = runCli(["check", "--json"], repo.dir); expect(status).toBe(0); const parsed = JSON.parse(stdout); - expect(parsed).toEqual([{ subject: "fix: fine", status: "ok" }]); + const hash = execFileSync("git", ["rev-parse", "HEAD"], { cwd: repo.dir, encoding: "utf8" }).trim(); + const shortHash = execFileSync("git", ["rev-parse", "--short", "HEAD"], { cwd: repo.dir, encoding: "utf8" }).trim(); + expect(parsed).toEqual([{ hash, shortHash, subject: "fix: fine", status: "ok" }]); }); it("prints the MISPLACED explanation to stderr only when something is actually misplaced", () => { diff --git a/cli/tests/fixtures/record.ts b/cli/tests/fixtures/record.ts new file mode 100644 index 0000000..df7462d --- /dev/null +++ b/cli/tests/fixtures/record.ts @@ -0,0 +1,19 @@ +import type { AgentCommitRecord } from "../../src/types.js"; + +/** A fixed, obviously fake hash for synthetic records. Tests that care about real hashes use TempRepo instead. */ +export const FAKE_HASH = "0123456789abcdef0123456789abcdef01234567"; +export const FAKE_SHORT_HASH = "0123456"; + +/** Builds a synthetic AgentCommitRecord with neutral defaults, overriding only what a test cares about. */ +export function record(overrides: Partial = {}): AgentCommitRecord { + return { + hash: FAKE_HASH, + shortHash: FAKE_SHORT_HASH, + subject: "subject", + context: null, + malformed: false, + ambiguous: false, + misplaced: false, + ...overrides, + }; +} diff --git a/cli/tests/fixtures/temp-repo.ts b/cli/tests/fixtures/temp-repo.ts index 49dcfb3..20195d5 100644 --- a/cli/tests/fixtures/temp-repo.ts +++ b/cli/tests/fixtures/temp-repo.ts @@ -32,6 +32,18 @@ export class TempRepo { }); } + /** Writes `content` to `path` (relative to the repo) and commits it with `message`, for tests that need a file's lines to change across commits. */ + commitFile(path: string, content: string, message: string): void { + writeFileSync(join(this.dir, path), content); + this.git(["add", "--", path]); + execFileSync("git", ["commit", "-q", "-F", "-"], { cwd: this.dir, input: message, encoding: "utf8" }); + } + + /** Writes `content` to `path` without committing, to leave a working-tree change. */ + writeFile(path: string, content: string): void { + writeFileSync(join(this.dir, path), content); + } + log(extraArgs: string[], format: string): string { return this.git(["log", `--format=${format}`, ...extraArgs]); } diff --git a/cli/tests/format.test.ts b/cli/tests/format.test.ts index 5f99031..0c98c42 100644 --- a/cli/tests/format.test.ts +++ b/cli/tests/format.test.ts @@ -1,22 +1,11 @@ import { describe, expect, it } from "vitest"; import { formatJson, formatPretty, formatPrettyList, sanitizeForTerminal } from "../src/format.js"; -import type { AgentCommitRecord } from "../src/types.js"; - -function record(overrides: Partial = {}): AgentCommitRecord { - return { - subject: "subject", - context: null, - malformed: false, - ambiguous: false, - misplaced: false, - ...overrides, - }; -} +import { FAKE_SHORT_HASH, record } from "./fixtures/record.js"; describe("formatPretty", () => { - it("prints just the subject when there's no trailer", () => { + it("prints just the short hash and subject when there's no trailer", () => { const out = formatPretty(record({ subject: "chore: bump dependency" })); - expect(out).toBe("chore: bump dependency"); + expect(out).toBe(`${FAKE_SHORT_HASH} chore: bump dependency`); }); it("prints common fields in the documented order, not object insertion order", () => { @@ -29,7 +18,7 @@ describe("formatPretty", () => { }), ); const lines = out.split("\n"); - expect(lines[0]).toBe("fix: retry backoff"); + expect(lines[0]).toBe(`${FAKE_SHORT_HASH} fix: retry backoff`); expect(lines[1]).toBe(" why: added exponential backoff"); expect(lines[2]).toBe(" breaking: false"); }); @@ -78,7 +67,7 @@ describe("formatPretty", () => { it("renders valid-but-non-object JSON (a bare string) without crashing or mislabeling it", () => { const out = formatPretty(record({ subject: "fix: odd", context: "just a string" })); - expect(out).toBe("fix: odd\n just a string"); + expect(out).toBe(`${FAKE_SHORT_HASH} fix: odd\n just a string`); }); it("neutralizes a right-to-left override in a why value instead of applying it to the terminal", () => { @@ -129,10 +118,10 @@ describe("formatPretty", () => { describe("formatPrettyList", () => { it("joins multiple records with a blank line between them", () => { const out = formatPrettyList([ - record({ subject: "first" }), - record({ subject: "second", context: { why: "reason" } }), + record({ shortHash: "aaaaaaa", subject: "first" }), + record({ shortHash: "bbbbbbb", subject: "second", context: { why: "reason" } }), ]); - expect(out).toBe("first\n\nsecond\n why: reason"); + expect(out).toBe("aaaaaaa first\n\nbbbbbbb second\n why: reason"); }); it("returns an empty string for an empty list, not a stray separator", () => { diff --git a/cli/tests/merge-draft.test.ts b/cli/tests/merge-draft.test.ts index 874cdb0..7a8aafb 100644 --- a/cli/tests/merge-draft.test.ts +++ b/cli/tests/merge-draft.test.ts @@ -1,20 +1,9 @@ import { afterEach, describe, expect, it } from "vitest"; import { buildMergeDraft, formatMergeDraft } from "../src/merge-draft.js"; import { parseLog, runGitLog } from "../src/parser.js"; -import type { AgentCommitRecord } from "../src/types.js"; +import { record } from "./fixtures/record.js"; import { TempRepo } from "./fixtures/temp-repo.js"; -function record(overrides: Partial = {}): AgentCommitRecord { - return { - subject: "subject", - context: null, - malformed: false, - ambiguous: false, - misplaced: false, - ...overrides, - }; -} - describe("buildMergeDraft", () => { it("unions touches and refs across commits, deduped", () => { const draft = buildMergeDraft([ diff --git a/cli/tests/parser.test.ts b/cli/tests/parser.test.ts index fa6ab1d..e08504b 100644 --- a/cli/tests/parser.test.ts +++ b/cli/tests/parser.test.ts @@ -1,3 +1,4 @@ +import { execFileSync } from "node:child_process"; import { mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -7,25 +8,36 @@ import { FIELD_SEP, GitCommandError, LOG_FORMAT, + LOG_FORMAT_FIELD_COUNT, + LogFormatError, NotAGitRepositoryError, parseLog, runGitLog, - SENTINEL, } from "../src/parser.js"; import { isAgentContextObject } from "../src/types.js"; +import { FAKE_HASH, FAKE_SHORT_HASH } from "./fixtures/record.js"; import { TempRepo } from "./fixtures/temp-repo.js"; -/** Builds a synthetic raw log record matching LOG_FORMAT's two NUL-delimited fields, for one commit. Subject is always the raw body's first line, matching how parseLog derives it, so tests don't pass it separately. */ +/** + * Builds one synthetic commit's raw log output, shaped exactly like + * LOG_FORMAT's real output: four NUL-terminated fields (hash, short hash, + * trailer extraction, body) followed by git's record-terminating newline. + * Subject is always the raw body's first line, matching how parseLog + * derives it, so tests don't pass it separately. + */ function chunk(trailerSection: string, rawBody: string): string { - return trailerSection + FIELD_SEP + rawBody + SENTINEL; + return [FAKE_HASH, FAKE_SHORT_HASH, trailerSection, rawBody].map((f) => f + FIELD_SEP).join("") + "\n"; } +const fakeHashes = { hash: FAKE_HASH, shortHash: FAKE_SHORT_HASH }; + describe("parseLog", () => { it("returns context: null for a commit with no trailer", () => { const raw = chunk("", "chore: no trailer"); const { records, warnings } = parseLog(raw); expect(records).toHaveLength(1); expect(records[0]).toEqual({ + ...fakeHashes, subject: "chore: no trailer", context: null, malformed: false, @@ -59,7 +71,7 @@ describe("parseLog", () => { expect(records[0].context).toBeNull(); expect(records[0].malformed).toBe(true); expect(records[0].misplaced).toBe(false); - expect(warnings).toEqual([{ subject: "fix: broken trailer", reason: "malformed-json" }]); + expect(warnings).toEqual([{ ...fakeHashes, subject: "fix: broken trailer", reason: "malformed-json" }]); }); it("flags duplicate Agent-Context trailers as ambiguous rather than picking one", () => { @@ -72,7 +84,7 @@ describe("parseLog", () => { expect(records[0].context).toBeNull(); expect(records[0].ambiguous).toBe(true); expect(records[0].misplaced).toBe(false); - expect(warnings).toEqual([{ subject: "fix: two trailers", reason: "ambiguous-trailer" }]); + expect(warnings).toEqual([{ ...fakeHashes, subject: "fix: two trailers", reason: "ambiguous-trailer" }]); }); it("flags a trailer that's present in the body but not recognized as a trailer as misplaced", () => { @@ -90,7 +102,7 @@ describe("parseLog", () => { expect(records[0].malformed).toBe(false); expect(records[0].ambiguous).toBe(false); expect(records[0].misplaced).toBe(true); - expect(warnings).toEqual([{ subject: "fix: github prefill shape", reason: "misplaced-trailer" }]); + expect(warnings).toEqual([{ ...fakeHashes, subject: "fix: github prefill shape", reason: "misplaced-trailer" }]); }); it("does not flag misplaced when there's genuinely no Agent-Context text anywhere in the body", () => { @@ -174,6 +186,11 @@ describe("parseLog", () => { expect(warnings).toHaveLength(2); }); + it("throws LogFormatError on output that isn't a whole number of records, rather than guessing", () => { + expect(() => parseLog(FAKE_HASH + FIELD_SEP + FAKE_SHORT_HASH + FIELD_SEP)).toThrow(LogFormatError); + expect(() => parseLog("not log output at all")).toThrow(LogFormatError); + }); + it("handles an empty log (no commits) without error", () => { const { records, warnings } = parseLog(""); expect(records).toHaveLength(0); @@ -227,6 +244,33 @@ describe("runGitLog + parseLog, against a real temp git repo", () => { expect(warnings).toHaveLength(0); }); + it("reports each commit's real full and abbreviated hash", () => { + repo = new TempRepo(); + repo.commit('fix: hashed\n\nAgent-Context: {"why":"x"}'); + const expectedHash = execFileSync("git", ["rev-parse", "HEAD"], { cwd: repo.dir, encoding: "utf8" }).trim(); + const expectedShort = execFileSync("git", ["rev-parse", "--short", "HEAD"], { cwd: repo.dir, encoding: "utf8" }).trim(); + const { records } = parseLog(runGitLog([], repo.dir)); + expect(records[0].hash).toBe(expectedHash); + expect(records[0].shortHash).toBe(expectedShort); + }); + + it("keeps an empty-message commit and its neighbors as separate, correct records", () => { + // Regression test for a real bug, reproduced against real git before + // the fix: records used to be split on a "\0\0" boundary, and an + // empty message (--allow-empty-message) produces two adjacent empty + // fields that are byte-identical to that boundary. The commit next to + // it was swallowed and the remains misreported as MISPLACED. + repo = new TempRepo(); + repo.commit('fix: before\n\nAgent-Context: {"why":"kept"}'); + execFileSync("git", ["commit", "-q", "--allow-empty", "--allow-empty-message", "-m", ""], { cwd: repo.dir }); + repo.commit("chore: after"); + const { records, warnings } = parseLog(runGitLog([], repo.dir)); + expect(records.map((r) => r.subject)).toEqual(["chore: after", "", "fix: before"]); + expect(records[1]).toMatchObject({ context: null, misplaced: false }); + expect(records[2].context).toEqual({ why: "kept" }); + expect(warnings).toHaveLength(0); + }); + it("correctly isolates a duplicate-trailer commit from real git output, not just synthetic input", () => { repo = new TempRepo(); repo.commit( @@ -372,13 +416,14 @@ describe("checkGitVersion", () => { // pins the constant's own shape so a future edit can't silently drop a // field without a test noticing. describe("LOG_FORMAT", () => { - it("has exactly two NUL-delimited fields ending in the double-NUL sentinel", () => { + it("emits hash, short hash, trailer, and body, each NUL-terminated", () => { expect(FIELD_SEP).toBe("\0"); - expect(SENTINEL).toBe("\0\0"); + expect(LOG_FORMAT_FIELD_COUNT).toBe(4); // The format string itself uses git's %x00 escape syntax, not a literal // NUL, since it's passed as a process argument (see LOG_FORMAT's own // comment for why an embedded NUL there is invalid). - expect(LOG_FORMAT).toBe("%(trailers:key=Agent-Context,valueonly)%x00%B%x00%x00"); + expect(LOG_FORMAT).toBe("%H%x00%h%x00%(trailers:key=Agent-Context,valueonly)%x00%B%x00"); + expect(LOG_FORMAT.split("%x00")).toHaveLength(LOG_FORMAT_FIELD_COUNT + 1); expect(LOG_FORMAT).not.toContain("\0"); }); }); diff --git a/cli/tests/query.test.ts b/cli/tests/query.test.ts index c1aa85d..3aea77f 100644 --- a/cli/tests/query.test.ts +++ b/cli/tests/query.test.ts @@ -1,17 +1,7 @@ import { describe, expect, it } from "vitest"; -import { queryRecords } from "../src/query.js"; +import { queryRecords, touchesPath } from "../src/query.js"; import type { AgentCommitRecord } from "../src/types.js"; - -function record(overrides: Partial = {}): AgentCommitRecord { - return { - subject: "subject", - context: null, - malformed: false, - ambiguous: false, - misplaced: false, - ...overrides, - }; -} +import { record } from "./fixtures/record.js"; // No `type` key anywhere here: SPEC.md is explicit that `type` isn't a // field this format uses, so realistic fixtures shouldn't have one, and @@ -107,3 +97,55 @@ describe("queryRecords", () => { expect(subjects).not.toContain("fix: ambiguous trailer"); }); }); + +describe("touches filtering", () => { + const touchFixtures = [ + record({ subject: "a: exact dir", context: { touches: ["src/auth"] } }), + record({ subject: "b: file inside", context: { touches: ["src/auth/session.ts", "docs"] } }), + record({ subject: "c: broader dir", context: { touches: ["src"] } }), + record({ subject: "d: sibling with shared prefix", context: { touches: ["src/authz"] } }), + record({ subject: "e: unrelated", context: { touches: ["cli/src/auth"] } }), + record({ subject: "f: no touches", context: { why: "x" } }), + record({ subject: "g: touches not an array", context: { touches: "src/auth" } }), + ]; + const subjects = (touches: string) => queryRecords(touchFixtures, { touches }).map((r) => r.subject); + + it("matches entries inside, equal to, or containing the queried path", () => { + expect(subjects("src/auth")).toEqual(["a: exact dir", "b: file inside", "c: broader dir"]); + }); + + it("matches a file query against a directory entry that contains it", () => { + expect(subjects("src/auth/session.ts")).toEqual(["a: exact dir", "b: file inside", "c: broader dir"]); + }); + + it("never matches on a partial path segment", () => { + expect(subjects("src/au")).toEqual(["c: broader dir"]); + expect(subjects("src/auth")).not.toContain("d: sibling with shared prefix"); + }); + + it("is anchored at the repo root, not a suffix match", () => { + expect(subjects("src/auth")).not.toContain("e: unrelated"); + }); + + it("tolerates ./, leading /, and trailing / on both the query and the entry", () => { + expect(touchesPath("./src/auth/", "/src/auth")).toBe(true); + expect(subjects("./src/auth/")).toEqual(subjects("src/auth")); + }); + + it("never matches an empty or root-only query, rather than matching everything", () => { + expect(subjects("")).toEqual([]); + expect(subjects("/")).toEqual([]); + expect(touchesPath("", "src")).toBe(false); + }); + + it("is case-sensitive, like git paths", () => { + expect(subjects("SRC/auth")).toEqual([]); + }); + + it("skips records without a touches array, and non-string entries", () => { + expect(subjects("src/auth")).not.toContain("f: no touches"); + expect(subjects("src/auth")).not.toContain("g: touches not an array"); + const mixed = [record({ subject: "h", context: { touches: [42, null, "src/auth"] } })]; + expect(queryRecords(mixed, { touches: "src/auth" })).toHaveLength(1); + }); +});