fix(webhook): surface push-notification delivery path at INFO (closes #61)#62
Merged
fix(webhook): surface push-notification delivery path at INFO (closes #61)#62
Conversation
…61) Quinn reportedly accepts `tasks/pushNotificationConfig/set` but never POSTs to the registered callback URL. Investigation found the actual delivery path is likely firing, but nothing in docker logs could confirm it: Python's default root log level is WARNING, and `server.py` never called `logging.basicConfig`, so every `logger.info` in the A2A handler — including "push config registered" and "webhook delivered" — was silently filtered. Looking at the logs directly with log-level instrumentation showed the `opentelemetry.context` logger at WARNING, while the `a2a_handler` logger was NOTSET (inherits root = WARNING). Only WARNING+ lines ever made it through, so we only saw the 2 "webhook request error" warnings from a real DNS blip at 02:16 UTC; everything else — success, registration, late-binding branch, the whole normal path — ran silent. Fix: three changes, scoped to observability only (no behavior change): 1. `server.py` — install `logging.basicConfig(level=INFO)` with a formatter at module import. Operators can tune via `LOG_LEVEL` env var. INFO is the right default for a production agent. 2. `a2a_handler.py::_deliver_webhook` — upgrade successful delivery log from DEBUG → INFO, and include task id + final state + URL in the message so docker-logs grep works naturally. 3. `a2a_handler.py` JSON-RPC `pushNotificationConfig/set` handler — add the matching "push config registered (jsonrpc)" INFO line the REST alias has had since PR #19. Both paths now surface identically. Regression test: `test_webhook_success_is_logged_at_info_level` asserts a successful delivery produces an INFO record — this is the specific visibility gap that masked #61 for a full day of polling fallback. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughChanged logging setup and webhook/push-notification logging in the A2A handler; JSON-RPC Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@a2a_handler.py`:
- Around line 663-665: The log statement in a2a_handler that prints
push_config.url (used with record.id, record.state, resp.status_code) exposes
full webhook URLs and may leak secrets; replace direct URL logging with a
sanitized form before calling processLogger.info: implement or reuse a helper
(e.g., sanitize_url or mask_sensitive_url) that strips userinfo and query
parameters (or replaces them with placeholders) and use that sanitized value in
the "[a2a] webhook delivered..." log; apply the same fix to the other identical
logging site referenced (the second occurrence around the push/response
logging).
In `@server.py`:
- Around line 33-35: The LOG_LEVEL environment value should be validated before
calling logging.basicConfig to avoid a ValueError at import; read
os.environ.get("LOG_LEVEL", "INFO"), upper-case it, use logging.getLevelName()
to check whether it maps to a numeric level (valid), and if not fall back to
"INFO" (and optionally log a warning) and then pass the validated string to
logging.basicConfig (referencing logging.basicConfig and the LOG_LEVEL env
lookup).
In `@tests/test_a2a_handler.py`:
- Around line 686-690: The test currently only checks the "webhook delivered"
prefix, which lets regressions in task/state/URL slip through; update the
assertion that examines info_records (from caplog.records at logging.INFO) to
assert that at least one record's r.getMessage() contains the full required
fields: the task identifier, the state value, and the target URL (in addition to
"webhook delivered"). Locate the check that builds info_records and replace the
broad any("webhook delivered" in r.getMessage()) with an explicit check that
parses or searches r.getMessage() for the task id, state, and URL substrings (or
compares against the exact expected message) so the test fails if any of those
fields are missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 19dce1ce-a6eb-4a47-b445-735d4ae4b9b0
📒 Files selected for processing (3)
a2a_handler.pyserver.pytests/test_a2a_handler.py
Comment on lines
+663
to
+665
| "[a2a] webhook delivered task=%s state=%s → %s (%s)", | ||
| record.id, record.state, push_config.url, resp.status_code, | ||
| ) |
There was a problem hiding this comment.
Avoid logging full webhook URLs at INFO (secret leakage risk).
These logs include full callback URLs. If operators use query tokens or userinfo in URLs, secrets get persisted in logs.
Proposed fix (sanitize URL before logging)
+from urllib.parse import urlsplit, urlunsplit
+
+def _log_safe_url(raw_url: str) -> str:
+ try:
+ parsed = urlsplit(raw_url)
+ host = parsed.hostname or ""
+ port = f":{parsed.port}" if parsed.port else ""
+ netloc = f"{host}{port}"
+ # Drop query/fragment and any userinfo
+ return urlunsplit((parsed.scheme, netloc, parsed.path, "", ""))
+ except Exception:
+ return "<invalid-url>"
+
@@
- logger.info(
- "[a2a] webhook delivered task=%s state=%s → %s (%s)",
- record.id, record.state, push_config.url, resp.status_code,
- )
+ logger.info(
+ "[a2a] webhook delivered task=%s state=%s → %s (%s)",
+ record.id, record.state, _log_safe_url(push_config.url), resp.status_code,
+ )
@@
- logger.info(
- "[a2a] push config registered (jsonrpc) task=%s state=%s → %s",
- task_id, record.state, cfg.url,
- )
+ logger.info(
+ "[a2a] push config registered (jsonrpc) task=%s state=%s → %s",
+ task_id, record.state, _log_safe_url(cfg.url),
+ )Also applies to: 1215-1217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@a2a_handler.py` around lines 663 - 665, The log statement in a2a_handler that
prints push_config.url (used with record.id, record.state, resp.status_code)
exposes full webhook URLs and may leak secrets; replace direct URL logging with
a sanitized form before calling processLogger.info: implement or reuse a helper
(e.g., sanitize_url or mask_sensitive_url) that strips userinfo and query
parameters (or replaces them with placeholders) and use that sanitized value in
the "[a2a] webhook delivered..." log; apply the same fix to the other identical
logging site referenced (the second occurrence around the push/response
logging).
Comment on lines
+33
to
+35
| logging.basicConfig( | ||
| level=os.environ.get("LOG_LEVEL", "INFO").upper(), | ||
| format="%(asctime)s %(levelname)s %(name)s %(message)s", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify current usage in repo
rg -n 'LOG_LEVEL|basicConfig\(' server.py
# Verify stdlib behavior for valid/invalid level names
python - <<'PY'
import logging
for value in ["INFO", "DEBUG", "INF0", "NOT_A_LEVEL"]:
try:
logging.basicConfig(level=value, force=True)
print(f"{value}: OK")
except Exception as e:
print(f"{value}: ERROR -> {type(e).__name__}: {e}")
PYRepository: protoLabsAI/quinn
Length of output: 351
Validate LOG_LEVEL before passing it to basicConfig.
At line 34, an invalid env value (e.g., LOG_LEVEL=INF0) raises ValueError and prevents server startup at import time. The logging.basicConfig() function does not gracefully handle invalid level names.
Proposed fix
+_raw_log_level = os.environ.get("LOG_LEVEL", "INFO")
+_resolved_log_level = getattr(logging, _raw_log_level.upper(), None)
+if not isinstance(_resolved_log_level, int):
+ _resolved_log_level = logging.INFO
+
logging.basicConfig(
- level=os.environ.get("LOG_LEVEL", "INFO").upper(),
+ level=_resolved_log_level,
format="%(asctime)s %(levelname)s %(name)s %(message)s",
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server.py` around lines 33 - 35, The LOG_LEVEL environment value should be
validated before calling logging.basicConfig to avoid a ValueError at import;
read os.environ.get("LOG_LEVEL", "INFO"), upper-case it, use
logging.getLevelName() to check whether it maps to a numeric level (valid), and
if not fall back to "INFO" (and optionally log a warning) and then pass the
validated string to logging.basicConfig (referencing logging.basicConfig and the
LOG_LEVEL env lookup).
Comment on lines
+686
to
+690
| info_records = [r for r in caplog.records if r.levelno == logging.INFO] | ||
| assert any("webhook delivered" in r.getMessage() for r in info_records), ( | ||
| "successful delivery must log at INFO, not DEBUG — otherwise the " | ||
| "default WARNING log level silently hides whether delivery is working" | ||
| ) |
There was a problem hiding this comment.
Lock the full log contract, not just the prefix.
This test currently passes even if task/state/URL details regress. Please assert those required fields explicitly.
Proposed test hardening
- info_records = [r for r in caplog.records if r.levelno == logging.INFO]
- assert any("webhook delivered" in r.getMessage() for r in info_records), (
+ info_messages = [r.getMessage() for r in caplog.records if r.levelno == logging.INFO]
+ assert any(
+ "webhook delivered" in m
+ and "task=test-task-id" in m
+ and f"state={COMPLETED}" in m
+ and "https://example.com/hook" in m
+ for m in info_messages
+ ), (
"successful delivery must log at INFO, not DEBUG — otherwise the "
"default WARNING log level silently hides whether delivery is working"
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_a2a_handler.py` around lines 686 - 690, The test currently only
checks the "webhook delivered" prefix, which lets regressions in task/state/URL
slip through; update the assertion that examines info_records (from
caplog.records at logging.INFO) to assert that at least one record's
r.getMessage() contains the full required fields: the task identifier, the state
value, and the target URL (in addition to "webhook delivered"). Locate the check
that builds info_records and replace the broad any("webhook delivered" in
r.getMessage()) with an explicit check that parses or searches r.getMessage()
for the task id, state, and URL substrings (or compares against the exact
expected message) so the test fails if any of those fields are missing.
Spec audit of Quinn against the A2A streaming guide (quinn#61 follow- up) found that the JSON-RPC `message/send` handler built its result dict inline at lines 1124-1129 — every other path that returns a Task (SSE initial frame, REST alias, `tasks/get`, `tasks/resubscribe`) uses `_task_to_response(record)` which correctly sets `kind: "task"`. The inline build omitted it. `@a2a-js/sdk`'s client routes results by the `kind` discriminator; missing it means the SDK may treat the response as a `Message` instead of a Task, which is the same class of bug that Quinn #40 fixed on the streaming side. Fix: replace the inline dict with `_task_to_response(record)` so every Task snapshot goes through a single builder. Updated test_message_send_returns_submitted to assert the kind discriminator + contextId are present on the result. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
a2a_handler.py (1)
663-665:⚠️ Potential issue | 🟠 MajorDo not log raw webhook URLs at INFO.
Both log lines persist full callback URLs; this can leak credentials/query tokens into logs. Log a sanitized URL form instead.
Proposed sanitization patch
+from urllib.parse import urlsplit, urlunsplit + +def _log_safe_url(raw_url: str) -> str: + try: + p = urlsplit(raw_url) + host = p.hostname or "" + port = f":{p.port}" if p.port else "" + netloc = f"{host}{port}" + return urlunsplit((p.scheme, netloc, p.path, "", "")) + except Exception: + return "<invalid-url>" @@ - logger.info( - "[a2a] webhook delivered task=%s state=%s → %s (%s)", - record.id, record.state, push_config.url, resp.status_code, - ) + logger.info( + "[a2a] webhook delivered task=%s state=%s → %s (%s)", + record.id, record.state, _log_safe_url(push_config.url), resp.status_code, + ) @@ - logger.info( - "[a2a] push config registered (jsonrpc) task=%s state=%s → %s", - task_id, record.state, cfg.url, - ) + logger.info( + "[a2a] push config registered (jsonrpc) task=%s state=%s → %s", + task_id, record.state, _log_safe_url(cfg.url), + )Also applies to: 1215-1218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@a2a_handler.py` around lines 663 - 665, The INFO log currently prints raw webhook URLs (push_config.url) which can leak secrets; update the logging in the webhook delivery code (the log call using record.id, record.state, push_config.url, resp.status_code) to sanitize or redact push_config.url before logging (e.g., strip query parameters, replace sensitive query values with "[REDACTED]" or log only scheme+host+path). Apply the same sanitization to the other occurrence that logs push_config.url (the second block around the other webhook delivery log). Ensure you still include record.id, record.state and resp.status_code in the log but never emit the full raw URL at INFO.tests/test_a2a_handler.py (1)
686-690:⚠️ Potential issue | 🟡 MinorAssert the full INFO log contract, not just the prefix.
This still passes if task id/state/URL details regress; assert those required fields explicitly in the message match.
Suggested test hardening
- info_records = [r for r in caplog.records if r.levelno == logging.INFO] - assert any("webhook delivered" in r.getMessage() for r in info_records), ( + info_messages = [r.getMessage() for r in caplog.records if r.levelno == logging.INFO] + assert any( + "webhook delivered" in m + and "task=test-task-id" in m + and f"state={COMPLETED}" in m + and "https://example.com/hook" in m + for m in info_messages + ), ( "successful delivery must log at INFO, not DEBUG — otherwise the " "default WARNING log level silently hides whether delivery is working" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_a2a_handler.py` around lines 686 - 690, The test currently only checks that an INFO log contains the substring "webhook delivered" (using info_records and caplog), which allows regressions in required payload fields; update the assertion to assert the full INFO log contract by finding the INFO record whose message contains "webhook delivered" and then assert that it includes the expected task id, task state, and the webhook URL (use the test's task.id, task.state and the variable/fixture used for the URL) so the exact fields are present and match expected values rather than only the prefix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@a2a_handler.py`:
- Around line 663-665: The INFO log currently prints raw webhook URLs
(push_config.url) which can leak secrets; update the logging in the webhook
delivery code (the log call using record.id, record.state, push_config.url,
resp.status_code) to sanitize or redact push_config.url before logging (e.g.,
strip query parameters, replace sensitive query values with "[REDACTED]" or log
only scheme+host+path). Apply the same sanitization to the other occurrence that
logs push_config.url (the second block around the other webhook delivery log).
Ensure you still include record.id, record.state and resp.status_code in the log
but never emit the full raw URL at INFO.
In `@tests/test_a2a_handler.py`:
- Around line 686-690: The test currently only checks that an INFO log contains
the substring "webhook delivered" (using info_records and caplog), which allows
regressions in required payload fields; update the assertion to assert the full
INFO log contract by finding the INFO record whose message contains "webhook
delivered" and then assert that it includes the expected task id, task state,
and the webhook URL (use the test's task.id, task.state and the variable/fixture
used for the URL) so the exact fields are present and match expected values
rather than only the prefix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 44dd140b-801d-422e-be21-368597778bd7
📒 Files selected for processing (2)
a2a_handler.pytests/test_a2a_handler.py
mabry1985
pushed a commit
that referenced
this pull request
Apr 17, 2026
…61) Real root cause of push-notification failures against Workstacean. Visibility fix in #62 surfaced the actual error — Workstacean's callback endpoint was returning HTTP 401 "Invalid notification token", not the 404/silence we initially feared. The A2A spec's PushNotificationConfig allows two interchangeable shapes for the webhook bearer token: 1. Top-level `token` — the simple form @a2a-js/sdk serialises by default. Workstacean's SkillDispatcherPlugin uses this path: `setTaskPushNotificationConfig({ pushNotificationConfig: {url, token} })` (a2a-executor.ts:329-331). 2. Structured `authentication.credentials` — RFC-8821 AuthenticationInfo with `schemes` + `credentials`. Quinn only read shape #2. Workstacean's top-level token fell on the floor → stored `cfg.token = None` → `_deliver_webhook` skipped the `Authorization: Bearer <token>` header → Workstacean's callback handler found no bearer, computed `providedToken = ""`, mismatched the expected token, and rejected with 401 for every delivery. Live evidence from v0.1.8 logs: push config registered (jsonrpc) task=1167ad7f... → .../callback/1167ad7f... webhook delivered task=1167ad7f... state=completed → .../callback/1167ad7f... (401) Fix: factored token extraction into `_extract_push_token(cfg)` that checks top-level first, falls back to `authentication.credentials`, and returns None cleanly when neither is present. All three PushNotificationConfig parse sites (JSON-RPC set handler, REST alias handler, shared `_parse_push_config` at submit time) now route through it — no more divergent token parsing across the three entry points. 4 regression tests cover: top-level token, structured authentication, both-present-precedence, none-present-is-None. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 tasks
mabry1985
added a commit
that referenced
this pull request
Apr 17, 2026
…61) (#64) Real root cause of push-notification failures against Workstacean. Visibility fix in #62 surfaced the actual error — Workstacean's callback endpoint was returning HTTP 401 "Invalid notification token", not the 404/silence we initially feared. The A2A spec's PushNotificationConfig allows two interchangeable shapes for the webhook bearer token: 1. Top-level `token` — the simple form @a2a-js/sdk serialises by default. Workstacean's SkillDispatcherPlugin uses this path: `setTaskPushNotificationConfig({ pushNotificationConfig: {url, token} })` (a2a-executor.ts:329-331). 2. Structured `authentication.credentials` — RFC-8821 AuthenticationInfo with `schemes` + `credentials`. Quinn only read shape #2. Workstacean's top-level token fell on the floor → stored `cfg.token = None` → `_deliver_webhook` skipped the `Authorization: Bearer <token>` header → Workstacean's callback handler found no bearer, computed `providedToken = ""`, mismatched the expected token, and rejected with 401 for every delivery. Live evidence from v0.1.8 logs: push config registered (jsonrpc) task=1167ad7f... → .../callback/1167ad7f... webhook delivered task=1167ad7f... state=completed → .../callback/1167ad7f... (401) Fix: factored token extraction into `_extract_push_token(cfg)` that checks top-level first, falls back to `authentication.credentials`, and returns None cleanly when neither is present. All three PushNotificationConfig parse sites (JSON-RPC set handler, REST alias handler, shared `_parse_push_config` at submit time) now route through it — no more divergent token parsing across the three entry points. 4 regression tests cover: top-level token, structured authentication, both-present-precedence, none-present-is-None. Co-authored-by: GitHub CI <ci@example.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mabry1985
pushed a commit
that referenced
this pull request
Apr 17, 2026
Follow-up to #64 / #62. Two operator-visible tweaks in the streaming + push notifications section: - Call out that the token-parsing accepts both spec shapes (top-level `token` used by @a2a-js/sdk + Workstacean, and structured `authentication.credentials` for RFC-8821-style callers). - Document `LOG_LEVEL=INFO` (the default) as the switch that surfaces every push-config registration + webhook delivery attempt. Paired with workstacean docs that call out these as gold-standard requirements for every a2a agent.
This was referenced Apr 17, 2026
mabry1985
added a commit
that referenced
this pull request
Apr 19, 2026
Follow-up to #64 / #62. Two operator-visible tweaks in the streaming + push notifications section: - Call out that the token-parsing accepts both spec shapes (top-level `token` used by @a2a-js/sdk + Workstacean, and structured `authentication.credentials` for RFC-8821-style callers). - Document `LOG_LEVEL=INFO` (the default) as the switch that surfaces every push-config registration + webhook delivery attempt. Paired with workstacean docs that call out these as gold-standard requirements for every a2a agent. Co-authored-by: GitHub CI <ci@example.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Quinn #61 reports push-notification callbacks never delivered despite `tasks/pushNotificationConfig/set` accepting. Investigation found the delivery path is likely firing — but nothing in docker logs could confirm it.
Root cause
Python's default root log level is WARNING, and `server.py` never called `logging.basicConfig`. So every `logger.info(...)` in `a2a_handler.py` — including "push config registered" and "webhook delivered" — was silently filtered. The only `a2a_handler` lines visible in production were the WARNING/ERROR paths (5xx retries, DNS failures, terminal delivery-failed), which made the happy path literally invisible.
Evidence gathered live against the running `quinn` container:
```
$ docker exec quinn python -c "
import logging, a2a_handler as h
print('root:', logging.getLevelName(logging.getLogger().level))
print('a2a_handler:', logging.getLevelName(h.logger.level))
"
root: WARNING
a2a_handler: NOTSET
```
Only `logger.warning` and `logger.error` emissions ever reached stderr. The 2 warnings we did see in the last 24h were from a real transient DNS blip at 02:16 UTC; every other delivery ran silent.
Fix
Three observability-only changes (no behavior change):
Regression test `test_webhook_success_is_logged_at_info_level` asserts successful delivery emits an INFO record, so the visibility gap that masked this bug can't recur.
Closes
Closes #61
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests