Skip to content

fix(token-id-capture): skip instruction content in the conversation d… - #3935

Merged
kajalj22 merged 1 commit into
mainfrom
sdd/token-capture-digest-skip-instruction-content
Oct 7, 2026
Merged

kajalj22 merged 1 commit into
mainfrom
sdd/token-capture-digest-skip-instruction-content

Conversation

@sdevare-nv

Copy link
Copy Markdown
Contributor

OpenCode regenerates its system prompt on every request, and that prompt carries today's date. conversation_digest hashed the system prompt's content, so every rollout that crossed 00:00 UTC failed the parent context check on its next model call and lost its token lineage. In one NeMo RL SWE training step this rejected 453 of 512 rollouts as placeholders.

System and developer items now contribute their role and position to the digest but not their content. Every user, assistant and tool turn is still covered. FINGERPRINT_VERSION goes from 2 to 3, so ledger entries hashed the old way are ignored rather than compared against the new digest.

@copy-pr-bot

copy-pr-bot Bot commented Oct 2, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.


_FINGERPRINT_DOMAIN = b"nemo-gym-lineage"
_CONTEXT_DOMAIN = b"nemo-gym-lineage-context"
_INSTRUCTION_ROLES = frozenset({"system", "developer"})

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.

This fixes the Chat Completions shape but not the other two. The Responses instructions field and the Anthropic system field reach the digest through the _ng_request_envelope pseudo-turn built in base_responses_api_model._request_envelope, and that item still hashes the instruction text in full. I traced the hash inputs with the patch applied: a date-only change in the system prompt still produces different digests for both the Responses and Anthropic paths, so the Claude Code agent has the midnight bug today, and the @ai-sdk/openai TODO in opencode_sandboxed_agent/app.py:603 would bring it back for OpenCode the day someone resolves it.

Suggest splitting the envelope into two items, an instructions item with a role that _is_instruction_item recognizes and a tools item that stays fully hashed. That's a local edit since _ENVELOPE_ROLE isn't referenced anywhere else, and the version bump to 3 already covers it. I prototyped this and all three shapes then survive the date change while tool-schema changes, removed instructions, and edited user turns are still rejected. The module docstring's claim that "Chat, Responses, and Anthropic shapes normalize to the same hash input" should hold again after that, whereas right now this change breaks it.

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.

To be added as a follow up by @pthombre

@pthombre
pthombre marked this pull request as ready for review October 6, 2026 23:07
@pthombre

pthombre commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

/ok to test 5fc5a31

@nemo-automation-bot
nemo-automation-bot Bot requested review from a team October 6, 2026 23:07
@yaoyu-33 yaoyu-33 added area:training Training framework integrations and training-data interfaces bug Something isn't working complexity:low Localized change in one scope with a small, straightforward review surface ready-to-merge PR is approved, current, and waiting for final CI or merge labels Oct 6, 2026
@hemildesai
hemildesai force-pushed the sdd/token-capture-digest-skip-instruction-content branch from 5fc5a31 to fe4709f Compare October 7, 2026 16:29
@copy-pr-bot

copy-pr-bot Bot commented Oct 7, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@hemildesai

Copy link
Copy Markdown
Contributor

/ok to test fe4709f

@copy-pr-bot

copy-pr-bot Bot commented Oct 7, 2026

Copy link
Copy Markdown

/ok to test fe4709f

@hemildesai, there was an error processing your request: E2

See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/

…igest

OpenCode regenerates its system prompt on every request, and that prompt
carries today's date. conversation_digest hashed the system prompt's
content, so every rollout that crossed 00:00 UTC failed the parent context
check on its next model call and lost its token lineage. In one NeMo RL
SWE training step this rejected 453 of 512 rollouts as placeholders.

System and developer items now contribute their role and position to the
digest but not their content. Every user, assistant and tool turn is still
covered. FINGERPRINT_VERSION goes from 2 to 3, so ledger entries hashed the
old way are ignored rather than compared against the new digest.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Sugam Devare <sdevare@nvidia.com>
Signed-off-by: Hemil Desai <hemildesai@users.noreply.github.com>
@hemildesai
hemildesai force-pushed the sdd/token-capture-digest-skip-instruction-content branch from fe4709f to 3aa2c3e Compare October 7, 2026 18:49
@hemildesai

Copy link
Copy Markdown
Contributor

/ok to test 3aa2c3e

@hemildesai
hemildesai added this pull request to the merge queue Oct 7, 2026
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/37671896789

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 7, 2026
@kajalj22
kajalj22 added this pull request to the merge queue Oct 7, 2026
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/37700527898

Merged via the queue into main with commit 7da7416 Oct 7, 2026
44 checks passed
@kajalj22
kajalj22 deleted the sdd/token-capture-digest-skip-instruction-content branch October 7, 2026 23:22
pthombre added a commit that referenced this pull request Oct 8, 2026
Follow-up to #3935, which stopped conversation_digest from hashing system
and developer content. That covered the Chat Completions shape only. The
Responses `instructions` field and the Anthropic `system` field reach the
digest through the `_ng_request_envelope` pseudo-turn, where they were
serialized together with the tool schemas and hashed in full. A date-only
change in the system prompt therefore still broke the parent link for
Responses and Anthropic callers (Claude Code today, OpenCode again once it
moves to @ai-sdk/openai).

The envelope is now two items. Tools keep a reserved role and are hashed in
full. Instructions take the `system` role, so the digest counts their
position but not their content, the same rule #3935 applies to Chat system
messages. Tools come first so a Responses request yields the same item
sequence as the equivalent Chat request, and the two now digest identically.
Removed instructions and changed tool schemas are still rejected.

Entries hashed the old way are already invalidated by the FINGERPRINT_VERSION
bump to 3 in #3935.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Pranav Prashant Thombre <pthombre@nvidia.com>

This branch was previously deployed

1 inactive deployment
public — 3aa2c3ea Deployed Oct 7, 2026 by copy-pr-bot[bot] via release / finalize / notify #4578
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:training Training framework integrations and training-data interfaces bug Something isn't working complexity:low Localized change in one scope with a small, straightforward review surface ready-to-merge PR is approved, current, and waiting for final CI or merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants