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

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions .claude/skills/release/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -97,21 +97,23 @@ Run, in this order, and **stop on any failure**:

```bash
npm ci # clean install, lockfile authoritative
npm audit --omit=dev # zero high/critical in runtime deps
npm audit --omit=dev # zero vulnerabilities in root runtime deps
npm run build # full build; emits declarations used by test type-checks
npm run audit:packed-consumer # zero vulnerabilities in a fresh tarball consumer
npm run check # type-check server + web + tests against fresh dist
npm run test:unit # fast unit suite
npm run test:browser # Playwright browser journeys
npm run test:e2e # API + worktree/Docker/MCP/restart E2E
```

Rules:
- **`npm audit` must show 0 vulnerabilities** (any severity, runtime deps). If it doesn't, stop and report what's flagged — do not release with known vulns. If a finding is genuinely a false positive (e.g. dev-only path), have the user explicitly acknowledge before continuing.
- **Both audits must show 0 vulnerabilities** at every severity. The packed-consumer command installs the just-built tarball under normal npm settings because a clean root audit cannot see dependency-owned shrinkwrap findings. Any finding blocks publish; there are no release exceptions.
- Registry advisory availability is deliberately release-only, not part of normal unit, browser, or E2E gates. If the advisory service or clean consumer install is unavailable, stop the release rather than skipping the packed-consumer audit.
- Don't skip browser or E2E tests "because they're slow" — releases are the one place flakes bite users.
- Build must precede `check`: `tsconfig.tests2.json` follows intentional imports of emitted `dist/server/*.js` declarations, so a clean checkout cannot type-check the test graph before the build.
- Build must precede both `audit:packed-consumer` and `check`: the audit packs built output, while `tsconfig.tests2.json` follows intentional imports of emitted `dist/server/*.js` declarations.
- If any test fails, the failure is the bug. Fix it or abort the release; do not retry hoping it's flaky.

Long-running steps (`build`, `test:browser`, `test:e2e`) should use `bash_bg` so output stays inspectable.
Long-running steps (`build`, `audit:packed-consumer`, `test:browser`, `test:e2e`) should use `bash_bg` so output stays inspectable.

## 3. Decide whether to bump the binary sub-packages

Expand Down Expand Up @@ -320,7 +322,7 @@ Report to the user:
- npm package URL (`https://www.npmjs.com/package/bobbit/v/<new-version>`)
- Whether provenance was attached
- Whether binaries were republished, and which versions
- Any audit findings that were explicitly accepted
- Root and packed-consumer audit results (both must be clean)

## Rules / best practices

