Skip to content

BE-1650: histories catalog — pagination params + 31-day window - #73

Merged
ariel-greenfeld merged 4 commits into
mainfrom
be-1650-statement-timeout-on-corev1-device-histories-index-server
Sep 28, 2026
Merged

ariel-greenfeld merged 4 commits into
mainfrom
be-1650-statement-timeout-on-corev1-device-histories-index-server

Conversation

@ariel-greenfeld

@ariel-greenfeld ariel-greenfeld commented Aug 24, 2026 •

Copy link
Copy Markdown
Contributor

Follows xyte-io/server#9302 (histories statement-timeout fix, changes the public contract of GET /core/v1/organization/devices/histories):

  • page/per_page added to the getHistories catalog entry (they were missing — agents could not paginate; the endpoint always supported them)

  • endpoints.md: the canonical example used from: 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 status

  • skills/xyte-cli/data/public-endpoints.json synced byte-identical via scripts/sync_skills_data.mjs

  • Also removes the accidentally tracked node_modules -> ../xyte-cli/node_modules symlink (committed in 7db7a56; .gitignore's node_modules/ never matched a symlink). No runtime effect.

  • drift-overrides.json gets a getHistories entry recording the docs divergence (page/per_page + 31-day cap).

Affected vitest files (endpoints, skills-notes-guidance, skills-sync) run green under Node 22.

⚠️ Should merge before/with the next hub release so agents don't follow the broken example.

🤖 Generated with Claude Code

…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>
Copilot AI lite review requested due to automatic review settings August 24, 2026 15:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_page to the organization.devices.getHistories catalog entry.
  • Update skills/xyte-cli/references/endpoints.md to document the 31-day from/to constraint and show pagination usage.
  • Sync skills/xyte-cli/data/public-endpoints.json to 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.

Comment thread src/api-catalog/public-endpoints.json
Comment thread skills/xyte-cli/references/endpoints.md Outdated
Comment thread skills/xyte-cli/references/endpoints.md Outdated

@borisd borisd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/api-catalog/public-endpoints.json Outdated
Comment thread src/api-catalog/public-endpoints.json Outdated

@borisd borisd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread skills/xyte-cli/references/endpoints.md Outdated
Comment thread src/api-catalog/public-endpoints.json

@borisd borisd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
borisd previously requested changes Sep 3, 2026

@borisd borisd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 neither page/per_page nor the 31-day cap — exactly the divergence drift-overrides.json records for organization.commands.getCommands. Consider a getHistories entry with appendNotes so the provenance survives a future catalog regeneration.

Comment thread skills/xyte-cli/references/endpoints.md Outdated
Comment thread src/api-catalog/public-endpoints.json Outdated
Comment thread tests/skills-notes-guidance.test.ts Outdated
Comment thread src/api-catalog/public-endpoints.json
Comment thread skills/xyte-cli/references/endpoints.md Outdated
@borisd

borisd commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Re-verified every open thread against the tree at 569b820 (not against the "Fixed in " replies) and re-checked the documented contract against hub master's histories_controller.rb + index.json.jbuilder.

Resolved 7 threads — the fixes are genuinely in the tree: the two Copilot doc threads (concrete integers in the --query-json example, "replace with the current timestamp" guidance), the Copilot queryParams test thread, and my four from 08-31/09-01 (inclusive bounds, 422 … not clamped wording, the OFFSET/deep-page-walk guidance verbatim in both surfaces, and the pinning tests in endpoints.test.ts + skills-notes-guidance.test.ts). The CHANGELOG Changed + Upgrade notes ask from the 09-01 body also landed. Catalog sync is byte-identical and enforced by tests/skills-sync.test.ts.

Still open — all five postdate the head commit, so nothing is expected of them yet; listing so the remaining scope is unambiguous:

  • skills/xyte-cli/references/endpoints.md:89 + src/api-catalog/public-endpoints.json:359 — [high] the to = previous from - 1 slide. created_at is microsecond-precision and the filters are created_at >= Time.at(from) / <= Time.at(to), so rows in the open interval (from-1, from) fall out of both windows. Confirmed against the controller. Fix both surfaces together and re-run npm run skills:sync.
  • tests/skills-notes-guidance.test.ts:37 — [high] toContain('to = previous from - 1') contract-locks that recipe; it has to move with the fix.
  • src/api-catalog/public-endpoints.json:359 — [high] the created_at desc, id desc note and the previous_from / previous from spelling split between JSON and md. Confirmed: the jbuilder emits uuid, create_at, name, space_id, model, partner, state — no row id, so the tiebreaker is real server-side but not client-verifiable.
  • src/api-catalog/public-endpoints.json:358 — [medium] response shape is undocumented; confirmed the key is misspelled create_at (not created_at) and serializes as an ISO8601 string, not epoch seconds. An agent can't compute the next window without this.
  • skills/xyte-cli/references/endpoints.md:46 — [low] still "defaults are last week"; the JSON note got the independent-defaults wording, the md row didn't.

Two items from my 09-03 review body are also still open: the untracked-in-the-description node_modules symlink removal ([medium] — the removal looks right, .gitignore's trailing slash never matched a symlink, but the body still says "JSON/docs only"), and the drift-overrides.json getHistories entry ([low] — confirmed absent).

One new [low]: endpoints.md never carries the per_page 1..1000 / rejected-not-clamped rule that the JSON note now has — line 89 covers the window and has_next_page but not the pagination bounds.

CI note: mergeStateStatus: BLOCKED is not about this diff. security fails on npm audit --audit-level=high for brace-expansion (GHSA-rgw5-rvv9-x895) — it fails identically on #72/#74/#75/#76, so it's a pre-existing dependency issue. validate (windows-latest, 22) fails on tests/cli-logging.test.ts:124 ("keeps only an active log file when maxFiles is 1", 5s timeout) — a Windows flake unrelated to this PR; the other 1247 tests pass and ubuntu/macos are green. Neither needs fixing here, but both will keep the merge button red.

Holding CHANGES_REQUESTED on the three highs.

- 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>
@ariel-greenfeld

Copy link
Copy Markdown
Contributor Author

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).

@borisd
borisd dismissed their stale review September 17, 2026 16:38

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 borisd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ariel-greenfeld
ariel-greenfeld merged commit 1528ef5 into main Sep 28, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants