Skip to content

[Sprint] sprint-loop-41#37

Merged
scealiontach merged 5 commits into
mainfrom
sprint/2026-05-12-sprint-loop-41
May 13, 2026
Merged

[Sprint] sprint-loop-41#37
scealiontach merged 5 commits into
mainfrom
sprint/2026-05-12-sprint-loop-41

Conversation

@scealiontach
Copy link
Copy Markdown
Owner

Sprint Plan — 2026-05-12 (sprint-loop-41)

Sprint goal

Eliminate five high-priority correctness defects in shared bash/ libraries
and command scripts that today silently corrupt output, leak resources, or
target the wrong namespace/remote. The selected work is a coherent
"library correctness" theme — each issue is a contained, well-specified bug
fix with concrete suggested fixes already in the issue body, allowing the
sprint to land observable behaviour fixes plus bats regression coverage
without scope creep.

Selected issues

SUR-2827 — Bug: copy-keys ignores -n NAMESPACE on follow-up kubectl get/cp calls

  • Description summary: bash/copy-keys threads -n NAMESPACE into the
    initial k8s::pod_names_for_label call but every downstream
    k8s::ctl get pod and k8s::ctl cp invocation (lines ~34–50) omits the
    namespace, so the script reads pod metadata and copies key material from
    whatever pod (if any) shares that name in the current-context default
    namespace. Result: broken symlinks, empty key files, or — worst case —
    copying keys from an unrelated pod.
  • Rationale: High-severity correctness/security bug affecting a key
    material extraction tool; the fix is well-scoped (one file, mirror the
    fetch-all-pod-logs ns_args=() pattern).
  • Definition of Done:
    • ns_args=() array built once at top of bash/copy-keys and applied to
      every k8s::ctl call that operates on a pod by name.
    • With -n foo, all kubectl operations target namespace foo.
    • With no -n, behaviour is unchanged from today (no spurious -n).
    • New bats spec tests/copy-keys.bats mocks k8s::ctl and asserts the
      namespace flag is forwarded on every call.
    • pre-commit run --all-files and make test pass.
  • Dependencies / ordering: Independent — touch bash/copy-keys only.

SUR-2828 — Bug: git::project_url matches any remote starting with "origin"

  • Description summary: git::project_url (bash/git.sh lines 54–68)
    resolves the origin URL via git remote -v | grep "^origin" | head -1;
    this matches origin-fork, origin-mirror, origin2, etc. Because
    git remote -v lists each remote twice and ordering between distinct
    remotes is not specified, head -1 is non-deterministic. The CHANGELOG
    generator (changelog) and update-repo-tags use this base URL — a
    misidentified origin silently links every CHANGELOG entry at the wrong
    repository.
  • Rationale: Affects release artifacts (CHANGELOG.md), is invisible
    unless audited, and the suggested fix (git config --get remote.origin.url)
    is one line.
  • Definition of Done:
    • git::project_url resolves the URL via git config --get remote.origin.url
      (preferred) or a tightened awk '$1=="origin" ...' selector.
    • Deprecated aliases git::projecturl / git::commit_url_base continue
      to forward correctly.
    • tests/git.bats gains a regression case proving that adding an
      origin-fork remote no longer pollutes git::project_url.
    • pre-commit run --all-files and make test pass.
  • Dependencies / ordering: Independent.

