Skip to content

[skycap] Fix format.md where it disagrees with the writer - #2425

Merged
kouroshHakha merged 4 commits into
NovaSky-AI:mainfrom
jadenfix:skycap-format-doc-fixes
Oct 7, 2026
Merged

kouroshHakha merged 4 commits into
NovaSky-AI:mainfrom
jadenfix:skycap-format-doc-fixes

Conversation

@jadenfix

@jadenfix jadenfix commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes the spots in skycap/docs/format.md that don't match what skycap writes (the format.md item in #2318). Docs only, plus a graph.py docstring fix and one test.

Changes

  • shadowed_by was backwards for token mode: a turn only reuses the chosen sibling if it extends its call or the render reproduces its tokens, so history can continue from the shadowed node. Fixed in the spec and graph.py, with a test.
  • Spec now states path order (by leaf id) and that a model node trains in the first path containing it.
  • A node can have mask_offset: 0, mask_rows: 0 with no sampling_mask file; the sidecar guarantee now covers only listed sidecars.
  • Each file must be exactly one zstd frame.
  • JS example copies the Node Buffer before taking views, since it can sit at a non-zero byteOffset in a pooled ArrayBuffer.
  • logprobs are NaN for every token of a node without has_logprobs; sidecars is {} for a token-mode trajectory with no tokens; added capture.use_raw_content.

Test plan

  • New test test_token_history_continues_from_the_shadowed_sibling_whose_tokens_the_render_reproduces covers shadowed_by.
  • Wrote and read back records to confirm the empty-mask, NaN-logprob and empty-sidecars behavior.
  • In Node 22 the old JS one-liner read [47, 0] instead of [1, 2] on a pooled Buffer; the new one is correct. The multi-frame line goes off [skycap] Follow-ups after the Harbor integration #2318 (Node 22 has no zstd).

Validation

cd skycap && uv run --isolated --group dev pytest: 136 passed → 137 passed (1 skipped both times). ruff and black pass at the pre-commit versions.

Part of #2318.


Note

Low Risk
Documentation and test coverage only; runtime graph/token behavior is unchanged in this diff.

Overview
Aligns skycap/docs/format.md (and the graph.py module/shadowed_by docstrings) with what the writer already does—no on-disk format or graph logic changes in this diff.

The biggest fix is shadowed_by in token mode: matching still prefers the latest model (or client) sibling, but later history only continues from that sibling when tokens agree (bridge or full render). Otherwise history forks via a client node with the rendered tokens—including reusing a shadowed client sibling—while a shadowed model sample never receives later history. Text mode is unchanged (always continues from the chosen sibling).

The spec also documents path order (by leaf id), one training target per shared model node (first path), when sidecar files exist (tokens only if any node has tokens; sampling mask only if there is at least one row; empty sidecars when token mode has no tokens), single-frame zstd files, sidecar existence tied to document.sidecars, capture.use_raw_content, NaN logprobs when has_logprobs is false, and a Node.js sidecar read pattern that copies decompressed bytes before typed array views.

Tests add two token-mode scenarios for shadowed siblings and rendered-token forking.

Reviewed by Cursor Bugbot for commit cc8dfd3. Bugbot is set up for automated code reviews on this repo. Configure here.

Corrections from the format.md list in NovaSky-AI#2318:

- shadowed_by: in token mode, later history can continue from the
  shadowed node, because a turn only reuses a matched node whose tokens
  the render reproduces. Same fix in graph.py's docstrings, plus a test.
- Path order (by leaf id) and training targets (first path containing
  the node) are now in the spec, not only in samples.py.
- A node can have mask_offset 0 and mask_rows 0 with no sampling_mask
  sidecar written; the sidecar guarantee now covers only listed sidecars.
- Require exactly one zstd frame per file.
- The JS example now copies a Node Buffer before taking typed views.
- logprobs are NaN for every token, scaffold included, of a node without
  has_logprobs; sidecars is {} for a token-mode trajectory with no
  tokens; capture.use_raw_content is listed.

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request updates the documentation and implementation details regarding trajectory file formats, specifically addressing zstd frame constraints, sidecar existence conditions, and token-mode history continuation. It clarifies how shadowed_by behaves in token mode versus text mode, adds a JavaScript/Node.js example for safe buffer decompression, and introduces a new test to verify that token history continues from the shadowed sibling whose tokens the render reproduces. There are no review comments to address.

@greptile-apps

greptile-apps Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Medium risk] Clarifies token-mode history logic in docs and code.

The PR appears safe to merge; no actionable issue was identified.

Reviews (1) · Last reviewed commit: "[skycap] Fix format.md where it disagree..."

kouroshHakha and others added 3 commits October 7, 2026 17:11
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…mode

A shadowed model sample never receives later token-mode history: the
fallback in turn.plan/commit only creates or reuses client nodes. Fix the
format.md row and the graph.py docstrings, and add a test where the render
reproduces a shadowed model sample's tokens but the turn still forks.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@kouroshHakha kouroshHakha left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

fixed a couple of areas that needed commenting inplace. Thanks for the contribution. merging.

@kouroshHakha
kouroshHakha merged commit 1d64026 into NovaSky-AI:main Oct 7, 2026
5 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.

2 participants