fix(postmerge-5120): 4 Copilot findings — drop devShell install.sh auto-run + §24 reframe + workflow comment#5121
Merged
AceHack merged 1 commit intoMay 26, 2026
Conversation
… P1 §24/mkpasswd misframe + Nit workflow comment
P0 (full-ai-cluster/flake.nix shellHook): running tools/setup/install.sh
automatically on every `nix develop` shell entry has large host-side
side effects (apt/brew installs, profile edits, network fetches,
possible sudo prompts) and breaks typical devShell expectations.
Reliably FAILS on NixOS (no apt) which is in supportedSystems. Fix:
drop the auto-run shellHook, replace with a one-line hint pointing
operators at install.sh for manual host setup when needed. (The
maintainer 2026-05-26 question "is nixos debian based or we going to
have to update our deps managment like it uses apt?" lands on the
exact root cause — NixOS uses nix, not apt; install.sh currently
only supports brew + apt paths.)
P1 (flake.nix comment block): reworded to drop the inaccurate
"4th consumer per §24" framing (§24 specifies three; saying "4th"
while citing §24 was inconsistent) and to drop the mkpasswd claim
(mkpasswd is NOT in tools/setup/manifests/{brew,apt}; for the iter-5.3
prompt-for-password substrate, mkpasswd comes via the NixOS installer
ISO's environment.systemPackages, not via install.sh). New comment
states the actual contract: install.sh is the canonical dev-laptop
tooling source per §24's three-consumer set; devShell is nix-managed
admin tooling only.
Nit (build-ai-cluster-iso.yml audit step): reworded to honestly
describe the 7z source-of-truth split — CI relies on ubuntu-24.04
runner-default 7z (not install.sh; the workflow doesn't invoke
install.sh); manifests cover local-dev parity only. Future-proof
note added for ubuntu-version change.
The 4th post-merge thread (P1 on tools/ci/audit-installer-iso-content.ts
line 183 about exit-3 doc) is stale: 585f9d2 already widened the
exit-3 description to "missing path OR present-but-empty"; the review
was based on an earlier commit. Resolving as no-op alongside this fix.
Co-Authored-By: Claude <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR removes automatic execution of tools/setup/install.sh from the full-ai-cluster Nix devShell (avoiding side effects and NixOS breakage) and tightens accompanying commentary about the install-script contract and 7z provenance in CI vs local setup.
Changes:
- Stop auto-running
tools/setup/install.shonnix developentry; replace with a manual hint in the devShellshellHook. - Reframe devShell comments to align with
GOVERNANCE.md §24(three install.sh consumers) and remove the prior “4th consumer” framing. - Update the workflow audit-step comment to describe where 7z comes from in CI vs local environments.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
full-ai-cluster/flake.nix |
Removes install.sh auto-run from devShell hook; updates explanatory comments and prints a manual host-setup hint. |
.github/workflows/build-ai-cluster-iso.yml |
Comment-only clarification about 7z dependency provenance for the ISO audit step. |
4 tasks
AceHack
added a commit
that referenced
this pull request
May 26, 2026
…frame (#5122) * fix(postmerge-5121): 2 Copilot findings — OS-scope shellHook install.sh hint + workflow-comment §24 reframe (1) full-ai-cluster/flake.nix shellHook: the unconditional "Host setup (rare): bash tools/setup/install.sh" hint conflicted with the preceding comment block stating auto-running install.sh fails on NixOS. Footgun — a NixOS dev would see the hint, run install.sh, and hit apt-get errors. Fix: scope the hint to hosts where install.sh actually works (macOS via uname -s = Darwin; Debian/Ubuntu via /etc/os-release ID/ID_LIKE check). Print nothing on NixOS — those operators get tooling via the devShell's nix-managed packages. (2) .github/workflows/build-ai-cluster-iso.yml audit step comment: "Manifests cover local-dev parity only" understates the manifests' role. GOVERNANCE.md §24 names manifests as the canonical dep declaration for ALL install.sh consumers (dev laptops, CI runners, devcontainer images) — not local-dev-only. Reworded to: (a) manifests ARE the canonical declaration for all install.sh consumers, AND (b) this SPECIFIC workflow doesn't invoke install.sh, relying instead on the ubuntu-24.04 runner-image's default 7z pre-install as a per-run-cost shortcut. Future-proof note preserved. Comment-only changes; no behavior change. Both findings caught by Copilot post-merge review on #5121. Co-Authored-By: Claude <noreply@anthropic.com> * fix(postmerge-5121): apt-get fallback needs `-y` to avoid CI hang (Copilot finding on #5122) The documented future-proof fallback ("if we drop the ubuntu-24.04 pre-install, add `apt-get install p7zip-full`") would hang interactively on CI runners that lack the assumeYes default. Fixed: explicit `sudo apt-get update && sudo apt-get install -y p7zip-full` with a parenthetical naming WHY `-y` matters so a future copy-paste doesn't re-introduce the hang. Also acknowledging the other Copilot finding on #5122 in the PR description update (separate `gh pr edit`): the shellHook conditional DOES change user-facing output behavior (NixOS hosts no longer see the install.sh hint); PR title said "comment + shellHook edits" but the description claimed "no behavior change" — substrate-honest correction. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Lior <lior@zeta.dev> Co-authored-by: Claude <noreply@anthropic.com>
AceHack
pushed a commit
that referenced
this pull request
May 26, 2026
…5/B-0794 wrong paths
(1) `[B-0247](../P*/B-0247-*.md)` — markdown links don't support globs;
GitHub won't resolve. Linked directly to
`../P1/B-0247-ace-dlc-content-packs-kernel-extensions-package-manager-2026-05-07.md`.
(2) `[B-0805](B-0805-...)` — relative path missing `../P1/` prefix;
B-0805 is under docs/backlog/P1/ while this row is under
docs/backlog/P2/. Fixed 5 occurrences via sed (lines 36, 104,
316, 355, 362).
(3) `[B-0794](B-0794-iter-5-4-...)` — same shape as (2): missing
`../P1/` prefix AND wrong slug. The actual on-main B-0794 slug is
`B-0794-node-self-registers-in-git-under-maintainers-cluster-nodes-
triggers-argocd-full-bringup-of-k8s-apps-charts-gitops-native-
cluster-substrate-aaron-2026-05-26.md` per `find docs/backlog
-name B-0794*`. Fixed 2 occurrences.
Pattern note: this is the same broken-link class Copilot caught
earlier in this session on #5121 (B-0794 wrong slug). I keep
authoring these from training-data default slugs instead of running
`find docs/backlog -name "B-NNNN*"` first — fits the empirical-anchor
pattern for the verify-existing-substrate-before-authoring rule
landing in parallel via PR #5131.
Co-Authored-By: Claude <noreply@anthropic.com>
AceHack
added a commit
that referenced
this pull request
May 26, 2026
… 'package manager of package managers'; B-0806 sits INSIDE Ace not parallel to it (#5130) * fix(B-0806): substrate-honest correction — Ace agenda already encodes "package manager of package managers"; B-0806 sits INSIDE Ace, not parallel to it The maintainer 2026-05-26 substrate-honest catch: "that is what ace has been since we first talked about it you just keep forgetting we have substantial backlog around this" Caught a recurrence of the same agent-discipline gap that produced the cascade #4 ISO audit failure (PR #5125) earlier today: authoring substrate from incomplete view of what already exists. The Ace package-manager-of-package-managers framing is canonical existing substrate, NOT a new architectural insight surfaced by B-0806. Existing Ace substrate I should have read first: - docs/agendas/ace-package-manager/AGENDA.md (OPERATOR-SELF-CLAIMED 2026-05-22; 13-stage Ace lifecycle; polyglot package contents; proto-governance via hats + multi-oracle BFT; symmetric/decentralized) - docs/trajectories/ace-package-manager-skill-crystallization-pipeline/ RESUME.md (active trajectory) - memory/project_ace_package_manager_unrestricted_local_models_guardian_ oversight_aaron_2026_05_07.md (canonical Aaron 2026-05-07 disclosure: unrestricted local models + Guardian/KSK + Bond Curve + Itron composition) - memory/feedback_aaron_ace_package_manager_homebrew_shape_bootstrap_ website_chat_interface_full_distribution_stack_no_setup_needed_2026_ 05_13.md (full distribution stack) - B-0247 (parent), B-0287 (closed format spec), B-0288 (in-progress CLI), B-0424 (repo-split), B-0742, B-0777 (related backlog cluster) - docs/research/2026-05-22-ace-package-format-spec-v2-substrate- engineering-pipeline-extension.md (DeepSeek 2026-05-22 substrate- engineering pipeline extension) Changes: - Reframed B-0806's Ace section as "this row sits INSIDE the Ace agenda as one instance of stage-8 (distribute), NOT parallel to it" - Added complete substrate-table citing the canonical Ace docs - Reworded architecture diagram annotations to credit canonical Ace framing (not my "architectural insight") - Explicitly named this as a second empirical anchor for the verify-existing-substrate-before-authoring discipline gap (sibling failure mode to cascade #4 ISO audit; PR #5125 + #5126) Also fixes MD040 (missing language on fenced code blocks at line 111 and 196) — `text` language tag added. Co-Authored-By: Claude <noreply@anthropic.com> * fix(B-0806): add NixOS-as-north-star framing per the maintainer 2026-05-26 The maintainer 2026-05-26: "nixos is our north star for declarative gitops ease" This is the FRAMING PRINCIPLE for the whole iter-7 arc: NixOS sets the gold-standard target; ansible+ace+crossplane exist to approximate the NixOS-native experience on platforms that don't have it (Windows, macOS, non-NixOS Linux). Every sub-target design decision answers: "does this make non-Nix MORE like NixOS, or does it add a parallel imperative-shape?" Former is the direction; latter is the failure mode. Added new top-section "## North star" capturing this verbatim, with the framing-implications for sub-target design decisions called out. Co-Authored-By: Claude <noreply@anthropic.com> * fix(B-0806): integrate hats + fork-negotiation into architecture flow per maintainer 2026-05-26 — 3rd same-pattern catch this session The maintainer 2026-05-26: "i'm assuming you have the hat / fork negoation for ace too" Third instance today of authoring-from-incomplete-view of the Ace substrate. I cited B-0742 + B-0777 in the previous correction's substrate-table but did NOT integrate hats + fork-negotiation into B-0806's architectural flow. The Ace agenda already specifies: "Hats = controls + self-bindings over time crystals (PAIR is load- bearing primitive)" + "proto-governance via skill-bound hats with multi-oracle BFT (authority + bindings tied to skills)" — canonical existing substrate I should have integrated, not bolted on. Changes: (1) Added "### Architectural integration of hats + fork-negotiation" section showing the 5-step Ace invocation flow for every `ace install <pkg>`: 1a. Hat resolution (skill-bound; PAIR primitive) 1b. Multi-oracle BFT proto-governance (N-of-M consent) 1c. Cross-fork ontology negotiation (per B-0741/B-0777; per-persona ontology maps) 1d. Guardian/KSK gate (per canonical Ace project memory; Bond Curve pricing; local receipts; high-risk multi-N-of-M) 1e. ace install proceeds + receipt written (2) Added B-0741 to the substrate-citation table with explicit "CLOSED prematurely earlier this session" annotation. The close was mechanically justified (DIRTY conflict) but the substrate is load-bearing for B-0806's architectural integration. (3) New "## Sub-row to re-file" section tracks B-0741 as a known dependency for iter-7 implementation; needs cherry-pick re-land per pr-triage-tiers Tier 3. (4) Updated "agent-discipline failure" note to mark this as the THIRD instance today (cascade #4 ISO audit / B-0806 Ace-section / B-0806 hats-fork-negotiation). Pattern is clear enough that the "verify-existing-substrate-before-authoring" rule extension to dep-pin-search-first-authority is genuinely load-bearing. Co-Authored-By: Claude <noreply@anthropic.com> * fix(B-0806): 2 Copilot P1 broken xrefs on #5130 — B-0247 glob + B-0805/B-0794 wrong paths (1) `[B-0247](../P*/B-0247-*.md)` — markdown links don't support globs; GitHub won't resolve. Linked directly to `../P1/B-0247-ace-dlc-content-packs-kernel-extensions-package-manager-2026-05-07.md`. (2) `[B-0805](B-0805-...)` — relative path missing `../P1/` prefix; B-0805 is under docs/backlog/P1/ while this row is under docs/backlog/P2/. Fixed 5 occurrences via sed (lines 36, 104, 316, 355, 362). (3) `[B-0794](B-0794-iter-5-4-...)` — same shape as (2): missing `../P1/` prefix AND wrong slug. The actual on-main B-0794 slug is `B-0794-node-self-registers-in-git-under-maintainers-cluster-nodes- triggers-argocd-full-bringup-of-k8s-apps-charts-gitops-native- cluster-substrate-aaron-2026-05-26.md` per `find docs/backlog -name B-0794*`. Fixed 2 occurrences. Pattern note: this is the same broken-link class Copilot caught earlier in this session on #5121 (B-0794 wrong slug). I keep authoring these from training-data default slugs instead of running `find docs/backlog -name "B-NNNN*"` first — fits the empirical-anchor pattern for the verify-existing-substrate-before-authoring rule landing in parallel via PR #5131. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Lior <lior@zeta.dev> Co-authored-by: Claude <noreply@anthropic.com>
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
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
Addresses 4 Copilot post-merge findings on #5120 (cascade #4 fix-fwd).
full-ai-cluster/flake.nixshellHook was auto-runningtools/setup/install.shon every `nix develop` entry. Reliably fails on NixOS (no apt; in supportedSystems) and has large side effects (apt/brew installs, sudo prompts, network fetches, profile edits). Replaced with a one-line hint. The maintainer 2026-05-26 question about NixOS-vs-debian lands on the same root cause.The 4th post-merge thread (exit-3 doc) is stale — already widened on main by 585f9d2; will resolve no-op alongside this merge.
Test plan
🤖 Generated with Claude Code