Skip to content

test: restore compact mock isolation - #1979

Merged
kevincodex1 merged 3 commits into
Twigpine:mainfrom
jatmn:fix/two-flaky-smoke-tests
Jul 16, 2026
Merged

kevincodex1 merged 3 commits into
Twigpine:mainfrom
jatmn:fix/two-flaky-smoke-tests

Conversation

@jatmn

@jatmn jatmn commented Jul 16, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • restore the real token-estimation module after compact tests so downstream microcompact token assertions use production estimates
  • restore the real Claude API module so downstream auto-compact context-window tests use production model metadata
  • restore the real GrowthBook module so downstream time-based microcompact configuration remains shape-correct

Validation

  • bun test --feature=UNATTENDED_RETRY --max-concurrency=1 src/services/compact/compact.test.ts src/services/compact/microCompact.test.ts src/services/compact/autoCompact.test.ts
  • bun run smoke
  • bun run typecheck
  • bun run test:full (7,169 pass; 23 unrelated pre-existing failures in provider-startup and commit-governance tests)

Reviewed AGENTS.md and CONTRIBUTING.md.

Final reviewed SHA: 036e4fb07fbf116a803f216c9cf320b6a4954c1e.

Summary by CodeRabbit

  • Tests
    • Improved test isolation by adding cache-busting imports and automatic restoration of compact-related mocked modules after the compact suite completes.
    • Enhanced standalone compact auto-import testing by explicitly restoring real utility implementations (context, errors, tokens) before importing the auto-compaction logic, preventing cross-suite stub leakage.

@coderabbitai

coderabbitai Bot commented Jul 16, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The test harness now cache-busts real compact-related modules, registers real utility implementations before auto-compact imports, and restores mocked modules during compact teardown with export identity checks.

Changes

Compact test isolation

Layer / File(s) Summary
Preserve real module dependencies
src/services/compact/autoCompact.test.ts, src/services/compact/compact.test.ts
Caches real utility and compact dependency implementations and registers them before importing code under test.
Restore compact test modules
src/services/compact/compact.test.ts
Restores compact-stubbed modules during afterAll and verifies key exports match the real implementations.

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

Possibly related PRs

Suggested reviewers: chioarub, adityachaudhary99, kevincodex1

🚥 Pre-merge checks | ✅ 7
✅ Passed checks (7 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Risk Surface Disclosed ✅ Passed Diff is test-only mock cleanup; comments frame it as defensive isolation, not a runtime auth/provider/permissions change, so no blocker was introduced.
No Hidden Policy Change ✅ Passed Only test isolation helpers changed; no production policy, routing, telemetry, or permission-policy logic was modified or hidden in cleanup.
Title check ✅ Passed The title is concise and accurately reflects restoring compact mock isolation in tests.
Description check ✅ Passed Summary and validation are provided, and the description is mostly complete despite missing Impact and Notes sections.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@jatmn jatmn self-assigned this Jul 16, 2026
@jatmn jatmn added the bug Something isn't working label Jul 16, 2026
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 16, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@src/services/compact/compact.test.ts`:
- Around line 741-764: The cleanup assertions currently cover only context,
errors, and tokens; extend the restoration checks to the compact-only modules
tokenEstimation, claude API, and growthbook. Import each restored module after
the existing Promise.all in the cleanup section and assert its relevant exported
module identity matches _realTokenEstimationModule, _realClaudeApiModule, and
_realGrowthBookModule, respectively.
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 06f6c259-1f2a-417c-94e4-bd374ddba38e

📥 Commits

Reviewing files that changed from the base of the PR and between 036e4fb and 39ff243.

📒 Files selected for processing (1)
  • src/services/compact/compact.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
  • GitHub Check: smoke-and-tests (22)
  • GitHub Check: smoke-and-tests (24.11.x)
  • GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

TypeScript code in this repository must use strict mode and ESM imports.

**/*.{ts,tsx}: When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Run bun run typecheck and bun run typecheck:type-tests for TypeScript changes.
Run provider tests and provider recommendation tests when changing provider behavior: bun run test:provider and bun run test:provider-recommendation.

Files:

  • src/services/compact/compact.test.ts
**/*

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*: Keep changes focused on one problem or feature and avoid mixing unrelated cleanup into the same change.
Preserve existing repository patterns unless intentionally refactoring them.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary changes, and adherence to project architecture before submitting them.
Follow the existing code style in touched files.
Prefer small, readable changes over broad rewrites.
Do not reformat unrelated files.
Keep comments useful and concise.
Provider changes must explicitly identify affected providers, limitations, and follow-up work in the pull request description.
Do not assign or use provider tags; provider tags are controlled by maintainers.
Run the relevant validation checks locally before submitting changes; pull requests must pass CI checks.
Run bun run security:pr-scan before submitting a pull request.
Dependency changes require a concrete project benefit, such as fixing a bug, addressing a security issue, or supporting an approved feature.
Do not change the project's language, core runtime, or dependency stack without prior maintainer agreement.

Files:

  • src/services/compact/compact.test.ts

⚙️ CodeRabbit configuration file

**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.

Files:

  • src/services/compact/compact.test.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Add or update tests when a code change affects behavior.

Files:

  • src/services/compact/compact.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}

⚙️ CodeRabbit configuration file

{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.

Files:

  • src/services/compact/compact.test.ts
🪛 ast-grep (0.44.1)
src/services/compact/compact.test.ts

[warning] 75-75: Do not use Math.random() to generate security-sensitive values such as tokens, secrets, passwords, API keys, salts, nonces, OTPs, or session IDs. Math.random() is not cryptographically secure and is predictable. Use crypto.randomBytes()/crypto.randomUUID() (Node) or crypto.getRandomValues() (Web Crypto) instead.
Context: Math.random()
Note: [CWE-330] Use of Insufficiently Random Values.

(insecure-random-security-token-typescript)


[warning] 90-90: Do not use Math.random() to generate security-sensitive values such as tokens, secrets, passwords, API keys, salts, nonces, OTPs, or session IDs. Math.random() is not cryptographically secure and is predictable. Use crypto.randomBytes()/crypto.randomUUID() (Node) or crypto.getRandomValues() (Web Crypto) instead.
Context: Math.random()
Note: [CWE-330] Use of Insufficiently Random Values.

(insecure-random-security-token-typescript)

🔇 Additional comments (2)
src/services/compact/compact.test.ts (2)

75-137: LGTM!


701-706: LGTM!

Comment thread src/services/compact/compact.test.ts
@jatmn
jatmn marked this pull request as ready for review July 16, 2026 02:57

@jatmn jatmn left a comment •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevincodex1
kevincodex1 merged commit 2516eef into Twigpine:main Jul 16, 2026
5 checks passed
@jatmn
jatmn deleted the fix/two-flaky-smoke-tests branch July 16, 2026 16:46
hotmanxp pushed a commit to hotmanxp/openclaude that referenced this pull request Jul 18, 2026
* test: restore compact test module stubs

* test: restore remaining compact mock modules

* test: isolate auto compact from compact mocks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants