[Sprint] sprint-loop-42#38
Merged
Merged
Conversation
Rewrite `::get_includes` to match a leading `@include[[:space:]]+` directive, strip any trailing `# comment`, and emit field 2 only. The previous `grep "^@include" | awk '{print $NF}'` surfaced trailing comment tokens, multi-arg trailing tokens, and bare `@include` lines as fake include names, leading `includer::find` to fail or produce noisy errors at release time. Also harden the companion Makefile filter: `grep -v ".sh$"` (regex; `.` matches any char) becomes `find ! -name '*.sh'` so the extension exclusion is a literal match. Adds `tests/pack-script.bats` with three regression cases (trailing comment, multi-arg, leading-only) and three fixtures under `tests/pack-script/`.
Extends `tests/bashadoc.bats` from the three SUR-2829 regression tests
to the five contract points enumerated in SUR-2835:
1. `@package` header rendering.
2. No-`@package` fallback to filename + bare-name function list.
3. `@doc` then `@arg` ordering, with the declare-f trailing
semicolons scrubbed by the `tr ';' ' '` step.
4. `function @Package() {` non-pollution regression (the SUR-2829
anchor fix; kept here so future refactors of the awk extraction
cannot silently regress it).
5. Non-`.sh` argument fails with exit 1 and the documented stderr.
Fixtures live under `tests/fixtures/bashadoc/` so each case can be
inspected directly rather than reading heredoc'd content inside the
spec. No production change to `bash/bashadoc` was required — the
SUR-2829 anchor fix already in place handles case 4 cleanly.
Adds `DAML_EXPORT_SOURCE_ONLY` source-only guard near the bottom of
`bash/daml-export`, mirroring the `K8S_SUPPORT_COLLECTOR_SOURCE_ONLY`
seam already in use for the same pattern. Guard defaults to executing,
so unwrapped invocations (including `tests/sur-1871-daml-export.sh`)
are unchanged.
Adds `tests/daml-export.bats` exercising the five contract points
enumerated in SUR-2838 in isolation:
1. `hex_to_dec` / `dec_to_hex` round-trip + 16-char hex padding.
2. `verifyExport` three-state return code (2 for empty/missing,
1 for exported-not-built, 0 when dar present).
3. `correct_archives` sed rewrite (single match, multi-match,
non-match passthrough) and idempotency on a second invocation.
4. `correct_export` dual-file rewrite (`daml.yaml --target=1.14
→ 1.12`, `Export.daml` import drop, archive correction chained).
5. Smoke check that the SOURCE_ONLY guard short-circuits the main
loop when the env var is set.
Test bodies run inside `bash -c` to insulate the bats `set -e` from
the trailing `&&` short-circuit in `options::add` triggered when the
script's option table is built on source.
Adds tests/pagerduty-alert.bats covering the entry script's own logic
(`bash/pagerduty.sh` is already exercised in tests/pagerduty.bats):
1. Missing required flags (-a / -s / -i) each individually exit
non-zero with the help banner.
2. With `-i incident`, `pagerduty::send_incident` receives
`service_id alert_type alert_title alert_from alert_token incident_key`
in that order.
3. Default ALERT_TITLE ("Test Alert") and default ALERT_FROM
("no-reply@blockchaintp.com") are applied when the corresponding
flags are omitted.
4. Non-`incident` `-i` values trigger `error::exit` with the
documented message.
5. A non-zero return from the stubbed send propagates as
`Failed to send incident` and a non-zero exit code.
The stub seam reserves pagerduty.sh's @include dedup guard before
sourcing the entry script, then in-process `pagerduty::send_incident`
records argv to a log file. exec::hide is reused as-is. No production
change to bash/pagerduty-alert was required.
SUR-2844 proposed that `describe_pods` creates an unused
`$out_dir_ns/pod` directory that ships empty inside every collected
support tarball. Investigation showed the premise is incorrect.
`k8s::get_pod_names` wraps `kubectl get pods -o name`, which emits
names in the `pod/<name>` form. The subsequent redirect
k8s::describe -n "$ns" "$pod" >"${out_dir_ns}/${pod}.describe"
therefore expands to `${out_dir_ns}/pod/<name>.describe` — files DO
land under `pod/`, and the `dirs::ensure "$out_dir_ns/pod"` is needed
for the redirect's parent dir to exist. Removing it (per the issue's
suggested option a) causes the redirect to fail with "No such file or
directory" and the collector misses every pod's describe output.
This commit makes that constraint explicit in a comment so a future
"cleanup" PR cannot regress this code path. No behavioural change.
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.
Sprint Plan — 2026-05-13 sprint-loop-42
Sprint goal
Close concrete coverage gaps in three undertested command scripts (
bashadoc,pagerduty-alert,daml-export) and harden two well-scoped quality issues(
pack-script@includeparsing;k8s-support-collectorempty-dir noise).Each item is small, self-contained, and lands behind existing test
infrastructure (bats + sprint regression scripts). The goal is to convert
"silent-failure" surfaces into asserted behaviour without expanding scope into
the wider library refactor backlog.
Selected issues
SUR-2835 — Test: bashadoc has no bats coverage
Description summary:
bash/bashadocships thedoc-*.tar.gzreleaseartifact via
make packagebut has no direct test file. The current "smoketest" passes even when bashadoc emits empty/garbage markdown because the tar
step never inspects content. The issue calls out specific brittleness around
@packageresolution andfunction @package() {lines indoc.shpollutingthe resolved package name.
Rationale:
bashadocis a published-artifact generator with no unitcoverage. The author already provided a concrete five-case test list, fixtures
suggestion, and known regression (the
@packageself-collision). This is thehighest-leverage testing addition in the backlog: small surface, high
downstream impact.
Definition of Done:
tests/bashadoc.batsexists and is wired intomake test_bats/pre-commit run bats --all-files.@packageheader,no-
@packagefallback,@doc/@argordering with semicolon stripping,function @package() {non-pollution regression, non-.sharg failswith exit 1.
tests/fixtures/bashadoc/and are referenced by path,not heredoc'd inline.
make testis green locally.bash/bashadoc(test-only, unless a fixture surfacesthe
@packageregression — then a minimal in-place fix is acceptable andmust be called out in the commit message).
SUR-2836 — Test: pagerduty-alert entry script has no bats coverage
Description summary:
bash/pagerduty-alert(the executable wrapper aroundpagerduty.sh) has no.batsfile even though the underlying library iswell covered. The script's option-validation,
ALERT_TYPEdispatch, defaultALERT_FROM/ALERT_TITLEhandling, anderror::exitnon-incident path areexercised only by manual invocation; a silently regressing default fails open.
Rationale: Same low-cost / high-payoff shape as SUR-2835. Stub seam
(
pagerduty::send_incident,exec::hide) already exists intests/pagerduty.bats; we can re-use it. Issue lists five exact assertions.Definition of Done:
tests/pagerduty-alert.batsexists with the five assertions enumerated inthe issue (missing-flag exits, stub argv ordering, default
ALERT_TITLE/ALERT_FROM, non-incidentALERT_TYPEpath, send-failure propagation).tests/stubs/, mirroring thetests/pagerduty.batspattern.
tests/bats/bin/bats tests/pagerduty-alert.batsand ispicked up by
make test_bats.bash/pagerduty-alertitself.SUR-2838 — Test: daml-export helper functions lack unit coverage
Description summary:
bash/daml-exporthas helpers (hex_to_dec,dec_to_hex,verifyExport,correct_archives,correct_export, plus theno-progress sentinel) exercised only via the SUR-1871 end-to-end regression.
Their documented contract points (three-state
verifyExportreturn code,correct_archivessed idempotency,correct_exportdaml.yaml/Export.damlrewrites) are not asserted in isolation. The issue suggests adding a
DAML_EXPORT_SOURCE_ONLYtest seam mirroringK8S_SUPPORT_COLLECTOR_SOURCE_ONLY.Rationale: Largest of the three test-coverage items because it requires a
small production change (
DAML_EXPORT_SOURCE_ONLYguard) before the batsspec can source the script without firing the main loop. The pattern is
already proven elsewhere, so risk is contained.
Definition of Done:
DAML_EXPORT_SOURCE_ONLY-style guard near the bottom ofbash/daml-exportso the helpers can be sourced without executing the mainloop. The guard MUST default to executing (no behavioural change for
unwrapped invocations).
tests/daml-export.batscovers the five cases from the issue(
hex_to_dec/dec_to_hexround-trip,verifyExportthree-state,correct_archivesrewrite + idempotency,correct_exportdual-filerewrite).
tests/sur-1871-daml-export.shstill passes unchanged(
tests/run.shgreen).pre-commit run shellcheck --all-filesclean against the modified script.SUR-2842 — Anti-pattern: pack-script extracts @include names via
awk '{print $NF}'Description summary:
bash/pack-script's::get_includesusesgrep "^@include" | awk '{print $NF}', which mis-parses inline comments(
@include foo # SUR-NNNN→SUR-NNNN), extra-arg lines, and unanchoredmatches. The companion
Makefilegrep -v ".sh$"filter has the samefragility class (
.matches any character).pack-scriptis the bin-tarballpacker, so a silent misparse ships a broken release artifact.
Rationale: Concrete fix sketch already in the issue (awk script that
strips comments + selects field 2). Production-affecting (release tarball),
small change, easy to test against the new fixture infrastructure landing
in SUR-2835 / SUR-2836.
Definition of Done:
bash/pack-script::get_includesrewritten to anchor^@include[[:space:]]+and strip trailing comments before extracting the include name (per the
awk snippet in the issue, or equivalent).
Makefilegrep -v ".sh$"replaced withfind -name '*.sh'exclusion (orequivalent) so the filter is a literal extension match, not a regex.
tests/pack-script.batsif absent, or extend existing) assertsthree regression cases: trailing-comment line, multi-arg line, leading-only
match.
make packagesucceeds and the produceddist/bin-*.tar.gzisbyte-identical to the prior run for unaffected scripts (eyeballed via
tar -tzf).SUR-2844 — Simplify: k8s-support-collector creates an unused $out_dir_ns/pod directory
Description summary:
describe_podsinbash/k8s-support-collectorcreates
$out_dir_ns/podbut never writes into it; the.describeoutputgoes to
$out_dir_ns/$pod.describe(which already includes the literalpod/prefix fromkubectl get pods -o name). Result: every collectedsupport tarball ships an empty directory. Issue offers two fixes: (a) drop the
unused
dirs::ensure, or (b) make the layout symmetric withlogs/bywriting under
pod/.Rationale: Smallest change in the sprint, but production-visible (support
bundles) and trivially verifiable. Use as the closing item to leave slack for
spillover from the test-coverage tickets.
Definition of Done:
(b, make functional) called out in the commit message with rationale.
fixture (
tests/sur-*.sh) or operator-facing doc that named the pathpre-fix.
pre-commit run shellcheck --all-filesclean.bash/k8s-support-collectoragainst a fixture namespace and inspect the produced directory tree.
Dependencies / ordering
The five items are largely independent. Suggested sequencing:
establishes the
pack-scriptfixture pattern that SUR-2835's bashadocfixtures will follow.
shared files.
DAML_EXPORT_SOURCE_ONLYseam; do after thepure-test items so reviewers see the test-only PRs first.
No cross-issue file overlap detected: pack-script (
bash/pack-script,Makefile), bashadoc (tests/), pagerduty-alert (tests/), daml-export(
bash/daml-export,tests/), k8s-support-collector(
bash/k8s-support-collector).Risks + mitigations
pack-scriptMakefile change breaks bin-tarball selection in CI.Mitigation: run
make clean packagelocally and difftar -tzf dist/bin-*.tar.gzagainst the pre-change tarball before pushing.DAML_EXPORT_SOURCE_ONLYguard mis-placed and breaks the main loop forreal invocations. Mitigation: model the guard exactly on
K8S_SUPPORT_COLLECTOR_SOURCE_ONLY; add a smoke assertion intests/daml-export.batsthat sourcing without the env var runs the loop(or, equivalently, that the existing
tests/sur-1871-daml-export.shstill passes unchanged).
function @package() {regression mid-PR.Mitigation: the in-place fix is small; commit it separately within the
same PR, with a clear "regression discovered by SUR-2835 fixtures" message.
bash/pagerduty.shsemantics.Mitigation: re-use the stub helpers already in
tests/pagerduty.batsrather than redefining; if helpers need promotion, move them into
tests/stubs/pagerduty.bashand have both specs source it.consumers of the support tarball. Mitigation: prefer option (a) —
drop the unused
dirs::ensure— unless an operator-facing doc explicitlypromises the
pod/subdir.mddoc/doc.shcoverage gaps(called out in SUR-2835). Mitigation: explicitly out-of-scope below.
^(fix|feature|refactor|sprint)/[a-zA-Z0-9\\-]+$)rejects per-issue branches with underscores. Mitigation: branch names
use the
feature/sur-NNNNform already returned by Linear'sgitBranchName.Out of scope
mddoc/doc.shtest coverage (called out in SUR-2835's "Why it matters"but not selected; file a follow-up if SUR-2835 fixtures make it cheap).
standard_defs.mkcleanup (SUR-2841) — touches releasemachinery, deserves its own focused sprint.
find-squashablefirst_squashoverload (SUR-2840),log.shsnapshot/restore flatten (SUR-2843) — both deferred to keep this sprint's blast
radius small (log.sh is sourced by every script).
doc-focused sprint can batch them with related runbook updates.
bash/update-repo-tags,make publish).tests/bats.Linear Evidence
ce9ebfde-ff2b-4f54-90f1-c388591ca110)a43901a0-b02b-4009-aae1-a6e8903d127d)list_issues(project="shell-scripts", state="Backlog", limit=100)against the Surinis team; followed byget_issue(includeRelations=true)andlist_issues(parentId=...)per candidate, pluslist_commentsper candidate.hasNextPage=false).manual-labelled Backlog issues skipped: 0 (none of the 10 carried themanuallabel).relations.blockedBy = [].<OPEN_PR_FILES>was[], so Filter B is a no-op for this run.Sub-issue Status
All selected issues were checked via
list_issues(parentId=...)and returned{"issues":[],"hasNextPage":false}. None of the selected issues havesub-issues; no parent-gate violations apply.
Linear State Transitions