Skip to content

Commit e77c00f

Browse files
authored
Merge branch 'main' into refactor/advisor-synthetic-context
2 parents 2588aa3 + 39fd60a commit e77c00f

10 files changed

Lines changed: 149 additions & 22 deletions

File tree

.github/workflows/e2e-advisor.yaml

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -163,8 +163,6 @@ jobs:
163163
E2E_ADVISOR_RUN_ANALYSIS: ${{ github.event_name == 'workflow_dispatch' && inputs.run_analysis == false && '0' || '1' }}
164164
# Preferred E2E advisor secret.
165165
E2E_ADVISOR_API_KEY: ${{ secrets.PI_E2E_ADVISOR_API_KEY }}
166-
# Optional local-provider-compatible fallback for future upstream/external-advisor use.
167-
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
168166
run: |
169167
cd "$ADVISOR_WORKDIR"
170168
node --experimental-strip-types "$ADVISOR_DIR/tools/e2e-advisor/analyze.mts" \
@@ -183,8 +181,6 @@ jobs:
183181
# Reuse the shared E2E advisor secret. The scenario advisor is a
184182
# separate prompt/agent but uses the same model and credential.
185183
E2E_ADVISOR_API_KEY: ${{ secrets.PI_E2E_ADVISOR_API_KEY }}
186-
# Optional local-provider-compatible fallback for future upstream/external-advisor use.
187-
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
188184
run: |
189185
cd "$ADVISOR_WORKDIR"
190186
node --experimental-strip-types "$ADVISOR_DIR/tools/e2e-advisor/scenarios.mts" \

.github/workflows/pr-review-advisor.yaml

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,8 +140,7 @@ jobs:
140140
PR_NUMBER: ${{ github.event.pull_request.number || inputs.target_pr }}
141141
PR_REVIEW_ADVISOR_RUN_ANALYSIS: ${{ github.event_name == 'workflow_dispatch' && inputs.run_analysis == false && '0' || '1' }}
142142
GH_TOKEN: ${{ github.token }}
143-
PR_REVIEW_ADVISOR_API_KEY: ${{ secrets.PR_REVIEW_ADVISOR_API_KEY || secrets.PI_PR_REVIEW_ADVISOR_API_KEY }}
144-
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
143+
PR_REVIEW_ADVISOR_API_KEY: ${{ secrets.PR_REVIEW_ADVISOR_API_KEY }}
145144
run: |
146145
cd "$ADVISOR_WORKDIR"
147146
if [ ! -f "$ADVISOR_DIR/tools/pr-review-advisor/analyze.mts" ]; then

test/pr-review-advisor.test.ts

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,12 @@ import path from "node:path";
66
import Ajv2020 from "ajv/dist/2020.js";
77
import { afterEach, describe, expect, it, vi } from "vitest";
88
import { githubGraphql } from "../tools/advisors/github.mts";
9+
import {
10+
ADVISOR_OPENAI_COMPATIBLE_BASE_URL,
11+
DEFAULT_ADVISOR_MODEL,
12+
DEFAULT_ADVISOR_PROVIDER,
13+
openAiAdvisorProviderConfig,
14+
} from "../tools/advisors/session.mts";
915
import {
1016
buildPromptTurns,
1117
buildSystemPrompt,
@@ -126,6 +132,34 @@ describe("PR review advisor", () => {
126132
afterEach(() => {
127133
vi.restoreAllMocks();
128134
});
135+
136+
it("configures the advisor through the hosted OpenAI-compatible service", () => {
137+
const config = openAiAdvisorProviderConfig("PR_REVIEW_ADVISOR_API_KEY") as {
138+
apiKey: string;
139+
baseUrl: string;
140+
models: Array<{
141+
id: string;
142+
compat?: Record<string, unknown>;
143+
reasoning: boolean;
144+
}>;
145+
};
146+
147+
expect(DEFAULT_ADVISOR_PROVIDER).toBe("openai");
148+
expect(DEFAULT_ADVISOR_MODEL).toBe("openai/openai/gpt-5.5");
149+
expect(config.apiKey).toBe("PR_REVIEW_ADVISOR_API_KEY");
150+
expect(config.baseUrl).toBe(ADVISOR_OPENAI_COMPATIBLE_BASE_URL);
151+
expect(config.models[0]?.id).toBe(DEFAULT_ADVISOR_MODEL);
152+
expect(config.models[0]?.reasoning).toBe(false);
153+
expect(config.models[0]?.compat).toMatchObject({
154+
supportsDeveloperRole: false,
155+
supportsReasoningEffort: false,
156+
supportsStore: false,
157+
supportsStrictMode: false,
158+
supportsUsageInStreaming: false,
159+
maxTokensField: "max_tokens",
160+
});
161+
});
162+
129163
it("normalizes advisor output into the schema-owned metadata", () => {
130164
const result = normalizeReviewResult(validResult(), metadata());
131165

@@ -632,6 +666,13 @@ jobs:
632666
ref: refs/pull/\${{ github.event.pull_request.head.sha }}/merge
633667
path: pr-workdir
634668
persist-credentials: false
669+
- name: Run PR review advisor
670+
env:
671+
PR_REVIEW_ADVISOR_API_KEY: \${{ secrets.PR_REVIEW_ADVISOR_API_KEY || secrets.PI_PR_REVIEW_ADVISOR_API_KEY }}
672+
OPENAI_API_KEY: \${{ secrets.OPENAI_API_KEY }}
673+
run: |
674+
cd "$ADVISOR_WORKDIR"
675+
node "$ADVISOR_DIR/tools/pr-review-advisor/analyze.mts" --schema "$ADVISOR_DIR/tools/pr-review-advisor/schema.json"
635676
`,
636677
);
637678

@@ -644,6 +685,8 @@ jobs:
644685
"workflow permissions.contents must be read",
645686
"review job must not be globally continue-on-error",
646687
"PR checkout must use the pull request head SHA as inert analysis data",
688+
"Run PR review advisor must receive PR_REVIEW_ADVISOR_API_KEY only from secrets.PR_REVIEW_ADVISOR_API_KEY",
689+
"Run PR review advisor must not receive OPENAI_API_KEY",
647690
]),
648691
);
649692
expect(errors.some((error) => error.includes("full commit SHA"))).toBe(true);

tools/advisors/session.mts

Lines changed: 72 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {
1616

1717
export const DEFAULT_ADVISOR_PROVIDER = "openai";
1818
export const DEFAULT_ADVISOR_MODEL = "openai/openai/gpt-5.5";
19+
export const ADVISOR_OPENAI_COMPATIBLE_BASE_URL = "https://inference-api.nvidia.com/v1";
1920
export const READ_ONLY_TOOLS = ["read", "grep", "find", "ls"];
2021

2122
const ZERO_COST = { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 };
@@ -29,12 +30,14 @@ const ZERO_USAGE = {
2930
};
3031

3132
type AdvisorProviderConfig = Parameters<ModelRegistry["registerProvider"]>[1];
33+
type AdvisorModelConfig = NonNullable<AdvisorProviderConfig["models"]>[number];
3234

3335
export type RunAdvisorResult = {
3436
/** Assistant text from the final turn. For single-turn callers, this is the full response. */
3537
text: string;
3638
raw: string;
3739
turnTexts: string[];
40+
turnErrors: string[];
3841
};
3942

4043
export type AdvisorSyntheticToolContentType = "diff" | "json" | "text";
@@ -84,9 +87,16 @@ export type RunReadOnlyAdvisorOptions = {
8487
export function openAiAdvisorProviderConfig(credentialEnv: string): AdvisorProviderConfig {
8588
return {
8689
api: "openai-completions",
87-
baseUrl: "https://integrate.api.nvidia.com/v1",
90+
baseUrl: ADVISOR_OPENAI_COMPATIBLE_BASE_URL,
8891
models: [
89-
advisorModel(DEFAULT_ADVISOR_MODEL, "GPT-5.5", 256000, 32768, true, ["text", "image"]),
92+
advisorModel(DEFAULT_ADVISOR_MODEL, "GPT-5.5", 256000, 32768, false, ["text", "image"], {
93+
supportsDeveloperRole: false,
94+
supportsReasoningEffort: false,
95+
supportsStore: false,
96+
supportsStrictMode: false,
97+
supportsUsageInStreaming: false,
98+
maxTokensField: "max_tokens",
99+
}),
90100
],
91101
["api" + "Key"]: credentialEnv,
92102
} as AdvisorProviderConfig;
@@ -99,8 +109,9 @@ export function advisorModel(
99109
maxTokens: number,
100110
reasoning: boolean,
101111
input: ("text" | "image")[],
102-
): NonNullable<AdvisorProviderConfig["models"]>[number] {
103-
return { id, name, reasoning, input, cost: ZERO_COST, contextWindow, maxTokens };
112+
compat?: AdvisorModelConfig["compat"],
113+
): AdvisorModelConfig {
114+
return { id, name, reasoning, input, cost: ZERO_COST, contextWindow, maxTokens, compat };
104115
}
105116

106117
export async function runReadOnlyAdvisor(
@@ -112,7 +123,9 @@ export async function runReadOnlyAdvisor(
112123
const { authStorage, modelRegistry } = prepareAdvisorConfig(provider, options.credentialEnv);
113124
const model = modelRegistry.find(provider, modelId);
114125
if (!model || !modelRegistry.hasConfiguredAuth(model)) {
115-
throw new Error(`Could not configure advisor model ${modelId}`);
126+
throw new Error(
127+
`Could not configure advisor model ${provider}/${modelId}; set ${options.credentialEnv}`,
128+
);
116129
}
117130

118131
const settingsManager = SettingsManager.inMemory({
@@ -154,20 +167,44 @@ export async function runReadOnlyAdvisor(
154167
const rawHeader = [
155168
modelFallbackMessage ? `[${options.logPrefix}] ${modelFallbackMessage}` : undefined,
156169
`[${options.logPrefix}] model=${model.provider}/${model.id}`,
170+
`[${options.logPrefix}] base_url=${model.baseUrl}`,
157171
`[${options.logPrefix}] tools=${READ_ONLY_TOOLS.join(",")}`,
158172
`[${options.logPrefix}] prompt_turns=${promptTurns.length}`,
159173
"--- ASSISTANT TEXT ---",
160174
].filter((line): line is string => Boolean(line));
161175

162176
const raw = new CappedBuffer(options.maxCaptureBytes, `${rawHeader.join("\n")}\n`);
163177
const turnTextBuffers: CappedBuffer[] = [];
178+
const turnErrors: string[] = [];
164179
let currentTurnText: CappedBuffer | undefined;
165180
let currentTurnName = "";
181+
let currentTurnError: string | undefined;
182+
183+
const captureTurnError = (source: string, message: string | undefined): void => {
184+
const normalized = normalizeProviderError(message);
185+
if (!normalized) return;
186+
currentTurnError ||= normalized;
187+
raw.append(`\n[${options.logPrefix}] ${source}: ${normalized}\n`);
188+
};
166189

167190
const unsubscribe = session.subscribe((event: AgentSessionEvent) => {
168-
if (event.type === "message_update" && event.assistantMessageEvent.type === "text_delta") {
169-
currentTurnText?.append(event.assistantMessageEvent.delta);
170-
raw.append(event.assistantMessageEvent.delta);
191+
if (event.type === "message_update") {
192+
if (event.assistantMessageEvent.type === "text_delta") {
193+
currentTurnText?.append(event.assistantMessageEvent.delta);
194+
raw.append(event.assistantMessageEvent.delta);
195+
return;
196+
}
197+
if (event.assistantMessageEvent.type === "error") {
198+
captureTurnError(
199+
"assistant_stream_error",
200+
event.assistantMessageEvent.error.errorMessage || event.assistantMessageEvent.reason,
201+
);
202+
return;
203+
}
204+
return;
205+
}
206+
if (event.type === "message_end") {
207+
captureTurnError("assistant_message_error", assistantMessageError(event.message));
171208
return;
172209
}
173210
if (event.type === "tool_execution_start") {
@@ -214,6 +251,7 @@ export async function runReadOnlyAdvisor(
214251
for (const [index, turn] of promptTurns.entries()) {
215252
currentTurnName = turn.name;
216253
currentTurnText = new CappedBuffer(options.maxCaptureBytes);
254+
currentTurnError = undefined;
217255
turnTextBuffers.push(currentTurnText);
218256
const turnIndex = `${index + 1}/${promptTurns.length}`;
219257
injectSyntheticToolResults({
@@ -232,6 +270,9 @@ export async function runReadOnlyAdvisor(
232270
raw.append(
233271
`\n[${options.logPrefix}] user_turn_end ${turnIndex} ${turn.name} textBytes=${turnTextBytes}\n`,
234272
);
273+
if (currentTurnError) {
274+
turnErrors.push(`${turn.name}: ${currentTurnError}`);
275+
}
235276
currentTurnText = undefined;
236277
currentTurnName = "";
237278
}
@@ -264,7 +305,28 @@ export async function runReadOnlyAdvisor(
264305
if (truncationNotes.length > 0) raw.appendFooter(`\n${truncationNotes.join("\n")}\n`);
265306

266307
const turnTexts = turnTextBuffers.map((buffer) => buffer.toString());
267-
return { text: turnTexts.at(-1) || "", raw: raw.toStringWithTrailingNewline(), turnTexts };
308+
return {
309+
text: turnTexts.at(-1) || "",
310+
raw: raw.toStringWithTrailingNewline(),
311+
turnTexts,
312+
turnErrors,
313+
};
314+
}
315+
316+
function assistantMessageError(message: unknown): string | undefined {
317+
if (!message || typeof message !== "object") return undefined;
318+
const record = message as { role?: unknown; stopReason?: unknown; errorMessage?: unknown };
319+
if (record.role !== "assistant") return undefined;
320+
if (record.stopReason !== "error" && record.stopReason !== "aborted") return undefined;
321+
return typeof record.errorMessage === "string" && record.errorMessage.trim()
322+
? record.errorMessage
323+
: String(record.stopReason);
324+
}
325+
326+
function normalizeProviderError(message: string | undefined): string | undefined {
327+
if (!message) return undefined;
328+
const normalized = message.trim().replace(/\s+/g, " ");
329+
return normalized || undefined;
268330
}
269331

270332
function normalizePromptTurns(promptTurns: AdvisorPromptTurn[]): AdvisorPromptTurn[] {
@@ -447,7 +509,7 @@ function prepareAdvisorConfig(
447509
): { authStorage: AuthStorage; modelRegistry: ModelRegistry } {
448510
const authStorage = AuthStorage.inMemory();
449511
const modelRegistry = ModelRegistry.inMemory(authStorage);
450-
const credential = process.env[credentialEnv] || process.env.OPENAI_API_KEY;
512+
const credential = process.env[credentialEnv]?.trim();
451513
if (credential) {
452514
authStorage.setRuntimeApiKey(provider, credential);
453515
modelRegistry.registerProvider(provider, openAiAdvisorProviderConfig(credentialEnv));

tools/e2e-advisor/README.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,8 @@ Configure this repository secret for E2E recommendations:
4444

4545
- `PI_E2E_ADVISOR_API_KEY`
4646

47-
The analyzer uses the fixed `openai/openai/gpt-5.5` advisor model and also accepts `OPENAI_API_KEY` for local runs.
47+
The analyzer uses the fixed `openai/openai/gpt-5.5` advisor model through the
48+
OpenAI-compatible `https://inference-api.nvidia.com/v1` service.
4849

4950
If advisor credentials are unavailable, the advisor writes a low-confidence unavailable result instead of
5051
making deterministic recommendations.
@@ -78,7 +79,8 @@ node --experimental-strip-types tools/e2e-advisor/analyze.mts \
7879
--out-dir artifacts/e2e-advisor
7980
```
8081

81-
Set `E2E_ADVISOR_API_KEY` or `OPENAI_API_KEY` locally, or configure the repository `PI_E2E_ADVISOR_API_KEY` secret. Run `npm install` first so the Pi SDK dependency is available.
82+
Set `E2E_ADVISOR_API_KEY` locally, or configure the repository `PI_E2E_ADVISOR_API_KEY`
83+
secret. Run `npm install` first so the Pi SDK dependency is available.
8284

8385
## Output contract
8486

tools/e2e-advisor/analyze.mts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,10 @@ async function main(): Promise<void> {
162162
"utf8",
163163
)}`,
164164
);
165+
if (sdkResult.turnErrors.length > 0) {
166+
writeFailure(`Advisor SDK provider error: ${sdkResult.turnErrors.join("; ")}`);
167+
process.exit(1);
168+
}
165169
} catch (error: unknown) {
166170
const reason = error instanceof Error ? error.message : String(error);
167171
fs.writeFileSync(artifacts.raw, `Advisor SDK execution failed: ${reason}\n`);

tools/e2e-advisor/scenarios.mts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,10 @@ async function main(): Promise<void> {
201201
"utf8",
202202
)}`,
203203
);
204+
if (sdkResult.turnErrors.length > 0) {
205+
writeFailure(`Scenario advisor SDK provider error: ${sdkResult.turnErrors.join("; ")}`);
206+
process.exit(1);
207+
}
204208
} catch (error: unknown) {
205209
const reason = error instanceof Error ? error.message : String(error);
206210
fs.writeFileSync(artifacts.raw, `Scenario advisor SDK execution failed: ${reason}\n`);

tools/pr-review-advisor/README.md

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,9 +52,8 @@ Configure this repository secret for review analysis:
5252

5353
- `PR_REVIEW_ADVISOR_API_KEY`
5454

55-
The workflow also accepts the legacy `PI_PR_REVIEW_ADVISOR_API_KEY` secret as a
56-
fallback. The analyzer uses the fixed `openai/openai/gpt-5.5` advisor model and
57-
also accepts `OPENAI_API_KEY` for local runs.
55+
The analyzer uses the fixed `openai/openai/gpt-5.5` advisor model through the
56+
OpenAI-compatible `https://inference-api.nvidia.com/v1` service.
5857

5958
If advisor credentials are unavailable, the advisor writes a low-confidence unavailable result
6059
instead of failing closed without artifacts.
@@ -94,7 +93,7 @@ node --experimental-strip-types tools/pr-review-advisor/analyze.mts \
9493
--out-dir artifacts/pr-review-advisor
9594
```
9695

97-
Set `PR_REVIEW_ADVISOR_API_KEY` or `OPENAI_API_KEY` locally, or configure the repository
96+
Set `PR_REVIEW_ADVISOR_API_KEY` locally, or configure the repository
9897
`PR_REVIEW_ADVISOR_API_KEY` secret. Run `npm install` first so the Pi SDK dependency is
9998
available.
10099

tools/pr-review-advisor/analyze.mts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,10 @@ async function main(): Promise<void> {
324324
});
325325
fs.writeFileSync(artifacts.raw, sdkResult.raw);
326326
logProgress(`PR review advisor conversation finished: turns=${sdkResult.turnTexts.length}`);
327+
if (sdkResult.turnErrors.length > 0) {
328+
writeFailure(`PR review advisor SDK provider error: ${sdkResult.turnErrors.join("; ")}`);
329+
process.exit(1);
330+
}
327331
} catch (error: unknown) {
328332
const reason = error instanceof Error ? error.message : String(error);
329333
fs.writeFileSync(artifacts.raw, `PR review advisor SDK execution failed: ${reason}\n`);

tools/pr-review-advisor/workflow-boundary.mts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,20 @@ export function validatePrReviewAdvisorWorkflowBoundary(
141141
requireRunContains(errors, analyze, 'cd "$ADVISOR_WORKDIR"');
142142
requireRunContains(errors, analyze, "$ADVISOR_DIR/tools/pr-review-advisor/analyze.mts");
143143
requireRunContains(errors, analyze, "$ADVISOR_DIR/tools/pr-review-advisor/schema.json");
144+
if (analyze) {
145+
const analyzeEnv = asRecord(analyze.env);
146+
if (
147+
stringValue(analyzeEnv.PR_REVIEW_ADVISOR_API_KEY).trim() !==
148+
"${{ secrets.PR_REVIEW_ADVISOR_API_KEY }}"
149+
) {
150+
errors.push(
151+
"Run PR review advisor must receive PR_REVIEW_ADVISOR_API_KEY only from secrets.PR_REVIEW_ADVISOR_API_KEY",
152+
);
153+
}
154+
if (Object.hasOwn(analyzeEnv, "OPENAI_API_KEY")) {
155+
errors.push("Run PR review advisor must not receive OPENAI_API_KEY");
156+
}
157+
}
144158

145159
const comment = requireStep(errors, steps, "Post PR review advisor comment");
146160
requireRunContains(errors, comment, "$ADVISOR_DIR/tools/pr-review-advisor/comment.mts");

0 commit comments

Comments
 (0)