Conversation
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Hygiene✅ Deterministic PR hygiene checks passed. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: luvs01/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab7f684bfc
ℹ️ 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 normalized = normalizeMoonshotSchemaNode(rooted, rooted, { | ||
| activeRefs: new Set<string>(), | ||
| inlineSizeCache: new WeakMap<Record<string, unknown>, number>(), | ||
| remainingExpansions: MOONSHOT_MAX_REF_EXPANSIONS, | ||
| remainingInlineBytes: MOONSHOT_MAX_INLINED_SCHEMA_BYTES, |
There was a problem hiding this comment.
Share the byte allowance across the whole tool catalog
The allowance is initialized inside normalizeMoonshotToolParameters, so toolsToChatFormat grants a fresh 1 MiB amplification budget to every tool. Because the inbound tool arrays have no aggregate count or output-size limit, a catalog containing many individually small schemas can still exhaust memory while constructing and serializing the final request; for example, I reproduced a 128-tool, 3.36 MB catalog expanding to about 82 MB. Allocate the budget once per request/catalog and pass it through each normalization, or enforce an aggregate serialized-output cap.
Useful? React with 👍 / 👎.
…ication Resolve conflicts from the openai-chat module split: port the serialized-byte inline budget into src/adapters/openai-chat/tool-schema.ts, move the adapter-registry note to structure/decisions/ADR-0093, and keep the moved Moonshot regression tests at tests/providers/moonshot-tool-schema.test.ts. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…test.ts) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Decision records are historical; the byte-budget reasoning moves out of ADR-0093 into ADR-0355 and the chat-compat contract now states the inline-byte bound alongside depth, node, and expansion bounds. Co-Authored-By: Epinephrine <luvs01@hanmail.net>
|
Fixed in 3b8cb5e: ADR-0093 restored verbatim; the byte-budget reasoning now lives in a new record, |
|
Fixed in 3b8cb5e: the "Chat structured-output compatibility" contract now states the walk is depth-, node-, expansion-, and inline-byte-bounded (shared 1 MiB allowance, bare- |
|
Addressed in 3b8cb5e — ADR-0093 is restored byte-for-byte to its original text; the byte-budget rationale now lives in the new ADR-0355 (linked from the owning contract). |
|
Addressed in 3b8cb5e — the contract now states the walk is depth-, node-, expansion-, and inline-byte-bounded (each inlined reference charged its serialized size against a shared 1 MiB allowance, over-budget keeps the bare- |
…t tools A per-tool 1 MiB allowance let a large catalog multiply the cap by its tool count, reopening the request amplification the budget exists to bound. Hoist the allowance to one MoonshotInlineByteBudget per toolsToChatFormat call so every tool spends from the same pool. Adds a regression test: two tools carrying ~0.6 MB inline targets each now share one allowance, so only the first of four sibling refs inlines and the rest keep the bare-$ref fallback. Co-Authored-By: Epinephrine <luvs01@hanmail.net>
|
Fixed in 292e7e1 — the 1 MiB allowance is now one |
Motivation
$defspropertiesmap into many sibling-bearing$refnodes could multiply serialized output and exhaust process memory.$ref-with-siblings semantics (conjunction forrequired/properties, node-narrowing otherwise) while making inlining resource-bounded and safe.Description
MOONSHOT_MAX_INLINED_SCHEMA_BYTES(1 MiB) and an iterative estimatorserializedJsonBytesUpToto bound how many bytes of a referenced schema may be inlined. (src/adapters/openai-chat.ts)remainingInlineBytesandinlineSizeCacheso large referenced targets are measured once and charged before being copied, and reuse cached sizes for repeated references. (src/adapters/openai-chat.ts)$refinstead of duplicating the large map, and keep the existing depth/node/expansion guards. (src/adapters/openai-chat.ts)propertiesdefinition referenced many times and asserts that emitted output stays bounded and that some references remain as bare$ref. (tests/moonshot-tool-schema.test.ts)structure/10_adapter-registry.md)Testing
bun test tests/moonshot-tool-schema.test.ts, which passed (17 tests, 0 failures).bun run typecheck, which completed without type errors.bun run privacy:scan, which passed.bun run testwas not used for acceptance here because unrelated, pre-existing management/auth tests timed out/failed in this environment; the change was validated by the focused tests above.Codex Task