Twinned from https://github.com/cfsh/ninefold/issues/506 by tools/beads/import_github.py's reconcile pass.
Suggestions from ninefold-reviewer's review of #500. None blocked that PR — it landed
result=CONVERGED, coverage 7/7, with all four #411 items covered or deferred to #492.
These are the residuals.
Filed unclaimed and unlabelled, per the backlog convention.
python3, and neither caller can report it missingmanifest.sh and publish.sh became exec python3 … shims. That is the tier's established
pattern, but these two callers verify less than any earlier shim's caller did.
f5.sh:122 — fatal, and silent about why.
```sh bash tools/playtest/manifest.sh > "$SESSION/manifest.json" ```
No || … guard, under set -e (f5.sh:19) — unlike the publish call twelve lines earlier,
which has one:
```sh bash tools/playtest/publish.sh || echo "⚠ playtest publish FAILED …" >&2 ```
The old manifest.sh ran set -uo pipefail (no -e), guarded every git call with
2>/dev/null, and ended on printf '}\n' — it could not exit non-zero. manifest.py has no
top-level handler (publish.py has one), so any raise exits 1, including repo_root's
PreflightError (nflib/__init__.py:176). The > has already truncated the file, so:
manifest.json left in the session dirf5.sh aborts at line 122.playtest-session (line 135) never written — the sitting is never armed✓ build … synced line never printedA 0-byte manifest is also not the state .claude/commands/playtest.md now documents as safe.
It says a *missing* manifest is normal; empty-and-unparseable is a third state nothing handles.
The game's quit path — silent by construction.
PlaytestJournal.PublishSession checks FindBash() (:138) and then fires
OS.CreateProcess(bash, …) (:142) without reading the exit code. There is no equivalent
python3 check, and [playtest] publishing … in the background prints either way. This is the
signature #500's own commit message warns about for *deleting* publish.sh — "would not break
loudly; every same-sitting publish would quietly stop … until a /playtest came up empty" — now
reachable through a dependency instead of a missing file.
Suggested, both in piece 8 (f5.py) rather than as a patch to #500:
- a || echo "⚠ build manifest FAILED …" guard on the manifest call, so it degrades the way
publish already does instead of killing the run;
- a python3-presence check beside FindBash, logging the same way the no-bash path does;
- optionally, have f5.sh skip writing manifest.json at all rather than leaving a truncated
one, so the documented "missing is normal" path is the one that actually gets taken.
REPO env knob, in the form the tier already rejectedtools/playtest/manifest.py, main():
```python gh = Gh(os.environ.get("REPO", DEFAULT_REPO)) ```
The shell it replaced hardcoded -R cfsh/ninefold and read no env var, so this is new surface.
tools/reviewer/dispatch.py:1017-1021 documents the hazard and uses the other form:
```python
# or, not a dict default: ${REPO:-…} treats EMPTY as unset and
gh = Gh(os.environ.get("REPO") or "cfsh/ninefold")
```
with a dedicated test at tools/tests/test_dispatch.py:740. With REPO="" exported,
gh pr list -R "" fails, open_prs fail-opens to {}, and every pr field silently vanishes
from the file /playtest treats as ground truth for routing.
tools/pr/merge_pr.py:517 and tools/pr/poll.py:971 have the same form, so this is tier-wide
drift, not a lapse in #500. Worth one sweep converting all three to or, plus a shared helper
so a fourth site cannot reintroduce it.
Recorded, not worth their own issues:
- work = Git(cwd=tmp, runner=git._run) (publish.py) reaches through a private attribute.
The injection seam is real and the fix that enabled the push-failure test was the right call;
it just wants a public accessor (Git.runner, or a Git.at(path) that clones the runner)
so the pattern is sanctioned rather than tolerated.
- rows() builds JSON by string interpolation, so a branch name containing " produces
invalid JSON. git check-ref-format permits " in ref names. Faithfully ported from the
shell and therefore correctly out of scope for the byte-identical commit — but the hand-rolled
envelope is now permanent, so it wants either an escape on branch or a documented
assumption.
- open_prs keeps the LAST match for a duplicated headRefName; the shell's
awk '$1==b {print $2; exit}' kept the first. Reachable only with two open PRs from one head
branch to different bases.
- Dead call in a test. test_a_manifest_only_session_is_not_selected_for_shipping
(tools/tests/test_playtest.py) runs capture(publish.sessions_to_ship, …), discards the
result, then calls publish.sessions_to_ship again and asserts on that. The first call is
vestigial.
#500 says Closes #411, but #411's headline item (item 1, fast-forwarded branches) lands in
#492, which says only Refs #411. Merging #500 first auto-closes #411 with its titular item
still in an open PR. Merge order or linkage, either is fine — raised as gate G3 on the PR.
None.
No comments.