ci: allow-list completeness lint + two-phase service startup

[tomassrnka]

Two follow-ups from #3479, one commit each (+ review fixes).

Allow-list completeness lint. select-tests.sh rejects stale allow-list entries; nothing caught the missing direction — a new snapshot-path test nobody lists silently never runs compressed. check-allowlist-completeness.sh (wired into the existing check-tests-allowlist CI step) requires every test touching a snapshot-lifecycle symbol to be listed or carry // compression-tests:excluded <reason>. Package-local wrappers must join its symbol list, so tests can't hide behind helpers. Running it on main found 6 rotted-out tests (cross-team fork/resume/connect, envd CA-cert fs-only reboot) — added; 15 fixture-only tests got exclusion markers. Grey call for review: the two TestEgressFirewall*PersistsAfterResume are marked excluded per the tsv's egress exclusion, but read arguably as snapshot-integrity — flip if you disagree.

Two-phase service startup. start-service.sh gains start/wait subcommands (legacy form intact) and prints the log tail on health-wait timeout (was diagnostics-free). Boot order: otel ∥ orchestrator, then API ∥ client-proxy. Measured honestly: no wall-time win (~2s; the step is dominated by migrations/seed) — the value is the timeout diagnostics; happy to drop the reordering if you prefer minimal diff.

Verified: partition proof 227 tests exactly once; lint red/green paths CI-tested; marker form survives the repo formatter.

🤖 Generated with Claude Code

[cursor]

PR Summary

Low Risk
Changes are CI scripts, integration test metadata, and startup ordering/diagnostics; no production auth, data, or payment paths.

Overview
This PR closes the gap where new snapshot-path integration tests could silently skip compressed CI runs: a completeness lint now requires each such test to be allow-listed or marked // compression-tests:excluded, and the check runs in the existing allow-list CI step. Running it on main recovered six tests that had rotted out of compression-tests.tsv and documents fifteen fixture-only tests with exclusion markers.

start-service.sh adds separate start and wait modes (legacy one-shot behavior remains) and dumps the last 30 log lines when health polling times out. CI service boot starts otel and orchestrator together, waits for orchestrator then otel, then starts API and client-proxy in parallel and waits on both—preserving the rule that API does not start before orchestrator is healthy.

^{Reviewed by Cursor Bugbot for commit 6a836ed. Bugbot is set up for automated code reviews on this repo. Configure here.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

[tomassrnka]

Measured results (run 30693622934, all green)

Commit 1 (allow-list lint): validated. The lint runs in every integration shard via check-tests-allowlist, passes on the fixed tree, and fails correctly when a marker is removed. One interaction found the hard way: the repo formatter rewrites the marker to // compression-tests:excluded (hyphen ≠ machine-directive pattern), so the lint accepts the formatted form — that's the third commit.

Commit 2 (two-phase startup): no measured wall win — being honest about it. Start Services came in at 40–46s vs 42–48s baseline (~2s, within noise). The step's cost is dominated by DB health-wait + migrations + seed, not the four service boots — prebuilt binaries turn healthy in 1–2 polls, so the serial waits I parallelized were already near-free. What the commit still delivers: the log tail on health-wait timeout (previously a timeout produced zero diagnostics) and the start/wait split for anyone restructuring later. If you'd rather keep the diff minimal, I can drop the two-phase reordering and keep only the script's subcommands + failure diagnostics — say the word.

Unrelated to this PR but visible in the run: shards queued 2.1–6.2 min because three pushes in ~10 min stacked runs against the runner group's 24-slot ceiling (wall 17.3 min despite healthy shard durations). Same story as the #3479 measurements — the max-concurrency raise is worth doing.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 09b14cab8a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Detect auto-pause calls regardless of formatting

The completeness check only recognizes the exact single-line text WithAutoPause(true) or WithAutoResume(true). Go permits and gofmt preserves multiline calls such as WithAutoPause( true, ) across lines, and tests can also set these fields through variables, so a new snapshot-path test using either form and no explicit pause call passes this lint while remaining absent from compression-tests.tsv; the compressed jobs in .github/workflows/integration_tests.yml then silently skip it. Parse the Go syntax or otherwise match these calls independently of whitespace and literal spelling.

Useful? React with 👍 / 👎.

[claude]

LGTM — approving despite the inline nit; it flags a latent robustness gap in the new lint script that isn't reachable by anything in the current test tree.

Extended reasoning...

Overview

This PR touches only CI tooling: a new bash lint script (check-allowlist-completeness.sh) that checks the compression test allow-list for missing entries, wiring it into the existing make check-tests-allowlist target, a two-phase parallel service-startup refactor of start-service.sh/start-services/action.yml, and a batch of test-file comment annotations (// compression-tests:excluded ...) plus allow-list additions in compression-tests.tsv. No production service code (API, orchestrator, envd, client-proxy) is modified.

Security risks

None. This is CI-only tooling operating on the repository's own test tree; it does not touch auth, crypto, secrets, or any code path that runs in production. The new script only reads test files and the tsv allow-list to emit lint failures.

Level of scrutiny

Low-to-moderate is appropriate: it's a new bash/awk lint plus a CI orchestration reshuffle, not business logic. The only inline finding is nit-severity and explicitly scoped to a shape of Go code (receiver-method helpers / package-level func-literal vars on the snapshot path) that doesn't exist anywhere in the current test tree, so it cannot cause a false negative today — it's a robustness gap for future contributors, not a live bug. I also confirmed the previously-flagged cursor[bot] 'prefix false match' issue is fixed in the current script (uses grep -qxF for exact match).

Other factors

The author already ran this in CI (linked run, all green) and was transparent that the two-phase startup change measured no real wall-clock win, keeping it anyway for the timeout diagnostics and start/wait split. The test-annotation changes are mechanical (adding comments, no behavior change) and the tsv additions are straightforward allow-list entries. Given the nit is well-scoped and the blast radius is CI tooling only, this doesn't need to block on human review.

[claude]

🟡 The awk in the completeness-lint's scan loop (check-allowlist-completeness.sh:60-81) tracks the enclosing function name (fn) incorrectly in two ways: it never resets fn when a function body ends, so a symbol hit on any line between two top-level functions gets misattributed to the previous function instead of being recognized as outside any function; and for a receiver method like func (h *helper) pauseViaMethod(...), the name-extraction logic strips the token down to an empty string, which both drops that method's own symbol hits and can reset the misattribution state early. No test in the current tree triggers either path, so this is a latent robustness gap rather than a live false negative today.

Extended reasoning...

What the bug is. The scan-loop awk derives the enclosing function name (fn) for attributing snapshot-symbol hits with two related defects:

1. fn is set only by the /^func / rule (fn = $2; sub(/\(.*/, "", fn)) and is never reset when a function body ends — the only other rule that fires on non-comment lines clears cbuf (the exclusion-comment buffer), not fn. So a snapshot-symbol occurrence on any line between the end of one function and the start of the next (a package-level var/const initializer, a package-level func-literal var, etc.) inherits whatever fn was last set to.
2. For a receiver method (func (h *helper) pauseViaMethod(t *testing.T) {), awk's whitespace field split makes $2 the token (h, and sub(/\(.*/, "", fn) strips from the first ( onward, leaving fn="". Every subsequent line inside that method's body then fails the fn != "" guard on the symbol-matching rule, so hits inside the method are dropped entirely rather than attributed anywhere.

Code path. Both are visible directly in the awk invocation inside the per-file loop (~lines 60-81): the /^func / block is the sole writer of fn, and the symbol-match block downstream ($0 ~ symre && fn != "" && !reported[fn]) is the sole reader. Nothing in between resets fn on function exit, and nothing guards against the receiver-method field-split producing an empty name.

Why nothing today prevents it. The script's own design assumes every top-level declaration is a plain func name(...) — which is what every current helper (pauseSandbox, createSnapshotTemplate, etc.) and every current test looks like. Receiver methods and package-scope func-literal vars are valid, unremarkable Go constructs, but the parser has no fallback for them.

Impact. Two failure modes, both undermining the lint's stated purpose ("cannot rot into silently reduced coverage"):

• Misattribution (issue 1): a hit between functions is charged to the wrong (earlier) function. In the common case this produces a loud, confusing false-positive failure against an innocent function. In the worse case — a genuine new snapshot-path helper defined as a package-level func-literal var — its hits get attributed to an already-reported earlier function and the real helper never gets forced into the SYMBOLS/allow-list net.
• Dropped hits (issue 2): a snapshot-symbol call inside a receiver-method helper is invisible to the lint entirely — it's neither flagged as an unlisted helper nor attributed to any calling test, so a test that only exercises the pause path through such a helper passes the lint with a clean bill of health.

Step-by-step proof (issue 1, misattribution). Given a test file:

func helperNoSymbol(t *testing.T) {}

var globalThing = PostSandboxesSandboxIDPauseWithResponse

• Line 1 matches /^func /: fn becomes helperNoSymbol, fline=1.
• Line 2 (blank) and line 3 (var globalThing = ...) don't match /^func / or /^\/\//, so only cbuf is cleared — fn is untouched.
• Line 3 matches symre (contains PostSandboxesSandboxIDPauseWithResponse) and fn is still "helperNoSymbol", so the tool prints helperNoSymbol 1 -, attributing a package-scope symbol usage to a function that never uses it.

Step-by-step proof (issue 2, dropped hit). Given:

func (h *helper) pauseViaMethod(t *testing.T) {
    h.client.PostSandboxesSandboxIDPauseWithResponse(...)
}

func TestSomething(t *testing.T) {
    h.pauseViaMethod(t)
}

• On the receiver-method line, $2 is (h; sub(/\(.*/, "", fn) strips everything from the first (, leaving fn="".
• The body line touching PostSandboxesSandboxIDPauseWithResponse fails the fn != "" guard and produces no output at all.
• pauseViaMethod is never forced into SYMBOLS, and TestSomething — which exercises the pause path only via this method — is never flagged as needing an allow-list entry or exclusion marker. The lint exits 0.

How to fix. Track function scope robustly, e.g. reset fn="" on encountering a top-level (column-0, non-comment) line that isn't itself a /^func / match after the function body plausibly ends (or track brace depth), and extract the function/method name using a pattern that skips an optional (receiver *Type) clause (e.g. splitting on the last func ... ) name( segment or a dedicated regex) instead of blindly taking $2.

Severity assessment. All verifiers independently confirmed both mechanisms by tracing/reproducing the awk logic, and all converged on nit: every existing helper and test in the tree is a plain top-level func name(...), so the primary guarantee (a new TestXxx on the snapshot path must be listed or excluded) holds today — Go test functions can't be receiver methods or func-literal vars, so the exact "test invisibility" failure mode is only reachable through unusual package-local helper idioms that don't currently exist in this codebase. This is a real latent gap in newly-added CI tooling worth fixing for robustness, but it doesn't cause any incorrect behavior on the tree as it stands and shouldn't block this PR.

[tomassrnka]

Both bot findings were real and are fixed in 0036a31: receiver-method name mangling + missing function-end reset (claude), and multiline/variable-arg autopause forms (codex). Each was reproduced with a probe file before fixing; the probe now trips all three paths and the tree is green without it.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 0036a31. Configure here.}

[tomassrnka]

cursor's Low was right — my variable-arg alternative matched the f in false. Fixed in 97937ed: the scan now strips literal WithAutoPause(false)/WithAutoResume(false) from a line before matching, so any other argument shape flags and only the explicit opt-out stays quiet. Probe-verified: false-only test quiet, true/variable-arg tests flagged.