Repository navigation
feat: add a bounded manual relay capacity runner - #737
owenthereal wants to merge 16 commits into
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (6)
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 944f592ebb
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 9c96079892
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c712d38cd
ℹ️ 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".
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 9c712d38cd
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
|
@codex review Please review c6690c0. The peer-prefix, overlapping-restoration and encoded-output findings are fixed with behavioral regressions, fresh full repository/race verification and independent review. The existing SFTP confinement thread remains open while the separately approved narrow host-library safeguard is implemented. Windows raw capture now covers both intermittent readiness fixtures; current-head CI remains the runtime check. |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review Round 3 is 0efeb68: approved optional rooted SFTP with explicit worker-completion cleanup, SFTP disabled for zero-transfer phases, and pinned benchmark PTY geometry for the captured Windows redraw issue. Independent whole-branch correctness/security review and fresh local verification passed. The first full-suite shutdown-test failure, ten unchanged isolated passes, and one unchanged full recheck are retained in the PR body; its original cause remains unproved. Current-head Windows runtime CI and rebuilt-generator smoke remain pending. This PR is not authorized for merge or deployment. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 0efeb68004
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
|
The current source review on
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: d3f3978036
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
|
Review hold after a full thread audit: two earlier security findings remain unresolved—SFTP writes can exceed any in-root disk quota (comment 4489569094), and partial terminal lines can retain the per-reader scanner maximum across many channels (comment 4489715432). Both premises are verified on the current head. The head-specific code-review verdict did not close these earlier security threads. Current-head CI passes after one unchanged macOS SFTP-test rerun; the original six-second deadline failure and its diagnosis are retained. The approved Windows geometry correction passed both full transport scenarios. Three review-fix rounds have already run, so I am preparing a concrete, bounded additional security-fix proposal for Owen, including the extra optional quota API and narrowly scoped fixture-path updates. This PR remains open pending that scope decision, correction/review, and the remaining live validation. |
|
@codex review The approved bounded security round is now in b870950. The SFTP write-quota and terminal-buffer findings have targeted baseline RED/GREEN evidence, fresh full local verification, and independent correctness/security sign-off. Please review the current head. Current-head CI and separate live-validation gaps remain explicit in the PR body; merge is still held. |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: b870950580
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| func (r *runState) actual(n int64) { | ||
| r.rec.mu.Lock() | ||
| r.rec.summary.PayloadBytes += n | ||
| r.rec.mu.Unlock() |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
When the explicitly pinned relay is compromised, it can use its trusted CA and the registered guest public key to open another shell and inject canonical R frames. The helper echoes these as A frames to every channel, but actual only increments a counter; unlike reserve, it never cancels at PayloadBudgetBytes. Fresh evidence beyond the fixed scanner buffers is the host’s per-guest 1 MiB AsyncWriter: a valid 1-host/998-guest/4-channel phase has 3,992 sinks, so the relay can drain the pacing sink, stall the rest, and make about 1 MiB of echoed output retain roughly 3.9 GiB before any budget stop. Enforce an aggregate peer-input/output quota and reject unexpected sessions before helper fan-out.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed by independent source review on b870950: the pinned relay can attest the registered guest identity, extra shared-shell sessions lack a numeric admission bound, and actual received bytes do not enforce the payload cap. The ~3.9 GiB example is conditional source-derived arithmetic: it requires a legal one-target configuration with sufficient payload budget and host-facing follower windows that allow pending queues to build. It is not measured RSS; smaller configurations can also admit unplanned sinks. A complete remedy needs admission before sink allocation plus aggregate retained-output accounting, including blocked in-flight buffers. This is a new finding beyond the explicitly approved fourth fix round. I am preparing a bounded proposal for Owen; no fifth-round code changes are made and this thread remains unresolved. Production and the isolated measurement image remain unchanged.
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: b870950580
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| if h.session.root != nil { | ||
| return h.session.root.Open(path) |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
When transfer_concurrency is 4 or 8 and the manifest-pinned relay is compromised, rooted SFTP constrains writes, but this path returns a fresh file for every authorized OPEN without read-open or byte accounting. Because the relay may attest the configured guest public key, it can repeatedly open the deterministic transfer file, retain handles to exhaust the runner's descriptors, or repeatedly read it to generate unbudgeted egress. These requests multiplex over an admitted SSH transport, so neither the connection guard nor PayloadBudgetBytes sees them. Apply lifetime per-file/aggregate accounting to reads and cap simultaneous handles.
Useful? React with 👍 / 👎.
Upterm needs reproducible measurements before choosing public relay connection limits. This adds a manual runner that creates real hosts and guests, verifies terminal traffic and SFTP contents, and exercises reconnects, multiple channels and forwarded connections against explicitly recorded test targets.
The runner requires an expiring manifest and bounded phase configuration. It pins keys and resolved destinations, rejects production and unrecorded routes, authorizes a separate guest identity, and stops on failed delivery, lost telemetry, deadlines, payload bounds or its 512 MiB encoded-output quota. JSONL records admission, latency, recovery, resources and cleanup. Normal tests use loopback fixtures.
Synthetic hosts disable SFTP when transfers are unused. Transfer phases use a private directory through optional
Host.SFTPRootandHost.SFTPWriteLimitsAPIs. Only assigned transfer filenames are writable; each has a 16 MiB extent and lifetime accepted-write allowance, with a shared host total. Reservations occur atomically before I/O and are never refunded; reconnects, overlapping writes, deletion and failed opens cannot replenish them. Each name permits one writable instance. Descriptor-rooted content and metadata operations, consent and read-only checks remain authoritative.Host.WaitSFTP(ctx)joins workers and handles before cleanup; failed cleanup verification retains the root and directory. Nil policies preserve existing host behavior.Both terminal readers retain bounded buffers while preserving the existing 1 MiB raw-line rejection, complete canonical payload checks, terminal prefixes, CR handling and sequence/barrier assertions. The helper uses pinned 2048×100 geometry to avoid redraw changes during the measured workload.
Validation on
b870950580f5a060893a1c4b012da63a59045e34:CONSUL_URL=http://127.0.0.1:18570 make build test TEST_COVER=: passed in467.117s with31 tested packages,8 packages with no test files, and no failure/race markers. Focused host/internal/capacity race tests, native and Windowsgo vet ./..., a static Linux runner build and pinned lint passed.0efeb680and exact image provenance. Pure SSH and private WebSocket each passed the matched 150-connection, one-channel, 30-minute workload; WebSocket continued consuming CPU credits, and both showed late memory decline. A two-host/two-guest public SSH/WSS path check passed. The combined two-hour hold remains invalid after control timeouts, and the current-source low-load forwarding smoke failed admission, so no production cap or public capacity matrix is validated. All temporary apps/Machines and the public IPv6 were removed and independently verified absent; the watchdog and dedicated local processes exited, and temporary keys were removed. These measurements do not validate the newer security commit, whose separate live-generator smoke remains unperformed.Risk: high because the runner manages concurrent network connections, identities and cancellation, and adds optional host-library filesystem APIs. Merge remains blocked until the deferred security finding is fixed and reviewed, followed by Owen's approval of this high-risk PR. Revert this PR to remove the runner and optional APIs. Production settings, public CLI behavior, dependencies and CI workflows are unchanged.
Rulings I made: