fix(hal-mcp): push_mission_context uses targeted UPDATE, not upsert

[BluegReeno]

Summary

• push_mission_context used a batched upsert to write field-note edits back to edifice_notes. Every push failed with null value in column "project_id" of relation "edifice_notes" violates not-null constraint, because the upsert payload only carried the edited fields, not the full row — an INSERT fallback (upsert's semantics on no match) tripped the NOT NULL on project_id.
• Replaces the batched upsert with a per-observation, targeted UPDATE ... .eq("id", note_id).select("id"). A targeted update never inserts, so it never needs project_id (or any other NOT NULL column) in its payload — the schema was already correct, only the write path was wrong.
• Fixes the two adjacent defects called out in the issue's Fix direction:
  • updated is now counted from the rows the UPDATE actually returned/affected, not assumed from the input length.
  • An unknown note_id (no row matched) is now routed into errors, never silently inserted as a new row.
• No DDL. No migration file. No change to any NOT NULL constraint on edifice_notes — per the hard constraint, the schema is correct and stays untouched.
• Idempotent: re-pushing the same observations twice re-issues identical UPDATEs and reports the same updated count with no state change.

Changes

• supabase/functions/hal-mcp/index.ts (+14/-13): swap the batched upsert call for a per-note targeted UPDATE ... .eq("id", note_id).select("id"); derive updated from returned rows; route unmatched note_ids into errors.
• supabase/functions/hal-mcp/index.test.ts (+77/-29): rewrite the four existing note-path unit tests to mock the new .update().eq().select() chain; add a new "unknown note_id → error, not insert" test; also fix a fifth pre-existing test (push_mission_context — isError when obs succeed but crop fails, not enumerated in the original investigation) that mocked the old upsert shape and would otherwise fail red against the new handler.

Validation

• make test (deno check on both edge functions + offline pytest): ✅ 157 passed, 17 deselected.
• deno test index.test.ts (hal-mcp): ✅ 61 passed, 0 failed — including the 5 rewritten push_mission_context note-path tests and the new unknown-note_id case.
• deno lint on touched files: net-zero new findings (4 pre-existing findings, identical before/after; not part of the CI gate, which runs deno check only).
• Hard constraint verified: no migration file added, no NOT NULL constraint on edifice_notes touched — a targeted UPDATE never needs project_id in its payload.
• Idempotency verified by design: re-pushing identical observations re-issues identical UPDATEs with no row-count or state drift.

Fixes #74

[BluegReeno]

🔍 Comprehensive PR Review

PR: #76
Reviewed by: 3 agents (code-review, error-handling, test-coverage) — comment-quality and docs-impact artifacts were not produced for this run (see note below)
Date: 2026-07-25

---

Summary

This PR replaces the batched .upsert() in push_mission_context's note-write loop with a per-observation targeted .update(fields).eq("id", note_id).select("id"), exactly as scoped by issue #74. All three agents independently reach APPROVE: all four acceptance criteria are met (untouched columns preserved, updated derived from actually-affected rows, unknown note_id routed to errors not inserted, idempotent re-push), no migration files are touched, no workaround/silent-fallback pattern was introduced, and CI is green.

The one gap all three converge on, from different angles: no test currently proves that one observation's failure doesn't affect a sibling's success in the same batch call — a coverage gap on currently-correct code, not a defect.

Verdict: APPROVE

Severity	Count
🔴 CRITICAL	0
🟠 HIGH	1
🟡 MEDIUM	1
🟢 LOW	3

---

🟠 High Issues (test-coverage hardening, not blocking)

No test exercises a mixed batch (partial success + partial failure) in one call

📍 supabase/functions/hal-mcp/index.ts:434-441 / index.test.ts:758-882

The rewritten loop's cross-iteration aggregation (updated/errors across all observations in a call) is the actual new behavior this fix introduces — the old upsert failed the whole batch atomically, the new loop isolates failures per row. Every current test sends homogeneous outcomes (all-success, or a single failure alone); none combines success + not-found + error in one call. A future refactor (e.g. the parallelization the issue explicitly reserves as follow-up work) could silently break result isolation and every existing test would still pass.

View suggested test

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 s = stub(_adminClient, "from", returnsNext([
    noteUpdateChain({ data: [{ id: "n1" }], error: null }) as any,
    noteUpdateChain({ data: [], error: null }) 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" },
      ],
    }, 608));
    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);
  } finally {
    s.restore();
    Deno.env.set("SUPABASE_SECRET_KEYS", DEFAULT_SECRET_KEYS);
  }
});

---

🟡 Medium Issues (Needs Decision)

eq() argument never captured/asserted in the shared test mock

📍 supabase/functions/hal-mcp/index.test.ts:748-756 (noteUpdateChain)

The mock discards its eq(...) argument, so no test actually verifies the fix targets the correct edifice_notes row by note_id — a wrong-row-targeting regression would pass every current test.

Options: Fix now (record eq() args + assert, ~6 call sites) | Bundle into a follow-up issue with the HIGH item above | Skip (matches file-wide convention)

Recommendation: bundle with the HIGH finding into one follow-up test-hardening PR — cheap together, neither blocks this draft.

---

🟢 Low Issues

View 3 low-priority suggestions

Issue	Location	Suggestion
data === null (no error) untested for the not-found branch	index.ts:443-445	Add a { data: null, error: null } mock variant; fold into the same follow-up
noteUpdateChain looks like it duplicates makeUpdateMock but actually terminates the chain differently (array vs. single-row)	index.test.ts:749	No action — confirmed EXTENDS, not DUPLICATE
Idempotency (issue #74 AC #4) has no test at any level	index.test.ts (absent)	No unit-test action — follows from Postgres UPDATE semantics, not app logic; belongs at integration level if ever needed

---

✅ What's Good

• Root cause fixed, not papered over — old silent-success bug fully replaced, no try/except swallowing, no fabricated project_id.
• Fail-fast preserved — every error path lands in errors and flips isError: true.
• Per-row isolation is a genuine improvement over the old atomic-batch upsert.
• The incidentally-touched 5th test (isError when obs succeed but crop fails) was independently re-verified by two agents against its original assertions, not just trusted from the deviation note.
• Zero migration/DDL changes confirmed via git diff; all CI checks green.
• KISS/YAGNI respected — no premature parallelization, matching the issue's explicit scope limit.

---

📋 Suggested Follow-up Issues

Issue Title	Priority	Related Finding
"hal-mcp: add mixed-outcome + eq()-targeting test coverage for push_mission_context note loop"	P3	Bundles the HIGH + MEDIUM + one LOW finding above

---

Note on missing agents

comment-quality and docs-impact artifacts were not produced for this run. docs-impact is already marked N/A in scope.md (no DDL/contract change in this PR), and the diff introduces no non-obvious comments requiring quality review, so this gap is flagged for visibility but not expected to change the verdict.

---

Next Steps

1. No auto-fix applied — zero CRITICAL/HIGH production-code issues; the HIGH finding is a test addition, left for your call rather than auto-applied to a draft PR.
2. Optionally open the bundled follow-up issue before merging, or merge as-is and track separately — draft status + green CI mean neither finding is merge-blocking.

---

Reviewed by Archon comprehensive-pr-review workflow
Artifacts: /Users/renaud/.archon/workspaces/BluegReeno/hal/artifacts/runs/e875e7c33d74a11daca5d9ac5fa1555f/review/

[BluegReeno]

⚡ Self-Fix Report (Aggressive)

Status: COMPLETE
Pushed: ✅ Changes pushed to archon/task-fix-issue-74 (commit 8430535)
Philosophy: Fix everything unless clearly a new concern

---

Fixes Applied (3 total)

Severity	Count
🔴 CRITICAL	0
🟠 HIGH	1
🟡 MEDIUM	1
🟢 LOW	1

View all fixes

• ✅ HIGH — mixed-batch isolation untested (index.test.ts) — added push_mission_context — mixed batch: one succeeds, one not found, pinning the cross-iteration updated/errors aggregation this PR introduces.
• ✅ MEDIUM — eq() row-targeting unverifiable (index.test.ts:noteUpdateChain) — noteUpdateChain now records eq() calls on chain._eqCalls; asserted in the multi-observation success test and the new mixed-batch test that each row targets its correct note_id.
• ✅ LOW — data === null branch untested (index.test.ts) — added push_mission_context — null data (no error) is treated as not found.

---

Tests Added

• push_mission_context — mixed batch: one succeeds, one not found
• push_mission_context — null data (no error) is treated as not found
• (extended) push_mission_context — per-observation update succeeds now asserts eq() targeted the correct note_id per row

---

Skipped (2)

Finding	Reason
noteUpdateChain "duplicates" makeUpdateMock	Reviewer's own verdict: EXTENDS, not DUPLICATE (different terminal chain shape) — no action needed
Idempotency unit test (issue #74 AC #4)	Reviewer's own recommendation was no unit-test action — property follows from Postgres UPDATE semantics, not app logic; belongs at integration/migration-smoke-test layer if ever verified

---

Suggested Follow-up Issues

(none — the one bundled follow-up the consolidated review suggested was fully absorbed into this fix)

---

Validation

✅ Type check | ⚠️ Lint (4 pre-existing findings, none in changed lines; not part of CI) | ✅ Tests (63 passed)

---

Self-fix by Archon · aggressive mode · fixes pushed to archon/task-fix-issue-74