Repository navigation
feat(continuous-ops): fail loudly on what's required: one preflight, a lean config (XWING-2440) - #339
Merged
smith-xyz merged 11 commits intoNov 2, 2026
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smith-xyz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
smith-xyz
marked this pull request as ready for review
October 30, 2026 18:47
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
PR Summary by QodoUnify required preflight checks and fail closed in continuous operations
AI Description
Diagram
High-Level Assessment
Files changed (20)
|
Code Review by QodoView medium (4)
1. One forge outage skips other repositories
|
![qodo-for-traust-security[bot]](https://avatars.githubusercontent.com/u/330043060?s=60&v=4){kind=small}
Comment on lines
-50
to
-51
| ("gh", False), | ||
| ("glab", False), |
There was a problem hiding this comment.
🟠 Medium
3. Doctor stops checking gh, which the router still needs 🐞 Bug ≡ Correctness
The PR removes ("gh", False) from TOOLS and deletes check_auth, on the grounds that no forge
CLI is required any more. But build_rescan_worklist still runs gh api for the GitHub rate-limit
check, stage 1, stage 2, the tree count and the tripwire patch fetch, so a runner with no gh or an
unauthenticated one now passes doctor with no row at all and then marks every GitHub repo as an
error mid-run.
Agent Prompt
## Issue description
Doctor no longer reports `gh` presence or auth, but the rescan router's GitHub stages still call `gh api`.
## Fix Focus Areas
- src/traust/cli/doctor.py[57-70]
- src/traust/cli/doctor.py[305-310]
## Recommended Fix
Put `("gh", False)` back in `TOOLS` and restore a gh-only auth probe (`gh auth status`, or a check that `GITHUB_TOKEN` is set) under the non-`--quick` path. Leave `glab` out, since it is no longer used by the router.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Partially addressed; leaving this thread open. The manual worklist now refuses before stage 1 (exit 2 with install/auth guidance) if GitHub rows need gh but the CLI is missing; GitLab-only/pack runs do not require it. However traust admin doctor still does not check gh or its authentication, and unauthenticated GitHub worklist calls can still fail per row. I corrected the PR description rather than claim full doctor readiness. A conditional auth/readiness policy is still needed before this finding is fully resolved.
This was referenced Oct 30, 2026
openshift-ci
Bot
added
the
needs-rebase
Contributor
Author
|
Qodo round 2 — the three new findings, resolved (commit 9d44f66):
Gates: |
openshift-ci
Bot
removed
the
needs-rebase
Contributor
Author
|
checking if this is resolved |
…nfig (XWING-2440)
core#312 (e8ab152) carries Context.preflight, PreflightError and the requires: vocabulary (contracts#102 at 55b69a9, transitively). Two tests adapt to the pinned core: Config now records which keys traust.yaml set (the example compares by model_dump), and a changed repository's observation scans its patch for secrets (gitleaks required), so the observe fixture answers the compare patch and registers a clean scanner.
… delegates (XWING-2440) traust admin doctor now runs Context.preflight on the security pack's enabled assets — the same door the engine's start gate refuses on — and every finding is a FAIL row naming its fix (a required gap was a WARN before). The convention paths the plan keeps soft are shown as defaults in use (feeds cache chain, GIT_HOSTS, bicep), the stale gh/glab PATH and auth checks go (no forge CLI is required anymore), and --config-home wins for the env-resolved audit too. scripts/install_traust --doctor calls traust admin doctor instead of running its own shell checks; the separate check path (file parsing, retired files, load smoke, completeness, toolchain doctor) is gone.
…y (XWING-2440) The registry is required: models spend exits 1 naming the fix (copy config/model-registry.yaml into the config home) instead of skipping the declaration. The HARNESS_REQUIRE_SPEND_LEDGER env switch and the PENDING marker go.
…up front (XWING-2440) The GitLab stages read through core's ForgeApi (no glab CLI on the runner): the host must be a known forge (traust.yaml forge_hosts plus $GIT_HOSTS) and its token comes from the forge_tokens env var named for it, sent only as a header. gitleaks is required — the run refuses before the worklist builds (core's tool preflight, requires: tools semantics), never scanner_unavailable mid-run.
…2440) Qodo findings on the delegation and the forge REST move: - install_traust --doctor forwards --config-home (the wrapper's flag named a home the delegated doctor ignored, auditing TRAUST_CONFIG_HOME/default instead) and honours --toolchain PROFILE again: the profile's scanner doctor (traust admin toolchain doctor) runs beside the preflight, so a degraded profile cannot pass silently. - A GitLab host named only in traust.yaml's forge_hosts now classifies: split_repo_url takes the known-host mapping, and main threads _known_hosts(core) — the same merge the forge client gets — through stage 1, the quota estimate and the no-network branch, so a configured host routes instead of landing unsupported-host. - The $GIT_HOSTS comment tells the truth: it widens reach, not just recognition (the hosts feed the forge client's set).
…nt (XWING-2440)
The two Qodo findings the doctor PR owns:
- repo_state declares `requires: {tools: [gitleaks]}`: core's
observation scans each change's patch for secrets and fails without
the scanner, so the engine's start gate and doctor now refuse on it
before any work (a FAIL row naming the install pin, never a WARN).
Doctor's tests pin the scanner side with a stubbed-PATH fixture so
the config-row assertions stay machine-independent, and one test
proves the missing-gitleaks FAIL row on a complete home.
- The rescan worklist refuses up front (exit 2, fix named) when GitHub
repos are in the population and gh is missing: the router is a
hand-run lane, not a pack asset, so there is no requires row to hang
the CLI on — the door-level refusal is its home, the same convention
as the gitleaks preflight. Without GitHub repos the run proceeds
(GitLab hosts go through core's forge REST).
…XWING-2440) The three new Qodo findings on the doctor PR: - A GitHub Enterprise host classified github-kind queried github.com: every gh fetcher (stage 1/2, the tree count, the tripwire patch) now takes the row's host and names it (gh api --hostname), so a GHE repo reads its own API, never github.com's data for the same org/name; github.com's calls stay flagless (gh's default target). The quota reads are github.com's, so its gates (the preflight estimate, stage-2 deferral, the tripwire and preroute budgets) count only github.com rows — an enterprise host runs its own limits. - The delegated doctor's --toolchain check loads the chosen home too: the scanner doctor forwards --config-home like the preflight above it, so it never reports on TRAUST_CONFIG_HOME instead (or exit 2 on a home that doesn't resolve while the chosen one is fine). - A relative --config-home resolves against the caller's directory, not the repo root the doctor's subshell cds into: the wrapper absolutizes it once at its boundary (do_install absolutizes the same way), so the CI gate audits the home the caller named.
smith-xyz
force-pushed
the
feat/fail-loudly-on-what-s-required-one-preflight-a-l
branch
from
November 2, 2026 02:04
6d4a012 to
d5e9adf
Compare
Contributor
Author
|
Rebase/comment follow-up for Relevant Qodo dispositions, with direct replies on the inline threads:
Michele’s review of the changed tests, behavior and security gates is still requested; no merge/auto-merge was enabled. |
Code Review by Qodo
1.
|
|
Code review by qodo was updated up to the latest commit 36e6e0b |
|
Code review by qodo was updated up to the latest commit 4d6765b |
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
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.
Plan:
plans/traust-standardization/0-continuous-ops/in-progress/22-shaun-fail-loud-preflight.mdStarted by Shaun. Rebases onto merged
developmentbdb72e9; Michele’s review is requested for the changed tests, behavior and security gates. Do not merge before that review.What
The traust half of fail-loudly-on-what's-required: one preflight, the traust rows' decisions applied, adoption stays lean.
e01d4202(replacing the base branch’s orphaned282ebbeeand the old feature pin); it carriesContext.preflight, checkout and never-audited observation behavior.uv.lockis regenerated.traust admin doctorreports core's preflight. It runsContext.preflighton the security pack's enabled assets — the same door the engine's start gate refuses on — and every finding is a FAIL row that names its fix (a required gap was a WARN the report carried on past), exiting non-zero. The convention paths the plan keeps soft are shown as defaults in use (the feeds-cache chain,GIT_HOSTS, bicep, zero-configtraust.yamlvia each value's source row). The pack/engine does not need forge CLIs; the manual GitHub rescan worklist still needsghfor GitHub rows, while GitLab uses Forge REST. Its missing-ghguard fails up front, but doctor does not checkghauthentication.scripts/install_traust --doctordelegates totraust admin doctor; its own shell check path (file parsing, retired files, load smoke, completeness, toolchain doctor) is gone — one code path.traust admin toolchain doctorremains for per-profile scanner checks,traust config migratefor old multi-file homes.traust registry models spendrefuses (exit 1, naming the fix) when the config home has nomodel-registry.yaml. TheHARNESS_REQUIRE_SPEND_LEDGERenv switch and thePENDINGmarker go.requires: tools:semantics, neverscanner_unavailablemid-run. The GitLab stages read through core's forge REST (ForgeApivia the context facade): noglabCLI on the runner, the host a known forge (forge_hosts, plus$GIT_HOSTS), its token theforge_tokensenv var named for it, sent only as a header. Per-repo statuses keep their meaning (401/403 →no-credentials, transport give-up →unreachable).traust init: adoption stays copytraust.example.yaml, fill in, run doctor. A fresh home from the example fails preflight naming exactlylocations.workspace,locations.analysis_results,locations.progress_trackerandledger.public_key_path(no facts — traust's composition root registers the pack's defaults) and passes once they are set.Notes:
e01d4202.assets.yamland call the sameContext.preflight; no engine import (the architecture test forbids one).assets.yamlcarries its firstrequires:row —repo_state: {tools: [gitleaks]}(core's observation scans each change's patch for secrets and fails without the scanner), landed here so the doctor's FAIL row has something to fire on; doctor reads the rows generically (validated against contracts' pack schema,enabled: falseskipped, merged).model-registry.yamlnamesconfig/model-registry.example.yaml; traust shipsconfig/model-registry.yaml(no.exampleinfix). A core-side wording wart, noted here rather than forked.requires: feeds:entry is checkable once the pack declares one.Reviewer checks (from the plan)
except: return defaultorlog.warningfollowed by carrying on, for anything required.Quality checklist
Legacy files
Each file this plan replaces: reuse (core already does it), retire (the model made it obsolete) or port.
scripts/install_traustdo_doctor(the shell check path)--doctorrunstraust admin doctor(core's preflight). The duplicate shell parsing/load/completeness checks are gone; explicit--toolchain PROFILEalso runstraust admin toolchain doctoragainst that same config home.traust config migrateremains separatesrc/traust/cli/groups/registry.py_declare_spend's declaration-skip path (HARNESS_REQUIRE_SPEND_LEDGER,PENDINGmarker)models spendexits 1 naming the fix, and core's completeness audit names a missingmodel-registry.yamlat preflightsrc/traust/cli/build_rescan_worklist.py_glab_api(theglabsubprocess transport)ForgeApi): same per-repo status kinds, token fromforge_tokensas a header (never argv, never glab's auth store), hosts fromforge_hosts∪$GIT_HOSTSsrc/traust/cli/doctor.pycheck_tools'gh/glabrows andcheck_authghand fails up front when missing. Doctor does not verify GitHub CLI auth; see the partially addressed Qodo finding belowChanged tests
tests/test_doctor.pyhomefixture{"home": …}traust.yaml → a complete home (locations, ledger key, the shipped model registry)tests/test_doctor.py::test_an_unreachable_store_fails_and_skips_the_catalogcheck_corereturnsNonetests/test_doctor.py::test_main_reports_the_core_rows_and_exits_by_failures--quickno longer skips authtests/test_install_traust.py::test_doctor_smoke_on_a_fresh_install→test_doctor_delegates_to_traust_admin_doctor--doctordelegates: the test now asserts admin doctor's report and rc 1 on a fresh home naming the unset keystests/test_install_traust.py::test_doctor_names_retired_files_and_the_migrate_commandtraust config migrateremains the fold pathtests/test_install_traust.py::test_doctor_passes_on_the_operational_configtests/test_registry_models_spend.py::test_without_a_model_registry_the_declaration_is_skipped→…_is_refusedtests/test_build_rescan_worklist.py::TestGitlabCompare→TestGitlabRest_glab_apimonkeypatching → aForgeApifake; the glab-missing case → REST failure mapping (401/403 → no-credentials, transport give-up → unreachable, other HTTP → error)tests/test_build_rescan_worklist.py::TestInjectionGuards::test_gitlab_rejects_traversal_segmentgitlab_repo_info(host, path)→gitlab_repo_info(None, host, path)tests/test_build_rescan_worklist.py::TestMainEndToEnd::_patch_networkpreflight_failuresstubbedtests/test_build_rescan_worklist.py::TestTripwire::test_scanner_missing_records_and_skipstest_gitleaks_missing_refuses_before_the_worklist_builds+test_no_tripwire_skips_the_gitleaks_requirementtests/test_build_rescan_worklist.py::TestTripwire(hit / no-hits / quota-short)rw.shutil.whichpatchestests/test_build_rescan_worklist.py::TestBanBreaker::test_stage1_aborts_after_streakstage1(entries, jobs=1, …)→stage1(entries, 1, None, …)tests/test_security_router.py::test_tripwire_matches_the_legacy_routerrw.shutil.whichpatchtests/test_traust_example.py::test_the_example_as_copied_is_the_zero_config_installConfig(...) == Config()→model_dump()comparisonprovided, for preflight), so model equality no longer holds for a copied filetests/test_portfolio_observe.py(homefixture /forge_apihelper)stream); a clean gitleaks scanner is registered; the never-audited test now expects repository and default-branch readstests/test_impact_doors.py::test_sbom_without_a_document_reads_the_commits_manifests(re-pin to core#312's head)Workspacewithout a providerprovider="forge-archive"tests/test_manifest_sbom_lane.py::test_the_cli_records_the_observed_heads_sbom_once(re-pin)Workspaceliteral without a providerprovider="forge-archive"tests/test_doctor.py::test_main_reports_the_core_rows_and_exits_by_failures,::test_a_required_gap_is_a_fail_row_and_a_nonzero_exit,::test_a_fresh_home_from_traust_example_yaml_names_exactly_what_is_still_neededgitleaks_on_pathfixture parameter (a stubbed-PATH scanner that passes the pin); assertions unchangedrepo_statenow requires gitleaks, so the config-row assertions must not depend on the machine's scannerstests/test_build_rescan_worklist.py::TestMainEndToEnd._patch_network(and its subclass helpers)ghexecutable on a private PATHtests/test_build_rescan_worklist.py's gh fetcher fakes (fake_gh_info,fake_gh_cmp,failing_gh, the tripwiregh_patch/ preroutetree_countlambdas —tests/test_security_router.py's too)(project, sha)/(p, r)→ also the row's host (keyword-defaultedhost="github.com")gh apicall names its host now; the fakes mirror the fetcher signaturestests/test_build_rescan_worklist.py::TestPreroute/TestTripwire_rows_and_keysfixtureshostkey)_gh_host), the default the quota gates — the fixtures keep proving that fallbackNew tests (Michele’s review required): doctor's required-gap FAIL/exit, doctor/engine same-findings parity over one pack fixture, the fresh-home lean-adoption test, the defaults-in-use rows, the worklist's forge-client hosts/tokens and gitleaks refusal tests; the GHE-host routing/quota tests (
--hostnameon every gh call, github.com's quota never gating an enterprise row); the doctor's chosen-home tests (the toolchain check on the forwarded home, a relative home resolved against the caller's cwd).Verification after rebase
make lint-fix: passed;make test: 2,757 passed, 86 skipped (TMPDIR=/tmp); commit and push hooks passed.d5e9adfis running. Michele’s review remains requested.Qodo findings addressed
repo_statedeclaresrequires: {tools: [gitleaks]}— the engine's start gate and doctor refuse on it before any work (a FAIL row naming the install pin, never a WARN); a test proves the missing-scanner FAIL row on a complete home--config-homeflag)install_traust --doctorforwards--config-hometo the delegatedtraust admin doctor(the env/default paths already saw the same home the old checks did); a test proves the flag wins overTRAUST_CONFIG_HOME--doctorno longer refuses a missing config home--toolchainflag)install_traust --doctor --toolchain PROFILEruns the profile's scanner doctor (traust admin toolchain doctor) beside the preflight again, so a degraded profile cannot pass silently; by default scanner requirements come from the pack'srequires:rows (therepo_staterow above is the first)ghbut it is missing, while GitLab-only runs do not require it. Doctor still does not verifygh/authentication and unauthenticated GitHub calls can fail per row; do not claim the complete operator-readiness finding fixedsplit_repo_urltakes the known-host mapping and main threads_known_hosts(core)— the same merge the forge client gets (traust.yamlforge_hosts∪$GIT_HOSTS) — through stage 1, the quota estimate and the no-network branch, so a host named only in config routes instead of landingunsupported-host$GIT_HOSTScomment now says what the merge does: the hosts feed the forge client's set, so they widen reach, not just recognitiongh apicalls: every fetcher (stage 1/2, the tree count, the tripwire patch) now takes the row's host and names it (gh api --hostname), so a GHE repo reads its own API, never github.com's data for the same org/name; the quota reads are github.com's, so its gates (the preflight estimate, stage-2 deferral, the tripwire and preroute budgets) now count only github.com rows — an enterprise host runs its own limits--toolchain PROFILEdoctor forwards--config-homelike the preflight call above it, so the scanner check loads the home the wrapper was given, notTRAUST_CONFIG_HOME/the default (which could exit 2 on a home that doesn't resolve while the chosen one is fine)do_doctorresolves the home once at the wrapper's boundary —~expanded, a relative path anchored to the caller's directory — before the subshellscdinto the repo root, so--doctor --config-home ./myhomerun from outside the repo audits the caller's home (do_installabsolutizes the same way)Cross-repo coordination
Track:
development. Use these explicit related-repository targets rather than assuming the connected repository'smainis the matching overhaul baseline.Review targets
Integration and rollout
Adopt the Core preflight door and reconcile its pin with the other application adoption PRs. The Engine companion shares the pack/preflight contract; it is not a new application-to-Engine Python import.
Branch URLs are compatibility baselines; PR URLs are proposed companion code. Neither proves that a manifest/lockfile pin already contains the changes or that the target is deployed. Honor the prerequisites above, reconcile compatible dependency pins before rollout, and do not dismiss a real compatibility gap merely because another PR is planned.