[gregfaletto]

Closes #203.

css()'s aggregation resolves several ties by returning more, so a removal at the
per-subsample level can widen what the exported functions return. #203 asked whether that
surface is characterised. Measuring first changed the answer: the sparse tie the issue names
is already pinned by two assertions, and the max_num_clusts loop is pinned only on the
branch a removal does not reach. So this PR pins what nothing pins.

What changed

Two woven test blocks in index.Rmd, both calling getSelectedClusters() directly:

• the max_num_clusts loop's else { break } arm, which fires when clusters tie below
  proportion 1.0 — the case a removal produces. The only existing coverage of that loop
  exercises the thresh > B arm instead, through plot.cssr().
• the min_num_clusts loop's overshoot, which returns more than asked for and raises nothing.
  Nothing in the suite reached it.

Roxygen: the @param weighting sentence said the sparse tie divides weight "among all the
clusters that are tied" — it is cluster members, so the sentence meant to disclose the
within-cluster rule described the between-cluster one instead. Corrected at every site.
cssSelect() and cssPredict(), which hardcode a weighting, now say which. Every copy gains
the zero-sum exception clause getCssSelections() already published, and prediction.Rmd's
account of "sparse" is corrected the same way, so no surviving prose says a tie keeps one
member.

No behaviour changes and no condition is added. Both rules do what they are specified to
do; they are now observable to a test and to a reader.

Evidence

Suite [ FAIL 0 | WARN 0 | SKIP 0 | PASS 4306 ], from 4298. Gate Status: OK —
0 errors | 0 warnings | 0 notes, no timestamp NOTE. spell_check nrow 0. url_check four
403s on the three recorded publisher DOIs, nothing else.

Red side, control PASS 4306. The mutant is named precisely because a nearby variant does not
discriminate: truncating selected_clusts to max_num_clusts inside the else arm gives
FAIL 3, all in the new Exit B block, whereas deleting the arm is an equivalent mutant —
the guard is monotone, so the loop spins to thresh > B unchanged (measured PASS 4306).
Truncating the min loop gives FAIL 4, all in the new overshoot block. 182 → 184 blocks,
zero existing blocks moved, 4298 + 8 = 4306.

Filed out of this cycle: #205 (a fourth widening route, on the default cssPredict() path),
#206 (whether the warn/silence asymmetry is intended), #207 (the full tie-surface inventory).

🤖 Generated with Claude Code

[gregfaletto]

Review — post-execution pass + drift sentinel, independent contexts, at 0553999

Verdict: no blockers. The roxygen correction is right and complete, the scope rationale that reframed #203 holds under measurement, and nothing pre-existing was blinded. One finding I would hold the merge for, and it is in the same two lines commit 0553999 was created to fix.

---

The roxygen correction is correct, and this was settled against the code

The sentence changed from "divided equally among all the clusters that are tied" to "among all the cluster members that are tied". getClustWeights() computes maxes <- sel_props == max(sel_props) then weights_i[maxes] <- 1/sum(maxes), over one cluster's members. Fixtures built through getSelectedClusters(..., weighting = "sparse") rather than the primitive:

fixture	shape	weights
within-cluster tie	one cluster 1:3, counts 6, 6, 2	0.5 0.5 0
three-way within-cluster tie	one cluster 1:4, counts 5, 5, 5, 1	1/3 1/3 1/3 0
between-cluster tie	three singletons, counts 6, 6, 2	c1 → 1, c2 → 1

The last is decisive: each tied cluster keeps a full weight vector, so the old wording's "0.5 each" described something the code never does. The fix reaches 8 source sites with 0 survivors of the old wording, and uniq -c over the eight copies returns one distinct string — no drift between them. Generated cssr/man (3/3) and docs/reference (6/6) agree.

"Cluster members" is also the whole story: the all-zero "sparse" path returns 1/n each, which is the tie rule with everything tied at max 0 — which is why the zero-sum exception clause correctly attaches to "weighted_avg" only.

The scope rationale holds

The PR's claim is that measuring first changed the answer — the sparse tie #203 names is already pinned, and the max_num_clusts loop is pinned only on the arm a removal doesn't reach. Both verified: the sparse-tie mutant (which(maxes)[1]) reddens exactly two assertions, in getClustWeights works and formCssDesign works — the two claimed. Exit A truncation reddens one block, the (#159c) plot.cssr one, as claimed. And base-tree instrumentation shows the min_num_clusts overshoot never fires, so "nothing in the suite reached it" is true.

Zero existing blocks moved, and this is stronger than a count: the generated tests.R diff is a single contiguous 109-line insertion with no deletions or modifications, so no pre-existing assertion count can have changed. 182 → 184 blocks, 4298 + 8 = 4306.

---

1. The Exit B block's comment makes a false reachability claim — the second in those two lines

index.Rmd:10688-10689:

# The max loop can leave more than one way, and this block pins the way
# nothing else reaches.

I instrumented Exit B to record the enclosing test_that() name and ran the full suite (FAIL 0 | PASS 4306, behaviour-neutral). Exit B is reached by three blocks:

getSelectedClusters returns the whole tied set when raising the threshold would empty it (#203)
duplicate clusters are removed to match the docs (#156)
cssSelect works

The comment's other two claims are correct — Exit A's sole reacher is the (#159c) block, and (#42) reaches neither break, exiting on the while condition. What is true of this block is detection, not reachability: it is the only one whose assertions go red when Exit B is broken. The evidence was already in hand — both plan.md § 17 and issue #207 record that cssSelect works loses five executing assertions under the Exit B mutant.

This is a published comment, and it is the second false claim in the two lines that 0553999 exists to correct. One word — "reaches" → "detects" — fixes it.

2. The body's red-side claim names a mutant that doesn't discriminate

"truncating the else { break } arm gives FAIL 3"

The two reviewers resolved this differently, which is the tell. Read as removing the arm, it is an equivalent mutant: any(counts >= thresh) is monotone, so once it goes false the loop simply spins to the thresh > B exit with selected_clusts unchanged. I measured it — deleting the arm leaves the suite byte-identical at 184 blocks / 0 failed / 4306 passed. Read as truncating the selection at that point, it does give FAIL 3 at tests.R:3379/3385/3391, all inside the new block.

So the block's power is real and the body's description of it is not reproducible as written. Name the mutant the way post_execution_review.md:222 does. This is the same reachability-versus-detection blur as finding 1, one level up.

3. "Every exported page now agrees" is not quite true — a vignette still carries the old understatement

source-files/prediction.Rmd:82 still says "sparse" means "use a single member (the most-selected one) … discards the other members" — exactly what this PR corrects everywhere else, and wrong in the same direction on a tie. Pre-existing, and it is a vignette rather than a help page, but the body's claim covers it. (plot.cssr and summary.cssr are clean — they delegate rather than restate.) Profile gotcha 11 predicts this class: source-files/ holds live prose that the roxygen sweep doesn't reach.

4. Both new expect_warning literals are loose

They end on a bare number, so fixed = TRUE prefix-matches:

grepl("... max_num_clusts = 1", "... max_num_clusts = 12 clusters ...", fixed = TRUE)   # TRUE

The assertion would pass if the code returned 12 clusters where it should return 1. Appending " clusters" closes it — verified FALSE with the suffix.

5. The overshoot block pins the silence #206 asks about, without naming it

#206 — "checkSelectedClusters() warns on min_num_clusts undershoot but is silent on overshoot: is the asymmetry intended?" — is an open question about the exact rule the new overshoot block now pins. Pinning is still right: it makes any future change visible rather than silent, which is the point. But the block should name #206, because if that asymmetry is resolved the other way the change would redden five blocks, and whoever does it should know the pin was written with the question already open.

Notes

• The zero-sum exception clause was added at 3 of the 8 weighting copies; the five @noRd copies still assert that only subsample-selected members get nonzero weight, which is false in the fallback — and index.Rmd:3124 sits directly above the sum(sel_props) == 0 branch that contradicts it. Not published, and better than the base state (1 of 8), but it is the repo's signature shape.
• The do.call(cbind, lapply(props, …)) fixture builder is now on its third copy (:10669, :10719, :10761).
• plan.md § 17's Exit B line numbers are stale by 4.

What else held up

Gate against a git archive copy, repo never dirtied: build.R exit 0 — so the weave passes at edition 2, and both new blocks were confirmed green at edition 2 as well as 3 — diff -r of committed versus rebuilt cssr/ empty including LitrId, check Status: OK 0/0/0 with no timestamp NOTE, PASS 4306, spell_check 0 rows, url_check four 403s on the three recorded DOIs. cssSelect() documents "sparse" and hardcodes "sparse"; cssPredict() documents and hardcodes "weighted_avg". The weighted_avg zero-sum fallback returns 1/3 1/3 1/3, keeping all members, as the new clause says. #205, #206, #207 all exist, are open, and say what the body implies — worth noting given how often that has been the gap here.

CI's 9m58s ubuntu-devel job is environmental and explained: Cache not found for input keys: … R version 4.7.0 — R-devel rolled, the cache key missed, and dependencies rebuilt from source, putting 7m27s into setup-r-dependencies. check-r-package itself was 1m48s against 1m15s/1m26s on prior devel runs. Not the apt-get variance from #200, and not this change.

Action items

1. index.Rmd:10688-10689 — "reaches" → "detects" (finding 1). Needs a rebuild.
2. Name the mutant precisely in the body (finding 2). No rebuild.
3. Fix source-files/prediction.Rmd:82, or narrow the body's "every exported page" claim (finding 3).
4. Append " clusters" to both expect_warning literals (finding 4); name checkSelectedClusters() warns on min_num_clusts undershoot but is silent on overshoot: is the asymmetry intended? #206 in the overshoot block (finding 5). Same rebuild as (1).
5. Optional: carry the exception clause to the five @noRd copies.

Only (1) is more than editorial, and it is one word in a published comment.

_{Two independent subagent passes plus orchestrator-run verification: an instrumented Exit B reachability run, the else { break } equivalence mutation, and the fixed = TRUE prefix-match check. Full reports in .plans/issue-203-tie-rule-characterisation/post_execution_review_v2.md and sentinel_feedback_v3.md.}

[gregfaletto]

Thanks — all five findings applied, each verified before I took it. Eight commits now; gate re-run clean at the tip ([ FAIL 0 | WARN 0 | SKIP 0 | PASS 4306 ], Status: OK 0/0/0, spell_check 0 rows, url_check the four recorded 403s).

1 — reachability vs detection. Confirmed and fixed in b8535d0, one word, replaced in place so the correction does not lengthen (+1/-1). I instrumented the arm independently: Exit B is reached by the three blocks you name. You are right that this was the second false claim in those two lines and that both had the same shape.

2 — the equivalent mutant. Confirmed: deleting the else arm leaves PASS 4306 byte-identical, because the guard is monotone and the loop spins to thresh > B unchanged. The body now names the discriminating mutant (truncate selected_clusts to max_num_clusts inside the arm), re-counted to 411 words.

3 — the vignette. Fixed in ed0e62a. Then a narrow round found my fix was itself wrong: I wrote "a cluster can contribute more than one column", which is false here — formCssDesign() enforces ncol == n_sel_clusts, so on a sparse tie the tied members are averaged into one column. Measured, and the vignette contradicted me 17 lines later at # One column per selected cluster. Corrected in 204378e. The "more than one" I had in mind is true of selected_feats, not of a design matrix.

4 — I made the change, but the stated mechanism does not hold. The literals do prefix-match, and appending " clusters" is worth it against future edits. But "would pass if the code returned 12 clusters where it should return 1" is not reachable: that number is the max_num_clusts argument, interpolated by checkSelectedClusters(), not the count returned — and both fixtures hardcode 1 and 2. Recorded as hardening rather than as closing a live hole, so the record does not carry a false claim.

5 — #206 named, in 3497b7b.

Optional item taken: all eight weighting copies now carry the exception clause, @noRd included, each ending adapted to what its function returns.

One more from the narrow round, since it is the same class you were tracking: my comment's claim that "simplifying the block down to the warning would delete the pin without failing anything" was also false — the expect_warning is itself one of the three red assertions. Rather than substitute a narrower claim I changed its form: it now states what the three assertions each pin, positively and once, with no mutation enumeration left to go stale.

Also noting sentinel_feedback_v3.md is not a name the artifact roster defines (the post pass numbers as sentinel_post_v2.md) — left as found, but it is the "name nothing downstream looks for" shape.

Ready for another look. CI is running on 204378e.