Skip to content

fix(detection-quality): preserve export-ignored regression sources - #325

Merged
smith-xyz merged 3 commits into
developmentfrom
fix/regression-pairs-complete-trees
Oct 31, 2026
Merged

smith-xyz merged 3 commits into
developmentfrom
fix/regression-pairs-complete-trees

Conversation

@chubirka

Copy link
Copy Markdown
Member

What

Fix the export-ignored regression-fixture loss carried by merged PR #313:

  • Read the before/fixed finding paths from complete commit-pinned trees through the existing checkout service, not forge tarballs that omit tracked export-ignore files.
  • Remove the obsolete kept-path helper parameter; preserve release on success, read failure and partial acquisition failure.
  • Add real-Git/service coverage for export-ignored source, default-branch drift, genuinely absent/unchanged files and worktree cleanup, with per-test temporary-workspace isolation.

The intentional tradeoff is Git-backed complete trees instead of small archive downloads. No schema, public API or dependency-pin changes.

Verification

  • The new real-Git regression failed against the exact merged development baseline: the confirmed fixed finding yielded no regression pair.

  • Focused SQLite regression-pair suite: 20 passed.

  • Standalone SQLite/service smoke produced the exact vulnerable/fixed PairFile despite export-ignore and a later default-branch commit, and released both worktrees.

  • Mandatory pre-push gates passed: lint plus 8,389 passing unit/coverage cases, 1 existing skip, 96.19% coverage (80% required).

  • Native PostgreSQL 18.6 integration suite: 3,031 cases passed, 3 existing skips, across the complete invocation and targeted reruns of two initially environment-blocked cases. The isolated cluster was set to UTC and temporary roots moved outside the workstation's parent Git repository; no unrelated production behavior was changed.

  • An initial parallel unit run exposed shared-temporary-directory interference. Added checkout regressions now isolate their own temporary workspaces; the complete pre-push coverage run passed afterward.

Related: #313; companion checkout-provider fix addresses the shared transport/cache findings. Continuous operations / XWING-2440.

This PR is for review only; auto-merge is not requested.

@smith-xyz

Copy link
Copy Markdown
Contributor

/agentic_review

@qodo-for-traust-security

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

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1)



View medium (1)
🟠 **Medium**
1. Some pinned commits lose regression pairs 🐞 Bug
Description
_pinned now routes paired reads through MirrorCheckout, whose fetch imports only branch and tag
refs before checking whether the requested SHA is reachable. A recorded commit available only
through another forge ref, such as a pull-request ref, is marked unreadable even if its
commit-pinned archive can still be retrieved.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Regression pairs now require the mirror to contain each recorded SHA, but its fetch excludes commits available only under other forge refs.
## Fix Focus Areas
- src/traust_core/v1/services/detection_quality.py[967-968]
- src/traust_core/v1/providers/mirror.py[105-130]
## Recommended Fix
Add a safe way for the checkout service to acquire a requested recorded commit when it is absent from fetched heads and tags, while continuing to use complete Git trees for regression sources. Cover a commit reachable only through a non-branch, non-tag ref and verify the resulting pair.

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

Dismiss ↗ | View ↗



Resolved findings
1. Repository symlinks expose host files ✓ Resolved
Description
_pinned now supplies full Git worktrees to workspace_file, which follows symlinks at a finding
path without checking that their targets remain inside the worktree. If a reported path is a symlink
to a readable host file in the pre-fix commit, a confirmed fix can place that file's contents in a
regression pair and persist them in a rule draft.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Full Git worktrees preserve repository symlinks, and regression-pair reads can follow them outside the checkout.
## Fix Focus Areas
- src/traust_core/v1/services/detection_quality.py[967-971]
- src/traust_core/v1/providers/sources.py[42-46]
## Recommended Fix
Before reading a finding file, reject symlinks in the file and its parent path or otherwise enforce that the resolved regular file belongs to the checkout. Add a regression test with an export-ignored finding path symlinked outside the worktree; verify that no host-file contents enter a pair or draft.

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


2. Large findings can exhaust worker memory ✓ Resolved
Description
Removing keep bypasses the archive provider's 64 MiB cap, while _pair still loads each requested
file in full with workspace_file and retains its text in the returned pair. A confirmed finding
whose path names a very large tracked file can therefore consume unbounded memory during regression
drafting, even when the pair limit is 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
Complete-tree checkout removes the prior kept-file size cap, but regression drafting still reads and retains complete file contents.
## Fix Focus Areas
- src/traust_core/v1/services/detection_quality.py[967-981]
- src/traust_core/v1/providers/sources.py[42-46]
## Recommended Fix
Enforce a size budget on regression-pair source reads before loading file contents, and return a visible unreadable or oversized skip when it is exceeded. Cover a tracked file above the budget using the mirror-backed checkout.

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



@smith-xyz

Copy link
Copy Markdown
Contributor

I checked the three Qodo claims against this diff and the shared provider; they are not safe to dismiss yet. workspace_file in providers/sources.py calls is_file()/read_text() on a worktree path, which follows a symlink outside the checkout, and has no byte cap; replacing the kept archive with a full mirror therefore exposes both the host-file and unbounded-read paths. The mirror currently fetches heads/tags, so a commit retained only by a review ref can also lose its pair. #326 proposes SHA fallback for that last case, but its bare-SHA attribution has a separate open trust-boundary finding. Please keep #325 in review until containment/size limits and a proven-repository-ref fetch are tested; I am not editing your branch concurrently.

@smith-xyz

Copy link
Copy Markdown
Contributor

Fixed two of the three Qodo findings in 29f915a. Regression-source reads now refuse symlinks escaping the worktree and cap each before/after file at 1 MiB; oversized inputs are explicitly skipped as unreadable rather than retained in a pair. Both end-to-end regressions failed before the fix, and provider tests cover the exact byte boundary and allowed internal symlinks. make lint-fix, make test (8,392 passed), and pre-push coverage (96.70%) passed. The review-ref finding remains open: core#326 aa323c8 now fetches current advertised PR/MR heads safely instead of trusting a bare SHA. I will add its regression and reconcile this branch only after #326 passes review/merges; I am not claiming the remaining finding fixed yet. Please review the 1 MiB skip policy and changed test/behavior before #325 merges.

@smith-xyz

Copy link
Copy Markdown
Contributor

The first Python 3.12 CI run on 29f915a failed in an unrelated archive cleanup assertion: that test counted checkout-* directories in the shared system temp root while pytest-xdist was running other checkout tests. Commit 8023609 scopes this existing test’s temp root to its own tmp_path; the full local suite and pre-push coverage passed again. Fresh CI is running. This changed existing test still needs human review, and the review-ref Qodo finding remains pending.

@smith-xyz
smith-xyz merged commit 67d480d into development Oct 31, 2026
6 checks passed
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