Skip to content

Verify runtime archive checksums before extraction#1949

Merged
danielmeppiel merged 4 commits into
microsoft:mainfrom
fallintoplace:fix/runtime-checksum-verification
Jul 4, 2026
Merged

Verify runtime archive checksums before extraction#1949
danielmeppiel merged 4 commits into
microsoft:mainfrom
fallintoplace:fix/runtime-checksum-verification

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

  • fetch release metadata for the selected runtime version before downloading
  • verify the downloaded archive against the published SHA-256 digest before extraction
  • add a hermetic regression test that covers both matching and mismatched digests

Testing

  • uv run --extra dev pytest -q tests/unit/test_runtime_setup_codex_script.py
  • uv run --extra dev pytest -q tests/unit/test_runtime_manager.py tests/unit/test_runtime_windows.py

Copilot AI review requested due to automatic review settings June 28, 2026 21:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR strengthens the Codex runtime setup flow by fetching GitHub release metadata up front and verifying a downloaded runtime archive’s SHA-256 digest before extracting it, with a new hermetic regression test to cover both success and failure cases.

Changes:

  • Add GitHub release-metadata fetch + SHA-256 verification to setup-codex.sh before tar extraction.
  • Add hermetic unit tests that stub curl and validate both matching and mismatched digests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
scripts/runtime/setup-codex.sh Fetch release metadata, extract published digest, verify archive checksum prior to extraction, and improve temp-dir handling.
tests/unit/test_runtime_setup_codex_script.py New hermetic tests for checksum verification behavior using a fake curl and a generated tarball.

Comment thread scripts/runtime/setup-codex.sh Outdated
Comment on lines +202 to +203
temp_dir="$(mktemp -d "${TMPDIR:-/tmp}/apm-codex-install.XXXXXX")"
trap "rm -rf '$temp_dir'" EXIT
Comment on lines +152 to +158
if [[ "$normalized_actual" != "$normalized_expected" ]]; then
log_error "Checksum verification failed for downloaded Codex archive."
exit 1
fi

log_success "Verified Codex archive checksum"
}
Comment thread scripts/runtime/setup-codex.sh Outdated
Comment on lines +57 to +68
awk -v asset_name="$asset_name" '
index($0, "\"name\": \"" asset_name "\",") > 0 { in_asset=1; next }
in_asset && /"digest":/ {
gsub(/.*"digest":[[:space:]]*"/, "")
gsub(/".*/, "")
print
exit
}
in_asset && /"browser_download_url":/ {
exit
}
'
@danielmeppiel danielmeppiel added panel-review Trigger the apm-review-panel gh-aw workflow and removed panel-review Trigger the apm-review-panel gh-aw workflow labels Jun 30, 2026
@github-actions

Copy link
Copy Markdown

APM Review Panel: ship_with_followups

Runtime SHA-256 checksum verification is a ship-worthy security improvement; 3 regression-trap test gaps and a silent token-in-argv exposure are the priority follow-ups.

cc @minhvu -- a fresh advisory pass is ready for your review.

PR #1949 delivers a real, first-iteration supply-chain integrity check at the runtime download layer. The Supply Chain Security Expert confirms the threat model is correctly scoped: the check defends against CDN-layer tampering and transit corruption; the circular-trust limitation (digest and archive URL share the same GitHub API trust root) is acknowledged and acceptable given current ecosystem constraints. The mktemp + trap fix materially tightens the TOCTOU window. The happy-path and mismatch-rejection tests are hermetically constructed and prove the core promise holds on this commit -- checksum fires before extraction, a mismatch aborts the install, and the shell harness is reproducible.

The test-coverage-expert returned three outcome: missing findings on the secure_by_default surface, and these outrank all opinion-only recommended findings per the panel's evidence-weighting contract. The three gaps are: (1) the authenticated fetch path -- GITHUB_TOKEN is popped before either test runs, so the auth branch is dead code in CI; (2) the empty-digest guard -- an asset absent from metadata is never exercised, meaning the guard could be accidentally removed with no signal; (3) the malformed-digest format guard -- the mismatch test uses a valid 64-hex-char string, so the regex branch is never entered. All three are regression traps on the security core of this PR and belong in the same release cycle, not a distant backlog.

Two further items deserve tracking. The Auth Expert's finding that curl -H 'Authorization: Bearer $token' places the raw token in the process argument list -- visible via ps aux on shared CI runners and developer workstations -- is a non-trivial real-world exposure on a security hardening PR. The fix (curl --config with process substitution) is well-understood and belongs in a dedicated auth hardening issue. Separately, the [Unreleased] ### Security section in CHANGELOG.md already exists with an attribution pattern. A two-line entry with @minhvu credit costs nothing, satisfies the community-over-feature-count principle, and anchors the 'Secure by default' positioning narrative.

Dissent. The DevX UX Expert (add CODEX_SKIP_VERIFY escape hatch; recommended) and the Supply Chain Security Expert (hard failure is more secure) are in mild tension. I side with the Security Expert for this first iteration -- strict verification by default is correct (fail loudly, predictably). The escape hatch belongs in a follow-up PR with an explicit written trade-off in the CHANGELOG, not as a condition of this merge.

Aligned with: Secure by default (advances promise at the binary-fetch boundary -- first concrete proof point at the download layer), OSS community-driven (P7: external contributor @minhvu delivers a security feature; CHANGELOG attribution is the P7 obligation), and Pragmatic as npm (UX floor requires a skip-verify escape hatch before the install experience degrades at scale -- follow-up PR, not a merge gate).

Growth signal. PR #1949 pairs with Executable Trust Governance (0.22.0) for a credible 'supply chain security' story arc in the next release post. The unlock is a single CHANGELOG [Unreleased] ### Security entry with @minhvu attribution -- this gives the contributor public credit and gives the release drafter a concrete proof point for the 'Secure by default' tagline. Community security PRs earned at zero internal research cost are worth amplifying explicitly rather than letting them dissolve into the commit log.

Panel summary

Persona B R N Takeaway
Python Architect 0 2 2 Solid checksum verification pipeline with good guard-clause discipline; 3 new failure paths lack test coverage and awk JSON parsing is undocumented.
CLI Logging Expert 0 2 3 Checksum failure omits actual/expected values and recovery guidance; auth fallback is now fully silent; pre-existing emoji symbols in log helpers are out of scope but worth tracking.
DevX UX Expert 0 3 2 Good security intent but no --skip-verify escape hatch, non-actionable error on missing digest, and fragile awk JSON parser create brittle first-install UX.
Supply Chain Security Expert 0 2 3 PR meaningfully improves install-time integrity; circular trust and TOCTOU window are the notable residual risks, both acceptable given available ecosystem constraints.
OSS Growth Hacker 0 2 1 Supply-chain security win that earns the README 'Secure by default' tagline; CHANGELOG entry missing to tell the story and credit the community contributor.
Auth Expert 0 2 1 Core auth refactor is sound; main gaps are process-list token exposure and complete loss of auth diagnostic logging from the old code.
Doc Writer 0 2 1 CHANGELOG Security entry missing; new checksum error message has no troubleshooting doc; runtime-compatibility.md Codex section omits integrity step.
Test Coverage Expert 0 3 0 Checksum-verify and mismatch-reject both tested hermetically; auth fetch path, empty-digest guard, and malformed-digest-format guard have no regression tests.
Performance Expert -- -- -- inactive

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

  1. [Test Coverage Expert] Add test for authenticated fetch path: inject GITHUB_TOKEN, assert Authorization header is sent to GitHub API -- outcome:missing on secure_by_default surface; the auth branch is dead code in both existing tests -- a regression in token auth logic produces no CI signal
  2. [Test Coverage Expert] Add test for empty archive_digest guard: feed metadata JSON that omits the target platform asset -- outcome:missing on secure_by_default surface; guard could be removed and the script would trip a confusing downstream error with no CI catch
  3. [Test Coverage Expert] Add test for malformed-digest format guard: inject digest='sha256:notahex_short' and assert distinct exit message -- outcome:missing on secure_by_default surface; the format-validation regex branch is never entered -- truncated or corrupt digests would reach the wrong error path undetected
  4. [Auth Expert] Move GITHUB_TOKEN off curl argv via --config / process substitution to prevent ps aux exposure -- raw token in process argument list is visible to other users on shared CI runners and developer workstations; the fix is well-understood and belongs in a dedicated auth hardening issue
  5. [OSS Growth Hacker] Add CHANGELOG [Unreleased] ### Security entry with @minhvu attribution and Verify runtime archive checksums before extraction #1949 reference before next tag -- P7 obligation (community contributor deserves public credit) and growth unlock (anchors Secure by default positioning with a concrete proof point for the release post)

Architecture

classDiagram
    direction LR
    class SetupCodexScript {
        <<IOBoundary>>
        +setup_codex()
    }
    class FetchLayer {
        <<ChainOfResponsibility>>
        +fetch_release_metadata(url)
        +fetch_github_api(url, use_auth)
    }
    class ParseLayer {
        <<Pure>>
        +extract_release_tag()
        +extract_release_asset_digest(asset_name)
    }
    class VerifyLayer {
        <<Strategy>>
        +sha256_file(path)
        +verify_archive_checksum(path, digest)
    }
    class SetupCommon {
        <<IOBoundary>>
        +download_file(url, dest, name)
        +detect_platform()
        +verify_binary(path, name)
    }
    class TestSuite {
        <<TestDouble>>
        +test_verifies_checksum_before_extracting()
        +test_rejects_mismatched_checksum()
        -_write_fake_curl(path)
        -_run_setup(tmp_path, release_json, tarball)
    }
    SetupCodexScript *-- FetchLayer : calls
    SetupCodexScript *-- ParseLayer : pipes stdin
    SetupCodexScript *-- VerifyLayer : calls
    SetupCodexScript ..> SetupCommon : sources
    TestSuite ..> SetupCodexScript : subprocess
    FetchLayer ..> ParseLayer : validates response
    class FetchLayer:::touched
    class ParseLayer:::touched
    class VerifyLayer:::touched
    class TestSuite:::touched
    classDef touched fill:#fff3b0,stroke:#d47600
Loading
flowchart TD
    A([setup_codex]) --> B[FS: mktemp -d + trap cleanup EXIT]
    B --> C[fetch_release_metadata]
    C --> D{token env var set?}
    D -- yes --> E[NET: fetch_github_api with Bearer token]
    E --> F{extract_release_tag non-empty?}
    F -- yes --> G[return authenticated metadata JSON]
    F -- no --> H[NET: fetch_github_api no auth header]
    D -- no --> H
    H --> I{extract_release_tag non-empty?}
    I -- no --> J[exit 1: Failed to fetch release metadata]
    I -- yes --> G
    G --> K[extract_release_tag via grep+sed]
    K --> L[extract_release_asset_digest via awk line-scan]
    L --> M{archive_digest empty?}
    M -- yes --> N[exit 1: Failed to find checksum metadata]
    M -- no --> O[NET: download_file from github.com releases]
    O --> P[verify_archive_checksum]
    P --> Q[normalize: strip sha256 prefix + tr to lowercase]
    Q --> R{matches regex 0-9a-f x64?}
    R -- no --> S[exit 1: invalid SHA-256 digest format]
    R -- yes --> T[IO: sha256_file via sha256sum or shasum -a 256]
    T --> U{actual hex == expected hex?}
    U -- no --> V[exit 1: Checksum verification failed - binary NOT installed]
    U -- yes --> W[log_success: Verified Codex archive checksum]
    W --> X[FS: tar -xzf archive -C temp_dir]
    X --> Y[FS: mv codex to runtimes/codex + rm -rf temp_dir]
    Y --> Z([configure + verify_binary + done])
Loading

Recommendation

The core security promise holds on this commit: checksum verification fires before extraction, a mismatch aborts the install, and the hermetic test harness is well-constructed. No panelist returned a blocking finding. Merge this. The five follow-ups above are the right next-cycle targets -- the three regression-trap tests should land before the next release tag (same cycle, not a distant backlog), the token-in-argv fix should become a dedicated auth hardening issue filed at merge time, and the CHANGELOG entry should be added before the release post goes out. None of these gate the merge.


Full per-persona findings

Python Architect

  • [recommended] 3 new failure paths in shell functions have no test coverage at tests/unit/test_runtime_setup_codex_script.py
    The PR exercises happy-path (checksum match) and mismatch, but leaves three distinct exit paths untested: (1) fetch_release_metadata failing entirely (both curl attempts exhausted); (2) archive_digest empty because release metadata lacks a digest field for the asset; (3) verify_archive_checksum rejecting a syntactically invalid digest.
    Suggested: Add test_setup_codex_rejects_missing_digest, test_setup_codex_rejects_invalid_digest_format, and test_setup_codex_handles_metadata_fetch_failure.

  • [recommended] extract_release_asset_digest awk parser assumes GitHub API key order; coupling is undocumented at scripts/runtime/setup-codex.sh:54
    The awk script relies on GitHub serializing JSON with 'name' before 'digest' within each asset object. If GitHub inserts additional fields between them in a future response, the parser silently returns empty, causing setup to abort with 'Failed to find checksum metadata'.
    Suggested: Add a comment documenting the assumed key order and add a unit test piping a representative GitHub API JSON through the bash function.

  • [nit] trap string uses single-quote-in-double-quote; cleanup function is the safer idiom at scripts/runtime/setup-codex.sh:203
    Suggested: _apm_codex_cleanup() { rm -rf "$temp_dir"; }; trap _apm_codex_cleanup EXIT

  • [nit] fetch_release_metadata silently discards auth failure with no diagnostic before unauthenticated fallback at scripts/runtime/setup-codex.sh:102
    Suggested: Add log_warning "Authenticated GitHub API request failed; retrying without credentials..." before the unauthenticated fallback.

CLI Logging Expert

  • [recommended] Checksum mismatch error omits actual/expected digests and provides no recovery guidance at scripts/runtime/setup-codex.sh:152
    verify_archive_checksum() has both normalized_expected and normalized_actual in scope. Without them the user cannot assess severity (network bit-flip vs supply-chain tampering) and has no actionable next step.
    Suggested: log_error "Checksum mismatch for $(basename "$archive_path"): expected $normalized_expected, got $normalized_actual." followed by a retry guidance line.

  • [recommended] Two checksum-setup exits provide no recovery path for the user at scripts/runtime/setup-codex.sh:234
    Lines 145 and 234 both exit 1 with no hint of what to try next -- upgrade version, retry, file a bug.
    Suggested: Append actionable follow-up text to both error messages.

  • [nit] Auth fallback path is now fully silent; loss of visibility into authenticated vs unauthenticated fetch at scripts/runtime/setup-codex.sh:102

  • [nit] release_tag null-check at line 221 is effectively unreachable dead code at scripts/runtime/setup-codex.sh:221
    Suggested: Remove the guard, or add a comment: # defensive: fetch_release_metadata() already validates non-empty tag.

  • [nit] Pre-existing emoji symbols in log helpers violate ASCII-only convention (not introduced by this PR) at scripts/runtime/setup-common.sh:20

DevX UX Expert

  • [recommended] Hard failure on missing digest field with no --skip-verify escape hatch blocks install for any release where GitHub API omits the digest at scripts/runtime/setup-codex.sh:233
    GitHub's release asset API does not guarantee a 'digest' field on all releases. Users pinning older versions get a hard exit with no recovery. Every comparable tool (npm --no-integrity-check, pip --no-verify) exposes an escape hatch.
    Suggested: Add CODEX_SKIP_VERIFY env var or --skip-verify flag. When set, skip the digest check and emit a log_warning.

  • [recommended] Error message 'Failed to find checksum metadata for $archive_name.' is non-actionable at scripts/runtime/setup-codex.sh:234
    Violates the 'failure mode is the product' principle -- no hint of what CODEX_SKIP_VERIFY or minimum version is.
    Suggested: Expand to two lines: reason + next step ('To install without verification, set CODEX_SKIP_VERIFY=1 and re-run.').

  • [recommended] Fragile awk JSON parser assumes pretty-printed GitHub API response with trailing comma -- silently returns empty on minified or reordered JSON at scripts/runtime/setup-codex.sh:57
    Suggested: Prefer jq when available; fix awk to not require trailing comma.

  • [nit] Specific-version path starts network fetch silently with no progress log at scripts/runtime/setup-codex.sh:211

  • [nit] Checksum mismatch error omits actual and expected digests at scripts/runtime/setup-codex.sh:153

Supply Chain Security Expert

  • [recommended] Digest and download URL share the same GitHub API trust root (circular trust) at scripts/runtime/setup-codex.sh:232
    A GitHub release compromise lets an attacker poison both simultaneously. The check defends against CDN-only tampering and transit corruption, but the limitation should be documented in code.
    Suggested: Add a comment explaining what the verification does and does not cover. When openai/codex publishes SLSA provenance or sigstore attestations, upgrade to verify against that independent source.

  • [recommended] TOCTOU window between checksum verify (line 244) and tar extraction (line 248) at scripts/runtime/setup-codex.sh:244
    verify_archive_checksum reads $tar_file, then tar reads it again independently. mktemp significantly reduces attack surface but does not eliminate the window.
    Suggested: Open a single fd at download time and pass /dev/fd/N to both sha256sum and tar.

  • [nit] Authenticated fetch failure is silently swallowed before unauthenticated fallback at scripts/runtime/setup-codex.sh

  • [nit] Trap form does not guard against empty temp_dir
    Suggested: trap 'rm -rf "${temp_dir:?}"' EXIT (single-quotes defer expansion; :? causes handler to error rather than expand to empty).

  • [nit] tar extraction lacks --no-same-owner, relevant if script is run under sudo at scripts/runtime/setup-codex.sh:248
    Suggested: tar --no-same-owner -xzf "$tar_file" -C "$temp_dir"

OSS Growth Hacker

  • [recommended] No CHANGELOG entry -- community security contribution goes unrecognized and unmarketed at CHANGELOG.md
    An active [Unreleased] ### Security section exists with attribution precedent. Without an entry, the security narrative loses a concrete data point and the contributor gets zero public credit.
    Suggested: Add under [Unreleased] ### Security: 'Codex runtime setup now fetches the publisher-signed SHA-256 digest from GitHub release metadata and verifies the downloaded archive before extraction; a mismatch aborts the install. (by @minhvu; Verify runtime archive checksums before extraction #1949)'

  • [recommended] Hard failure on missing digest has no user-actionable guidance -- dead-end for first-run users in constrained environments at scripts/runtime/setup-codex.sh
    Suggested: Add guidance line referencing minimum version with digest support and GITHUB_TOKEN rate limit advice.

  • [nit] Success log omits the digest -- visible security evidence is free marketing at scripts/runtime/setup-codex.sh
    Suggested: Include leading 16 hex chars: log_success "Verified Codex archive checksum (sha256:${normalized_actual:0:16}...)"

Auth Expert

  • [recommended] Raw token value placed in process argv via -H/-header flag, visible in ps aux on shared machines at scripts/runtime/setup-codex.sh:86
    curl -fsSL -H "Authorization: Bearer $token" embeds the token in the process argument list. Any user with access to /proc/(pid)/cmdline or ps aux sees the full token.
    Suggested: curl -fsSL --config <(printf 'header = "Authorization: Bearer %s"\n' "$token") "$url" to keep the token off argv. For wget, only support authenticated calls via the curl path.

  • [recommended] Auth failure is completely silent; removes old-code token-source logging, makes 401/403/network-error indistinguishable at scripts/runtime/setup-codex.sh:107
    A 401 (expired token), 403 (scope insufficient), and a transient network error all look identical -- the function silently falls through. The fallback 'check your internet connection' message is actively misleading when the real cause is a bad token.
    Suggested: Add log_info "Fetching release metadata (authenticated)" before auth attempt; log_warning "Authenticated fetch failed, retrying without auth" before fallback.

  • [nit] GH_TOKEN omitted from fallback chain; inconsistent with Python token_manager.py TOKEN_PRECEDENCE at scripts/runtime/setup-codex.sh:79
    Suggested: Add elif [[ -n "${GH_TOKEN:-}" ]]; then token="$GH_TOKEN" after GITHUB_APM_PAT check.

Doc Writer

  • [recommended] CHANGELOG.md [Unreleased] ### Security section has no entry for this PR at CHANGELOG.md:34
    An active ### Security subsection exists. SHA-256 archive verification is a supply-chain hardening change security reviewers need to find there.
    Suggested: Add under [Unreleased] ### Security: entry describing the feature with author attribution (by @minhvu) (Verify runtime archive checksums before extraction #1949).

  • [recommended] Error 'Failed to find checksum metadata for (archive)' has no documented recovery path at docs/src/content/docs/integrations/runtime-compatibility.md:268
    Suggested: Add a Troubleshooting entry in runtime-compatibility.md explaining the cause (older releases pre-date digest metadata) and the fix (upgrade to rust-v0.118.0 or newer).

  • [nit] Codex 'This automatically:' list in runtime-compatibility.md omits the new integrity step at docs/src/content/docs/integrations/runtime-compatibility.md:93
    Suggested: Add bullet: 'Verifies the SHA-256 checksum of the downloaded archive against the published GitHub release digest before extraction'

Test Coverage Expert

  • [recommended] Authenticated fetch path in fetch_release_metadata() is dead code in both tests at tests/unit/test_runtime_setup_codex_script.py
    _run_setup() pops GITHUB_TOKEN and GITHUB_APM_PAT before invoking the script, so the auth branch is never entered. A regression in token auth logic produces no CI signal.
    Proof (missing): tests/unit/test_runtime_setup_codex_script.py::test_setup_codex_uses_token_for_metadata_fetch -- proves: When GITHUB_TOKEN is set, APM sends Authorization header to GitHub API for Codex release metadata. [secure-by-default]
    assert 'auth=yes' in (tmp_path / 'curl.log').read_text()

  • [recommended] Empty archive_digest guard (asset absent from metadata) has no regression test at tests/unit/test_runtime_setup_codex_script.py
    The guard at line 233 prevents proceeding with an empty digest but is never exercised. If removed, the script would trip a confusing downstream error.
    Proof (missing): tests/unit/test_runtime_setup_codex_script.py::test_setup_codex_aborts_when_digest_absent_from_metadata -- proves: When GitHub API lacks a digest for the target platform, APM exits non-zero with 'Failed to find checksum metadata'. [secure-by-default]
    assert result.returncode != 0 and 'Failed to find checksum metadata' in result.stdout + result.stderr

  • [recommended] verify_archive_checksum malformed-digest-format guard is never exercised at tests/unit/test_runtime_setup_codex_script.py
    The mismatch test uses '0'*64 (valid hex). The format-validation regex branch is never entered -- truncated digests would reach the wrong error path undetected.
    Proof (missing): tests/unit/test_runtime_setup_codex_script.py::test_setup_codex_rejects_malformed_digest_format -- proves: When GitHub API returns a non-hex digest, APM exits with a message distinguishing format failure from value mismatch. [secure-by-default]
    assert result.returncode != 0 and 'did not include a valid SHA-256 digest' in result.stdout + result.stderr

Performance Expert -- inactive

PR #1949 modifies only scripts/runtime/setup-codex.sh (a one-time runtime setup helper) and tests/unit/test_runtime_setup_codex_script.py, which are entirely outside the APM dependency resolution, cache, and install hot paths that define this reviewer's activation criteria.

This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.

Generated by PR Review Panel for issue #1949 · 420.5 AIC · ⌖ 12 AIC · ⊞ 5.5K ·

@github-actions github-actions Bot removed the panel-review Trigger the apm-review-panel gh-aw workflow label Jun 30, 2026
@fallintoplace

Copy link
Copy Markdown
Contributor Author

I will have a look at this PR later with these new findings and push the fix.

fallintoplace and others added 3 commits June 30, 2026 18:30
Fold shepherd review follow-ups for PR microsoft#1949 by making release digest extraction tolerate jq/minified JSON, documenting the GitHub Releases digest dependency, improving checksum failure recovery output, adding GH_TOKEN metadata auth, and moving the tamper proof into the integration suite with shared hermetic helpers.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel

danielmeppiel commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

APM Review Panel: ship_now

Fail-closed SHA-256 verification of Codex runtime archives before extraction closes a supply-chain integrity gap with hermetic test proof.

cc @fallintoplace @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

All panel lenses converge: the PR verifies Codex runtime archive integrity before extraction, fails closed when digest metadata is absent or mismatched, and now carries integration-tier proof plus mutation-break evidence. Supply-chain review found no TOCTOU gap and no token leakage. Test coverage confirmed the tampered-archive subprocess test runs hermetically on macOS and fails when the checksum guard is neutered.

Aligned with: secure by default: archive integrity is verified before extraction; oss community driven: CHANGELOG credits @fallintoplace; pragmatic as npm: one metadata API call plus SHA-256 hashing is acceptable for one-shot setup.

Growth signal. The security entry is release-note mineable for enterprise evaluators looking for runtime-download integrity guarantees.

Panel summary

Persona B R N Takeaway
Python Architect 0 0 1 Functional decomposition is sound; shared test helpers now avoid unit/integration coupling.
CLI Logging Expert 0 0 1 New checksum output is ASCII and token-not-in-output coverage is solid.
DevX UX Expert 0 1 2 Fail-closed checksum UX is right; mismatch and metadata errors now include recovery hints.
Supply Chain Security Expert 0 0 0 SHA-256 fail-closed before extraction is correct; no TOCTOU, no token leakage.
OSS Growth Hacker 0 0 1 Security changelog entry now captures the trust signal.
Auth Expert 0 0 2 Release metadata auth is scoped and non-leaking; GH_TOKEN support was added.
Doc Writer 0 1 0 Runtime docs now mention Codex checksum verification and fail-closed behavior.
Test Coverage Expert 0 0 1 Tampered-archive rejection has integration-tier proof; module-level skip was added.
Performance Expert 0 0 1 Extra metadata RTT and hash are immaterial for one-shot setup.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 1 follow-up

  1. [CLI Logging Expert] Repo-wide runtime logging cleanup -- migrate setup-common.sh logging helpers to ASCII status markers across all runtime setup scripts. Deferred because it predates this PR and affects multiple runtime scripts outside checksum scope.

Folded in this run

  • (copilot) Safer temp-dir cleanup trap instead of interpolating temp_dir into the trap string -- resolved in 2b91a16.
  • (copilot) New checksum success output should stay ASCII on the checksum path -- resolved in 2b91a16.
  • (copilot) Release asset digest parsing should tolerate JSON formatting changes -- resolved in 2b91a16.
  • (panel) Prefer jq for digest extraction with an awk fallback that handles minified JSON -- resolved in 2b91a16.
  • (panel) Document the GitHub Releases asset.digest dependency in code and docs -- resolved in 2b91a16.
  • (panel) Prove auth tokens are sent only in headers and never appear in output/logs -- resolved in 2b91a16.
  • (panel) Add integration-tier tamper proof for checksum refusal before extraction -- resolved in 2b91a16.
  • (panel) Move shared runtime setup test helpers out of unit tests -- resolved in 2b91a16.
  • (panel) Add GH_TOKEN metadata-auth support and coverage -- resolved in 2b91a16.
  • (panel) Improve checksum and missing-digest failure messages with recovery hints -- resolved in 2b91a16.
  • (panel) Add CHANGELOG security entry with contributor credit -- resolved in 2b91a16.
  • (panel) Update apm runtime setup docs and apm-guide usage notes -- resolved in 2b91a16.

Copilot signals reviewed

  • scripts/runtime/setup-codex.sh:203 -- LEGIT: cleanup trap interpolation was brittle; replaced with a cleanup function that quotes the variable at execution time (resolved in 2b91a16).
  • scripts/runtime/setup-codex.sh:158 -- LEGIT: the new checksum success path needed ASCII-safe output; replaced with [+] output for that path (resolved in 2b91a16).
  • scripts/runtime/setup-codex.sh:68 -- LEGIT: the original awk parser required a trailing comma and stable formatting; added jq-first parsing plus a more tolerant awk fallback (resolved in 2b91a16).

Deferred (out-of-scope follow-ups)

  • (panel) Migrate all runtime setup logging helpers in setup-common.sh to ASCII markers -- scope boundary: PR scope is Codex checksum verification; this is a cross-runtime output cleanup affecting all setup scripts.

Regression-trap evidence (mutation-break gate)

  • tests/integration/test_runtime_setup_codex_script.py::test_setup_codex_refuses_tampered_archive_before_extracting -- deleted if [[ "$normalized_actual" != "$normalized_expected" ]]; then; test FAILED as expected; guard restored.

Lint contract

uv run --extra dev ruff check src/ tests/, uv run --extra dev ruff format --check src/ tests/, uv run --extra dev python -m pylint --disable=all --enable=R0801 --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/, and bash scripts/lint-auth-signals.sh all passed locally.

CI

Fresh CI on head 2b91a16 is green: CI run 28718324836 passed Lint, Build & Test Shard 1, Build & Test Shard 2, APM Self-Check, PR Binary Smoke, and Coverage Combine; CodeQL run 28718324823 passed Analyze (python/actions); NOTICE Drift Check run 28718324825 passed; Merge Gate run 28718324855 passed. Local integration suite: uv run --extra dev pytest tests/integration -q -> 10112 passed, 242 skipped, 16 deselected, 2 xfailed in 428.53s (0:07:08).

Mergeability status

Captured from gh pr view 1949 --json mergeable,mergeStateStatus,statusCheckRollup immediately after the last push of this run.

PR head SHA CEO stance iters folds defers Copilot rounds CI mergeable mergeStateStatus notes
#1949 2b91a16 ship_now 1 12 1 2 green MERGEABLE BLOCKED pending required review

Recommendation

All in-scope findings were folded, the integration proof is mutation-verified, the full integration suite passed locally, and fresh GitHub CI is green. Ready for maintainer review.


Full per-persona findings

Python Architect

  • [nit] Integration test imported private unit helpers. Folded by extracting shared helpers to tests/utils/runtime_setup_codex.py.

CLI Logging Expert

  • [nit] Raw [+] echo is visually inconsistent with existing setup-common.sh color helpers. Deferred as repo-wide runtime logging cleanup.

DevX UX Expert

  • [recommended] Checksum mismatch error omitted expected/actual digest prefixes and recovery action. Folded.
  • [nit] Digest-absent and malformed-digest errors lacked next action. Folded.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

  • [nit] Add CHANGELOG security entry. Folded.

Auth Expert

  • [nit] GH_TOKEN was not consulted for metadata fetch. Folded.
  • [nit] GITHUB_TOKEN precedence differs from APM module-token precedence. Accepted for GitHub Models/runtime metadata context.

Doc Writer

  • [recommended] Document Codex runtime checksum verification. Folded.

Test Coverage Expert

  • [nit] Integration test should have module-level win32 skip marker. Folded.

Performance Expert

  • [nit] Pinned-version path adds one metadata API RTT. Accepted as the integrity-verification tradeoff.

This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.

@danielmeppiel danielmeppiel merged commit de5b06b into microsoft:main Jul 4, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants