[ops] Add scripts/ops/discord.py for one-way channel notifications#5229
[ops] Add scripts/ops/discord.py for one-way channel notifications#5229
Conversation
Webhook URLs resolve from DISCORD_WEBHOOK_<CHANNEL> env vars first, falling back to gcloud secret manager (marin-discord-webhook-<channel>) so the same script works locally and in GH Actions. Channels: internal-discuss, code-review.
|
Claude finished @rjpower's task in 4m 10s —— View job Code review
1 issue posted inline (scripts/ops/discord.py#L81-L83):
Other items considered and not flagged (not high-signal under the review bar):
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b234c02f1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| text=True, | ||
| stderr=subprocess.PIPE, | ||
| ).strip() | ||
| except subprocess.CalledProcessError as e: |
There was a problem hiding this comment.
When DISCORD_WEBHOOK_<CHANNEL> is unset, _webhook_url shells out to gcloud; however, only CalledProcessError is caught. On machines without the gcloud CLI installed, this raises FileNotFoundError and bypasses the intended contextual RuntimeError, so local ops runs fail with an unhelpful traceback instead of a clear remediation message.
Useful? React with 👍 / 👎.
| }, | ||
| method="POST", | ||
| ) | ||
| with urllib.request.urlopen(req, timeout=10) as resp: |
There was a problem hiding this comment.
urllib.request.urlopen raises HTTPError for 4xx/5xx before yielding a response object, so the resp.status >= 300 branch is skipped for real Discord failures. That means webhook errors return an uncaught HTTPError instead of the script’s structured RuntimeError path (with channel/body context), making failure handling inconsistent in production.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| with urllib.request.urlopen(req, timeout=10) as resp: | ||
| if resp.status >= 300: | ||
| raise RuntimeError(f"discord webhook {channel!r} returned {resp.status}: {resp.read()!r}") |
There was a problem hiding this comment.
The resp.status >= 300 branch is essentially dead code, and the corresponding test only passes because urlopen is mocked.
urllib.request.urlopen runs through the default OpenerDirector, whose HTTPDefaultErrorHandler raises urllib.error.HTTPError for any non-2xx response that no other handler claims (3xx codes that are not redirects, plus all 4xx/5xx). It will not return a response object whose .status is >= 300 — which means the docstring claim "Raises on non-2xx response" is honored only by HTTPError propagating from urlopen(...), not by the RuntimeError you construct here.
Concretely:
- In production, a 4xx/5xx from Discord raises
urllib.error.HTTPErrorfrom line 81, never reaches lines 82-83, and surfaces to callers asHTTPErrorrather than the formattedRuntimeError. test_post_raises_on_http_errorsets upfake_resp.status = 500and assertsRuntimeError, but realurlopenwould never hand a 500 back as a response — it would raise. The test exercises a code path that cannot happen.
Two reasonable fixes (pick one — not a one-line suggestion):
- Catch
urllib.error.HTTPErroraroundurlopenand re-raise asRuntimeError(f"discord webhook {channel!r} returned {e.code}: {e.read()!r}"). Then update the test to makeurlopenraiseHTTPErrorinstead of returning a fake response, and drop the now-unreachableif resp.status >= 300block. - Or just delete the dead branch and let
HTTPErrorpropagate; update the test and thepost()docstring to match.
2 participants
Webhook URLs resolve from DISCORD_WEBHOOK_ env vars first, falling back to gcloud secret manager (marin-discord-webhook-) so the same script works locally and in GH Actions. Channels: internal-discuss, code-review.