Replace Tinfoil sidecar with in-process Rust SDK#235
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (3)
WalkthroughThe backend replaces the separate Tinfoil Go proxy with an in-process Rust SDK client. Provider routes, health checks, startup configuration, deployment tooling, local workflows, and documentation now use ChangesTinfoil SDK migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant OpenSecret
participant ProviderClient
participant TinfoilSDK
participant TinfoilService
Client->>OpenSecret: OpenAI-compatible request
OpenSecret->>ProviderClient: Build and send ProviderRequest
ProviderClient->>TinfoilSDK: Discover and attest
TinfoilSDK->>TinfoilService: Send provider request
TinfoilService-->>TinfoilSDK: Response or SSE stream
TinfoilSDK-->>ProviderClient: ProviderResponse
ProviderClient-->>OpenSecret: Response or stream
OpenSecret-->>Client: JSON or SSE response
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/nitro-deploy.md`:
- Line 1159: Update the fenced systemd example in the documentation to include
an appropriate language tag, such as ini or systemd, immediately after the
opening fence so it satisfies markdownlint MD040.
In `@entrypoint.sh`:
- Around line 876-879: The failure handling around
tinfoil_proxy_api_key_encrypted extraction must not log secret-bearing buffers.
Remove the log statements that include tinfoil_proxy_api_key_response and the
corresponding decryption output near the related failure block, while retaining
only a non-sensitive error message.
- Around line 901-907: Update the Tinfoil Proxy API key decoding flow to capture
the exit status of base64 -d before accepting tinfoil_proxy_api_key. Reject the
key when decoding fails or produces empty output, preserving the existing error
log and exit behavior.
- Around line 917-923: Update the `/app/opensecret` startup flow to gate
readiness on an actual health/readiness check instead of the fixed `sleep 5`,
and supervise the background process so the entrypoint exits if it terminates
before becoming or while remaining ready. Preserve the existing environment
variables and startup logging while ensuring `socat` starts only after readiness
succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 22b57b6a-0425-4bac-afc1-08b0290c5c77
⛔ Files ignored due to path filters (2)
pcrDev.jsonis excluded by!pcrDev.jsonpcrProd.jsonis excluded by!pcrProd.json
📒 Files selected for processing (5)
docs/nitro-deploy.mddocs/tinfoil-rust-sdk-parity.mdentrypoint.shsrc/main.rssrc/provider_client.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/tinfoil-rust-sdk-parity.md
- src/provider_client.rs
- src/main.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/nitro-deploy.md`:
- Line 1159: Update the fenced systemd example in the documentation to include
an appropriate language tag, such as ini or systemd, immediately after the
opening fence so it satisfies markdownlint MD040.
In `@entrypoint.sh`:
- Around line 876-879: The failure handling around
tinfoil_proxy_api_key_encrypted extraction must not log secret-bearing buffers.
Remove the log statements that include tinfoil_proxy_api_key_response and the
corresponding decryption output near the related failure block, while retaining
only a non-sensitive error message.
- Around line 901-907: Update the Tinfoil Proxy API key decoding flow to capture
the exit status of base64 -d before accepting tinfoil_proxy_api_key. Reject the
key when decoding fails or produces empty output, preserving the existing error
log and exit behavior.
- Around line 917-923: Update the `/app/opensecret` startup flow to gate
readiness on an actual health/readiness check instead of the fixed `sleep 5`,
and supervise the background process so the entrypoint exits if it terminates
before becoming or while remaining ready. Preserve the existing environment
variables and startup logging while ensuring `socat` starts only after readiness
succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 22b57b6a-0425-4bac-afc1-08b0290c5c77
⛔ Files ignored due to path filters (2)
pcrDev.jsonis excluded by!pcrDev.jsonpcrProd.jsonis excluded by!pcrProd.json
📒 Files selected for processing (5)
docs/nitro-deploy.mddocs/tinfoil-rust-sdk-parity.mdentrypoint.shsrc/main.rssrc/provider_client.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/tinfoil-rust-sdk-parity.md
- src/provider_client.rs
- src/main.rs
🛑 Comments failed to post (4)
docs/nitro-deploy.md (1)
1159-1159: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the fenced systemd example.
markdownlintreports MD040 for this fence. Use an appropriate tag such asiniorsystemd.🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 1159-1159: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/nitro-deploy.md` at line 1159, Update the fenced systemd example in the documentation to include an appropriate language tag, such as ini or systemd, immediately after the opening fence so it satisfies markdownlint MD040.Source: Linters/SAST tools
entrypoint.sh (3)
876-879: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not log secret-bearing buffers on failure.
$tinfoil_proxy_api_key_responsecontains encrypted secret material, while$decryption_outputcan contain the plaintext key. Logging either buffer exposes credentials to retained logs. This is the same issue raised previously, but both logs remain in the current code.Also applies to: 893-897
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@entrypoint.sh` around lines 876 - 879, The failure handling around tinfoil_proxy_api_key_encrypted extraction must not log secret-bearing buffers. Remove the log statements that include tinfoil_proxy_api_key_response and the corresponding decryption output near the related failure block, while retaining only a non-sensitive error message.
901-907: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail printf '\n== entrypoint slice ==\n' file='entrypoint.sh' sed -n '890,915p' "$file" printf '\n== locate decoder usage ==\n' rg -n 'base64 -d|tinfoil_proxy_api_key|decrypted_api_key' "$file" printf '\n== base64 malformed-input probe ==\n' python3 - <<'PY' import subprocess, textwrap, shlex cases = [ ("valid", "YWJj"), ("invalid_tail", "YWJj$"), ("invalid_mid", "YW$Jj"), ("invalid_padding", "YQ==="), ("empty", ""), ] for name, s in cases: p = subprocess.run( ["bash", "-lc", f"printf %s {shlex.quote(s)} | base64 -d 2>/dev/null"], capture_output=True, text=True ) print(f"{name}: status={p.returncode} decoded={p.stdout!r}") PYRepository: OpenSecretCloud/opensecret
Length of output: 2826
Check the base64 decoder’s exit status before using the API key.
base64 -dcan emit partial output on malformed input, so the current emptiness-only guard can let a bad key through. Require a zero exit status and non-empty output. entrypoint.sh:901-907🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@entrypoint.sh` around lines 901 - 907, Update the Tinfoil Proxy API key decoding flow to capture the exit status of base64 -d before accepting tinfoil_proxy_api_key. Reject the key when decoding fails or produces empty output, preserving the existing error log and exit behavior.
917-923: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show the relevant section of entrypoint.sh with line numbers sed -n '880,960p' entrypoint.sh | cat -n # Search for supervision/readiness-related helpers and opensecret process handling rg -n -C 2 -e '\b(run_forever|wait|kill -0|curl|health|ready|vsock)\b|/app/opensecret|opensecret' entrypoint.shRepository: OpenSecretCloud/opensecret
Length of output: 11951
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the full file size before reading more if needed wc -l entrypoint.sh # Map the script's structure to find any functions that supervise child processes ast-grep outline entrypoint.sh --view expandedRepository: OpenSecretCloud/opensecret
Length of output: 210
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the restart-on-exit helper and the startup patterns around opensecret and other services sed -n '1,80p' entrypoint.sh | cat -n printf '\n---\n' sed -n '520,580p' entrypoint.sh | cat -n printf '\n---\n' sed -n '840,880p' entrypoint.sh | cat -nRepository: OpenSecretCloud/opensecret
Length of output: 7004
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the end of entrypoint.sh for any wait/monitoring of opensecret or a final exec tail -n 80 entrypoint.sh | cat -n # Search for explicit waits or signal traps in the script rg -n -C 2 -e '^\s*wait\b|trap\b|kill\b|opensecret|/app/opensecret|socat|exec ' entrypoint.shRepository: OpenSecretCloud/opensecret
Length of output: 6247
Add readiness gating and supervise
/app/opensecret.
/app/opensecretis started in the background and only given a fixed 5s delay beforesocatbegins. A slower startup can race the first requests, and an early exit leaves the entrypoint running without a backend.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@entrypoint.sh` around lines 917 - 923, Update the `/app/opensecret` startup flow to gate readiness on an actual health/readiness check instead of the fixed `sleep 5`, and supervise the background process so the entrypoint exits if it terminates before becoming or while remaining ready. Preserve the existing environment variables and startup logging while ensuring `socat` starts only after readiness succeeds.
1 participant
Summary
tinfoil-rsSDK inside OpenSecret, removing one process and HTTP hop from completions and the other Tinfoil-backed hot paths.TINFOIL_API_KEYdirectly to OpenSecret.Before and after network contract
The checked-in Go proxy was captured before removal, then the same credential, model, prompts, temperature, limits, and streaming settings were exercised through the Rust SDK. The normalized decrypted HTTP request sections have no diff.
GET /v1/modelse3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855; HTTP 200; 14 models1815c5ecd2c36834c59f7ce354737ad1b5bc2b93f176068c561f451957c6a717;chat.completion; exactparity-okd15a8b974409567cc3fef3d708406dedb99ee5578edeaf05ecc9dca9168120c8; SSE with terminal[DONE][DONE]Literal packets necessarily differ because the localhost hop is gone and TLS is randomized. The meaningful boundary is method/path/query, forwarded end-to-end headers, exact body bytes, status/schema/content type, and SSE framing. The SDK now talks directly over attested WebPKI plus SPKI-pinned, origin-bound TLS. A known byte-vector body may use
Content-Lengthwhere the old Go path used chunked framing; that is an intentional transport improvement, not an API change. Full analysis is indocs/tinfoil-rust-sdk-parity.md.Reliability and security
Rollout dependency
Merge this PR together with OpenSecretCloud/opensecret-workspaces#8. The updated helper intentionally supports only this SDK backend. Existing feature workspaces must first stop services with the old helper so any running sidecar is terminated, then rebase both repositories before starting services again. The old helper expects recipes removed here; the new helper does not provide the sidecar required by old backend branches.
Verification
a4bedae9858e28d50cba76c4501e2bf058f854eenix build .#default --no-link,nix flake check, entrypoint shell syntax, andgit diff --checkpassedRequired deployment handoff
The new
pcrDev.jsonandpcrProd.jsonmeasurements are committed and reproducibly verified. The corresponding signed entries are not yet inpcrDevHistory.jsonorpcrProdHistory.jsonbecause the authorized signing private key is not available in this environment. An authorized maintainer must append both signed history entries and update any KMS or client trust configuration before deployment.The released SDK currently verifies AMD SEV-SNP enclaves; TDX is not yet implemented. This matches the current Tinfoil deployment and is documented as an explicit future-provider constraint.