Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ For features with background jobs, polling, retries, cancellation, or shutdown:
- Bound the HTTP drain during shutdown. After its grace period, abort and await owned work before awaiting server closure so an admitted poll cannot deadlock teardown.
- Polling endpoints should read only the state they need. Do not rebuild Git history or the full review merely to retrieve background-job status.
- Back off recurring external-status polling to a bounded cap. Reset the interval only after a meaningful lifecycle change or explicit user action.
- When synchronous work moves behind an await (a worker thread, a subprocess, an async API), list every await it adds and apply the local-generation rule under "Guarded external actions" to each one. A local await must not follow the final remote validation: the last await before an irreversible external action is that validation.
- Work that leaves a serializing path (the event loop, a lock, a single queue) needs its own concurrency bound. Queued work must honour cancellation and deadlines.
- Never compare clock readings across threads or processes. Pass a relative budget and let each side count it on its own monotonic clock.
- A test that runs the server in the test's own process must wait for observable state before acting. Do not rely on the server blocking the test's event loop to order steps.

## Async review UI

Expand Down
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,15 +74,15 @@ const { plan, warnings } = importPlan(planText, 'yaml', {
allowedCommands: [['npm', 'test']],
issue: 412,
}, nextRevision);
const history = readHistory(repoPath, baseCommit, headCommit);
const history = await readHistory(repoPath, baseCommit, headCommit);
const segments = linkHistory(plan, history, trustedCommitLedger, checkoutPathKey);
```

Inputs such as `planText` and the ledger must come from the trusted runner. `runner/store` owns the database, persistent request lifecycle, revision allocation, and atomic Apply. The browser sends review commands through `runner/review`; it cannot write ledger ownership or approval fingerprints. See [storage decisions](docs/implementation/persistent-review-store.md).

## Current limits and safety

- History must be linear and descend from the requested base. Merge histories are rejected with a rebase instruction; repositories using grafts, shallow ancestry, object alternates, or symlinked object storage are rejected (storage inspection is limited to 100,000 entries). Reads are bounded to 500 commits, 32 MiB per Git response, 64 MiB of unique blob bytes across the history (callers may lower `maxBlobBytes`), 8 MiB of cumulative diff output, 20,000 cumulative file records (including the final diff), and a shared 30-second monotonic deadline for a read. Git children receive only the remaining time and are killed on timeout. Filesystem inspections check the deadline between operations; a blocked filesystem syscall still requires an external worker supervisor. Oversized work fails explicitly.
- History must be linear and descend from the requested base. Merge histories are rejected with a rebase instruction; repositories using grafts, shallow ancestry, object alternates, or symlinked object storage are rejected (storage inspection is limited to 100,000 entries). Reads are bounded to 500 commits, 32 MiB per Git response, 64 MiB of unique blob bytes across the history (callers may lower `maxBlobBytes`), 8 MiB of cumulative diff output, 20,000 cumulative file records (including the final diff), and a shared 30-second monotonic deadline for a read. Git runs as asynchronous child processes that receive only the remaining time; a timeout or an abort `signal` kills the running one, and the read settles only after it has exited. Filesystem inspections check the deadline between operations. The review server runs history reads and linking in a worker thread (`runner/history-job.ts`), so they never block it; a filesystem syscall that blocks still holds that worker until it returns. Oversized work fails explicitly.
- Linking separately bounds cumulative split lines and candidate segments to 100,000 each, reference/work operations to 1,000,000, and text/origin strings to 32 Mi UTF-16 code units. It checks before expanding lines/origin sets; grouping no longer repeatedly splits accumulated content. Callers may lower these budgets. Linking has a 30-second overall deadline and each line diff uses at most 2 seconds or the remaining total, whichever is smaller.
- The caller selects and trusts the repository and its Git administrative directory. Normal Git discovery, linked-worktree gitfiles, and symlinked gitdirs are supported; object-storage links and alternates inside that selected gitdir are rejected. This adapter is not a filesystem-containment boundary for untrusted repository roots.
- Git administrative metadata and object storage must remain unchanged during a read; these library checks do not isolate a concurrently hostile filesystem.
Expand Down
16 changes: 16 additions & 0 deletions core/path-identity.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/** How the checkout compares paths. Linking needs it to decide whether two paths name the same file. */
export interface PathIdentity { caseSensitive: boolean; unicodeNormalization: 'none' | 'NFC' }

export function validPathIdentity(identity: PathIdentity | undefined): identity is PathIdentity {
return typeof identity?.caseSensitive === 'boolean' && ['none', 'NFC'].includes(identity.unicodeNormalization);
}

/** The comparison key of a path under a checkout's path identity. */
export function pathKeyFor(identity: PathIdentity): (path: string) => string {
if (!validPathIdentity(identity)) throw new Error('Known checkout path identity is required.');
return path => {
if (!identity.caseSensitive && /[^\x20-\x7e]/.test(path)) throw new Error('Non-ASCII case-insensitive paths require a filesystem-specific identity adapter.');
const normalized = identity.unicodeNormalization === 'NFC' ? path.normalize('NFC') : path;
return identity.caseSensitive ? normalized : normalized.toLowerCase();
};
}
15 changes: 15 additions & 0 deletions docs/implementation/read-only-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,3 +130,18 @@ The answer polling endpoint previously called `ReviewService.load()` every two s
The displayed review remains the owner of note text, ordering, snapshot, plan revision, assignment, and reference-validity metadata. Poll responses update only `answer` and `answerActive` for IDs already displayed. The existing generation guard rejects responses started before an action or Refresh, and unknown note IDs are ignored. Explicit Refresh and mutating actions still call the full review service to validate current repository state.

The focused browser regression observes a completed answer and its durable SQLite state with zero review loads, verifies that the status payload omits note text, then proves explicit Refresh still performs a full load. Existing regressions continue to cover a poll returning after a newer submission and cancellation remaining active after a persisted attempt fails.

## History work off the server thread

Review loading used to run history reading and linking on the server's event loop. Git ran as synchronous child processes and object storage was walked synchronously, so one load could stop every other request, cancellation and shutdown for up to its 30-second deadline.

`ReviewService.load` now runs both in a worker thread (`runner/history-job.ts`, `runner/history-worker.ts`). The rest of the load (Store reads and writes, approval states and the token) stays on the server thread.

- **Lifecycle.** A job is running until its worker replies, fails or exits. Its promise settles only after the worker thread has ended; if terminating the worker fails, the job waits for the thread's exit.
- **Concurrency.** At most two history workers run at once (`MAX_HISTORY_WORKERS`), so tabs and refreshes cannot start any number of threads and Git processes. A later job waits for a slot, and each slot is held until its worker thread has ended. A waiting job that is aborted, or whose budget runs out while it waits, never starts: the wait counts against its budget.
- **Deadline.** What the load's overall deadline leaves is the worker's budget, counted from the worker's start: the history read gets what remains when it starts, and linking gets what the read left. If the worker has not answered one second after its budget, the server cancels it; until the worker reports that it has started, it also gets five seconds to start. The load still refuses a result that arrives after its own overall deadline.
- **Cancellation.** An abort (a request ending at shutdown, a merge or preparation deadline) sends the worker a cancel. The worker kills its running Git process, waits for it to exit, and replies. A worker that has not replied within one second is terminated: one in synchronous work (linking, blob decoding, the object storage walk), which runs no Git process, or, rarely, one whose killed Git process is stuck in the kernel, which then outlives the load. The load rejects with the abort's own reason.
- **Staleness.** The plan, snapshot, review version and ledger ownership are read before the worker starts. Every ledger write records a new snapshot, so an unchanged review version, plan revision and snapshot after the worker mean its inputs are current. If they changed (another load recorded a new HEAD, or a write landed), the load reads the history again from the current state, in at most three reads in all within its deadline, and is refused as stale only after that.
- **Review commands.** A command applies, synchronously, to a review loaded just before it (inside the recorded action when it has an action ID). Its writes are guarded by that review's state. A load failure before the command is not recorded under its action ID, so the same ID can be resent. If the reload after the command was applied fails, or shutdown stops it, the refusal says `outcomeUnknown`, and a question saved during shutdown still gets its retryable "Server stopped" answer.
- **Merge and preparation.** Every merge load takes the click's operation signal; a replayed click's status refresh takes the coordinator's shutdown signal, and shutdown waits for it. Every preparation load takes the preparation's signal. A review load awaits a worker, so neither runs one after its final remote read: merge admission inspects the pull request once more after its final review load, and preparation inspects it after its last review load. Both then check local trust and the review's generations in the same turn as the write. Preparation also checks its generations after reading the old history it rebases and before it records a moved pull request head. Each write is either in the same turn as those checks, or (a rebase, after its authorization read) guarded again by the Store in its own transaction.

Loading
Loading