Skip to content

fix(corpus): key the total-failure guard on failures, not on skips - #471

Open
JOhnsonKC201 wants to merge 1 commit into
calesthio:mainfrom
JOhnsonKC201:fix/corpus-total-failure-with-skips
Open

fix(corpus): key the total-failure guard on failures, not on skips#471
JOhnsonKC201 wants to merge 1 commit into
calesthio:mainfrom
JOhnsonKC201:fix/corpus-total-failure-with-skips

Conversation

@JOhnsonKC201

Copy link
Copy Markdown

Summary

Follow-up to #466. That PR added a fail-closed guard so a corpus build that processes candidates and embeds none of them stops reporting success. The guard is right, but it is keyed on skipped, which disables it as soon as a single already-present clip shows up in the run.

total_failure = bool(candidates_seen) and not added_ids and not skipped

skipped counts clips that were already in the corpus. It says nothing about whether the candidates we actually processed succeeded. So a run that skips one already-present clip and then fails the other nine on a broken decoder or CLIP stack returns success=True, clips_added: 0, error: None, which is exactly the silent-empty-index failure the guard was added to prevent.

The existing comment already describes the intended rule: "a no-result or skip-only run remains valid." A skip-only run is one where failed == 0. This keys the condition on that instead.

Related issue

Refs #466, #424

Changes

  • tools/video/corpus_builder.py: key the guard on failed > 0 rather than not skipped, so already-present clips no longer suppress a total processing failure.
  • tools/video/corpus_builder.py: make the diagnostic accurate when prior clips exist. "corpus index is empty" is false in that case, so the message now reports no clips added (N already present). The original wording is unchanged when skipped == 0.
  • tests/tools/test_corpus_builder_total_failure.py: extend the run_builder fixture with an optional existing argument that marks clip ids as already present, and add two regression tests.

Behavior is unchanged for every case #466 already covered.

Testing

Verified the new test fails on current main before the fix, with the exact silent success described above:

assert result.success is False
E  AssertionError: assert True is False
   ToolResult(success=True, ..., 'clips_added': 0, 'clips_skipped_existing': 2,
              'clips_failed': 3, ..., error=None)

After the fix:

python -m pytest tests/tools/test_corpus_builder_total_failure.py -v
4 passed

python -m pytest tests/ -k corpus
12 passed, 1 skipped

Both tests added by #466 pass unchanged. Nothing else in the tree asserts on the old error wording.

Platform: Windows, Python 3.13.

Checklist

  • The change is focused on a single logical concern.
  • I ran the relevant tests locally (make test-contracts / make test) where applicable.
  • I updated docs/README if behavior or usage changed.
  • No unrelated files (build artifacts, local config) are included in the diff.

The guard added in calesthio#466 exempts any run that contains a skipped clip:

    total_failure = bool(candidates_seen) and not added_ids and not skipped

`skipped` counts clips that were already in the corpus, so it says
nothing about whether the candidates we actually processed succeeded. A
run that skips one already-present clip and then fails the other nine on
a broken decoder or CLIP stack returns success with clips_added: 0 and
error: None, which is the same silent-empty-index failure the guard was
added to prevent.

The comment already states the intended rule, "a no-result or skip-only
run remains valid", and a skip-only run is one where failed == 0, so key
the condition on failed instead of on skipped.

Also make the diagnostic accurate when prior clips are present, since
"corpus index is empty" is false in that case.

Adds two regression tests: total failure alongside skips must fail
closed, and a skip-only run must stay successful.
@JOhnsonKC201
JOhnsonKC201 requested a review from calesthio as a code owner August 3, 2026 15:18
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.

1 participant