fix(entry-explore-mcp-app): surface save errors + resolve pack id via provenance

MoltNet-Diary: 8a53c230-a665-4de1-aff7-42363fa33714
Task-Group: diary-map-app

Addresses PR #1229 review (#1229):
- onSaveZone no longer fails silently (surfaces saveError)
- resolvePackIdByCid uses packs_provenance (pagination-safe) not packs_list
- removed dead listZonePacks (YAGNI)
- RESTORE_STEP clears activeZoneId as its own invariant

Author: legreffier[bot]

apps/mcp-host-e2e/manual/diary-map-fixture.ts

@@ -160,6 +160,9 @@ async function main() {
   url.searchParams.set('autorun', '1');
   url.searchParams.set('server', `${harness.mcpBaseUrl}/mcp`);
   url.searchParams.set('clientId', agent.clientId);
+  // Throwaway fixture credentials for a local ephemeral stack ONLY. Putting a
+  // secret in URL params leaks it to browser history / logs / Referer headers —
+  // never do this with real agent credentials.
   url.searchParams.set('clientSecret', agent.clientSecret);
   url.searchParams.set(
     'args',

libs/entry-explore-mcp-app/src/MapApp.tsx

@@ -29,7 +29,7 @@ function pivotsForZone(
 ): Pivot[] {
   return zones
     .filter((zone) => zone.id !== activeZoneId)
-    .slice(0, 4)
+    .slice(0, 4) // V1: intentionally cap at 4 pivots to keep the row compact
     .map((zone) => ({ id: zone.id, label: `→ ${zone.label}` }));
 }
 
@@ -62,6 +62,7 @@ export function MapApp() {
   const [entries, setEntries] = useState<EntryCardEntry[]>([]);
   const [loadingEntries, setLoadingEntries] = useState(false);
   const [saving, setSaving] = useState(false);
+  const [saveError, setSaveError] = useState<string | null>(null);
 
   const adapter = useMemo(() => (app ? new McpDiaryAdapter(app) : null), [app]);
 
@@ -107,6 +108,7 @@ export function MapApp() {
     async (zone: typeof activeZone) => {
       if (!adapter || !map || !zone) return;
       setSaving(true);
+      setSaveError(null);
       try {
         if (!zone.packId) {
           const draft = await adapter.createZonePack({
@@ -115,6 +117,12 @@ export function MapApp() {
             entryIds: zone.entryIds,
             provenance: zone.provenance,
           });
+          // createZonePack resolves the UUID from the CID; if that came back
+          // empty, pinning later would no-op — surface it rather than silently
+          // marking the zone saved.
+          if (!draft.packId) {
+            throw new Error('Could not resolve the saved pack id.');
+          }
           dispatch({
             type: 'MATERIALIZE_ZONE',
             zoneId: zone.id,
@@ -124,6 +132,10 @@ export function MapApp() {
           await adapter.setZonePinned(zone.packId, true);
           dispatch({ type: 'PIN_ZONE', zoneId: zone.id, pinned: true });
         }
+      } catch (err) {
+        // Don't let the save fail silently (the button would just reset to
+        // "Save this zone" and re-clicking would retry blindly). Show why.
+        setSaveError(err instanceof Error ? err.message : 'Save failed.');
       } finally {
         setSaving(false);
       }
@@ -163,10 +175,7 @@ export function MapApp() {
       <Breadcrumb
         trail={map.trail}
         activeStep={map.activeStep}
-        onRestore={(index) => {
-          dispatch({ type: 'RESTORE_STEP', index });
-          dispatch({ type: 'SHOW_OVERVIEW' });
-        }}
+        onRestore={(index) => dispatch({ type: 'RESTORE_STEP', index })}
       />
 
       {activeZone ? (
@@ -202,6 +211,7 @@ export function MapApp() {
         zone={activeZone}
         onSaveZone={(zone) => void onSaveZone(zone)}
         saving={saving}
+        saveError={saveError}
       />
     </Shell>
   );

libs/entry-explore-mcp-app/src/adapter/mcp-adapter.test.ts

@@ -122,21 +122,16 @@ describe('McpDiaryAdapter.listTags', () => {
 });
 
 describe('McpDiaryAdapter.createZonePack', () => {
-  it('creates an UNPINNED pack with provenance, then resolves its id by CID', async () => {
-    // packs_create returns only packCid (no id); the adapter must look the id
-    // up via packs_list because packs_update (pin) needs the UUID.
+  it('creates an UNPINNED pack with provenance, then resolves its id from the CID', async () => {
+    // packs_create returns only packCid (no id); the adapter resolves the UUID
+    // via packs_provenance (metadata.rootPackId), not a paginated packs_list.
     const { app, calls } = caller((input) => {
       if (input.name === 'packs_create') {
         return Promise.resolve(textResult({ packCid: 'bafy-zone' }));
       }
-      if (input.name === 'packs_list') {
+      if (input.name === 'packs_provenance') {
         return Promise.resolve(
-          textResult({
-            items: [
-              { id: 'other', packCid: 'bafy-other' },
-              { id: 'pack-1', packCid: 'bafy-zone' },
-            ],
-          }),
+          textResult({ metadata: { rootPackId: 'pack-1' } }),
         );
       }
       return Promise.resolve(textResult({}));
@@ -170,14 +165,36 @@ describe('McpDiaryAdapter.createZonePack', () => {
       { entry_id: 'e2', rank: 1 },
       { entry_id: 'e1', rank: 2 },
     ]);
-    // Second call resolves the UUID from packs_list, matching on packCid.
-    expect(calls[1].name).toBe('packs_list');
+    // The UUID is resolved deterministically from the CID via provenance.
+    expect(calls[1].name).toBe('packs_provenance');
+    expect(calls[1].arguments).toMatchObject({ pack_cid: 'bafy-zone' });
     expect(draft).toEqual({
       packId: 'pack-1',
       packCid: 'bafy-zone',
       pinned: false,
     });
   });
+
+  it('returns an empty packId when packs_create yields no CID (no provenance call)', async () => {
+    const { app, calls } = caller((input) => {
+      if (input.name === 'packs_create') {
+        return Promise.resolve(textResult({}));
+      }
+      return Promise.resolve(textResult({}));
+    });
+    const adapter = new McpDiaryAdapter(app);
+
+    const draft = await adapter.createZonePack({
+      diaryId: 'd1',
+      label: 'X',
+      entryIds: ['e1'],
+      provenance: { basis: 'x', searches: [] },
+    });
+
+    expect(draft.packId).toBe('');
+    // No CID -> we never attempt provenance resolution.
+    expect(calls.some((c) => c.name === 'packs_provenance')).toBe(false);
+  });
 });
 
 describe('McpDiaryAdapter.setZonePinned', () => {
@@ -204,27 +221,3 @@ describe('McpDiaryAdapter.setZonePinned', () => {
     ).toBeGreaterThan(Date.now());
   });
 });
-
-describe('McpDiaryAdapter.listZonePacks', () => {
-  it('returns only diary-map-zone packs', async () => {
-    const { app } = caller(() =>
-      Promise.resolve(
-        textResult({
-          items: [
-            {
-              id: 'p1',
-              pinned: false,
-              params: { kind: 'diary-map-zone', label: 'A' },
-            },
-            { id: 'p2', pinned: true, params: { kind: 'compile' } },
-          ],
-        }),
-      ),
-    );
-    const adapter = new McpDiaryAdapter(app);
-
-    const out = await adapter.listZonePacks('d1');
-
-    expect(out).toEqual([{ packId: 'p1', label: 'A', pinned: false }]);
-  });
-});

libs/entry-explore-mcp-app/src/adapter/mcp-adapter.ts

@@ -6,8 +6,9 @@
  * existing deterministic server tools (`entries_list`, `entries_search`,
  * `diary_tags`). Zone materialization/validation wraps the existing packs tools
  * (`packs_create`, `packs_update`, `packs_list`) so a zone becomes an unpinned
- * draft pack carrying its search provenance in `params`, then a pinned pack once
- * the human validates it.
+ * draft pack carrying its search provenance in `params` (`packs_create`), then a
+ * pinned pack once the human validates it (`packs_update`). The pack UUID is
+ * resolved from the create CID via `packs_provenance`.
  *
  * Tool results arrive as `{ content: [{ type:'text', text }], structuredContent }`;
  * {@link parseToolJson} reads either shape (mirrors libs/task-mcp-app).
@@ -176,28 +177,27 @@ export class McpDiaryAdapter implements DiaryDataAdapter {
     const json = parseToolJson(result);
     const packCid = typeof json.packCid === 'string' ? json.packCid : '';
     // packs_create returns only packCid (CustomPackResult), not the pack UUID,
-    // but packs_update (pin) needs the UUID. Resolve it from packs_list by CID.
-    const packId = packCid
-      ? await this.resolvePackIdByCid(input.diaryId, packCid)
-      : '';
+    // but packs_update (pin) needs the UUID. Resolve it deterministically from
+    // the CID via packs_provenance (its metadata.rootPackId IS the UUID) —
+    // never via packs_list, which paginates (default limit 20) and could miss a
+    // freshly-created pack in a diary with many packs.
+    const packId = packCid ? await this.resolvePackIdByCid(packCid) : '';
     return { packId, packCid, pinned: false };
   }
 
-  /** Resolve a pack's UUID from its CID via packs_list (needed to pin later). */
-  private async resolvePackIdByCid(
-    diaryId: string,
-    packCid: string,
-  ): Promise<string> {
+  /**
+   * Resolve a pack's UUID from its CID via packs_provenance, whose
+   * `metadata.rootPackId` is the pack UUID. Deterministic and pagination-free
+   * (packs_update needs the UUID to pin). Returns '' if it can't be resolved.
+   */
+  private async resolvePackIdByCid(packCid: string): Promise<string> {
     const result = await this.app.callServerTool({
-      name: 'packs_list',
-      arguments: { diary_id: diaryId },
+      name: 'packs_provenance',
+      arguments: { pack_cid: packCid, depth: 0 },
     });
     const json = parseToolJson(result);
-    const items = Array.isArray(json.items) ? json.items : [];
-    const match = items
-      .map((item) => asRecord(item))
-      .find((item) => item.packCid === packCid);
-    return match && typeof match.id === 'string' ? match.id : '';
+    const metadata = asRecord(json.metadata);
+    return typeof metadata.rootPackId === 'string' ? metadata.rootPackId : '';
   }
 
   /** Pin (validate) or unpin a zone's draft pack. */
@@ -214,27 +214,4 @@ export class McpDiaryAdapter implements DiaryDataAdapter {
       }),
     });
   }
-
-  /** List the diary's draft zone packs (unpinned, kind=diary-map-zone). */
-  async listZonePacks(
-    diaryId: string,
-  ): Promise<Array<{ packId: string; label: string; pinned: boolean }>> {
-    const result = await this.app.callServerTool({
-      name: 'packs_list',
-      arguments: { diary_id: diaryId },
-    });
-    const json = parseToolJson(result);
-    const items = Array.isArray(json.items) ? json.items : [];
-    return items
-      .map((item) => asRecord(item))
-      .filter((item) => asRecord(item.params).kind === 'diary-map-zone')
-      .map((item) => ({
-        packId: typeof item.id === 'string' ? item.id : '',
-        label:
-          typeof asRecord(item.params).label === 'string'
-            ? (asRecord(item.params).label as string)
-            : 'Zone',
-        pinned: item.pinned === true,
-      }));
-  }
 }

libs/entry-explore-mcp-app/src/components/NextSteps.tsx

@@ -13,6 +13,8 @@ export interface NextStepsProps {
   zone: Zone | null;
   onSaveZone: (zone: Zone) => void;
   saving: boolean;
+  /** Surfaced when the last save attempt failed, so it isn't a silent no-op. */
+  saveError?: string | null;
 }
 
 /**
@@ -26,6 +28,7 @@ export function NextSteps({
   zone,
   onSaveZone,
   saving,
+  saveError,
 }: NextStepsProps) {
   const saved = Boolean(zone?.packId);
   return (
@@ -59,6 +62,11 @@ export function NextSteps({
           </button>
         ) : null}
       </div>
+      {saveError ? (
+        <p className="next-steps-error" role="alert">
+          {saveError}
+        </p>
+      ) : null}
     </div>
   );
 }

libs/entry-explore-mcp-app/src/state/map.test.ts

@@ -107,7 +107,7 @@ describe('mapReducer', () => {
     expect(activeTrailStep(next).why).toBe('narrowed to autonomy');
   });
 
-  it('RESTORE_STEP moves back with ZERO refetch and truncates the future', () => {
+  it('RESTORE_STEP moves back with ZERO refetch, truncates the future, and clears the active zone', () => {
     let next = mapReducer(map, {
       type: 'REFINE_SEARCH',
       label: 'autonomy',
@@ -122,12 +122,17 @@ describe('mapReducer', () => {
       query: 'identity',
       visibleEntryIds: ['e3'],
     })!;
+    // Focus a zone so we can assert RESTORE_STEP clears it on its own.
+    next = mapReducer(next, { type: 'FOCUS_ZONE', zoneId: 'a' })!;
     expect(next.trail).toHaveLength(3);
+    expect(next.activeZoneId).toBe('a');
 
     const restored = mapReducer(next, { type: 'RESTORE_STEP', index: 0 })!;
     // The cached visible set is reused verbatim — no new ids, no fetch needed.
     expect(restored.activeStep).toBe(0);
     expect(restored.trail).toHaveLength(1);
+    // Returning to a past step returns to its overview — zone cleared.
+    expect(restored.activeZoneId).toBeNull();
     expect(activeTrailStep(restored).visibleEntryIds).toEqual([
       'e1',
       'e2',

libs/entry-explore-mcp-app/src/state/map.ts

@@ -198,10 +198,14 @@ export function mapReducer(
 
     case 'RESTORE_STEP': {
       if (action.index < 0 || action.index >= state.trail.length) return state;
+      // Restoring a past camera position always returns to that step's
+      // overview, so clearing the focused zone is part of this action's own
+      // invariant (callers must not have to pair it with SHOW_OVERVIEW).
       return {
         ...state,
         trail: state.trail.slice(0, action.index + 1),
         activeStep: action.index,
+        activeZoneId: null,
       };
     }
 

libs/entry-explore-mcp-app/src/styles.css

@@ -285,6 +285,11 @@ body {
   opacity: 0.5;
   cursor: default;
 }
+.next-steps-error {
+  margin: 0;
+  font-size: 0.82rem;
+  color: var(--molt-error, #f04060);
+}
 
 /* ---- empty / fallback ---- */
 .fallback {