[brettheap]

Lane: xfactory-2 (xFactory-2)
Claim: #37 (comment)
Governing issue: #37

What this does

Adds scripts/setup-estate-commands.sh, which installs openRepoShape, openRepoTools, park and resume on the host from workBenches' own vendored pin (devBenches/base-image/upstream-pin.yaml). It runs update-upstream.py check first; only if that passes does it print the two pinned commits and run the two vendored shims' own --install, invoked as files so each places its own bytes (and openRepoTools copies park/resume from beside itself) with no network and no version logic of this script's own. setup.sh calls it under a new log_header "ESTATE COMMANDS" block, after shell setup and before the interactive selector, best-effort like the Wave widgets step (WORKBENCHES_SKIP_ESTATE_COMMANDS=1 skips it; any failure here is caught and setup continues). The regression test is wired into .github/workflows/speckit-git-bash.yml immediately after the existing pin-check step. A paragraph under ## setup.sh Flow, plus a sentence under ### Re-running setup.sh, documents the mechanism in the README.

Review round: an adversarial review found and named six reproducible defects, D1-D6; all six are fixed in a4bdbe8, entirely inside the script, its test, and the README — the vendored shims are untouched (ruling 4).

The four rulings, and where each lives in the diff

1. "Version" means the pin, not a number. Reads the two pinned commits from update-upstream.py list (a stable, parseable form) and prints openRepoShape pinned at <40hex> / openRepoTools pinned at <40hex>. Neither shim gains a --version change.
2. The host follows the pin, both ways. Invokes both shims' --install unchanged, relaying their own per-file already installed ... (unchanged) / updated / installed verbs. setup.sh's new block is default-on and best-effort, matching the Wave widgets pattern.
3. Drift refuses. update-upstream.py check runs before anything is placed; its exit code is now read precisely: 1 (drift) prints the existing refusal and exits 1; any other non-zero (a structural pin refusal) gets its own message and exits 2 (fix for D5).
4. Nothing upstream changes. No commit to openRepoShape/openRepoTools, and this diff does not touch the Dockerfile, upstream-pin.yaml, files/, or files/speckit-worktree/.

Guarantees added by the review round (D1-D6)

• D1 (symlinked target) — the shim's cp wrote through a symlinked park, silently editing another repository. Fixed: pre-flight refuses (exit 1) before either shim runs if any of the four targets is a symlink, naming it.
• D2 (directory-named target) — cp copied into a directory standing in for a target, reporting success while installing nothing. Fixed: pre-flight refuses if a target exists and is not a regular file.
• D3 (unpinned bytes reported as the pin) — removing a pin row the documented way (apply --remove, which deletes the row and the file together) let check pass while the shim silently fetched the missing file from the network at $*_REF/$*_REPO. Fixed two ways: (a) pre-flight now also requires the pin file to exist, every file either shim needs to be present on disk, and every file the shims would install to have a row in update-upstream.py list's own output — refusing (exit 2) and naming the file before any shim could reach for the network; (b) for defense in depth, OPENREPOSHAPE_REPO/_REF and OPENREPOTOOLS_REPO/_REF are overridden to a sentinel (pinned-by-workBenches-no-fetch) that cannot resolve, for the duration of both installer runs, so a fetch that ever became reachable despite (a) fails loudly naming the sentinel instead of silently succeeding against the operator's own exported environment.
• D4 (partial installs, two independent transactions) — a read-only pre-existing park let openRepoShape/openRepoTools install before the second shim failed partway through its own three-file loop, leaving a mismatched trio. Fixed: the same pre-flight checks all four targets (existence, symlink, regular-file, writability, and bin-dir writability) before either shim runs, so a bad target refuses before anything at all is placed.
• D5 (check exit 2 misreported as drift) — see ruling 3 above; the pin file's own existence is also now checked explicitly before check even runs.
• D6 (hygiene) — removed this script's own third "not on $PATH" line (both shims already print their own) and the seen-dedupe it needed; switched both list-output parses from a printf | awk '{exit}' pipeline to a here-string, so there is no live producer process an early awk exit could ever send SIGPIPE under pipefail if the output ever grew.

After both installers exit 0, all four placed files are independently re-verified — regular, non-symlink, mode exactly 0755, cmp-identical to the vendored copy — and only then does the script report Estate commands verified against the vendored pin. (closes D1-D4 residue in one place).

Tests

devcontainer.test/test-setup-estate-commands.sh, 76 assertions, all green:

GREEN: setup-estate-commands regression test passed

Original (a)-(e) unchanged in behavior (scenario (a) now also asserts exactly two PATH warnings — one per shim, not a third from this script). New:

• (g) [D1] park a symlink into another temp dir → exit 1, refusal names it, the link target's bytes and the rest of the bin dir untouched.
• (h) [D2] park a directory → exit 1, refusal, nothing copied into it.
• (i) [D4 guard] pre-seeded read-only park (0444) → exit 1 before openRepoShape/openRepoTools/resume are placed.
• (j) a base-image copy with upstream-pin.yaml deleted → exit 2, "missing", bin dir empty.
• (k) [D3] resume's row and vendored file both removed via apply --remove (confirmed check still passes first) → our pre-flight exits 2 naming resume, bin dir empty, no "fetch"/"github" in the output.
• (l) every file placed in scenario (a) is mode 0755 (stat -c %a).
• (m) the sentinel: run with operator-style *_REF/*_REPO values set → still exit 0, unchanged install, neither operator value reaches the output. Since neither shim echoes $REPO/$REF on the happy path (only on --version or inside an unreachable fetch-failure message), a live grep-for-zero-occurrences can't by itself distinguish "overridden" from "never consulted" — so this scenario also asserts, by reading the script's own source, that all four export …="$ESTATE_SENTINEL" lines are present, per the reviewer's own fallback ("assert instead by a method you can justify, or drop (m) and say so").

devcontainer.test/test.sh is a Layer 0 container harness (registers only 3 of its 10 sibling test-*.sh files, as in-container helpers) — not a general runner list. test-ai-cli-install-summary.sh, host-side like this one, is already unregistered there, so this test follows that precedent and is not added to it; it is wired into CI directly.

Also: bash -n clean on both scripts and setup.sh; python3 devBenches/base-image/update-upstream.py check exits 0 on this branch; git diff --stat origin/main...HEAD still touches exactly the same 5 files. shellcheck is not on this machine's PATH.

CI-driven follow-up (fef6916)

The first push of the fix round (a4bdbe8) failed CI's regression job: scenario (k)'s setup ran update-upstream.py apply --source openrepotools --remove resume --yes, unguarded, inside this set -euo pipefail test. apply calls gh api to prove the target commit is reachable from openRepoTools' default branch; that CI job has no GH_TOKEN, so gh api failed, apply exited 2 on its own Refusal, and the unguarded call took the whole test down with it (exit 2, no PASS/FAIL for the scenario). The reviewer's own instruction for (k) was already "edit the copied pin file by hand to drop the three resume lines, delete the copied file" — switched to exactly that (a small python3 edit + rm), which is both truer to the instruction and removes the network dependency entirely, rather than adding an unguarded network call the rest of this offline suite doesn't have. 76/76 green after, confirmed on CI (fef6916).

Eagle pre-check (unchanged from the original PR; not re-run this round)

openRepoShape and resume identical to vendored; openRepoTools and park differ (a separate, already-merged PR bumped the pin since — see #39/#38). The real installer has still not been run on this branch; that is left for the acceptance pass after review, as before.

🤖 Generated with Claude Code

[sourcery-ai]

Sorry @brettheap, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 3 hours and 30 minutes by commenting @sourcery-ai review. Upgrade to get a review now.

[sourcery-ai]

Reviewer's Guide

The PR adds a pin-validated host installer for openRepoShape, openRepoTools, park, and resume, integrates it into setup.sh as a best-effort step, documents its behavior, and validates the install/refusal lifecycle in CI.

Sequence diagram for pin-validated estate command installation

sequenceDiagram
    participant Setup as setup.sh
    participant Installer as setup-estate-commands.sh
    participant Checker as update-upstream.py
    participant Shape as openRepoShape
    participant Tools as openRepoTools
    participant Host as Host bin directories

    Setup->>Installer: run
    Installer->>Checker: check
    alt vendored copies drift
        Checker-->>Installer: non-zero with finding
        Installer-->>Setup: refuse, install nothing
    else pin check passes
        Installer->>Checker: list
        Checker-->>Installer: pinned commits
        Installer->>Shape: --install
        Shape->>Host: place openRepoShape
        Installer->>Tools: --install
        Tools->>Host: place openRepoTools, park, resume
        Installer-->>Setup: success or installer failure
    end
    Setup->>Setup: continue best-effort

Loading

Flow diagram for setup.sh estate command integration

flowchart TD
    A["setup.sh"] --> B["ESTATE COMMANDS block"]
    B --> C{"WORKBENCHES_SKIP_ESTATE_COMMANDS=1?"}
    C -->|Yes| D["Print skipped; continue setup"]
    C -->|No| E["Run setup-estate-commands.sh"]
    E --> F{"update-upstream.py check passes?"}
    F -->|No| G["Refuse installation; continue setup"]
    F -->|Yes| H["Run vendored openRepoShape --install"]
    H --> I["Run vendored openRepoTools --install"]
    I --> J["Place or update four host commands"]
    J --> K["Continue setup"]

Loading

File-Level Changes

Change	Details	Files
Add a pin-gated installer that delegates placement of all four estate commands to the vendored shims.	Runs the upstream pin check before any installation and refuses on drift. Reads and prints pinned commits from the updater’s list output. Invokes both shims by file path so they install their own command sets and report unchanged, updated, or installed status. Supports configurable base-image and bin directories, skip behavior, PATH warnings, and explicit failure handling.	scripts/setup-estate-commands.sh
Integrate estate-command installation into the host setup flow as an optional best-effort step.	Adds a default-on ESTATE COMMANDS section after shell setup and before interactive selection. Allows failures to be reported while setup continues. Documents pin-based installation, replacement behavior, reruns, and the skip variable.	setup.sh README.md
Add regression coverage for installation, replacement, refusal, and skip scenarios.	Verifies four executable files are installed byte-identically from vendored copies. Covers idempotent reruns, locally modified command replacement, corrupted-vendor refusal, and skip behavior. Runs the regression test in CI immediately after the pin check and updates workflow path filters.	devcontainer.test/test-setup-estate-commands.sh .github/workflows/speckit-git-bash.yml

Possibly linked issues

• setup.sh installs the estate commands on the host from the vendored pin — openRepoShape, openRepoTools, park, resume placed from files/openreposhape and files/openrepotools, re-placed when the pin moves #37: The PR directly implements issue setup.sh installs the estate commands on the host from the vendored pin — openRepoShape, openRepoTools, park, resume placed from files/openreposhape and files/openrepotools, re-placed when the pin moves #37's requested script, setup.sh integration, documentation, pin checks, refresh behavior, and tests.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[Copilot]

🟢 Approval recommended

No approval-blocking issues remain; the final review note is a non-blocking nit.

Pull request overview

Adds pin-validated, offline installation of estate commands during host setup, with drift refusal, tests, CI wiring, and documentation.

Changes:

• Integrates optional estate command installation into setup.sh.
• Adds regression coverage for installation, replacement, drift refusal, and skipping.
• Documents pin-based installation and runs the test in CI.

File summaries

File	Summary
setup.sh	Runs estate command setup best-effort.
scripts/setup-estate-commands.sh	Validates pins and installs commands.
README.md	Documents pin-based installation.
devcontainer.test/test-setup-estate-commands.sh	Tests installation and drift refusal.
.github/workflows/speckit-git-bash.yml	Runs regression coverage.

Review details

Suppressed comments (1)

scripts/setup-estate-commands.sh:83

• update-upstream.py check can fail for an executable-bit mismatch or a structurally invalid pin, not only for changed bytes. In those cases this message incorrectly says the vendored bytes do not match and can send the user toward the wrong remediation; describe the integrity-check failure generically while preserving the detailed finding printed above.

    echo "Estate command install refused: the vendored copies do not match" >&2
    echo "$BASE_IMAGE_DIR/upstream-pin.yaml (see the finding above). NOTHING" >&2
    echo "was installed, because the vendored bytes are not the pinned bytes." >&2

• Files reviewed: 5/5 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Two moderate findings remain regarding installer safety and hermetic CI testing, along with one messaging nit.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

devcontainer.test/test-setup-estate-commands.sh:303

• update-upstream.py apply is not offline: it calls the GitHub API/gh to validate the commit and fetch the tree/files. This new test invokes it, but this workflow does not provision an authenticated gh/token, so the regression suite can fail before reaching the assertions on runners without preconfigured GitHub CLI access. Construct the row/file-removed fixture locally, or explicitly provision the required tool; keeping this test hermetic is preferable.

# suite does not assume (and does not have in every CI job). The end state

scripts/setup-estate-commands.sh:129

• update-upstream.py check returns status 1 for more than byte drift: it also reports mode-only drift and unpinned files. In those cases the added wrapper text says the vendored bytes are not pinned, which is inaccurate and obscures the actual finding/remediation. Use wording that covers any vendored-tree violation instead of asserting a byte mismatch.

    echo "Estate command install refused: the vendored copies do not match" >&2
    echo "$PIN_FILE (see the finding above). NOTHING was installed, because" >&2
    echo "the vendored bytes are not the pinned bytes." >&2

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[brettheap]

LANDED — lane xfactory-2 (xFactory-2), session de233a3a@Eagle, 2026-09-10T22:04Z, PR #40 → 83c3fed into opensoft/workBenches main (merge commit, the repository's convention).

Review record: Opus adversarial review at 02bad7c found six reproduced defects (symlinked target written through; directory target copied into and reported placed; unpinned network fetch reachable when a pin row is dropped; partial sets on read-only or dangling targets; check exit 2 conflated with drift; PATH warning triplicated) — all fixed in a4bdbe8 + fef6916 (pre-flight refusals, pin-row check for all four files, no-fetch sentinel on the shims' REPO/REF variables, post-install verification, exit-2 distinction; 76 assertions). Re-verification at fef6916: MERGE.

Acceptance on Eagle (scripts/setup-estate-commands.sh from main; setup.sh parses): update-upstream.py check 5 rows ok; openRepoShape pinned at 0fa73b7, openRepoTools pinned at ff1f01a (moved by #39 before this landed); four commands already installed (unchanged); cmp silent ×4; park --help and openRepoShape --help run; exit 0.

Known residue, non-blocking, unclaimed: (i) the two installer-failure branches do not say the four commands may be in a mixed state (one line each); (ii) file_mode_octal aborts silently under set -e on a host with no stat; (iii) the exact-0755 post-check would refuse a correct install on a filesystem that cannot hold that mode (DrvFs-style). Raven's operator steps are now: clone workBenches, run ./setup.sh, rebuild the image.

[Copilot]

🔵 Needs a closer look

Preflight must prevent partial installs caused by invalid bin directories or failed permission changes.

Review details

Suppressed comments (2)

scripts/setup-estate-commands.sh:217

• This only checks the directory when -d succeeds, so an existing non-directory OPENREPOTOOLS_BIN_DIR (or a directory with write permission but no search/execute permission) passes preflight. With a valid shape bin dir and this invalid tools bin dir, openRepoShape --install completes first and openRepoTools then fails in mkdir/cp, leaving a partial estate install despite the D4 all-target preflight. Reject existing non-directories and validate both write and search permission (and add a regression case for separate bin dirs) before invoking either shim.

    if [ -d "$dir" ] && [ ! -w "$dir" ]; then
        refuse_preflight "$dir exists and is not writable, so $target could not be placed."
    fi

scripts/setup-estate-commands.sh:214

• -w only proves that the current user can write the existing file, but each vendored shim unconditionally runs chmod 755 after copying (openRepoShape:194-195; openRepoTools:171-172). A group/ACL-writable file owned by another user can therefore pass this preflight, be modified, and then make chmod fail; because the shape shim runs first, the result is a partial install. Preflight must also guarantee the mode change can succeed, or the installer needs an atomic permission-safe path.

    if [ -e "$target" ] && [ ! -w "$target" ]; then
        refuse_preflight "$target exists and is not writable."
    fi

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite