feat(g01): connect paired terminal execution to bounded broker handoff - #62
feat(g01): connect paired terminal execution to bounded broker handoff#62jjangg96 wants to merge 39 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ 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. |
|
Independent Luna/max contract review has reproduced a blocking integration defect at head 507850c: paired-terminal local preparation requests a cleanup receipt, but brokerPreparationReceipt.valid requires the paired approval phase. The current synthetic positive broker test bypasses that real entrypoint. Review also identified missing actual TLS/Unix coverage through exported RunPairedTerminal. Merge is held; both reviewers are finishing on the frozen head before the integrator receives a combined TDD fix task. No live execution is authorized or claimed. |
![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: 507850c808
ℹ️ 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".
| phase := approval.Phase | ||
| if approval.Mode == "paired-terminal" { | ||
| phase = "cleanup" | ||
| } | ||
| return invokeBrokerPreparation(ctx, binary, files.StateDirectory, snapshotPath, files.ControllerStateDirectory, phase) |
There was a problem hiding this comment.
Every production paired-terminal invocation prepares the controller journal with phase cleanup, so the child returns a receipt whose Phase is cleanup; however, brokerPreparationReceipt.valid requires that field to equal p.approval.Phase, which can only be empty or paired-terminal for this mode. Consequently brokerExecute rejects the receipt at the preparation gate before authentication, and the paired handoff can never run. Track the actual preparation phase in the plan or validate cleanup for paired mode.
Useful? React with 👍 / 👎.
| if paired { | ||
| if a.Mode != "paired-terminal" || e.Worker == nil || !brokerSHA256.MatchString(e.Worker.Approval) || e.Worker.ApprovalFile.Device == 0 || e.Worker.ApprovalFile.Inode == 0 || e.Worker.State.Device == 0 || e.Worker.State.Inode == 0 { | ||
| return false | ||
| } | ||
| } else if e.Worker != nil || a.Mode == "paired-terminal" { |
There was a problem hiding this comment.
When a paired invocation reads an admission ledger containing any earlier controller claim, this branch rejects that historical event solely because the current approval mode is paired-terminal. The terminal path necessarily starts from a controller journal with a prior successful create (newBaselineListenerHeld requires it in baseline_listener.go:54-71), so the normal controller-to-paired sequence cannot pass admission. The reciprocal check also prevents an approved controller inspect or cleanup from reading a failed paired claim, blocking recovery of retained resources; validate each event according to its own slot rather than the current request mode.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| func (a pairedWorkerApproval) validate(now time.Time) error { | ||
| if !a.RunnerUpdatesDisabled || !brokerSHA40.MatchString(a.HarnessSHA) || !brokerSHA40.MatchString(a.WorkflowSHA) || !brokerNonce.MatchString(a.OwnerNonce) || !brokerComponent.MatchString(a.Controller) || !brokerComponent.MatchString(a.DaemonID) || !strings.HasPrefix(a.ImageID, "sha256:") || !brokerSHA256.MatchString(strings.TrimPrefix(a.ImageID, "sha256:")) || a.Image != pairedWorkerImage || !filepath.IsAbs(a.Endpoint) || filepath.Clean(a.Endpoint) != a.Endpoint || len(a.Endpoint) > 103 || strings.ContainsAny(a.Endpoint, "\x00\r\n") || !a.ExpiresAt.After(now) || a.ExpiresAt.After(now.Add(24*time.Hour)) || len(a.Phases) != 4 { |
There was a problem hiding this comment.
A worker approval whose DaemonID contains : or is 101–128 characters long passes the authoritative liveworker.Approval.Validate contract (liveworker/profile.go:24,29-34) and can be matched against Docker's /info response, but this broker-side copy validates it with the narrower brokerComponent expression and rejects it before handoff. Because paired mode consumes an already-reviewed worker approval, the broker should apply the same daemon-ID validation contract rather than silently excluding otherwise valid worker configurations.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad7c2cf44c
ℹ️ 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".
| s := replay(j.Events()) | ||
| if s.uncertain || s.deleted || s.setID != 0 || s.reserved || s.workObserved || len(j.Events()) != 0 { | ||
| return receipt, ErrQuarantine |
There was a problem hiding this comment.
When paired mode uses the controller state required by RunPairedTerminal, this condition rejects it because that state must already contain successful create and inventory records: newBaselineListenerHeld rejects a missing creation, while runPairedBaselineModeWithBinding requires an inventory record. Conversely, an empty journal that passes here reaches execution with setID == 0 and is rejected. Fresh evidence in this revision is the new PreparePairedJournal path enforcing len(j.Events()) == 0; it should validate and preserve the prerequisite controller history instead.
Useful? React with 👍 / 👎.
| ctx, cancel := context.WithTimeout(parent, 30*time.Second) | ||
| defer cancel() |
There was a problem hiding this comment.
With the production cadence, the paired child cannot finish inside this deadline: sampleRounds(1, 4) and sampleRounds(5, 8) enforce a five-second gap after every prior round, so eight fresh rounds require at least 35 seconds before terminal cleanup even starts. The fixture replaces that cadence, but a real exec.CommandContext child is killed at 30 seconds, leaving journal and runner state uncertain and making successful paired-terminal completion impossible; use the parent/approval deadline or a bound covering the full terminal sequence.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ecb06c175
ℹ️ 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".
| } else if a.Mode == "paired-terminal" { | ||
| slot = "paired-terminal" |
There was a problem hiding this comment.
When all nine original slots have already been recorded, this newly valid tenth slot passes the current replay and appends its claim, but the parser at line 257 still rejects ledgers with more than 20 newline-split entries. Nine completed slots already occupy the header plus 18 events; appending the paired claim creates 21 entries, so a failed paired preparation permanently prevents a later authorized inspect or cleanup from reopening the ledger, while a completed pair leaves it even larger. Raise the structural limit to cover the header and claim/completion records for all ten slots.
Useful? React with 👍 / 👎.
| newSDKAPIForCommand = func(a livecanary.Approval, c livecanary.Credentials, stateDirectory string) (*livecanary.SDKAPI, error) { | ||
| config, err := readFixtureEndpointConfig(stateDirectory) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return livecanary.NewSDKAPIForPairedFixture(a, c, config.BaseURL, []byte(config.CAPEM)) |
There was a problem hiding this comment.
When g01-live is built with g01_pair_fixture, this override sends the broker-issued installation token to the loopback URL and CA selected by paired-fixture.json, yet validBrokerBuild rejects only osusergo and therefore accepts a clean, correctly stamped fixture-enabled binary. Such a binary can satisfy the approved hash/build checks while replacing the production GitHub transport (for both ordinary controller execution and paired execution) with a test endpoint that can capture the credential. Make the broker reject the g01_pair_fixture build tag.
Useful? React with 👍 / 👎.
| if p.worker != nil && p.worker.check() != nil { | ||
| return errBroker |
There was a problem hiding this comment.
When the worker journal or permanent worker admission claim is malformed, locked, or already bound to another state directory, this check still succeeds because brokerWorkerPlan.check validates only the approval file and state-directory identity. The broker consequently records the paired claim and mints a token before the child reaches liveworker.OpenJournal in paired_terminal.go:295 and refuses the locally invalid state; because the paired slot is one-shot, correcting the worker state cannot retry the terminal handoff. Add a credential-free worker preparation/receipt check before authenticated calls, analogous to the controller preparation gate.
Useful? React with 👍 / 👎.
|
Independent contract and security reviews of 0ecb06c both reject merge despite successful Public CI. Current Codex findings independently reproduced: r3956753241 production binary gate accepts fixture-enabled executable; r3956753229 tenth paired slot exceeds ledger reopen capacity; r3956753245 malformed worker journal reaches mint before child refusal. Five previous findings were verified addressed, not merely declared stale. Contract reviewer additionally ran a strengthened temporary real-cadence bridge: child test 37.663s, wall 38.78s; this is local offline evidence, not yet a checked-in regression nor live recovery proof. A sole Luna/max runtime implementer is assigned all three fixes, permanent bounded wall-clock regression and reviewed CI integration. Grok continues separate PR59 security work. Both reviewers settled before writer assignment. No live operations authorized/performed; fresh independent reviews, exact-head Codex and CI remain required. |
|
@codex review |
|
Coordinator checkpoint on 1ebf0b1: runtime fixes and checked-in real-cadence bridge are pushed; reported race bridge duration36.196312875s/package39.633s. However, the CI seam uses120s despite the explicit45s approval, lacks the requested G02 generated failure/coverage matrix, and docs/CI still describes a single G02 invocation. Merge is held. A narrow CI/docs-only implementer is correcting the contract and measuring actual45s behavior; runtime files are frozen during that correction. Two fresh independent reviews will target the resulting final head, not a moving checkout. Codex review has been requested for currenthead; findings will be retained across subsequent pushes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ebf0b1e50
ℹ️ 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".
| path := filepath.Join(p.statePath, "journal.jsonl") | ||
| file, err := openBrokerPrivateFile(path, 0600, 1<<20) | ||
| if err != nil { | ||
| return errBroker | ||
| } | ||
| defer file.Close() | ||
| info, err := file.Stat() | ||
| if err != nil || brokerFileIdentity(info) != p.preparationReceipt.Journal { | ||
| return errBroker | ||
| } | ||
| data, err := io.ReadAll(io.NewSectionReader(file, 0, (1<<20)+1)) | ||
| if err != nil || brokerBytesDigest(data) != p.preparationReceipt.JournalDigest { |
There was a problem hiding this comment.
When the worker admission.json is edited or replaced after the initial preparation receipt but before a later authenticated call, checkPrepared reaches this function and revalidates only journal.jsonl; the receipt's Claim and ClaimDigest are never compared with the current admission claim. Consequently guardLive can allow preflight and token minting against invalid worker authority, while the execution child refuses only after the one-shot paired slot has been consumed. Re-run the canonical worker preparation check or validate the current claim inode and digest alongside the journal at each pre-mint boundary.
Useful? React with 👍 / 👎.
| - `/tmp/g01-paired-broker-review-0ecb06c.md` | ||
| - `/tmp/g01-paired-broker-independent-review-0ecb06c.md` |
There was a problem hiding this comment.
Replace these reviewer-local /tmp report references with durable repository or PR artifacts, or summarize their non-sensitive results directly. These paths are unavailable to other reviewers and CI, and committing personal machine paths or references to private test logs is explicitly prohibited by the repository instructions.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf278729a4
ℹ️ 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".
| GOTOOLCHAIN="${exact_toolchain}" "${go_cmd}" test -race -count=1 -timeout=45s -run "${real_pair_cadence_regex}" ./... | ||
| GOTOOLCHAIN="${exact_toolchain}" "${go_cmd}" test -race -count=1 -timeout=45s -skip "${real_pair_cadence_regex}" ./... |
There was a problem hiding this comment.
On a hosted or contended runner that is only about 12% slower than the recorded machine, either public-CI partition can time out despite correct behavior. Fresh evidence at this head is docs/evidence/g01-paired-broker.md:170-185, which records package times of 40.151s and 40.471s against these 45s limits; the named test alone includes seven unavoidable five-second cadence gaps plus a Git clone, a go build, and the bridge setup. go help testflag defines -timeout as: “If a test binary runs longer than duration d, panic.” Keep the split, but provide enough timeout headroom for normal hosted-runner variance.
Useful? React with 👍 / 👎.
The paired broker compared the worker journal inode/digest after preparation but never reopened admission.json against the receipt Claim/ClaimDigest, so same-inode mutation and replacement could mint and launch. Revalidate the canonical claim at every pre-auth/mint/launch fence. Cache synthetic RSA keys and split the remaining TestPaired family from the unfiltered G02 complement so the 45-second race partitions stay inside the existing deadline. Replace committed reviewer-local report paths with GitHub-backed evidence.
|
@codex review |
|
Coordinator pre-review note for head |
|
Confirmed coordinator reproduction on immutable head
Result: exit 1, package 0.401s, Reproduction uses existing This confirms the earlier pathname-binding hypothesis. Pin/revalidate the canonical claim location resolved by the trusted preparation contract, rather than searching candidate paths for any matching inode. Keep fixture path mechanisms out of production authority selection. No live resources/credentials were accessed; no production fix was made by the coordinator. Merge remains blocked until resolved and independently reproved. |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
checkPrepared accepted a relocated admission inode when the original pathname was replaced, because workerClaimPath searched sibling and account-root candidates for a matching inode. Bind the named claim path from the trusted preparation directory or the native-account pin only. Offline fixtures use an explicit resolver seam.
|
@codex review |
|
Coordinator independent regression checkpoint at exact head 773cccc: replayed the unchanged TestReviewPreparedWorkerClaimRelocationRefuses counterexample from the prior 92632dd rejection in a disposable source archive. Command: GOTOOLCHAIN=go1.26.8 go test -count=1 -timeout=60s -run '^TestReviewPreparedWorkerClaimRelocationRefuses$' . Result: exit 0, package 0.483s. The prior same test failed at 92632dd because relocation bypassed the canonical claim path. This verifies that one preparation-fence regression only, not full mint/launch safety or live ACK/acquisition/JIT recovery. Independent full contract/security review, exact-head Codex verdict, all historical findings, and CI remain merge gates. No live operations performed. |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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". |
1 participant
Summary
Closes #60. Ref #1; this does not close G01/G02 or authorize live execution.
Connects the reviewed paired terminal path to one same-process tagged g01-live mode and a fixed broker handoff, with bounded worker approval/state binding and one-shot admission.
Evidence
Initial head: 507850c.
Behavioral red: 3acd5ab. Green: 507850c.
The implementer reports Go 1.26.8 race/vet, tagged CLI and TLS/Unix fixture partitions, both experiment modules and GOTOOLCHAIN=go1.26.8 make check passed. Exact recorded commands/results and limitations are in docs/evidence/g01-paired-broker.md. Independent verification is pending.
Review and remaining gaps
Independent Luna/max contract and security/recovery reviews are running on this head. Hosted CI and exact-head GitHub Codex review must pass before conditional SHA-matched merge. Issue #61 separately addresses existing fixture deadline headroom without dropping tests.
No live GitHub/App/runner/workflow, Docker/Lima, Keychain or launchd operations were performed by the implementation. Real acknowledgement/acquisition/JIT recovery evidence remains outstanding. This is not the production daemon or supported crash resume.
Rollback
Source-only revert of these offline changes. No live resource was created. Future uncertain live outcomes require retaining owned journals/claims/resources, not broad cleanup or replay.