Skip to content

fix(cli): guard device fallback state#75544

Merged
vincentkoc merged 2 commits into
mainfrom
qa-critical-human-paths-next-20260501
May 6, 2026
Merged

fix(cli): guard device fallback state#75544
vincentkoc merged 2 commits into
mainfrom
qa-critical-human-paths-next-20260501

Conversation

@vincentkoc
Copy link
Copy Markdown
Member

Summary

  • stop openclaw devices local fallback when the active Gateway names a pending request that is absent from the local pairing store
  • keep the happy-path loopback fallback when the same request exists locally
  • add regression coverage and an Unreleased changelog note

Tests

  • FORCE_COLOR=0 pnpm test:serial src/cli/devices-cli.test.ts
  • git diff --check origin/main..HEAD
  • FORCE_COLOR=0 pnpm exec oxfmt --check --threads=1 src/cli/devices-cli.ts src/cli/devices-cli.test.ts CHANGELOG.md
  • Testbox tbx_01kqhaz9ewzybzwxqa2tsg57np: OPENCLAW_TESTBOX=1 pnpm check:changed passed (GitHub run 25208306627)

@openclaw-barnacle openclaw-barnacle Bot added cli CLI command changes size: S maintainer Maintainer-authored PR labels May 1, 2026
@vincentkoc vincentkoc self-assigned this May 1, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 1, 2026

Codex review: passed.

Summary
The PR adds request-id-aware guards to openclaw devices local list/approve fallback, regression tests, and a changelog entry.

Reproducibility: yes. Source inspection and the added mocked tests show the failure path: the Gateway reports req-profile while the local store has or can approve req-default, and current main can fall back to the wrong local state.

Real behavior proof
Not applicable: This is a maintainer/member PR, so the external-contributor real behavior proof gate does not apply.

Next step before merge
No narrow PR-diff repair remains, but exact-head checks have an unrelated type failure and the PR is already paused for human review.

Security
Cleared: The diff is auth-sensitive but now fails closed before local pairing mutation and does not touch CI, dependencies, release scripts, or other supply-chain surfaces.

Review details

Best possible solution:

Keep the narrow request-id guard and regression coverage, then resolve the exact-head CI/human-review state before merge.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection and the added mocked tests show the failure path: the Gateway reports req-profile while the local store has or can approve req-default, and current main can fall back to the wrong local state.

Is this the best way to solve the issue?

Yes. The current branch validates the Gateway request id before local approval mutation, preserves loopback fallback when the request matches, and adds focused regression coverage; the remaining blocker is CI/human-review state rather than a different code design.

What I checked:

  • Current main fallback bug: Current main accepts any loopback pairing-required error for local fallback and calls approveDevicePairing(requestId, ...) without checking the Gateway-reported request id, so a profile/state-dir mismatch can mutate the wrong local store. (src/cli/devices-cli.ts:220, 58c706451e1d)
  • Documented fallback surface: The devices CLI docs define the existing behavior as local loopback list/approve fallback when pairing scope is unavailable and no explicit --url is passed, so this is a guard on existing behavior rather than a new feature. Public docs: docs/cli/devices.md. (docs/cli/devices.md:155, 58c706451e1d)
  • Prior review finding addressed: The earlier ClawSweeper review flagged that approve fallback approved a local request before validating the Gateway request id; current head now compares gatewayRequestId before calling approveDevicePairing. (src/cli/devices-cli.ts:258, f45c58580705)
  • Absent local request guard: When the Gateway request id matches the operator target but local state has no such pending request, the PR throws the mismatch error instead of returning unknown requestId, which keeps the fallback fail-closed. (src/cli/devices-cli.ts:267, f45c58580705)
  • Regression coverage: The PR adds tests for list fallback with a missing Gateway request, approve fallback with an absent local request, and explicit approve of a different local request before mutation. (src/cli/devices-cli.test.ts:593, f45c58580705)
  • Exact-head CI blocker: GitHub check-runs for the current head show check-test-types failing in src/agents/model-fallback.test.ts, outside the PR's changed files; the related aggregate check job also failed. (src/agents/model-fallback.test.ts:1091, f45c58580705)

Likely related people:

  • steipete: Recent devices CLI and pairing-state history includes admin approval retry, admin scope approval, shared CLI fixtures, pairing state recovery, and shared pairing connect detail assembly. (role: recent maintainer / adjacent owner; confidence: high; commits: c3f5c20f2cc0, e8f13c625e34, 3381e4c375bd; files: src/cli/devices-cli.ts, src/cli/devices-cli.test.ts, src/infra/device-pairing.ts)
  • obviyus: Recent merged work preserved local pairing fallback for upgrades and surfaced pending pairing upgrade request details consumed by this CLI fallback path. (role: feature introducer / adjacent owner; confidence: medium; commits: 84f535c315da, 67d2026e22e9, f070a92e19fb; files: src/cli/devices-cli.ts, src/cli/devices-cli.test.ts, src/gateway/protocol/connect-error-details.ts)
  • shakkernerd: GitHub path history identifies the original devices CLI local pairing fallback change as CLI: recover devices commands via local pairing fallback. (role: introduced local fallback behavior; confidence: medium; commits: aa3c8f732b72; files: src/cli/devices-cli.ts)
  • coygeek: Prior merged work fixed a nearby devices CLI approval hazard where implicit latest-device approval could pair the wrong requester, which is closely related to this fail-closed request-id guard. (role: adjacent approval-safety owner; confidence: medium; commits: 192ee081e77e; files: src/cli/devices-cli.ts, src/cli/devices-cli.test.ts, src/infra/device-pairing.ts)

Remaining risk / open question:

  • Exact-head CI currently reports an unrelated check-test-types failure in src/agents/model-fallback.test.ts; automerge should wait for maintainer/CI handling even though the changed files look sound.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 58c706451e1d.

@vincentkoc
Copy link
Copy Markdown
Member Author

/clawsweeper automerge

@vincentkoc vincentkoc marked this pull request as ready for review May 1, 2026 11:55
@clawsweeper clawsweeper Bot added the clawsweeper:automerge Maintainer opted this PR into bounded ClawSweeper-reviewed automerge label May 1, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 1, 2026

🦞🦞
ClawSweeper automerge is enabled for this PR.

I added clawsweeper:automerge and asked ClawSweeper to review this head. If ClawSweeper emits a repair marker or requests changes, I will repair/rebase the branch and ask for another review, up to the configured round limit.

Draft PRs stay fix-only until GitHub marks them ready for review. A maintainer can pause this with /clawsweeper stop.

Automerge progress:

  • 2026-05-01 11:59:06 UTC review queued `b8a89fa9b8ed` (queued)

@clawsweeper clawsweeper Bot added the clawsweeper:human-review Needs maintainer review before ClawSweeper can continue label May 1, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 1, 2026

🦞🦞
ClawSweeper is pausing this repair loop for human review.

Source: clawsweeper[bot]
Reason: structured ClawSweeper verdict: needs-human (sha=b8a89fa9b8ed4729a0d730e5b74754959b53dce5)

I added clawsweeper:human-review and left the final call with a maintainer.

@vincentkoc vincentkoc force-pushed the qa-critical-human-paths-next-20260501 branch 2 times, most recently from ff483e6 to ce9efd1 Compare May 5, 2026 23:58
@vincentkoc vincentkoc force-pushed the qa-critical-human-paths-next-20260501 branch from ce9efd1 to f45c585 Compare May 6, 2026 00:00
@openclaw-barnacle openclaw-barnacle Bot added the agents Agent runtime and tooling label May 6, 2026
@vincentkoc vincentkoc merged commit 01377dd into main May 6, 2026
110 of 112 checks passed
@vincentkoc vincentkoc deleted the qa-critical-human-paths-next-20260501 branch May 6, 2026 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent runtime and tooling clawsweeper:automerge Maintainer opted this PR into bounded ClawSweeper-reviewed automerge clawsweeper:human-review Needs maintainer review before ClawSweeper can continue cli CLI command changes maintainer Maintainer-authored PR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant