Codex/v1.0.1 release candidate - #71
Closed
Coding-Dev-Tools wants to merge 12 commits into
Closed
Conversation
… ports PinnedHTTPSHandler.https_open read self._check_hostname unconditionally. Python 3.12 folded check_hostname into the SSL context: HTTPSHandler no longer keeps the attribute and HTTPSConnection no longer accepts the keyword, so every request raised AttributeError. build_pinned_https_opener replaces the default handler, so this broke Cloud Sync, session refresh, and managed compute on 3.12/3.13 -- all declared-supported interpreters. Forward the keyword only where it still exists. PinnedHTTPSConnection.set_tunnel validated the raw netloc urllib hands it, so a hosted endpoint with an explicit port behind an HTTPS proxy resolved "cloud.example:8443" verbatim, failed, and set an invalid SNI name. Split host:port with _get_hostport first, the way http.client does. Restore the legacy relay-token fallback in /sync/run: an unreadable cloud session raised immediately, so installations still authenticating with a legacy sync token went from working to a hard 409. Also raise a real OSError instead of `raise None` when the connect loop ends with no recorded error under python -O, where the assert is stripped. Nothing exercised https_open before, which is why CI stayed green on 3.12; both new tests fail against the previous code and pass against this one.
…nt I/O Two review findings from the automated re-review of eaf68e9. set_tunnel pinned only the first vetted address for CONNECT while the direct path retried all of them. A dual-stack endpoint whose first address is unreachable from the proxy therefore failed every hosted request with no fallback. Keep the whole vetted list and retry it, redialing the proxy each attempt because a failed CONNECT leaves the socket unusable. UnicodeError is caught alongside OSError: http.client idna-encodes the tunnel host, which fails on a bare IPv6 literal -- a reason to try the next address rather than abort. access_for_workspace's preflight calls configured() -> _load() before _refresh_lock() can normalize filesystem failures, and _load caught UnsafeStateFile but not plain OSError. An unreadable or stale state mount escaped as an unhandled exception and became an opaque 500, contrary to the behaviour the surrounding comment claims. Convert those to CloudSessionError at the source so every caller benefits, and correct the comment, which described a "connect first" response the code never produced for I/O failures.
…line gate Two further review findings, both verified before fixing. Python 3.12 caches an authority in _tunnel_headers["Host"] when set_tunnel runs (3.11 does not -- confirmed against both interpreters). The retry loop added in 713e2db reassigned _tunnel_host but left that header pinned to the first address, so a strict proxy would reject the retry as a Host/authority mismatch. Rebuild the header for each target, with IPv6 bracketing. AGENTS.md documents `python -m pytest tests/ -q` as a fully offline gate, but relay and cloud URL validation resolve their destination to reject private/reserved targets, so the suite silently required working DNS. With DNS blocked it failed five tests, not the one the review named: the sync-token fail-closed test plus four in test_provider_error_redaction.py and test_relay_device_credentials.py. Stub getaddrinfo in the suite-wide isolation fixture; tests needing a specific resolution result still override it. Verified green with DNS blocked, and the security behaviour of resolving at validation time is unchanged.
Verified against four interpreters before changing anything. Python 3.11 and 3.12 wrap the tunnel host with _wrap_ipv6 and already emit `CONNECT [2606:...]:443`, but 3.9.25 and 3.10.20 serialize _tunnel_host verbatim and emit `CONNECT 2606:...:443` -- an ambiguous authority strict proxies reject. This package supports 3.9, so normalize the target ourselves; 3.11+ leave an already-bracketed value untouched, so one code path is correct everywhere. The new test drives the real http.client._tunnel over a fake socket and asserts the request line, so it covers whichever serialization the interpreter uses. All of 3.9.25, 3.10.20, 3.11.15 and 3.12.10 now emit the bracketed form. Also corrects a comment added in 713e2db: encoding a bare IPv6 tunnel host does not raise, so the UnicodeError guard is defensive rather than the IPv6 handler the comment claimed.
Managed-compute consent now defaults to off, so a default installation's Overview immediately calls /analytics and gets the new 409 consent_required. Only the full Analytics view routed that through managedConsentRequired()/managedConsentHtml(); the Overview fell through to the generic branch and rendered an opaque "managed cloud operation failed" string -- the worst first-run experience for exactly the new default. Mirror the Analytics handling in loadOverviewAnalytics so the card shows the opt-in instructions and a CLOUD pill instead. Reuses the existing s87 style token, so scripts/externalize_dashboard_assets.py reports no CSP drift.
http.client.HTTPConnection.connect raises the http.client.connect audit event before opening its socket, which is how a host process records or blocks outbound connections. PinnedHTTPSConnection overrides connect() and calls _create_connection directly on both the direct and proxy paths, so it never raised that event: every hosted request -- Cloud Sync, session refresh, managed compute -- was invisible to those hooks, silently and with no way to notice. Measured with a real audit hook on 3.11.15 and 3.12.10: stock HTTPSConnection raised 1 event, the pinned client raised 0 on both paths. It now raises 1 on each, matching stock. The direct path names the vetted address actually dialled rather than the hostname, so a hook deciding by address sees the destination the socket really opens; the proxy path names the proxy, which is the socket a tunnelled request actually opens and what stock audits in the same situation. Each retry is audited, so the event count tracks real dials.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 715a016e30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| def _connect_directly(self): | ||
| last_error = None | ||
| for target in _validated_addresses(self.host): |
There was a problem hiding this comment.
When DNS becomes unavailable or resolves to a non-global address after initial endpoint validation, _validated_addresses() raises ValueError here. urllib only wraps OSError from HTTPSConnection.request() as URLError, while the relay, cloud-session, and managed-feature clients catch their documented transport exceptions rather than this ValueError; consequently a transient DNS change or rejected rebind escapes as an unhandled exception/HTTP 500 instead of the bounded “unreachable” result. Convert connection-time validation failures to an OSError/URLError-compatible exception (including the analogous proxy-tunnel path).
Useful? React with 👍 / 👎.
| @@ -229,7 +244,8 @@ def _post_refresh(control_url: str, refresh: str, workspace_id: str, | |||
| exc.read(_MAX_RESPONSE_BYTES + 1) | |||
| if exc.code in {401, 403}: | |||
There was a problem hiding this comment.
When the control plane rejects refresh with HTTP 402 for an inactive entitlement, this condition skips it and raises the default CloudSessionError(status=503). _sync_all() therefore records a temporary outage rather than an authorization denial, so the new total-401/402/403 recovery CTA is not activated even when every workspace is denied. Include 402 in the propagated authorization statuses.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Type
Verification
python -m pytest tests/ -qpassesruff check .passespython -m eval.harness --dataset eval/datasets/sample.jsonl --k 5passes