refactor(proxy): separate proxy intent from activation - #1199
Conversation
PR Review SummarySize
Affected crates
Blast radius — ContainedThis PR touches: source code Updated automatically on each push to this PR. |
There was a problem hiding this comment.
Code Review
This pull request refactors ProxyLaunchOptions by grouping its flat fields into structured intent types: DomainFilterIntent, EndpointFilterIntent, CredentialProxyIntent, UpstreamProxyIntent, TlsInterceptIntent, and OpenUrlIntent. The boolean active field is replaced with an is_active() helper method. Additionally, validation is added to ensure that infra-only flags (such as --proxy-port or TLS intercept settings) are not used without an activating proxy feature. There are no review comments, so we have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let (plain_entries, endpoint_entries): (Vec<_>, Vec<_>) = allow_domain | ||
| .into_iter() | ||
| .partition(|e| matches!(e, crate::profile::AllowDomainEntry::Plain(_))); |
There was a problem hiding this comment.
PRs good - and the following is only something I know from being tripped up before in this area (but mine was worse as it allowed rather then denied).
This let sorts entries by which enum variant , but there are actually two ways to write a plain "allow this whole domain" entry in a profile:
-
String form → parses to
Plain:
{ "network": { "allow_domain": [ "foo.alwaysfurther.ai" ] } } -
Object form → parses to
WithEndpoints:
{ "network": { "allow_domain": [
{ "domain": "foo.alwaysfurther.ai", "endpoints": [ { "method": "GET", "path": "/v1/**" } ] }
] } }
Now the gotcha: endpoints has #[serde(default)], so you're allowed to write the object form and just leave em off:
{ "network": { "allow_domain": [ { "domain": "cdn.example.com" } ] } }
So you've got two ways to express the exact same intent that produce different internal variants,
the PR's partition keys on the variant (matches!(e, Plain(_))), so the second form gets misfiled into the endpoint bucket, and then dropped downstream.
The rest of the codebase avoids this trap by keying on !endpoints.is_empty() ("does this entry have path rules?") rather than on which variant it happens to be.
So I think its as simples as :
.partition(|e| !matches!(e, AllowDomainEntry::WithEndpoints { endpoints, .. } if !endpoints.is_empty()));
Hope that make sense , the debug_assert below can also get patched up with !endpoints.is_empty()
The rest is all good stuff , so will approve once you fix the above as if my musings are correct this would remove a domain from the proxy allowlist (mine was worse, it allowed a non allowed through).
There was a problem hiding this comment.
Ah I see, yea I tested locally and if endpoints is empty, the filter has nothing to match against so every request is failed rather than treated as a plain domain and passed through. Good spot!!
Replace the flat `ProxyLaunchOptions` struct with focused intent structs: `DomainFilterIntent`, `EndpointFilterIntent`, `CredentialProxyIntent`, `UpstreamProxyIntent`, `TlsInterceptIntent`, and `OpenUrlIntent`. Remove the stored `active: bool` field. Proxy activation is now derived via `is_active()`, which returns true when any activating intent struct is `Some`. `WithEndpoints` allow-domain entries are split from plain CONNECT-tunnel entries at prepare time, making the TLS-intercept requirement explicit in the type. Infra-only flags (`--proxy-port`, `--proxy-ca-validity`, `--trust-proxy-ca`) now return an error if used without an activating proxy feature instead of silently doing nothing. Signed-off-by: Aleksy Siek <aleksy@alwaysfurther.ai>
Signed-off-by: Aleksy Siek <aleksy@alwaysfurther.ai>
…ergence ledger Records the four equivalence findings (D-09 nolabs-ai#1077, D-01 nolabs-ai#1048/nolabs-ai#1091, D-02 nolabs-ai#1151, D-10 nolabs-ai#1132), two won't-sync findings (D-05 nolabs-ai#1192, D-04 nolabs-ai#1199), and the one deliberate fork-divergence (D-07 nolabs-ai#1197, fix 0c08e5d) with their guard-test fn names so future syncs expect the Cluster F divergence and never blind-cherry-pick the tls_intercept/RouteSelection/TlsInterceptIntent hunks (D-11). Signed-off-by: Oscar Mack Jr <oscar.mack.jr@gmail.com>
* refactor(proxy): separate proxy intent from activation Replace the flat `ProxyLaunchOptions` struct with focused intent structs: `DomainFilterIntent`, `EndpointFilterIntent`, `CredentialProxyIntent`, `UpstreamProxyIntent`, `TlsInterceptIntent`, and `OpenUrlIntent`. Remove the stored `active: bool` field. Proxy activation is now derived via `is_active()`, which returns true when any activating intent struct is `Some`. `WithEndpoints` allow-domain entries are split from plain CONNECT-tunnel entries at prepare time, making the TLS-intercept requirement explicit in the type. Infra-only flags (`--proxy-port`, `--proxy-ca-validity`, `--trust-proxy-ca`) now return an error if used without an activating proxy feature instead of silently doing nothing. Signed-off-by: Aleksy Siek <aleksy@alwaysfurther.ai> * fix: review comments Signed-off-by: Aleksy Siek <aleksy@alwaysfurther.ai> * cargo fmt Signed-off-by: Aleksy Siek <aleksy@alwaysfurther.ai> --------- Signed-off-by: Aleksy Siek <aleksy@alwaysfurther.ai>
Linked Issue
Closes #1099
Summary
Replace the flat
ProxyLaunchOptionsstruct with focused intent structs:DomainFilterIntent,EndpointFilterIntent,CredentialProxyIntent,UpstreamProxyIntent,TlsInterceptIntent, andOpenUrlIntent.Remove the stored
active: boolfield. Proxy activation is now derived viais_active(), which returns true when any activating intent struct isSome.WithEndpointsallow-domain entries are split from plain CONNECT-tunnel entries at prepare time, making the TLS-intercept requirement explicit in the type.Infra-only flags (
--proxy-port,--proxy-ca-validity,--trust-proxy-ca) now return an error if used without an activating proxy feature instead of silently doing nothing.Test Plan
Checklist
CHANGELOG.mdif needed