BE-1650: histories catalog — pagination params + 31-day window - #73
Conversation
…t-status semantics Follows xyte-io/server#9302: page/per_page were absent from the getHistories catalog entry (agents could not paginate), and the canonical endpoints.md example used from=0, which now 422s (window capped at 31 days, to >= from). skills/ data copy synced byte-identical via scripts/sync_skills_data.mjs; vitest not run locally (no node_modules, node v20 < required 22). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Updates the bundled public endpoint catalog and skill reference docs to reflect the updated public contract for GET /core/v1/organization/devices/histories, ensuring agents can paginate and won’t follow an invalid wide-range time window example.
Changes:
- Add
page/per_pageto theorganization.devices.getHistoriescatalog entry. - Update
skills/xyte-cli/references/endpoints.mdto document the 31-dayfrom/toconstraint and show pagination usage. - Sync
skills/xyte-cli/data/public-endpoints.jsonto match the source catalog updates.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/api-catalog/public-endpoints.json | Adds pagination query params and clarifying notes for getHistories. |
| skills/xyte-cli/references/endpoints.md | Updates the endpoint matrix row and rewrites the getHistories example for the 31-day window + pagination. |
| skills/xyte-cli/data/public-endpoints.json | Mirrors the source catalog update for shipped skill data consistency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
borisd
left a comment
There was a problem hiding this comment.
Cross-checked every declared fact against hub/app/controllers/core/v1/organization/devices/histories_controller.rb on hub master (BE-1650, xyte-io/server#9302, merged 2026-08-25) — param names, page min 1 / default 1, per_page min 1 / max 1000 / default 100, from default 1.weeks.ago, to default now, to >= from, MAX_WINDOW = 31.days with a strict > (so exactly 31 days is accepted), the online|offline|unavailable|error enum, and has_next_page (the jbuilder emits has_next_page, not the next_page that getDevices returns — the note is right to differ from that row). All accurate; 422 confirmed for both Errors::InvalidParams and RailsParam::InvalidParameterError via exception_handler.rb. page/per_page were indeed already supported pre-#9302, and the partner history endpoints have no 31-day cap, so leaving them untouched is correct. Sync is byte-identical, and no runtime call site in this repo calls getHistories (the from: 0 sites are all getIncidents), so nothing breaks at runtime.
Two gaps in the notes, both for the agent-facing sliding-window walk the docs now recommend.
Automated deep review — flagged for @borisd.
borisd
left a comment
There was a problem hiding this comment.
Two inline findings, plus two repo-convention gaps with no diff line to hang on.
CHANGELOG. Four of the five most recent catalog changes on main (84ad83a, 2afb28f, 7e3268c, 0c5f585) shipped a CHANGELOG.md entry, and this one changes the shipped skill bundle: workspaces that already ran xyte-cli init keep the 422-producing example until they run xyte-cli skills refresh — exactly the case 0.14.0 called out under "Upgrade notes". Worth an [Unreleased] > Changed entry plus that refresh note.
Timing. The body says "should merge before/with the next hub release", but xyte-io/server#9302 merged 2026-08-25, so the currently published skill bundle already ships an example that 422s against production. That makes this more urgent than the body implies, not less.
Nit (defaults). "from defaults to 1 week ago, to defaults to now" reads as "omit either side". The two defaults are applied independently, before the to >= from check, so a to-only call with to more than a week in the past fails with to must be greater than or equal to from even though the caller never sent from. One clause — send both; a single-sided window pairs with the default on the other side — removes a confusing 422.
Verified independently: the sync is byte-identical (sha256 matches between the two JSONs at this head), and no runtime call site in this repo calls getHistories, so nothing here breaks at runtime. Points from the earlier review are not restated; both of its notes gaps still stand unchanged at this head (5abbc1b).
Deep review by Claude on Boris's behalf - findings only, no verdict.
borisd
left a comment
There was a problem hiding this comment.
Requesting changes: 4 unresolved review threads from me are still open on this PR.
Marking the review status so the queue reflects that the ball is with you — no new findings in this pass. Please address the open threads (or reply if you disagree with one) and re-request my review.
- Catalog notes: inclusive from/to bounds, independent defaults (send both), 422-not-clamped for page/per_page, stable newest-first ordering, and deep-page-walk warning; skill data re-synced byte-identical. - endpoints.md: concrete runnable timestamps in the getHistories example, replace-with-current-epoch guidance, to = previous from - 1 sliding step, prefer-narrow-window guidance. - tests: pin new getHistories queryParams and notes (31 days, has_next_page) and the endpoints.md guidance prose. - CHANGELOG: Unreleased entry + skills refresh upgrade note. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
borisd
left a comment
There was a problem hiding this comment.
Verified the documented claims against hub's histories_controller.rb (31-day window, per_page 1..1000 default 100, ordering, 422 mapping) — those are accurate. The sliding-window recipe is the blocker: it silently loses rows, and a test now contract-locks it. Details inline.
[medium] This PR also deletes the tracked node_modules -> ../xyte-cli/node_modules symlink while the body says 'JSON/docs only'. The removal is probably correct (.gitignore's node_modules/ trailing slash never matched the symlink), but call it out in the description/CHANGELOG or split it out.
Additional notes (lines outside the diff):
src/api-catalog/drift-overrides.json:1— [low] The public docs list neitherpage/per_pagenor the 31-day cap — exactly the divergencedrift-overrides.jsonrecords fororganization.commands.getCommands. Consider agetHistoriesentry withappendNotesso the provenance survives a future catalog regeneration.
|
Re-verified every open thread against the tree at Resolved 7 threads — the fixes are genuinely in the tree: the two Copilot doc threads (concrete integers in the Still open — all five postdate the head commit, so nothing is expected of them yet; listing so the remaining scope is unambiguous:
Two items from my 09-03 review body are also still open: the untracked-in-the-description One new [low]: CI note: Holding |
- slide with to = current window from (not from - 1), dedupe boundary rows on (uuid, create_at) - drop the id desc tiebreaker from the ordering note (id is not in the response) - document the response shape (items, uuid, create_at ISO8601, no row id) - independent from/to defaults wording and per_page 1..1000 rule in endpoints.md - pin the corrected recipe in skills-notes-guidance.test.ts - add a getHistories drift-overrides entry Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
from claude: 46567c1 addresses the remaining items from the review body too: endpoints.md now carries the per_page 1..1000 / rejected-not-clamped rule, drift-overrides.json has a getHistories entry, and the node_modules symlink removal is now called out in the description (it was tracked by accident in 7db7a56; dropped rather than split out since it has no runtime effect). |
AI: All five threads verified fixed on 46567c1 — the window-slide recipe now uses from/to boundaries with inclusive bounds and (uuid, create_at) dedupe in both the skill doc and the agent-consumed catalog copy (byte-identical), the test pins the corrected sentence, the response-shape note matches the hub jbuilder field for field, and the defaults wording is right. Dismissing.
borisd
left a comment
There was a problem hiding this comment.
AI: Re-checked every catalog claim against hub's core/v1 histories controller: page/per_page bounds (rejected, not clamped), status filter on current status, inclusive from/to with independent defaults, the 31-day cap and to<from both raising 422, has_next_page without count. Doc/catalog-only change, tests cover the notes and byte-identity. Two [Low] leftovers, not blocking: drift-overrides.json has no runtime importer or test pinning the new getHistories entry, so it can rot silently; and .gitignore still has node_modules/ with the trailing slash, so a re-created node_modules symlink shows up untracked again.
…istories-index-server
Follows xyte-io/server#9302 (histories statement-timeout fix, changes the public contract of GET /core/v1/organization/devices/histories):
page/per_pageadded to thegetHistoriescatalog entry (they were missing — agents could not paginate; the endpoint always supported them)endpoints.md: the canonical example usedfrom: 0, to: 2000000000— that window now 422s (31-day cap,to >= from). Example rewritten to a bounded window + pagination, with a note on sliding windows.notes added: window rules, pagination defaults,
status= device's current statusskills/xyte-cli/data/public-endpoints.jsonsynced byte-identical viascripts/sync_skills_data.mjsAlso removes the accidentally tracked
node_modules -> ../xyte-cli/node_modulessymlink (committed in 7db7a56;.gitignore'snode_modules/never matched a symlink). No runtime effect.drift-overrides.jsongets agetHistoriesentry recording the docs divergence (page/per_page + 31-day cap).Affected vitest files (endpoints, skills-notes-guidance, skills-sync) run green under Node 22.
🤖 Generated with Claude Code