You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Closes#549. Apple Bash 3.2 can misparse nested command substitutions in the timer-refresh fixtures, aborting the aggregate before its required summary while returning zero. That can make make test-suite report success without completing its assertions.
How I fixed it
Run the affected fixtures through functions with isolated subshells, retaining all existing timer-read and liveness assertions. The aggregate EXIT trap now cleans its sandbox, preserves nonzero failures and turns an incomplete zero exit into failure. Register a bounded regression directly in tests/run.sh and correct the testing documentation. Shorten one existing fixture comment to keep the aggregate within its unchanged file ceiling.
#556 remains separate: configured worker-root discovery has an independent cause and needs an external-root regression plus hardware contract proof.
Here is the proof
Ponytail review: cut two unnecessary fixture wrappers; reuse the existing assertions and suite bootstrap/summary, with no new dependency or product changes.
Head: 3e57830e41e589e3ab75fad2b96a541a6b4edd46.
At base ed00e1525e3a4e858e39c94f1894c3edb88a0aab, the new bounded regression exits 1: its early-zero-exit case observes exit 0 instead of failure. With only the completion guard restored, the same regression exits 1 on the original refresh fixture: native Bash reports the NextElapse command-substitution parser error and n: unbound variable. Both red runs use the actual aggregate bootstrap and refresh fixture, not a recreated parser example.
/bin/bash tests/test-suite-liveness.sh and bash tests/test-suite-liveness.sh — PASS on the fixed tree, under native Bash 3.2 and installed Bash 5 respectively. Covers premature zero/nonzero exit, nounset, sourced syntax error, normal completion, failed assertions, sandbox cleanup, and execution of all 27 existing bounded timer-refresh assertions.
PATH="/usr/bin:/bin:/usr/sbin:/sbin:$PATH" timeout 120 /bin/bash tests/test-suite-liveness.sh — PASS with native macOS tools first on PATH.
FILE_BUDGET_REQUIRE_BASE=1 bash scripts/lint-file-budget.sh, make lint-topology, npx --yes markdownlint-cli2@0.22.1 tests/README.md, and git diff --cached --check — PASS.
Adversarial verifier and security review are clean. All eight GitHub checks passed on head 3e57830e41e589e3ab75fad2b96a541a6b4edd46: shell, YAML and Markdown lint, the full test suite, coverage, Docker e2e, secret scan and workflow audit. Verified with gh pr checks 593 --repo p2pool-starter-stack/rigforge --json name,state,link; GitHub CI and security checks record the results.
Required proof follows the GitHub tiers 1–3 row for test fixtures and the suite completion guard. The operator confirmed that this scope requires no tier-4 job. The full host-dependent suite and container tests passed in GitHub CI; no local Docker or hardware testing was run. #549 remains separate from PR #592. This PR stays draft for normal runner review; the configured milestone freeze and explicit holds remain in force. Merge still requires agent review PASS followed by VijitSingh97’s formal approval on the current head.
Reviewed all four changed files against #549: Bash 3.2 must execute the refresh fixtures, and an aggregate that exits before its summary must fail. Head 3e57830 satisfies that scope with independently reproduced red/green tests and eight successful GitHub checks.
Read runbook README, ops/rules.md and ops/local-ask.md: established trust, validation and publication requirements.
Read AGENTS.md, CONTRIBUTING.md and ponytail skills: checked repository fit, correctness, security, comment discipline and unnecessary machinery; no blocking findings.
gh pr view 593 and gh issue view 549: trusted authors; read claim and comments; no earlier review findings on this PR.
Connected source reads and registration/trap searches: new regression executes directly from tests/run.sh through make test-suite and CI; existing refresh assertions remain.
gh run view 39272859110 --json headSha,conclusion,status: completed success on the reviewed head.
gh run view 39272853074 --json headSha,conclusion,status: completed success on the same head.
gh run view 39272859110 --log, followed by raw log reads: registered regression passed; aggregate reached 2,460 passed, zero failed; coverage aggregate reached 2,342 passed, zero failed.
/bin/bash --version: native Bash 3.2.57.
/bin/bash tests/test-suite-liveness.sh: exit 0; all seven cases passed, including the actual fixtures and their 27 bounded assertions.
bash tests/test-suite-liveness.sh: exit 0 under installed Bash 5.
Native-tool PATH with timeout 120 /bin/bash tests/test-suite-liveness.sh: exit 0.
Detached scratch checkout with pre-fix guard and fixtures restored from parent ed00e15: regression exited 1; early-zero case expected 1 but got 0.
Scratch checkout with only pre-fix fixtures retained: regression exited 1 with the original NextElapse parser error and n: unbound variable.
Restored head fixtures in scratch checkout: regression exited 0; all seven cases passed.
FILE_BUDGET_REQUIRE_BASE=1 bash scripts/lint-file-budget.sh: exit 0; ceilings and monotonic baseline passed.
make lint-topology: exit 0; self-tests and repository scan passed.
Local shfmt 3.13.1 initially exited 1 on existing formatting. Downloaded and verified the release-asset SHA-256 for pinned 3.14.1; its check of all three shell files exited 0.
git diff origin/develop...HEAD --check: exit 0.
Runner detect-topology.py over PR body, comments/reviews and issue comments: clean. Initial --help invocation failed because the detector treats it as a filename; corrected invocation passed.
gh api pulls/593/comments: empty.
rigforge#493 body:5: The topology detector flags one line in this older linked issue. The operator should scrub it; it is outside this PR’s publication scope.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stage/awaiting-mergethe reviewer's PASS is on the head; merging when checks and the base allow
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's the problem
Closes #549. Apple Bash 3.2 can misparse nested command substitutions in the timer-refresh fixtures, aborting the aggregate before its required summary while returning zero. That can make
make test-suitereport success without completing its assertions.How I fixed it
Run the affected fixtures through functions with isolated subshells, retaining all existing timer-read and liveness assertions. The aggregate EXIT trap now cleans its sandbox, preserves nonzero failures and turns an incomplete zero exit into failure. Register a bounded regression directly in
tests/run.shand correct the testing documentation. Shorten one existing fixture comment to keep the aggregate within its unchanged file ceiling.#556 remains separate: configured worker-root discovery has an independent cause and needs an external-root regression plus hardware contract proof.
Here is the proof
Ponytail review: cut two unnecessary fixture wrappers; reuse the existing assertions and suite bootstrap/summary, with no new dependency or product changes.
Head:
3e57830e41e589e3ab75fad2b96a541a6b4edd46.ed00e1525e3a4e858e39c94f1894c3edb88a0aab, the new bounded regression exits 1: its early-zero-exit case observes exit 0 instead of failure. With only the completion guard restored, the same regression exits 1 on the original refresh fixture: native Bash reports theNextElapsecommand-substitution parser error andn: unbound variable. Both red runs use the actual aggregate bootstrap and refresh fixture, not a recreated parser example./bin/bash tests/test-suite-liveness.shandbash tests/test-suite-liveness.sh— PASS on the fixed tree, under native Bash 3.2 and installed Bash 5 respectively. Covers premature zero/nonzero exit, nounset, sourced syntax error, normal completion, failed assertions, sandbox cleanup, and execution of all 27 existing bounded timer-refresh assertions.PATH="/usr/bin:/bin:/usr/sbin:/sbin:$PATH" timeout 120 /bin/bash tests/test-suite-liveness.sh— PASS with native macOS tools first on PATH.shellcheck --severity=warning tests/run.sh tests/test-api-refresh-liveness.sh tests/test-suite-liveness.sh— PASS.shfmt -i 4 -d tests/run.sh tests/test-api-refresh-liveness.sh tests/test-suite-liveness.sh— PASS with GitHub release-asset-digest-verified v3.14.1.FILE_BUDGET_REQUIRE_BASE=1 bash scripts/lint-file-budget.sh,make lint-topology,npx --yes markdownlint-cli2@0.22.1 tests/README.md, andgit diff --cached --check— PASS.Adversarial verifier and security review are clean. All eight GitHub checks passed on head
3e57830e41e589e3ab75fad2b96a541a6b4edd46: shell, YAML and Markdown lint, the full test suite, coverage, Docker e2e, secret scan and workflow audit. Verified withgh pr checks 593 --repo p2pool-starter-stack/rigforge --json name,state,link; GitHub CI and security checks record the results.Required proof follows the GitHub tiers 1–3 row for test fixtures and the suite completion guard. The operator confirmed that this scope requires no tier-4 job. The full host-dependent suite and container tests passed in GitHub CI; no local Docker or hardware testing was run. #549 remains separate from PR #592. This PR stays draft for normal runner review; the configured milestone freeze and explicit holds remain in force. Merge still requires agent review PASS followed by VijitSingh97’s formal approval on the current head.