PR #699 follow-ups: manifest.py's hand-assembled JSON, after the encoding fix nf-dem ← Beads

open priority 2 task unassigned

Twinned from https://github.com/cfsh/ninefold/issues/703 by tools/beads/import_github.py's reconcile pass.

Suggestions from ninefold-reviewer's review of #699. All three are residuals from the CONVERGED verdict at 18a3480 — advisory, none blocking, none reachable with any branch name this repo has ever had. Filed unclaimed.

Context: #699's C1 fix routed every value in manifest.json through json.dumps, because S4 introduced the first free-form string the file ever emits (run.tree, a filesystem path). These are the edges that fix left.

1. in_build() decodes what rows() no longer encodes that way

tools/playtest/manifest.py

rows() now emits json.dumps(branch). in_build() reads those same rows back with re.search(r'"branch": "([^"]*)"', row) — a raw-text regex, unchanged by the fix.

They agree for every real ref, since json.dumps("feat/x") is "feat/x". They diverge for the two characters json.dumps escapes that a git ref name may legally hold:

Either way branch in in_build fails inside excluded_rows, so a branch git says is in the build could appear in branches and excluded simultaneously — the self-contradictory document excluded_rows' own docstring rules out.

Fix: parse the row instead of matching it — json.loads(row)["branch"] — or compare against json.dumps(branch).

2. The envelope's four top-level fields still interpolate bare

tools/playtest/manifest.py, in manifest():

```python f' "built": "{built_stamp()}",', f' "branch": "{branch}",', f' "f5_sha": "{head_sha}",', f' "base_sha": "{base_sha}",', ```

Safe by exactly the accident #699's own commit message declared insufficient for rows() — *"safe by ACCIDENT, not design … so it is routed through too"*. built is generated, the two shas are hex, and branch is git branch --show-current, which is f5/integration in practice — but a " is legal in a ref name, and the reason the rows() pair was converted was consistency rather than a live exploit.

Consistency-only, and cheap: four json.dumps calls, byte-identical output.

3. Three tests are filed under the wrong class

tools/tests/test_manifest.py

ManifestEncodingTest was inserted mid-class, so ProvenanceReadTest's last three methods ended up as members of it:

No coverage is lost — all 31 methods still run, which is why the count checks out. But they sit under a class docstring about encoding that says nothing about them, and the next encoding test added at the end of the class lands after three unrelated ones. Move the new class below ProvenanceReadTest.

Dependencies

None.

Comments

No comments.

Add a comment