From ac4f94f8e45de2daf2b3706ac7ebfd9e9f152c6c Mon Sep 17 00:00:00 2001 From: Renaud Laborbe <150821561+BluegReeno@users.noreply.github.com> Date: Sat, 25 Jul 2026 12:14:28 +0200 Subject: [PATCH 1/3] Fix: push_mission_context uses targeted UPDATE, not partial upsert (#74) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit push_mission_context built a partial row per observation and called .upsert() on edifice_notes. Supabase compiles upsert to INSERT ... ON CONFLICT DO UPDATE, so the INSERT arm must satisfy every NOT NULL column (including project_id, never in the partial payload) — every push failed with a not-null violation. Changes: - Replace the batched upsert with a per-observation UPDATE ... WHERE id = note_id ... RETURNING id, which never touches columns the caller did not supply (no schema change, no DDL) - Count `updated` from rows actually returned by the database, not from the number of rows sent - Report an unknown note_id in `errors` ("not found") instead of silently inserting an orphan row — an UPDATE cannot create rows - Rewrite the note-path unit tests to mock .update().eq().select() and add a test pinning the unknown-note_id behavior Behaviour stays idempotent: pushing the same observations twice re-issues the same UPDATE and reports the same count with no state change. Fixes #74 --- supabase/functions/hal-mcp/index.test.ts | 95 +++++++++++++++++------- supabase/functions/hal-mcp/index.ts | 38 +++++----- 2 files changed, 91 insertions(+), 42 deletions(-) diff --git a/supabase/functions/hal-mcp/index.test.ts b/supabase/functions/hal-mcp/index.test.ts index c8075b1..ceff020 100644 --- a/supabase/functions/hal-mcp/index.test.ts +++ b/supabase/functions/hal-mcp/index.test.ts @@ -740,18 +740,31 @@ Deno.test("resolveWhoamiPayload — userClaims without email: user_email coalesc }); // --------------------------------------------------------------------------- -// push_mission_context — batch upsert (#25) +// push_mission_context — per-observation targeted UPDATE (#74) // --------------------------------------------------------------------------- -Deno.test("push_mission_context — batch upsert succeeds: updated = rows.length", async () => { - const STATIC_KEY = "test-static-key-abc123"; - Deno.env.set("SUPABASE_SECRET_KEYS", JSON.stringify({ hal_api_key: STATIC_KEY })); +// One .update().eq().select() chain that resolves the given result. select("id") +// is the terminal awaited call, so it returns the promise. +// deno-lint-ignore no-explicit-any +function noteUpdateChain(result: { data: unknown; error: unknown }): any { // deno-lint-ignore no-explicit-any - const upsertMock: any = { - upsert: () => Promise.resolve({ error: null }), + const chain: any = { + update: () => chain, + eq: () => chain, + select: () => Promise.resolve(result), }; - // deno-lint-ignore no-explicit-any - const s = stub(_adminClient, "from", returnsNext([upsertMock as any])); + return chain; +} + +Deno.test("push_mission_context — per-observation update succeeds: updated counts matched rows", async () => { + const STATIC_KEY = "test-static-key-abc123"; + Deno.env.set("SUPABASE_SECRET_KEYS", JSON.stringify({ hal_api_key: STATIC_KEY })); + const s = stub(_adminClient, "from", returnsNext([ + // deno-lint-ignore no-explicit-any + noteUpdateChain({ data: [{ id: "n1" }], error: null }) as any, + // deno-lint-ignore no-explicit-any + noteUpdateChain({ data: [{ id: "n2" }], error: null }) as any, + ])); try { const res = await fetchHandler(callTool(STATIC_KEY, "push_mission_context", { observations: [ @@ -771,15 +784,13 @@ Deno.test("push_mission_context — batch upsert succeeds: updated = rows.length } }); -Deno.test("push_mission_context — batch upsert DB error returns isError with updated=0", async () => { +Deno.test("push_mission_context — DB error returns isError with updated=0", async () => { const STATIC_KEY = "test-static-key-abc123"; Deno.env.set("SUPABASE_SECRET_KEYS", JSON.stringify({ hal_api_key: STATIC_KEY })); - // deno-lint-ignore no-explicit-any - const upsertMock: any = { - upsert: () => Promise.resolve({ error: { message: "constraint violation" } }), - }; - // deno-lint-ignore no-explicit-any - const s = stub(_adminClient, "from", returnsNext([upsertMock as any])); + const s = stub(_adminClient, "from", returnsNext([ + // deno-lint-ignore no-explicit-any + noteUpdateChain({ data: null, error: { message: "constraint violation" } }) as any, + ])); try { const res = await fetchHandler(callTool(STATIC_KEY, "push_mission_context", { observations: [{ note_id: "n1", description: "Test" }], @@ -788,6 +799,7 @@ Deno.test("push_mission_context — batch upsert DB error returns isError with u assertEquals(body.result.isError, true); const result = JSON.parse(body.result.content[0].text); assertEquals(result.updated, 0); + assertEquals(result.errors[0].includes("n1"), true); assertEquals(result.errors[0].includes("constraint violation"), true); } finally { s.restore(); @@ -798,7 +810,16 @@ Deno.test("push_mission_context — batch upsert DB error returns isError with u Deno.test("push_mission_context — empty patch observation is skipped", async () => { const STATIC_KEY = "test-static-key-abc123"; Deno.env.set("SUPABASE_SECRET_KEYS", JSON.stringify({ hal_api_key: STATIC_KEY })); - // No from() call expected — rows array stays empty, upsert not called + // No from() call expected — the only observation has no updatable fields + let updateCalled = false; + // deno-lint-ignore no-explicit-any + const chain: any = { + update: () => { updateCalled = true; return chain; }, + eq: () => chain, + select: () => Promise.resolve({ data: [], error: null }), + }; + // deno-lint-ignore no-explicit-any + const s = stub(_adminClient, "from", returnsNext([chain as any])); try { const res = await fetchHandler(callTool(STATIC_KEY, "push_mission_context", { observations: [{ note_id: "n1" }], @@ -809,7 +830,9 @@ Deno.test("push_mission_context — empty patch observation is skipped", async ( assertEquals(result.updated, 0); assertEquals(result.skipped, 1); assertEquals(result.errors, []); + assertEquals(updateCalled, false); } finally { + s.restore(); Deno.env.set("SUPABASE_SECRET_KEYS", DEFAULT_SECRET_KEYS); } }); @@ -817,22 +840,44 @@ Deno.test("push_mission_context — empty patch observation is skipped", async ( Deno.test("push_mission_context — assessment does not set condition_index", async () => { const STATIC_KEY = "test-static-key-abc123"; Deno.env.set("SUPABASE_SECRET_KEYS", JSON.stringify({ hal_api_key: STATIC_KEY })); - let capturedRows: unknown[] = []; + let capturedFields: Record = {}; // deno-lint-ignore no-explicit-any - const upsertMock: any = { - upsert: (rows: unknown[]) => { - capturedRows = rows; - return Promise.resolve({ error: null }); - }, + const chain: any = { + update: (fields: Record) => { capturedFields = fields; return chain; }, + eq: () => chain, + select: () => Promise.resolve({ data: [{ id: "n1" }], error: null }), }; // deno-lint-ignore no-explicit-any - const s = stub(_adminClient, "from", returnsNext([upsertMock as any])); + const s = stub(_adminClient, "from", returnsNext([chain as any])); try { const res = await fetchHandler(callTool(STATIC_KEY, "push_mission_context", { observations: [{ note_id: "n1", assessment: "2" }], }, 603)); await parseMcpResponse(res); // consume stream so tool handler completes - assertEquals("condition_index" in (capturedRows[0] as Record), false); + assertEquals("condition_index" in capturedFields, false); + } finally { + s.restore(); + Deno.env.set("SUPABASE_SECRET_KEYS", DEFAULT_SECRET_KEYS); + } +}); + +Deno.test("push_mission_context — unknown note_id is reported in errors, not inserted", async () => { + const STATIC_KEY = "test-static-key-abc123"; + Deno.env.set("SUPABASE_SECRET_KEYS", JSON.stringify({ hal_api_key: STATIC_KEY })); + const s = stub(_adminClient, "from", returnsNext([ + // zero rows matched, no error — an UPDATE cannot insert + // deno-lint-ignore no-explicit-any + noteUpdateChain({ data: [], error: null }) as any, + ])); + try { + const res = await fetchHandler(callTool(STATIC_KEY, "push_mission_context", { + observations: [{ note_id: "does-not-exist", description: "orphan edit" }], + }, 607)); + const body = await parseMcpResponse(res); + assertEquals(body.result.isError, true); + const result = JSON.parse(body.result.content[0].text); + assertEquals(result.updated, 0); + assertEquals(result.errors.some((e: string) => e.includes("does-not-exist") && e.includes("not found")), true); } finally { s.restore(); Deno.env.set("SUPABASE_SECRET_KEYS", DEFAULT_SECRET_KEYS); @@ -1076,7 +1121,7 @@ Deno.test("push_mission_context — isError when obs succeed but crop fails", as Deno.env.set("SUPABASE_SECRET_KEYS", JSON.stringify({ hal_api_key: STATIC_KEY })); // deno-lint-ignore no-explicit-any - const notesMock: any = { upsert: () => Promise.resolve({ error: null }) }; + const notesMock: any = noteUpdateChain({ data: [{ id: "n1" }], error: null }); // deno-lint-ignore no-explicit-any const cropMock: any = { upsert: () => Promise.resolve({ error: { message: "crop upsert failed" } }) }; // deno-lint-ignore no-explicit-any diff --git a/supabase/functions/hal-mcp/index.ts b/supabase/functions/hal-mcp/index.ts index 728c07a..7490324 100644 --- a/supabase/functions/hal-mcp/index.ts +++ b/supabase/functions/hal-mcp/index.ts @@ -420,27 +420,31 @@ registerTool( let skipped = 0; const errors: string[] = []; - const rows: Record[] = []; for (const obs of observations) { if (!obs.note_id) { skipped++; continue; } - const row: Record = { id: obs.note_id }; - if (obs.type) row.type = obs.type; - if (obs.description) row.description = obs.description; - if (obs.location) row.location = obs.location; - if (obs.zone) row.zone = obs.zone; + const fields: Record = {}; + if (obs.type) fields.type = obs.type; + if (obs.description) fields.description = obs.description; + if (obs.location) fields.location = obs.location; + if (obs.zone) fields.zone = obs.zone; if (obs.assessment !== undefined) { - row.assessment = obs.assessment; + fields.assessment = obs.assessment; + } + if (obs.recommendations) fields.recommendations = obs.recommendations; + if (obs.metadata && Object.keys(obs.metadata).length) fields.metadata = obs.metadata; + if (Object.keys(fields).length === 0) { skipped++; continue; } + + const { data, error } = await db.from("edifice_notes") + .update(fields) + .eq("id", obs.note_id) + .select("id"); + if (error) { + errors.push(`note ${obs.note_id}: ${error.message}`); + } else if (!data || data.length === 0) { + errors.push(`note ${obs.note_id}: not found`); + } else { + updated++; } - if (obs.recommendations) row.recommendations = obs.recommendations; - if (obs.metadata && Object.keys(obs.metadata).length) row.metadata = obs.metadata; - if (Object.keys(row).length === 1) { skipped++; continue; } - rows.push(row); - } - - if (rows.length > 0) { - const { error } = await db.from("edifice_notes").upsert(rows); - if (error) errors.push(error.message); - else updated = rows.length; } if (building_id && building_description) { From 8430535bbc5756ad0932a1af358434e1499c23be Mon Sep 17 00:00:00 2001 From: Renaud Laborbe <150821561+BluegReeno@users.noreply.github.com> Date: Sat, 25 Jul 2026 12:26:53 +0200 Subject: [PATCH 2/3] test(hal-mcp): pin mixed-batch isolation + eq() row-targeting for push_mission_context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixed: - HIGH: no test covered mixed outcomes (one success, one not-found) in a single push_mission_context call — the exact new cross-iteration aggregation behavior this PR introduced. Added "mixed batch: one succeeds, one not found". - MEDIUM: noteUpdateChain discarded eq() arguments, so row-targeting by note_id was unverifiable. noteUpdateChain now records eq() calls on chain._eqCalls; asserted in the multi-observation success test and the new mixed-batch test. - LOW: data === null (no error) branch of the not-found guard was untested. Added "null data (no error) is treated as not found". Skipped: - noteUpdateChain vs makeUpdateMock "duplication" (LOW) — reviewers independently confirmed EXTENDS not DUPLICATE (different terminal chain shape); no action needed. - Idempotency unit test (LOW) — reviewers' own recommendation was no unit-test action, since it's an UPDATE-semantics property, not application logic; would belong at the integration/migration-smoke-test layer if ever verified. All 63 Deno tests pass; deno check clean. --- supabase/functions/hal-mcp/index.test.ts | 69 +++++++++++++++++++++--- 1 file changed, 61 insertions(+), 8 deletions(-) diff --git a/supabase/functions/hal-mcp/index.test.ts b/supabase/functions/hal-mcp/index.test.ts index ceff020..3514524 100644 --- a/supabase/functions/hal-mcp/index.test.ts +++ b/supabase/functions/hal-mcp/index.test.ts @@ -744,27 +744,28 @@ Deno.test("resolveWhoamiPayload — userClaims without email: user_email coalesc // --------------------------------------------------------------------------- // One .update().eq().select() chain that resolves the given result. select("id") -// is the terminal awaited call, so it returns the promise. +// is the terminal awaited call, so it returns the promise. Records eq() calls on +// chain._eqCalls so tests can assert the correct row was targeted by note_id. // deno-lint-ignore no-explicit-any function noteUpdateChain(result: { data: unknown; error: unknown }): any { + const eqCalls: unknown[][] = []; // deno-lint-ignore no-explicit-any const chain: any = { update: () => chain, - eq: () => chain, + eq: (col: string, val: unknown) => { eqCalls.push([col, val]); return chain; }, select: () => Promise.resolve(result), }; + chain._eqCalls = eqCalls; return chain; } Deno.test("push_mission_context — per-observation update succeeds: updated counts matched rows", async () => { const STATIC_KEY = "test-static-key-abc123"; Deno.env.set("SUPABASE_SECRET_KEYS", JSON.stringify({ hal_api_key: STATIC_KEY })); - const s = stub(_adminClient, "from", returnsNext([ - // deno-lint-ignore no-explicit-any - noteUpdateChain({ data: [{ id: "n1" }], error: null }) as any, - // deno-lint-ignore no-explicit-any - noteUpdateChain({ data: [{ id: "n2" }], error: null }) as any, - ])); + const chain1 = noteUpdateChain({ data: [{ id: "n1" }], error: null }); + const chain2 = noteUpdateChain({ data: [{ id: "n2" }], error: null }); + // deno-lint-ignore no-explicit-any + const s = stub(_adminClient, "from", returnsNext([chain1 as any, chain2 as any])); try { const res = await fetchHandler(callTool(STATIC_KEY, "push_mission_context", { observations: [ @@ -778,6 +779,8 @@ Deno.test("push_mission_context — per-observation update succeeds: updated cou assertEquals(result.updated, 2); assertEquals(result.skipped, 0); assertEquals(result.errors, []); + assertEquals(chain1._eqCalls, [["id", "n1"]]); + assertEquals(chain2._eqCalls, [["id", "n2"]]); } finally { s.restore(); Deno.env.set("SUPABASE_SECRET_KEYS", DEFAULT_SECRET_KEYS); @@ -884,6 +887,56 @@ Deno.test("push_mission_context — unknown note_id is reported in errors, not i } }); +Deno.test("push_mission_context — null data (no error) is treated as not found", async () => { + const STATIC_KEY = "test-static-key-abc123"; + Deno.env.set("SUPABASE_SECRET_KEYS", JSON.stringify({ hal_api_key: STATIC_KEY })); + const s = stub(_adminClient, "from", returnsNext([ + // deno-lint-ignore no-explicit-any + noteUpdateChain({ data: null, error: null }) as any, + ])); + try { + const res = await fetchHandler(callTool(STATIC_KEY, "push_mission_context", { + observations: [{ note_id: "does-not-exist", description: "orphan edit" }], + }, 609)); + const body = await parseMcpResponse(res); + assertEquals(body.result.isError, true); + const result = JSON.parse(body.result.content[0].text); + assertEquals(result.updated, 0); + assertEquals(result.errors.some((e: string) => e.includes("does-not-exist") && e.includes("not found")), true); + } finally { + s.restore(); + Deno.env.set("SUPABASE_SECRET_KEYS", DEFAULT_SECRET_KEYS); + } +}); + +Deno.test("push_mission_context — mixed batch: one succeeds, one not found", async () => { + const STATIC_KEY = "test-static-key-abc123"; + Deno.env.set("SUPABASE_SECRET_KEYS", JSON.stringify({ hal_api_key: STATIC_KEY })); + const chain1 = noteUpdateChain({ data: [{ id: "n1" }], error: null }); + const chain2 = noteUpdateChain({ data: [], error: null }); + // deno-lint-ignore no-explicit-any + const s = stub(_adminClient, "from", returnsNext([chain1 as any, chain2 as any])); + try { + const res = await fetchHandler(callTool(STATIC_KEY, "push_mission_context", { + observations: [ + { note_id: "n1", description: "Fissure façade" }, + { note_id: "n2", description: "orphan edit" }, + ], + }, 610)); + const body = await parseMcpResponse(res); + assertEquals(body.result.isError, true); + const result = JSON.parse(body.result.content[0].text); + assertEquals(result.updated, 1); + assertEquals(result.errors.length, 1); + assertEquals(result.errors[0].includes("n2") && result.errors[0].includes("not found"), true); + assertEquals(chain1._eqCalls, [["id", "n1"]]); + assertEquals(chain2._eqCalls, [["id", "n2"]]); + } finally { + s.restore(); + Deno.env.set("SUPABASE_SECRET_KEYS", DEFAULT_SECRET_KEYS); + } +}); + // --------------------------------------------------------------------------- // Phase 3: crop_region + bounding-box annotations round-trip // --------------------------------------------------------------------------- From 5082189dbaf214a2e32e9db44bc5514a21886a02 Mon Sep 17 00:00:00 2001 From: Renaud Laborbe <150821561+BluegReeno@users.noreply.github.com> Date: Sat, 25 Jul 2026 12:29:40 +0200 Subject: [PATCH 3/3] simplify: reduce complexity in changed files --- supabase/functions/hal-mcp/index.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/supabase/functions/hal-mcp/index.ts b/supabase/functions/hal-mcp/index.ts index 7490324..b0e05f8 100644 --- a/supabase/functions/hal-mcp/index.ts +++ b/supabase/functions/hal-mcp/index.ts @@ -427,9 +427,7 @@ registerTool( if (obs.description) fields.description = obs.description; if (obs.location) fields.location = obs.location; if (obs.zone) fields.zone = obs.zone; - if (obs.assessment !== undefined) { - fields.assessment = obs.assessment; - } + if (obs.assessment !== undefined) fields.assessment = obs.assessment; if (obs.recommendations) fields.recommendations = obs.recommendations; if (obs.metadata && Object.keys(obs.metadata).length) fields.metadata = obs.metadata; if (Object.keys(fields).length === 0) { skipped++; continue; }