mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-09 02:08:19 +01:00
fix: pre-landing review fixes (pass 2)
The cmd_pack EXIT trap bakes the temp path in: a set -e failure unwinds the function before the trap runs and a local $tmp is unset there, so the old form leaked the snapshot. The leak test creates TMPDIR where the pack reads it and asserts the failure is the missing index. CHANGELOG examples no longer spell a full national number. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
1f77181596
commit
f1635d0f63
3 files changed
+6
-4
No files matched your search
+2
-2
@@ -11,14 +11,14 @@
|
||||
- **`docs/skills.md` `/ship` "The PR body"**: the four Capy sections in order (What changed, Why, Blast radius, Approvals), how to write the Blast radius item, and what `gstack-prose-lint` gates. `captain/templates/merge-ask.md` carries `## Blast radius` between Why and Approvals; `bin/gstack-pr-shots`'s example caption and the `capy-pr` section heading name it too.
|
||||
|
||||
### Fixed
|
||||
- **`lib/redact-patterns.ts` `isCiRunIdentifier`**: an 11-digit GitHub Actions run id right after a run word on the same line (`screen 40477638211 GREEN`, `gate=…`, `rerun passed …`, `run #…`) is not `pii.phone.e164`, in every scanner (`gstack-approvals add`, `gstack-decision-trail`, the pregate secrets check, `gstack-post`); the run-word regex moved there from `lib/gstack-post.ts`, which imports it for its chained-id allowlist. A 10-digit national number after a run word, an 11-digit number with a leading 1 (`failed 14155551234` is a US or Chinese number with its trunk prefix; run ids passed 20 billion in 2025), a bare 11-digit number and `job`, `id`, `workflow` or `attempt` before one still report.
|
||||
- **`lib/redact-patterns.ts` `isCiRunIdentifier`**: an 11-digit GitHub Actions run id right after a run word on the same line (`screen 40477638211 GREEN`, `gate=…`, `rerun passed …`, `run #…`) is not `pii.phone.e164`, in every scanner (`gstack-approvals add`, `gstack-decision-trail`, the pregate secrets check, `gstack-post`); the run-word regex moved there from `lib/gstack-post.ts`, which imports it for its chained-id allowlist. A 10-digit national number after a run word, an 11-digit number with a leading 1 (`failed 1415…` is a US or Chinese number with its trunk prefix; run ids passed 20 billion in 2025), a bare 11-digit number and `job`, `id`, `workflow` or `attempt` before one still report.
|
||||
- **`scripts/ubicloud/ubi-runner.sh pack`**: the live `.git` is never tarred. The refs are fetched into a fresh repository through git's transport (`--update-shallow`, so a shallow CI or Capy checkout packs), the `remote.*` and `branch.*` config is replayed so `origin/<base>` and each branch's upstream resolve on the VM, a detached HEAD gets a `detached-<sha12>` branch, the index is copied as before, and a pack that fails removes its temporary repository; a review subagent's `git diff` or a trial merge's `worktree add` during the first minute of a remote gate no longer makes tar exit 1 (`PREGATE_REMOTE_INFRA`). The linked-worktree path uses the same snapshot.
|
||||
- **`lib/pregate/import-graph.ts`**: a relative string literal (`'../ship/sections/pr-body.md.tmpl'` through `new URL(…, import.meta.url)` or `readFileSync`) is a dependency edge from the file that names it, like a relative import; `test/ship-publication-gates.test.ts` is now selected when the pr-body template changes (the remote free gate missed it in GSTA-121's ship). A literal that climbs out of the repository names nothing.
|
||||
- **`scripts/lib/windows-curation.ts`**: `test/pregate-remote-stamp-only.test.ts` is `KNOWN_WINDOWS_INCOMPATIBLE` (it expects the Linux-host lanes verdict and skips itself on win32), as #3485's follow-up asked.
|
||||
|
||||
### Tests
|
||||
- `test/paid-free-boundary.test.ts`: release metadata alone and beside `CHANGELOG.md` keeps mode `pr` with no paid case; `ETHOS.md` with `NOTICE.md` selects the two skills' gate cases and judge and nothing else. `test/pr-dependencies.test.ts`, `test/paid-pr-profile.test.ts`, `test/exercised-map.test.ts`: `SECURITY.md` as the unregistered example.
|
||||
- `test/redact-context-exemptions.test.ts`: six run-word rows with run id 40477638211 and eight controls; `test/approval-record.test.ts`: an approval whose words cite `screen 40477638211 GREEN` is recorded verbatim, a phone number still refuses the row; `test/pregate-allowlist.test.ts`: its run-id fixture reads `job <id>`, which the engine does not exempt, so the allowlist mechanism it tests still has a finding to silence.
|
||||
- `test/redact-context-exemptions.test.ts`: six run-word rows with run 40477638211 and eight controls; `test/approval-record.test.ts`: an approval whose words cite `screen 40477638211 GREEN` is recorded verbatim, a phone number still refuses the row; `test/pregate-allowlist.test.ts`: its run-id fixture reads `job <id>`, which the engine does not exempt, so the allowlist mechanism it tests still has a finding to silence.
|
||||
- `test/ubicloud-runner.test.ts`: pack of a plain checkout under twenty seconds of index, worktree and ref churn, three times; the unpacked snapshot has the same HEAD, `origin/main`, remote URL, status and `gstack-wtree` fingerprint, a detached HEAD lands on `detached-<sha12>`, a pack with no index exits 1 and leaves nothing in `TMPDIR`, and a branch's upstream survives the snapshot.
|
||||
- `test/pregate-import-graph-paths.test.ts`: relative literals through `new URL` and `readFileSync`, one and two directories up, and a literal that escapes the repository.
|
||||
|
||||
|
||||
@@ -607,7 +607,8 @@ cmd_pack() {
|
||||
head=$(git -C "$src" rev-parse --verify -q HEAD) || die "pack: $src has no commits; the VM needs a HEAD to fingerprint"
|
||||
branch=$(git -C "$src" symbolic-ref -q --short HEAD || true)
|
||||
tmp=$(mktemp -d)
|
||||
trap 'rm -rf "$tmp"' EXIT
|
||||
# The path is baked in: a set -e failure unwinds the function before the trap runs, and a local $tmp is unset there.
|
||||
trap "rm -rf $(printf %q "$tmp")" EXIT
|
||||
git init -q --bare "$tmp/.git"
|
||||
git -C "$tmp/.git" fetch -q --no-tags --update-shallow "$src" '+refs/*:refs/*' \
|
||||
|| die "pack: could not snapshot the refs of $src"
|
||||
|
||||
@@ -334,8 +334,9 @@ test('pack ships a linked worktree with a self-contained .git on the checked-out
|
||||
// The snapshot carries no hook path or other local config from the source checkout.
|
||||
expect(sh('git config --local --list', vm)).not.toContain('hooksPath');
|
||||
// A failed pack leaves no object copy behind in TMPDIR, and a branch's upstream rides along with its remote.
|
||||
sh('mkdir tmpd && git checkout -q -b topic && git branch -q --set-upstream-to=origin/main topic', src);
|
||||
sh('mkdir ../tmpd && git checkout -q -b topic && git branch -q --set-upstream-to=origin/main topic', src);
|
||||
const broken = Bun.spawnSync(['bash', '-c', `rm -f .git/index && TMPDIR=../tmpd ${pack} > ../pack-broken.tgz; echo rc=$?; ls ../tmpd`], { cwd: src, timeout: 60_000 });
|
||||
expect(broken.stderr.toString()).toContain('.git/index');
|
||||
expect(broken.stdout.toString().trim()).toBe('rc=1');
|
||||
sh('git reset -q', src);
|
||||
sh('mkdir vm2 && ' + pack.replace('pack .', 'pack src') + ' | tar -xzf - -C vm2', d);
|
||||
|
||||
Reference in new issue
Block a user