feat: read the engine's unfulfilled-keys report instead of diffing declared vs delivered

[ralphstodomingo]

Issue for this PR

Closes #1307

Type of change

• Bug fix
• New feature
• Refactor / code improvement
• Documentation

What does this PR do?

When a workspace is bound, the attach toast says "N of M declared integration tools available" and lists what is declared but absent. Until now that list came from a client-side diff: fetch the workspace's allowlist from the API, subtract the tool names the engine served. A diff can name keys, never reasons — an expired Jira token, an MCP server whose binary is not installed, an integration the tenant removed from the catalog, an extension tool with no VS Code window, and a key the provider does not offer all read the same.

@altimateai/datamate 0.7.3 (AltimateAI/altimate-mcp-engine#248, released by AltimateAI/altimate-mcp-engine#250) reports every declared-but-unserved key with a reason under _meta["ai.altimate/unfulfilled"] on each tools/list response. This PR reads it:

• MCP catalog keeps the _meta of a server's last tools/list page per client. paginate keeps only each page's items, so the result object — the only carrier of _meta — was dropped. A listing starts with none; any page that carries one sets it; a listing without one clears it. Exposed as MCP.listMeta(name) (undefined while not connected).
• Attach takes the gaps from the report instead of the diff. no-bridge entries stay out of the "missing" line, as absent extension tools without an IDE were already treated as expected; every other reason is named, grouped, with the engine's detail (e.g. spawn docker ENOENT) — Declared but not available — no usable connection: jira_search_issues; server could not be started or reached (spawn docker ENOENT): gh_list_prs, gh_create_pr. The detail is masked when the report is parsed (control characters collapsed, then Telemetry.maskString, as subprocess stderr is), so the toast and the attach log carry the masked text. The "N of M" headline still counts declared keys that are present, and the toast is a warning when a gap is reported or fewer declared tools are callable than declared. The attached outcome carries the full report for later surfaces.
• No report means no claim. An engine that sends no _meta (nothing at or above the floor does) yields an outcome with neither missing nor unfulfilled, and a toast with no gap line — not "all served".
• Floor moves to 0.7.3 (MIN_ENGINE_VERSION), the first engine that emits the report (0.7.2 was released without it). @altimateai/datamate@0.7.3 is on npm as latest.

The client no longer reads what it cannot know: the allowlist lookup (declared()) is kept only for the headline's denominator and the extension-tool count, and a report without a reachable allowlist still names the gaps.

Claims

• C1 — Nothing is invented. missing and unfulfilled exist on the outcome only when the engine sent a well-formed report; a malformed or absent _meta yields neither, and a malformed one is logged as a warning once per transition into that state, even when the announcement itself is unchanged (tests: "an engine that sends no report is not read as having no gaps", "a report that turns malformed is logged once per transition, without a second toast"; parseUnfulfilled cases).
• C2 — no-bridge never counts as missing, and every other reason does — including unknown-key on an extension key while a bridge is connected (reportedMissing; test "no-bridge entries in the report are expected, never missing").
• C3 — The catalog keeps the report across the paths that list tools: initial connect, the tools/list_changed refresh, the post-OAuth reconnect all go through McpCatalog.defs → listTools, which is the only writer (catalog-list-meta.test.ts covers first page, multi-page, and clearing).
• C4 — A gap whose reason, integration or error text changes is announced again; an identical report is not (the signature carries integration, key, reason and detail; tests "a gap whose reason changed is announced again" and "a gap whose error text changed under the same reason is announced again").
• C5 — Servers other than the engine are unaffected: _meta is retained per client but read only for datamate; tool conversion and the stored defs are unchanged.
• C6 — A detail is masked before anything shows or logs it: parseUnfulfilled collapses control characters and applies Telemetry.maskString, so the toast and log.info both carry the masked text (test "a detail is masked and flattened before anything shows or logs it").
• C7 — Severity follows what is callable: warning when a gap is reported, or when a report is present and fewer declared tools are callable than declared (two raw keys that sanitise to one catalog entry); with no report nothing is claimed and it stays info (the collision test asserts warning, the no-report test info).

Residuals

• R1 — Reasons outside the engine's current set are shown verbatim (a newer engine may add one) rather than dropped.
• R2 — The toast shows at most five keys across groups and truncates a detail at 60 characters; the full report is on the outcome.
• R3 — The floor bump refuses 0.7.2 and older engines; that is the intended contract, and the reason is in the MIN_ENGINE_VERSION comment.
• R4 — Telemetry.maskString is the codebase's general masking, not a secret detector: it masks paths and known key shapes, and a short bare token passes through it. What keeps configuration out of detail is the engine at the floor, which sends only known error shapes and reduces a spawn failure to a plain command name.

How did you verify your code works?

• bun run typecheck clean; prettier clean on the files this PR touches (the files that were already non-conforming on main are left as they were).

• test/altimate/workspace and test/mcp on the branch merged with current main: 964 pass, 6 fail. The 6 (5 mcp.headers, 1 oauth-auto-connect) fail the same way with main's src/mcp swapped in, so they are environmental, not this change.

• New tests: 6 attach cases (reasons in the toast, no-bridge exclusion, no-report, report-without-allowlist, reason-change re-announce, the existing inventory case now stating the engine's report, error-text re-announce, and the collision case's warning), describeMissing/parseUnfulfilled/reportedMissing unit cases (including detail masking), 6 catalog cases through a scripted client (other test files mock the MCP SDK process-wide, so a real SDK client here would list nothing; the SDK's _meta passthrough is covered by the real-engine e2e run).

• End to end through the real MCP service (test/mcp/engine-unfulfilled.e2e.test.ts, env-guarded, skipped in CI): the published @altimateai/datamate@0.7.3 (a clean npm i) is spawned over stdio by MCP.add exactly as the overlay spawns it, against a fake Altimate API, a real second MCP server and a missing binary; MCP.listMeta("datamate") returns the five-entry report with the expected reasons and the toast text reads Declared but not available — no usable connection: jira_search_issues; not offered by the integration: ghost; server could not be started or reached (spawn altimate-e2e-missing-binary ENOENT): whatever; no longer in the catalog: retired_tool. — 1 pass, and the run leaves no temporary HOME behind. Run it with ALTIMATE_ENGINE_E2E_ROOT=<engine checkout with dist/> bun test test/mcp/engine-unfulfilled.e2e.test.ts from packages/opencode.

• Engine → CLI through the real attach path (evidence): bootstrap + beforeTurn on a bound directory against the published @altimateai/datamate@0.7.3 (a clean npm i, on PATH as datamate). Settled outcome attached with declared: 5, missing: [jira_search_issues, ghost, whatever, retired_tool], the full report incl. the no-bridge entry, and the exact toast text; 8/8 checks. A 0.7.2 build (the report present, the version that shipped without it) is refused as engine-too-old; 2/2.

Screenshots / recordings

Not a UI change beyond toast text; the exact strings are asserted in the tests above.

Checklist

• I have tested my changes locally
• I have not included unrelated changes in this PR

🤖 Generated with Claude Code

https://claude.ai/code/session_01GHBUvb843k1R7UAGi8Ya9b

Appendix — complexity delta (altimate-code: engine unfulfilled report)

e8c21c2af7 → 93c879af8 · only functions this diff touches · advisory, not a gate.

✅ No touched function changed in complexity (12 touched, 4 new, all under 10).

ℹ️ How to read these numbers

Cognitive (Sonar spec) counts breaks in linear reading flow — each if/loop/catch/ternary/boolean-operator switch adds 1, and nesting makes every further break cost more. It approximates how much you must hold in your head to follow the function: 0–5 trivial · 6–10 easy · 11–15 moderate (15 = Sonar's recommended per-function cap) · 16–25 hard to follow · >25 needs decomposition.

CCN (cyclomatic) counts independent paths — also the minimum number of test cases for full branch coverage of the function.

Only functions this diff touches are measured, as deltas — pre-existing complexity is not counted against this change. Rising numbers aren't automatically wrong; they're where review attention should go. Test files excluded.

---

Summary by cubic

Closes #1307. Workspace attach now reads the engine’s ai.altimate/unfulfilled report instead of diffing declared and delivered tools, so missing-tool notices include actionable reasons and details rather than only tool names.

Attach behavior

• Excludes no-bridge entries; every other valid reason is reported.
• Groups missing tools by reason and integration, preserving the engine’s detail.
• Leaves missing and unfulfilled unset when the report is absent; logs a warning once when it is malformed.
• Re-announces a gap when its reason, integration, or error text changes, but not when the report is identical.
• Counts distinct sanitized catalog entries, including collisions across ordinary and extension tools.
• Accepts numeric integration IDs and stores them as strings.
• Raises MIN_ENGINE_VERSION to 0.7.3, which requires @altimateai/datamate 0.7.3 or newer.

MCP catalog

• Preserves each client’s tools/list _meta and exposes it as MCP.listMeta(name).
• Commits tools and their report together through MCP.snapshot(name) to prevent mismatched refreshes.
• Keeps the previous report for pending or failed listings; a completed listing without _meta clears it.

^{Written for commit 4aa8e68. Summary will update on new commits.}

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[ralphstodomingo]

Engine → altimate-code, through the real attach path

This runs the CLI's production attach code under a real instance with no model turn: the binding cache, the datamate lookup on PATH, the --version probe, the allowlist lookup, the MCP spawn, the report and the toast are all the real code (bootstrap(dir, …) → beforeTurn(sessionID)). Only two things are not production: the SaaS API is a local fake (it serves both the CLI's endpoints and the engine's, and every call is asserted 200), and the toast sink is captured instead of published to a TUI bus. The engine is the 0.7.2 release candidate: AltimateAI/altimate-mcp-engine#250's head with AltimateAI/altimate-mcp-engine#248 merged in, built locally (datamate --version → 0.7.2), reached through a shim on PATH exactly as a global install would be.

Declared by the workspace: jira (no connection on this machine), vscode-power-user (extension), mcp-ok (a real second MCP server offering only echo), mcp-missing-binary (command absent), retired-integration (not in the catalog).

Result — release candidate (head engine-rc-0.7.2, 6170 ms from beforeTurn to settled outcome)

Toast (variant warning):

1 of 5 declared integration tools available. Declared but not available — no usable connection: jira_search_issues; not offered by the integration: ghost; server failed to start (spawn altimate-e2e-missing-binary ENOENT): whatever; no longer in the catalog: retired_tool.

Settled outcome:

{
  "kind": "attached",
  "available": 1,
  "declared": 5,
  "missing": [
    "jira_search_issues",
    "ghost",
    "whatever",
    "retired_tool"
  ],
  "unfulfilled": [
    {
      "key": "jira_search_issues",
      "integrationId": "jira",
      "reason": "invalid-connection"
    },
    {
      "key": "pu_lineage",
      "integrationId": "vscode-power-user",
      "reason": "no-bridge"
    },
    {
      "key": "ghost",
      "integrationId": "mcp-ok",
      "reason": "unknown-key"
    },
    {
      "key": "whatever",
      "integrationId": "mcp-missing-binary",
      "reason": "spawn-failed",
      "detail": "spawn altimate-e2e-missing-binary ENOENT"
    },
    {
      "key": "retired_tool",
      "integrationId": "retired-integration",
      "reason": "catalog-missing"
    }
  ]
}

• PASS outcome is attached
• PASS declared counted from the allowlist (5 CLI-servable keys)
• PASS missing = every gap the engine reported except no-bridge
• PASS outcome carries the full report incl. the no-bridge entry
• PASS spawn-failed detail carries the engine's error
• PASS exactly one toast, warning
• PASS toast text
• PASS every API call served (no 404)

API calls, in order: GET /skills -> 200, GET /datamates/77/summary -> 200, GET /datamate_integrations/ -> 200, GET /dbt/v3/validate-credentials -> 200, GET /datamates -> 200, GET /datamate_integrations -> 200, GET /mask -> 200, GET /datamate_integrations/custom -> 200, GET /connections -> 200 — the first three are the CLI's, the rest the engine's.

Result — floor negative (engine 0.7.1, headless)

The same run against a 0.7.1 build settles engine-too-old (found 0.7.1) and prints one stderr line:

Workspace "e2e-rc": 5 integration tools need datamate 0.7.2+ (found 0.7.1). Update with: npm i -g @altimateai/datamate@0.7.2

• PASS engine below the floor is refused as engine-too-old
• PASS refusal names the found version and the floor

Not covered

Windows; a real tenant; the TUI rendering of the toast (the text is asserted, the widget is not).

Reproduce — .e2e/engine-to-cli.ts + .e2e/run.sh (run from packages/opencode)

.e2e/run.sh <engine checkout with dist/> out.json            # attach
HEADLESS=1 .e2e/run.sh <0.7.1 engine> out.json too-old      # floor negative

run.sh (starts bun with an isolated HOME — Bun caches os.homedir() at startup, so the script cannot set it itself):

#!/usr/bin/env bash
# usage: .e2e/run.sh <engine-root> <out.json> [EXPECT]
set -uo pipefail
H=$(mktemp -d /tmp/e2e-engine-to-cli-home-XXXXXX)
env -i HOME="$H" PATH="$H/bin:/usr/local/bin:/usr/bin:/bin:$(dirname "$(command -v bun)"):$(dirname "$(command -v node)")" TERM=dumb ALTIMATE_WORKSPACE=1 ${HEADLESS:+ALTIMATE_CODE_HEADLESS=1} ENGINE_ROOT="$1" EXPECT="${3:-}" \
  timeout 180 bun .e2e/engine-to-cli.ts > "$2" 2> "${2%.json}.stderr"
echo "RC=$? HOME=$H"

engine-to-cli.ts:

// Engine → altimate-code, through the CLI's real attach path. Nothing is
// stubbed except the toast sink (so its text can be captured) and the SaaS
// API (served locally). The binding, the `datamate` lookup on PATH, the
// version probe, the allowlist lookup, the MCP spawn and the report all run
// the production code under a real instance — no model turn.
import http from "node:http"
import path from "node:path"
import { mkdtempSync, mkdirSync, writeFileSync, chmodSync } from "node:fs"
import { tmpdir } from "node:os"
import { execFileSync } from "node:child_process"

const engineRoot = process.env["ENGINE_ROOT"]!
const node = Bun.which("node")!
const DATAMATE_ID = 77

// ---- fake SaaS: engine endpoints + the CLI's own ----------------------------
const catalog = [
  { id: "jira", type: "tool", name: "Jira", description: "", url: "", supportsLocalConnectionTest: true, supportsSaasConnectionTest: false,
    config: [{ key: "url", name: "URL", type: "string", required: true }, { key: "email", name: "Email", type: "string", required: true }, { key: "token", name: "Token", type: "string", required: true }],
    tools: [{ key: "jira_search_issues", name: "Search issues" }] },
  { id: "vscode-power-user", type: "extension", name: "Power User for dbt", description: "", url: "", supportsLocalConnectionTest: false, supportsSaasConnectionTest: false, config: [],
    tools: [{ key: "pu_lineage", name: "Lineage" }] },
]
const custom = [
  { id: "mcp-ok", type: "mcp", name: "Echo MCP", description: "", url: "", config: [],
    toolConfig: [{ key: "type", name: "type", type: "string", required: false, value: "stdio" }, { key: "command", name: "command", type: "string", required: true, value: node },
      { key: "arguments", name: "arguments", type: "array", required: false, value: [path.join(import.meta.dir, "../test/mcp/fixtures/echo-mcp-server.mjs")] }],
    tools: [{ key: "echo" }, { key: "ghost" }] },
  { id: "mcp-missing-binary", type: "mcp", name: "Missing MCP", description: "", url: "", config: [],
    toolConfig: [{ key: "type", name: "type", type: "string", required: false, value: "stdio" }, { key: "command", name: "command", type: "string", required: true, value: "altimate-e2e-missing-binary" }],
    tools: [{ key: "whatever" }] },
]
const datamate = {
  id: String(DATAMATE_ID), name: "e2e-rc", description: "", privacy: "private", memory_enabled: false, knowledge_engine_enabled: false, knowledge_bases: [],
  integrations: [
    { id: "jira", type: "tool", name: "Jira", description: "", url: "", tools: [{ key: "jira_search_issues" }] },
    { id: "vscode-power-user", type: "extension", name: "PU", description: "", url: "", tools: [{ key: "pu_lineage" }] },
    { id: "mcp-ok", type: "mcp", name: "Echo MCP", description: "", url: "", tools: [{ key: "echo" }, { key: "ghost" }] },
    { id: "mcp-missing-binary", type: "mcp", name: "Missing MCP", description: "", url: "", tools: [{ key: "whatever" }] },
    { id: "retired-integration", type: "tool", name: "Retired", description: "", url: "", tools: [{ key: "retired_tool" }] },
  ],
}
const hits: string[] = []
const api = http.createServer((req, res) => {
  const url = new URL(req.url ?? "/", "http://x"); const p = url.pathname.replace(/\/+$/, "") || "/"
  const json = (code: number, body?: unknown) => { hits.push(`${req.method} ${url.pathname} -> ${code}`); res.writeHead(code, { "content-type": "application/json" }); res.end(body === undefined ? "" : JSON.stringify(body)) }
  if (p === "/dbt/v3/validate-credentials") return json(200, { ok: true })
  if (p === "/datamates") return json(200, { datamates: [datamate] })
  if (p === `/datamates/${DATAMATE_ID}/summary`) return json(200, { datamate })
  if (p === "/datamate_integrations") return json(200, catalog)
  if (p === "/datamate_integrations/custom") return json(200, { items: custom })
  if (p === "/mask") return json(200, { mask_data: [] })
  if (p === "/connections") return json(200, { connections: [] })
  if (p === `/datamates/${DATAMATE_ID}/knowledge_bases`) return json(200, { knowledge_bases: [] })
  if (p === `/datamates/${DATAMATE_ID}/knowledge_engine_description`) return json(200, {})
  if (p === "/datamates/audit/create_batch") return json(204)
  if (p === "/skills") return json(200, { skills: [] })
  return json(404, { detail: `unhandled ${p}` })
})
await new Promise<void>((r) => api.listen(0, "127.0.0.1", r))
const apiUrl = `http://127.0.0.1:${(api.address() as { port: number }).port}`

// ---- isolated HOME: the wrapper starts this process with HOME already pointing
// at a fresh directory (Bun caches os.homedir() at startup, so setting it here
// would be too late for Global.Path); this script only fills it in.
const home = process.env["HOME"]!
if (!home.includes("e2e-engine-to-cli-home-")) throw new Error(`refusing to run against a real HOME: ${home}`)
mkdirSync(path.join(home, ".altimate"), { recursive: true })
writeFileSync(path.join(home, ".altimate/altimate.json"), JSON.stringify({ altimateUrl: apiUrl, altimateInstanceName: "e2e", altimateApiKey: "e2e-key" }))
writeFileSync(path.join(home, ".altimate/settings.json"), "{}")
writeFileSync(path.join(home, ".altimate/connections.json"), "[]")
// `datamate` on PATH → the engine under test, on node (the published bin is a node shebang script)
const bin = path.join(home, "bin"); mkdirSync(bin)
writeFileSync(path.join(bin, "datamate"), `#!/bin/sh\nexec "${node}" "${path.join(engineRoot, "dist/cli.js")}" "$@"\n`); chmodSync(path.join(bin, "datamate"), 0o755)
process.env["PATH"] = `${bin}:${process.env["PATH"]}`
process.env["ALTIMATE_WORKSPACE"] = "1"
const resolved = Bun.which("datamate")
const engineVersion = execFileSync(resolved!, ["--version"], { encoding: "utf8" }).trim().split("\n").pop()

// a bound project directory
const project = mkdtempSync(path.join(tmpdir(), "e2e-engine-to-cli-project-"))
execFileSync("git", ["init", "-q", project])

// ---- production modules, imported only after the environment is shaped ----
const { bootstrap } = await import("../src/cli/bootstrap")
const { recordApprovedBinding } = await import("../src/altimate/workspace/state")
const { beforeTurn, settledOutcome } = await import("../src/altimate/workspace/engine-overlay")
const { syncInternals } = await import("../src/altimate/workspace/engine-seams")
const toasts: { title: string; message: string; variant: string }[] = []
syncInternals.notify = async (t) => { toasts.push(t) }   // capture only; no TUI bus here
const lines: string[] = []
syncInternals.printLine = (l) => { lines.push(l) }

await recordApprovedBinding(project, { datamateId: DATAMATE_ID, datamateName: "e2e-rc", repoRemote: null, projectPath: project, linkedAt: Date.now() })
const t0 = Date.now()
const result = await bootstrap(project, async () => {
  await beforeTurn("s1")
  return settledOutcome("s1")
})
const elapsedMs = Date.now() - t0
api.close()

const checks: string[] = []
const check = (label: string, ok: boolean) => checks.push(`${ok ? "PASS" : "FAIL"} ${label}`)
const attached = result?.kind === "attached" ? result : undefined
if (process.env["EXPECT"] === "too-old") {
  check("engine below the floor is refused as engine-too-old", result?.kind === "engine-too-old")
  check("refusal names the found version and the floor", lines.concat(toasts.map((t) => t.message)).some((l) => l.includes(engineVersion!) && l.includes("0.7.2")))
} else {
  check("outcome is attached", !!attached)
  check("declared counted from the allowlist (5 CLI-servable keys)", attached?.declared === 5)
  check("missing = every gap the engine reported except no-bridge", JSON.stringify(attached?.missing) === JSON.stringify(["jira_search_issues", "ghost", "whatever", "retired_tool"]))
  check("outcome carries the full report incl. the no-bridge entry", !!attached?.unfulfilled?.some((u) => u.key === "pu_lineage" && u.reason === "no-bridge"))
  check("spawn-failed detail carries the engine's error", /ENOENT/.test(attached?.unfulfilled?.find((u) => u.key === "whatever")?.detail ?? ""))
  check("exactly one toast, warning", toasts.length === 1 && toasts[0].variant === "warning")
  check("toast text", toasts[0]?.message === "1 of 5 declared integration tools available. Declared but not available — no usable connection: jira_search_issues; not offered by the integration: ghost; server failed to start (spawn altimate-e2e-missing-binary ENOENT): whatever; no longer in the catalog: retired_tool.")
  check("every API call served (no 404)", !hits.some((h) => / -> 404$/.test(h)))
}
console.log(JSON.stringify({ engineRoot, home, resolvedDatamate: resolved, engineVersion, elapsedMs, outcome: result, toasts, lines, apiHits: hits, checks, verdict: checks.every((c) => c.startsWith("PASS")) ? "ALL PASS" : "FAILURES" }, null, 2))
process.exit(checks.every((c) => c.startsWith("PASS")) ? 0 : 1)

[ralphstodomingo]

One more commit on this head, 82531540c — accept numeric integration ids in the engine report. A tenant-created (custom) MCP integration's id arrives from the engine as a number; the parser treated the whole report as malformed over that one field and the attach announced no gaps at all. The id is now taken as a string (unit test added). Found by the real-chain run for the attach-report post (#1310); the engine side stringifies too (AltimateAI/altimate-mcp-engine#248 64e5bb4), so either side alone is enough.

[saravmajestic]

Multi-model review — client half

Reviewed jointly with AltimateAI/altimate-mcp-engine#248 as one feature. The wire contract between the two halves agrees exactly: meta key, all four field names, all six reason spellings, and the empty-array-vs-absent distinction match. parseUnfulfilled's fail-closed behaviour correctly keeps "I don't know" distinct from "nothing missing", and unknown future reasons pass through verbatim (R1). Release sequencing is tracked separately.

Major

1. Tools and their report are not committed atomically across a refresh

src/mcp/catalog.ts:166,187-190 deletes _meta when a listing starts and republishes it page by page, while defs are replaced only after the whole listing succeeds (src/mcp/index.ts:744-748).

Consequences:

• While a refresh is pending, the previous tools stay visible but their report is gone — a turn silently loses its gap line.
• If a later page fails, partial _meta sits beside stale defs.
• reconcile reads the two through Promise.all (engine-overlay.ts:651), so it has no guarantee of a coherent snapshot.

This bites C3 ("the catalog keeps the report across the paths that list tools") on the tools/list_changed refresh path specifically — the report is kept across completed listings, not across a pending one.

Suggested: accumulate { tools, meta } locally and commit them together on success; retain the last good snapshot on failure; expose one snapshot accessor to the overlay.

Minor

2. The headline and the gap line can disagree (raw vs sanitized key space)

declared.keys are raw API tool.key strings (engine-probes.ts:135-137). present comes from MCP.tools() keys, which pass through sanitize = value.replace(/[^a-zA-Z0-9_-]/g, "_") (mcp/catalog.ts:145, mcp/index.ts:1081), and engineToolKeys only strips the prefix — it can't restore the original name (engine-types.ts:149-154).

So a served schema.inspect becomes datamate_schema_inspect → schema_inspect and never matches the raw declaration. The headline at engine-overlay.ts:662 undercounts.

The undercount itself pre-dates this PR — the old served = declared.keys.length - missing.length used the same mismatched comparison. What changes is the presentation: previously the mismatched key was also (wrongly) named in the missing line; now the engine correctly reports nothing, so the toast reads "3 of 5 declared integration tools available." with no gap line at all. Headline and report contradict each other silently.

Suggested: normalize both sides to one key space before the present.has(k) lookup, or derive served from the report rather than from present.

3. describeMissing attributes one integration's error to another

engine-types.ts:285-303 groups solely by reason and uses the group's first non-empty detail for every key in it. Two integrations that both fail spawn-failed — one spawn docker ENOENT, one a bad path — render under a single error. integrationId is carried on every entry and ignored here, so the toast can give actively wrong repair guidance.

Suggested: group by (integrationId, reason), or only show a shared detail when every member's detail is identical.

4. "Last page's _meta" is really "last page that had one"

catalog.ts:166,187-190 clears once when the listing starts, then sets only if (result._meta !== undefined). A listing whose first page carries _meta and whose final page does not retains the first page's value — which doesn't match the stated per-page clearing. Harmless against today's single-page engine response, but ambiguous as a general accessor contract, and test/mcp/catalog-list-meta.test.ts:58 only covers _meta on the final page.

Suggested: pick a rule (last-page-authoritative vs any-page), implement it inside the completed snapshot, and add a first-page-only case.

5. Truncation is not redaction

engine-types.ts truncates detail to 60 chars into the notification, and engine-overlay.ts:685-691 logs the full report. The engine currently emits raw error.message (flagged on the engine PR); until that's bounded and sanitized upstream, sensitive text can appear at the start of a detail and survive truncation.

6. spawn-failed renders as "server failed to start"

REASON_PHRASE maps it that way, but the engine records transport construction, connect and list failures under that reason — so an auth rejection on a running server reads as a startup failure. Worth aligning the phrase with the engine's actual taxonomy.

Verified sound

• MCP.listMeta(DATAMATE_KEY) resolves correctly — the engine is registered under DATAMATE_KEY (engine-overlay.ts:387,560,634).
• C5 holds: _meta is retained per client via WeakMap<Client, …> and read only for datamate; a reconnect creates a new Client, so old metadata can't transfer.
• C2 holds: reportedMissing excludes only no-bridge; unknown-key on an extension key with a connected bridge does count.
• C1 holds: parseUnfulfilled returns undefined on malformed input, so a bad report yields neither missing nor unfulfilled.
• C4's signature change is right — key=reason means a gap whose reason changed re-announces.
• The served change from declared - missing to filter(present.has) is an improvement for the no-bridge case, which the old arithmetic counted as served.

[ralphstodomingo]

Re-review disposition — 45a8d02c8

Answering the multi-model review (client half), paired with the engine half on AltimateAI/altimate-mcp-engine#248.

Fixed

• Major 1 — tools and report not committed atomically. The catalog now commits _meta only when a listing completes (the last page that carries one wins; a listing with none clears it; a pending or failed listing leaves the previous value), and MCP stores it in State.meta in the same statement as State.defs. The overlay reads both through one accessor, MCP.snapshot(name), which is one synchronous pass over the state — so a refresh landing between two reads can no longer pair one listing's tools with another's report. Tests: "a _meta on the first page is kept when the last page carries none", "a listing that fails part-way leaves the previous _meta standing", plus the existing three.
• Minor 2 — headline and gap line in different key spaces. served and extServed compare declared keys after sanitize, the same transform the MCP layer applies to tool names, so a served tool whose raw key it renamed still counts.
• Minor 3 — one integration's error attributed to another. describeMissing groups by reason AND integration. Test: "two integrations that failed the same way keep their own details".
• Minor 4 — "last page's _meta" semantics. Stated and tested as "the last page that carries one wins" (first-page-only case added).
• Minor 6 — spawn-failed phrase. Now "server could not be started or reached", which covers construction, connect and list failures as the engine records them.

Recorded, not changed

• Minor 5 — truncation is not redaction. The engine half now bounds and redacts detail before it enters _meta (publicDetail on fix: add altimate-core + dbt-integration to build externals #248); the client keeps truncating for the toast and logs what the engine sent.

Verified on this head: typecheck clean; test/mcp, test/altimate/workspace, test/altimate/plugin and the tool-race suite 735 pass. The six failures in headers.test.ts and oauth-auto-connect.test.ts fail identically on the untouched head in this environment (they need a network or a real server) and are not from this change.

[ralphstodomingo]

@codex review

Scoped review against the claims below (head 45a8d02c8). For each claim, say whether the code holds it, with a concrete failing scenario where it does not. Please do not re-raise the residuals at the end.

Claims

• C1 — parseUnfulfilled fails closed: a malformed or absent report yields undefined (neither missing nor unfulfilled on the outcome), and unknown future reasons pass through verbatim.
• C2 — reportedMissing excludes only no-bridge; every other reason the engine reports is a gap, and an unknown-key on an extension key with a connected bridge counts.
• C3 — The tools of a listing and its _meta are committed together and read together: State.meta[name] is written in the same statement as State.defs[name] on every path that stores a listing, deleted with it on every path that drops one, and MCP.snapshot(name) reads both in one synchronous pass. A pending or failed refresh leaves the last good pair standing.
• C4 — The catalog commits _meta when a listing completes: the last page carrying one wins, a listing with none clears it, and a failed listing leaves the previous value.
• C5 — served and extServed compare declared keys in the sanitised key space, so the headline and the gap line agree on a served tool whatever its raw key.
• C6 — describeMissing groups by reason and integration; a group's detail is only ever one of its own members' details.
• C7 — A gap whose reason changed re-announces (the reasons are in the announcement signature).

Residuals (already accepted)

• R1 — The client truncates detail for the toast; bounding and redaction happen engine-side (AltimateAI/altimate-mcp-engine#248).
• R2 — spawn-failed stays one reason on the wire; the phrase covers construction, connect and list failures.
• R3 — MCP.listMeta(name) remains for callers that need only the report; the overlay uses snapshot.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-25T10:59:56.318511Z	4aa8e68	Manual request

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 45a8d02c8c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[ralphstodomingo]

@codex review

Scoped falsification round on the fixes since the last round (45a8d02c8 → 820147aee). For each claim, say whether the code holds it, with a concrete failing scenario where it does not. Please do not re-raise the accepted residuals.

Fix claims

• F1 — Every commit of a listing stores the tools and the _meta that the SAME listing returned (defsWithMeta): on create, in the tools/list_changed handler, and in storeClient on connect and after OAuth. Two overlapping refreshes for one client can no longer commit one listing's tools with the other's report.
• F2 — served and extServed count a declared key only when its sanitised form is in the catalog AND the engine does not report it unfulfilled; colliding raw keys therefore cannot both count.
• F3 — parseUnfulfilled returns undefined for an entry whose detail is present and not a string; a string, an empty string, or an absent detail parses.

Accepted residuals

• R1 — McpCatalog.listMeta(client) still exists as a per-client read for the catalog tests; nothing on the commit path uses it.
• R2 — Truncation of detail for the toast stays client-side; bounding and redaction are engine-side.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 820147aee2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[ralphstodomingo]

@codex review

Final scoped falsification round (round 3 of 3) on the fix since the last round (820147aee → 945210965). For the claim below, say whether the code holds it, with a concrete failing scenario where it does not. Please do not re-raise the accepted residuals.

Fix claim

• G1 — served and extServed are the number of distinct sanitised catalog entries among the declared keys that are present in the catalog and not reported unfulfilled; declarations that collide after sanitising count once, whether or not the engine listed both.

Accepted residuals

• R1 — A colliding pair where the engine reports one key and lists the other counts the listed one (the report is the authority on which the served tool stands for).
• R2 — McpCatalog.listMeta(client) remains as a per-client read for the catalog tests; nothing on the commit path uses it.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9452109654

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[ralphstodomingo]

Disposition after three Codex rounds — 413fadceb

Three rounds is the cap, so this is the last: the round-3 finding (an ordinary key and an extension key colliding after sanitising were counted in both groups) is fixed in 413fadceb and pinned by a test. No further Codex round; the remaining verification is by hand.

Verified on this head: typecheck clean; test/mcp, test/altimate/workspace, test/altimate/plugin and the tool-race suite pass (the six headers/oauth-auto-connect failures are environmental and identical on the untouched head). Ready for a human review.

[sahrizvi]

Consensus review (Claude + Codex + Gemini + MiniMax + Qwen + MiMo, 6/8 configured participants, quorum met)

Superseded — see the follow-up review below requesting changes.

1 MAJOR and 4 MINOR findings posted as inline comments (Nits omitted per request — see the full write-up for those plus positive observations, missing-test notes, and the disagreements the panel investigated and rejected during convergence).

Full review with attribution: reviews/pr-1308-consensus-review.md in the team's review archive.

[sahrizvi]

MAJOR (Security) — u.detail (the engine's raw spawn-error / connection-failure text for a custom MCP integration) is captured here and later only length-truncated (DETAIL_CHARS = 60) before reaching the attach toast, never content-redacted. This can contain command args, URLs with embedded credentials, tokens, or paths. The same unredacted value also reaches log.info in full at engine-overlay.ts:716.

This codebase already has the right pattern for exactly this class of data — mcp/index.ts:232 wraps subprocess stderr with Telemetry.maskString(...) before it reaches logs/status. detail should go through the same masking (plus stripping control characters) before it's stored on group.detail here, so both the toast and the log inherit the redaction.

Flagged by: Codex — verified against the existing maskString precedent.

[ralphstodomingo]

Fixed in two places.

• Engine (the source): since 0.7.3, which this PR now requires, the engine sends detail only for known error shapes, such as spawn docker ENOENT, connect ECONNREFUSED host:port or Invalid URL. Anything else arrives as details in the engine log, and a spawn failure is reduced to a plain command name.
• Client (19f8eae): parseUnfulfilled now collapses control characters and runs Telemetry.maskString, the same treatment as subprocess stderr in mcp/index.ts. Because this happens at parse time, the toast and log.info both carry the masked text. Test: "a detail is masked and flattened before anything shows or logs it".

One limit, recorded as R4: maskString masks paths and known key shapes, but a short bare token passes through it. The guarantee that no configuration reaches the client comes from the engine side.

[sahrizvi]

MINOR (Logic) — variant is warning only when missingReport.length > 0. But served < declared.keys.length can also happen with an empty report — e.g. two raw keys that sanitize to the same catalog entry ("1 of 2 declared integration tools available" currently renders as info). The report is authoritative about reasons; the client is authoritative about what's actually callable post-sanitization, and severity should reflect the latter. The absent-report case is already tested and asserts info (today's deliberate behavior) — the collision case specifically has no variant assertion.

Flagged by: Codex.

[ralphstodomingo]

Fixed in 19f8eae. The toast is now a warning when a gap is reported, and also when a report is present and fewer declared tools are callable than were declared. The collision case (two raw keys that sanitise to one catalog entry) asserts warning. The no-report case stays info, as before. The no-bridge test now declares only the served keys, so nothing but its no-bridge entries could affect the variant.

[sahrizvi]

MINOR (Logic) — the re-announcement signature is built from ${u.key}=${u.reason} only. If a spawn-failed detail changes from e.g. spawn docker ENOENT to a different actionable error while reason stays spawn-failed, the outcome updates but the toast doesn't re-fire — the user is left with stale remediation text. Consider folding integrationId/detail into the signature too.

Flagged by: Codex.

[ralphstodomingo]

Fixed in 19f8eae. The signature now carries integration, key, reason and detail, so a changed error text under the same reason re-announces. Test: "a gap whose error text changed under the same reason is announced again".

[sahrizvi]

MINOR (Design/Robustness) — a report of N valid entries plus 1 malformed one is discarded wholesale (return undefined on the first bad entry), and the toast silently falls back to showing no gap information at all. This is deliberate and tested (the file's own comment: "Undefined... is not the same as knowing there are none") and the failure mode is honest rather than dangerous — but there's currently no diagnostic signal when it happens. Worth a logWarning on discard, purely for diagnosability.

Flagged by: MiMo (downgraded from an initial MAJOR — confirmed as Minor by 2 convergence reviewers given the fail-closed behavior is intentional and tested).

[ralphstodomingo]

Added in 19f8eae. When a report is present but malformed and gets dropped, the attach logs a warning. It fires once per verdict, and it logs only the workspace, never the report content.

[sahrizvi]

MINOR (Testing) — isolatedHome() (line 170) creates a temp dir via mkdtempSync that's never removed here in finally — only api.close() is called. Leaves config/log directories behind on every enabled run, especially in CI. Consider an rmSync(home, { recursive: true, force: true }) alongside api.close().

Flagged independently by 2 reviewers: Codex + MiMo.

[ralphstodomingo]

Fixed in 19f8eae. home is removed in finally next to api.close(). I re-ran it against the published 0.7.3, and no temporary HOME was left behind.

[sahrizvi]

Requesting changes

Reversing the earlier approve on this consensus review. The MAJOR finding in the inline comments above — engine-reported detail strings (spawn errors / connection failures from arbitrary custom MCP integrations, which can contain credentials, tokens, or paths) reach both log.info and the attach toast unredacted, bypassing this codebase's own existing Telemetry.maskString convention (mcp/index.ts:232) — should be fixed before merge.

The 4 MINOR findings are not blocking but worth addressing in this PR or a fast follow-up.

Full review with attribution: reviews/pr-1308-consensus-review.md in the team's review archive.

[ralphstodomingo]

Closing while the workspaces line is re-scoped with the team (this is section WL-2 of the write-up shared internally). The branch stays as it is; this reopens unchanged once that section is signed off.

[ralphstodomingo]

Engine → altimate-code through the real attach path, on 0.7.3

The same harness as the earlier evidence comment (.e2e/engine-to-cli.ts + .e2e/run.sh), re-run on head 3fa055b21 after the floor moved to 0.7.3.

Release candidate: engine main with AltimateAI/altimate-mcp-engine#248 and AltimateAI/altimate-mcp-engine#250 merged, built locally, datamate --version = 0.7.3. Settled attached in 6241 ms, declared: 5, missing: ['jira_search_issues', 'ghost', 'whatever', 'retired_tool'].

1 of 5 declared integration tools available. Declared but not available — no usable connection: jira_search_issues; not offered by the integration: ghost; server could not be started or reached (spawn altimate-e2e-missing-binary ENOENT): whatever; no longer in the catalog: retired_tool.

• PASS outcome is attached
• PASS declared counted from the allowlist (5 CLI-servable keys)
• PASS missing = every gap the engine reported except no-bridge
• PASS outcome carries the full report incl. the no-bridge entry
• PASS spawn-failed detail carries the engine's error
• PASS exactly one toast, warning
• PASS toast text
• PASS every API call served (no 404)

Floor negative: a build of the engine at AltimateAI/altimate-mcp-engine#248's head, versioned 0.7.2, so it emits the report but carries the version that shipped without it. Settles engine-too-old (found 0.7.2).

• PASS engine below the floor is refused as engine-too-old
• PASS refusal names the found version and the floor

The harness's expected toast text was updated for the spawn-failed wording this PR already uses ("server could not be started or reached"); nothing else changed.

[ralphstodomingo]

@codex review

Scope: the changes since 3fa055b21, which are 19f8eae01 (review fixes) and 87efc09a0 (test isolation), plus the merge of main in 906c367fa. Please check them against claims C1 to C7 and residuals R1 to R4 in the description. Report a finding only if a claim can be shown false with concrete input, or if a residual understates a risk. The TypeScript job failure (FileSystemEntry in packages/sdk) and the createThenBindOrRebind test failure both come from main and are out of scope.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 87efc09a07

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Log malformed reports before deduplicating announcements

C1 is false when a session first observes no report (or a valid empty report) and a later reconciliation receives a malformed report with the same inventory. Both states produce the same gaps value, so this return runs before the malformed-report warning and the malformed metadata is silently discarded. Move the validation warning ahead of announcement deduplication, or include report validity in the signature, so the first transition to malformed metadata is always diagnosed.

Useful? React with 👍 / 👎.

[ralphstodomingo]

Confirmed and fixed in 4aa8e68. The session record now remembers whether the last report was malformed. The warning is checked before the announcement dedupe and logged once per transition into the malformed state, with no second toast. Test: "a report that turns malformed is logged once per transition, without a second toast". It walks empty, then malformed twice, then empty, then malformed again, and fails with the warning placed after the dedupe.

[ralphstodomingo]

@codex review

Scope: only 4aa8e68b1, the fix for your finding on the malformed-report warning. Please try to falsify claim C1 as reworded in the description ("logged as a warning once per transition into that state, even when the announcement itself is unchanged"), using concrete sequences of reports across turns. Everything else was covered by the previous round.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: 4aa8e68b1a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".