Expand Down
14 changes: 7 additions & 7 deletions docs/design/async-background-cleanup-issue-analysis.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ POST /api/preview/mount
-> JSON response
```

`persistPreviewArtifact` is synchronous today. The route does not broadcast or return success when artifact persistence fails, although the live mount has already been written. Reuse is per session and returns the first valid matching candidate in filesystem enumeration order.
`persistPreviewArtifact` is synchronous today. The route does not broadcast or return success when artifact persistence fails, although the live mount has already been written. Reuse is per session and returns the first valid matching candidate in the order produced by that lookup's own candidate enumeration.

#### Restore

Expand Down Expand Up @@ -152,7 +152,7 @@ Direct immutable artifact serving in `src/server/preview/content-route.ts::handl
| `restorePreviewArtifact` | sync copy/list/hash/stat/rename/remove | Stage and validate before wipe; backup only when live mount existed; rollback on swap failure |
| `removeArtifacts` | `rmSync({ recursive: true })` | Invalid session ID is a no-op; missing tree is success; idempotent |
| `sweepOrphanArtifacts` | `existsSync`, `readdirSync`, per-directory `rmSync` | Keep valid known session IDs case-insensitively; delete every other directory, including non-session directory names; ignore non-directories; sorted `removed`/`kept` results; per-item failure isolation |
| `findPreviewArtifactByHash` | `existsSync`, `readdirSync`, then sync read/list/hash per candidate | Preserve `readdir` candidate order and first valid reuse; corrupt/mismatched candidates are skipped, not deleted |
| `findPreviewArtifactByHash` | `existsSync`, `readdirSync`, then sync read/list/hash per candidate | Preserve the sequence returned to that lookup and first valid reuse; never infer order from a separate enumeration; corrupt/mismatched candidates are skipped, not deleted |
| `validateArtifactMount` | sync stat/list/hash | Entry present, exact file-set equality, then content hash equality |
| `copyDirectory`, `writeJsonAtomic`, `hashDirectory`, `listFiles`, `safeStat`, `wipeContents`, `moveContents` | all filesystem work is synchronous; recursive listing can overflow/defer for a deep/wide tree | Preserve sorted POSIX relative paths and fallback copy-on-rename-failure behavior |
| `src/server/preview/mount.ts::mountDir` as called above | injected `mountFs.mkdirSync` | Do not create a mount merely to test whether it existed during restore |
Expand All @@ -179,7 +179,7 @@ Directories and non-files are not hashed. Directory-read failure currently contr
5. Serialize preview mutation/persist/restore by session ID. Synchronous code currently gives these operations an implicit critical section; converting them independently would allow a second mount write to change the live tree while the first request is hashing or copying it. The lock must cover mount mutation through artifact persistence and cover the whole restore swap/rollback.
6. GET mount snapshot takes the same session lock so entry, mtime, content hash, and artifact ID describe one tree version.
7. For SSE, wait for the session lock, subscribe while holding it, take/write the bootstrap snapshot, then release. Events that occurred before the lock are represented by the snapshot; mutations cannot broadcast a newer event until bootstrap is written. This preserves bootstrap-before-live ordering without a missed-event window.
8. Candidate validation in `findPreviewArtifactByHash` remains sequential in the `readdir` result order. Internal traversal/hash work is bounded, but candidates must not race for “first valid match.”
8. Candidate validation in `findPreviewArtifactByHash` remains sequential in the order emitted by that lookup's own enumeration. After migration, this is the `Dirent` sequence from its `opendir()`/`Dir.read()` stream; no equivalence is promised with a separate `readdir()` call, another stream, or lexical sorting. Concurrent metadata work retains stream indexes, but exact validation must not race for “first valid match.”
9. `sweepOrphanArtifacts` may delete independent session directories through the bounded mapper. Collect outcomes in enumeration order and sort both public arrays at the end exactly as today.
10. Bounded removal must propagate non-`ENOENT` errors to the owning purge listener. `force`/missing remains idempotent. This makes the existing listener error log meaningful instead of hiding every removal failure inside `removeArtifacts`.

Expand Down Expand Up @@ -474,7 +474,7 @@ Missing acceptance coverage:

- wide/deep artifact stores and traversal ceiling;
- event-loop progress while lookup/hash/copy/delete is deferred;
- several candidates in a controlled enumeration order;
- several candidates in a controlled lookup stream order;
- invalid session/artifact metadata, wrong hash, extra/missing files and missing entry as independent skip cases;
- exact first valid reuse selection when multiple candidates match;
- stable hash parity for nested/binary/empty files and sorted record file names;
Expand Down Expand Up @@ -553,7 +553,7 @@ Do not scan all of `server.ts::handleApiRoute` as one root, which would pull unr

### 9.2 Deterministic unit tests

Use deferred promise filesystem/command-runner fakes. Start an operation, queue an unrelated microtask/`setImmediate`, assert it runs while I/O is held, then release the I/O and assert the result. Do not use wall-clock performance thresholds.
Use deferred promise filesystem/command-runner fakes. Start an operation, queue an unrelated microtask/`setImmediate`, assert it runs while I/O is held, then release the I/O and assert the result. Do not use wall-clock performance thresholds. Signal readiness explicitly from the operation boundary being observed; a finite number of event-loop turns is not proof that prerequisite asynchronous work has reached that boundary. Register every test-owned hold for failure-safe cleanup, release it before awaiting stop/listener barriers, and also release it in the test's `finally` path so an earlier assertion cannot strand teardown.

Add focused tests for:

Expand All @@ -572,7 +572,7 @@ Add an in-process integration test with injectable/deferred preview I/O:
2. start a preview bootstrap/reuse scan and hold a deep metadata/hash read;
3. concurrently issue `GET /api/health` and `POST /api/sessions`;
4. assert both requests complete before releasing the artifact traversal;
5. release and assert exact artifact selection.
5. release and assert selection of the first valid artifact from that controlled stream.

Repeat the ordering assertion for purge-triggered mount/artifact deletion: hold deletion, prove health/session creation complete, then release and assert the purge response waits for cleanup. Use existing browser preview journeys to verify POST, SSE, historical artifact switching, reload restoration, and cleanup still work; add a browser-v2 journey only if those existing journeys cannot exercise the new awaited route seams.

Expand All @@ -596,7 +596,7 @@ No excluded finding is declared safe; it is simply not part of this focused firs
Implementation and review should explicitly confirm:

- preview hashes and sorted file lists are byte-for-byte stable;
- first valid artifact candidate selection is unchanged;
- first valid artifact candidate selection follows the lookup's own streamed enumeration order;
- corrupt artifacts are skipped, never reused or auto-deleted by lookup;
- restore validation precedes mutation and rollback remains available;
- all cleanup APIs remain idempotent and response shapes/counts remain unchanged;
Expand Down
8 changes: 4 additions & 4 deletions docs/design/async-background-cleanup.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ Missing entries are handled at the operation that owns them. `ENOENT` remains su
| One-root deletion | `removeTree` is an iterative streaming lane over an identity-detached root, not a recursive promise tree. |
| Multi-root deletion | `removePreviewTrees` and `sweepOrphanArtifacts` run no more than the shared number of independent root lanes. Preview backup/quarantine cleanup also uses a single global ownership lane. |

Metadata reads may settle concurrently within one candidate batch, but results are processed in filesystem enumeration order. Exact mount validation is sequential and the next batch is not prefetched during validation. This preserves the first valid matching candidate even when metadata reads complete out of order.
Here, filesystem enumeration order means the `Dirent` sequence emitted by this lookup's own `opendir()`/`Dir.read()` stream. A separate `readdir()` call—or even a separate stream—is not an ordering oracle: no cross-call, cross-API, or lexical equivalence is promised. Metadata reads may settle concurrently within one candidate batch, but the bounded mapper retains each stream index. Exact mount validation then consumes those indexed results sequentially, without prefetching the next batch, so reuse remains the first valid matching candidate from the lookup's own stream.

A candidate is reusable only when:

Expand Down Expand Up @@ -199,7 +199,7 @@ A missing worktree short-circuits the transcript stat; missing paths and I/O err
Asynchrony does not change public selection or response semantics:

- bounded maps retain input result order; inventories apply their existing final sort;
- artifact metadata may finish out of order, but reuse remains first-valid filesystem order;
- artifact metadata may finish out of order, but reuse remains first-valid order from that lookup's own `Dir.read()` stream;
- hashes and metadata file lists remain stable and sorted;
- orphan cleanup returns an exact count and deterministic bounded samples;
- mutation prune and archive stats retain exact count rules;
Expand All @@ -219,10 +219,10 @@ Behavioral coverage is split by invariant:
- Preview unit suites cover wide/deep traversal, ordered batched candidates, corruption and missing entries, exact hash/file-list parity, catalog bounds and generation races, descriptor/root/parent identity substitution, transactional install/rollback, quarantine restoration, cleanup idempotency, and per-item failure isolation.
- Worktree suites cover deferred event-loop progress, shared ceilings, pure pool construction and initialization/stop/drain barriers, boot sweep-before-pool ordering, deterministic inventory results, live ownership arriving during a scan, archived-branch and shared/container/pool protections, and targeted symlink-safe deletion.
- Plan-mutation, archive-purge, and orphan suites use deferred fake I/O to pin event-loop progress, bounded read-ahead, goal serialization, atomic decisions, timer single-flight, stale callback fencing, awaited stop/shutdown, retention boundaries, listener ownership, scanner sampling, and every orphan-preservation decision.
- `tests2/integration/search-preview-api.test.ts` holds artifact validation and purge deletion while `GET /api/health` and `POST /api/sessions` complete, then proves the preview operation itself still waits and selects the exact artifact. It also pins SSE bootstrap-before-live ordering.
- `tests2/integration/search-preview-api.test.ts` holds artifact validation and purge deletion while `GET /api/health` and `POST /api/sessions` complete, then proves the preview operation itself still waits and selects the first valid artifact from a controlled `Dir.read()` stream. It also pins SSE bootstrap-before-live ordering.
- `tests2/integration/preview-purge-listener-error.test.ts` proves the awaited listener owns an artifact deletion failure without making the gateway unhealthy. Existing preview route and browser journeys preserve POST, restore, reload, and historical artifact behavior.

The tests use deferred ordering and observed concurrency, not machine-speed thresholds.
The tests use deferred ordering and observed concurrency, not machine-speed thresholds. Readiness comes from an explicit signal at the operation boundary under test; a finite number of event-loop turns cannot prove that earlier asynchronous filesystem work has reached that boundary under contention. Test-owned deferreds that can hold a stop or listener barrier are released in failure-safe cleanup before awaiting the barrier. Otherwise an earlier assertion failure can skip the normal release and make correct production teardown wait indefinitely for the test's hold.

## Deferred synchronous-I/O areas

Expand Down
Loading