fix(hal-mcp): push_mission_context upserts partial note rows — every push fails on project_id NOT NULL

[BluegReeno]

Summary

push_mission_context writes partial note updates with .upsert(), which Postgres executes as
INSERT … ON CONFLICT DO UPDATE. The INSERT arm must satisfy every NOT NULL column, but the
payload only ever carries the fields the caller changed. Any push therefore fails with:

null value in column "project_id" of relation "edifice_notes" violates not-null constraint

/edifice push is broken end to end: field observations edited on site cannot be pushed back.

Location

supabase/functions/hal-mcp/index.ts:441

const row: Record<string, unknown> = { id: obs.note_id };
if (obs.type) row.type = obs.type;
if (obs.description) row.description = obs.description;
// … only the fields the caller supplied
rows.push(row);
// …
const { error } = await db.from("edifice_notes").upsert(rows);   // ← INSERT … ON CONFLICT

project_id is never part of row, because the tool's contract is a partial update keyed by
note_id — the row already exists and already has its project_id.

Two further defects in the same block

1. updated is fabricated. :443 sets updated = rows.length — the number of rows sent,
not the number affected. On success the tool reports rows it may never have touched. The
{ updated, skipped, errors } contract the skill relays to the user is not trustworthy.

2. upsert would silently create orphan notes. A note_id that does not exist should be an
error, not an insert. Today the NOT NULL on project_id is the only thing preventing a bogus
note_id from creating a phantom note — the constraint is masking the design fault rather than
the design being right.

Fix direction

Replace the batch upsert with a targeted UPDATE per observation, which is what the contract
has always meant:

const { data, error } = await db
  .from("edifice_notes")
  .update(fields)          // row without `id`
  .eq("id", obs.note_id)
  .select("id");

• Count updated from the returned rows, so a non-existent note_id lands in skipped or
  errors instead of being reported as written.
• A push of N observations becomes N statements. Mission-sized payloads are a few dozen rows, so
  this is acceptable; if it ever isn't, move it to a plpgsql RPC doing
  UPDATE … FROM (VALUES …) in one statement — do not go back to upsert.
• Behaviour must stay idempotent: pushing the same observations twice changes nothing.

Reproduction

1. Open a mission in the local workspace, edit a note description.
2. Run /edifice push.
3. errors contains the null value in column "project_id" message; nothing is written.

Acceptance

• push_mission_context updates existing edifice_notes rows by note_id, preserving
  project_id and every other column the payload does not mention.
• updated reflects rows actually affected.
• An unknown note_id is reported, never inserted.
• Pushing twice is a no-op.

Related

• BluegReeno/bluegreen-marketplace#31 — the skill-side report. PR
  BluegReeno/bluegreen-marketplace#41 only documents the failure as a known issue; the fix
  belongs here.

[BluegReeno]

🔍 Investigation: push_mission_context upserts partial note rows

Type: BUG

Assessment

Metric	Value	Reasoning
Severity	HIGH	/edifice push is broken end to end for every observation edit, no workaround from the skill side.
Complexity	LOW	Single function (push_mission_context) plus its unit tests, no schema change, no new integration points.
Confidence	HIGH	Root cause explicit in the issue body with the exact failing SQL error; fix direction fully specified; current code read directly.

---

Problem Statement

push_mission_context builds a partial row per observation and calls .upsert(rows) on edifice_notes, which Postgres compiles to INSERT … ON CONFLICT DO UPDATE. The INSERT arm must satisfy every NOT NULL column, including project_id, which the partial payload never carries — so every push fails.

---

Root Cause Analysis

WHY: /edifice push fails on every observation edit
↓ BECAUSE: push_mission_context batches partial rows into upsert(), compiled as
  INSERT … ON CONFLICT DO UPDATE
  Evidence: supabase/functions/hal-mcp/index.ts:441

↓ BECAUSE: the INSERT arm must satisfy every NOT NULL column, but the row built at
  :426-437 only ever contains `id` + caller-supplied fields — never project_id
  Evidence: supabase/functions/hal-mcp/index.ts:426

↓ ROOT CAUSE: upsert is the wrong primitive for a "partial update of an existing row"
  contract; a targeted UPDATE keyed by note_id never needs project_id in its payload
  Evidence: supabase/functions/hal-mcp/index.ts:440-444

Introduced by 8ac2466b (PR #25, 2026-06-30), which moved from per-row logic to a single batched upsert to save round trips — the batching optimization broke correctness.

---

Implementation Plan

Step	File	Change
1	supabase/functions/hal-mcp/index.ts:423-444	Replace the row-collecting loop + single batch upsert with a per-observation targeted .update(fields).eq("id", obs.note_id).select("id"); count updated from returned rows; route unmatched note_ids into errors.
2	supabase/functions/hal-mcp/index.test.ts:746-840	Rewrite the 4 existing note-path tests to mock the .update().eq().select() chain instead of bare .upsert; add a new test for the unknown-note_id case.

📋 Detailed implementation steps

Required change (index.ts:423-444):

for (const obs of observations) {
  if (!obs.note_id) { skipped++; continue; }
  const fields: Record<string, unknown> = {};
  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) 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++;
  }
}

• .update(fields) never carries id/project_id → compiles to a pure UPDATE, never touches NOT NULL columns the caller didn't supply.
• .select("id") returns rows actually modified via RETURNING — fixes updated being fabricated from rows.length.
• Zero-match UPDATE returns data: [] with no error → routed into errors instead of silently dropped or (under old upsert) turned into a phantom insert.
• Idempotent: re-running the same push re-issues the same UPDATE … SET x = <same value>, same updated count, no state change on the second run.

No DDL, no change to edifice_notes schema/constraints — the whole point of the fix is that a targeted UPDATE never needs project_id in its payload.

---

Validation

cd supabase/functions/hal-mcp && deno check index.ts && deno test --allow-env --allow-net index.test.ts
make test

Manual: push existing note_id (updated:1, unchanged columns preserved) → push twice (idempotent, no-op) → push unknown note_id (updated:0, isError, no row created) → push mixed valid+unknown (partial success reported accurately).

---

Next Step

To implement: /implement-issue 74

---

Investigated by Claude • 2026-07-25

[BluegReeno]

✅ Issue Resolution Report

PR: #76 (#76)
Status: COMPLETE

---

Summary

push_mission_context batched partial note rows into .upsert(), which Postgres compiles to INSERT ... ON CONFLICT DO UPDATE — the INSERT arm required every NOT NULL column (including project_id), so every push failed. Fixed by replacing the batched upsert with a per-observation targeted UPDATE ... .eq("id", note_id).select("id") in push_mission_context, exactly per the issue's "Fix direction" section. Both adjacent defects are covered: updated now counts rows Postgres actually returned via RETURNING, and an unknown note_id is routed into errors instead of silently creating an orphan row. No schema change, no DDL migration — the NOT NULL constraint on edifice_notes.project_id was never touched. Idempotency is preserved by construction: a targeted UPDATE re-run with identical values re-matches the same row with no side effect.

---

Changes Made

File	Change
supabase/functions/hal-mcp/index.ts	Per-observation .update(fields).eq("id", note_id).select("id") replaces the batched .upsert(rows); updated counts actually-affected rows; unmatched note_id → errors, never inserted.
supabase/functions/hal-mcp/index.test.ts	Rewrote 5 note-path tests for the new mock chain shape; added tests for unknown note_id, data === null not-found, and mixed-outcome batches; eq() call args now captured and asserted for correct row-targeting.

---

Validation

✅ deno check (both edge functions) | ✅ deno lint (net-zero new findings) | ✅ deno test (63 passed) | ✅ make test (157 passed, 17 deselected)

---

Review & Self-Fix

• 5 review findings (0 CRITICAL, 1 HIGH, 1 MEDIUM, 3 LOW) — all three independent review agents (code-review, error-handling, test-coverage) reached APPROVE, zero production-code bugs
• 3 tests added/extended to fix HIGH/MEDIUM/LOW findings (mixed-batch isolation, eq() row-targeting assertion, data === null branch)
• 0 docs updated (no schema/contract change)
• 0 code simplifications needed (implementation was already minimal per the issue's own fix snippet)

---

Unaddressed Items

Finding	Severity	Reason
noteUpdateChain "duplicates" makeUpdateMock	LOW	Reviewer's own verdict: EXTENDS, not DUPLICATE — no action needed, flagged for visibility only
Idempotency (issue AC #4) has no unit test	LOW	Property follows from Postgres UPDATE semantics, not app logic; true verification belongs at the integration/migration-smoke-test layer, not unit tests

---

Suggested Follow-up Issues

No follow-up issues needed — the review's one bundled test-hardening suggestion was fully absorbed into this same PR.

---

Resolved by Archon workflow e875e7c33d74a11daca5d9ac5fa1555f