SUR-2829 — Bug: bashadoc emits wrong function list when input has no @Package directive

  • Description summary: bash/bashadoc extracts the package name with
    grep "@package" "$1" | awk '{print $NF}' | tail -1. For files without
    an @package annotation, $package is either empty or { (because the
    grep matches function @package() { in doc.sh). The downstream
    branch checks [ "$package" != "." ], where "." is a sentinel that is
    never assigned — so the "no package" branch is unreachable. Result:
    bashadoc bash/doc.sh emits a # `{` package header with zero
    functions, and any future helper without @package silently ships
    garbage markdown into dist/doc-*.tar.gz.
  • Rationale: Doc tarball is a release artifact; silent corruption is
    the worst failure mode.
  • Definition of Done:
    • Package extraction anchored: awk '/^[[:space:]]*@package /{print $2}'.
    • "No package" case detected by -n "$package" test, not the unreachable
      "." sentinel.
    • bashadoc bash/doc.sh produces a header derived from the filename and
      a non-empty (or correctly empty) function list with no `{` artifacts.
    • New tests/bashadoc.bats spec covers: (a) library with @package,
      (b) script without @package, (c) doc.sh (matches function @package).
    • pre-commit run --all-files and make test pass.
  • Dependencies / ordering: Independent. Pairs naturally with SUR-2835
    (bashadoc bats coverage), but SUR-2835 is out of scope for this sprint.

SUR-2830 — Bug: secret::clear only checks SECRET_TMPFILES[0]; sparse arrays leak tempfiles

  • Description summary: secret::clear in bash/secret.sh (~lines
    197–202) guards removal with [ -n "${SECRET_TMPFILES[0]}" ]. A sparse
    array (element 0 unset, later indices populated) makes the guard fail and
    the cleanup silently no-ops, leaving 0600 tempfiles containing decrypted
    secret material under $TMPDIR until OS-level /tmp cleanup.
  • Rationale: Direct violation of the "secrets never touch persistent
    storage" invariant established by SUR-2324; tiny fix; high security
    blast radius if the brittle guard ever encounters a sparse array.
  • Definition of Done:
    • secret::clear uses [ "${#SECRET_TMPFILES[@]}" -gt 0 ] and resets
      SECRET_TMPFILES=() after removal.
    • tests/secret.bats gains a sparse-array regression: pre-seed
      SECRET_TMPFILES=("$tmp0" "$tmp1"), unset 'SECRET_TMPFILES[0]', run
      secret::clear, assert $tmp1 no longer exists.
    • pre-commit run --all-files and make test pass.
  • Dependencies / ordering: Independent.

SUR-2831 — Anti-pattern: exec::capture installs a RETURN trap that leaks into the caller's scope

  • Description summary: exec::capture (bash/exec.sh ~line 34) installs
    trap 'rm -f "$tmpout"' RETURN and never clears it. The trap fires on
    every subsequent function return in the caller's chain and, because
    $tmpout is local, becomes a no-op rm -f "". But the trap slot is
    permanently occupied: any caller that installs its own RETURN trap loses
    it (last-writer-wins). Contradicts the chained-trap discipline added in
    SUR-2324 for secret.sh.
  • Rationale: Latent foot-gun in a widely-included library; the fix
    (inline cleanup instead of trap, matching the no-tempfile branch of the
    same function) is small and removes a class of debugging headaches.
  • Definition of Done:
    • exec::capture cleans up $tmpout inline (no RETURN trap), or
      explicitly trap - RETURN before returning.
    • After calling exec::capture, a caller-installed trap '...' RETURN
      still fires as expected (covered by a bats regression).
    • tests/exec.bats (or new file if absent) asserts no RETURN trap is
      left installed after exec::capture returns.
    • pre-commit run --all-files and make test pass.
  • Dependencies / ordering: Independent.

Risks + mitigations

  • Risk: git::project_url change to git config --get remote.origin.url
    could differ from git remote -v parsing in edge cases (insteadOf URL
    rewriting). Mitigation: keep awk '$1=="origin" && $3=="(fetch)"' as
    a fallback if git config returns empty; cover both paths in bats.
  • Risk: bats specs depending on mocked kubectl/git may be flaky on
    contributor machines with unusual PATH. Mitigation: use
    helpers::source_lib + isolated $HOME (helpers::isolate_home) and
    declare mock functions before sourcing the library under test.
  • Risk: Inline cleanup in exec::capture may need to handle exit code
    preservation (local rc=$?). Mitigation: explicit local rc=$?
    before rm -f, return $rc; bats spec asserts both stdout capture and
    exit code propagation.
  • Risk: bashadoc regex change might break correctly-annotated
    libraries that put @package mid-line. Mitigation: anchor on
    ^[[:space:]]*@package and add a positive-case bats fixture for every
    library currently in bash/.
  • Risk: secret::clear reset of SECRET_TMPFILES=() might break a
    consumer that re-reads the array post-clear. Mitigation: audit
    callers (grep -rn SECRET_TMPFILES bash/); none today rely on
    post-clear state.
  • Risk: Five bug fixes in a single sprint may exceed the available
    capacity if any one fix uncovers a deeper issue. Mitigation: each
    issue is independent — a stretched item can be dropped without
    blocking the others.

Out of scope

  • SUR-2832 / SUR-2834 documentation gaps.
  • SUR-2835, SUR-2836, SUR-2838 test-coverage backlog items (separate
    "testing" theme).
  • SUR-2840 / SUR-2842 / SUR-2843 / SUR-2844 simplification items.
  • SUR-2841 Makefile cleanup (Maven/Sonar/FOSSA dead code).
  • Any change to release pipeline (update-repo-tags, make publish).
  • Reworking the chained-trap pattern in secret.sh beyond the
    secret::clear guard fix.

Linear Evidence

  • Linear team verified: Surinis (id ce9ebfde-ff2b-4f54-90f1-c388591ca110)
  • Linear project used: shell-scripts (id a43901a0-b02b-4009-aae1-a6e8903d127d)
  • Query/filter used: list_issues(project="shell-scripts", state="Backlog", limit=100)
    then per-issue get_issue(includeRelations=true), list_issues(parentId=...),
    and list_comments(issueId=...).
  • Approx count of Backlog issues reviewed: 15
  • Approx count of manual-labelled Backlog issues skipped: 0
  • Issues skipped due to unmerged blockers: 0 — []
  • Issues skipped due to open-PR file overlap: 0 — [] (OPEN_PR_FILES was empty)

Sub-issue Status

No selected parent issue has sub-issues. Verified via
list_issues(parentId=<SUR-ID>) for each of SUR-2827, SUR-2828, SUR-2829,
SUR-2830, SUR-2831 — all returned empty.

Parent Issue Sub-issue Sub-issue Status Eligible?
SUR-2827 (none) n/a Yes
SUR-2828 (none) n/a Yes
SUR-2829 (none) n/a Yes
SUR-2830 (none) n/a Yes
SUR-2831 (none) n/a Yes

Linear State Transitions

Issue ID Previous State New State
SUR-2827 Backlog Todo
SUR-2828 Backlog Todo
SUR-2829 Backlog Todo
SUR-2830 Backlog Todo
SUR-2831 Backlog Todo

The previous grep '@Package' also matched 'function @Package() {' in
doc.sh, yielding '{' as the package name. Combined with the unreachable
"$package" != '.' sentinel, the no-package branch never fired —
bashadoc bash/doc.sh emitted a '`{`' header with garbage markdown,
which is what currently ships in dist/doc-*.tar.gz for doc.md.

Anchor extraction on a leading-whitespace '@Package' directive (field 2)
and detect the no-package case with [ -n "$package" ] so the
filename-derived fallback actually runs.

Adds tests/bashadoc.bats covering: a library with @Package, a script
without @Package, and bash/doc.sh itself (which defines function
@Package and would otherwise emit '{'). Verified by re-tarballing
dist/doc-*.tar.gz and inspecting doc/bash/doc.md.
git::project_url previously matched any remote starting with 'origin'
(origin-fork, origin-mirror, origin2, ...) and used 'head -1' over the
interleaved 'git remote -v' output, making the result non-deterministic
when multiple remotes were configured.

Switch to 'git config --get remote.origin.url' for an exact-name lookup,
with an awk fallback that still scopes to the literal 'origin' fetch URL
in the unlikely event 'git config' returns empty.

Affects CHANGELOG generation (changelog) and release tagging
(update-repo-tags) — both call git::project_url to compose links.

Adds a regression to tests/git.bats covering an origin/origin-fork pair.
secret::clear gated removal on [ -n "${SECRET_TMPFILES[0]}" ]. A
sparse array (element 0 unset, later indices populated by 'unset
'SECRET_TMPFILES[0]') makes the guard fail and the cleanup silently
no-op, leaving 0600 tempfiles with decrypted secret material under
$TMPDIR until OS-level /tmp cleanup runs. Direct violation of the
'secrets never touch persistent storage' invariant from SUR-2324.

Switch to [ "${#SECRET_TMPFILES[@]}" -gt 0 ], reset SECRET_TMPFILES=()
after removal, and add a sparse-array regression to tests/secret.bats.
Audit confirmed no caller relies on post-clear array state.
exec::capture installed 'trap rm -f "$tmpout" RETURN' but never
cleared it. The trap persisted into the caller's scope and, because
$tmpout is a function-local, fired as a no-op 'rm -f ""' on every
subsequent return. The real damage: the RETURN trap slot is permanently
occupied, so any caller-installed RETURN trap is silently clobbered
(last-writer-wins). Latent foot-gun contradicting the chained-trap
discipline SUR-2324 established for secret.sh.

Clean tmpout inline immediately after reading it; no trap installation.
The exit code is already captured into $exit_code from PIPESTATUS[0]
before cleanup, so propagation is preserved.

Adds two regressions to tests/exec.bats:
- after exec::capture returns, 'trap -p RETURN' is empty in the caller.
- a caller-installed RETURN trap still fires after exec::capture.
bash/copy-keys passed -n NAMESPACE only to the initial
k8s::pod_names_for_label call. The downstream 'k8s::ctl get pod' and
'k8s::ctl cp' invocations omitted the namespace, so the script read
pod metadata and copied key material from whatever pod (if any) shared
the same name in the current-context default namespace. Worst case:
broken symlinks, empty key files, or key material copied from an
unrelated pod.

Build ns_args=() once at the top, mirroring the fetch-all-pod-logs
pattern, and expand it into every downstream k8s::ctl call.

Extends tests/copy-keys.bats with two assertions: with -n my-ns every
recorded kubectl invocation carries '-n my-ns'; without -n no
invocation carries a stray namespace flag.
@scealiontach scealiontach marked this pull request as ready for review May 13, 2026 06:05
@scealiontach scealiontach merged commit 7dbf292 into main May 13, 2026
3 checks passed
@scealiontach scealiontach deleted the sprint/2026-05-12-sprint-loop-41 branch May 13, 2026 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant