PR #500 follow-ups: playtest shims' python3 dependency, and a REPO env knob nf-xppv ← Beads

open priority 2 task unassigned

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.

1. Both playtest shims now need python3, and neither caller can report it missing

manifest.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:

A 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.

2. REPO env knob, in the form the tier already rejected

tools/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.

3. Sub-floor notes

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.

Not a residual — flagged on the PR instead

#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.

Dependencies

None.

Comments

No comments.

Add a comment