Skip to content

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

Merged
smith-xyz merged 5 commits into
traust-security:developmentfrom
smith-xyz:feat/fail-loudly-on-what-s-required-one-preflight-a-l
Nov 1, 2026
Merged

smith-xyz merged 5 commits into
traust-security:developmentfrom
smith-xyz: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 4, the traust-engine part only: the start gate on core's preflight. The traust step (doctor on the same door) is a later PR pinned to this one.

Core pin: 09ad226 — core#312's head (contains #311, #318; the Context.preflight door; contracts at 726d273). The previous pin e8ab152 was orphaned by a rebase, so the branch is re-pinned. Engine #132 and this PR now carry the identical pin 09ad226; merge after the core PRs.

What

  • run, shadow and claim call Context.preflight on the pack's enabled assets' requirements and exit non-zero with the full gap list before planning anything (ops/cli.py); claim checks the assets the claim runner can claim (runnable), a pass checks every enabled asset. The CLI registers core's shipped toolchain so tool requirements are checked against the shipped pins.
  • enabled: false is the only exemption, filtered at the pack layer (ops/pack.py::requirements): a disabled asset is not preflighted, not planned, not in freshness.fail overdue, not claimable, and a route to its lane plans nothing.
  • An enabled asset without run or model.role is a pack error at Pack.load (naming enabled: false as the exemption); the "unrunnable → warning, keep ticking" path is gone (Pack.unrunnable deleted, TickReport.unrunnable with it).
  • The engine refuses to start when the pack sets route_on and no --router is given (a ConfigError naming the flag), instead of _route silently returning with routing off.
  • A route to a lane the pack runs no asset for is a ConfigError that fails the pass before planning anything (_route runs ahead of the planning loop), instead of a warning the run carries on past.
  • The stale-upstream log stays untouched: stale stops are withheld upstream-stale orders, an explicit degraded outcome.
  • The suite's fixture config home gains a traust.yaml (locations + the ledger key — the audited fields) so core's preflight, which promotes the completeness audit's warnings to findings, passes for the CLI tests.

No new or changed repository queries: preflight's facts check reuses core's read-only artifacts.latest_imported door, so no test-repositories / perf run. make lint-fix && make test: 158 passed; the clean base (old pin, no changes): 146 passed — no merge skew.

Deviations to look at:

  • Router lanes ⊆ pack lanes "at load": implemented as the ConfigError above, raised at the first route (still before any planning in that pass). A true load-time check needs the router's lane set, which the core Router protocol does not expose — that would be a core door (noted as a follow-up), not something this engine PR can invent.
  • A disabled asset's lane is skipped, not an error: enabled: false is declared-optional in config (the plan's reviewer check allows exactly that), so a route into a disabled asset's lane plans nothing, matching what the old unrunnable skip did.

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
src/traust_engine/ops/pack.py::unrunnable + the controller's log.warning("not planned, no run or model.role") retire an enabled asset without run or model.role is a pack error at Pack.load; enabled: false is the exemption. The warn-and-carry-on path (weeks of partial data that looks complete) is gone
ops/controller.py::_route's silent early return when route_on is set and no router was given port into the CLI start gate: run/shadow refuse with a ConfigError naming --router
ops/controller.py's log.warning("routed lanes no pack asset runs") port into a ConfigError raised by the tick before planning anything: router lanes must be a subset of the pack's lanes

Changed tests

