From 1f77181596c490cb26b72ce8e617d07e7c7ba8a6 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 27 Oct 2026 11:17:33 +0000 Subject: [PATCH] fix: pre-landing review fixes Red Team pass 1 (all below the floor): an 11-digit span with a leading 1 after a run word is a US or Chinese national number, not a run id, and reports again; ubi-runner pack removes its temporary repository when it fails and replays branch.* with remote.* so a branch's upstream resolves on the VM; the conservative-refusal example in docs/review-policy.md reads in the refusing direction; the touchfiles header names why ETHOS.md sits only under the two prompts that quote it. Step 6's selection found captain/templates/merge-ask.md as the one unknown dependency of this PR and registered captain/templates/** in FREE_ONLY_PR_FILES (#3499). Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- docs/review-policy.md | 2 +- lib/redact-patterns.ts | 7 +++++-- scripts/test-pr-profile.ts | 1 + scripts/ubicloud/ubi-runner.sh | 9 ++++++--- test/helpers/touchfiles-data.ts | 4 ++++ test/paid-free-boundary.test.ts | 2 +- test/redact-context-exemptions.test.ts | 6 +++++- test/ubicloud-runner.test.ts | 7 +++++++ 8 files changed, 30 insertions(+), 8 deletions(-) diff --git a/docs/review-policy.md b/docs/review-policy.md index 365c7c55e..046220eb6 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -56,7 +56,7 @@ whatever class it resembles. | **own-artifact hardening** | A check missing between two things the repository's own tooling writes: a screen verdict not compared with the receipt it belongs to; a receipt schema that does not declare a key the writer records; a policy loader that defaults when the base branch's own file is invalid; a size read that follows a symlink inside the repository. | The only writer is the repository's maintainers through its tools; no caller inside the intended case reaches the gap, and no test fails. A writer outside that trust boundary makes it a bypass, and CRITICAL. | | **unwired or unread** | A helper with no caller, a field written and never read, a duplicated helper, a header comment naming a step that does not run it, a key-version constant that moved before any release carried the old value. | Nothing takes the path, so nothing breaks; it is dead code to delete or wire in its own change. | | **bounded repeat work** | A re-dispatch on every call, a redundant screen after main moves, an orphaned scratch branch, a refusal that could come one step earlier in the sequence, a settle that could wait one more check. | The operation completes, nothing is lost, and the cost is one more bounded run; uncapped spend is the sideways sweep's CRITICAL, not this class. | -| **conservative refusal** | Stale state scoped wider than its cause: a RED verdict that keeps refusing a head after main moved, a pending result seated as a result and never re-read before the merge. | A refusal costs a rerun and never passes something wrong; a wrong pass would be the floor. | +| **conservative refusal** | Stale state scoped wider than its cause: a RED verdict that keeps refusing a head after main moved, a pending result that keeps refusing after it settled. | A refusal costs a rerun and never passes something wrong; a wrong pass would be the floor. | | **artifact presentation** | A string from an artifact printed verbatim into a parsed line (whitespace, control characters); a classifier that reads a non-zero exit code beside a non-RED verdict as RED. | Display and classification of the repository's own output; it forges no parsed field. | | **test gap** | A state-matrix cell without a test, a coverage percentage, a weak or flaky test, a fixture that shares the coordinator's environment. | Category `testing`, below the floor by definition above; a gap is work for the test audit, not a defect in the diff. | diff --git a/lib/redact-patterns.ts b/lib/redact-patterns.ts index 2cdb02037..2c368f9c1 100644 --- a/lib/redact-patterns.ts +++ b/lib/redact-patterns.ts @@ -333,7 +333,10 @@ export function isNumericMetadataValue(span: string, match: RegExpExecArray): bo * receipt, a READY block or an approval record names one (`screen 40477638211 * GREEN`, `gate=40323989742`, `rerun passed 40287239206`, `run #19368712345`); * generic words (id, job, attempt, workflow) exempt nothing, since `customer id - * 14155551234` is a number. The same digits anywhere else still report. + * 14155551234` is a number, and a span with a leading 1 exempts nothing either: + * `failed 14155551234` is a US or Chinese national number with its trunk + * prefix, and Actions run ids passed 20 billion in 2025. The same digits + * anywhere else still report. */ const CI_ID_POSITION_BEFORE = /(?:--(?:run|job)(?:-id)?(?:[ \t]+|[ \t]*=[ \t]*)|\bgh[ \t]+run[ \t]+(?:view|watch|rerun|download|cancel)[ \t]+|\/actions\/runs\/|\/jobs?\/|(?:run|job)_id["']?[ \t]*[:=][ \t]*["']?)$/i; @@ -345,7 +348,7 @@ export function isCiRunIdentifier(span: string, match: RegExpExecArray): boolean const { start } = spanBounds(match); const lineStart = input.lastIndexOf("\n", start - 1) + 1; const before = input.slice(Math.max(lineStart, start - 80), start); - return CI_ID_POSITION_BEFORE.test(before) || (span.length === 11 && CI_RUN_WORD_BEFORE.test(before)); + return CI_ID_POSITION_BEFORE.test(before) || (span.length === 11 && span[0] !== "1" && CI_RUN_WORD_BEFORE.test(before)); } /** diff --git a/scripts/test-pr-profile.ts b/scripts/test-pr-profile.ts index d6b428d46..188710401 100644 --- a/scripts/test-pr-profile.ts +++ b/scripts/test-pr-profile.ts @@ -235,6 +235,7 @@ export const FREE_ONLY_PR_FILES = [ 'scripts/paid-test-durations.json', 'scripts/ubicloud/**', 'capy/skill/**', // The vendored Capy skill (Drive bootstrapper): run by Capy agents, read by no paid case; test/capy-skill-wrapper.test.ts covers it. + 'captain/templates/**', // The /captain crew templates (CREW.md, merge-ask.md, slot-call.md, …): copied into a Captain's drive by people and Captains, read by no paid case; free test/captain-skill.test.ts covers them (#3499). 'tsconfig.test.json', // Read only by `tsc -p tsconfig.test.json` (typecheck:test); Bun's runtime reads tsconfig.json. '.gitignore', // Changes which untracked files git reports, never tracked content a case reads. '.gstack/generated.json', // gstack's own regen registry: gstack-regen/gstack-pregate read it from origin/ of the repo they run in; paid cases run in fixture repos. diff --git a/scripts/ubicloud/ubi-runner.sh b/scripts/ubicloud/ubi-runner.sh index d5853668a..0a493d74c 100755 --- a/scripts/ubicloud/ubi-runner.sh +++ b/scripts/ubicloud/ubi-runner.sh @@ -597,7 +597,9 @@ cmd_down() { # origin/ still resolves, and the index is copied (git writes it # atomically through a rename). A linked worktree's .git is a one-line # pointer into the main repository, useless on the VM; the same snapshot -# replaces it, on the worktree's own branch and index. +# replaces it, on the worktree's own branch and index. Not carried: submodule +# repositories (.git/modules) and a partial clone's promisor (the fetch pulls +# every missing blob first); gstack has neither. cmd_pack() { local src tmp gitdir branch head entry src=$(cd "${1:-.}" && pwd) @@ -605,6 +607,7 @@ 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 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" @@ -617,7 +620,7 @@ cmd_pack() { gitdir=$(cd "$src" && cd "$(git rev-parse --git-common-dir)" && pwd) while IFS= read -r -d '' entry; do git -C "$tmp/.git" config --add "${entry%%$'\n'*}" "${entry#*$'\n'}" - done < <(git -C "$gitdir" config --local --null --get-regexp '^remote\.' 2>/dev/null || true) + done < <(git -C "$gitdir" config --local --null --get-regexp '^(remote|branch)\.' 2>/dev/null || true) # The checkout's index rides along so the VM's status matches, except a # split index, whose shared half lives only in the original git dir. [ "$(git -C "$src" config --bool core.splitIndex 2>/dev/null)" = true ] \ @@ -630,7 +633,7 @@ cmd_pack() { done \ | tar --null -T - -c -C "$tmp" .git -zf - ) - rm -rf "$tmp" + rm -rf "$tmp"; trap - EXIT else tar -C "$src" -czf - . fi diff --git a/test/helpers/touchfiles-data.ts b/test/helpers/touchfiles-data.ts index 378c9ff50..01cde91db 100644 --- a/test/helpers/touchfiles-data.ts +++ b/test/helpers/touchfiles-data.ts @@ -19,6 +19,10 @@ /** * E2E test touchfiles — keyed by testName (the string passed to runSkillTest). * Each test lists the file patterns that, if changed, require the test to run. + * + * ETHOS.md is registered under the /office-hours and /plan-ceo-review cases, + * whose prompts quote it; the tier>=3 preamble (Search Before Building) only + * names it as a read-on-demand pointer that no paid case asserts on (#3499). */ export const E2E_TOUCHFILES: Record = { 'ship-skipped-queued-finding': [ 'lib/outside-review-result.ts', 'lib/gate-outcomes.ts', 'bin/gstack-codex-probe','bin/gstack-state-root.sh', 'lib/state-root.ts', 'package.json', 'bun.lock', '.github/docker/Dockerfile.ci', diff --git a/test/paid-free-boundary.test.ts b/test/paid-free-boundary.test.ts index dbe3cc47b..08e3bc41e 100644 --- a/test/paid-free-boundary.test.ts +++ b/test/paid-free-boundary.test.ts @@ -114,7 +114,7 @@ describe('paid/free dependency boundary', () => { expect(result.selection).toEqual({ e2e: [], judges: [] }); } // Release metadata no paid case reads keeps the PR profile, alone and beside a mapped prompt (#3499). - for (const file of ['NOTICE.md', 'LICENSE', 'licenses/MIT-pstack.txt']) { + for (const file of ['NOTICE.md', 'LICENSE', 'licenses/MIT-pstack.txt', 'captain/templates/merge-ask.md']) { expect(fs.existsSync(path.join(ROOT, file)), file).toBe(true); const result = computePaidCaseSelection({ profile: 'pr', env: {}, changedFiles: [file, 'CHANGELOG.md'] }); expect(result.coverage?.mode, file).toBe('pr'); diff --git a/test/redact-context-exemptions.test.ts b/test/redact-context-exemptions.test.ts index 6b579d917..ff664802f 100644 --- a/test/redact-context-exemptions.test.ts +++ b/test/redact-context-exemptions.test.ts @@ -239,8 +239,12 @@ describe("pii.phone.e164: GitHub Actions run and job ids in their id positions", "https://example.com/contact/37235771700", '{"phone_id": 37235771700}', "run 4155550123 laps", - // A run word before a 10-digit national number, a bare 11-digit number and a generic word exempt nothing. + // A run word before a 10-digit national number, an 11-digit number with a leading 1 (a US or Chinese + // national number with its trunk prefix), a bare 11-digit number and a generic word exempt nothing. "gate 4155550123", + "call failed 14155551234", + "screen 13812345678 GREEN", + "run 19368712345", "the id is 40477638211", "job 40477638211", "workflow 40477638211", diff --git a/test/ubicloud-runner.test.ts b/test/ubicloud-runner.test.ts index 7802dec7d..58cfc6b4c 100644 --- a/test/ubicloud-runner.test.ts +++ b/test/ubicloud-runner.test.ts @@ -333,6 +333,13 @@ test('pack ships a linked worktree with a self-contained .git on the checked-out expect(wtree(vm)).toBe(wtree(src)); // 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); + 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.stdout.toString().trim()).toBe('rc=1'); + sh('git reset -q', src); + sh('mkdir vm2 && ' + pack.replace('pack .', 'pack src') + ' | tar -xzf - -C vm2', d); + expect(sh('git rev-parse --abbrev-ref @{u}', join(d, 'vm2')).trim()).toBe('origin/main'); } finally { rmSync(d, { recursive: true, force: true }); } }, 90_000);