fix(postmerge-5352): Copilot 5 findings — schema (roles/registration.maintainer/hardware.storage) + MAC parsing + subshell error handling + comment-name redaction#5355
Merged
AceHack merged 1 commit intoMay 26, 2026
Conversation
…ing + subshell error handling + comment-name redaction 5 legitimate findings on PR #5352 (iter-5.4.1 self-registration), all real bugs that would block end-to-end self-registration: 1. **CRITICAL — subshell could kill installer** (line 806 of #5352): subshell inherited `set -euo pipefail`; ANY failure inside (git push permission denied, gh pr create scope missing, network drop) would propagate out + abort the installer BEFORE nixos-install runs. Step 6.9 is documented warning-only/skippable so it MUST never abort. FIX: subshell-local `set +e` + outer `|| true` defense-in-depth + explicit success/fail handling around git push + gh pr create with WARN-to-stderr on failure. 2. **MAC parsing wrong** (line 730 of #5352): `awk ... $(NF-2)` extracted `brd` not the MAC. `ip -o link` outputs `link/ether <MAC> brd <broadcast>`. FIX: `awk '{for(i=1;i<=NF;i++) if($i=="link/ether"){print $(i+1); exit}}'` parses the field AFTER `link/ether` correctly. 3. **Schema mismatch — spec.roles array** (line 749 of #5352): had `spec.role: $HOST` (scalar) but B-0813 ClusterNode CRD defines `spec.roles` as ARRAY. FIX: `spec.roles:\n - $HOST`. 4. **Schema mismatch — spec.registration.maintainer** (line 749 of #5352): had `spec.maintainer: $MAINTAINER` (top-level) but B-0817 places maintainer under `spec.registration.maintainer` (B-0813 CRD doesn't allow arbitrary spec fields; the reconciler reads `spec.registration.*` for maintenance metadata). FIX: nested under `spec.registration:` with timestamp + flake-commit + flake-host + registered-via siblings. Also added `metadata.labels` for the standard `zeta.lucent-financial-group.com/maintainer` label to support kubectl grouping. 5. **Schema mismatch — spec.hardware.storage** (line 761 of #5352): had `storage:` as sibling of `hardware:` but B-0813 places storage UNDER hardware block. FIX: indent storage 6 spaces (under hardware:) instead of 4 (sibling). Storage lines indented to 8 spaces accordingly. Same for network block (moved under hardware). 6. **Name attribution in comment** (line 691 of #5352): comment had "maintainers/aaron/cluster-nodes/" — direct maintainer name in current-state script. Per AGENT-BEST-PRACTICES no-name-attribution in .claude/rules/** + code/docs. FIX: replaced with placeholder "maintainers/<operator>/cluster-nodes/". No new substrate; all 5 fixes preserve existing structure + intent. Bash syntax OK. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
10 tasks
AceHack
deleted the
otto/fix-pr-5352-copilot-5-findings-schema-mac-subshell-error-handling-2026-05-26
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes 5 legitimate Copilot findings on merged PR #5352 (iter-5.4.1 self-registration). All 5 are real bugs that would block end-to-end self-registration.
5 fixes
Why CRITICAL #1 matters
Per the operator's CORE REQUIREMENT (B-0835): post-boot fully-operational chain without operator login. If Step 6.9 aborts the installer (because of a transient gh-API failure OR scope issue), nixos-install NEVER RUNS and the install fails completely. Step 6.9 is documented warning-only/skippable; the subshell hazard made that documentation a lie.
Schema source
Test plan
link/ether aa:bb:cc:dd:ee:ff brd ff:ff:ff:ff:ff:ff→aa:bb:cc:dd:ee:ff✓🤖 Generated with Claude Code