Commit 6ad7a00
committed
## What
Three files in `plugins/antianqi/openclaw-acp-bridge/`:
- `scripts/acp_inbox.py`:
- `ACPInbox.__init__` no longer takes `base_url=`. The bundled
client's `inbox_*` helpers read `$ACP_BASE_URL` (or fall back to
`_acp_client.DEFAULT_BASE_URL`); a per-instance `base_url` was
silently ignored. The constructor is now `(default_timeout)`;
the public API is honest.
- `ACPInbox.read` now takes `limit=None` and forwards it to the
underlying `_acp_client.inbox_read`. The docstring previously
advertised `read(limit=...)` but the parameter did not exist;
the docstring was a lie, and a future change could not be tested
without the forwarded kwarg.
- The CLI (`acp_inbox.py --action ping`) no longer accepts
`--base-url`. Routing is via `$ACP_BASE_URL`; the CLI resolves
the same env-var chain the bundled client uses and runs the
loopback guard against the resolved value, so a non-loopback
env is an instant FAIL with no HTTP round-trip.
- `scripts/test_inbox_goudan.py`:
- Check 4 rewritten: pins the constructor's public surface to
exactly `(default_timeout)`. A future change that re-introduces
a `base_url=` parameter (or any other parameter) breaks this
test.
- Check 5 rewritten: the loopback guard check is now
`_acp_client._check_loopback(...)`, not a constructor-time
check on a dead parameter.
- New Check 13b: mocks `_acp_client.inbox_read` and asserts
that `ACPInbox.read(limit=42)` forwards `limit=42` to the
underlying call. Negative-injection: `read()` (without
`limit=...`) still calls `inbox_read` once.
- Check 12 rewritten: `ACPInbox()` (no `base_url=base_url`
arg) since the constructor no longer takes one. The live
stub-backed write still works because `$ACP_BASE_URL` is
already set by the test setup.
- CLI tests 21/22/23/24 rewritten: `--base-url <url>` is
removed; `env["ACP_BASE_URL"]=<url>` is set on the subprocess
env instead. Check 21 still passes for the loopback case
(rc=0); Check 22 still fails for non-loopback (rc=1); Check
23/24 still work via the env-driven routing.
- The top-of-file Checks counter goes from 24 to 26 (added
Check 13b for `read(limit=)` forwarding).
## Why
PR #30 round-7 (hetaoBackend, 2026-09-02T01:08:36Z): the
wrapper documents a `base_url=...` parameter on the constructor
and a `read(limit=...)` parameter on `read`. Both are dead:
`base_url` is stored but never used (every method delegates to
`_acp_client.inbox_*` which reads `$ACP_BASE_URL`), and
`read(limit=...)` is in the docstring but not in the signature.
"Please make the wrapper endpoint and limit parameters effective
(or remove them from the public contract) and add delegation
tests that use different constructor/env URLs and assert the
forwarded limit."
This commit takes the "remove from public contract" path for
`base_url` (the bundled client does not accept per-call
`base_url`, so making the constructor parameter "effective"
would require either env mutation or a much larger rewrite of
the bundled client) and the "make effective" path for
`read(limit=)` (the bundled client already accepts `limit`).
## Validation
- `python scripts/test_inbox_goudan.py` (CI mode,
`SMOKE_SKIP_LIVE=1`): **21 / 21 PASS, 0 FAIL, 10 SKIP**.
The 10 skipped are the live server checks.
- `python scripts/test_inbox_goudan.py` (live, stub-backed):
**41 / 41 PASS, 0 FAIL, 0 SKIP** on Windows + Python 3.14.
Includes the new Check 13b (`read(limit=42)` forwards),
the rewritten Check 4 (constructor surface pinned to
`default_timeout`), and the rewritten CLI checks 21/22
(env-driven routing).
- `python scripts/smoke.py` (PR #3 mavis-side smoke,
regression check): **26 / 26 PASS, 0 FAIL**. Zero
regression on the mavis side.
- `python scripts/test_no_redirect.py` (PR #3 no-redirect
regression): **PASS**. The no-redirect guarantee still
holds for the underlying `client/_acp_client`; the
wrapper inherits it.
- `node scripts/validate.mjs`: no new FAIL on
`plugins/antianqi/openclaw-acp-bridge/`. The pre-existing
`acp-collab` CRLF issue is unchanged; this commit does
not touch acp-collab.
## Test evidence
End-to-end on Windows + Python 3.14, 2026-09-02 (Asia/Shanghai):
- 24 → 26 tests in `test_inbox_goudan.py`. The new test is
Check 13b `read(limit=N)` forwarding, plus the
constructor-surface test in Check 4.
- All four CLI checks (21, 22, 23, 24) now use
`env["ACP_BASE_URL"]=...` instead of `--base-url ...`.
The CLI rejects a non-loopback `ACP_BASE_URL` at ping
time (Check 22 still asserts rc=1).
- The wrapper no longer accepts `base_url=` at the
constructor. A caller passing `ACPInbox(base_url="...")`
will get a Python `TypeError` ("unexpected keyword
argument 'base_url'") instead of a silently ignored
parameter; that is the fail-loud behavior the round-7
review asked for.
- `read(limit=None)` calls `_acp_client.inbox_read(...)`
without `limit`; `read(limit=42)` calls it with
`limit=42`. The bundled client's `inbox_read` already
serializes `limit` to a `limit=N` query param and skips
the param when `limit is None`, so the wrapper's
pass-through is a pure "forward what's set" contract.
## Design compliance
- **No credentials.** No token, no host, no env var added
to the test or to `acp_inbox.py`; the wrapper reads
`$ACP_TOKEN` and `$ACP_BASE_URL` from the existing
client.
- **No network beyond loopback.** N/A; no new HTTP call.
- **No telemetry.** N/A.
- **No third-party services.** Stdlib only (`urllib`,
`json`, `os`, `sys`, `time`, `pathlib`, `inspect`).
- **No hardcoded paths.** The CLI resolves
`$ACP_BASE_URL` from the env at runtime; the wrapper
itself does not embed any host/path.
- **Fail-closed.** Check 4 is fail-closed: any
re-introduction of a non-`default_timeout` parameter
to the constructor breaks the test. Check 13b is
fail-closed: a future change that drops the
`limit=...` forwarding breaks the test.
- **Inherits loopback + no-redirect.** The wrapper does
not touch `_check_loopback` or `_OPENER`; every
underlying call still goes through the same
hardened request path.
## Notes for the reviewer
- This commit was prepared on the same
`add-acp-inbox-bridge-skill` branch that PR #30 head
`a536628` is built on. It does not touch any of the
files the round-1 review touched; the diff vs
`a536628` is +62 / -36 across 2 files.
- The `base_url` removal is intentionally hard. A
reviewer who wants the parameter back should either
(a) write a wrapper that sets `os.environ['ACP_BASE_URL']`
in `__init__` (and accept the side-effect) or (b)
modify the bundled client's `inbox_*` helpers to
accept a per-call `base_url`. (b) is a more invasive
change to the round-1-approved `client/_acp_client.py`
and should land in a separate PR.
- `read(limit=...)` was a documented but unimplemented
parameter from `a536628`. The round-7 review caught
it; this commit makes it work.
1 parent 00caf51 commit 6ad7a00
2 files changed
Lines changed: 140 additions & 78 deletions
Lines changed: 32 additions & 35 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
86 | 86 | | |
87 | 87 | | |
88 | 88 | | |
89 | | - | |
90 | | - | |
91 | | - | |
92 | | - | |
93 | | - | |
94 | | - | |
95 | | - | |
96 | | - | |
97 | | - | |
98 | | - | |
99 | | - | |
100 | | - | |
101 | | - | |
102 | | - | |
103 | | - | |
104 | | - | |
105 | | - | |
106 | | - | |
107 | 89 | | |
108 | 90 | | |
109 | 91 | | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
110 | 102 | | |
111 | 103 | | |
112 | 104 | | |
113 | 105 | | |
114 | | - | |
115 | 106 | | |
116 | 107 | | |
117 | | - | |
118 | | - | |
119 | | - | |
120 | | - | |
121 | | - | |
122 | | - | |
123 | 108 | | |
124 | 109 | | |
125 | 110 | | |
| |||
171 | 156 | | |
172 | 157 | | |
173 | 158 | | |
| 159 | + | |
174 | 160 | | |
175 | 161 | | |
176 | 162 | | |
177 | | - | |
178 | | - | |
179 | | - | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
180 | 170 | | |
181 | 171 | | |
182 | 172 | | |
183 | 173 | | |
184 | 174 | | |
185 | 175 | | |
| 176 | + | |
186 | 177 | | |
187 | 178 | | |
188 | 179 | | |
| |||
246 | 237 | | |
247 | 238 | | |
248 | 239 | | |
249 | | - | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
250 | 244 | | |
251 | 245 | | |
252 | | - | |
| 246 | + | |
253 | 247 | | |
254 | 248 | | |
255 | | - | |
256 | | - | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
257 | 252 | | |
258 | | - | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
259 | 256 | | |
260 | | - | |
| 257 | + | |
261 | 258 | | |
262 | | - | |
| 259 | + | |
263 | 260 | | |
264 | 261 | | |
265 | 262 | | |
| |||
Lines changed: 108 additions & 43 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
21 | 21 | | |
22 | 22 | | |
23 | 23 | | |
24 | | - | |
| 24 | + | |
25 | 25 | | |
26 | 26 | | |
27 | | - | |
28 | | - | |
| 27 | + | |
29 | 28 | | |
30 | 29 | | |
31 | | - | |
32 | | - | |
33 | | - | |
34 | | - | |
35 | | - | |
36 | | - | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
37 | 41 | | |
38 | 42 | | |
39 | 43 | | |
| |||
45 | 49 | | |
46 | 50 | | |
47 | 51 | | |
| 52 | + | |
| 53 | + | |
48 | 54 | | |
49 | 55 | | |
50 | 56 | | |
| |||
187 | 193 | | |
188 | 194 | | |
189 | 195 | | |
190 | | - | |
191 | | - | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
192 | 203 | | |
193 | | - | |
194 | | - | |
195 | | - | |
196 | | - | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
197 | 215 | | |
198 | | - | |
199 | | - | |
200 | | - | |
201 | | - | |
202 | | - | |
203 | | - | |
204 | | - | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
205 | 228 | | |
206 | | - | |
| 229 | + | |
207 | 230 | | |
208 | | - | |
209 | | - | |
| 231 | + | |
| 232 | + | |
210 | 233 | | |
211 | 234 | | |
212 | 235 | | |
213 | | - | |
| 236 | + | |
214 | 237 | | |
215 | 238 | | |
216 | 239 | | |
217 | | - | |
218 | | - | |
| 240 | + | |
| 241 | + | |
219 | 242 | | |
220 | 243 | | |
221 | 244 | | |
| |||
301 | 324 | | |
302 | 325 | | |
303 | 326 | | |
304 | | - | |
| 327 | + | |
| 328 | + | |
| 329 | + | |
| 330 | + | |
| 331 | + | |
305 | 332 | | |
306 | 333 | | |
307 | 334 | | |
| |||
329 | 356 | | |
330 | 357 | | |
331 | 358 | | |
| 359 | + | |
| 360 | + | |
| 361 | + | |
| 362 | + | |
| 363 | + | |
| 364 | + | |
| 365 | + | |
| 366 | + | |
| 367 | + | |
| 368 | + | |
| 369 | + | |
| 370 | + | |
| 371 | + | |
| 372 | + | |
| 373 | + | |
| 374 | + | |
| 375 | + | |
| 376 | + | |
| 377 | + | |
| 378 | + | |
| 379 | + | |
| 380 | + | |
| 381 | + | |
| 382 | + | |
| 383 | + | |
| 384 | + | |
| 385 | + | |
| 386 | + | |
| 387 | + | |
| 388 | + | |
| 389 | + | |
| 390 | + | |
| 391 | + | |
| 392 | + | |
| 393 | + | |
332 | 394 | | |
333 | 395 | | |
334 | 396 | | |
| |||
489 | 551 | | |
490 | 552 | | |
491 | 553 | | |
492 | | - | |
493 | | - | |
| 554 | + | |
| 555 | + | |
| 556 | + | |
| 557 | + | |
| 558 | + | |
494 | 559 | | |
| 560 | + | |
| 561 | + | |
495 | 562 | | |
496 | 563 | | |
497 | | - | |
498 | | - | |
499 | | - | |
| 564 | + | |
| 565 | + | |
500 | 566 | | |
501 | 567 | | |
502 | 568 | | |
503 | 569 | | |
504 | 570 | | |
505 | 571 | | |
506 | | - | |
507 | | - | |
| 572 | + | |
| 573 | + | |
508 | 574 | | |
| 575 | + | |
| 576 | + | |
509 | 577 | | |
510 | 578 | | |
511 | | - | |
512 | | - | |
513 | | - | |
| 579 | + | |
| 580 | + | |
514 | 581 | | |
515 | 582 | | |
516 | 583 | | |
| |||
526 | 593 | | |
527 | 594 | | |
528 | 595 | | |
529 | | - | |
530 | | - | |
| 596 | + | |
531 | 597 | | |
532 | 598 | | |
533 | 599 | | |
| |||
546 | 612 | | |
547 | 613 | | |
548 | 614 | | |
549 | | - | |
550 | | - | |
| 615 | + | |
551 | 616 | | |
552 | 617 | | |
553 | 618 | | |
| |||
0 commit comments