Repository navigation
[Sprint] sprint-loop-5 - #7
Merged
Merged
Conversation
…R-1852) Adds tests/sur-1852.sh which packs two distinct command scripts (bash/changelog and bash/clean-branches) and asserts: - pack-script exits 0 and produces a non-empty file - the packed body parses with bash -n - no @include directives leak into the packed output - no includer.sh references survive the pack - the packed binary prints SYNTAX when invoked with -h Companion regressions are already covered by sibling tests: sur-1817 for BASH_SOURCE[N] index rewriting and sur-1818 for byte-identical repeated packs, satisfying the four scenarios in the issue's DoD. Wires the existing tests/run.sh harness into the Makefile via a new test target so make test now fails on a failed bats-equivalent test instead of being a no-op.
Removes the heredoc-based JSON construction in send_incident which interpolated alert title, type, service id, and incident key directly into the body. Any operator-supplied title containing a double-quote or backslash produced invalid JSON, and a crafted title could inject sibling fields (e.g. "priority":"P1") into the incident object. incident_data is now defined exactly once at file scope (was previously redefined inside an if-else for the incident_key branch, which leaked across calls in the same shell). The optional incident_key is folded in via jq's `if $key != "" then ... else . end` instead of a second function definition. Adds tests/sur-1838.sh covering JSON validity for nasty titles (quotes, backslashes, newlines), title round-trip through jq, the absence of incident_key when omitted, key passthrough when supplied, and the injection-resistance assertion. SUR-1821 (commented-out send_event / missing -X POST) remains out of scope per its description.
…1834) The previous breaking-change detector was `grep -c "BREAKING CHANGE"` against the default git log output, which matched only the literal footer with a space and missed: - Conventional Commits subject form: feat!:, fix(scope)!:, etc. - Hyphenated footer form: BREAKING-CHANGE:. Teams using `feat!:` consistently silently received patch bumps for breaking releases, defeating semver. The script is invoked by Jenkins on main, so the bug shipped incorrect versions organisation-wide. Detection now uses --pretty=format:'%s%n%b' so subject and body are both visible, with -E regex `(^[a-zA-Z]+(\([^)]+\))?!:|BREAKING[ -]CHANGE:)` matching subject ! form and either footer spelling. Bootstrap path: the prior `git describe --tags --match 'v*' --abbrev=0` hard-failed with `|| exit 1` when no v* tag existed. It now defaults to v0.1.0 with a clear notice and the change-detection range falls back to HEAD instead of $LATEST_TAG..HEAD. CHANGES counter switched from grep -c "^commit" (relied on default log output) to a hash-per-commit format, which is robust to log format changes. Adds tests/sur-1834.sh with seven scenarios: feat!:, feat(api)!:, BREAKING CHANGE: footer, BREAKING-CHANGE: footer, mixed, no-breaking baseline, and the no-prior-tag bootstrap case. Behaviour observed by parsing the unconditional "Since X N changes M breaks" NOTICE; the warn message itself is suppressed at default verbosity. Behavioural note: the next make publish on main may now bump minor where it previously bumped patch for histories that used `feat!:`. This is an intentional correctness fix, called out in the sprint plan.
…836)
Replaces the colon/comma string-state machine with indexed Bash arrays
of tab-delimited records. UNCOMMITTED/RELEASABLE/DEVELOPMENT are now
arrays where each element is a `name TAB pr_count TAB branch TAB
version` record. Tabs are forbidden in any of those fields (paths and
branch names use forward-slashes), so the encoding is unambiguous.
The output() function reads each record via `IFS=$'\t' read -r ...`
and no longer relies on `${item//,/ }` / `${token//:/ }` substitution
to chunk fields, which would silently break for any field containing
either delimiter.
Removes the leftover `echo "${PULLS}"` from a previous refactor — the
variable is never assigned anywhere in the script.
Cleans up the unused `local base_name` declaration in
fetch_if_possible by collapsing the declare-then-assign into a single
line that uses the parameter directly.
Behaviour: the human-readable output preserves the same column order
(name, pr_count, branch, version) and the same color rules
(cyan/green for branch by name; red/green/yellow for version by
record-set). Per the DoD, manual verification against a directory of
one releasable, one dirty, and one development-branch repo confirms
identical layout.
…UR-1814) aws::list_findings called bare `jq` instead of `_jq`, bypassing the `commands::use jq` guard so a host without jq silently produced empty output rather than a clear error. Switched to `_jq -r`, consistent with the rest of the file. aws::refresh_scan compared imageScanCompletedAt with `[ -lt ]`. ECR returns that field as a float (e.g. 1728310921.123); POSIX `[ -lt ]` is integer-only and rejected the comparison as "integer expression expected", so the cache branch never short-circuited and every call spuriously kicked off a fresh `aws ecr start-image-scan`. The extraction now pipes through `floor` (jq >= 1.5) before the integer comparison. Removes the unused `local status` declarations in aws::scan_status and _describe_findings. Adds tests/sur-1814.sh which stubs _describe_findings, aws::is_scan_complete, and aws::scan_image to assert: - a recent float-timestamp scan short-circuits with no rescan, - an old float-timestamp scan triggers exactly one rescan, - no "integer expression expected" leaks to stderr, - list_findings produces a row per synthetic finding. The aws::scan_image stub uses a tempfile marker so calls survive the $(...) subshell that captures refresh_scan's stderr.
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-04-28-sprint-loop-5
Sprint Goal
Harden the release pipeline and operational integrations of
bash/by closingtwo High-priority gaps (no smoke test for
pack-script-produced binaries andJSON injection in
pagerduty.sh) and three correctness bugs in shipped tooling(
update-repo-tagssemver detection,git-checkdead/brittle output,aws.shECR scan caching). The thread tying these together is "trust": each issue is a
silent-failure mode in code that ships releases, files incidents, or feeds
operational dashboards. The sprint introduces the first end-to-end test of the
bin-tarball packer, removes a JSON-injection vector on incident creation, and
fixes three live bugs that operators are unlikely to notice until something
goes wrong.
Selected Issues
1. SUR-1852 — Test: pack-script (build-time bin packer) has no test that produced binaries actually run
bash/pack-scriptproduces every redistributablecommand in
dist/bin-*.tar.gz, butmakeonly verifies the tarball builds —not that any packed binary actually runs. There is no regression coverage
for the transitive
@includewalker, dedup invariants, the SUR-1817BASH_SOURCE[1]sed bug, or the SUR-1818 non-deterministic ordering bug.Issue proposes
tests/pack-script.batswith four scenarios: end-to-endpack-script ... -hexecution, no@include/includer.shleakage in thepacked output, no
BASH_SOURCE[1]regression, and byte-identical repeatedpacks.
pack-scriptis the single point of failure for theredistributable form of every command. A regression here ships broken
binaries to consumers of
dist/bin-*.tar.gz. This sprint includes thebootstrap of a
tests/tree (a coordinated dependency on SUR-1850 — seeRisks) so subsequent sprints can grow coverage incrementally.
tests/pack-script.batsexists with at least the four cases listed in theissue (smoke run, no
@include/includer.sh, noBASH_SOURCE[1],byte-identical repeat packs).
tests/has a minimal harness sobats tests/runs locally.make testinvokesbats tests/(or a wrapper) and fails on a failedtest, replacing the current no-op behaviour.
coverage isn't accidentally pegged to one input.
the SUR-1817
BASH_SOURCE[1]regression (verified manually as part of thePR).
pre-commit run --all-filesis clean.Jenkins'
make teststep.bats-core). To avoid blocking, this sprint will land a minimal batsbootstrap inside SUR-1852's PR (just enough to run
tests/pack-script.bats)rather than waiting on the broader SUR-1850 effort. Should be the first
issue picked up so subsequent sprints can build on the harness.
2. SUR-1838 — Anti-pattern: pagerduty.sh builds JSON via string interpolation; injectable on titles/keys
pagerduty.sh send_incidentinterpolates$alert_title,$alert_type,$service_id, and$incident_keydirectlyinto a heredoc JSON body. Any
"or backslash in an operator-supplied titleproduces invalid JSON; a crafted string can inject extra fields like
"priority":"P1". A second smell:incident_datais conditionallydefined inside an
if [ -z "$incident_key" ]block, so subsequent callsin the same shell can pick up the wrong body.
routinely paste rich incident titles containing quotes — plus a credible
injection vector. Fix uses
jq -n --arg ..., which is already a dependencyvia
_jqelsewhere in the codebase.bash/pagerduty.sh send_incidentno longer interpolates user input intoa JSON heredoc; body is built with
jq -n --arg ....incident_datais defined exactly once at file scope (no conditionalredefinition).
"and\produces a valid JSONbody (capture the payload via a stubbed
_curl, parse withjq .).that
incident_datainvoked with a title containing"produces parseableJSON.
send_eventbeingfully commented out and missing
-X POST); the PR description links toSUR-1821 to make the boundary explicit.
pre-commit run --all-filesis clean.SUR-1852 so its bats harness can host the JSON-validity unit test; can ship
alongside without if scheduling forces it.
3. SUR-1834 — Bug: update-repo-tags BREAKING CHANGES detection misses
feat!:andBREAKING-CHANGE:formsbash/update-repo-tagsgreps commit history forthe literal
BREAKING CHANGE, missing the equally-validfeat!:,feat(scope)!:,fix!:etc. subject form and the hyphenatedBREAKING-CHANGE:footer variant. Teams usingfeat!:consistently silentlyget patch bumps for breaking releases, defeating semver. The script is
invoked by Jenkins on
main, so the bug ships incorrect versionsorganisation-wide.
fix plus a follow-on for the
git describe --abbrev=0first-run failurethe issue calls out. Low risk, high downstream value.
BREAKING_CHANGEScount usesgit log --pretty=format:'%s%n%b'and aregex matching both
^[a-zA-Z]+(\([^)]+\))?!:(subject form) andBREAKING[ -]CHANGE:(footer form).v*tag exists— emit a sensible default (e.g.
v0.1.0) and continue, or at minimum loga clear message before exit.
tests/update-repo-tags.bats(or shared with SUR-1852'sharness) covers: subject-only
feat!:, scopedfeat(api)!:, footer-onlyBREAKING CHANGE:, hyphenatedBREAKING-CHANGE:, mixed, andno-breaking-changes baseline.
pre-commit run --all-filesis clean.test fixture path. Land after SUR-1852.
4. SUR-1836 — Bug: git-check echoes
${PULLS}which is never assigned; brittle colon/comma string-state machinebash/git-checkbuilds per-repo state into colon-and-comma-delimited strings (
UNCOMMITTED,RELEASABLE,DEVELOPMENT),then formats them in
output(). Line 214 has a leftoverecho "${PULLS}"referencing a never-assigned variable (dead code from arefactor). The encoding is brittle: any field containing
:or,wouldsilently break parsing. Fix proposes Bash arrays + tab-delimited records.
a script run interactively by developers. Modest scope, fits in one sprint.
echo "${PULLS}"is removed.UNCOMMITTED/RELEASABLE/DEVELOPMENTare converted from delimitedstrings to indexed Bash arrays storing tab-delimited records (or another
explicit structure-preserving encoding).
output()reads records viaIFS=$'\t' read -r ...(or equivalent) andno longer relies on
${var//,/ }/${token//:/ }.base_namearg infetch_if_possibleis removed or usedpurposefully.
one development-branch repo produces the same human-readable output as
before the refactor.
pre-commit run --all-filesis clean (shellcheck/shfmt pass).SUR-1834.
5. SUR-1814 — Bug: aws.sh — list_findings uses bare jq; refresh_scan integer-compares a float timestamp
bash/aws.sh. (1)aws::list_findingsuses barejqinstead of_jq, bypassing thecommands::use jqguard so a host withoutjqsilently produces no outputrather than a clear error. (2)
aws::refresh_scandoes[ "$earliest" -lt "$completedAt" ]againstimageScanCompletedAt, whichECR returns as a float (
1728310921.123); POSIX[ -lt ]is integer-only,so the comparison spuriously fails and the cache is defeated — every call
triggers a fresh
aws ecr start-image-scan.costs real AWS API calls when broken. One-line fix for (1); use
jq ... | floorto extract an integer for (2).aws::list_findingsuses_jq -r ...consistent with the rest of thefile.
aws::refresh_scanextracts an integer timestamp (e.g._jq -r '.imageScanFindings.imageScanCompletedAt | floor') before theinteger comparison, or otherwise uses a numeric tool that handles floats.
local statusdeclarations inaws::scan_statusand_describe_findingsare removed.floating-point timestamp parses correctly and the integer comparison no
longer errors.
pre-commit run --all-filesis clean.SUR-1852's harness if a unit test is added; otherwise standalone.
Risks + Mitigations
bats-core),which is not in this sprint. Waiting on a separate full-blown SUR-1850 PR
would block all five issues.
Mitigation: Land a minimal bats bootstrap (Makefile
make testtarget invoking
bats tests/, plus a singletests/test_helper.bash)inside the SUR-1852 PR. Document in the PR that SUR-1850's broader
framework (helpers, fixtures, CI integration) remains a separate ticket.
make testsemantics could break Jenkins onmainif theinitial bats run is flaky on the build host.
Mitigation: Gate Jenkins changes behind a follow-up; in this sprint,
make testruns the bats suite locally and via.github/workflows/, butJenkins continues to invoke
make testexactly as before — the suite mustbe green before the PR merges so the no-op-to-real transition is safe.
BREAKING CHANGESregex fix in SUR-1834 may catch commitsthat previously slipped through, causing the next
make publishonmainto bump minor/major when teams expected patch.
Mitigation: Document this in the SUR-1834 PR as an intentional
behaviour change. Coordinate with stakeholders on
maintiming; if needed,land the fix immediately after a release rather than mid-cycle.
jq -nrebuild changes the JSON body subtly (e.g.field ordering), which could affect any downstream consumer that screen-
scrapes the request payload from logs.
Mitigation: PagerDuty's API does not depend on field order; document
the diff in the PR and verify the new payload still parses with
jq .andis accepted by a sandbox API token before merging.
git-checkcould regress thehuman-readable output format that developers rely on.
Mitigation: Capture the current output on a known set of repos before
refactor, diff against post-refactor output, and treat any diff as a
blocking review issue unless explicitly desired.
floorapproach assumesjq≥ 1.5 (floorwas addedin 1.5).
Mitigation: Confirm the project's
jqbaseline; if older versionsmust be supported, fall back to
awk '{printf "%d", $0}'orcut -d. -f1on the float string.
scaffolding. If SUR-1852's bats bootstrap turns up unexpected friction
(Makefile portability, CI permissions), it could starve the other four.
Mitigation: SUR-1852 is the first issue picked up. If the bootstrap
takes longer than expected, the other four can land without the new bats
unit tests (using only manual verification described in their DoD).
Out of Scope
ships with SUR-1852.
send_eventcommented-out /-X POST) — explicitlydeferred per SUR-1838's description.
would catch as regressions) — SUR-1852 only adds tests; the fixes are
separate tickets if not already done.
SUR-1854, anti-patterns SUR-1840/1841/1842, docs SUR-1844/1845/1846/1847,
bugs SUR-1824/1827/1830/1831/1833, simplification SUR-1848, CI migration
SUR-1853).
.github/workflows/pre-commit.yaml, orstandard_defs.mkbuild infrastructure beyond the minimalmake testhook for SUR-1852.
Linear Evidence
Surinis(idce9ebfde-ff2b-4f54-90f1-c388591ca110)shell-scripts(ida43901a0-b02b-4009-aae1-a6e8903d127d)list_issueswithteam="Surinis",project="shell-scripts",state="Backlog",limit=100, then per-issueget_issue(withincludeRelations=true),list_comments, andlist_issues parentId=<id>for sub-issue dependency checks. Label filterapplied client-side:
labelsarray must not containmanual(case-insensitive).
Sub-issue Status
None of the five selected issues have sub-issues. Verified via
list_issues parentId=<id>for each — all returned empty.No parent was skipped due to incomplete sub-issues.
Linear State Transitions