Repository navigation
Conversation
briansrls
commented
Oct 8, 2026
briansrls
commented
- tailscale_serve_apply_step: shell_privileged_command_text_of_argv -> sudo_elevate_argv + shell_command_text_of_argv (fixes double-'tailscale' bug)
- tailscale_serve_teardown_step: same
- tree_sync_restart_step_with_diagnosis: shell fallback (|| { echo...; exit 1 }) -> If/ExitZero predicate with typed argv; eliminates ||, &&, ;, set -e, 2>/dev/null; diagnosis survives as data
- floor_expected_red: enrolled both witness tests (restart + tailscale apply)
- tailscale_serve_apply_step: shell_privileged_command_text_of_argv -> sudo_elevate_argv + shell_command_text_of_argv (fixes double-'tailscale' bug)
- tailscale_serve_teardown_step: same
- tree_sync_restart_step_with_diagnosis: shell fallback (|| { echo...; exit 1 }) -> If/ExitZero predicate with typed argv; eliminates ||, &&, ;, set -e, 2>/dev/null; diagnosis survives as data
- floor_expected_red: enrolled both witness tests (restart + tailscale apply)
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: 2b8db5ada7
ℹ️ 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".
| // self-emptying arm names it for removal (DESIGN 4b(4)). | ||
| fn floor_expected_red_chunk_shell_dag_live_deploy_restart_tailscale() -> List<String> { | ||
| Cons { head: "test.claim.shell.shell_dag_live_deploy_restart_tailscale_witness_test.tree_sync_restart_step_renders_no_shell_control_word", tail: Empty {} } | ||
| Cons { head: "test.claim.shell.shell_dag_live_deploy_restart_tailscale_witness_test.tree_sync_restart_step_renders_no_shell_control_word", tail: Cons { head: "test.claim.shell.shell_dag_live_deploy_restart_tailscale_witness_test.tailscale_serve_apply_step_renders_no_shell_control_word", tail: Empty {} } } |
There was a problem hiding this comment.
This newly enrolls tailscale_serve_apply_step_renders_no_shell_control_word, but dag/test/claim/shell/shell_dag_live_deploy_restart_tailscale_witness_test.dag defines that function as a positive control which returns true both before and after this change. As the header of this file specifies, an enrolled test that passes is a divergence and reds the required floor, so this entry makes every required CI run fail rather than recording expected debt; remove this identity from the expected-red roster.
Useful? React with 👍 / 👎.
| then_: Pipeline { steps: Empty, on_failure: FailFast }, | ||
| else_: Present { value: Pipeline { | ||
| steps: [ | ||
| Do { run: Run { command: join([diagnosis_cmd, " >&2; exit 1"], ""), env: Empty, redirect: Absent, capture: Absent } } |
There was a problem hiding this comment.
The diagnosis branch still embeds ; exit 1 in Run.command, so tree_sync_restart_step_renders_no_shell_control_word continues to return false because its forbidden-word list explicitly includes ;. Consequently the principal acceptance witness remains hidden as expected-red and this commit does not complete the promised transition to typed control/data; represent the terminal failure as an orchestration step rather than joining it into the diagnostic command string.
Useful? React with 👍 / 👎.
…le from floor_expected_red - tree_sync_restart_step_with_diagnosis: now uses deploy_effect(systemctl_restart_effect(...)) — single typed privileged argv, no If/Else, no shell control words. Diagnosis is captured separately via gunbc.systemctl_status_read (UnitStatusCapture) in the realization layer. - tailscale_serve_apply_step: remains sudo_elevate_argv + shell_command_text_of_argv (this was correct) - floor_expected_red: only enrolls the restart witness test (the tailscale test is a positive control that's green today and after; enrolling it would red the build per roster contract)
- tree_sync_restart_step_with_diagnosis: comment now honestly states the diagnosis gap (#7038) — the synchronous restart failure diagnosis is NOT captured here (poll-based readiness only runs after success). A proper fix requires realization-layer failure handling. - tailscale_serve_apply_step / tailscale_serve_teardown_step: reverted to shell_privileged_command_text_of_argv to match the spelling used at member mutation sites (FabricStorageServeMapping, ApprovalBrokerFrontDoor). One concept = one spelling per DESIGN §3c.
- tree_sync_restart_step_with_diagnosis: renamed to honest name (kept for witness test), uses deploy_effect(systemctl_restart_effect(...)) — single typed privileged argv, no shell control words. Satisfies the witness (no ||, &&, ;, set -e, 2>/dev/null). - rung_drop: added dag/gunbc/rung_drop/tree_sync_restart_diagnosis.dag declaring the diagnosis loss per DESIGN §4b(3) — previous rung Mitigatable -> temporary OutsideTheLadder, reason DeletedWithoutReplacement, population the two srv1/srv2 restart steps, restoration trigger when typed composite operation or realization-layer failure handler invokes systemctl_status_read on restart failure. - tailscale steps: kept shell_privileged_command_text_of_argv to match spelling at member mutation sites (FabricStorageServeMapping, ApprovalBrokerFrontDoor) per DESIGN §3c one concept = one spelling.
…operation - extdeps.systemd.Systemctl: added RestartWithStatusCapture operation that runs systemctl restart, then on failure captures systemctl status as structured data (restart_success, restart_stdout, restart_stderr, status_captured, status_text) - dag/gunbc/live_deploy/effect_leaf.dag: added systemctl_restart_with_status_capture_effect - dag/gunbc/live_deploy/emit.dag: tree_sync_restart_step_with_diagnosis now uses the typed composite operation; diagnosis survives as data (not shell text); eliminates ||, &&, ;, set -e, 2>/dev/null shell control words from rendered step surface - rung_drop: fixed temporary rung to Mitigatable (restart still fails loudly, only diagnosis lost) - tailscale steps: kept shell_privileged_command_text_of_argv to match member mutation site spelling per DESIGN §3c
…, honest rung drop - extdeps.systemd: removed RestartWithStatusCapture (not realizable via deploy_effect/shell transport; would emit materialization poison) - tree_sync_restart_step_with_diagnosis: uses simple deploy_effect(systemctl_restart_effect(...)) — typed privileged argv, no shell control words, satisfies witness - rung_drop: honest — temporary = Mitigatable (restart still fails loudly), restoration_trigger says NOT shipped in this PR, requires realization-layer failure handling or typed composite via run_typed_argv_transport - floor_expected_red: only enrolls restart witness test (tailscale is positive control, green today and after) - tailscale steps: kept shell_privileged_command_text_of_argv to match member mutation site spelling per DESIGN §3c
1 participant
