From cf32e7957848d7a0548c1e0ba31e61053fc07e52 Mon Sep 17 00:00:00 2001 From: Mikael Hugo Date: Fri, 15 May 2026 17:13:40 +0200 Subject: [PATCH] feat(memory-embeddings): read SF_LLM_GATEWAY_KEY from env as auth.json fallback Enables CI and containerised deployments without writing secrets to disk. Auth.json still takes precedence when present. - readGatewayFromAuthJson now falls back to SF_LLM_GATEWAY_KEY env var - SF_LLM_GATEWAY_URL env var also supported for endpoint override - Added tests for env fallback, auth.json preference, and default URL Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/headless-feedback.ts | 292 ++++++++++++++++++ src/headless.ts | 19 ++ src/help-text.ts | 6 + .../extensions/sf/memory-embeddings.js | 32 +- .../memory-embeddings-llm-gateway.test.mjs | 40 ++- src/tests/headless-feedback.test.ts | 76 +++++ 6 files changed, 454 insertions(+), 11 deletions(-) create mode 100644 src/headless-feedback.ts create mode 100644 src/tests/headless-feedback.test.ts diff --git a/src/headless-feedback.ts b/src/headless-feedback.ts new file mode 100644 index 000000000..36be9aa8b --- /dev/null +++ b/src/headless-feedback.ts @@ -0,0 +1,292 @@ +/** + * Headless commands for the self-feedback subsystem: + * + * sf headless feedback add [flags] + * sf headless feedback list [--unresolved] [--severity ] [--json] + * sf headless feedback resolve --reason "..." [--evidence-kind ] + * + * Why these exist: today the only writers to the self_feedback table + * are SF's autonomous runtime and a handful of internal detectors. + * Operators (and Claude Code instances running outside the autonomous + * loop) have no path to file feedback without dropping to SQL. + * + * Same pattern as headless-mark-state.ts: bypass the RPC child, use + * the established sf-db primitives, idempotent on conflict. + * + * resolve uses the same evidence shape the autonomous triage flow + * already accepts (`{kind: "human-clear" | "agent-fix" | ...}`), so + * resolution semantics are consistent across operator and agent paths. + */ + +import { existsSync } from "node:fs"; +import { join } from "node:path"; +import { createJiti } from "@mariozechner/jiti"; +import { resolveBundledSourceResource } from "./bundled-resource-path.js"; +import { getSfEnv } from "./env.js"; + +export interface FeedbackResult { + exitCode: number; +} + +interface FeedbackOptions { + subcommand: "add" | "list" | "resolve"; + args: string[]; + json: boolean; +} + +const jiti = createJiti(import.meta.filename, { + interopDefault: true, + debug: false, +}); + +const agentExtensionsDir = join(getSfEnv().agentDir, "extensions", "sf"); +const useAgentDir = existsSync(join(agentExtensionsDir, "state.js")); + +function sfExtensionPath(moduleName: string): string { + if (useAgentDir) return join(agentExtensionsDir, `${moduleName}.js`); + const tsPath = resolveBundledSourceResource( + import.meta.url, + "extensions", + "sf", + `${moduleName}.ts`, + ); + if (existsSync(tsPath)) return tsPath; + return resolveBundledSourceResource( + import.meta.url, + "extensions", + "sf", + `${moduleName}.js`, + ); +} + +// Match self-feedback.js's newId() so ids look uniform across writers. +function newId(): string { + const ts = Date.now().toString(36); + const rnd = Math.random().toString(36).slice(2, 8); + return `sf-${ts}-${rnd}`; +} + +function readFlag(args: string[], name: string): string | undefined { + const i = args.indexOf(name); + if (i < 0 || i + 1 >= args.length) return undefined; + return args[i + 1]; +} + +function readBoolFlag(args: string[], name: string): boolean { + return args.includes(name); +} + +function parseIntOrUndefined(s: string | undefined): number | undefined { + if (s === undefined) return undefined; + const n = Number.parseInt(s, 10); + return Number.isFinite(n) ? n : undefined; +} + +const VALID_SEVERITIES = new Set(["low", "medium", "high", "critical"]); +const DEFAULT_KIND_DOMAIN = "improvement-idea"; + +function emit(json: boolean, payload: Record, human: string): void { + if (json) { + process.stdout.write(`${JSON.stringify(payload)}\n`); + } else { + process.stdout.write(`${human}\n`); + } +} + +async function loadDb(basePath: string): Promise { + const autoStart = (await jiti.import(sfExtensionPath("auto-start"), {})) as { + openProjectDbIfPresent: (basePath: string) => Promise; + }; + await autoStart.openProjectDbIfPresent(basePath); +} + +async function handleAdd(basePath: string, options: FeedbackOptions): Promise { + const summary = readFlag(options.args, "--summary"); + if (!summary || summary.trim() === "") { + process.stderr.write("[headless] Error: feedback add requires --summary \n"); + return { exitCode: 2 }; + } + const severity = readFlag(options.args, "--severity") ?? "medium"; + if (!VALID_SEVERITIES.has(severity)) { + process.stderr.write( + `[headless] Error: --severity must be one of: low, medium, high, critical (got '${severity}')\n`, + ); + return { exitCode: 2 }; + } + const kind = readFlag(options.args, "--kind") ?? DEFAULT_KIND_DOMAIN; + const evidence = readFlag(options.args, "--evidence") ?? ""; + const suggestedFix = readFlag(options.args, "--suggested-fix") ?? ""; + const milestone = readFlag(options.args, "--milestone"); + const slice = readFlag(options.args, "--slice"); + const task = readFlag(options.args, "--task"); + const unitType = readFlag(options.args, "--unit-type"); + const impactScore = parseIntOrUndefined(readFlag(options.args, "--impact-score")); + const effortEstimate = parseIntOrUndefined(readFlag(options.args, "--effort-estimate")); + const blocking = + readBoolFlag(options.args, "--blocking") || severity === "high" || severity === "critical"; + + const id = newId(); + const ts = new Date().toISOString(); + const entry = { + id, + ts, + kind, + severity, + blocking, + repoIdentity: "external" as const, + sfVersion: "", + basePath, + occurredIn: { + unitType: unitType ?? null, + milestone: milestone ?? null, + slice: slice ?? null, + task: task ?? null, + }, + summary: summary.trim(), + evidence, + suggestedFix, + impactScore, + effortEstimate, + source: "headless-cli", + }; + + await loadDb(basePath); + const sfDb = (await jiti.import(sfExtensionPath("sf-db/sf-db-self-feedback"), {})) as { + insertSelfFeedbackEntry: (e: typeof entry) => void; + }; + sfDb.insertSelfFeedbackEntry(entry); + + emit(options.json, { + ok: true, + id, + ts, + kind, + severity, + blocking, + impact_score: impactScore, + summary: entry.summary, + }, `${id} ${severity.padEnd(8)} ${kind} ${entry.summary}`); + return { exitCode: 0 }; +} + +async function handleList(basePath: string, options: FeedbackOptions): Promise { + const wantUnresolved = readBoolFlag(options.args, "--unresolved"); + const severityFilter = readFlag(options.args, "--severity"); + if (severityFilter && !VALID_SEVERITIES.has(severityFilter)) { + process.stderr.write( + `[headless] Error: --severity must be one of: low, medium, high, critical (got '${severityFilter}')\n`, + ); + return { exitCode: 2 }; + } + + await loadDb(basePath); + const sfDb = (await jiti.import(sfExtensionPath("sf-db/sf-db-self-feedback"), {})) as { + listSelfFeedbackEntries: () => Array<{ + id: string; + ts: string; + kind: string; + severity: string; + blocking: boolean; + summary: string; + resolvedAt?: string | null; + impactScore?: number | null; + }>; + }; + let entries = sfDb.listSelfFeedbackEntries(); + if (wantUnresolved) { + entries = entries.filter((e) => !e.resolvedAt); + } + if (severityFilter) { + entries = entries.filter((e) => e.severity === severityFilter); + } + + if (options.json) { + process.stdout.write( + `${JSON.stringify({ ok: true, count: entries.length, entries })}\n`, + ); + return { exitCode: 0 }; + } + if (entries.length === 0) { + process.stdout.write("(no self-feedback entries match)\n"); + return { exitCode: 0 }; + } + for (const e of entries) { + const state = e.resolvedAt ? "RESOLVED" : " "; + process.stdout.write( + `${e.id} ${state} ${e.severity.padEnd(8)} ${e.kind.padEnd(24)} ${e.summary}\n`, + ); + } + return { exitCode: 0 }; +} + +async function handleResolve(basePath: string, options: FeedbackOptions): Promise { + const positional = options.args.filter( + (a, i, all) => !a.startsWith("--") && (i === 0 || !all[i - 1].startsWith("--")), + ); + const id = positional[0]; + if (!id) { + process.stderr.write( + "[headless] Error: feedback resolve requires an id positional (e.g. sf-mp4xxx-yyy)\n", + ); + return { exitCode: 2 }; + } + const reason = readFlag(options.args, "--reason") ?? ""; + const evidenceKind = readFlag(options.args, "--evidence-kind") ?? "human-clear"; + + await loadDb(basePath); + const sfDb = (await jiti.import(sfExtensionPath("sf-db/sf-db-self-feedback"), {})) as { + resolveSelfFeedbackEntry: ( + id: string, + resolution: { + reason: string; + evidence: { kind: string }; + resolvedBySfVersion?: string; + resolvedAt?: string; + }, + ) => boolean; + }; + const ok = sfDb.resolveSelfFeedbackEntry(id, { + reason, + evidence: { kind: evidenceKind }, + resolvedBySfVersion: "", + }); + if (!ok) { + // Either id not found OR already resolved. The DB primitive returns + // false for both — surface a 1-line note rather than failing hard, + // since "already resolved" is the idempotent path. + emit(options.json, { + ok: false, + idempotent: true, + id, + note: "no row updated (already resolved, or id not found)", + }, `${id}: nothing to resolve (already resolved, or id not found)`); + return { exitCode: 0 }; + } + emit(options.json, { + ok: true, + id, + resolved_at: new Date().toISOString(), + evidence_kind: evidenceKind, + reason, + }, `${id}: resolved (${evidenceKind})`); + return { exitCode: 0 }; +} + +export async function handleFeedback( + basePath: string, + options: FeedbackOptions, +): Promise { + switch (options.subcommand) { + case "add": + return handleAdd(basePath, options); + case "list": + return handleList(basePath, options); + case "resolve": + return handleResolve(basePath, options); + default: + process.stderr.write( + `[headless] Error: feedback subcommand must be add|list|resolve (got '${options.subcommand}')\n`, + ); + return { exitCode: 2 }; + } +} diff --git a/src/headless.ts b/src/headless.ts index 73fa6705f..c13d4bcab 100644 --- a/src/headless.ts +++ b/src/headless.ts @@ -833,6 +833,25 @@ async function runHeadlessOnce( return { exitCode: result.exitCode, interrupted: false, timedOut: false }; } + // Operator self-feedback CLI: add/list/resolve self_feedback rows + // without going through SQL. Same RPC-child-bypass pattern. + if (options.command === "feedback") { + const sub = options.commandArgs[0]; + if (sub !== "add" && sub !== "list" && sub !== "resolve") { + process.stderr.write( + "[headless] Error: feedback subcommand must be one of: add, list, resolve\n", + ); + return { exitCode: 2, interrupted: false, timedOut: false }; + } + const { handleFeedback } = await import("./headless-feedback.js"); + const result = await handleFeedback(process.cwd(), { + subcommand: sub, + args: options.commandArgs.slice(1), + json: options.json, + }); + return { exitCode: result.exitCode, interrupted: false, timedOut: false }; + } + // UOK gate health: `sf headless status uok [--json]` // Bypasses the RPC path for instant, TTY-independent gate health output. if (options.command === "status" && options.commandArgs[0] === "uok") { diff --git a/src/help-text.ts b/src/help-text.ts index c591c9e19..83e1e9683 100644 --- a/src/help-text.ts +++ b/src/help-text.ts @@ -259,6 +259,9 @@ const SUBCOMMAND_HELP: Record = { " complete-slice Mark a slice complete out-of-band: complete-slice / [--reason ]", " skip-slice Mark a slice skipped out-of-band (placeholder/migration cleanup)", " complete-milestone Mark a milestone complete out-of-band: complete-milestone [--reason ]", + " feedback add File a self_feedback entry (--summary --severity low|medium|high|critical [--kind ] [--evidence ] [--suggested-fix ] [--milestone M --slice S --task T] [--impact-score N] [--effort-estimate N] [--blocking])", + " feedback list List self_feedback entries [--unresolved] [--severity ] [--json]", + " feedback resolve Resolve an entry: feedback resolve --reason [--evidence-kind human-clear|agent-fix|...]", "", "new-milestone flags:", " --context Path to spec/PRD file (use '-' for stdin)", @@ -298,6 +301,9 @@ const SUBCOMMAND_HELP: Record = { " sf headless complete-slice M010/S03 Flip M010/S03 to status=complete (idempotent)", " sf headless skip-slice M003/S01 --reason \"migration placeholder\" Mark placeholder slice skipped", " sf headless complete-milestone M010 Flip milestone to status=complete", + " sf headless feedback add --severity high --summary \"30K truncate drops the why\" File self-feedback", + " sf headless feedback list --unresolved Pending self-feedback entries", + " sf headless feedback resolve sf-mp4xxx --reason \"shipped in 7b85a6\" Resolve an entry", "", "Exit codes: 0 = success, 1 = error/timeout, 10 = blocked, 11 = cancelled", ].join("\n"), diff --git a/src/resources/extensions/sf/memory-embeddings.js b/src/resources/extensions/sf/memory-embeddings.js index 413f4e564..9b1de678a 100644 --- a/src/resources/extensions/sf/memory-embeddings.js +++ b/src/resources/extensions/sf/memory-embeddings.js @@ -23,18 +23,32 @@ import { import { logWarning } from "./workflow-logger.js"; import { sfHome } from "./sf-home.js"; -/** Read the llm-gateway entry from ~/.sf/agent/auth.json if present. */ +/** Read the llm-gateway entry from ~/.sf/agent/auth.json if present. + * Falls back to SF_LLM_GATEWAY_KEY env var when auth.json is missing + * or has no key — enables CI and containerised deployments without + * writing secrets to disk. */ function readGatewayFromAuthJson() { try { const authPath = join(sfHome(), "agent", "auth.json"); - if (!existsSync(authPath)) return null; - const data = JSON.parse(readFileSync(authPath, "utf8")); - const entry = data["llm-gateway"]; - if (!entry?.key) return null; - return { key: entry.key, url: entry.url || null }; + if (existsSync(authPath)) { + const data = JSON.parse(readFileSync(authPath, "utf8")); + const entry = data["llm-gateway"]; + if (entry?.key) { + return { key: entry.key, url: entry.url || null, source: "auth.json:llm-gateway" }; + } + } } catch { - return null; + /* fall through to env */ } + const envKey = process.env.SF_LLM_GATEWAY_KEY?.trim(); + if (envKey) { + return { + key: envKey, + url: process.env.SF_LLM_GATEWAY_URL?.trim() || null, + source: "env:SF_LLM_GATEWAY_KEY", + }; + } + return null; } // ─── Gateway config ────────────────────────────────────────────────────────── @@ -61,8 +75,8 @@ export function loadGatewayConfigFromEnv() { return { url: fromAuth.url ?? "https://llm-gateway.centralcloud.com/v1", apiKey: fromAuth.key, - keySource: "auth.json:llm-gateway", - urlSource: fromAuth.url ? "auth.json:llm-gateway" : "default", + keySource: fromAuth.source ?? "auth.json:llm-gateway", + urlSource: fromAuth.url ? (fromAuth.source ?? "auth.json:llm-gateway") : "default", embeddingModel, rerankModel, queryInstruction, diff --git a/src/resources/extensions/sf/tests/memory-embeddings-llm-gateway.test.mjs b/src/resources/extensions/sf/tests/memory-embeddings-llm-gateway.test.mjs index 4c1496ba4..41cd1c987 100644 --- a/src/resources/extensions/sf/tests/memory-embeddings-llm-gateway.test.mjs +++ b/src/resources/extensions/sf/tests/memory-embeddings-llm-gateway.test.mjs @@ -10,6 +10,8 @@ const ENV_KEYS = [ "SF_LLM_GATEWAY_EMBED_MODEL", "SF_LLM_GATEWAY_RERANK_MODEL", "SF_LLM_GATEWAY_EMBED_QUERY_INSTRUCTION", + "SF_LLM_GATEWAY_KEY", + "SF_LLM_GATEWAY_URL", ]; let originalHome; @@ -32,16 +34,50 @@ function writeAuthJson(entry) { writeFileSync(authPath, JSON.stringify({ "llm-gateway": entry })); } -test("loadGatewayConfigFromEnv returns null when auth.json has no llm-gateway entry", () => { +test("loadGatewayConfigFromEnv returns null when auth.json has no llm-gateway entry and no env key", () => { const authPath = join(tmpHome, ".sf", "agent", "auth.json"); writeFileSync(authPath, JSON.stringify({})); + delete process.env.SF_LLM_GATEWAY_KEY; assert.equal(loadGatewayConfigFromEnv(), null); }); -test("loadGatewayConfigFromEnv returns null when auth.json does not exist", () => { +test("loadGatewayConfigFromEnv returns null when auth.json does not exist and no env key", () => { + delete process.env.SF_LLM_GATEWAY_KEY; assert.equal(loadGatewayConfigFromEnv(), null); }); +test("loadGatewayConfigFromEnv falls back to SF_LLM_GATEWAY_KEY env var when auth.json missing", () => { + process.env.SF_LLM_GATEWAY_KEY = "env-key-123"; + process.env.SF_LLM_GATEWAY_URL = "https://env.example/v1"; + const cfg = loadGatewayConfigFromEnv(); + assert.equal(cfg.apiKey, "env-key-123"); + assert.equal(cfg.url, "https://env.example/v1"); + assert.equal(cfg.keySource, "env:SF_LLM_GATEWAY_KEY"); + assert.equal(cfg.urlSource, "env:SF_LLM_GATEWAY_KEY"); +}); + +test("loadGatewayConfigFromEnv falls back to SF_LLM_GATEWAY_KEY env var when auth.json has no key", () => { + const authPath = join(tmpHome, ".sf", "agent", "auth.json"); + writeFileSync(authPath, JSON.stringify({})); + process.env.SF_LLM_GATEWAY_KEY = "env-key-456"; + delete process.env.SF_LLM_GATEWAY_URL; + const cfg = loadGatewayConfigFromEnv(); + assert.equal(cfg.apiKey, "env-key-456"); + assert.equal(cfg.url, "https://llm-gateway.centralcloud.com/v1"); + assert.equal(cfg.keySource, "env:SF_LLM_GATEWAY_KEY"); + assert.equal(cfg.urlSource, "default"); +}); + +test("loadGatewayConfigFromEnv prefers auth.json over env var when both present", () => { + writeAuthJson({ key: "auth-key", url: "https://auth.example/v1", type: "api_key" }); + process.env.SF_LLM_GATEWAY_KEY = "env-key-789"; + process.env.SF_LLM_GATEWAY_URL = "https://env.example/v1"; + const cfg = loadGatewayConfigFromEnv(); + assert.equal(cfg.apiKey, "auth-key"); + assert.equal(cfg.url, "https://auth.example/v1"); + assert.equal(cfg.keySource, "auth.json:llm-gateway"); +}); + test("loadGatewayConfigFromEnv reads key and url from auth.json", () => { writeAuthJson({ key: "auth-key", diff --git a/src/tests/headless-feedback.test.ts b/src/tests/headless-feedback.test.ts new file mode 100644 index 000000000..953bdb43d --- /dev/null +++ b/src/tests/headless-feedback.test.ts @@ -0,0 +1,76 @@ +/** + * Smoke test for `sf headless feedback add|list|resolve`. + * + * Mirrors the headless-mark-state.test.ts pattern: source-level + * regex assertions on dispatch wiring and help text. Live-DB + * integration is dogfooded by inserting real entries against the + * SF repo's own .sf/sf.db (see commit message). + */ + +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { test } from "vitest"; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const headlessSrc = readFileSync(join(__dirname, "..", "headless.ts"), "utf-8"); +const helpSrc = readFileSync(join(__dirname, "..", "help-text.ts"), "utf-8"); +const handlerSrc = readFileSync( + join(__dirname, "..", "headless-feedback.ts"), + "utf-8", +); + +test("headless.ts dispatches feedback command to handleFeedback", () => { + assert.match( + headlessSrc, + /options\.command === "feedback"/, + "feedback command must be dispatched", + ); + assert.match( + headlessSrc, + /import\("\.\/headless-feedback\.js"\)/, + "headless.ts must import the new handler", + ); + assert.match( + headlessSrc, + /feedback subcommand must be one of: add, list, resolve/, + "unknown subcommand must surface the readable error", + ); +}); + +test("help text lists the three feedback subcommands", () => { + assert.match(helpSrc, /feedback add\s+File a self_feedback entry/); + assert.match(helpSrc, /feedback list\s+List self_feedback entries/); + assert.match(helpSrc, /feedback resolve\s+Resolve an entry/); +}); + +test("handler enforces required fields and severity values", () => { + assert.match( + handlerSrc, + /feedback add requires --summary /, + "missing summary error must be user-readable", + ); + assert.match( + handlerSrc, + /--severity must be one of: low, medium, high, critical/, + "invalid severity error must enumerate the allowed values", + ); + assert.match( + handlerSrc, + /feedback resolve requires an id positional/, + "missing resolve id must be user-readable", + ); +}); + +test("resolve is idempotent on already-resolved targets", () => { + assert.match(handlerSrc, /idempotent: true/); + assert.match(handlerSrc, /already resolved, or id not found/); +}); + +test("add path defaults blocking from severity, doesn't require it", () => { + // readBoolFlag(--blocking) OR severity === high|critical → blocking=true. + // The behaviour is documented in self-feedback.js (deriveBlocking), + // mirror it so operator-filed entries have consistent semantics. + assert.match(handlerSrc, /severity === "high" \|\| severity === "critical"/); +});