Skip to content

Commit e2f32fa

Browse files
nghiatruongtpsiam-truongtrungnghia
authored andcommitted
fix: restore test isolation, and finish the audit's group B
Three tests edited files inside the repository — adapters/nextjs/adapter.env and common/.editorconfig — backing them up and restoring after. Sequentially that was safe. Once the suites ran with --jobs they raced the tests that read those same files, and one left ADAPTER_TIER="Z" in a tracked file. I had stated that --jobs changed no isolation because bats gives each test its own process and BATS_TEST_TMPDIR; that is true of temp directories, and these tests never worked in theirs. copy_toolbox gives a test that must modify the toolbox its own copy. Verified by comparing the working tree before and after a run — it is now unchanged, which is the claim I should have measured the first time rather than asserted. Also in this commit: - two php apps each keep their own pre-commit hook. Both fragments define , and the merge is key-wise, so one app's php went unformatted. - install.sh's mv is checked: unchecked, it returned 0 through the trap, so a failed write reported success and left the generated password on disk. - install.sh's cleanup trap now bakes its path in and names the signals. A plain EXIT trap on a local does neither, and a Ctrl-C mid-download left the password file behind — which the comment beside it claimed it did not. - the moving-tag suppression comments and ADR-0005 now state what the tag costs: a write channel into every generated project that outlives handover and revoked access, carrying contents: write and the release app's key. ADR-0005 previously mitigated only the accidental case, with a smoke suite that cannot detect a deliberate one.
1 parent c5249f8 commit e2f32fa

12 files changed

Lines changed: 133 additions & 39 deletions

File tree

‎common/.github/workflows/build.yml‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,18 @@ jobs:
1414
permissions:
1515
contents: read
1616
packages: write
17-
# This account's own reusable workflow, tracked by a moving major tag on
18-
# purpose (ADR-0005) so one fix reaches every project at once. The
19-
# third-party actions inside it are still sha-pinned; the trade-off is
20-
# scoped to a repository this account controls.
17+
# A moving major tag, on purpose (ADR-0005): one fix reaches every project
18+
# at once, without a pull request into each. The third-party actions inside
19+
# are sha-pinned.
20+
#
21+
# What that costs, stated plainly because the tag outlives the engagement:
22+
# this is a write channel into this repository's CI that survives handover
23+
# and any revoked access, and release.yml grants the called workflow
24+
# contents: write, packages: write and the release app's private key. One
25+
# force-push of the tag reaches every project generated from this toolbox
26+
# simultaneously, with no review and no signal here. Accepted, not solved —
27+
# sha-pinning with Renovate is the alternative, and it gives up the
28+
# one-fix-reaches-everything the tag was chosen for.
2129
uses: you/.github/.github/workflows/app-build.yml@v1 # zizmor: ignore[unpinned-uses,ref-confusion]
2230
with:
2331
image: ghcr.io/you/@PROJECT_NAME@

‎common/.github/workflows/ci.yml‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,18 @@ jobs:
1515
permissions:
1616
contents: read
1717
pull-requests: read
18-
# This account's own reusable workflow, tracked by a moving major tag on
19-
# purpose (ADR-0005) so one fix reaches every project at once. The
20-
# third-party actions inside it are still sha-pinned; the trade-off is
21-
# scoped to a repository this account controls.
18+
# A moving major tag, on purpose (ADR-0005): one fix reaches every project
19+
# at once, without a pull request into each. The third-party actions inside
20+
# are sha-pinned.
21+
#
22+
# What that costs, stated plainly because the tag outlives the engagement:
23+
# this is a write channel into this repository's CI that survives handover
24+
# and any revoked access, and release.yml grants the called workflow
25+
# contents: write, packages: write and the release app's private key. One
26+
# force-push of the tag reaches every project generated from this toolbox
27+
# simultaneously, with no review and no signal here. Accepted, not solved —
28+
# sha-pinning with Renovate is the alternative, and it gives up the
29+
# one-fix-reaches-everything the tag was chosen for.
2230
uses: you/.github/.github/workflows/app-ci.yml@v1 # zizmor: ignore[unpinned-uses,ref-confusion]
2331
with:
2432
roots: '["docs"]'

