feat(preflight): P041 — flag a typed leaf whose audience contradicts its gate-broken category (CONNECT-812 PF-29) - #3592
Open
zaman-atlan wants to merge 2 commits into
Conversation
…its gate-broken category
The gate keeps DEPENDENCY_UNAVAILABLE, RATE_LIMITED, RESOURCE_EXHAUSTED and
CANCELLED in _GATE_BROKEN_CATEGORIES so a failure in one of them fails *open* —
the gate could not form a verdict, so the run proceeds rather than blaming the
source. A leaf in one of those categories that also declares audience=USER says
the opposite: that the customer caused it and must fix it. Both cannot be true,
and the taxonomy already answers which — 8 of the SDK's 9 gate-broken leaves
resolve to PLATFORM or APP_OWNER.
RateLimitedError was the sole outlier, verified against the resolved MRO of all
21 AppError subclasses. Its USER audience implied a remediation ("reduce crawl
concurrency") that Atlan owns and the customer cannot perform, and it aggregated
every throttle against the customer's SLA.
Fixes the leaf and adds the rule that keeps it fixed:
- RateLimitedError.audience USER -> APP_OWNER. A 429 is our call rate against a
customer-owned endpoint, so the locus is the team that chose the concurrency;
wire.py already resolves an unclear locus to APP_OWNER. PLATFORM would route
throttles to platform-oncall, who control neither the rate nor the source.
- P041 GateBrokenCategoryUserAudience, STATIC/WARN, scope BOTH — it grades the
SDK's own leaves as well as app subclasses, and lands green because the leaf
is fixed in the same change.
The rule compares two constants the SDK already publishes, so it makes no
judgement about whether a message matches a category — the part that makes
category-correctness undecidable in general. Only an audience bound in the class
body is flagged; an inherited USER is deliberately not resolved across the SDK
boundary, since once the leaf is correct, inheriting from it is right.
Verified end to end: against the real leaves.py the rule reports exactly one
finding at leaves.py:98 (RateLimitedError) before the fix and zero after, with
no false positives across the other 20 leaves.
CONNECT-812 PF-29.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zaman-atlan
requested review from
Aryamanz29,
OnkarVO7,
atlan-ci,
cmgrote and
vaibhavatlan
as code owners
September 1, 2026 10:32
Contributor
📜 Docstring Coverage ReportRESULT: PASSED (minimum: 30.0%, actual: 81.0%) Detailed Coverage ReportThis message was truncated. Download full message |
Contributor
📦 Trivy Vulnerability Scan Results
Report SummaryCould not generate summary table (data length mismatch: 9 vs 8). Scan Result Detailspackages/conformance/uv.lockuv.lock |
Contributor
📦 Trivy Secret Scan Results
Report SummaryCould not generate summary table (data length mismatch: 9 vs 8). Scan Result Detailspackages/conformance/uv.lockuv.lock |
Contributor
☂️ Code Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
cmgrote
requested changes
Sep 2, 2026
cmgrote
left a comment
cmgrote
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: additive in mechanism, but 4 defects — 2 of them blocking
First, the reassuring part. This is additive to the recent preflight work. It touches zero files that #3521 or the last-48h merges (8f65ab4, a57bfce, 22d2095, c3ade68) changed in preflight_gate.py. The only merge conflict — against both main and #3521 — is packages/conformance/conformance/docs/rules/prescriptions.md, which is auto-generated (gen-rule-docs regenerates it). That's noise, not overlap.
Two other things check out, so nobody needs to re-litigate them:
RateLimitedErrorreally is the only SDK leaf with a gate-broken category andaudience=USER. The "eight of nine" claim in the rule docs holds.- No SDK code branches on
audience— it is wire/telemetry metadata only. So the flip changes attribution and the customer-facing voice, not control flow.
Now the problems.
1. (blocking) P041 is a retired ID, and the package forbids reusing it
packages/conformance/conformance/suite/rules/prescriptions.py:9-16 states the policy explicitly:
Rule-id stability (non-migration policy)
P-ids are a permanent public contract: each is exposed in SARIFhelp_uriand referenced by inline# conformance: ignore[Pxxx]suppressions across the fleet. A P-id therefore never migrates and never changes … the original P-id is never reused or reassigned.
P041 was a live SDR rule until it was retired 9 days ago in ca5861749 ("fix(conformance): remove obsolete P041 rule", #3367). Reassigning it means:
- any
# conformance: ignore[P041]still sitting in a fleet app repo, written for the old SDR rule, now silently suppresses this new and unrelated rule; - SARIF history and any dashboard keyed on rule id conflates two different rules.
Fix: take the next free id (P052+) and leave P041 retired.
2. (blocking) The checker fires 4 false positives on the SDK's own tree
scope=BOTH, so this runs against application_sdk/ too. I ran the PR's checker over the SDK error modules on current main:
| Class | P041 fires | Real MRO category |
actually gate-broken? |
|---|---|---|---|
errors/leaves.py::RateLimitedError |
yes | RATE_LIMITED |
yes — correct hit |
storage/errors.py::StorageNotFoundError |
yes | NOT_FOUND |
no |
storage/errors.py::StoragePermissionError |
yes | PERMISSION |
no |
storage/errors.py::StorageConfigError |
yes | INVALID_INPUT |
no |
infrastructure/secrets.py::SecretNotFoundError |
yes | NOT_FOUND |
no |
Verified against the real classes rather than inferred:
StorageNotFoundError FailureCategory.NOT_FOUND Audience.USER gate_broken=False
StoragePermissionError FailureCategory.PERMISSION Audience.USER gate_broken=False
StorageConfigError FailureCategory.INVALID_INPUT Audience.USER gate_broken=False
SecretNotFoundError FailureCategory.NOT_FOUND Audience.USER gate_broken=False
Cause: _gate_broken_audience.py:107 (_carries_gate_broken_category) resolves an inherited category with
return any(b in derived for b in _base_names(cls, aliases))which ignores base order. StorageNotFoundError(NotFoundError, StorageError) takes its category from its first base (NOT_FOUND), but StorageError descends from DependencyUnavailableError, so the any() matches anyway. Same shape for the other three: a non-gate-broken first base plus a StorageError/SecretStoreError mixin.
These four are exactly the classes the rule should stay silent on — the gate does not treat them as plumbing, and USER is the right audience for a missing bucket or a bad config.
Fix: resolve the inherited category MRO-first rather than with any(). suite/checks/_entrypoint_contract_fields.py:287 already has the MRO-ordered, cycle-safe walk to model this on.
Worth noting the green Conformance / Prescriptions check does not clear this: the leg runs soft, so these four would ship as unnoticed warnings.
3. Two docs still contradict the leaves.py change
The RateLimitedError audience flip is not propagated to:
docs/adr/0013-error-hierarchy-and-failure-taxonomy.md:105— the table row still readsUSER. This ADR is the taxonomy's source of truth, so it should not be the last thing to learn about a taxonomy change.docs/concepts/common.md:55— the hierarchy tree still rendersaudience=USER.
4. (non-blocking) Reconcile the wording against #3521
#3521, about to merge, adds _CLIENT_FAULT_CATEGORIES (preflight_gate.py:863) containing RATE_LIMITED and CANCELLED, and emits outcome="client_fault" for them on interactive surfaces. This PR argues the same two categories are not the customer's, and bans audience=USER on them.
Both can be right — #3521's "client" is the caller of the HTTP/SDR surface, not the customer — but once both land, a 429 on an interactive surface emits outcome=client_fault next to failure.audience=APP_OWNER, and whoever reads that dashboard will have to work out that the two words mean different things. A one-line note at either definition site would settle it. No code change needed here.
Summary
1 and 2 need fixing before merge. 3 is a small follow-through in the same PR. 4 is a note to coordinate with #3521.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The preflight gate keeps
DEPENDENCY_UNAVAILABLE,RATE_LIMITED,RESOURCE_EXHAUSTEDandCANCELLEDin_GATE_BROKEN_CATEGORIES(preflight_gate.py:423) so a failure in one of them fails open — the gate could not form a verdict, so the run proceeds rather than blaming the source.A leaf in one of those categories that also declares
audience = USERsays the opposite: that the customer caused it and must fix it. Both cannot be true, and the taxonomy already answers which.Resolved against the real MRO of all 21
AppErrorsubclasses —RateLimitedErroris the sole outlier of the nine gate-broken leaves:Concretely: the remediation a
USERaudience implies — reduce crawl concurrency — is something Atlan controls and the customer cannot change. Meanwhile every throttle aggregated against the customer's SLA.Changes
The fix —
RateLimitedError.audienceUSER→APP_OWNER. A 429 is our call rate against a customer-owned endpoint, so the locus is the team that chose the concurrency;wire.py:36-39already resolves an unclear locus toAPP_OWNER.PLATFORMwould route throttles to platform-oncall, who control neither the rate nor the source.The rule that keeps it fixed —
P041 GateBrokenCategoryUserAudience,STATIC/WARN, scopeBOTHso it grades the SDK's own leaves as well as app subclasses. It lands green because the leaf is fixed in the same change.Follows PR #3492 (P047) for file layout: rule definition + one private detector module + registration + generated docs + tests.
Why this rule is cheap and safe
It compares two constants the SDK already publishes. No semantics, no judgement about whether a message matches a category — the part that makes category-correctness undecidable in general (cf. PF-21, where
P034was satisfied by the wrong typed error).Scope is deliberately narrow, matching the P series' false-negative-over-false-positive stance:
audiencebound in the class body is flagged. An inheritedUSERis not resolved across the SDK boundary — once the leaf itself is correct, inheriting from it is the right answer, and modelling SDK internals from an app repo is exactly the false-positive surface to avoid.categoryleaves the gate-broken set and is not flagged here;P002governs that redeclaration.Verification
Not just synthetic fixtures — run against the real
application_sdk/errors/leaves.py:leaves.pyleaves.py:98, classRateLimitedErrorNo false positives across the other 20 leaves.
The 10 new tests cover both directions: fires on each of the four gate-broken categories, on a subclass inheriting the category from an SDK leaf, and through an in-file intermediate; stays silent on
APP_OWNER/PLATFORM, on a non-gate-broken category (AUTH, whereUSERis correct), on inherit-only, and on a subclass that redeclares a safe category. Plus the inline-suppression path.Note on the rule ID
CONNECT-812 specced this as
P040on 2026-08-12, when that was the next free ID.P040(TransformTemplateReservedKeyword) andP042–P050have shipped since, and P-ids are permanent and never reused — so this takesP041, the one remaining gap in the series.Registry rows PF-01/PF-31 (
P051) and EP-03/EP-03b (P052) need the same renumbering when they land.Follow-ups (not in this PR)
RedshiftRateLimitError.audience = APP_OWNERoverride (redshift#387) can drop it now that the leaf is correct.AppTimeoutErrorbuilt by the gate leavesoperation/timeout_seconds/elapsed_secondsnull though all three exist on the class (preflight_gate.py:1086-1093) — SDK-side instance of PF-28.CONNECT-812 PF-29.
🤖 Generated with Claude Code