Repository navigation
fix(rich): phase-D1 canonical closure + preflight legal-set alignment (#669) - #687
Conversation
preflightContentDocumentStructure counted every raw JSON value (objects, arrays, scalar strings) against PREFLIGHT_NODE_BUDGET = totalNodes * 2 + 64 (4064), while CONTENT_LIMITS.totalNodes counts grammar nodes: a marked text run costs ~6 raw units, so legal documents at ~30% of the serialized budget and well under 2000 grammar nodes were rejected before the schema parse (#673 C1 / PC-F02, evidence #686 — 7/7 shape families HIDDEN_STRICTER_SET). Remove the raw-node and fan-out budgets: the serialized-size gate already bounds the walk (every raw JSON value serializes to >= 1 char), and raw nesting depth stays bounded by PREFLIGHT_RAW_DEPTH_BUDGET, which is what actually protects the recursive parser. Grammar-node budgets remain the post-parse CONTENT_LIMITS authority, so preflight no longer defines a hidden stricter legal set (RC-04).
Normalization can change the measured shape (adjacent same-mark text runs merge, duplicate marks dedup), so a schema-valid input could normalize to a canonical value outside CONTENT_LIMITS — accepted on write, rejected by the same authority on every later re-validation (B-F01 / PC-F01, RC-03; the 20001-char merge class, evidence #686). Both durable Rich writers shared the hole: the answer canonicalizer and the question/option content seam. Add canonicalizeContentDocument to @exam/contracts — normalize + re-validate the canonical output through the one Rich authority (ContentDocumentV1Schema = grammar + CONTENT_LIMITS) — and route both writers through it. Legality is now decided on the representation that is actually persisted. SaveAnswer maps the rejection to its existing INVALID_ANSWER category; authoring maps it to ValidationError (400). No second limit table, schema, or normalizer.
R1 PC-F01 merge-class seed (20000+1 unmarked runs) rejected at the answer
canonicalizer, the authoring seam (400), and the save-answer route
(INVALID_ANSWER) — the Phase-C L3 gap driven end to end.
R2 Canonical textRun boundary closed from both sides (T-1+1 accepted and
schema-legal; T-1+2 rejected), derived from CONTENT_LIMITS.
R3 Legality decided on the canonical form: duplicate-mark dedup + merge,
hardBreak as a non-merge barrier, RC-03 fixed point on accepted values.
R4 The seven Phase-C preflight-shrink families (plain runs, marked runs,
duplicate marks, empty paragraphs, marked tables) pass preflight and
the public parse entry at their measured failure scales.
R5 Genuine violations still rejected: hostile nesting, oversized
serialization, and node-count overflow now named by the CONTENT_LIMITS
walker instead of a preflight budget.
Plus a bounded seeded property (300 cases, seed 0x66900001): every accepted
answer stays schema-legal after canonicalization. Production functions
only — no duplicated oracle. Evidence provenance: #686 (research branch,
not a parent of this branch). #669 #673
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL 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 |
jnhu76
left a comment
jnhu76
left a comment
There was a problem hiding this comment.
D1 focused review: CHANGE_REQUIRED (one narrow safety blocker).
The RC-03 canonical-closure repair itself looks correct: both durable Rich writer seams now share canonicalizeContentDocument, which delegates to the existing normalizer and then re-validates the actual canonical output through ContentDocumentV1Schema; the permanent regressions cover PC-F01 at helper/authoring/real SaveAnswer-route boundaries and keep the existing error categories.
The RC-04 authority correction is also directionally correct: removing the raw-node/fan-out budgets eliminates the hidden stricter language and leaves grammar-node limits to CONTENT_LIMITS.
F1 — serialized-size gate does not actually bound/short-circuit the raw walk. In preflightContentDocumentStructure, when JSON.stringify(value).length > CONTENT_LIMITS.serializedChars, the code pushes a violation but then continues into the iterative DFS. That contradicts the new comments/tests saying the oversized value is rejected "before the raw walk runs" and weakens the preflight safety story after removing the old node/fan-out early exits. For an already-known oversized hostile value, there is no need to traverse the whole structure at all.
Required correction: once serialization succeeds and exceeds the authoritative serialized-size limit, return the violation immediately (after any envelope checks only if there is a specific contract reason to retain them; preferably keep this as the cheap structural gate). Add a regression that proves an oversized fan-out does not enter/traverse the raw DFS—not merely that a violation is eventually returned. Keep the legal-set behavior unchanged.
No broad redesign requested. After this focused fix I only need to re-review this preflight path; RC-03 does not need reopening.
F1 review finding (#687): an oversized payload had its verdict recorded, but the raw DFS still traversed the hostile structure — the serialized budget was not actually the traversal bound the section comment claimed. An oversized payload is now rejected before the walk; envelope diagnostics (O(1) property reads, existing error contract) are kept. The regression test pairs oversize with beyond-raw-depth nesting and asserts the size violation is the ONLY output, proving the walk never ran.
F1 修复：preflight 序列化门禁短路（commit
|
jnhu76
left a comment
jnhu76
left a comment
There was a problem hiding this comment.
Focused re-review of F1 follow-up (38b6b735) only.
Verdict: F1 CLOSED.
The implementation now short-circuits after the serialized-size verdict and before the raw DFS. Envelope checks remain O(1), while oversized hostile structure is no longer traversed. The new dual-violation regression (oversize + raw depth > budget) is a good behavioral proof: it requires the result to contain only the serialized-size violation, so any accidental DFS would reintroduce the depth violation and fail the test.
No canonicalization code was touched; the previously accepted RC-03/RC-04 repair remains unchanged.
Focused verdict:
F1_OVERSIZED_PREFLIGHT_WALK = CLOSED
RC-03_CANONICAL_CLOSURE = PASS
RC-04_LEGAL_SET_ALIGNMENT = PASS
PRELIGHT_SAFETY = PASS
PHASE_D1_CONTENT = PASS
Exact-head CI for 38b6b735 is still in progress at review time, so merge should wait for that run to finish green. No broad rerun or further D1 code change is requested.
1 participant
Phase D1 of #669 — PC-F01 (RC-03) + PC-F02 (RC-04) only
D1-A restores canonical closure at the durable Rich acceptance boundary; D1-B removes the preflight's hidden stricter legal set. Evidence provenance: #686 is the Phase-C research evidence PR (closed without merge, branch preserved) — it is not a parent of this branch; D1 branches from live master
6c50ebfe(#685). Ledger context: #673. Do not merge before final review; no authority document is changed.Root causes (as-built, confirmed against Phase-C evidence)
ContentDocumentV1Schemarejects — yet the canonicalizer returned it for persistence. Both durable writers shared the hole: the answer canonicalizer (validateAnswerForQuestion) and the question/option content seam (resolveQuestionContentWrite).preflightContentDocumentStructurecounted every raw JSON value (objects, arrays, scalar strings) againsttotalNodes * 2 + 64 = 4064, whileCONTENT_LIMITS.totalNodescounts grammar nodes (a marked text run costs ~6 raw units). All 7 Phase-C shape families were rejected at ~30% of the serialized budget and well under 2000 grammar nodes.Repair (production: 4 files)
canonicalizeContentDocumentin@exam/contracts: normalize + re-validate the canonical output through the one Rich authority (ContentDocumentV1Schema= grammar +CONTENT_LIMITS). Both durable writers now persist only its accepted value; legality is decided on the representation actually stored. SaveAnswer keeps its existingINVALID_ANSWERcategory; authoring keepsValidationError→ 400. No second limit table, schema, normalizer, or acceptance predicate.CONTENT_LIMITSauthority. Deepest-legal nesting still passes (unchanged depth budget); hostile depth/size still rejected pre-parse.Regression coverage (R1–R5 + property)
INVALID_ANSWER) — closes the Phase-C "L3 inferred, not executed" gap by driving the real route.textRunboundary closed from both sides, derived fromCONTENT_LIMITS(T−1+1 accepted and schema-legal; T−1+2 rejected).CONTENT_LIMITSwalker instead of a preflight budget.Historical compatibility
New durable acceptance only; no read path, classification, or persisted value is touched. Values persisted pre-fix by the PC-F01 chain were already rejected by every re-validation read path (Phase-C downstream chain); D1 does not change their read behavior. Preflight loosening is prospective: the save/authoring seams rejected over-budget documents before persisting, so no persisted value changes classification (if any pre-preflight-era value exists, the loosening can only make it readable, never unreadable). In-flight replay of an over-limit payload now hits Rich canonicalization before the replay check, per contract §13 precedence — contract-conformant, no migration.
Verification
pnpm verify:staticgreen (format, code-quality, arch, db-config, db-journal, env-contract, eslint, typecheck, openapi).server.shutdown.test.tsEADDRINUSE :3000, because the spawned dev-mode child bindsDEV_API_PORTregardless of itsAPP_PORT(runtimeConfig.tsport-ownership switch) and a live dev server on this host holds :3000. Unrelated to this branch; classified with evidence.pnpm buildgreen;bash scripts/e2e/run.sh rich-content2/2 shards green.Follow-up candidates (not bundled)
server.shutdown.test.tschild port isolation: setDEV_API_PORT(or an explicit mode) so a running dev server cannot collide.@exam/domainvia staledist; rebuild before per-package test runs (or alias to src).