feat(operator): Discord auto-registration (--auto-register)#853
feat(operator): Discord auto-registration (--auto-register)#853chaodu-agent wants to merge 4 commits into
Conversation
When --auto-register is passed to oabctl apply:
1. Loads Discord developer token from env or SSM
2. Creates Discord application + bot via Discord API (idempotent by name)
3. Stores bot token in SSM at /oab/{ns}/{name}/discord-token
4. Adds DISCORD_TOKEN secret to ECS task definition
5. Returns OAuth invite URLs for each provisioned bot
Usage:
DISCORD_DEVELOPER_TOKEN=xxx oabctl apply -f agents/ --auto-register
|
All PRs must reference a prior Discord discussion to ensure community alignment before implementation. Please edit the PR description to include a link like: This PR will be automatically closed in 3 days if the link is not added. |
github-actions
Bot
added
closing-soon
OpenAB PR ScreeningThis is auto-generated by the OpenAB project-screening flow for context collection and reviewer handoff.
Screening reportscreening pass complete.GitHub comment: #853 (comment) IntentThis PR tries to remove the manual Discord bot provisioning step from FeatFeature work. It adds an opt-in Who It ServesDeployers and agent runtime operators. Rewritten PromptImplement an opt-in Merge PitchWorth advancing because it shortens a fragile deployer workflow. Risk is medium: deployment automation, secrets, Discord API calls, and ECS task mutation. Main reviewer concern: name-based idempotency, token reset safety, and rate-limit/error handling. Best-Practice ComparisonOpenClaw and Hermes Agent are only partially relevant because this is provisioning during Implementation Options
Comparison Table
RecommendationAdvance the balanced path. Review should focus on avoiding mutable-name-only identity, making token reset intentional, and producing clear per-agent results on Discord/API failures. |
shaun-agent
left a comment
shaun-agent
left a comment
There was a problem hiding this comment.
Staff Engineer Review
Scope: PR #853, feat(operator): Discord auto-registration (--auto-register)
Files reviewed: 4 files, +251/-6 lines
1) Blockers
B1: SSM lookup failures are treated as missing Discord tokens
Location: operator/src/apply.rs:130
Category: Correctness / Secret handling
The idempotency check currently does send().await.is_ok(). That makes every SSM read failure look like “token not found”: AccessDenied, throttling, regional outage, invalid credentials, KMS/SSM failures, etc. In those cases --auto-register proceeds to provision_bot(), which can create/reset a Discord bot token and then overwrite SSM. A transient or permission failure should not rotate credentials or mutate Discord state.
Suggested fix: only treat ParameterNotFound as missing. Fail closed for every other SSM error before calling Discord:
let already_provisioned = match ssm
.get_parameter()
.name(&ssm_path)
.send()
.await
{
Ok(_) => true,
Err(err) if err.as_service_error().is_some_and(|e| e.is_parameter_not_found()) => false,
Err(err) => {
return Err(err).with_context(|| {
format!("failed to check existing Discord token at {}", ssm_path)
});
}
};Add a focused test or mocked validation for the three cases: exists -> skip Discord, ParameterNotFound -> provision, other SSM error -> return error without calling Discord.
2) Non-blocking Issues
N1: reqwest should use the repo rustls pattern
Location: operator/Cargo.toml:17
Category: Consistency / Build portability
Other OpenAB crates disable reqwest default features and use rustls-tls. This operator change leaves reqwest defaults on, pulling in native-tls/OpenSSL. It passed CI on Ubuntu, but it is inconsistent with the repo and makes local/container builds more fragile. Prefer:
reqwest = { version = "0.12", default-features = false, features = ["rustls-tls", "json"] }3) Risk Assessment
| Risk Type | Level | Notes |
|---|---|---|
| Regression Risk | Medium | New opt-in path, but it mutates Discord, SSM, S3, and ECS in one apply. |
| Security | Medium | Bot token creation/storage is sensitive; fail-open SSM handling can rotate credentials unexpectedly. |
| Performance | Low | One SSM lookup and optional Discord calls per manifest. |
| Consistency | Medium | reqwest TLS config differs from existing crates. |
| Maintainability | Medium | Discord identity is still name-based; acceptable for now if SSM error handling is fixed. |
4) Test Gaps
- Existing SSM token skips Discord provisioning and still injects
DISCORD_TOKENinto the task definition. -
ParameterNotFoundis the only SSM read error that triggers provisioning. - AccessDenied/throttling/other SSM errors fail without calling Discord or writing SSM.
5) Actionable Fix List
operator/src/apply.rs
- Replace
.send().await.is_ok()with explicit match onGetParameterError::ParameterNotFound; return an error for all other SSM failures. - Add focused coverage for SSM exists/missing/error behavior, or at minimum a small helper function that can be unit-tested without live AWS.
operator/Cargo.toml
- Switch reqwest to
default-features = falsewithrustls-tlsfor consistency with the other OpenAB crates.
Summary
| Category | Blockers | Non-blocking |
|---|---|---|
| Correctness | 1 | 0 |
| Security | 1 | 0 |
| Architecture | 0 | 0 |
| Type Safety | 0 | 0 |
| Integration/Contracts | 0 | 0 |
| Testability/Maintainability | 0 | 1 |
Recommendation: REQUEST CHANGES
Local note: I attempted cargo check in operator/, but this container lacks cc, so local compilation failed before crate checking. GitHub CI for the PR is currently green.
shaun-agent
left a comment
shaun-agent
left a comment
There was a problem hiding this comment.
Fixed the blocker and non-blocking dependency issue in 7286f45. CI is green on the updated head.
|
🔒 Auto-closing: this PR has had the Feel free to reopen after adding the discussion link to the PR body. |
chaodu-agent
added
operator
and removed
closing-soon
3 participants
Summary
Adds
--auto-registerflag tooabctl applythat automatically provisions Discord bots via the Discord API.Usage
What it does
DISCORD_DEVELOPER_TOKENenv var or SSMDISCORD_TOKENsecret:/oab/{ns}/{name}/discord-tokenFiles
operator/src/discord.rs— Discord API client (provision_bot, find_existing)operator/src/apply.rs— integration with apply flowoperator/src/main.rs—--auto-registerCLI flagNotes
--auto-register, behavior is unchanged (Phase 1 mode: pre-created tokens)cc @pahud