Repository navigation
feat: correlate provider requests with the root session - #1883
Conversation
79ab983 to
579240b
Compare
579240b to
af94b5d
Compare
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
Review of af94b5db7 against merged proposal #1882
Compared the 17-file implementation diff against approved baseline 8f049d8a4, ADR 0360, and docs/acceptance/root-session-provider-correlation.md. Extra focus: reliability, trust boundaries, correlation isolation, retries/fallbacks, and continuation lifecycles. Five actionable inline comments are attached; implementation findings are distinguished from missing regression proofs.
Spec
- Important: Codex loses the new root header when its inner request policy reconstructs headers. AC2.1 / ADR 0360 do not state that exception. Resolve the implementation/contract discrepancy explicitly rather than treating the test's empty-header expectation as conformance. This does not imply changing the pre-existing active-header behavior without its own compatibility decision.
- Advisory: The proposal's retry/fallback and concurrent-isolation proof is incomplete. The concrete test gaps are detailed independently below.
Standards
No documented-standards violations found in the reviewed implementation. Inward dependencies, additive engine API snapshot/changelog, isolated test fixtures, and authoritative server-owned rooting are preserved. Formatter/compiler/linter rules were not re-reported as review findings.
Test adequacy
Four important proof gaps, each with an inline comment:
- AC2.1: generic HTTP 500 retries do not exercise encrypted-content fallback through official composition.
- AC2.2: aggregate ID-pair counts cannot identify whole-pair swaps between originating requests; retry overlap is not coordinated, and only Responses is covered.
- AC1.4: initial-prompt tests do not exercise the separately rooted retry, approval, and MCP continuation entries.
- AC1.1 / AC3.1: directly invoking pre-rooted auxiliary helpers bypasses the configured caller-to-engine propagation paths.
Advisory: A gated parent-cancellation test combining detached reviewer lifetime, its independent timeout, and root retention would strengthen reliability (engine/agent/root_session_correlation_test.go:107). Existing tests separately cover reviewer cancellation and parked-child cancellation/timeout; this combined case is optional strengthening, not an established acceptance blocker.
Domain: security and reliability
- Important, cross-confirmed: Codex root-header loss, independently confirmed by the architecture and duplication reviews. Keep credential policy strict when resolving it; do not forward arbitrary incoming headers.
- No demonstrated authentication/authorization bypass or credential leak found. Reviewed validation limits, client/request cloning, authoritative ingress overwrite, source-derived/source-less team rooting, redirect refusal, credential-transport ordering, and detached review's root-only context copying.
- A noncanonical-header-map observation had no established attacker-reachable path and is not reported as a vulnerability. A suspected
CloseIdleConnectionslifecycle regression was withdrawn because no affected production disposal caller was established. - Advisory: Plain custom-provider/proxy HTTP-client overrides now duplicate protocol-constructor defaults (
internal/app/registry.go:948–977,:1811–1812). Optional simplification only; retain credential-bearing overrides, listing separation, and wire tests. The library-reuse review found no additional issue.
Verification and limits
- Focused root-correlation race tests passed for
internal/appandinternal/adapter/server; independent reviewers also ran focused engineagent/porttests and security-boundary tests successfully. - Current GitHub checks are green, including race, lint, API compatibility, standalone modules, and docs. I did not rerun the entire release-readiness gate suite locally.
- Temporary mutation experiments in an isolated review worktree were interrupted and are not evidence for these findings. All three experimental edits were reverted; the review worktree has no diff. No implementation changes were committed or pushed.
- The PR body still names ADR 0359 and says #1882 is open with no approved baseline. Please refresh it to ADR 0360 and merged baseline
8f049d8a4so reviewers see the actual contract.
No high-severity security/reliability blocker established, but full proposal conformance should not be claimed until the Codex discrepancy is resolved. Counts below preserve independent axes; the same Codex discrepancy counts on Spec and Domain, but is posted only once inline.
PANEL: ship_blockers=0 important=6 advisory=3 reviewer_failures=0
| openaicodex.BaseURL, | ||
| openai.WithHTTPClient(policy.HTTPClient()), | ||
| openai.WithHTTPClient(withRootSessionCorrelation(policy.HTTPClient())), |
There was a problem hiding this comment.
[P2 · proposal conformance] Codex drops the new root header after this wrapper adds it.
withRootSessionCorrelation executes before openaicodex.RequestPolicy.RoundTrip; authorizedRequest then rebuilds the entire header map (internal/adapter/openaicodex/policy.go:87–105). Consequently Codex inference never sends the root ID. The new EveryEntryCarriesBothIDs test explicitly expects an empty root for Codex (root_session_transport_test.go:435–436), so its passing result does not demonstrate the merged contract.
ADR 0360's official OpenAI Responses composition requirement and AC2.1 contain no Codex exception. Please either project the validated context-derived root after trusted header reconstruction on the inference path, or obtain an explicit contract amendment documenting the exclusion. Preserve the existing credential/header allowlist and listing boundary; don't copy arbitrary incoming headers through the policy. The pre-existing absence of the active header is a separate compatibility question, not a reason to exempt the new root implicitly.
There was a problem hiding this comment.
Addressed in b106933. Codex inference now decorates the copied policy transport after trusted-header reconstruction and derives both validated active and root IDs from request context. Arbitrary incoming headers remain stripped, while the original policy used by model listing stays undecorated. The entry and remint tests now require both IDs and retain the exact policy-header assertions.
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusInternalServerError) |
There was a problem hiding this comment.
[P2 · test adequacy, AC2.1] This proves retries, but not provider-specific fallback correlation.
Every response here is HTTP 500. The plan also requires both IDs on provider-specific fallbacks and names TestStreamRecoversInvalidEncryptedContent as proof, but that existing test constructs the provider directly without this composition decorator and observes only X-Mecatl-Session-ID.
Please add the invalid-encrypted-content → successful cleaned replay case through newOpenAICompatEntry, capture both correlation headers on the rejected request and fallback request, and assert successful recovery. That would exercise the actual fallback boundary rather than treating repeated generic failures as evidence for it. This is a missing regression proof, not a claim that fallback currently loses the header.
There was a problem hiding this comment.
Addressed in b106933. Added an official newOpenAICompatEntry invalid-encrypted-content recovery test that captures the rejected request and cleaned replay, requires both correlation headers on each, and verifies the reasoning item is present only before fallback.
| active, root := fmt.Sprintf("active-%d", i), fmt.Sprintf("root-%d", i) | ||
| ctx := port.WithRootSessionID(port.WithSessionID(context.Background(), session.SessionID(active)), session.SessionID(root)) | ||
| seq, _ := provider.Stream(ctx, port.LLMRequest{Model: "gpt-5", Messages: []session.Message{session.NewUserMessage("hello")}}) |
There was a problem hiding this comment.
[P2 · test adequacy, AC2.2] Associate each observed ID pair with its originating request.
All goroutines send the same payload, and the assertions below only count aggregate active-N/root-N pairs. A regression that swaps a complete pair between two requests can preserve those counts and pass. Also, starting goroutines does not guarantee the initial/retry overlap that AC2.2 explicitly calls out.
Give each request a unique payload marker, assert its expected active/root pair at the HTTP boundary on every attempt, and use bounded, cancellation-aware barriers to force retry interleaving. Run the same proof against Chat Completions and Anthropic composition as well as Responses. This tests cross-request attribution rather than just internal consistency of the two headers.
There was a problem hiding this comment.
Addressed in b106933. The concurrent isolation proof now uses unique payload markers and cancellation-aware generation barriers to force initial and retry overlap. It asserts payload-to-active/root attribution on every attempt for Responses, Chat Completions, and Anthropic with SDK retries disabled and exactly two outer attempts.
| svc, provider, sess := rootCorrelationService(t) | ||
| run, err := svc.StartRun(port.WithRootSessionID(context.Background(), "forged-caller-root"), sess.ID, "go") |
There was a problem hiding this comment.
[P2 · test adequacy, AC1.4] Cover the other authoritative run-entry boundaries too.
This test and the HTTP/gRPC companions exercise initial prompts only. The implementation adds separate rooting calls for failed-run retry, approval resumption (including persisted approval), and both granted/terminal MCP authorization continuations. These are distinct places where a caller-carried root must be overwritten; the initial-prompt proof does not exercise them.
Please extend the existing continuation lifecycle fixtures with a forged pre-populated root context and assert the authoritative active/root pair on the resulting provider call. In particular, cover RetryFailedRun, restored approval, and both MCP continuation branches. The code currently roots these paths correctly; this comment is about retaining a regression test at each security-relevant composition boundary, not an existing ingress bypass.
There was a problem hiding this comment.
Addressed in b106933. Existing lifecycle fixtures now inject forged active/root context and assert the authoritative pair at the provider boundary for RetryFailedRun, restored ApproveRun, persisted ResolveRunAsk, and granted and terminal MCP authorization continuations.
| {"guardrail-checker", "guardrail-checker-", root, func(_ *testing.T, llm port.LLMProvider) { | ||
| _, _ = agent.RunGuardrailCheck(rooted, newEngine(agent.Deps{LLM: llm, Catalog: tool.NewCatalog()}), "check this") | ||
| }}, | ||
| {"model-router", "model-router-", root, func(_ *testing.T, llm port.LLMProvider) { | ||
| _, _, _, _ = agent.RunModelRouter(rooted, newEngine(agent.Deps{LLM: llm, Catalog: tool.NewCatalog()}), agent.ModelRouteRequest{ | ||
| TaskPrompt: "inspect", Categories: []agent.ModelRouteCategory{{Name: "fast"}}, | ||
| }) | ||
| }}, | ||
| {"fork-judge", "fork-judge-", root, func(_ *testing.T, llm port.LLMProvider) { | ||
| judge := agent.NewEngineJudge(newEngine(agent.Deps{LLM: llm, Catalog: tool.NewCatalog()})) | ||
| _, _, _ = judge.Judge(rooted, []agent.BranchSummary{{Label: "branch-0", Summary: "a"}, {Label: "branch-1", Summary: "b"}}, "pick one") |
There was a problem hiding this comment.
[P2 · test adequacy, AC1.1 / AC3.1] The auxiliary rows bypass the caller-to-engine propagation seam.
These are real auxiliary engines, but the test supplies rooted directly to their helpers. For example, the judge row cannot detect root loss in ParallelTool.judgeWinner, because it never executes a Parallel call with join: "judge". The router and guardrail rows similarly bypass their configured production callers.
Keep these lower-level assertions, but add provider observations through the actual Parallel judge join, configured Subagent routing, and guardrail invocation. Use a parent whose active ID differs from its inherited root so resetting to the immediate parent is detectable. The earlier delegated rows already exercise real Subagent/Parallel/Team tools, and the separate scripted-reviewer test covers root delivery to the reviewer interface; those are useful and need not be replaced.
There was a problem hiding this comment.
Addressed in b106933. Delegated-family tests now use a parent active ID distinct from the inherited root. Added observations through configured named-Subagent routing, the actual Parallel join=judge path, and configured guardrail hook dispatch; the lower-level assertions remain.
There was a problem hiding this comment.
Rechecked at b1069333b: the real Parallel judge and configured guardrail dispatch now address those parts of this finding. The configured semantic-router proof is still missing, though.
TestBuildAgentSubagentEnginesResolvedModelOnRequest pins Model: "fast" and resolves it to cheap-id (internal/app/agentdefs_test.go:330–368). That exercises named-child selection, but never invokes the semantic classifier. The root-specific router test still calls RunModelRouter directly with pre-rooted context (internal/app/root_session_transport_test.go:429), while the configured-routing E2E records model/system prompt without active/root identities (internal/app/subagent_agent_router_test.go:74–77). Root loss at the configured caller in internal/app/build.go:7293 would therefore escape these assertions.
Please add or extend a fixture with an unpinned named Subagent and semantic routing enabled, using a parent whose active ID differs from its inherited root. Observe the actual classifier-provider request and assert its model-router-* active ID plus the unchanged inherited root. This remains a regression-proof gap under AC1.1 / AC3.1, not a demonstrated runtime bug.
The other four actionable findings are addressed: Codex final-transport stamping with listing/header-policy separation, official-composition encrypted-content fallback, payload-attributed concurrent retries across all three protocols, and the retry/approval/MCP continuation boundaries. Focused offline tests passed, including repeated transport race tests and continuation race tests. I would keep this thread open for the remaining configured-router assertion.
There was a problem hiding this comment.
Addressed in ae86733. Added TestRootSessionProviderCorrelation_Scenario3_ConfiguredRouterPreservesRoot in internal/app/root_session_transport_test.go.
It builds the production per-session engine through sessionEngineFactory, with semantic routing enabled and an unpinned reviewer agent def. The parent runs as parent-active under an inherited root of inherited-root, then delegates to the def. The test observes context identities at the provider port and asserts that the classifier request has a model-router-* active ID and still carries inherited-root. It also checks the parent pair and that the routed child landed on the classified model with the same root.
I mutation-checked it: resetting the root to the immediate parent at the RunModelRouter call in buildModelRouterTask makes it fail with root "parent-active".
Separate from this thread: I rebased onto main. #1510 replaced guardrail hooks with the contextual ToolReviewer, so the configured-guardrail proof now runs through buildGuardrailsActionReviewer + attachGuardrailReviewer and expects the reviewer's guardrail-review-* child session (562603f). What it asserts about the root is unchanged.
af94b5d to
b106933
Compare
|
Addressed the review findings in
The optional combined reviewer-cancellation strengthening remains advisory and was not added; existing tests continue to cover detached reviewer cancellation and parked-child cancellation/timeout separately. I also refreshed the PR body to ADR 0360, merged Plan / Interface PR #1882, and approved baseline Verification: |
Add X-Mecatl-Root-Session-ID next to X-Mecatl-Session-ID so provider and delegated-router requests can be grouped by the causal root conversation while each child keeps its own active session ID (ADR 0360). The engine seeds the root in prepareRun when the context carries none, so Subagent, Parallel, team members, judges, reviewers, guardrail checks and the model router inherit it without extra wiring. detachedRunContext keeps the root when nested work must outlive parent cancellation. Server run entries overwrite caller context with the authoritative session through rootedRunContext. Composition wraps every final inference HTTP client and the Jev classifier client with a transport that stamps the validated root on each attempt. Tests cover every child family and every provider entry that brings its own HTTP client. The OpenAI Codex request policy rebuilds its header set from constants, so neither correlation header reaches chatgpt.com. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Apply correlation after the Codex request policy rebuilds trusted headers, retaining both active and root IDs without broadening its allowlist. Strengthen fallback, retry-isolation, continuation, and real auxiliary-caller proofs requested in review, and advance the approved acceptance plan to its landed candidate state. Co-Authored-By: Mecatl <noreply@mecatl.dev>
Main's contextual guardrails integration (#1510) removed buildGuardrailsHooks in favour of an engine ToolReviewer, which broke the build of this branch's configured-guardrail correlation test after rebase. Drive the same dispatch through the real buildGuardrailsActionReviewer and attachGuardrailReviewer wiring, and expect the reviewer's guardrail-review- child session as the active ID while root stays intact. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
b106933 to
562603f
Compare
Review follow-up on AC1.1 / AC3.1: the existing router proof invoked RunModelRouter directly with a pre-rooted context, so root loss at the configured caller in buildModelRouterTask would go unnoticed. Drive an unpinned named Subagent through the production per-session engine with semantic routing enabled and a parent whose active ID differs from its inherited root, then assert the classifier request carries a model-router-* active ID and the unchanged inherited root. Resetting the root at the configured caller makes this test fail. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
Rechecked at ae86733a4. The remaining configured-router review request is addressed.
Spec
TestRootSessionProviderCorrelation_Scenario3_ConfiguredRouterPreservesRoot now drives an unpinned named Subagent through sessionEngineFactory with semantic routing enabled. It distinguishes parent-active from inherited-root, observes the actual classifier-provider request with a model-router-* active ID and unchanged root, and verifies the child uses the classified model. This closes the AC1.1 / AC3.1 proof gap rather than bypassing the configured caller.
Standards
No new findings. The fixture uses supplied in-memory stores/assets; this factory path does not discover operator user-model state.
Test adequacy
The guardrail rebase adaptation also exercises the contextual reviewer through buildGuardrailsActionReviewer and attachGuardrailReviewer, preserving the inherited root with a distinct reviewer session.
One optional, non-blocking improvement: the event drains at internal/app/root_session_transport_test.go:69 and :496 could assert successful terminal completion as well as provider observations. A late run failure can otherwise be ignored. This does not invalidate the correlation assertions or keep the previous request open.
Domain
No new correctness or security findings in this follow-up. The earlier Codex final-transport/header-policy separation, official-composition encrypted-content fallback, payload-attributed retries across all three protocols, and retry/approval/MCP continuation fixes remain present.
CI is green on this head, including race, lint, API compatibility and documentation gates. Focused local checks also passed; I did not rerun the full local gate suite. No implementation changes made.
All five original actionable review requests are addressed. Approving; no merge performed.
PANEL: ship_blockers=0 important=0 advisory=1 reviewer_failures=0
…mit-coauthor-guidance * origin/main: (32 commits) docs: plan optional native Kubernetes execution provider (#1579) feat(mecatui): restore session status metrics (#1891) docs(plan): bound agent inventory viewport (#1946) docs: include user-docs in matlatl and use local documentation links (#1934) feat(mecatui): bound sessions inventory (#1944) feat(studio): show session delegation activity (#1907) docs(plan): define Studio session activity (#1885) Release TypeScript SDK v0.4.0 (#1940) Release v0.0.41 (#1941) docs(mecatui): plan bounded sessions inventory (#1906) feat(studio): add exact plan control and authorization review prerequisite (#1909) test(app): eliminate real ToolHive probe deadline wait (#1937) docs(plan): define Studio chat approvals and exact plan control (#1888) test(sdk): enforce SDK/TUI high-level parity (#1471) (#1917) docs(plan): propose SDK high-level parity coverage (#1471) (#1916) docs: improve gRPC schema table readability (#1842) test(app): eliminate real waits in durable learning tests (#1933) feat(studio): align chat transcript and composer (#1886) docs(plan): define Studio chat transcript and composer (#1880) feat: correlate provider requests with the root session (#1883) ...
Summary
X-Mecatl-Root-Session-IDalongsideX-Mecatl-Session-ID, so provider, gateway, and Jev logs can group a run tree while each child retains its active session identity.8f049d8a4.How the root flows:
prepareRunseeds the root from the session only when none is inherited. Nested engines preserve it, including detached work that outlives parent cancellation.Development stage
Contract linkage
8f049d8a4Interface conformance
landedcandidate transition.Issue relationship
None. Directed operator request, no tracking issue.
Type of change
Test plan
task lint)KUBECONFIG=/dev/null task test:race)go run ./cmd/mecademo): tool call, permission ask/approval, and resulttask docs)task site:build)Review-driven regression coverage now includes:
User-facing change
Outbound model requests carry
X-Mecatl-Root-Session-ID. Operators can group provider and gateway logs by conversation. The field is outbound-only, grants no authority, and gateways must not route on it. The mecak8s deployment page documents it.🤖 Generated with Mecatl