Skip to content

fix(token-id-capture): split instructions out of the request envelope - #4153

Open
pthombre wants to merge 1 commit into
mainfrom
pthombre/token-capture-envelope-split
Open

pthombre wants to merge 1 commit into
mainfrom
pthombre/token-capture-envelope-split

Conversation

@pthombre

@pthombre pthombre commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

What changed and why

Follow-up to #3935, requested in this review thread. #3935 stopped conversation_digest from hashing system/developer content so a rollout that crosses 00:00 UTC keeps its token lineage when the harness regenerates a dated system prompt. 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 built in base_responses_api_model._request_envelope. That item serialized the instruction text together with the tool schemas and was hashed in full, so a date-only change in the system prompt still broke the parent link for the Responses and Anthropic paths. Claude Code (Anthropic system) has the midnight bug today, and OpenCode would get it back the day the @ai-sdk/openai TODO in opencode_sandboxed_agent/app.py is resolved.

This PR splits the envelope into two items:

  • a tools item with a reserved role (_ng_request_tools) that is still hashed in full, and
  • an instructions item with the system role, which _is_instruction_item recognizes, so the digest counts its position but not its content.

Tools come first so a Responses request yields the same item sequence as the equivalent Chat request; the two now digest identically, which restores the module docstring's claim that the three dialects normalize to the same hash input. Removed instructions and changed tool schemas are still rejected.

No further FINGERPRINT_VERSION bump: the bump to 3 in #3935 already invalidates entries hashed the old way; this PR is rebased onto main now that #3935 has merged. A mismatch during any deploy window only yields an unresolved parent, never a wrong one.

Behavior change called out: test_served_calls_do_not_link_across_a_changed_system_prompt encoded the pre-#3935 rule that a changed Anthropic system prompt must not link. That rule is what #3935 reversed, so the test is rewritten as test_served_calls_link_across_a_regenerated_system_prompt: a regenerated prompt links, a removed prompt does not.

Issue

No tracking issue. #3935 was opened directly from the NeMo RL training failure described in its body (453 of 512 rollouts rejected as placeholders after midnight UTC). This PR completes that fix for the remaining two request dialects.

Validation

Reproduced the gap before the change and confirmed the fix with a script that runs a date-only system prompt change through _request_messages + conversation_digest for each dialect:

Dialect #3935 alone This PR
Chat Completions (system message) link survives link survives
Responses (instructions) link breaks link survives
Anthropic (system via anthropic_request_to_responses) link breaks link survives

Tests run against the final diff:

pytest tests/unit_tests/test_token_id_capture.py tests/unit_tests/test_conversation_fingerprint.py \
  tests/unit_tests/test_base_responses_api_model.py tests/unit_tests/test_external_capture_handlers.py \
  tests/unit_tests/test_terminal_attribution.py tests/unit_tests/test_token_capture_conformance.py \
  tests/unit_tests/test_token_capture_ledger.py tests/unit_tests/test_token_capture_staging_routes.py \
  tests/unit_tests/test_responses_api_model_streaming.py
# 571 passed

pytest tests/unit_tests/ --ignore tests/unit_tests/test_docker_compose.py \
  --deselect tests/unit_tests/test_benchmarks.py::TestDiscoverBenchmarksInDir::test_every_repo_benchmark_appears_in_listing
# 6321 passed, 168 skipped, 13 failed. Every failure is environmental to my sandbox and unrelated to this diff:
#   11x test_opensandbox_compose.py (no Docker), test_sandbox_stop_retry.py (tenacity extra not installed),
#   test_slurm_script.py (pip cache warning from a read-only $HOME breaks an output assertion).
# test_docker_compose.py hangs without Docker; the deselected benchmark test fails on DNS resolution (gaierror).

ruff check / ruff format --check on the three changed files: clean
git diff --check: clean

pre-commit run could not fetch hook repositories from my sandbox (proxy blocked the clone); the ruff, trailing-whitespace, and end-of-file checks it would run were run directly. DCO sign-off is on the commit.

New test test_the_parent_link_survives_a_date_change_in_every_dialects_instructions fails on the #3935 branch for the Responses and Anthropic shapes and passes with this PR.

Rollout evidence

N/A. This changes the lineage digest's hash inputs for the model server's capture path only; it does not alter agent, verifier, or model behavior. The unit tests above exercise the served /v1/messages and /v1/chat/completions paths through a real SimpleResponsesAPIModel with capture enabled.

🤖 Generated with Claude Code

@pthombre pthombre added bug Something isn't working area:core Shared APIs, servers, telemetry, health, and registries needs-review PR is ready for code review and waiting on a reviewer labels Oct 7, 2026
@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.

@yaoyu-33 yaoyu-33 added the complexity:medium Single-domain change with interacting parts or a moderate review surface label Oct 7, 2026
@pthombre
pthombre requested a review from ananthsub October 7, 2026 07:00
@hemildesai
hemildesai force-pushed the sdd/token-capture-digest-skip-instruction-content branch 2 times, most recently from fe4709f to 3aa2c3e Compare October 7, 2026 18:49
Base automatically changed from sdd/token-capture-digest-skip-instruction-content to main October 7, 2026 23:22
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>
@pthombre
pthombre force-pushed the pthombre/token-capture-envelope-split branch from c6e07a0 to 2ab3aea Compare October 8, 2026 06:11
@github-actions github-actions Bot added the sla:review-overdue Review response is over the one-business-day SLA label Oct 8, 2026
@cmunley1

Copy link
Copy Markdown
Contributor

/ok to test 2ab3aea

@nemo-automation-bot
nemo-automation-bot Bot requested a review from a team October 20, 2026 08:33
@nemo-automation-bot
nemo-automation-bot Bot requested a review from a team October 20, 2026 08:33

This branch was previously deployed

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

Labels

area:core Shared APIs, servers, telemetry, health, and registries bug Something isn't working complexity:medium Single-domain change with interacting parts or a moderate review surface needs-review PR is ready for code review and waiting on a reviewer sla:review-overdue Review response is over the one-business-day SLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants