test: isolate aggregate durability timing (#255)

[mohanagy]

Summary

• isolate logical audit, Console, and OAuth profile-rename contracts from host-dependent FileHandle.sync latency
• retain dedicated real-sync audit integrity and OAuth profile-rename integration tests
• keep process-tree containment at the real replacement-start boundary, drain bounded restart work, and use a minimal MCP fixture for shutdown timing
• leave production code, timeouts, skips, coverage thresholds, platform gates, security behavior, and public APIs unchanged

Refs #255

Root cause and TDD evidence

The aggregate suite reproduced three exact five-second logical-suite timeouts in audit integrity/reader work, remote OAuth/Console work, and OAuth profile-rename transactions. It also reproduced a descendant containment test waiting for a replacement fixture counter after the containment ordering contract had already passed, plus a shutdown-timing test paying the generic fixture startup cost.

The focused logical/durability contract was tightened first and failed on the current tree before the implementation. The implementation stubs only FileHandle.sync in named logical suites. Real-sync coverage remains in tests/audit.test.ts, tests/audit-integrity-durability.test.ts, tests/oauth-profile-rename-durability.test.ts, and the broader durability/platform suites.

Local validation

• npm test: 142 passed files, 1 platform file skipped; 1780 passed tests, 33 conditional/platform skips
• npm run test:core: 22 passed files, 1 platform file skipped; 425 passed tests
• npm run test:coverage: 95.65% statements/lines, 91.88% branches, 98.66% functions
• npm run lint
• npm run typecheck
• npm run build
• npm run smoke:cli
• npm run check:pack: 48 files verified
• npm run test:package: 26 passed tests
• git diff --check

All commands used supported Node 22.22.3. No timeout was increased and no test or platform gate was disabled.

Review

An independent exact-tree review found no blockers. It confirmed the logical sync stubs are restored per test, the audit and OAuth real-sync paths remain unmocked, the minimal shutdown fixture is cross-platform and shell-free, and the replacement containment assertion remains at the real transport start boundary.

Summary by CodeRabbit

• Bug Fixes

  • Improved durability handling for audit records, Console operations, and OAuth profile renames across varying filesystem conditions.
  • Strengthened shutdown and restart behavior so related processes are fully cleaned up before replacements start.
  • Preserved fail-safe locking, transactional recovery, containment, and production audit-write behavior.

• Tests

  • Added coverage for real filesystem durability, audit verification, OAuth profile renames, shutdown timing, and process cleanup.
  • Expanded cross-platform and timeout-related contract coverage.

[mohanagy]

@coderabbitai review

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR separates logical filesystem tests from host sync latency, adds real-filesystem audit and OAuth durability coverage, introduces a delayed-shutdown upstream fixture, and refines descendant-reaping assertions.

Changes

Durability and lifecycle test contracts

Layer / File(s)	Summary
Logical filesystem sync isolation tests/helpers/logical-file-sync.ts, tests/audit-*.test.ts, tests/console-application-service.test.ts, tests/oauth-profile-rename-transaction.test.ts	Adds cached FileHandle.sync stubbing and applies it before logical audit, Console, and OAuth tests; a contract suite verifies the intended suite boundaries.
Real audit and OAuth durability validation tests/audit-integrity-durability.test.ts, tests/oauth-profile-rename-durability.test.ts	Adds temporary-real-filesystem checks for audit journal verification, OAuth profile renaming, backup creation, configuration updates, and journal cleanup.
Upstream shutdown and descendant reaping contracts tests/fixtures/shutdown-delay-upstream.mjs, tests/helpers/upstream-manager-contracts.ts	Adds a delayed-shutdown JSON-RPC fixture and updates teardown/restart assertions for timeout handling and descendant reaping.
Release note CHANGELOG.md	Documents the durability-boundary and shutdown-contract test changes under the unreleased fixes. :contentReference[oaicite:0]{index=0}

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• mohanagy/miftah#69: Related audit integrity durability and managed journal testing.
• mohanagy/miftah#279: Touches the OAuth profile-rename transaction test surface.
• mohanagy/miftah#293: Touches upstream lifecycle contract testing infrastructure.

Poem

A bunny checks the journal chain,
While sync sleeps softly in the rain.
OAuth hops from work to studio,
Shutdown waits—then off we go!
Tests tidy every trail away.

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore

❌ Failed checks (1 error)

Check name	Status	Explanation	Resolution
Docstring Coverage	❌ Error	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title is concise and accurately reflects the main change: isolating durability timing in tests.
Description check	✅ Passed	Includes summary, rationale, and concrete validation results; it only diverges from the template by omitting a dedicated Security impact heading.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/issue-255-audit-aggregate-isolation

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

tests/helpers/upstream-manager-contracts.ts (1)

1004-1012: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Do not discard the restart result.

await restarting.catch(() => undefined) allows this test to pass when replacement startup fails after its transport starts. Keep the early rejection handler if needed, but finish with await expect(restarting).resolves.toBeDefined().

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/helpers/upstream-manager-contracts.ts` around lines 1004 - 1012, Update
the restart assertion in the test around manager.restart("work") so the restart
promise’s final outcome is validated rather than discarded. Retain the early
rejection handler if needed to avoid unhandled rejection during waitFor, but
replace the final restarting catch with an assertion that restarting resolves to
a defined value.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/audit-logical-suite-contract.test.ts`:
- Around line 24-32: Update the assertions in the audit logical suite contract
test to verify that each relevant suite registers a beforeEach callback
containing the awaited stubFileSyncForLogicalTest call, rather than only
checking for the helper token. Preserve the existing positive and negative
expectations for auditDurabilitySuite and the durability suites.

---

Outside diff comments:
In `@tests/helpers/upstream-manager-contracts.ts`:
- Around line 1004-1012: Update the restart assertion in the test around
manager.restart("work") so the restart promise’s final outcome is validated
rather than discarded. Retain the early rejection handler if needed to avoid
unhandled rejection during waitFor, but replace the final restarting catch with
an assertion that restarting resolves to a defined value.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 80c52bc6-03ee-4f43-a384-b55de8d6d00a

📥 Commits

Reviewing files that changed from the base of the PR and between 3ecab5b and 511ba93.

📒 Files selected for processing (11)

• CHANGELOG.md
• tests/audit-integrity-durability.test.ts
• tests/audit-integrity.test.ts
• tests/audit-log-reader.test.ts
• tests/audit-logical-suite-contract.test.ts
• tests/console-application-service.test.ts
• tests/fixtures/shutdown-delay-upstream.mjs
• tests/helpers/logical-file-sync.ts
• tests/helpers/upstream-manager-contracts.ts
• tests/oauth-profile-rename-durability.test.ts
• tests/oauth-profile-rename-transaction.test.ts

[mohanagy]

Addressed both CodeRabbit findings on exact head 4d9c6c4: the logical-suite contract now proves registration of an awaited beforeEach sync stub, and the descendant-reaping contract now requires manager.restart("work") to resolve successfully instead of discarding its result. The early rejection handler remains only to avoid an unhandled-rejection race while containment is observed. Focused result: 2 files / 15 tests passed; lint, typecheck, and git diff --check passed. Current-head CI is running.

Both findings were addressed in 4d9c6c4. CodeRabbit confirmed the inline fix on the current head and returned a successful current-head status with no new findings; the outside-diff restart finding is fixed by asserting the original promise resolves. Focused validation and independent exact-head review found no blockers.