diff --git a/frontend/authored-outbox.js b/frontend/authored-outbox.js index e16347a..1685ce9 100644 --- a/frontend/authored-outbox.js +++ b/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 || '') && diff --git a/frontend/today-progress.js b/frontend/today-progress.js index 4d98a5d..04d53ee 100644 --- a/frontend/today-progress.js +++ b/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 { await photos?.checkpoint?.(); return true; } - catch (error) { status.textContent = error.message + ' Your photos remain here; retry.'; return false; } + try { + await photos?.checkpoint?.(); + 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; diff --git a/tests/test_authored_outbox.py b/tests/test_authored_outbox.py index 110b14f..58bc4d1 100644 --- a/tests/test_authored_outbox.py +++ b/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))}); diff --git a/tests/test_today_progress.py b/tests/test_today_progress.py index 39d1c80..80836e4 100644 --- a/tests/test_today_progress.py +++ b/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 progress=createProgress({{storage,getLogin:()=> 'timmy',makeId:()=> 'stable-progress-op',admit:async message=>{{calls.push(message);return {{background:true}};}}}}); +const calls=[]; let cleanupFails=true; let cleanupCalls=0; +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')}}; -let error=''; -try {{await progress.post(target,'',[photo],cleanup);}} catch(caught){{error=caught.message;}} +const cleanup=async()=>{{cleanupCalls++;if(cleanupFails)throw new Error('draft cleanup blocked')}}; +let error=''; let deliveryAdmitted=false; +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); -process.stdout.write(JSON.stringify({{error,calls,after:progress.load(target.identity)}})); +const recovered=await progress.post(target,'edited text must not be admitted',[{{...photo,operationId:'replacement'}}],cleanup); +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 [call["operationId"] for call in output["calls"]] == ["stable-progress-op", "stable-progress-op"] + assert output["error"] == "Progress update is already queued; photo cleanup is pending." + 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