fix: bind Today progress retries to evidence (Closes #1056)

frontend/authored-outbox.js

@@ -55,6 +55,20 @@ function createAuthoredOutbox({ storage, fetchJson, coordinator, backgroundSync,
     });
   }
 
+  function operationFingerprint(message) {
+    return JSON.stringify({
+      kind:String(message.kind || ''), repository:String(message.repository || ''),
+      number:Number(message.number || 0), notificationId:Number(message.notificationId || 0),
+      body:String(message.body || ''), targetKind:String(message.targetKind || ''),
+      decision:String(message.decision || 'comment'), expectedHeadSha:String(message.expectedHeadSha || ''),
+      comments:Array.isArray(message.comments) ? message.comments : [],
+      blockerRepository:String(message.blockerRepository || ''), blockerNumber:Number(message.blockerNumber || 0),
+      present:message.present === true, title:String(message.title || ''),
+      expectedUpdatedAt:String(message.expectedUpdatedAt || ''),
+      attachments:messageAttachments(message).map(attachmentMetadata),
+    });
+  }
+
   function clearConfirmedReviewState(item) {
     if (item.kind !== 'pull-review') return;
     try {
@@ -93,7 +107,12 @@ function createAuthoredOutbox({ storage, fetchJson, coordinator, backgroundSync,
     const items = read();
     const requestedOperationId = String(message.operationId || '').slice(0, 128);
     const existing = requestedOperationId && items.find(item => item.operationId === requestedOperationId);
-    if (existing) return { ...existing };
+    if (existing) {
+      if (operationFingerprint(existing) !== operationFingerprint(message)) {
+        throw new Error('This operation ID is already bound to a different queued action.');
+      }
+      return { ...existing };
+    }
     if (message.kind === 'pull-review') {
       const queuedReview = items.find(item => item.kind === 'pull-review' &&
         item.repository === String(message.repository || '') &&

frontend/today-progress.js

@@ -5,6 +5,13 @@ maxLength = 2000, maxItems = 20 }) {
   const login = () => String(getLogin() || '').trim().toLowerCase();
   const storageKey = () => login() ? prefix + encodeURIComponent(login()) : '';
   const validIdentity = identity => typeof identity === 'string' && identity.length > 0 && identity.length <= 500;
+  const attachmentFingerprint = attachments => JSON.stringify((Array.isArray(attachments) ? attachments : []).filter(Boolean).slice(0, 5).map(value => ({
+    filename:String(value.filename || ''), contentType:String(value.contentType || ''),
+    note:String(value.note || '').replace(/\s+/g, ' ').trim().slice(0, 240),
+    operationId:String(value.operationId || '').slice(0, 128),
+    markdown:String(value.confirmed?.markdown || ''),
+  })));
+  const payloadFingerprint = (body, attachments) => JSON.stringify({body, attachments:attachmentFingerprint(attachments)});
   const validRecord = record => record && typeof record.body === 'string' && (record.body.length > 0 || record.has_attachments === true) &&
     record.body.length <= maxLength && typeof record.operation_id === 'string' && record.operation_id.length > 0 &&
     record.operation_id.length <= 128;
@@ -42,9 +49,10 @@ maxLength = 2000, maxItems = 20 }) {
     return read()[identity]?.body || '';
   }
 
-  function save(identity, value, hasAttachments = false) {
+  function save(identity, value, attachments = []) {
     if (!validIdentity(identity) || !storageKey()) return false;
     const body = String(value || '').trim();
+    const hasAttachments = attachments === true || (Array.isArray(attachments) && attachments.filter(Boolean).length > 0);
     if (body.length > maxLength) return false;
     const drafts = read();
     if (!body && !hasAttachments) {
@@ -53,9 +61,13 @@ maxLength = 2000, maxItems = 20 }) {
     }
     if (!drafts[identity] && Object.keys(drafts).length >= maxItems) return false;
     const previous = drafts[identity];
+    const fingerprint = payloadFingerprint(body, Array.isArray(attachments) ? attachments : []);
+    const previousFingerprint = previous?.payload_fingerprint ||
+      (previous && !previous.has_attachments ? payloadFingerprint(previous.body, []) : '');
     drafts[identity] = {
       body,
-      operation_id: previous?.body === body ? previous.operation_id : String(makeId()).slice(0, 128),
+      operation_id: previousFingerprint === fingerprint ? previous.operation_id : String(makeId()).slice(0, 128),
+      payload_fingerprint:fingerprint,
       ...(hasAttachments ? {has_attachments:true} : {}),
     };
     return write(drafts);
@@ -69,8 +81,24 @@ maxLength = 2000, maxItems = 20 }) {
 
   async function post(target, value, attachments = [], completeEvidence) {
     if (!validTarget(target)) throw new Error('An active Today issue or pull request is required.');
+    const pendingRecord = read()[target.identity];
+    if (pendingRecord?.cleanup_pending === true) {
+      try {
+        if (typeof completeEvidence === 'function') await completeEvidence();
+      } catch (_error) {
+        const error = new Error('Progress update is already queued; photo cleanup is pending.');
+        error.deliveryAdmitted = true;
+        throw error;
+      }
+      const pendingDrafts = read();
+      if (pendingDrafts[target.identity]?.operation_id === pendingRecord.operation_id) {
+        delete pendingDrafts[target.identity];
+        write(pendingDrafts);
+      }
+      return { background:true, alreadyAdmitted:true, cleanupRecovered:true };
+    }
     const evidence = (Array.isArray(attachments) ? attachments : [attachments]).filter(Boolean).slice(0, 5);
-    if ((value !== undefined || evidence.length) && !save(target.identity, value ?? load(target.identity), evidence.length > 0)) {
+    if ((value !== undefined || evidence.length) && !save(target.identity, value ?? load(target.identity), evidence)) {
       throw new Error('Progress update could not be saved on this device.');
     }
     const record = read()[target.identity];
@@ -81,7 +109,18 @@ maxLength = 2000, maxItems = 20 }) {
       body:record.body, operationId:record.operation_id,
       ...(evidence.length ? {attachments:evidence} : {}),
     });
-    if (typeof completeEvidence === 'function') await completeEvidence();
+    const admittedDrafts = read();
+    if (admittedDrafts[target.identity]?.operation_id === record.operation_id) {
+      admittedDrafts[target.identity] = { ...admittedDrafts[target.identity], cleanup_pending:true };
+      write(admittedDrafts);
+    }
+    try {
+      if (typeof completeEvidence === 'function') await completeEvidence();
+    } catch (_error) {
+      const error = new Error('Progress update is already queued; photo cleanup is pending.');
+      error.deliveryAdmitted = true;
+      throw error;
+    }
     const drafts = read();
     if (drafts[target.identity]?.operation_id === record.operation_id) {
       delete drafts[target.identity];
@@ -103,12 +142,16 @@ function createTodayProgressView({ progress, currentTarget, qs, photos, announce
   const update = () => { launcher.hidden = !currentTarget(); };
   const checkpoint = async () => {
     if (!openedTarget) return false;
-    if (progress.save(openedTarget.identity, body.value, photos?.has?.())) {
+    try {
-      try { await photos?.checkpoint?.(); return true; }
+      await photos?.checkpoint?.();
-      catch (error) { status.textContent = error.message + ' Your photos remain here; retry.'; return false; }
+      const checkpointAttachments = await photos?.serialize?.() || [];
+      if (progress.save(openedTarget.identity, body.value, checkpointAttachments)) return true;
+      status.textContent = 'Update must be 2,000 characters or fewer and device storage must be available.';
+      return false;
+    } catch (error) {
+      status.textContent = error.message + ' Your photos remain here; retry.';
+      return false;
     }
-    status.textContent = 'Update must be 2,000 characters or fewer and device storage must be available.';
-    return false;
   };
   const close = () => {
     if (sheet.open) sheet.close();
@@ -158,7 +201,9 @@ function createTodayProgressView({ progress, currentTarget, qs, photos, announce
         'Progress update saved for next launch. Today is still on the same item.');
       close();
     } catch (error) {
-      status.textContent = error.message + ' Your update remains on this item; retry.';
+      status.textContent = error.deliveryAdmitted ?
+        error.message + ' Retry to finish local cleanup; delivery will not be queued again.' :
+        error.message + ' Your update remains on this item; retry.';
       body.focus();
     } finally {
       button.disabled = false;

tests/test_authored_outbox.py

@@ -15,6 +15,44 @@ def run_node(script: str):
     return json.loads(result.stdout)
 
 
+def test_repeated_operation_id_rejects_a_different_comment_payload():
+    script = f"""
+const createAuthoredOutbox = require({json.dumps(str(OUTBOX))});
+const values = new Map();
+const storage = {{getItem:key=>values.get(key)||null,setItem:(key,value)=>values.set(key,value)}};
+const outbox=createAuthoredOutbox({{storage,getOwnerLogin:()=> 'timmy'}});
+const original={{kind:'issue-comment',repository:'o/r',number:7,body:'Done',operationId:'stable',attachments:[{{filename:'before.jpg',contentType:'image/jpeg',note:'Before',operationId:'photo-1'}}]}};
+const identical=outbox.enqueue(original);
+let error='';
+try {{ outbox.enqueue({{...original,attachments:[{{filename:'after.jpg',contentType:'image/jpeg',note:'After',operationId:'photo-2'}}]}}); }}
+catch (caught) {{ error=caught.message; }}
+process.stdout.write(JSON.stringify({{identical,error,items:outbox.list()}}));
+"""
+    output = run_node(script)
+    assert output["identical"]["operationId"] == "stable"
+    assert output["error"] == "This operation ID is already bound to a different queued action."
+    assert len(output["items"]) == 1
+    assert output["items"][0]["attachment"]["operationId"] == "photo-1"
+
+
+def test_repeated_pull_review_operation_uses_the_default_comment_decision():
+    script = f"""
+const createAuthoredOutbox = require({json.dumps(str(OUTBOX))});
+const values = new Map();
+const storage = {{getItem:key=>values.get(key)||null,setItem:(key,value)=>values.set(key,value)}};
+const outbox=createAuthoredOutbox({{storage,getOwnerLogin:()=> 'timmy'}});
+const review={{kind:'pull-review',repository:'o/r',number:8,body:'Notes',expectedHeadSha:'abc',operationId:'review-op'}};
+outbox.enqueue(review);
+let error='';
+try {{ outbox.enqueue(review); }} catch (caught) {{ error=caught.message; }}
+process.stdout.write(JSON.stringify({{error,items:outbox.list()}}));
+"""
+    output = run_node(script)
+    assert output["error"] == ""
+    assert len(output["items"]) == 1
+    assert output["items"][0]["decision"] == "comment"
+
+
 def test_authored_outbox_quarantines_legacy_and_cross_account_messages():
     script = f"""
 const createAuthoredOutbox = require({json.dumps(str(OUTBOX))});

tests/test_today_progress.py

@@ -123,25 +123,59 @@ process.stdout.write(JSON.stringify({{error,retained,after:progress.load(target.
     }
 
 
-def test_photo_cleanup_failure_retries_with_the_same_comment_operation():
+def test_photo_payload_changes_rotate_the_progress_operation_identity():
+    script = f"""
+const createProgress = require({json.dumps(str(TODAY_PROGRESS))});
+const values = new Map(); const ids = ['progress-1', 'progress-2', 'progress-3'];
+const storage = {{getItem:key=>values.get(key)||null,setItem:(key,value)=>values.set(key,value),removeItem:key=>values.delete(key)}};
+const progress=createProgress({{storage,getLogin:()=> 'timmy',makeId:()=>ids.shift(),admit:async()=>({{}})}});
+const identity='issue:stackchain/dashboard:12:';
+const before={{filename:'proof.jpg',contentType:'image/jpeg',note:'Before',operationId:'photo-1'}};
+const replacement={{filename:'proof.jpg',contentType:'image/jpeg',note:'After',operationId:'photo-2'}};
+progress.save(identity, 'Repair complete', [before]);
+const first=JSON.parse(values.values().next().value).drafts[identity].operation_id;
+progress.save(identity, 'Repair complete', [before]);
+const unchanged=JSON.parse(values.values().next().value).drafts[identity].operation_id;
+progress.save(identity, 'Repair complete', [replacement]);
+const replaced=JSON.parse(values.values().next().value).drafts[identity].operation_id;
+replacement.note='Redacted replacement';
+progress.save(identity, 'Repair complete', [replacement]);
+const recaptioned=JSON.parse(values.values().next().value).drafts[identity].operation_id;
+process.stdout.write(JSON.stringify({{first,unchanged,replaced,recaptioned}}));
+"""
+    output = run_node(script)
+    assert output == {
+        "first": "progress-1",
+        "unchanged": "progress-1",
+        "replaced": "progress-2",
+        "recaptioned": "progress-3",
+    }
+
+
+def test_photo_cleanup_failure_resumes_cleanup_without_readmitting_delivery():
     script = f"""
 const createProgress = require({json.dumps(str(TODAY_PROGRESS))});
 const values=new Map();
 const storage={{getItem:key=>values.get(key)||null,setItem:(key,value)=>values.set(key,value),removeItem:key=>values.delete(key)}};
-const calls=[]; let cleanupFails=true;
+const calls=[]; let cleanupFails=true; let cleanupCalls=0;
-const progress=createProgress({{storage,getLogin:()=> 'timmy',makeId:()=> 'stable-progress-op',admit:async message=>{{calls.push(message);return {{background:true}};}}}});
+const options={{storage,getLogin:()=> 'timmy',makeId:()=> 'stable-progress-op',admit:async message=>{{calls.push(message);return {{background:true}};}}}};
+let progress=createProgress(options);
 const target={{identity:'pull:stackchain/dashboard:22:',kind:'pull',repository:'stackchain/dashboard',number:22}};
 const photo={{filename:'proof.webp',contentType:'image/webp',blob:{{size:9}},operationId:'photo-op'}};
-const cleanup=async()=>{{if(cleanupFails)throw new Error('draft cleanup blocked')}};
+const cleanup=async()=>{{cleanupCalls++;if(cleanupFails)throw new Error('draft cleanup blocked')}};
-let error='';
+let error=''; let deliveryAdmitted=false;
-try {{await progress.post(target,'',[photo],cleanup);}} catch(caught){{error=caught.message;}}
+try {{await progress.post(target,'',[photo],cleanup);}} catch(caught){{error=caught.message;deliveryAdmitted=caught.deliveryAdmitted===true;}}
+progress=createProgress(options);
 cleanupFails=false;
-await progress.post(target,undefined,[photo],cleanup);
+const recovered=await progress.post(target,'edited text must not be admitted',[{{...photo,operationId:'replacement'}}],cleanup);
-process.stdout.write(JSON.stringify({{error,calls,after:progress.load(target.identity)}}));
+process.stdout.write(JSON.stringify({{error,deliveryAdmitted,calls,cleanupCalls,recovered,after:progress.load(target.identity)}}));
 """
     output = run_node("(async()=>{" + script + "})().catch(error=>{console.error(error);process.exit(1)})")
-    assert output["error"] == "draft cleanup blocked"
+    assert output["error"] == "Progress update is already queued; photo cleanup is pending."
-    assert [call["operationId"] for call in output["calls"]] == ["stable-progress-op", "stable-progress-op"]
+    assert output["deliveryAdmitted"] is True
+    assert len(output["calls"]) == 1
+    assert output["cleanupCalls"] == 2
+    assert output["recovered"]["alreadyAdmitted"] is True
     assert output["after"] == ""
 
 
@@ -192,6 +226,9 @@ def test_mobile_progress_sheet_is_accessible_bundled_and_safe_area_aware():
     assert "lanes:{ today:" in TODAY_PROGRESS.read_text()
     assert "photos:todayProgressPhotos" in dashboard
     assert "const controller = issueAttachment.mount" in TODAY_PROGRESS.read_text()
+    assert "const checkpointAttachments = await photos?.serialize?.() || [];" in TODAY_PROGRESS.read_text()
+    assert "progress.save(openedTarget.identity, body.value, checkpointAttachments)" in TODAY_PROGRESS.read_text()
+    assert "error.deliveryAdmitted" in TODAY_PROGRESS.read_text()
     assert ".today-progress-panel" in css
     assert "env(safe-area-inset-bottom)" in css
     assert ".today-progress-actions button" in css and "min-height:44px" in css