Repository navigation
fix(conformance): certify gates fail closed (deps install, whole-tree contract drift via SDK regeneration), keep the GitHub token out of app code across certify and the CI reusables, and move conditional shell out of run: blocks [FND-4244] - #4731
Conversation
|
/lens |
lens summary · updated after round 10 (re-review · only commits since 0c8e21d)✅ Ready to merge — every finding is resolved Open findings: 🔴 0 critical · 🟠 0 high · 🟡 0 medium · ⚪ 0 low (nit) lens reads this PR as — From the intent and changed workflow/script symbols, this PR addresses fail-open certification when dependency installation fails, contract-drift checks that could miss generated changes, unguarded conditional shell in CI, and overly broad GitHub credentials reaching dependency-build paths. It also applies the credential change to reusable CI workflows and image builds. How: It makes install outcomes part of the certification verdict, regenerates contracts through the SDK’s shared regeneration function and checks the full working tree, adds a CI guard while moving branching workflow logic into Python scripts, and replaces broad tokens with read-only App tokens scoped to declared private repositories. The mirror-clone step is separated from dependency installation so app build backends and later test steps do not inherit that token. Approach check — sound. ✔️ Resolved (21)
🕘 Round history (17)
round 10 · $0.806 of $1.00 · 302 model calls · 0 failed requests · 75% prompt cache hits · gpt-6-luna |
…tion Keep the Slack webhook and publish token out of curl argv, fail the gitleaks gate closed on an unreadable report, return no PR number when none is open, bound sync_dapr_version's commands with timeouts, group SDR workflow links by per-class summary file, and tighten the guard's comment, continuation and ceiling handling.
|
/lens recheck F-24f867 F-5069cc Both behaviours are unchanged from the merge base. This PR moves inline shell into scripts and does not change behaviour. On main, .github/workflows/build-and-publish-app.yaml:753 warns and exits 0 when poe is unavailable, and :867 and :893 warn on a failed app install and exit 1 only when CD or UT is "failure". certify.py reproduces this exactly. Failing closed here would block publishing for several fleet apps today. It is a deliberate gate-policy change, tracked separately in Linear, and is not a regression introduced by this PR. |
|
❌ lens · re-check after round 1 · F-24f867, F-5069cc by @cmgrote — 4 blocking: F-a1d718, F-04dbed, F-24f867, F-5069cc Open findings: 🔴 1 critical · 🟠 3 high · 🟡 5 medium · ⚪ 3 low (nit) Approach check —
$0.168 of $1.00 · 58 model calls · 0 failed requests
|
|
/lens |
|
❌ lens · round 2 · re-review · only commits since 434662a — 2 blocking: F-24f867, F-5069cc Open findings: 🔴 0 critical · 🟠 2 high · 🟡 0 medium · ⚪ 0 low (nit) Approach check — ✅ concerns addressed
$0.206 of $1.00 · 75 model calls · 0 failed requests |
…whole working tree [FND-4539, FND-4700] (#4736)
The pyright baseline from main (FND-4245) fails any finding in a file it does not list, and every script and test this PR adds was new to it. - Tests import build_and_publish modules through the package under TYPE_CHECKING, so pyright resolves them; the runtime imports are unchanged. - The sdr-e2e tests load their modules with importlib: the action directory has a hyphen and cannot be a package. - Annotate test parameters and type the parsed JSON/YAML data.
|
/lens |
|
✅ lens · round 3 · re-review · only commits since 88e9a66 — ready to merge — every finding resolved · $0.26 Open findings: 🔴 0 critical · 🟠 0 high · 🟡 0 medium · ⚪ 0 low (nit) Approach check — ✅ concerns addressed
$0.255 of $1.00 · 92 model calls · 0 failed requests |
atlan-ci
left a comment
There was a problem hiding this comment.
lens: ready to merge — every finding at every level is resolved (round 3).
…e generate The certify contract-drift check ran each app's own `poe generate`, so it was a second generator that disagreed with the generated-freshness check and Renovate's toolkit bump, and it ran app-defined shell in a job that holds write tokens. It now calls renovate_pkl_sync.regenerate, the function the freshness check already uses (pkl eval, then contract/post-generate.sh, then ruff), and still requires a clean working tree afterwards. - No poe requirement: an app is no longer failed for lacking poethepoet. - A contract that does not regenerate, or a regeneration that cannot run, fails closed. - Drift needs pkl, not the app's dependencies, so it no longer waits on the app install; the pkl CA-bundle seed step (only for poe tasks) goes. - Tests pin that certify uses the freshness check's exact function and that no certify step runs poe. Claude-Session: https://claude.ai/code/session_01AbKqGbjx7uUk6aM6jzSxNF
|
❌ lens · round 8 · re-review · only commits since 21bbd0a — 1 blocking: F-6e4829 Open findings: 🔴 1 critical · 🟠 0 high · 🟡 0 medium · ⚪ 0 low (nit) Approach check — ✅ sound
$0.736 of $1.00 · 273 model calls · 0 failed requests |
Lens F-6e4829 is anchored at the mint's repositories: line, and its verify step only sees the lines around it, not git_auth.py where ALLOWED_PRIVATE_DEPS is enforced (4f69f82). Say at every mint what caps the scope, and name the repos step for what it does.
|
/lens recheck F-6e4829 |
|
✅ lens · re-check after round 8 · F-6e4829 by @cmgrote — ready to merge — every finding resolved · $0.74 Open findings: 🔴 0 critical · 🟠 0 high · 🟡 0 medium · ⚪ 0 low (nit) Approach check — ✅ sound
$0.738 of $1.00 · 274 model calls · 0 failed requests
|
|
/lens |
|
lens: nothing to review — round cap (5) reached; comment |
|
/lens force |
|
❌ lens · round 9 · re-review · only commits since 4f69f82 — 1 blocking: F-b29afb Open findings: 🔴 0 critical · 🟠 1 high · 🟡 0 medium · ⚪ 0 low (nit) 🟠 F-b29afb · high · bug — SDK helper checkouts use the caller token and failWhere: On a consumer-app run with Approach check — ✅ sound
$0.779 of $1.00 · 292 model calls · 0 failed requests |
atlanhq/application-sdk is public, so a consumer app's GITHUB_TOKEN can clone it from the reusable workflow; build-decision's identical tokenless checkout already succeeds in consumer runs. Correct the certify comment that claimed otherwise.
|
/lens recheck F-b29afb |
|
❌ lens · re-check after round 9 · F-b29afb by @cmgrote — 1 blocking: F-b29afb Open findings: 🔴 0 critical · 🟠 1 high · 🟡 0 medium · ⚪ 0 low (nit) Approach check — ✅ sound
$0.781 of $1.00 · 293 model calls · 0 failed requests
|
|
/lens recheck F-b29afb Public repo: GET /repos/atlanhq/application-sdk gives visibility=public, and any token clones a public repo. Main already has this identical tokenless checkout in build-decision, and it succeeds in consumer Build & Publish runs (atlan-mysql-app 42560901721, atlan-bigquery-app 42258810961). |
|
Open findings: 🔴 0 critical · 🟠 1 high · 🟡 0 medium · ⚪ 0 low (nit) Approach check — ✅ sound
$0.785 of $1.00 · 294 model calls · 0 failed requests
|
|
/lens recheck F-b29afb Public repo: GET /repos/atlanhq/application-sdk gives visibility=public, and any token clones a public repo. Main already has this identical tokenless checkout in build-decision, and it succeeds in consumer Build & Publish runs (atlan-mysql-app 42560901721, atlan-bigquery-app 42258810961). |
|
Open findings: 🔴 0 critical · 🟠 1 high · 🟡 0 medium · ⚪ 0 low (nit) Approach check — ✅ sound
$0.789 of $1.00 · 295 model calls · 0 failed requests
|
|
/lens recheck verify-budget F-b29afb Public repo: GET /repos/atlanhq/application-sdk gives visibility=public, and any token clones a public repo. Main already has this identical tokenless checkout in build-decision, and it succeeds in consumer Build & Publish runs (atlan-mysql-app 42560901721, atlan-bigquery-app 42258810961). |
|
❌ lens · re-check after round 9 · F-b29afb by @cmgrote — 1 blocking: F-b29afb Open findings: 🔴 0 critical · 🟠 1 high · 🟡 0 medium · ⚪ 0 low (nit) Approach check — ✅ sound
$0.796 of $1.00 · 296 model calls · 0 failed requests
|
|
/lens dismiss F-b29afb False positive: application-sdk is public; the identical tokenless checkout lens quotes (L197-204, build-decision) is unchanged from main and succeeds in consumer runs (atlan-mysql-app 42560901721, atlan-bigquery-app 42258810961). |
|
✅ lens · dismissal after round 9 · F-b29afb by @fyzanshaik-atlan — ready to merge — every finding resolved · $0.80 Open findings: 🔴 0 critical · 🟠 0 high · 🟡 0 medium · ⚪ 0 low (nit) Approach check — ✅ sound
$0.796 of $1.00 · 296 model calls · 0 failed requests |
|
/lens force |
|
✅ lens · round 10 · re-review · only commits since 0c8e21d — ready to merge — every finding resolved · $0.81 Open findings: 🔴 0 critical · 🟠 0 high · 🟡 0 medium · ⚪ 0 low (nit) Approach check — ✅ sound
$0.806 of $1.00 · 302 model calls · 0 failed requests |
atlan-ci
left a comment
There was a problem hiding this comment.
lens: ready to merge — every finding at every level is resolved (round 10).
Fixes FND-4244, FND-4538, FND-4539 FND-4700, FND-4739, FND-4740 and FND-4744. #4738 (FND-4538), #4736 (FND-4539 + FND-4700, with #4757 squashed in), #4787 (FND-4739), #4790 (FND-4740) and #4792 (FND-4744) are merged into this branch. Do not merge until the rollout prerequisites are done: callers use
build-and-publish-app.yaml@mainand the reusables and composites@main, so everything here goes live for about 71 app repos the moment it lands.Summary
run:" rule. The three worst files are migrated into tested Python scripts.apt_packagesinput.poe generate, and fails closed when the contract doesn't regenerate.generated/.certifyjob token is read-only.git_tokenBuildKit secret is a read-only App token, notORG_PAT_GITHUB.FND-4244: guard the no-conditional-shell rule and migrate the worst workflows
docs/standards/ci.mdbans branching shell inrun:blocks, but nothing checked it.Guard:
.github/scripts/tests/test_no_conditional_run_blocks.pyaction.y*ml. actionlint skips composite actions, so they had no check before.if/for/while/until/case/select, and on a test chained with&&/||([ … ] && x,test -n "$X" || { …; }).${{ }}are blanked before matching. Inline heredoc Python is scanned too.CI script testsworkflow, which has no path filter, so it runs on every PR.Allowlist:
.github/scripts/conditional_run_allowlist.txtALLOWLIST_CEILINGcaps the size.Migrations (stdlib Python; every external command goes through a stub-able wrapper, and every branch has a test):
build-and-publish-app.yaml.github/scripts/build_and_publish/{publish,unit_tests_gate,certify,leak_scan,prepare,manifest}.py.github/actions/sdr-e2e/action.yaml.github/actions/sdr-e2e/{sdr_steps,sdr_report}.pycheck-dapr-version.yaml.github/scripts/sync_dapr_version.py/tmp/sdr-e2ebefore setup-deps wipes the workspace.repin-application-sdk.shnow has a single copy, inconnector-integration-tests/. A test enforces that.FND-4538: fail the certify gate closed when an app's dependencies don't install
When
uv syncfailed, contract drift and unit tests were skipped and the verdict still passed. Only a "Certification incomplete" warning appeared.certify.py verdictnow blocks publish when the app install fails, incertifyor in anycertify-unitleg. An app with notests/unitstill passes.certify_unit_shards.py verdictreportsinstall_failedinstead of folding it intoskipped.certifyandcertify-unitsetUV_PYTHONfromcontainer_python_version.py print-version, which matches the app image's Python. The runner's system Python is newer, and wheel-less pins (for examplepyarrow==20.0.0) built from source and failed or timed out.apt_packagesinput (default empty), installed beforeuv syncin both jobs. The guard is byte-identical to tests-reusable's.FND-4539: drift regenerates with the SDK, never the app's
poe generateThe gate used to run each app's own
poe generate. That made it a second generator, separate from the one the generated-freshness check and Renovate's toolkit bump use, so apps could pass one check and fail the other. It also ran app-defined shell in a job that holds write tokens.The gate now:
renovate_pkl_sync.regenerate, the exact functioncheck_generated_freshness.pyuses:pkl eval, thencontract/post-generate.shif present, then ruff on generated Python. A test pins that it is the same function.poe, and doesn't require it. A wiring test fails if any certify step runspoe.pkl. The pkl CA-bundle seed step existed only forpoetasks, so it's removed.Any post-processing an app does to generated output must now live in the contract's output paths or in
contract/post-generate.sh.FND-4700: the drift gate checks the whole working tree
The stale check was
git diff --exit-code -- generated/from the app root, which missed two things:generated/, notapp/generated/or root files likeatlan.yaml.git diffignores untracked files.After regeneration, the gate now fails when
git status --porcelain --untracked-files=allis non-empty. The::error::lists the paths (capped at 50), never their contents. Gitignored files don't count.FND-4739: keep the GitHub token out of app code (squashed from #4787)
certifyandcertify-unitused to write the fleet App token into the runner's global git config, falling back toORG_PAT_GITHUBwhen the App token was missing. It stayed there for the rest of the job, so unit tests and coverage could read it.certifyalso inheritedpackages,actionsandpull-requests: write, and its checkouts left the job token in.git/config.certify.py install-depsruns through the shared.github/scripts/git_auth.py: it mirror-clones the app's private deps with the token, then runsuv syncagainst the local mirrors with no token in uv's env (see Token never reaches uv). Nothing is written to git config, and the separate "Configure git auth" steps are gone.--all-groups. Tests and coverage runuv run --frozen, so nothing after the install fetches.ORG_PAT_GITHUBfallback for the install credential. Build backends run during the install, and the PAT is org-admin. The SDK checkout keeps its fallback, because no app code runs there.permission-contents: read.certify's job token iscontents: read, and both its checkouts usepersist-credentials: false.Inputs and secrets are unchanged for callers.
FND-4740: keep the GitHub token out of app code in the CI reusables (squashed from #4790)
For an app with a private atlanhq git dependency, the unit, integration and e2e jobs in
tests-reusable.yaml, pre-commit inchecks-reusable.yaml, the conformance suite andrelease-version-bump.yamlwroteORG_PAT_GITHUB(org-admin) into~/.gitconfigbeforeuv sync. Every later step could read it, including tests, pre-commit hooks and conformance series that import app code. All six jobs also exposed the PAT as job-levelenv: GIT_TOKEN, opted in or not.permission-contents: read, only when the caller opts in. If it doesn't mint, a guard step fails namingFLEET_APP_IDandFLEET_APP_PRIVATE_KEY..github/scripts/git_auth.pymirror-clones the private deps with the token and runs the fetching command against the mirrors with no token in its env (see Token never reaches uv). Nothing is persisted. Once one authenticated sync has filled the uv cache and venv, lateruv sync/uv run/uv lock --checkfetch nothing (checked against a blocked github.com).setup-deps,connector-unit-tests,connector-integration-testsandsdr-e2etake an optionalgit-auth-tokeninput. A caller without one installs exactly as before.uv syncbefore the D series, so the vendored detect action never sees the token.uv lockinto its own step; the "drop rewrite before push" step is gone.docs/standards/ci.mdgets a section on handling a token for a private git dependency, and the bootstrap templates name the App secrets instead ofORG_PAT_GITHUB.Out of scope: the bump job's checkout keeps its repo-scoped token in
.git/config. The Docker build secret is covered by FND-4744 below.FND-4744: read-only App token for the image build's
git_tokensecret (squashed from #4792)The Docker builds passed
ORG_PAT_GITHUBas thegit_tokenBuildKit secret, which a Dockerfile reads to clone a private atlanhq git dependency duringuv sync. The mount keeps it out of the image, but not out of theRUN: every build backend thatuv syncruns could read the org-admin PAT.owner: atlanhq,permission-contents: read,continue-on-error, no PAT fallback) and passes it as the secret. This coverstests-reusable.yaml'sbuild-e2e-imageande2e(git-token),build-and-publish-app.yaml'sbuild, and both attempts inbuild-and-scan.yaml'sbuild.build-and-scan.yamlhas no flag, and several callers build private-dependency images without settingprivate_git_auth, which gates certify only. A Dockerfile without the secret mount ignores it. If a mint fails,docker/build-push-actionskips the empty secret with a warning, so only a build that needs the token fails, at its clone.ORG_PAT_GITHUB. Fleet packages are org-level, and the App only has repo-level Packages permission.Inputs and secrets are unchanged for callers.
Token never reaches uv or a build backend (lens rounds 5-6)
uv syncanduv lockrun PEP 517 build backends as the job's user. They inherit uv's environment and can read every ancestor's (/proc/<pid>/environ). So the token must be in neither. Now:git_auth.py mirroris the only step given the token. It mirror-clones each private atlanhq repo the app names into$RUNNER_TEMP/git-mirrorsand exits. A bare mirror clone runs no repository code, and git stores the remote URL as written, so the token isn't left in the mirror.git_auth.py run -- <cmd>runs in a later step with no token, pointing those remotes at the mirrors with a token-freeurl.file://<mirror>.insteadOf. It refuses to start ifGIT_TOKENis set. This covers setup-deps, the lock checks, the conformance pre-sync, the version-bump re-lock and certify's install. The composites' SDK repins reuse setup-deps' mirrors. Public atlanhq repos (e.g.application-sdkas a git source) are fetched from GitHub as before.git_auth.pyatjob.workflow_sha, runsgit_auth.py repos(everypyproject.tomlanduv.lockin the checkout, minus repos an unauthenticatedgit ls-remotecan read), removes the checkout from the workspace, and mints a read-only App token scoped to exactly that list. It skips the mint when the list is empty, becausecreate-github-app-tokenwith norepositoriesgrants the whole installation. Every private-deps and image-build mint uses this shape (_scoped_mint.pypins it). It's inline rather than a composite because callers pin composites@main, so a new one can't run before merge, and composite steps can'tcontinue-on-error. certify's SDK-checkout token is scoped toapplication-sdk.pyproject.toml/uv.lock, so the scope is also capped byALLOWED_PRIVATE_DEPSingit_auth.py, read from the workflow's own commit:atlan-query-intelligence-app(redactors, 9 connectors),sqlglot-for-lorien(Atlan's SQLGlot fork, QI app only), and two marked for near-term migration:server-sdk(toatlan-application-sdk-api) andai-application-sdk(the metering client, 3 apps on 3 different raw commits).reposandmirrorboth fail closed on any other private repo.atlanhq/common-app-serveris the one aggregator allowed to name any, matched onGITHUB_REPOSITORY. Checked against 111 fleet repos: none refused.uv lockwhenprivate_git_authis off, so a caller pinningsdk_scripts_refto a revision withoutgit_auth.pykeeps working.uv0.13 and git:uv lockanduv sync --frozeninstall a git dep (hatchling build) from the mirror over bothssh://andhttps://forms, and the lock keeps the original URL.Things to review
private_git_auth: truebut noFLEET_APP_*secrets used to fall back to the PAT. It now fails certify. Every known caller usessecrets: inherit.uv run --frozenagainst the locked dependencies. Before, a staleuv.lockcould be re-locked during the test step.uv syncno longer see the token, in uv's env or any ancestor's (lens rounds 5-6): only the mirror step holds it, and it exits before uv runs.FLEET_APP_*secrets now fails instead of falling back to the PAT. All 9 callers that set the flag pass the App secrets.RUN) can read a token.release_tagin "Compute image tags" is now passed throughenv:instead of being written into the script. This removes a script-injection path.validate-channelandunit-tests-gateget the same sparse.sdk-entrypointcheckout the file already uses.certify-unit's sparse checkout now includes/Dockerfile.sedtreated.as a wildcard.?).except Exception: passblocks in the sdr report code now print one line naming the exception.Blast radius (measured 2026-11-05)
Contract drift (FND-4539 + FND-4700). Certify's new drift check (SDK regeneration from a depth-1 clone, then a whole-tree
git status) was run against the default branch of all 106 caller apps with a pkl contract. The app's dependencies aren't needed, so none were installed.poe generatemerge.py), auth (hand-authoredatlan.yaml), notification (contract/generated/layout)contract/post-generate.shwith no output change, atlanhq/atlan-notification-app#28. generic-miner: FND-4741 (merged), which folds the credential root intocontract/app.pkl. auth: FND-4742, which generatesatlan.yamlfromapp.pkl.atlan.yaml, andapp/generated/holds files it no longer emitsbigquery and redshift are clean only because their
post-generate.shruns. It callspython, so it needspythononPATH; GitHub runners have it.Install credential (FND-4739 + FND-4740). No app changes left. 9 fleet repos set the private-git-deps flag: six already passed the App secrets, and the other three gained them in atlanhq/atlan-bigquery-app#712, atlanhq/atlan-snowflake-app#872 and atlanhq/atlan-tableau-app#356, all merged. A caller without the
FLEET_APP_*secrets now fails instead of falling back to the PAT.Image build secret (FND-4744). No app changes. 15 fleet repos have a Dockerfile that mounts
git_token, and every one of their callers ofbuild-and-publish-app.yaml/build-and-scan.yamlusessecrets: inherit. Each one's Dockerfile uses the token only foruv sync, so the scoped token covers it: the private repos come from the app'spyproject.tomlanduv.lockfiles, including sub-packages (memory-app'sserver/namesserver-sdk).Rollout
This PR is the switch: the gates go live for every caller when it merges.
app/generated/is empty; stale files there make it refuse and name them._input.pythat plainpkl evalleft behind.apt_packagesforpykerberos. It can't merge earlier, because that input doesn't exist onmainuntil this PR lands.Testing
pytest .github/scripts/tests:test_build_and_publish_certify.py,test_build_and_publish_certify_wiring.py,test_certify_unit_shards.py) pass on the combined tree: 78 passed.pytest .github/scripts/testscheck covers that.tag.gpgsign=false, because the author machine's global git config signs tags.app/generated/file, untracked file and rootatlan.yamlcases.test_private_git_deps_wiring.pyhas 51 tests; 38 fail on the old workflows.test_git_auth.pyruns the mirror path against real git and a real child process, which fetches through the rewrite and finds no token in its env. The lens round 5 tests (scoped mint, mirror, release-bump split) all fail on the previous commit.test_image_build_git_token.pyhas 23 tests; 10 fail on the old workflows.@main, so the reusables' end-to-end token path can only be exercised after merge.Not done here: the remaining 39 allowlisted conditional blocks, and turning shellcheck back on.
🤖 Generated with Claude Code
https://claude.ai/code/session_01AbKqGboFK2UACRjz2eg4FV