fix(handler): filesystem-resolve bundle marketplace entrypoints for input-contract and bare-manifest - #3090
Merged
Merged
Conversation
…nput-contract and bare-manifest
Bundle ("uber") apps expose marketplace entry points only as generated
contract dirs (app/generated/<ep>/manifest.json) whose DAGs are computed
per submission; their @entrypoint registrations are the DAG-node
workflows (<ep>-post, <ep>-build-raw-sql, ...), never the marketplace
name. Registry-driven resolution therefore 404s:
- GET /workflows/v1/input-contract?entrypoint=<ep> -> 404 "No input
contract for entrypoint", which breaks Heracles' /v1/app creation for
every bundle entry point (a production RCA: the caller's
retry-without-entrypoint then validated against a sibling DAG-node's
schema and died on the bare manifest call)
- bare GET /workflows/v1/manifest -> 404 "No manifest available" (the
registry-derived candidates are DAG-node workflows with no manifest
dirs), breaking submit-time pre-validation for bundles
The manifest and configmap routes are already filesystem-first for
exactly this layout; this gives the remaining two surfaces the same
authority:
- input-contract: on a registry miss for a named entrypoint whose
generated dir exists, serve AppInputContract from
app.generated.<ep_snake>._input (in-place) or app.<ep_snake>._input
(the bundle regen relocation path — kebab generated dirs are not
importable, so regen moves _input.py into the entrypoint package)
- bare manifest: append the on-disk entry points (alphabetical) to the
fallback candidates so a bundle serves deterministically
Registry hits keep winning; apps without generated dirs are unchanged.
Aryamanz29
requested review from
OnkarVO7,
atlan-ci,
cmgrote,
louisnow and
vaibhavatlan
as code owners
August 10, 2026 11:05
Contributor
📜 Docstring Coverage ReportRESULT: PASSED (minimum: 30.0%, actual: 78.3%) Detailed Coverage ReportThis message was truncated. Download full message |
Contributor
📦 Trivy Vulnerability Scan Results
Report Summary
Scan Result Detailspackages/conformance/uv.lockrequirements.txtuv.lock |
Contributor
📦 Trivy Secret Scan Results
Report Summary
Scan Result Detailspackages/conformance/uv.lockrequirements.txtuv.lock |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
☂️ Code Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
Member
Author
|
@sdk-resolve |
…injection) The route validates the name before calling, but the helper must be locally safe: regex-reject path metacharacters and prove containment under CONTRACT_GENERATED_DIR before touching disk.
Contributor
|
🤖 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. |
![mothership[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…ution CodeQL's taint engine does not model pathlib.is_relative_to as a py/path-injection sanitizer, so the prior .resolve()+is_relative_to guard still raised 2 high alerts. Switch to the repo's proven pattern (mirrors _validated_temp_path): os.path.realpath + explicit prefix containment, which CodeQL recognizes as a sanitizer. Behavior is unchanged — the regex still rejects path metacharacters and the resolved path must stay under CONTRACT_GENERATED_DIR.
Contributor
|
@sdk-review |
Contributor
Earlier @sdk-review trigger (click to expand)🔍 SDK Review (mothership) triggered by @mothership-ai[bot] at 2026-08-10T11:33:23.875Z. 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 |
Contributor
SDK Review (mothership): PR #3090 — fix(handler): filesystem-resolve bundle marketplace entrypoints for input-contract and bare-manifestVerdict: READY TO MERGE
FindingsNo findings. Two candidate findings from the review agents were verified against the code and withdrawn:
Holistic RecommendationsNone — the approach is correct. One optional (non-blocking) observation: Strengths
CI: all passing |
Contributor
|
🤖 SDK Resolve — final report. This PR is merge-ready; a human needs to take it from here (resolver never merges). Outcome: merge-ready ✅ — green CI, zero open findings (nits included), verdict Details:
What is left for you:
@Aryamanz29 — your turn. |
cmgrote
reviewed
Aug 10, 2026
cmgrote
left a comment
cmgrote
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the RCA write-up — the table showing which of the four routes work today is exactly the right framing, and the input-contract 404 is a real break that needs unblocking.
My concern is not this diff's correctness but its direction, and I think there's a fact that changes the calculus: App.pkl already declares every entry point explicitly, and nothing in the SDK runtime reads that declaration.
The declaration already exists
open class Entrypoint(contract-toolkit/src/App.pkl:1514) carriesname— the marketplace name, i.e. precisely the identity_resolve_app_entrypointcan't see — pluspackageId,contract, and the display metadata.packageIdalready encodes the distinction this PR is trying to recover by asking "does a directory exist": per its own doc, "Entrypoints without apackageIdare always routable via?entrypoint=but do not appear as marketplace cards."effectiveEntrypoints(:3473) synthesises the implicit entry point for single-entrypoint apps from the bundle-level fields, so the list is uniformly populated for both the flat and nested shapes.renderEntrypoint(:3387) emits it intoatlan.yaml, and the doc comment at:201says the block is "still emitted for routing".
Meanwhile AppRegistry is built solely from _scan_entrypoints() in application_sdk/app/_ep_registration.py — @entrypoint decorators only. grep atlan.yaml application_sdk/ returns nothing but Dapr-drift log strings in main.py.
So CONTRACT_GENERATED_DIR.glob("*/manifest.json") infers at request time a list the contract already states declaratively one file over. The same is true of #2776's alphabetical registry walk. We have two registries with different membership — the decorator registry and the app/generated/*/ filesystem — and #2764, #2776 and this PR each teach one more route to guess at the other. Bare /manifest is now at 7 candidate sources; /input-contract at 5 module candidates.
The glob isn't a faithful proxy for the declaration
Those per-entry-point dirs are written only when (entrypoint.contract != null) and gated on emitGeneratedArtifacts (App.pkl:3656). With that flag off, entry points stay declared and routable while the new fallback silently finds nothing — a 404 that looks identical to the one being fixed here, with no signal pointing at the cause. The glob also can't distinguish a marketplace card from a DAG-node artifact dir. packageId can.
One specific request on this diff
I'd take the input-contract half — it unblocks the break, and the two-import-path guesswork is bounded and documented.
I'd drop candidate #7 (the bare-manifest disk glob). Picking alphabetically among all on-disk entry points means a bundle with N marketplace entry points now answers a bare GET /manifest with a confidently wrong manifest instead of 404-ing. The PR frames that as "deterministic instead of 404-ing", but deterministic-and-arbitrary isn't correct — for submit-time pre-validation, "you must specify an entrypoint" is the more honest answer, and a wrong-manifest bug will be considerably harder to diagnose than the 404 it replaces.
Follow-up
Filed as FND-180 for the durable version, which is small and retires far more than it adds:
- Toolkit emits
effectiveEntrypointsasapp/generated/entrypoints.json. It has to go there rather than being read fromatlan.yaml—atlan.yamlisn't reliably in the runtime image (the app template doesCOPY . ., but several apps copy onlyapp/), whereasapp/generated/ships in every case. - Toolkit emits each entry point's input schema as JSON next to its
manifest.json, so/input-contractstops importing Python at request time — noimportlib, nosys.modulesmutation in tests, no circular-import dance, no cold-start cost. That also makes the kebab-dir_input.pyrelocation irrelevant to the SDK, which is worth doing on its own: conformance already carries a duplicate(root / "app").rglob("_input.py")fallback inchecks/sdr.py:704for the same relocation. - One resolver reads the declared list; the decorator registry keeps its distinct job of saying which method runs for the entry points that have one.
That splits the two questions cleanly — the contract owns which entry points exist, the registry owns what executes — and lets us delete the #2764 flat fallback, the #2776 alphabetical walk, and both branches added here.
Not blocking on the follow-up, and no objection to landing the unblock under time pressure. Two asks: drop candidate #7, and add a comment pointing at FND-180 on the code that stays so the next person hitting this doesn't add candidate #8.
Per review: serving one of N marketplace manifests picked alphabetically on a bare /manifest call is confidently wrong rather than helpfully deterministic — callers must name the entry point, and the honest answer when they don't is the existing 404. The input-contract half (the actual /v1/app break) stays. The declarative resolver that can answer the bare call properly — toolkit-emitted entrypoints.json — is FND-180.
Member
Author
|
Thanks @cmgrote — agreed on all three points, and the review request is applied as of 4cd62d5:
+1 on FND-180's shape — the |
Collaborator
|
@sdk-resolve |
Contributor
|
🤖 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. |
Member
Author
Live validation on a test tenant (branch image
|
| Route | Before (main-7e73524) | After (this PR + application-sdk#3090) |
|---|---|---|
GET /workflows/v1/input-contract?entrypoint=asset-export-basic |
404 "No input contract for entrypoint" | 200 — serves the new AppInputContract |
GET /workflows/v1/input-contract?entrypoint=admin-export |
404 | 200 |
GET /workflows/v1/input-contract?entrypoint=asset-export-advanced |
404 | 200 |
GET /workflows/v1/manifest?entrypoint=asset-export-basic |
200 | 200 (unchanged) |
GET /workflows/v1/input-contract?entrypoint=does-not-exist |
404 | 404 (contract preserved) |
The first row is the exact break behind the POST /v1/app 500 (app validation error: 404 - No manifest available): Heracles' create flow died on the contract fetch, and the fallback path then hit the bare manifest 404.
End-to-end POST /v1/app create+run on the test tenant is running next; result will be posted here.
Contributor
|
@sdk-review |
Contributor
Earlier @sdk-review trigger (click to expand)🔍 SDK Review (mothership) triggered by @mothership-ai[bot] at 2026-08-10T13:25:37.549Z. 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 |
Contributor
|
The review environment was temporarily unreachable. This is usually transient. Please retry by commenting |
Collaborator
|
@sdk-resolve |
Contributor
|
🤖 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. |
Contributor
|
All 3 VPN connection attempts to Please retry by commenting |
Collaborator
|
@sdk-resolve |
Contributor
|
🤖 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. |
Collaborator
|
@sdk-review |
Contributor
Earlier @sdk-review trigger (click to expand)🔍 SDK Review (mothership) triggered by @cmgrote at 2026-08-10T14:52:22.555Z. 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 |
5 tasks
Collaborator
|
@sdk-resolve |
Contributor
|
🤖 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. |
Contributor
|
@sdk-review |
Contributor
Earlier @sdk-review trigger (click to expand)🔍 SDK Review (mothership) triggered by @mothership-ai[bot] at 2026-08-10T17:26:55.063Z. 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 |
Contributor
SDK Re-review (mothership): PR #3090 — fix(handler): filesystem-resolve bundle marketplace entrypoints for input-contract and bare-manifestVerdict: READY TO MERGE
Delta from previous review
FindingsNone. Holistic RecommendationsNone — the error predicate is the right shape: Strengths
CI: no failures (sdk-review pending — this run; several optional legs skipped as usual) |
atlan-ci
previously approved these changes
Aug 10, 2026
atlan-ci
left a comment
atlan-ci
left a comment
Collaborator
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.
Contributor
|
🤖 SDK Resolve — final report. This PR is merge-ready; a human needs to take it from here (the resolver never merges). Outcome: merge-ready ✅ — green CI, zero open findings (nits included), verdict
Rounds this run: 1 review round (round 3 overall on this PR). Findings:
One human follow-up (non-blocking): the - name: Warn on conformance discovery failure
if: steps.download.outputs.discovery_error == 'true'
run: |
echo "::warning::Conformance SARIF discovery failed (gh run list / artifact probe error) — dashboard may be stale."Final CI: all required checks pass on HEAD Merge is yours — I do not merge. @cmgrote — it's your turn. |
`fetch_conformance_sarif.py` now emits a distinct `discovery_error` output so a failed `gh run list` / artifact probe is separable from a repo that genuinely has no conformance data yet. Consume it: annotate the run with a warning when discovery itself failed, so a stale conformance panel is attributable instead of silently indistinguishable from "nothing to publish". The step could not land with the rest of the change — pushes touching `.github/workflows/**` are rejected for the resolver app (no `workflows` permission), so the output was inert until now.
Collaborator
|
@sdk-review |
Contributor
|
🔍 SDK Review (mothership) triggered by @cmgrote at 2026-08-10T17:49:02.669Z. 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 |
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.
Contributor
SDK Re-review (mothership): PR #3090 — fix(handler): filesystem-resolve bundle marketplace entrypoints for input-contract and bare-manifestVerdict: READY TO MERGE
Delta from previous review
FindingsNone. Holistic RecommendationsNone. This closes the loop opened two rounds ago: (1) discovery stopped conflating "no data" with "discovery broke" by probing newest-first and exposing Strengths
CI: no failures |
cmgrote
enabled auto-merge
August 10, 2026 18:03
cmgrote
approved these changes
Aug 10, 2026
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
4 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.
Problem
Bundle ("uber") apps expose marketplace entry points only as generated contract dirs (
app/generated/<ep>/manifest.json) whose DAGs are computed per submission viacompute_manifest. Their@entrypointregistrations are the DAG-node workflows (<ep>-post,<ep>-build-raw-sql, …) — never the marketplace name. Registry-driven resolution therefore fails on live bundles (reproduced on a tenant against the deployed app):GET /workflows/v1/manifest?entrypoint=<ep>GET /workflows/v1/input-contract?entrypoint=<ep>GET /workflows/v1/input-contractThe input-contract 404 breaks Heracles'
POST /v1/appcreation for every bundle entry point (Sev1 RCA): the create flow fetches the input contract with the entrypoint → 1003 → the eventual manifest fetch dies with "No manifest available".Fix (scoped per review)
input-contract only: on a registry miss for a named entrypoint whose generated dir exists, serve
AppInputContractfromapp.generated.<ep_snake>._input(in-place) orapp.<ep_snake>._input(the bundle regen relocation path — kebab generated dirs aren't importable, so regen scripts move_input.pyinto the entrypoint package). Entry-point names are regex-validated and containment-proven underapp/generated/before touching disk.Registry hits keep winning; apps without generated dirs are byte-for-byte unchanged — the fallback can only turn this specific 404 into a 200.
Per @cmgrote's review, the earlier bare-manifest disk-glob fallback was dropped (serving one of N marketplace manifests picked alphabetically is confidently wrong; the honest answer is the existing 404) — a regression test now pins that behavior. The durable design — toolkit-emitted
app/generated/entrypoints.json+ static JSON schemas, one declarative resolver, delete the #2764/#2776 fallbacks and this one — is FND-180.Tests
4 tests in
TestBundleMarketplaceEntrypoints(disk-resolved contract, both 404-preservation cases, bare-manifest-stays-404). Full unit suite green.Validation
Deployed to a test tenant via a companion app build: all three bundle entry points' input contracts now serve 200 (were 404), unknown entrypoints still 404, manifest route unchanged — see the results table on atlanhq/atlan-csa-uber-app#171.
Merge train (this PR is the head)
AppInputContractclasses this route serves; currently branch-pinned to this PR, swaps to the released SDK before merge./v1/apppath works once (2) is deployed on a tenant.