Conversation
|
@codex review |
![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: 1ba9774d0d
ℹ️ 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".
| if machine.workloadOutputs.Kind != "" && !(machine.activated && | ||
| machine.workload.Kind == ProcessStatusUnknownV1 && | ||
| machine.runtimeObservation.Kind == RuntimeObservationLostV1 && | ||
| machine.workloadOutputs.Kind == WorkloadOutputFinalizationFailedV1) { |
There was a problem hiding this comment.
When termination starts from a terminate, host-cancel, or controller-loss action and the output deadline expires before the forced workload stop is observed, ObservationWorkloadOutputFinalizationExpiredV1 records failed output while the workload status remains unknown. This guard then rejects the subsequent ObservationWorkloadExitV1 because its only exception is runtime-observation loss. Since the deadline explicitly covers workload shutdown, this is a valid ordering and prevents the authoritative workload-exit event/status from being recorded after a timeout; allow a late terminal workload observation for an activated session whose output finalization failed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Strengthens the controlled-session lifecycle state machine to enforce a host-owned “workload output finalization” barrier (including a timeout/expiry path) before allowing controller completion, and updates documentation/tests to reflect the new semantics.
Changes:
- Introduces an explicit
AwaitingWorkloadOutputFinalizationstate and a new observation kind for output-finalization expiry. - Tightens lifecycle gating so
completerequires finalized workload outputs and finish requires no pending output/controller finalization. - Expands lifecycle tests and updates the controlled-session design doc to reflect the output-finalization barrier + timeout outcome.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/controlledsession/lifecycle.go | Adds output-finalization pending state + expiry observation and integrates it into termination/finish/completion gating. |
| internal/controlledsession/lifecycle_test.go | Adds coverage for output barrier sequencing, expiry behavior, and concurrency/idempotence around finalization. |
| docs/CONTROLLED_SESSION_DESIGN.md | Updates implementation status and event semantics to include the output-finalization timeout. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| case ObservationWorkloadOutputFinalizationExpiredV1: | ||
| if observation.WorkloadStatus != nil || observation.WorkloadOutputFinalizationStatus != nil || observation.Finish != nil || !machine.waitingOutputs { | ||
| return transition, fmt.Errorf("%w: output-finalization expiry carries only a required reason while output finalization is pending", ErrObservationRejected) | ||
| } | ||
| if err := validateRequiredSafeTextV1("workload output finalization expiry reason", observation.Reason); err != nil { | ||
| return transition, fmt.Errorf("%w: %v", ErrObservationRejected, err) | ||
| } | ||
| machine.completeOutputFinalizationLocked(WorkloadOutputFinalizationStatusV1{ | ||
| Kind: WorkloadOutputFinalizationFailedV1, | ||
| Reason: observation.Reason, | ||
| }) |
| - Implementation state: Initial global sandbox prerequisites, trusted | ||
| application-startup verification, controlled-session authorization, and the | ||
| initial framed protocol are implemented; lifecycle, controlled networking, | ||
| and Docker orchestration remain later slices | ||
| application-startup verification, controlled-session authorization, the | ||
| framed protocol, and lifecycle state machine, including its output-finalization | ||
| barrier and timeout outcome, are implemented; bounded output draining, | ||
| controlled-session networking, and Docker orchestration remain later slices |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0998ce369
ℹ️ 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".
| machine.latchLocked(CauseRuntimeObservationLostV1, &transition) | ||
| machine.failOutputsForRuntimeObservationLossLocked(observation.Reason) |
There was a problem hiding this comment.
When runtime observation is lost while the PTY or buffered output is still open, this call immediately records output finalization as failed and clears waitingOutputs, even though losing runtime observation does not establish that the output surfaces were forcibly closed. The controller can consequently send complete, and the host can accept ObservationFinishedV1, before the no-more-output invariant is actually established, allowing a terminal result to race with later output. Keep the barrier pending until an explicit observation confirms that the remaining surfaces have been closed, even if the eventual status must be failed.
Useful? React with 👍 / 👎.
|
@codex review |
|
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". |
Stack created with Sapling. Best reviewed with ReviewStack.