Skip to content

fix(conformance): document orjson depth and integer limits in O001 - #4798

Merged
cmgrote merged 3 commits into
mainfrom
docs/o001-orjson-depth-limit
Nov 9, 2026
Merged

cmgrote merged 3 commits into
mainfrom
docs/o001-orjson-depth-limit

Conversation

@SwarnadeepChatterjee

@SwarnadeepChatterjee SwarnadeepChatterjee commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Changelog

  • The O001 rule doc now names the inputs orjson rejects that stdlib json accepts: values nested 254 or more levels deep (TypeError: Recursion limit reached) and integers above 64 bits.
  • New guidance for code that serializes source-system or API payloads: before swapping, check whether the value can reach those limits. If it can, first ask whether the app needs the deeply nested part at all, and drop unused subtrees before serialization rather than keeping a stdlib json fallback. If the app genuinely needs the deep value or integers above 64 bits, staying on stdlib json is a design decision for a human reviewer. It is not a remediated end state, and the finding stays open unless the reviewer chooses to suppress it.
  • rationale mentions both limits.
  • terminal_state ("Already correct when") is unchanged from main. An earlier revision of this PR added a "Case B" that accepted a try-orjson/except-TypeError fallback as a correct end state. That was removed: suppressions are already the general escape hatch, and a dedicated carve-out would bless a workaround that hides the real question.
  • docs/rules/by-id/O001.md and docs/rules/optimizations.md regenerated with generate_rule_docs. No detector or tier changes.

Additional context (e.g. screenshots, logs, links)

  • In a prior incident, a bare json→orjson O001 swap in a connector's extract path failed every retry on deeply nested source metadata; the same data crawled fine before the swap.
  • In that connector's payload, the only field that grows without bound is a formula expression tree inside each column's metadata detail, which the app stores but never reads. Dropping it removes the limit, keeps orjson, and shrinks the stored output. This was measured on the connector's fixture corpus; the production record itself was not logged.
  • The existing gate ("behavioural tests catch a bytes/str regression") checks return types, not input shapes, so unit tests with small values pass while real payloads fail.
  • Verified locally: orjson 3.13.0 serializes a 253-level dict and raises at 254.

Checklist

  • Additional tests added
  • All CI checks passed
  • Relevant documentation updated

generate_rule_docs --check and pre-commit pass locally. The conformance tests covering O001, the catalog, remediation references and rule-doc generation pass locally (419 passed).


Copyleft License Compliance

  • Have you used any code that is subject to a Copyleft license (e.g., GPL, AGPL, LGPL)?
  • If yes, have you modified the code in the context of this project? please share additional details.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AbKqGYdoCpwNuU4Ams1SbH

…t the fallback

orjson.dumps raises TypeError: Recursion limit reached for values nested
254+ levels deep and cannot serialize integers above 64 bits. A bare
json->orjson swap on a source-payload path broke an extract (CONAT-7331).
Document both limits, recommend try-orjson/fallback-to-json on TypeError,
and accept that fallback as a valid ignore[O001] end state so remediation
does not remove it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PckAM1Jg1diDsnZUgxDaGH
@linear

linear Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

@SwarnadeepChatterjee SwarnadeepChatterjee changed the title docs(conformance): document orjson depth/int limits in O001 and accept the fallback fix(conformance): document orjson depth/int limits in O001 and accept the fallback Nov 7, 2026
…nesting instead

Remove case B from O001's terminal state. A try-orjson/except-TypeError
fallback to stdlib json no longer counts as a correct end state of its
own: suppressions are already the general escape hatch, and blessing
the fallback hides the real question.

Replace the fallback recommendation with guidance to check whether the
deeply nested part of a source payload is used at all. In the prior
incident the only unbounded field was a subtree the app stored but
never read, so dropping it before serialization removes the limit and
keeps the orjson path. Drop the Linear link from the public rule text.

Claude-Session: https://claude.ai/code/session_01AbKqGYdoCpwNuU4Ams1SbH
@cmgrote cmgrote changed the title fix(conformance): document orjson depth/int limits in O001 and accept the fallback docs(conformance): document orjson depth and integer limits in O001 Nov 9, 2026
@cmgrote

