Skip to content

Say which run compare chose when more than one matched - #58

Merged
deepskandpal merged 2 commits into
deepskandpal:mainfrom
dchaudhari7177:25-compare-ambiguous-run-warning
Aug 30, 2026
Merged

Say which run compare chose when more than one matched#58
deepskandpal merged 2 commits into
deepskandpal:mainfrom
dchaudhari7177:25-compare-ambiguous-run-warning

Conversation

@dchaudhari7177

Copy link
Copy Markdown
Contributor

Closes #25

compare now names the run it chose when more than one matched. Exit code stays 0 — this is the disclosure side of the principle the issue states, not a refusal.

The message

say(
    f"langchef: compared {variant_run.run_id}, the newest of "
    f"{len(candidates)} runs for arm 'variant' in suite {name!r} "
    f"({others} other{'' if others == 1 else 's'} not compared). "
    "Pass --variant to choose."
)

On stderr, via the existing say(). It carries the three things a person needs: which run was used, how many matched, and what to do about it.

Why a disclosure and not a refusal

readout refuses (#13) because it is gate two. compare is the exploratory command, and refusing there would break the ordinary use where re-running an arm is exactly what you want to be doing. The comment in the code states the rule so the next person does not have to re-derive it:

Inside a gate ambiguity is a refusal; outside one it is a disclosure. Silence is neither.

for_experiment(), not latest() — and why that mattered

Per #12. The subtle part: latest() is already a thin wrapper over for_experiment() that returns found[0] and throws the count away — and the count is the entire disclosure. Keeping latest() and counting separately would have created exactly the second resolution path the issue warns about, where the next version of this bug gets written. So the call site resolves once and reads both the choice and the count off the same list.

Behaviour on the existing paths is unchanged: same run chosen (newest first, ids sort chronologically), same no variant run to compare error when there is nothing to compare or the newest is the baseline.

Tests — all four AC lines

test asserts
test_ambiguous_compare_names_the_run_it_chose with var-a and var-b present, var-b is chosen and stderr names it, the count, and --variant
test_ambiguous_compare_keeps_stdout_pure_json json.loads(result.out) parses the whole of stdout, and the warning text is not in it
test_unambiguous_compare_stays_silent one matching run produces no disclosure
test_named_variant_is_never_ambiguous passing --variant silences it

That second one is deliberately phrased as "parse all of stdout and assert the prose is absent" rather than "check it starts with {" — "we write to stderr" is the kind of claim that quietly stops being true, and the contract has no --format flag for this to become.

The fourth exists because naming the run is the remedy the message suggests, and a remedy that does not work is worse than no message at all.

Exit code is asserted Exit.OK in every one.

Verification

uv run ruff check .          # All checks passed!
uv run ruff format --check . # 82 files already formatted
uv run pytest tests/         # 186 passed, 1 failed

Collected tests go 183 → 187 (+4). The single failure is test_docs_in_sync.py::test_verify_script_is_executable, which fails identically on a clean main here — Git for Windows does not preserve the executable bit on scripts/verify.sh. Unrelated to this branch; green on Linux CI.

Held area:cli / area:workspace only; #32 is on area:judge in a separate branch, so no lane collision.

`readout` refuses at exit 2 when several runs match an arm and none was named
(deepskandpal#13). `compare` silently took the newest, which carries the same reshaping
risk: with a warm cache a re-run is free, so an arm accumulates runs quickly
and nobody counts them.

Refusing here would be wrong. `compare` sits outside gate two -- it is the
exploratory command, and re-running an arm is exactly what you should be doing
there. So this is a disclosure, not a refusal: exit code stays 0, and the
warning names the run that was used, how many matched, and how many were not
compared.

The general principle, which the comment states where the next person will
read it: inside a gate ambiguity is a refusal; outside one it is a disclosure.
Silence is neither.

Prose goes to stderr, so it costs an agent nothing -- agents read stdout, and
stdout stays one pure JSON document. A test asserts that by parsing the whole
of stdout and checking the warning's text is not in it, because "we write to
stderr" is the kind of claim that quietly stops being true.

Resolution goes through runs.for_experiment() rather than runs.latest(), per
deepskandpal#12. latest() is a thin wrapper that returns found[0] and discards the count,
and the count is the entire disclosure -- so calling it and then counting
separately would have been the second resolution path the issue warns about.

Four tests: the warning fires when two runs match, stdout stays pure JSON, an
unambiguous compare stays silent, and naming --variant silences it -- that last
one because naming the run is the remedy the message suggests, and a remedy
that does not work is worse than no message.
@deepskandpal

Copy link
Copy Markdown
Owner

Taking this one for #25.

Two PRs arrived for the same issue, this one and #56, and that duplicate is our process failure rather than anyone's carelessness. AGENTS.md asks a claimant to move the issue to lifecycle:doing, outside contributors cannot set labels, so #25 never showed as taken for three hours. An Action is going in to fix that.

On the code: treating for_experiment() as the single resolution path is right, and the note that latest() discards the count when the count is the whole disclosure is the sharpest line in either PR.

One thing to know before you wait on a review. CI has not run on any of your three PRs. Workflows on fork PRs from first-time contributors are held pending approval, so #57, #58 and #56 are all sitting without checks. That is on us to release, not something wrong with your branches.

@deepskandpal

Copy link
Copy Markdown
Owner

Reviewed and accepted for #25. All five acceptance criteria are met: for_experiment() reused rather than a second resolution path, disclosure on stderr, stdout asserted pure JSON, silent when only one run matches, exit code 0.

I ran the full verify.sh against this branch in a local worktree while CI approval was pending: 10 of 10 steps pass.

The note in the code about latest() discarding the count when the count is the whole disclosure is the sharpest line in any of the three PRs that landed on this issue.

One mechanical thing before merge: main now requires both verify (python 3.12) and verify (python 3.13). This branch predates the matrix so it carries only the 3.12 check and shows as blocked. Merge main and the second leg will run.

@deepskandpal
deepskandpal merged commit 3af40af into deepskandpal:main Aug 30, 2026
2 checks passed
deepskandpal added a commit that referenced this pull request Aug 30, 2026
#57 and #67 landed on origin while #28, #31 and #54 were being merged locally.
tests/test_flow.py conflicted: #28 added a per-criterion attribution test and
#58 added four ambiguity-disclosure tests, and git tangled them because both
open with identical fixture setup. Resolved as the union of both, reconstructed
from each side's full version rather than hand-patched, so no assertion was
silently dropped in the middle of a hunk.
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.

compare resolves the variant by latest — the hazard readout just lost

2 participants