From efbf420e909c1f6e9cfefb4ddcaae5a6d54e22c5 Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 23 Sep 2026 08:48:15 -0700 Subject: [PATCH 1/8] Add local plan-indexed review screen and experiment setup --- .github/workflows/ci.yml | 2 + .gitignore | 4 + README.md | 4 +- docs/experiments/review-protocol.md | 40 ++++++++++ docs/implementation/read-only-review.md | 30 ++++++++ package-lock.json | 66 +++++++++++++++++ package.json | 8 +- playwright.config.ts | 2 + runner/review.ts | 84 +++++++++++++++++++++ runner/store.ts | 33 +++++++-- scripts/demo.ts | 40 ++++++++++ scripts/plant.ts | 80 ++++++++++++++++++++ test/browser/review.spec.ts | 47 ++++++++++++ test/plant.test.ts | 17 +++++ test/review.test.ts | 31 ++++++++ tsconfig.json | 27 +++++-- vitest.config.ts | 2 + web/cli.ts | 19 +++++ web/public/app.js | 98 +++++++++++++++++++++++++ web/public/index.html | 11 +++ web/public/style.css | 5 ++ web/server.ts | 44 +++++++++++ 22 files changed, 680 insertions(+), 14 deletions(-) create mode 100644 docs/experiments/review-protocol.md create mode 100644 docs/implementation/read-only-review.md create mode 100644 playwright.config.ts create mode 100644 runner/review.ts create mode 100644 scripts/demo.ts create mode 100644 scripts/plant.ts create mode 100644 test/browser/review.spec.ts create mode 100644 test/plant.test.ts create mode 100644 test/review.test.ts create mode 100644 vitest.config.ts create mode 100644 web/cli.ts create mode 100644 web/public/app.js create mode 100644 web/public/index.html create mode 100644 web/public/style.css create mode 100644 web/server.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 836b526..ab3d4c5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,3 +17,5 @@ jobs: - run: npm ci --ignore-scripts - run: npm run typecheck - run: npm test + - run: npx playwright install --with-deps chromium + - run: npm run test:browser diff --git a/.gitignore b/.gitignore index c938c74..cad3c41 100644 --- a/.gitignore +++ b/.gitignore @@ -2,3 +2,7 @@ node_modules/ coverage/ dist/ .DS_Store + +.codeboost-local/ +playwright-report/ +test-results/ diff --git a/README.md b/README.md index 9c5b0cf..9a1aca1 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ Review agent-made Git changes one plan item at a time. The approved plan lists each item's files and acceptance checks; the review engine shows which item produced each change and flags foreign or overlapping work. -**Status:** the first library slice is implemented. There is no application, server, agent runner, database, or merge command yet. Follow the [build order](docs/designs/codeboost-plan-indexed-review.md#build-order-and-the-gono-go-check); the read-only screen and real-PR go/no-go experiment come before agent execution. +**Status:** the plan/linking library, SQLite store, and local read-only review screen are implemented. Run `npm run demo` and open its private local URL. There is no agent execution or merge command. The human go/no-go experiment is still pending; see [the local review guide](docs/implementation/read-only-review.md). ## Development @@ -41,7 +41,7 @@ const history = readHistory(repoPath, baseCommit, headCommit); const segments = linkHistory(plan, history, trustedCommitLedger, checkoutPathKey); ``` -Inputs such as `planText` and the ledger must come from the caller. The future `runner/store` owns the database and ledger; this library does not infer them from commit messages. Before saving a suggested edit, the store must load its captured identity/revision binding by opaque suggestion ID, reject canceled or consumed IDs, and compare-and-swap the plan revision plus consume/invalidate old suggestions in one transaction. The pure `applySuggestion` function requires that trusted binding and validates a copy, but cannot lock storage or prevent replay by itself. Applying one card stales its siblings; refresh and review regenerated cards before the next Apply. Approvals and choices likewise require the stored plan identity. +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 diff --git a/docs/experiments/review-protocol.md b/docs/experiments/review-protocol.md new file mode 100644 index 0000000..8291265 --- /dev/null +++ b/docs/experiments/review-protocol.md @@ -0,0 +1,40 @@ +# Plan-indexed review: go/no-go protocol + +Status: **not started; issue pairs and human reviewer still required**. This document does not claim a passed gate. Complete the pair table and commit its exact revision before any timed review. No pair or threshold may change once the first review begins. + +## Pair selection and order + +Choose four pairs of comparable, small real issues in a repository owned by the reviewer. Match each pair on scope, language, and estimated review complexity before implementing either issue. Record issue URLs, PR URLs, base/head SHAs, and which review method each gets. Do not reuse the demo fixture as an experimental issue. + +| Pair | Real issue A / PR | Real issue B / PR | A method | B method | +|---|---|---|---|---| +| 1 | Pending selection | Pending selection | Raw GitHub diff | codeboost | +| 2 | Pending selection | Pending selection | codeboost | Raw GitHub diff | +| 3 | Pending selection | Pending selection | Raw GitHub diff | codeboost | +| 4 | Pending selection | Pending selection | codeboost | Raw GitHub diff | + +The reviewer must not implement the paired changes or inspect the plants before deciding on the PR. An operator prepares plans and commits by hand, records trusted ledger ownership, and supplies both the plan and code to each review method. The earlier one-issue manual assignment also remains required: record how every change maps to the attribution table and turn unclear cases into fixtures. + +## Planting + +The operator runs `node scripts/plant.ts review.json NEW_DIRECTORY plant-input.json`. The input contains `declaredText`, `undeclaredText`, and a fresh top-level `undeclaredPath`. The helper creates a separate local clone and database, randomly selects eligible owned commits, amends those commits while preserving readable messages/trailers, records old/new mappings and ownership, and writes the returned `review.json` for the local UI. It never pushes or changes the source repository. Both plants are owned through the ledger, so the undeclared-file plant is out of scope rather than foreign. + +The helper supports linear histories and regular, top-level declared files retained through the remaining history. It refuses unsupported targets and stops on cherry-pick conflicts; the operator must inspect a failed disposable clone and choose a fresh output path, never silently resolve a conflict or alter the frozen issue pair. Plant text must represent a plausible unrelated code change, not an obvious marker. The automated test uses obvious markers only to verify the helper. + +`sealed.json` outside the clone records locations and selected commits with owner-only file permissions. It is unblinding data, not encryption. The operator must withhold it from the reviewer until that PR's decision is recorded. Do not show CLI input or sealed data to the reviewer. Open PRs only after inspecting the prepared branch; PR publication is a separate action. + +## Measurement + +Time each review from first opening to the recorded decision. Record elapsed seconds, unexplained changes, the undeclared-file plant found (yes/no), and declared-file plant found (yes/no). Record decisions before unblinding. Also record interruptions and tool failures without silently dropping trials. + +Pass only when all are true: + +- codeboost catches at least 3 of its 4 undeclared-file plants and more than raw-diff review catches; +- median codeboost time is no slower than median raw-diff time; +- no change is left unexplained. + +Report declared-file catch rate for both methods without a pass threshold. Stop after four pairs. If within one catch of the bar, perform four more preselected pairs once, then decide. Do not add further trials. Record the outcome in a committed results document before merge/agent/planning/queue/learning implementation begins. + +## Current evidence + +Engineering tests prove browser interactions, persistence, attribution, and planting mechanics. They do **not** establish human review speed or catch rates. No real issue pairs, timed decisions, or catch-rate results exist yet. diff --git a/docs/implementation/read-only-review.md b/docs/implementation/read-only-review.md new file mode 100644 index 0000000..8102d63 --- /dev/null +++ b/docs/implementation/read-only-review.md @@ -0,0 +1,30 @@ +# Read-only review screen (#3) + +Started from `dd6a1e7`, after merging the SQLite store PR. This delivers the local review surface and experiment tooling; issue #3 remains open until the human go/no-go work is complete. + +## Run it + +`npm ci --ignore-scripts`, then `npm run demo`. Open the private loopback URL printed by the command. The demo uses a real disposable Git repository and SQLite database under ignored `.codeboost-local/demo`; restart preserves approvals and notes. No source files in the codeboost checkout are edited by the demo. Stop with Ctrl+C. + +For an existing trusted store, use `npm start -- --config /absolute/path/review.json`. The JSON has `database`, `repository`, `identity` (`repositoryId`, `taskId`, `planId`), and `pathIdentity` (`caseSensitive` boolean, `unicodeNormalization` equal to `none` or `NFC`). Paths should be absolute. Supply actual checkout identity rules, not an OS guess. The demo probes its local filesystem; it uses only simple ASCII fixture paths. Unsupported or more complex filesystem equivalence must not be approximated by this adapter. + +The selected repository's current HEAD is compared to the stored base. Refresh observes new heads and creates an immutable snapshot; approvals are recomputed for the resulting changes. It does not fetch, rebase, run tests, invoke agents, or merge. Histories still obey the library's linear/complete/bounded-history constraints. + +## UI and storage + +The app follows the Evidence Desk tokens and self-hosts IBM Plex Sans/Mono. It has item and exception rows, four status checks, a provenance gutter, file metadata, shared-hunk labels, assignments, standalone acceptance, no-change confirmation, approval counts/staleness, before/after approval evidence, per-item questions/change requests, keyboard controls, resizable/collapsible side panes, desktop breakpoints, and loading/error/empty states. Questions are saved for discussion; no AI answer is fabricated. Tests/AI review are explicitly not run. Change requests remain pending for a future revision workflow. + +The server binds only 127.0.0.1 and requires its random private token for APIs. Host/origin checks, a restrictive CSP, bounded UTF-8 JSON bodies, and DOM escaping prevent another website from reading or writing local review state. Browser commands contain item/segment IDs and the reviewed state token, never approval fingerprints or ledger ownership. The runner derives those from Git/store data. A database review counter prevents concurrent review actions from approving unseen assignments; plan and snapshot CAS remain enforced. + +Store schema v2 adds the review counter and per-item notes through a transactional v1 migration. The test suite verifies existing revisions and ledger entries survive. This is an automatic local SQLite migration, not a migration against a shared environment. + +## Known limits and remaining gates + +- No agent answers, test execution, AI findings ingestion, send-to-agent action, or merge control is included. These belong after the go/no-go gate. Four checks distinguish unavailable evidence from success. +- File cards show mode/path/object IDs; binary/image previews and byte-size retrieval are not implemented and explicitly say unavailable. The pure history API does not expose binary bytes. +- The protocol at `docs/experiments/review-protocol.md` must be filled with the real pairs and committed before the first timed review. The manual assignment, real paired PRs, human timing, and final result are pending. Do not mark issue #3 closed or claim the gate passed. +- The planting helper is intentionally limited to disposable clones and supported regular top-level paths; it never publishes PRs. + +## Validation + +Baseline before this slice: 157 tests. Run `npm run typecheck`, `npm test`, and `npm run test:browser`. Browser tests use real Git and SQLite with Chromium and cover persistence, assignments, metadata, no-change approval, stale views, unsafe origins, keyboard/breakpoints, error display, and untrusted text. A browser regression exposed false stale reasons from JSON field order; structural comparison replaced that check. The plant test verifies source HEAD stays unchanged and both plants retain ledger ownership. diff --git a/package-lock.json b/package-lock.json index b9320b5..dbdc3e5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8,11 +8,14 @@ "name": "codeboost", "version": "0.0.0", "dependencies": { + "@fontsource/ibm-plex-mono": "5.3.0", + "@fontsource/ibm-plex-sans": "5.3.0", "ajv": "8.20.0", "diff": "9.0.0", "yaml": "2.9.1" }, "devDependencies": { + "@playwright/test": "1.63.0", "@types/node": "26.6.2", "typescript": "7.0.2", "vitest": "5.0.1" @@ -21,6 +24,24 @@ "node": ">=26.7.0" } }, + "node_modules/@fontsource/ibm-plex-mono": { + "version": "5.3.0", + "resolved": "https://registry.npmjs.org/@fontsource/ibm-plex-mono/-/ibm-plex-mono-5.3.0.tgz", + "integrity": "sha512-eTgnZjZEGk1QtD3ZstF+Vclo2HLAni8YMy34/DxllwZvyz1lR/1RF/xTiAquOBO7MvqBx8D2Ig2WCPMVfdZu7Q==", + "license": "OFL-1.1", + "funding": { + "url": "https://github.com/sponsors/ayuhito" + } + }, + "node_modules/@fontsource/ibm-plex-sans": { + "version": "5.3.0", + "resolved": "https://registry.npmjs.org/@fontsource/ibm-plex-sans/-/ibm-plex-sans-5.3.0.tgz", + "integrity": "sha512-CbE4CbbEEZJX860XyUiRpsksXIQR8Rp2XDva2VO53NJox9tVNtusrysd2x5YkUEY3ErQ66W1IiiQL8/wihhw5w==", + "license": "OFL-1.1", + "funding": { + "url": "https://github.com/sponsors/ayuhito" + } + }, "node_modules/@jridgewell/resolve-uri": { "version": "3.1.2", "resolved": "https://registry.npmjs.org/@jridgewell/resolve-uri/-/resolve-uri-3.1.2.tgz", @@ -60,6 +81,22 @@ "url": "https://github.com/sponsors/oxc-project" } }, + "node_modules/@playwright/test": { + "version": "1.63.0", + "resolved": "https://registry.npmjs.org/@playwright/test/-/test-1.63.0.tgz", + "integrity": "sha512-oxMK4vllB9RK5NQ2l1pq1IfOf2AvnEuj/vYGDj0H2nMtmtZpKtCwt/l00GEO6xjGfpBNAvjovvYdCm50dRQkpQ==", + "dev": true, + "license": "Apache-2.0", + "dependencies": { + "playwright": "1.63.0" + }, + "bin": { + "playwright": "cli.js" + }, + "engines": { + "node": ">=20" + } + }, "node_modules/@rolldown/binding-android-arm-eabi": { "version": "1.2.9", "resolved": "https://registry.npmjs.org/@rolldown/binding-android-arm-eabi/-/binding-android-arm-eabi-1.2.9.tgz", @@ -1264,6 +1301,35 @@ "url": "https://github.com/sponsors/jonschlinkert" } }, + "node_modules/playwright": { + "version": "1.63.0", + "resolved": "https://registry.npmjs.org/playwright/-/playwright-1.63.0.tgz", + "integrity": "sha512-+7ziBLidS4NaNCdt57SUDT+wYmmd5fmiQejUic/kb+YsYSCPyOOE9sebzMjNmQrsnNpDJqd4WHvV/8lfKfUDUg==", + "dev": true, + "license": "Apache-2.0", + "dependencies": { + "playwright-core": "1.63.0" + }, + "bin": { + "playwright": "cli.js" + }, + "engines": { + "node": ">=20" + } + }, + "node_modules/playwright-core": { + "version": "1.63.0", + "resolved": "https://registry.npmjs.org/playwright-core/-/playwright-core-1.63.0.tgz", + "integrity": "sha512-rYCsBF/M5HjUch52bbtVONEFjv6Xu8sm8h72dNlR5bzIE1fvC/bxgspzkjSfU+MweEMmPM8KJebG6nnyxo5mCg==", + "dev": true, + "license": "Apache-2.0", + "bin": { + "playwright-core": "cli.js" + }, + "engines": { + "node": ">=20" + } + }, "node_modules/postcss": { "version": "8.5.28", "resolved": "https://registry.npmjs.org/postcss/-/postcss-8.5.28.tgz", diff --git a/package.json b/package.json index ff7fed9..d613eb6 100644 --- a/package.json +++ b/package.json @@ -8,14 +8,20 @@ }, "scripts": { "test": "vitest run", - "typecheck": "tsc --noEmit" + "typecheck": "tsc --noEmit", + "start": "node web/cli.ts", + "demo": "node web/cli.ts --demo", + "test:browser": "playwright test" }, "dependencies": { + "@fontsource/ibm-plex-mono": "5.3.0", + "@fontsource/ibm-plex-sans": "5.3.0", "ajv": "8.20.0", "diff": "9.0.0", "yaml": "2.9.1" }, "devDependencies": { + "@playwright/test": "1.63.0", "@types/node": "26.6.2", "typescript": "7.0.2", "vitest": "5.0.1" diff --git a/playwright.config.ts b/playwright.config.ts new file mode 100644 index 0000000..79d8a1a --- /dev/null +++ b/playwright.config.ts @@ -0,0 +1,2 @@ +import { defineConfig } from '@playwright/test'; +export default defineConfig({ testDir: './test/browser', fullyParallel: false, workers: 1, use: { browserName:'chromium', viewport:{width:1512,height:982}, trace:'retain-on-failure' } }); diff --git a/runner/review.ts b/runner/review.ts new file mode 100644 index 0000000..8b8e490 --- /dev/null +++ b/runner/review.ts @@ -0,0 +1,84 @@ +import { createHash } from 'node:crypto'; +import { basename } from 'node:path'; +import { isDeepStrictEqual } from 'node:util'; +import { Store, type ReviewState } from './store.ts'; +import type { PlanIdentity } from '../core/identity.ts'; +import { readHistory } from '../git/history.ts'; +import { linkHistory } from '../core/linking.ts'; +import { applyChoices, approvalStates, approveItem, choiceKeys } from '../core/approvals.ts'; + +export interface ReviewConfig { database: string; repository: string; identity: PlanIdentity; pathIdentity: { caseSensitive: boolean; unicodeNormalization: 'none' | 'NFC' }; demo?: boolean } +export class ReviewService { + store: Store; + config: ReviewConfig; + constructor(config: ReviewConfig) { + if (typeof config.pathIdentity?.caseSensitive !== 'boolean' || !['none', 'NFC'].includes(config.pathIdentity.unicodeNormalization)) throw new Error('Known checkout path identity is required.'); + this.config = config; this.store = new Store(config.database); + } + close() { this.store.close(); } + load() { + const { identity, repository, pathIdentity } = this.config; + const reviewVersion = this.store.reviewVersion(identity); + const plan = this.store.getPlan(identity); + let snapshot = this.store.getSnapshot(identity); + // HEAD changes are observed; no Git mutation is performed by the review service. + const history = readHistory(repository, snapshot.base, 'HEAD'); + if (history.head !== snapshot.head) snapshot = this.store.recordHistory(identity, { revision: plan.revision, snapshotId: snapshot.id, reviewVersion }, history.base, history.head, []); + const pathKey = (path: string) => { + const normalized = pathIdentity.unicodeNormalization === 'NFC' ? path.normalize('NFC') : path; + return pathIdentity.caseSensitive ? normalized : normalized.toLowerCase(); + }; + const raw = linkHistory(plan, history, this.store.ownership(identity, plan.revision), pathKey); + const saved = this.store.getReview(identity), keys = choiceKeys(raw, identity); + const segments = applyChoices(plan, raw, saved.choices, identity).map((segment, index) => { + const target = plan.items.find(item => item.id === segment.row); + if (target && segment.row !== raw[index]!.row) { + const declared = new Set(target.files.flatMap(file => [file.path, ...(file.renamed_from ? [file.renamed_from] : [])]).map(pathKey)); + segment.scope = declared.has(pathKey(segment.path)) ? 'in-scope' : 'out-of-scope'; + } + return { ...segment, key: keys[index]!, originalRow: raw[index]!.row }; + }); + const states = approvalStates(plan, segments, saved.approvals, identity); + const expected: ReviewState = { revision: plan.revision, snapshotId: snapshot.id, reviewVersion }; + const notes = this.store.getReviewNotes(identity); + if (this.store.reviewVersion(identity) !== reviewVersion || this.store.getPlan(identity).revision !== plan.revision || this.store.getSnapshot(identity).id !== snapshot.id) throw new Error('Stale review state. Reload before writing.'); + const items = plan.items.map(item => { + const owned = segments.filter(segment => segment.row === item.id); + const ambiguous = segments.filter(segment => segment.row === 'Ambiguous' && segment.owners.includes(item.id)).length; + const outside = owned.filter(segment => segment.scope === 'out-of-scope').map(segment => segment.path); + const prior = saved.approvals.find(approval => approval.item === item.id); + const before = prior ? JSON.parse(prior.fingerprint) : null; + const reasons: string[] = []; + if (states[item.id] === 'stale') { + if (before && !isDeepStrictEqual(before.item.acceptance, item.acceptance)) reasons.push('Acceptance checks changed'); + if (before && owned.some(segment => before.segments.some((old: { path: string; content: string; context: string }) => old.path === segment.path && old.content === segment.content && old.context !== segment.context))) reasons.push('Moved to another function'); + for (const dep of item.depends_on) if (states[dep] === 'stale') reasons.push(`Depends on ${dep}, which changed`); + if (!reasons.length) reasons.push('Code or plan definition changed'); + } + return { ...item, state: states[item.id], count: owned.length, reasons, before, + checks: { attributed: ambiguous ? `! ${ambiguous} ambiguous` : owned.length ? '✓ Attributed' : '– No changes', scope: outside.length ? `✕ ${new Set(outside).size} out of scope` : owned.length ? '✓ In scope' : '– No changes', tests: item.acceptance.some(check => check.type === 'cmd') ? '– Not run' : '– No tests defined', ai: '– Not run' }, outside: [...new Set(outside)], + }; + }); + const token = createHash('sha256').update(JSON.stringify({ expected, saved, plan, segments })).digest('hex'); + return { repository: basename(repository), demo: this.config.demo ?? false, plan, snapshot, expected, token, items, segments, notes, approved: items.filter(item => item.state === 'approved').length }; + } + act(input: unknown) { + if (!input || typeof input !== 'object') throw new Error('Invalid review command.'); + const command = input as Record; + const view = this.load(); + if (command.token !== view.token) throw new Error('Stale review state. Reload before writing.'); + const { identity } = this.config; + if (command.action === 'approve' && typeof command.item === 'string') { + const approval = approveItem(view.plan, view.segments, command.item, identity, command.confirmNoChange === true); + this.store.saveReview(identity, view.expected, [approval], []); + } else if ((command.action === 'assign' || command.action === 'accept') && typeof command.key === 'string') { + const segment = view.segments.find(segment => segment.key === command.key); + if (!segment || !['Ambiguous', 'Unplanned'].includes(segment.row)) throw new Error('This change cannot be assigned or accepted.'); + const item = command.action === 'assign' && typeof command.item === 'string' ? command.item : null; + this.store.saveReview(identity, view.expected, [], [{ key: segment.key, action: command.action, item }]); + } else if (command.action === 'note' && typeof command.item === 'string' && typeof command.text === 'string' && (command.kind === 'question' || command.kind === 'change')) { + this.store.addReviewNote(identity, view.expected, command.item, command.kind, command.text); + } else throw new Error('Unknown review command.'); + return this.load(); + } +} diff --git a/runner/store.ts b/runner/store.ts index f07b609..cc96c5e 100644 --- a/runner/store.ts +++ b/runner/store.ts @@ -11,7 +11,8 @@ export function requireSupportedNode(version = process.versions.node): void { throw new Error('codeboost requires Node 26.7.0 or later. Upgrade Node before opening the store.'); } export interface Snapshot { id: string; base: string; head: string } -export interface ReviewState { revision: number; snapshotId: string } +export interface ReviewState { revision: number; snapshotId: string; reviewVersion?: number } +export interface ReviewNote { id: string; item: string; kind: 'question' | 'change'; text: string; createdAt: string; revision: number; snapshotId: string } export interface LedgerEntry { sha: string; owner: string | null; origin: 'owned' | 'foreign'; sourceSha: string | null } export interface Checkpoint { id: string; revision: number; snapshotId: string; item: string; @@ -37,9 +38,9 @@ export class Store { this.#db.exec('PRAGMA foreign_keys=ON; PRAGMA journal_mode=WAL; PRAGMA synchronous=FULL;'); this.#transaction(() => { const version = this.#get('PRAGMA user_version')!.user_version; - if (version !== 0 && version !== 1) throw new Error('Unsupported store schema version.'); - if (version === 1) return; - this.#db.exec(` + if (version !== 0 && version !== 1 && version !== 2) throw new Error('Unsupported store schema version.'); + if (version === 2) return; + if (version === 0) this.#db.exec(` CREATE TABLE plans (key TEXT PRIMARY KEY, issue INTEGER NOT NULL, revision INTEGER NOT NULL, snapshot_id TEXT); CREATE TABLE revisions (key TEXT NOT NULL REFERENCES plans(key), revision INTEGER NOT NULL, data TEXT NOT NULL, PRIMARY KEY(key,revision)); CREATE TABLE snapshots (key TEXT NOT NULL REFERENCES plans(key), id TEXT NOT NULL, data TEXT NOT NULL, PRIMARY KEY(key,id)); @@ -52,6 +53,9 @@ export class Store { CREATE TABLE continuations (key TEXT NOT NULL REFERENCES plans(key), checkpoint_id TEXT NOT NULL, revision INTEGER NOT NULL, PRIMARY KEY(key,checkpoint_id,revision)); PRAGMA user_version=1; `); + this.#db.exec(`ALTER TABLE plans ADD COLUMN review_version INTEGER NOT NULL DEFAULT 0; + CREATE TABLE review_notes (key TEXT NOT NULL REFERENCES plans(key), id TEXT NOT NULL, data TEXT NOT NULL, PRIMARY KEY(key,id)); + PRAGMA user_version=2;`); }); } catch (error) { this.#db.close(); throw error; } } @@ -70,7 +74,8 @@ export class Store { } #expect(key: string, expected: ReviewState): void { const row = this.#current(key); - if (row.revision !== expected.revision || row.snapshot_id !== expected.snapshotId) throw new Error('Stale review state. Reload before writing.'); + if (row.revision !== expected.revision || row.snapshot_id !== expected.snapshotId || + (expected.reviewVersion !== undefined && row.review_version !== expected.reviewVersion)) throw new Error('Stale review state. Reload before writing.'); } #context(key: string, context: PlanContext): void { if (identityKey(context.identity) !== key || context.issue !== this.#current(key).issue) throw new Error('Plan context identity/issue mismatch.'); @@ -93,7 +98,7 @@ export class Store { sha(base); sha(head); const key = identityKey(context.identity), plan = importPlan(source, format, context, 1).plan; return this.#transaction(() => { - this.#run('INSERT INTO plans VALUES (?,?,?,NULL)', key, plan.issue, 0); + this.#run('INSERT INTO plans (key,issue,revision,snapshot_id) VALUES (?,?,?,NULL)', key, plan.issue, 0); this.#savePlan(key, plan, 0); this.#snapshot(key, base, head); return plan; @@ -237,8 +242,24 @@ export class Store { if (!choice.key || !['assign','accept'].includes(choice.action) || (choice.action === 'assign' ? !plan.items.some(item => item.id === choice.item) : choice.item !== null)) throw new Error('Invalid segment choice.'); this.#run('INSERT OR REPLACE INTO choices VALUES (?,?,?)', key, choice.key, encode({ ...choice, ...expected })); } + this.#run('UPDATE plans SET review_version=review_version+1 WHERE key=?', key); }); } + reviewVersion(identity: PlanIdentity): number { return this.#current(identityKey(identity)).review_version as number; } + addReviewNote(identity: PlanIdentity, expected: ReviewState, item: string, kind: ReviewNote['kind'], text: string): ReviewNote { + const key = identityKey(identity); + return this.#transaction(() => { + this.#expect(key, expected); + if (!this.getPlan(identity).items.some(entry => entry.id === item) || !['question', 'change'].includes(kind) || typeof text !== 'string' || !text.trim() || text.length > 4000) throw new Error('Invalid review note.'); + const note = { id: randomUUID(), item, kind, text: text.trim(), createdAt: new Date().toISOString(), revision: expected.revision, snapshotId: expected.snapshotId }; + this.#run('INSERT INTO review_notes VALUES (?,?,?)', key, note.id, encode(note)); + this.#run('UPDATE plans SET review_version=review_version+1 WHERE key=?', key); + return note; + }); + } + getReviewNotes(identity: PlanIdentity): ReviewNote[] { + return this.#db.prepare('SELECT data FROM review_notes WHERE key=? ORDER BY rowid').all(identityKey(identity)).map(row => decode(row.data)); + } getReview(identity: PlanIdentity): { approvals: (Approval & ReviewState)[]; choices: (SegmentChoice & ReviewState)[] } { const key = identityKey(identity); this.#current(key); return { diff --git a/scripts/demo.ts b/scripts/demo.ts new file mode 100644 index 0000000..59b2560 --- /dev/null +++ b/scripts/demo.ts @@ -0,0 +1,40 @@ +import { existsSync, mkdirSync, mkdtempSync, rmSync, readFileSync, writeFileSync, chmodSync } from 'node:fs'; +import { resolve, join } from 'node:path'; +import { execFileSync } from 'node:child_process'; +import { randomUUID } from 'node:crypto'; +import { Store } from '../runner/store.ts'; +import type { ReviewConfig } from '../runner/review.ts'; +import type { Plan } from '../core/plan.ts'; +/** Disposable fixture only. Never runs against the user's working repository. */ +export function createDemo(directory: string): ReviewConfig { + const root = resolve(directory), configPath = join(root, 'review.json'); + if (existsSync(configPath)) return JSON.parse(readFileSync(configPath, 'utf8')) as ReviewConfig; + if (existsSync(root)) throw new Error('Demo directory exists without a configuration. Choose a new empty path.'); + mkdirSync(root, { recursive: true }); const repository = join(root, 'retry-service'); mkdirSync(repository); + const git = (...args: string[]) => execFileSync('git', ['-c','core.hooksPath=/dev/null',...args], { cwd: repository, encoding: 'utf8', stdio: ['ignore','pipe','pipe'] }).trim(); + git('init','-b','main'); git('config','user.name','Codeboost Demo'); git('config','user.email','demo@example.invalid'); git('config','commit.gpgsign','false'); + const write = (path: string, text: string | Buffer) => writeFileSync(join(repository,path),text); + const commit = (message: string) => { git('add','-A');git('commit','-m',message);return git('rev-parse','HEAD'); }; + write('retry.ts', 'export function delay(attempt: number) {\n return 100 * attempt;\n}\n'); + write('README.md','# Retry service\n\nRetries use a fixed delay.\n');write('run.sh','#!/bin/sh\nprintf "ready\\n"\n'); + const base=commit('Initial service'); + write('retry.ts','export function delay(attempt: number) {\n return Math.min(5000, 100 * 2 ** attempt);\n}\n');const first=commit('Bound exponential retries\n\nPlan-Item: P1'); + write('README.md','# Retry service\n\nRetries use bounded exponential backoff.\n');chmodSync(join(repository,'run.sh'),0o755);const second=commit('Document retry behavior\n\nPlan-Item: P2'); + write('debug.log','temporary debug output\n');const head=commit('Unrelated diagnostic output'); + const identity={repositoryId:randomUUID(),taskId:randomUUID(),planId:randomUUID()}; + const plan: Plan={schema_version:1,revision:1,issue:3,summary:'Make retries predictable and document the behavior',questions:[],items:[ + {id:'P1',title:'Bound exponential retries',intent:'Cap the backoff at five seconds so callers have a predictable upper bound.',files:[{path:'retry.ts',kind:'edit',renamed_from:null,change:'Use capped exponential backoff.'}],acceptance:[{type:'check',text:'The delay grows exponentially and never exceeds five seconds.'}],depends_on:[]}, + {id:'P2',title:'Document retry behavior',intent:'Describe the retry strategy for maintainers.',files:[{path:'README.md',kind:'edit',renamed_from:null,change:'Explain the capped exponential delay.'}],acceptance:[{type:'check',text:'Documentation agrees with the implementation.'}],depends_on:['P1']}, + {id:'P3',title:'Confirm API compatibility',intent:'Confirm the function signature is unchanged.',files:[{path:'retry.ts',kind:'edit',renamed_from:null,change:'Keep the public function signature.'}],acceptance:[{type:'check',text:'No API change is needed.'}],depends_on:['P1']}, + ]}; + const probe=mkdtempSync(join(root,'identity-')); + let pathIdentity:ReviewConfig['pathIdentity']; + try { writeFileSync(join(probe,'case'),'');writeFileSync(join(probe,'café'),'');pathIdentity={caseSensitive:!existsSync(join(probe,'CASE')),unicodeNormalization:existsSync(join(probe,'cafe\u0301'))?'NFC':'none'}; } + finally {rmSync(probe,{recursive:true});} + const config:ReviewConfig={database:join(root,'review.sqlite'),repository,identity,pathIdentity,demo:true}; + const store=new Store(config.database); + try { store.createPlan(JSON.stringify(plan),'json',{identity,issue:3,baseEntries:['retry.ts','README.md','run.sh'].map(path=>({path,kind:'file' as const})),pathKey:p=>p,allowedCommands:[]},base,head); + store.recordHistory(identity,{revision:1,snapshotId:store.getSnapshot(identity).id},base,head,[{sha:first,owner:'P1',origin:'owned',sourceSha:null},{sha:second,owner:'P2',origin:'owned',sourceSha:null}]); + } finally {store.close();} + writeFileSync(configPath,JSON.stringify(config,null,2)+'\n',{mode:0o600});return config; +} diff --git a/scripts/plant.ts b/scripts/plant.ts new file mode 100644 index 0000000..cada7ec --- /dev/null +++ b/scripts/plant.ts @@ -0,0 +1,80 @@ +import { execFileSync } from 'node:child_process'; +import { existsSync, mkdirSync, writeFileSync, appendFileSync, lstatSync, readFileSync } from 'node:fs'; +import { resolve, join, dirname } from 'node:path'; +import { randomInt, randomUUID } from 'node:crypto'; +import { pathToFileURL } from 'node:url'; +import { Store } from '../runner/store.ts'; +import type { ReviewConfig } from '../runner/review.ts'; +import { readHistory } from '../git/history.ts'; +import { isRepoPath, type BaseEntry } from '../core/plan.ts'; +export interface PlantInput { declaredText: string; undeclaredText: string; undeclaredPath: string } +/** Prepare a separate local experiment clone. Never rewrites/pushes the source repository. + * The experiment operator runs this before the reviewer sees the PR; sealed.json is unblinding data. + */ +export function plant(config: ReviewConfig, destination: string, input: PlantInput): string { + const root=resolve(destination); + if(existsSync(root))throw new Error('Experiment destination must not exist.'); + if((!isRepoPath(input.undeclaredPath)||input.undeclaredPath.includes('/'))||!input.declaredText?.trim()||!input.undeclaredText?.trim()||input.declaredText.length>4000||input.undeclaredText.length>4000)throw new Error('Invalid plant input.'); + const source=new Store(config.database); + let plan, snapshot, entries; + try{plan=source.getPlan(config.identity);snapshot=source.getSnapshot(config.identity);entries=source.getLedger(config.identity);}finally{source.close();} + if(plan.items.some(item=>item.files.some(file=>file.path===input.undeclaredPath||file.renamed_from===input.undeclaredPath)))throw new Error('Undeclared plant must be outside every declared file.'); + const history=readHistory(config.repository,snapshot.base,snapshot.head); + const owners=new Map(entries.map(entry=>[entry.sha,entry.owner])); + const candidates=history.commits.flatMap((commit,index)=>{ + const item=plan.items.find(item=>item.id===owners.get(commit.sha)); + return item?.files.filter(file=>file.kind==='edit'&&!file.path.includes('/')).map(file=>({index,item:item.id,path:file.path}))??[]; + }); + if(!candidates.length)throw new Error('No owned commit with an editable top-level file is available for planting.'); + const declared=candidates[randomInt(candidates.length)]!; + const owned=history.commits.map((commit,index)=>({index,owner:owners.get(commit.sha)})).filter(entry=>entry.owner&&plan!.items.some(item=>item.id===entry.owner)); + const outside=owned[randomInt(owned.length)]!; + const env={...Object.fromEntries(Object.entries(process.env).filter(([key])=>!key.startsWith('GIT_'))),GIT_CONFIG_NOSYSTEM:'1',GIT_CONFIG_GLOBAL:'/dev/null'}; + const gitRaw=(cwd:string,...args:string[])=>execFileSync('git',['-c','core.hooksPath=/dev/null','-c','commit.gpgsign=false',...args],{cwd,env,encoding:'utf8',stdio:['ignore','pipe','pipe'],timeout:30000,maxBuffer:32*1024*1024}); + const git=(cwd:string,...args:string[])=>gitRaw(cwd,...args).trim(); + // Refuse existing paths, symlink targets, and unsupported declared-file transitions before creating output. + for(const commit of history.commits){ + const tree=git(config.repository,'ls-tree','-r',commit.sha,'--',input.undeclaredPath); + if(tree)throw new Error('Undeclared plant path already exists in the source history.'); + } + for(const commit of history.commits.slice(declared.index)){ + if(!/^100(?:644|755) blob /.test(git(config.repository,'ls-tree',commit.sha,'--',declared.path)))throw new Error('Declared plant needs a regular file retained through the remaining history.'); + } + mkdirSync(root,{recursive:true});const repository=join(root,'repository'); + git(root,'clone','--no-local','--no-checkout',resolve(config.repository),repository); + git(repository,'config','user.name','Codeboost Experiment');git(repository,'config','user.email','experiment@example.invalid'); + git(repository,'checkout','-b','review-experiment',snapshot.base); + const baseEntries:BaseEntry[]=git(repository,'ls-tree','-rz',snapshot.base).split('\0').filter(Boolean).map(record=>{ + const split=record.indexOf('\t'),[mode,,oid]=record.slice(0,split).split(' '),path=record.slice(split+1); + if(mode==='160000')return {path,kind:'gitlink'}; + if(mode==='120000')return {path,kind:'symlink',target:gitRaw(repository,'cat-file','blob',oid!)}; + return {path,kind:'file'}; + }); + const mappings=[]; + try{ + for(const [index,commit] of history.commits.entries()){ + git(repository,'cherry-pick','--no-commit',commit.sha); + if(index===declared.index){const path=join(repository,declared.path);if(!lstatSync(path).isFile())throw new Error('Declared plant target is not regular.');appendFileSync(path,'\n'+input.declaredText+'\n');} + if(index===outside.index){const path=join(repository,input.undeclaredPath);mkdirSync(dirname(path),{recursive:true});writeFileSync(path,input.undeclaredText+'\n',{flag:'wx'});} + git(repository,'add','-A');git(repository,'commit','--allow-empty','-C',commit.sha); + mappings.push({oldSha:commit.sha,newSha:git(repository,'rev-parse','HEAD')}); + } + const identity={repositoryId:config.identity.repositoryId,taskId:randomUUID(),planId:randomUUID()}; + const output:ReviewConfig={...config,repository,database:join(root,'review.sqlite'),identity,demo:false}; + const store=new Store(output.database); + try{ + const pathKey=(path:string)=>{const p=config.pathIdentity.unicodeNormalization==='NFC'?path.normalize('NFC'):path;return config.pathIdentity.caseSensitive?p:p.toLowerCase();}; + store.createPlan(JSON.stringify(plan),'json',{identity,issue:plan.issue,baseEntries,pathKey,allowedCommands:[]},snapshot.base,snapshot.head); + const validEntries=entries.filter(entry=>entry.owner===null||plan.items.some(item=>item.id===entry.owner)); + store.recordHistory(identity,{revision:1,snapshotId:store.getSnapshot(identity).id},snapshot.base,snapshot.head,validEntries); + store.recordRebase(identity,{revision:1,snapshotId:store.getSnapshot(identity).id},snapshot.base,mappings.at(-1)!.newSha,mappings); + }finally{store.close();} + writeFileSync(join(root,'sealed.json'),JSON.stringify({declared,outside,undeclaredPath:input.undeclaredPath,mappings},null,2),{mode:0o600}); + const configPath=join(root,'review.json');writeFileSync(configPath,JSON.stringify(output,null,2),{mode:0o600});return configPath; + }catch(error){throw new Error(`Planting stopped. Source is unchanged; inspect the disposable clone at ${repository}. ${error instanceof Error?error.message:String(error)}`);} +} +if(process.argv[1]&&import.meta.url===pathToFileURL(resolve(process.argv[1])).href){ + const [configFile,destination,inputFile]=process.argv.slice(2); + if(!configFile||!destination||!inputFile)throw new Error('Usage: node scripts/plant.ts review.json NEW_DIRECTORY plant-input.json'); + console.log(plant(JSON.parse(readFileSync(configFile,'utf8')),destination,JSON.parse(readFileSync(inputFile,'utf8')))); +} diff --git a/test/browser/review.spec.ts b/test/browser/review.spec.ts new file mode 100644 index 0000000..a7ea3bb --- /dev/null +++ b/test/browser/review.spec.ts @@ -0,0 +1,47 @@ +import { test, expect } from '@playwright/test'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { execFileSync } from 'node:child_process'; +import { createDemo } from '../../scripts/demo.ts'; +import { startServer } from '../../web/server.ts'; +let root: string, app: Awaited>; +test.beforeEach(async () => { root=mkdtempSync(join(tmpdir(),'codeboost-browser-'));app=await startServer(createDemo(join(root,'demo')),0); }); +test.afterEach(async () => { await app.close();rmSync(root,{recursive:true,force:true}); }); +test('reviews real changes, persists approval and conversation, and assigns foreign code',async({page})=>{ + const errors:string[]=[];page.on('pageerror',e=>errors.push(e.message)); + await page.goto(app.url);await expect(page.getByRole('heading',{name:'Bound exponential retries'})).toBeVisible(); + await expect(page.getByText('0 of 3 approved')).toBeVisible();await expect(page.getByText('– No tests defined',{exact:false}).first()).toBeVisible(); + await page.getByRole('button',{name:'Approve P1',exact:true}).click();await expect(page.getByText('1 of 3 approved')).toBeVisible(); + await page.getByRole('button',{name:'Request change',exact:true}).click();await page.getByLabel('Change to request').fill('Add a test for the upper bound.');await page.getByRole('button',{name:'Save change request'}).click();await expect(page.getByText('Add a test for the upper bound.',{exact:true})).toBeVisible(); + await page.reload();await expect(page.getByText('1 of 3 approved')).toBeVisible();await expect(page.getByText('Add a test for the upper bound.',{exact:true})).toBeVisible(); + await page.getByRole('button',{name:/Unplanned changes/}).click();await page.getByLabel('Assign change 1 to').selectOption('P1');await page.getByRole('button',{name:'Assign',exact:true}).first().click(); + await page.getByRole('button',{name:/P1 Bound exponential retries/}).click();await expect(page.getByText('! Stale:',{exact:false})).toBeVisible();await expect(page.getByRole('heading',{name:'At approval'})).toBeVisible(); + await page.screenshot({path:'test-results/review-desktop.png',fullPage:true});expect(errors).toEqual([]); +}); +test('shows file metadata and no-change confirmation, supports narrow desktop and keyboard',async({page})=>{ + await page.goto(app.url);await page.getByRole('button',{name:/P2 Document retry behavior/}).click();await expect(page.getByText('File mode changed',{exact:false})).toBeVisible();await expect(page.getByText('✕ Out of scope',{exact:true})).toBeVisible(); + await page.getByRole('button',{name:/P3 Confirm API compatibility/}).click();await page.getByRole('button',{name:'Confirm no change needed',exact:true}).click();await expect(page.getByText('1 of 3 approved')).toBeVisible(); + await page.setViewportSize({width:1280,height:900});await expect(page.getByRole('complementary',{name:'Conversation',exact:true})).not.toBeVisible();await page.getByRole('button',{name:'Conversation',exact:true}).click();await expect(page.getByRole('complementary',{name:'Conversation',exact:true})).toBeVisible(); + await page.getByLabel('Question about this item').fill('npa');await expect(page.getByRole('heading',{name:'Confirm API compatibility'})).toBeVisible(); + await page.keyboard.press('Escape');await page.keyboard.press('p');await expect(page.getByRole('heading',{name:'Document retry behavior'})).toBeVisible(); + await page.setViewportSize({width:1000,height:800});await expect(page.getByRole('heading',{name:'A little more room to review'})).toBeVisible(); +}); +test('refresh makes changed code stale and stale browser actions cannot approve it',async({page,context})=>{ + await page.goto(app.url);await page.getByRole('button',{name:'Approve P1',exact:true}).click();await expect(page.getByText('1 of 3 approved')).toBeVisible(); + const other=await context.newPage();await other.goto(app.url);await expect(other.getByText('1 of 3 approved')).toBeVisible(); + const repository=app.service.config.repository;writeFileSync(join(repository,'retry.ts'),'export function delay(attempt: number) {\n return 42;\n}\n');execFileSync('git',['-c','core.hooksPath=/dev/null','commit','-am','External change'],{cwd:repository,stdio:'pipe'}); + await page.getByRole('button',{name:'Refresh',exact:true}).click();await expect(page.getByText('! Stale:',{exact:false}).first()).toBeVisible(); + await other.getByRole('button',{name:/P3 Confirm API compatibility/}).click();await other.getByRole('button',{name:'Confirm no change needed',exact:true}).click();await expect(other.getByRole('status').filter({hasText:'Stale review state'})).toBeVisible(); +}); +test('requires private credentials and rejects foreign origins',async({request})=>{ + const base=app.url.split('#')[0]!; + expect((await request.get(base+'api/review')).status()).toBe(403); + expect((await request.get(base+'api/review',{headers:{'x-codeboost-token':app.token,origin:'https://example.invalid'}})).status()).toBe(403); + expect((await request.get(base+'api/review',{headers:{'x-codeboost-token':app.token}})).status()).toBe(200); +}); +test('shows an honest history error and keeps markup in notes as text',async({page})=>{ + await page.goto(app.url);await page.getByLabel('Question about this item').fill('');await page.getByRole('button',{name:'Save question'}).click();await expect(page.getByText('',{exact:true})).toBeVisible();await expect(page.locator('#notes img')).toHaveCount(0); + const repository=app.service.config.repository;execFileSync('git',['checkout','--orphan','unrelated'],{cwd:repository,stdio:'pipe'});execFileSync('git',['-c','core.hooksPath=/dev/null','commit','-m','Unrelated history'],{cwd:repository,stdio:'pipe'}); + await page.getByRole('button',{name:'Refresh',exact:true}).click();await expect(page.getByText('Could not read this branch’s history.',{exact:false})).toBeVisible();await expect(page.getByRole('button',{name:'Approve P1',exact:true})).not.toBeVisible(); +}); diff --git a/test/plant.test.ts b/test/plant.test.ts new file mode 100644 index 0000000..afe0aba --- /dev/null +++ b/test/plant.test.ts @@ -0,0 +1,17 @@ +import { it,expect,afterEach } from 'vitest'; +import { mkdtempSync,readFileSync,rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { execFileSync } from 'node:child_process'; +import { createDemo } from '../scripts/demo.ts'; +import { plant } from '../scripts/plant.ts'; +import { ReviewService } from '../runner/review.ts'; +const roots:string[]=[];afterEach(()=>roots.splice(0).forEach(root=>rmSync(root,{recursive:true,force:true}))); +it('plants in an isolated clone, retains ledger attribution, and leaves source history unchanged',()=>{ + const root=mkdtempSync(join(tmpdir(),'codeboost-plant-'));roots.push(root);const config=createDemo(join(root,'source')); + const head=()=>execFileSync('git',['rev-parse','HEAD'],{cwd:config.repository,encoding:'utf8'}).trim();const before=head(); + const path=plant(config,join(root,'experiment'),{declaredText:'// planted extra behavior',undeclaredText:'unrelated diagnostic',undeclaredPath:'extra.txt'}); + expect(head()).toBe(before);const output=JSON.parse(readFileSync(path,'utf8'));const service=new ReviewService(output); + try {const view=service.load();expect(view.segments.some(s=>s.path==='extra.txt'&&s.scope==='out-of-scope')).toBe(true);expect(view.segments.some(s=>s.content.includes('// planted extra behavior')&&s.row.startsWith('P'))).toBe(true);}finally{service.close();} + expect(JSON.parse(readFileSync(join(root,'experiment','sealed.json'),'utf8')).mappings).toHaveLength(3); +},15000); diff --git a/test/review.test.ts b/test/review.test.ts new file mode 100644 index 0000000..9c6097e --- /dev/null +++ b/test/review.test.ts @@ -0,0 +1,31 @@ +import { afterEach, it, expect } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { DatabaseSync } from 'node:sqlite'; +import { createDemo } from '../scripts/demo.ts'; +import { ReviewService } from '../runner/review.ts'; +const roots:string[]=[];const services:ReviewService[]=[]; +afterEach(()=>{services.splice(0).forEach(service=>service.close());roots.splice(0).forEach(root=>rmSync(root,{recursive:true,force:true}));}); +function fixture(){const root=mkdtempSync(join(tmpdir(),'codeboost-review-'));roots.push(root);const config=createDemo(join(root,'demo'));const service=new ReviewService(config);services.push(service);return {service,config};} +it('expires a browser token after another view assigns a segment and recomputes scope honestly',()=>{ + const {service}=fixture();const view=service.load();const foreign=view.segments.find(s=>s.row==='Unplanned')!; + const next=service.act({action:'assign',key:foreign.key,item:'P1',token:view.token}); + expect(next.items[0]!.checks.scope).toContain('out of scope'); + expect(()=>service.act({action:'approve',item:'P1',token:view.token})).toThrow(/Stale/); +}); +it('persists bounded per-item notes without creating a plan revision',()=>{ + const {service,config}=fixture();const view=service.load();service.act({action:'note',item:'P1',kind:'question',text:'Why this limit?',token:view.token}); + const reopened=new ReviewService(config);services.push(reopened);expect(reopened.load().notes[0]!.text).toBe('Why this limit?');expect(reopened.load().plan.revision).toBe(1); + expect(()=>service.act({action:'note',item:'P2',kind:'change',text:'x'.repeat(4001),token:service.load().token})).toThrow(/Invalid/); +}); +it('migrates a v1 database without losing its plans or ledger',()=>{ + const {service,config}=fixture();service.close();services.splice(services.indexOf(service),1); + const db=new DatabaseSync(config.database);db.exec('ALTER TABLE plans DROP COLUMN review_version; DROP TABLE review_notes; PRAGMA user_version=1;');db.close(); + const migrated=new ReviewService(config);services.push(migrated);expect(migrated.load().plan.revision).toBe(1);expect(migrated.store.getLedger(config.identity)).toHaveLength(2);expect(migrated.load().notes).toEqual([]); +}); +it('rejects a concurrent store review edit through the atomic review counter',()=>{ + const {service,config}=fixture();const other=new ReviewService(config);services.push(other);const view=service.load(); + other.store.addReviewNote(config.identity,view.expected,'P1','change','Please explain'); + expect(()=>service.store.saveReview(config.identity,view.expected,[],[])).toThrow(/Stale/); +}); diff --git a/tsconfig.json b/tsconfig.json index 0f69925..420c965 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -1,9 +1,26 @@ { "compilerOptions": { - "target": "ES2024", "module": "NodeNext", "moduleResolution": "NodeNext", - "strict": true, "noUncheckedIndexedAccess": true, "resolveJsonModule": true, - "esModuleInterop": true, "skipLibCheck": true, "noEmit": true, - "allowImportingTsExtensions": true, "types": ["node"] + "target": "ES2024", + "module": "NodeNext", + "moduleResolution": "NodeNext", + "strict": true, + "noUncheckedIndexedAccess": true, + "resolveJsonModule": true, + "esModuleInterop": true, + "skipLibCheck": true, + "noEmit": true, + "allowImportingTsExtensions": true, + "types": [ + "node" + ] }, - "include": ["core/**/*.ts", "git/**/*.ts", "runner/**/*.ts", "test/**/*.ts"] + "include": [ + "core/**/*.ts", + "git/**/*.ts", + "runner/**/*.ts", + "test/**/*.ts", + "web/**/*.ts", + "scripts/**/*.ts", + "playwright.config.ts" + ] } diff --git a/vitest.config.ts b/vitest.config.ts new file mode 100644 index 0000000..c7de951 --- /dev/null +++ b/vitest.config.ts @@ -0,0 +1,2 @@ +import { defineConfig } from 'vitest/config'; +export default defineConfig({ test: { include: ['test/*.test.ts'] } }); diff --git a/web/cli.ts b/web/cli.ts new file mode 100644 index 0000000..50b39d3 --- /dev/null +++ b/web/cli.ts @@ -0,0 +1,19 @@ +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import { parseArgs } from 'node:util'; +import { startServer } from './server.ts'; +import { createDemo } from '../scripts/demo.ts'; +import { requireSupportedNode } from '../runner/store.ts'; +requireSupportedNode(); +const { values } = parseArgs({ options: { demo: { type:'boolean' }, directory:{type:'string'}, config:{type:'string'}, port:{type:'string'}, help:{type:'boolean'} } }); +if (values.help || (!values.demo && !values.config)) { + console.log('codeboost local review\n\nDemo: npm run demo\nExisting store: npm start -- --config /absolute/path/review.json\nOptions: --port 4318 --directory /path/to/demo\n\nThe configuration binds a trusted repository, database, plan identity, and known path identity. No agent, tests, or merge commands run.'); +} else { + const port = Number(values.port ?? '4318'); + if (!Number.isInteger(port) || port < 0 || port > 65535) throw new Error('Invalid port.'); + const config = values.demo ? createDemo(values.directory ?? '.codeboost-local/demo') : JSON.parse(readFileSync(resolve(values.config!), 'utf8')); + const app = await startServer(config, port); + console.log(`Review ready: ${app.url}\nRepository: ${config.repository}\nDatabase: ${config.database}\nSource files are read-only. Press Ctrl+C to stop.`); + let stopping=false; + for(const signal of ['SIGINT','SIGTERM'] as const) process.on(signal,()=>{if(!stopping){stopping=true;void app.close().then(()=>process.exit(0));}}); +} diff --git a/web/public/app.js b/web/public/app.js new file mode 100644 index 0000000..20f5ac8 --- /dev/null +++ b/web/public/app.js @@ -0,0 +1,98 @@ +const $ = id => document.getElementById(id); +const esc = value => String(value ?? '').replace(/[&<>"']/g, c => ({ '&': '&', '<': '<', '>': '>', '"': '"', "'": ''' })[c]); +const credential = location.hash.slice(1) || sessionStorage.getItem('codeboost-token') || ''; +if (location.hash) { sessionStorage.setItem('codeboost-token', credential); history.replaceState(null, '', location.pathname); } +let data, selected, change = 0, mode = 'question', since = false, busy = false; +const drafts = new Map(); +const statusClass = text => text.startsWith('✓') ? 'good' : text.startsWith('✕') ? 'bad' : text.startsWith('!') ? 'warn' : 'neutral'; +async function api(path, body) { + const response = await fetch(path, { method: body ? 'POST' : 'GET', headers: { 'x-codeboost-token': credential, ...(body ? { 'Content-Type': 'application/json' } : {}) }, body: body ? JSON.stringify(body) : undefined }); + const value = await response.json(); if (!response.ok) throw new Error(value.error || 'Request failed.'); return value; +} +function rememberDraft() { if (selected) drafts.set(`${selected}:${mode}`, $('message').value); } +async function refresh() { + if (busy) return; busy = true; + $('banner').textContent = 'Linking changes to plan items…'; + try { rememberDraft(); data = await api('/api/review'); selected ??= data.items[0]?.id || 'Unplanned'; since = data.items.find(item => item.id === selected)?.state === 'stale'; render(); } + catch (error) { $('banner').textContent = `Could not read this branch’s history. ${error.message} Use Refresh to retry.`; $('code').innerHTML = '

