fix: restore PR #569 changes for Joe review

[lmilbaum]

Recreates the complete change from #569, including its two worker-tag audit fixes, for Joe (shrinedogg) to review after the original PR was prematurely merged.

This draft is stacked on revert/pr-569-premature-merge, the branch of draft revert PR #570, so the full original change is visible for review now. Do not merge either PR automatically. After #570 lands, retarget this PR to main and wait for Joe's review before any merge.

The change validates lifecycle metadata before Flux activation, preserves source metadata through pivot, and eventually repairs four lifecycle tags on discoverable managed EKS workers. JSON/NUL transport preserves supported tag values, and exact UTC expiry validation rejects impossible dates/times.

Validation: full mise run validate passed. At current head (stacked on revert/pr-569-premature-merge), git diff origin/main...HEAD is empty; the full #569 diff will appear once this PR is retargeted to main after #570 lands. The aws-cli image pin is at 2.37.12@sha256:ce44b800..., carrying forward the bump from #592 that the revert-and-restore pair would otherwise have dropped.

Operational limits remain: an external IAM grant is required for the existing ACK static principal; live EC2 acceptance was not performed; orphaned workers that leave their ASG before discovery are outside the eventual repair guarantee. No live cluster state was inspected or changed. Main still contains the original change until #570 is separately reviewed and merged.

🤖 Generated with Claude Code

[lmilbaum]

Recovery note from Codex for Joe:

Joe, this draft PR recreates the complete changes from #569, which I merged prematurely before your review. It includes the PR #475 audit fixes and the follow-up fixes for supported tag characters and calendar-valid expiry timestamps. Its repository tree exactly matches #569’s merged tree and passed mise run validate.

This PR is stacked on the revert branch from #570 so you can review the full original diff now. After #570 is reviewed and merged, this PR should be retargeted to main. Neither recovery PR has been merged; this PR should remain open pending your review. Worker tagging remains eventual and requires the documented external IAM grant and live EC2 acceptance checks.

[shrinedogg]

