mirror of
https://github.com/garrytan/gbrain.git
synced 2026-08-14 00:48:18 +00:00
fix(gateway): brainstorm/propose_takes model-config takeovers — configured-model cost preview, judge config key, provider-probe skip, narrow page projection (#3120)
* fix(cycle): propose_takes skips cleanly when the chat provider is unavailable + narrow page projection Takeover of PR #1979 by @shawnduggan. The original PR gated on a hardcoded ANTHROPIC_API_KEY heuristic (modelNeedsAnthropicKey defaulting to true), which master deliberately removed elsewhere — it misclassified non-Anthropic stacks and fought the tier-config model resolution. This lands the intent the master-blessed way: probe the RESOLVED chat model (opts.model ?? getChatModel()) via probeChatModel — same semantics as patterns.ts / think/index.ts — and skip the phase cheaply when the provider can't run. Injected extractors are never gated. Also keeps the PR's uncontested half: load proposal candidates with a narrow projection (slug, source_id, compiled_truth) instead of listPages' SELECT p.*, preserving sourceIds > sourceId scope precedence and updated_desc ordering. Co-authored-by: shawnduggan <shawnduggan@users.noreply.github.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(brainstorm): price cost preview against the configured chat model + models.brainstorm.judge config key Takeover of PR #1855 by @starm2010, shrunk to the brainstorm-only portion (the cycle-phase hunks are superseded by the resolveModel-in- phase approach already on master). The cost preview + hard cost ceiling previously always priced anthropic:claude-sonnet-4-6 even when the configured chat_model (which the gateway actually runs) was something else; modelStr now resolves override → config.chat_model → fallback. The judge phase honors a new models.brainstorm.judge config key when no --judge-model flag is passed, resolved in the orchestrator so every caller (brainstorm, lsd, eval-brainstorm) benefits. Co-authored-by: starm2010 <starm2010@users.noreply.github.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(test): use withEnv()/emptyHome() in propose-takes no-key tests check:test-isolation R1 flagged direct process.env mutation in the two new no-key tests. Swap the hand-rolled save/mutate/restore for the canonical withEnv() helper (+ emptyHome() for the hermetic GBRAIN_HOME). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Garry Tan <garrytan@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
Garry Tan
parent
97df1e78b7
commit
ca47c054b8
@@ -486,9 +486,44 @@ const DEFAULT_PARALLELISM = 4;
|
||||
* src/core/errors.ts (the v0.19.0 envelope every new agent-facing
|
||||
* surface uses) rather than introducing a new BrainstormError class.
|
||||
*/
|
||||
/** File-config slice the orchestrator reads (see loadConfig in core/config.ts). */
|
||||
export interface BrainstormRunConfig {
|
||||
embedding_model?: string;
|
||||
chat_model?: string;
|
||||
emotional_weight?: { user_holder?: string };
|
||||
}
|
||||
|
||||
/**
|
||||
* Model used for the cost preview + hard cost ceiling. Mirrors what the
|
||||
* gateway will actually run: explicit --model override, else the configured
|
||||
* chat_model (gateway default), else the hardcoded gateway fallback. Before
|
||||
* this resolved through config, a non-Sonnet chat_model got its preview
|
||||
* priced against the wrong model. (Takeover of PR #1855 by @starm2010.)
|
||||
*/
|
||||
export function resolveBrainstormChatModel(
|
||||
config: { chat_model?: string },
|
||||
modelOverride?: string,
|
||||
): string {
|
||||
return modelOverride ?? config.chat_model ?? 'anthropic:claude-sonnet-4-6';
|
||||
}
|
||||
|
||||
/**
|
||||
* Judge-phase model precedence: --judge-model flag, else the
|
||||
* `models.brainstorm.judge` config key, else undefined (falls back to
|
||||
* `modelOverride` then the gateway default at the runJudge callsite).
|
||||
*/
|
||||
export async function resolveBrainstormJudgeModel(
|
||||
engine: BrainEngine,
|
||||
judgeModelFlag?: string,
|
||||
): Promise<string | undefined> {
|
||||
if (judgeModelFlag) return judgeModelFlag;
|
||||
const configured = await engine.getConfig('models.brainstorm.judge');
|
||||
return configured ?? undefined;
|
||||
}
|
||||
|
||||
export async function runBrainstorm(
|
||||
engine: BrainEngine,
|
||||
config: { embedding_model?: string; emotional_weight?: { user_holder?: string } },
|
||||
config: BrainstormRunConfig,
|
||||
opts: BrainstormOptions
|
||||
): Promise<BrainstormResult> {
|
||||
// v0.39.3.0 (Phase 5, CV11+T4): outer try/catch around the orchestrator
|
||||
@@ -510,7 +545,7 @@ export async function runBrainstorm(
|
||||
|
||||
async function runBrainstormImpl(
|
||||
engine: BrainEngine,
|
||||
config: { embedding_model?: string; emotional_weight?: { user_holder?: string } },
|
||||
config: BrainstormRunConfig,
|
||||
opts: BrainstormOptions,
|
||||
): Promise<BrainstormResult> {
|
||||
// v0.39.0.0 T10: install a gateway-layer BudgetTracker scope around the
|
||||
@@ -530,7 +565,7 @@ async function runBrainstormImpl(
|
||||
|
||||
async function _runBrainstormInner(
|
||||
engine: BrainEngine,
|
||||
config: { embedding_model?: string; emotional_weight?: { user_holder?: string } },
|
||||
config: BrainstormRunConfig,
|
||||
opts: BrainstormOptions,
|
||||
): Promise<BrainstormResult> {
|
||||
const profile = opts.profile ?? BRAINSTORM_PROFILE;
|
||||
@@ -539,7 +574,7 @@ async function _runBrainstormInner(
|
||||
const embedFn = opts.embedQueryFn ?? embedQuery;
|
||||
|
||||
// ---- Phase 0: cost preview + TTY grace ----
|
||||
const modelStr = opts.modelOverride ?? 'anthropic:claude-sonnet-4-6';
|
||||
const modelStr = resolveBrainstormChatModel(config, opts.modelOverride);
|
||||
const { aborted, estimate } = await previewCostAndWait({
|
||||
profile,
|
||||
model: modelStr,
|
||||
@@ -848,7 +883,7 @@ async function _runBrainstormInner(
|
||||
far_slug: i.far_slug,
|
||||
}));
|
||||
const judgeResult = await runJudge(profile.judge_config, judgeInput, {
|
||||
modelOverride: opts.judgeModel ?? opts.modelOverride,
|
||||
modelOverride: (await resolveBrainstormJudgeModel(engine, opts.judgeModel)) ?? opts.modelOverride,
|
||||
chatFn: opts.chatFn,
|
||||
activeBiasTags: activeBiasTags ?? undefined,
|
||||
abortSignal: opts.abortSignal,
|
||||
|
||||
@@ -962,6 +962,7 @@ export const KNOWN_CONFIG_KEYS: readonly string[] = [
|
||||
'models.subagent',
|
||||
'models.expansion',
|
||||
'models.chat',
|
||||
'models.brainstorm.judge',
|
||||
'models.eval.longmemeval',
|
||||
'facts.extraction_model',
|
||||
// #2113: output-token cap for the per-turn facts extractor (default 4000).
|
||||
|
||||
@@ -39,11 +39,11 @@
|
||||
|
||||
import { randomUUID, createHash } from 'node:crypto';
|
||||
import { BaseCyclePhase, type ScopedReadOpts, type BasePhaseOpts } from './base-phase.ts';
|
||||
import { chat as gatewayChat, getChatModel } from '../ai/gateway.ts';
|
||||
import { chat as gatewayChat, getChatModel, probeChatModel } from '../ai/gateway.ts';
|
||||
import { normalizeModelId } from '../model-id.ts';
|
||||
import { writeReceipt } from '../extract/receipt-writer.ts';
|
||||
import { upsertExtractRollup } from '../extract/rollup-writer.ts';
|
||||
import { GBrainError } from '../types.ts';
|
||||
import type { Page, PageFilters } from '../types.ts';
|
||||
import type { OperationContext } from '../operations.ts';
|
||||
import type { BrainEngine } from '../engine.ts';
|
||||
import type { PhaseStatus, CyclePhase } from '../cycle.ts';
|
||||
@@ -160,6 +160,48 @@ export interface ProposeTakesResult {
|
||||
warnings: string[];
|
||||
}
|
||||
|
||||
/** Narrow projection of `pages` — the only columns this phase reads. */
|
||||
interface ProposeTakesPageRow {
|
||||
slug: string;
|
||||
source_id: string;
|
||||
compiled_truth: string | null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Load proposal candidates with a narrow projection instead of
|
||||
* `engine.listPages` (`SELECT p.*`). The phase only reads slug, source_id
|
||||
* and compiled_truth — skipping timeline/frontmatter/title keeps large
|
||||
* toasted columns out of the hot path. Scope precedence mirrors
|
||||
* `sourceScopeOpts`: federated array (`sourceIds`) beats scalar
|
||||
* (`sourceId`); ordering matches `PAGE_SORT_SQL.updated_desc` with an id
|
||||
* tiebreak for determinism. (Takeover of PR #1979's projection by
|
||||
* @shawnduggan.)
|
||||
*/
|
||||
async function listCandidatePages(
|
||||
engine: BrainEngine,
|
||||
scope: ScopedReadOpts,
|
||||
limit: number,
|
||||
): Promise<ProposeTakesPageRow[]> {
|
||||
const where = ['deleted_at IS NULL'];
|
||||
const params: unknown[] = [];
|
||||
if (scope.sourceIds && scope.sourceIds.length > 0) {
|
||||
params.push(scope.sourceIds);
|
||||
where.push(`source_id = ANY($${params.length}::text[])`);
|
||||
} else if (scope.sourceId) {
|
||||
params.push(scope.sourceId);
|
||||
where.push(`source_id = $${params.length}`);
|
||||
}
|
||||
params.push(limit);
|
||||
return engine.executeRaw<ProposeTakesPageRow>(
|
||||
`SELECT slug, source_id, compiled_truth
|
||||
FROM pages
|
||||
WHERE ${where.join(' AND ')}
|
||||
ORDER BY updated_at DESC, id DESC
|
||||
LIMIT $${params.length}`,
|
||||
params,
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Compute the content_hash key for the idempotency cache. SHA-256 of the
|
||||
* page body suffices — page slug + prompt_version are separate columns in
|
||||
@@ -330,6 +372,34 @@ class ProposeTakesPhase extends BaseCyclePhase {
|
||||
const phaseStartMs = Date.now();
|
||||
const proposalRunId = `propose-${new Date().toISOString().slice(0, 19).replace(/[-:T]/g, '')}-${randomUUID().slice(0, 8)}`;
|
||||
|
||||
const modelId = opts.model ?? getChatModel();
|
||||
|
||||
// With the default (gateway) extractor, skip cheaply when the resolved
|
||||
// model's provider can't run — same probe semantics as patterns.ts /
|
||||
// think/index.ts: unknown provider/model or Anthropic-without-key skips;
|
||||
// other providers' auth surfaces lazily at chat() time. An injected
|
||||
// extractor bypasses the gateway, so it is never gated. (Takeover of
|
||||
// PR #1979's intent by @shawnduggan.)
|
||||
if (!opts.extractor) {
|
||||
const probe = probeChatModel(normalizeModelId(modelId));
|
||||
if (!probe.ok) {
|
||||
return {
|
||||
summary: `propose_takes skipped: ${probe.detail}`,
|
||||
details: {
|
||||
reason: 'no_provider',
|
||||
model: modelId,
|
||||
pages_scanned: 0,
|
||||
cache_hits: 0,
|
||||
cache_misses: 0,
|
||||
proposals_inserted: 0,
|
||||
budget_exhausted: false,
|
||||
warnings: [],
|
||||
},
|
||||
status: 'skipped',
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
const result: ProposeTakesResult = {
|
||||
pages_scanned: 0,
|
||||
cache_hits: 0,
|
||||
@@ -340,19 +410,12 @@ class ProposeTakesPhase extends BaseCyclePhase {
|
||||
};
|
||||
|
||||
// Load pages eligible for proposal. Source-scoped per BaseCyclePhase.
|
||||
const pageFilters: PageFilters = {
|
||||
...scope,
|
||||
limit: pageLimit,
|
||||
sort: 'updated_desc',
|
||||
};
|
||||
const pages: Page[] = await engine.listPages(pageFilters);
|
||||
const pages = await listCandidatePages(engine, scope, pageLimit);
|
||||
|
||||
if (opts.reporter) {
|
||||
opts.reporter.start('propose_takes.pages' as never, pages.length);
|
||||
}
|
||||
|
||||
const modelId = opts.model ?? getChatModel();
|
||||
|
||||
for (const page of pages) {
|
||||
// Phase deadline check. Break (not throw) so the phase returns a
|
||||
// partial result with deadline_hit:true; work already banked stays.
|
||||
@@ -509,4 +572,5 @@ export const __testing = {
|
||||
contentHash,
|
||||
hasCompleteFence,
|
||||
extractExistingTakesForDedup,
|
||||
listCandidatePages,
|
||||
};
|
||||
|
||||
@@ -0,0 +1,65 @@
|
||||
/**
|
||||
* Brainstorm model configurability (takeover of PR #1855 by @starm2010).
|
||||
*
|
||||
* - The cost preview + hard cost ceiling price the model that will actually
|
||||
* run: --model override → configured chat_model → gateway fallback. Before
|
||||
* this, the preview always priced anthropic:claude-sonnet-4-6 even when
|
||||
* the configured chat_model was something else.
|
||||
* - The judge phase honors the `models.brainstorm.judge` config key when no
|
||||
* --judge-model flag is passed.
|
||||
*/
|
||||
|
||||
import { describe, test, expect } from 'bun:test';
|
||||
import {
|
||||
resolveBrainstormChatModel,
|
||||
resolveBrainstormJudgeModel,
|
||||
} from '../../src/core/brainstorm/orchestrator.ts';
|
||||
import type { BrainEngine } from '../../src/core/engine.ts';
|
||||
|
||||
function mockEngine(configValues: Record<string, string>): { engine: BrainEngine; reads: string[] } {
|
||||
const reads: string[] = [];
|
||||
const engine = {
|
||||
async getConfig(key: string): Promise<string | null> {
|
||||
reads.push(key);
|
||||
return configValues[key] ?? null;
|
||||
},
|
||||
} as unknown as BrainEngine;
|
||||
return { engine, reads };
|
||||
}
|
||||
|
||||
describe('resolveBrainstormChatModel', () => {
|
||||
test('--model override wins over config', () => {
|
||||
expect(resolveBrainstormChatModel({ chat_model: 'openai:gpt-5' }, 'anthropic:claude-opus-4-6'))
|
||||
.toBe('anthropic:claude-opus-4-6');
|
||||
});
|
||||
|
||||
test('configured chat_model wins over the hardcoded fallback', () => {
|
||||
expect(resolveBrainstormChatModel({ chat_model: 'openai:gpt-5' }))
|
||||
.toBe('openai:gpt-5');
|
||||
});
|
||||
|
||||
test('falls back to the gateway default model when nothing is configured', () => {
|
||||
expect(resolveBrainstormChatModel({})).toBe('anthropic:claude-sonnet-4-6');
|
||||
});
|
||||
});
|
||||
|
||||
describe('resolveBrainstormJudgeModel', () => {
|
||||
test('--judge-model flag wins without touching config', async () => {
|
||||
const { engine, reads } = mockEngine({ 'models.brainstorm.judge': 'openai:gpt-5' });
|
||||
const out = await resolveBrainstormJudgeModel(engine, 'anthropic:claude-opus-4-6');
|
||||
expect(out).toBe('anthropic:claude-opus-4-6');
|
||||
expect(reads).toHaveLength(0);
|
||||
});
|
||||
|
||||
test('models.brainstorm.judge config key is honored when no flag is passed', async () => {
|
||||
const { engine, reads } = mockEngine({ 'models.brainstorm.judge': 'openai:gpt-5' });
|
||||
const out = await resolveBrainstormJudgeModel(engine);
|
||||
expect(out).toBe('openai:gpt-5');
|
||||
expect(reads).toEqual(['models.brainstorm.judge']);
|
||||
});
|
||||
|
||||
test('returns undefined (defer to modelOverride / gateway default) when unset', async () => {
|
||||
const { engine } = mockEngine({});
|
||||
expect(await resolveBrainstormJudgeModel(engine)).toBeUndefined();
|
||||
});
|
||||
});
|
||||
@@ -15,6 +15,7 @@
|
||||
*/
|
||||
|
||||
import { describe, test, expect } from 'bun:test';
|
||||
import { withEnv, emptyHome } from './helpers/with-env.ts';
|
||||
import {
|
||||
runPhaseProposeTakes,
|
||||
parseExtractorOutput,
|
||||
@@ -52,6 +53,14 @@ function buildMockEngine(opts: {
|
||||
},
|
||||
async executeRaw<T>(sql: string, params?: unknown[]): Promise<T[]> {
|
||||
captured.push({ sql, params: params ?? [] });
|
||||
// Narrow candidate-page projection (replaces listPages in the phase).
|
||||
if (sql.includes('SELECT slug, source_id, compiled_truth')) {
|
||||
return opts.pages.map((p) => ({
|
||||
slug: p.slug,
|
||||
source_id: p.source_id,
|
||||
compiled_truth: p.compiled_truth,
|
||||
})) as T[];
|
||||
}
|
||||
// SELECT idempotency check
|
||||
if (sql.includes('SELECT id FROM take_proposals')) {
|
||||
const [sourceId, slug, ch, pv] = params ?? [];
|
||||
@@ -476,4 +485,72 @@ New prose appended here.`;
|
||||
resetGateway();
|
||||
}
|
||||
});
|
||||
|
||||
test('default extractor skips cleanly when the Anthropic chat model has no key', async () => {
|
||||
// Empty GBRAIN_HOME so hasAnthropicKey's config-file fallback can't find
|
||||
// the operator's real key.
|
||||
await withEnv({ GBRAIN_HOME: emptyHome(), ANTHROPIC_API_KEY: undefined }, async () => {
|
||||
configureGateway({ chat_model: 'anthropic:claude-sonnet-4-6', env: {} });
|
||||
try {
|
||||
const { engine, captured } = buildMockEngine({
|
||||
pages: [buildPage({ slug: 'wiki/a', body: 'claim-ish prose' })],
|
||||
});
|
||||
const result = await runPhaseProposeTakes(buildCtx(engine));
|
||||
|
||||
expect(result.status).toBe('skipped');
|
||||
expect((result.details as Record<string, unknown>).reason).toBe('no_provider');
|
||||
// Skips BEFORE touching the engine — no page scan, no cache probes.
|
||||
expect(captured).toHaveLength(0);
|
||||
} finally {
|
||||
resetGateway();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
test('an injected extractor is never gated on provider availability', async () => {
|
||||
await withEnv({ GBRAIN_HOME: emptyHome(), ANTHROPIC_API_KEY: undefined }, async () => {
|
||||
configureGateway({ chat_model: 'anthropic:claude-sonnet-4-6', env: {} });
|
||||
try {
|
||||
const { engine } = buildMockEngine({
|
||||
pages: [buildPage({ slug: 'wiki/b', body: 'still processed' })],
|
||||
});
|
||||
const extractor: ProposeTakesExtractor = async () => [];
|
||||
const result = await runPhaseProposeTakes(buildCtx(engine), { extractor });
|
||||
|
||||
expect(result.status).toBe('ok');
|
||||
expect((result.details as Record<string, unknown>).pages_scanned).toBe(1);
|
||||
} finally {
|
||||
resetGateway();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
test('loads proposal candidates with a narrow page projection', async () => {
|
||||
const pages = [buildPage({ slug: 'wiki/narrow', body: 'A narrow projection avoids unrelated page columns.' })];
|
||||
const { engine, captured } = buildMockEngine({ pages });
|
||||
const extractor: ProposeTakesExtractor = async () => [];
|
||||
await runPhaseProposeTakes(buildCtx(engine), { extractor });
|
||||
|
||||
const pageSelect = captured.find(c => c.sql.includes('FROM pages'));
|
||||
expect(pageSelect).toBeDefined();
|
||||
expect(pageSelect!.sql).toContain('SELECT slug, source_id, compiled_truth');
|
||||
expect(pageSelect!.sql).not.toContain('*');
|
||||
// Scalar sourceId scope from ctx binds as a plain equality param.
|
||||
expect(pageSelect!.params[0]).toBe('default');
|
||||
});
|
||||
|
||||
test('narrow projection: federated sourceIds beat scalar sourceId', async () => {
|
||||
const { engine, captured } = buildMockEngine({ pages: [] });
|
||||
const extractor: ProposeTakesExtractor = async () => [];
|
||||
const ctx = {
|
||||
...buildCtx(engine),
|
||||
auth: { allowedSources: ['team-a', 'team-b'] },
|
||||
} as OperationContext;
|
||||
await runPhaseProposeTakes(ctx, { extractor });
|
||||
|
||||
const pageSelect = captured.find(c => c.sql.includes('FROM pages'));
|
||||
expect(pageSelect).toBeDefined();
|
||||
expect(pageSelect!.sql).toContain('source_id = ANY(');
|
||||
expect(pageSelect!.params[0]).toEqual(['team-a', 'team-b']);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user