cmgrote commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator

/lens

@cmgrote cmgrote changed the title docs(conformance): document orjson depth and integer limits in O001 fix(conformance): document orjson depth and integer limits in O001 Nov 9, 2026
@atlan-app-fleet

atlan-app-fleet Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

lens summary · updated after round 2 (re-review · only commits since d6eb41f)

✅ Ready to merge — every finding is resolved

Open findings: 🔴 0 critical · 🟠 0 high · 🟡 0 medium · ⚪ 0 low (nit)

lens reads this PR as — O001 recommends replacing stdlib json.dumps with orjson, but a bare swap can fail on real source/API payloads that include structures or integers outside orjson's supported range; small unit-test inputs may not reveal that production failure. The PR documents these limits and recommends pruning unused payload subtrees before serialization. How: It updates the rule definition and regenerated rule docs without changing the detector or enforcement tier, and says to retain stdlib json with a justified suppression only when the unsupported value is genuinely required. That is a focused documentation change, but its new suppression guidance does not align with O001's existing stated terminal state.

Approach check — ✅ every concern has been addressed.

  • ✔️ The deep-value suppression case is not an accepted O001 terminal state (A1) — addressed in round 2
✔️ Resolved (1)
id severity where finding found resolved how
F-5bec01 🟡 medium packages/conformance/conformance/suite/rules/optimizations.py Suppression advice contradicts O001's terminal state round 1 round 2 its code was removed or rewritten
🕘 Round history (2)
round run commits new resolved blocking open cost log
1 first review fbe1afe..d6eb41f 1 0 0 $0.014 run
2 re-review · only commits since d6eb41f d6eb41f..d8c0bf5 0 1 0 $0.010 run

round 2 · $0.024 of $1.00 · 23 model calls · 0 failed requests · 77% prompt cache hits · gpt-6-luna

…epted

The new guidance said a site that genuinely needs a deeply nested value
or integers above 64 bits should stay on stdlib json "behind the usual
justified suppression". O001's terminal state does not accept that case,
so the text promised an end state the rule declares unremediated. Say
instead that it is a design decision for a human reviewer and that the
finding stays open unless the reviewer chooses to suppress it.

Claude-Session: https://claude.ai/code/session_01AbKqGYdoCpwNuU4Ams1SbH
@cmgrote

cmgrote commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator

/lens

@atlan-app-fleet

Copy link
Copy Markdown
Contributor

✅ lens · round 2 · re-review · only commits since d6eb41f — ready to merge — every finding resolved · $0.02

Open findings: 🔴 0 critical · 🟠 0 high · 🟡 0 medium · ⚪ 0 low (nit)
Resolved this round: 1

Approach check — ✅ concerns addressed

  • Problem: O001 recommends replacing stdlib json.dumps with orjson, but a bare swap can fail on real source/API payloads that include structures or integers outside orjson's supported range; small unit-test inputs may not reveal that production failure. The PR documents these limits and recommends pruning unused payload subtrees before serialization.
  • How the PR solves it: It updates the rule definition and regenerated rule docs without changing the detector or enforcement tier, and says to retain stdlib json with a justified suppression only when the unsupported value is genuinely required. That is a focused documentation change, but its new suppression guidance does not align with O001's existing stated terminal state.
  • ✔️ The deep-value suppression case is not an accepted O001 terminal state (A1) — addressed in round 2

$0.024 of $1.00 · 23 model calls · 0 failed requests

Full summary · Run log

@atlan-ci atlan-ci 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.

lens: ready to merge — every finding at every level is resolved (round 2).

@cmgrote
cmgrote added this pull request to the merge queue Nov 9, 2026
Merged via the queue into main with commit 377cd57 Nov 9, 2026
30 of 31 checks passed
@cmgrote
cmgrote deleted the docs/o001-orjson-depth-limit branch November 9, 2026 10:51
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.

3 participants