diff --git a/host-setup/README.md b/host-setup/README.md index 0c2f1052..f079d8de 100644 --- a/host-setup/README.md +++ b/host-setup/README.md @@ -78,7 +78,7 @@ bash menu.sh host-setup/menu.sh # from a checkout of this repository, or of any other repo in the fleet ``` -It answers a different question than `bootstrap.sh` does. `bootstrap.sh` stands a host up and stops. `menu.sh` loops, because a person sitting down with it usually wants more than one thing done in a sitting, and it tells the hub apart from whichever repo it happens to be run from: a checkout of `ptr727/ProjectTemplate` gets the hub tasks (audit a cataloged repo, check the generated Skills distributions), a checkout of any other repo gets the downstream tasks (check or pull the hub's verbatim-owned files into that repo's own worktree, per [`scripts/carry.py`][carry]), and both get the host tasks this directory's `linux/` tooling already provides. Run from neither, only the host tasks show, since there is no repository to audit or pull files into. +It answers a different question than `bootstrap.sh` does. `bootstrap.sh` stands a host up and stops. `menu.sh` loops, because a person sitting down with it usually wants more than one thing done in a sitting, and it tells the hub apart from whichever repo it happens to be run from: a checkout of `ptr727/ProjectTemplate` gets the hub tasks (audit a cataloged repo, check the generated Skills distributions), a checkout of any other repo gets the downstream tasks too (check or pull the hub's verbatim-owned files into that repo's own worktree, per [`scripts/carry.py`][carry]), and every run gets the host tasks this directory's `linux/` tooling already provides. Only the downstream tasks need a repo to run from. The hub tasks fetch the hub themselves when there is no local checkout to reuse, so they still show and still work when the menu is run entirely standalone, off no checkout at all. Reaching `spec/audit.py` and `scripts/carry.py` from outside a hub checkout means fetching one, the same "hosted and reached, never carried" model [`scripts/README.md`][scripts-readme] states for those tools generally. `menu.sh` clones fresh rather than reusing `bootstrap.sh`'s tarball, since `scripts/carry.py` itself checks that its hub argument is a real git checkout on a freshly fetched `origin/main` with no local changes, and a full clone rather than a shallow one, since `spec/audit.py` walks the hub's own commit history to judge whether a carried copy is trailing the file it was copied from. Run from inside the hub itself, that same freshness is confirmed against the local checkout before a hub task uses it, falling back to a fresh clone when the local checkout has moved on, so an audit or a Skills-distribution check never silently reads a stale or feature-branch tree. diff --git a/host-setup/menu.sh b/host-setup/menu.sh index 56a9f044..6c094679 100755 --- a/host-setup/menu.sh +++ b/host-setup/menu.sh @@ -84,14 +84,32 @@ remove_unowned_hub_check() { return 1 } +# The lock lives only here, in the wrapper, so the locked body below can use its ordinary fail/return pattern with no awareness of it. +# A RETURN trap was tried and dropped: bash does not scope one to the function that set it, so it re-fires (against an already-unset local by then) on whatever function returns next, which surfaced as this script's own "unbound variable" crash on the very next return up the call chain. fetch_hub() { # --dry-run promises to change nothing, and fetching is the one real change this whole script makes to the host. [[ $DRY_RUN == true ]] && { fail "This task needs a fetched hub checkout, and fetching one is itself a change --dry-run does not make. Run without --dry-run, or from inside a hub checkout already on $DEFAULT_REF." return 1 } - step "Fetching $HUB_REPO at $REF" mkdir -p "$DIR" + # Held for the rest of this fetch, so a second menu.sh sharing this --dir blocks here instead of passing remove_unowned_hub_check and deleting the tree this one is still cloning into. + # Closed unconditionally on the way out, success or failure, rather than left open for the rest of this process: interactive_menu's loop keeps it alive well past this one fetch otherwise, and a lock nothing ever releases blocks every other menu.sh sharing this --dir until this session quits. + local lock_fd rc + exec {lock_fd}>"$DIR/hub.lock" + if ! flock "$lock_fd"; then + fail "Could not lock $DIR/hub.lock" + exec {lock_fd}>&- + return 1 + fi + rc=0 + fetch_hub_locked || rc=$? + exec {lock_fd}>&- + return "$rc" +} + +fetch_hub_locked() { + step "Fetching $HUB_REPO at $REF" remove_unowned_hub_check || return 1 rm -rf "$DIR/hub" # A full clone of the default branch first, whatever $REF names: spec/audit.py walks the hub's own history to judge whether a carried copy is trailing the file it was copied from, and a shallow clone would read every file as changed at the truncation boundary and misreport every repo as stale. @@ -100,6 +118,8 @@ fetch_hub() { fail "Could not clone $HUB_REPO. Check that this host reaches github.com." return 1 } + # Marked as ours the moment the clone lands rather than only once every later step also succeeds, so a failure below still leaves a tree remove_unowned_hub_check will clean up on the next run instead of blocking every retry as somebody else's. + touch "$(marker_path)" # A branch name is already checked out by the clone above. # A tag, a pull request ref, or a commit needs an explicit fetch and checkout, since "git clone --branch" only takes a branch or a tag, not an arbitrary commit. if [[ $REF != "$DEFAULT_REF" ]]; then @@ -114,7 +134,6 @@ fetch_hub() { return 1 } fi - touch "$(marker_path)" HUB_ROOT="$DIR/hub" HUB_FETCHED=true info "Cloned to $HUB_ROOT" @@ -196,10 +215,11 @@ host_tool() { # The Python tools under scripts/ and spec/ resolve their own root from __file__ rather than the working directory, so they are called by absolute path from wherever this script runs and need no cd. # Checked here rather than upfront in main, the same reasoning host-setup/linux/install-skills.sh already carries: a host with no interpreter yet can still use every host action, and only the actions that need one name it as their own prerequisite. +# The prerequisite failure returns 127, bash's own "command not found" convention, so a caller reading a specific exit code from the tool itself (build_dist.py's 0-clean/1-stale contract) can tell "python3 never ran" apart from "python3 ran and returned 1". hub_python() { command -v python3 >/dev/null || { - fail "python3 is required for this task; host-setup/linux/install-tools.sh provides it" - return 1 + fail "python3 is required for this task. host-setup/linux/install-tools.sh provides it." + return 127 } local script="$1" shift @@ -220,18 +240,32 @@ audit_repo() { check_skills_dist() { ensure_hub_root || return 1 - if hub_python scripts/build_dist.py --check; then - info "Every generated Skills distribution matches .agents/skills/" - else - info "A generated Skills distribution is stale; this menu does not regenerate it from a fetched checkout, since the result has to be committed in the hub itself" - fi + local rc=0 + hub_python scripts/build_dist.py --check || rc=$? + # Only 0 (clean) and 1 (stale) are outcomes scripts/build_dist.py --check documents for itself, so only those two read as a check result. + # Anything else, 127 included, is hub_python or the tool itself failing to run rather than a finding, and is reported as the task error it is. + case "$rc" in + 0) info "Every generated Skills distribution matches .agents/skills/" ;; + 1) info "A generated Skills distribution is stale. This menu does not regenerate it from a fetched checkout, since the result has to be committed in the hub itself." ;; + *) + fail "scripts/build_dist.py --check did not run to completion (exit $rc)" + return 1 + ;; + esac } carry_action() { local mode="$1" [[ -n $DOWNSTREAM_ROOT ]] || { - fail "No downstream repo checkout found; run this menu from inside the target repo's own worktree" + fail "No downstream repo checkout found. Run this menu from inside the target repo's own worktree." + return 1 + } + # Its hub argument must be exactly on origin/main by carry.py's own requirement, and a non-default --ref checks out something else entirely, so this would always fail deep inside carry.py with no clue why. + # Refused here instead, with the actual reason. + [[ $REF == "$DEFAULT_REF" ]] || + { + fail "Pulling hub files needs the hub's $DEFAULT_REF branch, and this session was started with --ref $REF. Run without --ref, or start a separate session on $DEFAULT_REF for this task." return 1 } local default="$DOWNSTREAM_NAME" @@ -271,7 +305,8 @@ print_menu() { log "Hub, ptr727/ProjectTemplate:" log " 10 Audit a cataloged repo" log " 11 Check the generated Skills distributions are current" - if [[ -n $DOWNSTREAM_ROOT ]]; then + # Also gated on REF: carry.py always rejects a hub checkout that is not exactly on the default ref, so these tasks cannot work in a non-default --ref session regardless of a downstream repo being detected. + if [[ -n $DOWNSTREAM_ROOT && $REF == "$DEFAULT_REF" ]]; then log "" log "Downstream, the repo this menu is run from:" log " 12 Check what the hub would change here, change nothing" @@ -351,8 +386,11 @@ parse_args() { --dir) [[ $# -ge 2 ]] || die "--dir takes a path" [[ $2 == /* ]] || die "--dir takes an absolute path, and \"$2\" is relative" - [[ $2 != "/" ]] || die "--dir may not be the root directory" - DIR="${2%/}" + # Canonicalized before the root check, since a literal "/tmp/.." is not the string "/" but resolves to it the moment anything below opens a path under it. + local canonical + canonical=$(readlink -m -- "$2") || die "--dir could not be resolved: \"$2\"" + [[ $canonical != "/" ]] || die "--dir may not be the root directory" + DIR="$canonical" shift ;; -h | --help) diff --git a/scripts/README.md b/scripts/README.md index 264b182c..f416a0a8 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -221,7 +221,7 @@ The match is on the block's heading rather than anywhere in the body, and on the Regenerates [`.github/skills/`][github-skills-dist] and [`.claude-plugin/fleet-skills/`][fleet-skills-dist] from [`.agents/skills/`][agents-skills], the hub's own hand-authored fleet Skills. Codex and opencode read `.agents/skills/` directly, GitHub Copilot reads `.github/skills/`, and Claude Code reads the generated plugin published through [`.claude-plugin/marketplace.json`][marketplace]. `.agents/skills/` stays the one place a skill is hand-edited. Both generated trees are never hand-edited. -`--check` is the read-only mode: it exits `1` when either generated tree differs from `.agents/skills/`, comparing a digest over every file rather than a file count or timestamp. CI runs `--check` rather than trusting a contributor to have run the generator, the same reason `spec/audit.py` exists rather than trusting a hand-carried file. +`--check` is the read-only mode: it exits `1` when either generated tree differs from `.agents/skills/`, comparing a digest over every file rather than a file count or timestamp, and `2` on a real failure (a symlink under `.agents/skills/`, an unreadable file), so a caller reading the exit code can tell that apart from the stale finding. CI runs `--check` rather than trusting a contributor to have run the generator, the same reason `spec/audit.py` exists rather than trusting a hand-carried file. ## `carry.py` diff --git a/scripts/build_dist.py b/scripts/build_dist.py index 74cf18ba..ea3787db 100755 --- a/scripts/build_dist.py +++ b/scripts/build_dist.py @@ -9,7 +9,11 @@ place a skill's content is ever hand-edited. Usage: python3 scripts/build_dist.py regenerate distributions from .agents/skills/ - python3 scripts/build_dist.py --check read-only: exit 1 if a distribution is stale + python3 scripts/build_dist.py --check read-only: exit 0 clean, 1 stale, 2 on a real + failure (a symlink under .agents/skills/, an + unreadable file), so a caller reading the exit + code can tell a finding apart from the check + itself not having run. """ from __future__ import annotations @@ -162,8 +166,10 @@ def is_stale(): names = skill_names() try: manifest = json.loads(PLUGIN_MANIFEST.read_text(encoding="utf-8")) - except (OSError, json.JSONDecodeError): + except json.JSONDecodeError: + # A corrupted or hand-edited manifest is exactly the stale case this function exists to catch. return True + # OSError (an unreadable file, one removed between the is_file() check above and this read) is deliberately not caught here: --check's own caller needs it to propagate as the execution failure it is, not read as this function's ordinary stale result. # The full manifest, not only "skills". # A hand-edited description/author/version is exactly as much a corrupted-plugin case as a hand-edited skills list. # The manifest is entirely deterministic from `names`, so comparing all of it costs nothing extra to get right. @@ -190,16 +196,18 @@ def main(): parser.add_argument( "--check", action="store_true", - help="read-only: exit 1 if a generated skill distribution is stale", + help="read-only: exit 0 clean, 1 stale, 2 on a real failure", ) args = parser.parse_args() if args.check: try: stale = is_stale() - except ValueError as exc: + except (ValueError, OSError) as exc: + # 2 rather than 1, so a caller reading the exit code (host-setup/menu.sh among them) can tell this apart from the stale result below, which also exits 1 by this flag's own documented contract. + # OSError alongside ValueError: is_stale() reads several files beyond the one call already wrapped in its own try/except, and a permissions problem or a file removed out from under it raises that, not ValueError. print(exc, file=sys.stderr) - return 1 + return 2 if stale: print( "Generated skill distributions are stale: run `python3 scripts/build_dist.py`.", diff --git a/scripts/tests/test_build_dist.py b/scripts/tests/test_build_dist.py index 82959074..3ca2f74c 100755 --- a/scripts/tests/test_build_dist.py +++ b/scripts/tests/test_build_dist.py @@ -7,6 +7,7 @@ from __future__ import annotations import json +import os import sys import unittest from pathlib import Path @@ -265,6 +266,52 @@ def test_main_reports_a_symlink_cleanly_instead_of_a_raw_traceback(self) -> None exit_code = build_dist.main() self.assertEqual(exit_code, 1) + def test_check_reports_a_symlink_as_2_not_1(self) -> None: + """1 is --check's own documented "stale" result, so a caller reading the exit code (host-setup/menu.sh among them) needs a different code to tell a real failure apart from that finding. + + is_stale() short-circuits to True (stale, exit 1) the moment the distribution stamp is + missing, so the symlink has to be introduced only after a clean regenerate() already + produced one, reaching the digest walk that actually raises rather than the early return. + """ + self.make_skill("foo") + build_dist.regenerate() + (self.skills_src / "foo" / "escape").symlink_to(self.tmp) + from unittest import mock + + with mock.patch("sys.argv", ["build_dist.py", "--check"]), mock.patch("builtins.print"): + exit_code = build_dist.main() + self.assertEqual(exit_code, 2) + + def test_check_reports_an_os_error_as_2_not_1(self) -> None: + """is_stale() reads several files beyond the one already wrapped in its own try/except, and a permissions problem or a file removed out from under it raises OSError there, not ValueError.""" + from unittest import mock + + with ( + mock.patch("sys.argv", ["build_dist.py", "--check"]), + mock.patch("builtins.print"), + mock.patch.object(build_dist, "is_stale", side_effect=OSError("permission denied")), + ): + exit_code = build_dist.main() + self.assertEqual(exit_code, 2) + + def test_check_reports_an_unreadable_manifest_as_2_not_1(self) -> None: + """The manifest read has its own try/except inside is_stale() (JSONDecodeError, a genuinely stale manifest), and an OSError there has to propagate through it rather than being caught by the same clause, or an unreadable file reads as the ordinary stale result this test would otherwise miss.""" + if os.name != "posix": + self.skipTest( + "chmod does not carry POSIX unreadable-file semantics, and os.geteuid() does not exist, on this platform" + ) + if os.geteuid() == 0: + self.skipTest("running as root ignores the permission bits this test depends on") + self.make_skill("foo") + build_dist.regenerate() + build_dist.PLUGIN_MANIFEST.chmod(0o000) + self.addCleanup(build_dist.PLUGIN_MANIFEST.chmod, 0o644) + from unittest import mock + + with mock.patch("sys.argv", ["build_dist.py", "--check"]), mock.patch("builtins.print"): + exit_code = build_dist.main() + self.assertEqual(exit_code, 2) + if __name__ == "__main__": unittest.main()