Skip to content

feat(spike): optional tls feature for the portable-runtime crate - #779

Merged
mashkovd merged 1 commit into
mainfrom
feat/portable-runtime-tls-feature
Oct 23, 2026
Merged

mashkovd merged 1 commit into
mainfrom
feat/portable-runtime-tls-feature

Conversation

@mashkovd

Copy link
Copy Markdown
Member

Why

This is the split decided on #777: slice 3 runs http-only as approved, and TLS lands separately, then gets measured in slice 3b before the #746 verdict. The #777 proposal rejected rustls because the offline implementer cannot produce checksummed Cargo.lock entries, which would leave the dependency to an unreviewed fix-up commit. This PR adds the dependency instead, reviewed, so slice 3b only writes code against an already-locked set.

What

  • Cargo.toml: an optional feature tls = ["dep:rustls", "dep:webpki-roots"], off by default, plus rust-version = "1.85".
    • rustls =0.23.46 uses default-features = false with ring, std and tls12. It uses the ring provider instead of aws-lc-rs, so the cross-build matrix needs no cmake or NASM.
    • webpki-roots =1.0.11 bundles the Mozilla roots, so the single binary does not depend on the host trust store.
  • src/tls.rs (compiled only with tls): client_config() builds a ClientConfig with the ring provider, safe default protocol versions and the bundled roots. One offline unit test covers it. The https upstream path is not wired: that is slice 3b.
  • Cargo.lock: 27 crates with checksums. It was generated on the pinned 1.85.0 toolchain in CI with CARGO_RESOLVER_INCOMPATIBLE_RUST_VERSIONS=fallback cargo generate-lockfile (workflow_dispatch run 39998733759, from a temporary step that this PR no longer contains). Nothing was generated locally.
  • portable-runtime-spike.yml: build, test and clippy with --features tls on both OSes, next to the existing default-build steps. cargo audit already scans the whole lock.
  • README: "std-only by default", the feature, the version choices, and how to regenerate the lock.

Evidence

Run 39998733759 (ubuntu-latest and macos-14) was green with this exact Cargo.toml and lock:

  • default and --features tls build, test and clippy with -D warnings all pass; the tls build runs 42 lib tests (41 + 1);
  • cargo audit scanned 27 crate dependencies against 1351 advisories and found nothing.

Overlap

The #777 implementer PR may touch Cargo.toml, Cargo.lock and portable-runtime-spike.yml. Whichever merges second rebases.

Refs #746, #777

🤖 Generated with Claude Code

https://claude.ai/code/session_01RJN2vCgLpFwMDwXxGJfKMN

Slice 3 of #746 (#777) was approved http-only; the owner wants TLS
measured before the #746 verdict. The offline implementer cannot write
checksummed lock entries, so the dependency lands here, reviewed, and
slice 3b only writes code against it.

rustls with the ring provider avoids cmake/NASM in the cross-build
matrix; webpki-roots keeps the single binary independent of the host
trust store. The feature is off by default, so the default build stays
std-only. Lock generated on the pinned 1.85.0 toolchain in CI run
39998733759 (MSRV fallback resolver); cargo audit clean on 27 crates.

Refs #746, #777

Claude-Session: https://claude.ai/code/session_01RJN2vCgLpFwMDwXxGJfKMN
@claude

claude Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Claude finished @mashkovd's task in 15s —— View job


Review of PR #779 (commit 7b037d1)

  • Read the pinned diff
  • Review Cargo.toml, src/tls.rs, lib.rs, the workflow and the README
  • Submit the verdict

Verdict: approved, with 0 P1, 0 P2 and 0 P3 findings.

  • The tls feature is optional and off by default. Both dependencies are exact-pinned (=0.23.46, =1.0.11), and rustls uses the ring provider with default features off.
  • client_config() is correct. It uses the ring provider, safe default protocol versions, the bundled roots and no client auth. The offline unit test covers it.
  • pub mod tls is gated on cfg(feature = "tls"), so the default build is unchanged.
  • The CI steps use --locked and add build, test and clippy with -D warnings for --features tls. This matches the green run 39998733759 described in the PR body.
  • I reviewed the lockfile only as a diff summary. Its correctness rests on that CI run and on cargo audit.

I did not run builds or tests, since this was a static review. CI runs those as required checks.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No P1/P2 findings (0 P3). Good to merge.

@github-actions

Copy link
Copy Markdown
Contributor

Antigravity (agy) review

Model: gemini-3.8-flash-high · commit 7b037d1.

No significant issues found.

@mashkovd
mashkovd merged commit 0768857 into main Oct 23, 2026
17 checks passed
@mashkovd
mashkovd deleted the feat/portable-runtime-tls-feature branch October 23, 2026 18:05
mashkovd added a commit that referenced this pull request Oct 23, 2026
Main now carries #779: an optional tls feature with exact-pinned
rustls and a 27-entry lock. The report still said rustls could not be
added and that the lock held only the package, and it framed the TLS
measurement as a conditional follow-up. Under the split recorded on
#777 the TLS variant is measured in slice 3b (#780) before the #746
verdict, so the text now says that and keeps the numbers labelled as
the default http-only build.

Refs #777

Claude-Session: https://claude.ai/code/session_018xZPHSuSUp2cnd1HE6ByPD
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.

1 participant