1204: Error when summarise_scores() by contains metric columns

[nikosbosse]

Description

This PR closes #1204.

summarise_scores() accepted metric/score columns in by (e.g. summarise_scores(scores, by = c("model", "wis"))) and silently returned a broken result: an unsummarised table (same row count as the input, since grouping by near-unique numeric score values aggregates nothing) with a duplicated column name (wis appearing both as a grouping column and as a "summarised" score column), which breaks downstream data.table operations.

The fix adds a check after the metrics attribute is retrieved: if by contains any metric column, summarise_scores() now errors via cli_abort(), naming the offending columns. Erroring (rather than warn-and-drop) is consistent with the precedent set by #1179, which introduced an error for the sibling empty-metrics duplicate-column case in the same function. The @param by documentation was updated accordingly, and a NEWS entry added.

New tests failed before the fix, as expected:

FAILURE: 'test-summarise_scores.R:46:3' -----------
Expected `summarise_scores(scores_quantile, by = c("model", "wis"))` to throw a error.

FAILURE: 'test-summarise_scores.R:52:3' -----------
Expected `summarise_scores(scores_quantile, by = c("model", "wis", "bias"))` to throw a error.

[ FAIL 2 | WARN 0 | SKIP 0 | PASS 17 ]

After the fix, the full test suite passes locally (0 failed, 909 passed).

Dev note: bug identified by an LLM audit (#1189); fix and tests implemented with LLM support, directed by @nikosbosse.

Checklist

• My PR is based on a package issue and I have explicitly linked it.
• I have included the target issue or issues in the PR title as follows: issue-number: PR title
• I have tested my changes locally.
• I have added or updated unit tests where necessary.
• I have updated the documentation if required.
• I have built the package locally and run rebuilt docs using roxygen2. (The man/summarise_scores.Rd update matches the roxygen2 output; only this file's documentation changed.)
• My code follows the established coding standards and I have run lintr on the changed files to check for style issues introduced by my changes.
• I have added a news item linked to this PR.
• I have reviewed CI checks for this PR and addressed them as far as I am able.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.16%. Comparing base (06ed56d) to head (bbb6920).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1213   +/-   ##
=======================================
  Coverage   98.15%   98.16%           
=======================================
  Files          41       41           
  Lines        2225     2234    +9     
=======================================
+ Hits         2184     2193    +9     
  Misses         41       41           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[nikosbosse]

Automated review (Claude Fable, directed by @nikosbosse):

Verdict: approve. All checks passed.

• Fail-before/pass-after verified independently: with R/ reverted to origin/main (tests kept), the two new tests fail exactly as claimed (no error thrown for by = c("model", "wis")); with the fix restored they pass.
• Full test suite green: 0 failed, 909 passed (matches the PR description).
• Edge cases probed beyond the PR's tests: by = a single metric column, by = all metric columns (message correctly names every offending column), the summarize_scores() alias, a renamed metric column with a correctly updated metrics attribute (all error as intended), and chained legitimate calls with fun = signif, digits = 2 (still work).
• Brief compliance: errors via cli_abort() rather than warn-and-drop, consistent with the summarize_scores() produces duplicate column names when input has no score columns #1179 precedent, as decided. Check placed after get_metrics.scores(scores, error = TRUE) and before the empty-metrics check, matching the plan.
• Scope/docs/NEWS: diff touches only R/summarise_scores.R, its .Rd, the test file, and NEWS.md. NEWS entry accurate; @param by roxygen updated and .Rd matches; PR title follows the NNNN: title convention.
• Tests are meaningful: the second assertion pins the exact offending-column listing ("wis" and "bias"), so a warn-and-drop or vague-error implementation would fail it.

One out-of-scope observation (no action needed): if a user renames a metric column without updating the metrics attribute (against the documented contract), grouping by the renamed column still silently returns an unsummarised table — pre-existing behaviour, no duplicate column names, not part of #1204.

[nikosbosse]

It's a behaviour change, but I think a good one