fix(server): name rejected certificates when linking peers#28060

open1

main

maria/takeover-pr-27128

opened2d agobyMmaria

View on GitHubTwo-way sync enabledPR last imported 1d ago

Problem

Linking an HTTPS peer with a rejected certificate reports that nothing answered as T3 Code. The descriptor probe discards the TLS rejection.

Change

Preserve the origin and Node certificate code when linking. Stored-link status and resolution still skip rejected addresses and verify the next peer before sending credentials. Keep the original implementation and regression coverage, with a smaller scoped HTTPS test listener.

Scope and approval

Contribution triage marked #27128 eligible under policy cb9629e689. This focused fix qualifies under the obvious-bug exception: a rejected peer certificate names the wrong failure cause. Scope remains in peer probing and its regression coverage.

Verification

The real self-signed HTTPS regression fails on current main with not_a_t3_environment and passes with this fix.
Blacksmith: 66 peer tests passed; final simplified listener: 13 module tests passed; vp run check:changed passed formatting, lint, knip, GraphQL, affected typechecks and 14 tests.
Real Chrome client: Settings → Connections → Link, matching address and data before/after. The error now includes DEPTH_ZERO_SELF_SIGNED_CERT.
Two independent code reviews approved head 8c7c1134dc. CI test checks and lint passed. Build and Typecheck is blocked by a main regression: AgentSessionScanner.ts:1203 constructs a TranscriptCandidate without required homePath (TS2741). The same omission exists on current main; this PR does not touch that file. CI log. Macroscope approved, and CodeRabbit returned no actionable code findings. Its description warning is addressed by the scope rationale above.
Unverified: native desktop/mobile clients and non-loopback remote TLS. Mixed picked-machine probe results retain their existing generic message.

Evidence

before: linking misidentifies a rejected certificate as no T3 environment

after: linking names the rejected certificate and TLS code

Takeover

Replaces #27128, with thanks to eyeveil for the original implementation.

Built with GPT-6.1-Sol in the Codex harness on T3 Code.

Checks

2 failing · 21 successful · 8 skipped

Some checks failed

Failing2

Build and TypecheckGitHub
Failed
CheckGitHub
Failed

Added 1 commit2d ago

fix(server): name rejected certificates when linking peers8c7c113

MmacroscopeappBotapproved these changes2d ago

MmacroscopeappBot2d ago

Approvability

Verdict: Approved at 8c7c113

Macroscope's review found this PR approvable — Peer-link failures now identify rejected certificates and their TLS error codes. Existing certificate verification, peer identity checks, and credential-handling safeguards remain unchanged.

^{You can add or adjust custom eligibility rules. Learn more.}

Ggithub-actionsBot2d ago

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ℹ️ The exact PR base did not have a successful artifact. Baseline uses the latest successful main measurement shown below.

Provider	Metric	Main baseline	This PR	Impact	PR ceiling
Codex	Total thread wire	5.0 KiB	5.0 KiB	0 B (0.0%)	6.8 KiB	✅
Codex	Thread snapshot wire	3.8 KiB	3.8 KiB	0 B (0.0%)	4.9 KiB	✅
Codex	Live turn WebSocket wire	1.2 KiB	1.2 KiB	0 B (0.0%)	2.0 KiB	✅
Codex	Live turn WebSocket decoded	21.6 KiB	21.6 KiB	0 B (0.0%)	24.4 KiB	✅
Codex	Live turn messages	2	2	0 (0.0%)	8	✅
Claude	Total thread wire	5.0 KiB	5.0 KiB	0 B (0.0%)	6.8 KiB	✅
Claude	Thread snapshot wire	3.8 KiB	3.8 KiB	0 B (0.0%)	4.9 KiB	✅
Claude	Live turn WebSocket wire	1.3 KiB	1.3 KiB	0 B (0.0%)	2.0 KiB	✅
Claude	Live turn WebSocket decoded	21.9 KiB	21.9 KiB	0 B (0.0%)	24.4 KiB	✅
Claude	Live turn messages	2	2	0 (0.0%)	8	✅

Baseline: 80fd67e · PR result: 8c7c113 · Source CI: failure

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

Codex decoded thread snapshot: 108.5 KiB
Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

Aauditbenchaireviewed2d ago

Open in Audit Bench Ai

🤖 Automated review by Audit Bench Ai, not a personal review. It was posted through the GitHub account connected to Audit Bench Ai. Install the Audit Bench Ai GitHub App to have reviews posted by a separate bot account.

No actionable comments were generated in this review. 🎉

Merge risk: 🟢 Low · up to 8c7c113

The change specifically targets TLS certificate rejections during descriptor probing, while existing link resolution safely catches rejections via Effect.orElseSucceed.

Merge readiness: ⛔ Not ready to merge

Pre-merge checks: ✅ 9 · ❌ 1

❌ Failed checks: 1 error

Check name	Status	Explanation	Resolution
CI checks	❌	2 of 33 failing: Check, Build and Typecheck.	Open the failing runs on GitHub and fix them before merging.

✅ Passed checks: 9 passed

Check name	Status	Explanation
Merge conflicts	✅	Merges cleanly into main.
Secrets	✅	No keys, passwords or tokens in the changed code.
Blocking issues	✅	No critical or high-severity issues in the changed lines.
Tests	✅	Tests were changed alongside the code.
PR size	✅	173 lines changed.
Title check	✅	The title clearly states that rejected certificates will be named when linking peers.
Description check	✅	The description clearly explains the problem, proposed solution, and verification results.
Linked issues check	✅	The PR replaces PR #27128 and directly implements the stated certificate error handling fix.
Out of scope changes check	✅	All changes are focused on certificate error extraction and test coverage for peer links.

📝 Walkthrough

This PR improves peer linking error reporting when connecting to an HTTPS peer with an untrusted or invalid TLS certificate. Instead of silently discarding the TLS error and reporting that no T3 environment answered, the server now traverses the error cause chain to identify Node certificate errors and surface them explicitly. Existing behavior for stored link fallback and peer resolution is preserved via fallback handling in answersAs.

File	Changes
`apps/server/src/peer/PeerLinks.test.ts`	Adds unit and integration tests verifying TLS certificate error extraction and peer linking behavior against an untrusted HTTPS server.
`apps/server/src/peer/PeerLinks.ts`	Extracts TLS certificate error codes from failure causes and fails `describe` with an explicit message instead of returning empty results.

ℹ️ Review info

Commit: 8c7c113
Verdict: ✅ Pass
Findings: none
Review: local checks plus AI review of the changed lines
Scope: Reviewed 2/2 selected files (of 2 total): 2 reviewed by AI, 0 from cache, 0 passed local checks with nothing risky enough to send to AI. Found 0 finding(s), No secrets found.

💬 Reply to any inline comment and Audit Bench Ai will recheck that code and answer in the thread. Mention @auditbench in the conversation to ask about the whole PR.

🛠️ Open the full report and fix with AI

Thanks for using Audit Bench Ai! Free pull request reviews for open source: paste any public PR at auditbenchai.com/scan.

_{Reviewed by Audit Bench Ai · AI code review for security, logic and performance · Review any public PR free}

CcoderabbitaiBot2d ago

Review in Change Stack →

No actionable comments were generated in the recent review.

External image Recent review info

External image Run configuration

Source mode: Auto
Repository: pingdotgg/t3code
Branch: maria/takeover-pr-27128
File: .coderabbit.config.ts
Commit: 8c7c113
Layers: Repository TS config
Review profile: CHILL
Plan: Advanced
Run ID: 953149b6-caa2-4b61-ae89-fd720fc86054

External image Commits

Reviewing files that changed from the base of the PR and between 94a53a2f79c3b61f3a94ac6a97311ce0deb69306 and 8c7c1134dcf3edc51d51ed2522dc066ae5fd3515.

External image Files ignored due to path filters: 2

apps/server/src/peer/fixtures/localhost-cert.pem is excluded by !**/*.pem
apps/server/src/peer/fixtures/localhost-key.pem is excluded by !**/*.pem

External image Files selected for processing: 2

apps/server/src/peer/PeerLinks.test.ts
apps/server/src/peer/PeerLinks.ts

Limit details: You’ve used all 10 included reviews currently available.

Walkthrough

PeerLinks now detects recognized TLS certificate errors, reports them during linking, and treats failed probes as no match during environment checks. Status and resolution checks use environment identity verification, allowing resolution to continue to a later candidate after a certificate rejection.

Changes: Peer link TLS handling

Layer / File(s)	Summary
Certificate error detection and probe handling `apps/server/src/peer/PeerLinks.ts`, `apps/server/src/peer/PeerLinks.test.ts`	`certificateErrorCode` searches nested error causes for recognized TLS certificate codes. `describe` reports certificate failures and maps ordinary probe failures to no descriptor. Tests cover the error codes and untrusted-certificate behavior.
Candidate environment checks `apps/server/src/peer/PeerLinks.ts`, `apps/server/src/peer/PeerLinks.test.ts`	`statusOf` and `resolve` use `answersAs` to verify the expected environment ID. Tests verify that resolution tries a later address after an untrusted certificate.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Possibly related PRs

pingdotgg/t3code#27128: Implements similar TLS certificate detection and peer resolution behavior.
pingdotgg/t3code#26551: Changes preferred-route probing in PeerLinks status and resolution paths.
pingdotgg/t3code#26475: Rechecks peerOrigin for each resolution candidate.

Suggested labels: bugfix, size:M

Suggested reviewers: juliusmarminge

Merge Risk: External image Minimal · up to 8c7c1

No merge-blocking risk was identified in the certificate-handling change.

Pre-merge checks: ^Passed 4

Check name	Status	Explanation
Title check	Passed	The title clearly and concisely describes the main change: naming rejected TLS certificates when linking peers. It uses a conventional commit format.
Linked Issues check	Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	Passed	Check skipped because no linked issues were found for this pull request.
Description check	Passed	The pull request description includes all required sections: Problem, Change, Scope and approval, and Verification. It describes the failure, the fix, approval basis, focused test results, observed be…

Full details: Description check

Explanation

The pull request description includes all required sections: Problem, Change, Scope and approval, and Verification. It describes the failure, the fix, approval basis, focused test results, observed behavior, blocked checks, unverified areas, and evidence. It also includes the required agent and harness details.

Finishing Touches

Generate docstrings

Commit to this branch
Create a new PR

Generate unit tests (beta)

Commit to this branch
Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts