fix(agents): reject bind specs with extra colon segments by ly-wang19 · Pull Request #95572 · openclaw/openclaw

ly-wang19 · 2026-06-21T14:42:00Z

Summary

openclaw agents bind --bind <spec> silently mis-parsed any spec carrying an extra colon segment. parseBindingSpecs (src/commands/agents.bindings.ts) split each spec with split(":", 2), which truncates: matrix:work:extra parsed as ["matrix", "work"], the :extra was dropped, and a binding was created for account work with no error reported — the user asked to bind one thing and silently got another.

Account ids can never contain : — they are constrained by VALID_ID_RE (/^[a-z0-9][a-z0-9_-]{0,63}$/i, src/routing/account-id.ts:9) — so a third colon segment is always a malformed spec, not an account id to truncate to.

Changes

src/commands/agents.bindings.ts — split the spec fully (split(":") capturing ...extraSegments); if any extra segment is present, push an Invalid binding ... Account id cannot contain ":" error and skip, matching the existing empty-account guard's shape. Check ordering is unknown-channel → extra-segments → empty-account → resolve.
src/commands/agents.bind.matrix.integration.test.ts — regression test that matrix:work:extra yields the extra-segments error with no binding, plus a control that matrix:work still parses.

This is the correct owner boundary: parseBindingSpecs is the only place that owns splitting <channel>:<account>; account-id.ts is a coercing normalizer (never rejects); and there is no VALID_ID_RE validation later on the bind write path, so a malformed id would otherwise persist to openclaw.json and later canonicalize to a different account (work:extra → work-extra). Rejecting at parse time is strictly safer than truncating or rejoining.

Real behavior proof

Behavior addressed: agents bind --bind matrix:work:extra silently dropped :extra and bound account work with no error (split(":", 2) truncation). It now reports Invalid binding "matrix:work:extra". Account id cannot contain ":". Use <channel>:<account>, for example telegram:default. and creates no binding. Valid specs are unchanged.

Real environment tested: Node 22.22.1, macOS, no model / network / device. A ./node_modules/.bin/tsx harness imports the real parseBindingSpecs, registers a matrix channel in the plugin registry, and calls it with matrix:work:extra, matrix:work, and matrix. BEFORE is origin/main's parseBindingSpecs (swapped in via git show, no other change).

Exact steps or command run after this patch:

# harness imports the real parseBindingSpecs + sets up the channel registry
./node_modules/.bin/tsx _bind-proof.mts

Evidence after fix:

BEFORE (origin/main):
  spec="matrix:work:extra" -> bindings=[{...accountId:"work"}] errors=[]    # silent wrong bind
  spec="matrix:work"       -> bindings=[{...accountId:"work"}] errors=[]
  spec="matrix"            -> bindings=[{...accountId:"main"}] errors=[]
AFTER (this branch):
  spec="matrix:work:extra" -> bindings=[] errors=["Invalid binding ... Account id cannot contain ':' ..."]
  spec="matrix:work"       -> bindings=[{...accountId:"work"}] errors=[]    # unchanged
  spec="matrix"            -> bindings=[{...accountId:"main"}] errors=[]    # unchanged

Observed result after fix: the malformed 3-segment spec is rejected with an explicit error and produces no binding; matrix:work and matrix are byte-identical before and after. Single trailing colon (telegram:) still hits the pre-existing empty-account error (unchanged).

What was not tested: a full openclaw agents bind run against a built CLI (the harness drives the real parseBindingSpecs, which is exactly what the command calls to turn --bind args into route bindings). The new regression test in agents.bind.matrix.integration.test.ts fails on origin/main and passes after this change.

Additional checks

node scripts/run-vitest.mjs src/commands/agents.bind.matrix.integration.test.ts — 3 pass with the fix; verified fail-before by swapping in origin/main's agents.bindings.ts → the new test fails.
Formatter (oxfmt), static analysis (oxlint --tsconfig), and tsgo:core pass for the changed files. Diff: +9/−1 prod, +26 test.

clawsweeper · 2026-06-21T14:43:36Z

Codex review: passed. Reviewed June 21, 2026, 11:06 PM ET / 03:06 UTC.

Summary
The PR changes parseBindingSpecs to reject <channel>:<account>:... bind specs and adds Matrix parser regression coverage for the malformed and valid forms.

PR surface: Source +8, Tests +26. Total +34 across 2 files.

Reproducibility: yes. Source inspection of current main shows parseBindingSpecs truncating with split(":", 2), and the PR body provides before/after terminal output from a harness importing the real parser.

Review metrics: 1 noteworthy metric.

CLI validation behavior: 1 malformed bind-spec class rejected. Extra-colon --bind specs change from silent truncation to an explicit parser error, which maintainers should notice before merge.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

[P1] Existing scripts or operator snippets that accidentally pass an extra colon in openclaw agents ... --bind will now fail with an explicit error instead of silently creating a truncated binding; that is safer, but it is still a CLI compatibility behavior change.

Maintainer options:

Accept malformed-spec rejection (recommended)
Land the PR as-is because documented bind specs do not allow colon-containing account ids and the current behavior silently binds the wrong account.
Require a compatibility path
If maintainers want to preserve lenience for malformed scripts, ask for a deprecation or release-note plan before changing parser behavior.

Next step before merge

[P2] No repair job is needed; this PR is already in the automerge lane and the remaining action is exact-head gates plus maintainer acceptance of the compatibility tradeoff.

Security
Cleared: The diff only changes CLI parser logic and Vitest coverage; it does not touch workflows, dependencies, lockfiles, secrets, package execution, or supply-chain surfaces.

Review details

Best possible solution:

Merge the shared parser rejection and regression coverage once maintainers accept the malformed-spec fail-closed behavior and exact-head gates pass.

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

Yes. Source inspection of current main shows parseBindingSpecs truncating with split(":", 2), and the PR body provides before/after terminal output from a harness importing the real parser.

Is this the best way to solve the issue?

Yes. The shared parseBindingSpecs function is the narrow owner because all affected agent bind entry points consume it before config writes; later validation would duplicate command logic or allow malformed account ids into config.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against a1828110704f.

Label changes

Label changes:

add merge-risk: 🚨 compatibility: The PR intentionally makes malformed extra-colon --bind values fail closed instead of preserving the previous silently truncated behavior.

Label justifications:

P2: This is a focused CLI parsing bug with limited blast radius and clear regression coverage.
merge-risk: 🚨 compatibility: The PR intentionally makes malformed extra-colon --bind values fail closed instead of preserving the previous silently truncated behavior.
rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
status: 🚀 automerge armed: This PR is in ClawSweeper's automerge lane. Sufficient (terminal): The PR body includes copied before/after terminal output from a harness importing the real parser and showing the malformed spec rejected while valid specs remain unchanged.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes copied before/after terminal output from a harness importing the real parser and showing the malformed spec rejected while valid specs remain unchanged.

Evidence reviewed

PR surface:

Source +8, Tests +26. Total +34 across 2 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	9	1	+8
Tests	1	26	0	+26
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	2	35	1	+34

What I checked:

Current-main truncation: Current main still uses trimmed.split(":", 2) inside parseBindingSpecs, so an input like matrix:work:extra is parsed as channel matrix and account work before any error can be emitted. (src/commands/agents.bindings.ts:319, a1828110704f)
PR head parser behavior: PR head fully splits the bind spec, checks extraSegments.length, emits an explicit invalid-binding error, and continues without producing a binding. (src/commands/agents.bindings.ts:319, 3e1af31fc4da)
Shared caller boundary: agents bind and agents unbind both call parseBindingSpecs through resolveParsedBindingsOrExit, and parser errors exit before config mutation. (src/commands/agents.commands.bind.ts:127, a1828110704f)
Agent add uses same parser: The non-interactive agents add --bind path calls parseBindingSpecs and throws parser errors before applying route bindings. (src/commands/agents.commands.add.ts:187, a1828110704f)
Documented bind syntax: The user-facing CLI docs document only <channel>:*, <channel>:<account>, and <channel> forms, with no colon-containing account syntax. Public docs: docs/cli/agents.md. (docs/cli/agents.md:64, a1828110704f)
Account-id contract: VALID_ID_RE accepts only alphanumeric, underscore, and dash account ids, while invalid characters are canonicalized rather than preserved as account-id syntax. (src/routing/account-id.ts:7, a1828110704f)

Likely related people:

gumadeiras: Merged PR Agents: add account-scoped bind and routing commands #27195 introduced account-scoped bind commands, the shared parser surface, and the relevant docs/tests. (role: feature introducer; confidence: high; commits: 96c77025263d; files: src/commands/agents.bindings.ts, src/commands/agents.commands.bind.ts, src/commands/agents.commands.add.ts)
steipete: History shows command splitting and later command-test cleanup around the same agent command modules. (role: recent area contributor; confidence: medium; commits: a58ff1ac6347, 310b5e4f6a22; files: src/commands/agents.bindings.ts, src/commands/agents.commands.add.ts, src/commands/agents.commands.bind.ts)
Takhoffman: Recent history includes a persisted agent binding key fix in the same parser module, adjacent to binding identity behavior. (role: adjacent behavior contributor; confidence: medium; commits: 187449d14951; files: src/commands/agents.bindings.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works

ClawSweeper keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

NianJiuZst

Detailed review on top of ClawSweeper's diamond-lobster verdict (which is right — this is a narrow, well-tested fix).

The split(":", 2) → split(":") change is correct. Old code silently dropped extra segments ("matrix:work:extra" → ("matrix", "work")), which is exactly the kind of "looks like it worked, silently lost data" bug that bites users months later. New code rejects explicitly with a clear error message.

Error message consistency check. The error string starts with 'Invalid binding "..."'. Worth a quick grep across src/commands/agents.bindings.ts to confirm this matches the convention for other rejection paths. If other rejections use a different prefix ('Error:', 'Cannot parse:', etc.), this one will feel inconsistent in CLI output.

Telemetry/log gap. A rejected bind spec is currently a no-op except for printing the error. If users hit this in the wild (legitimate typo, or a workaround they were relying on), there's no signal that why their bind is failing beyond the one error line. Consider whether the parser should also log the count of rejected specs at the end of parseBindingSpecs so users see "5 valid, 2 invalid, run openclaw agents list-bindings to inspect" rather than just an error stream.

Migration risk. The old behavior silently accepted extra colons. If any user has a working bind spec with extra colons (maybe in a config file they paste from a doc or template), they'll suddenly start getting this error after upgrading. Worth either:

Documenting the breaking change in CHANGELOG.md / release notes.
Adding a one-time migration helper that detects "specs with extra colons that would now fail" and warns users to fix them.

Option 2 is overkill for a parse fix, but option 1 (a single release note line) is cheap insurance.

Test coverage. The new tests cover the positive and negative cases. One edge case worth a test: "matrix:work:" (trailing colon). The current split(":") returns ("matrix", "work", "") — extraSegments would be [""] (length 1), so it would be rejected. Worth pinning the behavior with a test that asserts the trailing-colon case is rejected, not silently accepted.

Diamond lobster maintsream note. With CS + my review both diamond-lobster-clean, this is ready for maintainer review as soon as required CI checks finish. The "Risk before merge: required CI queued" line in CS's review is just a status note, not a blocker.

Takhoffman · 2026-06-21T18:01:27Z

@clawsweeper automerge

clawsweeper · 2026-06-21T18:01:30Z

🦞✅
ClawSweeper merged this PR after the passing review.

Source: clawsweeper[bot]
Feedback: structured ClawSweeper verdict: pass (sha=3e1af31fc4da24772443f85a384f05c2136278cd)
Merge status: merged by ClawSweeper automerge
Merged at: 2026-06-22T03:07:51Z
Merge commit: 540ec53f9931

What merged:

The PR changes parseBindingSpecs to reject <channel>:<account>:... bind specs and adds Matrix parser regression coverage for the malformed and valid forms.
PR surface: Source +8, Tests +26. Total +34 across 2 files.
Reproducibility: yes. Source inspection of current main shows parseBindingSpecs truncating with split(":", 2), and the PR body provides before/after terminal output from a harness importing the real parser.

Automerge notes:

PR branch already contained follow-up commit before automerge: fix(agents): reject bind specs with extra colon segments

The automerge loop is complete.

Automerge progress:

2026-06-21 18:01:27 UTC review queued 7faf2cdb9523 (queued)

2026-06-21 22:44:39 UTC review passed 7faf2cdb9523 (structured ClawSweeper verdict: pass (sha=7faf2cdb95230e2f0529f44d4fa46f9765c6b...)

2026-06-21 18:08:07 UTC repair queued 7faf2cdb9523 (autonomous) Run: https://github.com/openclaw/clawsweeper/actions/runs/27913030851

2026-06-22 01:32:23 UTC review queued 7faf2cdb9523 (queued)

2026-06-21 21:25:47 UTC repair queued 7faf2cdb9523 (autonomous) Run: https://github.com/openclaw/clawsweeper/actions/runs/27917958447

2026-06-21 22:37:39 UTC repair queued 7faf2cdb9523 (autonomous) Run: https://github.com/openclaw/clawsweeper/actions/runs/27919709188

2026-06-21 23:07:10 UTC repair queued 7faf2cdb9523 (autonomous) Run: https://github.com/openclaw/clawsweeper/actions/runs/27920409071

2026-06-21 23:44:33 UTC repair queued 7faf2cdb9523 (autonomous) Run: https://github.com/openclaw/clawsweeper/actions/runs/27921306009

2026-06-22 00:15:32 UTC repair queued 7faf2cdb9523 (autonomous) Run: https://github.com/openclaw/clawsweeper/actions/runs/27922155204

2026-06-22 01:35:24 UTC repair queued 7faf2cdb9523 (autonomous) Run: https://github.com/openclaw/clawsweeper/actions/runs/27924254307

2026-06-22 03:07:39 UTC review passed 3e1af31fc4da (structured ClawSweeper verdict: pass (sha=3e1af31fc4da24772443f85a384f05c213627...)

2026-06-22 03:07:53 UTC merged 3e1af31fc4da (merged by ClawSweeper automerge)

`parseBindingSpecs` split each `--bind <spec>` with `split(":", 2)`, which truncates: `matrix:work:extra` parsed as `["matrix","work"]`, silently dropping `:extra` and binding to account `work` with no error. Account ids never contain `:` (VALID_ID_RE in routing/account-id), so a third segment is always a malformed spec, not an account id to truncate to. Split fully and reject a spec that carries extra segments, matching the existing empty-account error shape. `matrix:work` and `matrix` are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

vincentkoc · 2026-06-22T03:04:53Z

Clownfish 🐠 reef update

Thanks for the contribution here. Clownfish was able to repair this branch in place, which keeps the original PR as the clean canonical path.

Source PR: #95572
Validation: pnpm test src/commands/agents.bind.matrix.integration.test.ts; pnpm check:changed
The contributor trail stays intact here: branch history, PR context, and changelog credit all still point back to this work.

fish notes: model gpt-5.5, reasoning medium; reviewed against 3e1af31.

) Summary: - The PR changes `parseBindingSpecs` to reject `<channel>:<account>:...` bind specs and adds Matrix parser regression coverage for the malformed and valid forms. - PR surface: Source +8, Tests +26. Total +34 across 2 files. - Reproducibility: yes. Source inspection of current main shows `parseBindingSpecs` truncating with `split(":", 2)`, and the PR body provides before/after terminal output from a harness importing the real parser. Automerge notes: - PR branch already contained follow-up commit before automerge: fix(agents): reject bind specs with extra colon segments Validation: - ClawSweeper review passed for head 3e1af31. - Required merge gates passed before the squash merge. Prepared head SHA: 3e1af31 Review: openclaw#95572 (comment) Co-authored-by: ly-wang19 <ly-wang19@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: openclaw-clownfish[bot] <280122609+openclaw-clownfish[bot]@users.noreply.github.com>

openclaw-barnacle Bot added commands Command implementations size: XS labels Jun 21, 2026

NianJiuZst reviewed Jun 21, 2026

View reviewed changes

Comment thread src/commands/agents.bindings.ts Outdated

Comment thread src/commands/agents.bindings.ts Outdated

ly-wang19 force-pushed the fix/agents-bind-spec-extra-segments branch from 504a980 to 7faf2cd Compare June 21, 2026 16:29

vincentkoc force-pushed the fix/agents-bind-spec-extra-segments branch from 7faf2cd to 90061fb Compare June 22, 2026 03:01

fix(agents): reject bind specs with extra colon segments

3e1af31

clawsweeper Bot merged commit 540ec53 into openclaw:main Jun 22, 2026
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(agents): reject bind specs with extra colon segments#95572

fix(agents): reject bind specs with extra colon segments#95572
clawsweeper[bot] merged 2 commits into
openclaw:mainfrom
ly-wang19:fix/agents-bind-spec-extra-segments

ly-wang19 commented Jun 21, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented Jun 21, 2026 •

edited

Loading

Uh oh!

NianJiuZst left a comment

Uh oh!

Uh oh!

Uh oh!

Takhoffman commented Jun 21, 2026

Uh oh!

clawsweeper Bot commented Jun 21, 2026 •

edited

Loading

Uh oh!

vincentkoc commented Jun 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Uh oh!

Conversation

ly-wang19 commented Jun 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Real behavior proof

Additional checks

Uh oh!

clawsweeper Bot commented Jun 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NianJiuZst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Takhoffman commented Jun 21, 2026

Uh oh!

clawsweeper Bot commented Jun 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vincentkoc commented Jun 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ly-wang19 commented Jun 21, 2026 •

edited

Loading

clawsweeper Bot commented Jun 21, 2026 •

edited

Loading

clawsweeper Bot commented Jun 21, 2026 •

edited

Loading