Skip to content

feat(continuous-ops): fail loudly on what's required: one preflight, a lean config (XWING-2440) - #312

Merged
smith-xyz merged 28 commits into
developmentfrom
feat/fail-loudly-on-what-s-required-one-preflight-a-l
Oct 31, 2026
Merged

smith-xyz merged 28 commits into
developmentfrom
feat/fail-loudly-on-what-s-required-one-preflight-a-l

Conversation

@smith-xyz

@smith-xyz smith-xyz commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Plan: plans/traust-standardization/0-continuous-ops/in-progress/22-shaun-fail-loud-preflight.md

Started by shaun. Draft until ready for review. This PR is the plan's step 3, the traust-core part only: one preflight plus the core rows of the step-1 inventory. The engine gate (step 4) and traust's doctor (step 5) are later PRs pinned to this one.

Contracts pin: 55b69a9a (contracts#102, the requirement vocabulary). Note: core#311 pins contracts#103 (1436d659…) — two open core PRs currently carry different contracts pins; whoever merges second rebases and re-pins; the final pin lands after both contracts PRs merge.

What

One preflight door, built on what core already has (Toolchain.preflight / preflight_failures, config_completeness_audit) — no second readiness path:

  • Requirement (models.operations, the pack schema's requires: vocabulary from contracts#102: tools, config keys, recorded facts, feed sources, secrets), validated against the closed vocabularies (traust.yaml's schema keys + model-registry; the recorded-fact artifact names in models.artifacts.RECORDED_FACTS).
  • Context.preflight(requirements) -> None (context.preflight): merges the enabled assets' requirements, checks each kind and raises PreflightError(ConfigError) listing every unmet requirement with its fix in one message:
    • tools — on PATH, version ≥ the external-tools.yaml pin, database present (preflight_failures, now carrying the pin so the fix names it);
    • config — the traust.yaml top-level keys the file actually set (Config.provided, a private attr so the contracts schema keeps owning the public field set), model-registry against the registered model catalog;
    • facts — each recorded fact's artifact exists (artifacts.latest_imported, read-only, no new query), the fix naming the record door;
    • feeds — the source is enabled (feed source or cached feed) and reachable (its readiness/probe);
    • secrets — each env var resolves;
    • the config home audit — config_completeness_audit's errors and warnings become findings (the confirmed decision for that row).
  • The core "required" rows of the step-1 inventory (decisions confirmed in contracts#102):
    • observation (_secret_hits): a secret scan that cannot run fails the observation instead of warning and recording None; a patch fetch inside the quota reserve still defers it (the explicit QUOTA_DEFERRED outcome, no document kept).
    • Context.deps_dev stops being | None: it raises naming the feeds source to enable.
    • ReportingService.lint_threat_models requires the ATT&CK and ATLAS catalogs (no more shape-only fallback); it raises naming the register doors.
    • the spend and ATT&CK-coverage views are always in the view set and raise naming their file when their catalog is missing (the SlaView pattern; NotFoundError so a batch refresh records the failure per view), never silently absent.
    • validation precedents refuse without an evidence reader (Context always wires one) instead of warning and excluding them.
  • Keep-soft rows stay soft, never a silent fallback: symbol-index build failure, Universal Ctags, ledger/identity absent, the grype DB dir default, the bicep resolution chain, feed sources without a probe (reported unchecked).

Deviations to look at:

  • ReportingService catalogs: implemented as "the lint door raises without them" rather than dropping | None from the constructor — the constructor-required shape would force every core test fixture that calls services() (~50 files) to register pack tables, for behaviour traust already guarantees (it always registers both). Same user-visible outcome: no shape-only degradation.
  • Audit warnings vs the ledger row: preflight promotes all completeness warnings to findings, per the confirmed decision for the audit row — including ledger.public_key_path unset, which sits next to the keep-soft "ledger/identity absent" row (whose doors still raise on use). If the ledger warning should stay soft, that is a one-line filter.
  • Docs map (compliance_sources): the door already raises per product (RedHatDocSource.fetch); recording a product → docs slug/version map in core needs a contracts schema, so the NEED(config: product -> docs slug/version) marker stays.
  • enabled: false exempts an asset at the pack layer (contracts#102); preflight only ever sees the enabled assets' requirements — the engine (step 4) does the filtering.

No repository queries were added or changed (the facts check reuses artifacts.latest_imported), so no test-repositories / perf run; make test shows the same single Postgres-marked skip as before the branch.

Qodo findings addressed

  • Fixed — typo'd requirement kinds were silently ignored: Requirement now refuses an unknown requires: kind with a ValidationError naming the kind and the valid ones, instead of Dto's extra-ignore dropping it. The contracts pack schema already rejects unknown kinds (additionalProperties: false, the requirement vocabulary), so this closes the core-side parsing door.
  • Fixed — valid config homes failed the readiness gate: Config records the home it was loaded from (Config.config_home, a private attr like provided), and preflight's completeness audit examines that home, not whatever $TRAUST_CONFIG_HOME happens to point at.
  • Fixed — disabled feeds passed the readiness gate: load_feeds now opts out access: disabled cached sources the way the source loader always has, so a required feed whose source is disabled fails preflight naming the feeds registry to enable.
  • Fixed — missing tool setup hid other gaps: an unregistered toolchain is one more preflight finding (tools: no toolchain config: Context.register_toolchain()), not a ConfigError abort before the other checks; every gap still lands in one message.
  • Fixed — secret-scan results lost provenance (test helper): the support's default scanner now carries the scanner version and the scanned file in its Acquisition, the provenance the adapter door records.
  • Left with a reason — big GitHub changes keep their refusal, not their churn data: the except SourceError around _secret_hits is this plan's explicit decision, not a silent drop: a patch-fetch refusal returns the observation carrying the refusal's status (QUOTA_DEFERRED, ERROR, …) and error string, surfaced as the order's outcome; the code comment and _secret_hits' docstring state the contract (a source refusal defers the observation, a missing or failing scanner fails it — partial data must not look complete). Withholding the churn numbers from a refused observation is that fail-loud outcome; the old warn-and-record-None path was the silent one.

Reviewer checks (from the plan)

  • No new except: return default or log.warning followed by carrying on, for anything required.
  • Optional means declared optional in config, never inferred from absence.
  • One preflight code path: doctor, engine and doors all call it.
  • Every error names its fix.

Quality checklist

  • 1. Comments only for the non-trivial
  • 2. Docstrings don't bloat
  • 3. Don't reinvent
  • 4. Shared logic is a helper, in one place
  • 5. Tests cover the edges
  • 6. Integration tests are thorough and use real data, sanitized
  • 7. Minimal, clean code
  • 8. Queries are low latency, and you checked
  • 9. Tests change only on purpose, and a person said so

Legacy files

Each file this plan replaces: reuse (core already does it), retire (the model made it obsolete) or port.

File Verdict Where it went / why it's gone
none — no legacy file touched; this PR adds the preflight and applies the inventory's core rows

Changed tests

Every existing test edited, with why (each is an inventory row's confirmed decision; new tests are not listed):

Test Old → new Why
tests/v1/context/test_feeds.py::test_no_deps_dev_without_an_enabled_source renamed test_deps_dev_without_an_enabled_source_raises; asserted ctx.deps_dev is None → now expects ConfigError naming the feeds source the deps_dev door stops being `
tests/v1/context/test_tool_reports.py::test_osv_api_runs_through_the_registry_lookup_and_deps_dev asserted the OSV API report runs with resolver=None → now registers a deps_dev source and asserts the resolver is passed through same row: the osv API path requires deps.dev
tests/v1/providers/tools/test_toolchain.py::test_check_uses_the_pin ToolCheck equality now includes expected="1.6.0" the check carries its pin so preflight's fix names it
tests/v1/services/findings/test_precedents.py (World.precedents helper) FindingPrecedents(uow) → wired with a ValidationRecords reader validation precedents refuse without an evidence reader
tests/v1/services/reporting/conftest.py (reporting fixture) no catalogs → registers the ATT&CK and ATLAS fixture tables threat-model lint requires the pinned catalogs
tests/v1/services/reporting/test_threat_model.py::test_attack_refs_are_checked_against_the_catalog_when_one_is_registered renamed …_against_the_pinned_catalog; registered only ATT&CK → registers both the lint door requires both catalogs, no shape-only fallback
tests/v1/services/reporting/test_threat_model.py::test_atlas_refs_are_checked_against_the_catalog_when_one_is_registered renamed …_against_the_pinned_catalog; registered only ATLAS → registers both same
tests/v1/services/test_insight_spend.py::test_the_spend_view_is_built_only_on_a_model_registry renamed test_the_spend_view_without_a_model_registry_names_the_file; ValidationError "'spend' is not built" → NotFoundError "no model registry: …" the spend view is always registered and raises naming its file (SlaView pattern)
tests/v1/services/test_insight_attack_coverage.py::test_the_view_renders_through_context expected ValidationError "not built" before registering → expects NotFoundError "no ATT&CK catalog: …" same, for the ATT&CK coverage view
tests/v1/services/test_portfolio_observation.py::test_batch_quota_decision_does_not_query_live_quota expected source calls [info, head, compare] → [info, head, compare, compare_patch] the observation now fetches the patch to scan it (gitleaks required)
tests/v1/services/test_portfolio_observation.py (compared() helper) a gitlab compare carried no patch → carries one the gitlab scan reads compared.patch
tests/v1/services/test_portfolio_observation_secrets.py::test_an_unscanned_patch_is_none_and_says_why (7 cases) split into test_a_scan_that_cannot_run_fails_the_observation (6 cases: the observation now raises, nothing recorded) and test_a_patch_fetch_inside_the_quota_reserve_defers_the_observation (quota case → QUOTA_DEFERRED, no document) a scan that cannot run fails the observation; the quota reserve defers it (inventory: gitleaks in observation)
tests/v1/services/test_portfolio_observation_secrets.py::test_scanner_failure_does_not_log_exception_text_or_repository_url asserted a "not scanned" warning without the secret → now asserts the ServiceError propagates and nothing is logged same row: no warn-and-carry-on
tests/v1/services/test_portfolio_observation_store.py (Observed helper) service built with no tools → built with default_tools() (a clean gitleaks) same row: the observation requires gitleaks
tests/v1/observation_support.py (support, not a test) observer() defaulted to no tools and an empty patch at quota 100 → defaults to a registered clean gitleaks, a non-empty patch and a quota above the tripwire reserve models the real deployment (traust registers the toolchain); tests that want no gitleaks pass tools=None explicitly
tests/v1/context/test_traust_yaml.py::test_an_empty_traust_yaml_is_the_defaults Config.from_home(empty traust.yaml) == Config() → the two model_dump()s are compared Config now records the home it was loaded from (a private attr, like provided) so preflight audits the loaded home; an empty file's values are still the defaults

New tests: tests/v1/context/test_preflight.py (17: one per requirement kind, every gap at once, the audit as findings, dedupe across assets, unknown/duplicate/empty and typo'd-kind requirements rejected, a complete home passes, the audit examines the loaded home, a missing toolchain is one more gap, a disabled feed source fails), test_lint_without_the_catalogs_names_the_register_doors, test_validation_precedents_without_an_evidence_reader_refuse, test_osv_api_without_deps_dev_is_refused, test_the_default_scanner_carries_the_adapter_doors_provenance.

  • A person reviewed each changed test and the behaviour change is intended

Cross-repo coordination

Track: development. Use these explicit related-repository targets rather than assuming the connected repository's main is the matching overhaul baseline.

Review targets

Integration and rollout

The requirement vocabulary is already merged into Contracts development. The linked Engine and application PRs are the corresponding preflight callers; reconcile their Core pins before rollout.

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.

…bility

ref is required (an order always names its commit); history is none, a
depth or an anchor sha, and writable asks for a worktree. SourceProvider
loses checkout/release: they move to the checkout service's providers.
A worktree per order under one cache root, removed on release; the host's
forge token rides git's configuration environment, never argv. The git
hardening flags move next to GitCheckout, shared.
…XWING-2440)

Kept files, no history, read-only go to ArchiveCheckout; everything else
to MirrorCheckout. refresh(repo, head) fetches a moved head, evict
(idle_days) drops idle mirrors; wired once in Context.services().
ExposureService calls the service. The cache root is locations
.mirror_cache, defaulting under the data home.
…fs and objects

Review fixes for the mirror cache:

- evict skips a mirror whose worktrees/ still holds an entry, so a
  checkout held longer than idle_days keeps its mirror
- a mirror counts as healthy only when refs/ and objects/ exist beside
  HEAD; a crash mid-rmtree re-initializes instead of failing every fetch
- one flock per mirror serializes concurrent fetches (git's ref locks)
- the fetch refspec narrows to refs/heads/* and refs/tags/*, dropping
  refs/pull/* and refs/merge-requests/*
- GIT_ALLOW_PROTOCOL=https joins the fetch env (defense in depth
  against an insteadOf rewrite to ext::)
- RepoCheckoutService refuses keep=() (an empty keep set would yield an
  empty workspace) the same way it refuses a zero history depth
- CheckoutRequest and Workspace are re-exported by the context facade
  for consumers fenced from providers
@smith-xyz
smith-xyz marked this pull request as ready for review October 30, 2026 18:47
@smith-xyz
smith-xyz requested a review from chubirka as a code owner October 30, 2026 18:47
@qodo-for-traust-security

Copy link
Copy Markdown

PR Summary by Qodo

Add a shared preflight and fail on missing required inputs

✨ Enhancement 🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes


AI Description

• Add one preflight that reports all unmet asset requirements and deployment gaps with fixes.
• Refuse observations, views, linting, and precedents when required inputs are unavailable.
• Pin the requirement contracts and test the new readiness and failure behavior.
Diagram

graph TD
  R["Asset requirements"] --> C["Context preflight"] --> P["Merged checks"] --> T["Toolchain checks"] --> E["Preflight error"]
  P --> F["Config and facts"] --> E
  P --> S["Feeds and secrets"] --> E
  P --> H["Config home audit"] --> E
Loading
High-Level Assessment

Use the PR’s shared Context preflight: it reuses the existing toolchain and config-home checks rather than introducing a second readiness system. Keeping fail-loud behavior at the affected service doors also protects direct callers before the later engine and doctor integrations.

Files changed (28) +740 / -170

Enhancement (7) +234 / -34
config.pyTrack keys explicitly set in traust.yaml +14/-1

Track keys explicitly set in traust.yaml

• Records loaded top-level keys in a private attribute, allowing preflight to distinguish file-provided settings from defaults without changing the public config schema.

src/traust_core/v1/context/config.py

context.pyExpose preflight and refuse missing required sources +29/-28

Expose preflight and refuse missing required sources

• Adds Context.preflight, makes deps_dev raise when its feed source is absent, and always registers spend and ATT&CK coverage views so they can report missing catalogs explicitly.

src/traust_core/v1/context/context.py

preflight.pyAggregate deployment readiness findings +120/-0

Aggregate deployment readiness findings

• Merges asset requirements and checks tools, explicit config, recorded facts, feeds, secrets, and config-home audit results. Raises one PreflightError containing every collected gap and its fix.

src/traust_core/v1/context/preflight.py

errors.pyIntroduce PreflightError +5/-0

Introduce PreflightError

• Adds a ConfigError subtype for unmet deployment requirements.

src/traust_core/v1/errors.py

artifacts.pyDefine requireable recorded facts +11/-0

Define requireable recorded facts

• Lists the artifact types whose recorded presence an asset may require.

src/traust_core/v1/models/artifacts.py

operations.pyValidate asset requirement declarations +43/-0

Validate asset requirement declarations

• Adds Requirement for tools, config, facts, feeds, and secrets. Rejects empty or duplicate entries and validates config and fact names against their closed vocabularies.

src/traust_core/v1/models/operations.py

toolchain.pyInclude version pins in tool readiness findings +12/-5

Include version pins in tool readiness findings

• Carries the expected version through tool checks so missing-tool and outdated-version messages name the required pin.

src/traust_core/v1/providers/tools/toolchain.py

Bug fix (5) +82 / -68
precedents.pyRefuse validation precedents without an evidence reader +16/-16

Refuse validation precedents without an evidence reader

• Raises ConfigError instead of warning and silently excluding validation precedents when their evidence reader is not configured.

src/traust_core/v1/services/findings/precedents.py

insight.pyMake ATT&CK coverage name its missing catalog +19/-8

Make ATT&CK coverage name its missing catalog

• Accepts an absent catalog during view construction but raises NotFoundError with the registration fix when coverage is built.

src/traust_core/v1/services/insight.py

insight_spend.pyMake spend views require a model registry +14/-2

Make spend views require a model registry

• Raises NotFoundError naming model-catalog registration when a spend view is built without its registry.

src/traust_core/v1/services/insight_spend.py

observation.pyFail observations when secret scanning cannot run +22/-31

Fail observations when secret scanning cannot run

• Propagates missing-scanner, readiness, patch, and scan failures rather than recording unscanned secrets as None. Source failures during patch retrieval retain the explicit deferred or error observation outcome.

src/traust_core/v1/services/observation.py

service.pyRequire both catalogs for threat-model lint +11/-11

Require both catalogs for threat-model lint

• Removes the shape-only identifier fallback and raises a configuration error when either ATT&CK or ATLAS is unregistered.

src/traust_core/v1/services/reporting/service.py

Tests (13) +417 / -61
test_feeds.pyTest missing deps.dev feed refusal +5/-3

Test missing deps.dev feed refusal

• Changes the disabled-source expectation from None to ConfigError.

tests/v1/context/test_feeds.py

test_preflight.pyCover aggregated preflight findings +244/-0

Cover aggregated preflight findings

• Tests satisfied requirements, deduplication, each requirement category, config-home audit findings, and invalid declarations.

tests/v1/context/test_preflight.py

test_tool_reports.pyCover resolver use and missing-source refusal +16/-1

Cover resolver use and missing-source refusal

• Registers deps.dev for the successful OSV API report and verifies that reporting without it raises.

tests/v1/context/test_tool_reports.py

observation_support.pyProvide a ready secret scanner in observation fixtures +40/-7

Provide a ready secret scanner in observation fixtures

• Adds a clean gitleaks test provider and nonempty patches so ordinary observations satisfy the new required scan. Keeps explicit scanner omission available to failure tests.

tests/v1/observation_support.py

test_toolchain.pyAssert expected tool version is retained +1/-0

Assert expected tool version is retained

• Updates the pinned-tool check expectation to include its required version.

tests/v1/providers/tools/test_toolchain.py

test_precedents.pyTest evidence-reader requirement +23/-3

Test evidence-reader requirement

• Wires an evidence reader into normal precedent tests and adds a refusal case when validation precedents lack one.

tests/v1/services/findings/test_precedents.py

conftest.pyRegister catalogs for reporting tests +10/-1

Register catalogs for reporting tests

• Supplies pinned ATT&CK and ATLAS fixture catalogs to the shared reporting fixture.

tests/v1/services/reporting/conftest.py

test_threat_model.pyTest catalog-required threat-model lint +15/-7

Test catalog-required threat-model lint

• Adds a missing-catalog refusal case and registers both catalogs for identifier-validation cases.

tests/v1/services/reporting/test_threat_model.py

test_insight_attack_coverage.pyExpect an explicit missing ATT&CK catalog error +2/-2

Expect an explicit missing ATT&CK catalog error

• Verifies that building coverage without the catalog raises NotFoundError instead of reporting an unbuilt view.

tests/v1/services/test_insight_attack_coverage.py

test_insight_spend.pyExpect an explicit missing model-registry error +5/-3

Expect an explicit missing model-registry error

• Verifies that a spend build without its registry names the missing dependency.

tests/v1/services/test_insight_spend.py

test_portfolio_observation.pyAdjust observation fixtures for patch scanning +9/-2

Adjust observation fixtures for patch scanning

• Supplies a comparison patch and expects the GitHub patch fetch during a normal observation.

tests/v1/services/test_portfolio_observation.py

test_portfolio_observation_secrets.pyTest failed scans and quota deferral +37/-31

Test failed scans and quota deferral

• Replaces warning-and-partial-record expectations with exception and no-record assertions. Separately verifies that a patch fetch inside the quota reserve yields QUOTA_DEFERRED without keeping a document.

tests/v1/services/test_portfolio_observation_secrets.py

test_portfolio_observation_store.pySupply the required scanner in storage tests +10/-1

Supply the required scanner in storage tests

• Registers the clean gitleaks fixture for stored-observation tests.

tests/v1/services/test_portfolio_observation_store.py

Documentation (1) +2 / -2
README.mdDocument preflight and mandatory threat-model catalogs +2/-2

Document preflight and mandatory threat-model catalogs

• Describes the shared readiness door and its requirement categories. Clarifies that threat-model lint refuses to run without registered ATT&CK and ATLAS catalogs.

README.md

Other (2) +5 / -5
pyproject.tomlPin contracts to the requirement vocabulary +1/-1

Pin contracts to the requirement vocabulary

• Updates the traust-contracts Git pin to the revision supplying the pack requirement vocabulary.

pyproject.toml

uv.lockLock the updated contracts revision +4/-4

Lock the updated contracts revision

• Updates the traust-contracts source and dependency entries to the new pin; lockfile revision and resolution-marker metadata also change.

uv.lock

@qodo-for-traust-security

qodo-for-traust-security Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Code Review by Qodo


Resolved findings
1. Secret-scan results lose provenance ✓ Resolved
Description
CleanGitleaks.acquire creates Acquisition with only the provider and acquisition time, leaving
the scanner version and scan source unset. Every observation using the default scanner therefore
produces results without complete provenance even though the artifact contains the scanner version.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The added `CleanGitleaks.acquire` provider result leaves `Acquisition.version` and `Acquisition.source` unset, so observation results lack complete provider provenance.

## Fix Focus Areas
- tests/v1/observation_support.py[57-68]
- src/traust_core/v1/providers/base.py[38-44]

## Recommended Fix
Populate `Acquisition` with the scanner version and the patch source used by the fixture, in addition to provider and acquisition time. Keep the provenance consistent with production tool providers and update assertions if needed.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. New scan tests leave signatures untyped ✗ Dismissed
Description
test_a_scan_that_cannot_run_fails_the_observation leaves its parameters and return type
unannotated. The new test boundary therefore lacks the required type information for later callers
and edits.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

Issue description
The newly added scan-failure test has unannotated parameters and return type, contrary to the requirement that every function parameter and return type be explicit.

Fix Focus Areas
- tests/v1/services/test_portfolio_observation_secrets.py[234-238]
- tests/v1/services/test_portfolio_observation_secrets.py[243-251]

Recommended Fix
Add explicit types to every parameter and a `-> None` return annotation on both newly added test functions, using the existing fixture and domain types where appropriate.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Missing tool setup hides other gaps ✓ Resolved
Description
_tool_findings() accesses ctx.toolchain directly when any asset requires a tool, although that
property raises ConfigError if the toolchain was not registered. In that state, preflight stops
before checking config, facts, feeds, secrets, or the home audit, so the caller does not receive the
promised list of every unmet requirement.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A missing toolchain registration aborts preflight before its remaining checks run.

## Fix Focus Areas
- src/traust_core/v1/context/preflight.py[41-59]
- src/traust_core/v1/context/context.py[541-552]

## Recommended Fix
Turn the missing-registration ConfigError into a tool-setup finding, then continue collecting the other requirement and audit findings. Add a test combining an unregistered toolchain with another unmet requirement.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Typo'd requirement kinds are silently ignored ✓ Resolved
Description
Requirement inherits Dto, whose config is extra="ignore", so a misspelled kind in a
requires: block (e.g. tool: or secret:) is dropped without an error. Preflight then skips that
requirement and passes, even though the PR promises a vocabulary that fails loudly.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Requirement` ignores unknown keys, so a misspelled requirement kind disappears and preflight passes.

## Fix Focus Areas
- src/traust_core/v1/models/operations.py[97-105]

## Recommended Fix
Add `model_config = pydantic.ConfigDict(frozen=True, extra="forbid")` to `Requirement`, or base it on `Model`. Add a test showing that an unknown kind raises.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Valid config homes fail the readiness gate ✓ Resolved
Description
preflight_findings() calls config_completeness_audit() without the home used to load the
context’s configuration. When a caller constructs a context from an explicit config file but does
not set TRAUST_CONFIG_HOME, the audit examines the environment or default home instead and can
reject that deployment or report gaps from a different one.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Preflight audits the process-global config home rather than the home from which its Context loaded traust.yaml.

## Fix Focus Areas
- src/traust_core/v1/context/preflight.py[41-54]
- src/traust_core/v1/context/config.py[279-313]

## Recommended Fix
Retain the source config directory when loading Config and pass that directory explicitly to config_completeness_audit. Cover an explicit config path with an unset or different TRAUST_CONFIG_HOME.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. Big GitHub changes lose their churn data ✗ Dismissed
Description
The new except SourceError around _secret_hits in observe_forge_facts throws away the whole
comparison when the GitHub patch fetch fails. It returns facts with the refusal's status (e.g.
ERROR from CommandSourceProvider._refused) and no ahead_by, changed_lines or truncation
data, where the old code kept those numbers and only set secret_hits=None. A diff too large for
GitHub's diff endpoint gives a 4xx, so _keep skips that status and the router keeps the old
observation. That hits the repos with the most change since their audit, and it happens on every
run.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
When the GitHub patch fetch fails (for example a 406 because the diff is too large), `observe_forge_facts` returns facts with no comparison metrics and status ERROR. The churn measurements are never kept, and large-change repositories stop getting fresh observations.

## Fix Focus Areas
- src/traust_core/v1/services/observation.py[240-259]

## Recommended Fix
Only the quota-reserve SourceError (`ComparisonStatus.QUOTA_DEFERRED`) should defer. For any other patch-fetch refusal, still merge the comparison metrics (`ahead_by`, `compare_truncated`, `metrics.model_dump()`, `tree_first_party_files`) into the returned facts, and set the status and error so the scan gap is clear. Do not drop the measurements. Add a test for a 406 / too-large diff from compare_patch.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


7. Product-map preflight cannot pass ⊘ Outdated
Description
RECORDED_FACTS makes product-definitions-map a required recorded fact, but the mounted
traust-contracts snapshot defines it as a config schema and has no corresponding artifact schema or
storage profile. When an asset requires that fact, _fact_findings looks for an imported artifact
that the mounted contract's artifact storage cannot accept, so the advertised recording door cannot
satisfy the check.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The mounted contracts snapshot defines `product-definitions-map` as configuration, not a storable artifact, while preflight accepts it as a recorded fact. Confirm whether the newly pinned contracts revision adds artifact support; if not, coordinate that support or remove this fact requirement.

## Fix Focus Areas
- src/traust_core/v1/models/artifacts.py[376-384]
- src/traust_core/v1/context/preflight.py[93-102]
- /cross_repos/traust-contracts/config/v1/product-definitions-map.schema.json[1-15]
- /cross_repos/traust-contracts/src/traust_contracts/v1/storage/store.py[370-389]

## Recommended Fix
Add and pin a matching artifact schema and storage profile in traust-contracts before accepting this name in `RECORDED_FACTS`, or exclude it until the recording contract supports it.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


8. Disabled feeds pass the readiness gate ✓ Resolved
Description
_feed_findings() accepts a required name from ctx.feeds when it is absent from
ctx.feed_sources, then relies on the cached provider’s check() to declare it ready. For a cached
registry entry with access: disabled, the source loader excludes it but the cached-feed loader
still registers it, and the cached provider’s unconditional ready result leaves no preflight
finding.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Preflight treats a disabled cached feed as enabled because it remains registered and its readiness check always succeeds.

## Fix Focus Areas
- src/traust_core/v1/context/preflight.py[105-120]
- src/traust_core/v1/context/feeds.py[86-95]
- src/traust_core/v1/context/feeds.py[146-153]

## Recommended Fix
Make cached-feed registration honor access: disabled, so a requirement for that feed produces the existing not-enabled finding. Test preflight with a disabled cached feed entry.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


9. Catalog comment says ATLAS is shape-checked ✓ Resolved
Description
The new comment above Context.atlas_catalog says ATLAS IDs are shape-checked only when no catalog
is registered. This PR makes ReportingService.lint_threat_models raise ConfigError in that case
instead, so anyone reading the comment will expect a fallback that no longer exists.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The comment on `atlas_catalog` describes the removed shape-only fallback.

## Fix Focus Areas
- src/traust_core/v1/context/context.py[329-331]

## Recommended Fix
Rewrite the comment to say that both the ATT&CK coverage view and threat-model lint raise without their catalogs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Comment thread tests/v1/observation_support.py Resolved
Comment thread src/traust_core/v1/context/preflight.py Resolved
Comment thread src/traust_core/v1/context/preflight.py Resolved
Comment thread src/traust_core/v1/context/preflight.py Resolved
Comment thread src/traust_core/v1/models/operations.py Resolved
Comment thread src/traust_core/v1/context/context.py Resolved
Comment thread src/traust_core/v1/models/artifacts.py Resolved
CheckoutRequest.repo carries a LocalRepoUrl beside HttpsRepoUrl (a file://
URL or an absolute path to a git repository), and RepoCheckoutService
routes a local origin to LocalCheckout: a git worktree straight off the
repository the caller materialized — no mirror, no fetch, no cache —
released by removing and pruning the worktree so the materialized
repository re-runs clean. An unhealthy path (no HEAD/refs/objects, bare
root or .git/) is refused with a typed error; the https path is unchanged.
@smith-xyz
smith-xyz force-pushed the feat/fail-loudly-on-what-s-required-one-preflight-a-l branch from e8ab152 to 09ad226 Compare October 30, 2026 19:36
Qodo finding on #311/#313/#318: HttpsRepoUrl validated the path segments
but not the host, so https://../owner/repo built a mirror path outside
the cache root (the host becomes a directory component under it).
CheckoutRequest.repo carries a LocalRepoUrl beside HttpsRepoUrl (a file://
URL or an absolute path to a git repository), and RepoCheckoutService
routes a local origin to LocalCheckout: a git worktree straight off the
repository the caller materialized — no mirror, no fetch, no cache —
released by removing and pruning the worktree so the materialized
repository re-runs clean. An unhealthy path (no HEAD/refs/objects, bare
root or .git/) is refused with a typed error; the https path is unchanged.
…o feat/fail-loudly-on-what-s-required-one-preflight-a-l
… evict

Qodo findings on #311/#313/#318: HttpsRepoUrl validated path segments
but not the host, so https://../owner/repo escaped the mirror cache
root; GitLab's git smart-HTTP endpoints take HTTP Basic, not the REST
PRIVATE-TOKEN header, so private GitLab mirrors fetched
unauthenticated (ForgeApi.git_headers shapes auth per transport); the
last-checkout touch ran after the worktree's cleanup handler, so a
failed touch stranded the worktree; a negative idle_days cleared the
whole cache.
CheckoutRequest.repo carries a LocalRepoUrl beside HttpsRepoUrl (a file://
URL or an absolute path to a git repository), and RepoCheckoutService
routes a local origin to LocalCheckout: a git worktree straight off the
repository the caller materialized — no mirror, no fetch, no cache —
released by removing and pruning the worktree so the materialized
repository re-runs clean. An unhealthy path (no HEAD/refs/objects, bare
root or .git/) is refused with a typed error; the https path is unchanged.
…o feat/fail-loudly-on-what-s-required-one-preflight-a-l
@smith-xyz
smith-xyz merged commit 2beaf99 into development Oct 31, 2026
6 checks passed
@chubirka

Copy link
Copy Markdown
Member

Historical code-review findings

This PR has already merged (2026-10-31T00:33:51Z). The findings below apply to reviewed commit be352271ef2cffd066ec1b267f54ddadb512b5fe; they are not a claim that later revisions or the current branch still contain each defect. Merge prevention is no longer possible for this PR. Please reconcile these findings with the merged code and address any remaining defects.

[P2] Preserve the deployment process policy in checkout runners

Location: src/traust_core/v1/context/context.py:771–775

The new checkout service creates an unconditional git allowlist and a 600-second runner instead of using the deployment's process policy; the local provider at line 781 does the same. A deployment that deliberately excludes git from process.allowed_binaries can therefore still execute it through services().checkouts, and a smaller process.timeout_seconds is discarded. This bypasses the existing execution boundary, rather than merely reporting incorrect readiness. Preserve the configured binary allowlist and timeout ceiling when wiring both checkout providers.

Evidence: At the pinned head, Config(process=ProcessConfig(allowed_binaries=('wc',), timeout_seconds=0.1)) makes ctx.process.allows('git') false, but Context._wire constructs ProcessRunner(('git',), timeout_seconds=FETCH_TIMEOUT_SECONDS) and the local runner similarly. ProcessRunner._validate enforces its own allowlist, so these fresh runners replace rather than inherit the configured restriction. This entire wiring is added relative to PR312's pinned base.

Verification: Executed RequiredPreflight-checkout-repro.py against this exact core head with real git and a disposable local repository: configured runner allows git: False; local checkout succeeded despite denial: True; mirror and local runner timeouts were both 600.0 rather than configured 0.1. CoreCheckout-smoke.py 311 independently produced configured_git_allowed False / checkout_git_allowed True. No application source was changed.

[P1] Use HTTP Basic authentication for GitHub git fetches

Location: src/traust_core/v1/providers/forge_api.py:55–58

The GitHub branch returns the REST API Bearer header for the git smart-HTTP transport. GitHub rejects that header on /info/refs even when the same configured token has repository access, so every mirror checkout with a configured GitHub token fails authentication, including public repositories where the supplied invalid auth prevents anonymous access. Encode the GitHub token as HTTP Basic (for example x-access-token:), as required by the git transport, rather than reusing REST authentication.

Evidence: MirrorCheckout._fetch_env obtains ForgeApi.git_headers(repo.host) and installs its Authorization value in git's http..extraheader. At this head GitLab uses Basic but the GitHub return at line 58 still uses Bearer. The new mirror fetch path and this header method are added in PR312's base-to-head diff.

Verification: Read-only real GitHub smart-HTTP probes with one valid gh credential returned HTTP 401 text/plain for Bearer and HTTP 200 application/x-git-upload-pack-advertisement for Basic using the same token, on both traust-core and traust-internal. Executed CoreCheckout-auth-probe.py through bash with Homebrew Python; no token was printed or written. An initial Framework-Python HTTPS attempt failed certificate validation before HTTP and was replaced by the functioning Homebrew interpreter.

[P2] Serialize mirror eviction with in-flight checkout acquisition

Location: src/traust_core/v1/providers/mirror.py:96–100

An idle mirror remains eligible for eviction while checkout is fetching it: _fetch holds the sibling .lock, but evict ignores that lock, and checkout updates last-use / registers its worktree only after the fetch. Concurrent cache maintenance can therefore delete a mirror during an active checkout, causing the real git fetch or subsequent worktree creation to fail. Use the same per-mirror synchronization for eviction and hold acquisition protection until the worktree or equivalent live-use marker is established, then recheck idleness/live worktrees before deleting.

Evidence: _fetch takes flock at lines 109-110; evict only checks the old traust-last-checkout timestamp and worktrees directory before shutil.rmtree. checkout calls _touch and worktree add after _fetch returns, leaving the old-idle mirror unprotected from eviction. These methods are new in PR312.

Verification: Executed CoreCheckout-smoke.py 311 with PYTHONPATH pointing to this pinned head. A real disposable git mirror was marked 40 days idle, and eviction was deterministically interleaved immediately before the actual fetch while _fetch held its lock. evict(30) removed https://github.com/o/r; checkout then raised ProcessError, git exit 128, cannot change to the deleted mirror directory. The repro substitutes a local git origin only to avoid external network dependency.

[P2] Accept valid SHA-256 commits in the new local checkout path

Location: src/traust_core/v1/providers/local.py:37–40

The new local provider accepts GitSha values containing 64 hexadecimal characters but checks them using GitCheckout.reachable, whose private regex accepts only 7-40 characters. Consequently a healthy local repository created with git init --object-format=sha256 can never be checked out: an existing commit is reported as absent before git is even asked. Extend the reachability validator to the same supported SHA shapes as GitSha so the local seam honors its accepted input.

Evidence: models.values.GitSha permits SHA-1 or SHA-256 values; providers.checkout._SHA remains [0-9a-f]{7,40}, and reachable short-circuits to false on a 64-character SHA. LocalCheckout and its call to reachable are new relative to PR312's pinned base.

Verification: Executed CoreCheckout-smoke.py 318 against this exact core head. Created and committed a real disposable SHA-256 git repository; git cat-file confirmed the 64-character commit exists. LocalCheckout.checkout raised NotFoundError for that same commit.

[P2] Fetch requested commits retained only under PR or MR refs

Location: src/traust_core/v1/providers/mirror.py:124–126

The mirror fetches only branch and tag refs, without fetching the requested commit. A valid requested SHA retained solely by refs/pull//head or refs/merge-requests//head is therefore absent from a fresh mirror even though the forge still serves it. Whole-tree checkout then reports NotFoundError, preventing analysis or verification of such PR/MR heads; the same problem affects refresh for a retained non-branch head. Fetch the requested SHA (or its forge change ref) when it is not reachable after the normal branch/tag update, and only then decide the commit is unavailable.

Evidence: At this exact head, checkout passes only request.repo to _fetch and checks request.ref afterward. _fetch's only refspecs are +refs/heads/:refs/heads/ and +refs/tags/:refs/tags/ at lines 125-126; neither checkout nor refresh requests the missing SHA. These methods are added in PR312 relative to its own pinned base.

Verification: Executed CoreCheckout-pr-ref-smoke.py against pinned core312 be35227. A real isolated git origin contained the requested commit exclusively under refs/pull/1/head, confirmed by git cat-file and git rev-parse. The production MirrorCheckout with a fresh mirror fetched real git branch/tag refs and raised NotFoundError for that existing SHA. The local origin substitution only removes external-network dependence.

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.

2 participants