fix: request raw-run semantics for the reflection distiller sub-run#928
fix: request raw-run semantics for the reflection distiller sub-run#928gorkem2020 wants to merge 2 commits into
Conversation
|
This PR currently conflicts with the latest Please rebase onto or merge the latest |
a52df9e to
bbac3ca
Compare
app3apps
left a comment
There was a problem hiding this comment.
Re-reviewed the conflict-resolved head bbac3ca. The change is narrow, source and dist stay aligned, and the dedicated regression plus the full test suite pass. The call site now consistently requests raw-run semantics for the reflection distiller.
Non-blocking follow-up: the current tests prove that modelRun: true is forwarded, but they do not execute the real host hook dispatcher. A host-level test should verify that foreign before_prompt_build hooks are skipped across the supported host range and document how raw-run semantics interact with promptMode: "minimal".
|
After #919 was merged, this PR now has merge conflicts with the latest |
…b-run The reflection distiller's embedded sub-session (runEmbeddedPiAgent, sessionKey shaped temp:memory-reflection:<agentId>) only sat inside our own hook-skip guards (CortexReach#916, CortexReach#922). The host's before_prompt_build dispatch still fired for every OTHER registered plugin's hooks, so a foreign plugin's injected system-context block (observed live: 16.6KB from another plugin's recall-policy injector) got prepended to the distiller's model call, wasting tokens and risking distillate steering from instructions the sub-run has no tools to act on. Pass modelRun: true in the runEmbeddedPiAgent call so the host's shouldSkipPromptBuildHooks({ isRawModelRun }) resolves true for this run (isRawModelRun = params.modelRun === true || params.promptMode === "none"), skipping before_prompt_build dispatch for all plugins, not just ours. The explicit promptMode: "minimal" already set on this call takes precedence over the raw-run default of promptMode "none", so the reflection prompt shape is unaffected.
Verified red (modelRun/promptMode-literal assertions failed) against the pre-fix index.ts, green against the fix in the prior commit. Wire the new test into the local npm test chain and the core-regression CI group.
bbac3ca to
4a8cd2b
Compare
Summary
runEmbeddedPiAgent, sessionKey shapedtemp:memory-reflection:<agentId>) now passesmodelRun: true, so the host'sshouldSkipPromptBuildHooks({ isRawModelRun })guard skipsbefore_prompt_builddispatch for every registered plugin during that call, not just this plugin's own hooks.openai/gpt-oss-120bgeneration): a foreign plugin's ~16.6KB injected system-context block (recall-tool policy text describing tools the sub-run has no access to) was being prepended to the distiller's model call before this fix.Why
#916 and #922 taught this plugin's own
before_prompt_buildhooks (auto-recall, the self-improvement reset reminder, both reflection injectors) to skip the distiller's sub-session viaisInternalReflectionSessionKey(). That guard only covers hooks registered by this plugin. The host's dispatch guard forbefore_prompt_buildlives one layer up, inshouldSkipPromptBuildHooks, and only short-circuits when the run itself is marked raw:isRawModelRunis derived asparams.modelRun === true || params.promptMode === "none". The distiller'srunEmbeddedPiAgentcall never set either, so the host kept firing every other plugin'sbefore_prompt_buildhooks into the sub-run. On the live fleet this meant another installed plugin's recall-policy injector (abefore_prompt_buildhook unrelated to memory reflection) prepended its own instructions and tool guidance to the distillation prompt, a call that runs withdisableTools: trueand has no way to act on that guidance. Pure token waste plus a risk of steering the distillate with unrelated directives.Passing
modelRun: truerequests the host's raw-run semantics directly, which is the mechanismshouldSkipPromptBuildHooksactually checks. The call's explicitpromptMode: "minimal"is preserved and still takes precedence over the raw-run default ofpromptMode: "none", so the reflection prompt shape (no skills catalog, minimal surface) is unchanged; only the foreign-hook dispatch is skipped.Interplay with #922: that PR skips this plugin's own auto-recall hook for the distiller sub-session, from inside the sub-session. This PR requests raw-run semantics at the call site instead, so every plugin's
before_prompt_buildhook is skipped before the sub-run even starts. The two are complementary and touch different lines (this PR only adds three lines directly after the existingdisableTools/disableMessageToolfields), so a later rebase of either branch should be straightforward.Scope note on smart extraction
The original defect report described both the reflection distiller and smart extraction's embedded sub-run as exposed to this issue. Investigation (source read plus live Langfuse trace comparison) shows smart extraction's LLM calls do not go through the host's embedded-agent-runner at all:
SmartExtractoris constructed with anLlmClientbuilt bycreateLlmClient()(src/llm-client.ts), which talks directly to the configuredllm.baseURL(an external OpenAI-compatible endpoint or the provider's own OAuth backend) via theopenaiSDK. That path never callsrunEmbeddedPiAgent, so the host'sbefore_prompt_builddispatch (andshouldSkipPromptBuildHooks/isRawModelRun) never runs for it, there is no sub-run to mark raw.A same-model (
openai/gpt-oss-120b) trace comparison from the live fleet confirms this: smart extraction's actual system prompt is exactly"You are a memory extraction assistant. Always respond with valid JSON only.", with no injected block, while the reflection distiller's trace at the same time carries the ~16.6KB foreign injection this PR fixes. The two code paths were easy to conflate at a glance since they share the same downstream model; the trace data disambiguates them. No code change was made toSmartExtractororsrc/llm-client.ts.Tests
test/raw-run-distiller-hooks.test.mjs, mocking the Layer 1 embedded runner the same way astest/reflection-per-agent-lane.test.mjs:generateReflectionTextpassesmodelRun: truetorunEmbeddedPiAgent.promptMode: "minimal"(anddisableTools/disableMessageTool) survive alongsidemodelRun.runEmbeddedPiAgentcall site, so no other call was silently marked raw.modelRun: trueis an unconditional literal, not gated behind a host-version capability check; older hosts that ignore the field simply fail open to the pre-fix behavior.Stash-verified red before the fix (both
modelRunassertions failed against the pre-fixindex.ts), green after.Test plan
npm test(full local suite, excluding the pre-existingtest/cjk-recursion-regression.test.mjsEADDRINUSE :11434 conflict against a real local Ollama daemon, unrelated to this change)node scripts/verify-ci-test-manifest.mjsnpm run verify-package-runtime(rebuildsdist/)dist/index.jscommitted alongside theindex.tssource change