feat(setup): verify Discord channel bindings against live server (closes #198)#199
feat(setup): verify Discord channel bindings against live server (closes #198)#199Yeachan-Heo merged 3 commits intodevfrom
Conversation
#198) Add a binding-verify layer so misbound repo→channel routes are caught before clawhip writes config. - new `clawhip config verify-bindings [--json]` audits every channel ID in routes/defaults/monitors against live Discord, reporting match, mismatch, not-found, forbidden, unauthorized, no-token, or transport errors. Exits non-zero on any failed binding for CI drift detection. - new `clawhip setup --bind REPO=CHANNEL_ID [--expect-name REPO=NAME]` resolves the channel via the Discord bot API, surfaces both id and live name (e.g. `bind: clawhip -> 1480171113253175356 (#clawhip-dev)`), writes `[[routes]]` with a `channel_name` hint, and refuses stale or mismatched bindings before save. - new optional `channel_name` hint field on `[[routes]]`, `[defaults]`, `[[monitors.git.repos]]`, and `[[monitors.tmux.sessions]]`. Advisory only, `skip_serializing_if = Option::is_none`, so existing configs load and round-trip unchanged. - `DiscordClient::lookup_channel` is a read-only probe: it does not touch the DLQ or circuit breaker, so binding checks can't degrade the dispatch circuit. Tested with 389 passing tests (17 binding_verify unit + 6 discord lookup mock-HTTP + 3 CLI parse tests added), clippy clean under `-D warnings`, and three end-to-end smoke tests against a mock Discord server (correct bind, name mismatch refused, stale 404 ID refused). Operator `--verify-bindings` audit confirmed mismatch and not-found verdicts block config writes. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ceb93e2b49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/main.rs
Outdated
| .filter_map(|pair| { | ||
| let (repo, name) = pair.split_once('=')?; | ||
| Some((repo.trim().to_string(), name.trim().to_string())) | ||
| }) |
There was a problem hiding this comment.
Reject malformed --expect-name entries
run_setup silently drops any --expect-name value that is not exactly repo=name because filter_map ignores pairs where split_once('=') fails. In practice, a typo like --expect-name repo:name disables the expected-name safety check for that binding, so the command can save a misbound route even though the operator explicitly asked for name verification.
Useful? React with 👍 / 👎.
| match status { | ||
| StatusCode::NOT_FOUND => ChannelLookup::NotFound, | ||
| StatusCode::FORBIDDEN => ChannelLookup::Forbidden, | ||
| StatusCode::UNAUTHORIZED => ChannelLookup::Unauthorized, | ||
| other => { |
There was a problem hiding this comment.
Retry channel lookups on Discord 429 responses
lookup_channel maps all non-401/403/404 responses to Transport and does not special-case 429 Too Many Requests, so setup --bind / config verify-bindings can fail on transient rate limits and report drift/errors even when bindings are valid. Since this feature may issue many /channels/{id} reads in a loop, it should honor Discord rate-limit responses (e.g., retry after retry_after) rather than failing immediately.
Useful? React with 👍 / 👎.
Formatting-only reflow after #198 binding-verify feature. No semantic changes — verified via cargo test (389 passing), cargo clippy --all-targets -- -D warnings (clean), and cargo fmt --check (clean). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3025a3679c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // tmux monitors | ||
| for (index, session) in config.monitors.tmux.sessions.iter().enumerate() { | ||
| if let Some(channel) = session.channel.as_deref() | ||
| && !channel.is_empty() | ||
| { |
There was a problem hiding this comment.
Audit workspace and cron channel IDs
collect_bindings only enumerates defaults, routes, git monitors, and tmux monitors, then returns, so config verify-bindings never checks [[monitors.workspace]] channel or [[cron.jobs]] channel targets. In configs that use workspace or cron delivery, deleted/forbidden channel IDs will be missed by the audit and only surface later as runtime delivery failures, which defeats the drift-detection guarantee advertised for this command.
Useful? React with 👍 / 👎.
| for route in &mut self.routes { | ||
| route.sink = normalize_text(Some(route.sink.clone())).unwrap_or_else(default_sink_name); |
There was a problem hiding this comment.
Normalize defaults channel-name hint
The new normalization pass handles channel_name for routes/git/tmux, but [defaults].channel_name is not normalized. If that field is present as empty/whitespace in TOML, it is kept as Some(""), then treated as a required expected name during verification, producing false mismatches for valid channels and causing verify-bindings (and setup --verify-bindings) to fail unnecessarily.
Useful? React with 👍 / 👎.
…view) The previous `--expect-name` parser used `filter_map` which silently dropped any entry without `=`, so `--expect-name clawhip` would just never enter the override map and the name-match guard in `setup --bind` would be bypassed without warning. An operator asking clawhip to enforce a channel name could have the assertion silently skipped. Extract `parse_expect_name_overrides` and make it hard-fail on: - entries without `=` (`"clawhip"`) - empty repo after trim (`"=dev"`, `" =dev"`) - empty name after trim (`"clawhip="`, `"clawhip= "`) - duplicate repo keys (prevents ambiguous overrides) When the operator asks us to enforce a name, we must either enforce it or refuse the command — never quietly drop the assertion. Adds 9 unit tests in `src/main.rs` covering the happy path, whitespace trimming, every malformed shape, and the empty-input case. The missing-equals test is explicitly marked as the #198 review regression. Verified locally: 391 tests passing (+9 from prior 382), `cargo clippy --all-targets -- -D warnings` clean, `cargo fmt --check` clean. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ab8436fb3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(expected) = expect_map.get(repo) { | ||
| let expected_clean = expected.trim().trim_start_matches('#'); |
There was a problem hiding this comment.
Reject unmatched --expect-name entries
The bind loop only checks --expect-name when expect_map.get(repo) succeeds, so any typoed repo key is silently ignored and the binding still gets written without the requested name guard. For example, --bind clawhip=... --expect-name clawihp=clawhip-dev will skip verification for clawhip instead of failing. This reintroduces a safety-check bypass path; please fail when any --expect-name key does not match a --bind repo.
Useful? React with 👍 / 👎.
Summary
clawhip config verify-bindings [--json]that walks every channel ID in routes/defaults/monitors and audits each against live Discord server state. Exits non-zero on drift so CI/operator scripts can catch stale or misbound routes.clawhip setup --bind REPO=CHANNEL_ID [--expect-name REPO=NAME]as an operator-facing bind flow that resolves the channel live, surfacesbind: oh-my-codex -> 1480171106324189335 (#omx-dev), writes a[[routes]]entry with achannel_namehint, and refuses stale / forbidden / name-mismatched bindings before writing config.channel_namefield on[[routes]],[defaults],[[monitors.git.repos]],[[monitors.tmux.sessions]]. Backward-compatible (skip_serializing_if = Option::is_none) — old configs load and round-trip unchanged.DiscordClient::lookup_channelis a read-only probe: no DLQ, no circuit breaker interaction, so audits cannot degrade the dispatch path.Closes #198.
Why
Per the issue, the current flow lets operators encode raw channel IDs without verifying they still belong to the intended channel. Today we hit a misbind live while editing routes under pressure (omc-dev / omx-dev / clawhip-dev are adjacent). This lands the minimum viable binding-verify layer: name/ID are both surfaced, the config write is blocked on drift, and there's a standalone audit command for drift detection after the fact.
Operator-facing flow
Test plan
cargo test→ 389 passing, 0 failing (17 newbinding_verifyunit tests + 6 newdiscord::lookup_channel_*mock-HTTP integration tests + 3 new CLI parse tests)cargo clippy --all-targets -- -D warnings→ clean--bindwith matching--expect-nameresolves, printsbind: clawhip -> 1480171113253175356 (#clawhip-dev), and writes a route withchannel_name = "clawhip-dev"--expect-nameis refused:bind clawhip: channel 1480171113253175356 live name is #clawhip-dev but --expect-name requires #wrong-name(exit 1)bind clawhip: channel 0000000000000 not found on Discord(exit 1)verify-bindingssmoke test exercised match / mismatch / not-found verdicts in both text and--jsonoutput; process exits non-zero on drift as expected.Backward compatibility
channel_nameis optional and skip-serialized whenNone. Existing configs without it load, validate, and round-trip unchanged.DiscordClient.🤖 Generated with Claude Code