Skip to content

refactor(exec): share the user-command execution contract - #48

Merged
johnny4young merged 4 commits into
mainfrom
codex/refactor-command-execution
Oct 8, 2026
Merged

johnny4young merged 4 commits into
mainfrom
codex/refactor-command-execution

Conversation

@johnny4young

@johnny4young johnny4young commented Oct 6, 2026 •

Copy link
Copy Markdown
Owner

Summary

cmd_run and cmd_each each spelled out the same policy for running a user command: unset the inherited GOROOT, prepend the resolved version's bin to PATH, keep the caller's GOTOOLCHAIN, and exec the original arguments. This PR moves that policy into one normal (non-subshell) function, _gos_exec_version_command, and leaves process ownership visible at the callers.

This is a bounded, behavior-preserving refactor. It does not claim the existing CLI is broken or faster.

What changed

  • gos.sh: new _gos_exec_version_command <version_dir> <command…>, which does unset GOROOT; PATH="<dir>/bin:$PATH" exec "$@".
    • run calls it directly after releasing the mutation lock, so the user command replaces gos and supplies the exit status.
    • each calls it inside its existing per-version subshell, so the parent keeps the original environment, collects each status, continues, and prints the same aggregate summary.
  • docs/ARCHITECTURE.md: a new "User-command execution" section covering the contract, the ownership of the lock, environment and status, and why the helper must not become a subshell.
  • CHANGELOG.md: an Unreleased entry.
  • tests/side-by-side.bash: new regressions (below).

Resolution, installation, locks, child-argument parsing, stdout routing and the Bash 3.2 floor stay at their existing boundaries.

Why

One definition of the execution boundary keeps run and each from drifting apart, for example one of them starting to leak GOROOT or to override GOTOOLCHAIN.

How it was tested

  • The expanded tests/side-by-side.bash checks:
    • PID identity across launcher → gos run → user command, which detects any extra wrapper process;
    • the exact argv, including spaces, an empty argument, --json and a literal *;
    • that GOROOT is removed, an explicit GOTOOLCHAIN is preserved, PATH is exactly what is expected, and the lock is released;
    • that each isolates PATH per version and continues after child status 23;
    • that a real self-SIGTERM yields 143 for run, and that each keeps collecting signaled children.
  • Baseline parity: the expanded tests also pass against unchanged main 05d9e25. Two deliberate negative controls failed as intended (turning the helper into a subshell breaks PID identity; forcing GOTOOLCHAIN=local breaks the caller's policy) and were removed afterwards.
  • Author's local run: scripts/validate-local.bash, with all 31 Linux suites, command-surface sync, workflow YAML/invariants, ShellCheck 0.11.0, shfmt 3.13.1, Zsh syntax, CLI smoke and whitespace checks.
  • Independent review run on head bf2c046: full scripts/run-tests.bash --jobs 4 passed. As root, only install-transaction.bash failed, and it fails identically on main (root cause fixed separately in fix(sudo): keep line endings when replaying captured command output #49). ShellCheck 0.11.0, shfmt 3.13.1 and bash -n were clean.
  • Hosted CI: all 10 jobs passed on exact head bf2c046 in run 37400338952, including native macOS Bash 3.2, Linux Fish and Windows Bash/PowerShell. All 11 commit checks, including GitGuardian, are green.

Review follow-ups

  • The review found nothing blocking, so no follow-up commits were needed.
  • Two optional nits remain:
    • the helper comment could say explicitly that it intentionally leaves GOTOOLCHAIN alone;
    • exec skips EXIT traps (a pre-existing, documented property).

Decisions taken

  • The helper stays a plain function rather than a subshell function, because moving the process boundary into it would change run's process-replacement semantics.
  • The second enumeration in the installed bare-minor resolver is intentionally deferred. Snapshotting it would change observations under concurrent version-directory mutation, and the timing evidence doesn't justify it.

Known limitations & follow-ups

  • tests/side-by-side.bash keeps its existing Windows exclusion and symlink capability probe, so these commands are not Windows execution coverage.

Merge order

Independent of the other open PRs: it is based on main and doesn't touch their files. Merges cleanly with #44, #45, #46, #47 and #49.

@johnny4young
johnny4young marked this pull request as ready for review October 6, 2026 03:50

@johnny4young johnny4young left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: share the user-command execution contract (run / each)

What it does: moves the duplicated unset GOROOT; PATH="<dir>/bin:$PATH" exec "$@" from cmd_run and cmd_each into _gos_exec_version_command (gos.sh ~L2621-2629), documents the contract in docs/ARCHITECTURE.md, and adds regression tests in tests/side-by-side.bash.

Verdict: ready. Nothing blocking or should-fix.

Checked

  • Behavior is unchanged. The helper is a plain function, so in cmd_run the exec still replaces the gos process itself after _gos_release_lock. In cmd_each the ( … ) || rc=$? subshell is still the process boundary, so GOROOT/PATH cannot leak into the next iteration, and set -e cannot abort the loop. Arguments are forwarded as "$@" / "${command[@]}" and never re-split or evaled, so there is no new injection or word-splitting surface. GOTOOLCHAIN is left as the caller set it (the probe-only GOTOOLCHAIN=local only appears as command prefixes in the version probes).
  • Exit status: a failed exec (command not found) still exits 127 from the non-interactive shell exactly as before. A signal-derived status (143) propagates through run and is collected per version by each. Both are now covered.
  • The tests are well designed. The PID-capturing launcher proves there is no wrapper process. The child checks the exact argv ('two words' '' --json '*'), confirms GOROOT is unset, GOTOOLCHAIN is preserved, PATH is prefixed exactly once, and the lock is released. each continuing after failed and signaled children is also asserted.
  • Local: full scripts/run-tests.bash --jobs 4 passed except install-transaction.bash, which also fails on main in this container because it runs as root (environment-only, unrelated). side-by-side.bash passed. ShellCheck 0.11.0, shfmt 3.13.1 and bash -n were clean. CI is green on all 11 checks.

Nits (optional)

  • 🟡 gos.sh helper comment: the line "Probe-only GOTOOLCHAIN=local must never leak into the user's command" describes an invariant the helper preserves by not touching GOTOOLCHAIN. Saying that explicitly ("intentionally does not set or unset GOTOOLCHAIN") would stop a future edit from "fixing" it there.
  • 🟡 Pre-existing, not introduced here: exec skips the EXIT trap, so any temp state registered for cleanup before cmd_run reaches the helper is never cleaned up. That seems fine today, since the lock is released explicitly, but it is worth keeping in mind as a documented property of the helper.

Generated by Claude Code

claude added 3 commits October 8, 2026 05:11
- drop the helper's named local, which bash exported over a caller variable
- assert a caller-exported version_dir reaches the child unchanged
- exercise run's install-then-exec path so the lock check is meaningful
- document why the helper must stay free of named locals

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ExmnfUngrDYjAheGvYwMnn
@johnny4young
johnny4young merged commit 02279fc into main Oct 8, 2026
11 checks passed
@johnny4young
johnny4young deleted the codex/refactor-command-execution branch October 8, 2026 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants