fix: address 8 round-2 Codex findings in remote reconciliation - #56
Conversation
- P1: pass local pinned checkout to installer for drift check and reconcile
- P1: fetch LKG commit independently (not from depth-one HEAD checkout)
- P1: serialize reconcile with installer mutations via flock
- P2: add optional units to installer checkpoints
- P2: disable optional timers on uninstall
- P2: run template validator with --strict --tree-paths
- P2: source rendered env before health.py calls
- P2: gate reconcile timer on remote repo identity (OWNER/REPO, not local path)
- P2: disputed — reconciliation failures in health.py is a separate concern,
reconcile already reports state independently via state.json
All 9 findings reacted (8 thumbs-up, 1 thumbs-down per Nick's convention).
|
@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: e4740f5b8a
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbde944da1
ℹ️ 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".
- P1: fix durable repo identity in rendered env too (not just state.json) - P1: restore optional timer states in restore_systemd_snapshot - P1: authenticate LKG fetch with reconciliation token - P1: share installer lock to prevent concurrent mutations - P1: check controller health before skipping same-commit drift reconcile - P2: disable reconcile timer on local checkout path
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4387be9b6
ℹ️ 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".
…er upgrade; reconcile all same-commit drift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b2cfe452f
ℹ️ 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".
1 participant
Round-2 fixes for remote configuration reconciliation
Fixes 8 of 9 review findings from the previous PR #54 round-2 Codex review.
Changes
scripts/remote-reconcile.sh— 6 fixes:OWNER/REPOSITORYto the installer for drift checks and upgrades, so a private desired-state repository is accessible without separate credentials./run/ci-fleet-reconcile.lock) withflockto serialize against concurrent installer mutations.--strict --tree-paths, matching the installer's own validation chain.run_health_checkwrapper function.scripts/install-worker-controller.sh— 3 fixes:OWNER/REPOSITORY(not a local checkout path), preventing spurious fetch attempts.Disputed
One P2 finding ("Add reconciliation failures to fleet health monitoring") was disputed with a thumbs-down. The reconcile timer is monitored by systemd like all other fleet timers, and reconciliation state is independently reported via
state.json. Adding health.py integration for reconcile failures is a separate enhancement, not part of this scope.Verification