Skip to content

fix(llm): bound complete serialized inputs before provider calls - #980

Open
Patrick-Erichsen wants to merge 5 commits into
NVIDIA:mainfrom
Patrick-Erichsen:fix-llm-input-budget
Open

Patrick-Erichsen wants to merge 5 commits into
NVIDIA:mainfrom
Patrick-Erichsen:fix-llm-input-budget

Conversation

@Patrick-Erichsen

@Patrick-Erichsen Patrick-Erichsen commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

HTTP batching can undercount short lines and omit serialized prompt overhead, producing requests larger than the model budget. Count the complete prompt with the configured or recognized tokenizer, use a conservative UTF-8 byte bound for unknown or unloaded codecs, and refine line chunks until they fit. Check again before synchronous, asynchronous and formatting calls; retain input that cannot fit as an explicit unsubmitted failure.

Every original line, finding and overlapping boundary remains represented. Fixed prompts that cannot fit are refused without expensive per-line refinement. Codec use stays offline, and equivalent clients reuse their chosen counter. Findings that depend on the line range are checked by the exact chunk predicate. Configured budgets, retries and deadlines are unchanged.

Validation: the short-line regression fails on unchanged main and passes here. 902 related tests pass (five integration tests deselected), repository Ruff lint passes, and all 337 files pass formatting. Credential-redaction tests use real constructor state behind an inert provider and preserve every secret-absence assertion. The formatting test allows the first mocked request, then lowers the allowance and proves that formatting cannot submit a second request. Both tests also pass independently. Fresh full CI remains required.

A model-free planner replay of the six original files from openclaw/clawhub#4214 preserves all 77,584 bundled-code lines and bounds serialized input at the configured 787,500 limit. This replay is supporting evidence; downstream deployment and package recovery have separate validation. Focused mypy retains one verified pre-existing SecretStr/callable-union error from the untouched base.

Fixes #977.

Signed-off-by: Patrick Erichsen <patrick.a.erichsen@gmail.com>
Signed-off-by: Patrick Erichsen <patrick.a.erichsen@gmail.com>
Signed-off-by: Patrick Erichsen <patrick.a.erichsen@gmail.com>
The redaction test bypassed constructor state required by serialized token accounting. Construct it with the existing inert provider fixture while retaining credential redaction assertions.

The formatting-limit test set the input budget before the initial request, so the new submission guard correctly stopped before formatting. Reduce the budget after the successful mocked response and prove formatting refuses a second request.

Signed-off-by: Patrick Erichsen <patrick.a.erichsen@gmail.com>

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[SkillSpector Review]

Decision: Approved

Value and readiness

The change addresses #977's demonstrated short-line undercount and unmeasured prompt overhead. It is ready for maintainer/PIC review: planning measures numbered source plus the real prompt/schema, and synchronous, asynchronous and formatting calls recheck against the actual client before invoking a provider. Requests that cannot fit remain explicit unsubmitted failures, rather than silently dropping source.

Material findings

No required changes identified. src/skillspector/llm_analyzer_base.py:169-239 selects a configured counter, a previously loaded recognized codec, or a conservative UTF-8 byte bound without vocabulary downloads. Equivalent refreshed clients retain their selected counter. The revised line planner preserves ranges/overlap and uses range-specific findings when refining chunks; an irreducible fixed wrapper avoids expensive per-line refinement. The final guard at :1455-1492 includes structured-output instructions and framing allowance, and all three invocation paths call it. New assertions cover short/multibyte lines, wrappers/findings/metadata, empty files, unknown/failed codecs, no vocabulary/network initialization, client refresh, unsplittable input without provider calls and formatting refusal; existing credential canaries remain intact.

PIC tradeoffs

The cold/unknown-codec byte fallback intentionally uses a smaller effective text allowance and can create more batches. An oversized single physical line remains an explicit failure in this PR; #992 proposes bounded spans separately. Existing deadlines/retries remain in place, so this planner fix is not evidence that a full real-provider scan always finishes within its budget.

Verification and gaps

Reviewed the complete six-file diff and exact-head planner/invocation/serialization/model-budget and meta-redaction context at 8e1c1c3a1c8f89f119e3c78e8cb685d25d5fe4a3, linked issue #977, all available reviews/discussion/threads (none), and the relevant test assertions. Hosted test-unit reports 17,826 passed and 15 failed; all 15 failure identities match the independently fetched main e14c450 baseline run 41513800480 (PowerShell ownership and quoted-printf cases). Five other checks succeeded. This supports baseline attribution for those observed failures, not a clean CI claim; the red job blocks merging and must become green without a waiver. No contributor code/tests/builds/imports/hooks or regexes were executed locally; real model/package recovery remains unverified.

Preserve main response recovery and failed-request diagnostics. Guard the
new raw recovery calls with the same actual-client serialized limits.
Add sync/async retained-evidence and diagnostic-wrapper regression cases.

Validation: independent static review and git diff --cached --check.
Contributor code and tests were not executed. Hosted CI is required.

Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[SkillSpector Review]

Decision: Approved

Value and readiness: The resolved integration preserves complete serialized-input budgeting while retaining current main's field-by-field response recovery and safe failed-request diagnostics. No required source correction remains after independent review. It is ready for final maintainer/PIC source review; merging remains gated by fresh hosted validation.

Material findings: The additive method conflict is resolved by keeping both method groups. The original actual-client guards remain before normal synchronous, asynchronous and formatting requests. Main added raw recovery request paths since the original approval; both now use the same serialized token/byte guard immediately before the existing diagnostic wrapper. A refused recovery retains previously recovered HIGH evidence and an explicit failure rather than submitting excess input or discarding findings. All twelve relevant main diagnostic, recovery, retry and detailed-aggregation methods remain text-identical except the two guarded raw request paths. Six new regression cases cover sync/async serialized-overhead refusal, refreshed-client refusal, retained HIGH evidence, no provider request or ordinal on refusal, and permitted diagnostic-wrapper/partial-warning behavior. No unresolved code finding identified.

PIC tradeoffs: Unknown/unloaded codecs retain the conservative offline UTF-8 bound. Raw recovery conservatively includes schema/framing in its request allowance and may refuse earlier; it preserves evidence and reports incomplete work. No new product decision identified. Preserve #980's reviewed commit in the permitted merge history so dependent #992 can follow without reintroducing its changes.

Verification and gaps: Combined the prior complete feature assessment with the current main-relative diff, conflict resolution, source traces, independent static review and regression inspection at the reviewed commit. Static AST parsing, Ruff lint/format and Git whitespace checks passed; no contributor application code, tests, imports, setup, hooks, scanner or regex probes were executed. At the review inventory, lint, changes, DCO, TypeScript and docker-smoke succeeded; test-unit was still in progress. The earlier paired 15-failure evidence is not a waiver of CI on this new integration.

Reviewed head: 66bbf9cb62d63bb09bb166459138568becdaeba6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HTTP batches can exceed model input budgets through line rounding and prompt overhead

2 participants