‎common/.github/workflows/docs.yml‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,16 @@ jobs:
1111
# the union of what app-docs's own jobs ask for, and no more.
1212
permissions:
1313
contents: read
14-
# This account's own reusable workflow, tracked by a moving major tag on
15-
# purpose (ADR-0005) so one fix reaches every project at once. The
16-
# third-party actions inside it are still sha-pinned; the trade-off is
17-
# scoped to a repository this account controls.
14+
# A moving major tag, on purpose (ADR-0005): one fix reaches every project
15+
# at once, without a pull request into each. The third-party actions inside
16+
# are sha-pinned.
17+
#
18+
# What that costs, stated plainly because the tag outlives the engagement:
19+
# this is a write channel into this repository's CI that survives handover
20+
# and any revoked access, and release.yml grants the called workflow
21+
# contents: write, packages: write and the release app's private key. One
22+
# force-push of the tag reaches every project generated from this toolbox
23+
# simultaneously, with no review and no signal here. Accepted, not solved —
24+
# sha-pinning with Renovate is the alternative, and it gives up the
25+
# one-fix-reaches-everything the tag was chosen for.
1826
uses: you/.github/.github/workflows/app-docs.yml@v1 # zizmor: ignore[unpinned-uses,ref-confusion]

‎common/.github/workflows/release.yml‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,18 @@ jobs:
1616
issues: write
1717
packages: write
1818
pull-requests: write
19-
# This account's own reusable workflow, tracked by a moving major tag on
20-
# purpose (ADR-0005) so one fix reaches every project at once. The
21-
# third-party actions inside it are still sha-pinned; the trade-off is
22-
# scoped to a repository this account controls.
19+
# A moving major tag, on purpose (ADR-0005): one fix reaches every project
20+
# at once, without a pull request into each. The third-party actions inside
21+
# are sha-pinned.
22+
#
23+
# What that costs, stated plainly because the tag outlives the engagement:
24+
# this is a write channel into this repository's CI that survives handover
25+
# and any revoked access, and release.yml grants the called workflow
26+
# contents: write, packages: write and the release app's private key. One
27+
# force-push of the tag reaches every project generated from this toolbox
28+
# simultaneously, with no review and no signal here. Accepted, not solved —
29+
# sha-pinning with Renovate is the alternative, and it gives up the
30+
# one-fix-reaches-everything the tag was chosen for.
2331
uses: you/.github/.github/workflows/app-release.yml@v1 # zizmor: ignore[unpinned-uses,ref-confusion]
2432
# release please opens its pull request as a github app when the repository
2533
# has RELEASE_APP_ID and RELEASE_APP_PRIVATE_KEY, so the checks on that pull

‎common/.github/workflows/security.yml‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,16 @@ jobs:
1414
permissions:
1515
contents: read
1616
security-events: write
17-
# This account's own reusable workflow, tracked by a moving major tag on
18-
# purpose (ADR-0005) so one fix reaches every project at once. The
19-
# third-party actions inside it are still sha-pinned; the trade-off is
20-
# scoped to a repository this account controls.
17+
# A moving major tag, on purpose (ADR-0005): one fix reaches every project
18+
# at once, without a pull request into each. The third-party actions inside
19+
# are sha-pinned.
20+
#
21+
# What that costs, stated plainly because the tag outlives the engagement:
22+
# this is a write channel into this repository's CI that survives handover
23+
# and any revoked access, and release.yml grants the called workflow
24+
# contents: write, packages: write and the release app's private key. One
25+
# force-push of the tag reaches every project generated from this toolbox
26+
# simultaneously, with no review and no signal here. Accepted, not solved —
27+
# sha-pinning with Renovate is the alternative, and it gives up the
28+
# one-fix-reaches-everything the tag was chosen for.
2129
uses: you/.github/.github/workflows/app-security.yml@v1 # zizmor: ignore[unpinned-uses,ref-confusion]

‎common/install.sh‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,19 +50,32 @@ download_release_assets() {
5050
echo "downloading example.env..."
5151
local tmp_env
5252
tmp_env="$(mktemp ./.env.XXXXXX)" || return 1
53-
trap 'rm -f "$tmp_env"' EXIT
53+
# Two changes from the obvious `trap 'rm -f "$tmp_env"' EXIT`, both needed
54+
# before a Ctrl-C stopped leaving the generated password on disk: the path is
55+
# baked in with printf %q, because bash unwinds function locals before
56+
# running the trap; and the signals are named, because a plain EXIT trap does
57+
# not run when one kills the shell.
58+
# shellcheck disable=SC2064 # expanding now is the point
59+
trap "rm -f $(printf '%q' "$tmp_env")" EXIT INT TERM HUP
5460
if ! curl -fsSL "${RepoUrl}/example.env" -o "$tmp_env"; then
55-
trap - EXIT
61+
trap - EXIT INT TERM HUP
5662
rm -f "$tmp_env"
5763
return 1
5864
fi
5965
if ! generate_database_password "$tmp_env"; then
60-
trap - EXIT
66+
trap - EXIT INT TERM HUP
6167
rm -f "$tmp_env"
6268
return 1
6369
fi
64-
mv "$tmp_env" ./.env
65-
trap - EXIT
70+
# checked, like every other step here: an unchecked mv returns 0 through the
71+
# trap below, so a failure reported success and left the password file behind.
72+
if ! mv "$tmp_env" ./.env; then
73+
rm -f "$tmp_env"
74+
trap - EXIT INT TERM HUP
75+
echo "could not write .env" >&2
76+
return 1
77+
fi
78+
trap - EXIT INT TERM HUP
6679
}
6780

