Skip to content

tests: replace dead shim tripwires with a live no-coverage check - #77

Open
cargo-affected-bot wants to merge 2 commits into
mainfrom
tests/replace-dead-shim-tripwires
Open

tests: replace dead shim tripwires with a live no-coverage check#77
cargo-affected-bot wants to merge 2 commits into
mainfrom
tests/replace-dead-shim-tripwires

Conversation

@cargo-affected-bot

Copy link
Copy Markdown
Collaborator

Problem

Five assertions across the two basename-collision scenarios grep stderr for strings the code no longer emits, so they are permanently true:

$ rg 'failed to resolve binary_id|basename fallback ambiguous' src/
$   # no matches

Both strings belonged to the pre-NEXTEST_BINARY_ID shim, which resolved binaries by path and probed byte markers to disambiguate colliding basenames. That resolver is gone — the shim now reads NEXTEST_BINARY_ID straight from the env (src/shim.rs#L115-L120) — but the assertions that pinned its behavior stayed behind. tests/CLAUDE.md calls silent under-selection "the one failure mode this tool cannot detect downstream"; these two scenarios are supposed to be part of that guard and currently aren't.

The gap is not theoretical, because a failure to resolve a binary_id is a soft failure. That test lands as TestOutcome::Skipped, and as long as some other binary produced rows, collect still exits 0 (src/collect.rs#L376-L379). So the surviving collect.status.success() assertion doesn't cover it either: a green collect whose database is missing one of the two colliding binaries is exactly the shape both scenarios exist to catch.

Fix

Point each assertion at "produced no coverage" — the line collect prints whenever any test is skipped — and drop the now-redundant second basename fallback ambiguous check. The module docs' used to bail with … history is untouched; those describe the old shim in the past tense and are accurate.

Verification

Per tests/CLAUDE.md ("a test written for a bug must be seen failing against the pre-fix behavior"), each new assertion was run against an injected break — a temporary branch in shim.rs returning Skipped for one of the two colliding binaries, simulating a resolution failure on exactly one target. Both scenarios failed on the new assertion; neither would have failed on the old one.

Injected-break output

Break applied in src/shim.rs (not committed):

let outcome = if binary_id.contains("wt_perf") {
    TestOutcome::Skipped { reason: "TEMP BREAK for tripwire verification".into() }
} else {
    extract(&dir, Path::new(binary), &env)
};

duplicate_basename_with_stripped_debuginfo_resolves_correctly — nextest reports both tests passing, collect exits 0, the database gets one test's rows instead of two, and the old string appears nowhere in stderr:

    Starting 2 tests across 4 binaries
        PASS [   0.007s] (1/2) dup_strip_wt_perf::builds builds
        PASS [   0.012s] (2/2) dup_strip_mock_stub::builds builds
     Summary [   0.012s] 2 tests run: 2 passed, 0 skipped
  skipped dup_strip_wt_perf::builds::builds: TEMP BREAK for tripwire verification
1 test produced no coverage
storing coverage for 1 tests (3 ranges)...
done. 1 tests, 3 ranges stored in target/affected/coverage.db (0.6s total)

panicked at tests/functional/duplicate_target_names.rs:87:5:
duplicate-basename binaries must not cost a test its coverage

lib_bin_same_basename_resolves_via_nextest_binary_id with the break narrowed to bin/wt-perf (both of its targets share the wt_perf_collide package, so the broader predicate tripped the hard mappings.is_empty() bail instead):

    Starting 2 tests across 2 binaries
        PASS [   0.007s] (1/2) wt_perf_collide::bin/wt-perf tests::bin_test_invokes_lib
        PASS [   0.012s] (2/2) wt_perf_collide tests::lib_test_double
     Summary [   0.012s] 2 tests run: 2 passed, 0 skipped
  skipped wt_perf_collide::bin/wt-perf::tests::bin_test_invokes_lib: TEMP BREAK …
1 test produced no coverage
storing coverage for 1 tests (3 ranges)...

With the break reverted, cargo test is green: 41 passed, 0 failed. cargo fmt --check and cargo clippy --all-targets both clean.

Overlap with open PRs

#31 and #63 both retouch the message on !stderr.contains("basename fallback ambiguous") in lib_bin_collision.rs — the assertion this PR deletes. Their other hunks (module docs, inline comments) are independent and unaffected. Whichever lands second will want a trivial conflict resolution on that hunk; if this one lands first, those hunks simply become moot.

The two basename-collision scenarios grep stderr for `failed to resolve
binary_id` and `basename fallback ambiguous` — strings the pre-NEXTEST_BINARY_ID
shim emitted and the current one does not, so five assertions were
permanently true.

A binary_id the shim can't resolve is a soft failure: the test lands as
Skipped and collect still exits 0 on the other binary's rows, so
status.success() doesn't cover it either. Assert on "produced no coverage"
instead — the line collect prints for any skip.

@cargo-affected-bot cargo-affected-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Self-review. CI is green on all three platforms, and the core claim checks out: rg finds neither failed to resolve binary_id nor basename fallback ambiguous anywhere in src/, and Skipped is only ever constructed from real extraction failures in shim.rs — never for a test that merely executes nothing — so the new assertions can't false-positive on the empty builds() test.

One observation beyond the inline nit. The replacements are still string-greps against a collect message, the same shape as the two they retire: a future reword of eprintln!("{skipped} test{s} produced no coverage") in collect.rs puts all four assertions back in the permanently-true state this PR exists to fix, silently. On the first collect in both scenarios that's cushioned — the SELECT DISTINCT binary_id assertions below would fail independently if a target were dropped. The uncushioned spot is the second collect in each scenario (recollect): neither re-queries test_regions afterwards, so the stderr grep is the sole guard there and also the only genuinely new coverage this PR adds. Re-running the same binary_id query after the recollect would pin the property against message edits as well as against a dropped target.

Comment thread tests/functional/lib_bin_collision.rs Outdated
Review noted the replacements are still string-greps against a collect
message, so a reword would silently retire them again — the same drift
this PR fixes. The first collect is cushioned by the existing
SELECT DISTINCT binary_id assertions; the second collect had no DB
re-check at all. Hoist those queries into per-file helpers and run them
after both collects, so the property is pinned independently of the
message text.
max-sixty pushed a commit to max-sixty/tend that referenced this pull request Aug 11, 2026
…g output accepted (#864)

## Problem

Step 2 asks the survey subagent whether each run's output was "accepted
or rejected" but never says who has to do the accepting. Nothing in the
prompt distinguishes a human merging a PR from the bot replying to its
own review thread, so the subagent fills the gap with the most natural
reading — a reply arrived, therefore someone accepted it — and reports a
self-conversation as human acceptance.

That is the worst-shaped failure this skill has: it produces a **false
all-clear**. A survey that credits the bot's own reply to a human
doesn't add noise a later gate can filter, it removes the signal that
would have sent the leg to Step 3.

## Evidence

This leg ([run
31081926964](https://github.com/max-sixty/tend/actions/runs/31081926964),
target `max-sixty/cargo-affected`). The survey subagent reported PR
[#77](max-sixty/cargo-affected#77) twice under
"Runs with accepted output":

> Outcome: ACCEPTED - human developer responded positively to feedback
> Outcome: ACCEPTED - human developer incorporated feedback

and concluded `PR 77: Developer accepted and incorporated bot feedback
(2 cycles of acceptance)`. There is no developer. The PR is bot-authored
and every actor on it is the bot:

```
$ gh api "repos/max-sixty/cargo-affected/pulls/77/comments?per_page=100" --jq '[.[].user.login] | unique'
["cargo-affected-bot"]
```

The same report also listed the PR author as `cargo-affected-bot`, so
the contradiction was internal to the summary — the acceptance claim was
reached without ever comparing the replying login against the bot's.

This is the fourth cheap-subagent mis-attribution recorded in the
[evidence
gist](https://gist.github.com/dca23a6e6a0d8cae2665944ba31676fb), and the
second of this specific sub-class (bot activity attributed to a human):

| Occurrence | Shape |
| --- | --- |
| gist "Analysis-side", leg ending 07:5x | inline replies + commit
`551e059` credited to "human applied suggested changes" |
| gist "Analysis-side", following leg | PR #54 bodies credited to a run
that had not yet started |
| gist "Analysis-side", streak-break leg | fabricated no-op causes;
window boundary ignored |
| **this leg** | **bot's own reply chain on PR #77 reported as human
acceptance** |

Prior legs' response was to stop delegating — ten consecutive legs
record "this leg used **no** survey subagent". That is a per-leg
workaround for a defect in the shared prompt, and it leaves the skill's
stated default (`Delegate all broad exploration to a cheap subagent`)
pointing at a step that mis-reports actors.

## Fix

Three small changes to Step 2, all inside the prompt template:

- Require the subagent to **name the login** behind every
acceptance/rejection signal, sourced from a single `gh pr view --json
number,state,author,mergedBy,reviews,comments,commits` that covers all
five actor surfaces — merge actor, reviews (with state), inline
comments, conversation comments, and commits. A comments-only check
can't source it: neither comments endpoint carries review records or the
merge actor, so a maintainer merging a bot PR without commenting would
come back as bot-only. Inline commenters need no separate call — every
inline comment, including a standalone reply, belongs to a review record
that `reviews` returns. The block notes that `commits` truncates at 100
(oldest-first) while `comments` and `reviews` paginate in full.
- Give bot-only threads their own report bucket (`bot-only — no human
signal`) so they can't be silently filed under "accepted".
- Tell the main agent to verify actor attribution before it enters a
finding, and to judge bot-only threads on content rather than treating
the bucket as either outcome.

The rule is stated so it stays correct when the bot legitimately works
on its own PR: bot-only is *not* a defect (a self-review followed by the
author's fix is designed behavior), it just isn't **acceptance**.
Nothing here discourages delegation — it fixes the instruction the
delegation was missing.

## Gate assessment

- **Evidence level**: High — consistent pattern across multiple
sessions. 4 occurrences total, 2 in the human/bot sub-class this change
addresses. High needs 2–3.
- **Structural or stochastic**: structural in the prompt. The subagent
is never asked for actor identity, so no wording of the reply can be
relied on to supply it; which specific claim it invents is stochastic,
the omission is not.
- **Change type**: targeted fix — a missing requirement in an existing
prompt, plus a report bucket. Normal bar, met.
- **Passes both gates.**

Second finding this leg did **not** pass and is recorded in the gist
instead: the three-hop `tend-mention` chain on the same PR #77. That one
is already fixed by open PR #849, and I've added this window's evidence
there rather than opening a duplicate.

---------

Co-authored-by: tend-agent <270458913+tend-agent@users.noreply.github.com>
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