Review could not load

Retry after resolving the error above. Open task shows the last loaded task, if available.

'; $('approve').hidden = true; } + finally { busy = false; } +} +async function act(command) { + if (busy || !data) return false; busy = true; + try { rememberDraft(); data = await api('/api/action', { ...command, token: data.token }); render(); return true; } + catch (error) { $('banner').textContent = `${error.message} Refresh to review the latest state.`; return false; } + finally { busy = false; } +} +function select(id) { rememberDraft(); selected = id; change = 0; since = data.items.find(item => item.id === id)?.state === 'stale'; render(); } +function render() { + const item = data.items.find(item => item.id === selected); + if (!item && !['Unplanned', 'Ambiguous', 'Accepted'].includes(selected)) { selected = data.items[0]?.id || 'Unplanned'; return render(); } + $('repository').textContent = data.repository; + $('issue').textContent = `#${data.plan.issue} ${data.plan.summary} · r${data.plan.revision}`; + $('progress').textContent = `${data.approved} of ${data.items.length} approved`; + $('banner').textContent = data.demo ? 'Demo repository · real Git changes and local SQLite storage. No tests or AI review have been run for this demo.' : ''; + const unplanned = data.segments.filter(s => s.row === 'Unplanned').length, ambiguous = data.segments.filter(s => s.row === 'Ambiguous').length; + $('attention').innerHTML = ` · `; + const symbols = { approved: '✓', stale: '!', unreviewed: '○' }; + $('items').innerHTML = data.items.map(entry => `${entry.id === selected ? `
Declared files${entry.files.map(file => `${esc(file.path)}`).join('')}
` : ''}`).join('') + ['Ambiguous','Unplanned','Accepted'].map(row => ``).join(''); + document.querySelectorAll('[data-select]').forEach(button => button.addEventListener('click', () => select(button.dataset.select))); + $('item-id').textContent = item?.id || 'REVIEW EXCEPTIONS'; $('item-title').textContent = item?.title || selected; + $('item-details').innerHTML = item ? `

${esc(item.intent)}

${item.reasons.map(reason => `

! Stale: ${esc(reason)}

`).join('')}` : ''; + $('approve').hidden = !item; $('approve').textContent = item?.count ? `Approve ${item.id}` : 'Confirm no change needed'; $('approve').disabled = item?.state === 'approved'; + $('view-toggle').innerHTML = item?.state === 'stale' ? `` : ''; + document.querySelectorAll('[data-since]').forEach(button => button.addEventListener('click', () => { since = button.dataset.since === 'true'; renderCode(); })); + $('checks').innerHTML = item ? Object.entries(item.checks).map(([name,text]) => ``).join('') : ''; + document.querySelectorAll('[data-check]').forEach(button => button.addEventListener('click', () => { + if (button.dataset.check === 'attributed' && item.checks.attributed.startsWith('!')) select('Ambiguous'); + else showDialog(`

${esc(button.textContent)}

${esc(button.dataset.check === 'scope' ? item.outside.join('\n') || 'Every attributed change is inside this item’s declared files.' : button.dataset.check === 'tests' ? item.acceptance.map(check => `${check.type}: ${check.text}`).join('\n') + '\n\nThis screen does not run commands.' : button.dataset.check === 'ai' ? 'No AI review has run. This does not mean the change has no problems.' : 'Attribution comes from the commit ledger, not commit messages.')}
`); + })); + $('notes').innerHTML = item ? data.notes.filter(note => note.item === selected).map(note => `
You · ${note.kind === 'change' ? 'Change requested' : 'Question'}

${esc(note.text)}

r${note.revision} · ${esc(new Date(note.createdAt).toLocaleString())}${note.kind === 'change' ? ' · Pending' : ' · Awaiting discussion'}
`).join('') || '

No conversation yet. Keep questions and requested changes beside the evidence.

' : '

Select a plan item to add a question or request a change.

'; + $('message').disabled = !item; $('save-note').disabled = !item; $('message').value = drafts.get(`${selected}:${mode}`) || ''; + renderCode(); +} +function renderCode() { + const item = data.items.find(item => item.id === selected), segments = data.segments.filter(s => s.row === selected); + change = Math.max(0, Math.min(change, segments.length - 1)); + $('change-count').textContent = segments.length ? `Change ${change + 1} of ${segments.length}` : 'No changes'; + $('previous').disabled = !segments.length || change === 0; $('next').disabled = !segments.length || change === segments.length - 1; + let comparison = ''; + if (since && item?.state === 'stale' && item.before) { + const prior = item.before.segments.map(s => `${s.path} ${s.operation || ''}\n${s.content}`).join('\n'); + const now = segments.map(s => `${s.path} ${s.operation || ''}\n${s.content}`).join('\n'); + comparison = `

At approval

${esc(prior || 'No changes')}
Approved plan item
${esc(JSON.stringify(item.before.item,null,2))}

Now

${esc(now || 'No changes')}
Current plan item
${esc(JSON.stringify(data.plan.items.find(p=>p.id===item.id),null,2))}
`; + } + $('code').innerHTML = comparison + (segments.map((segment,index) => { + const provenance = segment.row === 'Accepted' ? 'Accepted' : segment.row; + let content; + if (segment.kind === 'file') { + const meta = JSON.parse(segment.content); + const label = meta.oldPath !== meta.newPath && meta.oldPath && meta.newPath ? 'Renamed file' : meta.oldMode === '160000' || meta.newMode === '160000' ? 'Submodule pointer' : meta.oldMode === '120000' || meta.newMode === '120000' ? 'Symbolic link' : meta.oldMode !== meta.newMode && meta.oldMode && meta.newMode ? 'File mode changed' : 'File change'; + content = `
${esc(provenance)}
▧ ${label}
Path
${esc(meta.oldPath || 'Absent')} → ${esc(meta.newPath || 'Absent')}
Mode
${esc(meta.oldMode || 'Absent')} → ${esc(meta.newMode || 'Absent')}

No preview available · size unavailable

Details · content IDs
${esc(JSON.stringify(meta,null,2))}
`; + } else { + const lines = segment.content.split('\n'); if (lines.at(-1) === '') lines.pop(); + const start = segment.operation === '+' ? segment.newLine : segment.oldLine; + content = `
${esc(provenance)}
${lines.map((_,i)=>start===null?'':start+i).join('\n')}
${esc(segment.operation)}
${esc(segment.content)}
`; + } + const choices = ['Unplanned','Ambiguous'].includes(segment.row) ? `

Assigning this change makes the selected item’s approval stale.

` : ''; + return `
${esc(segment.path)}${segment.scope === 'out-of-scope' ? '✕ Out of scope' : esc(segment.scope)}
${content}
${esc(segment.context || 'File-level change')}${segment.sharesHunkWith.length ? ` · Shares a hunk with ${esc(segment.sharesHunkWith.join(', '))}` : ''}
${choices}
`; + }).join('') || '

No changes in this row

There are no current segments here. An item with no changes requires explicit confirmation before approval.

'); + document.querySelectorAll('[data-assign]').forEach(button => button.addEventListener('click', () => { const index = Number(button.dataset.assign); const item = document.querySelector(`[data-target="${index}"]`).value; if (item) act({ action:'assign', key:segments[index].key, item }); })); + document.querySelectorAll('[data-accept]').forEach(button => button.addEventListener('click', () => act({ action:'accept', key:segments[Number(button.dataset.accept)].key }))); + document.querySelectorAll('[data-since]').forEach(button => button.setAttribute('aria-pressed', String((button.dataset.since === 'true') === since))); +} +function moveChange(delta) { change += delta; renderCode(); document.querySelector(`[data-change="${change}"]`)?.scrollIntoView({ block:'nearest' }); } +function setMode(value) { rememberDraft(); mode = value; $('ask').setAttribute('aria-pressed',String(mode==='question')); $('request').setAttribute('aria-pressed',String(mode==='change')); $('composer-label').textContent = mode==='change'?'Change to request':'Question about this item'; $('save-note').textContent = mode==='change'?'Save change request':'Save question'; $('message').value = drafts.get(`${selected}:${mode}`)||''; } +function showDialog(html) { $('dialog-body').innerHTML = html; $('dialog').showModal(); } +$('approve').onclick = () => { const item=data.items.find(item=>item.id===selected); if(item) act({ action:'approve',item:selected,confirmNoChange:item.count===0 }); }; +$('reload').onclick=refresh; $('review-link').onclick=event=>{event.preventDefault();refresh();}; +$('next').onclick=()=>moveChange(1); $('previous').onclick=()=>moveChange(-1); +$('ask').onclick=()=>setMode('question'); $('request').onclick=()=>setMode('change'); +$('composer').onsubmit=async event=>{event.preventDefault();const item=selected,kind=mode; if(await act({action:'note',item,kind,text:$('message').value})){drafts.delete(`${item}:${kind}`);$('message').value='';$('saved').textContent=kind==='change'?'Saved for the next revision.':'Question saved. No agent has been invoked.';}}; +$('conversation-toggle').onclick=()=>{document.body.classList.toggle('conversation-open');document.body.classList.remove('conversation-closed');}; +$('collapse-conversation').onclick=()=>{document.body.classList.remove('conversation-open');document.body.classList.add('conversation-closed');}; +$('collapse-plan').onclick=()=>{document.querySelector('.plan-pane').hidden=true;$('show-plan').hidden=false;};$('show-plan').onclick=()=>{document.querySelector('.plan-pane').hidden=false;$('show-plan').hidden=true;}; +$('help').onclick=()=>showDialog('

Keyboard shortcuts

j / k — next / previous change

n / p — next / previous item

a — approve item

r — request change

? — shortcuts

Esc — leave a text field or close this dialog

'); +$('close-dialog').onclick=()=>$('dialog').close(); +$('task').onclick=()=>showDialog(data?`

Task #${data.plan.issue}

${esc(data.plan.summary)}

Revision ${data.plan.revision} · ${data.demo?'Demo repository':'Local repository'}

Base: ${esc(data.snapshot.base)}\nHead: ${esc(data.snapshot.head)}

Source files are read-only in this review app. Approvals and discussion are saved locally.

`:'

No task loaded

Check the CLI configuration and retry.

'); +document.addEventListener('keydown',event=>{if(event.key==='Escape'&&['TEXTAREA','INPUT','SELECT'].includes(document.activeElement?.tagName)){document.activeElement.blur();return;}if(event.ctrlKey||event.metaKey||event.altKey||$('dialog').open||['TEXTAREA','INPUT','SELECT','BUTTON'].includes(document.activeElement?.tagName)||!data)return;if(!['j','k','n','p','a','r','?'].includes(event.key))return;event.preventDefault();if(event.key==='j')moveChange(1);if(event.key==='k')moveChange(-1);if(['n','p'].includes(event.key)){const ids=data.items.map(p=>p.id);select(ids[Math.max(0,Math.min(ids.length-1,ids.indexOf(selected)+(event.key==='n'?1:-1)))]);}if(event.key==='a')$('approve').click();if(event.key==='r'){document.body.classList.add('conversation-open');document.body.classList.remove('conversation-closed');setMode('change');$('message').focus();}if(event.key==='?')$('help').click();}); +await refresh(); diff --git a/web/public/index.html b/web/public/index.html new file mode 100644 index 0000000..08c504d --- /dev/null +++ b/web/public/index.html @@ -0,0 +1,11 @@ + +Review · codeboost +

A little more room to review

Widen this window to at least 1280px to see the code and its plan together.

+
codeboostLocal review
+
Review Loading…
+ +
+
REVIEW

Linking changes to plan items…

+
+
j / k changen / p itema approver request changeReview only · source files stay unchanged
+
 diff --git a/web/public/style.css b/web/public/style.css new file mode 100644 index 0000000..1482276 --- /dev/null +++ b/web/public/style.css @@ -0,0 +1,5 @@ +@font-face{font-family:'IBM Plex Sans';src:url('/fonts/ibm-plex-sans-latin-400-normal.woff2') format('woff2');font-weight:400;font-display:swap}@font-face{font-family:'IBM Plex Sans';src:url('/fonts/ibm-plex-sans-latin-500-normal.woff2') format('woff2');font-weight:500;font-display:swap}@font-face{font-family:'IBM Plex Sans';src:url('/fonts/ibm-plex-sans-latin-600-normal.woff2') format('woff2');font-weight:600;font-display:swap}@font-face{font-family:'IBM Plex Mono';src:url('/fonts/ibm-plex-mono-latin-400-normal.woff2') format('woff2');font-weight:400;font-display:swap} +:root{--canvas:#101216;--surface:#171B21;--raised:#20262E;--selected:#213448;--line:#343D49;--outline:#758295;--text:#E8ECF1;--muted:#A5AFBD;--primary:#8ABFFF;--on-primary:#101216;--hover:#A6CEFF;--success:#8FDDA8;--warning:#E9BE6E;--error:#F2847E;--neutral:#9AA3B0;--added:#1B2C24;--removed:#33201F;color-scheme:dark} +*{box-sizing:border-box}body{margin:0;background:var(--canvas);color:var(--text);font:13px/1.385 'IBM Plex Sans','Segoe UI',sans-serif}button,input,textarea,select{font:inherit}button,select{min-height:28px;border:1px solid var(--line);border-radius:4px;background:var(--raised);color:var(--text);padding:4px 10px;cursor:pointer}button:hover,select:hover{background:var(--selected)}button:disabled{color:var(--muted);cursor:default;background:var(--surface)}:focus-visible{outline:2px solid var(--primary);outline-offset:2px}button.primary,#approve{background:var(--primary);color:var(--on-primary);border-color:var(--primary)}#approve:hover{background:var(--hover)}[hidden]{display:none!important}a{color:var(--primary)}h1{font-size:20px;line-height:1.4;margin:4px 0 0;font-weight:600}h2{font-size:16px}p{margin:8px 0}pre,code,.mono,.eyebrow{font-family:'IBM Plex Mono',Menlo,Consolas,monospace;font-size:12px;font-variant-numeric:tabular-nums}pre{white-space:pre-wrap;overflow-wrap:anywhere}.muted,.eyebrow{color:var(--muted)}.good{color:var(--success)}.warn{color:var(--warning)}.bad{color:var(--error)}.neutral{color:var(--neutral)} +#app{height:100vh;display:flex;flex-direction:column}.app-bar{height:44px;flex-shrink:0;border-bottom:1px solid var(--line);display:flex;align-items:center;padding:0 16px;gap:24px}.brand{display:flex;align-items:center;gap:8px;font-weight:600}.app-bar nav{display:flex;gap:20px;align-items:center;flex:1}.app-bar nav span{color:var(--muted)}.app-bar nav a{height:44px;display:flex;align-items:center;text-decoration:none;border-bottom:2px solid var(--primary)}#repository{font-size:12px;color:var(--muted)}.review-strip{height:48px;flex-shrink:0;display:flex;align-items:center;gap:16px;padding:0 16px;border-bottom:1px solid var(--line)}#issue{margin-left:16px}#progress{margin-left:auto}.workspace{display:flex;min-height:0;flex:1;overflow:hidden}.plan-pane{width:232px;min-width:200px;max-width:420px;resize:horizontal;overflow:auto;border-right:1px solid var(--line);background:var(--surface);flex-shrink:0}.pane-heading{height:40px;display:flex;align-items:center;justify-content:space-between;font-size:12px;color:var(--muted);padding:8px 12px;border-bottom:1px solid var(--line)}.pane-heading button{padding:0 8px}.plan-row{width:100%;text-align:left;border:0;border-radius:0;border-bottom:1px solid var(--line);padding:12px;background:transparent;min-height:64px}.plan-row.selected{background:var(--selected)}.row-title{display:flex;align-items:center;gap:8px;font-weight:500}.row-title .count{margin-left:auto;color:var(--muted)}.row-sub{display:flex;gap:12px;margin-top:8px;font-size:12px}.row-sub span{cursor:help}.selected-files{font-size:12px;color:var(--muted);padding:8px 12px;border-bottom:1px solid var(--line);overflow-wrap:anywhere}.selected-files code{display:block;margin-top:4px}#attention{padding:8px 12px;border-bottom:1px solid var(--line);display:flex;flex-wrap:wrap;gap:4px}#attention button{border:0;padding:2px 0;background:none;color:var(--error);font-size:12px}.code-pane{flex:1;min-width:0;display:flex;flex-direction:column;background:var(--canvas)}.code-heading{display:flex;gap:12px;align-items:center;padding:16px;border-bottom:1px solid var(--line)}.code-heading>div{flex:1;min-width:0}.code-heading h1{font-size:16px}.change-toolbar{height:44px;display:flex;align-items:center;gap:8px;padding:8px 16px;border-bottom:1px solid var(--line)}#view-toggle{margin-right:auto;display:flex;gap:4px}#view-toggle button[aria-pressed=true]{color:var(--primary);border-color:var(--primary)}#change-count{font-size:12px;color:var(--muted)}#item-details{padding:0 16px}#item-details p{margin:8px 0}#code{flex:1;overflow:auto;padding:16px}#checks{display:flex;gap:16px;flex-wrap:wrap;padding:12px 16px;border-top:1px solid var(--line);font-size:12px}#checks button{background:none;border:0;font-size:12px;padding:0;min-height:20px}.change{border:1px solid var(--line);margin-bottom:16px;scroll-margin:8px}.change.active{outline:1px solid var(--outline)}.file-heading{padding:8px 12px;background:var(--surface);border-bottom:1px solid var(--line);display:flex;flex-wrap:wrap;gap:8px}.file-heading code{overflow-wrap:anywhere}.change-meta{padding:8px 12px;border-top:1px solid var(--line);font-size:12px;color:var(--muted)}.diff{display:grid;grid-template-columns:56px 44px 24px minmax(0,1fr);font:12px/1.5 'IBM Plex Mono',Menlo,monospace}.provenance{grid-column:1;grid-row:1;align-self:stretch;padding:8px 4px;text-align:center;color:var(--muted);overflow-wrap:anywhere;border-right:1px solid var(--line);font-size:10px}.provenance.unplanned{background:repeating-linear-gradient(135deg,transparent,transparent 6px,#f2847e24 6px,#f2847e24 8px);color:var(--error)}.line-numbers{color:var(--muted);text-align:right;padding:8px}.sign{padding-top:8px}.diff pre{margin:0;padding:8px 12px 8px 0;white-space:pre;overflow:auto}.diff.added{background:var(--added)}.diff.removed{background:var(--removed)}.file-card{display:flex}.file-card .provenance{width:56px;flex-shrink:0}.file-values{padding:12px;overflow-wrap:anywhere;flex:1;min-width:0}.file-values dl{display:grid;grid-template-columns:72px 1fr;gap:8px;margin:12px 0}.file-values dd{margin:0}.choice-controls{padding:12px;border-top:1px solid var(--line);display:flex;flex-wrap:wrap;gap:8px;align-items:center}.choice-controls p{width:100%;margin:0 0 4px;color:var(--warning)}.conversation-pane{width:344px;min-width:280px;max-width:480px;flex-shrink:0;display:flex;flex-direction:column;background:var(--surface);border-left:1px solid var(--line);resize:horizontal;overflow:auto}.tabs{display:flex;border-bottom:1px solid var(--line)}.tabs button{flex:1;background:none;border:0;border-bottom:2px solid transparent;border-radius:0;height:40px}.tabs button[aria-pressed=true]{border-bottom-color:var(--primary);color:var(--primary)}.conversation-help{font-size:12px;padding:4px 12px}#notes{flex:1;overflow:auto;padding:12px;font-size:14px;line-height:1.5}.note{padding-bottom:16px;margin-bottom:16px;border-bottom:1px solid var(--line)}.note p{white-space:pre-wrap;overflow-wrap:anywhere}.note small{font-size:12px;color:var(--muted)}#composer{padding:12px;border-top:1px solid var(--line)}#composer label{font-size:12px;display:block;margin-bottom:8px}textarea{background:var(--canvas);color:var(--text);border:1px solid var(--outline);border-radius:4px;width:100%;padding:8px;resize:vertical;min-height:90px;margin-bottom:8px}footer{height:36px;flex-shrink:0;border-top:1px solid var(--line);display:flex;align-items:center;gap:24px;padding:0 16px;font-size:12px}footer>span:last-child{margin-left:auto}footer button{font-size:12px;min-height:24px}#banner:not(:empty){padding:12px 16px;background:var(--surface);border-bottom:1px solid var(--line);color:var(--warning)}.empty{padding:48px 24px;color:var(--muted);max-width:640px}.comparison{display:grid;grid-template-columns:1fr 1fr;gap:12px;margin-bottom:16px}.comparison section{min-width:0;background:var(--surface);padding:12px;border:1px solid var(--line)}details>summary{cursor:pointer;color:var(--primary)}dialog{background:var(--raised);color:var(--text);border:1px solid var(--line);border-radius:6px;width:640px;max-height:80vh;overflow:auto}dialog::backdrop{background:#101216aa}.narrow{display:none}#conversation-toggle{font-size:12px}#saved{display:block;font-size:12px;color:var(--success);margin-top:8px} +@media(min-width:1440px){#conversation-toggle{display:none}body.conversation-closed #conversation-toggle{display:block}body.conversation-closed .conversation-pane{display:none}}@media(min-width:1280px) and (max-width:1439px){.plan-pane{width:208px}.conversation-pane{display:none}body.conversation-open .conversation-pane{display:flex;width:300px}.app-bar nav{gap:12px}.app-bar{gap:16px}#repository{display:none}}@media(max-width:1279px){#app{display:none}.narrow{display:block;margin:15vh auto;padding:32px;max-width:600px}}@media(prefers-reduced-motion:no-preference){button{transition:background-color 80ms ease-out}.good{transition:color 150ms ease-out}} diff --git a/web/server.ts b/web/server.ts new file mode 100644 index 0000000..845de61 --- /dev/null +++ b/web/server.ts @@ -0,0 +1,44 @@ +import { createServer } from 'node:http'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { randomBytes, timingSafeEqual } from 'node:crypto'; +import { ReviewService, type ReviewConfig } from '../runner/review.ts'; +const publicRoot = new URL('./public/', import.meta.url); +export async function startServer(config: ReviewConfig, port = 4318) { + const service = new ReviewService(config), token = randomBytes(32).toString('hex'); + const server = createServer(async (req, res) => { + const address = server.address(); const actualPort = address && typeof address !== 'string' ? address.port : port; + const origin = `http://127.0.0.1:${actualPort}`; + res.setHeader('Cache-Control', 'no-store'); res.setHeader('X-Content-Type-Options', 'nosniff'); + res.setHeader('Content-Security-Policy', "default-src 'self'; script-src 'self'; style-src 'self'; font-src 'self'; connect-src 'self'; img-src 'self' data:; frame-ancestors 'none'; base-uri 'none'; form-action 'self'"); + const json = (status: number, value: unknown) => { res.writeHead(status, { 'Content-Type': 'application/json' }); res.end(JSON.stringify(value)); }; + try { + if (req.headers.host !== `127.0.0.1:${actualPort}` || (req.headers.origin && req.headers.origin !== origin)) { json(403, { error: 'Local origin required.' }); return; } + const path = new URL(req.url ?? '/', origin).pathname; + if (path.startsWith('/api/')) { + const supplied = req.headers['x-codeboost-token']; + if (typeof supplied !== 'string' || supplied.length !== token.length || !timingSafeEqual(Buffer.from(supplied), Buffer.from(token))) { json(403, { error: 'Open the private local URL printed by the CLI.' }); return; } + if (req.method === 'GET' && path === '/api/review') { json(200, service.load()); return; } + if (req.method !== 'POST' || path !== '/api/action' || req.headers['content-type'] !== 'application/json') { json(405, { error: 'Unsupported request.' }); return; } + const chunks: Buffer[] = []; let size = 0; + for await (const chunk of req) { size += chunk.length; if (size > 16384) { json(413, { error: 'Request too large.' }); return; } chunks.push(chunk); } + const body = new TextDecoder('utf-8', { fatal: true }).decode(Buffer.concat(chunks)); + json(200, service.act(JSON.parse(body))); return; + } + if (req.method !== 'GET') { json(405, { error: 'Method not allowed.' }); return; } + const files: Record = { '/': ['index.html', 'text/html'], '/app.js': ['app.js', 'text/javascript'], '/style.css': ['style.css', 'text/css'] }; + const file = files[path]; + if (file) { res.writeHead(200, { 'Content-Type': `${file[1]}; charset=utf-8` }); res.end(readFileSync(new URL(file[0], publicRoot))); return; } + const font = /^\/fonts\/(ibm-plex-(?:sans|mono)-latin-(?:400|500|600)-normal\.woff2)$/.exec(path); + if (font) { + const family = font[1]!.startsWith('ibm-plex-sans') ? 'ibm-plex-sans' : 'ibm-plex-mono'; + res.writeHead(200, { 'Content-Type': 'font/woff2' }); res.end(readFileSync(fileURLToPath(new URL(`../node_modules/@fontsource/${family}/files/${font[1]}`, import.meta.url)))); return; + } + json(404, { error: 'Not found.' }); + } catch (error) { json(409, { error: error instanceof Error ? error.message : 'Review failed.' }); } + }); + server.requestTimeout = 15000; + await new Promise((resolve, reject) => { server.once('error', reject); server.listen(port, '127.0.0.1', () => { server.removeListener('error', reject); resolve(); }); }).catch(error => { service.close(); throw error; }); + const address = server.address(); if (!address || typeof address === 'string') throw new Error('Cannot determine local address.'); + return { server, service, token, url: `http://127.0.0.1:${address.port}/#${token}`, close: () => new Promise((resolve, reject) => server.close(error => { service.close(); error ? reject(error) : resolve(); })) }; +} From 19aabad7237213ded665c0c6cd4cc6c454137e58 Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 23 Sep 2026 09:01:25 -0700 Subject: [PATCH 2/8] Harden review actions and complete bounded file previews --- .gitattributes | 4 + core/linking.ts | 2 +- docs/implementation/read-only-review.md | 8 +- git/history.ts | 16 +- runner/review.ts | 21 +- test/browser/review.spec.ts | 24 + test/review.test.ts | 13 +- web/public/app.js | 501 +++++++++++++++--- web/public/index.html | 144 ++++- web/public/style.css | 673 +++++++++++++++++++++++- web/server.ts | 2 +- 11 files changed, 1300 insertions(+), 108 deletions(-) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..e90622b --- /dev/null +++ b/.gitattributes @@ -0,0 +1,4 @@ +# These are hand-authored application sources, not build output. +web/public/*.js linguist-generated=false linguist-vendored=false +web/public/*.css linguist-generated=false linguist-vendored=false +web/public/*.html linguist-generated=false linguist-vendored=false diff --git a/core/linking.ts b/core/linking.ts index e9e4960..945eab8 100644 --- a/core/linking.ts +++ b/core/linking.ts @@ -1,7 +1,7 @@ import { diffArrays } from 'diff'; import type { Plan } from './plan.ts'; -export interface FileVersion { oid: string; mode: string; text: string | null } +export interface FileVersion { oid: string; mode: string; text: string | null; byteSize?: number; preview?: string } export interface ContextRange { oldStart: number; oldCount: number; newStart: number; newCount: number; name: string } export interface FileDelta { oldPath: string | null; newPath: string | null; diff --git a/docs/implementation/read-only-review.md b/docs/implementation/read-only-review.md index 8102d63..8f5d9f2 100644 --- a/docs/implementation/read-only-review.md +++ b/docs/implementation/read-only-review.md @@ -21,10 +21,16 @@ Store schema v2 adds the review counter and per-item notes through a transaction ## Known limits and remaining gates - No agent answers, test execution, AI findings ingestion, send-to-agent action, or merge control is included. These belong after the go/no-go gate. Four checks distinguish unavailable evidence from success. -- File cards show mode/path/object IDs; binary/image previews and byte-size retrieval are not implemented and explicitly say unavailable. The pure history API does not expose binary bytes. +- File cards show mode/path/object IDs and blob byte sizes. PNG/JPEG/GIF/WebP previews are bounded to 1 MiB each and 4 MiB across a history; unsupported/oversized images say unavailable. Gitlink byte sizes are not applicable. SVG/HTML is never embedded. - The protocol at `docs/experiments/review-protocol.md` must be filled with the real pairs and committed before the first timed review. The manual assignment, real paired PRs, human timing, and final result are pending. Do not mark issue #3 closed or claim the gate passed. - The planting helper is intentionally limited to disposable clones and supported regular top-level paths; it never publishes PRs. ## Validation Baseline before this slice: 157 tests. Run `npm run typecheck`, `npm test`, and `npm run test:browser`. Browser tests use real Git and SQLite with Chromium and cover persistence, assignments, metadata, no-change approval, stale views, unsafe origins, keyboard/breakpoints, error display, and untrusted text. A browser regression exposed false stale reasons from JSON field order; structural comparison replaced that check. The plant test verifies source HEAD stays unchanged and both plants retain ledger ownership. + +A large-change regression reproduced HTTP 413 when the browser sent the full content-based choice key for a 20 KB segment. Browser segment IDs are now bounded SHA-256 identifiers; the runner reconstructs the original identity/content/copy key before saving the choice. This retains choice expiry semantics without sending source content back in a review command. + +## Review round 1 + +Reproduced and fixed no-change approval with item-owned ambiguous segments; the runner refuses it and the UI directs the user to resolve attribution first. Previously approved no-change items become stale if ambiguous work appears. Reproduced malformed non-ASCII credentials returning a generic conflict instead of unauthorized; credentials now require the expected ASCII hex shape before constant-time comparison. Reproduced stale item controls surviving a failed refresh; errors now discard the loaded view and require refresh. Added acceptance persistence and whole-plan empty-state browser coverage. No findings declined. diff --git a/git/history.ts b/git/history.ts index 1ed1f4d..892b0a8 100644 --- a/git/history.ts +++ b/git/history.ts @@ -89,7 +89,8 @@ export function readHistory(repo: string, baseRef: string, headRef = 'HEAD', lim return { sha, parent, files: [] as FileDelta[] }; }); if (expectedParent !== head) throw new Error('The base must be an ancestor of the head.'); - const blobs = new Map(); + const blobs = new Map(); + let previewBytes = 0; const version = (oid: string, mode: string): FileVersion | null => { if (/^0+$/.test(oid)) return null; if (mode === '160000') return { oid, mode, text: null }; // gitlink is not a local blob @@ -100,9 +101,18 @@ export function readHistory(repo: string, baseRef: string, headRef = 'HEAD', lim blobBytes += data.length; let text: string | null = null; if (!data.includes(0)) { try { text = new TextDecoder('utf-8', { fatal: true, ignoreBOM: true }).decode(data); } catch { /* binary */ } } - blobs.set(oid, text); + let preview: string | undefined; + // Only bounded raster images; never embed SVG/HTML or fetch external references. + const mime = data.subarray(0,8).equals(Buffer.from([137,80,78,71,13,10,26,10])) ? 'image/png' : + data.subarray(0,3).equals(Buffer.from([255,216,255])) ? 'image/jpeg' : + /^GIF8[79]a$/.test(data.subarray(0,6).toString('ascii')) ? 'image/gif' : + data.subarray(0,4).toString('ascii') === 'RIFF' && data.subarray(8,12).toString('ascii') === 'WEBP' ? 'image/webp' : null; + if (mime && data.length <= 1024 * 1024 && previewBytes + data.length <= 4 * 1024 * 1024) { + preview = `data:${mime};base64,${data.toString('base64')}`; previewBytes += data.length; + } + blobs.set(oid, { text, byteSize: data.length, ...(preview ? { preview } : {}) }); } - return { oid, mode, text: blobs.get(oid)! }; + return { oid, mode, ...blobs.get(oid)! }; }; const diff = (from: string, to: string, contexts: boolean): FileDelta[] => { const raw = accountDiff(run('diff', '--ignore-submodules=none', '--no-relative', '--raw', '-z', '--no-abbrev', '--no-ext-diff', '--no-textconv', '-M', from, to, '--')); diff --git a/runner/review.ts b/runner/review.ts index 8b8e490..84e1fb6 100644 --- a/runner/review.ts +++ b/runner/review.ts @@ -25,20 +25,32 @@ export class ReviewService { const history = readHistory(repository, snapshot.base, 'HEAD'); if (history.head !== snapshot.head) snapshot = this.store.recordHistory(identity, { revision: plan.revision, snapshotId: snapshot.id, reviewVersion }, history.base, history.head, []); const pathKey = (path: string) => { + if (!pathIdentity.caseSensitive && /[^\x20-\x7e]/.test(path)) throw new Error('Non-ASCII case-insensitive paths require a filesystem-specific identity adapter.'); const normalized = pathIdentity.unicodeNormalization === 'NFC' ? path.normalize('NFC') : path; return pathIdentity.caseSensitive ? normalized : normalized.toLowerCase(); }; const raw = linkHistory(plan, history, this.store.ownership(identity, plan.revision), pathKey); const saved = this.store.getReview(identity), keys = choiceKeys(raw, identity); + const deltas = new Map(history.final.map(file => [JSON.stringify([file.newPath ?? file.oldPath, file.oldPath]), file])); + let previewBudget = 6 * 1024 * 1024; + const preview = (value: string | undefined) => { + if (!value || value.length > previewBudget) return null; + previewBudget -= value.length; return value; + }; const segments = applyChoices(plan, raw, saved.choices, identity).map((segment, index) => { const target = plan.items.find(item => item.id === segment.row); if (target && segment.row !== raw[index]!.row) { const declared = new Set(target.files.flatMap(file => [file.path, ...(file.renamed_from ? [file.renamed_from] : [])]).map(pathKey)); segment.scope = declared.has(pathKey(segment.path)) ? 'in-scope' : 'out-of-scope'; } - return { ...segment, key: keys[index]!, originalRow: raw[index]!.row }; + const delta = segment.kind === 'file' ? deltas.get(JSON.stringify([segment.path, segment.oldPath])) : undefined; + const file = delta ? { oldSize: delta.before?.byteSize ?? null, newSize: delta.after?.byteSize ?? null, beforePreview: preview(delta.before?.preview), afterPreview: preview(delta.after?.preview) } : null; + return { ...segment, file, key: createHash('sha256').update(keys[index]!).digest('hex'), originalRow: raw[index]!.row }; }); const states = approvalStates(plan, segments, saved.approvals, identity); + for (const item of plan.items) { + if (states[item.id] === 'approved' && ((!segments.some(segment => segment.row === item.id) && segments.some(segment => segment.row === 'Ambiguous' && segment.owners.includes(item.id))) || item.depends_on.some(id => states[id] === 'stale'))) states[item.id] = 'stale'; + } const expected: ReviewState = { revision: plan.revision, snapshotId: snapshot.id, reviewVersion }; const notes = this.store.getReviewNotes(identity); if (this.store.reviewVersion(identity) !== reviewVersion || this.store.getPlan(identity).revision !== plan.revision || this.store.getSnapshot(identity).id !== snapshot.id) throw new Error('Stale review state. Reload before writing.'); @@ -55,7 +67,7 @@ export class ReviewService { for (const dep of item.depends_on) if (states[dep] === 'stale') reasons.push(`Depends on ${dep}, which changed`); if (!reasons.length) reasons.push('Code or plan definition changed'); } - return { ...item, state: states[item.id], count: owned.length, reasons, before, + return { ...item, state: states[item.id], count: owned.length, ambiguousCount: ambiguous, reasons, before, checks: { attributed: ambiguous ? `! ${ambiguous} ambiguous` : owned.length ? '✓ Attributed' : '– No changes', scope: outside.length ? `✕ ${new Set(outside).size} out of scope` : owned.length ? '✓ In scope' : '– No changes', tests: item.acceptance.some(check => check.type === 'cmd') ? '– Not run' : '– No tests defined', ai: '– Not run' }, outside: [...new Set(outside)], }; }); @@ -69,13 +81,16 @@ export class ReviewService { if (command.token !== view.token) throw new Error('Stale review state. Reload before writing.'); const { identity } = this.config; if (command.action === 'approve' && typeof command.item === 'string') { + const item = view.items.find(item => item.id === command.item); + if (item && item.count === 0 && item.ambiguousCount > 0) throw new Error('Resolve this item’s ambiguous changes before confirming no change is needed.'); const approval = approveItem(view.plan, view.segments, command.item, identity, command.confirmNoChange === true); this.store.saveReview(identity, view.expected, [approval], []); } else if ((command.action === 'assign' || command.action === 'accept') && typeof command.key === 'string') { const segment = view.segments.find(segment => segment.key === command.key); if (!segment || !['Ambiguous', 'Unplanned'].includes(segment.row)) throw new Error('This change cannot be assigned or accepted.'); const item = command.action === 'assign' && typeof command.item === 'string' ? command.item : null; - this.store.saveReview(identity, view.expected, [], [{ key: segment.key, action: command.action, item }]); + const storedKey = choiceKeys(view.segments, identity)[view.segments.indexOf(segment)]!; + this.store.saveReview(identity, view.expected, [], [{ key: storedKey, action: command.action, item }]); } else if (command.action === 'note' && typeof command.item === 'string' && typeof command.text === 'string' && (command.kind === 'question' || command.kind === 'change')) { this.store.addReviewNote(identity, view.expected, command.item, command.kind, command.text); } else throw new Error('Unknown review command.'); diff --git a/test/browser/review.spec.ts b/test/browser/review.spec.ts index a7ea3bb..5887a3d 100644 --- a/test/browser/review.spec.ts +++ b/test/browser/review.spec.ts @@ -45,3 +45,27 @@ test('shows an honest history error and keeps markup in notes as text',async({pa const repository=app.service.config.repository;execFileSync('git',['checkout','--orphan','unrelated'],{cwd:repository,stdio:'pipe'});execFileSync('git',['-c','core.hooksPath=/dev/null','commit','-m','Unrelated history'],{cwd:repository,stdio:'pipe'}); await page.getByRole('button',{name:'Refresh',exact:true}).click();await expect(page.getByText('Could not read this branch’s history.',{exact:false})).toBeVisible();await expect(page.getByRole('button',{name:'Approve P1',exact:true})).not.toBeVisible(); }); +test('can assign a large foreign change without sending its content back in the command',async({request})=>{ + const repository=app.service.config.repository;writeFileSync(join(repository,'debug.log'),'x'.repeat(20000)+'\n');execFileSync('git',['-c','core.hooksPath=/dev/null','commit','-am','Large foreign change'],{cwd:repository,stdio:'pipe'}); + const base=app.url.split('#')[0]!,headers={'x-codeboost-token':app.token};const view=await(await request.get(base+'api/review',{headers})).json();const segment=view.segments.find((s:{content:string;row:string})=>s.row==='Unplanned'&&s.content.length>19000); + const response=await request.post(base+'api/action',{headers:{...headers,'Content-Type':'application/json'},data:{action:'assign',item:'P1',key:segment.key,token:view.token}}); + expect(response.status()).toBe(200); +}); +test('shows bounded raster previews and byte sizes for file-change cards',async({page})=>{ + const repository=app.service.config.repository;writeFileSync(join(repository,'pixel.png'),Buffer.from('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAwMCAO+aRHsAAAAASUVORK5CYII=','base64'));execFileSync('git',['add','pixel.png'],{cwd:repository});execFileSync('git',['-c','core.hooksPath=/dev/null','commit','-m','Add image'],{cwd:repository,stdio:'pipe'}); + await page.goto(app.url);await page.getByRole('button',{name:/Unplanned changes/}).click();await expect(page.getByRole('img',{name:'Current image in pixel.png'})).toBeVisible();await expect(page.getByRole('img',{name:'Current image in pixel.png'})).toHaveJSProperty('naturalWidth',1);await expect(page.getByText('Size: N/A → 68 bytes',{exact:true})).toBeVisible(); +}); +test('rejects malformed non-ASCII credentials consistently',async({request})=>{ + const response=await request.get(app.url.split('#')[0]+'api/review',{headers:{'x-codeboost-token':'é'.repeat(64)}});expect(response.status()).toBe(403); +}); +test('keeps stale item controls unavailable after a failed refresh',async({page})=>{ + await page.goto(app.url);const repository=app.service.config.repository;execFileSync('git',['checkout','--orphan','unrelated'],{cwd:repository,stdio:'pipe'});execFileSync('git',['-c','core.hooksPath=/dev/null','commit','-m','Other history'],{cwd:repository,stdio:'pipe'}); + await page.getByRole('button',{name:'Refresh',exact:true}).click();await expect(page.getByText('Could not read this branch’s history.',{exact:false})).toBeVisible();await expect(page.getByRole('button',{name:/P3 Confirm API compatibility/})).toHaveCount(0); +}); +test('accepts a foreign segment and keeps that choice across reload',async({page})=>{ + await page.goto(app.url);await page.getByRole('button',{name:/Unplanned changes/}).click();await page.getByRole('button',{name:'Accept as is',exact:true}).first().click();await page.getByRole('button',{name:/Accepted 1/}).click();await expect(page.getByRole('article').first()).toBeVisible();await page.reload();await page.getByRole('button',{name:/Accepted 1/}).click();await expect(page.getByRole('article').first()).toBeVisible(); +}); +test('shows the whole-plan empty state without claiming checks passed',async({page})=>{ + const {repository,identity}=app.service.config;const base=app.service.store.getSnapshot(identity).base;execFileSync('git',['reset','--hard',base],{cwd:repository,stdio:'pipe'}); + await page.goto(app.url);await expect(page.getByRole('heading',{name:'No code changes yet'})).toBeVisible();await expect(page.getByRole('button',{name:'Confirm no change needed',exact:true})).toBeVisible();await expect(page.getByRole('button',{name:'AI review: – Not run',exact:true})).toBeVisible(); +}); diff --git a/test/review.test.ts b/test/review.test.ts index 9c6097e..ce90a0f 100644 --- a/test/review.test.ts +++ b/test/review.test.ts @@ -1,10 +1,12 @@ -import { afterEach, it, expect } from 'vitest'; +import { afterEach, it, expect, vi } from 'vitest'; import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { DatabaseSync } from 'node:sqlite'; import { createDemo } from '../scripts/demo.ts'; import { ReviewService } from '../runner/review.ts'; +// Each integration case performs several bounded real-Git reads. +vi.setConfig({ testTimeout: 15000 }); const roots:string[]=[];const services:ReviewService[]=[]; afterEach(()=>{services.splice(0).forEach(service=>service.close());roots.splice(0).forEach(root=>rmSync(root,{recursive:true,force:true}));}); function fixture(){const root=mkdtempSync(join(tmpdir(),'codeboost-review-'));roots.push(root);const config=createDemo(join(root,'demo'));const service=new ReviewService(config);services.push(service);return {service,config};} @@ -29,3 +31,12 @@ it('rejects a concurrent store review edit through the atomic review counter',() other.store.addReviewNote(config.identity,view.expected,'P1','change','Please explain'); expect(()=>service.store.saveReview(config.identity,view.expected,[],[])).toThrow(/Stale/); }); +it('refuses no-change confirmation while the item still owns ambiguous changes',async()=>{ + const {service,config}=fixture();const {writeFileSync}=await import('node:fs');const {execFileSync}=await import('node:child_process'); + writeFileSync(join(config.repository,'retry.ts'),'export function delay(attempt: number) {\n return Math.min(10000, 200 * 2 ** attempt);\n}\n'); + execFileSync('git',['-c','core.hooksPath=/dev/null','commit','-am','P2 changes retry'],{cwd:config.repository,stdio:'pipe'}); + const head=execFileSync('git',['rev-parse','HEAD'],{cwd:config.repository,encoding:'utf8'}).trim();const snapshot=service.store.getSnapshot(config.identity); + service.store.recordHistory(config.identity,{revision:1,snapshotId:snapshot.id},snapshot.base,head,[{sha:head,owner:'P2',origin:'owned',sourceSha:null}]); + const view=service.load();expect(view.items[0]!.count).toBe(0);expect(view.segments.some(s=>s.row==='Ambiguous'&&s.owners.includes('P1'))).toBe(true); + expect(()=>service.act({action:'approve',item:'P1',confirmNoChange:true,token:view.token})).toThrow(/ambiguous/i); +}); diff --git a/web/public/app.js b/web/public/app.js index 20f5ac8..ba183cf 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -1,98 +1,433 @@ -const $ = id => document.getElementById(id); -const esc = value => String(value ?? '').replace(/[&<>"']/g, c => ({ '&': '&', '<': '<', '>': '>', '"': '"', "'": ''' })[c]); -const credential = location.hash.slice(1) || sessionStorage.getItem('codeboost-token') || ''; -if (location.hash) { sessionStorage.setItem('codeboost-token', credential); history.replaceState(null, '', location.pathname); } -let data, selected, change = 0, mode = 'question', since = false, busy = false; +const $ = (id) => document.getElementById(id); +const esc = (value) => + String(value ?? "").replace( + /[&<>"']/g, + (c) => + ({ "&": "&", "<": "<", ">": ">", '"': """, "'": "'" })[ + c + ], + ); +const credential = + location.hash.slice(1) || sessionStorage.getItem("codeboost-token") || ""; +if (location.hash) { + sessionStorage.setItem("codeboost-token", credential); + history.replaceState(null, "", location.pathname); +} +let data, + selected, + change = 0, + mode = "question", + since = false, + busy = false; const drafts = new Map(); -const statusClass = text => text.startsWith('✓') ? 'good' : text.startsWith('✕') ? 'bad' : text.startsWith('!') ? 'warn' : 'neutral'; +const statusClass = (text) => + text.startsWith("✓") + ? "good" + : text.startsWith("✕") + ? "bad" + : text.startsWith("!") + ? "warn" + : "neutral"; async function api(path, body) { - const response = await fetch(path, { method: body ? 'POST' : 'GET', headers: { 'x-codeboost-token': credential, ...(body ? { 'Content-Type': 'application/json' } : {}) }, body: body ? JSON.stringify(body) : undefined }); - const value = await response.json(); if (!response.ok) throw new Error(value.error || 'Request failed.'); return value; + const response = await fetch(path, { + method: body ? "POST" : "GET", + headers: { + "x-codeboost-token": credential, + ...(body ? { "Content-Type": "application/json" } : {}), + }, + body: body ? JSON.stringify(body) : undefined, + }); + const value = await response.json(); + if (!response.ok) throw new Error(value.error || "Request failed."); + return value; +} +function rememberDraft() { + if (selected) drafts.set(`${selected}:${mode}`, $("message").value); +} +function showFailure(message) { + data = null; + $("banner").textContent = message; + $("progress").textContent = "Review unavailable"; + $("item-title").textContent = "Review unavailable"; + $("change-count").textContent = ""; + for (const id of [ + "items", + "attention", + "notes", + "checks", + "view-toggle", + "item-details", + ]) + $(id).innerHTML = ""; + $("approve").hidden = true; + $("message").disabled = true; + $("save-note").disabled = true; + $("previous").disabled = true; + $("next").disabled = true; + $("code").innerHTML = + '

Review could not load

Refresh after resolving the error above.

'; } -function rememberDraft() { if (selected) drafts.set(`${selected}:${mode}`, $('message').value); } async function refresh() { - if (busy) return; busy = true; - $('banner').textContent = 'Linking changes to plan items…'; - try { rememberDraft(); data = await api('/api/review'); selected ??= data.items[0]?.id || 'Unplanned'; since = data.items.find(item => item.id === selected)?.state === 'stale'; render(); } - catch (error) { $('banner').textContent = `Could not read this branch’s history. ${error.message} Use Refresh to retry.`; $('code').innerHTML = '

Review could not load

Retry after resolving the error above. Open task shows the last loaded task, if available.

'; $('approve').hidden = true; } - finally { busy = false; } + if (busy) return; + busy = true; + $("banner").textContent = "Linking changes to plan items…"; + try { + rememberDraft(); + data = await api("/api/review"); + selected ??= data.items[0]?.id || "Unplanned"; + since = data.items.find((item) => item.id === selected)?.state === "stale"; + render(); + } catch (error) { + showFailure( + `Could not read this branch’s history. ${error.message} Use Refresh to retry.`, + ); + } finally { + busy = false; + } } async function act(command) { - if (busy || !data) return false; busy = true; - try { rememberDraft(); data = await api('/api/action', { ...command, token: data.token }); render(); return true; } - catch (error) { $('banner').textContent = `${error.message} Refresh to review the latest state.`; return false; } - finally { busy = false; } + if (busy || !data) return false; + busy = true; + try { + rememberDraft(); + data = await api("/api/action", { ...command, token: data.token }); + render(); + return true; + } catch (error) { + showFailure(`${error.message} Refresh to review the latest state.`); + return false; + } finally { + busy = false; + } +} +function select(id) { + rememberDraft(); + selected = id; + change = 0; + since = data.items.find((item) => item.id === id)?.state === "stale"; + render(); } -function select(id) { rememberDraft(); selected = id; change = 0; since = data.items.find(item => item.id === id)?.state === 'stale'; render(); } function render() { - const item = data.items.find(item => item.id === selected); - if (!item && !['Unplanned', 'Ambiguous', 'Accepted'].includes(selected)) { selected = data.items[0]?.id || 'Unplanned'; return render(); } - $('repository').textContent = data.repository; - $('issue').textContent = `#${data.plan.issue} ${data.plan.summary} · r${data.plan.revision}`; - $('progress').textContent = `${data.approved} of ${data.items.length} approved`; - $('banner').textContent = data.demo ? 'Demo repository · real Git changes and local SQLite storage. No tests or AI review have been run for this demo.' : ''; - const unplanned = data.segments.filter(s => s.row === 'Unplanned').length, ambiguous = data.segments.filter(s => s.row === 'Ambiguous').length; - $('attention').innerHTML = ` · `; - const symbols = { approved: '✓', stale: '!', unreviewed: '○' }; - $('items').innerHTML = data.items.map(entry => `${entry.id === selected ? `
Declared files${entry.files.map(file => `${esc(file.path)}`).join('')}
` : ''}`).join('') + ['Ambiguous','Unplanned','Accepted'].map(row => ``).join(''); - document.querySelectorAll('[data-select]').forEach(button => button.addEventListener('click', () => select(button.dataset.select))); - $('item-id').textContent = item?.id || 'REVIEW EXCEPTIONS'; $('item-title').textContent = item?.title || selected; - $('item-details').innerHTML = item ? `

${esc(item.intent)}

${item.reasons.map(reason => `

! Stale: ${esc(reason)}

`).join('')}` : ''; - $('approve').hidden = !item; $('approve').textContent = item?.count ? `Approve ${item.id}` : 'Confirm no change needed'; $('approve').disabled = item?.state === 'approved'; - $('view-toggle').innerHTML = item?.state === 'stale' ? `` : ''; - document.querySelectorAll('[data-since]').forEach(button => button.addEventListener('click', () => { since = button.dataset.since === 'true'; renderCode(); })); - $('checks').innerHTML = item ? Object.entries(item.checks).map(([name,text]) => ``).join('') : ''; - document.querySelectorAll('[data-check]').forEach(button => button.addEventListener('click', () => { - if (button.dataset.check === 'attributed' && item.checks.attributed.startsWith('!')) select('Ambiguous'); - else showDialog(`

${esc(button.textContent)}

${esc(button.dataset.check === 'scope' ? item.outside.join('\n') || 'Every attributed change is inside this item’s declared files.' : button.dataset.check === 'tests' ? item.acceptance.map(check => `${check.type}: ${check.text}`).join('\n') + '\n\nThis screen does not run commands.' : button.dataset.check === 'ai' ? 'No AI review has run. This does not mean the change has no problems.' : 'Attribution comes from the commit ledger, not commit messages.')}
`); - })); - $('notes').innerHTML = item ? data.notes.filter(note => note.item === selected).map(note => `
You · ${note.kind === 'change' ? 'Change requested' : 'Question'}

${esc(note.text)}

r${note.revision} · ${esc(new Date(note.createdAt).toLocaleString())}${note.kind === 'change' ? ' · Pending' : ' · Awaiting discussion'}
`).join('') || '

No conversation yet. Keep questions and requested changes beside the evidence.

' : '

Select a plan item to add a question or request a change.

'; - $('message').disabled = !item; $('save-note').disabled = !item; $('message').value = drafts.get(`${selected}:${mode}`) || ''; + const item = data.items.find((item) => item.id === selected); + if (!item && !["Unplanned", "Ambiguous", "Accepted"].includes(selected)) { + selected = data.items[0]?.id || "Unplanned"; + return render(); + } + $("repository").textContent = data.repository; + $("issue").textContent = + `#${data.plan.issue} ${data.plan.summary} · r${data.plan.revision}`; + $("progress").textContent = + `${data.approved} of ${data.items.length} approved`; + $("banner").textContent = data.demo + ? "Demo repository · real Git changes and local SQLite storage. No tests or AI review have been run for this demo." + : ""; + const unplanned = data.segments.filter((s) => s.row === "Unplanned").length, + ambiguous = data.segments.filter((s) => s.row === "Ambiguous").length; + $("attention").innerHTML = + ` · `; + const symbols = { approved: "✓", stale: "!", unreviewed: "○" }; + $("items").innerHTML = + data.items + .map( + (entry) => + `${entry.id === selected ? `
Declared files${entry.files.map((file) => `${esc(file.path)}`).join("")}
` : ""}`, + ) + .join("") + + ["Ambiguous", "Unplanned", "Accepted"] + .map( + (row) => + ``, + ) + .join(""); + document + .querySelectorAll("[data-select]") + .forEach((button) => + button.addEventListener("click", () => select(button.dataset.select)), + ); + $("item-id").textContent = item?.id || "REVIEW EXCEPTIONS"; + $("item-title").textContent = item?.title || selected; + $("item-details").innerHTML = item + ? `

${esc(item.intent)}

${item.reasons.map((reason) => `

! Stale: ${esc(reason)}

`).join("")}` + : ""; + $("approve").hidden = !item; + $("approve").textContent = item?.count + ? `Approve ${item.id}` + : item?.ambiguousCount + ? "Resolve ambiguous changes" + : "Confirm no change needed"; + $("approve").disabled = item?.state === "approved"; + $("view-toggle").innerHTML = + item?.state === "stale" + ? `` + : ""; + document.querySelectorAll("[data-since]").forEach((button) => + button.addEventListener("click", () => { + since = button.dataset.since === "true"; + renderCode(); + }), + ); + $("checks").innerHTML = item + ? Object.entries(item.checks) + .map( + ([name, text]) => + ``, + ) + .join("") + : ""; + document.querySelectorAll("[data-check]").forEach((button) => + button.addEventListener("click", () => { + if ( + button.dataset.check === "attributed" && + item.checks.attributed.startsWith("!") + ) + select("Ambiguous"); + else + showDialog( + `

${esc(button.textContent)}

${esc(button.dataset.check === "scope" ? item.outside.join("\n") || "Every attributed change is inside this item’s declared files." : button.dataset.check === "tests" ? item.acceptance.map((check) => `${check.type}: ${check.text}`).join("\n") + "\n\nThis screen does not run commands." : button.dataset.check === "ai" ? "No AI review has run. This does not mean the change has no problems." : "Attribution comes from the commit ledger, not commit messages.")}
`, + ); + }), + ); + $("notes").innerHTML = item + ? data.notes + .filter((note) => note.item === selected) + .map( + (note) => + `
You · ${note.kind === "change" ? "Change requested" : "Question"}

${esc(note.text)}

r${note.revision} · ${esc(new Date(note.createdAt).toLocaleString())}${note.kind === "change" ? " · Pending" : " · Awaiting discussion"}
`, + ) + .join("") || + '

No conversation yet. Keep questions and requested changes beside the evidence.

' + : '

Select a plan item to add a question or request a change.

'; + $("message").disabled = !item; + $("save-note").disabled = !item; + $("message").value = drafts.get(`${selected}:${mode}`) || ""; renderCode(); } function renderCode() { - const item = data.items.find(item => item.id === selected), segments = data.segments.filter(s => s.row === selected); + const item = data.items.find((item) => item.id === selected), + segments = data.segments.filter((s) => s.row === selected); change = Math.max(0, Math.min(change, segments.length - 1)); - $('change-count').textContent = segments.length ? `Change ${change + 1} of ${segments.length}` : 'No changes'; - $('previous').disabled = !segments.length || change === 0; $('next').disabled = !segments.length || change === segments.length - 1; - let comparison = ''; - if (since && item?.state === 'stale' && item.before) { - const prior = item.before.segments.map(s => `${s.path} ${s.operation || ''}\n${s.content}`).join('\n'); - const now = segments.map(s => `${s.path} ${s.operation || ''}\n${s.content}`).join('\n'); - comparison = `

At approval

${esc(prior || 'No changes')}
Approved plan item
${esc(JSON.stringify(item.before.item,null,2))}

Now

${esc(now || 'No changes')}
Current plan item
${esc(JSON.stringify(data.plan.items.find(p=>p.id===item.id),null,2))}
`; + $("change-count").textContent = segments.length + ? `Change ${change + 1} of ${segments.length}` + : "No changes"; + $("previous").disabled = !segments.length || change === 0; + $("next").disabled = !segments.length || change === segments.length - 1; + let comparison = ""; + if (since && item?.state === "stale" && item.before) { + const prior = item.before.segments + .map((s) => `${s.path} ${s.operation || ""}\n${s.content}`) + .join("\n"); + const now = segments + .map((s) => `${s.path} ${s.operation || ""}\n${s.content}`) + .join("\n"); + comparison = `

At approval

${esc(prior || "No changes")}
Approved plan item
${esc(JSON.stringify(item.before.item, null, 2))}

Now

${esc(now || "No changes")}
Current plan item
${esc(
+      JSON.stringify(
+        data.plan.items.find((p) => p.id === item.id),
+        null,
+        2,
+      ),
+    )}
`; } - $('code').innerHTML = comparison + (segments.map((segment,index) => { - const provenance = segment.row === 'Accepted' ? 'Accepted' : segment.row; - let content; - if (segment.kind === 'file') { - const meta = JSON.parse(segment.content); - const label = meta.oldPath !== meta.newPath && meta.oldPath && meta.newPath ? 'Renamed file' : meta.oldMode === '160000' || meta.newMode === '160000' ? 'Submodule pointer' : meta.oldMode === '120000' || meta.newMode === '120000' ? 'Symbolic link' : meta.oldMode !== meta.newMode && meta.oldMode && meta.newMode ? 'File mode changed' : 'File change'; - content = `
${esc(provenance)}
▧ ${label}
Path
${esc(meta.oldPath || 'Absent')} → ${esc(meta.newPath || 'Absent')}
Mode
${esc(meta.oldMode || 'Absent')} → ${esc(meta.newMode || 'Absent')}

No preview available · size unavailable

Details · content IDs
${esc(JSON.stringify(meta,null,2))}
`; - } else { - const lines = segment.content.split('\n'); if (lines.at(-1) === '') lines.pop(); - const start = segment.operation === '+' ? segment.newLine : segment.oldLine; - content = `
${esc(provenance)}
${lines.map((_,i)=>start===null?'':start+i).join('\n')}
${esc(segment.operation)}
${esc(segment.content)}
`; - } - const choices = ['Unplanned','Ambiguous'].includes(segment.row) ? `

Assigning this change makes the selected item’s approval stale.

` : ''; - return `
${esc(segment.path)}${segment.scope === 'out-of-scope' ? '✕ Out of scope' : esc(segment.scope)}
${content}
${esc(segment.context || 'File-level change')}${segment.sharesHunkWith.length ? ` · Shares a hunk with ${esc(segment.sharesHunkWith.join(', '))}` : ''}
${choices}
`; - }).join('') || '

No changes in this row

There are no current segments here. An item with no changes requires explicit confirmation before approval.

'); - document.querySelectorAll('[data-assign]').forEach(button => button.addEventListener('click', () => { const index = Number(button.dataset.assign); const item = document.querySelector(`[data-target="${index}"]`).value; if (item) act({ action:'assign', key:segments[index].key, item }); })); - document.querySelectorAll('[data-accept]').forEach(button => button.addEventListener('click', () => act({ action:'accept', key:segments[Number(button.dataset.accept)].key }))); - document.querySelectorAll('[data-since]').forEach(button => button.setAttribute('aria-pressed', String((button.dataset.since === 'true') === since))); + $("code").innerHTML = + comparison + + (segments + .map((segment, index) => { + const provenance = + segment.row === "Accepted" ? "Accepted" : segment.row; + let content; + if (segment.kind === "file") { + const meta = JSON.parse(segment.content); + const label = + meta.oldPath !== meta.newPath && meta.oldPath && meta.newPath + ? "Renamed file" + : meta.oldMode === "160000" || meta.newMode === "160000" + ? "Submodule pointer" + : meta.oldMode === "120000" || meta.newMode === "120000" + ? "Symbolic link" + : meta.oldMode !== meta.newMode && + meta.oldMode && + meta.newMode + ? "File mode changed" + : "File change"; + content = `
${esc(provenance)}
▧ ${label}
Path
${esc(meta.oldPath || "Absent")} → ${esc(meta.newPath || "Absent")}
Mode
${esc(meta.oldMode || "Absent")} → ${esc(meta.newMode || "Absent")}

Size: ${segment.file?.oldSize ?? "N/A"} → ${segment.file?.newSize ?? "N/A"} bytes

${segment.file?.beforePreview || segment.file?.afterPreview ? `
${segment.file.beforePreview ? `
Before
Previous image in ${esc(segment.path)}
` : ""}${segment.file.afterPreview ? `
After
Current image in ${esc(segment.path)}
` : ""}
` : '

No preview available

'}
Details · content IDs
${esc(JSON.stringify(meta, null, 2))}
`; + } else { + const lines = segment.content.split("\n"); + if (lines.at(-1) === "") lines.pop(); + const start = + segment.operation === "+" ? segment.newLine : segment.oldLine; + content = `
${esc(provenance)}
${lines.map((_, i) => (start === null ? "" : start + i)).join("\n")}
${esc(segment.operation)}
${esc(segment.content)}
`; + } + const choices = ["Unplanned", "Ambiguous"].includes(segment.row) + ? `

Assigning this change makes the selected item’s approval stale.

` + : ""; + return `
${esc(segment.path)}${segment.scope === "out-of-scope" ? "✕ Out of scope" : esc(segment.scope)}
${content}
${esc(segment.context || "File-level change")}${segment.sharesHunkWith.length ? ` · Shares a hunk with ${esc(segment.sharesHunkWith.join(", "))}` : ""}
${choices}
`; + }) + .join("") || + (data.segments.length + ? '

No changes in this row

There are no current segments here. An item with no changes requires explicit confirmation before approval.

' + : '

No code changes yet

This branch has no changes against the selected base. Open task shows the compared commits.

')); + document.querySelectorAll("[data-assign]").forEach((button) => + button.addEventListener("click", () => { + const index = Number(button.dataset.assign); + const item = document.querySelector(`[data-target="${index}"]`).value; + if (item) act({ action: "assign", key: segments[index].key, item }); + }), + ); + document + .querySelectorAll("[data-accept]") + .forEach((button) => + button.addEventListener("click", () => + act({ + action: "accept", + key: segments[Number(button.dataset.accept)].key, + }), + ), + ); + document + .querySelectorAll("[data-since]") + .forEach((button) => + button.setAttribute( + "aria-pressed", + String((button.dataset.since === "true") === since), + ), + ); +} +function moveChange(delta) { + change += delta; + renderCode(); + document + .querySelector(`[data-change="${change}"]`) + ?.scrollIntoView({ block: "nearest" }); +} +function setMode(value) { + rememberDraft(); + mode = value; + $("ask").setAttribute("aria-pressed", String(mode === "question")); + $("request").setAttribute("aria-pressed", String(mode === "change")); + $("composer-label").textContent = + mode === "change" ? "Change to request" : "Question about this item"; + $("save-note").textContent = + mode === "change" ? "Save change request" : "Save question"; + $("message").value = drafts.get(`${selected}:${mode}`) || ""; } -function moveChange(delta) { change += delta; renderCode(); document.querySelector(`[data-change="${change}"]`)?.scrollIntoView({ block:'nearest' }); } -function setMode(value) { rememberDraft(); mode = value; $('ask').setAttribute('aria-pressed',String(mode==='question')); $('request').setAttribute('aria-pressed',String(mode==='change')); $('composer-label').textContent = mode==='change'?'Change to request':'Question about this item'; $('save-note').textContent = mode==='change'?'Save change request':'Save question'; $('message').value = drafts.get(`${selected}:${mode}`)||''; } -function showDialog(html) { $('dialog-body').innerHTML = html; $('dialog').showModal(); } -$('approve').onclick = () => { const item=data.items.find(item=>item.id===selected); if(item) act({ action:'approve',item:selected,confirmNoChange:item.count===0 }); }; -$('reload').onclick=refresh; $('review-link').onclick=event=>{event.preventDefault();refresh();}; -$('next').onclick=()=>moveChange(1); $('previous').onclick=()=>moveChange(-1); -$('ask').onclick=()=>setMode('question'); $('request').onclick=()=>setMode('change'); -$('composer').onsubmit=async event=>{event.preventDefault();const item=selected,kind=mode; if(await act({action:'note',item,kind,text:$('message').value})){drafts.delete(`${item}:${kind}`);$('message').value='';$('saved').textContent=kind==='change'?'Saved for the next revision.':'Question saved. No agent has been invoked.';}}; -$('conversation-toggle').onclick=()=>{document.body.classList.toggle('conversation-open');document.body.classList.remove('conversation-closed');}; -$('collapse-conversation').onclick=()=>{document.body.classList.remove('conversation-open');document.body.classList.add('conversation-closed');}; -$('collapse-plan').onclick=()=>{document.querySelector('.plan-pane').hidden=true;$('show-plan').hidden=false;};$('show-plan').onclick=()=>{document.querySelector('.plan-pane').hidden=false;$('show-plan').hidden=true;}; -$('help').onclick=()=>showDialog('

Keyboard shortcuts

j / k — next / previous change

n / p — next / previous item

a — approve item

r — request change

? — shortcuts

Esc — leave a text field or close this dialog

'); -$('close-dialog').onclick=()=>$('dialog').close(); -$('task').onclick=()=>showDialog(data?`

Task #${data.plan.issue}

${esc(data.plan.summary)}

Revision ${data.plan.revision} · ${data.demo?'Demo repository':'Local repository'}

Base: ${esc(data.snapshot.base)}\nHead: ${esc(data.snapshot.head)}

Source files are read-only in this review app. Approvals and discussion are saved locally.

`:'

No task loaded

Check the CLI configuration and retry.

'); -document.addEventListener('keydown',event=>{if(event.key==='Escape'&&['TEXTAREA','INPUT','SELECT'].includes(document.activeElement?.tagName)){document.activeElement.blur();return;}if(event.ctrlKey||event.metaKey||event.altKey||$('dialog').open||['TEXTAREA','INPUT','SELECT','BUTTON'].includes(document.activeElement?.tagName)||!data)return;if(!['j','k','n','p','a','r','?'].includes(event.key))return;event.preventDefault();if(event.key==='j')moveChange(1);if(event.key==='k')moveChange(-1);if(['n','p'].includes(event.key)){const ids=data.items.map(p=>p.id);select(ids[Math.max(0,Math.min(ids.length-1,ids.indexOf(selected)+(event.key==='n'?1:-1)))]);}if(event.key==='a')$('approve').click();if(event.key==='r'){document.body.classList.add('conversation-open');document.body.classList.remove('conversation-closed');setMode('change');$('message').focus();}if(event.key==='?')$('help').click();}); +function showDialog(html) { + $("dialog-body").innerHTML = html; + $("dialog").showModal(); +} +$("approve").onclick = () => { + const item = data?.items.find((item) => item.id === selected); + if (item?.count === 0 && item.ambiguousCount > 0) { + select("Ambiguous"); + return; + } + if (item) + act({ + action: "approve", + item: selected, + confirmNoChange: item.count === 0, + }); +}; +$("reload").onclick = refresh; +$("review-link").onclick = (event) => { + event.preventDefault(); + refresh(); +}; +$("next").onclick = () => moveChange(1); +$("previous").onclick = () => moveChange(-1); +$("ask").onclick = () => setMode("question"); +$("request").onclick = () => setMode("change"); +$("composer").onsubmit = async (event) => { + event.preventDefault(); + const item = selected, + kind = mode; + if (await act({ action: "note", item, kind, text: $("message").value })) { + drafts.delete(`${item}:${kind}`); + $("message").value = ""; + $("saved").textContent = + kind === "change" + ? "Saved for the next revision." + : "Question saved. No agent has been invoked."; + } +}; +$("conversation-toggle").onclick = () => { + document.body.classList.toggle("conversation-open"); + document.body.classList.remove("conversation-closed"); +}; +$("collapse-conversation").onclick = () => { + document.body.classList.remove("conversation-open"); + document.body.classList.add("conversation-closed"); +}; +$("collapse-plan").onclick = () => { + document.querySelector(".plan-pane").hidden = true; + $("show-plan").hidden = false; +}; +$("show-plan").onclick = () => { + document.querySelector(".plan-pane").hidden = false; + $("show-plan").hidden = true; +}; +$("help").onclick = () => + showDialog( + "

Keyboard shortcuts

j / k — next / previous change

n / p — next / previous item

a — approve item

r — request change

? — shortcuts

Esc — leave a text field or close this dialog

", + ); +$("close-dialog").onclick = () => $("dialog").close(); +$("task").onclick = () => + showDialog( + data + ? `

Task #${data.plan.issue}

${esc(data.plan.summary)}

Revision ${data.plan.revision} · ${data.demo ? "Demo repository" : "Local repository"}

Base: ${esc(data.snapshot.base)}\nHead: ${esc(data.snapshot.head)}

Source files are read-only in this review app. Approvals and discussion are saved locally.

` + : "

No task loaded

Check the CLI configuration and retry.

", + ); +document.addEventListener("keydown", (event) => { + if ( + event.key === "Escape" && + ["TEXTAREA", "INPUT", "SELECT"].includes(document.activeElement?.tagName) + ) { + document.activeElement.blur(); + return; + } + if ( + event.ctrlKey || + event.metaKey || + event.altKey || + $("dialog").open || + ["TEXTAREA", "INPUT", "SELECT", "BUTTON"].includes( + document.activeElement?.tagName, + ) || + !data + ) + return; + if (!["j", "k", "n", "p", "a", "r", "?"].includes(event.key)) return; + event.preventDefault(); + if (event.key === "j") moveChange(1); + if (event.key === "k") moveChange(-1); + if (["n", "p"].includes(event.key)) { + const ids = data.items.map((p) => p.id); + select( + ids[ + Math.max( + 0, + Math.min( + ids.length - 1, + ids.indexOf(selected) + (event.key === "n" ? 1 : -1), + ), + ) + ], + ); + } + if (event.key === "a") $("approve").click(); + if (event.key === "r") { + document.body.classList.add("conversation-open"); + document.body.classList.remove("conversation-closed"); + setMode("change"); + $("message").focus(); + } + if (event.key === "?") $("help").click(); +}); await refresh(); diff --git a/web/public/index.html b/web/public/index.html index 08c504d..b530d7e 100644 --- a/web/public/index.html +++ b/web/public/index.html @@ -1,11 +1,135 @@ -Review · codeboost -

A little more room to review

Widen this window to at least 1280px to see the code and its plan together.

-
codeboostLocal review
-
Review Loading…
- -
-
REVIEW

Linking changes to plan items…

-
-
j / k changen / p itema approver request changeReview only · source files stay unchanged
-
 + + + + + Review · codeboost + + + + +
+

A little more room to review

+

+ Widen this window to at least 1280px to see the code and its plan + together. +

+
+
+
+ codeboost + + Local review +
+
+
Review Loading…
+
+ +
+ +
+ +
+
+
+ REVIEW +

Linking changes to plan items…

+
+ +
+
+
+
+ +
+
+
+
+ +
+
+ j / k changen / p itema approver request changeReview only · source files stay unchanged +
+
+ +
+ + + + diff --git a/web/public/style.css b/web/public/style.css index 1482276..ce0a700 100644 --- a/web/public/style.css +++ b/web/public/style.css @@ -1,5 +1,668 @@ -@font-face{font-family:'IBM Plex Sans';src:url('/fonts/ibm-plex-sans-latin-400-normal.woff2') format('woff2');font-weight:400;font-display:swap}@font-face{font-family:'IBM Plex Sans';src:url('/fonts/ibm-plex-sans-latin-500-normal.woff2') format('woff2');font-weight:500;font-display:swap}@font-face{font-family:'IBM Plex Sans';src:url('/fonts/ibm-plex-sans-latin-600-normal.woff2') format('woff2');font-weight:600;font-display:swap}@font-face{font-family:'IBM Plex Mono';src:url('/fonts/ibm-plex-mono-latin-400-normal.woff2') format('woff2');font-weight:400;font-display:swap} -:root{--canvas:#101216;--surface:#171B21;--raised:#20262E;--selected:#213448;--line:#343D49;--outline:#758295;--text:#E8ECF1;--muted:#A5AFBD;--primary:#8ABFFF;--on-primary:#101216;--hover:#A6CEFF;--success:#8FDDA8;--warning:#E9BE6E;--error:#F2847E;--neutral:#9AA3B0;--added:#1B2C24;--removed:#33201F;color-scheme:dark} -*{box-sizing:border-box}body{margin:0;background:var(--canvas);color:var(--text);font:13px/1.385 'IBM Plex Sans','Segoe UI',sans-serif}button,input,textarea,select{font:inherit}button,select{min-height:28px;border:1px solid var(--line);border-radius:4px;background:var(--raised);color:var(--text);padding:4px 10px;cursor:pointer}button:hover,select:hover{background:var(--selected)}button:disabled{color:var(--muted);cursor:default;background:var(--surface)}:focus-visible{outline:2px solid var(--primary);outline-offset:2px}button.primary,#approve{background:var(--primary);color:var(--on-primary);border-color:var(--primary)}#approve:hover{background:var(--hover)}[hidden]{display:none!important}a{color:var(--primary)}h1{font-size:20px;line-height:1.4;margin:4px 0 0;font-weight:600}h2{font-size:16px}p{margin:8px 0}pre,code,.mono,.eyebrow{font-family:'IBM Plex Mono',Menlo,Consolas,monospace;font-size:12px;font-variant-numeric:tabular-nums}pre{white-space:pre-wrap;overflow-wrap:anywhere}.muted,.eyebrow{color:var(--muted)}.good{color:var(--success)}.warn{color:var(--warning)}.bad{color:var(--error)}.neutral{color:var(--neutral)} -#app{height:100vh;display:flex;flex-direction:column}.app-bar{height:44px;flex-shrink:0;border-bottom:1px solid var(--line);display:flex;align-items:center;padding:0 16px;gap:24px}.brand{display:flex;align-items:center;gap:8px;font-weight:600}.app-bar nav{display:flex;gap:20px;align-items:center;flex:1}.app-bar nav span{color:var(--muted)}.app-bar nav a{height:44px;display:flex;align-items:center;text-decoration:none;border-bottom:2px solid var(--primary)}#repository{font-size:12px;color:var(--muted)}.review-strip{height:48px;flex-shrink:0;display:flex;align-items:center;gap:16px;padding:0 16px;border-bottom:1px solid var(--line)}#issue{margin-left:16px}#progress{margin-left:auto}.workspace{display:flex;min-height:0;flex:1;overflow:hidden}.plan-pane{width:232px;min-width:200px;max-width:420px;resize:horizontal;overflow:auto;border-right:1px solid var(--line);background:var(--surface);flex-shrink:0}.pane-heading{height:40px;display:flex;align-items:center;justify-content:space-between;font-size:12px;color:var(--muted);padding:8px 12px;border-bottom:1px solid var(--line)}.pane-heading button{padding:0 8px}.plan-row{width:100%;text-align:left;border:0;border-radius:0;border-bottom:1px solid var(--line);padding:12px;background:transparent;min-height:64px}.plan-row.selected{background:var(--selected)}.row-title{display:flex;align-items:center;gap:8px;font-weight:500}.row-title .count{margin-left:auto;color:var(--muted)}.row-sub{display:flex;gap:12px;margin-top:8px;font-size:12px}.row-sub span{cursor:help}.selected-files{font-size:12px;color:var(--muted);padding:8px 12px;border-bottom:1px solid var(--line);overflow-wrap:anywhere}.selected-files code{display:block;margin-top:4px}#attention{padding:8px 12px;border-bottom:1px solid var(--line);display:flex;flex-wrap:wrap;gap:4px}#attention button{border:0;padding:2px 0;background:none;color:var(--error);font-size:12px}.code-pane{flex:1;min-width:0;display:flex;flex-direction:column;background:var(--canvas)}.code-heading{display:flex;gap:12px;align-items:center;padding:16px;border-bottom:1px solid var(--line)}.code-heading>div{flex:1;min-width:0}.code-heading h1{font-size:16px}.change-toolbar{height:44px;display:flex;align-items:center;gap:8px;padding:8px 16px;border-bottom:1px solid var(--line)}#view-toggle{margin-right:auto;display:flex;gap:4px}#view-toggle button[aria-pressed=true]{color:var(--primary);border-color:var(--primary)}#change-count{font-size:12px;color:var(--muted)}#item-details{padding:0 16px}#item-details p{margin:8px 0}#code{flex:1;overflow:auto;padding:16px}#checks{display:flex;gap:16px;flex-wrap:wrap;padding:12px 16px;border-top:1px solid var(--line);font-size:12px}#checks button{background:none;border:0;font-size:12px;padding:0;min-height:20px}.change{border:1px solid var(--line);margin-bottom:16px;scroll-margin:8px}.change.active{outline:1px solid var(--outline)}.file-heading{padding:8px 12px;background:var(--surface);border-bottom:1px solid var(--line);display:flex;flex-wrap:wrap;gap:8px}.file-heading code{overflow-wrap:anywhere}.change-meta{padding:8px 12px;border-top:1px solid var(--line);font-size:12px;color:var(--muted)}.diff{display:grid;grid-template-columns:56px 44px 24px minmax(0,1fr);font:12px/1.5 'IBM Plex Mono',Menlo,monospace}.provenance{grid-column:1;grid-row:1;align-self:stretch;padding:8px 4px;text-align:center;color:var(--muted);overflow-wrap:anywhere;border-right:1px solid var(--line);font-size:10px}.provenance.unplanned{background:repeating-linear-gradient(135deg,transparent,transparent 6px,#f2847e24 6px,#f2847e24 8px);color:var(--error)}.line-numbers{color:var(--muted);text-align:right;padding:8px}.sign{padding-top:8px}.diff pre{margin:0;padding:8px 12px 8px 0;white-space:pre;overflow:auto}.diff.added{background:var(--added)}.diff.removed{background:var(--removed)}.file-card{display:flex}.file-card .provenance{width:56px;flex-shrink:0}.file-values{padding:12px;overflow-wrap:anywhere;flex:1;min-width:0}.file-values dl{display:grid;grid-template-columns:72px 1fr;gap:8px;margin:12px 0}.file-values dd{margin:0}.choice-controls{padding:12px;border-top:1px solid var(--line);display:flex;flex-wrap:wrap;gap:8px;align-items:center}.choice-controls p{width:100%;margin:0 0 4px;color:var(--warning)}.conversation-pane{width:344px;min-width:280px;max-width:480px;flex-shrink:0;display:flex;flex-direction:column;background:var(--surface);border-left:1px solid var(--line);resize:horizontal;overflow:auto}.tabs{display:flex;border-bottom:1px solid var(--line)}.tabs button{flex:1;background:none;border:0;border-bottom:2px solid transparent;border-radius:0;height:40px}.tabs button[aria-pressed=true]{border-bottom-color:var(--primary);color:var(--primary)}.conversation-help{font-size:12px;padding:4px 12px}#notes{flex:1;overflow:auto;padding:12px;font-size:14px;line-height:1.5}.note{padding-bottom:16px;margin-bottom:16px;border-bottom:1px solid var(--line)}.note p{white-space:pre-wrap;overflow-wrap:anywhere}.note small{font-size:12px;color:var(--muted)}#composer{padding:12px;border-top:1px solid var(--line)}#composer label{font-size:12px;display:block;margin-bottom:8px}textarea{background:var(--canvas);color:var(--text);border:1px solid var(--outline);border-radius:4px;width:100%;padding:8px;resize:vertical;min-height:90px;margin-bottom:8px}footer{height:36px;flex-shrink:0;border-top:1px solid var(--line);display:flex;align-items:center;gap:24px;padding:0 16px;font-size:12px}footer>span:last-child{margin-left:auto}footer button{font-size:12px;min-height:24px}#banner:not(:empty){padding:12px 16px;background:var(--surface);border-bottom:1px solid var(--line);color:var(--warning)}.empty{padding:48px 24px;color:var(--muted);max-width:640px}.comparison{display:grid;grid-template-columns:1fr 1fr;gap:12px;margin-bottom:16px}.comparison section{min-width:0;background:var(--surface);padding:12px;border:1px solid var(--line)}details>summary{cursor:pointer;color:var(--primary)}dialog{background:var(--raised);color:var(--text);border:1px solid var(--line);border-radius:6px;width:640px;max-height:80vh;overflow:auto}dialog::backdrop{background:#101216aa}.narrow{display:none}#conversation-toggle{font-size:12px}#saved{display:block;font-size:12px;color:var(--success);margin-top:8px} -@media(min-width:1440px){#conversation-toggle{display:none}body.conversation-closed #conversation-toggle{display:block}body.conversation-closed .conversation-pane{display:none}}@media(min-width:1280px) and (max-width:1439px){.plan-pane{width:208px}.conversation-pane{display:none}body.conversation-open .conversation-pane{display:flex;width:300px}.app-bar nav{gap:12px}.app-bar{gap:16px}#repository{display:none}}@media(max-width:1279px){#app{display:none}.narrow{display:block;margin:15vh auto;padding:32px;max-width:600px}}@media(prefers-reduced-motion:no-preference){button{transition:background-color 80ms ease-out}.good{transition:color 150ms ease-out}} +@font-face { + font-family: "IBM Plex Sans"; + src: url("/fonts/ibm-plex-sans-latin-400-normal.woff2") format("woff2"); + font-weight: 400; + font-display: swap; +} +@font-face { + font-family: "IBM Plex Sans"; + src: url("/fonts/ibm-plex-sans-latin-500-normal.woff2") format("woff2"); + font-weight: 500; + font-display: swap; +} +@font-face { + font-family: "IBM Plex Sans"; + src: url("/fonts/ibm-plex-sans-latin-600-normal.woff2") format("woff2"); + font-weight: 600; + font-display: swap; +} +@font-face { + font-family: "IBM Plex Mono"; + src: url("/fonts/ibm-plex-mono-latin-400-normal.woff2") format("woff2"); + font-weight: 400; + font-display: swap; +} +:root { + --canvas: #101216; + --surface: #171b21; + --raised: #20262e; + --selected: #213448; + --line: #343d49; + --outline: #758295; + --text: #e8ecf1; + --muted: #a5afbd; + --primary: #8abfff; + --on-primary: #101216; + --hover: #a6ceff; + --success: #8fdda8; + --warning: #e9be6e; + --error: #f2847e; + --neutral: #9aa3b0; + --added: #1b2c24; + --removed: #33201f; + color-scheme: dark; +} +* { + box-sizing: border-box; +} +body { + margin: 0; + background: var(--canvas); + color: var(--text); + font: + 13px/1.385 "IBM Plex Sans", + "Segoe UI", + sans-serif; +} +button, +input, +textarea, +select { + font: inherit; +} +button, +select { + min-height: 28px; + border: 1px solid var(--line); + border-radius: 4px; + background: var(--raised); + color: var(--text); + padding: 4px 10px; + cursor: pointer; +} +button:hover, +select:hover { + background: var(--selected); +} +button:disabled { + color: var(--muted); + cursor: default; + background: var(--surface); +} +:focus-visible { + outline: 2px solid var(--primary); + outline-offset: 2px; +} +button.primary, +#approve { + background: var(--primary); + color: var(--on-primary); + border-color: var(--primary); +} +#approve:hover { + background: var(--hover); +} +[hidden] { + display: none !important; +} +a { + color: var(--primary); +} +h1 { + font-size: 20px; + line-height: 1.4; + margin: 4px 0 0; + font-weight: 600; +} +h2 { + font-size: 16px; +} +p { + margin: 8px 0; +} +pre, +code, +.mono, +.eyebrow { + font-family: "IBM Plex Mono", Menlo, Consolas, monospace; + font-size: 12px; + font-variant-numeric: tabular-nums; +} +pre { + white-space: pre-wrap; + overflow-wrap: anywhere; +} +.muted, +.eyebrow { + color: var(--muted); +} +.good { + color: var(--success); +} +.warn { + color: var(--warning); +} +.bad { + color: var(--error); +} +.neutral { + color: var(--neutral); +} +#app { + height: 100vh; + display: flex; + flex-direction: column; +} +.app-bar { + height: 44px; + flex-shrink: 0; + border-bottom: 1px solid var(--line); + display: flex; + align-items: center; + padding: 0 16px; + gap: 24px; +} +.brand { + display: flex; + align-items: center; + gap: 8px; + font-weight: 600; +} +.app-bar nav { + display: flex; + gap: 20px; + align-items: center; + flex: 1; +} +.app-bar nav span { + color: var(--muted); +} +.app-bar nav a { + height: 44px; + display: flex; + align-items: center; + text-decoration: none; + border-bottom: 2px solid var(--primary); +} +#repository { + font-size: 12px; + color: var(--muted); +} +.review-strip { + height: 48px; + flex-shrink: 0; + display: flex; + align-items: center; + gap: 16px; + padding: 0 16px; + border-bottom: 1px solid var(--line); +} +#issue { + margin-left: 16px; +} +#progress { + margin-left: auto; +} +.workspace { + display: flex; + min-height: 0; + flex: 1; + overflow: hidden; +} +.plan-pane { + width: 232px; + min-width: 200px; + max-width: 420px; + resize: horizontal; + overflow: auto; + border-right: 1px solid var(--line); + background: var(--surface); + flex-shrink: 0; +} +.pane-heading { + height: 40px; + display: flex; + align-items: center; + justify-content: space-between; + font-size: 12px; + color: var(--muted); + padding: 8px 12px; + border-bottom: 1px solid var(--line); +} +.pane-heading button { + padding: 0 8px; +} +.plan-row { + width: 100%; + text-align: left; + border: 0; + border-radius: 0; + border-bottom: 1px solid var(--line); + padding: 12px; + background: transparent; + min-height: 64px; +} +.plan-row.selected { + background: var(--selected); +} +.row-title { + display: flex; + align-items: center; + gap: 8px; + font-weight: 500; +} +.row-title .count { + margin-left: auto; + color: var(--muted); +} +.row-sub { + display: flex; + gap: 12px; + margin-top: 8px; + font-size: 12px; +} +.row-sub span { + cursor: help; +} +.selected-files { + font-size: 12px; + color: var(--muted); + padding: 8px 12px; + border-bottom: 1px solid var(--line); + overflow-wrap: anywhere; +} +.selected-files code { + display: block; + margin-top: 4px; +} +#attention { + padding: 8px 12px; + border-bottom: 1px solid var(--line); + display: flex; + flex-wrap: wrap; + gap: 4px; +} +#attention button { + border: 0; + padding: 2px 0; + background: none; + color: var(--error); + font-size: 12px; +} +.code-pane { + flex: 1; + min-width: 0; + display: flex; + flex-direction: column; + background: var(--canvas); +} +.code-heading { + display: flex; + gap: 12px; + align-items: center; + padding: 16px; + border-bottom: 1px solid var(--line); +} +.code-heading > div { + flex: 1; + min-width: 0; +} +.code-heading h1 { + font-size: 16px; +} +.change-toolbar { + height: 44px; + display: flex; + align-items: center; + gap: 8px; + padding: 8px 16px; + border-bottom: 1px solid var(--line); +} +#view-toggle { + margin-right: auto; + display: flex; + gap: 4px; +} +#view-toggle button[aria-pressed="true"] { + color: var(--primary); + border-color: var(--primary); +} +#change-count { + font-size: 12px; + color: var(--muted); +} +#item-details { + padding: 0 16px; +} +#item-details p { + margin: 8px 0; +} +#code { + flex: 1; + overflow: auto; + padding: 16px; +} +#checks { + display: flex; + gap: 16px; + flex-wrap: wrap; + padding: 12px 16px; + border-top: 1px solid var(--line); + font-size: 12px; +} +#checks button { + background: none; + border: 0; + font-size: 12px; + padding: 0; + min-height: 20px; +} +.change { + border: 1px solid var(--line); + margin-bottom: 16px; + scroll-margin: 8px; +} +.change.active { + outline: 1px solid var(--outline); +} +.file-heading { + padding: 8px 12px; + background: var(--surface); + border-bottom: 1px solid var(--line); + display: flex; + flex-wrap: wrap; + gap: 8px; +} +.file-heading code { + overflow-wrap: anywhere; +} +.change-meta { + padding: 8px 12px; + border-top: 1px solid var(--line); + font-size: 12px; + color: var(--muted); +} +.diff { + display: grid; + grid-template-columns: 56px 44px 24px minmax(0, 1fr); + font: + 12px/1.5 "IBM Plex Mono", + Menlo, + monospace; +} +.provenance { + grid-column: 1; + grid-row: 1; + align-self: stretch; + padding: 8px 4px; + text-align: center; + color: var(--muted); + overflow-wrap: anywhere; + border-right: 1px solid var(--line); + font-size: 10px; +} +.provenance.unplanned { + background: repeating-linear-gradient( + 135deg, + transparent, + transparent 6px, + #f2847e24 6px, + #f2847e24 8px + ); + color: var(--error); +} +.line-numbers { + color: var(--muted); + text-align: right; + padding: 8px; +} +.sign { + padding-top: 8px; +} +.diff pre { + margin: 0; + padding: 8px 12px 8px 0; + white-space: pre; + overflow: auto; +} +.diff.added { + background: var(--added); +} +.diff.removed { + background: var(--removed); +} +.file-card { + display: flex; +} +.file-card .provenance { + width: 56px; + flex-shrink: 0; +} +.file-values { + padding: 12px; + overflow-wrap: anywhere; + flex: 1; + min-width: 0; +} +.file-values dl { + display: grid; + grid-template-columns: 72px 1fr; + gap: 8px; + margin: 12px 0; +} +.file-values dd { + margin: 0; +} +.choice-controls { + padding: 12px; + border-top: 1px solid var(--line); + display: flex; + flex-wrap: wrap; + gap: 8px; + align-items: center; +} +.choice-controls p { + width: 100%; + margin: 0 0 4px; + color: var(--warning); +} +.conversation-pane { + width: 344px; + min-width: 280px; + max-width: 480px; + flex-shrink: 0; + display: flex; + flex-direction: column; + background: var(--surface); + border-left: 1px solid var(--line); + resize: horizontal; + overflow: auto; +} +.tabs { + display: flex; + border-bottom: 1px solid var(--line); +} +.tabs button { + flex: 1; + background: none; + border: 0; + border-bottom: 2px solid transparent; + border-radius: 0; + height: 40px; +} +.tabs button[aria-pressed="true"] { + border-bottom-color: var(--primary); + color: var(--primary); +} +.conversation-help { + font-size: 12px; + padding: 4px 12px; +} +#notes { + flex: 1; + overflow: auto; + padding: 12px; + font-size: 14px; + line-height: 1.5; +} +.note { + padding-bottom: 16px; + margin-bottom: 16px; + border-bottom: 1px solid var(--line); +} +.note p { + white-space: pre-wrap; + overflow-wrap: anywhere; +} +.note small { + font-size: 12px; + color: var(--muted); +} +#composer { + padding: 12px; + border-top: 1px solid var(--line); +} +#composer label { + font-size: 12px; + display: block; + margin-bottom: 8px; +} +textarea { + background: var(--canvas); + color: var(--text); + border: 1px solid var(--outline); + border-radius: 4px; + width: 100%; + padding: 8px; + resize: vertical; + min-height: 90px; + margin-bottom: 8px; +} +footer { + height: 36px; + flex-shrink: 0; + border-top: 1px solid var(--line); + display: flex; + align-items: center; + gap: 24px; + padding: 0 16px; + font-size: 12px; +} +footer > span:last-child { + margin-left: auto; +} +footer button { + font-size: 12px; + min-height: 24px; +} +#banner:not(:empty) { + padding: 12px 16px; + background: var(--surface); + border-bottom: 1px solid var(--line); + color: var(--warning); +} +.empty { + padding: 48px 24px; + color: var(--muted); + max-width: 640px; +} +.comparison { + display: grid; + grid-template-columns: 1fr 1fr; + gap: 12px; + margin-bottom: 16px; +} +.comparison section { + min-width: 0; + background: var(--surface); + padding: 12px; + border: 1px solid var(--line); +} +details > summary { + cursor: pointer; + color: var(--primary); +} +dialog { + background: var(--raised); + color: var(--text); + border: 1px solid var(--line); + border-radius: 6px; + width: 640px; + max-height: 80vh; + overflow: auto; +} +dialog::backdrop { + background: #101216aa; +} +.narrow { + display: none; +} +#conversation-toggle { + font-size: 12px; +} +#saved { + display: block; + font-size: 12px; + color: var(--success); + margin-top: 8px; +} +@media (min-width: 1440px) { + #conversation-toggle { + display: none; + } + body.conversation-closed #conversation-toggle { + display: block; + } + body.conversation-closed .conversation-pane { + display: none; + } +} +@media (min-width: 1280px) and (max-width: 1439px) { + .plan-pane { + width: 208px; + } + .conversation-pane { + display: none; + } + body.conversation-open .conversation-pane { + display: flex; + width: 300px; + } + .app-bar nav { + gap: 12px; + } + .app-bar { + gap: 16px; + } + #repository { + display: none; + } +} +@media (max-width: 1279px) { + #app { + display: none; + } + .narrow { + display: block; + margin: 15vh auto; + padding: 32px; + max-width: 600px; + } +} +@media (prefers-reduced-motion: no-preference) { + button { + transition: background-color 80ms ease-out; + } + .good { + transition: color 150ms ease-out; + } +} + +.image-previews { + display: flex; + gap: 12px; + flex-wrap: wrap; +} +.image-previews figure { + margin: 8px 0; + max-width: 100%; +} +.image-previews img { + max-width: 100%; + max-height: 240px; + object-fit: contain; +} +.image-previews figcaption { + font-size: 12px; + color: var(--muted); +} diff --git a/web/server.ts b/web/server.ts index 845de61..d086f54 100644 --- a/web/server.ts +++ b/web/server.ts @@ -17,7 +17,7 @@ export async function startServer(config: ReviewConfig, port = 4318) { const path = new URL(req.url ?? '/', origin).pathname; if (path.startsWith('/api/')) { const supplied = req.headers['x-codeboost-token']; - if (typeof supplied !== 'string' || supplied.length !== token.length || !timingSafeEqual(Buffer.from(supplied), Buffer.from(token))) { json(403, { error: 'Open the private local URL printed by the CLI.' }); return; } + if (typeof supplied !== 'string' || !/^[a-f0-9]{64}$/.test(supplied) || !timingSafeEqual(Buffer.from(supplied), Buffer.from(token))) { json(403, { error: 'Open the private local URL printed by the CLI.' }); return; } if (req.method === 'GET' && path === '/api/review') { json(200, service.load()); return; } if (req.method !== 'POST' || path !== '/api/action' || req.headers['content-type'] !== 'application/json') { json(405, { error: 'Unsupported request.' }); return; } const chunks: Buffer[] = []; let size = 0; From 227ac8d2e20a0f7f9efda7ca77296b5a5beb704d Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 23 Sep 2026 09:08:47 -0700 Subject: [PATCH 3/8] Require attribution resolution before item approval --- docs/implementation/read-only-review.md | 4 ++++ runner/review.ts | 4 ++-- test/browser/review.spec.ts | 5 +++++ test/review.test.ts | 2 ++ web/public/app.js | 10 +++++----- 5 files changed, 18 insertions(+), 7 deletions(-) diff --git a/docs/implementation/read-only-review.md b/docs/implementation/read-only-review.md index 8f5d9f2..504d85d 100644 --- a/docs/implementation/read-only-review.md +++ b/docs/implementation/read-only-review.md @@ -34,3 +34,7 @@ A large-change regression reproduced HTTP 413 when the browser sent the full con ## Review round 1 Reproduced and fixed no-change approval with item-owned ambiguous segments; the runner refuses it and the UI directs the user to resolve attribution first. Previously approved no-change items become stale if ambiguous work appears. Reproduced malformed non-ASCII credentials returning a generic conflict instead of unauthorized; credentials now require the expected ASCII hex shape before constant-time comparison. Reproduced stale item controls surviving a failed refresh; errors now discard the loaded view and require refresh. Added acceptance persistence and whole-plan empty-state browser coverage. No findings declined. + +## Review round 2 + +Extended the attribution guard to mixed items with both owned and ambiguous segments. The regression reproduced ordinary approval succeeding with unresolved attribution. Approval now rejects any item-specific ambiguity, previously approved affected items become stale, and the UI routes every such item to attribution resolution. Added browser coverage for this mixed case. No findings declined. diff --git a/runner/review.ts b/runner/review.ts index 84e1fb6..38ce52b 100644 --- a/runner/review.ts +++ b/runner/review.ts @@ -49,7 +49,7 @@ export class ReviewService { }); const states = approvalStates(plan, segments, saved.approvals, identity); for (const item of plan.items) { - if (states[item.id] === 'approved' && ((!segments.some(segment => segment.row === item.id) && segments.some(segment => segment.row === 'Ambiguous' && segment.owners.includes(item.id))) || item.depends_on.some(id => states[id] === 'stale'))) states[item.id] = 'stale'; + if (states[item.id] === 'approved' && (segments.some(segment => segment.row === 'Ambiguous' && segment.owners.includes(item.id)) || item.depends_on.some(id => states[id] === 'stale'))) states[item.id] = 'stale'; } const expected: ReviewState = { revision: plan.revision, snapshotId: snapshot.id, reviewVersion }; const notes = this.store.getReviewNotes(identity); @@ -82,7 +82,7 @@ export class ReviewService { const { identity } = this.config; if (command.action === 'approve' && typeof command.item === 'string') { const item = view.items.find(item => item.id === command.item); - if (item && item.count === 0 && item.ambiguousCount > 0) throw new Error('Resolve this item’s ambiguous changes before confirming no change is needed.'); + if (item && item.ambiguousCount > 0) throw new Error('Resolve this item’s ambiguous changes before approval.'); const approval = approveItem(view.plan, view.segments, command.item, identity, command.confirmNoChange === true); this.store.saveReview(identity, view.expected, [approval], []); } else if ((command.action === 'assign' || command.action === 'accept') && typeof command.key === 'string') { diff --git a/test/browser/review.spec.ts b/test/browser/review.spec.ts index 5887a3d..e136db8 100644 --- a/test/browser/review.spec.ts +++ b/test/browser/review.spec.ts @@ -69,3 +69,8 @@ test('shows the whole-plan empty state without claiming checks passed',async({pa const {repository,identity}=app.service.config;const base=app.service.store.getSnapshot(identity).base;execFileSync('git',['reset','--hard',base],{cwd:repository,stdio:'pipe'}); await page.goto(app.url);await expect(page.getByRole('heading',{name:'No code changes yet'})).toBeVisible();await expect(page.getByRole('button',{name:'Confirm no change needed',exact:true})).toBeVisible();await expect(page.getByRole('button',{name:'AI review: – Not run',exact:true})).toBeVisible(); }); +test('routes mixed owned and ambiguous items to attribution resolution',async({page})=>{ + const {repository,identity}=app.service.config;writeFileSync(join(repository,'retry.ts'),'export function delay(attempt: number) {\n return Math.min(10000, 200 * 2 ** attempt);\n}\n');execFileSync('git',['-c','core.hooksPath=/dev/null','commit','-am','P2 changes retry'],{cwd:repository,stdio:'pipe'}); + const head=execFileSync('git',['rev-parse','HEAD'],{cwd:repository,encoding:'utf8'}).trim(),snapshot=app.service.store.getSnapshot(identity);app.service.store.recordHistory(identity,{revision:1,snapshotId:snapshot.id},snapshot.base,head,[{sha:head,owner:'P2',origin:'owned',sourceSha:null}]); + await page.goto(app.url);await page.getByRole('button',{name:/P2 Document retry behavior/}).click();await page.getByRole('button',{name:'Resolve ambiguous changes',exact:true}).click();await expect(page.getByRole('heading',{name:'Ambiguous',exact:true})).toBeVisible(); +}); diff --git a/test/review.test.ts b/test/review.test.ts index ce90a0f..d882be9 100644 --- a/test/review.test.ts +++ b/test/review.test.ts @@ -39,4 +39,6 @@ it('refuses no-change confirmation while the item still owns ambiguous changes', service.store.recordHistory(config.identity,{revision:1,snapshotId:snapshot.id},snapshot.base,head,[{sha:head,owner:'P2',origin:'owned',sourceSha:null}]); const view=service.load();expect(view.items[0]!.count).toBe(0);expect(view.segments.some(s=>s.row==='Ambiguous'&&s.owners.includes('P1'))).toBe(true); expect(()=>service.act({action:'approve',item:'P1',confirmNoChange:true,token:view.token})).toThrow(/ambiguous/i); + expect(view.items[1]!.count).toBeGreaterThan(0); + expect(()=>service.act({action:'approve',item:'P2',token:view.token})).toThrow(/ambiguous/i); }); diff --git a/web/public/app.js b/web/public/app.js index ba183cf..30517c9 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -159,10 +159,10 @@ function render() { ? `

${esc(item.intent)}

${item.reasons.map((reason) => `

! Stale: ${esc(reason)}

`).join("")}` : ""; $("approve").hidden = !item; - $("approve").textContent = item?.count - ? `Approve ${item.id}` - : item?.ambiguousCount - ? "Resolve ambiguous changes" + $("approve").textContent = item?.ambiguousCount + ? "Resolve ambiguous changes" + : item?.count + ? `Approve ${item.id}` : "Confirm no change needed"; $("approve").disabled = item?.state === "approved"; $("view-toggle").innerHTML = @@ -324,7 +324,7 @@ function showDialog(html) { } $("approve").onclick = () => { const item = data?.items.find((item) => item.id === selected); - if (item?.count === 0 && item.ambiguousCount > 0) { + if (item?.ambiguousCount > 0) { select("Ambiguous"); return; } From 7a5a9e0e552ebd78065995c95b1e12735f61d7b4 Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 23 Sep 2026 09:18:20 -0700 Subject: [PATCH 4/8] Confine demo fixtures and isolate helper Git environments --- docs/implementation/read-only-review.md | 4 +++ scripts/demo.ts | 19 ++++++++-- scripts/git-environment.ts | 9 +++++ scripts/plant.ts | 3 +- test/browser/review.spec.ts | 3 ++ test/demo.test.ts | 46 +++++++++++++++++++++++++ web/public/app.js | 2 +- 7 files changed, 81 insertions(+), 5 deletions(-) create mode 100644 scripts/git-environment.ts create mode 100644 test/demo.test.ts diff --git a/docs/implementation/read-only-review.md b/docs/implementation/read-only-review.md index 504d85d..4232bbd 100644 --- a/docs/implementation/read-only-review.md +++ b/docs/implementation/read-only-review.md @@ -38,3 +38,7 @@ Reproduced and fixed no-change approval with item-owned ambiguous segments; the ## Review round 2 Extended the attribution guard to mixed items with both owned and ambiguous segments. The regression reproduced ordinary approval succeeding with unresolved attribution. Approval now rejects any item-specific ambiguity, previously approved affected items become stale, and the UI routes every such item to attribution resolution. Added browser coverage for this mixed case. No findings declined. + +A toolbar-focus browser regression reproduced review shortcuts being disabled while a button had focus. Only text-entry/select controls now suppress the single-letter shortcuts; native Enter/Space button behavior is unchanged. The plan-row click case already worked because rendering replaces the focused row. + +Review round 3 identified demo fixture config escapes and inherited Git environment redirection. Regression checks first reproduced the config, symlink, and environment failures. Existing demos now require exact fixture paths and ordinary config/database/repository objects (including `.git` and SQLite sidecars). Demo and plant Git commands share a case-insensitive environment scrub and disable inherited global/system Git configuration. Four helper tests cover these boundaries, including mixed-case variable names for Windows. diff --git a/scripts/demo.ts b/scripts/demo.ts index 59b2560..9ed1ac8 100644 --- a/scripts/demo.ts +++ b/scripts/demo.ts @@ -1,17 +1,30 @@ -import { existsSync, mkdirSync, mkdtempSync, rmSync, readFileSync, writeFileSync, chmodSync } from 'node:fs'; +import { existsSync, mkdirSync, mkdtempSync, rmSync, readFileSync, writeFileSync, chmodSync, lstatSync } from 'node:fs'; import { resolve, join } from 'node:path'; import { execFileSync } from 'node:child_process'; import { randomUUID } from 'node:crypto'; import { Store } from '../runner/store.ts'; import type { ReviewConfig } from '../runner/review.ts'; import type { Plan } from '../core/plan.ts'; +import { isolatedGitEnvironment } from './git-environment.ts'; /** Disposable fixture only. Never runs against the user's working repository. */ export function createDemo(directory: string): ReviewConfig { const root = resolve(directory), configPath = join(root, 'review.json'); - if (existsSync(configPath)) return JSON.parse(readFileSync(configPath, 'utf8')) as ReviewConfig; + if (existsSync(root) && lstatSync(root).isSymbolicLink()) throw new Error('Demo fixture root must not be a symlink.'); + if (existsSync(configPath)) { + const check = (path: string, directory: boolean) => { + const stat = lstatSync(path); + if (stat.isSymbolicLink() || (directory ? !stat.isDirectory() : !stat.isFile())) throw new Error('Demo fixture paths must be ordinary local files and directories.'); + }; + check(configPath, false); + const config = JSON.parse(readFileSync(configPath, 'utf8')) as ReviewConfig; + if (config.demo !== true || config.repository !== join(root, 'retry-service') || config.database !== join(root, 'review.sqlite')) throw new Error('Demo configuration must refer to its own fixture.'); + check(config.repository, true); check(join(config.repository, '.git'), true); check(config.database, false); + for (const suffix of ['-wal', '-shm']) if (existsSync(config.database + suffix)) check(config.database + suffix, false); + return config; + } if (existsSync(root)) throw new Error('Demo directory exists without a configuration. Choose a new empty path.'); mkdirSync(root, { recursive: true }); const repository = join(root, 'retry-service'); mkdirSync(repository); - const git = (...args: string[]) => execFileSync('git', ['-c','core.hooksPath=/dev/null',...args], { cwd: repository, encoding: 'utf8', stdio: ['ignore','pipe','pipe'] }).trim(); + const git = (...args: string[]) => execFileSync('git', ['-c','core.hooksPath=/dev/null',...args], { cwd: repository, env: isolatedGitEnvironment(), encoding: 'utf8', stdio: ['ignore','pipe','pipe'] }).trim(); git('init','-b','main'); git('config','user.name','Codeboost Demo'); git('config','user.email','demo@example.invalid'); git('config','commit.gpgsign','false'); const write = (path: string, text: string | Buffer) => writeFileSync(join(repository,path),text); const commit = (message: string) => { git('add','-A');git('commit','-m',message);return git('rev-parse','HEAD'); }; diff --git a/scripts/git-environment.ts b/scripts/git-environment.ts new file mode 100644 index 0000000..6013e8b --- /dev/null +++ b/scripts/git-environment.ts @@ -0,0 +1,9 @@ +/** Git environment names are case-insensitive on Windows. */ +export function isolatedGitEnvironment(): NodeJS.ProcessEnv { + return { + ...Object.fromEntries(Object.entries(process.env).filter(([key]) => !/^GIT_/i.test(key))), + GIT_CONFIG_NOSYSTEM: '1', + GIT_CONFIG_GLOBAL: '/dev/null', + GIT_TERMINAL_PROMPT: '0', + }; +} diff --git a/scripts/plant.ts b/scripts/plant.ts index cada7ec..b83b354 100644 --- a/scripts/plant.ts +++ b/scripts/plant.ts @@ -1,3 +1,4 @@ +import { isolatedGitEnvironment } from './git-environment.ts'; import { execFileSync } from 'node:child_process'; import { existsSync, mkdirSync, writeFileSync, appendFileSync, lstatSync, readFileSync } from 'node:fs'; import { resolve, join, dirname } from 'node:path'; @@ -29,7 +30,7 @@ export function plant(config: ReviewConfig, destination: string, input: PlantInp const declared=candidates[randomInt(candidates.length)]!; const owned=history.commits.map((commit,index)=>({index,owner:owners.get(commit.sha)})).filter(entry=>entry.owner&&plan!.items.some(item=>item.id===entry.owner)); const outside=owned[randomInt(owned.length)]!; - const env={...Object.fromEntries(Object.entries(process.env).filter(([key])=>!key.startsWith('GIT_'))),GIT_CONFIG_NOSYSTEM:'1',GIT_CONFIG_GLOBAL:'/dev/null'}; + const env=isolatedGitEnvironment(); const gitRaw=(cwd:string,...args:string[])=>execFileSync('git',['-c','core.hooksPath=/dev/null','-c','commit.gpgsign=false',...args],{cwd,env,encoding:'utf8',stdio:['ignore','pipe','pipe'],timeout:30000,maxBuffer:32*1024*1024}); const git=(cwd:string,...args:string[])=>gitRaw(cwd,...args).trim(); // Refuse existing paths, symlink targets, and unsupported declared-file transitions before creating output. diff --git a/test/browser/review.spec.ts b/test/browser/review.spec.ts index e136db8..0b64eb5 100644 --- a/test/browser/review.spec.ts +++ b/test/browser/review.spec.ts @@ -74,3 +74,6 @@ test('routes mixed owned and ambiguous items to attribution resolution',async({p const head=execFileSync('git',['rev-parse','HEAD'],{cwd:repository,encoding:'utf8'}).trim(),snapshot=app.service.store.getSnapshot(identity);app.service.store.recordHistory(identity,{revision:1,snapshotId:snapshot.id},snapshot.base,head,[{sha:head,owner:'P2',origin:'owned',sourceSha:null}]); await page.goto(app.url);await page.getByRole('button',{name:/P2 Document retry behavior/}).click();await page.getByRole('button',{name:'Resolve ambiguous changes',exact:true}).click();await expect(page.getByRole('heading',{name:'Ambiguous',exact:true})).toBeVisible(); }); +test('keeps review shortcuts active while a toolbar button has focus',async({page})=>{ + await page.goto(app.url);await expect(page.getByRole('heading',{name:'Bound exponential retries'})).toBeVisible();await page.getByRole('button',{name:'Refresh',exact:true}).focus();await page.keyboard.press('n');await expect(page.getByRole('heading',{name:'Document retry behavior'})).toBeVisible(); +}); diff --git a/test/demo.test.ts b/test/demo.test.ts new file mode 100644 index 0000000..2dc7e26 --- /dev/null +++ b/test/demo.test.ts @@ -0,0 +1,46 @@ +import { afterEach, expect, it, vi } from 'vitest'; +import { mkdtempSync, renameSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { isolatedGitEnvironment } from '../scripts/git-environment.ts'; +import { createDemo } from '../scripts/demo.ts'; +const roots: string[] = []; +afterEach(() => { vi.unstubAllEnvs(); roots.splice(0).forEach(root => rmSync(root, { recursive: true, force: true })); }); +function root() { const path = mkdtempSync(join(tmpdir(), 'codeboost-demo-')); roots.push(path); return path; } +it('rejects existing demo configs redirected outside their fixture', () => { + const base = root(), directory = join(base, 'demo'), config = createDemo(directory); + expect(createDemo(directory)).toEqual(config); + for (const key of ['repository', 'database'] as const) { + writeFileSync(join(directory, 'review.json'), JSON.stringify({ ...config, [key]: join(base, 'outside') })); + expect(() => createDemo(directory)).toThrow(/fixture/i); + } +}); +it('rejects symlinked demo roots, configs, repositories and databases', () => { + const base = root(), directory = join(base, 'demo'); createDemo(directory); + symlinkSync(directory, join(base, 'alias')); + expect(() => createDemo(join(base, 'alias'))).toThrow(/fixture/i); + for (const name of ['review.json', 'retry-service', 'review.sqlite']) { + const path = join(directory, name), backup = join(base, name); + // Move each object out of the fixture and replace it with a symlink. + renameSync(path, backup); symlinkSync(backup, path); + expect(() => createDemo(directory)).toThrow(/fixture/i); + rmSync(path); renameSync(backup, path); + } +}); +it('scrubs inherited Git repository and config environment in demo commands', () => { + const base = root(); + vi.stubEnv('GIT_DIR', join(base, 'outside.git')); + vi.stubEnv('GIT_WORK_TREE', join(base, 'outside')); + vi.stubEnv('GIT_CONFIG_COUNT', '1'); + vi.stubEnv('GIT_CONFIG_KEY_0', 'init.defaultBranch'); + vi.stubEnv('GIT_CONFIG_VALUE_0', 'inherited'); + const config = createDemo(join(base, 'demo')); + expect(readFileSync(join(config.repository, 'retry.ts'), 'utf8')).toContain('Math.min'); +}); + +it('scrubs Git environment names case-insensitively for Windows', () => { + vi.stubEnv('git_dir', '/outside'); vi.stubEnv('Git_Work_Tree', '/outside'); + const env = isolatedGitEnvironment(); + expect(env).not.toHaveProperty('git_dir'); expect(env).not.toHaveProperty('Git_Work_Tree'); + expect(env.GIT_CONFIG_NOSYSTEM).toBe('1'); +}); diff --git a/web/public/app.js b/web/public/app.js index 30517c9..61c4a32 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -397,7 +397,7 @@ document.addEventListener("keydown", (event) => { event.metaKey || event.altKey || $("dialog").open || - ["TEXTAREA", "INPUT", "SELECT", "BUTTON"].includes( + ["TEXTAREA", "INPUT", "SELECT"].includes( document.activeElement?.tagName, ) || !data From 6f9aee67e1de2df89e25aaaaf4f659cb435ae7bf Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 23 Sep 2026 09:25:53 -0700 Subject: [PATCH 5/8] Align planting path guards and accepted scope labels --- docs/implementation/read-only-review.md | 2 ++ scripts/plant.ts | 8 +++++++- test/browser/review.spec.ts | 2 +- test/plant.test.ts | 5 +++++ web/public/app.js | 2 +- 5 files changed, 16 insertions(+), 3 deletions(-) diff --git a/docs/implementation/read-only-review.md b/docs/implementation/read-only-review.md index 4232bbd..d12c52c 100644 --- a/docs/implementation/read-only-review.md +++ b/docs/implementation/read-only-review.md @@ -42,3 +42,5 @@ Extended the attribution guard to mixed items with both owned and ambiguous segm A toolbar-focus browser regression reproduced review shortcuts being disabled while a button had focus. Only text-entry/select controls now suppress the single-letter shortcuts; native Enter/Space button behavior is unchanged. The plan-row click case already worked because rendering replaces the focused row. Review round 3 identified demo fixture config escapes and inherited Git environment redirection. Regression checks first reproduced the config, symlink, and environment failures. Existing demos now require exact fixture paths and ordinary config/database/repository objects (including `.git` and SQLite sidecars). Demo and plant Git commands share a case-insensitive environment scrub and disable inherited global/system Git configuration. Four helper tests cover these boundaries, including mixed-case variable names for Windows. + +Review round 4 had no inline findings but raised two summary concerns. Both were reproduced: planting accepted a non-ASCII path under case-insensitive identity even though the viewer refuses it; accepted cards retained an unplanned scope label. Planting now applies the same path guard, and accepted cards explicitly say “Accepted outside plan.” The underlying provenance remains intact. Unit/browser regressions cover both fixes; no concerns were declined. diff --git a/scripts/plant.ts b/scripts/plant.ts index b83b354..b92d909 100644 --- a/scripts/plant.ts +++ b/scripts/plant.ts @@ -16,10 +16,17 @@ export function plant(config: ReviewConfig, destination: string, input: PlantInp const root=resolve(destination); if(existsSync(root))throw new Error('Experiment destination must not exist.'); if((!isRepoPath(input.undeclaredPath)||input.undeclaredPath.includes('/'))||!input.declaredText?.trim()||!input.undeclaredText?.trim()||input.declaredText.length>4000||input.undeclaredText.length>4000)throw new Error('Invalid plant input.'); + const pathKey=(path:string)=>{ + if(!config.pathIdentity.caseSensitive && /[^\x20-\x7e]/.test(path)) throw new Error('Non-ASCII case-insensitive paths require a filesystem-specific identity adapter.'); + const p=config.pathIdentity.unicodeNormalization==='NFC'?path.normalize('NFC'):path; + return config.pathIdentity.caseSensitive?p:p.toLowerCase(); + }; + pathKey(input.undeclaredPath); const source=new Store(config.database); let plan, snapshot, entries; try{plan=source.getPlan(config.identity);snapshot=source.getSnapshot(config.identity);entries=source.getLedger(config.identity);}finally{source.close();} if(plan.items.some(item=>item.files.some(file=>file.path===input.undeclaredPath||file.renamed_from===input.undeclaredPath)))throw new Error('Undeclared plant must be outside every declared file.'); + for(const item of plan.items) for(const file of item.files) { pathKey(file.path); if(file.renamed_from) pathKey(file.renamed_from); } const history=readHistory(config.repository,snapshot.base,snapshot.head); const owners=new Map(entries.map(entry=>[entry.sha,entry.owner])); const candidates=history.commits.flatMap((commit,index)=>{ @@ -64,7 +71,6 @@ export function plant(config: ReviewConfig, destination: string, input: PlantInp const output:ReviewConfig={...config,repository,database:join(root,'review.sqlite'),identity,demo:false}; const store=new Store(output.database); try{ - const pathKey=(path:string)=>{const p=config.pathIdentity.unicodeNormalization==='NFC'?path.normalize('NFC'):path;return config.pathIdentity.caseSensitive?p:p.toLowerCase();}; store.createPlan(JSON.stringify(plan),'json',{identity,issue:plan.issue,baseEntries,pathKey,allowedCommands:[]},snapshot.base,snapshot.head); const validEntries=entries.filter(entry=>entry.owner===null||plan.items.some(item=>item.id===entry.owner)); store.recordHistory(identity,{revision:1,snapshotId:store.getSnapshot(identity).id},snapshot.base,snapshot.head,validEntries); diff --git a/test/browser/review.spec.ts b/test/browser/review.spec.ts index 0b64eb5..9118065 100644 --- a/test/browser/review.spec.ts +++ b/test/browser/review.spec.ts @@ -63,7 +63,7 @@ test('keeps stale item controls unavailable after a failed refresh',async({page} await page.getByRole('button',{name:'Refresh',exact:true}).click();await expect(page.getByText('Could not read this branch’s history.',{exact:false})).toBeVisible();await expect(page.getByRole('button',{name:/P3 Confirm API compatibility/})).toHaveCount(0); }); test('accepts a foreign segment and keeps that choice across reload',async({page})=>{ - await page.goto(app.url);await page.getByRole('button',{name:/Unplanned changes/}).click();await page.getByRole('button',{name:'Accept as is',exact:true}).first().click();await page.getByRole('button',{name:/Accepted 1/}).click();await expect(page.getByRole('article').first()).toBeVisible();await page.reload();await page.getByRole('button',{name:/Accepted 1/}).click();await expect(page.getByRole('article').first()).toBeVisible(); + await page.goto(app.url);await page.getByRole('button',{name:/Unplanned changes/}).click();await page.getByRole('button',{name:'Accept as is',exact:true}).first().click();await page.getByRole('button',{name:/Accepted 1/}).click();await expect(page.getByRole('article').first()).toBeVisible();await expect(page.getByText('Accepted outside plan',{exact:true})).toBeVisible();await page.reload();await page.getByRole('button',{name:/Accepted 1/}).click();await expect(page.getByRole('article').first()).toBeVisible(); }); test('shows the whole-plan empty state without claiming checks passed',async({page})=>{ const {repository,identity}=app.service.config;const base=app.service.store.getSnapshot(identity).base;execFileSync('git',['reset','--hard',base],{cwd:repository,stdio:'pipe'}); diff --git a/test/plant.test.ts b/test/plant.test.ts index afe0aba..4b04c94 100644 --- a/test/plant.test.ts +++ b/test/plant.test.ts @@ -15,3 +15,8 @@ it('plants in an isolated clone, retains ledger attribution, and leaves source h try {const view=service.load();expect(view.segments.some(s=>s.path==='extra.txt'&&s.scope==='out-of-scope')).toBe(true);expect(view.segments.some(s=>s.content.includes('// planted extra behavior')&&s.row.startsWith('P'))).toBe(true);}finally{service.close();} expect(JSON.parse(readFileSync(join(root,'experiment','sealed.json'),'utf8')).mappings).toHaveLength(3); },15000); +it('refuses non-ASCII plants on case-insensitive filesystems pending an identity adapter', () => { + const root=mkdtempSync(join(tmpdir(),'codeboost-plant-path-'));roots.push(root); + const config=createDemo(join(root,'source'));config.pathIdentity.caseSensitive=false; + expect(()=>plant(config,join(root,'experiment'),{declaredText:'// extra',undeclaredText:'diagnostic',undeclaredPath:'café.txt'})).toThrow(/filesystem-specific identity adapter/); +},15000); diff --git a/web/public/app.js b/web/public/app.js index 61c4a32..92dfbf4 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -268,7 +268,7 @@ function renderCode() { const choices = ["Unplanned", "Ambiguous"].includes(segment.row) ? `

Assigning this change makes the selected item’s approval stale.

` : ""; - return `
${esc(segment.path)}${segment.scope === "out-of-scope" ? "✕ Out of scope" : esc(segment.scope)}
${content}
${esc(segment.context || "File-level change")}${segment.sharesHunkWith.length ? ` · Shares a hunk with ${esc(segment.sharesHunkWith.join(", "))}` : ""}
${choices}
`; + return `
${esc(segment.path)}${segment.row === "Accepted" ? "Accepted outside plan" : segment.scope === "out-of-scope" ? "✕ Out of scope" : esc(segment.scope)}
${content}
${esc(segment.context || "File-level change")}${segment.sharesHunkWith.length ? ` · Shares a hunk with ${esc(segment.sharesHunkWith.join(", "))}` : ""}
${choices}
`; }) .join("") || (data.segments.length From 1c9e053f9b4cd9652f882567aab67c3108ad1b49 Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 23 Sep 2026 09:33:47 -0700 Subject: [PATCH 6/8] Reject fixture ancestor escapes and canonical planting collisions --- docs/implementation/read-only-review.md | 2 ++ scripts/demo.ts | 17 ++++++++++++++--- scripts/plant.ts | 8 ++++---- test/demo.test.ts | 4 ++++ test/plant.test.ts | 5 +++++ 5 files changed, 29 insertions(+), 7 deletions(-) diff --git a/docs/implementation/read-only-review.md b/docs/implementation/read-only-review.md index d12c52c..567f204 100644 --- a/docs/implementation/read-only-review.md +++ b/docs/implementation/read-only-review.md @@ -44,3 +44,5 @@ A toolbar-focus browser regression reproduced review shortcuts being disabled wh Review round 3 identified demo fixture config escapes and inherited Git environment redirection. Regression checks first reproduced the config, symlink, and environment failures. Existing demos now require exact fixture paths and ordinary config/database/repository objects (including `.git` and SQLite sidecars). Demo and plant Git commands share a case-insensitive environment scrub and disable inherited global/system Git configuration. Four helper tests cover these boundaries, including mixed-case variable names for Windows. Review round 4 had no inline findings but raised two summary concerns. Both were reproduced: planting accepted a non-ASCII path under case-insensitive identity even though the viewer refuses it; accepted cards retained an unplanned scope label. Planting now applies the same path guard, and accepted cards explicitly say “Accepted outside plan.” The underlying provenance remains intact. Unit/browser regressions cover both fixes; no concerns were declined. + +Review round 5 found a symlinked-ancestor escape and raw-spelling collisions in planting guards. Both regression cases failed before the fix. Demo creation/reuse now checks every ancestor (canonicalizing only the OS temporary-directory prefix); planting compares canonical identities for declared files and every path in the base/commit trees before cloning. The summary's status-styling concern had no additional concrete example; accepted cards already use the explicit label added in round 4. diff --git a/scripts/demo.ts b/scripts/demo.ts index 9ed1ac8..bc11009 100644 --- a/scripts/demo.ts +++ b/scripts/demo.ts @@ -1,5 +1,6 @@ -import { existsSync, mkdirSync, mkdtempSync, rmSync, readFileSync, writeFileSync, chmodSync, lstatSync } from 'node:fs'; -import { resolve, join } from 'node:path'; +import { existsSync, mkdirSync, mkdtempSync, rmSync, readFileSync, writeFileSync, chmodSync, lstatSync, realpathSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { resolve, join, relative, isAbsolute, dirname } from 'node:path'; import { execFileSync } from 'node:child_process'; import { randomUUID } from 'node:crypto'; import { Store } from '../runner/store.ts'; @@ -8,7 +9,17 @@ import type { Plan } from '../core/plan.ts'; import { isolatedGitEnvironment } from './git-environment.ts'; /** Disposable fixture only. Never runs against the user's working repository. */ export function createDemo(directory: string): ReviewConfig { - const root = resolve(directory), configPath = join(root, 'review.json'); + const requested = resolve(directory), temporary = resolve(tmpdir()); + const tempRelative = relative(temporary, requested); + // The OS temp directory may itself use a system alias (e.g. /var on macOS). + // Canonicalize that trusted prefix only; reject user-created symlinks below it. + const root = !tempRelative.startsWith('..') && !isAbsolute(tempRelative) ? resolve(realpathSync(temporary), tempRelative) : requested; + for (let path = root; ; path = dirname(path)) { + const stat = lstatSync(path, { throwIfNoEntry: false }); + if (stat?.isSymbolicLink()) throw new Error('Demo fixture ancestors must not be symlinks.'); + if (dirname(path) === path) break; + } + const configPath = join(root, 'review.json'); if (existsSync(root) && lstatSync(root).isSymbolicLink()) throw new Error('Demo fixture root must not be a symlink.'); if (existsSync(configPath)) { const check = (path: string, directory: boolean) => { diff --git a/scripts/plant.ts b/scripts/plant.ts index b92d909..d350eff 100644 --- a/scripts/plant.ts +++ b/scripts/plant.ts @@ -25,7 +25,7 @@ export function plant(config: ReviewConfig, destination: string, input: PlantInp const source=new Store(config.database); let plan, snapshot, entries; try{plan=source.getPlan(config.identity);snapshot=source.getSnapshot(config.identity);entries=source.getLedger(config.identity);}finally{source.close();} - if(plan.items.some(item=>item.files.some(file=>file.path===input.undeclaredPath||file.renamed_from===input.undeclaredPath)))throw new Error('Undeclared plant must be outside every declared file.'); + if(plan.items.some(item=>item.files.some(file=>pathKey(file.path)===pathKey(input.undeclaredPath)||(file.renamed_from!==null&&pathKey(file.renamed_from)===pathKey(input.undeclaredPath)))))throw new Error('Undeclared plant must be outside every declared file.'); for(const item of plan.items) for(const file of item.files) { pathKey(file.path); if(file.renamed_from) pathKey(file.renamed_from); } const history=readHistory(config.repository,snapshot.base,snapshot.head); const owners=new Map(entries.map(entry=>[entry.sha,entry.owner])); @@ -41,9 +41,9 @@ export function plant(config: ReviewConfig, destination: string, input: PlantInp const gitRaw=(cwd:string,...args:string[])=>execFileSync('git',['-c','core.hooksPath=/dev/null','-c','commit.gpgsign=false',...args],{cwd,env,encoding:'utf8',stdio:['ignore','pipe','pipe'],timeout:30000,maxBuffer:32*1024*1024}); const git=(cwd:string,...args:string[])=>gitRaw(cwd,...args).trim(); // Refuse existing paths, symlink targets, and unsupported declared-file transitions before creating output. - for(const commit of history.commits){ - const tree=git(config.repository,'ls-tree','-r',commit.sha,'--',input.undeclaredPath); - if(tree)throw new Error('Undeclared plant path already exists in the source history.'); + for(const sha of [snapshot.base,...history.commits.map(commit=>commit.sha)]){ + const paths=gitRaw(config.repository,'ls-tree','-rz','--name-only',sha).split('\0').filter(Boolean); + if(paths.some(path=>pathKey(path)===pathKey(input.undeclaredPath)))throw new Error('Undeclared plant path already exists in the source history.'); } for(const commit of history.commits.slice(declared.index)){ if(!/^100(?:644|755) blob /.test(git(config.repository,'ls-tree',commit.sha,'--',declared.path)))throw new Error('Declared plant needs a regular file retained through the remaining history.'); diff --git a/test/demo.test.ts b/test/demo.test.ts index 2dc7e26..5cbb925 100644 --- a/test/demo.test.ts +++ b/test/demo.test.ts @@ -44,3 +44,7 @@ it('scrubs Git environment names case-insensitively for Windows', () => { expect(env).not.toHaveProperty('git_dir'); expect(env).not.toHaveProperty('Git_Work_Tree'); expect(env.GIT_CONFIG_NOSYSTEM).toBe('1'); }); +it('rejects symlinked ancestors before creating a demo', () => { + const base = root(); symlinkSync(base, join(base, 'alias')); + expect(() => createDemo(join(base, 'alias', 'nested', 'demo'))).toThrow(/fixture/i); +}); diff --git a/test/plant.test.ts b/test/plant.test.ts index 4b04c94..ba9e697 100644 --- a/test/plant.test.ts +++ b/test/plant.test.ts @@ -20,3 +20,8 @@ it('refuses non-ASCII plants on case-insensitive filesystems pending an identity const config=createDemo(join(root,'source'));config.pathIdentity.caseSensitive=false; expect(()=>plant(config,join(root,'experiment'),{declaredText:'// extra',undeclaredText:'diagnostic',undeclaredPath:'café.txt'})).toThrow(/filesystem-specific identity adapter/); },15000); +it('rejects canonical declared and existing path collisions before creating a clone', () => { + const root=mkdtempSync(join(tmpdir(),'codeboost-plant-collision-'));roots.push(root); + const config=createDemo(join(root,'source'));config.pathIdentity.caseSensitive=false; + for (const path of ['RETRY.TS','RUN.SH']) expect(()=>plant(config,join(root,'experiment'),{declaredText:'// extra',undeclaredText:'diagnostic',undeclaredPath:path})).toThrow(/outside every declared file|already exists/); +},15000); From 58cad78b978a5731a7fe61eaa97e8e3aa4b59d32 Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 23 Sep 2026 09:41:08 -0700 Subject: [PATCH 7/8] Keep experiment destinations outside the source checkout --- docs/implementation/read-only-review.md | 2 ++ scripts/plant.ts | 9 +++++++-- test/plant.test.ts | 19 +++++++++++++++++-- 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/docs/implementation/read-only-review.md b/docs/implementation/read-only-review.md index 567f204..5f5c62e 100644 --- a/docs/implementation/read-only-review.md +++ b/docs/implementation/read-only-review.md @@ -46,3 +46,5 @@ Review round 3 identified demo fixture config escapes and inherited Git environm Review round 4 had no inline findings but raised two summary concerns. Both were reproduced: planting accepted a non-ASCII path under case-insensitive identity even though the viewer refuses it; accepted cards retained an unplanned scope label. Planting now applies the same path guard, and accepted cards explicitly say “Accepted outside plan.” The underlying provenance remains intact. Unit/browser regressions cover both fixes; no concerns were declined. Review round 5 found a symlinked-ancestor escape and raw-spelling collisions in planting guards. Both regression cases failed before the fix. Demo creation/reuse now checks every ancestor (canonicalizing only the OS temporary-directory prefix); planting compares canonical identities for declared files and every path in the base/commit trees before cloning. The summary's status-styling concern had no additional concrete example; accepted cards already use the explicit label added in round 4. + +Review round 6 reproduced destination-inside-source mutation; planting now resolves the existing destination ancestor and rejects canonical source descendants before creating files, including `.git` and symlink aliases. The Git-environment finding was declined: `gitRaw` already passes `{cwd, env, ...}` to `execFileSync`. A new end-to-end test with inherited `GIT_DIR`/`GIT_WORK_TREE` passed before any production change. The summary's snapshot-race claim supplied no concrete interleaving; the service already checks revision/snapshot/review version before publishing a view and writes use atomic store CAS, covered by concurrent-write tests. diff --git a/scripts/plant.ts b/scripts/plant.ts index d350eff..85e2bc2 100644 --- a/scripts/plant.ts +++ b/scripts/plant.ts @@ -1,7 +1,7 @@ import { isolatedGitEnvironment } from './git-environment.ts'; import { execFileSync } from 'node:child_process'; -import { existsSync, mkdirSync, writeFileSync, appendFileSync, lstatSync, readFileSync } from 'node:fs'; -import { resolve, join, dirname } from 'node:path'; +import { existsSync, mkdirSync, writeFileSync, appendFileSync, lstatSync, readFileSync, realpathSync } from 'node:fs'; +import { resolve, join, dirname, basename, relative, isAbsolute } from 'node:path'; import { randomInt, randomUUID } from 'node:crypto'; import { pathToFileURL } from 'node:url'; import { Store } from '../runner/store.ts'; @@ -15,6 +15,11 @@ export interface PlantInput { declaredText: string; undeclaredText: string; unde export function plant(config: ReviewConfig, destination: string, input: PlantInput): string { const root=resolve(destination); if(existsSync(root))throw new Error('Experiment destination must not exist.'); + let parent=root; const missing:string[]=[]; + while(!lstatSync(parent,{throwIfNoEntry:false})) { missing.unshift(basename(parent)); parent=dirname(parent); } + const canonicalDestination=resolve(realpathSync(parent),...missing); + const sourceRelative=relative(realpathSync(config.repository),canonicalDestination); + if(sourceRelative===''||(!sourceRelative.startsWith('..'+(process.platform==='win32'?'\\':'/'))&&sourceRelative!=='..'&&!isAbsolute(sourceRelative))) throw new Error('Experiment destination must be outside the source repository.'); if((!isRepoPath(input.undeclaredPath)||input.undeclaredPath.includes('/'))||!input.declaredText?.trim()||!input.undeclaredText?.trim()||input.declaredText.length>4000||input.undeclaredText.length>4000)throw new Error('Invalid plant input.'); const pathKey=(path:string)=>{ if(!config.pathIdentity.caseSensitive && /[^\x20-\x7e]/.test(path)) throw new Error('Non-ASCII case-insensitive paths require a filesystem-specific identity adapter.'); diff --git a/test/plant.test.ts b/test/plant.test.ts index ba9e697..a24985d 100644 --- a/test/plant.test.ts +++ b/test/plant.test.ts @@ -1,5 +1,5 @@ -import { it,expect,afterEach } from 'vitest'; -import { mkdtempSync,readFileSync,rmSync } from 'node:fs'; +import { it,expect,afterEach,vi } from 'vitest'; +import { mkdtempSync,readFileSync,rmSync,symlinkSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { execFileSync } from 'node:child_process'; @@ -25,3 +25,18 @@ it('rejects canonical declared and existing path collisions before creating a cl const config=createDemo(join(root,'source'));config.pathIdentity.caseSensitive=false; for (const path of ['RETRY.TS','RUN.SH']) expect(()=>plant(config,join(root,'experiment'),{declaredText:'// extra',undeclaredText:'diagnostic',undeclaredPath:path})).toThrow(/outside every declared file|already exists/); },15000); + +it('rejects experiment destinations inside the source, including symlink aliases', () => { + const root=mkdtempSync(join(tmpdir(),'codeboost-plant-destination-'));roots.push(root); + const config=createDemo(join(root,'source'));symlinkSync(config.repository,join(root,'alias')); + for(const destination of [join(config.repository,'experiment'),join(config.repository,'.git','experiment'),join(root,'alias','nested','experiment')]) { + expect(()=>plant(config,destination,{declaredText:'// extra',undeclaredText:'diagnostic',undeclaredPath:'extra.txt'})).toThrow(/outside the source repository/); + } +},15000); +it('passes its isolated environment to every planting Git process', () => { + const root=mkdtempSync(join(tmpdir(),'codeboost-plant-env-'));roots.push(root); + const config=createDemo(join(root,'source')); + vi.stubEnv('GIT_DIR',join(root,'outside.git'));vi.stubEnv('GIT_WORK_TREE',join(root,'outside')); + try {expect(plant(config,join(root,'experiment'),{declaredText:'// extra',undeclaredText:'diagnostic',undeclaredPath:'extra.txt'})).toBe(join(root,'experiment','review.json'));} + finally {vi.unstubAllEnvs();} +},15000); From e58288218064e204d0db93c42d4364ac61a29557 Mon Sep 17 00:00:00 2001 From: mchwang Date: Wed, 23 Sep 2026 09:50:06 -0700 Subject: [PATCH 8/8] Bound preview dimensions and reject dangling SQLite sidecars --- docs/implementation/read-only-review.md | 2 ++ git/history.ts | 12 ++++++++++-- package-lock.json | 13 +++++++++++++ package.json | 1 + scripts/demo.ts | 2 +- test/demo.test.ts | 7 +++++++ test/history.test.ts | 7 +++++++ 7 files changed, 41 insertions(+), 3 deletions(-) diff --git a/docs/implementation/read-only-review.md b/docs/implementation/read-only-review.md index 5f5c62e..269faae 100644 --- a/docs/implementation/read-only-review.md +++ b/docs/implementation/read-only-review.md @@ -48,3 +48,5 @@ Review round 4 had no inline findings but raised two summary concerns. Both were Review round 5 found a symlinked-ancestor escape and raw-spelling collisions in planting guards. Both regression cases failed before the fix. Demo creation/reuse now checks every ancestor (canonicalizing only the OS temporary-directory prefix); planting compares canonical identities for declared files and every path in the base/commit trees before cloning. The summary's status-styling concern had no additional concrete example; accepted cards already use the explicit label added in round 4. Review round 6 reproduced destination-inside-source mutation; planting now resolves the existing destination ancestor and rejects canonical source descendants before creating files, including `.git` and symlink aliases. The Git-environment finding was declined: `gitRaw` already passes `{cwd, env, ...}` to `execFileSync`. A new end-to-end test with inherited `GIT_DIR`/`GIT_WORK_TREE` passed before any production change. The summary's snapshot-race claim supplied no concrete interleaving; the service already checks revision/snapshot/review version before publishing a view and writes use atomic store CAS, covered by concurrent-write tests. + +Review round 7 reproduced oversized-dimension raster previews and dangling SQLite sidecar symlinks. Preview metadata is now parsed from the bounded buffer with image-size: at most 8,192 pixels per side, 4 million pixels per image and 16 million pixels across unique preview blobs. Unknown dimensions omit the preview; existing compressed/response byte limits remain. Sidecars use lstat so dangling links are rejected. Both new regressions failed before the fixes. The summary mentioned rename-scope/literal-path concerns without specific findings; no additional behavior was inferred from that shorthand. diff --git a/git/history.ts b/git/history.ts index 892b0a8..ecc8dfe 100644 --- a/git/history.ts +++ b/git/history.ts @@ -1,3 +1,4 @@ +import { imageSize } from 'image-size'; import { execFileSync } from 'node:child_process'; import { lstatSync, opendirSync } from 'node:fs'; import { resolve as resolvePath, join } from 'node:path'; @@ -90,7 +91,7 @@ export function readHistory(repo: string, baseRef: string, headRef = 'HEAD', lim }); if (expectedParent !== head) throw new Error('The base must be an ancestor of the head.'); const blobs = new Map(); - let previewBytes = 0; + let previewBytes = 0, previewPixels = 0; const version = (oid: string, mode: string): FileVersion | null => { if (/^0+$/.test(oid)) return null; if (mode === '160000') return { oid, mode, text: null }; // gitlink is not a local blob @@ -108,7 +109,14 @@ export function readHistory(repo: string, baseRef: string, headRef = 'HEAD', lim /^GIF8[79]a$/.test(data.subarray(0,6).toString('ascii')) ? 'image/gif' : data.subarray(0,4).toString('ascii') === 'RIFF' && data.subarray(8,12).toString('ascii') === 'WEBP' ? 'image/webp' : null; if (mime && data.length <= 1024 * 1024 && previewBytes + data.length <= 4 * 1024 * 1024) { - preview = `data:${mime};base64,${data.toString('base64')}`; previewBytes += data.length; + try { + // Parse metadata from the already bounded buffer; never allocate decoded pixels. + const { width, height } = imageSize(data), pixels = width * height; + if (Number.isSafeInteger(pixels) && width > 0 && height > 0 && width <= 8192 && height <= 8192 && pixels <= 4_000_000 && previewPixels + pixels <= 16_000_000) { + preview = `data:${mime};base64,${data.toString('base64')}`; + previewBytes += data.length; previewPixels += pixels; + } + } catch { /* Unknown or malformed dimensions: metadata card only. */ } } blobs.set(oid, { text, byteSize: data.length, ...(preview ? { preview } : {}) }); } diff --git a/package-lock.json b/package-lock.json index dbdc3e5..fde54e9 100644 --- a/package-lock.json +++ b/package-lock.json @@ -12,6 +12,7 @@ "@fontsource/ibm-plex-sans": "5.3.0", "ajv": "8.20.0", "diff": "9.0.0", + "image-size": "2.0.4", "yaml": "2.9.1" }, "devDependencies": { @@ -945,6 +946,18 @@ "node": "^8.16.0 || ^10.6.0 || >=11.0.0" } }, + "node_modules/image-size": { + "version": "2.0.4", + "resolved": "https://registry.npmjs.org/image-size/-/image-size-2.0.4.tgz", + "integrity": "sha512-QRUkFFsRV/6fuESxb9Vkq+a0LkSrgKXuc2NEqfikiXxxN/G3tjWt5EVUlMaImRBZRZK/jRBEbYvpPYZL8t08Zw==", + "license": "MIT", + "bin": { + "image-size": "bin/image-size.js" + }, + "engines": { + "node": ">=18" + } + }, "node_modules/json-schema-traverse": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/json-schema-traverse/-/json-schema-traverse-1.0.0.tgz", diff --git a/package.json b/package.json index d613eb6..b169120 100644 --- a/package.json +++ b/package.json @@ -18,6 +18,7 @@ "@fontsource/ibm-plex-sans": "5.3.0", "ajv": "8.20.0", "diff": "9.0.0", + "image-size": "2.0.4", "yaml": "2.9.1" }, "devDependencies": { diff --git a/scripts/demo.ts b/scripts/demo.ts index bc11009..cd2abbb 100644 --- a/scripts/demo.ts +++ b/scripts/demo.ts @@ -30,7 +30,7 @@ export function createDemo(directory: string): ReviewConfig { const config = JSON.parse(readFileSync(configPath, 'utf8')) as ReviewConfig; if (config.demo !== true || config.repository !== join(root, 'retry-service') || config.database !== join(root, 'review.sqlite')) throw new Error('Demo configuration must refer to its own fixture.'); check(config.repository, true); check(join(config.repository, '.git'), true); check(config.database, false); - for (const suffix of ['-wal', '-shm']) if (existsSync(config.database + suffix)) check(config.database + suffix, false); + for (const suffix of ['-wal', '-shm']) if (lstatSync(config.database + suffix, { throwIfNoEntry: false })) check(config.database + suffix, false); return config; } if (existsSync(root)) throw new Error('Demo directory exists without a configuration. Choose a new empty path.'); diff --git a/test/demo.test.ts b/test/demo.test.ts index 5cbb925..1f91797 100644 --- a/test/demo.test.ts +++ b/test/demo.test.ts @@ -48,3 +48,10 @@ it('rejects symlinked ancestors before creating a demo', () => { const base = root(); symlinkSync(base, join(base, 'alias')); expect(() => createDemo(join(base, 'alias', 'nested', 'demo'))).toThrow(/fixture/i); }); +it('rejects dangling SQLite sidecar symlinks', () => { + const base=root(),directory=join(base,'demo');const config=createDemo(directory); + for(const suffix of ['-wal','-shm']) { + const path=config.database+suffix;rmSync(path,{force:true});symlinkSync(join(base,'missing'+suffix),path); + expect(()=>createDemo(directory)).toThrow(/fixture/i);rmSync(path); + } +}); diff --git a/test/history.test.ts b/test/history.test.ts index cc7f3e8..205a93f 100644 --- a/test/history.test.ts +++ b/test/history.test.ts @@ -328,3 +328,10 @@ it('rejects shallow parent rewriting before loading history', () => { f.write('.git/shallow', `${head}\n`); expect(() => readHistory(f.dir, f.base, head)).toThrow(/shallow/i); }); +it('omits raster previews with oversized declared dimensions', () => { + const f=fixture(); const base=f.git('rev-parse','HEAD'); + const image=Buffer.from('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAwMCAO+/lXsAAAAASUVORK5CYII=','base64'); + image.writeUInt32BE(100000,16);image.writeUInt32BE(100000,20); + f.write('oversized.png',image);const head=f.commit(); + expect(readHistory(f.dir,base,head).final.find(file=>file.newPath==='oversized.png')?.after?.preview).toBeUndefined(); +});