test: add native compaction behavior regression suite - #2260
Conversation
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (GLM)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: APPROVE — zero findings (P0–P3). (Posted as COMMENT because the reviewer shares the PR author's GitHub identity; GitHub rejects APPROVE from the author.)
Verified the suite pins real behavior, not mocks:
- All assertions traced to source.
buildProviderSettingsbranches (N1),reserveBasedThreshold45k/33k reserves + flooring (N2), thePROVIDER_NO_SDK_AUTO_COMPACTempty-set +shouldUseHyperNeoCompactFallbackinvariant (N3), theSDKMessageHandler.refreshContextUsagegate (N4, sdk-message-handler.ts:1147-1169), and theMessageQueueinternal-flag exclusion (message-queue.ts:285-292) all match the real implementations. Magic numbers (45_000, 33_000, 272_000, 128_000, 1_048_576, 262_144) confirmed againstcontext-tracker.ts,codex-models.ts, andkimi-provider.ts. - N4 handler tests are not vacuous. The harness constructs a real
ContextFetcher;toContextInfoalways returns non-null for the mockedgetContextUsageshape, andqueryObjectis set, sorefreshContextUsagereaches theshouldUseHyperNeoCompactFallbackgate. Removing that gate would maketotalUsed(set well pastreserveBasedThreshold) cross the threshold withshouldCompactAtmocked true →/compactenqueue would fire and thenot.toHaveBeenCalledWith('/compact', true)assertions would fail. The tests genuinely catch the C3/C4 regression class. - Gap claims are honest. Existing
sdk-message-handler.test.tsexercises the/compactnon-enqueue gate only foropenrouter/custom-provider/anthropic— neveranthropic-codexorkimi/kimi-k3(gap a). No existing test assertsPROVIDER_NO_SDK_AUTO_COMPACT.size === 0orshouldUseHyperNeoCompactFallback === false(gap b).onMessageYieldedappears 0 times inmessage-queue.test.ts, so the yield-time broadcast exclusion for internal/compactis newly pinned (gap d). N2 codex/delta values are additive tocontext-tracker.test.ts.
Verification: 28/28 pass; tsc --build --noEmit clean; oxlint . clean. PR is additive (1 new file, no source changes), so no integration/regression risk to existing paths.
Nice consolidation — the four-group structure maps cleanly to the four recurred defect classes and the header doc cross-references the deep per-value suites accurately.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13fd191245
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0584855d7e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const k2Window = buildProviderSettings('kimi', 262_144, 'kimi-k2.7-code')?.autoCompactWindow; | ||
| const k3Window = buildProviderSettings('kimi', 1_048_576, 'kimi-k3')?.autoCompactWindow; | ||
| expect(k2Window).toBe(262_144); |
There was a problem hiding this comment.
Fresh evidence since the prior comment is this new “ACTIVE” assertion, but it still inspects only the value passed into buildProviderSettings, not the SDK-effective threshold. query-options-builder.ts:265-290 documents that the SDK clamps Kimi K2.7's 262,144 override and environment value to a 200,000-token window and therefore compacts at 167,000; this test can remain green if that effective threshold or the SDK/options wiring regresses, while incorrectly labeling 262,144 as active. Drive the SDK boundary or assert its reported effective capacity and auto-compact threshold instead. CLAUDE.mdL114-L119
Useful? React with 👍 / 👎.
| import { | ||
| SDKMessageHandler, | ||
| type SDKMessageHandlerContext, | ||
| } from '../../../../src/lib/agent/sdk-message-handler'; |
There was a problem hiding this comment.
Running the repository's canonical ./scripts/test-daemon.sh 1-core shard shows this new suite failing before any tests execute: earlier session tests install a process-wide mock.module('@hyperneo/shared/sdk/type-guards', ...) without isSDKModelRefusalFallbackMessage, so importing SDKMessageHandler here throws Export named 'isSDKModelRefusalFallbackMessage' not found. The shard intentionally uses one job because Bun module mocks leak between files (scripts/test-daemon.sh:60-67), so this suite must be placed or isolated so it loads before that mock, or the shared mock surface must include the handler's required exports. CLAUDE.mdL46-L50
Useful? React with 👍 / 👎.
| expect(getModelContextWindow('gpt-5.5')).toBe(272_000); | ||
| expect(getModelContextWindow('gpt-5.3-codex')).toBe(272_000); | ||
| expect(getModelContextWindow('gpt-5.4-mini')).toBe(128_000); | ||
| expect(getModelContextWindow('codex-mini')).toBe(128_000); // alias resolves |
There was a problem hiding this comment.
This “per-model” check covers only three canonical Codex models plus one alias, while MODEL_CONTEXT_WINDOWS also contains gpt-5.4 and gpt-5.1-codex-mini. The N1 loop passes each omitted entry's current value back into buildProviderSettings and only checks that auto-compaction is not disabled, so it would not detect either window being changed incorrectly; because AnthropicToCodexBridgeProvider.buildSdkConfig writes each entry directly to CLAUDE_CODE_AUTO_COMPACT_WINDOW, such a change would alter production compaction capacity without failing this regression suite. Assert the expected value for every canonical map entry. CLAUDE.mdL114-L119
Useful? React with 👍 / 👎.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (GLM)
Model: glm-5.1 | Client: NeoKai | Provider: GLM
Recommendation: APPROVE — zero findings (P0–P3) on commit 0584855. Both prior threads resolved. (Posted as COMMENT because the reviewer shares the PR author's GitHub identity.)
Re-review of the new changes (commit 0584855):
- N4 production call-site test (the codex-bot gap). Verified genuine. Adding
test-fallback-providertoPROVIDER_NO_SDK_AUTO_COMPACTopens the gate; withtotalUsed=195_000past the 167k reserve threshold andshouldCompactAtmocked true, the handler reaches the realenqueue('/compact', true)call site (sdk-message-handler.ts:1165). The positive assertions (shouldCompactAtSpy/markCompactionTriggeredSpytoHaveBeenCalled) are a good complement to the negative kimi/codex assertions, andtoHaveBeenCalledWith('/compact', true)genuinely fails if the call site'strueflips tofalseor is omitted. The set is restored infinally(runs even on assertion throw), and the N3size === 0invariant still holds. Notably this avoidsmock.module— the right call given the suspected leak mechanism. - N2 active/dormant reframe. Accurate.
buildProviderSettingsarmed window = the ACTIVE kimi/codex threshold;reserveBasedThreshold= the DORMANT fallback reserve, unreachable while the set stays empty. The bridging test ties them correctly. Matches real source. - MessageQueue test. Correctly rescoped as mechanism-only, pointing at the new call-site test.
CI red — independently verified as pre-existing, NOT this PR:
- Cited dev run
30215248587(commit8073dcb6= the #2246 squash, which does not contain this file) failedDaemon Unit Tests (1-core)with the identical symptom:(fail) getProviderService > should return ProviderService instanceatprovider-service.test.ts:1379:47. - Last green dev 1-core: 2026-07-17 (
389c8b12) — matches your claim. - One precision note (not a blocker): the failure does reproduce on macOS too. Running
provider-service.test.tsalone locally fails on a different test (applyEnvVarsToProcess > clears provider-leaked official Kimi env vars) than the full shard does — that test-variance under reordering is the signature of order-dependent/global-state instability withinprovider-service.test.ts, independent of this PR. The new file passes 30/30 alone and in-shard, uses nomock.module, and restores all mutated state (PROVIDER_NO_SDK_AUTO_COMPACTinfinally,setModelsCacheinafterEach+finally), so it neither causes nor aggravates that failure. Agreed it belongs to the separate dev fix you escalated.
Verification: 30/30 pass (full 1-core shard locally: 3628/3630, sole failure is the pre-existing provider-service.test.ts); tsc --build --noEmit, oxlint ., biome format all clean; PR CI shows only 1-core red, all other checks (incl. Lint/Knip/Type/Web) green.
Consolidated Kimi + Codex auto-compaction contract pinning the four recurred defect classes (Forge episode 851158bf): - N1 native settings: buildProviderSettings never disables auto-compact for kimi/codex (codex native; kimi armed with real window incl. K3 1M) - N2 thresholds: reserveBasedThreshold per-provider (kimi 45k vs codex 33k) and per-model CLAUDE_CODE_AUTO_COMPACT_WINDOW source values - N3 NeoKai fallback: PROVIDER_NO_SDK_AUTO_COMPACT empty invariant; shouldUseHyperNeoCompactFallback false for kimi/codex (dormant extension point) - N4 no /compact injection: handler-level gate for codex + kimi-k3 (previously only kimi-k2.7 covered) and the internal-flag transcript- exclusion mechanism Fills the gaps left by the scattered unit tests (query-options-builder, kimi-provider, anthropic-to-codex-bridge, sdk-message-handler) with a single self-contained contract suite.
…fy N2 - N4: add a test that opens the fallback gate for a test-only provider (PROVIDER_NO_SDK_AUTO_COMPACT, restored in finally) and drives the handler to verify the production enqueue call site passes internal=true. The MessageQueue test now documents it pins the mechanism only. - N2: reframe to distinguish the active SDK-native window (buildProviderSettings + CLAUDE_CODE_AUTO_COMPACT_WINDOW, the effective kimi/codex threshold) from the dormant fallback reserve (reserveBasedThreshold), with a bridging test tying them. reserveBasedThreshold is unreachable for kimi/codex while the fallback set stays empty.
0584855 to
84a4942
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
6d47aa7 to
84a4942
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84a49429af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { provider: 'kimi', model: 'kimi-k2.7-code', contextWindow: 262_144, sdkMaxTokens: 200_000 }, | ||
| { provider: 'kimi', model: 'kimi-for-coding', contextWindow: 262_144, sdkMaxTokens: 200_000 }, | ||
| { provider: 'kimi', model: 'kimi-k3', contextWindow: 1_048_576, sdkMaxTokens: 1_048_576 }, |
There was a problem hiding this comment.
When the selected route is Kimi K3 or K2.7 Highspeed, this “every model” matrix never exercises the production model ID: KimiProvider.MODELS exposes kimi-k3[1m] and kimi-k2.7-code-highspeed, while the matrix substitutes the bare K3 alias and omits Highspeed entirely. A regression limited to either canonical route could therefore leave this regression suite green; derive the Kimi entries from KimiProvider.MODELS or add both missing IDs explicitly.
Useful? React with 👍 / 👎.
| ['glm', 'glm'], | ||
| ['openrouter', 'openrouter'], | ||
| ['ollama', 'ollama'], | ||
| ['minimax', 'minimax'], | ||
| ['gemini', 'gemini'], |
There was a problem hiding this comment.
When fallback behavior regresses specifically for ACP or Ollama Cloud, this provider matrix still passes because it omits the registered acp and ollama-cloud provider IDs, while including gemini, which is not a built-in registered by providers/factory.ts. The empty-set assertion only catches changes made through that set; a provider-specific branch added to shouldUseHyperNeoCompactFallback would evade it. Derive these cases from the built-in provider IDs or include the two missing production providers explicitly.
Useful? React with 👍 / 👎.
…Service test
session-lifecycle-sdk-title.test.ts registered a top-level
mock.module('../../../../src/lib/provider-service', ...) whose factory returned
an INCOMPLETE stub (mockProviderService lacked getDefaultProvider/getProviderApiKey).
Bun's mock.module is process-global, so this mock leaked into provider-service.test.ts:
when it won the module cache, getProviderService() returned the stub and the
'getProviderService > should return ProviderService instance' test failed with
getDefaultProvider undefined — but only in the full 1-core shard on Linux CI
(passes in isolation and on macOS), so it presented as an intermittent red-CI flake
on every dev PR since 2026-07-26.
The mock was fully redundant: the test already injects its stub via the
SessionLifecycleConfig.titleGenerationProviderServiceForTesting DI hook
(mockTitleProviderService). Removed the leaking mock.module and the now-unused
mockProviderService object. The file's own design note (top-of-file + line ~85)
already prescribes exactly this — only external packages should be mock.module'd.
The pull_request event was skipped for 00fe257 after rapid force-pushes. This empty commit re-triggers CI to confirm the getProviderService flake is gone.
1 participant
Consolidated regression suite pinning the Kimi + Codex auto-compaction contract (Task #754), plus a fix for a dev-wide CI flake that was blocking every PR since 2026-07-26.
Compaction suite — four contract groups mapping to the recurred defect classes (Forge episode `851158bf`): native settings never disabled (N1), provider-specific reserve thresholds — kimi 45k vs codex 33k (N2), the proactive NeoKai `/compact` fallback kept as a dormant empty-set extension point (N3), and literal `/compact` never entering the transcript/provider request via handler-level gating + the internal-flag transcript-exclusion mechanism (N4).
Flake fix — `session-lifecycle-sdk-title.test.ts` registered a top-level `mock.module('../../../../src/lib/provider-service')` with an incomplete stub (missing `getDefaultProvider`/`getProviderApiKey`). Bun's `mock.module` is process-global, so it leaked into `provider-service.test.ts`: when it won the module cache, `getProviderService()` returned the stub and `getProviderService > should return ProviderService instance` failed with `getDefaultProvider` undefined — but only in the full 1-core shard on Linux CI (passes in isolation and on macOS), so it surfaced as an intermittent red-CI flake. The mock was fully redundant (the test already injects its stub via the `titleGenerationProviderServiceForTesting` DI hook), so it and its unused stub object were removed. The file's own design notes already prescribe this.