6881
# Fails hard if the substitution misses: a password silently staying

‎docs/decisions/0005-share-ci-through-reusable-workflows.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,15 @@ commit or a fixed minor version.
2828
Mitigated by running the smoke suite against the workflow repository's
2929
`main` before moving `v1` to point at it, and by rolling back with a tag
3030
move rather than a revert-and-redeploy per project.
31+
- The blast radius of a *deliberate* change is the same, and the smoke suite
32+
does not detect one. `v1` is a write channel into every generated project's
33+
CI that outlives the engagement: it keeps working after handover, after
34+
collaborator access is revoked, and after the client stops working with the
35+
account that owns it. `app-release.yml` grants the called workflow
36+
`contents: write`, `packages: write` and the release app's private key, so
37+
whoever controls the tag controls those in every project at once. Accepted
38+
rather than solved. SHA-pinning each call site with Renovate bumping them is
39+
the alternative, and it gives up the property this decision was made for.
3140
- A generated project's own `.github/workflows/` carries no logic to fix
3241
later — the entire reason for the split. Any logic that did end up there
3342
would need a per-project patch to change, which is exactly what this

‎lib/adapter.sh‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,13 @@ merge_lefthook_fragment() {
5454
rendered="$(mktemp)"
5555
sed "s|@APP_ROOT@|${rel}/|g" "$fragment" > "$rendered"
5656

57+
# Suffix every command name with the app it came from. The merge below is
58+
# key-wise, so two apps of the same language — both laravel adapters define
59+
# `pint` — would otherwise leave one hook scoped to whichever was applied
60+
# last, and the other app's code unformatted on commit, silently.
61+
yq --inplace "(.. | select(has(\"commands\")) | .commands) |=
62+
with_entries(.key |= . + \"-${rel//\//-}\")" "$rendered"
63+
5764
# cleaned up on both paths: under `set -e` a yq failure leaves immediately
5865
# and the file survives the run. Not a RETURN trap — that fires again in
5966
# callers, where $rendered is out of scope and the shell aborts.

‎tests/helpers/setup.bash‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,3 +27,21 @@ assert_ok() {
2727
false
2828
}
2929
}
30+
31+
# copy_toolbox — a private copy of the toolbox for a test that has to modify
32+
# it. Two tests need to see how the scripts behave against a broken adapter or
33+
# a drifted file; editing the real tree made them race each other once the
34+
# suites started running with --jobs, and one left ADAPTER_TIER="Z" behind in
35+
# a tracked file. The scripts resolve their own root from their location, so
36+
# running them out of the copy is enough.
37+
copy_toolbox() {
38+
local dest="${BATS_TEST_TMPDIR}/toolbox"
39+
mkdir -p "$dest"
40+
cp -R "${SCAFFOLD_ROOT}/adapters" "${SCAFFOLD_ROOT}/lib" \
41+
"${SCAFFOLD_ROOT}/scripts" "${SCAFFOLD_ROOT}/common" \
42+
"${SCAFFOLD_ROOT}/scaffold" "$dest/"
43+
cp "${SCAFFOLD_ROOT}/UPSTREAM" "$dest/" 2>/dev/null || true
44+
mkdir -p "$dest/docs"
45+
cp "${SCAFFOLD_ROOT}/docs/PROVENANCE.md" "$dest/docs/" 2>/dev/null || true
46+
printf '%s' "$dest"
47+
}

‎tests/new-laravel-api.bats‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,3 +93,15 @@ teardown() {
9393
run mise run //apps/worker:install
9494
assert_ok
9595
}
96+
97+
@test "two php apps each keep their own pint hook" {
98+
scaffold new "$PROJECT" --api laravel-api --app laravel-inertia
99+
# Both fragments define pre-commit.commands.pint, and yq's merge is key-wise,
100+
# so the second overwrote the first — one hook survived, scoped to one app,
101+
# and the other app's php was never formatted on commit. Silently.
102+
run yq -r '.pre-commit.commands | keys | .[]' "${PROJECT}/lefthook.yml"
103+
assert_ok
104+
local hooks
105+
hooks="$(printf '%s\n' "$output" | grep -c pint)"
106+
[ "$hooks" -eq 2 ] || { echo "expected 2 pint hooks, found ${hooks}:"; echo "$output"; false; }
107+
}

0 commit comments

Comments
 (0)