Repository navigation
test(openshell): prove model, tool and gateway outages fail closed - #2393
Conversation
…reams Run the route probes as Sandbox processes behind OpenShell's proxy: the model placeholder is refused at a tool source's host, direct connections reach neither a denied host nor a bare address, and a tool source whose upstream presents an untrusted certificate never receives a request. After replacement, point cluster DNS for the model provider at that upstream and require the model turn to fail without a request reaching it. Record that commands Codex starts can use the model route, because OpenShell matches policy binaries against ancestors, and move the suite's coverage notes to their own testing page.
Custom model endpoints must serve Responses, not model listing, so probe the inserted key with a minimal Responses request.
Count the foreign-host probe as refused only on 403 with OpenShell's credential_endpoint_mismatch code; a transport failure or another error now fails the check. Require the allowed host to answer without the proxy settings, so the denied-host checks run with an outbound path, and count untrusted-upstream TLS failures only for the expected server name.
Take each dependency of the credentialed route away and restore it before asserting: - the model provider: cluster DNS sends the model host to a Service with no endpoints; the turn fails without the key, and the same revision serves the next turn once DNS is restored; - a tool upstream: OpenShell ends the request without a response, and the token is substituted again once the upstream returns; - the Namespace's OpenShell gateway: while it is scaled to zero the running Agent cannot serve a turn, a source read reports failed, registration returns 503, and a redeploy stays queued; once it is back, the redeploy activates and serves a turn. After the gateway restarts, an OAuth2 source due in OpenShell's first refresh sweep fails with refresh_configuration_invalid and is retried an hour later (NVIDIA/OpenShell#4526). The test accepts that state and requires a forced rotation to restore the token, and the reference documents that recovery. Share the Sandbox probe helper between phases, match the probe command by its unique name, and ask again when the model skips the exec tool.
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. What this changesThis PR adds real OpenShell tool, model-provider, and gateway outage tests, recovery assertions, and matching documentation. Example: The token-echo Service temporarily has no endpoints.
Review scores
ProductKind: Test only · Worth it: Yes · Fix scope: Partial Merge readiness✅ Ready for maintainer review This PR adds useful outage coverage absent from current main; the conflict resolution and selected-credential repair address the recorded concerns, with no actionable findings remaining. Priority: P2 Before mergeNone. FindingsNone. Tests
Agent review detailsHow this fits togetherOCC registers credential sources and deploys dedicated Codex Harnesses through selected Drivers; OpenShell mediates their model and tool requests, while the integration suite disrupts dependencies and observes API, worker, and runtime outcomes. flowchart TB
Suite[Test suite] --> OCC[OCC API and worker]
OCC --> Codex[Dedicated Codex Harness]
Suite --> Gateway[Agent Gateway]
Gateway --> OpenShell[OpenShell gateway and proxy]
OpenShell --> Codex
Codex --> OpenShell
OpenShell --> Model[Model provider]
OpenShell --> Tool[Tool upstream]
Technical reviewBest possible solution: Retain the focused real-runtime outage coverage and its explicit separation from broader milestone and release qualification. Do we have a high-confidence way to reproduce the issue? This is primarily a coverage change; source inspection confirms the missing outage phases on main and the login-mode forwarding mismatch repaired by this PR. Is this the best way to solve the issue? Extending the existing production-path suite is appropriate because it observes real dependency failures, exact credential recovery, and durable reconciliation without adding a competing implementation. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against e03429ce3d88. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes: No label changes. Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (3 earlier review cycles)
Reviewed October 15, 2026, 7:58 PM ET / 23:58 UTC (Revision 4). |
|
Free's clanker here (an AI agent working on Free's behalf). I reviewed only the last commit, Verdict: the approach holds up. Each outage is real: a ClusterIP Service with no endpoints refuses connections, and a StatefulSet scaled to 0 has no Pod left. Each dependency comes back before any assertion runs, and every wait has a bound. The run log in the body matches what the code asserts. I found three low-severity gaps where an assertion proves less than the docs claim. None of them blocks the stack, but all three are cheap to tighten.
Nits (optional):
What I checked:
|
…sifying it curl prints 200 even when the response body then fails to arrive, for example on a timeout (exit 28). The classifier then read the missing length as 0 and accepted the result as an empty header. Count a 200 only when curl exits 0, accept an empty header only when the echo reported length 0, and log the probe's exit code.
Tool upstream: the echo answers 200 without a credential, so recovery now requires curl to exit 0 and the echo's digest to match tool A's token, and the outage request must not deliver it. Gateway: replace the fixed 60-second wait with a bounded wait for a failed reconcile attempt while the gateway is still at zero replicas, and require the deployment to stay queued. Check the gateway-down turn for the model key outside the try that classifies it, so a turn that completed and leaked the key can no longer read as failed. Check every suite credential on each probe turn before anything is printed, delete the model-outage Service and the outage Secret, and rewrap two reference lines.
openclaw#2374 was squash-merged, so its original commits stay on this branch. Resolve the conflicts by applying only this branch's changes after 954910a on top of main, and rewrap three reference lines.
|
Free's clanker here (an AI agent working on Free's behalf). I re-reviewed the two commits after All three are addressed:
Local checks on What's left:
I'm leaving the merge to a maintainer. |
openclaw#2454 made the Credential Gateway Driver ask the Backend client where the gateway runs, so it can refuse token_url hosts in that namespace. The real suite's Backend client, which reaches each Namespace's gateway through a port-forward, had no placement(), so registering the first source failed with 500 INTERNAL_ERROR. Return the Namespace's gateway Service, and no placement when called without a namespace, as the production Backend does for a bare serviceName.
|
#2374 merged as a squash of ClawSweeper's review (
@freeqaz's review:
Nits:
One more change: The real OpenShell suite passed on The redeploy failed its first attempt while the gateway was stopped and stayed queued. All three OAuth2 sources hit NVIDIA/OpenShell#4526 after the restart and returned to @clawsweeper re-review re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
rating: 🦞 diamond lobster
freeqaz-openai
left a comment
freeqaz-openai
left a comment
There was a problem hiding this comment.
P2, in the conflict resolution please check the selected model credential. The exact-head Claw result resolves the prior findings; its real outage/recovery evidence is useful on the tuple tested. This is a new integration-oracle issue: main now defaults to a Codex access token, while this PR’s new absence checks name only the API-key slot. This is not evidence of a credential in actual output.
I reproduced three conflict blocks in two files against landed 04514330. In sandbox-driver-openshell-k3d-real.test.mjs, preserve main’s modelCredentialVariable placeholder check and this PR’s shared sandboxProbeFunction and runSandboxProbe. Also update the six non-conflicting oracle references: use main’s already-selected modelCredential in the probe’s credential list and the five model/gateway outage observation checks. Otherwise the list can omit the selected test value, while .includes(undefined) searches for the text “undefined,” not that value.
With only invented canaries in an isolated JS context, I exercised the actual selector and the six assertion operands. The mechanical resolution detected a planted selected value in 0/6 default-token checks (whether the unrelated API slot was absent or set); the corrected source detects it in 6/6. Native/custom-endpoint API mode remains 6/6, clean observations zero false positives. This discriminates the oracle; it is not six new live outage passes or a claim arbitrary output is safe.
In openshell-coverage.md keep main’s manual-full vs trusted-PR-advisory distinction and remove the obsolete “outages not covered.” I verified a complete four-file resolution applies to the exact main beforeimages. If #2440 lands first, retain its chart-version change; its proposed OpenShell 5a7728c3 is 16 commits behind the upstream #4527 fix by Git ancestry, so don’t remove the recovery workaround just because the version changes. Please bind ordinary CI and applicable outage qualification to the actual resulting source and selected credential mode. No new credential, value logging, bot rerun, cluster mutation or merge requested.
Resolve the Sandbox suite's conflicts with Codex PAT support: keep main's selected-credential inheritance check and this branch's shared probe helper. The probe helper and the outage phases now check main's selected modelCredential rather than OPENAI_API_KEY, which is unset in the default Codex token mode. Keep main's Backend client placement(), which supersedes this branch's stub. The coverage page keeps main's advisory-run sentence and drops the obsolete "Not covered" section.
…dbox Driver openclaw#2048 made the OpenShell Sandbox Driver choose Codex isolation from the admitted credential source's login mode, the third configureAgent argument. The suite's wrapper dropped it, so OCC always selected workspace-write. That matches the expectation in the default Codex token mode, but API-key mode (OCC_TEST_OPENSHELL_MODEL_BASE_URL) failed its danger-full-access check at the first deploy.
|
@freeqaz thanks. Addressed in
Verification. The real OpenShell suite passed on No model key or placeholder appeared in the prepare, run or cleanup logs, and the cluster was removed. Two earlier runs on the same commit failed on the provider's "Selected model is at capacity" response. Run 45 had already passed every phase above before hitting it. Not verified live: the default Codex token mode. I don't have a |
freeqaz-openai
left a comment
freeqaz-openai
left a comment
There was a problem hiding this comment.
Free’s clanker here. My requested change is fixed in 80805c826; thank you. The probe and all five outage observations now check the selected model credential. I exercised those six checks with invented canaries in four token/API-key configurations: each detected the planted value, and none flagged the clean observations. The test also forwards the admitted login mode and uses the existing Backend placement implementation.
I read your real outage and recovery results as API-key mode only, not a live default-token run. Required CI and First Agent Smoke passed on this head; neither runs that full outage scenario. This approval clears my outdated changes request, not those evidence limits or the need to receive the current-head Claw review.
|
Please review the conflict resolution and selected-credential fix on @clawsweeper re-review |
|
🦞👀 Re-review progress:
|
|
Free's clanker here (an AI agent working on Free's behalf). I reviewed
One heads-up: #2515 removes that third Not landing this; it is yours to merge. |
|
Free's clanker here (an AI agent working on Free's behalf). Heads-up: #2515 landed on main at 01:20 UTC as a rebase merge (26d00dd..0fbd964), and this branch now conflicts with main in Something to watch when you merge main: #2515 removed the third I haven't pushed anything to this branch. |
There was a problem hiding this comment.
Free’s clanker here. #2515 landed after the last review, and GitHub now reports a merge conflict. I reproduced it with PR head 80805c82, live main at a79ddca3, and merge base 91ac04d6: all three docs merge cleanly; the integration test has one conflicting hunk.
In createIntegrationSandboxDriverFactory, keep main’s side of that hunk only: the shared configurationDriver, its harnessHome, and the two-argument configureAgent(configuration, harness). Please don’t take main’s entire file. The current Driver no longer reads the third login-mode argument; this suite selects the actual PAT or API-key credential independently. This resolution preserves both the landed Driver behavior and this PR’s six credential-refusal checks, without choosing a new isolation policy.
I checked syntax and exercised those six expressions with synthetic PAT and API-key canaries, including the negative control that an API-key-only check misses the PAT. I didn’t run the live outage suite; the resulting head still needs its own required CI and review. No branch write or rerun from me.
Landing order with #2600: I tested both orders against actual main 80705670a, this head 80805c826, and that head c856ccb4b. If #2600 lands first, the resolution above remains the only conflict for this PR. In the reverse order, Git also puts #2600’s replacement diagnostic opposite this PR’s three outage tests; retain both blocks. Please don’t choose an entire file side.
Both private integrations produced the identical complete tree (b5231a3fc8) and test blob (0182f3906). I verified the three outage functions, six changed diagnostic/reconciliation functions, and their call sites in the combined file; syntax and the six secret-refusal oracles in five synthetic PAT/API-key configurations passed. This is source and merge evidence, not a new live outage run or an instruction to force an order. The published branch and original author’s runtime qualification remain unchanged.
openclaw#2515 removed SandboxDriver.configureAgent's login-mode argument and selects Codex full access for every login mode, so keep main's side of the suite's Sandbox Driver wrapper: the shared configurationDriver, its harnessHome and the two-argument configureAgent. That drops this branch's login-mode forwarding (80805c8), which no longer has a reader. The outage phases and the selected-credential checks are kept.
|
@freeqaz thanks for the resolution notes.
The real OpenShell suite passed on The redeploy failed its first attempt while the gateway was stopped and stayed queued. All three OAuth2 sources hit NVIDIA/OpenShell#4526 after the restart and returned to |
Problem
#118 criterion 4 also needs proof that the OpenShell credential route fails closed when its dependencies go away, and recovers when they return. #2374 covers foreign destinations, proxy bypass and untrusted upstreams; no test took the model provider, a tool upstream or the OpenShell gateway down.
#2374 merged as a squash, so
mainis merged into this branch (ca0988105,5122f1a25after #2048, then4c46e5f8eafter #2515 and #2600) rather than rebased; the branch holds only this PR's changes.Change
tests/integration/sandbox-driver-openshell-k3d-real.test.mjs(Codex scenario) adds three phases after the untrusted-model-upstream check, in this order. Each takes one dependency away and restores it before asserting:failed, registration returns503 DEPENDENCY_UNAVAILABLE, and a redeploy fails at least one attempt and stays queued without activating; the test waits for that attempt while the gateway is still stopped. Once it is back, the redeploy activates and serves a turn. A registration attempted during the outage is leftdeleting; the test deletes it, and then its Secret, after recovery.Supporting changes:
200from the foreign-host probe now counts only when curl exits 0, and an empty header only when the echo reports length0, so a body that never arrives fails the check.Docs:
docs/testing/openshell-coverage.mdreplaces "Not covered: outages" with an Outages section.docs/reference/drivers/openshell-credential-gateway.mdadds the gateway-outage limit and a troubleshooting row for the refresh race below.docs/reference/drivers/credential-gateway.mdfixes a troubleshooting row that said a status read returns503; it reportsfailed, ascredential-sources.mddocuments.Finding: OAuth2 refresh after a gateway restart (NVIDIA/OpenShell#4526)
In operator workspace mode, OpenShell's refresh worker sweeps immediately at startup, before its namespace watcher has filled the operator allowlist. Every OAuth2 refresh due in that sweep fails with
PermissionDenied("workspace … is not in the operator namespace allowlist"), which OpenShell classifies asrefresh_configuration_invalidand retries an hour later. When the current token expires, the Agent's tool calls to that source fail until then. The namespace is labeled throughout; Sandboxes and static providers are unaffected.Filed upstream as NVIDIA/OpenShell#4526, with a fix in NVIDIA/OpenShell#4527. The test accepts that state, with
TODO(NVIDIA/OpenShell#4526), and requires a forced rotation to return each source toready; the reference documents that recovery.Verification
Real OpenShell suite on a disposable k3d cluster, commit
4c46e5f8e(mainatb75d3e371plus this PR), Codex scenario, API-key mode (OCC_TEST_OPENSHELL_MODEL_BASE_URL=https://api.openai.com/v1),gpt-6-astra:Run on 2026-10-16, 23.3 minutes, 1 test passed, 0 failed. Proof lines, which contain no credential values:
failed:refresh_configuration_invalid), and each returned toreadyon a forced rotation. The rest of the suite, including the OAuth2 refresh phase, then passed on the restored tokens.pnpm docs:checkandpnpm docs:check-lengthpass.Not verified live: the default Codex token mode. No
CODEX_ACCESS_TOKENwas available. The lane preparation's presence check for it was satisfied with the same API key, which the suite does not read in API-key mode.CI on this PR does not run this suite: QA advisory skips fork PRs, and the Full Integration
openshelllane runs only frommain. The phases add about 8 minutes to the suite.Page length.
docs/reference/drivers/openshell-credential-gateway.mdis 2,491 words, under the 2,500 limit. To add the outage limit and troubleshooting row I removed five repeated statements (the shared Backend credential, two restatements of OAuth2 token pickup and redeploys, a repeatedabsentmapping, and a Limits bullet that repeated where the placeholder goes). It stays one page as the single reference for the Driver's configuration, trust boundary and troubleshooting.Risks
Test and docs only. The phases change cluster-wide CoreDNS and scale the Namespace's OpenShell gateway on the disposable cluster, and restore each before asserting.
Refs #118