Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughTruncated structured-output handling now recovers root-aligned partial JSON, validates recovered values against schemas, preserves Anthropic fallback text, and prevents schema-rejected strings or scalars from becoming ChangesStructured-output recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant AnthropicProvider
participant GenerationHandler
participant recoverStructuredData
participant coerceJsonToSchema
participant Schema
AnthropicProvider->>GenerationHandler: return tool input or fallback text
GenerationHandler->>recoverStructuredData: finalize structured output
recoverStructuredData->>coerceJsonToSchema: recover and select JSON candidate
coerceJsonToSchema->>Schema: validate candidate
Schema-->>coerceJsonToSchema: accept or reject
coerceJsonToSchema-->>recoverStructuredData: value and truncation status
recoverStructuredData-->>GenerationHandler: structuredData or content-only text
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/neurolink.ts (1)
5506-5543: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared scalar-recovery logic.
This scalar fallback duplicates
coerceTextModeinsrc/lib/core/modules/GenerationHandler.ts: the sameJSON.parse, the same empty-string normalization, the sameschemaAcceptsgate, and the same three warning messages. Two copies of this policy will drift when the recovery rules change again.Extract a shared helper next to
coerceJsonToSchema(for examplerecoverScalarRoot(text, schema)) that returns the decision, and let each caller apply it to its own result shape and logger prefix.🤖 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 `@src/lib/neurolink.ts` around lines 5506 - 5543, Extract the duplicated JSON scalar-recovery policy from the shown fallback and GenerationHandler.coerceTextMode into a shared helper located beside coerceJsonToSchema, such as recoverScalarRoot(text, schema). Have the helper perform JSON parsing, empty-string normalization, and schemaAccepts validation while returning the recovery decision; update both callers to apply that decision to their own result objects and logger prefixes, preserving the existing warning behavior.
🤖 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/lib/core/modules/GenerationHandler.ts`:
- Around line 1150-1166: Update GenerationHandler’s scalar recovery at
src/lib/core/modules/GenerationHandler.ts:1150-1166 and raw-string recovery at
src/lib/core/modules/GenerationHandler.ts:1192-1208 so coerceTextMode never
assigns schema-rejected partial values to structuredData, matching the
schemaAccepts contract; document the chosen public destination for partial
recovery in CLAUDE.md:30, stating whether it remains outside structuredData or
uses a separate result field.
In `@test/continuous-test-suite-coerce-truncation.ts`:
- Around line 70-97: Remove recovered payload values from the assertion
diagnostics in the truncation test: stop adding structured data details to
degraded and unflagged messages, and use fixed messages in the assertEqual
calls. Preserve the existing counts and failure conditions while ensuring
provider-like payload content cannot appear in assertion messages.
---
Nitpick comments:
In `@src/lib/neurolink.ts`:
- Around line 5506-5543: Extract the duplicated JSON scalar-recovery policy from
the shown fallback and GenerationHandler.coerceTextMode into a shared helper
located beside coerceJsonToSchema, such as recoverScalarRoot(text, schema). Have
the helper perform JSON parsing, empty-string normalization, and schemaAccepts
validation while returning the recovery decision; update both callers to apply
that decision to their own result objects and logger prefixes, preserving the
existing warning behavior.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 4118ac4f-da64-450c-a3f0-f464f47c5e3a
📒 Files selected for processing (7)
CLAUDE.mdpackage.jsonsrc/lib/core/modules/GenerationHandler.tssrc/lib/neurolink.tssrc/lib/providers/anthropic/client.tssrc/lib/utils/json/coerce.tstest/continuous-test-suite-coerce-truncation.ts
mansiverma897993
force-pushed
the
fix/1156-truncated-structured-output
branch
from
August 8, 2026 05:01
e09d420 to
8b814aa
Compare
murdore
force-pushed
the
fix/1156-truncated-structured-output
branch
2 times, most recently
from
August 14, 2026 19:15
cb231b8 to
5e6c584
Compare
murdore
reviewed
Aug 14, 2026
| schemaAccepts, | ||
| } from "../src/lib/utils/json/coerce.js"; | ||
|
|
||
| const { test, runSuite } = defineSuite("Truncated structured-output recovery"); |
Contributor
There was a problem hiding this comment.
@mansiverma897993 we have updated all the test cases that we run. We have moved away from unit test case format to end-to-end testing format.
Let me check the other test cases and see how this can be tested using the generate and stream command.
It would require a rewrite of this test case file, or maybe adding this one test case to other test files to verify that this change is actually fixing the issue.
Contributor
There was a problem hiding this comment.
Followed up on this properly — and I want to correct my own suggestion.
A straight rewrite into the e2e format would be the wrong move. The value of this suite is the prefix sweep: it walks every cut point from 1 to the full payload length and asserts the contract at each one. That is what found the real bug — the balanced-span scan matching a bracket pair inside a string value, turning [step 1] in a shell script into a syntactically valid but semantically bogus ["step 1"], reported with truncated: false. Silently wrong, at ~22% of cut points.
You cannot get that from generate/stream. A live model cannot be made to cut at 400 specific byte offsets; you would get one arbitrary cut per run and a suite whose coverage changes every time it executes. So this stays a pure suite — the sweep is the point, and it is cheap and deterministic.
What is genuinely missing is one live assertion that the fix reaches the public surface, and there is already a natural home for it. test/continuous-test-suite-json-e2e.ts has huge-output cells and today asserts the result is NOT truncated (line 18). Add a cell alongside them that forces the opposite: same huge-output prompt, generate({ schema, maxTokens: <small enough to guarantee a cut> }), then assert
structuredDatais a plain object and never a string — that is the exact regression thehuge-outputcell originally caught on the Anthropic-direct path, andjsonTruncated === true— the flag is onGenerateResult(src/lib/types/generate.ts:1025-1031), so a caller can actually distinguish a partial object from a complete one.
That splits the work correctly: exhaustive cut-point coverage stays pure and deterministic here, while the claim that a user of generate sees an object plus a truthful flag gets proven through the real path. No rewrite of this file needed — one added cell in the e2e suite.
Separately, and worth doing regardless of the above: CodeRabbit's other comment on this file is right, and it is a specific documented hazard rather than a style nit. defineSuite's test() classifies a thrown error as SKIP when its message matches isExpectedProviderError(). An assertion message that quotes the recovered payload can therefore contain something like stream_error or a status code and a genuine failure gets reported as ⊘ skipped with the run still exiting 0. It is written up in CLAUDE.md under "Keep payloads out of assertion messages" — describe the discrepancy, do not interpolate the value.
Contributor
|
@mansiverma897993 — this is CLEAN and rebased onto current 1. Payload values in assertion messages (thread) — the one I would do first, because it can hide real failures. 2. The recovery contract (thread) — CodeRabbit's original finding was too strict and it has since agreed. Keep publishing the partial object; that is the point of #1156. What needs changing is the CLAUDE.md sentence that has become untrue. Suggested wording is in my reply — the short version is that Also still open from my side: the suite is a pure prefix sweep and should stay that way — a live model cannot be made to cut at 400 specific offsets. What is missing is one cell in |
mansiverma897993
added a commit
to mansiverma897993/neurolink
that referenced
this pull request
Aug 15, 2026
Apply maintainer feedback on juspay#1304 (fixes juspay#1156): - keep cut position in truncation-suite assertion messages, drop recovered payload values (a payload fragment can match isExpectedProviderError() and downgrade a real failure to SKIP); verified by breaking an assertion and confirming it reports a failure and exits non-zero - document the recovery contract accurately: recovered structuredData is a plain object, not necessarily schema-valid, and jsonTruncated tells a caller a salvaged object from a complete one - add a test:json-e2e cell on the direct Anthropic path forcing a maxTokens cut and asserting structuredData is a plain object with jsonTruncated === true - extract the duplicated scalar-recovery policy into a shared recoverScalarRoot helper beside coerceJsonToSchema; ScalarRecoveryDecision lives in src/lib/types/utilities.ts
Contributor
Author
|
@murdore I have updated according to your suggestion can you plz take a look on it !! |
On the native Anthropic-direct path a max_tokens cut left `structuredData` as a string instead of the schema object. When output is truncated the root brace never closes, so the balanced-span scan walks past it and matches a bracket pair living INSIDE a string value - `[step 1]` in a shell script becomes `["step 1"]` - reported with `truncated: false`. At other cut points nothing parsed at all, coerceJsonToSchema returned null, and the caller kept the raw text. A prefix sweep over a realistic huge-output payload hit the first case at ~22% of cut points and the second at a handful more. coerce: mark candidates that start at the document's first opening bracket and order those first, so the partial real root beats a span scraped from inside a string; flag every candidate truncated when the root never closes; and when an unclosed root yields nothing trustworthy, back off to the last completed structural boundary and repair from there, returning a PARTIAL object rather than degrading to raw text. consumers: add schemaAccepts and gate structuredData on it - a scalar root, or an experimental_output string that is not an exact raw-text echo, is only published when the caller's schema accepts it. neurolink also re-runs recovery when a provider already produced a schema-rejected string. String-root schemas are unaffected. The shared scalar-recovery policy lives in recoverScalarRoot (with ScalarRecoveryDecision in src/lib/types), used by both neurolink.recoverStructuredData and GenerationHandler.coerceTextMode so it cannot drift. anthropic: forced-json mode drops text blocks because the payload rides in the synthetic tool's input; when the response is cut short that tool call can be missing entirely, leaving an empty completion. Keep the text as a fallback when no synthetic tool call arrived, so a partial object can still be salvaged. final_result still supersedes it. Adds test:coerce-truncation (8 tests) sweeping every truncation point of a huge-output payload, with assertion messages restricted to structural cut positions (never recovered payload values) so a genuine failure reports as FAIL, not SKIP. Adds a test:json-e2e cell on the direct Anthropic path that forces a maxTokens cut and asserts structuredData is a plain object with jsonTruncated === true. Fixes juspay#1156. Refs juspay#635, juspay#1152.
mansiverma897993
force-pushed
the
fix/1156-truncated-structured-output
branch
from
August 15, 2026 15:33
3678b43 to
87b20df
Compare
Contributor
|
@mansiverma897993 — merged as The fix itself is solid. I verified it rather than trusting the green run: built a worktree at your head commit, confirmed the recovery behaves, then deliberately broke Two things I want to flag, both my fault rather than yours: The On the test format — you were following my instruction, and my instruction was wrong. My earlier comment on this thread told you to keep the prefix-sweep as a standalone suite and add one e2e cell. That contradicted the direction already given on this PR: this repo has moved from unit-test format to end-to-end testing format. You did what I asked, so the mismatch is on me, not you. I'm raising a follow-up PR myself to remove Nothing for you to do here. Thanks again. |
Contributor
|
🎉 This PR is included in version 10.12.9 🎉 The release is available on: Your semantic-release bot 📦🚀 |
murdore
added a commit
that referenced
this pull request
Aug 15, 2026
This repo has moved from unit-test format to end-to-end testing format. `continuous-test-suite-coerce-truncation.ts` called `coerceJsonToSchema()` directly, which is the format we are moving away from. It landed in #1304 because a review comment of mine told the contributor to keep it — that comment contradicted the direction already given on the PR, so this removes it rather than leaving the contributor to undo my mistake. The truncation contract is still covered. #1304 also added a live cell to `continuous-test-suite-json-e2e.ts` that drives a real `generate()` with a small `maxTokens` to force a cut mid-JSON, then asserts `structuredData` is a plain object (never the raw string, which was the #1156 regression) and `jsonTruncated === true`. That exercises the same contract through the public surface. Removing the sweep does cost coverage: it walked every cut point from 1 to the payload length, and that exhaustiveness is what originally caught the balanced-span scan matching a bracket pair inside a string value. A live model cannot be made to cut at specific byte offsets, so that particular coverage does not transfer. The script had never been added to `test:unit`, so nothing in CI changes.
murdore
added a commit
that referenced
this pull request
Aug 15, 2026
Every test should exercise a surface this package ships. A suite that imports a module out of `src/lib/` and asserts on it directly tests an internal shape that is free to change, and it does not tell us whether a caller can reach the behaviour at all. Removes 80 suites that never construct `NeuroLink`, never call `generate()` / `stream()`, and never drive the built CLI, plus `envGuard.test.ts` and the orphaned `knowledgeGrounding.test.ts` — the latter had no npm script and never had one, so nothing has ever run it. Adds CLAUDE.md rule 15 stating the convention, so the next contribution does not re-add one. This is the same rule #1304 tripped over, where a review comment told the contributor to keep a unit suite. Conversions in the suites that mixed both styles: - `agents-live` borrowed `withTimeout` from `src/lib/` to bound its own test functions. That is plumbing, not a behaviour under test; replaced with a local `withDeadline`. - `model-not-found-retryable` asserted directly on `looksLikeModelNotFound` and `isNonRetryableForPool` across 19 cases. Those are dropped; the two tests that drive `NeuroLink.generate()` through a ModelPool and prove the failover actually happens are what remain. - Suites that only ever imported the public class now take it from `../dist/index.js`, the shipped entry, matching the 31 suites that already did. `NeuroLink` is deliberately NOT repointed to `dist` in suites that stub an internal (`mcp-result-cache`, `model-pool`, `model-not-found-retryable`). The stub patches the module in the `src` graph; a `NeuroLink` built from `dist` is a separate bundled copy, so the stub goes inert and the test starts making real network calls. Caught by running them — the typecheck is clean either way, and `model-not-found-retryable` went from 0.01s to 45s with one silent skip and one failure. package.json: 63 dead scripts removed and the `test:unit`, `test:mcp:full` and `test:multimodal` aggregates rebuilt to reference only surviving scripts. `test:unit` keeps its name — it describes a cost tier (free, no live API calls), not a unit-testing tier. Coverage genuinely lost, recorded in the docs rather than left to be discovered: - ssrf (40 cases) — SSRF bypass categories, DNS rebinding, encoded IPv4 - log-sanitize (41 cases) — token formats, record/header redaction - stream-span (105 cases) — span lifetime, recordException ordering, and the sweep that caught providers using `withClientSpan` on stream paths - envGuard.test.ts — the self-check that every skip pattern had a fixture, which is what kept `isExpectedProviderError` from rotting - knowledgeGrounding — the knowledge feature now has no coverage at all, though it had none in practice before either The primitives and their eslint rules are untouched; what is gone is the tests that proved they hold. SAFETY-PRIMITIVES.md, CHECKLIST.md, adding-tests.md, test/README.md and envGuard.ts now say so at each point where they previously promised a passing suite. Verified: `pnpm run check` exit 0, `pnpm run lint` 0 errors, `pnpm run build` clean, every remaining test script resolves to a file that exists, and the suites touched here pass — mcp:infra 84/84, archive:security 6/6, office:security 9/9, model-pool 70/70, mcp-result-cache 5/5, model-not-found-retryable 2/2.
murdore
added a commit
that referenced
this pull request
Aug 15, 2026
Every test should exercise a surface this package ships. A suite that imports a module out of `src/lib/` and asserts on it directly tests an internal shape that is free to change, and it does not tell us whether a caller can reach the behaviour at all. Removes 80 suites that never construct `NeuroLink`, never call `generate()` / `stream()`, and never drive the built CLI, plus `envGuard.test.ts` and the orphaned `knowledgeGrounding.test.ts` — the latter had no npm script and never had one, so nothing has ever run it. Adds CLAUDE.md rule 15 stating the convention, so the next contribution does not re-add one. This is the same rule #1304 tripped over, where a review comment told the contributor to keep a unit suite. Conversions in the suites that mixed both styles: - `agents-live` borrowed `withTimeout` from `src/lib/` to bound its own test functions. That is plumbing, not a behaviour under test; replaced with a local `withDeadline`. - `model-not-found-retryable` asserted directly on `looksLikeModelNotFound` and `isNonRetryableForPool` across 19 cases. Those are dropped; the two tests that drive `NeuroLink.generate()` through a ModelPool and prove the failover actually happens are what remain. - Suites that only ever imported the public class now take it from `../dist/index.js`, the shipped entry, matching the 31 suites that already did. `NeuroLink` is deliberately NOT repointed to `dist` in suites that stub an internal (`mcp-result-cache`, `model-pool`, `model-not-found-retryable`). The stub patches the module in the `src` graph; a `NeuroLink` built from `dist` is a separate bundled copy, so the stub goes inert and the test starts making real network calls. Caught by running them — the typecheck is clean either way, and `model-not-found-retryable` went from 0.01s to 45s with one silent skip and one failure. package.json: 63 dead scripts removed and the `test:unit`, `test:mcp:full` and `test:multimodal` aggregates rebuilt to reference only surviving scripts. `test:unit` keeps its name — it describes a cost tier (free, no live API calls), not a unit-testing tier. Coverage genuinely lost, recorded in the docs rather than left to be discovered: - ssrf (40 cases) — SSRF bypass categories, DNS rebinding, encoded IPv4 - log-sanitize (41 cases) — token formats, record/header redaction - stream-span (105 cases) — span lifetime, recordException ordering, and the sweep that caught providers using `withClientSpan` on stream paths - envGuard.test.ts — the self-check that every skip pattern had a fixture, which is what kept `isExpectedProviderError` from rotting - knowledgeGrounding — the knowledge feature now has no coverage at all, though it had none in practice before either The primitives and their eslint rules are untouched; what is gone is the tests that proved they hold. SAFETY-PRIMITIVES.md, CHECKLIST.md, adding-tests.md, test/README.md and envGuard.ts now say so at each point where they previously promised a passing suite. Verified: `pnpm run check` exit 0, `pnpm run lint` 0 errors, `pnpm run build` clean, every remaining test script resolves to a file that exists, and the suites touched here pass — mcp:infra 84/84, archive:security 6/6, office:security 9/9, model-pool 70/70, mcp-result-cache 5/5, model-not-found-retryable 2/2.
murdore
added a commit
that referenced
this pull request
Aug 15, 2026
Every test should exercise a surface this package ships. A suite that imports a module out of `src/lib/` and asserts on it directly tests an internal shape that is free to change, and it does not tell us whether a caller can reach the behaviour at all. Removes 80 suites that never construct `NeuroLink`, never call `generate()` / `stream()`, and never drive the built CLI, plus `envGuard.test.ts` and the orphaned `knowledgeGrounding.test.ts` — the latter had no npm script and never had one, so nothing has ever run it. Adds CLAUDE.md rule 15 stating the convention, so the next contribution does not re-add one. This is the same rule #1304 tripped over, where a review comment told the contributor to keep a unit suite. Conversions in the suites that mixed both styles: - `agents-live` borrowed `withTimeout` from `src/lib/` to bound its own test functions. That is plumbing, not a behaviour under test; replaced with a local `withDeadline`. - `model-not-found-retryable` asserted directly on `looksLikeModelNotFound` and `isNonRetryableForPool` across 19 cases. Those are dropped; the two tests that drive `NeuroLink.generate()` through a ModelPool and prove the failover actually happens are what remain. - Suites that only ever imported the public class now take it from `../dist/index.js`, the shipped entry, matching the 31 suites that already did. `NeuroLink` is deliberately NOT repointed to `dist` in suites that stub an internal (`mcp-result-cache`, `model-pool`, `model-not-found-retryable`). The stub patches the module in the `src` graph; a `NeuroLink` built from `dist` is a separate bundled copy, so the stub goes inert and the test starts making real network calls. Caught by running them — the typecheck is clean either way, and `model-not-found-retryable` went from 0.01s to 45s with one silent skip and one failure. package.json: 63 dead scripts removed and the `test:unit`, `test:mcp:full` and `test:multimodal` aggregates rebuilt to reference only surviving scripts. `test:unit` keeps its name — it describes a cost tier (free, no live API calls), not a unit-testing tier. Coverage genuinely lost, recorded in the docs rather than left to be discovered: - ssrf (40 cases) — SSRF bypass categories, DNS rebinding, encoded IPv4 - log-sanitize (41 cases) — token formats, record/header redaction - stream-span (105 cases) — span lifetime, recordException ordering, and the sweep that caught providers using `withClientSpan` on stream paths - envGuard.test.ts — the self-check that every skip pattern had a fixture, which is what kept `isExpectedProviderError` from rotting - knowledgeGrounding — the knowledge feature now has no coverage at all, though it had none in practice before either The primitives and their eslint rules are untouched; what is gone is the tests that proved they hold. SAFETY-PRIMITIVES.md, CHECKLIST.md, adding-tests.md, test/README.md and envGuard.ts now say so at each point where they previously promised a passing suite. Verified: `pnpm run check` exit 0, `pnpm run lint` 0 errors, `pnpm run build` clean, every remaining test script resolves to a file that exists, and the suites touched here pass — mcp:infra 84/84, archive:security 6/6, office:security 9/9, model-pool 70/70, mcp-result-cache 5/5, model-not-found-retryable 2/2.
murdore
added a commit
that referenced
this pull request
Aug 15, 2026
Every test should exercise a surface this package ships. A suite that imports a module out of `src/lib/` and asserts on it directly tests an internal shape that is free to change, and it does not tell us whether a caller can reach the behaviour at all. Removes 80 suites that never construct `NeuroLink`, never call `generate()` / `stream()`, and never drive the built CLI, plus `envGuard.test.ts` and the orphaned `knowledgeGrounding.test.ts` — the latter had no npm script and never had one, so nothing has ever run it. Adds CLAUDE.md rule 15 stating the convention, so the next contribution does not re-add one. This is the same rule #1304 tripped over, where a review comment told the contributor to keep a unit suite. Conversions in the suites that mixed both styles: - `agents-live` borrowed `withTimeout` from `src/lib/` to bound its own test functions. That is plumbing, not a behaviour under test; replaced with a local `withDeadline`. - `model-not-found-retryable` asserted directly on `looksLikeModelNotFound` and `isNonRetryableForPool` across 19 cases. Those are dropped; the two tests that drive `NeuroLink.generate()` through a ModelPool and prove the failover actually happens are what remain. - Suites that only ever imported the public class now take it from `../dist/index.js`, the shipped entry, matching the 31 suites that already did. `NeuroLink` is deliberately NOT repointed to `dist` in suites that stub an internal (`mcp-result-cache`, `model-pool`, `model-not-found-retryable`). The stub patches the module in the `src` graph; a `NeuroLink` built from `dist` is a separate bundled copy, so the stub goes inert and the test starts making real network calls. Caught by running them — the typecheck is clean either way, and `model-not-found-retryable` went from 0.01s to 45s with one silent skip and one failure. package.json: 63 dead scripts removed and the `test:unit`, `test:mcp:full` and `test:multimodal` aggregates rebuilt to reference only surviving scripts. `test:unit` keeps its name — it describes a cost tier (free, no live API calls), not a unit-testing tier. Coverage genuinely lost, recorded in the docs rather than left to be discovered: - ssrf (40 cases) — SSRF bypass categories, DNS rebinding, encoded IPv4 - log-sanitize (41 cases) — token formats, record/header redaction - stream-span (105 cases) — span lifetime, recordException ordering, and the sweep that caught providers using `withClientSpan` on stream paths - envGuard.test.ts — the self-check that every skip pattern had a fixture, which is what kept `isExpectedProviderError` from rotting - knowledgeGrounding — the knowledge feature now has no coverage at all, though it had none in practice before either The primitives and their eslint rules are untouched; what is gone is the tests that proved they hold. SAFETY-PRIMITIVES.md, CHECKLIST.md, adding-tests.md, test/README.md and envGuard.ts now say so at each point where they previously promised a passing suite. Verified: `pnpm run check` exit 0, `pnpm run lint` 0 errors, `pnpm run build` clean, every remaining test script resolves to a file that exists, and the suites touched here pass — mcp:infra 84/84, archive:security 6/6, office:security 9/9, model-pool 70/70, mcp-result-cache 5/5, model-not-found-retryable 2/2.
murdore
added a commit
that referenced
this pull request
Aug 18, 2026
Rule 15 — tests are end-to-end only — was documented in a47c435 but only checked by review, and review is what let the original violation through: on #1304 a review comment of mine told a contributor to keep a unit suite, and nothing contradicted it. Adds `neurolink/e2e-tests-only`, an AST rule over files under `test/`. It flags a RUNTIME import from `src/lib/` or `src/cli/` in either form: import { FileDetector } from "../src/lib/utils/fileDetector.js"; const { X } = await import("../src/lib/whatever.js"); It deliberately does not flag: - type-only imports — `import type { Tool } from …`, and `import { type A, type B } from …` where every specifier is type-only. They are erased at compile time and assert nothing. - anything from `../dist/`, which is what callers actually load. - files on the `allow` list. `allow` is the determinism exception, and it lives in eslint.config.js with a one-line reason per entry: rag (chunk boundaries and reranker ordering), bugfixes (parser edge cases and outgoing wire format), proxy (429-cooldown and quota ordering), autoresearch (a task system with no public surface), and the chroma/pinecone filter translators. Adding to it is a review decision and the file's own header must say what determinism buys. The rule's message points at the fix rather than just the violation, and warns against the trap that cost three silent failures while writing a47c435: moving an import to `../dist/` in a file that also stubs or spies on that module makes the stub patch a different bundled copy, so the test starts doing real work while still typechecking clean. ## What it found on its first run One file, and it turns out to be a good sign rather than a bad one: `continuous-test-suite-error-classifier-contract.ts`, added in 5502259 after rule 15 landed. Its header already cites rule 15, already declares the determinism exception, already pins the all-src module graph, and justifies each category — synthetic rule tables, duck-typed error shapes no AWS SDK actually produces, module-export-shape checks. So it is allowlisted rather than converted; the convention was applied correctly without the rule existing yet. Verified by breaking it on purpose: a probe file importing a value from src/lib statically AND dynamically reports both, while `import type`, `{ type X }` and a `../dist/` import in the same file report nothing. `pnpm run lint` 0 errors, `pnpm run check` exit 0.
murdore
added a commit
that referenced
this pull request
Aug 18, 2026
Rule 15 — tests are end-to-end only — was documented in a47c435 but only checked by review, and review is what let the original violation through: on #1304 a review comment of mine told a contributor to keep a unit suite, and nothing contradicted it. Adds `neurolink/e2e-tests-only`, an AST rule over files under `test/`. It flags a RUNTIME import from `src/lib/` or `src/cli/` in either form: import { FileDetector } from "../src/lib/utils/fileDetector.js"; const { X } = await import("../src/lib/whatever.js"); It deliberately does not flag: - type-only imports — `import type { Tool } from …`, and `import { type A, type B } from …` where every specifier is type-only. They are erased at compile time and assert nothing. - anything from `../dist/`, which is what callers actually load. - files on the `allow` list. `allow` is the determinism exception, and it lives in eslint.config.js with a one-line reason per entry: rag (chunk boundaries and reranker ordering), bugfixes (parser edge cases and outgoing wire format), proxy (429-cooldown and quota ordering), autoresearch (a task system with no public surface), and the chroma/pinecone filter translators. Adding to it is a review decision and the file's own header must say what determinism buys. The rule's message points at the fix rather than just the violation, and warns against the trap that cost three silent failures while writing a47c435: moving an import to `../dist/` in a file that also stubs or spies on that module makes the stub patch a different bundled copy, so the test starts doing real work while still typechecking clean. ## What it found on its first run One file, and it turns out to be a good sign rather than a bad one: `continuous-test-suite-error-classifier-contract.ts`, added in 5502259 after rule 15 landed. Its header already cites rule 15, already declares the determinism exception, already pins the all-src module graph, and justifies each category — synthetic rule tables, duck-typed error shapes no AWS SDK actually produces, module-export-shape checks. So it is allowlisted rather than converted; the convention was applied correctly without the rule existing yet. Verified by breaking it on purpose: a probe file importing a value from src/lib statically AND dynamically reports both, while `import type`, `{ type X }` and a `../dist/` import in the same file report nothing. `pnpm run lint` 0 errors, `pnpm run check` exit 0.
murdore
added a commit
that referenced
this pull request
Aug 18, 2026
Rule 15 — tests are end-to-end only — was documented in a47c435 but only checked by review, and review is what let the original violation through: on #1304 a review comment of mine told a contributor to keep a unit suite, and nothing contradicted it. Adds `neurolink/e2e-tests-only`, an AST rule over files under `test/`. It flags a RUNTIME import from `src/lib/` or `src/cli/` in either form: import { FileDetector } from "../src/lib/utils/fileDetector.js"; const { X } = await import("../src/lib/whatever.js"); It deliberately does not flag: - type-only imports — `import type { Tool } from …`, and `import { type A, type B } from …` where every specifier is type-only. They are erased at compile time and assert nothing. - anything from `../dist/`, which is what callers actually load. - files on the `allow` list. `allow` is the determinism exception, and it lives in eslint.config.js with a one-line reason per entry: rag (chunk boundaries and reranker ordering), bugfixes (parser edge cases and outgoing wire format), proxy (429-cooldown and quota ordering), autoresearch (a task system with no public surface), and the chroma/pinecone filter translators. Adding to it is a review decision and the file's own header must say what determinism buys. The rule's message points at the fix rather than just the violation, and warns against the trap that cost three silent failures while writing a47c435: moving an import to `../dist/` in a file that also stubs or spies on that module makes the stub patch a different bundled copy, so the test starts doing real work while still typechecking clean. ## What it found on its first run One file, and it turns out to be a good sign rather than a bad one: `continuous-test-suite-error-classifier-contract.ts`, added in 5502259 after rule 15 landed. Its header already cites rule 15, already declares the determinism exception, already pins the all-src module graph, and justifies each category — synthetic rule tables, duck-typed error shapes no AWS SDK actually produces, module-export-shape checks. So it is allowlisted rather than converted; the convention was applied correctly without the rule existing yet. Verified by breaking it on purpose: a probe file importing a value from src/lib statically AND dynamically reports both, while `import type`, `{ type X }` and a `../dist/` import in the same file report nothing. `pnpm run lint` 0 errors, `pnpm run check` exit 0.
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
2 participants
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.
Fixes #1156.
The bug
On the native Anthropic-direct structured path, a
max_tokenscut leftstructuredDataas a string instead of the schema object.When output is truncated the root brace never closes, so
nextBalancedJsonSpanwalks past the root and matches a bracket pair living inside a string value[step 1]in a shell script becomes["step 1"]which is returned asstructuredDatawithtruncated: false. At other cut points nothing parsed at all,coerceJsonToSchemareturnednull, and the caller kept the raw text (a string).Reproduced deterministically with a prefix sweep over the e2e
huge-outputpayload ({summary, attachment{...content}}, shell script with[step N]markers), onecoerceJsonToSchemacall per cut point:null(caller keeps raw string)structuredDatanot a plain objectThe fix
coerce.tsmark candidates that start at the document's first opening bracket and order those first, so the partial real root beats a span scraped from inside a string; flag every candidatetruncatedwhen the root never closes; and when an unclosed root yields nothing trustworthy, back off to the last completed structural boundary and repair from there, returning a partial object rather than degrading to raw text.Consumers (
neurolink.ts,GenerationHandler.ts) newschemaAcceptsgate: a scalar root, or anexperimental_outputstring that is not an exact raw-text echo, is only published asstructuredDatawhen the caller's schema accepts it.neurolinkalso re-runs recovery when a provider already produced a schema-rejected string. String-root schemas (z.string()) are unaffected.anthropic/client.tsforced-json mode drops text blocks because the payload rides in the synthetic tool's input; when the response is cut short that tool call can be missing entirely, leaving an empty completion with nothing to recover. Text is now kept as a fallback when no synthetic tool call arrived.final_resultstill supersedes it.Test report
New suite
pnpm run test:coerce-truncation(8 tests) sweeps every truncation point of a huge-output payload and asserts a plain object +truncated: trueat each one, plus theschemaAcceptscontract.Verified red → green: against the pre-fix
coerce.tsthe suite fails 2/8 with exactly thenulland["step 1"]degradations above.bugfixesfailures are pre-existing and environmental (Windows symlinkEPERM, missingdist/clibuild, proxy process checks); the failing-test set was diffed against a cleanupstream/releasecheckout and is identical.tsc --noEmitclean andeslintat 0 errors / same warning count as baseline on all four touched files.Not covered here: the live
test:json-e2ehuge-outputcell needs provider credentials, so this was verified at the unit level against the exact text shapes that path produces.Summary by CodeRabbit
Bug Fixes
Tests