docs(scripts): record why the --all portability sweep is slow, not flaky - #3057
Conversation
`--all` was treated as environmentally unreliable across four separate
attempts, each written off as a command timeout or a container restart. That
diagnosis was wrong and it was self-perpetuating: "it got unlucky" invites
retrying the same run, which fails the same way and produces no new
information.
The real cause is structural. Per-file cost grows superlinearly with file
length, so a handful of the longest files dominate the whole sweep while the
great majority cost almost nothing -- measured here as ~123s for the single
worst file against ~3s for its own first 1200 lines, and ~10 minutes for the
full run. It therefore cannot fit a 600s command timeout on a tree this size,
and it has to be detached rather than retried.
Two traps that cost real time are recorded with it, because neither is
discoverable from a failing run:
- `pgrep -f 'check-shell-portability'` matches the WAITING shell's own
command line, so a waiter built on it can never see its condition clear.
Two waiters were lost to this. Waiting on the pid is the correct form.
- A per-file sweep keyed only on the timeout status silently passes over any
file exiting 1, so its silence reads as a clean audit when nothing checked.
Figures are stated as a dated observation rather than a standing claim, and no
file list or file count is hardcoded: the exact numbers move with the corpus
while the shape does not. That is the same surfaces-and-derivability discipline
the sibling skill-count gate's header now carries (#3050).
Also states why CI runs the changed-file mode and never `--all`, which was
true of ci.yml already but written down nowhere.
Comment-only; no behavior, no token-list edit, no new construct class.
Verified: shellcheck at the repo rcfile, shfmt, typos, editorconfig, the
gate's own suite at PASS=333 FAIL=0, and changelog-parity (scripts/ is not a
versioned plugin, so no bump).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ADsDCToTtjvYut3ZQXJHDM
|
Claude finished @kyle-sexton's task in 37s —— View job Security review —
|
|
Last security-reviewed head: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d5de2b401
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…t the clock
The waiter this block prescribed was fail-open, and fail-open in exactly the
way the paragraph immediately below it warns against. A
`while kill -0 "$pid"; do sleep 15; done` loop reports only WHEN the run ended.
Its own status is the last `sleep`'s, so it reports 0 whether the audit exited
0, 1 for a real violation, or 2 for a usage error. An operator following the
recipe as written could read a failed fleet audit as a clean one.
Measured rather than reasoned: a child exiting 3 leaves that loop reporting 0,
while `wait` on the same child returns 3.
The fix has to cover both cases, because the suggested `wait "$pid"` is correct
only for one of them:
- Child of the current shell: `wait "$pid"` gives the real status. Use it.
- Not a child (started in an earlier shell, or via `setsid` -- which is the
situation that motivated waiting on a pid at all): `wait` cannot help. It
fails with "pid N is not a child of this shell" and returns 127, a value
indistinguishable from a genuine failure if taken at face value. Verified
locally. The status is unavailable there, so the outcome must be read from
the run's own output, and a missing success line must count as unknown
rather than as pass.
Raised by an automated reviewer, which was right that the status was being
discarded. Its proposed `wait "$pid"` is adopted for the child case and its
limit stated for the other, rather than documented as a universal fix that
would fail 127 in the case this block actually describes.
Comment-only; verified no non-comment line differs from main. shellcheck at the
repo rcfile, shfmt, typos, editorconfig all clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ADsDCToTtjvYut3ZQXJHDM
|
Claude finished @kyle-sexton's task in 33s —— View job Security review —
|
|
Claude finished @kyle-sexton's task in 3m 57s —— View job Code Review —
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # `shell-portability-lint`): a per-PR fleet sweep would be minutes of runner time | ||
| # to re-prove files the PR did not touch. | ||
| # | ||
| # Two operational consequences, both of which have already cost real time: |
There was a problem hiding this comment.
P3 "Two operational consequences" but three bullets follow
The list below this line now has three top-level - bullets at the same indent: the 600s-timeout point (L31), the pgrep -f self-match point (L36), and the "waiting is not checking" fail-open point (L39, added in the second commit of this PR). The header text wasn't updated when the third bullet was added, so it undercounts what's actually enumerated.
Minor, but worth a one-word fix ("Three...") or folding the fail-open bullet in as a sub-point of the pid-wait bullet so the count and the list agree.
| # so it is 0 whether the audit exited 0, 1 (violation) or 2 (usage error). | ||
| # Measured: a child exiting 3 leaves that loop reporting 0. Reading it as | ||
| # "clean" is exactly the fail-open the next paragraph warns about. | ||
| # - If the run IS a child of your shell, `wait "$pid"` yields its real | ||
| # status (measured: 3 for that same child). Use it, and check it. |
There was a problem hiding this comment.
P2 The "measured: 3" example doesn't match this script's actual exit codes
Line 42 enumerates the audit's possible exits as 0, 1 (violation), or 2 (usage error) — that's accurate; the only exit statements in this file are exit 0, exit 1, exit 2 (verified by grep across the file), there's no set -e/trap, and a signal death would surface as 128+signal (e.g. 131 for SIGQUIT), not a bare 3.
But the very next sentence, and its echo at L46, claim wait "$pid" was measured returning 3 for "that same child" — a status outside the set this paragraph just enumerated as exhaustive. There's no code path in check-shell-portability.sh that can exit 3.
This block's whole point is to replace vague "it got unlucky" narratives with measured, checkable facts, so an unexplained status the script itself can't produce undercuts that credibility — a reader debugging a real 3 would wrongly assume it's a known, documented outcome of this specific audit. Worth either correcting the number to one that's actually producible (e.g. 1 for a violation) or clarifying what was actually measured if it wasn't this script's own exit.
| # so it is 0 whether the audit exited 0, 1 (violation) or 2 (usage error). | ||
| # Measured: a child exiting 3 leaves that loop reporting 0. Reading it as | ||
| # "clean" is exactly the fail-open the next paragraph warns about. | ||
| # - If the run IS a child of your shell, `wait "$pid"` yields its real | ||
| # status (measured: 3 for that same child). Use it, and check it. |
There was a problem hiding this comment.
P2 The "measured: 3" example doesn't match this script's actual exit codes
Line 42 enumerates the audit's possible exits as 0, 1 (violation), or 2 (usage error) — that's accurate; the only exit statements in this file are exit 0, exit 1, exit 2 (verified by grep), there's no set -e/trap, and a signal death would surface as 128+signal (e.g. 131 for SIGQUIT), not a bare 3.
But the very next sentence, and its echo at L46, claim wait "$pid" was measured returning 3 for "that same child" — a status outside the set this paragraph just enumerated as exhaustive. There's no code path in check-shell-portability.sh that can exit 3.
Given this block's whole point is to replace vague "it got unlucky" narratives with measured, checkable facts, an unexplained status that the script itself can't produce undercuts that credibility — a reader debugging a real 3 would wrongly assume it's a known/documented outcome of this specific audit. Worth either correcting the number to one of 0/1/2 (with a scenario that actually produces it, e.g. 1 for a violation) or noting that the measurement was against a different wrapper/child if that's actually what happened.
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…erence form (#3061) No linked issue ## Summary Two findings from a babysit session, both of which cost real time and neither of which was written down anywhere. They are unrelated in mechanism but share a cause: an operator hits them only in CI, with no local signal. ## Fix **1. `stuck-checks.md` — checks that never schedule.** That file covered checks which are *present and never settle*, detected under `UNSTABLE`. It did not cover the opposite failure with the same surface complaint. A conflicted PR has no computable merge ref, so `pull_request` workflows are never created — **absent** rather than pending or failing, and therefore invisible to `checks.stuck`, which can only classify records that exist. What makes it actively misleading is that `pull_request_target` lanes run against the base and still pass. The PR shows a short all-green list with no failures anywhere — reading as "passing" or "not started yet" — while most gates are simply missing. The remedy is merge-or-rebase, not a CI investigation; diagnosing it as a trigger or App-token problem spends time on an uninvolved mechanism, which has already happened here once. The section **names** the two `pull_request_target` lanes rather than counting the split, and says to derive the split from the `on:` blocks. A count in prose goes stale the next time a lane is added — the exact defect the two recent skill-count-gate changes addressed. **2. New `docs/conventions/tracker-reference-form`.** What a code comment may say about an issue. It needs a written home precisely because the `comment-hygiene` lane **cannot be run locally**: the action is not vendored, so a violation is discoverable only after pushing. Written from the action source at the SHA `ci.yml` pins, not from memory — and that mattered. The belief carried into this change was that `PR #N` and `owner/repo#N` are simply rejected, but the tree contains *passing* instances of both. Reading the action resolved the contradiction: the allowlist is an extension set that excludes Markdown and YAML, so those instances are out of scope rather than tolerated. The doc states that scoping explicitly, so the counterexamples don't later read as evidence the rule is lax. Documentation only; no executable surface. ## Verification - `check-changelog-parity.sh --check`, `--check-bump origin/main`, `--check-order` — all green; `source-control` bumped with a matching entry - `check-skill-count-claims.sh --check` — every claim matches the tree - `check-shell-portability.sh --paths` on the changed skill file — clean - `markdownlint-cli2` on both docs — 0 issues - `typos`, `editorconfig-checker`, and `jq` on the manifest — clean The one rule this PR documents that it cannot verify locally is `comment-hygiene` itself; the added comment text was grepped for each rejected shape before pushing, and both changed docs are Markdown, which the policy does not scan. ## Related Refs #3050 and #3057 — the two changes whose sessions produced these findings. This PR closes out the last of them, so nothing from that work remains unrecorded. --- _Generated by [Claude Code](https://claude.ai/code/session_01ADsDCToTtjvYut3ZQXJHDM)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
2 participants
No linked issue
Summary
scripts/check-shell-portability.sh --allwas treated as environmentallyunreliable across four separate attempts, each written off as a command timeout
or a container restart. That diagnosis was wrong, and it was the
self-perpetuating kind: "it got unlucky" invites retrying the same run, which
fails the same way and yields no new information. Nothing in the repo recorded
the real reason.
Fix
Adds a
COST of --allblock to the script's header.The cause is structural, not environmental. Per-file cost grows superlinearly
with file length, so a handful of the longest files dominate the whole sweep
while the great majority cost almost nothing. It cannot fit a 600s command
timeout on a tree this size, so it must be detached rather than retried.
Two traps are recorded alongside it, because neither is discoverable from a
failing run:
pgrep -f 'check-shell-portability'matches the waiting shell's owncommand line, so a waiter built on it can never see its condition clear. Two
waiters were lost to this before it was spotted. Waiting on the pid is correct.
exiting 1, so its silence reads as a clean audit when nothing was actually
checked.
The block also states why CI runs the changed-file mode and never
--all— trueof
ci.yml'sshell-portability-lintalready, but written down nowhere.On the numbers: they are stated as a dated observation rather than a standing
claim, and no file list or file count is hardcoded. The exact figures move with
the corpus; the shape does not. That is deliberately the same
surfaces-and-derivability discipline the sibling skill-count gate's header
gained in #3050 — hardcoding "seven files over N lines" here would have created
precisely the drift that gate exists to catch.
Comment-only. No behavior, no token-list edit, no new construct class.
Verification
shellcheckat the repo's own.shellcheckrc— clean (re-run after rebase)shfmt -d— clean (re-run after rebase)scripts/check-shell-portability.test.sh, which CI runs ahead of the gate —PASS=333 FAIL=0typos,editorconfig-checker— cleanscripts/check-changelog-parity.sh --check— no version bump required;scripts/is not a versioned plugin and nothing underplugins/is touched(#N)form thecomment-hygienelanerequires, checked before pushing because that lane is an external composite
action this repo does not vendor and cannot be run locally
mainrather than left on the base it was writtenagainst, so
stale-base-overlap-gatesees a current merge baseThe claim this documents was itself measured end to end:
--allcompleted atexit 0 with
No unexcused GNU-only constructs in 1329 shell file(s), which isthe verification that had been outstanding since #3034.
Related
Refs #3050 — the sibling gate's surfaces boundary, whose derivability discipline
this block deliberately follows.
Refs #3034 — the PR whose outstanding
--allverification this work completed.Generated by Claude Code