Repository navigation
test(notes): make the LLM e2e infra-tolerant and close a false green - #618
Merged
Merged
Conversation
First half of #589 — the spec change that has to land before the check can block merges. The workflow job and the OLLAMA_API_KEY secret are still outstanding. `isProviderUnreachable` splits the two things a real-provider check can fail on. Only an LLMError the provider positively classified as transient (timeout, 429, 5xx, socket) counts as infra; a non-retryable 4xx is a malformed request, and so is anything that isn't an LLMError — assertion failures and TypeErrors included. An unclassified LLMError fails too: every provider sets the flag explicitly, so an absent one means something unrecognised happened, and a visible red beats a silent skip. The predicate is a separate module with its own always-running spec, since it decides whether the gated check can go red at all. Found while verifying the skip path: the chunk-boundary test passed against an unreachable server in 20ms. enhanceAndCategorize never throws on a provider failure — its per-chunk fallback preserves the input entries — so asserting entry counts holds just as well with no provider at all. The test could not have detected the thing #589 wants it to guard. Two changes close that. A beforeAll probe establishes reachability once, so an absent host skips rather than being inferred from a later assertion; and the chunk test now requires that some descriptions actually differ from their inputs, which the fallback never produces. Verified against a dead port: both tests skip, where one previously reported a false pass. The positive path is unverified locally (no Ollama here) — it needs a run against a reachable model before the check is made blocking. Part of #589. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FADRmu9uhtCk3StMoqwV27
Contributor
|
Standing release PR: #614 · v0.42.0 queued (4 packages) · open 82d 6h 41m · ✅ ready to merge Release Preview — v0.41.2 → v0.41.3
These changes will be added to the release PR (#614) when merged: Changelogmonorepo 0.41.2 → 0.41.3
After merge — predicted release
Updated automatically by ReleaseKit |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
From Greptile review of #618, and correct: the up-front probe cannot see an outage that starts after it. enhanceAndCategorize swallows retryable provider errors and falls back to the input entries verbatim, so a host that dies mid-test produces exactly the same observable as a model that declined to rewrite — and the new rewrite assertion would have failed the check as a product regression. "Nothing was rewritten" now re-probes before being called a regression. The provider is the only thing that can disambiguate, since the fallback is silent by design. Verified both directions against mock Ollama servers, because a fix here can fail either way — too eager and every regression becomes a silent skip: - Answers the probe, then 503s: skips with "SKIPPED mid-run: provider became unreachable", where it previously failed red. - Stays up but returns unparseable output: still fails red ("expected 0 to be greater than 0"), which is the regression the check exists to catch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FADRmu9uhtCk3StMoqwV27
From Greptile's re-review of #618, and right: chunks fall back independently, so a host that dies after the first chunk left ~30 rewrites in place, kept the total above zero, and sailed past the re-probe into a green check — a quieter version of the false green this PR set out to remove. Counting per chunk closes it. Either chunk coming back untouched now triggers the re-probe, which is the correct reading for a test whose subject is the boundary: the point is that both sides of it were enhanced, not that 30 of 35 entries were. Chunk size and entry count now derive from LLM_DEFAULTS rather than a hardcoded 35, so the case keeps straddling the boundary if that constant moves. Verified with a mock that answers the probe, serves the first chunk correctly, then 503s: skips, where before the fix it passed. The healthy-but-broken mock still fails red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FADRmu9uhtCk3StMoqwV27
…ction Addresses the review findings on #618. isProviderUnreachable no longer accepts retryable:true on its own. Providers wrap any unexpected error as retryable, so a TypeError in provider code, a 200 HTML page, a null JSON body, a stream:false regression and a scheme-less base URL all skipped green. An LLMError with no cause is still accepted, because the provider classified it from an HTTP status or its own timeout. When there is a cause, the chain must contain a retryable status, a socket/undici code, or an abort/timeout. New spec cases cover a TypeError cause, a SyntaxError cause, and undici's fetch failed -> ECONNREFUSED. The e2e now wraps the provider in withRetry with isRetryableLLMError, as the pipeline does, so one 503/429/reset/timeout is absorbed and not reported as a regression. Errors that outlive the retries are recorded. An untouched chunk skips only when every recorded error is a proven outage. Otherwise it fails, and the message names the chunk and the errors with their cause chains, or says no provider error was recorded. The up-front probe and the re-probe are removed: the recorded errors answer the same question, and the maxTokens: 1 probe could fail a thinking model red. A 45s per-call timeout and a per-test breaker bound a mid-run hang to one retry budget (about 140s), which fits under the 240s test timeout. Under CI, OLLAMA_BASE_URL must be set. Every unreachable skip emits a ::warning:: annotation. Stale comments are corrected, and the no-op callProvider wrapper around enhanceAndCategorize is removed. Verified against a fake Ollama. Healthy and one-503 runs pass. HTML, null JSON, an injected provider TypeError, a removed stream:false and scheme-less URLs fail red with the cause chain. A dead port skips with ::warning::. A hang after the first chunk skips after about 138s per test. CI=1 with no base URL fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQN3v9Z4f1mu3xK4Z7K44e
…able predicate branches Third-review follow-ups: - Under CI vitest colours the `stdout | <test>` header, leaving escape codes in front of `::warning`, so the runner never parsed it as an annotation — every skip passed silently. Emit it on its own line, and pass the reason to ctx.skip(). - isProviderUnreachable: drop the status-in-cause and abort/timeout-name branches and the undici headers/body timeout codes — the Ollama provider classifies statuses and its own timeout itself, so they were unreachable; add EHOSTUNREACH/EHOSTDOWN, which production retries. Document the inherent blind spot (a provider-classified 5xx or timeout is trusted). - Assert no provider error was recorded when both chunks were rewritten; de-duplicate recorded errors by description; correct the breaker comment; size the test timeout for a chunk that needed corrective calls before a hang. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQN3v9Z4f1mu3xK4Z7K44e
…-test skip decision Fourth-review nits: any recorded provider error already ends its chunk in the verbatim fallback, so asserting none on the pass path guarded nothing. Document that the skip-vs-red call is made per test rather than per chunk. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQN3v9Z4f1mu3xK4Z7K44e
Remove the depth bound on the cause-chain walk (error causes aren't cyclic in practice) and the de-duplication of recorded errors by description, which only trimmed repeated lines from a failure message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQN3v9Z4f1mu3xK4Z7K44e
Without the breaker a full hang skips both tests inside the 330s timeout (measured 276s and 138s). EHOSTUNREACH/EHOSTDOWN aren't in production's NETWORK_ERROR_CODES; if one occurs the test goes red, the safe direction. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQN3v9Z4f1mu3xK4Z7K44e
This was referenced Oct 22, 2026
main now enforces style/useBlockStatements (#679). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQN3v9Z4f1mu3xK4Z7K44e
7 of 20 tasks
Keep the non-obvious why; drop restated mechanics and examples the code already shows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
6 of 20 tasks
Owner
Author
Drop comments that restate test names, test data, assertions or self-explanatory helpers; shorten the rest. The unclassified-error test now uses Ollama's real message instead of a comment pointing at it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
It calls the notes tasks directly with an OllamaProvider, which makes it an integration test against a live service, not an e2e (test/e2e drives the packed CLI). Rename it to ollama-live.spec.ts and RELEASEKIT_NOTES_E2E[_MODEL] to RELEASEKIT_NOTES_LIVE[_MODEL]. Its outage predicate assumes the Ollama provider's error shapes (SDK providers attach HTTP errors as a cause), and this spec is its only user, so it moves into the spec as isOllamaUnreachable. Its tests stay outside the opt-in suite so they run in every gate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merged
5 of 20 tasks
This was referenced Oct 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First half of #589: makes the notes LLM test against live Ollama safe to run as a blocking CI check. The CI job itself is #697.
What changes
Renamed: it isn't an e2e. It calls the notes tasks directly with an
OllamaProvider, so it's an integration test against a live service (test/e2edrives the packed CLI). It's nowollama-live.spec.ts, gated byRELEASEKIT_NOTES_LIVE/RELEASEKIT_NOTES_LIVE_MODEL.Outages skip, regressions fail.
isOllamaUnreachabledecides which is which:LLMErrorwith either no cause (the provider classified it, e.g. a 5xx or its own timeout) or a socket error code somewhere in its cause chain.LLMError, our ownTypeError, an unparseable response.retryablealone isn't enough, because providers mark any unknown error retryable.It's Ollama-specific (the SDK providers attach HTTP errors as a cause) and this spec is its only user, so it lives in the spec. Its tests sit outside the opt-in suite and run in every gate: if it calls something an outage too readily, real regressions skip silently.
A false green, fixed. The chunk-boundary test passed against an unreachable server in 20ms.
enhanceAndCategorizefalls back per chunk and returns entries verbatim, so entry counts hold with no provider at all. Each chunk must now come back with at least one rewritten entry.Retries like production. Calls go through
withRetryas in the pipeline, so a single 503, 429, reset or timeout is absorbed. Errors that survive the retries are recorded. An unchanged chunk skips only if every recorded error proves an outage; otherwise the test fails, naming the chunk and the errors. Each call times out at 45s, so even a host that hangs from the start skips within the 330s test timeout.Skips are visible. Every skip emits a
::warning::annotation. Under CI, a missingOLLAMA_BASE_URLfails instead of skipping against localhost on every run.Verification
Against a local fake Ollama under
CI=true GITHUB_ACTIONS=true:TypeError,streamregression, base URL without a scheme, 401, unknown modelCI=1withoutOLLAMA_BASE_URLAgainst a real model: #697's CI run executed this spec on Ollama Cloud (
minimax-m2.7:cloud). Both tests passed with no skip: chunk test 31.6s, text test 8.7s.Notes suite: 450 passed, 2 skipped (the opt-in live tests). Typecheck and lint clean.
Part of #589.
🤖 Generated with Claude Code
https://claude.ai/code/session_01FADRmu9uhtCk3StMoqwV27
https://claude.ai/code/session_01HQN3v9Z4f1mu3xK4Z7K44e