Skip to content

feat(conformance): add O005 UnresolvedAppNamePlaceholder - #3094

Merged
cmgrote merged 6 commits into
mainfrom
mrun/fnd-184-conformance-o005
Aug 10, 2026
Merged

feat(conformance): add O005 UnresolvedAppNamePlaceholder#3094
cmgrote merged 6 commits into
mainfrom
mrun/fnd-184-conformance-o005

Conversation

@AtMrun

@AtMrun AtMrun commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Adds O005 UnresolvedAppNamePlaceholder to the O-series conformance checks: flags a plain string literal that still carries an unsubstituted {app_name} token (not an f-string, not a .format(app_name=...) receiver, not a docstring).
  • Motivating incident: CONNECT-183 — a task-queue name literally frozen as "atlan-{app_name}-production" meant no worker ever polled it, hanging dbt:process to its 24h heartbeat backstop before failing.
  • The substitution helper this needs (substitute_app_name_placeholder) has been independently hand-rolled at least 4 times across separate codebases — Heracles (Go), native-migration-app, atlan-local-marketplace-app (CONNECT-191, #539), and atlan-hightouch-app (ARUN-1039) — because no shared, discoverable utility exists in the SDK yet. Detection here is deliberately shape-anchored, not import-anchored: it flags the unresolved literal directly rather than checking for a canonical helper call that doesn't exist yet.
  • WARN tier, not autofixable (the correct fix depends on where app_name is actually available in scope — sometimes an f-string is right, sometimes the value needs threading in from a caller), suppressible via # conformance: ignore[O005] <reason>.

Ref: FND-184 (support-patterns tracking ticket)

Test plan

  • New unit tests in tests/test_app_name_placeholder.py — fires on bare-literal assignment, call argument, dict value, and a .format() call missing the app_name kwarg; silent on f-strings, resolving .format(app_name=...) calls, module/function docstrings, and literals with no token; suppressed by inline directive
  • test_catalog_o_series_present updated to include O005
  • Docs regenerated via atlan-application-sdk-conformance gen-rule-docs (never hand-edited)
  • uv run --with pytest pytest tests/ -q --ignore=tests/test_sdk_contract_mixins.py — 2162 passed, 1 pre-existing unrelated failure (test_app_name_alignment.py::test_sdk_base_names_matches_templates_all, needs a sibling application_sdk install this standalone checkout doesn't have — documented pre-existing gap, not touched by this change)
  • pyproject.toml version and CHANGELOG.md deliberately left untouched (release-automation owned)

Not covered by this PR

  • Does not add the actual substitute_app_name_placeholder SDK helper — that's a separate, larger design decision (where does app_name come from at each call site) tracked as its own follow-up on FND-184.
  • Does not retroactively find existing unresolved placeholders already shipped in consumer app repos — this only catches new code going forward.

🤖 Generated with Claude Code

@linear

linear Bot commented Aug 10, 2026

Copy link
Copy Markdown

FND-184

@AtMrun

AtMrun commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

@sdk-resolve please review this PR, handle any review nuances/fixes, and get CI green.

@github-actions

Copy link
Copy Markdown
Contributor

🤖 SDK Resolve started. Driving this PR toward merge-ready — fixing CI + every @sdk-review finding, then requesting human review.

Follow progress →

This runs out-of-band and can take several minutes; I'll comment here when it finishes.

@mothership-ai

mothership-ai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

@sdk-review

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor
Earlier @sdk-review trigger (click to expand)

🔍 SDK Review (mothership) triggered by @mothership-ai[bot] at 2026-08-10T15:08:21.325Z.

Watch the workflow run live — the review summary will appear as a separate comment when complete (typical: 5–30 min, hard cap 2h).


🟥 Run ended without a clean completion — status error, cost $0.0, duration 7m 18s.
Error: sandbox_error — No conversation found with session ID: d026302a-c880-4461-954e-05354497319f

cmgrote added a commit that referenced this pull request Aug 10, 2026
…ic file

The dynamic-manifest hook replaces the manifest wholesale — `raw =
orjson.dumps(computed)` discarded the reconciled bytes — so everything it
emitted was served unreconciled: a task_queue, a freshly generated node, a
token it re-introduced. FND-195's guarantee that the served queue is stamped
from what the worker polls therefore did not hold for any app with a hook.

That is the worst possible population to miss. A bundle app's marketplace
entry points have their DAG computed per submission by exactly this hook, and
those are the CONNECT-183-shaped apps the guarantee exists for. #3090 widens
the exposure further: before it, a bare /manifest on a bundle 404'd; after it,
that route serves hook-computed DAGs through this same path.

Not a regression introduced here — the pre-FND-195 code also substituted
{deployment_name} before the hook and kept nothing afterwards. It only becomes
load-bearing once the queue value is the thing being guaranteed.

The pre-hook pass stays, so the hook still sees resolved values it may key on;
the second pass is idempotent (no template survives the first, so it is a
no-op unless the hook introduced something new).

Catches unresolved *tokens*, not a hook that hardcodes a concrete-but-wrong
queue: that string has no token to match, and normalising every `atlan-*`
queue would rewrite the legitimate cross-app dispatch nodes this deliberately
leaves alone. Conformance O005 (#3094) is the guard for that shape.

Both tests were checked against the pre-change code and fail there. The
unresolvable-name test asserts the graded outcome — deployment token filled,
{app_name} left visible — because asserting only the surviving token would
also pass against a build that never reconciles hook output.

Refs FND-195.
cmgrote added a commit that referenced this pull request Aug 10, 2026
O005 is claimed by UnresolvedAppNamePlaceholder (#3094), which will merge
first. Renumber this rule to O006 so the two land without an ID collision.
@cmgrote

cmgrote commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Pushed 7a24a171 to this branch — O005 was firing on the module that implements the behaviour it protects, and #3101 (FND-195) has since changed what the right implementation looks like. Both addressed; happy to back any of it out if you disagree with a call.

The problem

Run against #3101's head, O005 produced five findings:

File Line Shape
common/task_queue.py 86 APP_NAME_TOKEN = "{app_name}" — the token's own definition
common/task_queue.py 239, 243 PEP 257 attribute docstrings naming the token
handler/service.py 2009, 2023 the WARNING/ERROR logs diagnosing an unresolved token

None of those can freeze into an identifier. Two are general classes rather than quirks of that PR: attribute docstrings aren't body[0] of their class body, so the first-statement-only exclusion missed them; and any code that reports on the token has to quote it.

That combination is the failure mode worth avoiding — a rule that flags the canonical fix, the docs describing it, and the logs diagnosing it gets suppressed wholesale, and the true positives go with it.

What changed

Detection now anchors on the token reaching a value. Three exclusions, each deliberately narrow:

  • documentation — the value of any bare string expression statement (generalises the docstring exclusion to attribute docstrings; a string bound to nothing can't be dispatched)
  • diagnostic text — inside a logging call, warnings.warn, or a raise
  • token sentinels and message constants — bound to an ALL_CAPS name where the literal is exactly the token, or the name reads as prose (_MESSAGE, RATIONALE)

The narrowness is the part I'd most like you to check. 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 quietly swallowed. All ten of your original tests pass unchanged, and the five findings above are now zero.

Metadata realignment

The rationale and full_description predated FND-195 and said no canonical helper exists — that was the stated reason for shape-anchored rather than import-anchored detection. application_sdk.common.task_queue now provides derive_task_queue and resolve_manifest_tokens, so remediation has a single target and I've pointed the text at it.

I kept detection shape-anchored anyway, and made that explicit: the writers most worth catching are hand-authored templates outside the SDK that import nothing at all. So your original instinct holds — it just needed a different justification now that the helper exists.

Also: the checker's _MESSAGE is now built from _TOKEN instead of spelling the token inline, so this module doesn't rely on its own exclusions to avoid self-flagging.

Verification

  • your 10 original tests: pass unchanged
  • 10 new tests covering the exclusions and their limits
  • packages/conformance/tests: 2174 passed, 1 pre-existing unrelated failure (test_l010_fires_when_rebound_via_type_alias uses PEP 695 type X = Y, which doesn't parse on the 3.11 venv — not touched by this diff, but worth a look separately since CI runs 3.11)
  • rule docs regenerated; gen-rule-docs --check clean
  • pre-commit clean incl. pyright

One thing I did not decide for you

O005 is claimed by both this PR and #3089 (DirectRocksdictImport), both targeting 0.18.0, both adding a {#o005} docs anchor. _combine_rules() raises on duplicate IDs so it fails loudly rather than shipping — but whichever merges second needs the rule renumbered along with its docs anchor, tests, and remediation prose. Which PR yields is yours and the other author's call, not mine, so I left the ID alone.

Separately, and my error rather than yours: I'd written elsewhere that O005 guards a hook hardcoding a concrete-but-wrong queue (atlan-dbt-prod where the worker polls atlan-dbt-v3-prod). It doesn't — there's no token to match, so that case is currently unguarded. Correcting that on #3101 rather than expanding scope here.

@cmgrote

cmgrote commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

@sdk-resolve

@github-actions

Copy link
Copy Markdown
Contributor

🤖 SDK Resolve started. Driving this PR toward merge-ready — fixing CI + every @sdk-review finding, then requesting human review.

Follow progress →

This runs out-of-band and can take several minutes; I'll comment here when it finishes.

@mothership-ai

mothership-ai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

@sdk-review

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor
Earlier @sdk-review trigger (click to expand)

🔍 SDK Review (mothership) triggered by @mothership-ai[bot] at 2026-08-10T20:27:43.511Z.

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 completed, cost $5.749677999999999, duration 11m 8s.

@mothership-ai

mothership-ai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

SDK Review (mothership): PR #3094 — feat(conformance): add O005 UnresolvedAppNamePlaceholder

Verdict: NEEDS FIXES

The detector itself is well-built and well-tested (20 tests pass, dogfood is clean at 4 hits across testing/, WARN-tier so it won't break the gate). But two things need fixing before this lands. (1) The rule's runtime SARIF message — the text every user sees on a hit — tells them to remediate via application_sdk.common.task_queue (derive_task_queue / resolve_manifest_tokens, FND-195), a module that does not exist on main: it ships in the separate, still-open PR #3101. Until #3101 merges, the canonical fix the rule points to is a dead link. (2) The two exclusion helpers over-exempt, so the rule silently misses the exact unresolved-token shape it exists to catch (escaped-brace f-strings like f"{{app_name}}", and ALL_CAPS dispatch templates whose name happens to contain a prose fragment like MESSAGE_QUEUE). The class underneath both is "exemption matches shape without confirming the token actually resolves."


Findings

packages/conformance/conformance/suite/checks/optimizations/_app_name_placeholder.py

  • Important [RULE] L217 — _joined_str_children blanket-exempts every ast.Constant child of every ast.JoinedStr. Python parses f"atlan-{{app_name}}-production" as a JoinedStr, but the escaped braces evaluate to the literal runtime string atlan-{app_name}-production — the token is not interpolated, it survives verbatim into the value. Verified live: scan_text returns no O005 for task_queue = f"atlan-{{app_name}}-production" even though the runtime value still carries the token. This is a false negative on the precise dangerous shape. Path: only exempt JoinedStr pieces that are actual formatted fields (ast.FormattedValue), or detect escaped-brace literals and flag them; add a regression test.
  • Important [RULE] L191 — _sentinel_or_prose_constants matches prose names by substring: any(part in bare for part in _PROSE_NAME_PARTS). A real dispatch template bound to an ALL_CAPS name that merely contains a prose fragment is exempted — verified live: MESSAGE_QUEUE = "atlan-{app_name}-prod" (also HELP_QUEUE, DOC_QUEUE) returns no O005, while TASK_QUEUE correctly fires. The substring test can't tell "human-facing message" from "queue name with MESSAGE in it." Path: match prose names exactly or on delimited token boundaries (e.g. trailing _MESSAGE/_MSG), not substring; add a MESSAGE_QUEUE regression test.
  • Important [REMEDIATION] L73 — the shipped _MESSAGE (and the rule rationale/full_description/docs) direct users to remediate via application_sdk.common.task_queue (derive_task_queue / resolve_manifest_tokens, FND-195). That module does not exist on origin/main — only a private _derive_task_queue in application_sdk/main.py; the public module is added by open, unmerged PR fix(task-queue): derive the queue name once, and stamp it into the served manifest #3101. A user who gets an O005 hit on a released version and follows the message hits an import that doesn't exist. Path: land/release the helper in the same version boundary as O005, or soften the shipped message to lead with the in-scope fixes (f-string / .format(app_name=...)) and name the helper as "available from SDK ≥ the version that ships FND-195" rather than as the canonical target.

packages/conformance/conformance/programs/areas/optimizations.prose.md

  • Important [REMEDIATION] L112 (area file, unchanged in this PR) — the remediation area file carries per-rule fix prescriptions for O001–O004 but not O005. The generic O → optimizations series dispatch routes O005 here, and O005 is autofixable=false, so the remediation loop gets no O005-specific fix/suppression guidance. Convention in this repo is rule+remediation shipped together (the O004-adding commit 11a4f2b landed detector + catalog + prescription in one change). Path: add an O005 prescription covering the unresolved-literal judgment fix, legitimate cross-file resolution + # conformance: ignore[O005] suppression, and the not-yet-shipped helper dependency — in the same PR as the rule.

Class note (one root cause, several siblings)

Findings 1–3 of the [RULE]/[REMEDIATION] set above are instances of a single class: the exemption layer matches surface shape without confirming the {app_name} token actually resolves. Swept siblings that reproduce the same over-exemption: f"{dep}-{{app_name}}", HELP_QUEUE/DOC_QUEUE templates, and a .format(app_name=a) receiver on a template that also carries a second unresolved token ("atlan-{app_name}-{dep}".format(app_name=a) leaves {dep} — though that's a different token, the receiver exemption assumes the whole template resolved). Fix the exemption logic once (resolve-aware, not shape-aware) and the class closes.

Holistic Recommendations

Strengths

  • O005 is registered as a stable O-series WARN rule with BOTH scope and a catalog assertion (test_catalog.py updated to {O001..O005}).
  • Wired through the real optimizations.scan_text path (directive parsing + suppression covered, not just the bare detector).
  • Strong test suite: positive, negative, inline-suppression, nested-DAG, keyword-argument, diagnostic, docstring (incl. PEP 257 attribute docstrings), sentinel, and .format cases — 20 tests pass.
  • Deliberately WARN-tier and non-autofixable, matching the detector's limited cross-file context and keeping the dogfooded SDK gate green (only 4 unsuppressed hits, both in testing/).
  • Correctly keeps genuine dispatch shapes visible: keyword arguments, nested DAG literals, returned templates, and non-prose ALL_CAPS templates (TASK_QUEUE) all still fire.

CI: all passing (sdk-review pending = this run)
Models: Claude Opus (review) + adversarial skipped (GPT-5.3-codex endpoint returned HTTP 429 — deployment on cooldown, retried)
Cross-model agreement: 4/4 findings self-verified against live runtime behavior (adversarial unavailable)
Run: view workflow logs + cost

@mothership-ai

mothership-ai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

🤖 SDK Resolve — round 4. Picked up the latest review: 1 open finding(s) (1 blocking, 0 nit). Fixing it now, then I'll push and re-run @sdk-review automatically — I keep looping until every finding (nits included) is fixed + green CI + READY_TO_MERGE (anything I dispute I hand back to a human with a rationale, rather than merging over it). Progress: https://github.com/atlanhq/application-sdk/actions/runs/31428802458

mothership-ai Bot added a commit that referenced this pull request Aug 10, 2026
…ediation target

Addresses the round-1 review on #3094 (4 findings):

* Escaped-brace f-strings (f"{{app_name}}") are no longer exempt: the
  braces are not interpolated, so the runtime value carries the literal
  token — the exact frozen shape the rule exists to catch. The JoinedStr
  waiver is line-keyed (positions survive the SARIF emitter's re-parse);
  a docstring or diagnostic f-string on the same line stays exempt, and
  a resolving f-string quoting the token in a log call is never re-flagged.
* The prose-name exclusion now matches the trailing delimited segment
  (_MESSAGE, START_MESSAGE, VALIDATION_RATIONALE) instead of any
  substring, so queue templates whose names merely contain a prose
  fragment (MESSAGE_QUEUE, HELP_QUEUE, DOC_QUEUE) fire again.
* The runtime SARIF message and the rule rationale/description no longer
  send users to application_sdk.common.task_queue as if it existed on
  main — that helper ships with FND-195 (#3101, unmerged). The message
  now leads with the always-available fixes (f-string /
  .format(app_name=...)) and names the helper as the canonical target
  only once the FND-195 release lands.
* optimizations.prose.md gains the O005 fix prescription (the generic
  O-series remediation dispatch routes O005 here): in-scope resolution,
  threading from a caller, the version-gated helper, and when a
  cross-file-resolution suppression is legitimate.

Dogfood under the gate's exclude scope is byte-identical to the reviewed
HEAD (13 known testing/ hits, all WARN-tier). 28 O005 tests pass,
including new regressions for each finding.
@mothership-ai

mothership-ai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

@sdk-review

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor
Earlier @sdk-review trigger (click to expand)

🔍 SDK Review (mothership) triggered by @mothership-ai[bot] at 2026-08-10T21:03:12.789Z.

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 completed, cost $8.873450999999998, duration 16m 43s.

@mothership-ai

mothership-ai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

SDK Re-review (mothership): PR #3094 — feat(conformance): add O005 UnresolvedAppNamePlaceholder

Verdict: READY TO MERGE

All four round-1 findings are fixed and verified live against the detector. The exemption algebra is now resolve-aware for the two shapes that mattered (escaped-brace f-strings, prose-suffix constants), the runtime message no longer sends users to an unmerged helper, and the remediation prescription shipped in the same PR. One optional Nit remains: the line-keyed re-flag can over-report when a resolved or diagnostic literal shares a physical line with an escaped-brace f-string — rare, WARN-tier, non-blocking.


Delta from previous review

  • Resolved (4):
    • [RULE] _joined_str_children blanket-exempted escaped-brace f-strings (f"{{app_name}}") — fixed via line-keyed re-flag (_unresolved_joined_str_lines minus diagnostic/docstring lines); verified live: task_queue = f"atlan-{{app_name}}-production" now fires.
    • [RULE] prose-name exclusion matched substrings, exempting MESSAGE_QUEUE-style dispatch templates — fixed to trailing-delimited-segment matching; verified live: MESSAGE_QUEUE / HELP_QUEUE / DOC_QUEUE now fire, _MESSAGE / START_MESSAGE stay silent.
    • [REMEDIATION] _MESSAGE directed users to application_sdk.common.task_queue (FND-195, still unmerged in PR fix(task-queue): derive the queue name once, and stamp it into the served manifest #3101) — message now leads with the always-available f-string / .format(app_name=...) fixes and names the helper as the canonical target only once the FND-195 release lands.
    • [REMEDIATION] optimizations.prose.md had no O005 prescription — now covers in-scope resolution, threading from a caller, the version-gated helper, and when cross-file-resolution suppression is legitimate.
  • Still present (0)
  • New (1): Nit — line-keyed re-flag collision (below).
  • Downgraded (0)

Findings

packages/conformance/conformance/suite/checks/optimizations/_app_name_placeholder.py

  • Nit [RULE] L347 — the re-flag condition is line-keyed: an escaped-brace f-string on a physical line re-flags every exempt token-bearing constant on that line. Verified live: x="atlan-{app_name}".format(app_name=a); y=f"{{app_name}}" reports two findings (col 3 is the already-resolved .format receiver — a false positive), and a diagnostic literal sharing a line with an escaped-brace f-string is re-flagged despite the diagnostic exclusion. Same mechanism flags f"atlan-{{app_name}}".format(app_name=a) (a resolved receiver whose template happens to be an f-string). All shapes are semicolon-separated one-liners or an unusual-but-legal resolution idiom — rare in real connector code, WARN-tier. Path: optional cleanup — track the token-bearing Constant children of each unresolved JoinedStr and re-report only those nodes instead of every exempt constant on a flagged line.

Holistic Recommendations

Strengths

  • All four round-1 fixes verified against live runtime behavior, not just the diff; each fix carries a regression test (58 tests pass in the O005 + catalog files).
  • The exemption layer stays narrow by construction: TASK_QUEUE = "atlan-{app_name}-prod", keyword arguments, nested DAG literals, returned templates, and lowercase bindings all still fire.
  • Rule + remediation prescription + docs + catalog assertion shipped in one PR, matching the repo's pairing convention; WARN tier + autofixable=false matches the detector's cross-file blind spot.
  • Self-scan clean: the checker's own module produces zero unsuppressed O005 hits (the _MESSAGE construction deliberately avoids tripping its own rule).
  • Dogfood impact is honest and bounded: 13 known hits in testing/ (the .replace("{app_name}", ...) resolution sites — the rule's documented blind spot), all WARN-tier, gate green.

CI: all passing (sdk-review pending = this run; Endor Labs queued)
Models: Claude Opus (review) + adversarial skipped (over 70% time budget at Wave 2 decision point)
Cross-model agreement: 5/5 items self-verified against live runtime behavior (4 prior-finding resolutions + 1 new nit)
Run: view workflow logs + cost

atlan-ci
atlan-ci previously approved these changes Aug 10, 2026

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

SDK reviewer's verdict: READY TO MERGE.

Full review summary is in the comment posted on this PR.

@mothership-ai

mothership-ai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

@sdk-review

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor
Earlier @sdk-review trigger (click to expand)

🔍 SDK Review (mothership) triggered by @mothership-ai[bot] at 2026-08-10T21:33:27.909Z.

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 completed, cost $5.049845000000001, duration 10m 38s.

@mothership-ai

mothership-ai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

SDK Re-review (mothership): PR #3094 — feat(conformance): add O005 UnresolvedAppNamePlaceholder

Verdict: READY TO MERGE

The round-2 nit is fixed: the escaped-brace re-flag waiver is now node-identity-keyed instead of line-keyed, so it re-flags only an unresolved f-string's own token-bearing pieces. Verified live — both round-2 false-positive shapes now report exactly one finding, 200 escaped-brace f-strings under GC pressure all still fire, and the checker's self-scan stays clean. One optional Nit remains: a stale docstring sentence referencing the removed line-based waiver. All conformance CI legs are green on this HEAD.


Delta from previous review

  • Resolved (1):
    • [RULE] line-keyed re-flag over-reported exempt constants sharing a physical line with an escaped-brace f-string — fixed via node-identity keying (_unresolved_joined_strs / _diagnostic_joined_strs / _doc_joined_strs + per-piece flagged_pieces collection). Verified live: x="atlan-{app_name}".format(app_name=a); y=f"{{app_name}}" and logger.error("unresolved {app_name}"); y=f"{{app_name}}" now report exactly one finding each, and f"atlan-{{app_name}}".format(app_name=a) stays silent.
  • Still present (0)
  • New (1): Nit — stale docstring sentence in _diagnostic_constants (below).
  • Downgraded (0)

Findings

packages/conformance/conformance/suite/checks/optimizations/_app_name_placeholder.py

  • Nit [RULE] L167 — the _diagnostic_constants docstring still says "the line-based waiver in check_o005 would otherwise re-flag its escaped braces," but the waiver is no longer line-based; the sentence describes a mechanism this PR removed. Path: optional cleanup — reword to reference the escaped-brace re-flag pass, e.g. "Without this, the re-flag pass in check_o005 would re-flag diagnostic f-string pieces."

Strengths

  • The nit fix is verified against live runtime behavior, not just the diff: a 19-shape probe matrix (including both round-2 sibling shapes, multi-line implicit-concat f-strings, nested DAG literals, and mixed interpolation + escaped braces) matches expected verdicts exactly.
  • The id-recycling reasoning is sound and tested: all id-set membership is resolved while walked nodes are alive inside their own pass, flagged piece objects are held in a list until their ids are consumed, and a 200-fstring GC-pressure probe returns exactly 200 findings.
  • The _documentation_constants widening (f-string docstring pieces) matches the old whole-line exemption's behavior while eliminating unrelated same-line collisions; _resolving_format_receivers now covers f-string receivers, closing the f"{{app_name}}".format(app_name=a) gap the line-keyed version flagged.
  • Three regression tests directly cover both previous false-positive shapes and the newly-exempt resolving f-string receiver; 61 tests pass in the O005 + catalog files.
  • Dogfood clean: the checker's own module produces zero unsuppressed O005 hits at this HEAD.

CI: all passing (sdk-review pending = this run; Endor Labs pending)
Models: Claude Opus (review) + adversarial skipped (conformance-only scope)
Cross-model agreement: 1/1 delta change self-verified against live runtime behavior; conformance specialist concurred (1 nit)
Run: view workflow logs + cost

atlan-ci
atlan-ci previously approved these changes Aug 10, 2026

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

SDK reviewer's verdict: READY TO MERGE.

Full review summary is in the comment posted on this PR.

@mothership-ai

mothership-ai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

@sdk-review

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor
Earlier @sdk-review trigger (click to expand)

🔍 SDK Review (mothership) triggered by @mothership-ai[bot] at 2026-08-10T21:44:30.642Z.

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 completed, cost $2.081156000000001, duration 8m 38s.

@mothership-ai

mothership-ai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

SDK Re-review (mothership): PR #3094 — feat(conformance): add O005 UnresolvedAppNamePlaceholder

Verdict: NEEDS REBASE

The PR has merge conflicts with main and cannot be reviewed as-is. Main's feat(conformance): add O006 DirectRocksdictImport rule (#3089) landed on the same conformance catalog, checks-registry, rules, docs, and test files this PR touches, and a test merge of origin/main into this HEAD conflicts in all 5 shared files. Please rebase onto main (or update the branch) and resolve the conflicts, then comment @sdk-review for a fresh review. The prior review round (HEAD bb4f4a3) was READY_TO_MERGE with one optional Nit outstanding; the current HEAD already rewords that docstring, so once the conflicts are resolved the remaining delta should be small.


Delta from previous review

  • Resolved (1):
    • [RULE] stale _diagnostic_constants docstring referencing the removed line-based waiver — HEAD c3dbdae rewords it to reference the escaped-brace re-flag pass.
  • Still present (0)
  • New (1): merge conflicts with main in 5 files (below) — not a code-quality finding, a branch-state blocker.
  • Downgraded (0)

Findings

PR metadata

  • Important [STRUCT] branch state — the PR conflicts with main in packages/conformance/conformance/docs/rules/optimizations.md, packages/conformance/conformance/programs/areas/optimizations.prose.md, packages/conformance/conformance/suite/checks/optimizations/__init__.py, packages/conformance/conformance/suite/rules/optimizations.py, and packages/conformance/tests/test_catalog.py (verified via test merge of origin/main @ 70fe46e9 into HEAD c3dbdae). Path: immediate fix — rebase onto main, resolve the O005/O006 registration and catalog conflicts (both PRs add adjacent entries in the same lists), and re-trigger @sdk-review.

Strengths

  • Prior round's READY_TO_MERGE verdict covered the substantive checker logic; this round's only delta (the docstring reword) directly addresses the one outstanding Nit from that review.

CI: all passing (Endor Labs, Socket green; conformance legs not yet run on this HEAD; sdk-review pending = this run)
Models: Claude Opus (review) + adversarial skipped (branch-state exit before Phase 2)
Cross-model agreement: n/a — rebase exit
Run: view workflow logs + cost

AtMrun and others added 6 commits August 10, 2026 21:55
Flags a plain string literal still carrying an unsubstituted '{app_name}'
token — the shape that shipped a task queue no worker polls and hung
dbt:process to its 24h heartbeat backstop (CONNECT-183).

The substitution this needs has been independently hand-rolled at least
four times across separate codebases (Heracles, native-migration-app,
atlan-local-marketplace-app #539/CONNECT-191, atlan-hightouch-app
ARUN-1039) because no shared, discoverable utility exists yet in the SDK.
Detection is shape-anchored rather than import-anchored for that reason —
it flags the unresolved literal directly instead of the absence of a
canonical helper call.

WARN tier, not autofixable (the correct fix depends on where app_name is
actually available in scope), suppressible via
`# conformance: ignore[O005] <reason>`.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
…ediation target

Addresses the round-1 review on #3094 (4 findings):

* Escaped-brace f-strings (f"{{app_name}}") are no longer exempt: the
  braces are not interpolated, so the runtime value carries the literal
  token — the exact frozen shape the rule exists to catch. The JoinedStr
  waiver is line-keyed (positions survive the SARIF emitter's re-parse);
  a docstring or diagnostic f-string on the same line stays exempt, and
  a resolving f-string quoting the token in a log call is never re-flagged.
* The prose-name exclusion now matches the trailing delimited segment
  (_MESSAGE, START_MESSAGE, VALIDATION_RATIONALE) instead of any
  substring, so queue templates whose names merely contain a prose
  fragment (MESSAGE_QUEUE, HELP_QUEUE, DOC_QUEUE) fire again.
* The runtime SARIF message and the rule rationale/description no longer
  send users to application_sdk.common.task_queue as if it existed on
  main — that helper ships with FND-195 (#3101, unmerged). The message
  now leads with the always-available fixes (f-string /
  .format(app_name=...)) and names the helper as the canonical target
  only once the FND-195 release lands.
* optimizations.prose.md gains the O005 fix prescription (the generic
  O-series remediation dispatch routes O005 here): in-scope resolution,
  threading from a caller, the version-gated helper, and when a
  cross-file-resolution suppression is legitimate.

Dogfood under the gate's exclude scope is byte-identical to the reviewed
HEAD (13 known testing/ hits, all WARN-tier). 28 O005 tests pass,
including new regressions for each finding.
…s (O005 nit)

Round-2 review nit: the line-keyed waiver over-reported when a resolved
or diagnostic literal shared a physical line with an escaped-brace
f-string — `x="atlan-{app_name}".format(app_name=a); y=f"{{app_name}}"`
reported two findings (the already-resolved receiver was a false
positive), and `f"{{app_name}}".format(app_name=a)` was flagged despite
resolving at runtime.

The waiver is now node-keyed on the escaped-brace f-string's own
token-bearing Constant pieces, collected by object identity in a single
walk (an AST node's id is only stable while the object is alive, so an
id-set built across sequential walks can be silently corrupted by id
recycling). A piece another exclusion independently covers — a docstring
or diagnostic f-string piece, or a .format(app_name=...) receiver —
keeps its exemption, and a f-string receiver of a resolving .format call
is exempted alongside plain-literal receivers.

Regression tests pin all three nit shapes; the full O005 matrix (31
tests) and the 2186-test conformance suite pass.
…e-flag rework

The _diagnostic_constants docstring referenced the line-based waiver that
bb4f4a3 replaced with node-identity keying; point it at the re-flag pass
instead (round-3 review nit).
Regenerated via gen-rule-docs so the catalog page carries both O005
(this PR) and O006 (#3089, landed on main) after the rebase.
@mothership-ai
mothership-ai Bot force-pushed the mrun/fnd-184-conformance-o005 branch from c3dbdae to 1e05c7b Compare August 10, 2026 21:58
@mothership-ai

mothership-ai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

@sdk-review

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

🔍 SDK Review (mothership) triggered by @mothership-ai[bot] at 2026-08-10T21:59:22.787Z.

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 completed, cost $4.800601, duration 9m 19s.

@mothership-ai

mothership-ai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

SDK Re-review (mothership): PR #3094 — feat(conformance): add O005 UnresolvedAppNamePlaceholder

Verdict: READY TO MERGE

The rebase is clean. This round reviewed only the delta since the last reviewed HEAD (c3dbdae) — 173 lines where O005 was merged into a tree that had independently landed the sibling rule O006 DirectRocksdictImport (#3089). Both rules are registered, wired into scan_text, and asserted by the catalog tests; no content was lost or duplicated in the shared files. The substantive O005 detector and its behavior tests were approved in the earlier round (bb4f4a3) and are unchanged by this delta. The prior round's branch-state blocker (merge conflicts with main) is resolved — the PR is now mergeable.


Delta from previous review

  • Resolved (1):
    • [STRUCT] branch state — the merge conflicts with main in the 5 shared O005/O006 files are resolved; the rebase integrated both rules (verified: catalog holds O001–O006, scan_text calls check_o005 + check_o006, 61 targeted catalog + O005 behavior tests pass).
  • Still present (0)
  • New (0)
  • Downgraded (0)

Findings

None.

Strengths

  • Rebase integrated O006 without losing O005: both rules registered, both checks imported and invoked by scan_text, no duplicate rule IDs.
  • O005 registered correctly — stable ID, scope both, tier warn, category dag-write-path; O006 stays app-scoped. Catalog tests assert the right sets (app-scoped set has O006, not O005; O-series set has both).
  • Remediation prose for O005 is paired in the same PR, routed via the O-series area facet, classified "judgment", and correctly hedges the application_sdk.common.task_queue helper as shipping with FND-195 rather than pointing users at an import that does not exist on their release.
  • Detector has positive, negative, suppression, and exclusion tests, including the narrow-exclusion cases (ALL_CAPS queue templates still flagged, prose-named constants exempt, escaped-brace f-strings flagged, sibling literals not re-flagged).

CI: all passing (Endor Labs, Socket, CodeQL, Pre-commit, and every Conformance leg — Optimizations, Suite unit tests, Conformance Gate — green on this HEAD; sdk-review pending = this run)
Models: Claude Opus (review) + adversarial skipped (Wave 1 produced zero findings — nothing to challenge)
Cross-model agreement: n/a — no findings to challenge
Run: view workflow logs + cost

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

SDK reviewer's verdict: READY TO MERGE.

Full review summary is in the comment posted on this PR.

@mothership-ai

mothership-ai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

🤖 SDK Resolve — final report. @cmgrote this PR is merge-ready — handing back to a human for the merge call.

Outcome: MERGE-READY — green required CI + zero @sdk-review findings (nits included) + verdict READY_TO_MERGE on HEAD 1e05c7b9.

Note on reviewers: NO reviewer list is configured for this run (vars.SDK_RESOLVE_REVIEWERS is unset), so I requested review from you as the @sdk-resolve requester — please assign the right human reviewer if that's not you.

Rounds: 5 | Findings fixed: 7 | Dismissed: 0

Round Verdict Findings Action
1 NEEDS_FIXES 4 blocking All 4 fixed (see below)
2 READY_TO_MERGE 1 nit Fixed (node-identity-keyed waiver)
3 READY_TO_MERGE 1 nit Fixed (docstring reword)
4 NEEDS_REBASE 1 struct Rebased onto main (O006 landed); conflicts resolved
5 READY_TO_MERGE 0

Round 1 (4 findings):

  • Escaped-brace f-strings (f"{{app_name}}") were blanket-exempted even though the braces are not interpolated — the runtime value carries the literal token. Now flagged (with docstring/diagnostic f-strings still exempt).
  • Prose-name exclusion matched substrings, so MESSAGE_QUEUE/HELP_QUEUE/DOC_QUEUE dispatch templates were silently exempted. Now matches the trailing delimited segment only.
  • The runtime SARIF message pointed users to application_sdk.common.task_queue (FND-195), which didn't exist on any released SDK — it led with the helper as the canonical fix. Reworded to lead with the always-available f-string / .format(app_name=...) fixes and name the helper as landing with the FND-195 release. (FND-195 has since merged as fix(task-queue): derive the queue name once, and stamp it into the served manifest #3101.)
  • optimizations.prose.md had no O005 remediation prescription — added (in-scope resolution, threading from a caller, the version-gated helper, legitimate cross-file suppression).

Round 2 (1 nit): the line-keyed re-flag over-reported exempt constants sharing a physical line with an escaped-brace f-string (a resolved .format receiver, a diagnostic literal). Reworked to node-identity keying on the f-string's own token-bearing pieces; f"{{app_name}}".format(app_name=a) receivers are exempt too.

Round 3 (1 nit): stale docstring sentence referencing the removed line-based waiver — reworded.

Round 4 (1 struct): O006 DirectRocksdictImport (#3089) landed on main touching the same 5 files — rebased, resolved the O005/O006 registration + catalog conflicts (both rules registered, catalog asserts O001–O006), regenerated the rule docs.

Final CI: all checks pass (no failing, none pending) on 1e05c7b9. Not merging — that's the human gate.

@cmgrote
cmgrote added this pull request to the merge queue Aug 10, 2026
Merged via the queue into main with commit d4ff03d Aug 10, 2026
44 checks passed
@cmgrote
cmgrote deleted the mrun/fnd-184-conformance-o005 branch August 10, 2026 22:13
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