Manage bisect repair patches through the CLI - #170
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| function parseNumstat(output: Buffer): PatchFileSummary[] { | ||
| return output.toString('utf8').split('\0').filter(Boolean).map((record) => { | ||
| const match = /^(\d+|-)\t(\d+|-)\t([\s\S]+)$/.exec(record); | ||
| if (!match) throw new Error('Patch contains an unreadable file summary'); |
There was a problem hiding this comment.
Correctness: renamed-file patches break parseNumstat/inspectPatch.
git diff --numstat -z (and git apply --numstat -z, which shares the same formatter) emits a different record shape for a detected rename: the numeric fields are followed by an empty path field, then two separate NUL-terminated tokens (old path, new path) — instead of added\tdeleted\tpath\0.
parseNumstat does output.toString('utf8').split('\0').filter(Boolean), so for a rename record the empty-path token "N\tM\t" survives filter(Boolean) (it's non-empty as a string) but fails the regex /^(\d+|-)\t(\d+|-)\t([\s\S]+)$/ because the third group requires ≥1 character. That throws Patch contains an unreadable file summary for every capture that includes a git-detected rename.
Since inspectPatch backs captureWorkingTreePatch, captureSourceCommitPatch, importPatchFile, and BisectPatchRegistry.describe() (via patch list/show), this means any patch involving a renamed file can't be captured, registered, or even displayed — a plain "unreadable file summary" error with no hint that the real cause is a rename. Worth handling the two-token rename record explicitly (or passing --no-renames/-M0 to the underlying diff so renames are never emitted as such).
| async function assertMutable( | ||
| context: PatchCliContext, | ||
| deps: BisectPatchCliDependencies, | ||
| ): Promise<void> { | ||
| if (deps.assertMutable) return deps.assertMutable(context); | ||
| if (!context.projectSlug) return; | ||
| const outcome = await tryProxy({ | ||
| slug: context.projectSlug, | ||
| request: { v: PROTOCOL_VERSION, cmd: 'bisect-status' }, | ||
| }); | ||
| if (outcome.proxied && outcome.code !== 0) { | ||
| throw new Error(outcome.error ?? 'Cannot query bisect lease status'); | ||
| } | ||
| const activeSessionId = outcome.proxied | ||
| ? (outcome.data as { activeSessionId?: string | null } | undefined)?.activeSessionId | ||
| : null; | ||
| if (activeSessionId) { | ||
| throw new Error( | ||
| `Cannot modify bisect patches while session "${activeSessionId}" owns the project`, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Altitude/race: the "no mutation during an active bisect session" invariant is bolted onto the CLI layer, not enforced by BisectPatchRegistry itself.
assertMutable() is a plain check-then-act: it queries bisect-status over IPC, and only if that comes back with no activeSessionId do the five call sites (create, edit, remove, apply, updateMetadata — via runCreate/runEdit/runRemove/the apply action/runUpdate) proceed to call into BisectPatchRegistry, which writes the manifest/artifact directly via fs.renameSync with no lock held across the gap. A bisect session can begin between the status check and the actual write, so the registry mutation can race a running session reading bisect-repairs/manifest.json.
It's also purely opt-in: BisectPatchRegistry.create/edit/remove/apply/updateMetadata have zero internal awareness of session state — any future caller (a test helper, a new CLI command, a script driving the registry directly) silently loses this protection unless it remembers to call assertMutable first, unlike every other menu lifecycle action which is funneled through runProxiedAction's single serialization point in servers-menu.ts.
Consider moving the guard inside BisectPatchRegistry (or wrapping its mutating methods) so it's structurally impossible to bypass, and/or holding the check across the actual write rather than releasing between them.
| if ('all' in selector) { | ||
| selected = graph ?? [resolveCommit(repoDir, 'HEAD')]; | ||
| } else if ('commits' in selector) { | ||
| selected = selector.commits.map((ref) => resolveCommit(repoDir, ref)); | ||
| } else { | ||
| const from = selector.from | ||
| ? resolveCommit(repoDir, selector.from) | ||
| : goodSha; | ||
| if (!from) { | ||
| selected = [resolveCommit(repoDir, selector.through)]; | ||
| } else { | ||
| const through = resolveCommit(repoDir, selector.through); | ||
| selected = commitsBetween(repoDir, from, through, false); | ||
| } | ||
| } |
There was a problem hiding this comment.
patch verify silently narrows to a single commit when no explicit good/bad refs are given, contradicting its own description.
The verify command is documented as "Verify a registered patch against its configured commit scope," and the README says { all: true } "applies to every SHA measured by the session... including endpoints, primary and queued candidates, merge second parents, and merge child candidates."
But when goodRef/badRef aren't supplied (so graph is null):
- an
{ all: true }selector (line 382-383) verifies onlyHEAD— not "every SHA." - an interval selector with no explicit
from(line 387-391) verifies only thethroughendpoint — not the interval.
So shaka-perf bisect patch verify <id> run bare on an { all: true } patch (a common case — e.g. demo-ecommerce/bisect-repairs/manifest.json's backport-deals-abtest patch uses { all: true }) gives false confidence: it reports success after checking exactly one commit (HEAD) while silently skipping every other commit the patch is actually meant to cover, with no warning that the check was narrowed.
| function writeManifestAtomic(manifestPath: string, manifest: BisectPatchManifest): void { | ||
| fs.mkdirSync(path.dirname(manifestPath), { recursive: true }); | ||
| const temporary = temporaryPath(manifestPath); | ||
| try { | ||
| writeManifestFile(temporary, manifest); | ||
| fs.renameSync(temporary, manifestPath); | ||
| } finally { | ||
| fs.rmSync(temporary, { force: true }); | ||
| } | ||
| } | ||
|
|
||
| function writeManifestFile(file: string, manifest: BisectPatchManifest): void { | ||
| fs.writeFileSync(file, `${JSON.stringify(manifest, null, 2)}\n`, { flag: 'wx' }); | ||
| } | ||
|
|
||
| function temporaryPath(file: string): string { | ||
| return `${file}.${process.pid}.${Date.now()}.${Math.random().toString(16).slice(2)}.tmp`; | ||
| } |
There was a problem hiding this comment.
Reuse: reimplements an atomic-write helper that already exists (three times) in this same directory.
writeManifestAtomic/writeManifestFile/temporaryPath reinvent the "write to a temp file, then rename" idiom already present in this package as writeFileAtomic (report.ts), writeJsonAtomic (persistence.ts), and the inline pattern in writeBadRefTestsAtomic (state.ts). A fourth independent implementation means any future fix to the atomic-write contract (e.g. fsync-before-rename, permission bits, error handling) has to be applied by hand in up to four places instead of one shared helper the two-file transaction functions (writeCreateTransaction/replaceArtifactTransaction/removeTransaction) could build on.
| function finishCapture( | ||
| repoDir: string, | ||
| bytes: Buffer, | ||
| source: BisectPatchSource, | ||
| copyIgnore?: CopyIgnoreConfig, | ||
| ): CapturedPatch { | ||
| const files = inspectPatch(repoDir, bytes, copyIgnore); | ||
| return { | ||
| bytes, | ||
| files, | ||
| sha256: createHash('sha256').update(bytes).digest('hex'), | ||
| source, | ||
| }; |
There was a problem hiding this comment.
Efficiency: inspectPatch runs twice on identical bytes for every capture.
Every capture path (captureWorkingTreePatch, captureSourceCommitPatch, importPatchFile) computes paths: patchPaths(repoDir, bytes, options.copyIgnore) — which itself calls inspectPatch(repoDir, bytes, copyIgnore) — as an argument to finishCapture, and finishCapture then calls inspectPatch(repoDir, bytes, copyIgnore) again (line 172) to compute .files. Both calls shell out to git apply --numstat -z --binary - and re-run the same copy-ignore/forbidden-path validation over identical input, doubling subprocess spawn cost on every patch create/patch edit. patchPaths could just derive paths from the files array finishCapture already computes (or finishCapture could take the already-inspected files instead of re-deriving them).
| ): PatchVerificationResult[] { | ||
| const root = gitRoot(repoDir); | ||
| const shas = verificationShas(root, selector, options); | ||
| return shas.map((sha) => verifyAtCommit(root, id, sha, bytes)); |
There was a problem hiding this comment.
Efficiency: patch verification serializes disposable-worktree checks instead of running them concurrently.
verifyPatchBytes does a plain synchronous shas.map(sha => verifyAtCommit(...)), and each verifyAtCommit call does mkdtempSync + git worktree add + up to two git apply calls + git status + git worktree remove/prune — each a blocking execFileSync. For a wide { all: true } or long interval selector (tens of commits), this serializes tens of worktree round-trips instead of overlapping the I/O, inconsistent with the async + Promise.all style used elsewhere in this package (e.g. git.ts, repair-artifacts.ts's resolveApplicableShas). If sequential is intentional (e.g. to avoid git worktree add contention on the same repo), it'd be worth a comment; otherwise this is a straightforward candidate for bounded concurrency.
| return withoutPurpose; | ||
| } | ||
|
|
||
| function validatePatchId(id: string): void { |
There was a problem hiding this comment.
Reuse: validatePatchId duplicates the id regex already enforced by the manifest schema.
/^[A-Za-z0-9][A-Za-z0-9._-]*$/ is repeated here and in BisectPatchManifestEntrySchema.id (patch-manifest.ts). Two independent copies of the "valid patch id" rule in files added by the same PR can drift silently if one is ever tightened/loosened without the other. Also worth noting patch-capture.ts, patch-registry.ts, and patch-cli.ts each define their own local git/gitBuffer/gitRoot/resolveRef wrappers around execFileSync('git', …) rather than sharing one; ./git.ts already exports resolveCommit (used by repair-artifacts.ts in this same diff) that a shared sync equivalent could parallel.
| function writeCreateTransaction( | ||
| manifestPath: string, | ||
| directory: string, | ||
| entry: BisectPatchManifestEntry, | ||
| bytes: Buffer, | ||
| manifest: BisectPatchManifest, | ||
| ): void { | ||
| fs.mkdirSync(directory, { recursive: true }); | ||
| const artifactPath = path.join(directory, entry.filename); | ||
| if (fs.existsSync(artifactPath)) throw new Error(`Patch artifact already exists: ${artifactPath}`); | ||
| const artifactTemporary = temporaryPath(artifactPath); | ||
| const manifestTemporary = temporaryPath(manifestPath); | ||
| try { | ||
| fs.writeFileSync(artifactTemporary, bytes, { flag: 'wx' }); | ||
| writeManifestFile(manifestTemporary, manifest); | ||
| fs.renameSync(artifactTemporary, artifactPath); | ||
| try { | ||
| fs.renameSync(manifestTemporary, manifestPath); | ||
| } catch (error) { | ||
| fs.rmSync(artifactPath, { force: true }); | ||
| throw error; | ||
| } | ||
| } finally { | ||
| fs.rmSync(artifactTemporary, { force: true }); | ||
| fs.rmSync(manifestTemporary, { force: true }); | ||
| } | ||
| } | ||
|
|
||
| function replaceArtifactTransaction( | ||
| manifestPath: string, | ||
| directory: string, | ||
| entry: BisectPatchManifestEntry, | ||
| bytes: Buffer, | ||
| manifest: BisectPatchManifest, | ||
| ): void { | ||
| const artifactPath = path.join(directory, entry.filename); | ||
| const artifactTemporary = temporaryPath(artifactPath); | ||
| const manifestTemporary = temporaryPath(manifestPath); | ||
| const original = fs.readFileSync(artifactPath); | ||
| try { | ||
| fs.writeFileSync(artifactTemporary, bytes, { flag: 'wx' }); | ||
| writeManifestFile(manifestTemporary, manifest); | ||
| fs.renameSync(artifactTemporary, artifactPath); | ||
| try { | ||
| fs.renameSync(manifestTemporary, manifestPath); | ||
| } catch (error) { | ||
| fs.writeFileSync(artifactTemporary, original, { flag: 'wx' }); | ||
| fs.renameSync(artifactTemporary, artifactPath); | ||
| throw error; | ||
| } | ||
| } finally { | ||
| fs.rmSync(artifactTemporary, { force: true }); | ||
| fs.rmSync(manifestTemporary, { force: true }); | ||
| } | ||
| } | ||
|
|
||
| function removeTransaction( | ||
| manifestPath: string, | ||
| directory: string, | ||
| entry: BisectPatchManifestEntry, | ||
| manifest: BisectPatchManifest, | ||
| keepFile: boolean, | ||
| ): void { | ||
| const artifactPath = path.join(directory, entry.filename); | ||
| const heldArtifact = temporaryPath(artifactPath); | ||
| const manifestTemporary = temporaryPath(manifestPath); | ||
| try { | ||
| if (!keepFile) fs.renameSync(artifactPath, heldArtifact); | ||
| writeManifestFile(manifestTemporary, manifest); | ||
| try { | ||
| fs.renameSync(manifestTemporary, manifestPath); | ||
| } catch (error) { | ||
| if (!keepFile) fs.renameSync(heldArtifact, artifactPath); | ||
| throw error; | ||
| } | ||
| if (!keepFile) fs.rmSync(heldArtifact, { force: true }); | ||
| } finally { | ||
| fs.rmSync(manifestTemporary, { force: true }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Simplification: three near-identical ~25-line transaction functions.
writeCreateTransaction, replaceArtifactTransaction, and removeTransaction all do the same "write/rename artifact + write/rename manifest, roll back the artifact if the manifest rename fails" dance with slight variations. Any bugfix to the rollback ordering has to be hand-applied up to three times. A shared helper parameterized by the artifact-side mutation (write-new / replace / remove-or-keep) would collapse this to one implementation with three thin call sites.
| const rawBisect = raw && typeof raw === 'object' | ||
| ? (raw as { bisect?: unknown }).bisect | ||
| : undefined; | ||
| if ( | ||
| rawBisect && typeof rawBisect === 'object' | ||
| && Object.prototype.hasOwnProperty.call(rawBisect, 'repairs') | ||
| ) { | ||
| throw new Error( | ||
| at + 'bisect.repairs is not supported. Manage patches with:\n\n' + | ||
| ' shaka-perf bisect patch create <id>\n\n' + | ||
| 'Patch registrations are stored in bisect-repairs/manifest.json.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Altitude: reintroduces the per-key migration guard the codebase deliberately avoided.
The comment this block replaced explicitly documented the opposite design choice: "No per-key migration guards: every section is .strict(), so a removed or misspelled key is rejected by name on its own. Renames are documented in BREAKING_CHANGES.md rather than restated here — one list to maintain, and no risk of a future rename shipping without its guard." BisectConfigSchema is still .strict(), so Zod would already reject a stray bisect.repairs key with a generic "Unrecognized key(s)" error without this bespoke hasOwnProperty block. This is exactly the kind of special-cased guard the deleted comment says to avoid, and sets a precedent for future renames to get their own hand-written block instead of relying on .strict() + BREAKING_CHANGES.md alone.
| cleanupCommands: z.array(repairCommandSchema), | ||
| registeredAt: z.string(), | ||
| source: z.literal('config'), | ||
| source: z.literal('manifest'), |
There was a problem hiding this comment.
source narrowed from z.literal('config') to z.literal('manifest') with no compat shim. readBisectSession/parseBisectSession (lines 352-360) call sessionSchema.parse(value) directly, so a session.json persisted by pre-PR shaka-perf (with repairs[].source: "config") will throw a raw ZodError ("Invalid literal value, expected 'manifest'") instead of a clear message, on both bisect --resume and bisect --report-only. Worth at least a friendlier error, since every other "can't resume" case in this file raises a domain error rather than letting Zod's raw message through.
| if (outcome.proxied && outcome.code !== 0) { | ||
| throw new Error(outcome.error ?? 'Cannot query bisect lease status'); | ||
| } | ||
| const activeSessionId = outcome.proxied |
There was a problem hiding this comment.
assertMutable's lease check silently no-ops when tryProxy can't reach a compatible server: outcome.proxied is false both when no server is running (intended — nothing to protect against) and when a running server speaks a different PROTOCOL_VERSION (client.ts tryProxy treats a version mismatch the same as "not proxied"). Since this PR bumps PROTOCOL_VERSION 3→4, during a rolling upgrade an old shaka-perf servers process with a live bisect session would make this check pass through as activeSessionId = null, letting patch create/edit/remove/apply mutate the manifest/artifact a live session is relying on. Might be worth treating a version mismatch as "can't verify, assume mutable is unsafe" rather than "assume safe," or at least surfacing a warning.
| AllSelectorSchema, | ||
| ]), | ||
| prepareCommands: z.array(PatchCommandSchema), | ||
| cleanupCommands: z.array(PatchCommandSchema), |
There was a problem hiding this comment.
prepareCommands/cleanupCommands lost the .default([]) the old BisectRepairConfigSchema had for these fields (now bare z.array(...) inside a .strict() object, so they're required keys with no fallback). Every construction path in this repo (the registry's normalizeEntry, the CLI, demo-ecommerce/bisect-repairs/manifest.json) supplies both, so it's not exercised by tests — but a hand-edited manifest.json entry that omits either key (legal under the old per-repair config schema) now fails BisectPatchManifestSchema.safeParse with patches.N.prepareCommands: Required, and loadBisectPatchManifest throws for the whole manifest, not just that entry — every registered patch becomes unusable until it's fixed.
Review summaryRan a multi-angle review (correctness, removed-behavior audit, reuse/simplification/efficiency, altitude/conventions) over the new bisect patch-management code. 12 inline comments posted covering:
One item couldn't be posted inline (GitHub rejects anchors on files with no diff in this PR): 🤖 Generated with Claude Code |
43f8217 to
593bee2
Compare
593bee2 to
4445f60
Compare
| if ( | ||
| rawBisect && typeof rawBisect === 'object' | ||
| && Object.prototype.hasOwnProperty.call(rawBisect, 'repairs') | ||
| ) { | ||
| throw new Error( | ||
| at + 'bisect.repairs is not supported. Manage patches with:\n\n' + | ||
| ' shaka-perf bisect patch create <id>\n\n' + | ||
| 'Patch registrations are stored in bisect-repairs/manifest.json.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
This makes bisect.repairs a hard-rejected, removed config field — a breaking change for any consumer whose abtests.config.ts still has a bisect.repairs array (confirmed by the new "rejects the removed bisect.repairs configuration" test in config.test.ts).
Per this repo's CLAUDE.md ("Breaking changes" section): "Any change that can break an existing consumer's ... abtests.config.ts — a removed/renamed abTest() option, a moved or renamed config field, a changed default — MUST be logged in BREAKING_CHANGES.md under its Unreleased section, with the exact fix for affected tests."
BREAKING_CHANGES.md isn't touched anywhere in this PR's diff. Please add an Unreleased entry documenting the removal and the migration path (shaka-perf bisect patch create <id> + moving existing patches into bisect-repairs/manifest.json), matching the format of the existing entries in that file.
4445f60 to
8b27da6
Compare
| let appliesTo: BisectPatchSelector; | ||
| if (selected === 'all') appliesTo = { all: true }; | ||
| else if (selected === 'commits') { | ||
| const initial = 'commits' in current.appliesTo ? current.appliesTo.commits.join(', ') : ''; | ||
| const commits = splitValues(await prompt.input('Exact commit SHAs (comma-separated)', initial)); | ||
| if (commits.length === 0) throw new Error('At least one exact commit is required'); | ||
| appliesTo = { commits: commits as [string, ...string[]] }; | ||
| } else { | ||
| const interval = 'through' in current.appliesTo ? current.appliesTo : { through: '' }; | ||
| const from = await prompt.input('Inclusive lower SHA (blank uses session good SHA)', interval.from ?? ''); | ||
| const through = await prompt.input('Inclusive upper SHA', interval.through); | ||
| appliesTo = { ...(from ? { from } : {}), through }; | ||
| } |
There was a problem hiding this comment.
promptMetadata (used by patch update) builds the commits / from / through selector directly from raw prompt input, without resolving them through resolveRef the way selectorFromOptions does for patch create/patch edit (line ~364-368, which wraps every --at/--from/--through value in resolveRef(repoDir, ref)).
That means a selector typed during create/edit (via CLI flags or the interactive completeCreateAnswers flow, which also funnels through selectorFromOptions) always gets pinned to an immutable commit SHA at write time. A selector typed during patch update, though, is stored verbatim — e.g. entering HEAD~2 or a branch name for "Inclusive upper SHA" persists that literal string into the manifest. It isn't caught by the manifest schema either (IntervalSelectorSchema/CommitSelectorSchema just require non-empty trimmed strings, not a resolvable/immutable ref).
Since resolveApplicableShas / verifyPatchBytes re-resolve these refs at each future bisect run (via resolveCommit), a mutable ref persisted this way will silently re-resolve to whatever it points to at that later run, rather than staying pinned to what the operator meant when they ran patch update — a reproducibility gap relative to the create/edit paths. Consider routing promptMetadata's selector inputs through the same resolveRef call before returning appliesTo.
Review summaryReviewed the patch-registry/patch-CLI machinery this PR adds for managing bisect repair patches ( Overall: the core design is solid — atomic write/rollback transactions for manifest+artifact updates, hash-verified patch bytes, disposable-worktree verification that never touches the live experiment checkout, Two issues left as inline comments:
No security concerns beyond the above (patch bytes/paths are validated against traversal and configured copy-ignore paths in |
Review summarySolid, well-tested implementation of the patch-manifest/registry/CLI stack (atomic manifest+artifact writes with rollback, sha256 verification, worktree-isolated verification, careful copy-ignore/path-safety checks on captured/imported patches). Two things worth addressing before merge: 1. Missing
|
| if (graph) { | ||
| const allowed = new Set(graph); | ||
| selected = selected.filter((sha) => allowed.has(sha)); | ||
| } | ||
| selected = [...new Set(selected)]; | ||
| if (selected.length === 0) throw new Error('Patch selector does not match any commits in the verification range'); | ||
| return selected; |
There was a problem hiding this comment.
For an appliesTo: { commits: [...] } patch, this silently drops any selected SHA that falls outside the goodRef..badRef graph instead of erroring — it only throws if every selected commit ends up out of range. Since patch verify is documented as verifying "against its configured commit scope," a caller running shaka-perf bisect patch verify <id> <good-ref> <bad-ref> gets a success result with no indication that some of the patch's declared exact commits were never actually exercised. Worth either erroring when a commits selector has a SHA outside the supplied range, or always verifying the full declared selector and using the range only to scope all/interval selectors. No test currently covers a commits selector combined with a good/bad range that excludes one of the listed commits.
|
@claude help |
|
@claude code-review |
| async function assertMutable( | ||
| context: PatchCliContext, | ||
| deps: BisectPatchCliDependencies, | ||
| ): Promise<void> { | ||
| if (deps.assertMutable) return deps.assertMutable(context); | ||
| if (!context.projectSlug) return; | ||
| const outcome = await (deps.tryProxy ?? tryProxy)({ | ||
| slug: context.projectSlug, | ||
| request: { v: PROTOCOL_VERSION, cmd: 'bisect-status' }, | ||
| }); | ||
| if (!outcome.proxied) { | ||
| if (/^manifest v\d+, this CLI speaks v\d+$/.test(outcome.reason)) { | ||
| throw new Error( | ||
| `Cannot verify the bisect lease because ${outcome.reason}. ` + | ||
| 'Restart the running `shaka-perf servers` process and retry.', | ||
| ); | ||
| } | ||
| return; | ||
| } | ||
| if (outcome.proxied && outcome.code !== 0) { | ||
| throw new Error(outcome.error ?? 'Cannot query bisect lease status'); | ||
| } | ||
| const activeSessionId = (outcome.data as { | ||
| activeSessionId?: string | null; | ||
| } | undefined)?.activeSessionId; | ||
| if (activeSessionId) { | ||
| throw new Error( | ||
| `Cannot modify bisect patches while session "${activeSessionId}" owns the project`, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Check-then-act race on the bisect lease. assertMutable queries bisect-status once (line 537-539) and returns if no session is active, but the actual mutation (registry.create/edit/apply/remove, all plain local filesystem writes) happens afterward with no lock held for the gap in between — for create, that gap includes an unbounded interactive prompt sequence (completeCreateAnswers).
A concurrent beginBisectSession can acquire the lease in that window (it's a locked/queued runProxiedAction on the server side), so bisect-status can observe activeSessionId: null a moment before the session actually starts. The patch mutation then proceeds and rewrites bisect-repairs/manifest.json/the artifact while a bisect run is starting or already using the very state session.ts explicitly snapshots at bisect start to avoid mid-session edits leaking in — exactly the hazard this check exists to prevent.
Also worth a look while touching this function: the protocol-mismatch branch on line 542 detects an IPC version skew by regex-matching outcome.reason, a free-text string only ever produced for console.error debugging in twin-servers/ipc/client.ts:76. Nothing ties the two together at compile time — if that log wording ever changes, the regex silently stops matching and assertMutable falls through to if (!outcome.proxied) return, treating an undetected protocol mismatch as "no server running, safe to proceed."
| entry, | ||
| artifactPath, | ||
| files: inspectPatchAtRoot(this.root(), bytes), | ||
| hashValid: createHash('sha256').update(bytes).digest('hex') === entry.sha256, |
There was a problem hiding this comment.
describe() calls inspectPatchAtRoot(this.root(), bytes) with no copyIgnore argument, so it always falls back to defaultCopyIgnoreConfig() (see patch-capture.ts:150-171, which throws Patch targets configured copy-ignore path: ... for any file matching that config).
PatchRegistryOptions (line 34-38) has no copyIgnore field at all, even though PatchCliContext carries one and every CLI call site does new BisectPatchRegistry({ ...context }) — the field is silently dropped. capture()/patch-capture.ts correctly thread the project's configured copyIgnore through at capture time, but every later read path (list, get, apply, verify — all funnel through describe()) re-validates against the hardcoded default instead.
If a project's twinServers.copyIgnore differs from the default (e.g. drops audit-results), a patch that was captured and registered successfully will throw on every subsequent list/get/apply/verify. Worse, list() (line 59-61) maps describe() over every manifest entry with no per-entry try/catch, so one such patch throws and breaks patch list for the entire registry, not just that patch.
|
|
||
| function removeTransaction( | ||
| manifestPath: string, | ||
| directory: string, | ||
| entry: BisectPatchManifestEntry, | ||
| manifest: BisectPatchManifest, | ||
| keepFile: boolean, | ||
| ): void { | ||
| const artifactPath = path.join(directory, entry.filename); | ||
| const heldArtifact = temporaryPath(artifactPath); | ||
| const manifestTemporary = temporaryPath(manifestPath); | ||
| try { | ||
| if (!keepFile) fs.renameSync(artifactPath, heldArtifact); | ||
| writeManifestFile(manifestTemporary, manifest); | ||
| try { | ||
| fs.renameSync(manifestTemporary, manifestPath); | ||
| } catch (error) { | ||
| if (!keepFile) fs.renameSync(heldArtifact, artifactPath); | ||
| throw error; | ||
| } | ||
| if (!keepFile) fs.rmSync(heldArtifact, { force: true }); | ||
| } finally { | ||
| fs.rmSync(manifestTemporary, { force: true }); | ||
| } | ||
| } |
There was a problem hiding this comment.
removeTransaction renames the live artifact aside at line 321 (fs.renameSync(artifactPath, heldArtifact)) before attempting writeManifestFile(manifestTemporary, manifest) on the next line — but only the later fs.renameSync(manifestTemporary, manifestPath) has a catch that restores the artifact. If writeManifestFile itself throws (e.g. ENOSPC/EACCES writing to bisect-repairs/), the outer finally only removes manifestTemporary; heldArtifact is never renamed back to artifactPath.
Result: the manifest still lists the entry pointing at <id>.patch, but the file now sits under a random .patch.<pid>.<ts>.<rand>.tmp name, so get()/list()/apply() all throw Cannot read bisect patch "<id>" until someone manually renames it back.
Contrast with writeCreateTransaction/replaceArtifactTransaction above, which always write the manifest temp file before touching the live artifact — removeTransaction reverses that ordering, which is what opens this gap.
| const bytes = fs.readFileSync(patch.artifactPath); | ||
| const reverse = options.reverse === true; | ||
| if (!reverse && !canApply(repoDir, bytes, false)) { | ||
| const clean = isClean(repoDir); |
There was a problem hiding this comment.
isClean() (defined at line 361-363) is a bare git status --porcelain emptiness check — it reimplements requireClean() from git.ts but drops its allowedPaths exception, which the rest of the bisect module relies on to tolerate known-OK dirty paths (e.g. generated artifact dirs) when judging a repo clean.
Used here in apply(), this means: if the experiment repo has an allowed-dirty artifact present (exactly the case requireClean(..., { allowedPaths }) exists to handle elsewhere in bisect), patch apply will treat the tree as dirty and throw Patch "<id>" appears in uncommitted working-tree changes; clean the repository first, even though the rest of the bisect flow would consider that same tree clean. Worth reusing requireClean's allowedPaths-aware check here instead of a second, stricter implementation.
| updateMetadata(id: string, metadata: PatchMetadata): RegisteredPatch { | ||
| const loaded = this.load(); | ||
| const index = loaded.manifest.patches.findIndex((patch) => patch.id === id); | ||
| if (index < 0) throw new Error(`Unknown bisect patch "${id}"`); | ||
| const entry = normalizeEntry({ ...loaded.manifest.patches[index]!, ...metadata }); | ||
| const patches = [...loaded.manifest.patches]; | ||
| patches[index] = entry; | ||
| verifyPatchBytes( | ||
| this.options.repoDir, | ||
| entry.id, | ||
| entry.appliesTo, | ||
| fs.readFileSync(path.join(loaded.directory, entry.filename)), |
There was a problem hiding this comment.
Minor: updateMetadata reads the artifact bytes straight off disk (line 108) and hands them to verifyPatchBytes without first checking they match entry.sha256 — unlike apply(), which calls this.get(id) and explicitly throws when !patch.hashValid. If an artifact under bisect-repairs/ gets corrupted or hand-edited outside the CLI, patch update will "successfully" verify and persist metadata changes against the wrong bytes and report success, silently rewriting the manifest around a corrupt artifact. The corruption only surfaces later via apply()/verify()/list().
Review summaryThis PR migrates bisect repair config into a managed patch manifest/registry with a large new CLI surface ( Left 5 inline comments on
Also noted for awareness (not filed inline, lower priority): several |
1 participant
Summary
Stack
Depends on the universal-repairs PR. This is the fourth PR in the bisect repair stack.
Validation