[ptr727]

Summary

Review on the promotion pull request #1234 found that output_tracks's refusal of two dumped paths sharing a file name went uncaught. So check_results.py --baseline ended in a traceback, after the outcome comparison ran but before its differences printed.

• compare_logs reports the shared name as an unverified track check, naming the run it came from. The comparison still prints every outcome difference and the log-line warnings, and exits 1.
• RegressionTests/README.md "Comparing two runs" states how results and track dumps are keyed, and when a shared name leaves the track check unverified.

Declined: a result with no track dump in either run

A reviewer asked that a result with no dump in either run fail as unverified. Each tool read logs its Reading media info from tools line before the tools run (SidecarFile.cs), so a file no run dumped was never read in either run, and it has no output tracks to compare. A file that fails before its first read records such a result. A by-extension remux failure is one case, and a rename collision or a read-only file is another. Requiring a dump there would fail two runs of one build against each other. The file's outcome is still compared, and a file one run read and the other did not already fails, per #1235.

Test plan

• python3 -m unittest discover -s tests in RegressionTests/: 95 tests pass. test_shared_name_keeps_outcome_differences asserts the outcome difference, the exact unverified track check line, and the log-line warning.
• uvx ruff@0.17.3 check ., uvx ruff@0.17.3 format --check ., uvx mypy@latest, and prose_lint.py --diff origin/develop: clean.

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[ptr727]

A recorded local strict-review pass covers head 7e799e5c506c553835b67a9f2509be8608462080, the content this pull request carries at that commit against develop, and it recorded 1 finding.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Report shared track dump names as unverified checks

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 10-20 Minutes

AI Description

• Report ambiguous track dump names as unverified checks instead of crashing baseline comparisons.
• Preserve outcome differences and normalized log warnings, and exit with a failure.
• Add regression coverage and document the behavior.

Diagram

graph TD
  Compare["Baseline comparison"] --> Outcomes["Outcome differences"]
  Compare --> Logs["Log comparison"] --> Dumps["Track dumps"] --> Shared{"Shared name?"}
  Shared -->|yes| Unverified["Unverified check"]
  Shared -->|no| TrackDiff["Track differences"]
  Logs --> Warnings["Log warnings"]

Loading

High-Level Assessment

Catching the ambiguity at the comparison boundary preserves output_tracks' refusal to mix dumps while allowing other findings to print. A structured parser result was considered, but would add complexity for this single failure mode.

Files changed (3) +25 / -9

Bug fix (1) +15 / -8

check_results.pyConvert shared-name errors into track-check failures +15/-8

Convert shared-name errors into track-check failures

• Extracts each run's tracks separately and catches shared-name ValueErrors, identifying the affected run. It retains the no-dump and track-difference checks when extraction succeeds, then continues comparing normalized log lines.

RegressionTests/check_results.py

Tests (1) +9 / -0

test_check_results.pyTest shared-name failure alongside an outcome difference +9/-0

Test shared-name failure alongside an outcome difference

• Adds a baseline comparison test that requires exit code 1, the outcome difference, and an unverified track check naming the results run. It does not explicitly assert that normalized log-line warnings still print.

RegressionTests/tests/test_check_results.py

Documentation (1) +1 / -1

README.mdDocument ambiguous track dumps as unverified +1/-1

Document ambiguous track dumps as unverified

• Clarifies that two dumped paths sharing a file name leave the track check unverified, rather than describing an uncaught error.

RegressionTests/README.md

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Shared-name tests miss lost log warnings ✓ Resolved

Resolving commit: 6cd5942

Resolution reason

N/A

Description

test_shared_name_keeps_outcome_differences checks the outcome error and unverified-track error but
never checks the normalized-log warning produced by its extra dump line. If compare_logs returns
after detecting the shared name, this test still passes even though the promised log-line comparison
no longer runs.

---

Tip of the day

💡 Did you know, you can see how risky a PR is at a glance with blast radius labels

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[Copilot]

🟡 Changes recommended

The regression test does not verify that log-difference checking continues after the output-track failure, leaving the intended behavior insufficiently covered.

1 open issue

• Test that log differences are checked after output_tracks fails · New

What changed in this PR

This pull request makes regression-run comparisons resilient to duplicate track-dump filenames, reporting the track check as unverified while continuing outcome and normalized-log comparisons.

Changes:

• Handles shared dump-name errors without aborting comparison.
• Documents the new unverified-track behavior.
• Adds regression coverage for shared names and outcome differences.
• Review note: the regression test should also assert that normalized log warnings are emitted after the track check becomes unverified.

File	Summary
RegressionTests/​tests/​test_check_results.py	Adds coverage for shared dump names and preserved outcome differences; should also assert the normalized log warning.
RegressionTests/​README.md	Documents shared-name handling during run comparison.
RegressionTests/​check_results.py	Handles duplicate dump names without preventing outcome and normalized-log comparisons.

_{Review effort: Lite}

[ptr727]

A recorded local strict-review pass covers head 0ea42d413fdd485964dd211624c06541e12a8bca, the content this pull request carries at that commit against develop, and it recorded 1 finding.

[ptr727]

A recorded local strict-review pass covers head 6cd5942ec8d731be24a0679f78099d633350ccb9, the content this pull request carries at that commit against develop, and it recorded 0 findings.

[ptr727]

@qodo-free-for-open-source-projects finding 1, "Shared-name tests miss lost log warnings", is fixed in 6e785c3: test_shared_name_keeps_outcome_differences asserts the ::warning::log results only line for the extra dump, so lost log warnings fail it.

[qodo-free-for-open-source-projects]

free-for-open-source-projects finding 1, "Shared-name tests miss lost log warnings", is fixed in 6e785c3: test_shared_name_keeps_outcome_differences asserts the ::warning::log results only line for the extra dump, so lost log warnings fail it.

Thanks for the update. I can't verify commit 6e785c3c from here, and the findings catalog for this PR is currently empty, so I can't confirm the status of finding 1 on my end. I haven't changed anything.

If you want the review to reflect the fix, you can:

• Ask for a fresh review (for example, "@qodo review again"), which will re-check the test file against the latest commit.
• Dismiss finding 1 if you consider it resolved or not applicable. Only the PR author can do this, and I'll only do it on your explicit request.
• Ask me to explain the finding's evidence, so you can confirm the new assertion covers the normalized-log warning for /Test/Media/b/a.mkv.

[ptr727]

@coderabbitai review

[coderabbitai]

Action not completed

Pull request is closed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.