feat(conformance): add O006 DirectRocksdictImport rule - #3089
Conversation
Two connectors (atlan-thoughtspot-app, atlan-aws-smus-app) independently hand-rolled a RocksDB-backed DiskLookup with an asymmetric JSON serialize/deserialize step: put() special-cased str, get() unconditionally ran json.loads() on every read. A stored string that also happened to be valid bare JSON (a numeric-looking name, "true", "null") silently came back as int/bool/None instead of str, corrupting output columns (CNCT-80, CNCT-191). Neither connector had a fleet-wide signal nudging it toward the SDK's existing application_sdk.common.spillable_dict.SpillableDict, which wraps the same rocksdict.Rdict without the hand-rolled serialization step. O005 flags any direct `rocksdict` import in app code, mirroring O004's import-anchored shape and scope=APP posture (the SDK's own spillable_dict.py and rocksdb_utils.py are the intended callers of rocksdict and are excluded).
O005 is claimed by UnresolvedAppNamePlaceholder (#3094), which will merge first. Renumber this rule to O006 so the two land without an ID collision.
O005 fired five times on the module that implements the behaviour it is meant to protect. Run against PR #3101 (FND-195): common/task_queue.py:86 APP_NAME_TOKEN = "{app_name}" common/task_queue.py:239 attribute docstring naming the token common/task_queue.py:243 attribute docstring naming the token handler/service.py:2009 logger.error(... unresolved {app_name} ...) handler/service.py:2023 logger.warning(... unbaked {app_name} ...) None can freeze into an identifier. A rule that flags the canonical fix, the docs describing it, and the logs diagnosing it is a rule people suppress — which costs the true positives too. Detection now anchors on the token reaching a value. Three exclusions added, each narrow: * documentation — the value of any bare string expression statement. The previous check excluded only body[0] of Module/ClassDef/FunctionDef, so a PEP 257 attribute docstring (a bare string after a field annotation) was flagged. A string bound to nothing cannot be dispatched. * diagnostic text — inside a logging call, warnings.warn, or a raise. Reporting an unresolved token requires quoting it. * token sentinels and message constants — bound to an ALL_CAPS name where the literal is exactly the token (its own definition) or the name reads as prose (_MESSAGE, RATIONALE). Kept narrow deliberately: TASK_QUEUE = "atlan-{app_name}-prod" is ALL_CAPS but neither bare-token nor prose-named, so it still fires. New tests pin that, plus keyword arguments, values at any depth in a DAG literal, a returned template, and a bare token bound to a lowercase name — the shapes an over-broad exclusion would have swallowed. All ten original tests pass unchanged. Also realigned the rule metadata, which predated FND-195 and claimed no canonical helper exists. application_sdk.common.task_queue now provides derive_task_queue and resolve_manifest_tokens, so remediation has one target. Detection stays shape-anchored rather than import-anchored on purpose: the writers most worth catching are hand-authored templates outside the SDK that import nothing at all. The checker's own _MESSAGE is now built from _TOKEN rather than spelling the token inline, so this module does not depend on its own exclusions to avoid self-flagging. Rule docs regenerated. Note for a follow-up, not addressed here: O005 is claimed by both this PR and #3089 (DirectRocksdictImport), both targeting 0.18.0 — whichever merges second needs renumbering, including the {#o005} docs anchor. Deciding which yields is the two authors' call.
|
@sdk-resolve |
|
🤖 SDK Resolve started. Driving this PR toward merge-ready — fixing CI + every This runs out-of-band and can take several minutes; I'll comment here when it finishes. |
|
@sdk-review |
Earlier @sdk-review trigger (click to expand)🔍 SDK Review (mothership) triggered by @mothership-ai[bot] at 2026-08-10T20:25:08.050Z. Watch the workflow run live — the review summary will appear as a separate comment when complete (typical: 5–30 min, hard cap 2h). ✅ Completed — status |
SDK Review (mothership): PR #3089 — feat(conformance): add O006 DirectRocksdictImport ruleVerdict: NEEDS FIXES
Findings
Strengths
CI: all passing (no failing checks on this HEAD) |
|
@sdk-resolve |
|
🤖 SDK Resolve started. Driving this PR toward merge-ready — fixing CI + every This runs out-of-band and can take several minutes; I'll comment here when it finishes. |
|
🤖 SDK Resolve — round 3. Picked up the latest review: 1 open finding (0 blocking, 1 nit). Verdict is |
…ption The O-series remediation program prescribed fixes only for O001-O004, so an O006 finding routed to remediate-finding had no rule-specific guidance. Add an O006 section covering the migrate-to-SpillableDict judgment (pickled values, str|int|float|bool|bytes keys, no custom Options surface) and the justified-suppression paths (deliberate custom Options, out-of-range key types, native merge/append_to_key semantics SpillableDict does not provide), mirroring how O004 documents its intentional-legacy-pin suppression.
|
@sdk-review |
Earlier @sdk-review trigger (click to expand)🔍 SDK Review (mothership) triggered by @mothership-ai[bot] at 2026-08-10T20:58:07.954Z. Watch the workflow run live — the review summary will appear as a separate comment when complete (typical: 5–30 min, hard cap 2h). ✅ Completed — status |
SDK Re-review (mothership): PR #3089 — feat(conformance): add O006 DirectRocksdictImport ruleVerdict: READY TO MERGE
Delta from previous review
Findings
Holistic Recommendations
Strengths
CI: all passing (no failing checks on this HEAD; sdk-review status set by this run) |
atlan-ci
left a comment
There was a problem hiding this comment.
SDK reviewer's verdict: READY TO MERGE.
Full review summary is in the comment posted on this PR.
|
@sdk-review |
|
🔍 SDK Review (mothership) triggered by @mothership-ai[bot] at 2026-08-10T21:18:08.360Z. Watch the workflow run live — the review summary will appear as a separate comment when complete (typical: 5–30 min, hard cap 2h). ✅ Completed — status |
SDK Re-review (mothership): PR #3089 — feat(conformance): add O006 DirectRocksdictImport ruleVerdict: READY TO MERGE
Delta from previous review
Findings
Holistic Recommendations
Strengths (independently verified this round)
CI: all passing (no failing checks on this HEAD; sdk-review status set by this run) |
atlan-ci
left a comment
There was a problem hiding this comment.
SDK reviewer's verdict: READY TO MERGE.
Full review summary is in the comment posted on this PR.
|
🤖 SDK Resolve — merge-ready. @cmgrote this PR is now yours to review and merge. Outcome: MERGE-READY — green CI + every
Reviewer assignment: NO reviewer list is configured ( |
O005 fired five times on the module that implements the behaviour it is meant to protect. Run against PR #3101 (FND-195): common/task_queue.py:86 APP_NAME_TOKEN = "{app_name}" common/task_queue.py:239 attribute docstring naming the token common/task_queue.py:243 attribute docstring naming the token handler/service.py:2009 logger.error(... unresolved {app_name} ...) handler/service.py:2023 logger.warning(... unbaked {app_name} ...) None can freeze into an identifier. A rule that flags the canonical fix, the docs describing it, and the logs diagnosing it is a rule people suppress — which costs the true positives too. Detection now anchors on the token reaching a value. Three exclusions added, each narrow: * documentation — the value of any bare string expression statement. The previous check excluded only body[0] of Module/ClassDef/FunctionDef, so a PEP 257 attribute docstring (a bare string after a field annotation) was flagged. A string bound to nothing cannot be dispatched. * diagnostic text — inside a logging call, warnings.warn, or a raise. Reporting an unresolved token requires quoting it. * token sentinels and message constants — bound to an ALL_CAPS name where the literal is exactly the token (its own definition) or the name reads as prose (_MESSAGE, RATIONALE). Kept narrow deliberately: TASK_QUEUE = "atlan-{app_name}-prod" is ALL_CAPS but neither bare-token nor prose-named, so it still fires. New tests pin that, plus keyword arguments, values at any depth in a DAG literal, a returned template, and a bare token bound to a lowercase name — the shapes an over-broad exclusion would have swallowed. All ten original tests pass unchanged. Also realigned the rule metadata, which predated FND-195 and claimed no canonical helper exists. application_sdk.common.task_queue now provides derive_task_queue and resolve_manifest_tokens, so remediation has one target. Detection stays shape-anchored rather than import-anchored on purpose: the writers most worth catching are hand-authored templates outside the SDK that import nothing at all. The checker's own _MESSAGE is now built from _TOKEN rather than spelling the token inline, so this module does not depend on its own exclusions to avoid self-flagging. Rule docs regenerated. Note for a follow-up, not addressed here: O005 is claimed by both this PR and #3089 (DirectRocksdictImport), both targeting 0.18.0 — whichever merges second needs renumbering, including the {#o005} docs anchor. Deciding which yields is the two authors' call.
Regenerated via gen-rule-docs so the catalog page carries both O005 (this PR) and O006 (#3089, landed on main) after the rebase.
Linked issue
https://linear.app/atlan-epd/issue/FND-163/parquet-nulllarge-string-typing-two-duplicate-sdk-fixes-still-leaking
Problem
atlan-thoughtspot-appandatlan-aws-smus-appeach independently hand-rolled a RocksDB-backedDiskLookupclass with an asymmetric JSON serialize/deserialize step:put()special-casesstrand stores it raw;get()unconditionally triesjson.loads(). A stored string that also happens to be valid bare JSON (a numeric-looking name,"true","null") silently round-trips back asint/bool/Noneinstead ofstr— corrupting output columns and, in ThoughtSpot's case, crashing the downstream parquet writer (CNCT-191; the Teradata/Presto sibling bug is CNCT-80).Neither connector had a fleet-wide signal nudging it toward
application_sdk.common.spillable_dict.SpillableDict, which already wraps the samerocksdict.Rdictand pickles values directly — no hand-rolled serialize/deserialize step to get wrong. Two connectors independently wrote the same bug because nothing pointed either one at the SDK utility that already solves it.Fix
New O006
DirectRocksdictImportrule (WARN,scope=app), mirroring O004's import-anchored shape: flags any directfrom rocksdict import .../import rocksdictin app code, recommendingSpillableDictinstead.application_sdk.common.spillable_dictandapplication_sdk.common.incremental.storage.rocksdb_utils— the SDK's own intended callers ofrocksdict— are excluded via the existingscope=APPmechanism (the SDK is the publisher of this seam, not a subject of the rule).Numbered O006, not O005: #3094 (
UnresolvedAppNamePlaceholder) claims O005 and merges first, so this rule takes the next free ID.Not autofixable —
SpillableDict's key type is restricted tostr | int | float | bool | bytesand it has no customrocksdict.Optionstuning surface, so each site needs review before migrating. Suppressible with# conformance: ignore[O006] <reason>.Test plan
check_o006unit tests: from-import, aliased from-import, module import, submodule from-import, silent onSpillableDict/unrelated imports, inline suppression (test_asset_mapper.py)test_catalog_o_series_present,test_catalog_app_scoped_rules_are_the_expected_setdocs/rules/optimizations.mdviagen-rule-docspackages/conformancesuite: 2159 passed (1 pre-existing failure unrelated to this change —test_sdk_base_names_matches_templates_allneeds a siblingapplication_sdkinstall not present in this standalone checkout)🤖 Generated with Claude Code