Resolve bytes32 role names; stop losing reports and warnings - #355
Merged
Merged
Conversation
A task can exit 0 while logging a WARNING/ERROR for something it handled internally — a failed upload, a degraded data source, a skipped enrichment. That output only ever reached the DEBUG dump, so at the service's default LOG_LEVEL=INFO it was invisible in journald. This is not hypothetical: when Wavey Gist changed its API contract, the timelock-alerts task logged the 400 and exited 0, so `journalctl -u monitoring` showed only "task timelock-alerts ok in 249.6s". The broken integration ran unnoticed until someone read the Telegram alert body. WARNING/ERROR/CRITICAL lines from a passing task are now re-emitted at INFO, capped at 10 with a suppressed-count note so a chatty task can't flood the journal. The full-output DEBUG dump is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ures
Two gaps the legacy-payload breakage exposed:
- `raise_for_status()` discards the response body, so the only thing logged was
"400 Client Error ... for url". Wavey Gist had actually explained itself
("Legacy gist fields are no longer supported."), and that explanation never
reached the logs — the difference between a one-minute diagnosis and reading
the upload path to guess. Error bodies are now logged, capped at 300 chars.
- The upload used a bare `requests.post` with no retry, so a transient 5xx or
timeout dropped a report just as permanently as a contract change. It now
goes through `utils.http_client.request_with_retry`, which backs off on
5xx/timeouts and deliberately does not retry 4xx — a rejected payload still
fails fast rather than burning three attempts.
Also annotates the return as `str`, which mypy flagged once the module type-checked.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two changes to the AI explainer, both from the same postmortem.
**Role names.** The system prompt already promised that "a bytes32 the section
does NOT resolve stays unidentified", but nothing ever populated role
pre-images — so every `grantRole(bytes32,address)` was structurally guaranteed
to reach the model as an opaque digest, and severity was understated
accordingly. An InfiniFi LongTimelock batch granting unbounded iUSD mint and
burn to a new vault was summarized as "two roles ... whose names are not
resolvable from the calldata"; the model was following its instructions.
`utils/calldata/role_names.py` recovers the pre-image from two sources: a
static table of OpenZeppelin/common roles, and `keccak256("NAME")` constants
harvested from the target's verified source. The second costs no extra HTTP —
`fetch_source` is already memoized and disk-cached by the source-context fetch,
and returns every file of a multi-file contract concatenated, which is where
libraries like InfiniFi's `CoreRoles` live. Verified against the batch above:
both roles now resolve to RECEIPT_TOKEN_MINTER and RECEIPT_TOKEN_BURNER.
Only functions whose name mentions a role are inspected. Resolving every
bytes32 would label a timelock's all-zero `predecessor`/`salt` as
DEFAULT_ADMIN_ROLE — worse than leaving it unidentified — so that case is
covered by a test.
Resolved names feed both the prompt (a new Role Names section, with guidance
to judge severity by what the role permits) and the gist call flow, which now
renders the digest alongside its name rather than the digest alone.
**Unpublished reports.** The report exists only in memory while
`format_explanation_line` runs, so a failed upload destroyed it permanently and
recovery meant regenerating against a chain state that had since moved. Reports
that fail to upload are now spilled to `CACHE_DIR/unpublished-reports/`, making
a later recovery a re-upload instead of a reconstruction. The spill is
best-effort: if it fails the alert still goes out with its summary.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…impl Two defects found in review of the role-name resolver. The first made the feature a no-op on every real transaction. **Decoded bytes32 is raw bytes, not hex.** `decode_calldata` stores raw `eth_abi` output (`params = list(zip(param_types, decoded_values))`), so a real `grantRole` role argument arrives as 32 raw bytes. Both call sites stringified it, producing a Python repr (`b'aZh\x8d…'`) that `normalize_role_hash` then rejected — so the Role Names section was always empty and the report annotation never fired. Verified against the real calldata of tx 0xcfa148be…196e: `normalize_role_hash(str(value))` returned `""`. The unit tests passed because they hand-wrote string params, and the earlier end-to-end check fed hand-built strings rather than decoding calldata — so both exercised the resolver while skipping the extraction path that was broken. `normalize_role_hash` now takes `object` and handles bytes first, and both call sites pass the raw value. The collector test now builds its call with the real `decode_calldata`, and asserts the decoded param is `bytes` so the assumption can't silently regress. **Role constants live in the implementation, not the proxy.** For an upgradeable AccessControl contract, harvesting only the proxy's source finds nothing. `_candidate_sources` now falls back to the EIP-1967 implementation, mirroring `utils.source_context.get_source_context`. It is a generator, so a non-proxy target never pays the implementation-slot RPC — asserted by test. Verified end-to-end against mainnet: InfiniFiCore (non-proxy) resolves RECEIPT_TOKEN_MINTER/BURNER, and PortalHub (proxy) now resolves GOVERNOR, which is not in the static table and so could only have come from the implementation source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.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.
Three fixes from one postmortem. An InfiniFi LongTimelock batch (tx
0xcfa148be…196e, alert #901) wired a new Outland vault into core and granted it unbounded mint and burn of iUSD. The alert summarized that as "two roles … whose names are not resolvable from the calldata" and couldn't post its full report. Each of those was a separate gap.1. Resolve
bytes32role names (feat(llm))The system prompt already specified the behavior:
The model followed that exactly. Nothing in the codebase ever populated role pre-images, so every
grantRole(bytes32,address)was structurally guaranteed to come out unidentified, with severity understated to match.New
utils/calldata/role_names.pyrecovers the pre-image from:keccak256("NAME")constants harvested from the target's verified source.The second costs no extra HTTP:
fetch_sourceis already memoized and disk-cached by the source-context fetch, and returns every file of a multi-file contract concatenated — which is where libraries like InfiniFi'sCoreRoleslive. The source needed to resolve both roles was already sitting in/srv/cache/source-cache/when alert #901 was generated.Verified end-to-end against that transaction:
Only functions whose name mentions a role are inspected. Resolving every
bytes32would label a timelock's all-zeropredecessor/saltasDEFAULT_ADMIN_ROLE— worse than leaving it unidentified — so that's covered by a test.Resolved names feed both the prompt (new Role Names section, with guidance to judge severity by what the role actually permits) and the gist call flow, which now shows the name beside the digest rather than the digest alone.
2. Surface warnings from tasks that exit 0 (
fix(automation))runner.pylogged a passing task's stdout at DEBUG, and the service runsLOG_LEVEL=INFO. When Wavey Gist broke, the task logged the 400 and exited 0, so journald showed onlytask timelock-alerts ok in 249.6s— the broken integration ran unnoticed until someone read the Telegram body.WARNING/ERROR/CRITICAL lines from a passing task are now re-emitted at INFO, capped at 10 with a suppressed-count note. The DEBUG dump is unchanged.
3. Stop discarding diagnostics and reports (
fix(wavey-gist)+feat(llm))raise_for_status()threw away the response body, so logs showed400 Client Errorwhile Wavey Gist had actually said "Legacy gist fields are no longer supported." Error bodies are now logged (capped at 300 chars).requests.postwith no retry. It now usesutils.http_client.request_with_retry, which backs off on 5xx/timeouts and deliberately does not retry 4xx, so a rejected payload still fails fast.format_explanation_line, so a failed upload destroyed it permanently — which is why recovering #901 meant reconstructing rather than re-publishing. Failed reports now spill toCACHE_DIR/unpublished-reports/. Best-effort: if the spill fails, the alert still goes out with its summary.Testing
879 passed, 6 skipped·ruff checkandruff formatclean ·mypyintroduces no new errors in the touched files (the repo-wideDuplicate module named "main"failure is pre-existing onmain).New coverage: role hash normalization/harvesting/resolution incl. unverified-contract and API-failure paths; the non-role-
bytes32guard; runner warning surfacing and its cap; report role annotation; and the spill path incl. its failure mode.Notes for review
SYSTEM_INSTRUCTIONSbefore this affects live alert severity.utils/chains.pywas considered and deliberately left out; it's a product call, not a defect, and nothing here needs it.🤖 Generated with Claude Code