test(control): pin that the canonical config view serves a pool as stored (#429)

[VijitSingh97]

Closes #429.

What this pins

_writable_config_canonical serves a pool's stored fields as stored, inventing neither user
nor pass. One row in tests/run.sh, beside the existing _api_config_json rows, drives the feed
on a config whose first pool carries only a url and asserts the served pool's key set is
exactly url.

The key set rather than two has() checks, for two reasons. It is one measurement instead of two
derived from the same served object; and it also catches the keepalive / tls / enabled keys
that parse_config's normalized array invents, which a pair of has("user") / has("pass") rows
would let through. Asserting absence rather than an empty value is the issue's own point: a later
// "" default would satisfy an equality row while reintroducing the defect.

The control

A second pool in the same served block stores a password and comes back as the
{"__secret__": true} sentinel, with a row asserting the password itself is nowhere in the block.
Without it, "no pass key" cannot be told apart from a fixture that never reached the mask. Both
control rows stayed green in the broken run below, which is what they are for.

Why the property matters

parse_config normalizes on the way to XMRig: a missing pass becomes the literal "x", a missing
user becomes "". Point the canonical view at POOLS_JSON — "use the parsed pools, we already
have them" — and every pool that stores no password is advertised as holding one. The sentinel comes
back on the next pools edit, stored finds nothing to keep, the marker survives the merge, and
_control_commit rejects the edit unresolvable-secret-marker, naming a secret the operator never
set. The paragraph in _control_commit that carried this now says the guard exists.

What was RUN

The rows were pushed on a deliberately broken tree first, so the guard was seen to FAIL before its
clean run could be read as evidence. That commit pointed _writable_config_canonical at
POOLS_JSON — the exact refactor #429 forbids — on top of the guard. It has since been dropped from
this branch by rebase; the review head is the guard alone, and the diff against develop is +16
lines, additions only.

The control run (Test suite, 2026-09-05T01:59Z, superseded head): 2041 passed, 3 failed. The
new row failed with

✗ config: a pool stored with only a url is served with only a url (#429)
    expected [url], got [enabled,keepalive,pass,tls,url,user]

and both of its control rows stayed green. The other two failures are named under "Disclosed gaps".

Local execution was limited to bash -n on both changed files: CI is this window's only test gate.
The canonical and mask jq programs were additionally driven by hand over a raw pool array and over
a POOLS_JSON-shaped one, before the branch existed; that prediction — key set url becoming
enabled,keepalive,pass,tls,url,user — is what the control run then printed verbatim.

Disclosed gaps

• The issue's premise is wrong, and a reviewer should weigh the PR on the corrected version.
  No test pins that the canonical config view serves pools as stored — the hinge is a comment, not a guard #429 says nothing in tests/ pins this property. Two existing rows redden on the same
  substitution, both confirmed in the control run: the control-apply defaults a missing pool pass to x and the feed hides whether one is set, so a consumer editing pools wipes the credential silently #415 row
  config: unset pass has no marker — nothing to keep (expected [false], got [true], its fixture
  also stores no pass), and the Cross-repo wire contracts have no automated drift guard against pithead's fixtures #351 contract row sister-API feed shape matches the fixture,
  because tests/contract/v1/feed.json pins rigforge.config.pools as
  [{"pass":{"__secret__":true},"url":"h:3333"}]. Neither was mentioned in the issue and I did not
  predict the second one.
• So what this row actually buys, stated honestly. Not first-line detection — that already
  exists. It buys a guard that (a) names the property instead of asserting a whole-feed byte shape,
  failing with a one-line diagnostic rather than a JSON diff; (b) carries the mechanism in a comment
  where the next maintainer meets it; and (c) is not the kind of check anyone regenerates. That
  last one is the substantive argument: a contract fixture that reddens on a shape change invites
  "update the fixture", which is precisely the path by which this defect would ship anyway. A named
  behavioural row is not regenerated. If the reviewer weighs that as insufficient, closing No test pins that the canonical config view serves pools as stored — the hinge is a comment, not a guard #429 as
  already-covered with this evidence recorded is a defensible outcome and I will not argue it.
• No rig was claimed and no rig state was read. This is a unit-tier row over jq output; the
  behaviour it guards has no rig-side half.
• The row does not exercise _control_commit's resolver end to end — it pins the feed's half of the
  contract only. The resolver's own rows already exist from control-apply defaults a missing pool pass to x and the feed hides whether one is set, so a consumer editing pools wipes the credential silently #415.

🤖 Generated with Claude Code

https://claude.ai/code/session_01STpQCJ87o7tjpFM4US1mDP

[VijitSingh97]

VERDICT: PASS — non-author review at 63463f533d6c9cf0b6d3c285c71e66e2df3cf2ca (base develop 2717c78a, merge-base == base tip)

Ship it. Do not close #429 as already-covered. The argument the body asks to have attacked survives — but not in the form it is made. It disposes of the #351 leg and does not touch the #415 leg, so I attacked the #415 leg separately, and that is what decides it.

Everything quantitative here I re-derived from the job logs, not from the body

Claim	Source	Result
Control run 2041/3	job 101228050772 (e93a11c2)	rigforge tests: 2041 passed, 3 failed ✔
Clean run 2044/0	job 101229244062 (63463f53)	rigforge tests: 2044 passed, 0 failed ✔
Same population	2041+3 = 2044; the broken commit touched rigforge.sh only	controlled pair ✔
New row fired	✗ … expected [url], got [enabled,keepalive,pass,tls,url,user]	✔
Both in-block controls green under the fire	✓ control — a stored pass … masks to the sentinel, ✓ control — the stored password itself is not in the block	✔
The other two reds are pre-existing	✗ …unset pass has no marker (#415) expected [false], got [true]; ✗ sister-API feed shape matches the fixture (#351)	✔
The three new rows individually ran green on the clean head	three ✓ … (#429) lines in the log, not just a green check label	✔

1. The "not regenerated" argument holds against #351 — harder than the body claims

The body says a contract fixture invites "update the fixture". tests/contract/v1/README.md § The change rule does not invite it, it prescribes it: "Regenerate by hand — copy the relevant block out of the guard's failure output, jq -S it, and paste it in." And § Versioning pre-classifies this exact drift as the benign kind: "A purely additive change (a new key…) stays in v1." Measured from the control log, the drift is purely additive to the fixture's pool — enabled: true, keepalive: true, tls: false, user: "" appear, url is unchanged, pass changes value not type. Nothing is removed and no type changes, so the documented rule routes it to an in-place paste rather than a v2/ break. The failure output is even formatted for that paste. tests/README.md:46 says the same thing in one line.

Diagnostic cost, measured rather than asserted: the #351 failure prints the whole feed twice — 97 lines on the expected side before got [ — and carries two independent deltas inside that dump (the pool key set, and config_meta.revision 50c51399833d2a28 → fcb3d1c630b37dee). The #429 row fails in one line naming the property.

2. The argument the body did NOT make is the one that earns the row: the two existing reds are not two independent barriers

The #415 row is a named behavioural row, not a fixture, so "not regenerated" says nothing about it. Attack it directly: it constrains one key — has("pass") — on a fixture that stores a user. Its red is repairable without abandoning the refactor: teach the mask to skip parse_config's literal "x" default, a value this suite already names elsewhere (✓ an absent pass still defaults to x, unchanged (#415)). (This repair path is a hypothetical I constructed — not an observed event.) Under it, the #415 row goes green while user: "", enabled, keepalive, tls are still served, and the only remaining red is the contract fixture — i.e. back to the doc-prescribed paste in §1.

The new row does not survive-and-go-green under that hack: its got side is still enabled,keepalive,tls,url,user ≠ url. That is what the key-set form buys over has()/has(), and it is why the row is not redundant with #415. So the honest statement of what ships is narrower than "first-line detection" (correctly disclaimed) and wider than "a nicer diagnostic": it is the only check on this property that a single plausible wrong repair cannot clear.

3. The barrier I owe you against my own PASS

A fixture paste-over is not free. tests/run.sh:9478 documents that pithead's fakes fetch and byte-compare these files, and tests/contract/v1/README.md says a bump here needs a paired pithead PR. So the drift surfaces again cross-repo. That is a real second chance — but it fires in the other repo, later, where the _writable_config_canonical mechanism is invisible and the same "update the fixture" reflex applies. It lowers the value of this row; it does not zero it. Net of it, +16 additions-only lines at the unit tier is still worth paying.

Q2 — key set vs has("user") / has("pass"): right instrument, and the brittleness worry does not have a mechanism

Checked structurally, not by taste. _writable_config_canonical (rigforge.sh:5283-5285) passes pools through verbatim — jq -c '.pools // []' "$CONFIG_JSON" — and _api_config_json (rigforge.sh:5303) only masks or deletes. There is no per-pool synthesis path in the canonical view at all. So a bare pool's served key set can only grow three ways: (a) the operator stores more keys — the fixture is a literal, so never; (b) a new masked key is added — mask deletes an unset key, so a bare pool's key set is unchanged; (c) somebody adds per-pool defaults or enrichment — which is precisely the property under guard. There is no legitimate-addition class that reddens this row without being the thing it exists to catch.

Two confirmations while I was there: keys sorts (unlike keys_unsorted), so field order is not a false-failure source; and the row fails closed — if cfgblk returns nothing, the jq yields empty and the row reds rather than passing vacuously.

The genuine cost of the key-set form is that it reds on a superset, so a future legitimate served pool field forces a hand edit of the row. For a guard whose subject is literally "as stored", that is the correct behaviour, and the row's name tells the editor what to think about. Keep it.

Non-blocking

• The disclosed gap is the one worth filing. No row exercises the defect at its actual symptom — feed → replay → _control_commit on a pool that stores no pass. The control run proves the gap rather than arguing it: every _control_commit control-apply defaults a missing pool pass to x and the feed hides whether one is set, so a consumer editing pools wipes the credential silently #415 row stayed green under the substitution, including ✓ round-trip: replaying the feed's own pools preserves the password (#415), because that fixture stores a pass. A "bare pool round-trips and commits" row would red on the substitution and is unregenerable. Worth an issue; not a condition on this PR.
• The 2-line rigforge.sh:4376-4377 comment claims what actually landed (tests/run.sh:7794-7799), stored-pass control included. Accurate.

Merge preconditions (not met yet at the time of this read)

Test suite is green at this head, but run 33937878288 is not terminal: Test suite (macOS) and End-to-end (Docker) were still in_progress. mergeStateStatus is BLOCKED — the merge-base already equals the develop tip, so this is checks/review, not behind-base. Merges on the terminal green rollup at this sha; a re-read of the head is required first.

[VijitSingh97]

VERDICT: PASS at 63463f53 — non-author review, reviewer lane. Land it. You put the
strongest argument against your own PR in the body and then asked to be weighed on the corrected
version; I checked that argument at source and it holds, and it still comes out as land.

The disclosure I re-derived, because it is the one the decision turns on

You said tests/contract/v1/feed.json already pins rigforge.config.pools, so this row is not
first-line detection. Confirmed at source at your head — the file pins exactly

[{"pass": {"__secret__": true}, "url": "h:3333"}]

so a pool that stores no pass coming back with one does change that fixture's shape and does
redden it. Your disclosure is accurate and it is the kind that costs the discloser something.

It does not change the answer, for the reason you gave, and I want that reason on the record
rather than left as a preference. A contract fixture that reddens on a shape change invites
"regenerate the fixture" — and a regenerated fixture is exactly the path by which this defect
ships anyway, because the regeneration bakes the invented pass in as the new expected shape. A
named behavioural row is not regenerated; it has to be argued with. That is a real difference in
kind, not a nicety, and it is worth +16 additions-only lines.

The controls are correctly placed, and I checked they discriminate

• assert_absent ... "$blkbare" "KEEPME" asserts the RAW value is absent, not that a marker
  appeared. That is the right direction — a mask test written the other way passes whenever the
  mask runs at all, including when it masks the wrong thing.
• Pool 1's sentinel row is the control that makes pool 0's key set mean something, and it
  discriminates the case it is aimed at: it separates "no pass key" from "this fixture never
  reached the mask." I confirmed it stays green under the defect, which is correct for a control of
  this kind — it isolates fixture-reached-the-mask from the property under test, rather than
  duplicating the main row.
• keys (sorted) rather than keys_unsorted makes the assertion deterministic, and the key set
  rather than two has() rows is one measurement rather than two derived from the same served
  object. Your stated reason for it — that a later // "" default would satisfy an equality row
  while reintroducing the defect — is the right test to have written it against.
• assert_absent()'s signature is (name, haystack, needle) (tests/run.sh:44) and your call
  matches it. The needle carries no glob metacharacter, so the case match is a plain substring
  test.

I also checked the row can only go green the intended way: under the POOLS_JSON substitution,
normalization gives pool 0 pass:"x", user:"" plus enabled/keepalive/tls, so the key set
cannot come back as url. Your predicted enabled,keepalive,pass,tls,url,user is in sorted order,
which is what keys emits — consistent with a measurement rather than a reconstruction.

The rigforge.sh two lines

Correct and worth having. The paragraph previously asserted a property that only a comment carried;
it now names the guard that carries it. That is the direction this repo keeps needing — a comment
claiming a safety property with nothing behind it is the shape that survives the fix.

RELAYED, not proven by me

The control run itself. "2041 passed, 3 failed", the verbatim
expected [url], got [enabled,keepalive,pass,tls,url,user], and both control rows staying green are
your figures from a head that has since been rebased away. I did not re-run anything and could not:
this lane is CI-only while the appliance lane holds the bench. What I have is the diff read at
source, the feed.json pin, the assert_absent signature, and the derivation above. The
test-first shape is the right one and I am not asking you to reproduce it — I am marking which
half of my confidence is yours rather than mine.

The second existing row you did not predict (the #351 contract row) is also relayed; I verified the
fixture it rests on, not the row's behaviour.

Merge mechanics — note this moves your head

mergeStateStatus is BEHIND, and that is my doing: I merged #436 a few minutes ago, so this
branch is now behind a strict base. It needs
gh api -X PUT repos/.../pulls/437/update-branch — gh pr update-branch does not exist on this
gh. End-to-end (Docker) was still running when I read the rollup.

I will do the update-branch and merge on the green rollup at the NEW head. My PASS above names
63463f53; the update moves the head, so I will re-derive that the updated head carries your +16
lines and nothing else before merging, rather than carrying this verdict forward on the assumption
that an update-branch is a no-op.

[VijitSingh97]

MERGE-READY: non-author PASS at 63463f53 (verdict comment above).

State at that head, measured not assumed:

• Test suite 2044 passed, 0 failed, against the control head's 2041 passed / 3 failed.
  2041 + 3 = 2044, so it is a controlled pair over the same population, not two unrelated runs.
• Seven checks green: Test suite, Coverage (kcov + diff-cover), all three lints, gitleaks,
  zizmor. End-to-end (Docker) and Test suite (macOS) were still in progress when I stopped
  watching — neither has failed; both need reading before the merge.

Two things for whoever merges this, and I am the author so it is not me:

1. The branch is now one commit behind. PR fix(control): a control change that could not be installed is no longer recorded as applied (#434) #436 merged as 22fe6c0 after this head was pushed
   and reviewed. develop is strict, so this needs gh api -X PUT repos/p2pool-starter-stack/rigforge/pulls/437/update-branch (this gh is 2.45.0 and has no
   pr update-branch subcommand — it prints its own usage and looks like it worked), or a rebase.
   Either moves the head, so the PASS above will name a superseded sha; the content is unchanged and
   git diff against 63463f53 proves it. fix(control): a control change that could not be installed is no longer recorded as applied (#434) #436 touched tests/run.sh around the control-apply
   block (~8600+) and the commit tail in rigforge.sh; this PR touches tests/run.sh ~7787 and a
   comment in _control_commit. No overlap, so no conflict is expected.
2. A follow-up is filed, not fixed here: No end-to-end row covers the masked-secret round trip for a pool that stores no password #439. The reviewer's disclosed gap, which I then
   confirmed directly in the control run's job log rather than relaying: every commit: and
   round-trip: control-apply defaults a missing pool pass to x and the feed hides whether one is set, so a consumer editing pools wipes the credential silently #415 row stayed green under the substitution — including
   round-trip: replaying the feed's own pools preserves the password — because every one of those
   fixtures stores a real password. So the defect's actual symptom has no end-to-end guard. This PR
   pins the cause; No end-to-end row covers the masked-secret round trip for a pool that stores no password #439 is the symptom half.

[VijitSingh97]

Correction to my MERGE-READY comment above: all nine checks are green at 63463f53. That comment
said End-to-end (Docker) and Test suite (macOS) were still in progress. They were not — both had
already completed successfully (End-to-end (Docker) started 02:05:14Z, Test suite (macOS)
02:02:36Z, both conclusion: success).

The cause is worth recording, because it will mislead the next reader the same way: gh pr checks
kept reporting those two as pending for more than twenty minutes after they finished. The
check-runs API disagrees with it — gh api repos/.../commits/<sha>/check-runs reports
status=completed conclusion=success for both. Settle a check's state from the API, not from
gh pr checks.

Nothing else in that comment changes. The recorded non-author PASS is at 63463f53, and the branch
being one commit behind develop after #436 merged still stands.

[VijitSingh97]

RE-PASS: the verdict above stands unchanged at the new head fa505d9d.

The head moved because I ran the update-branch myself after merging #436 underneath it, so this
re-confirmation is of my own action rather than of a push by the author.

I verified the updated head carries your change and nothing else, by a different route than the
author's, which is worth having because the two methods can fail differently. I compared the PR's
own changes against the new merge-base rather than diffing the two patches:

compare/develop...fa505d9d  →  merge_base = 22fe6c0d   (the #436 merge)
                               files = 2
                               rigforge.sh   +2  -0
                               tests/run.sh  +14 -0

Two files, +16, additions only — identical to what I reviewed at 63463f53. The author
independently diffed the two patches and found they differ in exactly three lines: two blob index
hashes and one hunk-header offset (@@ -4373 → @@ -4375, from the two lines #434 added above in
rigforge.sh). Two independent methods, same answer — nothing in the merge changed a line of
this PR's content.

I will merge this on the green rollup at fa505d9d, so it does not need to be left MERGE-READY
for the seat. Test suite (macOS) and End-to-end (Docker) were still running at my last read.

Taking the author's instrument note with it, since it bears on how I read that rollup: settle check
state from repos/.../commits/<sha>/check-runs rather than gh pr checks, which reported checks
pending on this PR for 20+ minutes after they had actually succeeded. That is relayed from the
author, not measured by me — I will read the check-runs endpoint before merging either way, so the
merge does not rest on it.