feat(#7229)!: move to OpenShell 0.1.2

[fullsend-ai-coder]

Summary

Moves fullsend to OpenShell v0.1.2 (0.1.2 is a bug-fix release on top of 0.1.1; it supersedes Renovate's #7763). OpenShell 0.1 is a clean break from 0.0.x: gateway config schema v2, provider credentials validated against the profile, and fast SIGTERM sandbox stop / async delete. No 0.0.x compatibility is kept, because fullsend pins the version in CI and local installs need a clean reinstall anyway.

This PR began as the ADR 0030 annotation for #7229. That annotation is kept and updated for 0.1.x.

Breaking changes

• The pinned OpenShell is 0.1.2, and the action.yml gateway config is schema v2. A gateway older than 0.1 rejects that config, and 0.1 rejects a v1 config. Nothing that runs the action needs to act: it installs the pinned OpenShell on a fresh runner.
• Self-hosted runners and local installs have to reinstall. A persistent host (a GitLab runner VM or a developer machine) must reinstall OpenShell 0.1.2 and discard its 0.0.x gateway state. The installer needs OPENSHELL_ACK_BREAKING_UPGRADE=1. running-agents-locally.md walks through it. hack/gitlab-runner-vm/setup.sh does it automatically, but an existing VM needs setup.sh re-run (or the VM recreated): its installed executor still trusts its own 0.0.116 pin and refuses a job image that reports 0.1.1.
• Provider credentials must be declared in their profile. 0.1.x refuses a credential the profile does not declare, and an empty value for one it does. Custom providers with _NOOP_*/placeholder credentials drop them, and profiles declare real tokens under credentials:.
• When a repo picks this up depends on its workflow ref. A repo installed by a released CLI has its workflows pinned to a commit SHA (with the version as a comment), and its agents content follows the same version, so it moves to OpenShell 0.1.2 only when it upgrades past this release. A repo on @main moves as soon as this merges; one on the moving @v0 tag moves at the next release. At that point, a repo whose own config has provider files with a placeholder credential (such as the old _PLACEHOLDER_GATE_QUERY example) or a credential its profile does not declare fails at provider creation with credentials are not declared by profile '<id>': <KEY>. The fix is to remove the placeholder, or declare the credential under the profile's credentials:. Repos that use only the default provider files need no change.

Related issues

Closes #7751 (the OpenShell 0.1 migration). Closes #7229.
Related: #4808 (this PR re-syncs the doc's version once; it doesn't add the Renovate tracking #4808 asks for), #6708 (version/SHA consistency check), #6849 (version-gated flags), #4076 (Renovate release age), #6716 (placeholder-in-body reset).
Upstream: NVIDIA/OpenShell#1978 (credential-less providers, closed), #2855 / #3036 (stop SIGTERM), #3159 (per-exec kill, not planned).
Companion: fullsend-ai/agents#1512, which declares provider credentials in the agents profiles and drops _NOOP_*. The two merge back to back.

Changes

• .github/scripts/openshell-version.sh: 0.1.2 (6648bd0c2). install-openshell.sh prints the gateway journal when an install attempt fails.
• functional-tests.yml: OpenShell is installed after the Podman API socket is up, because the pinned podman driver keeps the gateway from starting without it.
• action.yml, functional-tests.yml: schema v2 gateway config. compute_driver = "podman" goes under [openshell.gateway]; supervisor_image and health_check_interval_secs = 10 go under [openshell.drivers.podman], matching upstream's packaged default.
• Scaffold providers/*.yaml: the _NOOP_* placeholder credentials are removed. 0.1.x creates a provider with no credentials and rejects any credential the profile does not declare.
• run_openai.go: the run-scoped OpenAI provider is created with its value, and the expiry is attached in the next call. 0.1.x refuses an empty value for a declared credential and takes an expiry only on update. If both the store and the delete fail, the credential is now expired in place rather than blanked, because blanking is refused on 0.1.x.
• stray_processes.go: the process-view comment is updated for 0.1.x. PID 1 is now openshell-sandbox, running as the sandbox user (its arguments differ between 0.1.1 and 0.1.2; the sweep never matches on them); it is spared as an ancestor.
• Docs:
  • running-agents-locally: 0.1.2, the v2 config, and a clean upgrade from 0.0.x as runnable steps (XDG paths; with Homebrew only the service commands differ).
  • bring-your-own-agent, gate-binaries: providers without a secret carry no credentials, and a token a provider does carry must be declared by its profile.
  • ADR 0030: stop semantics on 0.1.x.
• hack/gitlab-runner-vm/: pin_supervisor_image writes supervisor_image under [openshell.drivers.podman] in a v2 file. A v1 or version-less file is moved aside to gateway.toml.pre-0.1. Both install.sh paths set OPENSHELL_ACK_BREAKING_UPGRADE=1, and setup.sh and the per-job ensure_job_openshell_gateway stop the gateway, wipe the pre-0.1 store and TLS, and write the v2 config before installing. Unit-tested with stubs only (all *_test.sh pass on Fedora 43 and 44, six mutations caught); not run on a live VM.

Testing

The items below through go test ran locally on OpenShell v0.1.1 (Homebrew, macOS arm64, podman driver), with fullsend built from this branch and the agents profiles from agents#1512 applied:

• triage on claude (claude-opus-4-6): providers ready, gh through the proxy, Validation: passed, sandbox deleted in 0.5 s (was ~47 s).
• triage on codex 0.157.0: run-scoped OpenAI provider created, transcript kept, Validation: passed. On the kept sandbox, sandbox stop took 0.15–0.28 s and start-to-exec 0.38–0.52 s; codex state survived.
• triage on pi with openai/gpt-5.6-luna: OpenAI, Vertex and GitHub providers all ready, Validation: passed.
• Stray-process sweep: ran the real script against a planted nohup orphan. The orphan was killed; PID 1 and the keep-alive were spared.
• go test ./internal/cli/ ./internal/runtime/ ./internal/scaffold/ ./internal/harness/, and scaffold profiles pass openshell provider profile lint on 0.1.1.
• OpenShell 0.1.2 (311c1ef), macOS arm64 and Fedora 44 x86_64: after upgrading each from 0.1.1, triage on claude, and on codex and pi with openai/gpt-5.6-luna, all Validation: passed. On Fedora the stray-process sweep killed a planted orphan and spared PID 1 and the keep-alive. On macOS, a Homebrew 0.0.116 install holding providers from a real run was upgraded to 0.1.2 with the doc's steps, and the same three runs passed with the providers recreated.
• Fedora 44 x86_64 (RPM packages), on 3f3f5ea (0.1.1): triage on claude, and on codex and pi with openai/gpt-5.6-luna, all Validation: passed. The running-agents-locally upgrade block was run as written, starting from a real 0.0.116 install with its v1 config and state; the 0.1.1 gateway started on the first try.
• CI before merge cannot validate this change.
  • functional-tests runs on pull_request_target, so it executes main's workflow: the run on 1c03686 wrote main's v1 config next to this PR's 0.1.1 pin, and the gateway refused it (category=legacy_schema_v1).
  • behaviour/e2e dispatch runs that use agents main, whose profiles lack the credential declarations from agents#1513.
  • The first run on main after both merge is the Linux check. install-openshell.sh now prints the gateway journal on failure, which is how the legacy_schema_v1 cause was found.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:21 PM UTC · Completed 12:40 PM UTC

Commit: ccf72d3 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $4.43

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Risk Assessment: elevated (3/5)

Details

Score remains anchored at 3 (elevated), unchanged following the incremental 0.1.2 patch bump: while linked issue #7229 aligns with PR scope, the change spans 26 files touching 3 protected CI workflow paths, displays elevated regression history and change coupling on runner VM components, and introduces a breaking OpenShell schema migration with no opt-out rollback mechanism.

Previous run

Risk Assessment: elevated (3/5)

Details

Score remains anchored at 3 (elevated), unchanged from prior review rounds: while linked issue #7229 aligns with PR scope, the change touches 26 files across 3 protected CI workflow paths, exhibits elevated regression history and change coupling on runner VM components, and executes a breaking OpenShell 0.1.1 schema migration with no opt-out rollback mechanism.

Previous run (2)

Risk Assessment: elevated (3/5)

Details

Score remains anchored at 3 (elevated), unchanged from the prior five review rounds: while linked issue #7751 aligns with PR scope, the change touches 26 files across 3 protected CI workflow paths, exhibits elevated regression history and change coupling on runner VM components, and executes a breaking OpenShell 0.1.1 schema migration with no opt-out rollback mechanism (irreversible schema/credential-validation change).

Previous run (3)

Risk Assessment: elevated (3/5)

Details

Score remains anchored at 3 (elevated), unchanged from the prior four review rounds: while linked issue #7751 aligns with PR scope, the change touches 26 files across 3 protected CI workflow paths, exhibits elevated regression history and change coupling on runner VM components, and executes a breaking OpenShell 0.1.1 schema migration without an opt-out rollback mechanism. The single new commit since the prior assessment (completing the TLS-directory wipe alongside the gateway-directory wipe) is a small in-scope fix that does not change Tier 1 signals.

Previous run (4)

Risk Assessment: elevated (3/5)

Details

Score remains anchored at 3 (elevated): while linked issue #7751 aligns with PR scope, the change touches 26 files across 3 protected CI workflow paths, exhibits elevated regression history on runner VM components, and executes a breaking OpenShell 0.1.1 schema migration without an opt-out rollback mechanism.

Previous run (5)

Risk Assessment: elevated (3/5)

Details

Score remains anchored at 3 (elevated): while linked issue #7751 aligns closely with PR scope, the change touches 26 files across 3 protected CI workflow paths, displays elevated regression history and change coupling on runner VM components, and executes a breaking OpenShell 0.1.1 schema migration with no opt-out rollback mechanism.

Previous run (6)

Risk Assessment: elevated (3/5)

Details

Score remains anchored at 3 (elevated): although linked authorization issue #7751 resolves the prior scope-vs-issue mismatch, the PR spans 26 files across 3 protected CI paths, shows high git coupling and regression history on touched files, and carries a breaking OpenShell 0.1.1 schema/credential migration with no opt-out rollback mechanism.

Previous run (7)

Risk Assessment: elevated (3/5)

Details

Score increased from 1 (low, prior review of a docs-only version) to 3 (elevated) because the PR expanded to 25 files spanning CI workflows, two protected paths, and a breaking OpenShell config/credential-handling migration, with a scope-vs-issue mismatch and no rollback flag.

Previous run (8)

Risk Assessment: low (1/5)

Details

Re-review anchoring applied: Tier 1 signals unchanged from the prior assessment (same single docs file, same bot author, no protected/security/dependency/CI changes); Tier 2 shows low churn and no regression/revert history; Tier 3 confirms the linked low-priority issue scope matches the PR closely. Score preserved at 1 (low).

Previous run (9)

Risk Assessment: low (1/5)

Details

Docs-only, additive, single-file PR by a bot author with no protected paths, security-sensitive files, dependency changes, or CI workflow changes, low churn on the file, and a linked low-priority issue whose scope exactly matches the PR.

[fullsend-ai-review]

Review

Findings

Medium

• [protected-path] .github/scripts/install-openshell.sh, .github/scripts/openshell-version.sh, .github/workflows/functional-tests.yml — This PR modifies files under the protected path .github/. The PR links issues (Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229, Move to OpenShell 0.1.1 #7751) and explains its rationale for these changes in detail, so sufficient context exists — but human approval is always required for protected-path changes regardless of context.

Low

• [fail-open] internal/cli/run_openai.go:733 — ensureOpenAIProvider still cannot attach the credential value and its expiry in a single create call. OpenShell 0.1.x requires a declared credential's value on create and accepts --credential-expires-at only on update, so the live token is stored first (EnsureProviderLiteral, line 733) and expiry is applied only in the following call (UpdateProviderLiteralWithExpiry, line 737). The update-failure cleanup path (DeleteProvider, then SetProviderCredentialExpiry with context.Background()) is present and covered by tests. A process death or SIGKILL strictly between create succeeding and the update returning still leaves a live, non-expiring run-scoped credential, because the caller only registers the deferred delete after this function succeeds. This is an upstream OpenShell 0.1.x create/update split, not a regression from this PR; the 0.1.1 → 0.1.2 pin bump in this revision does not change the two-step store. Carried forward from prior review rounds at the same severity — this location is functionally unchanged since the prior review.
  Remediation: If a future OpenShell release accepts --credential-expires-at on provider create, pass value and expiry together. Otherwise, consider arming a best-effort deferred delete immediately after EnsureProviderLiteral succeeds, before the expiry-update call.

• [backward-compatibility] internal/cli/run.go:1505 — The generic EnsureProvider path (unchanged by this PR) still forwards a declared-but-empty credential value for any provider definition that carries one (buildProviderArgs emits --credential KEY= for an empty expanded value), and OpenShell 0.1.x refuses that form for a declared credential. This PR removes the _NOOP_* placeholder pattern from the scaffold's layered providers/ templates in the same commit as the version pin bump (now 0.1.2). providers/ is a "layered" directory, so standard CI consumers going through the reusable-workflow workspace-prep step re-copy providers/ from this repo's scaffold at job.workflow_sha and pick up the credential-less templates atomically with the version bump — narrower exposure than a first pass suggested. Residual risk: custom-named provider files that workspace prep does not overwrite, and locally-checked-out provider yaml files (not the bare embedded-scaffold case, which already picks up the updated templates) that still carry a copy of the old placeholder pattern.
  Remediation: In the OpenShell 0.0.x → 0.1 upgrade notes (docs/guides/user/running-agents-locally.md), mention that any leftover empty credentials: value (the old _NOOP_*/_PLACEHOLDER_* pattern) must be removed from custom or locally-checked-out provider yaml files that are not replaced by workspace prep.

• [breaking-change-coordination] action.yml:389 — action.yml unconditionally pins OpenShell (now 0.1.2) and writes schema-v2 gateway config. Consumer repos whose thin-caller workflows resolve fullsend's reusable workflows at the default upstream ref (main) pick up this pin on their very next run, independent of whether the companion fullsend-ai/agents#1512/#1513 PRs have merged. 0.1.2 is presented as a bug-fix release on top of 0.1.1 with no additional schema changes, so bumping the pin target from 0.1.1 to 0.1.2 in this revision does not change the coordination window — it is the same, already-disclosed 0.0.x → 0.1 schema/credential-declaration risk. The PR's own "Testing" section states CI cannot validate the cross-repo window before both PRs merge, and that the two are intended to merge back-to-back. Not a newly discovered blocking gap — noted for completeness, consistent with prior review rounds' conclusion on this same companion-PR relationship.

• [missing-breaking-change-marker] N/A (commit messages) — Per COMMITS.md, a breaking change must carry the ! suffix and a BREAKING CHANGE: trailer in both the PR title and its constituent commit messages. The PR title correctly carries ! (feat(#7229)!: move to OpenShell 0.1.2), which mitigates the primary release-notes risk since GoReleaser builds release notes from the merged PR's title/body via the GitHub API (changelog.use: github), not raw commit text. However, none of this PR's 14 individual commits carry a ! suffix or a BREAKING CHANGE: trailer (verified directly against gh api repos/fullsend-ai/fullsend/pulls/7233/commits). This is the same residual raw-git-history compliance gap noted in earlier review rounds on this PR; it was not re-raised in the several most recent rounds and remains unaddressed.
  Remediation: Optional — amend the commit messages that introduce the breaking behavior change so they individually carry ! and a BREAKING CHANGE: trailer, for consistency with COMMITS.md and to keep raw git history self-describing independent of the PR title.

Correctness, style-conventions, intent-coherence, and docs-currency sub-agent passes returned no findings against the current diff. In particular, the previously-fixed install-openshell.sh TLS-directory wipe, the double-failure credential-expiry-in-place fix (context.Background()), the gate-binaries.md credential-collision doc fix, and the running-agents-locally.md supervisor-image sync comment are not observed to have regressed in the current diff/tree. This is a static-analysis conclusion, not a runtime or CI verification. The incremental change since the prior review round (head SHA 3f3f5ea738c282822f1c86db354d89b9cecd7f97) is a version pin bump from 0.1.1 to 0.1.2 (an upstream bug-fix release per the PR body, with no schema changes), a documentation rewording correcting the Homebrew-vs-XDG gateway-state-paths guidance (verified internally consistent), and a code-comment update in stray_processes.go noting PID 1's arguments differ between 0.1.1 and 0.1.2. The PR's own "Testing" section states CI cannot fully validate this change before the companion PR merges.

Previous run

Review

Findings

Medium

• [protected-path] .github/scripts/install-openshell.sh, .github/scripts/openshell-version.sh, .github/workflows/functional-tests.yml — This PR modifies files under the protected path .github/. The PR links issues (Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229, Move to OpenShell 0.1.1 #7751) and explains its rationale for these changes in detail, so sufficient context exists — but human approval is always required for protected-path changes regardless of context.

Low

• [backward-compatibility] internal/cli/run.go:1505 — internal/cli/run.go's generic EnsureProvider path (unchanged by this PR) still sends a declared-but-empty credential value for any provider definition that carries one, and OpenShell 0.1.1 refuses an empty value for a declared credential. This PR removes the _NOOP_* placeholder pattern from the scaffold's layered providers/ templates in the same commit as the OpenShell 0.1.1 pin. providers/ is a "layered" directory (internal/scaffold/scaffold.go), so standard CI consumers going through the reusable-workflow workspace-prep step re-copy providers/ from this repo's scaffold at job.workflow_sha and pick up the credential-less templates atomically with the version bump — this is narrower exposure than a first pass suggested. Residual risk: custom-named provider files that workspace prep does not overwrite, and local fullsend run checkouts that skip workspace prep and still carry a copy of the old placeholder pattern (e.g. copied from the BYOA or gate-binaries guides, both updated in this PR, before this PR shipped the fix).
  Remediation: In the OpenShell 0.0.x → 0.1 upgrade notes (docs/guides/user/running-agents-locally.md), mention that any leftover empty credentials: value (the old _NOOP_*/_PLACEHOLDER_* pattern) must be removed from custom or locally-checked-out provider yaml files that are not replaced by workspace prep. No per-repo migration tooling or CI doctor check is needed for the default, workspace-prep-driven path.

• [breaking-change-coordination] action.yml:389 — action.yml unconditionally pins OpenShell to 0.1.1 and writes schema-v2 gateway config. Consumer repos whose thin-caller workflows resolve fullsend's reusable workflows at the default upstream ref (main) pick up this pin on their very next run, independent of whether the companion fullsend-ai/agents#1512/#1513 PRs have merged. This is a known, already-disclosed risk: the PR's own "Testing" section states CI cannot validate the cross-repo window before both PRs merge, and that the two are intended to merge back-to-back. Not a newly discovered blocking gap — noted for completeness, consistent with prior review rounds' conclusion on this same companion-PR relationship.

• [fail-open] internal/cli/run_openai.go:733 — ensureOpenAIProvider still cannot attach the credential value and its expiry in a single create call: OpenShell 0.1.1 requires a declared credential's value on create and accepts --credential-expires-at only on update, so the live token is stored first (EnsureProviderLiteral, line 733) and its expiry is applied only in the following call (UpdateProviderLiteralWithExpiry, line 737). The update-failure cleanup path (delete-then-expire, using context.Background()) is present and covered by tests. A process death or SIGKILL strictly between the create call succeeding and the update call returning still leaves a live, non-expiring run-scoped credential. This is an upstream OpenShell 0.1.x API constraint (the create/update split), not a regression introduced by this PR, and the window is narrow. Carried forward from prior review rounds at the same severity — this location is functionally unchanged since the prior review (the double-failure blanking fallback was replaced with an in-place expiry via SetProviderCredentialExpiry, which does not affect this specific finding).
  Remediation: If a future OpenShell release accepts --credential-expires-at on provider create, pass value and expiry together. Otherwise, consider arming a best-effort deferred delete immediately after EnsureProviderLiteral succeeds, before the expiry-update call.

Correctness, intent-coherence, style-conventions, and docs-currency sub-agent passes returned no findings against the current diff. In particular, the previously-fixed install-openshell.sh TLS-directory wipe, the setup.sh install-before-migrate ordering fix (install_openshell() now calls prepare_openshell_upgrade() before invoking the installer), and the gate-binaries.md stale "Credential collision limitation" passage are not observed to have regressed in the current diff/tree. This is a static-analysis conclusion, not a runtime or CI verification — the PR's own "Testing" section states CI cannot fully validate this change before the companion PR merges.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (2)

Review

Findings

Medium

• [protected-path] .github/scripts/install-openshell.sh, .github/scripts/openshell-version.sh, .github/workflows/functional-tests.yml — This PR modifies files under the protected path .github/. The PR links issues (Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229, Move to OpenShell 0.1.1 #7751) and explains its rationale for these changes in detail, so sufficient context exists — but human approval is always required for protected-path changes regardless of context.

Low

• [fail-open] internal/cli/run_openai.go:733 — ensureOpenAIProvider still cannot attach the credential value and its expiry in a single create call: OpenShell 0.1.1 requires a declared credential's value on create and accepts --credential-expires-at only on update, so the live token is stored first (EnsureProviderLiteral, line 733) and its expiry is applied only in the following call (UpdateProviderLiteralWithExpiry, line 737). The update-failure cleanup path (delete-then-expire) is present and covered by tests. A process death or SIGKILL strictly between the create call succeeding and the update call returning still leaves a live, non-expiring run-scoped credential. This is an upstream OpenShell 0.1.x API constraint (the create/update split), not a regression introduced by this PR, and the window is narrow. Carried forward from prior review rounds at the same severity — the code at this location is unchanged since the prior review.
  Remediation: If a future OpenShell release accepts --credential-expires-at on provider create, pass value and expiry together. Otherwise, consider arming a best-effort deferred delete immediately after EnsureProviderLiteral succeeds, before the expiry-update call.

The install-before-migrate ordering fix (hack/gitlab-runner-vm/setup.sh) and the TLS-directory wipe fix (.github/scripts/install-openshell.sh's for d in gateway tls loop) from earlier review rounds are both still present in the current diff/tree. A cross-repo-contracts review of action.yml's schema-v2 gateway config change and the companion fullsend-ai/agents#1512/#1513 merge-order dependency did not surface a new blocking issue beyond what the PR's own "Testing" section already discloses (CI cannot validate the cross-repo credential-declaration window before both PRs merge). No correctness, style, or docs-currency findings were raised against the current diff.

Previous run (3)

Review

Findings

Medium

• [protected-path] .github/scripts/install-openshell.sh, .github/scripts/openshell-version.sh, .github/workflows/functional-tests.yml — This PR modifies files under the protected path .github/. The PR links issues (Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229, Move to OpenShell 0.1.1 #7751) and explains its rationale for these changes in detail, so sufficient context exists — but human approval is always required for protected-path changes regardless of context.

Low

• [fail-open] internal/cli/run_openai.go:733 — ensureOpenAIProvider still cannot attach the credential value and its expiry in a single create call: OpenShell 0.1.1 requires a declared credential's value on create and accepts --credential-expires-at only on update, so the live token is stored first (EnsureProviderLiteral, line 733) and its expiry is applied only in the following call (UpdateProviderLiteralWithExpiry, line 737). The update-failure cleanup path (delete-then-expire) is present and correctly covered, but a process death or SIGKILL strictly between the create call succeeding and the update call returning still leaves a live, non-expiring run-scoped credential. This is an upstream OpenShell 0.1.x API constraint (the create/update split), not a regression introduced by this PR, and the window is narrow. Carried forward from prior review rounds at the same severity — the code at this location is unchanged since the prior review.
  Remediation: If a future OpenShell release accepts --credential-expires-at on provider create, pass value and expiry together. Otherwise, consider arming a best-effort deferred delete immediately after EnsureProviderLiteral succeeds, before the expiry-update call.

The prior medium finding about install-openshell.sh's 0.0.x upgrade branch not wiping the OpenShell TLS state directory (only the gateway directory) is not observed in the current diff: the new commit moves both gateway and tls aside via a for d in gateway tls loop, and the corresponding "Upgrading from OpenShell 0.0.x" section in docs/guides/user/running-agents-locally.md now mentions the TLS directory too.

Previous run (4)

Review

Findings

Medium

• [logic-error] .github/scripts/install-openshell.sh:24 — The 0.0.x -> 0.1.x in-place upgrade branch in install-openshell.sh (used directly by action.yml and functional-tests.yml, the public composite action's install path for any self-hosted/persistent GitHub Actions runner) only moves aside the gateway state directory (${XDG_STATE_HOME:-$HOME/.local/state}/openshell/gateway) before installing 0.1.1. It does not also move or remove the TLS directory (.../openshell/tls). This PR's own code comment in hack/gitlab-runner-vm/executor/gateway.sh states: "OpenShell's systemd user unit sets StateDirectory=openshell/gateway... TLS material lives in ~/.local/state/openshell/tls. Wiping both and restarting yields an empty profile registry and new mTLS certs — a process restart alone does not." This PR's own wipe_openshell_gateway_store() (gateway.sh) removes both ${root}/gateway and ${root}/tls together, and the new test fixtures in gateway_test.sh/setup_test.sh assert both are gone after an upgrade. The GitLab-runner-VM upgrade path (prepare_openshell_upgrade, called from setup.sh before install) correctly wipes both. The sibling fix landed in this same PR for the public composite-action path (install-openshell.sh) only wipes the gateway directory, leaving stale 0.0.x TLS material in place. The corresponding new "Upgrading from OpenShell 0.0.x" section added to docs/guides/user/running-agents-locally.md has the identical omission.
  Remediation: In install-openshell.sh's 0.0.* case branch, also move (or remove) ${XDG_STATE_HOME:-$HOME/.local/state}/openshell/tls alongside the gateway directory, matching wipe_openshell_gateway_store()'s behavior. Update the "Upgrading from OpenShell 0.0.x" section in docs/guides/user/running-agents-locally.md to mention the tls directory as well.

• [protected-path] .github/scripts/install-openshell.sh, .github/scripts/openshell-version.sh, .github/workflows/functional-tests.yml — This PR modifies files under the protected path .github/. The PR links issues (Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229, Move to OpenShell 0.1.1 #7751) and explains its rationale for these changes in detail, so sufficient context exists — but human approval is always required for protected-path changes regardless of context.

Low

• [fail-open] internal/cli/run_openai.go:734 — ensureOpenAIProvider still cannot attach the credential value and its expiry in a single create call: OpenShell 0.1.1 requires a declared credential's value on create and accepts --credential-expires-at only on update, so the live token is stored first (EnsureProviderLiteral, line 733) and its expiry is applied only in the following call (UpdateProviderLiteralWithExpiry, line 737). The caller (internal/cli/run.go) registers the deferred cleanup only after ensureOpenAIProvider returns successfully, and there is no defer inside the function covering this window. A process death or SIGKILL strictly between the create call succeeding and the update call returning leaves a live, non-expiring run-scoped credential. This round's independent re-verification confirms the failure-handling paths that run when the update call itself fails (delete-then-expire, both using context.Background()) are correctly covered and tested — the residual gap is specifically the case where the process is killed outright and no error-handling code runs at all. This is an upstream OpenShell 0.1.x API constraint (the create/update split), not a regression introduced by this PR, and the window is narrow. Carried forward from the prior two review rounds at the same severity; the test file changes since the last round add coverage for the create-failure path, not this window.
  Remediation: If a future OpenShell release accepts --credential-expires-at on provider create, pass value and expiry together. Otherwise, consider arming a best-effort deferred delete immediately after EnsureProviderLiteral succeeds, before the expiry-update call.

• [missing-breaking-change-marker] N/A (commit messages) — Per COMMITS.md, a breaking change must carry the ! suffix and a BREAKING CHANGE: trailer in both the PR title and its constituent commit messages. The PR title correctly carries ! (feat(#7229)!: move to OpenShell 0.1.1) and the PR body has a substantive "Breaking changes" section, so the primary release-notes risk is mitigated. However, none of this PR's 10 individual commits carry a ! suffix or a BREAKING CHANGE: trailer. This is the same residual raw-git-history compliance gap noted two review rounds ago; it was not re-raised in the immediately-prior review round and remains unaddressed.

A challenger pass adversarially re-examined this finding set; it argued for removing the fail-open finding above as an unclosable, non-regression upstream residual. That argument was not accepted here — an independent read of internal/cli/run.go's call site confirms the deferred cleanup is registered strictly after ensureOpenAIProvider returns, so the described window is real, even though it is narrow and pre-existing. The finding is kept at low severity, unchanged from the prior two rounds.

Other findings from earlier review rounds (the setup.sh install-before-migrate ordering bug, the missing-ACK gap, and the stale-doc findings in gate-binaries.md/running-agents-locally.md) are not observed in the current diff.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (5)

Review

Findings

Medium

• [protected-path] .github/scripts/install-openshell.sh, .github/scripts/openshell-version.sh, .github/workflows/functional-tests.yml — This PR modifies files under the protected path .github/. The PR links issues (Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229, Move to OpenShell 0.1.1 #7751, docs: running-agents-locally.md OpenShell version not updated by Renovate #4808) and explains its rationale for these changes in detail, so sufficient context exists — but human approval is always required for protected-path changes regardless of context.

Low

• [fail-open] internal/cli/run_openai.go:734 — ensureOpenAIProvider still cannot attach the credential value and its expiry in a single create call: OpenShell 0.1.1 requires a declared credential's value on create and accepts --credential-expires-at only on update, so the live token is stored first (EnsureProviderLiteral, line 734) and its expiry is applied only in the following call (UpdateProviderLiteralWithExpiry, line 737). A process death, panic, or context cancellation strictly between the two calls leaves a live, non-expiring run-scoped credential, since the caller only registers its deferred delete after this function returns success. This is an upstream API constraint (0.1.x's create/update split), not a regression introduced by this PR, and the residual window is narrow. Not observed to be addressed in this diff — carried forward from the prior review at the same severity. The sibling double-failure expire path (line 746) was fixed in this round: it now correctly uses context.Background().
  Remediation: If a future OpenShell release accepts --credential-expires-at on provider create, pass value and expiry together. Otherwise, consider arming a best-effort deferred delete immediately after EnsureProviderLiteral succeeds, before the expiry-update call.

• [sub-agent-failure] N/A — The challenger adjudication pass returned an empty adjudicated_findings array despite receiving one finding to challenge (it argued the fail-open finding above does not hold because the create/update sequence's error path already deletes-or-expires the credential using context.Background()). Per orchestrator policy, an empty adjudicated set from a non-empty input is treated as a challenger failure rather than a legitimate full removal, so the pre-challenger finding set (the fail-open finding above) was kept as-is, unadjudicated. A human reviewer may want to independently weigh the challenger's specific argument when deciding whether the fail-open finding above warrants action.

Other prior findings from the previous review round (the setup.sh install-before-migrate ordering bug, the install-openshell.sh missing-ACK gap, and both stale-doc findings in gate-binaries.md and running-agents-locally.md) were not observed in the current diff — the correctness, security, and docs-currency sub-agents traced the relevant code/doc paths in the new commits and found the described issues addressed. This is a static-analysis conclusion, not a runtime or CI verification.

Previous run (6)

Review

Findings

High

• [logic-error] hack/gitlab-runner-vm/setup.sh:999 — The in-place 0.0.x -> 0.1.1 upgrade path installs before migrating gateway.toml. setup.sh's main() calls install_openshell() (line 999) before configure_gateway() (line 1000). install_openshell() runs install-openshell.sh (which starts the upstream gateway service as part of the RPM install) and only afterward calls teardown_openshell_gateway, which wipes the gateway store/TLS but does not rewrite gateway.toml. pin_supervisor_image — the function that moves a v1 config aside and writes schema v2 — only runs inside configure_gateway(), after install_openshell() returns. The same install-before-migrate order exists in ensure_job_openshell_gateway (gateway.sh:438-439), which setup.sh's own header names as the per-job upgrade path. Upstream OpenShell's install.sh is not in this repo, but this PR's own artifacts treat gateway-start-during-install as a blocking step: install-openshell.sh dumps the gateway journal on failure because "the installer starts the gateway service"; functional-tests.yml was reordered to write v2 config (and wait for the Podman socket) before installing because "the pinned podman driver keeps the gateway from starting without it"; action.yml already writes v2 config before install; and the PR's own testing notes record a real CI failure caused by exactly this class of mismatch (category=legacy_schema_v1) when 0.1.1 was installed next to a v1 config. On a persistent GitLab runner VM still on 0.0.x, install_openshell() would start the gateway against the pre-existing v1 gateway.toml before that file is ever rewritten. setup_test.sh and gateway_test.sh only exercise the helper functions in isolation against a no-op stub installer, so they cannot catch this composed-order failure, and the PR's testing notes state the GitLab runner VM tooling is "not run on a live VM."
  Remediation: On the upgrade path, stop the unit, reap sandboxes, wipe the pre-0.1 store, and rewrite gateway.toml to schema v2 (pin_supervisor_image) before invoking install.sh, keeping OPENSHELL_ACK_BREAKING_UPGRADE=1. Apply the same order in ensure_job_openshell_gateway. Extend tests so a stub installer fails if gateway.toml is not already v2, and add a test exercising the composed install_openshell -> configure_gateway sequence against a v1 fixture.

Medium

• [error-handling-gap] .github/scripts/install-openshell.sh:26 — This script is invoked directly by action.yml (line 402) for any consumer of the public composite action, including self-hosted/persistent runners, but does not set OPENSHELL_ACK_BREAKING_UPGRADE=1 and does not wipe pre-0.1 gateway state before installing, unlike hack/gitlab-runner-vm/setup.sh and gateway.sh, which do both. action.yml does write schema-v2 gateway config before installing, so the v1-at-startup failure above does not apply to this path. But on a persistent self-hosted GitHub Actions runner still on OpenShell 0.0.x, the 0.1.1 installer refuses the upgrade without the ack, and even if a caller separately exports the ack, the leftover 0.0.x gateway store is left in place — the same incompatible state this PR documents 0.1 cannot read. Ephemeral (e.g. ubuntu-latest) runners are unaffected since they have no pre-existing state.
  Remediation: Set OPENSHELL_ACK_BREAKING_UPGRADE=1 on the install.sh invocation in install-openshell.sh, and detect an existing pre-0.1/non-v2 gateway installation to stop the unit and wipe gateway state before installing, mirroring the procedure already implemented in hack/gitlab-runner-vm and documented in running-agents-locally.md.

• [stale-doc] docs/guides/infrastructure/gate-binaries.md:252 — The providers/gate-query.yaml example was correctly updated to omit credentials (OpenShell 0.1.x allows credential-less providers), but the "Credential collision limitation" workaround later in the same file was not updated to match: it still states "OpenShell requires every provider to declare at least one credential key, so you supply a placeholder (as in the _PLACEHOLDER_GATE_QUERY example above) that carries no real secret" — a requirement that no longer exists in OpenShell 0.1.x, and a dangling reference to an identifier removed from the example above it. A reader following this now-obsolete workaround would create a provider with an empty declared credential value, which 0.1.1 rejects. Independently identified by two review passes.
  Remediation: Rewrite the workaround bullet under "Credential collision limitation" to describe omitting the credentials: block entirely for a credential-less provider, and drop the dangling _PLACEHOLDER_GATE_QUERY reference.

• [protected-path] .github/scripts/install-openshell.sh — This PR modifies files under the protected path .github/: .github/scripts/install-openshell.sh, .github/scripts/openshell-version.sh, and .github/workflows/functional-tests.yml. The PR links issues (Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229, Move to OpenShell 0.1.1 #7751, docs: running-agents-locally.md OpenShell version not updated by Renovate #4808) and explains its rationale for these changes in detail, so sufficient context exists — but human approval is always required for protected-path changes regardless of context.

Low

• [stale-doc] docs/guides/user/running-agents-locally.md:74 — The new schema v2 gateway.toml example hardcodes supervisor_image = "ghcr.io/nvidia/openshell/supervisor:0.1.1" with no inline comment noting it must track the installed OpenShell CLI version. The bash snippet earlier in the doc comments "# check the pin file for the current version", but a TOML file cannot expand variables, so this literal will silently drift out of sync with .github/scripts/openshell-version.sh on the next Renovate bump — the same class of drift issue docs: running-agents-locally.md OpenShell version not updated by Renovate #4808 raised for a different literal in this file.
  Remediation: Add an inline comment near the supervisor_image line (e.g. # match OPENSHELL_VERSION from openshell-version.sh) so readers know to keep it in sync.

• [fail-open] internal/cli/run_openai.go:733 — ensureOpenAIProvider still cannot attach the credential value and its expiry in a single create call: OpenShell 0.1.1 requires a declared credential's value on create and accepts --credential-expires-at only on update, so the live token is stored first (EnsureProviderLiteral, line 733) and its expiry is applied only in the following call (UpdateProviderLiteralWithExpiry, line 737). A process death, panic, or context cancellation strictly between the two calls leaves a live, non-expiring run-scoped credential, since the caller only registers its deferred delete after this function returns success. This is an upstream API constraint (0.1.x's create/update split), not a regression introduced by this PR, and the residual window is narrow.
  Remediation: If a future OpenShell release accepts --credential-expires-at on provider create, pass value and expiry together. Otherwise, consider arming a best-effort deferred delete immediately after EnsureProviderLiteral succeeds, before the expiry-update call.

• [fail-open] internal/cli/run_openai.go:746 — In the double-failure cleanup path (delete fails, then SetProviderCredentialExpiry replaces the old empty-string blanking), the expire call uses the request ctx, while the sibling cleanupRunScopedProvider path deliberately uses context.Background() because the run ctx is frequently already cancelled by the time cleanup runs. DeleteProvider is internally built on context.Background() with its own timeout and can still succeed after a cancelled request ctx; if delete also fails and this SetProviderCredentialExpiry inherits a cancelled ctx, it will not expire the live credential, leaving only an operator StepWarn as the remaining safety net.
  Remediation: Call SetProviderCredentialExpiry with context.Background() in this fallback branch, matching the pattern already used by cleanupRunScopedProvider.

• [missing-breaking-change-marker] action.yml:390 — The PR title now carries the required ! suffix (feat(#7229)!: move to OpenShell 0.1.1) and the PR body has a substantive "Breaking changes" section, resolving the release-notes-visibility risk that drove the prior review's high-severity finding (.goreleaser.yml's changelog.use: github builds release notes from the merged PR's title/body via the GitHub API, not raw commit text). However, per COMMITS.md, both the ! suffix and a BREAKING CHANGE: trailer are required "in both commit messages and PR titles" — none of this PR's 7 individual commit messages carry a ! suffix or a BREAKING CHANGE: trailer (e.g. "feat(Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229): run on OpenShell 0.1.1", "feat(Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229): port the GitLab runner VM tooling to OpenShell 0.1"). This is a residual policy-compliance gap for raw git history / tooling that scans commits directly rather than PR metadata; the primary release-notes risk is already mitigated.
  Remediation: Optional — amend the commit messages that introduce the breaking behavior change so they individually carry ! and a BREAKING CHANGE: trailer, for consistency with COMMITS.md and to keep raw git history self-describing independent of the PR title.

---

Labels: PR modifies CI scripts/workflows and self-hosted install tooling for the OpenShell runtime.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (7)

Review

Findings

High

• [missing-breaking-change-marker] action.yml:390 — This PR moves fullsend's pinned OpenShell version from 0.0.116 to 0.1.1 and rewrites the OpenShell gateway config that action.yml (the public composite-action interface, consumed by every repo referencing uses: fullsend-ai/fullsend@...) generates, from schema v1 (version = 1, supervisor_image under [openshell.gateway]) to schema v2 (version = 2, compute_driver under [openshell.gateway], supervisor_image/health_check_interval_secs under a new [openshell.drivers.podman] table). The PR body states this is "a clean break from 0.0.x... No 0.0.x compatibility is kept," and hack/gitlab-runner-vm/executor/gateway.sh's new comment states "OpenShell 0.1 rejects v1 and version-less files at startup." Per COMMITS.md's own criteria ("Default values change in ways that alter existing behavior"; "Validation is added that rejects previously accepted input"), this is a breaking change: self-hosted/local installs must wipe 0.0.x gateway state and reinstall with OPENSHELL_ACK_BREAKING_UPGRADE=1, and OpenShell 0.1.x now refuses an empty credential value that 0.0.x accepted. The PR title ("feat(Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229): move to OpenShell 0.1.1") carries neither the required ! suffix nor a BREAKING CHANGE: trailer. AGENTS.md calls a missing ! an important-severity finding, since GoReleaser builds release notes from merged PR titles — this break would be invisible to users reading them. (COMMITS.md separately prefers chore over feat for a pinned-tool version bump, but the missing ! is the required fix regardless of which prefix is used.)
  Remediation: Add the ! suffix to the PR title and squash/merge commit message (e.g. feat(#7229)!: move to OpenShell 0.1.1 or chore(openshell)!: move to OpenShell 0.1.1), and add a BREAKING CHANGE: trailer describing the schema v2 migration, the OPENSHELL_ACK_BREAKING_UPGRADE=1 requirement, and the manual upgrade steps already documented in this PR's running-agents-locally.md changes.

Medium

• [scope-creep] — Issue Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229's "Proposed change" asks only for a short ADR 0030 annotation of OpenShell's sandbox exec/stop signal semantics. This PR keeps that annotation but also lands a 25-file OpenShell 0.1.1 migration: CI pin and workflow config, action.yml's public gateway-config contract, self-hosted VM provisioning/gateway scripts, internal/cli/run_openai.go's credential-create path, and removal of _NOOP_* placeholder credentials across 7 scaffold provider templates. The PR body discloses the expansion ("This PR began as the ADR 0030 annotation for Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229... That annotation is kept and updated for 0.1.x"), and the ADR text was rewritten to describe 0.1.x's SIGTERM stop behavior — so the version bump is thematically coupled to keeping that annotation accurate, not a wholly unrelated feature smuggled in under a docs issue. Neither Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229 nor docs: running-agents-locally.md OpenShell version not updated by Renovate #4808 (a docs-drift issue) authorizes the broader migration, though.
  Remediation: File a dedicated tracking issue for the OpenShell 0.1.1 migration (breaking-change impact, coordination with the companion Declare provider credentials in profiles and drop _NOOP_* placeholders for OpenShell 0.1 agents#1512 PR) and reference that issue in the title/body alongside Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229.

• [protected-path] .github/scripts/openshell-version.sh — This PR modifies files under the protected path .github/ (.github/scripts/openshell-version.sh and .github/workflows/functional-tests.yml). The PR links issues (Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229, docs: running-agents-locally.md OpenShell version not updated by Renovate #4808) and explains its rationale, so sufficient context exists — but human approval is always required for protected-path changes regardless of context.

• [invalid-issue-closure] docs/guides/user/running-agents-locally.md:56 — The PR claims "Closes docs: running-agents-locally.md OpenShell version not updated by Renovate #4808," but docs: running-agents-locally.md OpenShell version not updated by Renovate #4808 asked for a systemic fix so future Renovate bumps of .github/scripts/openshell-version.sh also update this doc: either a renovate.json customManagers entry grouped with the existing openshell group, or removal of the hardcoded version. This PR does neither — renovate.json is untouched, and line 56 still hardcodes export OPENSHELL_VERSION=0.1.1 (the new gateway.toml example also hardcodes supervisor:0.1.1). This is a one-time manual re-sync; the next Renovate bump will stale these literals again exactly as docs: running-agents-locally.md OpenShell version not updated by Renovate #4808 describes. (The prerequisites table already points at the pin file as source of truth, pre-existing this PR, which partially mitigates but does not resolve the automation gap.)
  Remediation: Either add a customManagers entry to renovate.json covering the version literals in running-agents-locally.md, or drop "Closes docs: running-agents-locally.md OpenShell version not updated by Renovate #4808" from the PR description since the systemic drift issue remains unaddressed.

• [error-handling-gap] internal/cli/run_openai.go:747 — ensureOpenAIProvider now creates the run-scoped OpenAI provider with the live credential value because OpenShell 0.1.x rejects an empty value for a declared credential (the prior empty-create fallback and emptyCredentialRefusedRe were removed in this diff). The last-resort cleanup path was not updated to match: if UpdateProviderLiteralWithExpiry fails and the subsequent DeleteProvider also fails, the code still calls sandbox.UpdateProviderLiteral(ctx, name, empty) at line 747 with an all-empty-string credential map — the same empty-value shape 0.1.x is described as rejecting for a declared credential. If update-with-empty is rejected the same way create-with-empty now is, this safety net can no longer succeed on 0.1.x (the code does still warn the operator on failure — see lines 748-750 — so this is not a silent failure, but the credential-blanking guarantee itself may be broken). TestEnsureOpenAIProvider_StoreAndDeleteFailureBlanksCredential (run_openai_test.go:299) still asserts the blank update succeeds, but only against fakeOpenshellRecorder, a stub that does not enforce OpenShell 0.1.x's new empty-value validation, so it would not catch this regression against a real 0.1.x gateway.
  Remediation: Verify against a real OpenShell 0.1.1 gateway whether provider update --credential KEY= (empty value) is accepted for a declared credential. If rejected, replace the blanking fallback with a 0.1.x-compatible approach (e.g., an already-elapsed --credential-expires-at instead of blanking, or a bounded delete retry with a clear operator warning). Update TestEnsureOpenAIProvider_StoreAndDeleteFailureBlanksCredential to match the actual 0.1.x-compatible behavior.

• [stale-doc] docs/guides/infrastructure/gate-binaries.md:252 — This PR removes the _PLACEHOLDER_GATE_QUERY placeholder credential from the providers/gate-query.yaml example, replacing it with a credential-less provider definition (OpenShell 0.1.x supports providers with no credentials). The "Credential collision limitation" section later in the same file (line 252) was not updated to match: it still reads "OpenShell requires every provider to declare at least one credential key, so you supply a placeholder (as in the _PLACEHOLDER_GATE_QUERY example above)" — a dangling reference to an identifier and a requirement that no longer exist in this file or in OpenShell 0.1.x. Independently identified by two separate review passes.
  Remediation: Rewrite the first workaround bullet under "Credential collision limitation" (line 252) to describe a credential-less provider (no credentials: block, as shown in the updated gate-query.yaml example above), and drop the now-dangling _PLACEHOLDER_GATE_QUERY reference.

Low

• [fail-open] internal/cli/run_openai.go:737 — See also: [error-handling-gap] finding at internal/cli/run_openai.go:747 (same function). Distinct issue: the happy path now creates the provider with the live credential value at line 737 and attaches its expiry only in the following UpdateProviderLiteralWithExpiry call (line 741) — a comment states OpenShell 0.1.x takes an expiry only on update, so this two-call split is an upstream API constraint rather than an accidental omission, and cleanup already runs if the second call fails. The residual window is narrow: a process death, panic, or context cancellation strictly between the two calls (before the caller arms its own deferred cleanup) would leave a live, non-expiring credential with nothing armed to clean it up.
  Remediation: If OpenShell 0.1.1's provider create accepts --credential-expires-at at creation time, pass value and expiry together in the create call. Otherwise, consider arming a best-effort deferred delete immediately after EnsureProviderLiteral succeeds, before the expiry-update call, rather than only after the whole function returns.

• [code-organization] internal/cli/run_openai.go:733 — The empty map (lines 733-736) is allocated immediately before the happy-path EnsureProviderLiteral call but is only consumed in the double-failure blanking branch at line 747, separating its declaration from its single use site and from the comment describing the create/expiry sequence.
  Remediation: Move the empty := make(map[string]string, len(keys)) allocation and loop into the failure branch immediately before its use at line 747 (or remove it if the blanking fallback is replaced per the error-handling-gap finding above).

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (8)

Looks good to me

Previous run (9)

Review

Findings

High

• [incorrect-claim] docs/ADRs/0030-openshell-sandbox-interaction-model.md:188 — The new annotation overstates OpenShell's signal contract and misrecords why PR fix(#6936): send SIGINT into sandbox so cancelled runs report total_cost_usd #7208 closed. Three passages are in scope: the Command-execution pointer "Native sandbox exec / sandbox stop cannot deliver SIGINT into the invoked process tree" (lines 80-82); "Neither primitive supports graceful in-sandbox shutdown for telemetry-flush purposes" (lines 188-189); and "PR fix(#6936): send SIGINT into sandbox so cancelled runs report total_cost_usd #7208 attempted a secondary sandbox exec as a SIGINT side-channel for total_cost_usd and was closed unmerged once this contract was confirmed" (lines 194-196). The exec/stop analysis is correct as far as it goes: the original exec channel does not propagate host-side signals (feat(sandbox): let the caller terminate a sandbox exec command and everything it spawned NVIDIA/OpenShell#3159), and stop is SIGKILL not SIGINT. The overstatement is treating a second, separate sandbox exec that runs kill -INT <pid> as equally infeasible. PR fix(#6936): send SIGINT into sandbox so cancelled runs report total_cost_usd #7208's own body documents the opposite, with an E2E run on fullsend-playground/go-hello (run 34513766953): "Our test run confirmed the SIGINT mechanism works (total_cost_usd: 0.7879, was 0)", achieved via exactly that second-exec kill-INT path. The PR's own closing comment (ascerra, 2026-09-11, "Decision: Close this PR which tries to SIGINT-into-sandbox") cites supportability and value as the reasons for closing — bypassing OpenShell's supported stop lifecycle, a claude-only PID-file path that silently no-ops for the pi/codex runtimes, and total_cost_usd being a non-authoritative list-price estimate — not failed signal delivery. Production already ships the same second-exec-plus-in-guest-kill pattern in internal/runtime/stray_processes.go (killStrayProcessesTemplate: a sandbox.Exec call running kill -s TERM then kill -s KILL against known guest PIDs). As written, the ADR would teach a future reader that SIGINT-via-secondary-exec is fundamentally impossible — the opposite of the real blocker this annotation exists to record per issue Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229.
  Remediation: Rewrite the line 80-82 pointer and lines 188-189 / 194-196 to distinguish (1) the original exec channel cannot propagate host-side signals to the process it started (true; feat(sandbox): let the caller terminate a sandbox exec command and everything it spawned NVIDIA/OpenShell#3159) from (2) a second sandbox exec that runs kill -INT <pid> against a known guest PID, which does deliver the signal (E2E-verified by PR fix(#6936): send SIGINT into sandbox so cancelled runs report total_cost_usd #7208; already used in production by internal/runtime/stray_processes.go for TERM/KILL). State the actual reasons fix(#6936): send SIGINT into sandbox so cancelled runs report total_cost_usd #7208 was closed: an unsupported side-channel outside OpenShell's documented contract, PID-file coverage limited to the claude runtime, and total_cost_usd not worth the added risk/complexity — not that signal delivery itself failed.

Medium

• [adr-immutability-scope] docs/ADRs/0030-openshell-sandbox-interaction-model.md:168 — The PR adds a new ### Signal and lifecycle semantics heading under ## Decision, a structural form this Accepted ADR's Decision section did not previously use (Decision items are **Bold:** paragraphs; ### headings otherwise live only under Options). docs/contributing/adrs.md allows "minor annotations" on Accepted ADRs — cross-references, short notes, typo/link fixes, status changes — while flagging substantial rewrites of Context/Decision/Consequences for review. This is not a rewrite of the Commands: SSH decision, and issue Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229 explicitly authorized "a short section" on ADR 0030, so it is not unauthorized. It is, however, larger and more embedded than this file's own post-acceptance precedent (the four-line #6691 blockquote note immediately above it): only the first three lines of the new subsection are formatted as a blockquote note, and the remaining ~20 lines of upstream analysis and PR fix(#6936): send SIGINT into sandbox so cancelled runs report total_cost_usd #7208 history are unmarked prose in the same style as the ADR's original Decision content.
  Remediation: Keep the addition in ADR 0030 as issue Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229 requested, but wrap the entire new subsection (not just the first three lines) inside a blockquote/callout so it reads unambiguously as a post-acceptance annotation rather than a new peer Decision subsection. A new small ADR remains an option if the team would rather not grow the Decision section, but is not required given issue Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229's stated preference.

These findings should be addressed before merge: the high finding corrects a factual claim that is central to the annotation's purpose (documenting the real reason a signal-delivery approach was abandoned), and the medium finding brings the addition's formatting in line with this repo's Accepted-ADR annotation convention.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 12:42 PM UTC · Completed 12:48 PM UTC

Commit: ccf72d3 · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.01

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (bot-triggered)

Fixed both review findings on PR #7233 by correcting the ADR's overstated signal-delivery claim (distinguishing the original exec channel's signal-propagation limitation from the working second-exec kill-INT side-channel, and correcting the stated reason PR #7208 was closed) and by wrapping the new subsection fully in a blockquote to match the file's existing annotation convention. Verified both findings against PR #7208's actual body/closing comment and the production code in internal/runtime/stray_processes.go before making changes. Docs-only change: ran the ADR/doc lint scripts directly (lint-adr-status, lint-adr-numbers, lint-adr-frontmatter, lint-docs-links, lychee link check) since pre-commit could not fetch remote hook repos (HTTP 403, same infra issue noted in the original PR description), plus gitleaks and a trailing-whitespace/EOF check - all passed. No Go code changed, so no test suite or coverage gate applies.

Fixed (2):

1. incorrect-claim: ADR 0030 overstates OpenShell's signal contract and misrecords why PR fix(#6936): send SIGINT into sandbox so cancelled runs report total_cost_usd #7208 closed (docs/ADRs/0030-openshell-sandbox-interaction-model.md): Rewrote the Command-execution pointer (lines 80-83) and the Signal and lifecycle semantics subsection to distinguish the original sandbox exec channel (cannot propagate host-side signals into the process it started) from a second, independent sandbox exec that runs kill -INT <pid> against a known guest PID (which does deliver the signal - E2E-verified in PR fix(#6936): send SIGINT into sandbox so cancelled runs report total_cost_usd #7208 with total_cost_usd going from 0 to 0.7879, and already used in production by killStrayProcessesTemplate in internal/runtime/stray_processes.go for TERM/KILL). Also corrected the stated reason PR fix(#6936): send SIGINT into sandbox so cancelled runs report total_cost_usd #7208 was closed: it was an unsupported side-channel outside OpenShell's documented contract, the PID-file mechanism only reliably targets the runtime process for callers that exec into it (Claude, not Codex/Pi), and total_cost_usd is a list-price estimate rather than an authoritative billing figure - not that signal delivery failed. Verified all three claims directly: read PR fix(#6936): send SIGINT into sandbox so cancelled runs report total_cost_usd #7208's body and closing comment (ascerra, citing supportability/PID-file scope/estimate non-authoritativeness as the close reasons, with an E2E log showing total_cost_usd: 0.7879), and confirmed the production TERM/KILL second-exec pattern in internal/runtime/stray_processes.go.
2. adr-immutability-scope: new Signal and lifecycle semantics subsection mixes a 3-line blockquoted note with ~20 lines of unmarked prose, inconsistent with the file's post-acceptance annotation convention (docs/ADRs/0030-openshell-sandbox-interaction-model.md): Wrapped the entire Signal and lifecycle semantics subsection body in the blockquote (every paragraph now prefixed with '>'), matching the existing fix(sandbox): keep OpenShell 0.0.111+ sandboxes alive for later exec #6691 post-acceptance note convention in the same file, so the whole addition reads unambiguously as an annotation rather than a new peer Decision subsection. Kept the change confined to this PR's addition as issue Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229 requested; did not split it into a separate ADR since Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229 explicitly asked for a short section on ADR 0030.

Tests: passed

Decision points

• Kept the annotation inside ADR 0030 rather than splitting it into a new ADR (alternatives: Move the Signal and lifecycle semantics content to a new small ADR, Keep it as an annotation within ADR 0030, fully blockquoted; rationale: Issue Record OpenShell's sandbox exec/stop signal contract in an ADR so PR #7208's dead end isn't rediscovered #7229 explicitly requested a short section on ADR 0030, and the reviewer's medium finding confirmed a new ADR is optional, not required, given that preference - so the minimal fix was to bring the existing addition's formatting in line with the file's annotation convention rather than restructure it.)

---

Next steps:

• /fs-review — request a re-review of the changes
• /fs-fix <your instruction> — run another fix pass with specific guidance
• Push commits directly — review re-runs automatically on push
  _{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:50 PM UTC · Completed 1:02 PM UTC

Commit: cd5da60 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $2.27

Superseded by updated review

[waynesun09]

/fs-fix-stop

[github-actions]

Fix agent disabled for this PR. Remove the fullsend-no-fix label or use /fs-fix to re-engage.

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 1:39 PM UTC · Ended 1:45 PM UTC

Commit: aabf486 · View workflow run →

[github-actions]

Site preview

Preview: https://a993e609-site.fullsend-ai.workers.dev

Commit: 311c1ef517dcf51fba3167c701f188e856beaa7d

[waynesun09]

@Aliciapet11 thanks, good catch. 3f3f5ea rewrites the upgrade note in running-agents-locally.md as numbered, runnable steps:

1. Delete sandboxes with the 0.0.x CLI.
2. Stop the gateway (systemctl --user stop openshell-gateway).
3. Move the config and the gateway/tls state aside, using the XDG default paths.
4. Write the schema v2 config, then install with OPENSHELL_ACK_BREAKING_UPGRADE=1.

Writing the config before installing also fixes an ordering problem: on Linux packages the installer starts the gateway right away, so the old order ("install, then write the config") brought it up against the v1 file and it failed with legacy_schema_v1. A note covers Homebrew, which keeps its state under $(brew --prefix)/var/openshell and uses brew services.

Step 3 was run as written under bash and zsh on macOS and bash on Fedora, including a re-run on an already-moved tree.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:51 PM UTC · Completed 1:16 PM UTC

Commit: 3f3f5ea · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $11.61

[waynesun09]

Local verification of 3f3f5ea on OpenShell 0.1.1, with fullsend-ai/agents#1513:

Runtime / model	macOS arm64	Fedora 44 x86_64
claude (Vertex)	✅ passed	✅ passed
codex, openai/gpt-5.6-luna	✅ passed	✅ passed
pi, openai/gpt-5.6-luna	✅ passed	✅ passed

• Upgrade doc: on Fedora, the new "Upgrading from OpenShell 0.0.x" block in running-agents-locally.md was run as written, starting from a real 0.0.116 install with its schema v1 config and state. The 0.1.1 gateway started on the first try, and all three triage runs above passed afterwards against the recreated providers.
• Every run: profiles imported, providers ready, the run-scoped OpenAI provider created with its expiry and deleted afterwards, and the sandbox deleted in about 0.5 s.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:32 PM UTC · Completed 4:50 PM UTC

Commit: 311c1ef · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $7.56

Superseded by updated review

[fullsend-ai-review]

See the review comment for full details.

[waynesun09]

Approving. fullsend-ai/agents#1513 is merged (a05e2b356), so the credential-declaration half is on agents main; the failed e2e/behaviour jobs are re-running against it before enqueue. Verified locally on OpenShell 0.1.2 (macOS arm64 and Fedora 44 x86_64): claude, codex and pi triage, the stray-process sweep, and the documented upgrade from 0.0.116. All review threads are resolved.

[waynesun09]

Merged together with fullsend-ai/agents#1513 (both via a maintainer bypass, since their pre-merge CI could not pass until the other landed; both rulesets restored afterwards). The e2e re-run against the merged agents main passed before this merged.

Follow-ups:

• Open PRs branched before this merge still pin OpenShell 0.0.116 in their own code, so their e2e/behaviour will fail until the branch includes this change. Update the branch (a merge from main is enough) before the next review round or before enqueueing. The same applies to agents PRs whose changes select an agent eval in functional-tests.
• GitLab runner VMs @ggallen: once the fullsend-runner:dev image rebuilt from this merge is published, an existing VM refuses its jobs (its executor still trusts the 0.0.116 pin) until hack/gitlab-runner-vm/setup.sh is re-run on it or the VM is recreated.