^d D4: complete launcher-contract.md with required-tools/env/exit-codes/test-flags reference#397
Conversation
D4's acceptance criteria require docs/launcher-contract.md to document the launcher's required host tools, environment expectations, exit codes, and test override flags. The doc previously carried only the per-module contract sections; add a "Launcher reference" block covering those four topics. Every claim is grep-derived from scripts/workcell, scripts/lib/launcher/*.sh, and scripts/lib/*.sh (tool resolvers, scrub_host_process_env, the pinned TRUSTED_HOST_PATH, exit-code inventory, and the harness-only override flags with their gating). Doc-only; no code touched. markdownlint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9564abc35
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9564abc35
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Fixed all three (all verified against source): (1) exit codes — the 'only 0/1/2/88' claim ignored child-status passthrough; documented that a normal launch passes the supervised child's status through unchanged (exit ${DOCKER_STATUS} at 8797, ${BUILD_STATUS} at 8698, exit $? at 4182/4188) so an end-to-end run can surface any 0-255, cross-referencing stability-contract.md. (2+3) required-tools — curl and git's run_clean_host_command calls are sanitised-PATH by-name lookups, not absolute-resolved aborts; scoped the fail-closed guarantee to the resolver-backed tools (go/colima/docker + git's HOST_GIT_BIN) and marked curl optional (best-effort preflight, absence swallowed with || true). markdownlint green. |
|
@codex review |
…ct.md (P2/P3) (three accuracy fixes flagged in review: the Exit codes section claimed the launcher only ever exits 0/1/2/88, but a normal run passes the supervised child's status through unchanged - exit ${DOCKER_STATUS}/${BUILD_STATUS}/exit $? - so an end-to-end invocation can surface any 0-255 status per stability-contract.md; and the Required host tools section overstated the fail-closed guarantee - curl and git's run_clean_host_command invocations are sanitised-PATH by-name lookups, not absolute-resolved aborts, and curl is optional since its preflight is best-effort with || true; documented both mechanisms and marked curl optional) (markdownlint green, doc-only; doc/code accuracy)
|
Also fixed a markdownlint table-column error the accuracy edit introduced (the literal |
omkhar
force-pushed
the
d4/launcher-contract-reference
branch
from
1ac0788 to
9fa530b
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fa530b88a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…-contract.md (P2) (two more review accuracy fixes: the required-tools table implied a launcher-wide contract but omitted backend/build prerequisites - aws + session-manager-plugin for aws-ec2-ssm, gcloud for gcp-vm via missing_launch_host_tools_csv, and docker-buildx via ensure_workcell_trusted_buildx - so scoped the table to local core tools and added those; and the trust claim overstated resolve_fixed_host_tool, which only checks -x on the literal candidate with no canonicalisation or is_trusted_host_tool_path validation - unlike resolve_host_tool - so distinguished the weaker fixed-path allowlist from the canonical-target-validated resolver) (markdownlint green, doc-only; doc/code security-accuracy)
|
Fixed both (verified against source): (1) scoped the host-tool table to local core tools and added the backend/build prerequisites it omitted — aws + session-manager-plugin (aws-ec2-ssm), gcloud (gcp-vm) via missing_launch_host_tools_csv:6708, and docker-buildx via ensure_workcell_trusted_buildx. (2) corrected the fixed-resolver trust claim: resolve_fixed_host_tool only checks -x on the literal candidate (no canonicalisation / is_trusted_host_tool_path), a weaker fixed-path allowlist than resolve_host_tool which validates the canonical target (workcell:6660-6661) — documented the distinction rather than claiming a uniform guarantee. markdownlint green. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1021191bd7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…(P3) (the host-tool reference omitted gh, which the workcell publish-pr subcommand resolves as required on its non-dry-run path via the Go publish helper internal/publishpr; an operator satisfying only the local-core table could still fail at publication time, so added it to the out-of-table prerequisites note alongside the backend and buildx tools) (markdownlint green, doc-only; operator prerequisite completeness)
|
Fixed the P3: added gh to the out-of-table prerequisites note — workcell publish-pr resolves it as required on its non-dry-run path (internal/publishpr, ResolveHostTool gh true), so the operator prerequisite list is now complete. markdownlint green. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb6917fc1f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…P2) (the env expectations described a blanket env -i re-exec/clear, but that only holds when the shebang is honored on direct execution; when the launcher is invoked as bash scripts/workcell or /bin/bash -p scripts/workcell - the form the repo's own verify-invariants and publish-pr harnesses use - the shebang is bypassed, env -i never runs, and only the explicit scrub_host_process_env list plus the PATH reset apply; documented this so security reasoning does not assume unlisted variables are absent on the harness/subcommand path) (markdownlint green, doc-only; doc/code security-accuracy)
|
Fixed the P2: qualified the cleared-environment contract. The env -i wholesale clear only applies when the shebang is honored (direct execution); when invoked as bash/ |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 746dd3f24d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 746dd3f24d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…n launcher-contract.md (P2) (two more consequences of the shebang-bypassed bash/-p invocation path: (1) the Go-cache invariant claimed the wrappers can never run with an unsanitised Go cache root, but ensure_go_run_env honours a pre-set GOPATH/GOMODCACHE/GOCACHE which scrub_host_process_env does not unset - reframed as an ordering guarantee that yields a defined, not necessarily default, cache root; (2) the support-matrix scrub is gated only on the SANITIZED_ENTRYPOINT marker while honoring is gated on support_matrix_host_override_allowed - separated the two gates so the vars are documented as possibly present-but-ignored on the bash path rather than guaranteed absent) (markdownlint green, doc-only; doc/code security-accuracy)
|
Fixed both (verified against source): (1) Go-cache — reframed the 'never unsanitised Go cache root' invariant as an ordering guarantee, since ensure_go_run_env honours pre-set GOPATH/GOMODCACHE/GOCACHE (go-run-env.sh:33-35) which aren't scrubbed, so on the bash path a caller cache persists (defined, not necessarily default). (2) support-matrix scrub — separated the two gates: the startup scrub-skip is gated only on the SANITIZED_ENTRYPOINT marker (workcell:12-17), while detector honoring requires support_matrix_host_override_allowed (marker + recognised parent); documented that on the bash path the vars can be present-but-ignored, not guaranteed absent. markdownlint green. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e698d0ca10
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…s in launcher-contract.md (P2) (three accuracy corrections that narrow overstated claims: (1) support_matrix_host_override_allowed's parent check is a command-line substring match, not a verified process identity, so reframed the gates as harness conventions - not a spoof-proof boundary against a local operator - and deferred the threat model to docs/security/; (2) WORKCELL_TEST_CODEX_AUTH_FILE is rejected legacy input no resolver consumes, not a fixture consumed by subprocesses; (3) go has a second, weaker resolution path via go-run-env.sh under --audit-transcript that honours an unscrubbed WORKCELL_GO_BIN, documented alongside the aborting fixed resolver) (markdownlint green, doc-only; doc/code security-accuracy)
|
Fixed all three (verified against source) — each narrows an overstatement rather than adding a claim: (1) reframed the support-matrix gate as a harness convention (the parent check is a cmdline substring match in host-detect.sh, not verified identity, so it deters accidental leakage but isn't spoof-proof against a local operator) and deferred the threat model to docs/security/; (2) corrected WORKCELL_TEST_CODEX_AUTH_FILE to rejected legacy input (no resolver consumes it; workcell:8399 rejects with exit 2), not a subprocess-consumed fixture; (3) documented go's second resolver via go-run-env.sh (WORKCELL_GO_BIN -> command -v go -> fixed candidates) used by the --audit-transcript pty_transcript path, which doesn't abort and honours an unscrubbed WORKCELL_GO_BIN on the bash path. markdownlint green. |
|
Fixed the P3 (a factual error from an earlier round): resolve_go_bin IS fail-closed — it exits 1 with 'Missing required tool: go' (go-run-env.sh:66-68) — corrected the 'does not abort' wording to say fail-closed but not confined to fixed trusted paths (accepts a command -v go hit, honours an unscrubbed WORKCELL_GO_BIN), preserving the real bash-path caveat. markdownlint green. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7a7061035
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7a7061035
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… launcher-contract.md (P2) (two accuracy corrections on the shebang-bypassed path: (1) plain bash scripts/workcell processes BASH_ENV before the script body, so a startup-file injection has already run before scrub_host_process_env unsets it - the scrub is not a defence there - whereas /bin/bash -p ignores BASH_ENV; split the two forms rather than grouping them; (2) WORKCELL_TEST_CLAUDE_KEYCHAIN_EXPORT_FILE was wrongly called not-consumed - the self-staging --synthetic-claude-export path sets it internally (prepare_bundle.go:434) for the Claude credential resolver (resolve_provider_credentials.go:16), while an operator-supplied value is still rejected exit 2; documented the dual role) (markdownlint green, doc-only; corrects two factual errors)
|
Fixed both (verified against source): (1) split the bash-invocation forms — plain |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9feae91cbc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…he docker row claimed absolute resolve_host_tool resolution, but in the main launch path HOST_DOCKER_BIN is set to the bare name docker (workcell:8385) and run on TRUSTED_HOST_PATH via run_workcell_docker_client_command; resolve_host_tool_optional is only a probe/fallback in specific sub-paths; regrouped docker with the by-name-on-TRUSTED_HOST_PATH tools (git/curl) rather than the canonical-target-validated resolvers (colima, git's HOST_GIT_BIN)) (markdownlint green, doc-only; corrects a security-contract overstatement)
|
Fixed (verified against source): docker's main launch path sets HOST_DOCKER_BIN="docker" (bare name, workcell:8385) run on TRUSTED_HOST_PATH via run_workcell_docker_client_command (1895); resolve_host_tool_optional is only a probe/fallback in specific sub-paths (1099/1912). Corrected the row and regrouped docker with the by-name-on-TRUSTED_HOST_PATH tools (git/curl) instead of the canonical-target-validated resolvers. markdownlint green. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ae97a2f89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ae97a2f89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…esolution (P2) (the previous commit corrected docker's row and the resolver summary but left the section lead-in still listing docker among tools resolved to an absolute path and asserting the launcher never trusts a bare PATH lookup; reworded the lead-in so it states every tool is either absolute-resolved (go/colima/git's HOST_GIT_BIN) or run by name on the pinned TRUSTED_HOST_PATH (docker main path, git/curl), matching the rows and notes) (markdownlint green, doc-only; fixes an internal inconsistency)
|
Fixed: my previous docker correction left the section lead-in inconsistent (still listing docker as absolute-resolved and claiming no bare-PATH lookups). Reworded the lead-in to state every tool is either absolute-resolved (go/colima/git's HOST_GIT_BIN) or run by name on the pinned TRUSTED_HOST_PATH (docker main path, git/curl) — matching the rows and closing notes. markdownlint green. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d21a7ca35f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d21a7ca35f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…able by target (P2/P3) (my earlier docker/keychain edits cited a pre-D4 tree, so scripts/workcell anchors were ~249 lines too high; fixed docker (HOST_DOCKER_BIN=docker at 8136, run_workcell_docker_client_command in trusted-docker-client.sh:151, optional resolves at 850/1663/6721), the keychain self-staging probe (--synthetic-claude-export at 4804) and its reject guard (8154-8156); and scoped the required-tools table to note the resolved set varies by target backend - colima only for TARGET_BACKEND==colima, docker/context for a Docker Desktop target, curl only for the preflight) (markdownlint green, doc-only; auditable-contract accuracy)
|
Fixed both. Root cause of the stale anchors: my earlier docker/keychain edits were verified against a pre-D4 checkout, so scripts/workcell line numbers were ~249 too high. Corrected against the branch tree — docker (8136, trusted-docker-client.sh:151, 850/1663/6721), synthetic-claude-export (4804), keychain reject (8154-8156). Also scoped the required-tools table: the resolved set varies by target backend (colima only when TARGET_BACKEND==colima; a Docker Desktop target probes docker/context; curl only for the best-effort preflight). markdownlint green. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89da9e7ac3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… path (P2) (my prior edits wrongly documented docker's main path as leaving the bare name docker and not fail-closed; in fact the transient HOST_DOCKER_BIN=docker default (workcell:8136) is overridden by prepare_current_target_docker_client - called at workcell:8352 - which resolves docker via resolve_host_tool (workcell:866, canonical-target-validated, aborts if missing) before any docker use; regrouped docker with the fail-closed resolver tools consistently across the section lead-in, its row, and the closing, leaving only git's run_clean_host_command uses and curl as by-name-on-TRUSTED_HOST_PATH) (markdownlint green, doc-only; restores the correct security contract)
|
Fixed (verified the full flow this time): docker IS fail-closed in the main launch path — the transient HOST_DOCKER_BIN="docker" default (workcell:8136) is overridden by prepare_current_target_docker_client (called at workcell:8352), which resolve_host_tool-resolves docker (workcell:866, canonical-validated, aborts if missing) before any use. My earlier 'bare name, not fail-closed' framing was backwards; regrouped docker with the fail-closed resolvers consistently across the lead-in, its row, and the closing, leaving only git's run_clean_host_command uses and curl as by-name. markdownlint green. |
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acbb65af73
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Acknowledging the buildx P2 (posted alongside your LGTM as a non-major note): it's the same inherited-override class as the WORKCELL_GO_BIN caveat already documented — WORKCELL_TRUSTED_BUILDX_BIN isn't scrubbed on the |
1 participant
D4 (modularize the launcher) — completes the contract doc. Follows the three module extractions (#394 host-detect, #395 host-exec, #396 go-hostutil). D4's acceptance criteria require
docs/launcher-contract.mdto document required tools, environment expectations, exit codes, and test override flags — this adds a "Launcher reference" block with those four sections.What
A doc-only section (110 lines) after the intro:
resolve_fixed_host_toolfixed-path /resolve_host_tool{,_optional}candidates+trusted-PATH), and each tool (go,colima,docker,git,curl) with its purpose; abort-on-missing contract.TRUSTED_HOST_PATH, the fullscrub_host_process_envunset list, and derived vars (REAL_HOME,WORKCELL_GO_CACHE_ROOT,COLIMA_STATE_ROOT/HOME,GOPATH/GOMODCACHE/GOCACHE) + the go-hostutil ordering invariant.0/1/2/88), grouped by meaning.WORKCELL_TEST_*/WORKCELL_VERIFY_*flag, its effect, and its gating (harness-only; e.g. reject-on-presence guards for the credential-file vars, and the env-vs-internal split on the fault-injection flag).Accuracy
Every claim is grep-derived from
scripts/workcell+ the launcher modules (not assumed); I independently re-verified the exit-code set (0/1/2/88), the eight test/verify flags, the tool resolvers, and the reject-guard line numbers against source before pushing.Validation
markdownlint clean. Doc-only — no code touched.
🤖 Generated with Claude Code