Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -50,23 +50,6 @@ mock.module("../../../export/transcript-formatter.js", () => ({
buildAnalysisTranscript: () => "user: hi",
}));

// Default config stub — individual tests can override via mockGetConfig.
interface AnalysisConfigStub {
analysis: {
modelIntent?: string;
modelOverride?: string;
};
}
const mockGetConfig = mock(
(): AnalysisConfigStub => ({
analysis: {},
}),
);

mock.module("../../../config/loader.js", () => ({
getConfig: mockGetConfig,
}));

import { AssistantEventHub } from "../../assistant-event-hub.js";
import type { SendMessageDeps } from "../../http-types.js";
import { analyzeConversation } from "../analyze-conversation.js";
Expand All @@ -90,8 +73,6 @@ beforeEach(() => {
mockFindAnalysisConversationFor.mockImplementation(() => null);
mockGetConversationSource.mockReset();
mockGetConversationSource.mockImplementation(() => null);
mockGetConfig.mockReset();
mockGetConfig.mockImplementation(() => ({ analysis: {} }));
});

function makeConversation() {
Expand Down Expand Up @@ -221,12 +202,17 @@ describe("analyzeConversation", () => {
expect(allowedTools).toBeInstanceOf(Set);
expect(allowedTools?.size).toBe(0);

// Fires the agent loop.
// Fires the agent loop with the analyzeConversation call-site so the
// per-call provider config flows through `resolveCallSiteConfig`.
expect(conversation.runAgentLoop).toHaveBeenCalledWith(
expect.any(String),
"msg-1",
expect.any(Function),
expect.objectContaining({ isInteractive: false, isUserMessage: true }),
expect.objectContaining({
isInteractive: false,
isUserMessage: true,
callSite: "analyzeConversation",
}),
);
});

Expand Down Expand Up @@ -405,40 +391,36 @@ describe("analyzeConversation", () => {
expect(mockAddMessage).not.toHaveBeenCalled();
});

test("auto: passes modelOverride through to getOrCreateConversation when set in config", async () => {
mockGetConfig.mockImplementation(() => ({
analysis: {
modelIntent: "quality-optimized",
modelOverride: "claude-opus-4-6",
},
}));
test("auto: routes the agent loop through callSite: 'analyzeConversation'", async () => {
const conversation = makeConversation();
const deps = makeDeps(conversation);

await analyzeConversation("conv-1", deps, { trigger: "auto" });

expect(deps.getOrCreateConversation).toHaveBeenCalledWith(
"analysis-new",
expect.objectContaining({
modelIntent: "quality-optimized",
modelOverride: "claude-opus-4-6",
}),
expect(conversation.runAgentLoop).toHaveBeenCalledWith(
expect.any(String),
"msg-1",
expect.any(Function),
expect.objectContaining({ callSite: "analyzeConversation" }),
);
});

test("auto: does not pass modelOverride/modelIntent keys when config leaves them unset", async () => {
test("does not thread modelIntent/modelOverride into getOrCreateConversation", async () => {
// Per-call model selection now happens via the call-site resolver against
// `llm.callSites.analyzeConversation`, not via legacy modelIntent/
// modelOverride keys on the conversation create options.
Comment on lines +409 to +411

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.

🔴 Test comment references "legacy" removed code, violating AGENTS.md rule

The new test comment at assistant/src/runtime/services/__tests__/analyze-conversation.test.ts:409-411 says "Per-call model selection now happens via the call-site resolver … not via legacy modelIntent/modelOverride keys". This narrates history ("now happens via") and references removed code ("legacy modelIntent/modelOverride"), violating the assistant/AGENTS.md:21 rule about not referencing removed code in comments.

Suggested change
// Per-call model selection now happens via the call-site resolver against
// `llm.callSites.analyzeConversation`, not via legacy modelIntent/
// modelOverride keys on the conversation create options.
// Per-call model selection happens via the call-site resolver against
// `llm.callSites.analyzeConversation`; verify that modelIntent/modelOverride
// keys are not threaded into the conversation create options.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

const conversation = makeConversation();
const deps = makeDeps(conversation);

await analyzeConversation("conv-1", deps, { trigger: "auto" });

const [, passedOpts] = (
deps.getOrCreateConversation.mock.calls as unknown as Array<
[string, Record<string, unknown>]
>
)[0] ?? ["", {}];
expect(passedOpts).toBeDefined();
expect("modelIntent" in (passedOpts ?? {})).toBe(false);
expect("modelOverride" in (passedOpts ?? {})).toBe(false);
const calls = deps.getOrCreateConversation.mock
.calls as unknown as Array<[string, Record<string, unknown> | undefined]>;
expect(calls.length).toBe(1);
const passedOpts = calls[0]?.[1];
if (passedOpts !== undefined) {
expect("modelIntent" in passedOpts).toBe(false);
expect("modelOverride" in passedOpts).toBe(false);
}
});
});
32 changes: 16 additions & 16 deletions assistant/src/runtime/services/analyze-conversation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,18 @@
* Two triggers are supported:
* - **manual**: user-initiated analysis. Creates a fresh conversation each
* invocation, runs with `trustClass: "unknown"`, and strips the tool
* surface. Byte-for-byte unchanged from the original route logic.
* surface.
* - **auto**: called by the auto-analyze job when a source conversation
* reaches a natural pause. Reuses a rolling analysis conversation per
* parent (creating one if none exists), runs with `trustClass:
* "guardian"`, and keeps the full tool surface so the analysis agent can
* write memory and skills directly. Reads optional model overrides from
* `analysis.modelIntent` / `analysis.modelOverride` config.
* write memory and skills directly.
*
* Both triggers route the agent loop through `callSite: 'analyzeConversation'`
* so per-call provider/model selection flows through `resolveCallSiteConfig`
* against `llm.callSites.analyzeConversation` (populated by the unify-llm
* workspace migration from the legacy `analysis.modelIntent`/`modelOverride`).
Comment on lines +18 to +21

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.

🔴 Module docblock references "legacy" removed config fields, violating AGENTS.md rule

The new module-level comment at assistant/src/runtime/services/analyze-conversation.ts:20-21 says "populated by the unify-llm workspace migration from the legacy analysis.modelIntent/modelOverride". This narrates history and references removed config fields, violating the assistant/AGENTS.md:21 rule: "Comments should describe the current state of the codebase, not narrate its history."

Suggested change
* Both triggers route the agent loop through `callSite: 'analyzeConversation'`
* so per-call provider/model selection flows through `resolveCallSiteConfig`
* against `llm.callSites.analyzeConversation` (populated by the unify-llm
* workspace migration from the legacy `analysis.modelIntent`/`modelOverride`).
* Both triggers route the agent loop through `callSite: 'analyzeConversation'`
* so per-call provider/model selection flows through `resolveCallSiteConfig`
* against `llm.callSites.analyzeConversation`.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

*/
import { getConfig } from "../../config/loader.js";
import type { ServerMessage } from "../../daemon/message-protocol.js";
import {
AUTO_ANALYSIS_GROUP_ID,
Expand All @@ -31,7 +34,6 @@ import {
getMessages,
} from "../../memory/conversation-crud.js";
import { resolveConversationId } from "../../memory/conversation-key-store.js";
import type { ModelIntent } from "../../providers/types.js";
import { getLogger } from "../../util/logger.js";
import { buildAssistantEvent } from "../assistant-event.js";
import { DAEMON_INTERNAL_ASSISTANT_ID } from "../assistant-scope.js";
Expand Down Expand Up @@ -173,8 +175,6 @@ export async function analyzeConversation(
let prompt: string;
let trustClass: "unknown" | "guardian";
let stripTools: boolean;
let modelIntent: ModelIntent | undefined;
let modelOverride: string | undefined;

if (opts.trigger === "manual") {
const newConv = createConversation({
Expand Down Expand Up @@ -205,10 +205,6 @@ export async function analyzeConversation(
prompt = buildAutoAnalysisPrompt(transcript);
trustClass = "guardian";
stripTools = false;

const analysisConfig = getConfig().analysis;
modelIntent = analysisConfig.modelIntent;
modelOverride = analysisConfig.modelOverride;
}

// h. Load the conversation into memory with the appropriate trust
Expand All @@ -219,13 +215,14 @@ export async function analyzeConversation(
// Hoisted ahead of message persistence so the auto branch can detect a
// still-running prior agent loop on the rolling conversation and bail out
// before mutating any state. See concurrency guard below.
//
// Per-call model selection is no longer threaded through `getOrCreateConversation`;
// the runAgentLoop call below passes `callSite: 'analyzeConversation'` so the
// unified call-site resolver picks up provider/model from
// `llm.callSites.analyzeConversation`.
Comment on lines +219 to +222

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.

🔴 Comment references removed code with "no longer" phrasing, violating AGENTS.md rule

The new comment at assistant/src/runtime/services/analyze-conversation.ts:219 says "Per-call model selection is no longer threaded through getOrCreateConversation". The mandatory rule in assistant/AGENTS.md:21 states: "When writing or updating comments, do not reference code that has been removed. … Avoid phrases like 'no longer does X', 'previously used Y', or 'was removed in PR Z'".

Suggested change
// Per-call model selection is no longer threaded through `getOrCreateConversation`;
// the runAgentLoop call below passes `callSite: 'analyzeConversation'` so the
// unified call-site resolver picks up provider/model from
// `llm.callSites.analyzeConversation`.
// The runAgentLoop call below passes `callSite: 'analyzeConversation'` so the
// unified call-site resolver picks up provider/model from
// `llm.callSites.analyzeConversation`.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

const analysisConversation =
await deps.sendMessageDeps.getOrCreateConversation(
analysisConversationId,
{
...(modelIntent !== undefined ? { modelIntent } : {}),
...(modelOverride !== undefined ? { modelOverride } : {}),
},
);
Comment on lines 223 to 226

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.

🚩 getOrCreateConversation called without second argument — verify no downstream impact

The call to getOrCreateConversation at line 224 now omits the second options argument entirely (previously it passed modelIntent/modelOverride). This is safe because the call-site resolver in the provider layer handles model selection when callSite is set on the runAgentLoop options. However, if any code path inside getOrCreateConversation previously used modelIntent/modelOverride to configure the conversation object at creation time (e.g., setting a default provider on the conversation instance), that configuration would be silently lost. Worth a quick check that getOrCreateConversation's second parameter only affected provider selection and nothing else.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines 223 to 226

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve legacy analysis model override behavior

This change removes the per-run analysis.modelIntent/analysis.modelOverride read path, so auto-analysis now relies entirely on llm.callSites.analyzeConversation. Because the legacy analysis.* keys are still valid config fields and workspace migrations run at startup (not on later config edits), updates that still write analysis.* are now silently ignored, whereas they previously applied on the next run; that can unexpectedly change model selection, latency, and cost.

Useful? React with 👍 / 👎.


// h.1. Concurrency guard (auto trigger only). The rolling analysis
Expand Down Expand Up @@ -298,11 +295,14 @@ export async function analyzeConversation(
analysisConversation.abortController = new AbortController();
analysisConversation.currentRequestId = crypto.randomUUID();

// l. Fire-and-forget the agent loop
// l. Fire-and-forget the agent loop. `callSite: 'analyzeConversation'`
// routes the per-call provider config through `resolveCallSiteConfig`
// against `llm.callSites.analyzeConversation`.
analysisConversation
.runAgentLoop(prompt, messageId, onEvent, {
isInteractive: false,
isUserMessage: true,
callSite: "analyzeConversation",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid routing manual analysis through auto callsite

The new unconditional callSite: "analyzeConversation" applies to both manual and auto triggers, but the previous behavior only applied analysis-specific model overrides in the auto path. If llm.callSites.analyzeConversation is tuned for background auto-analysis (for example, a cheaper model), manual user-triggered analysis now inherits that setting and can regress output quality or fail under provider/model assumptions that were only intended for auto runs.

Useful? React with 👍 / 👎.

})
.catch((err) => {
log.error(
Expand Down
Loading