fix(security): cap bsdtar extraction size to prevent decompression bomb DoS [DEVA11Y-484]

[maunilm]

What

Adds a decompressed-size and entry-count guard to the CLI download/extract path, so a decompression bomb cannot exhaust developer or CI-runner disk.

Fixes DEVA11Y-484 (F-015, CWE-400, umbrella APPSEC-415).

Scope

This PR is deliberately narrowed to DEVA11Y-484's stated Remediation. Work that was previously in this branch — the regression suite, its CI workflow, and the Windows Expand-Archive backstop — was removed and is tracked in DEVA11Y-761, preserved on branch chore/DEVA11Y-484-followup-extraction-guard-harness. See Known gaps below.

Changes

Swift plugin — Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift

• startExtractionWatchdog polls the extraction directory every 50 ms while bsdtar runs and terminate()s it once the decompressed footprint crosses maxDecompressedBytes (200 MB) or maxArchiveEntries (10,000). A soft ceiling by design: peak disk ≈ maxBytes + (50 ms × write rate).
• A post-exit footprintExceeded re-check catches a bomb that finishes inside one poll interval.
• locateExecutable throws past 10,000 entries, per the ticket's ask.
• maxCompressedBytes (100 MB) checked against response.expectedContentLength and the downloaded file's actual size.

Attached to extractLocalArchive, which since #37 (DEVA11Y-473/474) is the single non-Windows extraction path — the archive is downloaded to a file and checksum-verified first, then extracted. The old streaming curl | bsdtar path that #37 deleted is gone, so there is no separate remote guard.

Launchers — scripts/{bash,zsh,fish}/cli.sh

• bsdtar … -O | head -c 209715200 with pipefail, so the cap is enforced by SIGPIPE, plus an explicit size assertion as a backstop.
• curl --max-filesize 104857600 plus a post-download size check (curl documents the flag as a no-op when the length is unknown).
• The cap writes to ${BINARY_PATH}.tmp and publication stays a later mv, so a rejected payload cannot truncate a previously-good cached binary — preserving fix(cli): verify downloaded CLI binary integrity before exec (DEVA11Y-473/474) #37's protection.
• Failure returns non-zero; the call site remains download_binary || exit $?, so fix(cli): verify downloaded CLI binary integrity before exec (DEVA11Y-473/474) #37's distinct exit 2 for an integrity mismatch survives.

Verification

No automated suite ships with this PR (see Known gaps), so this was verified directly against the live download endpoint:

• 27/27 assertions across bash/zsh/fish: real download exits 0 through fix(cli): verify downloaded CLI binary integrity before exec (DEVA11Y-473/474) #37's integrity check (38,017,898 B archive → 69,391,104 B binary, perms 0755); .tmp cleaned up after publish; re-run byte-identical; corrupt payload rejected; and the previously-cached binary's sha256 is unchanged after a rejected payload.
• Guard fires: with a 1 MB cap the pipeline returns non-zero at exactly the cap; with the real 200 MB cap it is clean.
• Swift guard, compiled standalone and driven against the real archive: real 200 MB cap does not flag (termStatus=0, 69,391,104 B, 1 entry); a 5 MB cap flags and SIGTERMs bsdtar mid-stream (termStatus=15), bounding disk to 36 MB of 66 MB; maxEntries=0 flags on entry count.
• Compressed cap: with a 1 MB cap curl aborts non-zero with nothing written; the real 100 MB cap passes the 38 MB archive.
• pipefail save/restore verified in both directions.
• swiftc -typecheck -parse-as-library clean; bash -n clean on all three launchers; all six .sha256 sidecars verify; self-update's own comparison matches for all three.

Headroom (CLI v1.52.1): largest platform archive is 41 MB compressed (2.4× under the 100 MB cap) and ~75.6 MB decompressed (2.65× under the 200 MB cap). The caps are duplicated in four places — they must move together when the CLI outgrows them.

Known gaps — owned, not hidden

1. No automated regression coverage ships with this PR. The suite was descoped; nothing prevents a future refactor from silently dropping the caps. DEVA11Y-761 item 1, and the highest-value follow-up.
2. Windows Expand-Archive is unguarded. No download cap enforcement mid-stream, no watchdog, no entry ceiling on that branch. Unchanged from main, but a real gap. DEVA11Y-761 item 3.
3. The Swift compressed cap rejects after the transfer, not mid-stream. URLSession.download(from:) has no byte-level hook, so an oversized archive is stopped before checksum/extract/exec but peak temporary disk during transfer is not bounded. Needs a URLSessionDownloadDelegate cancelling in didWriteData. The launchers do abort during transfer. DEVA11Y-761.
4. The launchers have no entry-count equivalent to the plugin's maxArchiveEntries. In -O mode an archive of millions of empty entries streams ~0 bytes, so head -c never fires; disk stays bounded but bsdtar still parses every entry. Open for review discussion — no cheap mechanism in -O mode.
5. Linux launcher path untested. Verified on macOS/arm64 against the live endpoint; CI has no Linux launcher job.

Note on the ticket's threat model

DEVA11Y-484 states the download has "no TLS, per scope.md:65". The URL in code is https:// and the live endpoint serves HTTPS with a 302 to https://sdk-assets.browserstack.com, which weakens the stated MitM reachability behind the AV:N / CVSS 5.3 rating. The BROWSERSTACK_A11Y_CLI_DOWNLOAD_URL override remains a genuine vector, so the fix stands — but the premise as written is inaccurate.

Refs DEVA11Y-484, DEVA11Y-761, APPSEC-415.

[Crash0v3rrid3]

Multi-agent code review — decompression-bomb guard (DEVA11Y-484)

Reviewed across security/adversarial, Swift concurrency, and shell-correctness lenses plus first-party verification against the PR head. The guard that ships is correctly implemented — but the PR description overstates the implementation on two verified counts, and I'd hold merge until those are reconciled.

What's correct (verified — no action needed)

• Swift concurrency is sound: watchdog thread lifecycle has no race/leak, terminate()→waitUntilExit()→removeItem ordering is safe, ExtractionLimitState's NSLock is correct, and forwardExit is -> Never/exit (no fall-through).
• Shell logic is correct: the bomb is genuinely caught (head -c cap → SIGPIPE → 141 via pipefail, with -ge as a backstop), legit downloads pass, and local x=$? captures the pipeline status correctly.
• scripts/fish/cli.sh is #!/usr/bin/env bash -il (a bash script), so the guard syntax is intact in all three wrappers.
• Path traversal / symlink escape is blocked by libarchive defaults (bsdtar -x without -P) — writes stay inside the polled directory.

Blocking / high-priority

1. (P1) The described test suite and CI workflow do not exist in the PR. The description details scripts/test/, run_tests.sh, check_drift.sh, .github/workflows/extraction-guard-tests.yml, and "53/53 assertions green." At the PR head none of these exist — scripts/ contains only the wrappers, and the only workflows present are Semgrep.yml, spm-smoke-test.yml, and verify-selfupdate-checksums.yml. A security-critical guard would merge with no regression protection. Please commit the suite + CI, or remove the claims from the description.

2. (P2) No compressed-download size cap exists, despite the Summary claiming one. The Summary states "curl --max-filesize (100 MB) caps the compressed download," but:

• scripts/*/cli.sh — the download curl (curl -fR -z … -L … -o …) has no --max-filesize.
• BrowserStackAccessibilityLint.swift — download(...) uses URLSession.shared.download(from: url) with no Content-Length/byte limit.

In the fix's own threat model (MITM of the HTTPS endpoint, or an attacker-controlled HTTPS override URL), a multi-GB compressed payload exhausts disk during download — before checksum or extraction — bypassing the entire decompression guard. Please add --max-filesize to the curl download and a byte cap to the Swift download, or strike the claim.

3. (P2) Windows extraction path is unguarded — see the inline note; either guard it or track it as an explicit follow-up.

Lower priority

Inline P3 comments cover: shell entry-count asymmetry, set +o pipefail toggled unconditionally, .tmp cleanup on chmod/mv failure, poll-interval doc drift (50 ms vs 200 ms), SIGTERM-only kill, extractionFootprint fail-open + hidden-file inconsistency, missing private on the new decls, and the libarchive-containment assumption.

Verdict: Not ready — the core guard is correct, but the compressed-download cap (#2) and the test/CI suite (#1) are described but absent, and Windows is unguarded (#3). Land the missing pieces or correct the description and consciously accept the gaps.

_{🤖 Multi-agent review via Claude Code (compound-engineering). Posted as comments, not a formal request-changes.}

[Crash0v3rrid3]

P3 — shell path lacks the Swift entry-count guard. In -O mode an archive of millions of tiny/empty entries streams ~0 bytes to stdout, so head -c never fills and never SIGPIPEs bsdtar. Disk stays bounded (good), but bsdtar still parses every entry (CPU/time drain) and a near-empty bogus payload passes the size check and gets chmod+mv'd into the cache. The Swift path guards this with maxArchiveEntries = 10_000; the wrappers have no equivalent. Consider an entry ceiling or --max-time on extraction.

(Applies identically to scripts/zsh/cli.sh and scripts/fish/cli.sh.)

[maunilm]

Acknowledged as a real gap, and deliberately not fixed in this PR — flagging rather than silently skipping.

Your analysis is right: in -O mode an archive of millions of empty entries streams ~0 bytes to stdout, so head -c never fills and never SIGPIPEs bsdtar. Disk stays bounded, but bsdtar parses every entry and a near-empty payload passes the size check and gets published. The plugin's maxArchiveEntries = 10_000 has no wrapper equivalent.

Why it is not in this commit: there is no cheap, correct mechanism in -O mode. The options I considered:

• bsdtar -tf pre-pass to count entries — doubles archive parsing and is itself unbounded on a millions-of-entries archive, so it moves the CPU drain rather than removing it.
• --max-time on extraction — a wall-clock proxy for an entry count; flaky on slow CI runners and does not actually bound entries.
• Extract to a directory instead of -O so the footprint is measurable like the plugin's — the correct fix, but that is a real change to the wrapper's extraction model, and the wrappers are what self-update ships to every user from main. Not something I want to land in the same PR as the guard, untested on Linux.

So: tracked as a follow-up on DEVA11Y-761 with your reasoning quoted, and listed under Known gaps item 4 in the rewritten PR description so it is owned rather than invisible.

Worth noting the residual is narrower than it was: the compressed-download cap added in 2c5fba8 (curl --max-filesize + post-download size check) bounds how large such an archive can be in the first place, so the CPU drain is capped at parsing a ≤100 MB archive rather than an unbounded one. That does not close the gap, but it does bound it.

Happy to take the "extract to a directory" approach as its own PR if you would rather not carry the gap.

[Crash0v3rrid3]

P3 — set +o pipefail is toggled unconditionally. Neither wrapper sets pipefail globally today, so this is safe now, but it disables the option outright rather than restoring the prior state. If a global set -o pipefail is ever added to these scripts, this line will silently switch it off for everything after download_binary. Prefer save/restore, e.g. capture set +o | grep pipefail before and restore it after.

Also note: if chmod/mv fail on the happy path just below, ${BINARY_PATH}.tmp is left on disk — the rm -f cleanup only runs on the size-rejection branch. Minor, but inconsistent with the explicit cleanup above.

(Applies identically to scripts/zsh/cli.sh and scripts/fish/cli.sh.)

[maunilm]

Both fixed in 2c5fba8, in all three launchers.

pipefail — now saved and restored rather than cleared:

local pipefail_was_set=0
case "$(set +o)" in *"-o pipefail"*) pipefail_was_set=1 ;; esac
set -o pipefail
bsdtar … | head -c "$max_decompressed" > "${BINARY_PATH}.tmp"
local extract_status=$?
[[ $pipefail_was_set -eq 1 ]] || set +o pipefail

Verified both directions: with set -o pipefail in the caller it is still set after download_binary returns; with it off, it stays off.

.tmp on the happy path — good catch, the asymmetry was real. chmod/mv are now guarded with cleanup on failure instead of a bare && chain:

if ! { chmod 0755 "${BINARY_PATH}.tmp" && mv -f "${BINARY_PATH}.tmp" "$BINARY_PATH"; }; then
  echo "BrowserStack CLI: failed to publish the downloaded binary." >&2
  rm -f "${BINARY_PATH}.tmp"
  return 1
fi
strip_quarantine

[Crash0v3rrid3]

P3 — poll-interval doc drift. This sleeps every 50 ms (0.05), but the PR description and the overshoot math in the docstring above refer to a "200 ms poll interval" — off by 4×. Either bump this to 0.2 or correct the description/comment so the documented worst-case footprint (maxBytes + pollInterval × writeRate) matches reality.

[maunilm]

Fixed in 2c5fba8. You were right that it was off by 4x — and the drift was in the docstring rather than the code, so I corrected the docs to the real 50 ms rather than slowing the poll:

/// the limit before it is killed, so peak disk use is roughly `maxBytes + (50 ms x disk
/// write rate)` — the poll interval below is 50 ms.

Kept 50 ms because it is what the measurements in the description were actually taken at: against the 400 MB fixture the watchdog bounded peak disk to 58 MB, and re-verified on this head against the real 38 MB archive with a 5 MB cap it bounds to 36 MB of 66 MB. Widening to 200 ms would loosen that overshoot 4x for no benefit.

The PR description has also been rewritten (it was stale in several places — see the top-level reply).

[Crash0v3rrid3]

P3 — SIGTERM only, no escalation. terminate() sends SIGTERM once and the loop breaks. bsdtar doesn't trap SIGTERM so this is fine in practice, but if it's ever slow to die (blocked I/O), waitUntilExit() on the main thread blocks with no SIGKILL fallback. Low impact; consider a bounded wait + kill(pid, SIGKILL) escalation for robustness.

[maunilm]

Agreed on the analysis, and taking your own read that it is low impact — not changing it in this PR.

For the record on why: bsdtar does not trap SIGTERM, so in practice it dies immediately; the watchdog breaks straight after terminate() and the loop condition is while process.isRunning, so the thread exits cleanly with no leak. The theoretical hang needs bsdtar blocked in uninterruptible I/O, in which case waitUntilExit() on the main thread would stall with no SIGKILL fallback.

A bounded wait plus kill(pid, SIGKILL) escalation is the right hardening and I would rather add it with a test that actually exercises the escalation path than add an untested kill to a security fix. Noted on DEVA11Y-761 alongside the other deferred items.

Verified on the current head that the non-pathological path behaves: against the real 38 MB archive with a 5 MB cap the watchdog fires and bsdtar reports terminationStatus = 15 (SIGTERM), with disk bounded to 36 MB of the 66 MB it would otherwise have written.

[Crash0v3rrid3]

P3 — two small issues in extractionFootprint.

1. Fails open: if fm.enumerator(...) returns nil (directory transiently unreadable/missing), this returns (0, 0) → footprintExceeded returns nil → "not exceeded" for that poll. Low risk since the plugin created the dir, but a transient failure silently disables the guard for that tick.
2. Inconsistent "entry" definition: this enumerator omits .skipsHiddenFiles, while locateExecutable's enumerator (line ~646) passes options: [.skipsHiddenFiles]. The same 10 000 ceiling therefore counts hidden files here but not there. Align the two so "entries" means the same thing in both guards.

[maunilm]

Both fixed in 2c5fba8.

1. Fail-open → fail-closed. You are right that (0, 0) silently disabled the guard for that poll. It now fails closed:

guard let enumerator = fm.enumerator(at: url, includingPropertiesForKeys: [...]) else {
    // Fail CLOSED: a directory we just created being unreadable is not a "0 bytes"
    // result, and returning (0, 0) would silently disable the guard for that poll.
    return (Int64.max, Int.max)
}

A transient failure now trips the ceiling and aborts rather than waving the archive through. Failing closed is the right default for a guard, and the false-positive cost is an aborted download with a clear message.

2. Hidden-file inconsistency. Also real — but after looking at both call sites I kept the difference and documented it rather than aligning them, because they are measuring different things:

• extractionFootprint measures what bsdtar actually wrote — dotfiles included, since they consume disk and count toward a "millions of tiny files" bomb. Adding .skipsHiddenFiles would let an all-dotfiles archive slip the entry ceiling.
• locateExecutable is searching for a binary, so skipping hidden files is correct there.

So the shared 10_000 is deliberately counting different sets. Comment added at the enumerator making that explicit so the next reader does not "fix" it:

// `.skipsHiddenFiles` is deliberately NOT set, so the entry count here matches what
// bsdtar actually wrote — including dotfiles. locateExecutable skips hidden files
// because it is searching for a binary, not measuring a footprint; the two use the
// same ceiling but count deliberately different things (DEVA11Y-484 review).

Happy to split into two named constants if you would rather the shared 10_000 not imply the two are equivalent.

[Crash0v3rrid3]

P3 — hygiene: these new top-level declarations lack private. ExtractionLimitState, extractionFootprint, footprintExceeded, and startExtractionWatchdog are the only non-private helpers in the file — every other helper (isTruthy, packageCacheRoot, hardwareIdentifier, …) is private. Harmless in a single-file plugin target, but worth marking private for consistency.

[maunilm]

Fixed in 2c5fba8 — all four are now private:

• private final class ExtractionLimitState
• private func extractionFootprint(at:)
• private func footprintExceeded(at:maxBytes:maxEntries:)
• private func startExtractionWatchdog(on:directory:maxBytes:maxEntries:)

Agreed it was inconsistent with every other helper in the file. swiftc -typecheck -parse-as-library is clean after the change.

[Crash0v3rrid3]

P2 — Windows extraction path is unguarded. This correctly notes the unzip/Expand-Archive path has no streaming guard, but Windows is a supported target (#if os(Windows) branches, browserstack-cli.exe, PowerShell checksum). A zip bomb there fully exhausts disk with no download cap, no watchdog, and no entry ceiling. It's out of this PR's stated 4-surface scope, so either add a guard to the Windows path or track it as an explicit follow-up so the gap is owned rather than just commented.

Also, defense-in-depth note for the non-Windows path: containment depends on libarchive's default behavior (bsdtar -x without -P neutralizes .., absolute paths, and symlink-through, keeping all writes inside the polled -C directory). That's correct today but load-bearing and unasserted — a future -P would let writes escape the polled dir and the footprint poll would measure nothing. Worth a comment pinning the assumption.

[maunilm]

Two parts here.

Windows — now tracked, not merely commented. Agreed it is a real gap, and it is explicitly owned: DEVA11Y-761 item 3, with the implementation preserved on chore/DEVA11Y-484-followup-extraction-guard-harness. That branch carries the prepareArtifact-level footprintExceeded backstop positioned against stagingDirectory before publishVersionDirectory — which is where it belongs after #32 restructured extraction, so a rejected archive never becomes a visible version directory.

It came out of this PR when the PR was narrowed to DEVA11Y-484's stated Remediation, which scopes the bsdtar paths only. I noted on the ticket that "Windows has no bomb guard" probably deserves its own security ticket rather than sitting in a cleanup task — say the word and I will raise one.

One thing that does help Windows in the meantime: the compressed-download cap added in 2c5fba8 sits in the shared download(from:to:), so it applies on Windows too. It does not bound decompression, but it stops a multi-GB archive reaching Expand-Archive at all.

libarchive containment — pinned. Good catch that it was load-bearing and unasserted. Now stated in the guard block:

// Containment assumption (load-bearing): `bsdtar -x` WITHOUT `-P` neutralises `..`,
// absolute paths and symlink-through, so every write lands inside the `-C` directory we
// poll. Adding `-P` would let writes escape that directory and the footprint poll would
// measure nothing — do not add it (DEVA11Y-484 review).

[maunilm]

Thanks — this was a genuinely useful review, and one of the two blockers was my fault in a way worth naming explicitly.

Everything below is in 2c5fba8, plus a rewritten PR description. All 9 checks green.

Blocker 1 (P1) — test suite and CI "described but absent": you were right, and the cause was a stale description

The suite and workflow really were not there. The reason is not that they were forgotten — they were deliberately removed when this PR was narrowed to DEVA11Y-484's stated Remediation (the PR had grown to +1044/−7 across 18 files for an XS ticket, with only 216 lines of production code). I removed the code and failed to update the description, so it kept advertising a harness, a CI workflow and "53/53 assertions" that no longer existed. That is exactly the kind of claim a reviewer should not have to discover by diffing, and it wasted your time.

The description is now rewritten to match the shipped code, with an explicit Known gaps section that owns all five gaps rather than implying coverage that does not exist.

The suite itself is not lost: preserved verbatim on chore/DEVA11Y-484-followup-extraction-guard-harness and tracked as DEVA11Y-761 item 1, flagged there as the highest-value follow-up. Your point stands that a security-critical guard is merging without regression protection — that is a conscious, documented trade, not an oversight.

Blocker 2 (P2) — no compressed cap: half stale description, half a real hole. Fixed.

Same root cause for the wording — the cap had been descoped as "not in the ticket's Remediation" and the Summary still claimed it. But your threat-model argument is the substantive part and I think it is correct: without a wire cap, a multi-GB compressed payload exhausts disk before the checksum or the decompression guard ever run, which walks around the entire fix. Deferring it on a scoping technicality was the wrong call.

Reinstated in 2c5fba8:

• Launchers: curl --max-filesize 104857600, plus an explicit post-download size check because curl documents the flag as a no-op when the length is unknown (chunked). Verified against the live endpoint: with a 1 MB cap curl aborts non-zero with 0 bytes written; with the real 100 MB cap the 38 MB archive passes. (Observed exit is 56 rather than 63 — with -L following the 302 to sdk-assets it aborts during receive, not pre-transfer. The comment states the observed behaviour, not the idealised one.)
• Plugin: maxCompressedBytes = 100 MB, checked against both response.expectedContentLength and the downloaded file's actual size, temp file removed on rejection. Applies on Windows too, since it lives in the shared download(from:to:).

One limitation I want to state plainly rather than let the description imply otherwise: URLSession.download(from:) has no byte-level hook, so on the Swift path these checks reject the archive after the transfer. They stop an oversized archive being verified, extracted, published or executed, but they do not bound peak temporary disk during the transfer — which is precisely the scenario you described. Bounding that needs a URLSessionDownloadDelegate cancelling in didWriteData. I chose not to rewrite that shared download path inside a security fix I cannot exercise end-to-end without credentials; it is documented inline, in Known gaps item 3, and on DEVA11Y-761. The launchers do abort during transfer, so the primary installer path is covered.

Blocker 3 (P2) — Windows unguarded

Tracked as DEVA11Y-761 item 3 with the implementation preserved on the follow-up branch, per your "either guard it or track it as an explicit follow-up so the gap is owned". Detail in the inline thread. I also noted on the ticket that this probably warrants its own security ticket rather than living in a cleanup task — happy to raise one.

P3s

Fixed: private on all four new decls; poll-interval doc drift (docs corrected to the real 50 ms rather than slowing the poll); extractionFootprint now fails closed on a nil enumerator; the .skipsHiddenFiles asymmetry documented as deliberate with reasoning; pipefail saved/restored and verified both directions; .tmp cleaned up on chmod/mv failure; libarchive containment assumption pinned in the guard block.

Deferred with reasoning in-thread: the launcher entry-count gap (no cheap correct mechanism in -O mode — the right fix is extracting to a directory, which is too large a change to the wrapper model to smuggle in here) and SIGTERM escalation (which you flagged low-impact yourself).

Verification on this head

No automated suite ships, so this was verified directly against the live endpoint: 27/27 assertions across bash/zsh/fish — real download through #37's integrity check, .tmp cleanup, byte-identical re-run, corrupt payload rejected, and the previously-cached binary's sha256 unchanged after a rejected payload (confirming the cap on ${BINARY_PATH}.tmp preserves #37's anti-truncation protection rather than undoing it). Swift guard compiled standalone and driven against the real archive: real 200 MB cap does not flag; 5 MB cap SIGTERMs bsdtar mid-stream bounding disk to 36 MB of 66 MB; maxEntries=0 flags on entry count. swiftc -typecheck clean, bash -n clean, all six sidecars verify, self-update's own comparison matches.

Ready for another look when you have a moment.

[Crash0v3rrid3]

Approved — re-review of 2c5fba8

Both blockers from my earlier review are resolved, and the fix is sound and honestly documented.

Resolved

• Compressed-download cap now implemented — Swift checks response.expectedContentLength and the downloaded file's actual size against maxCompressedBytes (100 MB); the launchers use curl --max-filesize + a wc -c backstop before checksum/extract. The post-transfer limitation is called out plainly and deferred to DEVA11Y-761.
• PR description corrected — the previously-fictional Tests/CI section was descoped (moved to the follow-up branch) and replaced with an honest "Known gaps — owned, not hidden" section. Claims now match code.

Verified

• All three .sha256 sidecars match their cli.sh (self-update verification intact); all three launchers are #!/usr/bin/env bash -il (bash syntax valid).
• Decompressed guard: bsdtar -O | head -c + pipefail correctly rejects a bomb via SIGPIPE (141) and passes a legit ~75 MB binary under the 200 MB cap; local extract_status=$? captures pipeline status; pipefail save/restore is correct; .tmp cleaned on chmod/mv failure.
• Swift: NSLock state thread-safe, watchdog has no leak, post-exit footprint recheck catches fast bombs, extractionFootprint now fails closed on a nil enumerator (prior fail-open fixed), forwardExit is -> Never, 10k-entry guard added to locateExecutable.

No new P0/P1. The remaining items (no automated regression tests, Windows Expand-Archive unguarded, shell entry-count gap, Swift mid-stream cap, cap duplication across 4 files) are all acknowledged and tracked under DEVA11Y-761.

One conscious sign-off for the record: the caps merge with no automated coverage — acceptable given it's explicitly the top DEVA11Y-761 follow-up.

[maunilm]

Claude Code Review (automated) — 6 inline finding(s). Full report in the PR comment below. Verdict: Failed - see PR comment.

[maunilm]

[Medium] This fail-closed branch is unreachable dead code

FileManager.enumerator(at:includingPropertiesForKeys:) does not return nil for a missing or unreadable directory — enumeration errors go to an errorHandler (default: skip and continue). Measured on this platform: a missing directory and a chmod 000 directory each yield a non-nil enumerator producing 0 elements, so both fall through to the loop and return (0, 0) — exactly the silent guard-disable this comment says it prevents.

Suggestion: fail closed from the error handler instead.

var enumerationFailed = false
guard let enumerator = fm.enumerator(
    at: url, includingPropertiesForKeys: [.isRegularFileKey, .fileSizeKey],
    options: [], errorHandler: { _, _ in enumerationFailed = true; return false }
) else { return (Int64.max, Int.max) }
…
return enumerationFailed ? (Int64.max, Int.max) : (total, count)

Reviewer: stack:devtools-review-changes (orchestrator-confirmed; stack-code-reviewer had this as "verified correct")

[maunilm]

[Medium] This abort path leaks the archive it exists to bound

forwardExit is -> Never and calls exit(code), so no defer runs. prepareArtifact holds two — one for stagingDirectory, one for archiveURL. This line hand-cleans only the staging directory, so the downloaded ≤100 MB archive stays in the cache on every guard trip. sweepStaleStaging reclaims it only after 3600 s and on a later successful prepareArtifact, so repeated runs inside the hour accumulate one archive each — inside the control meant to prevent disk exhaustion.

Suggestion: throw PluginError(…) instead of forwardExit so both defers run. Also matches locateExecutable's entry cap, which already throws for the same class of rejection.

Reviewer: stack:devtools-review-changes (orchestrator-confirmed)

[maunilm]

[Medium] Compressed-size check fails open on an unreadable size

If either the try? swallows a throw or the cast yields nil, the 100 MB cap is skipped with no diagnostic. That matters because this is the load-bearing half: expectedContentLength is -1 for chunked/unknown-length responses, so an attacker controlling BROWSERSTACK_A11Y_CLI_DOWNLOAD_URL — the threat model the comment above cites — just omits Content-Length.

Severity note: reported as High on the grounds that NSNumber → Int64 casting is Darwin-only. Measured here, the cast does succeed (attrs[.size] as? Int64 → 123456) and there is no Linux CI leg, so this is not a live fail-open today — downgraded to Medium. The try? path and the inconsistency with this file's own fail-closed stance still warrant fixing.

Suggestion: use the .fileSizeKey idiom already used later in this file, and fail closed:

guard let downloadedBytes = (try? tempURL.resourceValues(forKeys: [.fileSizeKey]).fileSize).map(Int64.init) else {
    try? fileManager.removeItem(at: tempURL)
    throw PluginError("Could not determine the downloaded archive's size; refusing to use it.")
}

(?? nil is also redundant — try? is already flattened.)

Reviewer: stack:devtools-review-changes (severity adjudicated by orchestrator)

[maunilm]

[Medium] SIGTERM with no escalation: fails-closed can become hangs-open

The watchdog sends one SIGTERM and breaks, while the main flow's waitUntilExit() is unbounded. If bsdtar does not die promptly — ignored or blocked in an uninterruptible write — the plugin hangs indefinitely rather than aborting with the intended "Aborting to prevent disk exhaustion" message, which is a worse failure mode than the one being guarded.

Mitigating: bsdtar does not trap SIGTERM, and the normal path was measured working (terminationStatus = 15, disk bounded to 36 MB of 66 MB).

Suggestion: after terminate(), grace-wait ~2 s then kill(process.processIdentifier, SIGKILL) if process.isRunning, or bound waitUntilExit().

Reviewer: stack-code-reviewer

[maunilm]

[Low] Pipeline is not errexit-safe, and this script sources user rc files

The shebang is #!/usr/bin/env bash -il, so ~/.bashrc / ~/.bash_profile are sourced. If a user's rc sets -e (common in "strict mode" boilerplate, often paired with -o pipefail), this bare pipeline aborts the script immediately: local extract_status=$? never runs, the user never sees the 200 MB message, and ${BINARY_PATH}.tmp is left behind — the exact residue the publish-failure cleanup below was added to prevent.

Suggestion:

Suggested change

	bsdtar -xvf "$BINARY_ZIP_PATH" -O | head -c "$max_decompressed" > "${BINARY_PATH}.tmp"
	local extract_status=0
	bsdtar -xvf "$BINARY_ZIP_PATH" -O | head -c "$max_decompressed" > "${BINARY_PATH}.tmp" || extract_status=$?

Applies identically to scripts/zsh/cli.sh and scripts/fish/cli.sh.

Reviewer: stack:devtools-review-changes

[maunilm]

[Low] -ge here vs -gt on the compressed check — correct, but undocumented

-ge is deliberate and necessary: head -c N emits exactly N bytes whether the input was N or 10 GB, so == N is indistinguishable from an overflow and must be rejected. The surrounding comment does not say so, and a future editor "fixing the inconsistency" to -gt would silently punch a hole in the guard.

Suggestion: add one line — # -ge, not -gt: head -c caps output at exactly N, so N bytes is indistinguishable from an overflow.

Reviewer: stack:devtools-review-changes + stack-code-reviewer

[maunilm]

Claude Code PR Review

PR: #25 • Head: efcf13c • Reviewers: stack:devtools-review-changes, stack-code-reviewer

Summary

Adds a decompression-bomb / disk-exhaustion guard to the CLI download-and-extract path for DEVA11Y-484: a 100 MB compressed cap, and a 200 MB / 10,000-entry decompressed cap enforced by a polling watchdog that SIGTERMs bsdtar in the Swift plugin plus a bsdtar … -O | head -c 209715200 guard with pipefail in the three (functionally-bash) shell launchers.

The core guard works. Both reviewers independently confirmed it, and the orchestrator re-verified at this head: the merge is clean, all six .sha256 sidecars recompute correctly, the ${BINARY_PATH}.tmp staging invariant holds, and the real artifact has 2.6× headroom on both caps. The findings below are all in edge-case handling around the guard, not the main path.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	Only size constants and checksum sidecars
High	Security	Authentication/authorization checks present	N/A	No auth surface
High	Security	Input validation and sanitization	Pass	This PR is the input-size validation; libarchive containment (-xpf, no -P) verified intact
High	Security	No IDOR — resource ownership validated	N/A	—
High	Security	No SQL injection (parameterized queries)	N/A	—
High	Correctness	Logic is correct, handles edge cases	Fail	Two confirmed defects: a documented fail-closed branch that provably never executes (F1), and an abort path that leaks the archive it exists to bound (F2). Happy path is correct.
High	Correctness	Error handling is explicit, no swallowed exceptions	Fail	try? + as? on the compressed-size read fails open with no diagnostic (F3); forwardExit skips both defer cleanups (F2)
High	Correctness	No race conditions or concurrency issues	Pass	NSLock usage correct and markExceeded idempotent (both reviewers); watchdog armed after run() (460 after 458) and exits on isRunning
Medium	Testing	New code has corresponding tests	Fail	Zero automated coverage ships (F5) — disclosed and tracked, but the control is untested
Medium	Testing	Error paths and edge cases tested	Fail	None of the rejection paths, the post-exit re-check, or the entry cap are exercised
Medium	Testing	Existing tests still pass (no regressions)	Pass	10/10 CI checks green at efcf13c, incl. verify-sidecars and the SwiftPM e2e
Medium	Performance	No N+1 queries or unbounded data fetching	Pass ⚠️	Watchdog re-walks the tree every 50 ms — O(entries) per tick (F8); trivial for the 1-entry artifact
Medium	Performance	Long-running tasks use background jobs	N/A	—
Medium	Quality	Follows existing codebase patterns	Pass ⚠️	F1/F3 diverge from the .fileSizeKey + fail-closed idiom already used elsewhere in the same file
Medium	Quality	Changes are focused (single concern)	Pass	7 files, all the guard; scope was deliberately narrowed and the descope is documented
Low	Quality	Meaningful names, no dead code	Fail	F1 is dead code — the guard let … else branch is unreachable
Low	Quality	Comments explain why, not what	Pass	Unusually well documented, including the load-bearing libarchive assumption
Low	Quality	No unnecessary dependencies added	Pass	None added

Findings

F1 — the fail-closed branch is unreachable dead code, so the guard still silently disables itself

• File: Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift:894
• Severity: Medium
• Reviewer: stack:devtools-review-changes (confirmed by orchestrator; contradicts stack-code-reviewer, which listed this as "verified correct")
• Issue: guard let enumerator = fm.enumerator(at:includingPropertiesForKeys:) else { return (Int64.max, Int.max) } cannot fire for the cases its comment names. FileManager.enumerator(at:includingPropertiesForKeys:) routes enumeration errors to an errorHandler (default: skip and continue), it does not return nil. Measured on this platform: a missing directory and a chmod 000 directory both yield a non-nil enumerator producing 0 elements — so both fall through to the loop and return (0, 0), which is exactly the silent guard-disable the comment claims to prevent.
• Suggestion: supply the error handler and fail closed from it:

  var enumerationFailed = false
  guard let enumerator = fm.enumerator(
      at: url, includingPropertiesForKeys: [.isRegularFileKey, .fileSizeKey],
      options: [], errorHandler: { _, _ in enumerationFailed = true; return false }
  ) else { return (Int64.max, Int.max) }
  …
  return enumerationFailed ? (Int64.max, Int.max) : (total, count)

F2 — the anti-disk-exhaustion abort path leaks the archive it exists to bound

• File: Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift:471
• Severity: Medium
• Reviewer: stack:devtools-review-changes (confirmed by orchestrator)
• Issue: forwardExit is -> Never and calls exit(code) (959–963), so no defer runs. prepareArtifact holds two — stagingDirectory (249) and archiveURL (260). The guard trip hand-cleans only directory (the staging dir) then forwardExits, so the downloaded ≤100 MB archive is left in the cache on every trip. sweepStaleStaging reclaims it only after 3600 s and on a later successful prepareArtifact, so repeated invocations inside the hour accumulate one archive each.
• Suggestion: throw PluginError(…) instead of forwardExit, so both defers run. Also makes rejection surfacing consistent with locateExecutable's entry cap (666), which already throws.

F3 — compressed-size check fails open on an unreadable size

• File: Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift:624
• Severity: Medium — downgraded from the reviewer's High; see note
• Reviewer: stack:devtools-review-changes (severity adjudicated by orchestrator)
• Issue: (try? fileManager.attributesOfItem(atPath:)[.size] as? Int64) ?? nil skips the cap with no diagnostic if either the try? swallows a throw or the cast yields nil. This is the load-bearing half of the compressed cap, because expectedContentLength is -1 for chunked/unknown-length responses — an attacker controlling BROWSERSTACK_A11Y_CLI_DOWNLOAD_URL (the threat model the comment itself cites) simply omits Content-Length.
• Severity note: the reviewer rated this High on the basis that NSNumber → Int64 conditional casting is a Darwin-only guarantee and unreliable on Linux corelibs-Foundation. Measured on the target platform, the cast succeeds (attrs[.size] as? Int64 → 123456), and there is no Linux CI leg or evident Linux consumer for this SwiftPM plugin, so it is not a live fail-open today. Downgraded to Medium; the try? path and the inconsistency with this file's own fail-closed stance still warrant the fix.
• Suggestion: use the idiom already present 270 lines later and fail closed:

  guard let downloadedBytes = (try? tempURL.resourceValues(forKeys: [.fileSizeKey]).fileSize).map(Int64.init) else {
      try? fileManager.removeItem(at: tempURL)
      throw PluginError("Could not determine the downloaded archive's size; refusing to use it.")
  }

F4 — no SIGKILL escalation after terminate(): fails-closed becomes hangs-open

• File: Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift:938
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue: the watchdog sends one SIGTERM and breaks; the main flow's waitUntilExit() (461) is unbounded. If bsdtar does not die promptly (blocked in an uninterruptible write), the plugin hangs indefinitely instead of aborting with the intended message — a worse failure mode than the one being guarded. Mitigating: bsdtar does not trap SIGTERM, and the orchestrator measured terminationStatus = 15 with disk bounded to 36 MB of 66 MB, so the normal path works.
• Suggestion: after terminate(), grace-wait ~2 s and kill(process.processIdentifier, SIGKILL) if still running, or bound waitUntilExit().

F5 — this security control ships with zero automated regression coverage

• File: tests/
• Severity: Medium
• Reviewer: both
• Issue: tests/extraction-guard was added in e850495 and removed in 72091b9 (deferred to DEVA11Y-761); absent at head. Untested: ExtractionLimitState's cross-thread flag, the post-exit footprintExceeded re-check, locateExecutable's 10,000-entry throw, and the shell head -c/SIGPIPE/pipefail interaction including the -ge boundary. Launcher scripts (bash syntax) is bash -n only; CLI binary integrity check exercises verify_binary_integrity, not download_binary. Candidly disclosed in the PR body — this is not undisclosed scope-cutting — but F1 and F3 are both defects a fixture test would have caught.
• Suggestion: both reviewers converge on the cheapest slice: a sibling functional-matrix job for download_binary against a local bomb fixture asserting rc≠0, $BINARY_PATH unchanged, and no ${BINARY_PATH}.tmp residue. That single assertion protects the cached-binary invariant this PR is built around.

F6 — Windows Expand-Archive path has no decompression guard

• File: Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift:448
• Severity: Medium
• Reviewer: stack-code-reviewer (confirmed by orchestrator)
• Issue: extractLocalArchive — and therefore the watchdog — is inside #if !os(Windows) (448). Windows uses unzip(archive:into:) (637) with only the shared 100 MB compressed cap. A high-ratio bomb under 100 MB compressed is uncaught there. Disclosed in code comments and the PR body, tracked as DEVA11Y-761 item 3.
• Suggestion: keep tracked; both reviewers flag it so it does not become a "someday" item.

Low

• F7 scripts/{bash,zsh,fish}/cli.sh:286 — pipeline is not errexit-safe. The shebang is #!/usr/bin/env bash -il, so user rc files are sourced; if a user's rc sets -e, the pipeline aborts before local extract_status=$?, the size message never prints, and ${BINARY_PATH}.tmp is left behind — the exact residue 299–303 was added to prevent. Fix: local extract_status=0 then … || extract_status=$?.
• F8 …swift:934 — watchdog re-walks the tree with resourceValues per element every 50 ms → O(entries) per tick. An archive sitting just under both ceilings never trips the guard but drives sustained stat load. Trivial at the real 1-entry artifact. Both reviewers raised it.
• F9 scripts/bash/cli.sh:292 — -ge (decompressed) vs -gt (compressed) asymmetry is correct but unexplained; head -c N emits exactly N whether the input was N or 10 GB, so == N must be rejected. A future "consistency fix" to -gt would silently punch a hole. Add one comment line.
• F10 …swift:624 — ?? nil after try? is redundant (SE-0230 flattens it). Resolved by the F3 rewrite. Both reviewers raised it.
• F11 …swift:174 — caps are compile-time constants in five places with no override. The entry cap is the fragile one: if the CLI ever ships unpacked node_modules, 10,000 entries is crossed instantly and every consumer hard-exits with no escape hatch until a plugin release.
• F12 scripts/bash/cli.sh:293 — the message conflates a corrupt archive (extract_status -ne 0) with a real size rejection; splitting it helps on-call triage.

Reviewer disagreement, adjudicated

• F1 — stack-code-reviewer listed the fail-closed enumerator branch under "Verified correct … correctly fails closed as required." stack:devtools-review-changes called it unreachable. The orchestrator tested both claims: missing dir → non-nil enumerator, 0 elements; chmod 000 dir → non-nil enumerator, 0 elements. The branch is unreachable; A is right, B's verification is wrong. Kept as a finding.
• F3 — reviewer rated High on Linux-bridging grounds; measured to work on the target platform (as? Int64 → 123456). Downgraded to Medium with the measurement recorded, rather than carrying a High the evidence does not support.
• Verdicts — stack-code-reviewer returned Approve; stack:devtools-review-changes returned REQUEST_CHANGES. The gate below follows the table, not a vote.

Raised by other reviewers (not independently confirmed)

• @Crash0v3rrid3 approved this PR on 2026-08-31T07:32:52Z after a multi-agent review on 2026-08-27. Their earlier P3s on pipefail save/restore, .tmp cleanup, poll-interval doc drift and private hygiene were addressed in 2c5fba8. Two of their P3s remain open by agreement — the launcher entry-count gap and SIGKILL escalation (the latter re-raised here as F4, upgraded to Medium because waitUntilExit() is unbounded on the main thread).
• Correction owed on an existing thread: the reply posted to their extractionFootprint thread stated the fail-open was fixed. F1 shows it was not — the fix landed on an unreachable branch. That thread should be corrected rather than left standing.

Orchestrator verification at this head

Independent of both reviewers: today's merge efcf13c brought only #38 (Dependabot Semgrep digest) and touched none of the 7 guard files — byte-identical to 2c5fba8. All six sidecars recompute clean. The guard writes to ${BINARY_PATH}.tmp (286), not $BINARY_PATH, so #37's anti-truncation invariant holds. pipefail save/restore present (283–288). #37's verify_binary_integrity, url_effective and curl -fR all present. CI 10/10 green.

---

Verdict: FAIL — core guard is sound and the merge is clean, but two High-priority table rows fail on confirmed defects: a documented fail-closed protection that provably never executes (F1) and an abort path that leaks the archive it bounds (F2). Both are small, local fixes.

[maunilm]

Claude Code PR Review

Continues the previous review — changes since efcf13c (FULL re-review).

PR: #25 • Head: 0af9ec2 • Reviewers: stack:devtools-review-changes, stack-code-reviewer

Summary

Decompression-bomb / disk-exhaustion guard for DEVA11Y-484: a 100 MB compressed cap and a 200 MB / 10,000-entry decompressed cap, enforced by a polling watchdog that SIGTERMs bsdtar in the Swift plugin and by bsdtar … -O | head -c with pipefail in the three launchers.

Since the previous FAIL at efcf13c, two commits landed. e0db1dd closed the three gating findings; 0af9ec2 closed a new HIGH the second round surfaced — a proven, indefinite extraction deadlock — plus a Medium that the first fix had itself introduced.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	Size constants and checksum sidecars only
High	Security	Authentication/authorization checks present	N/A	No auth surface
High	Security	Input validation and sanitization	Pass	This PR is the size validation; bsdtar -xpf (no -P) confirmed to refuse traversal (Path contains '..')
High	Security	No IDOR — resource ownership validated	N/A	—
High	Security	No SQL injection (parameterized queries)	N/A	—
High	Correctness	Logic is correct, handles edge cases	Pass	F1 dead fail-closed branch, F2 leaked archive resolved in e0db1dd; the stderr deadlock resolved in 0af9ec2. Each verified by execution, not inspection
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	F3 fail-open size read now fails closed; both forwardExit leaks converted to throw; measurement failure now reports its own reason
High	Correctness	No race conditions or concurrency issues	Pass	NSLock correct, markExceeded idempotent, watchdog armed after run(); the new stderr drain uses a dedicated queue + semaphore joined before the buffer is read
Medium	Testing	New code has corresponding tests	Fail	Zero automated coverage ships (carried)
Medium	Testing	Error paths and edge cases tested	Fail	Rejection paths, post-exit re-check and entry cap are unexercised in CI
Medium	Testing	Existing tests still pass (no regressions)	Pass	9/9 CI green at 0af9ec2
Medium	Performance	No N+1 queries or unbounded data fetching	Pass ⚠️	Watchdog re-walks the tree per 50 ms tick; overshoot now quantified at ~288 MB peak against a 200 MB cap on NVMe
Medium	Performance	Long-running tasks use background jobs	N/A	—
Medium	Quality	Follows existing codebase patterns	Pass	Now uses this file's own .fileSizeKey idiom and its throws convention
Medium	Quality	Changes are focused (single concern)	Pass ⚠️	The stderr drain is pre-existing plumbing, fixed here because it defeats this PR's guard rather than sitting beside it
Low	Quality	Meaningful names, no dead code	Pass	The unreachable branch is gone; the errorHandler now genuinely fires
Low	Quality	Comments explain why, not what	Pass	Load-bearing assumptions pinned, including a measured note that curl exits 56 not 63 here
Low	Quality	No unnecessary dependencies added	Pass	None

Findings

Resolved since efcf13c

• …swift:894 Medium — fail-closed enumerator branch was unreachable dead code Resolved in e0db1dd. Both reviewers re-verified by execution; errorHandler fires for a missing dir, a chmod 000 dir, and a nested unreadable subtree. Orchestrator independently confirmed.
• …swift:471 Medium — forwardExit bypassed both defers, leaking the ≤100 MB archive per guard trip Resolved in e0db1dd. extractLocalArchive is throws, caller is async throws, both defers registered before the call. One reviewer built a scratch SwiftPM plugin and confirmed SwiftPM renders the message via CustomStringConvertible.
• …swift:624 Medium — compressed-size check could fail open Resolved in e0db1dd. Now resourceValues(.fileSizeKey) + guard/throw; verified nil on a missing file, Optional(4242) on a real one.
• …swift:454 High — undrained stderr pipe deadlocked extraction indefinitely Resolved in 0af9ec2. See below.
• …swift:935 Medium — the fail-closed fix misreported I/O failure as a size violation Resolved in 0af9ec2. Footprint now carries measured: Bool; unmeasurable directories report "extraction directory could not be measured".
• cli.sh:286 Low — pipeline not errexit-safe, :292 Low — -ge undocumented, :293 Low — conflated diagnostics, …swift:490 Low — sibling forwardExit leak, cli.sh:255 Low — curl message conflation all Resolved.

On the High, for the record. stack:devtools-review-changes proved it rather than theorising: a 4,000-entry tar whose every member contains .. extracts to 0 bytes / 0 entries — under both ceilings, so the watchdog never fires — while bsdtar emits ~227 KB of warnings into a 64 KB pipe. The orchestrator reproduced it independently (226,939 bytes of stderr) and harnessed both plumbings against it:

old (read after wait) : *** NO RETURN within 15s -> DEADLOCK ***
new (concurrent drain): returned OK  rc=1  stderr captured=65536 bytes

The watchdog made it worse, not better — it busy-polls at 20 Hz for the duration of the hang. Fixed by draining stderr on a dedicated queue started immediately after run(), capping the retained buffer at 64 KB while continuing to read past it, and joining before the buffer is read.

Still open — carried forward, all agreed for DEVA11Y-761

• Medium …swift:962 — no SIGKILL escalation after terminate(). bsdtar does not trap SIGTERM and the normal path measures clean (terminationStatus = 15), but waitUntilExit() remains unbounded.
• Medium tests/ — zero automated regression coverage. Both rounds flagged it, and it is now demonstrably load-bearing: the dead branch, the fail-open read and the deadlock were all caught by review rather than by CI. The deadlock reproducer is a ready-made first case.
• Medium …swift:448 — Windows Expand-Archive has no decompression guard; re-confirmed unchanged, not a regression. The new compressed cap does at least stop a >100 MB archive reaching it.
• Low …swift:962 — watchdog is O(entries) per tick; overshoot measured at ~288 MB peak against a 200 MB cap.
• Low …swift:174 — caps hardcoded in five places with no override.

Reviewer notes

• stack-code-reviewer hit its maxTurns: 25 limit on first dispatch and returned a fragment rather than findings. It was resumed and delivered a full re-review, honestly marking five requested checks UNVERIFIED rather than passed. Those five — sidecar hashes, launcher parity, the .tmp invariant, NSLock/arming order, and fix(cli): verify downloaded CLI binary integrity before exec (DEVA11Y-473/474) #37's features — were verified by the orchestrator at this head instead.
• A reviewer suggested keeping the cached archive on a transient curl failure to preserve the -z fast path. Not adopted, deliberately: a partial write carries a fresh mtime, so keeping it risks the next -z revalidation returning 304 and handing a truncated archive to verify_binary_integrity. Documented in the code.

Raised by other reviewers (not independently confirmed)

• @Crash0v3rrid3 approved at 2026-08-31T07:32:52Z, before e0db1dd and 0af9ec2 landed. Their earlier P3s were addressed in 2c5fba8; the SIGKILL item they raised is carried above as Medium.

Orchestrator verification at 0af9ec2

Independent of both reviewers: 27/27 shell assertions across bash/zsh/fish against the live endpoint, including the cached-binary-survives-rejection invariant; the deadlock reproducer built and both plumbings harnessed; measured-flag semantics checked on four directory shapes (missing, chmod 000, nested-locked, normal); all six sidecars recomputed; swiftc -typecheck and bash -n clean; real artifact confirmed at 36.2 MB compressed → 66.2 MB / 1 entry decompressed, comfortably inside every cap.

---

Verdict: PASS — all previously gating findings resolved and re-verified by execution, plus a High found and fixed this round. The remaining five are Medium/Low, agreed for DEVA11Y-761; the absent regression suite is the one worth pulling forward, since review rather than CI caught every defect here.

[maunilm]

Claude Code PR Review

Continues the previous review — changes since 0af9ec2 (DELTA re-review: 1 file, 13 lines).

PR: #25 • Head: 17c3b7a • Reviewers: stack-code-reviewer, stack:devtools-review-changes

Verdict: PASS

No Critical or High findings. Both reviewers independently recommended PASS. Merge is not blocked by this review.

What changed since the last review

One commit (17c3b7a) bounding the bsdtar stderr excerpt shown to the user to 20 lines. The drain itself stays unbounded — that is what prevents the 64 KB pipe deadlock fixed in 0af9ec2, and it is intentionally preserved.

New findings in this delta

Both are non-blocking (Medium), but both are regressions or incompleteness introduced by this commit rather than pre-existing debt, and both were reproduced empirically rather than reasoned about.

Medium — head-only truncation drops bsdtar's decisive last line

BrowserStackAccessibilityLint.swift:512-517

prefix(20) keeps the first 20 lines; bsdtar's convention is the opposite — per-entry warnings stream first, the fatal cause and Error exit delayed from previous errors. come last. Reproduced with a 25-member ../ archive followed by a truncated payload: 27 lines / 1,325 bytes total, which is under the 64 KB retention cap, so pre-delta the user saw the whole thing including payload1: Truncated tar archive. Post-delta the excerpt is 20 identical Path contains '..' lines and the cause is gone.

Verified user-visible: a throwaway command plugin throwing the same PluginError shape prints error: <full multi-line message>, so SwiftPM uses String(describing:) semantics, not localizedDescription. The message does reach the Issue Navigator, so this loss is real.

Impact: support cannot distinguish a truncated download from a path-traversal rejection. Fix: keep both ends (head + tail), or collapse identical runs and always retain the final 3 lines.

Medium — the bound is line-count only; a single long line bypasses it

BrowserStackAccessibilityLint.swift:511-517

messageLines.count > maxMessageLines is the only gate, so few-but-huge lines pass through untouched. Reproduced: one pax entry with a 60,000-character .. path produces 2 stderr lines / 60,092 bytes, and the shipped bounding logic is a verbatim no-op on it — the full ~60 KB reaches the build log, which is the exact outcome this commit's own comment says it prevents.

Fix: add a byte cap alongside the line cap. Note that it must bound on the utf8 view — String.prefix(n) counts characters, so a naive prefix(4096) lets ~4× the cap through on multi-byte input.

Low findings

#	Finding	Location
1	Invalid or mid-scalar UTF-8 makes String(data:encoding:.utf8) return nil, and ?? "" then discards the entire diagnostic, falling back to the generic message. Two reachable paths: a non-UTF-8 filename in a hostile archive, and the byte-wise 64 KB cap slicing a multi-byte sequence (measured: 14 of 129 cut points land mid-scalar). Pre-existing, but on the line this delta rewrites. Fix: String(decoding:as: UTF8.self) — lossy, never nil.	:511
2	The omitted-line count is computed off the 64 KB-retained subset, so it under-reports ~3×: the real 4,000-entry archive emits ~4,001 lines but the marker renders … 1266 further …. Fix: track dropped lines in the drain and word it "at least N".	:516
3	The comment's framing is misleading: by the time line 511 reads stderrData, the 64 KB cap has already bounded it, so the cited ~200 KB volume cannot reach the log at that point. The real benefit is 64 KB → ~1 KB.	:506-510
4	For many-tiny-line inputs the "bounded" excerpt can be longer than the input (41 B → 78 B). Harmless; precision nit only.	:512-517

Previously deferred findings — all still stand, severities held stable

All five remain open and correctly triaged to DEVA11Y-761; none regressed or improved.

Finding	Location	Status
Medium — no SIGKILL escalation after terminate()	:1008	Still stands
Medium — guard ships with zero automated regression coverage	tests/	Still stands
Medium — Windows Expand-Archive path unguarded	:689-698	Still stands
Low — watchdog O(entries) per 50 ms tick; ~50% overshoot	:995-1016	Still stands (documented trade-off)
Low — caps hardcoded in five places, no override	:176-177	Still stands

Impact assessment

Public surface unchanged. Package.swift still exports one product (.plugin(name: "a11y-scan"), fixed verb: "scan"). The delta adds three local lets inside a private func on a private struct, under #if !os(Windows). No declaration, signature, flag, permission, or exit-code change — a no-op at API/ABI level for SPM consumers pinned to branch: "main" (where merging is releasing, since no tags exist).

No programmatic consumer of the message. PluginError is private to this file; the only consumer is the SwiftPM plugin host. The three shell launchers run a completely independent extraction with their own error strings and never observe this message. Nothing in-repo pattern-matches bsdtar error text.

Net observable change: the text of one error, on one branch (bsdtar exited non-zero AND the archive is not already an executable).

Test coverage

Zero coverage for this delta, and the guard as a whole remains untested — extractLocalArchive is private on a private struct in a plugin-only package with no library target, so it is structurally untestable without the extraction refactor already scoped to DEVA11Y-761.

Notes on this review

• code-review-graph MCP was unavailable, so impact analysis fell back to reading, grep, and empirical reproduction. That is absence of data, not evidence of safety.
• Reviewers read a git archive snapshot pinned at 17c3b7aa and sha256-verified against the pushed blob, because the local clone sits on a different branch carrying deferred work not in this PR.

[maunilm]

Claude Code PR Review

Continues the previous review — FULL re-review across six commits since 17c3b7a.

Head: c73f5fb • Reviewers: stack-code-reviewer, stack:devtools-review-changes

Verdict: PASS

No Critical or High findings. Both reviewers independently recommended PASS, and every actionable finding they raised has been fixed in this branch.

Scope note — read this before relying on the verdict

Both reviewers ran against head 0118ee0. Two commits landed after them, implementing their own findings:

• 52f011f — derives maxMessageLines from headLines + tailLines (reviewer 1's Low)
• c73f5fb — the impact review's Medium plus its actionable Lows

Those two carry author validation only, not independent subagent review. They are a constant derivation, comment corrections, workflow-hygiene pins, and test-harness changes; no guard logic changed. Stated explicitly so the human reviewer knows exactly where the independent eyes stopped.

What the six commits do

Commit	Change
13a8111	bsdtar stderr excerpt: head+tail retention, byte cap, lossy UTF-8 decode
1dd9e31	new tests/extraction-guard/ regression suite + CI workflow
3027e92	budget head and tail separately; clamp the tail from its end
0118ee0	fix an intermittent suite failure (fixture-generation race)
52f011f	derive the line threshold so the omitted count can't go negative
c73f5fb	close the impact review's Medium + Lows

Findings raised this round — all fixed

Medium — the oversized-download assertion carried a false rationale. Its comment claimed that removing --max-filesize moves the failure to bsdtar, so asserting "the failure came from curl" proves the flag is present. It does not: the explicit compressed_size > max_compressed backstop fires first and emits "maximum allowed download size" — the second pattern the assertion already accepts. The comment also contradicted the suite's own README. Fixed: reworded to claim only "rejected before extraction", and a fourth faithfulness grep added for --max-filesize itself. That closes the one hole in the mutation matrix — the flag's removal was previously undetectable by any behavioural assertion, and matters because without it a chunked or undeclared-length response can write unbounded bytes to disk before any check runs.

Medium (previous round) — the byte cap could discard the tail it existed to preserve. A blind prefix over the joined head+notice+tail cut inside the head when head lines were large, dropping the notice and the decisive cause. Reproduced at 13,264 B input. Fixed in 3027e92; the naive fix still failed, because clamping the tail with a prefix keeps its beginning while the cause is at its end — hence keepingEnd.

Low — omitted could go negative if headLines were ever bumped alone, printing "at least -3 further messages" to a user. Threshold now derived; the property is algebraic.

Low — start_server accepted any server on its port. A squatting local service would satisfy the readiness probe, every fixture would 404, and the run would fail with confusing per-case errors. Now token-verified with port retry. (The first version of that fix used a fixed token filename, which broke 2 of 4 concurrent cold starts — token files are now per-run.)

Low — fixtures never used production's format. The endpoint serves a .zip and cli.sh names the path BINARY_ZIP_PATH, yet every fixture was .tar.gz. Zip fixture and case added: 51 → 60 assertions.

Low — EXPECTED_MODE read from bash for all three variants. Now per variant.

Low — workflow hygiene. actions/checkout v3.5.3 (Node16, being removed from runner images) → v4.2.2; macos-latest → pinned macos-14; paths: filter added to the push trigger; concurrency group with cancel-in-progress.

Low — two docs-drift items. tests/README.md framed all of tests/ as end-to-end plugin harnesses; and sweepStaleStaging's comment still said the extract helpers forwardExit() past prepareArtifact's defers, which this PR made false. Both corrected.

Verification

Check	Result
Excerpt logic, 11 shapes, extracted verbatim from the file	all ≤4.4 KB, empty semantics preserved, cause + notice retained (226 KB in → 4,249 B)
End-to-end matrix through real extractLocalArchive, 25s hang deadline	6/6, happy path 0.06s, no hangs
Shell suite	60/60 × 3 variants
Concurrency	60/60 across 4 concurrent cold starts, staggered cold start, 6 warm serial runs
Mutation validation	5/5 guard removals caught (was 4/5)
Port-squat defence	confirmed against a decoy server on the target port
Self-update sidecars	all 6 consistent
CI	10/10; the new job has passed 3 consecutive runs

Release impact

Merge = release. No tags exist; SPM consumers pin branch: "main", so this lands at each consumer's next resolve.

Surface unchanged: Package.swift is not in the diff — product a11y-scan, verb scan, and both permissions are identical. No consumer manifest edit, no new permission prompt, no public declaration changed. The scan's own result code still forwards verbatim, so CI gating on scan findings is unaffected.

Observable changes: new hard-fail cases (archive >100 MB, footprint >200 MB or >10,000 entries); bsdtar extraction failure now throws rather than exiting with bsdtar's code (SwiftPM prints error: and exits 1); the archive and staging dir are no longer leaked into the cache on a guard trip; and diagnostics text differs — bounded head+tail excerpt where a single non-UTF-8 byte previously produced an empty message.

Headroom, measured against production: CLI 1.53.0 macos-arm64 is 36.2 MB compressed / 66.2 MB decompressed / 1 entry — 2.8× compressed and 3.0× decompressed headroom. Not a blocker. But there is no override env var, so a future CLI crossing either line breaks every consumer simultaneously. Worth a size assertion in the CLI release process; tracked below.

Still open — deferred to DEVA11Y-761

Finding	Severity
Swift extraction path has no automated coverage (structurally untestable until the library extraction)	Medium
No SIGKILL escalation after terminate()	Medium
Windows Expand-Archive path unguarded	Medium
Watchdog O(entries) per 50 ms tick; ~50% overshoot	Low
Caps hardcoded, no env override (see headroom note above)	Low
Shell path has no entry-count ceiling, unlike Swift's 10,000	Low
Stale .lock after SIGKILL forces a 300s wait then exit 1 (local dev only — CI workspaces are ephemeral)	Low

Notes on this review

• code-review-graph MCP was unavailable both rounds; impact analysis fell back to reading, grep, and empirical reproduction. That is absence of data, not evidence of safety.
• Reviewers read git archive snapshots pinned per head and sha256-verified against the pushed blob, because the local clone sits on a different branch with untracked leftovers.
• The review identity and the PR author are the same GitHub account here, so the skill's human-signal guard cannot distinguish them on this PR.