Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs 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". |
|
@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.
Pull request overview
Derives a controlled-session cleanup manifest from the durably recorded session ownership, validates it against the exact prepared runtime resources before startup, and introduces strict canonical JSON encoding/decoding for watchdog consumption.
Changes:
- Add
ControlledSessionCleanupManifestwith validation plus canonical encode/decode helpers. - Have the controlled-session supervisor persist ownership, derive a cleanup manifest, validate it against the prepared containers/channel, and reject non-empty networks/volumes.
- Extend test coverage for manifest derivation/canonicalization and for rejecting runtime cleanup resource selection.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/dockerdeploy/controlled_session_supervisor.go | Derive/validate cleanup manifest during prepare; adjust ownership recording flow. |
| internal/dockerdeploy/controlled_session_supervisor_test.go | Add coverage ensuring invalid cleanup resource selection is rejected before startup. |
| internal/deploy/live_run_queue_file_test.go | Update controlled-session ownership fixture channel directory path. |
| internal/deploy/controlled_session_cleanup_manifest.go | New manifest type with validation and strict canonical JSON encode/decode. |
| internal/deploy/controlled_session_cleanup_manifest_test.go | New tests for derivation, validation, and canonical/unknown-field rejection. |
| docs/CONTROLLED_SESSION_DESIGN.md | Document the immutable cleanup manifest derived from durable ownership. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| released = true | ||
| if err := operation.Unlock(); err != nil { | ||
| return fmt.Errorf("release operation lock before controlled-session startup: %w", err) | ||
| return deploy.ControlledSessionCleanupManifest{}, fmt.Errorf("release operation lock before controlled-session startup: %w", err) | ||
| } | ||
| return nil | ||
| return manifest, nil |
| // Networks and volumes are explicit arrays even while controlled sessions do | ||
| // not create either resource. Future slices may populate them only after their | ||
| // exact identities become part of durable ownership. |
| record; failed cleanup retains it. Reploy now derives the immutable watchdog | ||
| cleanup manifest from that recorded ownership before startup. It names only | ||
| the exact containers and private channel, carries the host boot identity, | ||
| represents the currently absent lease networks and volumes as empty arrays, | ||
| and omits protocol authority. The parent and watchdog will be the same |
682209d to
b4d58b6
Compare
8c03f9e to
f1ea356
Compare
ceebb6e to
2ca12cd
Compare
0aeea52 to
a1d6657
Compare
55a2b9a to
9a8cafd
Compare
0969ca5 to
fd03cc2
Compare
Summary
Why
The upcoming short-lived watchdog must receive a frozen cleanup target instead of accepting resource choices after startup.
Validation