[gregfaletto]

clusterLassoCore() called unique() on glmnet::predict.glmnet(fit, type = "nonzero"), and
getClusterSelsFromGlmnet() then called lengths() on the result. Both mean something different
for a data.frame than for the list the code assumed, and nothing asserted which had arrived.

Not a live wrong answer today, on either glmnet version — but only by accident.
clusterLassoCore() supplies no lambda, so the auto path starts at lambda_max where
df == 0; nonzeroCoef() yields NULL there, apply() cannot simplify, and 4.x's data.frame
coercion is skipped. Nothing in cssr asks for that.

Where it does bite is narrower and worse than the issue supposed. nonzeroCoef() has two
data.frame layouts with opposite conventions: multi-column is one column per penalty and
degenerates to the right answer; single-column — when every penalty selects exactly one feature,
so apply() collapses to a vector — is one row per penalty. On the latter, penalties selecting
{1},{1},{2},{2},{3} are read as a size-3 model {1,2,3} glmnet never selected, with the genuine
size-1 model dropped. A bare as.list() produces a third wrong answer. The fix disambiguates with
ncol(fit$beta) and guards the consumer.

cssLasso()'s sort(unique(unlist(pred))) from #188 is deliberately untouched — it flattens,
which is right there and wrong here.

Evidence

Gate: 0 errors ✔ | 0 warnings ✔ | 0 notes ✔ (the recorded timestamp NOTE is intermittent and
did not fire on this run); spell_check nrow = 0; url_check exactly the four recorded
publisher 403s. Suite [ FAIL 0 | WARN 0 | SKIP 0 | PASS 4194 ] against the 4153 baseline.

Red side, base tree with the final tests.R: [ FAIL 6 | WARN 0 | SKIP 0 | PASS 4188 ] — five
of the eight new blocks, six named assertions. Behaviour unchanged: identical() → TRUE through
both exported entry points. Every new block kills at least one mutant, and the square-fixture
block is the only thing in the suite catching a symmetric reorder of the branch predicates —
verified by mutation, every pre-existing block staying green under it.

Notes for review

stopifnot(identical(length(nonzero), n_pen)) is a new bound on the default path, verified
locally only on glmnet 4.1.9. CI installs 5.0 and is its cross-version test — a red job naming
that assertion is information, not flake.

No NEWS.md here and no user-visible change, so no bullet and no version bump; inst/CITATION
carries no version string. Two of the six new stopifnot() calls are deliberately unpinned —
post-conditions on a total normalisation, unreachable once the reshaping has run.

[gregfaletto]

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

Verdict: LGTM, no blockers. The correctness core — the layout disambiguation — is not merely argued but exhaustively verified, independently, on both glmnet versions. Every finding below is about what the change leaves unpinned or about prose. The sharpest one is that the fix's correctness rests on the order of two predicates, and nothing in the suite would notice if they were swapped.

---

The disambiguation is correct, and this was settled by enumeration rather than argument

Two independent passes enumerated nonzeroCoef()'s reachable output shapes and ran the PR's normalisation against ground truth:

	patterns	data.frame returns	both predicates true	neither true	wrong answers
pass 1	1,157,324	26,300	1,851	0	0
pass 2	28,000 classifications across p 2–6 × n_pen 1–7 × both glmnet versions	2,019	—	0	0

The ambiguity resolves structurally, which is the part worth recording. In the bystep = TRUE, nr >= 2 branch:

which = apply(beta, 2, nzel, which)
if (!is.list(which)) which = data.frame(which)

apply() simplifies to a matrix only when every penalty selects the same number of features ≥ 2 (→ n_pen columns), and to a vector only when every penalty selects exactly one (→ exactly one column, n_pen rows). Ragged or NULL-containing results stay lists. So a row-major frame always has ncol == 1, and ncol == n_pen can only misfire at n_pen == 1 — where the frame is 1×1 and both readings coincide. The nr == 1 branch is genuinely unreachable: if (ncol(X_glmnet) < 2) returns before the fit, so nrow(beta) >= 2.

stopifnot(identical(length(nonzero), n_pen)) — flagged in the body as a new bound verified only on 4.1.9 — is a source-supported invariant, not an observation: apply() yields one result per column of fit$beta in both versions, the empty branch's length(dn) is safe because getcoef.R sets Dimnames on both the sparse and zeromat() paths, and dfmax/pmax/early stopping cannot desync the two because both sides read the same truncated beta. Confirmed across 9 fit configurations × 2 versions and 500 random fits. CI green 5/5 on glmnet 5.0 corroborates it.

Behaviour is unchanged on the installed glmnet across 1,152 configurations, 0 identical() mismatches — structurally expected, since 500/500 real fits return a list and the reshaping is dead code locally. The red side reproduces exactly: [ FAIL 5 | WARN 0 | SKIP 0 | PASS 4161 ], and all six new blocks kill at least one mutant by per-block attribution rather than suite totals.

---

1. Robustness — the predicate order is load-bearing, unstated, and untested

On a square input (k == n_pen, k ≥ 2) both branch predicates are true, and the two readings are completely different:

ncol == n_pen : TRUE
nrow == n_pen : TRUE          <- both true
column-major (as shipped): 1,2,3 | 1,2,4 | 1,3,4
row-major (if reordered) : 1,1,1 | 2,2,3 | 3,4,4

The shipped order is the correct one — multi-column is column-major, so testing ncol first is right. But the symmetric refactor (test nrow == n_pen first, move the readings to match) is wrong only on square input, and it scores 0 failed | 0 error | 4166 passed — identical to the control. Neither fixture is square: the single-column block is 66×1 and the multi-column block is 2×61. The existing mutant that transposes the branches is killed by an error; a predicate reorder sails through.

This matters more than a typical unpinned-guard note because the failure mode it admits is exactly #190's — confidently wrong model sizes behind a guard that reads as if it had settled the question.

The fix is cheap: a square fixture is directly constructible, since nlambda is honoured exactly at small values — measured, nlambda = 2 → n_pen = 2 and nlambda = 3 → n_pen = 3. Size the multi-column block's fixture to nrow = ncol(object$beta), or at minimum state in the comment that the order is load-bearing.

2. Robustness — the "loud direction" claim is true for the block it annotates and false for its unmentioned sibling

index.Rmd:16575 keeps the raw unique(predict.glmnet(...)) idiom deliberately, and argues that on a glmnet returning a data.frame those fixtures "would stop at the consumer's new guard rather than silently misreading it." That holds for getClusterSelsFromGlmnet works, whose five fixtures do reach the guard.

It does not hold for getSelectedSets works at index.Rmd:16252, which the comment doesn't mention. That block has six raw-idiom fixtures (:16267, :16306, :16356, :16416, :16479, :16525) and indexes lasso_sets[[5]] directly at :16269 and :16308 — it never calls getClusterSelsFromGlmnet(), so the guard cannot fire. On a data.frame, [[5]] is column 5: a well-formed integer vector, and the assertions pass silently. That is the quiet direction, in the one place the PR claims there isn't one.

Not a live defect — no glmnet in play returns a data.frame on this path — but it's the sibling-duplication pattern one level down, in the fixtures. One expect_true(is.list(lasso_sets)) after each fixture in both blocks closes it and makes the comment true as written.

3. Clarity — "the only block that catches that" is false

The list block's comment justifies keeping a block that is green pre-fix by claiming it uniquely catches the unconditional-reshape mutant. Measured: that mutant errors seven blocks, five of them pre-existing (protolasso works, clusterRepLasso works, #157, #158a, #158b). The rest of that comment is exact, down to the error string 'list' object cannot be coerced to type 'integer'. The block is still worth keeping; the justification just overstates its uniqueness.

4. Clarity — a dangling positional reference, in published prose

index.Rmd:16956 says "the clusterRepLasso() design block B uses." There is no block lettering anywhere in index.Rmd — grep -n "block A\|block B\|block C" returns only this line. It renders to docs/create/tests-for-competitor-methods.html, so it ships to the public site. Both figures it cites (66 and 61 penalties) are correct. git log -S attributes the line to d5f075d — the commit whose stated purpose was correcting inaccurate claims. Suggest "the clusterRepLasso() design the multi-column block uses."

5. Clarity — the multi-column trigger is stated too narrowly

index.Rmd:6112 says the layout arises when "every penalty selected the same k >= 2 features." The real condition is the same number of features — verified: columns {1,2} and {2,3}, different sets with uniform count, also produce the multi-column frame. As written it understates how easily that layout arrives.

6. Note — one of the three "deliberately unpinned" guards is different in kind

The PR is candid that deleting is.list(), !is.data.frame() (boundary) or the inner nrow() check leaves every block green, and calls this defence-in-depth rather than a coverage gap. Verified, and for two of the three the framing is right: they are post-conditions on a total normalisation, unpinnable except by mocking a container glmnet cannot emit. (The consumer's pair, by contrast, is pinned one-to-one — three separate mutants map to exactly one block each.)

The inner nrow(nonzero_mat) == n_pen is different: it is the else branch's own disambiguation predicate, its unreachability is empirical rather than structural, and it is trivially pinnable — data.frame(a = 1:2, b = 3:4) against n_pen = 66 trips it with one expect_error. Worth splitting the narrative so three guards stop sharing one justification.

---

What held up

• Every falsifiable claim in the body checks out — twelve of them, including the two I most expected to bend. The bare-as.list() "third wrong answer" is real (length 1, read as a size-5 model {1,1,2,2,3}, distinct from both the correct and the pre-fix answer), and the worked example is exact ({1},{1},{2},{2},{3} → a fabricated size-3 {1,2,3} with the genuine size-1 model dropped).
• The two call sites' comments are accurate about each other. cssLasso() flattens because its scalar s gives exactly one slot; this path preserves one slot per penalty. The divergence reason is stated at both ends and correct — genuinely complementary, not two folk theories. The prose is duplicated but the code isn't; a shared normaliser is now feasible but isn't this PR's job.
• The in-mock expect_gte(ncol(object$beta), 3L) fires and is load-bearing, at both editions — probed with a failing threshold. It exists to stop a shortened path from making the red-green fixture vacuously green, which is the right instinct, and the suite arithmetic reconciles exactly: 3+3+4+1+1+1 = 13 = 4166 − 4153.
• Gate clean: build.R exit 0 (so the edition-2 weave passes), rebuilt cssr/ byte-identical to the committed tree including LitrId, check Status: OK 0/0/0, spell_check nrow 0, url_check exactly the four recorded 403s, no new TODO, NAMESPACE/man/ empty diffs (correct — both functions are @noRd), CI green 5/5.
• Nothing was blinded. On 4.1.9 the is.data.frame branch is never entered, so unique() sees byte-identical input; all six new deparsed stopifnot messages were checked against every other stopifnot in the file for collisions — none.

One phrasing note: the environmental timestamp NOTE did fire during review and did not for the author, so the body's parenthetical is better as "intermittent" than as "did not fire."

Action items

1. Pin the square case, or state that the predicate order is load-bearing (finding 1) — nlambda = 2 gives a 2×2 fixture directly. Needs a rebuild.
2. Add expect_true(is.list(lasso_sets)) to the raw-idiom fixtures in both blocks, including getSelectedSets works, and widen the :16575 comment to cover it (finding 2). Same rebuild.
3. Correct "the only block that catches that" (3), the "block B" reference (4), and "the same k >= 2 features" → "the same number" (5). Same rebuild.
4. Optionally pin the inner nrow() check and split the three-guard narrative (6); soften the timestamp-NOTE phrasing in the body.

None of these blocks merge. (1) and (2) are the two I'd want in before it lands, since both concern coverage rather than wording.

_{Two independent subagent passes plus orchestrator-run verification of the shape rules, the predicate-order case and the sibling fixtures; full reports in .plans/issue-190-glmnet-nonzero-shape/post_execution_review_v2.md and sentinel_feedback_v3.md.}

[gregfaletto]

Confirmation — blocker cleared at 63b83c1

Verified independently rather than from the commit messages. Ready to merge; no open findings.

The blocker was in the fix I proposed

My review's action item 2 recommended expect_true(is.list(lasso_sets)) beside each raw-idiom fixture, to close the silent-pass hole in getSelectedSets works. That assertion cannot do the job:

is.list(data.frame)       : TRUE    <- the proposed assertion passes
is.data.frame(data.frame) : TRUE

A data.frame is a list — which is precisely why getClusterSelsFromGlmnet() carries two stopifnot() guards rather than one, a fact my own review quoted four lines from where it proposed the one-sided check. So bfed4c6 adopted it verbatim and shipped a canary blind to the container it was watching for, under a published comment saying the hole was closed. The recommendation was mine and it was wrong; 63b83c1 catching it is the round working as intended.

63b83c1 adds expect_false(is.data.frame(lasso_sets)) beside all eleven, so the pair now mirrors the consumer's guard pair exactly. Confirmed: 11 is.list assertions, 11 is.data.frame assertions, correctly paired at every raw-idiom fixture, in both getClusterSelsFromGlmnet works and getSelectedSets works.

The predicate-order gap is closed

Re-ran the symmetric reorder — swap the test and the readings together, wrong only on square input — against the new tree:

tree	blocks	failed	error	passed
63b83c1	174	0	0	4194
symmetric reorder	174	0	2	4189

Both errors are the new blocks: clusterLassoCore reads a square data.frame column-major (#190) and clusterLassoCore rejects a data.frame of neither orientation (#190). Before these, that mutant was invisible — 0 failures, identical to the control.

The second block also pins the inner nrow() check, reversing the earlier (c) disposition. That was the right reversal: unlike the two boundary guards, its unreachability was only empirical, and it cost one assertion to pin.

Everything else

Gate at 63b83c1: Status: OK, 0 errors / 0 warnings / 0 notes. CI green 5/5. Suite 166 → 174 blocks, 4166 → 4194 passing. All four prose corrections landed — no remaining hits for "same k >= 2 features", "block B", or "the only block that catches".

Worth noting that 63b83c1 also caught three claims bfed4c6's own edits had made stale — a "the last three" pointer to a set that had since grown to eight blocks, a "same k >= 2 features" corrected at only one of its two sites, and an inverted axis in the new square block's comment. Correcting a claim and leaving its twin stale is the failure mode this PR has been chasing from the start; catching it inside the same branch is the right place to catch it.

_{Round-5 confirmation: orchestrator-run reorder mutation, canary audit, and gate at 63b83c1.}