test(cache): pin the prompt-cache scope isolation invariant for per-response session ids

[JoaoMarcos44]

What does this PR do?

Pins the cache-scope isolation invariant for hosts that mint one physical session per response, and records the per-response affinity-key re-keying those hosts produce.

This PR owns two independent test contracts, deliberately:

| Contract | File | What it pins |

| --- | --- | --- |

| Cache-scope isolation (the title) | tests/agent/test_prompt_cache_scope.py | distinct physical ids never merge onto one affinity key; cron's per-fire timestamp stays the only carve-out |

| System-prompt persistence under a per-response session | tests/agent/test_system_prompt_restore.py | the #96570 lifecycle clarification — the null-prompt warning is a first-turn artifact, not a lost write |

They share issue #96570 as their origin, not a mechanism. The evidence is the routing/affinity key changing per response; no provider cache telemetry or billing outcome is measured or claimed anywhere in this PR.

This PR started as a production change to agent/transports/codex.py (strip a trailing UUID4 hex from the logical cache scope). Review showed that rule is not identity-safe, so that hunk is reverted and only tests remain. The demonstrated key-churn contract now has its own issue: #96811.

Why the id-normalization approach was dropped

Two negative controls, both from @cervantesh's review, are now executable tests:

1. External client-supplied identities. POST /v1/sessions preserves body["id"] / body["session_id"] verbatim, so two independent conversations may legitimately share a prefix and differ only in a trailing 32-hex identity. Stripping it merges them and makes their prompt_cache_key identical.
2. Studio's own truncation boundary. groupRuntimeSessionId() slices the semantic prefix gc_run_<room>_<profile>_<name> to 96 characters before appending the per-response token, so two members of one room whose names diverge past that boundary are distinguished only by that token. There is nothing left to key on once it is removed — which also shows the churn cannot be repaired from the id alone.

This is the same failure class recorded during the logical-scope design in #79017: speculative id-regex normalization truncates legitimate identities and collides distinct owners. The accepted design resolves scope from semantic lineage, and the host that knows the logical conversation has to declare it.

What the churn issue still is

Every conversation-affinity hint Hermes sends is derived from the physical session id through one normalizer, so a per-response id re-keys all four on every reply:

Consumer	Wire field	Purpose
plugins/model-providers/openrouter	body.session_id	OpenRouter sticky routing key
plugins/model-providers/nous	body.session_id	Nous Portal sticky key
plugins/model-providers/openrouter	x-grok-conv-id header	pins xAI's prompt cache to one backend
agent/transports/codex.py, agent/transports/chat_completions.py	prompt_cache_key	OpenAI / Codex cache routing

Tracked in #96811 with both candidate contracts (host reuses a stable per-member session id; or Hermes honors an explicit logical key such as the existing gateway_session_key, with the #79161 fork isolation designed in).

On the reported write path

The two tests in tests/agent/test_system_prompt_restore.py reproduce the Studio bridge lifecycle (row pre-created and user message pre-persisted before run_conversation()) and record that update_system_prompt() already persists there, and that a second turn restores without a warning. They pass before and after this branch — documentation, not a guard. That is why the closure keyword is Refs, not Fixes.

%%{init: {'theme': 'dark', 'themeVariables': { 'primaryColor': '#00f0ff', 'mainBkg': '#0a0a16', 'primaryTextColor': '#ffffff', 'primaryBorderColor': '#ff007f', 'lineColor': '#00f0ff'}}}%%
graph TD
    A[Physical Session Id] --> N{Cache Scope Normalizer}

    N -->|Cron per-fire timestamp<br/>the one carve-out| C[Stable Job Scope]
    N -->|Everything else<br/>kept verbatim| D[Identity Preserved]

    E[Rejected Rule<br/>Strip Trailing UUID Hex] -.->|Control 1<br/>API-supplied ids| F[Independent Conversations Merged]
    E -.->|Control 2<br/>Studio 96-char truncation| G[Distinct Members Merged]
    F --> H[Isolation Invariant Broken]
    G --> H

    D --> I[Per-Response Ids Still Churn<br/>OpenRouter · Nous · xAI · OpenAI]
    I --> J[Tracked In Follow-Up Issue<br/>Host Declares Logical Identity]

