Skip to content

[Sprint] sprint-loop-43#39

Merged
scealiontach merged 4 commits into
mainfrom
sprint/2026-05-13-sprint-loop-43
May 13, 2026
Merged

[Sprint] sprint-loop-43#39
scealiontach merged 4 commits into
mainfrom
sprint/2026-05-13-sprint-loop-43

Conversation

@scealiontach
Copy link
Copy Markdown
Owner

Sprint Plan — 2026-05-13 (loop 43)

Sprint Goal

Tighten the bash/ library and command scripts by closing four well-scoped issues
that combine documentation gaps, code-clarity refactors, and one latent bug. The
sprint targets log.sh, docker.sh, update-repo-tags, find-squashable, and
several BTP-era hardcoded identifiers across the toolkit — leaving the libraries
easier to read, easier to consume from outside the original BTP context, and
better documented through their @doc blocks and --help output.

Selected Issues

1. SUR-2843 — Simplify: log.sh __log_preset_* snapshot/restore dance can be flattened

  • Description summary: bash/log.sh lines 97–111 implement caller-preset
    preservation via an eight-variable snapshot/restore dance around log::level.
    The intent (from SUR-2347) is correct but the implementation is dense,
    fragile, and tends to be either deleted or copy-pasted incorrectly. The
    refactor folds caller-override semantics directly into log::level itself
    (e.g. [ -z "${LOG_DISABLE_TRACE+x}" ] && LOG_DISABLE_TRACE=true).
  • Rationale: log.sh is sourced by virtually every script and library in
    the repo. Reducing eight-variable bookkeeping to four conditional assignments
    cuts maintenance risk and removes a documented regression vector.
  • Definition of Done:
    • log::level accepts an override-respecting mode (or applies the
      ${VAR+x} guard inline) instead of the snapshot/restore region.
    • Lines 97–111 of bash/log.sh reduced to a single, scannable block.
    • Existing semantics preserved: caller pre-sets of any LOG_DISABLE_* are
      still honored across log::level calls.
    • log::level_increase continues to re-evaluate flags as it does today.
    • tests/log.bats covers the override-preservation path (add a case if
      missing) and tests/sur-2347-*.sh (if present) still passes.
    • bats tests/log.bats and make test pass locally.
    • No regression on SUR-2347 behavior (caller pre-sets honored at source-time).
  • Dependencies / ordering: None. Schedule first — isolated to one function.

2. SUR-2840 — Anti-pattern: find-squashable overloads first_squash as both sentinel and commit SHA

  • Description summary: bash/find-squashable (lines 30–53) uses one
    variable first_squash as both a "true"/"false" sentinel and a literal
    commit SHA. Two consequences: the code reads as confusingly boolean, and a
    squash group that runs to the very last commit in the range is never
    printed because the emit branch only fires when a non-matching commit
    follows. Suggested fix introduces explicit group_open / group_first
    variables and a post-loop flush.
  • Rationale: Real correctness bug (missed-tail-group) plus readability
    improvement on a script developers run frequently when curating history.
  • Definition of Done:
    • Loop in bash/find-squashable uses separate group_open (bool) and
      group_first (SHA) variables; no overloaded sentinel.
    • Post-loop flush emits a group that extends to the last commit in range.
    • git::files_changed | cksum cached across iterations (no double call
      per commit).
    • New tests/sur-2840-find-squashable.sh or tests/find-squashable.bats
      case covers a same-files run that closes on the final commit and
      asserts it is printed.
    • make test and pre-commit run --all-files pass.
  • Dependencies / ordering: After SUR-2843. Independent of others.

3. SUR-2834 — Docs: docker::cmd SIMULATE mode and update-repo-tags GPG signing requirement undocumented

  • Description summary: Two undocumented, behavior-changing knobs.
    (1) docker::cmd in bash/docker.sh honors SIMULATE to turn every
    docker invocation into an echoed no-op, but the @doc block does not
    mention it. (2) bash/update-repo-tags line 131 uses git tag -s for
    annotated tags, requiring a configured GPG key, but neither the help text
    nor options::description mentions the prerequisite — CI runs fail with
    an opaque gpg: signing failed error.
  • Rationale: Documentation-only change with disproportionate UX payoff:
    silent simulation in production and opaque CI release failures are exactly
    the bugs that drain hours when they surface.
  • Definition of Done:
    • docker::cmd @doc block describes SIMULATE and links to
      release-images -d as the documented consumer.
    • update-repo-tags options::description notes that annotated tags are
      GPG-signed and that a signing key (or an opt-out flag) is required.
    • Optionally add a -U / no-sign opt-out flag to update-repo-tags
      (stretch goal — note in body if deferred).
    • Regenerated bashadoc output (make package) reflects the new @doc
      text for docker::cmd.
    • pre-commit run --all-files and make test pass.
  • Dependencies / ordering: After SUR-2840. Shares bash/docker.sh with
    SUR-2832, so do SUR-2834 before SUR-2832 and rebase if needed.

4. SUR-2832 — Docs: hardcoded BTP/blockchaintp identifiers undocumented and not configurable

  • Description summary: Seven distinct hardcoded BTP/blockchaintp values
    across bash/aws.sh, bash/release-images, bash/docker.sh,
    bash/review-prs, bash/git-check, and bash/pagerduty-alert silently
    specialize behavior to a historical deployment. The fix promotes each value
    to an env var (or, where applicable, an explicit pattern argument) with the
    historical default preserved, and documents the variable in the function's
    @doc block and the script's options::description.
  • Rationale: New users running the toolkit outside the BTP context get
    silent zero-output or skipped-repo bugs. Promoting the constants to
    documented, overridable env vars unlocks reuse without breaking existing
    callers.
  • Definition of Done:
    • Each of the seven sites in the description table promoted to an
      overridable env var (or pattern argument for docker::list_versions /
      docker::list_official_versions) with the historical value as the
      default.
    • Every new env var documented in the relevant @doc block and in the
      consumer command's options::description.
    • At least one bats spec covering the override behavior of one of the
      promoted env vars (e.g. AWS_SCAN_SKIP_REPOS).
    • Regenerated dist/doc-*.tar.gz via make package reflects new docs.
    • pre-commit run --all-files and make test pass.
  • Dependencies / ordering: Last in the sprint — touches bash/docker.sh
    alongside SUR-2834 (different functions, but same file). Sequence after
    SUR-2834 to minimize rebase friction.

Risks + Mitigations

  • log.sh is sourced everywhere; a regression silences logs project-wide.
    Mitigation: keep the refactor behavior-preserving, add a bats spec for the
    override semantics first (TDD), and re-run make test (bats + sur scripts)
    before opening the PR.
  • find-squashable post-loop flush risks double-printing the final group.
    Mitigation: guard the flush with $group_open; add an explicit test that
    also covers the historical no-flush path.
  • docker.sh shared between SUR-2834 and SUR-2832. Mitigation: sequence
    the two issues serially on the sprint branch, rebase between them, and run
    shellcheck/shfmt before each commit.
  • update-repo-tags GPG documentation could imply a hard prerequisite that
    changes operator expectations.
    Mitigation: phrase documentation as "annotated
    tags are GPG-signed" rather than "GPG is required"; offer an opt-out flag.
  • BTP env-var defaults could silently regress in CI if the default value
    drifts from the original literal during the refactor. Mitigation: assert each
    hardcoded value matches the env-var default in the new bats spec.
  • bashadoc regeneration could produce noisy diff churn in dist/.
    Mitigation: only commit doc tarball regeneration if make package is part
    of the normal release flow for this repo; otherwise leave generated artifacts
    to CI.
  • Pre-commit hooks (commitizen, no-commit-to-branch) reject commits on a
    malformed branch.
    Mitigation: use sprint/2026-05-13-sprint-loop-43 exactly,
    matching the enforced regex.

Out of Scope

  • SUR-2841 (Simplify standard_defs.mk). Low priority and ~250 lines of
    cross-cutting deletion; deserves its own sprint to avoid risking other CI
    paths in this one.
  • Any wider refactor of log.sh beyond the snapshot/restore region.
  • Adding a -u <keyid> flag to update-repo-tags (only -U no-sign is in
    scope, and even that is a stretch goal).
  • Re-organizing the bash/ library structure or renaming commands.
  • Touching the tests/bats submodule pin or upgrading bats-core.

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 with team=Surinis, project=shell-scripts,
    state=Backlog, limit=100; followed by get_issue with
    includeRelations=true, list_issues parentId=<id> per candidate, and
    list_comments per candidate.
  • Approx count of Backlog issues reviewed: 5.
  • 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

Parent Issue Sub-issue Sub-issue Status Eligible?
SUR-2832 (none) n/a Yes
SUR-2834 (none) n/a Yes
SUR-2840 (none) n/a Yes
SUR-2841 (none) n/a Deferred — out of scope (Low priority, large refactor)
SUR-2843 (none) n/a Yes

No parent was skipped due to active sub-issues. SUR-2841 was deferred for
sprint-scoping reasons (out of scope above), not by the sub-issue rule.

Linear State Transitions

Issue ID Previous State New State
SUR-2843 Backlog Todo
SUR-2840 Backlog Todo
SUR-2834 Backlog Todo
SUR-2832 Backlog Todo

…-2843)

Lines 97-111 of bash/log.sh wrapped log::level in an eight-variable
snapshot/restore region to preserve caller pre-sets of LOG_DISABLE_*.
Fold the same semantics into four conditional assignments guarded by
${VAR+x}: when a caller has pre-set a flag the explicit value wins,
otherwise the flag is derived from LOG_LEVEL.

log::level itself is unchanged so log::level_increase /
log::level_decrease keep recomputing the disable flags as before.
bash/find-squashable used one variable, first_squash, as both a bool
sentinel ("true"/"false") and a commit SHA. Beyond the readability
cost, the emit branch only fired when a non-matching commit followed,
so a same-files group that ran to the oldest commit in the iteration
range was silently dropped.

Split the state into group_open (bool) and group_first (SHA), cache
git::files_changed | cksum once per iteration, and flush a trailing
group after the loop so the tail case prints. Add a bats regression
that fails on the previous implementation.
…(SUR-2834)

Two undocumented, behaviour-changing knobs caught users by surprise.
docker::cmd silently echoes commands when SIMULATE is non-empty
(release-images -d sets it); the @doc block now describes the contract
and points at the release-images consumer. update-repo-tags writes
GPG-signed annotated tags via git tag -s, which fails opaquely in CI
without a signing key; the options::description now flags the
requirement and a new -U flag opts out by writing an unsigned
annotated tag instead.
Seven hardcoded blockchaintp/BTP-era identifiers across the toolkit
silently specialized behaviour to a historical deployment, surfacing
as zero-output runs or skipped repositories for anyone outside that
context. Promote each to an overridable env var or pattern argument
while keeping the historical literal as the default:

  - bash/aws.sh:        AWS_SCAN_SKIP_REPOS (default blockchaintp/busybox)
  - bash/release-images: RELEASE_IMAGES_ORG  (default blockchaintp)
  - bash/docker.sh:     docker::list_versions / list_official_versions
                        accept an explicit pattern arg, with the
                        historical BTP regex as the default and
                        DOCKER_VERSION_PATTERN / DOCKER_OFFICIAL_VERSION_PATTERN
                        env overrides
  - bash/review-prs:    REVIEW_PRS_ORGS / REVIEW_PRS_INTEREST_ORGS
  - bash/git-check:     GIT_CHECK_GH_ORGS / GIT_CHECK_BB_ORGS
  - bash/pagerduty-alert: PAGERDUTY_FROM_DEFAULT

tests/sur-2832-btp-defaults.sh locks down both contracts: each new env
var's default matches the historical literal (preventing silent drift)
and the AWS_SCAN_SKIP_REPOS override actually skips the named repos.
@scealiontach scealiontach force-pushed the sprint/2026-05-13-sprint-loop-43 branch from 9427da0 to fd1aa84 Compare May 13, 2026 08:45
@scealiontach scealiontach marked this pull request as ready for review May 13, 2026 08:50
@scealiontach scealiontach merged commit b43170a into main May 13, 2026
3 checks passed
@scealiontach scealiontach deleted the sprint/2026-05-13-sprint-loop-43 branch May 13, 2026 08:55
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