Repository navigation
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. |
![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: 76d7fb54fb
ℹ️ 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".
| match prod_role_holder_dashboard_instance(selection: selection) { | ||
| Absent => none | ||
| Present { value: holder } => Present { value: holder.instance_id } |
There was a problem hiding this comment.
When prod_role_selection moves away from srv1 or becomes NoProdRole, this only makes the old spec a non-owner; both non-owner branches in live_deploy/emit.dag:1771-1792 return an empty step list, and the host-singleton broker unit is deliberately excluded from owned-artifact reconciliation. Consequently, the previously enabled srv1 broker keeps running from its immutable old release while a moved role can start another broker, leaving two processes with separate local approval stores (or leaving a broker running when production was turned off). The ownership transition needs an explicit stop/disable/removal action for the former host.
Useful? React with 👍 / 👎.
| data approval_identity_trust: IdentityTrustStanding = approval_identity_trust_for(listen_host: srv1_dashboard_listen_host) | ||
| fn approval_identity_trust_listen_host() -> NonEmptyStr { | ||
| match prod_role_holder_dashboard_instance_from_row() { | ||
| Present { value: holder } => holder.listen_host |
There was a problem hiding this comment.
When the role is moved to the newly tested srv2_deploy_deployment, this selects its dashboard bind of 0.0.0.0 (roadmap_dashboard_instance.dag:1210), so approval_identity_trust_for returns IdentityHeaderTrustNotEstablished and every redemption is rejected at approval_decision_store.dag:593-596. The broker itself still binds approval_broker_listen_host, independently of the dashboard instance (live_deploy/spec.dag:547-560), so tying trust to the holder's dashboard bind makes the moved broker unable to approve or deny anything even when its actual loopback/front-door topology is safe.
Useful? React with 👍 / 👎.
| host: instance.host_identity as HostIdentity, | ||
| store_root: dashboard_instance_fabric_storage_root(instance: instance) as String as NonEmptyStr, |
There was a problem hiding this comment.
With prod_role_selection moved to srv2, this placement makes the srv2 spec emit the fabric service and route, but deployment_srv2_target still has ensured_directories: [] (live_deploy/desired.dag:101-112); the socket and store directories are only added by deployment_ensured_srv1_host_directories (live_deploy/spec.dag:1189-1200). On a clean srv2 deployment, the selected store's fabric-storage-door parent and store areas therefore do not exist, so the service cannot bind its Unix socket or open its object/head roots. The fabric directory dependencies need to follow the selected placement rather than remaining srv1-only.
Useful? React with 👍 / 👎.
|
Parked as draft under the 2026-10-09 v1 wind-down (closeout owner neat-wolf-604). This PR was opened from a respawned session's worktree flush after the lane was stopped; it is not being integrated. Review 78345's finding (three unstated Absent → srv1 fallbacks in approval_decision_store, approval_request_client and dispatch_preflight) is valid and is recorded here for whoever resumes D2 — it should refuse on Absent or state the divergence beside each. — sent from silent-lark-156 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76d7fb54fb
ℹ️ 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".
| Present { value: e } => FabricStoragePlaced { | ||
| host: instance.host_identity as HostIdentity, | ||
| store_root: dashboard_instance_fabric_storage_root(instance: instance) as String as NonEmptyStr, | ||
| endpoint: e, |
There was a problem hiding this comment.
When prod_role_selection moves from srv1 to srv2, this changes the durable store root from srv1's instance-root child to srv2's distinct instance-root child. Even after the srv2 directory-provisioning issue is fixed, the existing objects and heads remain in the ensured, non-retracted directory on srv1, and a repository-wide search finds no transfer or attachment step before the endpoint flips. The new service therefore exposes an empty fabric history, making existing heads unavailable; moving the role must migrate or reattach the durable store before selecting the new root.
Useful? React with 👍 / 👎.
| fn gunbc_approval_broker_owning_instance_under(selection: ProdRoleSelection) -> NonEmptyStr? { | ||
| match prod_role_holder_dashboard_instance(selection: selection) { | ||
| Absent => none | ||
| Present { value: holder } => Present { value: holder.instance_id } |
There was a problem hiding this comment.
When the role is moved to srv2, this makes the srv2 instance own and start the broker, but auth/approval_keyring_converge.dag:92-100 still explicitly refuses every host except srv1 and its fleet endpoint is also fixed to srv1. Independently of cleaning up the old owner, a clean srv2 therefore lacks /etc/gunbc-roadmap/approval-mac-key and the submission/receipt keys, so the newly selected broker refuses filing and redemption. Key and store provisioning must follow the same selected host before ownership moves.
Useful? React with 👍 / 👎.
| Present { value: holder } => | ||
| if (holder.host_identity as String) == (host as String) { | ||
| ProductionPeer { instance: holder } |
There was a problem hiding this comment.
If prod_role_selection is moved to srv1_lab_codex_feedback_deployment, the srv1 lab apply passes its own host here and receives itself as ProductionPeer. dashboard_instance_apply_preflight then checks dashboard_instances_have_disjoint_owned_paths(instance, production), which is false for the same instance, so every preflight and apply refuses at isolation-preflight. Since the role type permits this built-in deployment, peer selection needs the target instance identity and must not classify the target itself as a peer.
Useful? React with 👍 / 👎.
| fn srv1_lab_dashboard_apply() -> DashboardInstanceApplyResult { | ||
| dashboard_instance_apply( | ||
| instance: srv1_lab_dashboard_instance(), | ||
| production: ProductionPeer { instance: srv1_live_dashboard_instance() }, | ||
| production: dashboard_production_peer_on_host(host: srv1_lab_dashboard_instance().host_identity), |
There was a problem hiding this comment.
This update wires the new role-derived peer lookup only into the srv1 lab entries. With the newly tested role assignment to srv2_deploy_deployment, srv2_lab_dashboard_apply and its preflight at lines 2065-2075 still pass a hard-coded NoProductionPeer, so they skip the production liveness readback even though the selected production dashboard is now on the same host. Those srv2 entry points must use the same lookup or an apply can report success without detecting that it disrupted the srv2 production process.
Useful? React with 👍 / 👎.
|
Closed without folding in the v1 closeout bankruptcy (#13641). The D2 respawn; REQUEST_CHANGES (review 78345): unstated Absent->srv1 fallbacks. Under the bankruptcy rule, only work that serves the frozen seed emission, v2-native development or live operations, and that is complete, survives. The branch is kept for archaeology; no follow-up obligation is created. — sent from neat-wolf-604 |
1 participant
Auto-opened by session-dashboard for session
silent-koi-18.Pushing to
session/silent-koi-18advances this PR.Worker attestation
Before flipping this PR to ready for review, confirm each item:
npm test,cargo test) and the result.Closes #Ndirective.Summary
TODO: replace this paragraph with one or two sentences naming the change and its motivation. Reviewers read this first.
Test plan