glm-5.3-flash
0a8d4d731f
resolve_refs_recursive recursed with no cycle detection and no depth
budget; a self-referential OpenAPI component stack-overflowed and
aborted the process (uncatchable, kills import()).
- add branch-scoped visited set on the JSON-pointer ref path: a ref
re-entering its own expansion chain errors cleanly with
AdapterError::SchemaParse naming the offending ref (OAI-01)
- add depth budget (MAX_REF_RESOLUTION_DEPTH = 64) bounding $ref hop
chains and schema nesting height; over-deep specs error cleanly
instead of exhausting the stack (OAI-01)
- clean loud error over depth-limited expansion: recursive schemas
(trees, linked lists, cursor pagination) fail import rather than
expand unboundedly
- shared refs to a common schema (diamond/repeated) still resolve —
visited set is branch-scoped, not global
- OAI-08: replace the two guarded expects in from_value
("paths is object", "schemas is object") with if-let paths
Verification: cargo test 188 passed / 0 failed; clippy
--all-targets -D warnings clean; fmt --check clean
96 lines
4.6 KiB
Markdown
96 lines
4.6 KiB
Markdown
---
|
||
id: review-001-ref-cycle-guard
|
||
name: Bounded, cycle-safe $ref resolution (OAI-01, OAI-08)
|
||
status: completed
|
||
depends_on: []
|
||
scope: narrow
|
||
risk: high
|
||
impact: component
|
||
level: implementation
|
||
tags: [adapters, review-001, from-openapi]
|
||
---
|
||
|
||
## Description
|
||
|
||
Review 001 finding OAI-01 — the single highest-severity finding in the
|
||
review (borderline critical, empirically verified):
|
||
`resolve_refs_recursive` (`src/adapters/openapi_spec.rs:199-221`) recurses
|
||
with no cycle detection and no depth budget. A self-referential component
|
||
(`{"$ref":"#/components/schemas/Node"}` inside Node — trees, linked
|
||
lists, cursor pagination: common and *valid* OpenAPI) recurses until the
|
||
stack is exhausted: `thread has overflowed its stack; fatal runtime
|
||
error` → **process abort**. Not a catchable panic; `import()` kills the
|
||
whole process — startup crash-loop, or remote DoS if specs are ever
|
||
runtime-refreshed/peer-supplied.
|
||
|
||
Fix: depth budget + visited set keyed on the JSON-pointer path, returning
|
||
a clean `CallError`/import error on cycles and over-deep specs. **Do not**
|
||
try to preserve full recursive expansion of recursive schemas — the goal
|
||
is a clean, loud error (or a depth-limited expansion where the adapter can
|
||
safely represent it), not unbounded expansion.
|
||
|
||
Ride-along: **OAI-08** — guarded `expect`s in `openapi_spec.rs:144,170`
|
||
(the "no `expect` outside tests" convention; `if let` costs nothing).
|
||
|
||
## Acceptance Criteria
|
||
|
||
- [x] Import of a self-referential spec returns an error — the process does not abort (the review's named acceptance gate: a test importing a self-referential spec would have caught the abort immediately)
|
||
- [x] Deeply-nested non-circular spec beyond the budget errors cleanly at import (test)
|
||
- [x] A non-recursive spec with `$ref` sharing (refs to a common schema) still imports identically — no false cycle positives (existing suite green)
|
||
- [x] OAI-08 `expect`s replaced
|
||
- [x] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass
|
||
|
||
## References
|
||
|
||
- docs/reviews/001-initial-implementation-review.md (Part E, OAI-01, OAI-08)
|
||
|
||
## Notes
|
||
|
||
> Agent fills during implementation. Do before anything
|
||
> deployment-facing. Deliberately split from the parameter/requestBody
|
||
> work (review-001-openapi-import-integrity) so the delicate
|
||
> cycle-detection change lands alone.
|
||
|
||
Implementation decisions:
|
||
|
||
- Depth budget set to 64 (`MAX_REF_RESOLUTION_DEPTH`, pub(crate) const in
|
||
openapi_spec.rs) — far above any legitimately expandable schema height,
|
||
2× serde_json's parse recursion limit. It bounds both `$ref` hop chains
|
||
and plain schema nesting.
|
||
- Clean loud error chosen over depth-limited expansion, per the task's
|
||
stated preference: recursive schemas (trees, linked lists, cursor
|
||
pagination) fail import with `AdapterError::SchemaParse` naming the
|
||
offending ref, rather than expanding unboundedly or truncating
|
||
silently.
|
||
- The visited set is branch-scoped (insert before recursing, remove on
|
||
return), so shared refs to one common schema — diamond or repeated —
|
||
resolve normally; only a ref re-entering its own expansion chain is a
|
||
cycle.
|
||
- OAI-08: both guarded `expect`s (`paths is object`, `schemas is
|
||
object`) replaced with `if let Some(..)`; the inner-body re-shape is
|
||
behavior-preserving (both sites were immediately preceded by an
|
||
`is_object()` check).
|
||
- Tests added in `src/adapters/openapi_spec.rs` (`mod tests`): direct
|
||
recursive + mutually-recursive error, over-deep non-circular error, two
|
||
no-false-positive cases (shared common schema, diamond reuse), and an
|
||
end-to-end `FromOpenAPI::import` of a self-referential spec asserting
|
||
an error return (process does not abort).
|
||
|
||
## Summary
|
||
|
||
Implemented bounded, cycle-safe `$ref` resolution in
|
||
`src/adapters/openapi_spec.rs` (review 001 OAI-01 + OAI-08):
|
||
`resolve_refs_recursive` now delegates to a `resolve_refs_bounded`
|
||
worker carrying a branch-scoped `HashSet` of in-flight JSON-pointer refs
|
||
and a depth counter against `MAX_REF_RESOLUTION_DEPTH` (64). A ref
|
||
re-entering its own expansion chain errors with
|
||
`AdapterError::SchemaParse` ("circular $ref detected …: <ref>"); a
|
||
ref-chain or nesting height beyond the budget errors similarly ("exceeded
|
||
depth budget of 64"). Import of a self-referential spec therefore
|
||
returns a clean import error instead of aborting the process via stack
|
||
overflow (OAI-01); OAI-08's two guarded `expect`s in `from_value` became
|
||
plain `if let` paths. Six tests added in `openapi_spec.rs` covering the
|
||
cyclic, over-deep, and shared-ref matrices plus the named acceptance
|
||
gate (end-to-end self-referential import errors, no abort). 188 lib
|
||
tests green; full `cargo test`, `cargo clippy --all-targets -- -D
|
||
warnings`, and `cargo fmt --check` pass. |