Loading

Related Issue

Fixes #96570
Fixes #96811

Type of Change

• Test coverage / regression controls (no production behavior change)

Changes Made

• tests/agent/test_prompt_cache_scope.py — TestPerResponseRunNonceIsolation: both review negative controls (test_external_uuid_identity_remains_isolated, test_studio_truncation_does_not_merge_members), cron as the only carve-out, the missing lineage on these rows, and the per-response churn recorded on all four affinity surfaces.
• tests/agent/test_system_prompt_restore.py — TestPerResponseSessionWritePath: the Studio bridge lifecycle, showing the system prompt lands in the pre-created row and restores on the next turn.
• agent/transports/codex.py — unchanged (the earlier hunk is reverted in cce4ffe0bf).

How to Test

scripts/run_tests.sh tests/agent/test_prompt_cache_scope.py tests/agent/test_system_prompt_restore.py tests/agent/transports tests/agent/test_codex_responses_adapter.py -q

Local results: 49 passed (test_prompt_cache_scope.py + test_system_prompt_restore.py), 330 passed (tests/agent/transports + test_codex_responses_adapter.py). ruff check clean on both changed files. Every test in this PR passes on unmodified main — they are controls and documentation, which is the point after the revert.

tests/agent/test_portal_tags.py::test_compress_context_preserves_ambient_context fails identically on unmodified main in my local environment (AttributeError: 'DaemonThreadPoolExecutor' object has no attribute '_initializer' under Python 3.14) — pre-existing and unrelated.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits
• I searched for existing PRs to make sure this isn't a duplicate — fix(group-chat): stop repeated prefix cache misses across turns #96589 also references Group chat sessions: stored system prompt is null every turn — rebuilt from scratch, prefix cache always misses #96570 and changes agent/conversation_loop.py; this PR touches no production file at all, so there is no overlap.
• My PR contains only changes related to this fix
• I've run the relevant test suites and all tests pass
• I've added tests for my changes
• I've tested on my platform: Windows 11

Documentation & Housekeeping

• Relevant documentation updates are N/A — the invariant is documented in the test docstrings, at the code it guards
• cli-config.yaml.example updates are N/A because no config keys changed
• CONTRIBUTING.md and AGENTS.md updates are N/A because no architecture or workflow changed
• I've considered cross-platform impact; tests only, no platform-specific code
• Tool description/schema updates are N/A because no tool behavior changed

[cervantesh]

Thanks for separating the write-path finding from the cache-affinity finding. The new tests are useful, and they confirm that update_system_prompt() already persists under the Studio lifecycle. I did find a blocking scope-collision in the production change at exact main@6dcebea7fc vs head@385de99739.

The trailing-hex rule is not identity-safe

Hermes' public session API accepts a client-provided id / session_id and preserves it as the physical conversation identity:

hermes-agent/gateway/platforms/api_server.py

Lines 3454 to 3457 in 6dcebea

	if not self._browser_control_enabled():
	return web.json_response(
	_openai_error(
	"Browser control is not enabled on this server.",

Using two valid independent IDs:

customer_chat_11111111111141118111111111111111
customer_chat_22222222222242228222222222222222

I get:

	main@6dcebea7fc	head@385de99739
normalized scopes	distinct	both customer_chat
prompt_cache_key with the same static prefix	distinct	identical

So a whole trailing 32-hex value is not "per-run noise by construction" for the API that consumes it. The regex also accepts any lowercase 32-hex token, not specifically a UUID4.

There is a second negative control in Studio's own producer shape. groupRuntimeSessionId() truncates the semantic prefix to 96 characters before appending the nonce:

https://github.com/EKKOLearnAI/hermes-studio/blob/4ae23c69bbf29a287206b9b2cc9def4b5e14cdf4/packages/server/src/modules/studio/services/group-chat/agent-clients.ts#L1269-L1273

For one ordinary-length room id and profile default, two member names sharing the first 66 characters but differing afterwards produce distinct physical ids because of their UUIDs, but this PR strips the only remaining distinguishing part and resolves both to the same scope. That directly contradicts the current test_distinct_rooms_and_members_stay_isolated; the test only covers short components. OpenRouter/Nous sticky session_id and x-grok-conv-id consume that normalized scope directly, so this control does not depend on identical system-prompt bytes.

This is also the same failure class already recorded during the logical-scope design in #79017: speculative id regex normalization truncated legitimate identities and collided distinct jobs, so the accepted design moved scope resolution to semantic lineage while preserving cross-scope isolation.

I think the cache-key churn finding is still worth addressing, but the host that knows the logical conversation needs to supply that identity explicitly (or another semantic owner needs to resolve it); globally inferring it from arbitrary session-id syntax is not safe. At minimum, the regression suite needs controls for:

1. two independent external sessions whose ids share a prefix and end in different 32-hex identities; and
2. two Studio logical identities that differ only beyond the producer's 96-character truncation boundary.

Separately, Fixes #96570 does not match what this PR proves: #96570 reports a lost update_system_prompt() write, while these tests show that write already succeeds and the production hunk addresses a different affinity-key issue. Refs #96570 plus a focused issue for the demonstrated key-churn contract would keep the closure semantics accurate.

[cervantesh]

For completeness, here is the sanitized, copy-pasteable witness used for the comparison above. These assertions express the isolation invariant, so both pass on main@6dcebea7fc and both fail on head@385de99739:

from agent.transports.codex import (
    ResponsesApiTransport,
    _cache_scope_from_session_id,
)


def prompt_cache_key(session_id: str) -> str:
    return ResponsesApiTransport().build_kwargs(
        model="gpt-5.5",
        messages=[{"role": "system", "content": "same"}],
        tools=[],
        session_id=session_id,
    )["prompt_cache_key"]


def test_external_uuid_identity_remains_isolated():
    first = "customer_chat_11111111111141118111111111111111"
    second = "customer_chat_22222222222242228222222222222222"

    assert _cache_scope_from_session_id(first) != _cache_scope_from_session_id(second)
    assert prompt_cache_key(first) != prompt_cache_key(second)


def test_studio_truncation_does_not_merge_members():
    room = "mabc1234qwerty"
    profile = "default"
    common_name = "A" * 70

    first = (
        f"gc_run_{room}_{profile}_{common_name}Worker"[:96]
        + "_11111111111141118111111111111111"
    )
    second = (
        f"gc_run_{room}_{profile}_{common_name}Reviewer"[:96]
        + "_22222222222242228222222222222222"
    )

    assert first != second
    assert _cache_scope_from_session_id(first) != _cache_scope_from_session_id(second)

The first test covers the public external-session contract. The second uses Studio's producer ordering exactly: truncate the semantic prefix first, then append the per-response token. No provider credentials or live cache telemetry are required to reproduce the scope regression.

[JoaoMarcos44]

You're right on both controls, and I reverted the production hunk in cce4ffe0bf. Confirming the reasoning rather than just the verdict:

Control 1 — external identities. POST /v1/sessions takes raw_id = body.get("id") or body.get("session_id") and preserves it verbatim (only path-safety, control chars and length are checked), so a whole trailing 32-hex value is a legitimate identity for that API, not per-run noise. Your customer_chat_* pair merges under the rule.

Control 2 — Studio's truncation, and why it settles the whole approach. groupRuntimeSessionId() slices gc_run_<room>_<profile>_<name> to 96 characters before appending the token. With room="mabc1234qwerty", profile="default" and 70+ character names, the semantic prefix is already 100 characters, so both members truncate to the same 96 and the physical ids differ only in their UUIDs. That is stronger than a counterexample to my regex: it means the nonce is the sole carrier of identity for that shape, so no rule that drops it can be safe — narrowing to a gc_run_ namespace would not have rescued it either. It also points at a real producer-side bug: appending the token before truncating (or truncating the components) would keep the prefix distinguishing.

The #79017 parallel is the right one; I should have applied it to my own change.

What the branch now contains (tests only, agent/transports/codex.py back to main):

• test_external_uuid_identity_remains_isolated and test_studio_truncation_does_not_merge_members — your two controls, verbatim in intent, as standing regression controls for the isolation invariant.
• test_cron_normalization_stays_the_only_carve_out — cron's per-fire timestamp remains the single accepted exception.
• test_lineage_walk_cannot_resolve_these_rows — the Studio rows carry no parent_session_id, so no semantic owner resolves them today.
• test_affinity_keys_churn_per_response_today / test_provider_sticky_key_churns_per_response_today — the reported symptom recorded on all four affinity surfaces, as the contract a fix would have to satisfy. All of these pass on unmodified main.

Closure semantics fixed: Fixes #96570 → Refs #96570, and the demonstrated churn now has its own issue, #96811, with the two candidate contracts: the host reuses one stable session id per room member, or Hermes honors an explicit logical key — gateway_session_key ("stable per-chat key", agent/agent_init.py:686, fed by X-Hermes-Session-Key) already carries that meaning for gateway platforms and agent/prompt_cache_scope.py simply does not consult it. The part that needs design there is keeping the #79161 isolation for /branch children, delegate subagents, tool children and background-review forks, which inherit no such key today — not the plumbing.

Thanks for the sanitized witness; it reproduced without any provider credentials, which is what made the second control easy to confirm.

[cervantesh]

Thanks for the thorough update. Reverting the production hunk fully addresses the identity-collision concern from my earlier review. The two new negative controls for external identities and Studio''s truncation boundary now preserve the durable isolation invariant we established.

I have one narrower scope question about the remaining test-only PR, rather than a new production-code objection.

As currently titled, the strict contract appears to be: preserve cache-scope isolation against unsafe inference from per-response session-id syntax. The following tests directly and durably support that contract:

• test_external_uuid_identity_remains_isolated
• test_studio_truncation_does_not_merge_members
• the control preserving cron''s existing carve-out

Some of the other tests appear to serve a different purpose:

• test_lineage_walk_cannot_resolve_these_rows
• test_affinity_keys_churn_per_response_today
• test_provider_sticky_key_churns_per_response_today

Those are useful evidence for #96811, but they assert the current missing-lineage/key-churn behavior. A correct implementation of #96811 would intentionally make those assertions fail, so they seem better suited as reproduction witnesses for that issue—or as RED tests inverted around its eventual stability contract—than as durable regression controls merged beforehand.

The TestPerResponseSessionWritePath tests are also useful and accurately document the #96570 lifecycle clarification, but they protect a separate prompt-persistence contract rather than the cache-scope isolation contract named by this PR. I do not see them as incorrect; I only think the PR should either state clearly that it owns both independent test contracts or keep the scopes separate.

One wording detail follows from the same evidence boundary: the tests demonstrate affinity-key churn, but not that the provider necessarily misses every warm prefix or bills every request uncached. Phrasing this as the routing/affinity mechanism changing each response would preserve the demonstrated finding without claiming provider telemetry that has not been measured.

Would it make sense to keep this PR focused on the durable isolation controls and leave the current-churn witnesses with #96811? That would preserve the value of the review findings without turning the defect that #96811 intends to fix into expected behavior.

[JoaoMarcos44]

Two of your three points are right and are now applied in 72cdd1c611. The third — that a #96811 fix would invert the churn assertions — I checked against the code and it does not hold, so I kept those tests and fixed what was actually wrong with them: their framing.

1. Evidence wording — agreed, fixed. The class docstring claimed the room "never lands back on a warm prefix". Nothing here measures provider cache telemetry or billing; the demonstrated finding is the routing/affinity key moving per response, and that is what it now says. Same bound added to the PR body.

2. Two independent contracts — agreed, now stated. The PR body carries an explicit table: test_prompt_cache_scope.py owns cache-scope isolation, test_system_prompt_restore.py owns system-prompt persistence under a per-response session. They share #96570 as an origin, not a mechanism.

While rewriting that section I found a real defect you did not flag: the body still read Fixes #96570 / Fixes #96811 despite my previous comment claiming I had switched to Refs. Merging would have auto-closed the follow-up issue this PR exists to open. Both are Refs now.

3. The churn witnesses do not invert. Concretely, build_kwargs derives the key from params.get("cache_scope_id") or session_id (agent/transports/codex.py:618), and the provider profiles from get_conversation_context() or session_id (plugins/model-providers/openrouter/__init__.py:137, Nous mirrors it). A #96811 fix supplies the logical identity through the first operand. These tests pass neither — they hand the transport two raw physical ids, exactly as an undeclared conversation does — so they keep asserting distinct-in → distinct-out:

no-logical-key       churn(a != b): True
with cache_scope_id  stable(c == d): True   # the fix, one layer up

test_parentless_rows_resolve_to_their_own_scope (renamed from test_lineage_walk_cannot_resolve_these_rows) is the same story on the resolver: the fixture agent is a SimpleNamespace(session_id, _session_db) with no declared key, so a resolver taught to honor gateway_session_key still walks it to the physical id.

So what those three tests actually pin is the isolation invariant restated at the wire layer — the same assertion shape as test_external_uuid_identity_remains_isolated, which uses the identical _prompt_cache_key helper. If some future change did invert them, it would have to be a syntax rule applied inside the transport, and it would take the two controls you asked to keep down with it. They fail together because they are one invariant.

What was genuinely wrong was that the names and docstrings said _today and "records the reported cost symptom", which reads as a defect snapshot and would rot into "expected behavior" precisely as you describe. Renamed and reframed:

Was	Now
test_lineage_walk_cannot_resolve_these_rows	test_parentless_rows_resolve_to_their_own_scope
test_affinity_keys_churn_per_response_today	test_distinct_ids_keep_distinct_affinity_keys
test_provider_sticky_key_churns_per_response_today	test_distinct_ids_keep_distinct_provider_sticky_keys

Each docstring now names the invariant first and the #96570 symptom second, and states where the logical identity has to arrive from instead.

The reproduction narrative belongs to #96811 either way, and I will carry it there in the issue body rather than leaving it to be inferred from test names here.

tests/agent/test_prompt_cache_scope.py + tests/agent/test_system_prompt_restore.py: 49 passed. No assertion changed in this commit — names, docstrings and the PR body only.

If you would still rather these three move out entirely, say so and I will cut them to #96811; I would just be trading a control that holds for one comment less.

[cervantesh]

Thanks for checking this against the actual precedence paths. I agree with your adjudication of the third point: without a declared logical key, distinct physical identities must remain distinct; a #96811 solution supplies the semantic identity through the higher-precedence cache_scope_id / conversation context, so these fallback isolation assertions do not invert.

The renamed tests and revised docstrings now express that durable invariant rather than freezing "today's churn" as expected behavior. The explicit two-contract table, bounded telemetry wording, and Refs semantics also resolve my remaining scope concerns. No need to move those tests out from my side.

[kshitijk4poor]

Merged via the P0 batch salvage PR #97704 (rebase merge — your commit landed on main with your authorship preserved).

#97704

Thanks for the fix!