fix(finops): unprefixed BigQuery location examples; one snapshot read for the reason; stricter tests

Bot review of 8c0eb30 on #1346. The note said to replace `<location>` with
`region-us`, which yields `region-region-us`; the examples are `us`, `eu`,
`us-central1`. The disabled reason is carried on `FallbackLookup` from the
same read as the snapshot, so the note cannot describe a different snapshot.
The wrapper test requires the real handler's FAILED branch (not a wrapper
catch), and precedence announcements are stubbed in setup.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6

Author: sahrizvi

packages/opencode/src/altimate/tools/finops-workspace.ts

@@ -84,9 +84,14 @@ export interface WorkspaceFallback {
 
 /** What the failure path learns about the workspace, in one read. */
 export type FallbackLookup =
-  | { state: "current"; fallbacks: WorkspaceFallback[] }
+  | { state: "current"; fallbacks: WorkspaceFallback[]; reason: Reason }
   | { state: "relinked" | "unreadable" }
 
+/** Why nothing is served, read once with the snapshot so the note describes the
+ * same snapshot the lookup did: `none` — no snapshot for the session; a disabled
+ * reason — the snapshot's own; `served` — enabled. */
+type Reason = "none" | "served" | NonNullable<Precedence.Precedence["disabledReason"]>
+
 /**
  * The workspace-served execute tools for the types a FinOps operation supports, if the
  * session is bound to a workspace that serves any. Reads the session's precedence
@@ -97,17 +102,18 @@ export type FallbackLookup =
  */
 export async function workspaceFallbacks(sessionID: string, supportedTypes: readonly string[]): Promise<FallbackLookup> {
   const precedence = Precedence.forSession(sessionID)
-  if (!precedence?.enabled) return { state: "current", fallbacks: [] }
+  if (!precedence) return { state: "current", fallbacks: [], reason: "none" }
+  if (!precedence.enabled) return { state: "current", fallbacks: [], reason: precedence.disabledReason ?? "served" }
   const fallbacks = Precedence.servedInventory(precedence).flatMap(({ type, served }) => {
     if (!supportedTypes.includes(type)) return []
     const execute = served.find((row) => row.capability === "sql_execute")
     return execute
       ? [{ workspaceName: precedence.workspaceName, workspaceId: precedence.workspaceId, type, modelKey: execute.modelKey }]
       : []
   })
-  if (fallbacks.length === 0) return { state: "current", fallbacks }
+  if (fallbacks.length === 0) return { state: "current", fallbacks, reason: "served" }
   const state = await Precedence.snapshotState(precedence)
-  return state === "current" ? { state, fallbacks } : { state }
+  return state === "current" ? { state, fallbacks, reason: "served" } : { state }
 }
 
 /** The sentence appended to a FinOps failure, or nothing when the workspace serves
@@ -126,8 +132,9 @@ export function workspaceFallbackNote(operation: FinopsOperation, fallbacks: Wor
   // The snapshot does not carry a BigQuery integration's location, and the engine
   // runs what it is given: the placeholder has to be explained, not left to be sent.
   const region = fallbacks.some((f) => f.type === "bigquery" && SOURCE[operation].bigquery)
-    ? " Replace `<location>` with the BigQuery connection's location (for example `region-us`, `region-eu`); " +
-      "if it is unknown, ask the engine for the connection's details first — the view is not reachable unqualified."
+    ? " Replace `<location>` with the BigQuery connection's location (for example `us`, `eu`, `us-central1`, " +
+      "giving `region-us.INFORMATION_SCHEMA…`); if it is unknown, ask the engine for the connection's details " +
+      "first — the view is not reachable unqualified."
     : ""
   return (
     `This tool only uses warehouse connections configured on this machine, and workspace ${label} ` +
@@ -141,20 +148,19 @@ export function workspaceFallbackNote(operation: FinopsOperation, fallbacks: Wor
  * materialised) says nothing: that is the plain local failure. Uncertainty must say so
  * (the precedence module's first claim), so the failure is marked `undetermined` and
  * says the workspace could not be consulted — mirroring `check()`'s own cases. */
-function undeterminedNote(sessionID: string, lookup: FallbackLookup): string | undefined {
+function undeterminedNote(lookup: FallbackLookup): string | undefined {
   if (lookup.state === "unreadable") {
     return "The workspace link could not be read while this call ran, so whether the workspace serves this connection type is unknown."
   }
-  if (lookup.state === "relinked") {
+  if (lookup.state !== "current") {
     return "The workspace binding changed while this call ran, so the previous routing decision no longer applies."
   }
-  const precedence = Precedence.forSession(sessionID)
   // No snapshot at all is the same unknown `check()` reports: a caller that never
   // resolved tools, or an entry evicted between resolution and this call.
-  if (!precedence) {
+  if (lookup.reason === "none") {
     return "No routing decision was available for this call, so whether the linked workspace serves this connection type is unknown."
   }
-  const reason = precedence.disabledReason
+  const reason = lookup.reason
   if (reason === "unattributed" || reason === "binding-unreadable" || reason === "derive-failed") {
     return (
       "Whether the linked workspace serves this connection type could not be determined this turn " +
@@ -185,7 +191,7 @@ export async function withWorkspaceFallback<T extends { metadata: Record<string,
       output: `${result.output}\n\n${note}`,
     }
   }
-  const undetermined = undeterminedNote(sessionID, lookup)
+  const undetermined = undeterminedNote(lookup)
   if (!undetermined) return result
   return {
     ...result,

packages/opencode/test/altimate/finops-workspace-fallback.test.ts

@@ -35,6 +35,8 @@ beforeEach(() => {
   process.env.ALTIMATE_WORKSPACE = "1"
   process.env.ALTIMATE_TELEMETRY_DISABLED = "true"
   bindTo(42, "analytics")
+  // `refresh` queues an announcement; keep it off the real event bridge. (bot review)
+  precedenceInternals.announce = async () => {}
   // The pilot's promise: no local warehouse connection at all.
   Registry.setConfigs({})
 })
@@ -55,34 +57,35 @@ describe("workspaceFallbacks", () => {
     await refresh(SESSION, SNOWFLAKE_TOOLS)
     expect(await workspaceFallbacks(SESSION, DEFAULT_FINOPS_TYPES)).toEqual({
       state: "current",
+      reason: "served",
       fallbacks: [
         { workspaceName: "analytics", workspaceId: "42", type: "snowflake", modelKey: "datamate_snowflake_execute_database_query" },
       ],
     })
   })
 
   test("is empty for a session with no routing decision", async () => {
-    expect(await workspaceFallbacks(SESSION, DEFAULT_FINOPS_TYPES)).toEqual({ state: "current", fallbacks: [] })
+    expect(await workspaceFallbacks(SESSION, DEFAULT_FINOPS_TYPES)).toMatchObject({ state: "current", fallbacks: [] })
   })
 
   test("is empty when routing is disabled for the session", async () => {
     process.env.ALTIMATE_INTEGRATIONS = "local"
     await refresh(SESSION, SNOWFLAKE_TOOLS)
-    expect(await workspaceFallbacks(SESSION, DEFAULT_FINOPS_TYPES)).toEqual({ state: "current", fallbacks: [] })
+    expect(await workspaceFallbacks(SESSION, DEFAULT_FINOPS_TYPES)).toMatchObject({ state: "current", fallbacks: [] })
   })
 
   test("ignores a served type the operation does not support", async () => {
     await refresh(SESSION, BIGQUERY_TOOLS)
     // The Snowflake-only operations (role hierarchy, user roles) get nothing from a
     // workspace that serves only BigQuery.
-    expect(await workspaceFallbacks(SESSION, ["snowflake"])).toEqual({ state: "current", fallbacks: [] })
+    expect(await workspaceFallbacks(SESSION, ["snowflake"])).toMatchObject({ state: "current", fallbacks: [] })
     const lookup = await workspaceFallbacks(SESSION, DEFAULT_FINOPS_TYPES)
     expect(lookup.state === "current" && lookup.fallbacks.map((f) => f.type)).toEqual(["bigquery"])
   })
 
   test("never names a tool the caller's agent cannot call", async () => {
     await refresh(SESSION, SNOWFLAKE_TOOLS, ANALYST_RULESET)
-    expect(await workspaceFallbacks(SESSION, DEFAULT_FINOPS_TYPES)).toEqual({ state: "current", fallbacks: [] })
+    expect(await workspaceFallbacks(SESSION, DEFAULT_FINOPS_TYPES)).toMatchObject({ state: "current", fallbacks: [] })
   })
 })
 
@@ -110,7 +113,8 @@ describe("workspaceFallbackNote", () => {
     for (const op of ["query_history", "analyze_credits", "expensive_queries", "warehouse_advice", "unused_resources", "role_grants"] as const) {
       expect(workspaceFallbackNote(op, bigquery)).toMatch(/region-<location>\.INFORMATION_SCHEMA/)
       // …and the placeholder is explained, since the snapshot carries no location.
-      expect(workspaceFallbackNote(op, bigquery)).toContain("Replace `<location>`")
+      expect(workspaceFallbackNote(op, bigquery)).toContain("for example `us`, `eu`")
+      expect(workspaceFallbackNote(op, bigquery)).not.toContain("`region-us`")
     }
     expect(workspaceFallbackNote("query_history", snowflake)).not.toContain("<location>")
     expect(workspaceFallbackNote("unused_resources", snowflake)).toContain("`QUERY_HISTORY`")
@@ -234,7 +238,10 @@ describe("through the tools", () => {
     for (const [def, args, table] of tools) {
       const tool = await initTool(def as never)
       const result = await tool.execute(args, ctx())
-      expect(result.title, tool.id).toMatch(/FAILED|ERROR/)
+      // FAILED, not ERROR: the real handler's no-connection branch, not a wrapper catching
+      // "No native handler". (bot review)
+      expect(result.title, tool.id).toMatch(/FAILED$/)
+      expect(result.output, tool.id).toContain("requires a configured warehouse")
       expect(result.output, tool.id).toContain("`datamate_snowflake_execute_database_query`")
       expect(result.output, tool.id).toContain(table)
       expect(result.metadata.workspace_fallback, tool.id).toEqual(["datamate_snowflake_execute_database_query"])