Skip to content

Stabilize QA tests and correct the manual runbooks - #1404

Merged
ymichael merged 7 commits into
mainfrom
bb/split-flaky-test-and-qa-fixes-into-pr-thr_7vjjsfxsns
Aug 12, 2026
Merged

ymichael merged 7 commits into
mainfrom
bb/split-flaky-test-and-qa-fixes-into-pr-thr_7vjjsfxsns

Conversation

@ymichael

@ymichael ymichael commented Aug 12, 2026 •

Copy link
Copy Markdown
Collaborator

What this fixes

Test-determinism and QA/runbook maintenance only, extracted from #1385.

Issue Simple explanation Fix
Docs watcher test was timing-dependent The test wrote a real file and then raced native FSEvents plus a 250ms debounce, so it failed whenever the watch-vaults service had not attached yet or the host was busy. Give the Docs plugin a watcher injection seam that defaults to native fs.watch, and have the test drive a fake watcher directly. It now asserts the watched root, the published vault-changed signal, and watcher cleanup on abort — with no filesystem-event timing.
Database-maintenance test depended on file order The @bb/server vitest project runs with isolate: false, so files share the module-level lastDatabaseMaintenanceCheckAt. periodic-sweeps.test.ts parks it at wall time, while this file swept from a hardcoded 1_000_000_000_000 (2001) — every sweep then looked "too soon". Start the synthetic clock a day ahead of wall time so it always leads any real timestamp another file records.
Daemon restart helper only worked once buildDaemonRestartCommand baked in the PID captured at build time, so a second run left the first replacement daemon alive and raced a second one onto the same port and data dir. Resolve the PID from the daemon PID file at run time (build-time PID as first-run fallback), and refuse to run when the resolved value is not a positive integer so a corrupt PID file cannot become a stray kill.
Archive runbook expected immediate destruction A last-live archived managed worktree stays revivable for the five-minute undo grace period (MANAGED_ENVIRONMENT_RETIRE_GRACE_MS), so the old expectation was wrong. Assert retiring plus a preserved worktree first, then permanently delete the archived thread and wait for destruction.
Runbook tracked the wrong restarted daemon DAEMON_PID=$! after eval "$RESTART_DAEMON_COMMAND" captures the shell's job, not the detached daemon. Read the authoritative PID file (.paths.daemonRestartPidPath) exported by the standalone harness after each restart.
Permission QA used non-public modes and unreliable probes readonly and workspace-write are not accepted by the CLI (public modes are accept-edits, auto, full), and Codex sandboxes commonly permit the OS temp dir, so /tmp probes were not reliably outside the workspace. Cover only the public modes and place the approval/denial probes under $HOME. A new contract test rejects stale mode names and pins the probe targets.
Provisioning runbook promised restart recovery The old "Server restart during environment provisioning" scenario killed the server mid-RPC and implied lost-result recovery and persisted attempt identity. Removed entirely. Replaced with the supported contract: a .bb-env-setup.sh hook fails exactly once, the thread and environment settle in error with thread_provisioning_failed, and one explicit bb thread tell ... --mode auto starts a fresh provisioning attempt that reaches ready before the turn runs. A contract test keeps the restart scenario from returning.

Scope

No product behavior, database schema, migration, Drizzle snapshot, provisioning implementation, host-daemon wire contract, or protocol-version change.

The full diff is 8 files. Only two are non-test sources:

  • plugins/docs/server.ts — adds the watcher injection seam; the default parameter is the existing native fs.watch implementation, so production behavior is unchanged.
  • tests/qa/src/shared.ts — QA-harness helper in the tests/qa package.

Deliberately omitted: the provider-interaction integration commit from #1385 (0fc13d45e). It deletes roughly 750 lines of live-provider scenarios covering auto permission mode plus the auto-ask/auto-deny harness presets. Its premise does hold on current main — auto pairs with approvalReviewer: "automatic", which the Codex adapter maps to auto_review rather than routing to onInteractiveRequest — so it is not strictly coupled to the excluded Codex runtime commits. It is omitted because it is a coverage deletion rather than a flaky-test or QA fix, and it cannot be validated here: src/integration*.test.ts is excluded from the default test task and only runs via test:integration against real providers. Removing unverifiable integration coverage does not belong in a no-product-change PR; it is left as a follow-up.

Validation

Run on this branch rebased onto current main:

  • turbo run typecheck test --filter=bb-plugin-simple-notes --filter=@bb/qa --force — 5/5 tasks; Docs 59 tests, QA 25 tests.
  • turbo run typecheck test --filter=@bb/server --force — 161 files, 1450 tests passed.
  • Database-maintenance flake reproduced before the fix with --no-file-parallelism --sequence.shuffle.files --sequence.seed=3 (4 failures), and passing on that same ordering after.
  • The standalone-restart regression assertions were confirmed to fail against the previous buildDaemonRestartCommand.
  • turbo run lint on the touched packages — no lint tasks are defined for them.
  • Prettier clean; final diff audited for schema/migration/contract/provisioning paths (none).

Runbook claims were checked against main rather than assumed: thread_provisioning_failed, error in threadStatusValues, bb thread wait --status, bb thread delete --yes, bb thread tell --mode auto, PROJECT_ROOT exported by the QA harness, and MANAGED_ENVIRONMENT_RETIRE_GRACE_MS = 5 * 60_000.

Pre-existing macOS-only failure, not from this PR

packages/agent-runtime/src/runtime.process-lifecycle.test.ts > bounds provider stderr while data arrives without a newline fails on clean origin/main on macOS. Its fake child writes 100KB to stderr and immediately calls process.exit(42). Pipe writes are asynchronous on macOS, so the child exits without flushing bytes that never reach the parent; this is distinct from the real parent-side exit-before-close drain bug. CI's Tests (packages) shard runs on Ubuntu, where the fixture passes.

#1402 fixes both issues separately: the fixture uses process.exitCode = 42 so Node flushes naturally, while the runtime waits for stream close before finalizing provider exit. Those provider-runtime changes are explicitly out of scope here, and this branch touches no agent-runtime files.

AGENT GENERATED: by Claude Opus 5 (1M context)

ymichael and others added 7 commits August 12, 2026 14:02
The watcher test wrote a real file and waited for a native FSEvents
callback plus the 250ms debounce, so it failed whenever the watch-vaults
service had not attached before the write or the host was busy enough to
delay the event past the assertion window.

Give the Docs plugin a watcher injection seam that defaults to the native
`fs.watch` implementation, and have the test supply a fake watcher it can
trigger directly. The test now asserts the watched root, the published
`vault-changed` signal, and watcher cleanup on abort without depending on
filesystem event timing. Production behavior is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The @bb/server vitest project runs with `isolate: false`, so every test
file in a worker shares the module-level `lastDatabaseMaintenanceCheckAt`
in periodic-sweeps.ts. periodic-sweeps.test.ts drives the real sweep
registry at `Date.now()`, which parks that timestamp at wall time. This
file then swept from a hardcoded 1_000_000_000_000 (2001), so whenever it
happened to run after periodic-sweeps.test.ts in the same worker its
synthetic sweeps looked "too soon" and every assertion failed.

Start the synthetic clock a day ahead of wall time so it always leads any
real timestamp another file may record. Reproduced the failure with
`--no-file-parallelism --sequence.shuffle.files --sequence.seed=3` and
confirmed the fix passes on the same ordering.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
buildDaemonRestartCommand baked the daemon PID captured at build time
into the shutdown step, so the string only worked for the first restart.
Running it a second time left the daemon started by the first run alive
and then raced a second daemon against it on the same port and data dir.

Resolve the PID from the daemon PID file at run time, keeping the
build-time PID as a fallback for the first restart before the file
exists, and refuse to run at all when the resolved value is not a
positive integer so a corrupt PID file cannot turn into a stray `kill`.
An empty PID stays valid and simply skips the shutdown step.

Extend the standalone restart test to run the command twice and assert
the first daemon is gone and a new PID is recorded. Verified the new
assertions fail against the previous implementation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Exercise the supported contract deterministically: let a setup hook fail once, wait for the failure to settle, then send a new auto-mode message to start a fresh provisioning attempt.

Remove the server-restart scenario and guard the runbook against reintroducing restart recovery or persisted provisioning-attempt semantics.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ymichael
ymichael force-pushed the bb/split-flaky-test-and-qa-fixes-into-pr-thr_7vjjsfxsns branch from 603561a to 27f7583 Compare August 12, 2026 21:07
@ymichael
ymichael merged commit 203950d into main Aug 12, 2026
10 checks passed
@ymichael
ymichael deleted the bb/split-flaky-test-and-qa-fixes-into-pr-thr_7vjjsfxsns branch August 12, 2026 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant