Skip to content

feat: add /status command to verify Better Stack connectivity - #15

Merged
jangid merged 2 commits into
mainfrom
feature/status-command
Jul 5, 2026
Merged

feat: add /status command to verify Better Stack connectivity#15
jangid merged 2 commits into
mainfrom
feature/status-command

Conversation

@harshaalphafi

@harshaalphafi harshaalphafi commented May 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

Implements the /status command from #12. The original issue requested three commands — /start, /help, and /status. The first two were implemented in commit 9bc167a and the issue was closed, but /status was skipped as optional at the time.

  • /status makes a live GET to the Better Stack incidents API using the configured bearer token
  • A placeholder is sent immediately, then edited in-place with the result (same UX pattern as /alert)
  • Gated behind ALLOWED_USERS — unauthorized users are rejected before any API call is made
  • 2-minute cooldown per user via the existing cooldowns map (namespaced as status:<userId>)
  • On success: shows bot running, API connected (HTTP 200), policy ID, and allowlist state
  • On failure: shows bot running, API unreachable with the error detail, and a warning that alerts cannot be triggered

Behaviour

Connected (authorized user):

✅ ASIR Bot Status

🤖 Bot: Running
🌐 Better Stack API: Connected (HTTP 200)
📋 Policy ID: `113934`
🔒 Auth: Allowlist active (5 users)

API unreachable or token invalid:

❌ ASIR Bot Status

🤖 Bot: Running
🌐 Better Stack API: Unreachable (HTTP 401)

The bot cannot trigger alerts until connectivity is restored.

Test plan

  • npx tsc --noEmit passes
  • /status in Telegram returns connected message with correct HTTP 200
  • /status with an invalid BETTER_STACK_API_TOKEN returns the unreachable message with HTTP 401
  • Unauthorized user receives 🚫 Error: You are not authorized to use this command.
  • A second /status within 2 minutes returns the cooldown message with remaining seconds
  • /help now lists /status

Makes a live GET request to the Better Stack incidents API to confirm
the bot token is valid and the API is reachable. Returns HTTP status,
policy ID (restricted for unauthorized users), and allowlist state.
Also adds /status to /help command list.

Closes #12

@jangid jangid left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Clean, well-structured PR. Follows existing patterns (edit-in-place UX, same error handling style). A few issues worth flagging:

Issues

  1. /status leaks connectivity info to unauthorized users/alert blocks unauthorized users, but /status lets anyone check if the API is connected, see the allowlist size, and probe error details. Consider gating it behind the same ALLOWED_USERS check, or at least returning a minimal response for unauthorized users.

  2. PR body says "Closes #12" — Issue #12 was about /help and /start, which are already implemented and closed. This should reference a new issue or drop the Closes line.

  3. No rate limiting on /status/alert has a 2-minute cooldown but /status makes an authenticated API call to Better Stack on every invocation with no throttle. A user (or bot spam) could hammer it. Consider reusing the cooldown map or adding a lighter one.

Nits

  • ?per_page=1 fetches a real incident just to verify connectivity. Works fine, but a lighter-weight endpoint (if Better Stack has one) would be cleaner. Not a blocker.
  • axiosError.message ?? 'unknown error'message on a real Error is always defined, so 'unknown error' is effectively dead code. Harmless.

What looks good

  • Same edit-in-place UX pattern as /alert
  • Policy ID hidden from unauthorized users
  • Proper Markdown escaping on error detail
  • Structured logging with userId and status
  • /help updated to include the new command

- Block unauthorized users before making any API call (mirrors /alert)
- Add 2-minute cooldown per user via a namespaced key (status:<userId>)
  to prevent API hammering
- Remove now-redundant authorized check and policyLine conditional
- Remove unreachable dead code (axiosError.message ?? 'unknown error')
@harshaalphafi

Copy link
Copy Markdown
Collaborator Author

Thanks for the thorough review @jangid! Here's what was addressed in commit 00fed42:

1. /status leaks connectivity info to unauthorized users — Fixed
Added the same ALLOWED_USERS guard at the top of the /status handler, before any API call or reply is made. Unauthorized users now get the same 🚫 Error: You are not authorized to use this command. response and the check is logged via log.warn. Since only authorized users can now reach the handler, the conditional policyLine (that was hiding the policy ID for unauthorized users) was also removed — it's redundant now.

2. No rate limiting on /status — Fixed
Reused the existing cooldowns map with a namespaced key status:<userId> to avoid colliding with /alert entries. Same COOLDOWN_MS (2 minutes) applies, with the same remaining-seconds feedback message.

3. axiosError.message ?? 'unknown error' dead code — Fixed
Removed the unreachable 'unknown error' fallback. The escapeMarkdown call now uses detail ?? '' to satisfy TypeScript's type checker since message can technically be undefined in the inferred type.

Noted (not changed):

  • ?per_page=1 endpoint — kept as-is since Better Stack doesn't expose a dedicated ping/health endpoint in v3. Happy to swap it if one surfaces.
  • PR body Closes #12 — will update the PR description separately.

@jangid jangid left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed after the ~5-week pause. Thanks @harshaalphafi — your commit 00fed42 cleanly addresses all three substantive points from my earlier review:

  • Auth gating/status now blocks unauthorized users with the same ALLOWED_USERS guard before any API call, mirroring /alert. The redundant policyLine conditional was correctly dropped.
  • Rate limiting ✅ Reusing the cooldowns map with a namespaced status:<userId> key (no collision with /alert) is exactly right.
  • Dead-code fallback ✅ Removed.

The command logic is good to go from my side. Two small things keep me at comment rather than approve:

  1. CI is red, and part of that is this PR. I installed deps and ran npm run fmt:check against your HEAD (00fed42): bot.ts fails prettier --check. Most of it is pre-existing tree-wide debt — main itself has been red since CI landed in f6057c5, and eslint.config.mjs/README.md fail independently of this PR — but your new /status block does contribute one genuine violation: the authLine ternary needs reformatting. A quick npm run fmt && git commit on the new code clears your part. (Separately, someone should land a tree-wide prettier --write on main to get CI green again — happy to do that in its own PR so this one isn't blocked on unrelated debt.)

  2. Closes #12 is still in the commit body. You noted you'd fix the PR description — that's still pending, and the commit message references the wrong issue (#12 was /help+/start). Point it at the actual /status issue or drop the line.

Not abandoned in my view — just stalled. The work is essentially done. If you can run npm run fmt on the new code and fix the issue reference, I'm happy to approve. If priorities have shifted and this isn't worth finishing, let's close it rather than leave it open.

@jangid

jangid commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Automated review — PR stale since 2026-06-22

The /status logic itself is in good shape: auth gate before any API call, namespaced status:<userId> cooldown, Markdown-escaped error detail, and structured logging all check out. tsc --noEmit passes and I found no new substantive issues and no committed secrets/tokens in the diff.

The two items keeping this at comment are the same ones @jangid flagged on 2026-06-22 — both still unaddressed (HEAD is still 00fed42, no new commits since):

  1. Prettier violation in the new code (contributes to red CI). Verified locally against HEAD 00fed42: the new authLine ternary fails prettier --check. Prettier wants:

    const authLine =
      ALLOWED_USERS.length === 0
        ? '⚠️ *Auth:* No allowlist set — all users can trigger alerts'
        : `🔒 *Auth:* Allowlist active (${ALLOWED_USERS.length} user${ALLOWED_USERS.length === 1 ? '' : 's'})`;

    The rest of bot.ts's prettier failures are pre-existing tree-wide debt (main has been red since CI landed) and are not this PR's responsibility — a separate tree-wide prettier --write PR should clear those. Running npm run fmt and committing clears your part.

  2. Closes #12 in commit 2abdd89 body. The PR description was updated, but the commit message still carries Closes #12. Issue Bot only handles /alert — no /help or /start commands #12 covered /help+/start (already implemented and closed), so merging this would auto-close the wrong (already-closed) issue or reference the wrong work. Point it at the actual /status issue or drop the line.

Neither is a code-correctness problem — the feature is essentially done. Running npm run fmt on the new block and fixing the issue reference should be enough to get this over the line.

Automated review; no approval implied.

@jangid
jangid merged commit 92561a3 into main Jul 5, 2026
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants