fix(system): address the S7 final review

- I1: an apply answer is a refusal only when our server sent it (a known code, or a 4xx whose body says success:false); a proxy's 5xx, an unreadable 2xx or a dropped connection says Outcome not confirmed and reads Activity again. ApiError gains 'refused'; the server's own pre-start failure is coded 'apply-failed'. - M1: the lost-response test lets the apply run, then drops its answer. - M2: a failed apply records its code in the job's scope; Activity offers Review again for an overtaken plan (Maintenance for a duplicate cleanup whose sources changed). - M3: a failed duplicate cleanup toasts the server's reason. - M4: a review button holds while its preview is read (one preview per double press). - M5: the storage guard also catches bracket and property assignment and CacheStorage/IndexedDB opens; why the Toolkit build is skipped is written down. - M6: a refused storage listing says so on the Metadata cache row. - M7, M8: D100 text corrected; workflow_storage has its title. - M9: a busy start names the job the server named.
2026-10-24 21:02:30 -07:00
parent 4cc6ee767a
commit a99a311e82
16 changed files with 354 additions and 44 deletions
@@ -2441,7 +2441,9 @@ Recorded deviations (plan D60–D70, D79–D81 renumbered D92–D102 and D111–
  finished · N groups found", with "Search log" and "Cancel search". The duplicate
  cleanup (`duplicate_cleanup`, not cancellable) reads "Removing duplicate files", counts
  Removed (green), Kept and Errors (Kept rows open the viewer), and toasts "Duplicate
-  cleanup finished · N removed · M errors". Every job keeps Download report. Tests: the
+  cleanup finished · N removed · M errors" or, when it failed, "Duplicate cleanup failed ·
+  <the server's status line>". A refused start names the job the server named, not the
+  first running one. Every job keeps Download report. Tests: the
  three S7 cases in `tests/js/workspace-activity-kinds.test.js`.
 - **D112** (Activity, S5): a legacy-page operation held on the registry (not
  cancellable, no outcome groups) reads as its server title ("Database restore (admin
@@ -2451,11 +2453,15 @@ Recorded deviations (plan D60–D70, D79–D81 renumbered D92–D102 and D111–
 - **D100** (Maintenance, S7): the concept's `maintenancePlan` dialog ("Review maintenance
  plan") on the server's read-only plan (`POST /maintenance/preview`), opened by each
  Review plan and by the health report's Review missing links. No sample note. The `h3`
-  is the operation's concept name; the `dl.meta-grid` keeps Scope / Protected / Backup /
+  is the operation's row title (the concept prints its `data-plan` name, "duplicate
+  prompts"); the `dl.meta-grid` keeps Scope / Protected / Backup /
  Media on disk with the plan's values: its scope label, its protected reasons joined
  with " · " ("Nothing is protected" when none), "Create and verify before changes
-  (<db>.backup_<date and time>)", "No safety backup (the policy is off)" or "Always
-  created and verified first (…)" for Store each workflow once, and its media line. A
+  (<db>.backup_<date and time>)" or "No safety backup (the policy is off)", and its
+  media line. (A `workflow_storage` plan would read "Store each workflow once" and
+  "Always created and verified first (…)", but Maintenance has no row for it: the
+  concept has none, so the migration is reviewed and applied through the API and only
+  its jobs appear, in Activity.) A
  `details` "Listed records (N)" ("(N of M)" past the server's 50) holds the records the
  plan names in a `pre`. The `p.small` says "Nothing changes until you apply. This
  review holds for N minutes." (from `expires_at`), plus "Not enough free disk space:
@@ -2466,8 +2472,19 @@ Recorded deviations (plan D60–D70, D79–D81 renumbered D92–D102 and D111–
  Activity. A refused apply stays and says why: "Another job is running: <title>."
  (D57), "This plan was already applied: <job>." (`plan-used`, linking the job), the
  server's line for an expired or replaced review (then the plan is reviewed again at
-  once), for a short disk and for nothing to do, and "Outcome not confirmed. Check
-  Activity before applying it again." when the answer was lost (the job may run).
+  once), for a short disk, for nothing to do and for the server's own pre-start failure
+  (`apply-failed`). Only our server's refusal is definitive: a known code, or a 4xx whose
+  body says `success:false`. A dropped connection, a proxy's 5xx (no code) or a 2xx whose
+  body cannot be read may follow a started job, so they say "Outcome not confirmed. Check
+  Activity before applying it again." and read the Activity list again
+  (`lost apply response…` runs the apply and then drops its answer; `a proxy error or an
+  unreadable answer is not a refusal` answers a 504 after the server started the job). A
+  review button holds ("Reviewing…", `aria-disabled`) while its preview is read, so a
+  double press sends one preview (`a review button pressed twice sends one preview`). A
+  failed apply records its code as the job's `scope.failure`; when the plan was overtaken
+  (`plan-changed`, `storage-changed`, `plan-expired`), its Activity view offers "Review
+  again", which reviews the same operation afresh (a duplicate cleanup whose sources
+  changed goes back to Maintenance to search again).
  Cancel and Escape write nothing (`Cancel writes nothing`: a read-only dump of the rows
  and the media hashes, `GET /__test__/system/digest`). `plan dialogs match the concept`
  (system-maintenance-concept.spec, both themes and sizes) compares each of the four
@@ -2493,7 +2510,8 @@ Recorded deviations (plan D60–D70, D79–D81 renumbered D92–D102 and D111–
  Prompt Manager owns as a cache (`web/js/cache-ownership.js`). The inventory of every
  storage writer has none (see "Browser cache ownership" below), so the row's line adds
  "Nothing is cached in this browser." and its Review plan is `aria-disabled` and opens
-  nothing. With owned entries (a test names one through the module's inventory), Review
+  nothing; a browser that refused to list its storage says "This browser did not allow
+  checking its storage." instead. With owned entries (a test names one through the module's inventory), Review
  plan opens "Review maintenance plan" › "Metadata cache" (Scope "N cached entries in
  this browser", Protected "Preferences and other tools’ data in this browser", Backup
  "Not needed: the database is not changed", Media "No files are removed") and "Clear
@@ -2926,7 +2944,12 @@ writes is a preference (`gallerySettings`, `pm_workspace_theme`,
 `web/sw.js` service worker that did never shipped). So nothing is removed, and the
 toast says "Nothing of Prompt Manager's is cached in this browser." `the inventory covers
 every browser storage writer in web/` (`tests/js/cache-ownership.test.js`) fails when a
-page writes a key that is neither a preference nor inventoried, or opens a cache. The
+page writes a key that is neither a preference nor inventoried, writes one it cannot name
+(a computed `setItem` key, a bracket or property assignment on a Storage), or opens
+CacheStorage or IndexedDB; `the storage scan reports every way of writing browser
+storage` pins those patterns. It skips `web/lib` (third-party builds) and the pinned
+Toolkit build (`web/js/shared/toolkit`, never hand-edited, verified by `npm run check`;
+its storage belongs to the Toolkit repository and a new pin is reviewed on its own). The
 gallery's server thumbnail clear stays its own request and runs first: refused or
 unreachable, it changes nothing and says so (the old code toasted success when the
 request failed); refused browser storage is reported, never hidden
@@ -37,7 +37,8 @@ runner runs); a refused start keeps it. The runner:

 The job log names the backup by its file name only, and the job says "No
 safety backup" when none was taken. Apply errors: ``invalid-request`` (400),
-``plan-expired`` (410: unknown, expired or forgotten after a restart),
+``apply-failed`` (500: nothing started), ``plan-expired`` (410: unknown,
+expired or forgotten after a restart),
 ``plan-used`` (409, with the job that applied it), ``storage-changed`` (409:
 a restore replaced the database since the review) and ``busy`` (409).
 """
@@ -644,7 +645,9 @@ class MaintenanceRoutesMixin:
            )
        except (OSError, sqlite3.Error):
            self.logger.exception("Could not measure the disk space for an apply")
-            return _failure(APPLY_FAILED, 500)
+            # Coded: the workspace tells the server's own refusal (nothing
+            # started) from a proxy's 5xx (the job may have started).
+            return _failure(APPLY_FAILED, 500, "apply-failed")
        if short is not None:
            # Refused before any job starts: nothing fails halfway for space.
            return _failure(short_space_message(short), 507, "insufficient-space")
@@ -808,7 +811,13 @@ def backup_note(backup):


 def _fail_job(job, code, message):
-    """End ``job`` failed with ``message`` (one error row) and its code."""
+    """End ``job`` failed with ``message`` (one error row) and its code.
+
+    The code is also the job's ``scope["failure"]``, so Activity can offer
+    Review again for a plan the library overtook.
+    """
+    if code:
+        job.scope["failure"] = code
    job.record("errors", {"path": "", "detail": message}, f"! {message}")
    job.update(status_line=message)
    job.publish({"type": "error", "code": code, "message": message})
@@ -21,6 +21,7 @@ test.beforeEach(async ({ page, request, browserName }) => {

 test.afterEach(async ({ page, request, browserName }) => {
    await saveCoverage(page, browserName);
+    await request.post('/__test__/release');
    await request.post('/__test__/jobs/restart');
    await request.post('/__test__/system/reset');
    expect(page.systemErrors).toEqual([]);
@@ -212,6 +213,49 @@ test('plan-changed asks for a new review', async ({ page, request }) => {
    await expect(toast(page)).toHaveText(`Duplicate prompt text failed · ${CHANGED}`);
    await expect(page.locator('#workspace-main')).toContainText(CHANGED);
    expect((await serverJson(request, '/__test__/system/digest')).rows).toEqual(before.rows);
+    // S7 review M2: the failed job offers Review again, which reviews the same operation afresh.
+    const previews = [];
+    page.on('request', item => { if (item.url().endsWith(PREVIEW_URL)) previews.push(item.postDataJSON()); });
+    await page.locator('#workspace-main').getByRole('button', { name: 'Review again', exact: true }).click();
+    await expect(dialog(page).locator('#modal-title')).toHaveText('Review maintenance plan');
+    await expect(dialog(page).locator('#modal-body > h3')).toHaveText('Duplicate prompt text');
+    expect(previews).toEqual([{ operation: 'duplicate_prompts', options: {} }]);
+});
+
+test('a review button pressed twice sends one preview (S7 review M4)', async ({ page, request }) => {
+    await expect(await request.post('/__test__/system/missing-file')).toBeOK();
+    await expect(await request.post('/__test__/system/duplicate-files')).toBeOK();
+    const previews = [];
+    page.on('request', item => { if (item.url().endsWith(PREVIEW_URL)) previews.push(item.postDataJSON().operation); });
+    await openMaintenance(page);
+    await row(page, 'Integrity & relationship checks').getByRole('button', { name: 'Check', exact: true }).click();
+    // Hold the preview at the server so the second press lands while it is read.
+    await expect(await request.post('/__test__/hold', { data: { path: PREVIEW_URL } })).toBeOK();
+    // The pressed button reads "Reviewing…" while held, so it is found by its place, not its name.
+    const missing = dialog(page).locator('.dialog-actions > button').last();
+    await expect(missing).toHaveText('Review missing links');
+    await missing.click();
+    await expect(missing).toHaveAttribute('aria-disabled', 'true');
+    await expect(missing).toHaveText('Reviewing…');
+    await missing.dispatchEvent('click');
+    await expect.poll(async () => (await serverJson(request, '/__test__/hold')).held[PREVIEW_URL]).toBe(1);
+    await expect(await request.post('/__test__/release')).toBeOK();
+    await expect(dialog(page).locator('#modal-body > h3')).toHaveText('Missing-file links');
+    expect(previews).toEqual(['missing_links']);
+    await page.keyboard.press('Escape');
+
+    await row(page, 'Duplicate image files').getByRole('button', { name: 'Find duplicates', exact: true }).click();
+    await page.locator('#workspace-main').getByRole('button', { name: 'Review groups', exact: true }).click();
+    await expect(await request.post('/__test__/hold', { data: { path: PREVIEW_URL } })).toBeOK();
+    const cleanup = dialog(page).locator('.dialog-actions > button').last();
+    await expect(cleanup).toHaveText('Review duplicate cleanup');
+    await cleanup.click();
+    await expect(cleanup).toHaveAttribute('aria-disabled', 'true');
+    await cleanup.dispatchEvent('click');
+    await expect.poll(async () => (await serverJson(request, '/__test__/hold')).held[PREVIEW_URL]).toBe(1);
+    await expect(await request.post('/__test__/release')).toBeOK();
+    await expect(dialog(page).locator('#modal-body > h3')).toHaveText('Duplicate image files');
+    expect(previews).toEqual(['missing_links', 'duplicate_files']);
 });

 test('plan-used links the job that applied it', async ({ page, request }) => {
@@ -232,15 +276,68 @@ test('plan-used links the job that applied it', async ({ page, request }) => {
    await expect(page).toHaveURL(`${workspace}#/system/maintenance`);
 });

+/**
+ * Record every toast the page shows (`[text, [link hrefs]]`): a job's finish toast may replace an earlier one before a
+ * test reads it.
+ */
+async function recordToasts(page) {
+    await page.addInitScript(() => {
+        window.toastLog = [];
+        const record = node => window.toastLog.push([node.textContent, [...node.querySelectorAll('a')].map(a => a.getAttribute('href'))]);
+        new MutationObserver(() => {
+            const node = document.getElementById('workspace-toast');
+            const last = window.toastLog.at(-1);
+            if (node && node.textContent && last?.[0] !== node.textContent) record(node);
+        }).observe(document, { subtree: true, childList: true, characterData: true });
+    });
+    return () => page.evaluate(() => window.toastLog);
+}
+
+const NOT_CONFIRMED = ['Outcome not confirmed. Check Activity before applying it again.', ['#/activity']];
+
+/** The maintenance jobs the server lists (the apply ran even though its answer was lost). */
+async function maintenanceJobs(request) {
+    return (await serverJson(request, '/prompt_manager/jobs')).jobs.filter(job => job.kind === 'maintenance');
+}
+
 test('lost apply response says Outcome not confirmed and points to Activity', async ({ page, request }) => {
    await expect(await request.post('/__test__/system/duplicate-prompts')).toBeOK();
-    await page.route(`**${APPLY_URL}`, route => route.abort('failed'));
+    // The server applies the plan; its answer never reaches the page.
+    const reads = [];
+    await page.route(`**${APPLY_URL}`, async route => {
+        await route.fetch();
+        page.on('request', item => { if (item.url().endsWith('/prompt_manager/jobs')) reads.push(item.url()); });
+        await route.abort('failed');
+    });
+    const toasts = await recordToasts(page);
    await openMaintenance(page);
    await row(page, 'Duplicate prompt text').getByRole('button', { name: 'Review plan', exact: true }).click();
    await dialog(page).getByRole('button', { name: 'Apply reviewed operation', exact: true }).click();
-    await expect(toast(page)).toHaveText('Outcome not confirmed. Check Activity before applying it again.');
-    await expect(toast(page).getByRole('link', { name: 'Check Activity', exact: true })).toHaveAttribute('href', '#/activity');
+    await expect.poll(toasts).toContainEqual(NOT_CONFIRMED);
    await expect(page).toHaveURL(`${workspace}#/system/maintenance`);
+    // The Activity list is read again at once, and the job the lost answer started is in it.
+    await expect.poll(() => reads.length).toBeGreaterThan(0);
+    expect((await maintenanceJobs(request)).map(job => job.title)).toEqual(['Duplicate prompt text']);
+    await page.evaluate(() => { location.hash = '#/activity'; });
+    await expect(page.locator('#workspace-main')).toContainText('Duplicate prompt text');
+});
+
+test('a proxy error or an unreadable answer is not a refusal: Outcome not confirmed (S7 review I1)', async ({ page, request }) => {
+    await expect(await request.post('/__test__/system/duplicate-prompts')).toBeOK();
+    const reads = [];
+    // A reverse proxy times out after the server started the job.
+    await page.route(`**${APPLY_URL}`, async route => {
+        await route.fetch();
+        page.on('request', item => { if (item.url().endsWith('/prompt_manager/jobs')) reads.push(item.url()); });
+        await route.fulfill({ status: 504, contentType: 'text/html', body: '<html><body>Gateway Timeout</body></html>' });
+    });
+    const toasts = await recordToasts(page);
+    await openMaintenance(page);
+    await row(page, 'Duplicate prompt text').getByRole('button', { name: 'Review plan', exact: true }).click();
+    await dialog(page).getByRole('button', { name: 'Apply reviewed operation', exact: true }).click();
+    await expect.poll(toasts).toContainEqual(NOT_CONFIRMED);
+    await expect.poll(() => reads.length).toBeGreaterThan(0);
+    expect(await maintenanceJobs(request)).toHaveLength(1);
 });

 test('a refused apply names the running job', async ({ page, request }) => {
@@ -387,3 +387,12 @@ test("ApiError keeps a validated running job on busy and a validated source on i
    }
    assert.deepEqual([new ApiError(409, "old caller").running, new ApiError(409, "old caller").source], [null, null]);
 });
+
+test("ApiError says whether the server itself refused (a parsed success:false body; S7 review I1)", () => {
+    assert.equal(new ApiError(400, "bad", null, null, { success: false, error: "bad" }).refused, true);
+    assert.equal(new ApiError(409, "busy", "busy", null, { success: false, code: "busy" }).refused, true);
+    // A proxy's page, a body that was not JSON, or no body at all: nothing says the server refused.
+    for (const body of [null, undefined, "<html>", { success: true }, [], { error: "x" }]) {
+        assert.equal(new ApiError(504, "Gateway Timeout", null, null, body).refused, false);
+    }
+});
@@ -68,9 +68,50 @@ test('removes only inventoried owned keys', async () => {
    assert.equal(CacheOwnership.clearMessage(result), CacheOwnership.NOTHING_CACHED);
 });

+/**
+ * Every browser-storage write in one file's text: `{keys}` (setItem keys, as written) and `problems` (writes no key
+ * can be read from: a bracket or property assignment on a Storage, a computed setItem key, an IndexedDB or
+ * CacheStorage open or write). The scan is textual, so it errs on the side of reporting.
+ */
+function storageWrites(text) {
+    const keys = [];
+    const problems = [];
+    for (const match of text.matchAll(/\.setItem\(\s*([^,)]+)/g)) {
+        const key = match[1].trim();
+        const literal = /^(?:'([^']+)'|"([^"]+)")$/.exec(key);
+        if (literal) keys.push(literal[1] ?? literal[2]);
+        else if (/^[A-Z_]+_KEY$/.test(key)) keys.push(`$${key}`);
+        else problems.push(`a computed setItem key: ${key}`);
+    }
+    if (/\b\w*[sS]torage\s*\[[^\]]+\]\s*=(?!=)/.test(text)) problems.push('a bracket assignment on a Storage');
+    if (/\b(?:local|session)Storage\s*\.\s*(?!getItem|setItem|removeItem|key\b|length\b|clear\b)\w+\s*=(?!=)/.test(text)) {
+        problems.push('a property assignment on a Storage');
+    }
+    if (/\bcaches\s*\.\s*open\s*\(/.test(text)) problems.push('opens a CacheStorage cache');
+    if (/\bindexedDB\s*\.\s*open\s*\(/.test(text)) problems.push('opens IndexedDB');
+    return { keys, problems };
+}
+
+test('the storage scan reports every way of writing browser storage (S7 review M5)', () => {
+    assert.deepEqual(storageWrites("localStorage.setItem('gallerySettings', x); storage.setItem(NOTICE_KEY, v);").keys,
+        ['gallerySettings', '$NOTICE_KEY']);
+    for (const text of ["localStorage['pm_cache'] = 1;", 'sessionStorage[key] = value;', 'storage[name] = x;',
+        'localStorage.pm_cache = 1;', 'sessionStorage.someKey = "v";', 'localStorage.setItem(key, 1);',
+        "caches.open('pm').then(cache => cache.put(url, response));", "indexedDB.open('pm-cache', 1);"]) {
+        assert.equal(storageWrites(text).problems.length, 1, text);
+    }
+    for (const text of ["if (localStorage['pm'] === x) {}", 'const value = localStorage.getItem(KEY);',
+        'localStorage.removeItem(KEY);', 'const n = localStorage.length;']) {
+        assert.deepEqual(storageWrites(text), { keys: [], problems: [] }, text);
+    }
+});
+
 test('the inventory covers every browser storage writer in web/', () => {
    // Every key a Prompt Manager page writes is a preference or an inventoried cache entry; a new writer must be
-    // classified here before a cache clear may remove it.
+    // classified here before a cache clear may remove it. Skipped: web/lib (third-party Viewer.js and Tailwind
+    // output) and web/js/shared/toolkit (the pinned ComfyAssets Toolkit build, never hand-edited here and verified by
+    // `npm run --prefix tools/vendor check`). Neither writes browser storage today; the Toolkit's own repository owns
+    // its storage, and a new pin is reviewed as its own change.
    const files = [];
    const walk = dir => {
        for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
@@ -85,15 +126,9 @@ test('the inventory covers every browser storage writer in web/', () => {
    for (const file of files) {
        const text = fs.readFileSync(file, 'utf8');
        for (const match of text.matchAll(/(?:const|export const)\s+([A-Z_]+_KEY)\s*=\s*'([^']+)'/g)) constants[match[1]] = match[2];
-        assert.equal(/caches\.open\(/.test(text), false, `${path.relative(WEB, file)} opens a CacheStorage cache`);
-        assert.equal(/indexedDB\.open\(/.test(text), false, `${path.relative(WEB, file)} opens IndexedDB`);
-        for (const match of text.matchAll(/\.setItem\(\s*([^,)]+)/g)) {
-            const literal = /^(?:'([^']+)'|"([^"]+)")$/.exec(match[1].trim());
-            const constant = /^[A-Z_]+_KEY$/.test(match[1].trim()) ? `$${match[1].trim()}` : null;
-            // A key computed at run time could be anything: it must be a literal or a named key constant.
-            assert.ok(literal || constant, `${path.relative(WEB, file)} writes a computed key: ${match[1]}`);
-            written.add(literal ? literal[1] ?? literal[2] : constant);
-        }
+        const { keys, problems } = storageWrites(text);
+        assert.deepEqual(problems, [], path.relative(WEB, file));
+        for (const key of keys) written.add(key);
    }
    const keys = [...written].map(key => (key.startsWith('$') ? constants[key.slice(1)] : key));
    assert.ok(keys.length >= 3, 'the writers were found');
@@ -216,3 +216,12 @@ test('a duplicate cleanup counts removed, kept and errors and cannot be cancelle
    assert.equal(finishToast({ ...cleanup, state: 'finished', counters: { removed: 3, kept: 1, errors: 1 } }),
        'Duplicate cleanup finished · 3 removed · 1 error');
 });
+
+test('a failed duplicate cleanup says the server reason (S7 review M3)', async () => {
+    const { finishToast } = await store;
+    const reason = 'The sources changed since the search. Search for duplicates again.';
+    const failed = job({ kind: 'duplicate_cleanup', title: 'Remove duplicate image files', state: 'failed', status_line: reason,
+        counters: { removed: 0, kept: 0, errors: 1 }, cancellable: false });
+    assert.equal(finishToast(failed), `Duplicate cleanup failed · ${reason}`);
+    assert.equal(finishToast({ ...failed, status_line: '' }), 'Duplicate cleanup failed · 0 removed · 1 error');
+});
@@ -437,3 +437,16 @@ test('numbers group the way the server writes them, whatever the browser locale'
        Number.prototype.toLocaleString = original;
    }
 });
+
+test('a busy start names the job the server named, not the first running one (S7 review M9)', async () => {
+    const { createActivityStore } = await source;
+    const { world, ...deps } = harness();
+    const store = createActivityStore(deps);
+    const holder = 'b'.repeat(32);
+    // Two read jobs run side by side; the server says the second one holds the slot.
+    world.list = [summary(), summary({ id: holder, kind: 'duplicates', title: 'Find duplicate image files' })];
+    world.postError = Object.assign(apiError(409, 'busy', 'busy'),
+        { running: { id: holder, title: 'Find duplicate image files' } });
+    const busy = await store.start('duplicates');
+    assert.deepEqual([busy.busy.id, busy.busy.title], [holder, 'Find duplicate image files']);
+});
@@ -61,6 +61,8 @@ test("plan dialog shows the server's scope, protected, backup policy and media l
    assert.deepEqual(detailLines(PLAN), ['a cat · 2 copies merged into the oldest', 'a dog']);
    assert.deepEqual(detailLines({ details: [null, { label: 7 }] }), []);
    assert.equal(OPERATION_TITLES.missing_links, 'Missing-file links');
+    // S7 review M8: the workflow migration's plan reads its own title (its job is started outside Maintenance).
+    assert.equal(OPERATION_TITLES.workflow_storage, 'Store each workflow once');
 });

 test('a refused apply names its reason; a lost answer is not confirmed', async () => {
@@ -79,7 +81,15 @@ test('a refused apply names its reason; a lost answer is not confirmed', async (
        { toast: 'This review expired. Review the plan again.', review: true });
    assert.deepEqual(applyRefusal(error(409, 'storage-changed', 'Replaced.')), { toast: 'Replaced.', review: true });
    assert.deepEqual(applyRefusal(error(507, 'insufficient-space', 'Not enough free disk space.')), { toast: 'Not enough free disk space.' });
-    assert.deepEqual(applyRefusal(error(500, null, 'Traceback …')), { toast: APPLY_FAILED });
+    // S7 review I1: only our server's own refusal is definitive. A proxy's 5xx, a 2xx whose body could not be read,
+    // or any answer without the server's refusal may have followed a started job: not confirmed, read Activity again.
+    assert.deepEqual(applyRefusal(error(504, null, 'Gateway Timeout')), { toast: NOT_CONFIRMED, lost: true });
+    assert.deepEqual(applyRefusal(error(500, null, 'Internal Server Error')), { toast: NOT_CONFIRMED, lost: true });
+    assert.deepEqual(applyRefusal(error(202, null, 'Invalid JSON response')), { toast: NOT_CONFIRMED, lost: true });
+    assert.deepEqual(applyRefusal(error(502, null, 'Bad Gateway', { refused: false })), { toast: NOT_CONFIRMED, lost: true });
+    assert.deepEqual(applyRefusal(error(400, null, 'token must be the plan token', { refused: true })), { toast: APPLY_FAILED });
+    assert.deepEqual(applyRefusal(error(500, 'apply-failed', 'Maintenance failed; nothing was changed.', { refused: true })),
+        { toast: 'Maintenance failed; nothing was changed.' });
    assert.equal(reviewRefusal(error(413, 'too-large', 'This plan is too large to review at once.')),
        'This plan is too large to review at once.');
    assert.equal(reviewRefusal(error(500, null, 'boom')), REVIEW_FAILED);
@@ -117,6 +127,9 @@ test('Metadata cache follows Q5: disabled with its reason while nothing is owned
    assert.equal(METADATA_CACHE_DESCRIPTION, 'Clear only Prompt Manager metadata cached by this browser.');
    assert.equal(metadataCacheDescription({ keys: [], caches: [] }), `${METADATA_CACHE_DESCRIPTION} ${NOTHING_CACHED_REASON}`);
    assert.equal(metadataCacheDescription({ keys: ['k'], caches: [] }), METADATA_CACHE_DESCRIPTION);
+    // S7 review M6: a browser that refused the listing is not claimed empty.
+    assert.equal(metadataCacheDescription({ keys: [], caches: [], denied: true }),
+        `${METADATA_CACHE_DESCRIPTION} This browser did not allow checking its storage.`);
 });

 test('only a finished duplicate search with groups offers Review groups', async () => {
@@ -128,3 +141,18 @@ test('only a finished duplicate search with groups offers Review groups', async
    assert.equal(hasDuplicateGroups(job('finished', 2, 'scan')), false);
    assert.equal(hasDuplicateGroups({ kind: 'duplicates', state: 'finished', counters: {} }), false);
 });
+
+test('a failed apply the library overtook offers Review again, back to its review (S7 review M2)', async () => {
+    const { reviewAgainOf } = await jobView;
+    const failed = (kind, failure, operation) => ({ kind, state: 'failed', scope: { operation, failure } });
+    assert.deepEqual(reviewAgainOf(failed('maintenance', 'plan-changed', 'duplicate_prompts')),
+        { operation: 'duplicate_prompts' });
+    assert.deepEqual(reviewAgainOf(failed('maintenance', 'storage-changed', 'optimize')), { operation: 'optimize' });
+    // A duplicate cleanup reviewed against a search the sources overtook: search again in Maintenance.
+    assert.deepEqual(reviewAgainOf(failed('duplicate_cleanup', 'sources-changed', 'duplicate_files')),
+        { section: '#/system/maintenance' });
+    assert.equal(reviewAgainOf(failed('maintenance', 'backup-failed', 'optimize')), null);
+    assert.equal(reviewAgainOf(failed('maintenance', 'plan-changed', 'not_an_operation')), null);
+    assert.equal(reviewAgainOf({ ...failed('maintenance', 'plan-changed', 'optimize'), state: 'finished' }), null);
+    assert.equal(reviewAgainOf(failed('scan', 'plan-changed', 'optimize')), null);
+});
@@ -949,6 +949,37 @@ class TestFinalReviewFixes(ApplyTestCase):
        self.assertEqual(reply["code"], "plan-used")
        self.assertEqual(self.api._jobs.list(), [])

+    async def test_an_apply_that_never_started_says_so_with_its_code(self):
+        """S7 review I1: the server's own pre-start failure is definitive, so
+        it carries a code the workspace can tell from a proxy's 5xx."""
+        from py.api import maintenance
+
+        self.save("Orphan")
+        plan = await self.preview("unused_components")
+        with patch.object(maintenance, "disk_plan", side_effect=OSError("disk")):
+            reply = await self.apply(plan["token"], 500)
+        self.assertEqual(reply["code"], "apply-failed")
+        self.assertEqual(reply["error"], maintenance.APPLY_FAILED)
+        self.assertEqual(self.api._jobs.list(), [])
+
+    async def test_a_failed_apply_names_its_failure_in_its_scope(self):
+        """S7 review M2: Activity offers Review again from the job's own record."""
+        orphan, soon = self.save("Orphan"), self.save("Soon a recipe")
+        plan = await self.preview("unused_components")
+        with self.api.db.model.get_connection() as conn:
+            conn.execute(
+                "INSERT INTO prompt_library_state(prompt_id,title) VALUES(?,?)",
+                (soon, "Recipe"),
+            )
+        body = await self.apply(plan["token"])
+        job = self.api._jobs.get(body["job"]["id"])
+        await asyncio.wait_for(asyncio.shield(job.task), WAIT)
+        self.assertEqual(
+            job.summary()["scope"],
+            {"operation": "unused_components", "failure": "plan-changed"},
+        )
+        self.assertIsNotNone(self.api.db.get_prompt_by_id(orphan))
+
    async def test_the_backup_decision_is_frozen_when_the_job_starts(self):
        PromptManagerConfig.SAFETY_BACKUP = False
        self.save("Orphan")
@@ -50,6 +50,10 @@
                running.title.length > 0 && running.title.length <= 200) {
                this.running = { id: running.id, title: running.title };
            }
+            // Whether our server itself refused: its body parsed and says success:false. A proxy's error page or an
+            // unreadable body is not a refusal (the request may have done its work; System S7 review I1).
+            this.refused = Boolean(current && typeof current === "object" && !Array.isArray(current) &&
+                current.success === false);
            // A used plan names the job that applied it (System S7); its title is null once that job was pruned.
            this.job = null;
            const job = current && current.job;
@@ -37,7 +37,8 @@ import { createMediaViewer } from './media-viewer.mjs';
 import { createInspectionController } from './inspection-state.mjs';
 import { createPromptSender } from './prompt-sender.mjs';
 import { imageKey } from './images-state.mjs';
-import { openDuplicatesReview } from './maintenance-review.mjs';
+import { OPERATION_TITLES, openDuplicatesReview, reviewPlan } from './maintenance-review.mjs';
+import { navigateTo } from './state.mjs';

 export const DETAIL_INTERVAL_MS = 1000;
 export const NOT_FOUND = 'This job is not in this session’s Activity. Jobs are kept until ComfyUI restarts.';
@@ -76,6 +77,26 @@ export function parseDetail(raw) {
    return { job, outcomes, log };
 }

+// Failures that mean the reviewed plan no longer matches: the review must be done again (S7 review M2).
+const OVERTAKEN = new Set(['plan-changed', 'storage-changed', 'plan-expired']);
+
+/**
+ * Where a failed apply's Review again goes (S7 review M2): a maintenance plan the library overtook is reviewed again
+ * (`{operation}`); a duplicate cleanup whose sources or plan changed needs a new search, in Maintenance (`{section}`);
+ * otherwise null.
+ */
+export function reviewAgainOf(job) {
+    if (job.state !== 'failed') return null;
+    const failure = job.scope?.failure;
+    if (job.kind === 'maintenance' && OVERTAKEN.has(failure) && Object.hasOwn(OPERATION_TITLES, job.scope?.operation)) {
+        return { operation: job.scope.operation };
+    }
+    if (job.kind === 'duplicate_cleanup' && (OVERTAKEN.has(failure) || failure === 'sources-changed')) {
+        return { section: '#/system/maintenance' };
+    }
+    return null;
+}
+
 /** D101: a duplicate search that finished with groups to review. */
 export function hasDuplicateGroups(job) {
    return job.kind === 'duplicates' && job.state === 'finished' && (job.counters.groups_found ?? 0) > 0;
@@ -135,6 +156,8 @@ export function mountActivityJobView(main, { store, scope, api, jobId }) {
    const review = createReviewMetadataButton(scope); // D87
    // System S7 (D101): a finished duplicate search that found groups reviews them.
    const reviewGroups = createButton('Review groups', 'ca-primary');
+    // S7 review M2: a failed apply whose plan the library overtook goes back to its review.
+    const reviewAgain = createButton('Review again', 'ca-primary');

    const progressPart = section('Progress', 'job-progress-title');
    const status = element('span', '', 'small');
@@ -237,6 +260,7 @@ export function mountActivityJobView(main, { store, scope, api, jobId }) {
            const focused = main.contains(document.activeElement) && document.activeElement !== parts.h1;
            download.remove();
            reviewGroups.remove();
+            reviewAgain.remove();
            cancel.remove();
            progressPart.node.remove();
            resultsPart.node.remove();
@@ -302,6 +326,8 @@ export function mountActivityJobView(main, { store, scope, api, jobId }) {
        else review.remove();
        if (hasDuplicateGroups(job)) { if (!reviewGroups.isConnected) download.after(reviewGroups); } // D101
        else reviewGroups.remove();
+        if (reviewAgainOf(job)) { if (!reviewAgain.isConnected) download.after(reviewAgain); }
+        else reviewAgain.remove();
        // Truth rule: a job that cannot be cancelled (a held legacy-page job) shows no Cancel.
        if (canCancel(job)) {
            if (!cancel.isConnected) parts.actions.append(cancel);
@@ -404,6 +430,12 @@ export function mountActivityJobView(main, { store, scope, api, jobId }) {
        }
    });
    scope.listen(reviewGroups, 'click', () => openDuplicatesReview({ api, store, scope, jobId, trigger: reviewGroups }));
+    scope.listen(reviewAgain, 'click', () => {
+        const job = current();
+        const target = job ? reviewAgainOf(job) : null;
+        if (target?.operation) reviewPlan({ api, store, scope, operation: target.operation, trigger: reviewAgain });
+        else if (target?.section) navigateTo(target.section);
+    });
    scope.listen(cancel, 'click', async () => {
        if (cancel.getAttribute('aria-disabled') === 'true') return;
        cancel.setAttribute('aria-disabled', 'true');
@@ -172,7 +172,9 @@ export const KINDS = Object.freeze({
    duplicate_cleanup: Object.freeze({
        ...GENERIC,
        pillVerb: 'Removing duplicate files',
-        finishToast: namedFinish(() => 'Duplicate cleanup', removedOf, cleanupLine),
+        // A failed cleanup says the server's reason (sources changed, no safety backup), as a maintenance apply does.
+        finishToast: job => (job.state === 'failed' && job.status_line ? `Duplicate cleanup failed · ${job.status_line}`
+            : namedFinish(() => 'Duplicate cleanup', removedOf, cleanupLine)(job)),
        line: cleanupLine,
        kept: removedOf,
        tiles: tiles([['removed', 'Removed', 'added'], ['kept', 'Kept', ''], ['errors', 'Errors', 'bad']]),
@@ -246,7 +246,8 @@ export function createActivityStore({ api, scope, connect = defaultConnect, noti
            } catch (error) {
                if (error?.status !== 409 || error?.code !== 'busy') throw error;
                await load();
-                return { busy: store.running()[0] ?? null, message: error.message };
+                // The job the server named holds the slot; read jobs run side by side, so the first running one may not.
+                return { busy: error.running ?? store.running()[0] ?? null, message: error.message };
            }
        },
        /**
@@ -49,12 +49,14 @@ export const OPERATION_TITLES = Object.freeze({
    unused_components: 'Unused prompt components',
    missing_links: 'Missing-file links',
    duplicate_files: 'Duplicate image files',
+    // Reviewed and applied through the API and seen in Activity; Maintenance has no row for it (the concept has none).
+    workflow_storage: 'Store each workflow once',
 });
 // The review is gone or no longer matches the database: review it again.
 const REVIEW_AGAIN = new Set(['plan-expired', 'storage-changed']);
 // Refusals whose server message is the whole truth.
 const SERVER_SAYS = new Set(['insufficient-space', 'nothing-to-do', 'too-large', 'invalid-request', 'not-found',
-    'job-running', 'no-groups']);
+    'job-running', 'no-groups', 'apply-failed']);

 const number = value => value.toLocaleString('en-US');

@@ -117,9 +119,17 @@ export function detailLines(plan) {
        .map(row => (row.detail ? `${row.label} · ${row.detail}` : row.label));
 }

-/** The toast for a refused or failed apply; `review` when the plan must be reviewed again. */
+/**
+ * The toast for a refused or failed apply; `review` when the plan must be reviewed again, `lost` when the outcome is
+ * not known. Only our server's own refusal is definitive (a known code, or a 4xx whose body says success:false): a
+ * dropped connection, a proxy's 5xx or a 2xx whose body could not be read may follow a started job.
+ */
 export function applyRefusal(error) {
-    if (!Number.isInteger(error?.status)) return { toast: NOT_CONFIRMED, lost: true };
+    const status = error?.status;
+    const known = error?.code && (error.code === 'busy' || error.code === 'plan-used' || REVIEW_AGAIN.has(error.code)
+        || SERVER_SAYS.has(error.code));
+    const refused = Number.isInteger(status) && status >= 400 && status < 500 && error.refused === true;
+    if (!known && !refused) return { toast: NOT_CONFIRMED, lost: true };
    if (error.code === 'busy') return { toast: busyToast(error.running) };
    if (error.code === 'plan-used') {
        const job = error.job;
@@ -203,12 +213,14 @@ export function showPlan(plan, { api, store, scope, trigger = null, reviewAgain,

 /**
 * Review `operation` (`POST /maintenance/preview`) and show its plan. `options` are the preview's (duplicate_files:
- * `{job_id, keep}`). Resolves once the plan is shown or the refusal toasted.
+ * `{job_id, keep}`). `hold` (default `trigger`, which gets focus back) is the pressed button: it reads "Reviewing…"
+ * and ignores presses until the review answered, so a double press sends one preview. Resolves once the plan is shown
+ * or the refusal toasted.
 */
-export async function reviewPlan({ api, store, scope, operation, options = {}, trigger = null, notify = showWorkspaceToast,
-    navigate = navigateTo }) {
+export async function reviewPlan({ api, store, scope, operation, options = {}, trigger = null, hold = trigger,
+    notify = showWorkspaceToast, navigate = navigateTo }) {
    const again = () => reviewPlan({ api, store, scope, operation, options, trigger, notify, navigate });
-    await holding(trigger, 'Reviewing…', async () => {
+    await holding(hold, 'Reviewing…', async () => {
        let plan;
        try {
            plan = await api.post(PREVIEW_URL, { operation, options }, { signal: scope.signal });
@@ -344,7 +356,7 @@ export async function openDuplicatesReview({ api, store, scope, jobId, trigger =
    scope.listen(cancel, 'click', () => closeWorkspaceDialog());
    scope.listen(review, 'click', () => {
        if (review.disabled) return;
-        reviewPlan({ api, store, scope, operation: 'duplicate_files', trigger,
+        reviewPlan({ api, store, scope, operation: 'duplicate_files', trigger, hold: review,
            options: { job_id: jobId, keep: keepEntries(chosen) }, notify, navigate });
    });
    const nodes = [status, groupsNode];
@@ -28,6 +28,7 @@ import { noun } from './plural.mjs';
 export const DUPLICATES_KIND = 'duplicates';
 export const FIND_FAILED = 'The duplicate search could not start. Try again.';
 export const NOTHING_CACHED_REASON = 'Nothing is cached in this browser.';
+export const STORAGE_UNCHECKED_REASON = 'This browser did not allow checking its storage.';
 export const METADATA_CACHE_DESCRIPTION = 'Clear only Prompt Manager metadata cached by this browser.';

 /** The concept's two surfaces and their rows: `[title, description, label, action, danger]`. */
@@ -75,10 +76,13 @@ export async function findDuplicates({ store, scope, notify = showWorkspaceToast
    }
 }

-/** D102: the Metadata cache row's description: the concept's, plus the reason its review is unavailable. */
+/**
+ * D102: the Metadata cache row's description: the concept's, plus the reason its review is unavailable. A browser that
+ * refused to list its storage is never claimed empty.
+ */
 export function metadataCacheDescription(entries) {
    if (entries.keys.length + entries.caches.length > 0) return METADATA_CACHE_DESCRIPTION;
-    return `${METADATA_CACHE_DESCRIPTION} ${NOTHING_CACHED_REASON}`;
+    return `${METADATA_CACHE_DESCRIPTION} ${entries.denied ? STORAGE_UNCHECKED_REASON : NOTHING_CACHED_REASON}`;
 }

 /** D102: review the owned browser entries, then clear exactly those and say what happened. */
@@ -136,7 +140,8 @@ function metadataCacheRow({ scope, notify, cache }) {
 */
 export function mountMaintenance(container, { api, activity, scope, navigate = navigateTo, notify = showWorkspaceToast,
    cache = ownership() }) {
-    const reviewMissing = trigger => reviewPlan({ api, store: activity, scope, operation: 'missing_links', trigger, notify, navigate });
+    const reviewMissing = (trigger, pressed = trigger) => reviewPlan({ api, store: activity, scope, operation: 'missing_links',
+        trigger, hold: pressed, notify, navigate });
    const actions = {
        health: button => runHealthAction({ api, activity, scope, button, onReviewMissing: reviewMissing, notify }),
        duplicates: () => findDuplicates({ store: activity, scope, notify, navigate }),
@@ -214,7 +214,7 @@ export function healthNodes(report, { scope, onReviewMissing }) {
    const close = createButton('Close');
    const review = createButton('Review missing links', 'ca-primary');
    scope.listen(close, 'click', () => closeWorkspaceDialog());
-    scope.listen(review, 'click', () => onReviewMissing());
+    scope.listen(review, 'click', () => onReviewMissing(review));
    const actions = el('div', undefined, 'dialog-actions');
    actions.append(close, review);
    return [list, el('p', healthSummary(report), 'small'), actions];
@@ -269,13 +269,13 @@ export async function runHealthCheck(api, { activity, signal } = {}) {

 /**
 * Open the Health report for `report` (Maintenance's notice and its Check action). Its "Review missing links" runs
- * `onReviewMissing(trigger)`: Maintenance reviews a fresh Missing-file links plan (S7), never a cleanup; without one it
- * goes to Maintenance.
+ * `onReviewMissing(trigger, pressed)`: Maintenance reviews a fresh Missing-file links plan (S7), never a cleanup,
+ * holding the pressed button while it is read; without one it goes to Maintenance.
 */
 export function openHealthReport(report, { scope, trigger = null, navigate = navigateTo, onReviewMissing = null }) {
    openContentDialog({ scope, title: 'Health report', trigger, nodes: healthNodes(report, { scope,
-        onReviewMissing: () => {
-            if (onReviewMissing) onReviewMissing(trigger);
+        onReviewMissing: pressed => {
+            if (onReviewMissing) onReviewMissing(trigger, pressed);
            else { closeWorkspaceDialog(); navigate('#/system/maintenance'); }
        } }) });
 }