From be8c13e1595796ead50e04f6312517f75f8107e2 Mon Sep 17 00:00:00 2001 From: Fengda Huang Date: Mon, 3 Aug 2026 12:17:16 +0800 Subject: [PATCH] fix(grok): keep plugin ids distinct and union config plugin tables The realpath dedupe added while addressing review feedback rewrote every plugin id to grok/plugin/, so a user-scope and a project-scope plugin sharing a directory name collapsed onto one id. asset-integrity groups name collisions and then dedupes by id, which silenced the plugin-name-collision finding exactly where it is needed, and checkup apply resolves plugins by id. Plugin records now keep their discovery-root qualified id while the dedupe map stays keyed on realpath. The user and project [plugins] tables were concatenated into one text and parsed once, so a project-declared paths key replaced the user-declared one. Each config is parsed separately now and paths/enabled/disabled are unioned. diagnostics.configPath again always names the user config and is null when the user home is out of scope, with projectConfigPath carrying the project file. Nested turn usage.modelUsage values fill any field flat usage omitted instead of only totalTokens, so partial flat records stay complete. render-report exports its host allowlist, pinned by a new test against the session platform registry, together with coverage for the .grok/better-harness default route, fail-closed unknown platforms, and --help staying usable with an invalid platform. Spec updated for the id, config-union, and usage behavior plus the new Grok-A4 project-scope criterion. Validated with npm test (1109 pass) and npm run pack:verify. Co-authored-by: QoderAI (Qwen 3.8 Max) --- docs/specs/2026-08-02-grok-host-adapter.md | 12 +-- scripts/agent-customize/providers/grok.mjs | 42 ++++++++--- scripts/harness-analysis/render-report.mjs | 11 ++- scripts/session-analysis/platforms/grok.mjs | 27 +++---- test/agent-customize.test.mjs | 81 +++++++++++++++++++++ test/harness-report-render-cli.test.mjs | 35 ++++++++- test/session-analysis-providers.test.mjs | 42 +++++++++++ 7 files changed, 215 insertions(+), 35 deletions(-) diff --git a/docs/specs/2026-08-02-grok-host-adapter.md b/docs/specs/2026-08-02-grok-host-adapter.md index 3e31aca..e1d0f40 100644 --- a/docs/specs/2026-08-02-grok-host-adapter.md +++ b/docs/specs/2026-08-02-grok-host-adapter.md @@ -26,7 +26,7 @@ | Skills | `$GROK_HOME/skills/`, `$GROK_HOME/bundled/skills/`, `~/.agents/skills/`, `/.grok/skills/`, `/.agents/skills/` | | Hooks | `$GROK_HOME/hooks/*.json`, project `.grok/hooks` when present | | MCP | `[mcp_servers.]` tables in user and project `config.toml` (enabled flag) | -| Plugins | Trusted user `$GROK_HOME/plugins/`, legacy `$GROK_HOME/installed-plugins/`, project `/.grok/plugins/`, plus `[plugins].paths` from config; physical roots are deduped by realpath | +| Plugins | Trusted user `$GROK_HOME/plugins/`, legacy `$GROK_HOME/installed-plugins/`, project `/.grok/plugins/`, plus `[plugins].paths` from config; physical roots are deduped by realpath while distinct roots keep distinct ids | | Sessions | `$GROK_HOME/sessions///` with `summary.json`, `updates.jsonl`, optional `chat_history.jsonl`, `signals.json` | | Long cwd groups | When `encodeURIComponent(cwd)` exceeds 255 bytes, Grok uses a slug+hash group directory and stores the original path in `.cwd` | | Report root | `/.grok/better-harness/` | @@ -43,7 +43,7 @@ - `updates.jsonl` is the authoritative conversation log; `chat_history.jsonl` is only used when updates are missing (never both). - Terminal tool results require an explicit terminal status (`completed` / `failed` / `error` / `cancelled` / `canceled`); progress and status-less `tool_call_update` stay metadata. -- Model usage comes from `turn_completed.usage` on `_x.ai/session/update` records, including nested `usage.modelUsage.` when flat totals are absent. +- Model usage comes from `turn_completed.usage` on `_x.ai/session/update` records; nested `usage.modelUsage.` values are summed and fill only the fields flat usage did not report. - `signals.contextTokensUsed` is context-window occupancy, not total spend; it is never mapped to `totalTokens`. ### Privacy @@ -54,6 +54,7 @@ ### Stated approximations - Plugin `enabled` follows `[plugins].enabled` / `[plugins].disabled` when present. When neither list is declared, discovered plugin directories are treated as enabled for inventory (filesystem presence), which is a documented approximation of Grok's runtime enablement/trust model rather than a full parity claim. +- The user and project `[plugins]` tables are unioned rather than overridden, so neither config can silently drop the other's declared paths, and the declaring scope is not recorded per entry. - Marketplace catalog entries under `marketplace-cache/` are not treated as installed plugins without an install root. ## Acceptance ids @@ -62,14 +63,15 @@ | --- | --- | | Grok-A1 | Provider inventory returns skills/hooks/mcp/plugins scopes for synthetic home + workspace | | Grok-A2 | `GROK_HOME` / `--grok-home` overrides default without foreign home fallback | -| Grok-A3 | One physical plugin root discovered via multiple path aliases counts once (realpath dedupe) | +| Grok-A3 | One physical plugin root discovered via multiple path aliases counts once (realpath dedupe); two distinct roots sharing a directory name stay distinct | +| Grok-A4 | Project-scope inventory (`includeUserHome: false`) records no user-home plugin roots or user config path | | Grok-S1 | Session sources list only cwd-matching sessions under encoded group dir | | Grok-S2 | Foreign session group excluded from sources | | Grok-S3 | Missing `signals.json` usage stays unobserved (not zero-filled); `contextTokensUsed` alone does not invent totals | | Grok-S4 | Unknown `updates.jsonl` events preserved as metadata | -| Grok-S5 | Nested `turn_completed.usage.modelUsage` contributes observed model usage | +| Grok-S5 | Nested `turn_completed.usage.modelUsage` contributes observed model usage, including completing partial flat records | | Grok-R1 | `platform=grok` accepted by evidence-bundle and session-analysis CLI help | -| Grok-R2 | HTML render default out root documents `.grok/better-harness`; unsupported `--platform` values fail closed | +| Grok-R2 | HTML render default out root resolves to `.grok/better-harness`; unsupported `--platform` values fail closed while `--help` stays usable | ## Smoke (local) diff --git a/scripts/agent-customize/providers/grok.mjs b/scripts/agent-customize/providers/grok.mjs index 8ef1b8b..c5a7d6a 100644 --- a/scripts/agent-customize/providers/grok.mjs +++ b/scripts/agent-customize/providers/grok.mjs @@ -226,8 +226,25 @@ async function collectPluginsFromRoot(pluginsRoot, installMatch, enabledSet, dis return plugins; } -async function collectGrokPlugins(grokHome, workspace, configText, { includeUserHome = true } = {}) { - const section = parseGrokPluginsSectionFromToml(configText ?? ""); +/** + * Union the [plugins] tables of several config files. Repeated keys across + * files add entries instead of replacing them, so a project config cannot + * silently drop user-declared plugin paths. + */ +export function mergeGrokPluginsSections(sections) { + const merged = { paths: [], enabled: [], disabled: [] }; + for (const section of sections) { + if (!section) continue; + for (const key of ["paths", "enabled", "disabled"]) { + for (const value of section[key] ?? []) { + if (!merged[key].includes(value)) merged[key].push(value); + } + } + } + return merged; +} + +async function collectGrokPlugins(grokHome, workspace, section, { includeUserHome = true } = {}) { const enabledSet = new Set(section.enabled); const disabledSet = new Set(section.disabled); const grokHomeReal = await resolveRealPath(grokHome); @@ -264,9 +281,10 @@ async function collectGrokPlugins(grokHome, workspace, configText, { includeUser const realPluginRoot = await resolveRealPath(plugin.rootPath); const existing = byRealRoot.get(realPluginRoot); if (!existing) { + // Keep the discovery-root qualified id so two different plugin roots + // that share a directory name stay distinguishable downstream. byRealRoot.set(realPluginRoot, { ...plugin, - id: `grok/plugin/${path.basename(realPluginRoot)}`, installSources: [match], installSource: match, installMatch: match, @@ -396,13 +414,18 @@ export async function collectGrokCustomizeInventory(options = {}) { const workspace = normalizeWorkspace(options.workspace ?? process.cwd()); const includeUserHome = options.includeUserHome !== false; const configPath = path.join(grokHome, "config.toml"); + const projectConfigPath = path.join(workspace, ".grok", "config.toml"); // Project config may declare plugins/MCP independently of the user home. const userConfigText = includeUserHome ? await readTomlText(configPath) : null; - const projectConfigText = await readTomlText(path.join(workspace, ".grok", "config.toml")); - // Merge user then project [plugins] tables so project enabled/disabled lists still apply. - const pluginConfigText = [userConfigText, projectConfigText].filter(Boolean).join("\n"); + const projectConfigText = await readTomlText(projectConfigPath); + // Union the user and project [plugins] tables so project enabled/disabled + // lists apply without discarding user-declared paths. + const pluginSection = mergeGrokPluginsSections([ + parseGrokPluginsSectionFromToml(userConfigText ?? ""), + parseGrokPluginsSectionFromToml(projectConfigText ?? ""), + ]); const [pluginPack, user, project] = await Promise.all([ - collectGrokPlugins(grokHome, workspace, pluginConfigText, { includeUserHome }), + collectGrokPlugins(grokHome, workspace, pluginSection, { includeUserHome }), includeUserHome ? collectGrokUserPrimitives(grokHome) : emptyPrimitives(), collectGrokWorkspacePrimitives(workspace, grokHome), ]); @@ -421,8 +444,9 @@ export async function collectGrokCustomizeInventory(options = {}) { // recordFiles already scoped by includeUserHome inside collectGrokPlugins. installedPluginRecordFiles: pluginPack.recordFiles, remotePluginInstallMarkersRequired: false, - configPath: includeUserHome ? configPath : path.join(workspace, ".grok", "config.toml"), - projectConfigPath: path.join(workspace, ".grok", "config.toml"), + // configPath always names the user config; null when the user home is out of scope. + configPath: includeUserHome ? configPath : null, + projectConfigPath, }, unsupported: [ "auth.json credentials (never inventoried as values)", diff --git a/scripts/harness-analysis/render-report.mjs b/scripts/harness-analysis/render-report.mjs index ab00feb..dad314e 100644 --- a/scripts/harness-analysis/render-report.mjs +++ b/scripts/harness-analysis/render-report.mjs @@ -41,6 +41,13 @@ function filesystemPathIdentity(value) { return process.platform === "win32" ? normalized.toLowerCase() : normalized; } +// Host ids accepted for report routing. Kept local so `--help` stays cheap; +// test/harness-report-render-cli.test.mjs guards it against the session +// platform registry in scripts/session-analysis/analyzer.mjs. +export const RENDER_REPORT_PLATFORMS = Object.freeze([ + "qoder", "codex", "claude", "cursor", "qwen", "copilot", "pi", "workbuddy", "grok", +]); + // Each Canvas mode owns its own analyzer companion filename so the two routes // stay independent even though they currently agree on `canvas.json`. const ANALYZER_CANVAS_DATA_FILE_BY_MODE = Object.freeze({ @@ -89,9 +96,7 @@ function parseArgs(argv) { } } const hostId = String(options.platform ?? options.provider ?? "").toLowerCase(); - const supportedHtmlHosts = new Set([ - "qoder", "codex", "claude", "cursor", "qwen", "copilot", "pi", "workbuddy", "grok", - ]); + const supportedHtmlHosts = new Set(RENDER_REPORT_PLATFORMS); // Allow --help even when a bad platform is present; validate only for real runs. if (!options.help && hostId && !supportedHtmlHosts.has(hostId)) { throw Object.assign( diff --git a/scripts/session-analysis/platforms/grok.mjs b/scripts/session-analysis/platforms/grok.mjs index 16ada23..5936c4c 100644 --- a/scripts/session-analysis/platforms/grok.mjs +++ b/scripts/session-analysis/platforms/grok.mjs @@ -88,7 +88,8 @@ function addUsageField(observed, key, value) { /** * Accept flat turn usage and/or nested usage.modelUsage. objects. - * Nested modelUsage values are summed across models for the turn total. + * Nested modelUsage values are summed across models and fill only the fields + * flat usage did not report, so partial flat records stay complete. */ function normalizeUsageFromTurn(usage) { if (!usage || typeof usage !== "object") return null; @@ -103,30 +104,22 @@ function normalizeUsageFromTurn(usage) { } const modelUsage = usage.modelUsage && typeof usage.modelUsage === "object" ? usage.modelUsage : null; - if (modelUsage && Object.keys(observed).length === 0) { + if (modelUsage) { + const nested = {}; for (const perModel of Object.values(modelUsage)) { if (!perModel || typeof perModel !== "object") continue; - addUsageField(observed, "inputTokens", finiteNumber(perModel.inputTokens, perModel.input_tokens)); - addUsageField(observed, "outputTokens", finiteNumber(perModel.outputTokens, perModel.output_tokens)); - addUsageField(observed, "totalTokens", finiteNumber(perModel.totalTokens, perModel.total_tokens)); + addUsageField(nested, "inputTokens", finiteNumber(perModel.inputTokens, perModel.input_tokens)); + addUsageField(nested, "outputTokens", finiteNumber(perModel.outputTokens, perModel.output_tokens)); + addUsageField(nested, "totalTokens", finiteNumber(perModel.totalTokens, perModel.total_tokens)); addUsageField( - observed, + nested, "cacheReadInputTokens", finiteNumber(perModel.cachedReadTokens, perModel.cacheReadInputTokens, perModel.cache_read_input_tokens), ); } - } else if (modelUsage && observed.totalTokens === undefined) { - // Flat partials present but total missing: fill from nested when available. - let nestedTotal = 0; - let saw = false; - for (const perModel of Object.values(modelUsage)) { - const value = finiteNumber(perModel?.totalTokens, perModel?.total_tokens); - if (value !== undefined) { - nestedTotal += value; - saw = true; - } + for (const [key, value] of Object.entries(nested)) { + if (observed[key] === undefined) observed[key] = value; } - if (saw) observed.totalTokens = nestedTotal; } return Object.keys(observed).length > 0 ? observed : null; diff --git a/test/agent-customize.test.mjs b/test/agent-customize.test.mjs index 2394eda..0f1bd73 100644 --- a/test/agent-customize.test.mjs +++ b/test/agent-customize.test.mjs @@ -2547,6 +2547,87 @@ test("Grok plugin inventory dedupes one physical plugin reached through multiple } }); +test("Grok plugin inventory keeps distinct roots that share a plugin directory name", async () => { + const root = await mkdtemp(path.join(os.tmpdir(), "better-harness-grok-plugin-name-clash-")); + const grokHome = path.join(root, "home", ".grok"); + const workspace = path.join(root, "workspace"); + await writeJson(path.join(grokHome, "plugins", "flow", "plugin.json"), { name: "flow" }); + await writeJson(path.join(workspace, ".grok", "plugins", "flow", "plugin.json"), { name: "flow" }); + + try { + const inventory = await collectAgentCustomizeInventory({ + provider: "grok", + grokHome, + workspace, + }); + assert.equal(inventory.plugins.length, 2); + // Same directory name, different physical roots: ids must stay distinguishable + // so downstream collision diagnostics are not silently collapsed. + assert.equal(new Set(inventory.plugins.map((plugin) => plugin.id)).size, 2); + assert.deepEqual( + inventory.plugins.map((plugin) => plugin.installMatch).sort(), + ["grok-plugins-dir", "grok-project-plugins-dir"], + ); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test("Grok plugin paths from the user and project config are unioned, not replaced", async () => { + const root = await mkdtemp(path.join(os.tmpdir(), "better-harness-grok-plugin-paths-")); + const grokHome = path.join(root, "home", ".grok"); + const workspace = path.join(root, "workspace"); + const userExtraRoot = path.join(root, "user-extra-plugins"); + const projectExtraRoot = path.join(root, "project-extra-plugins"); + await writeJson(path.join(userExtraRoot, "alpha", "plugin.json"), { name: "alpha" }); + await writeJson(path.join(projectExtraRoot, "beta", "plugin.json"), { name: "beta" }); + await writeText( + path.join(grokHome, "config.toml"), + ["[plugins]", `paths = ["${userExtraRoot.replaceAll("\\", "/")}"]`, ""].join("\n"), + ); + await writeText( + path.join(workspace, ".grok", "config.toml"), + ["[plugins]", `paths = ["${projectExtraRoot.replaceAll("\\", "/")}"]`, ""].join("\n"), + ); + + try { + const inventory = await collectAgentCustomizeInventory({ + provider: "grok", + grokHome, + workspace, + }); + assert.deepEqual(inventory.plugins.map((plugin) => plugin.name).sort(), ["alpha", "beta"]); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test("Grok inventory reports no user config path when the user home is out of scope", async () => { + const root = await mkdtemp(path.join(os.tmpdir(), "better-harness-grok-project-scope-")); + const grokHome = path.join(root, "home", ".grok"); + const workspace = path.join(root, "workspace"); + await writeJson(path.join(grokHome, "plugins", "user-only", "plugin.json"), { name: "user-only" }); + await writeText(path.join(workspace, ".grok", "config.toml"), "[plugins]\n"); + + try { + const inventory = await collectAgentCustomizeInventory({ + provider: "grok", + grokHome, + workspace, + includeUserHome: false, + }); + assert.equal(inventory.plugins.length, 0); + assert.deepEqual(inventory.diagnostics.installedPluginRecordFiles, []); + assert.equal(inventory.diagnostics.configPath, null); + assert.equal( + inventory.diagnostics.projectConfigPath, + path.join(workspace, ".grok", "config.toml"), + ); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + test("Grok project skills dedupe when .grok/skills symlinks to .agents/skills", async () => { const root = await mkdtemp(path.join(os.tmpdir(), "better-harness-grok-skill-symlink-")); const grokHome = path.join(root, "home", ".grok"); diff --git a/test/harness-report-render-cli.test.mjs b/test/harness-report-render-cli.test.mjs index 16995cf..9085b97 100644 --- a/test/harness-report-render-cli.test.mjs +++ b/test/harness-report-render-cli.test.mjs @@ -7,7 +7,8 @@ import path from "node:path"; import test from "node:test"; import { evaluateHtmlReport, renderHtml } from "../scripts/harness-analysis/renderers/html.mjs"; -import { renderReport } from "../scripts/harness-analysis/render-report.mjs"; +import { RENDER_REPORT_PLATFORMS, renderReport } from "../scripts/harness-analysis/render-report.mjs"; +import { SUPPORTED_SESSION_PLATFORMS } from "../scripts/session-analysis/analyzer.mjs"; import { renderCanvasTsx } from "../scripts/harness-analysis/renderers/qoder-canvas.mjs"; import { buildTaskLoopSourceCandidate } from "../scripts/harness-analysis/task-loop-source.mjs"; import { applyEpisodeReviews } from "../scripts/harness-analysis/episode-evidence-review.mjs"; @@ -840,6 +841,38 @@ test("render command writes disk-openable HTML artifacts", async () => { }); }); +test("render routes html output by host id and fails closed on unknown platforms", async () => { + await withTempDir("better-harness-render-platform-", async (root) => { + const findingsPath = path.join(root, "input.findings.json"); + await writeJson(findingsPath, sampleFindings()); + + const routed = runNode( + [renderPath, "--findings", findingsPath, "--mode", "html", "--platform", "grok", "--target", root, "--json"], + { cwd: root }, + ); + assert.equal(routed.status, 0, routed.stderr || routed.stdout); + const payload = parseRun(routed.stdout); + assert.equal(payload.outputLocation.requestedOut, ".grok/better-harness"); + assert.equal(payload.runDir.includes(path.join(".grok", "better-harness")), true); + + const rejected = runNode( + [renderPath, "--findings", findingsPath, "--mode", "html", "--platform", "grock", "--target", root, "--json"], + { cwd: root }, + ); + assert.equal(rejected.status, 1); + assert.match(rejected.stderr, /unsupported render platform: grock/u); + + // Help must stay usable even with an invalid platform so agents can self-correct. + const help = runNode([renderPath, "--help", "--platform", "grock"], { cwd: root }); + assert.equal(help.status, 0, help.stderr); + assert.match(help.stdout, /Usage: better-harness harness render/u); + }); +}); + +test("render platform allowlist matches the session platform registry", () => { + assert.deepEqual([...RENDER_REPORT_PLATFORMS].sort(), [...SUPPORTED_SESSION_PLATFORMS].sort()); +}); + test("HTML relative action metadata carries the finding's current repair revision", () => { const reportData = { ...sampleFindings(), diff --git a/test/session-analysis-providers.test.mjs b/test/session-analysis-providers.test.mjs index ce16ce6..a2ca234 100644 --- a/test/session-analysis-providers.test.mjs +++ b/test/session-analysis-providers.test.mjs @@ -1507,6 +1507,48 @@ test("Grok provider discovers long-path session groups via .cwd marker", async ( assert.equal(result.sources[0].exists, true); }); +test("Grok turn usage completes partial flat records from nested modelUsage", async () => { + const root = await fixtureRoot("session-grok-partial-usage-"); + const home = path.join(root, ".grok"); + const workspace = path.join(root, "workspace", "project"); + const sessionId = "eeeeeeee-eeee-4eee-8eee-eeeeeeeeeeee"; + const sessionDir = path.join(home, "sessions", encodeURIComponent(workspace), sessionId); + await mkdir(sessionDir, { recursive: true }); + await mkdir(workspace, { recursive: true }); + await writeFile(path.join(sessionDir, "summary.json"), JSON.stringify({ + info: { id: sessionId, cwd: workspace }, + created_at: "2026-08-01T10:00:00.000Z", + updated_at: "2026-08-01T10:05:00.000Z", + }, null, 2)); + await writeJsonl(path.join(sessionDir, "updates.jsonl"), [ + { + method: "_x.ai/session/update", + params: { + update: { + sessionUpdate: "turn_completed", + // Flat record reports input only; output/total live in nested modelUsage. + usage: { + inputTokens: 300, + modelUsage: { + "grok-4": { outputTokens: 80, totalTokens: 380 }, + }, + }, + }, + }, + }, + ]); + + const analyzer = new GrokSessionAnalyzer(); + const discovery = await analyzer.analyze({ command: "sources", workspace, home }); + assert.equal(discovery.sessions.length, 1); + const scope = await analyzer.resolveScope({ workspace, home }); + const events = await analyzer.readSession(discovery.sessions[0], scope, {}); + const usage = events.find((event) => event.type === "model.response.completed")?.modelUsage; + assert.equal(usage?.inputTokens, 300); + assert.equal(usage?.outputTokens, 80); + assert.equal(usage?.totalTokens, 380); +}); + test("Grok provider excludes foreign workspace session groups", async () => { const root = await fixtureRoot("session-grok-isolation-"); const home = path.join(root, ".grok");