fix(api): allow mixed date/block bounds on get_actions sort=asc - #174
Conversation
Combining a block-number bound with an ISO-date bound on get_actions (e.g. `after=<block>&before=<ISO date>`) returned `400 Invalid time value [after]`. `applyTimeFilter` chose a single date-OR-block branch based on whether *either* bound contained "T", so a block number passed alongside a date was fed to `new Date(...)` and threw. Each bound is now classified independently: ISO date strings -> the @timestamp range, bare positive integers -> the block_num range. The two can be mixed in both v1 and v2. This makes the documented escape hatch for querying ranges older than `max_asc_window_days` usable: block-number bounds legitimately bypass the recency window, and can now sit next to a date bound. Also: - v1 now applies real block_num filtering (a block-number after/before was previously dropped into the @timestamp range, silently matching all). - the `sort=asc ... must be within the last N days` error (v1 + v2) now points operators at the block-number workaround. - 6 new unit tests covering mixed/same-type bounds in applyTimeFilter.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enables mixing date/timestamp and block-number bounds for the after and before parameters in both v1 and v2 get_actions routes, resolving an issue where mixed bounds caused a 400 error. It also updates the error message for queries older than the maximum window to suggest block numbers as a workaround and adds corresponding unit tests. Feedback on the PR highlights a fragile classification logic in both routes: using .includes('T') to identify dates and parseInt for block numbers can incorrectly classify date strings like '2026-06-01' as block numbers. It is recommended to check for valid positive integers first and treat all other values as dates.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (after) { | ||
| if (typeof after === 'string' && after.includes('T')) { | ||
| tsRange['gte'] = after; | ||
| } else if (parseInt(after) > 0) { | ||
| blockRange['gte'] = after; | ||
| } | ||
| } | ||
| if (before) { | ||
| if (typeof before === 'string' && before.includes('T')) { | ||
| tsRange['lte'] = before; | ||
| } else if (parseInt(before) > 0) { | ||
| blockRange['lte'] = before; | ||
| } | ||
| } |
There was a problem hiding this comment.
The current classification logic relies on after.includes('T') to identify dates and parseInt(after) > 0 to identify block numbers. This is fragile and incorrect. For example, a valid date string like "2026-06-01" (which does not contain 'T') will fail the first check, and parseInt("2026-06-01") will return 2026, incorrectly classifying it as a block number.
Instead, we should classify block numbers first by checking if the value is a valid positive integer using Number.isInteger(Number(value)) && Number(value) > 0 (consistent with isValidBound used elsewhere in the codebase), and treat any other value as a date/timestamp.
| if (after) { | |
| if (typeof after === 'string' && after.includes('T')) { | |
| tsRange['gte'] = after; | |
| } else if (parseInt(after) > 0) { | |
| blockRange['gte'] = after; | |
| } | |
| } | |
| if (before) { | |
| if (typeof before === 'string' && before.includes('T')) { | |
| tsRange['lte'] = before; | |
| } else if (parseInt(before) > 0) { | |
| blockRange['lte'] = before; | |
| } | |
| } | |
| if (after) { | |
| if (Number.isInteger(Number(after)) && Number(after) > 0) { | |
| blockRange['gte'] = after; | |
| } else { | |
| tsRange['gte'] = after; | |
| } | |
| } | |
| if (before) { | |
| if (Number.isInteger(Number(before)) && Number(before) > 0) { | |
| blockRange['lte'] = before; | |
| } else { | |
| tsRange['lte'] = before; | |
| } | |
| } |
| if (query['after']) { | ||
| if (typeof query['after'] === 'string' && query['after'].includes('T')) { | ||
| try { | ||
| _lte = new Date(query['before']).toISOString(); | ||
| tsRange['gte'] = new Date(query['after']).toISOString(); | ||
| } catch (e: any) { | ||
| badRequest(e.message + ' [before]'); | ||
| badRequest(e.message + ' [after]'); | ||
| } | ||
| } else if (parseInt(query['after']) > 0) { | ||
| blockRange['gte'] = query['after']; | ||
| } | ||
| if (query['after']) { | ||
| } | ||
|
|
||
| if (query['before']) { | ||
| if (typeof query['before'] === 'string' && query['before'].includes('T')) { | ||
| try { | ||
| _gte = new Date(query['after']).toISOString(); | ||
| tsRange['lte'] = new Date(query['before']).toISOString(); | ||
| } catch (e: any) { | ||
| badRequest(e.message + ' [after]'); | ||
| badRequest(e.message + ' [before]'); | ||
| } | ||
| } else if (parseInt(query['before']) > 0) { | ||
| blockRange['lte'] = query['before']; | ||
| } | ||
| } |
There was a problem hiding this comment.
The current classification logic relies on query['after'].includes('T') to identify dates and parseInt(query['after']) > 0 to identify block numbers. This is fragile and incorrect. For example, a valid date string like "2026-06-01" (which does not contain 'T') will fail the first check, and parseInt("2026-06-01") will return 2026, incorrectly classifying it as a block number.
Instead, we should classify block numbers first by checking if the value is a valid positive integer using Number.isInteger(Number(value)) && Number(value) > 0 (consistent with isValidBound used elsewhere in the codebase), and treat any other value as a date/timestamp.
if (query['after']) {
if (Number.isInteger(Number(query['after'])) && Number(query['after']) > 0) {
blockRange['gte'] = query['after'];
} else {
try {
tsRange['gte'] = new Date(query['after']).toISOString();
} catch (e: any) {
badRequest(e.message + ' [after]');
}
}
}
if (query['before']) {
if (Number.isInteger(Number(query['before'])) && Number(query['before']) > 0) {
blockRange['lte'] = query['before'];
} else {
try {
tsRange['lte'] = new Date(query['before']).toISOString();
} catch (e: any) {
badRequest(e.message + ' [before]');
}
}
}![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: bab7b50bb2
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (parseInt(after) > 0) { | ||
| blockRange['gte'] = after; |
There was a problem hiding this comment.
For the v1 endpoint, this new block-number branch is not reachable for normal requests because src/api/routes/v1-history/get_actions/index.ts still declares both after and before as type: 'string', format: 'date-time'. With Fastify schema validation enabled on this route, a POST body such as { "sort": "asc", "after": "437506277" } is rejected before getActions runs, so the documented block-number workaround and the new v1 block_num filtering do not actually work through the API. The v1 schema needs to accept block-number strings/numbers as well as date-times.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Fixes get_actions time-bounding so after/before can be mixed between ISO timestamps and block numbers (especially important for sort=asc where block bounds are the documented workaround for querying older ranges).
Changes:
- Update v2
applyTimeFilterto classifyafterandbeforeindependently and apply@timestampand/orblock_numranges accordingly. - Update v1
get_actionsquery construction to support trueblock_numrange filtering and mixed bound types. - Add unit tests for
applyTimeFiltermixed-bound behavior; updatesort=ascguard error messaging; document the fix in the changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
tests/unit/query-guards.test.ts |
Adds unit coverage for applyTimeFilter mixed date/block bounds and normalization cases. |
src/api/routes/v2-history/get_actions/functions.ts |
Fixes applyTimeFilter to support mixed bound types; updates sort=asc recency error message. |
src/api/routes/v1-history/get_actions/get_actions.ts |
Implements mixed-bound filtering and real block_num range filtering; updates sort=asc recency error message. |
CHANGELOG.md |
Documents the mixed-bound fix, the v1 block_num filtering correction, and the improved operator guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else if (parseInt(query['after']) > 0) { | ||
| blockRange['gte'] = query['after']; | ||
| } |
| } else if (parseInt(query['before']) > 0) { | ||
| blockRange['lte'] = query['before']; | ||
| } |
| if (!isNaN(afterDate.getTime())) { | ||
| const maxAge = Date.now() - (maxAscWindowDays * 86400000); | ||
| if (afterDate.getTime() < maxAge) { | ||
| badRequest(`sort=asc "after" date must be within the last ${maxAscWindowDays} days`); | ||
| badRequest(`sort=asc "after" date must be within the last ${maxAscWindowDays} days — use block numbers for "after"/"before" to query older ranges`); | ||
| } |
| if (typeof after === 'string' && after.includes('T')) { | ||
| tsRange['gte'] = after; | ||
| } else if (parseInt(after) > 0) { | ||
| blockRange['gte'] = after; | ||
| } |
| if (typeof before === 'string' && before.includes('T')) { | ||
| tsRange['lte'] = before; | ||
| } else if (parseInt(before) > 0) { | ||
| blockRange['lte'] = before; | ||
| } |
| if (!isNaN(afterDate.getTime())) { | ||
| const maxAge = Date.now() - (maxAscWindowDays * 86400000); | ||
| if (afterDate.getTime() < maxAge) { | ||
| return {error: `sort=asc "after" date must be within the last ${maxAscWindowDays} days`}; | ||
| return {error: `sort=asc "after" date must be within the last ${maxAscWindowDays} days — use block numbers for "after"/"before" to query older ranges`}; | ||
| } |
…ema, guard window) Review feedback from gemini-code-assist, chatgpt-codex-connector, and copilot-pull-request-reviewer on PR #174: - Strict block-number classification (Gemini high; Copilot x4): replace the `.includes('T')` / `parseInt(...) > 0` heuristic with a shared `isBlockNumber()` using `Number.isInteger(Number(v)) && Number(v) > 0`. `parseInt("2026-01-01")` was 2026, so a date without a `T` was misread as block 2026 and the raw string then sent to the block_num range. `Number()` is strict (NaN for "2026-01-01"), so non-integers are correctly treated as dates. Applied to v1 and v2. - Guard/filter alignment (Copilot x2): the `sort=asc` recency window only fired for strings containing `T`, so `after=2020-01-01` (or `after=0`, which parses to year 2000) bypassed the guard. The window now applies to any non-block `after` bound, matching applyTimeFilter's classification. - v1 schema (Codex P2): `after`/`before` were pinned to `format: date-time`, so block-number bounds were rejected by schema validation before the handler ran — the v1 block-number workaround was unreachable. Relaxed to `type: string` (mirrors v2). Descriptions on both v1 and v2 now mention block numbers. Tests: 28/28 pass (added no-T date window checks, no-T classification regression, and isBlockNumber unit coverage).
|
Thanks for the reviews — addressed in 1. Fragile classification — export function isBlockNumber(v: any): boolean {
return Number.isInteger(Number(v)) && Number(v) > 0;
}
2. v1 schema rejects block numbers (chatgpt-codex-connector P2, 3. Guard ↔ filter mismatch lets dates without Tests: 28/28 pass ( Re: Gemini's "classify integers first, else date" suggestion — adopted that exact ordering, using |
Summary
An operator running Hyperion in production hit the
sort=ascquery guard and asked how to query a range older thanmax_asc_window_days. The documented escape hatch is to bound the query with block numbers instead of dates (block bounds legitimately bypass the recency window). But trying it surfaced a real bug:applyTimeFilterpicked a single date-OR-block branch based on whether either bound contained'T'. So whenbeforewas a date, the whole query took the date branch and the block-numberafterwas fed tonew Date("<block>").toISOString(), which throws. You could only use block numbers if both bounds were block numbers.Fix
Classify each bound independently:
T) →@timestamprangeblock_numrangeThe two range filters are
AND-ed, so a block-numberafterand an ISO-datebeforenow compose correctly. Applied to both v1 and v2.block_numfiltering — previously a block-numberafter/beforewas silently dropped into the@timestamprange (interpreted by ES as epoch-millis ≈ 1970, i.e. effectively no filter), even though the guard already advertised block numbers as valid bounds.sort=asc "after" date must be within the last N dayserror (v1 + v2) now points operators at the block-number workaround.The 90-day guard itself is unchanged — this is purely a correctness fix to
applyTimeFilterplus a clearer error message. No change to the DoS protectionsort=ascenforces.Testing
bun test tests/unit/query-guards.test.ts→ 23/23 pass (6 newapplyTimeFiltertests covering mixed and same-type bounds, normalization, and the empty case).400 ... within the last 90 days), the broken mix reproduced (400 Invalid time value [after]), and the same-type workarounds (both-block-numbers,sort=desc) returned200with correct ordering.Behavior matrix (after fix)
afterbefore@timestamprangeblock_numrangeblock_numgte +@timestamplte (previously 400)@timestampgte +block_numlte (previously 400)