fix: finalize v1.0.1 public client release candidate#69
Conversation
![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: 02c7bf5d6d
ℹ️ 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".
… 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eaf68e97f6
ℹ️ 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".
…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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 713e2dbc7a
ℹ️ 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".
…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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eba81f44a8
ℹ️ 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".
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 619599c146
ℹ️ 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".
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4319c58930
ℹ️ 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".
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.
There was a problem hiding this comment.
💡 Codex Review
engraphis/engraphis/cloud_features.py
Line 348 in 715a016
Normalize address-vetting failures during requests
When DNS changes after the initial base-URL validation—such as a rebinding to a private address or a transient lookup failure—PinnedHTTPSConnection raises ValueError from _validated_addresses(), but this handler catches only URLError, TimeoutError, and OSError. The exception therefore bypasses CloudFeatureError and _managed_call, causing managed endpoints such as /analytics and /maintenance/run to return an unstructured 500 instead of the intended bounded transient cloud error; include pinning-time ValueError failures in the transport normalization.
ℹ️ 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".
1 participant
Summary
Finalizes the public Engraphis 1.0.1 client candidate from all local commits that were ahead of
origin/main.Verification
Release boundary
This PR validates the public customer package and client-side Pro/Team surfaces. It does not tag, publish, deploy, enable production signup, or claim that live Stripe/email/Team/backup/alerting canaries have been completed.