Repository navigation
Merged
Conversation
…ites (#476, #477, #478) test-release-branch-437.sh and -461.sh decided pass/fail from whether the output had a "^FAIL " line, so a non-zero exit with no FAIL line at all -- check_release_tree.py crashing, not reporting an offender -- took the same branch as a clean run (#476). Both now classify "clean" / "crash" / "offenders" as three distinct outcomes via a shared classify_check_result(). test-release-branch-461.sh also hard-coded --ref HEAD while the workflow builds and publishes $SOURCE_REF; on workflow_dispatch with an older ref= input the two diverge and the equivalence step silently checked the wrong tree (#477). It now reads $SOURCE_REF from the environment, falling back to HEAD for a local/manual run. detect_heredoc_lines() in test-no-heredocs-459.sh folded awk's exit status into a trailing "|| true" before the grep pass, so an unreadable/vanished file produced the same empty string as a genuinely clean one (#478). awk's own rc is now captured separately and returned as 2, with a positive/negative control pair and a live-sweep path that reports an unscanned file rather than silently treating it as clean. Self-tests added for all three; each was confirmed red against the old logic before the fix and green after. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…d, accurate duplication comment Explore's self-review of 77d5759 found two things: - The #477 self-test only re-proved bash's own "${VAR:-default}" semantics in an isolated subshell, never touching the $REF variable the real build line actually uses -- it would keep passing even if that line reverted to a hardcoded "--ref HEAD" (the #477 regression itself). Added a third self-test that greps this script's own source for the build invocation passing "$REF", confirmed it fails against the old hardcoded line and passes against the current one. - classify_check_result() is independently duplicated in both test-release-branch-437.sh and test-release-branch-461.sh (no common sourced file between these two standalone suites), but the comment in each said "shared" / "mirrors" as if one sourced the other. Corrected both comments to say DUPLICATED and flag that the two copies must be kept in sync by hand. oss:auditor's self-review additionally flagged tests/test-no-heredocs-459.sh as apparently unwired from CI -- checked against .github/workflows/tests.yml and tests/run-all.sh (which globs test-*.sh with no exclusion list) and found this finding was wrong: the file does run on all three CI platforms via the `hooks` job's `bash tests/run-all.sh` step. No action taken; not logged to trap.d. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…nned stream, correct stale offender message A re-spawned Explore review of edb6893 found three things: - detect_heredoc_lines() in test-no-heredocs-459.sh captured awk's stderr into the same text grep -nE later scans, even on success ("2>&1" applied unconditionally). A non-fatal stderr warning from a different awk implementation (gawk on Linux CI vs. the BSD/mawk this was likely tested against) could splice extra text into the data being line-numbered, corrupting the real heredoc hits it reports -- a failure mode the #478 fix itself introduced. Fixed: stderr is now discarded on success (2>/dev/null) and only ever reported (to the caller's own stderr) on the failure path; nothing from awk's stderr enters the grep pipe. - The "offenders" ok-message in test-release-branch-437.sh's case block named the #439 ALLOWED_TOOLS_BROAD hold by file, but _KNOWN_ALLOWED_TOOLS_HOLDS has been empty since #439 resolved that hold -- the message described a specific, no-longer-live exception, inside the exact hunk this lane's own #476 fix touched (the if/else -> case restructuring) without correcting it. Reworded to name the variable generically so it stays accurate whether or not that set is ever populated again. - A third finding (the #477 fix narrows, but does not fully close, a cross-job-step race if a workflow_dispatch ref= input is a moving branch name rather than a tag/sha) is a workflow-level design question beyond what #477 asked for -- logged to trap.d/477.sha-not-threaded-across-jobsteps.md for curation rather than fixed in this lane. Confirmed test-no-heredocs-459.sh and test-release-branch-437.sh still pass (437's 2 failures are the same pre-existing, environment-only `claude plugin validate --strict` mismatch confirmed unrelated to this diff by running the suite against unmodified main). Co-Authored-By: Claude Sonnet 5 <[email protected]>
…read-failure path A second-pass Explore review of 0acd3ad (scoped to that diff alone, since it touched 3 files and tripped the needs-second-pass threshold) found that discarding awk's stderr via 2>/dev/null applied on BOTH the success and the failure path. The success-path redirect is the fix that round closed (stderr noise must never splice into $filtered, which grep -nE scans); the failure-path redirect was collateral -- it regressed the diagnostic versus the pre-#478 code, which used to report awk's actual stderr text ("awk: can't open file ..."). The generic "AWK_READ_FAILED($1)" message carried no indication of why. Fixed by capturing stdout and stderr into two separate places: $filtered (awk's stdout only, used by the grep pass exactly as before) and a scratch file under $TMPD read back only on the failure branch. Confirmed the real error text ("awk: can't open file ...") now appears in the diagnostic again while the success path is unchanged. tests/test-no-heredocs-459.sh: 6/0. Co-Authored-By: Claude Sonnet 5 <[email protected]>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Three local test-harness gaps, all found during release-audit review rather than by a live failure.
#476 --
tests/test-release-branch-437.shandtests/test-release-branch-461.shdecided pass/fail by checking whethercheck_release_tree.py's output had any^FAILline. A non-zero exit with no FAIL line at all (a crash/traceback) took the same "ok" branch as a genuinely clean run. Fixed with a shared-shapeclassify_check_result(rc, output)function (independently duplicated in both files -- no common sourced library between these two standalone suites) returningclean/crash/offenders, with self-tests exercising the exact function used by the real check.Closes #476
#477 --
test-release-branch-461.shhard-coded--ref HEADwhen building a release tree, while.github/workflows/release-branch.ymlbuilds and publishes$SOURCE_REF, which diverges on aworkflow_dispatchrun with an olderref=input. Fixed by readingREF="${SOURCE_REF:-HEAD}"from the job-level env var the workflow already sets, falling back toHEADfor local/manual runs, with a self-test grepping the script's own source so a revert to the hardcoded line fails the suite.Closes #477
#478 --
detect_heredoc_lines()intests/test-no-heredocs-459.shpipedawk ... | grep -nE ... || true, so an awk read failure (vanished file, permissions) produced the same empty string as a clean file. Fixed by capturing awk's own exit code separately from grep's, returning 2 on failure, with positive/negative self-test controls and a live-sweep path that reports an unscanned file distinctly from a clean one.Closes #478
Self-review
Three rounds (Explore + oss:auditor, concurrent, against each prior round's diff). Round 1 fixed two findings (a vacuous #477 self-test, a misleading "shared" comment on the duplicated classifier) and argued down one auditor finding (a false claim that
test-no-heredocs-459.shhas no CI coverage -- it does, viatests/run-all.sh's unconditional glob in thehooksjob across all three OSes). Round 2 (triggered by round 1 needing a second pass) fixed a stderr-splicing regression the round-1 #478 fix itself introduced, corrected a stale comment naming an already-resolved #439 exception, and logged one out-of-scope finding totrap.d/477.sha-not-threaded-across-jobsteps.md: the equivalence-test step still independently re-resolves $SOURCE_REF instead of receiving the already-resolved sha from the build step's own job output, leaving a narrower race window instead of none for a workflow_dispatch run against a moving ref -- a workflow-level change (threading a job output across steps) beyond this issue's own scope, not fixed here. Round 3 fixed a second-order regression in round 2's own fix (discarding awk's stderr on the read-failure path too, losing the diagnostic text) and found the auditor clean.Tests
claude plugin validate --strictCLI-version mismatch (confirmed identical against main's own clone with none of this diff present).python3 .oss/assemble_changelog.py --check: all 5 fragments pass.🤖 Generated with Claude Code
Co-Authored-By: Claude Sonnet 5 [email protected]
[AI-generated]