Every existing test edited or deleted, with why (each is the plan's step-4 decision; new tests are not listed):

Test Old → new Why
tests/test_engine_loop.py::test_a_route_to_an_unrunnable_lane_plans_nothing_for_it renamed test_a_route_to_a_disabled_assets_lane_plans_nothing_for_it; built iac_lane without run (asserted pack.unrunnable()) → now sets enabled: false on it the unrunnable path is a pack error at load; the not-planned behaviour lives on as the enabled: false exemption
tests/test_engine_loop.py::test_a_route_to_a_lane_no_asset_runs_plans_nothing_for_it renamed test_a_route_to_a_lane_no_asset_runs_fails_the_pass; asserted the "routed lanes no pack asset runs" warning and that the run carried on → now expects ConfigError router lanes must be a subset of the pack's lanes: a loud failure, not a warning
tests/test_engine_loop.py::test_an_asset_no_runner_can_make_is_reported_not_planned rewritten as test_a_disabled_asset_is_not_planned_or_reported; repo_state without run was reported unrunnable and skipped every tick → the test now disables it (enabled: false, with requires and freshness.fail) and asserts not planned, not overdue an enabled asset without run/model.role now fails Pack.load (covered in test_pack.py); the kept-in-pack-without-requirements behaviour is the exemption
tests/test_engine_loop.py::test_a_route_to_a_lane_no_runner_can_make_plans_nothing deleted the pack it built (diff_scan without run, an enabled asset) is now a pack error at Pack.load (covered by test_pack.py::test_an_enabled_asset_without_run_or_model_role_is_a_pack_error); the lane-not-planned behaviour is covered by the two rewrites above
tests/test_ops_shadow.py::test_shadow_prints_what_a_pass_would_plan_and_diff_compares_it shadow ran on the fixture pack (which sets route_on) with no --router → runs on the same pack without route_on (new _unrouted_pack helper; the _cli helper gained a pack keyword, default unchanged) a pack that sets route_on now refuses to start without --router
tests/test_ops_shadow.py::test_run_once_plans_and_admits same: run --once on the unrouted pack same

New tests: tests/test_ops_preflight.py (10: run and shadow refuse before planning naming every gap — gitleaks uninstalled plus an unrecorded adopter fact — one case per requirement kind crossing the engine boundary, a met requirement lets the engine start, claim checks only the assets it can claim, a routing pack without a router refuses to start), tests/test_pack.py (2: the pack error and the enabled: false exemption), tests/test_ops_runner.py (1: a disabled asset is not claimable).

  • 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

Adopt the Core #312 preflight door before the start gate. The application companion checks the same pack/preflight contract; this is shared behavior, not a new Python import from Engine into the application.

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.

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2026
…2440)

Pin core to e8ab152 (the preflight door, core PR 312; contracts PR 102
transitively at 55b69a9), then make the engine refuse before any work:

- run, shadow and claim call Context.preflight on the enabled assets'
  requirements (claim: the assets the runner can claim) and exit 2 naming
  every gap with its fix
- an enabled asset without run or model.role is a pack error at Pack.load;
  enabled: false is the only exemption (not planned, not claimed, not
  preflighted)
- refuse to start when the pack sets route_on and no --router is given
- a route to a lane the pack runs no asset for fails the pass instead of
  warning and carrying on
@openshift-ci

openshift-ci Bot commented Oct 30, 2026

Copy link
Copy Markdown

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2026
@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 a team as a code owner October 30, 2026 18:47
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2026
@qodo-for-traust-security

Copy link
Copy Markdown

PR Summary by Qodo

Gate continuous-ops starts on preflight and required pack configuration

✨ Enhancement 🐞 Bug fix 🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes


AI Description

• Preflight run, shadow, and claim against applicable asset requirements before planning or claiming
 orders.
• Reject unrunnable enabled assets and missing routers; fail passes when routes target unknown
 lanes.
• Exclude disabled assets from preflight, planning, freshness failures, and claims.
Diagram

graph TD
  P["Pack load"] --> V{"Assets valid?"} --> C["CLI start"] --> F["Core preflight"] --> G{"Gaps found?"}
  G -->|No| E["Pass or claim"]
  V -->|No| X["Configuration error"]
  C -->|Missing router| X
  G -->|Yes| X
  E -->|Unknown lane| X
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Declare router lanes for load-time validation
  • ➕ Could reject unsupported lanes before starting any pass, including before lease requeueing.
  • ➖ Requires a new router contract and may not fit routers whose outputs depend on repository state.

Recommendation: Keep core's single preflight as the shared requirements gate rather than duplicating checks in the engine. Runtime rejection of unknown lanes is a reasonable fit for dynamic routers; a declared-lanes contract is worth considering only if load-time validation becomes necessary.

Files changed (12) +256 / -74

Enhancement (2) +28 / -7
cli.pyPreflight passes and claims before execution +14/-3

Preflight passes and claims before execution

• Registers core's shipped toolchain and preflights all enabled assets for run and shadow, or only claimable assets for claim. Rejects a routing pack started without --router; failures use the existing error exit path.

src/traust_engine/ops/cli.py

pack.pyValidate enabled assets and expose their requirements +14/-4

Validate enabled assets and expose their requirements

• Adds enabled and requires fields to asset specifications. Pack validation rejects enabled assets lacking both a command and model role, while requirements() returns only enabled assets' requirements for preflight.

src/traust_engine/ops/pack.py