Reviewed at head 11748fc (unchanged since review capture), stacked on revert/pr-569-premature-merge (f1ca049, PR #570). This PR is unmergeable until #570 lands (or the pair is declined together); reviewed both against that base (the full 21-file restore of #569) and against main (net effect: one line in worker-run-tags/cronjob.yaml).

Blocking findings

1. mgmt/aws/infrastructure/worker-run-tags/cronjob.yaml:29 restores the aws-cli image pin from 6b17af9 (2.36.32 @ sha256:f630107e...), reverting the bump to 2.37.12 @ sha256:ce44b800... that main already merged in #592 (commit 1b0450c, landed after 6b17af9). cronjob.yaml is a new file in #569, so the revert in #570 deletes it and this restore recreates the pre-bump blob. All gates stay green because the stale tag and digest are internally consistent, so landing the #570+#571 pair as staged would silently downgrade the image on main. Fix: when re-applying, keep main's current image line (2.37.12 with its digest) instead of the 6b17af9 version of the file.

Non-blocking

2. PR description accuracy: "The final repository tree matches squash commit 6b17af9... exactly" was accurate when written but is false at this head (git diff 6b17af9..11748fc: 46 files, +609/-244). That divergence is almost entirely correct, since the branch keeps main's later advances (e.g. the #511/#561 audit fixes); the only regression it contains is the cronjob.yaml image line in finding 1. Please update the body when retargeting to main.

Verified locally at head 11748fc

• kustomize build mgmt/aws/infrastructure/worker-run-tags succeeds. The flux-ks dependsOn names (ack-controllers, aws-managed-clusters) exist in mgmt/aws/infrastructure/flux-ks.yaml, and the mounted Secret aws-credentials is defined in namespace ack-system.
• Runtime probe of the exact pinned digest (public.ecr.aws/aws-cli/aws-cli@sha256:f630107e...): /usr/bin/bash 5.2.15 and /usr/bin/python3 3.9.25 are present and json/re/sys/datetime import cleanly, so the CronJob command and the tag_data.py dependency set are satisfied on this image.
• tests/test-shell-run-tags.py: 3 tests OK.
• tests/test-worker-run-tags.py: 11 tests OK (uv run python, same interpreter path as CI).
• tests/test-aws-run-tags.py: OK.
• Touched YAML/TOML parses cleanly (cronjob.yaml, flux-ks.yaml, both kustomization.yaml files, validate.yml, mise.toml).
• CI at this head: all 9 checks SUCCESS.

Beyond the two items above, the change holds up: run metadata is validated before the FluxInstance activates reconciliation; pivot fails closed on missing, empty, or non-string source keys; TTL parsing rejects malformed, negative, compound, and overflowing values in both shell and Rust; the reconciler enforces the account guard, the region/cluster allowlist, and fresh ASG membership plus EKS ownership rechecks immediately before drift-only writes; and tag values survive NUL-delimited transport with exact UTC calendar validation. Docs (aws-iam.md, operations.md, bootstrap-cli.md) and AGENTS.md match the new behavior.

[shrinedogg]

Follow-up to my earlier CHANGES_REQUESTED review (5499814919), which was written against head 11748fc and submitted while the branch was being force-pushed. The new head 2e23e10 contains exactly the fix for that review's only blocking finding, so I re-verified everything at 2e23e10:

• The single delta between 11748fc and 2e23e10 is mgmt/aws/infrastructure/worker-run-tags/cronjob.yaml:29, which now pins aws-cli 2.37.12 @ sha256:ce44b800..., identical to main's line merged in #592.
• git diff origin/main...2e23e10 is empty: the branch tree is byte-identical to main. The PR carries no net change of its own; its purpose is to re-apply the #569 content on top of the #570 revert so the pair restores it on main after #570 lands. It remains unmergeable until #570 lands (or the pair is declined together).
• Re-ran at 2e23e10: kustomize build mgmt/aws/infrastructure/worker-run-tags OK; tests/test-shell-run-tags.py 3 tests OK; tests/test-worker-run-tags.py 11 tests OK; tests/test-aws-run-tags.py OK.
• Runtime probe of the new pinned digest (sha256:ce44b800...): /usr/bin/bash 5.2.15 and /usr/bin/python3 3.9.25 are present and the stdlib modules tag_data.py uses import cleanly.
• CI at 2e23e10: 8 of 9 checks pass; "Renovate airgap digest pinning" was still pending at review time (it passed at 11748fc, and the only change since is that image line).

No blocking findings remain at this head. One non-blocking item stands: the PR description still claims "The final repository tree matches squash commit 6b17af9... exactly", which is false at 2e23e10 (git diff 6b17af9..2e23e10: 47 files, +610/-245, nearly all of it correctly retained main-side progress such as the #511/#561 audit fixes). Please correct the body before retargeting to main; the go/no-go on the pair stays with the maintainer.

[lmilbaum]

Thanks for the reviews, Joe.

The blocking finding (cronjob.yaml image pin at 2.36.32) was already fixed in the force-push to 2e23e10 before your follow-up comment arrived -- glad the timing worked out.

The PR body has now been updated to remove the inaccurate "matches squash commit 6b17af9 exactly" claim. It now reads: at current head the tree is identical to main; the full #569 diff will appear once retargeted to main after #570 lands.

[shrinedogg]

APPROVE. Re-review at head cb5eac7, which is unchanged since the wave-3 capture. No blocking findings.

Prior findings, re-verified at this head:

• Wave-1 CHANGES_REQUESTED (review 5499814919, head 11748fc), the aws-cli digest pin: addressed. cronjob.yaml:29 pins 2.37.12 @ sha256:ce44b800..., byte-identical to the line main merged in #592.
• Wave-2 COMMENT (head 2e23e10) had zero findings. Since then the stack was rebuilt on current main: the base branch revert/pr-569-premature-merge moved to ae4f1e8 (rebase onto main 07dd4ad) and the head is the #569 change replayed as one commit on top of the revert. The PR's own change content is unchanged; the tree is byte-identical to main.

Verified locally against the stacked diff refs/review-pr570base...cb5eac7 (the full #569 change, 21 files, +1137/-154):

• CI at cb5eac7: all 9 checks green, including the Renovate airgap digest pinning job (red at capture time, green at this head).
• uv run --locked python tests/test-worker-run-tags.py: 11 tests OK (fake-CLI reconciler runs, safety rejections, kubectl kustomize render and wiring contract, expiry calendar and encoding bounds).
• uv run --locked python tests/test-aws-run-tags.py: OK.
• python3 tests/test-shell-run-tags.py: 3 tests OK (TTL parsing and rejection, bootstrap ordering, pivot copy and failure stop).
• Booted the exact pinned image digest (public.ecr.aws/aws-cli/aws-cli:2.37.12@sha256:ce44b800...): /usr/bin/python3 3.9.25 and /bin/bash 5.2.15 are both present, so reconcile.sh and the tag_data.py helper can run in the container; tag_data.py uses no post-3.9 syntax.
• bootstrap.sh:4 and pivot.sh:31 both set -euo pipefail, so prepare/apply failures abort before activate_flux; the ordering the new tests assert holds in the real scripts. seed_kubectl resolves SEED_KUBECONFIG, so pivot applies run tags with --kubeconfig MGMT_KUBECONFIG while the source read uses an explicit --context.
• Secret coupling checks out: the CronJob mounts ack-system/aws-credentials key credentials (declared in ack-controllers/aws-credentials.sops.yaml), and the Flux Kustomization dependsOn ack-controllers and aws-managed-clusters.

Reviewed the write guards specifically: exact account check plus a hard-coded designated-account assertion, region:cluster case allowlist, nodegroup/ASG/instance-ID format gates, ASG membership recheck and aws:eks:cluster-name ownership immediately before drift-only CreateTags, NUL-delimited transport and JSON tag writes that preserve whitespace and the CAPA-safe charset, exact UTC expiry validation including leap years and the 0001-9999 range, and overflow-bounded TTL arithmetic in both the shell (resolve_run_tags) and Rust (parse_duration_seconds, checked_expiry, from_configmap_value rejecting incomplete metadata).

Stack/draft note (process, not a defect): this PR is a draft stacked on #570 (revert/pr-569-premature-merge). GitHub reports it MERGEABLE and CLEAN into that base; against main the diff is empty by construction. After #570 lands, retarget this PR to main for Joe's review; neither PR should be merged automatically. Live acceptance of the worker-tag CronJob (operator IAM grant and an observed backfill) remains documented post-merge work in docs/operations.md, as the change itself states.

Looks good.

[shrinedogg]

APPROVE. Re-review at head 966e00b, unchanged since the wave-4 capture. The head moved from the wave-3-approved cb5eac7 by a rebase of the stacked pair onto the current base: #570's branch revert/pr-569-premature-merge is now 8b96d67 (the revert commit rebased onto main febb3d0), and this PR is the single restore commit on top of it (966e00b's parent is exactly 8b96d67, #570's current head). No blocking findings.

Delta since cb5eac7: docs-only content flowing in from the rebase (README restructure #597, local-host doc notes #603, proposal removal). The change patch itself is byte-identical to the wave-3-approved diff modulo hunk offsets: normalized full diffs (index lines and @@ headers stripped) compare equal.

Prior findings, re-verified at this head:

• Wave-1 CHANGES_REQUESTED (aws-cli digest pin): addressed. cronjob.yaml:29 pins public.ecr.aws/aws-cli/aws-cli:2.37.12 @ sha256:ce44b800..., byte-identical to the line main carries from #592. Stronger still: the full tree at this head is identical to current main across the entire change surface, since main still carries the prematurely merged #569 plus the #592 pin; this restore commit exactly re-applies what the revert in #570 removes, so the stacked pair is net-neutral against main until #570 lands.
• Wave-2 COMMENT: zero findings, nothing outstanding.
• Wave-3 APPROVE at cb5eac7: superseded by this verdict on the current head.

Verified locally at 966e00b:

• tests/test-worker-run-tags.py: 11 tests OK (uv run python).
• tests/test-aws-run-tags.py: OK (uv run python).
• tests/test-shell-run-tags.py: 3 tests OK (mise exec -- python3).
• bash -n passes on bootstrap-common.sh, bootstrap.sh, pivot.sh.
• CI at 966e00b: 9 of 9 checks pass, including Build kustomize overlays, Lint YAML, Lint Python (ruff), and the Renovate digest pinning jobs.

Draft and stack note (kept): this PR stays draft and stacked on #570. Do not merge either PR automatically. After #570 lands, retarget this PR to main and wait for Joe's review before any merge.

[shrinedogg]

Blocked on #570's recreation (see the comment there): main has been rebuilt on top of #569, so #570 as it stands would silently remove the worker-run-tags feature and #508's credential seeding. The APPROVED verdict at 966e00b stands for that head, but the pair needs #570 recreated against current main first; after it lands, mark this ready and it goes back into review.

[shrinedogg]

APPROVE: restore content verified at the new head 3537f64; fresh verdict after the Oct 16 rebase.

The head moved from 966e00b (approved Oct 13) to 3537f64 on Oct 16. Verified the move is a pure rebase: the branch delta line-for-line equals main's delta from febb3d0 to 184272a (i.e. #388, #508, #458); of #569's 21 files, 14 are bit-identical to the approved head and 7 (.github/workflows/validate.yml, AGENTS.md, bootstrap.sh, bootstrap-rs/src/main.rs, docs/aws-iam.md, mise.toml, pivot.sh) changed only to carry those main updates. No drift in the restored #569 content.

Verified:

• Stack shape: base is the #570 revert branch; the branch is two commits (revert + restore) on main 184272a; the restore commit re-adds exactly #569's 21-file set (git diff --name-status against the revert commit).
• Net-pair property: the head tree equals main 184272a's tree byte-for-byte, so once #570 lands and this branch is rebased onto the new main, merging lands main's current tree exactly: the reviewed #569 change is in history, no leftover hunks.
• aws-cli carry-forward: cronjob.yaml:29 pins 2.37.12 (digest prefix ce44b800), byte-identical to main's pin from #592; the revert-and-restore pair does not regress the bump.
• Feature read in context at this head: reconcile.sh (designated-account guard doubled against a hardcoded value, cluster allowlist, nodegroup/ASG/instance regex validation, ASG membership recheck immediately before the ownership read, InvalidInstanceID.NotFound races skipped, terminated/shutting-down skipped, four-tag idempotency compare, per-instance error isolation with exit status), tag_data.py (CAPA-aligned ASCII tag charset, exact UTC expiry with calendar validation, NUL-free field check, exactly-one-instance), bootstrap-common.sh resolve_run_tags (TTL validated before any mutation; digit-loop parse that never evaluates input as arithmetic; RFC3339 overflow bound), pivot ordering read_source_run_tags (all four keys nonempty) then prepare_flux, apply_run_tags, activate_flux, run_tags.rs (rerun-safe ConfigMap read, server-metadata stripping before the target apply, checked_expiry, RFC3339 calendar math with unit tests), cronjob.yaml hardening (nonroot 65532, no service account token, drop ALL, read-only rootfs, 55s deadline inside a 60s cycle, Forbid), flux-ks.yaml (wait plus dependsOn on ack-controllers and aws-managed-clusters), ConfigMap wiring via configMapGenerator.
• Local re-runs at this head: tests/test-shell-run-tags.py 3/3 OK (the TTL injection case x[$(touch ...)] exits before any kubectl/helm call; bootstrap and pivot sequences verified fail-fast), tests/test-worker-run-tags.py 11/11 OK under the repo's locked uv environment (includes kustomize render wiring against the real overlays), bash -n on reconcile.sh, py_compile on tag_data.py.
• Pinned image booted read-only: /bin/bash and python3 (3.9.25) are present; the tag_data.py cluster subcommand was verified in-image (a valid tag set emits the NUL fields and tag JSON; 2026-02-30T12:00:00Z is rejected with "day is out of range for month", exit 1).
• CI: 9/9 checks green at 3537f64 (Rendered Flux diff is not present because the PR diff against main is empty while stacked, as the body documents).

Warning:

1. The stack sits on main 184272a, which predates #572 (c48201a, the current main HEAD). After the fixed #570 merges, this branch must be rebased onto the new main before merging, as the body already plans. Merging the current branch as a plain merge commit would compute the merge base as 184272a and nullify the restore (this tree already equals that base), leaving #569 removed.

Cross-reference:

• #570 at 89f30f6 additionally rolls back #572's docs in four files; this restore does not bring that content back. The end state is clean only if #570 is fixed (changes requested there) before the pair merges.
• #569: this PR is the re-reviewable form of its squash commit 6b17af9.