Conversation
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
This focused network-proxy correctness fix is valid for OpenShell, but the boundary-preserving implementation introduces one reachable resource-amplification regression that must be addressed before pipeline handoff.
Action required: @pimlock, preserve the request boundary without byte-at-a-time framing I/O, or add a conservative tested bound on aggregate framing/chunk amplification.
Blocking findings:
GATOR-5cc8e2f8-01: chunked framing is relayed with one read and write per byte.
Carried findings:
- None
Non-blocking suggestions:
- None
Gator metadata
- Validation: Localized correctness and security fix for the supported REST network-proxy path, with a concrete reproducer and regression coverage.
- Docs: Not needed because the fix does not change the published UX or protocol contract.
- Checks: Current-head Branch Checks are still running; completed DCO, vouch, metadata, Helm, and Trivy gates are green.
- E2E:
test:e2eis required for network-proxy behavior but will be dispatched after blocking feedback is resolved. - Head SHA:
5cc8e2f808040ac8ecb7ebd0636316055595d459 - Base SHA:
cb6e88acb7fda83cd4355330b24dab1196105729 - Merge base SHA:
484f0768fc6a0d93e0a2be295c1679aed24e18a9 - Patch ID:
c36e941826640402277bae4353c7d9932a9ecd9d - Gator payload:
10 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
|
Label |
drew
left a comment
There was a problem hiding this comment.
gator-agent
Re-check After Author Update
Thanks @pimlock. I checked your 2026-09-21 update against the earlier chunk-framing amplification finding. The new 32 KiB aggregate framing limit covers chunk-size lines, per-chunk terminators, and trailers, and the counting-I/O regression exercises the previously unbounded tiny-chunk case. The prior finding is resolved, and the follow-up review found no new blockers in the author-only delta.
Blocking findings:
- No blocking findings remain.
Carried findings:
GATOR-5cc8e2f8-01: resolved by the current framing cap and regression coverage.
Gator metadata
- Validation: Localized correctness and security fix for the supported REST network-proxy path, with a concrete reproducer and regression coverage.
- Docs: Not needed because the fix does not change the published UX or protocol contract.
- Checks: Required current-head checks are running; no failures are currently reported.
- E2E:
test:e2eis applied and the current-head E2E workflow is running. - Head SHA:
107ce348e98ade65d52fdab10562dc07b0a22ebb - Base SHA:
cb6e88acb7fda83cd4355330b24dab1196105729 - Merge base SHA:
484f0768fc6a0d93e0a2be295c1679aed24e18a9 - Patch ID:
4a84d6fcb3d6c801f01ca6d6e65a717e9554d41d - Gator payload:
10 - Review mode:
follow_up - Previous reviewed SHA:
5cc8e2f808040ac8ecb7ebd0636316055595d459 - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:watch-pipeline
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
107ce34 to
738965b
Compare
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
drew
left a comment
There was a problem hiding this comment.
gator-agent
Re-check After Author Update
Thanks @pimlock. I reviewed the latest author-only change that boxes the sandbox runtime future. It does not alter the chunk-boundary implementation, reopen the resolved framing-amplification finding, or introduce a new blocker.
Blocking findings:
- No blocking findings remain.
Carried findings:
GATOR-5cc8e2f8-01: remains resolved by the framing cap and regression coverage.
Gator metadata
- Validation: Localized correctness and security fix for the supported REST network-proxy path, with a concrete reproducer and regression coverage.
- Docs: Not needed because neither the network fix nor the latest compile fix changes the published UX or protocol contract.
- Checks: Required current-head Branch Checks and E2E jobs are still running; no failures are currently reported.
- E2E:
test:e2eis applied and the current-head E2E workflow is running. - Head SHA:
249d6c741006299905b21275ce11ffa38dde449a - Base SHA:
cb6e88acb7fda83cd4355330b24dab1196105729 - Merge base SHA:
cb6e88acb7fda83cd4355330b24dab1196105729 - Patch ID:
d2105651050e1cdfc3c5613e075dc0367e884a98 - Gator payload:
10 - Review mode:
follow_up - Previous reviewed SHA:
107ce348e98ade65d52fdab10562dc07b0a22ebb - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:watch-pipeline
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
drew
left a comment
There was a problem hiding this comment.
gator-agent
Re-check After Author Update
Thanks @pimlock. I checked your latest update that replaces the aggregate framing cap with connection-scoped read-ahead and coalesced upstream writes. In the required critical-only review, the author delta does not introduce a new Critical defect, and the earlier framing-amplification finding remains resolved.
Blocking findings:
- No blocking findings remain.
Carried findings:
GATOR-5cc8e2f8-01: remains resolved by connection buffering, write coalescing, and the counting-I/O regression.
Gator metadata
- Validation: Localized correctness and security fix for the supported REST network-proxy path, with a concrete reproducer and regression coverage.
- Docs: Not needed because the fix does not change the published UX or protocol contract.
- Checks: Required current-head Branch Checks and E2E jobs are still running; no failures are currently reported.
- E2E:
test:e2eis applied and the current-head E2E workflow is running. - Head SHA:
02090a52e60a91f0de8960de632169573d3ce4f3 - Base SHA:
cb6e88acb7fda83cd4355330b24dab1196105729 - Merge base SHA:
cb6e88acb7fda83cd4355330b24dab1196105729 - Patch ID:
0af969138f73a35573bb2748fbff3abe8dd1cb22 - Gator payload:
10 - Review mode:
critical_only - Previous reviewed SHA:
249d6c741006299905b21275ce11ffa38dde449a - Review budget exhausted: yes
- Maintainer decision required: no
- Next state:
gator:watch-pipeline
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
|
The MCP E2E failure was caused by the 8 KiB write-coalescing added in the previous revision. A small chunk on the long-lived MCP stream remained buffered until end-of-body, so the elicitation callback could not run and Commit 619c86a keeps connection-scoped read-ahead but forwards each complete validated chunk immediately. It also adds an open-stream regression that verifies the first chunk reaches the destination before the terminal chunk is sent. Verified locally:
|
drew
left a comment
There was a problem hiding this comment.
gator-agent
Re-check After Author Update
Thanks @pimlock. I checked your September 21 update explaining that the previous write coalescing delayed a live MCP stream. The critical-only review covered the author delta that now forwards each complete validated chunk immediately and adds an open-stream regression; it found no newly introduced Critical defect.
Blocking findings:
- No blocking findings remain.
Carried findings:
GATOR-5cc8e2f8-01: remains resolved because reads stay connection-buffered and writes occur per validated chunk rather than per framing byte.
Gator metadata
- Validation: Localized correctness and security fix for the supported REST network-proxy path, with a concrete reproducer and regression coverage.
- Docs: Not needed because the fix does not change the published UX or protocol contract.
- Checks: Current-head Branch Checks and E2E are running with no reported failures; Helm Lint and Trivy Changes are green.
- E2E:
test:e2eis applied and the current-head E2E workflow is running. - Head SHA:
619c86ab26ca3a3fa733e02cfd18758d160402c6 - Base SHA:
cb6e88acb7fda83cd4355330b24dab1196105729 - Merge base SHA:
cb6e88acb7fda83cd4355330b24dab1196105729 - Patch ID:
e28f57b03e51be8c0f50acddd0c4632e7c645e59 - Gator payload:
10 - Review mode:
critical_only - Previous reviewed SHA:
02090a52e60a91f0de8960de632169573d3ce4f3 - Review budget exhausted: yes
- Maintainer decision required: no
- Next state:
gator:watch-pipeline
Summary
Preserve HTTP/1.1 request boundaries with connection-scoped read-ahead buffering so a following pipelined request remains available for its own L7 policy decision. Forward each complete validated chunk promptly without imposing a cumulative framing limit.
Related Issue
No issue required: this is an obvious localized correctness fix in the chunked request relay.
Failure Scenario and Fix
A client can send an allowed chunked request and the next request in one socket write:
Previously, the chunked relay could read both requests into its local 8 KiB buffer and write the entire buffer upstream before recognizing the
0\r\n\r\nterminator. The subsequentDELETEtherefore reached upstream as overflow from the allowed request before its own policy evaluation.The inspected connection now owns the read-ahead buffer for its full keep-alive lifetime:
The upstream receives only the allowed
POST. The remainingDELETEstays in the connection buffer, enters the normal request loop, and receives a separate policy decision.There is no longer a 32 KiB aggregate framing limit. Valid requests with many tiny chunks continue to relay. Underlying reads use the persistent 8 KiB connection buffer, and each complete validated chunk is forwarded promptly. Individual chunk-size and trailer lines remain subject to the existing 16 KiB line ceiling so an unterminated line cannot grow memory without bound.
Changes
Testing
mise run pre-commitpassesmise exec -- cargo test -p openshell-supervisor-network --lib(1,341 passed, 2 ignored)mise run --no-deps --skip-deps e2e:mcp(3 scenarios passed, including elicitation defaults)OPENSHELL_E2E_DOCKER_TEST=proxy_egress_pipeline mise run e2e:docker(11 passed)Checklist