Bug fix (2) +19 / -24
controller.pySkip disabled assets and reject unknown routed lanes +16/-21

Skip disabled assets and reject unknown routed lanes

• Removes unrunnable-asset reporting and excludes disabled assets from due planning, routed orders, and overdue freshness counts. A route to a lane with no pack asset now raises ConfigError before orders are planned.

src/traust_engine/ops/controller.py

runner.pyExclude disabled assets from claims +3/-3

Exclude disabled assets from claims

• Restricts runnable command assets to those marked enabled, so the claim runner does not select disabled assets.

src/traust_engine/ops/runner.py

Tests (6) +204 / -38
traust.yamlComplete the CLI test configuration home +9/-0

Complete the CLI test configuration home

• Supplies fixture locations and the ledger public-key path so core's completeness audit does not block otherwise valid preflight tests.

tests/fixtures/config/traust.yaml

test_engine_loop.pyAssert disabled-asset and routing behavior +12/-32

Assert disabled-asset and routing behavior

• Replaces unrunnable-asset expectations with checks that disabled assets are not planned or reported overdue. Verifies that routes to unsupported lanes fail the pass, while routes to disabled assets plan no order.

tests/test_engine_loop.py

test_ops_preflight.pyCover CLI preflight start gates +143/-0

Cover CLI preflight start gates

• Tests aggregated gaps, each requirement kind, successful startup, and the different requirement scopes for passes and claims. Also verifies that a routing pack without --router exits before creating orders.

tests/test_ops_preflight.py

test_ops_runner.pyTest disabled assets are not claimable +6/-0

Test disabled assets are not claimable

• Adds a runnable() assertion showing that disabling a command asset removes it from the claim runner's eligible set.

tests/test_ops_runner.py

test_ops_shadow.pyAdapt CLI tests to explicit router requirements +15/-5

Adapt CLI tests to explicit router requirements

• Allows the CLI test helper to accept a pack and uses a pack without route_on for run and shadow tests that do not provide a router.

tests/test_ops_shadow.py

test_pack.pyTest pack validation and requirement exemptions +19/-1

Test pack validation and requirement exemptions

• Checks that an enabled asset without a command or model role fails loading, and that a disabled asset's requirements are omitted from pack preflight requirements.

tests/test_pack.py

Other (2) +5 / -5
pyproject.tomlPin core to the preflight-capable revision +1/-1

Pin core to the preflight-capable revision

• Changes the traust-core Git revision to the version supplying Context.preflight and the required contracts vocabulary.

pyproject.toml

uv.lockLock compatible core and contracts revisions +4/-4

Lock compatible core and contracts revisions

• Updates the locked traust-core revision and its transitive traust-contracts revision to match the preflight and requirement interfaces.

uv.lock

@qodo-for-traust-security

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

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1)



🔴 High

1. A misspelled routing asset crashes pack loading 🐞 Bug ⭐ New
Description
Pack._graph indexes self.assets[self.route_on] in the disabled-asset check before verifying that
route_on names an asset. When an otherwise runnable pack has a misspelled route_on, the
resulting KeyError bypasses Pydantic validation and Pack.load’s error handling, reaching CLI
commands as an uncaught traceback instead of the intended ConfigError and exit code.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Pack._graph` looks up `self.assets[self.route_on]` before confirming that `route_on` names an asset. For an otherwise runnable pack with an unknown `route_on`, this raises a `KeyError` that bypasses `Pack.load`’s configuration-error handling.

## Fix Focus Areas
- src/traust_engine/ops/pack.py[126-145]
- tests/test_pack.py[81-85]

## Recommended Fix
Validate that `route_on` exists before looking up whether its asset is enabled: move the existing membership-and-`trigger.every` check ahead of the disabled-asset check, or guard the new lookup with `self.route_on in names`. Preserve the existing cadence validation. Make the unknown-`route_on` test’s asset runnable, for example with `a: {run: x}` or a model role, and assert that `Pack.load` raises `ConfigError` for the unknown route.

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

Dismiss ↗ | View ↗



Resolved findings
1. Disabling an upstream asset stalls its dependents ✓ Resolved
Description
Pack._graph accepts enabled assets whose deps or route_on name a disabled asset, without
rejecting the configuration at load. Because Controller.due skips disabled assets, a missing or
stale dependency materialization leaves the enabled dependent withheld with an upstream-stale stop
every window, while routing can quietly rely on an asset the engine never plans.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Pack loading accepts an enabled asset whose `deps` or `route_on` names a disabled asset. The engine does not plan the disabled asset, so a dependent without a fresh upstream materialization is repeatedly withheld, while routing can depend on an asset the engine never plans.

## Fix Focus Areas
- src/traust_engine/ops/pack.py[105-128]
- src/traust_engine/ops/controller.py[103-125]

## Recommended Fix
In `Pack._graph`, after the existing unmade check, validate that every dependency of an enabled asset is enabled and raise a `ValueError` identifying the affected dependent and disabled prerequisite. Also reject a `route_on` target that names a disabled asset. Add tests in `tests/test_pack.py` covering both configurations.

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


2. A failed route can consume its feed event ✓ Resolved
Description
_route now raises for an unknown lane after _changes has advanced the controller's in-memory
feed cursor, without restoring it on failure. If a caller corrects the router and retries on the
same EngineLoop, that pass reads beyond the failed event and cannot plan its route; restarting the
CLI creates a new cursor and avoids this particular loss.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
An unknown-lane error leaves the feed cursor past events that the failing pass did not plan.
## Fix Focus Areas
- src/traust_engine/ops/controller.py[175-220]
- src/traust_engine/ops/controller.py[222-246]
## Recommended Fix
Keep the feed cursor tentative until all routes have been validated, or restore its previous value before raising the unknown-lane error. Test a failed pass followed by a corrected retry on the same controller.

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


3. Disabled lanes appear as route mismatches ✓ Resolved
Description
Controller._route suppresses an order for a disabled lane, but _diff still supplies that lane to
compare_routes as one the pack runs. When expected routing includes the disabled lane, the
comparison retains it as an expected route and reports a difference against the intentionally absent
engine order.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Route comparisons treat disabled lanes as active even though routing no longer creates orders for them.
## Fix Focus Areas
- src/traust_engine/ops/cli.py[125-130]
- src/traust_engine/ops/shadow.py[161-185]
## Recommended Fix
Pass only enabled assets' lanes from `_diff` to `compare_routes`, so expected disabled lanes receive the same treatment as lanes with no runnable pack asset. Add a route-comparison test for a disabled lane.

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



Comment thread src/traust_engine/ops/controller.py Resolved
Comment thread src/traust_engine/ops/controller.py Resolved
Comment thread src/traust_engine/ops/pack.py Resolved
…st-engine into feat/fail-loudly-on-what-s-required-one-preflight-a-l
…, #318) — e8ab152 was orphaned by a rebase

@chubirka chubirka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review: changes required

Reviewed commit 84299d5cedad9830c653e83fa30fe05017a7bcfc. Found 1 actionable issue(s); details are attached as inline review comments.

Merge hold: keep this PR in draft with auto-merge disabled until the findings have been addressed and the affected behavior has been re-reviewed. Do not mark ready solely because unrelated CI passes.

Comment thread pyproject.toml Outdated

[tool.uv.sources]
traust-core = { git = "https://github.com/traust-security/traust-core.git", rev = "acf67b8a4409ed7afa1b79d7954636b5b169fa64" }
traust-core = { git = "https://github.com/traust-security/traust-core.git", rev = "09ad226d4887712ccd1096eb98169729f61a9980" }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Pin the preflight implementation that audits the supplied core home

The newly added run/shadow/claim gate is shipped against core 09ad226, whose preflight calls config_completeness_audit() without the Context's loaded home. As a result --core does not determine what the gate audits: a fully configured explicit --core is refused if TRAUST_CONFIG_HOME points at an incomplete unrelated home (or no default home exists). Conversely the audit can inspect a different complete deployment. This breaks the documented argument-driven engine startup contract and is masked by tests/conftest.py populating a separate ambient home. Re-pin core and the lockfile to a commit containing the loaded-home preflight fix, or bind the audit explicitly to the supplied configuration.

Evidence: ops/cli.py lines 217 and 232/235 load Context.from_file(args.core) then call its new preflight. The actual manifest and lockfile both pin 09ad226d4887712ccd1096eb98169729f61a9980; at that exact commit context/preflight.py line 45 calls config_completeness_audit() without config_home. Core PR312's reviewed be35227 head already supplies ctx.config.config_home, but that implementation is not what this PR installs. README lines 66-68 state everything the service needs comes from its arguments.

Verification: Executed RequiredPreflight-engine-repro.py using engine head 84299d5 and its actual core pin 09ad226, with real complete traust.yaml, model registry, minimal enabled pack, and a separate incomplete TRAUST_CONFIG_HOME. shadow --core /traust.yaml --pack --once returned 2 naming all three locations and ledger.public_key_path as unset despite their presence in the explicitly loaded file. The gate refused before creating the configured store.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. The existing #133 head still pins 09ad226 and the review hold stands. In its plan worktree I have changed the pin to core#327 (282ebbee) and regenerated the lockfile, but have NOT pushed: the new core readiness audit exposes test-fixture/config-home gaps and a disabled-route-on behavior expectation that require explicit test/behavior review. I will not claim this resolved or bypass the tests to get green.

@chubirka
chubirka marked this pull request as draft October 31, 2026 01:29
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 31, 2026
@smith-xyz

Copy link
Copy Markdown
Contributor Author

Addressed the three pending Qodo findings in ce9f2f5: pack validation now rejects an enabled asset depending on a disabled asset (and disabled route_on); a failed unknown-lane route restores the feed cursor so the corrected router retries the event; diff compares only enabled lanes. Regressions cover each path, including the CLI route comparison. Existing CLI success fixtures now copy the complete valid config home (not a placeholder model registry) to satisfy the pinned core’s completeness preflight; the old “disable repo_state but keep its dependents” test now disables the dependents and removes route_on. Local make lint-fix, make test (163 passed), make check-release, and the pre-push hooks passed. This changes behavior and existing tests, so Michele’s CHANGES_REQUESTED review remains in force pending re-review. The pin still names unmerged core#327; do not merge before a reviewed reachable core pin.

@smith-xyz
smith-xyz requested a review from chubirka October 31, 2026 16:16
@smith-xyz
smith-xyz marked this pull request as ready for review November 1, 2026 01:29
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 1, 2026
@smith-xyz
smith-xyz merged commit 4999f3c into traust-security:development Nov 1, 2026
6 of 7 checks passed
@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (4) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Disabled assets leave orders stranded 🐞 Bug ≡ Correctness
Description
runnable() excludes disabled assets, but Admission.admit() still admits previously planned
orders whenever their asset remains in the pack. If an order is deferred before its asset is
disabled, the next pass can queue it or request approval even though the normal runner can no longer
claim it; it can also consume admission capacity.
Code

src/traust_engine/ops/runner.py[61]

+    return tuple(name for name, spec in pack.assets.items() if spec.enabled and spec.run)
Evidence
The loop calls admission after planning; admission reads all planned orders and checks only whether
each asset exists. Its admit and approval paths lack an enabled check, while the changed runner
filter removes those assets from subsequent claims.

src/traust_engine/ops/loop.py[46-50]
src/traust_engine/ops/admission.py[59-98]
src/traust_engine/ops/runner.py[58-61]
src/traust_engine/ops/runner.py[89-99]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Existing planned orders for a newly disabled asset still pass admission, but the runner excludes that asset from claims.

## Fix Focus Areas
- src/traust_engine/ops/admission.py[59-98]
- src/traust_engine/ops/runner.py[58-61]
- tests/test_engine_loop.py[547-567]

## Recommended Fix
Give planned orders whose asset is disabled an explicit terminal or withheld outcome before admission and before they consume pacing or quota. Add a regression that plans an order, disables its asset, then verifies the next pass does not queue or request approval for it.

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

Dismiss ↗ | View ↗



Remediation recommended

2. Old routes trigger false diff failures 🐞 Bug ≡ Correctness
Description
_diff() now passes only enabled lanes to compare_routes(), which filters expected routes by that
set but retains every route found in shadow ticks. If a shadow log within the comparison window
contains a route from before its asset was disabled, a matching expected route is discarded while
the historical engine route remains, producing a difference and exit status 1.
Code

src/traust_engine/ops/cli.py[137]

+        lanes = iter(pack.lanes)
Evidence
Pack.lanes excludes disabled assets. The comparison applies that lane set only when reading expected
rows, while engine routes from ticks enter the result unfiltered; the added test covers only an
expected disabled route with no historical engine route.

src/traust_engine/ops/pack.py[103-106]
src/traust_engine/ops/cli.py[134-138]
src/traust_engine/ops/shadow.py[155-181]
tests/test_ops_shadow.py[327-340]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Disabling a lane filters expected routes but not historical engine routes from shadow ticks, creating a one-sided comparison.

## Fix Focus Areas
- src/traust_engine/ops/cli.py[134-138]
- src/traust_engine/ops/shadow.py[155-181]
- tests/test_ops_shadow.py[327-340]

## Recommended Fix
Apply the enabled-lane rule consistently to both sides of the route comparison, or compare each historical tick against the configuration effective when it was recorded. Cover a pre-disable shadow route and its expected route in the comparison window.

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

Dismiss ↗ | View ↗


3. A typo in route_on crashes with a traceback 🐞 Bug ≡ Correctness
Description
Pack._graph indexes self.assets[self.route_on] in the new disabled check before checking whether
route_on names an existing asset. When an otherwise runnable pack names a missing route source,
the resulting KeyError bypasses Pack.load’s ConfigError handling and the CLI’s error handler,
leaving run, shadow, claim, record, and diff with a raw traceback instead of the
controlled missing-route message.
Code

src/traust_engine/ops/pack.py[R126-127]

+        if self.route_on is not None and not self.assets[self.route_on].enabled:
+            raise ValueError(f"route_on {self.route_on!r} is disabled: routing would never run")
Evidence
The lookup at lines 126–127 precedes the missing-name guard at lines 141–145. Pack.load catches
OSError, yaml.YAMLError, DocumentError, and pydantic.ValidationError, but not the KeyError
from this lookup; the CLI does not catch it either. Previously, an unknown route_on produced a
ValueError that surfaced as a ConfigError. The existing missing-route test uses an asset without
run or model, so the earlier runnable-asset validation masks this path. This finding is based on
code reading, not execution.

src/traust_engine/ops/pack.py[111-149]
src/traust_engine/ops/pack.py[172-185]
src/traust_engine/ops/pack.py[119-143]
src/traust_engine/ops/pack.py[162-175]
src/traust_engine/ops/cli.py[238-240]
tests/test_pack.py[52-56]

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 `route_on` asset is indexed during `Pack._graph` validation before its existence is checked, allowing a `KeyError` to escape `Pack.load` and the CLI instead of producing a controlled configuration error.

## Fix Focus Areas
- src/traust_engine/ops/pack.py[126-145]
- tests/test_pack.py[52-56]

## Recommended Fix
Validate that `route_on` exists before indexing `self.assets`: move the existing missing-name and `trigger.every` check above the disabled check, or guard the disabled lookup with `self.route_on in names`. Add a regression test in `tests/test_pack.py` with a missing `route_on` and an otherwise runnable asset, and assert that `Pack.load` raises `ConfigError`.

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

Dismiss ↗ | View ↗


4. Core pin differs from the one the PR claims 🐞 Bug ☼ Reliability
Description
pyproject.toml and uv.lock pin traust-core to 282ebbee2ac065ea7f15456d2bafb4f63077c8b2, but
the PR description and commit 4 say the pin is 09ad226 (core #312's head) and identical to engine
#132's pin. The preflight door, Requirement, the pack schema that validate_config("pack", ...)
checks the new enabled/requires keys against, and the toolchain registration all come from this
pin, so the 158-passed result and the "no merge skew" claim may not apply to the revision that would
actually ship.
Code

pyproject.toml[23]

+traust-core = { git = "https://github.com/traust-security/traust-core.git", rev = "282ebbee2ac065ea7f15456d2bafb4f63077c8b2" }
Evidence
The diff shows rev = "282ebbee..." at pyproject.toml line 23, and uv.lock lines 871 and 909 record
the same rev. The PR description says: "Core pin: 09ad226 ... Engine #132 and this PR now carry the
identical pin 09ad226". I could not check the contents of either core revision from this repository.

pyproject.toml[22-23]
uv.lock[869-871]

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 traust-core pin in pyproject.toml/uv.lock (`282ebbee...`) does not match the pin the PR describes (`09ad226`, shared with engine #132).

## Fix Focus Areas
- pyproject.toml[23-23]
- uv.lock[869-871]

## Recommended Fix
Confirm which core revision is intended, which one contains `Context.preflight`, `register_toolchain` and the pack schema with `enabled`/`requires`. Then either re-pin to that revision with `uv lock` or update the PR description, and re-run the suite on the final pin.

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

Dismiss ↗ | View ↗


Grey Divider

Context sources
✅ Linked PRs:
  🌐 PR #132
❌ Tickets: failed to load — Ticket collection could not establish complete evidence
Review mode: Auto: 🧠 Deep: Dense cross-cutting preflight, routing, pack validation, and dependency-pin changes.

Grey Divider

Tip of the day
💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants