feat(hal-mcp): artifact-callable Edifice tools

[BluegReeno]

Summary

Makes hal-mcp callable from inside a Claude artifact (blocked downstream consumer: bluegreen-marketplace#50, Phases 2-3). Three changes, all in supabase/functions/hal-mcp/index.ts:

1. get_mission_context(mission_id) — returns the full mission context inline (flat header + observations[] + notes[] + photos[] metadata), same shape build_context.py produces, zero signed URLs. An artifact's CSP blocks all outbound fetch/<img> against Supabase Storage — connector calls are its only network path, so a signed URL is dead weight there.
2. get_mission_photo(photo_id, max_width?) — server-side resize + JPEG re-encode (@imagemagick/magick-wasm, the Deno-Edge-Function-compatible option since native sharp doesn't run there), returned as base64 image bytes inline (~1200px, q70, target ≈300KB, under the artifact's 16 MiB page ceiling).
3. readOnlyHint annotations — true on 12 read tools (list_edifice_missions, read_edifice_mission, get_mission_context, get_mission_photo, list_tasks, list_sprints, list_documents, get_document, list_projects, list_contacts, list_companies, whoami), explicit false on push_mission_context. Works around anthropics/claude-code#57398 (closed, not planned) — unannotated tools fire an Allow/Deny dialog per call with no "Always allow", so a mission-on-load artifact was previously unusable.

Also adds a test proving push_mission_context's partial-update path doesn't clobber sibling fields (behavior was already correct; this closes the acceptance criterion that asks for proof by test, not by reading).

Changes

• supabase/functions/hal-mcp/index.ts — extracted fetchMissionRaw() (shared raw-fetch helper, no behavior change to get_mission_with_assets), added get_mission_context + get_mission_photo, added annotations to 13 registerTool calls, ported buildHeader/buildObservationsAndNotes from build_context.py
• supabase/functions/hal-mcp/deno.json / deno.lock — added @imagemagick/magick-wasm@0.0.41 (wasm fetched from jsDelivr at runtime, memoized per isolate — no local asset, no config.toml change needed)
• supabase/functions/hal-mcp/index.test.ts — 5 new tests: get_mission_context shape/no-URL assertion, get_mission_photo error paths (unknown photo_id, storage download failure), tools/list readOnlyHint assertions, push_mission_context single-field no-clobber proof
• docs/prd.md — added the two new tools to the Edifice tool list
• docs/cross-repo-log.md, .claude/STATUS.md — cross-repo hygiene entries

Not in scope (per issue)

• Bumping plugins/hal/.mcp.json in bluegreen-marketplace — different repo, tracked via the docs/cross-repo-log.md entry as a downstream follow-up once this function is deployed.
• Returning updated rows from push_mission_context — issue phrases this as "Consider," not an acceptance criterion; current counts-only response is unchanged.
• Changing push_mission_context's update logic — confirmed already correct; only a proof test + annotation were added.
• Auto quality-reduction loop in get_mission_photo if a resized photo exceeds 400KB — fixed resize+encode call per the issue's stated target/ceiling.

Validation

• deno check --config=deno.json index.ts (both hal-mcp and edifice-map-generator) — pass, no errors
• deno test (hal-mcp): 68 passed / 0 failed, including the 5 new tests; all 9 pre-existing push_mission_context + get_mission_with_assets tests pass unmodified (confirms fetchMissionRaw() extraction is a pure refactor)
• make test (deno check ×2 Edge Functions + uv run pytest tests/ -q -m "not live"): 157 passed, 17 deselected
• uv run ruff check hal tests scripts: all checks passed

Deliberately unverified (flagged, not claimed)

Two acceptance criteria from the issue require a live deploy against shared prod (zgkvbjqlvebttbnkklpo) and were not exercised in this PR — deploy needs explicit user go-ahead per this repo's session rules:

• get_mission_context('3f2bd270-fd5f-4d09-a64e-063a66348dc8') returning the reference mission's 10 notes / 12 photos with zero URL fields — covered by a mocked-shape unit test only.
• get_mission_photo returning a real photo under 400 KB, decodable as JPEG — the resize/encode path itself is not unit-tested offline (it needs the ~14.6 MB wasm module, which would make make test network-dependent); only the tool's error paths (unknown photo_id, storage download failure) are unit-tested.
• tools/list showing readOnlyHint: true live on the deployed function — unit-verified via a mocked tools/list call; live re-check still pending.
• Deploying the Edge Function and bumping plugins/hal/.mcp.json downstream — deploy not run yet.

Fixes #82

[BluegReeno]

🔍 Comprehensive PR Review

PR: #83
Reviewed by: 5 specialized agents (code-review, error-handling, test-coverage, comment-quality, docs-impact)
Date: 2026-07-28

---

Summary

The Python→TypeScript port (buildHeader/buildObservationsAndNotes) is verified field-for-field accurate against build_context.py, the fetchMissionRaw() refactor is behavior-preserving (68/68 tests pass), and 13/13 readOnlyHint annotations on touched call sites are correct. But two real bugs and one contract regression bring 3 of 5 agents to REQUEST_CHANGES: get_mission_context silently swallows notes/photos/annotations fetch errors and can return what looks like a clean, disorder-free mission when the DB call actually failed. Separately, ensureMagick()'s wasm-init memoization permanently caches a rejected promise, turning one transient CDN blip into an isolate-lifetime outage of get_mission_photo. A readOnlyHint gap on two pre-existing tools went undetected because the new test hardcodes a name list instead of asserting the invariant structurally. docs/prd.md also now contradicts itself on tool count within the same file.

Verdict: REQUEST_CHANGES

Note: This PR is correctly a draft per explicit instruction — none of the below blocks anything today, but the two HIGH bugs (#1, #2) ship latent production failure modes the offline test suite can't exercise, and should be fixed before this leaves draft.

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

---

🟠 High Issues

1. get_mission_context silently drops notes/photos/annotations fetch errors

📍 supabase/functions/hal-mcp/index.ts:637-648, :298-374

fetchMissionRaw() already computes notesErr/photosErr/annotationsErr, but the handler never destructures them. If edifice_notes or edifice_photos fails transiently, get_mission_context returns a 200-equivalent success with observations: [] — indistinguishable from a genuinely empty mission. Its sibling get_mission_with_assets (same PR, same helper) correctly surfaces all three.

View fix

async ({ mission_id }, extra: unknown) => {
    const db = getDb(extra);
    const { project, projectErr, building, notes, notesErr, photos, photosErr, annotationsErr } =
      await fetchMissionRaw(db, mission_id);
    if (projectErr) return errorResult(`Project fetch failed: ${projectErr}`);
    if (!project) return errorResult("Project not found");
    if (notesErr) return errorResult(`Notes fetch failed: ${notesErr}`);
    if (photosErr) return errorResult(`Photos fetch failed: ${photosErr}`);
    if (annotationsErr) return errorResult(`Annotations fetch failed: ${annotationsErr}`);

    const header = buildHeader(project, building);
    const project_type = project.type || "diagnostic";
    const { observations, notes: freeNotes } = buildObservationsAndNotes(notes, photos, project_type);
    return okResult({ ...header, observations, notes: freeNotes, photos });
  }

Hard-fail rather than a has_errors side-channel — the tool's own contract is "same flat shape as context.json."

---

2. ensureMagick() permanently poisons on transient fetch/init failure

📍 supabase/functions/hal-mcp/index.ts:138-148
(flagged independently by both code-review and error-handling agents)

magickInit is assigned the promise before it settles. A rejected promise is still truthy, so if (!magickInit) never re-fires the fetch — one transient CDN blip permanently breaks get_mission_photo for the rest of that isolate's life.

View fix

function ensureMagick(): Promise<void> {
  if (!magickInit) {
    magickInit = (async () => {
      const res = await fetch(MAGICK_WASM_URL);
      if (!res.ok) throw new Error(`magick.wasm fetch failed: ${res.status}`);
      await initializeImageMagick(new Uint8Array(await res.arrayBuffer()));
    })().catch((err) => {
      magickInit = null; // don't pin a transient CDN failure for the isolate's lifetime
      throw err;
    });
  }
  return magickInit;
}

---

3. readOnlyHint sweep skips list_stages/get_document_file — and the test can't structurally catch it

📍 index.ts:804-830, :1867-1907; test at index.test.ts:227-251

Two pure-read tools sitting next to ones that got the annotation (list_projects, get_document) were left out. The tools/list test only asserts a hardcoded 12-name array, so it can't catch this or any future omission.

View fix

Code: add annotations: { readOnlyHint: true } to both registerTool calls.

Test — replace the hardcoded list with a structural invariant:

Deno.test("tools/list — every list_*/get_* tool is readOnlyHint:true except documented write side effects", async () => {
  const WRITE_SIDE_EFFECT_EXCEPTIONS = new Set(["get_mission_with_assets"]);
  for (const t of tools) {
    const looksReadOnly = /^(list_|get_)/.test(t.name) && !WRITE_SIDE_EFFECT_EXCEPTIONS.has(t.name);
    if (looksReadOnly) {
      assertEquals(t.annotations?.readOnlyHint, true, `${t.name} looks read-only but is missing readOnlyHint:true`);
    }
  }
});

---

4. buildHeader's suivi_chantier/devis branches (2 of 3 project types) have zero test coverage

📍 index.ts:484-530

Described as a field-for-field port from build_context.py with no live verification planned, a transcription bug in either untested branch ships straight to production. devis's default chiffrage array (3-row structured default) is entirely unverified.

View fix

Add 2 tests cloning the existing get_mission_context test's mock scaffolding — one with project.type = "suivi_chantier", one with "devis", asserting the type-specific fields and the chiffrage default. Full test bodies in test-coverage-findings.md Finding 2.

---

5. docs/prd.md contradicts itself on tool count

📍 docs/prd.md:215 (unchanged) vs. :51 (edited by this PR)

§3.2 now says "30 tools" but §6.1, ~90 lines later in the same file, still says "27 tools" — reads as an authoring error, not staleness.

View fix

- `hal-mcp` v0.2.1 — 30 tools, OAuth + API-key auth, RLS isolation. `whoami` +
  `workspace_members.is_default` for server-side default-workspace resolution.
- **Artifact-callable Edifice tools (#82)** — `get_mission_context` (inline context.json
  shape, zero signed URLs) + `get_mission_photo` (resized base64 JPEG) for consumers that
  can't follow a URL (Claude artifacts). `readOnlyHint: true`/`false` annotations on all
  13 relevant tools unblock artifact loading without an Allow/Deny dialog cascade.

---

🟡 Medium Issues (Needs Decision)

get_mission_context's header test never exercises the "building present" path

📍 index.ts:490,508,513,536-537

residence/adresse/type_batiment/description_batiment and cleanAddress()'s regex are only asserted via their empty-fallback path. Recommendation: fix now — one mock addition to an existing test.

OBSERVATION_TYPE comment implies full per-service_type coverage, but devis is silently unmapped

📍 index.ts:469

devis missions always put every note into notes[], never observations[] — the comment reads as if the mapping is exhaustive. Recommendation: fix now — reword to state the mechanism, not enumerate types (survives future project_type additions).

README.md tool count and Edifice tool list not updated

📍 README.md:32,113-115,142-145

Still says "27 MCP tools", omits get_mission_context/get_mission_photo from the Edifice bullet list — the exact capability issue #82 unblocks. Recommendation: fix now — same sentence already written in prd.md §3.2 to copy from.

---

🟢 Low Issues

View 5 low-priority suggestions

Issue	Location	Suggestion
get_mission_context description overclaims context.json parity (omits building 2D-map block, likely intentional)	index.ts:744-765	Tighten docstring to name fields explicitly
max_width accepts non-positive values, surfaces raw wasm exception	index.ts:657	z.number().int().positive().optional()
get_mission_photo's "no storage_path" branch untested	index.ts:787	Clone the "unknown photo_id" test pattern
"ported field-for-field" comment is an unverifiable cross-repo claim (one visible zone-field asymmetry, may be intentional)	index.ts:377	Add pointer to docs/cross-repo-log.md sync note
docs/architecture.md mermaid diagram already stale pre-PR, widened by 2 more tools	docs/architecture.md:32-38	Pre-existing drift — flag as standalone doc-hygiene follow-up, out of this PR's scope

---

✅ What's Good

• buildHeader/buildObservationsAndNotes verified field-for-field accurate against build_context.py.
• fetchMissionRaw() extraction is behavior-preserving — full 68-test suite passes, including 9 pre-existing tests unmodified.
• All 13 touched readOnlyHint call sites carry the correct value.
• push_mission_context no-clobber test genuinely proves the claim by capturing the actual .update() payload.
• Zero-signed-URL guarantee in get_mission_context is enforced by a real runtime assertion, not just a shape check.
• Cross-repo hygiene (docs/cross-repo-log.md, .claude/STATUS.md) is complete and accurate.
• New inline comments consistently explain why, matching this repo's comment bar.

---

📋 Suggested Follow-up Issues

Issue Title	Priority	Related Finding
Re-sync docs/architecture.md tool count/list with prd.md (pre-existing drift)	P3	LOW issue — architecture.md

---

Next Steps

1. Fix the 5 HIGH issues before this PR leaves draft — all have concrete, low-effort patches above.
2. Review the 3 MEDIUM issues (all low-effort, single-file changes).
3. Consider the 5 LOW issues for this PR or a follow-up pass.
4. Confirm the Migration smoke-test CI check (in-progress at scope time) has resolved.

---

Reviewed by 5-agent comprehensive PR review
Full artifact: artifacts/runs/dfefba2e640229da358a4c1b1fbd91dc/review/consolidated-review.md

[BluegReeno]

⚡ Self-Fix Report (Aggressive)

Status: COMPLETE
Pushed: ✅ Changes pushed to archon/task-feat-artifact-callable-edifice-tools
Philosophy: Fix everything unless clearly a new concern

---

Fixes Applied (12 total)

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

View all fixes

• ✅ get_mission_context silently drops notes/photos/annotations fetch errors (index.ts:642-649) — now hard-fails via errorResult on any of the three error fields instead of returning what looks like a clean, empty mission
• ✅ ensureMagick() permanently poisons on transient fetch/init failure (index.ts:139-150) — resets the memoized promise to null on rejection so the next call retries
• ✅ readOnlyHint sweep skips list_stages/get_document_file (index.ts:823, :1884) — added the annotation to both, closing the PR's own sweep gap
• ✅ buildHeader's suivi_chantier/devis branches untested — added 2 tests covering both branches (shape, "V-01" ref prefix, default chiffrage, unmapped-type routing)
• ✅ docs/prd.md self-contradicts on tool count (27 vs 30) — bumped §6.1 + added changelog bullet
• ✅ get_mission_context header test never exercises "building present" — new test with real building mock, proves cleanAddress() strips a parenthetical
• ✅ OBSERVATION_TYPE comment implies exhaustive coverage — restated as a general invariant (unmapped type → notes only)
• ✅ README.md tool count / Edifice list stale — bumped counts, added the 2 new tools, documented readOnlyHint
• ✅ max_width accepts non-positive values — .positive() added to the zod schema
• ✅ get_mission_photo's "no storage_path" branch untested — added test
• ✅ "ported field-for-field" comment is an unverifiable claim — added pointer to docs/cross-repo-log.md
• ✅ get_mission_context description overclaims context.json parity — tightened wording, notes the building 2D-map block is out of scope

---

Tests Added

• get_mission_context — building present: residence/adresse/type_batiment/description_batiment from edifice_buildings
• get_mission_context — suivi_chantier project_type shape + reservation observations
• get_mission_context — devis project_type shape, default chiffrage, no observation routing
• get_mission_photo — missing storage_path returns errorResult
• Rewrote tools/list readOnlyHint test as a structural invariant (every list_*/get_* tool) instead of a hardcoded 12-name list — will catch future omissions, not just the 2 found here

---

Skipped (1)

Finding	Reason
docs/architecture.md tool count/list stale	Pre-existing drift predating this PR by ≥1 prior increment; the reviewing agent itself recommended a dedicated follow-up doc-hygiene pass rather than a partial touch-up bundled into this PR

---

Suggested Follow-up Issues

1. Re-sync docs/architecture.md tool count/list with prd.md (P3) — pre-existing drift, unfrozen surface

---

Validation

✅ deno check (both functions) | ✅ Deno tests (72 passed: 68 pre-existing + 4 new) | ✅ make test (type-check + offline pytest, 157 passed)

Lint: N/A — no deno lint step configured in CI or Makefile; the handful of pre-existing lint findings predate this PR (confirmed via diff against base commit) and are out of scope.

---

Self-fix by Archon · aggressive mode · fixes pushed to archon/task-feat-artifact-callable-edifice-tools