From 859846c75fe87a6d8f9b0eb3505df7768a1bb595 Mon Sep 17 00:00:00 2001 From: DeyangChan Date: Sun, 9 Aug 2026 22:54:56 +0800 Subject: [PATCH 1/9] Quotes are escaped, and a save writes only to its own comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The workspace escaped & < and > but not quotes, while most of what it builds is an HTML attribute — so a quote in a reviewer's own words ended the attribute early. The story map and the build board escape the ids they interpolate for the same reason. A saved comment's fields were copied across by name, so a name like __proto__ reached the prototype instead of the object. The update check kept its cache at a fixed path in the shared temp directory, which another account on the machine can create first. Co-Authored-By: Claude Opus 5 (1M context) --- .github/scripts/changelog.mjs | 35 +++++++++++++++++++ .github/scripts/check-version.mjs | 4 +-- .github/scripts/publish-release.mjs | 16 +++------ CHANGELOG.md | 27 ++++++++++++++ e2e/support/world.mjs | 4 ++- plugins/vstack/.claude-plugin/plugin.json | 2 +- plugins/vstack/.codex-plugin/plugin.json | 2 +- .../phase-build/assets/build-board.html | 6 ++-- .../assets/check-subtraction.mjs | 21 ++++++++--- .../experimental/spec/assets/spec-tree.html | 6 ++-- .../start/assets/chooser-server.mjs | 5 ++- .../experimental/start/assets/chooser.html | 4 +-- plugins/vstack/lib/json-bridge.mjs | 4 ++- plugins/vstack/lib/shell/shell.js | 4 +-- plugins/vstack/lib/update-check.mjs | 26 +++++++++----- .../skills/review/assets/harvest-reference.js | 2 +- .../skills/review/assets/review-server.mjs | 17 +++++++-- .../skills/review/assets/workspace.html | 8 +++-- .../skills/review/tests/update-check.mjs | 3 +- .../assets/story-map-template.html | 34 +++++++++--------- 20 files changed, 164 insertions(+), 66 deletions(-) create mode 100644 .github/scripts/changelog.mjs diff --git a/.github/scripts/changelog.mjs b/.github/scripts/changelog.mjs new file mode 100644 index 0000000..11b624b --- /dev/null +++ b/.github/scripts/changelog.mjs @@ -0,0 +1,35 @@ +/* + * Finding a release's section in CHANGELOG.md. + * + * Both the pull-request check and the release publisher need the same answer, + * so the heading is recognised in one place. Matching is done on plain strings + * rather than by building a pattern out of the version: a version is data, and + * a pattern built from data is only ever as correct as its escaping. + */ + +/** True when this line is the heading for exactly this version. A heading runs + * `## 6.4.0 — 2026-08-09`, so anything may follow the number as long as the + * number itself ends there — `## 6.4.01` is a different release. */ +export const isHeadingFor = (line, version) => { + const heading = `## ${version}` + if (!line.startsWith(heading)) return false + const next = line.slice(heading.length)[0] + return next === undefined || !(next === "." || (next >= "0" && next <= "9")) +} + +/** The line index of that heading, or -1. */ +export const headingIndex = (lines, version) => + lines.findIndex(line => isHeadingFor(line, version)) + +/** + * Everything under this version's heading, up to the next release heading. + * Null when the changelog has no entry for it. + */ +export const sectionFor = (changelog, version) => { + const lines = changelog.split("\n") + const start = headingIndex(lines, version) + if (start === -1) return null + const rest = lines.slice(start + 1) + const next = rest.findIndex(line => line.startsWith("## ")) + return (next === -1 ? rest : rest.slice(0, next)).join("\n").trim() +} diff --git a/.github/scripts/check-version.mjs b/.github/scripts/check-version.mjs index b5bee4b..27eb8ca 100644 --- a/.github/scripts/check-version.mjs +++ b/.github/scripts/check-version.mjs @@ -16,6 +16,7 @@ */ import { execFileSync } from "node:child_process" import { readFileSync } from "node:fs" +import { headingIndex } from "./changelog.mjs" const MANIFEST = "plugins/vstack/.claude-plugin/plugin.json" const CHANGELOG = "CHANGELOG.md" @@ -80,8 +81,7 @@ if (previous !== null && !isHigher(parse(declared, MANIFEST), parse(previous, `$ ) } -const heading = new RegExp(`^## ${declared.replace(/\./g, "\\.")}\\b`, "m") -if (!heading.test(readFileSync(CHANGELOG, "utf8"))) { +if (headingIndex(readFileSync(CHANGELOG, "utf8").split("\n"), declared) === -1) { fail( `${CHANGELOG} has no entry for ${declared}, and that entry is published as the release notes.`, "", diff --git a/.github/scripts/publish-release.mjs b/.github/scripts/publish-release.mjs index bfe28f8..39334cb 100644 --- a/.github/scripts/publish-release.mjs +++ b/.github/scripts/publish-release.mjs @@ -15,6 +15,7 @@ */ import { execFileSync } from "node:child_process" import { readFileSync } from "node:fs" +import { sectionFor } from "./changelog.mjs" const MANIFEST = "plugins/vstack/.claude-plugin/plugin.json" const CHANGELOG = "CHANGELOG.md" @@ -32,23 +33,16 @@ try { // No release under that tag yet, which is the case this runs for. } -// Everything from this version's heading up to the next one. Written by a -// person, so it is published as-is rather than regenerated from commits. -const changelog = readFileSync(CHANGELOG, "utf8") -const heading = new RegExp(`^## ${version.replace(/\./g, "\\.")}\\b.*$`, "m") -const start = changelog.search(heading) +// Everything under this version's heading. Written by a person, so it is +// published as-is rather than regenerated from commits. +const notes = sectionFor(readFileSync(CHANGELOG, "utf8"), version) -if (start === -1) { +if (notes === null) { console.error(`${CHANGELOG} has no entry for ${version}, so there are no notes to publish.`) console.error("A pull request cannot merge without one, so this commit did not come through one.") process.exit(1) } -const rest = changelog.slice(start) -const nextRelease = rest.indexOf("\n## ", 1) -const section = (nextRelease === -1 ? rest : rest.slice(0, nextRelease)).trim() -const notes = section.slice(section.indexOf("\n") + 1).trim() - gh( "release", "create", tag, "--target", process.env.GITHUB_SHA, diff --git a/CHANGELOG.md b/CHANGELOG.md index 7072bb7..eef8bca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,33 @@ The version in `plugins/vstack/.claude-plugin/plugin.json` is what your host compares against to decide an update is available. See the release checklist in [`CONTRIBUTING.md`](CONTRIBUTING.md). +## 6.4.1 — 2026-08-09 + +**Fixed** + +- **A comment containing a double quote could break the page it was drawn on.** + The workspace escaped `&`, `<` and `>` but not quotes, and most of what it + builds is an HTML attribute — so a quote in your own words ended the attribute + early and the rest of the note was read as markup. Quotes are now escaped + everywhere the workspace writes them. The story map and the build board escape + the ids they put in attributes for the same reason. +- **A save could change every object in the review server, not just its own + comment.** A saved comment's fields were copied across by name, and a name + like `__proto__` reaches the prototype rather than the object. Those names are + now skipped. +- **The update check no longer keeps its cache in the shared temp directory.** + On a machine with more than one account, anyone could create that file first + and own what the check then wrote to it. It now lives in `~/.vstack/`, owned + by the reader and readable only by them. The move resets what the old cache + held, so a release you had already dismissed can ask once more. +- **A failed action shows what went wrong without the stack behind it.** The + message is the part a reader can act on and is still shown in full. +- **A second session starting at the same moment cannot reset the other's + counter.** The bridge's sequence file is now created in one step rather than + checked and then written. +- The phase-preview comparison reads `` and `` as the closing + tags they are, and strips nested comment markers until none are left. + ## 6.4.0 — 2026-08-09 **Changed** diff --git a/e2e/support/world.mjs b/e2e/support/world.mjs index 2bb70d6..6483109 100644 --- a/e2e/support/world.mjs +++ b/e2e/support/world.mjs @@ -88,9 +88,11 @@ export class ReviewWorld { this.live = true this.name = 'testapp' const appPort = this.port + 1 + const escapeHtml = s => String(s).replace(/[&<>"']/g, c => + ({ '&': '&', '<': '<', '>': '>', '"': '"', "'": ''' }[c])) this.app = http.createServer((req, res) => { res.writeHead(200, { 'content-type': 'text/html' }) - res.end(`Fixture

${req.url}

Settings`) + res.end(`Fixture

${escapeHtml(req.url)}

Settings`) }) await new Promise(resolve => this.app.listen(appPort, '127.0.0.1', resolve)) this.appOrigin = `http://127.0.0.1:${appPort}` diff --git a/plugins/vstack/.claude-plugin/plugin.json b/plugins/vstack/.claude-plugin/plugin.json index 12e92ac..b045006 100644 --- a/plugins/vstack/.claude-plugin/plugin.json +++ b/plugins/vstack/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "vstack", "displayName": "Visual Stack", - "version": "6.4.0", + "version": "6.4.1", "description": "Stop prompting. Start pointing. Visual Stack adds a Figma-like feedback layer to Claude Code. Create a new screen from a prompt, screenshot, reference site, or design system, or open an app you already have running. Click any element and leave feedback exactly where the problem is hiding, and Claude publishes the next revision into the same workspace. Compare revisions on a timeline, preview desktop, tablet, and mobile layouts, and keep every comment attached to the element, route, and version it refers to. Wireframes are self-contained HTML and review state stays in your project.", "author": { "name": "Cavalry Collective", diff --git a/plugins/vstack/.codex-plugin/plugin.json b/plugins/vstack/.codex-plugin/plugin.json index 87767f4..a0f062f 100644 --- a/plugins/vstack/.codex-plugin/plugin.json +++ b/plugins/vstack/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "vstack", - "version": "6.4.0", + "version": "6.4.1", "description": "Stop prompting. Start pointing. Visual Stack adds a Figma-like feedback layer to Codex. Create a new screen from a prompt, screenshot, reference site, or design system, or open an app you already have running. Click any element and leave feedback exactly where the problem is hiding, and Codex publishes the next revision into the same workspace. Compare revisions on a timeline, preview desktop, tablet, and mobile layouts, and keep every comment attached to the element, route, and version it refers to. Wireframes are self-contained HTML and review state stays in your project.", "author": { "name": "Cavalry Collective", diff --git a/plugins/vstack/experimental/phase-build/assets/build-board.html b/plugins/vstack/experimental/phase-build/assets/build-board.html index 26538bf..c7b8aef 100644 --- a/plugins/vstack/experimental/phase-build/assets/build-board.html +++ b/plugins/vstack/experimental/phase-build/assets/build-board.html @@ -722,7 +722,7 @@ stacks in the order things entered it: one promoted before a dialog would sit under it. Older browsers have no popover and lose nothing but the stacking. */ - try { el.showPopover() } catch {} + try { el.showPopover(); } catch {} el.classList.add('on'); clearTimeout(toastTimer); toastTimer = setTimeout(() => { @@ -856,7 +856,7 @@ // `defaultLang` is what the artifact was authored in — it opens that way // once, and after that the reader's own choice is the one that sticks. if (opts.defaultLang && !storedLang) lang = opts.defaultLang === 'zh' ? 'zh' : 'en'; - if (opts.lang) { lang = opts.lang === 'zh' ? 'zh' : 'en'; store.set(KEY.lang, lang) } + if (opts.lang) { lang = opts.lang === 'zh' ? 'zh' : 'en'; store.set(KEY.lang, lang); } // Not every page sends something back — the form doesn't — so the primary // action stays out of the bar unless a page asks for it. const send = $('#send'); @@ -1077,7 +1077,7 @@ const all = layout(); const mono = doc.subjects[subject].mono ? 'mono' : ''; NODES().innerHTML = all.map(n => ` -
${n.status === 'done' ? '✓' : n.status === 'failed' ? '!' diff --git a/plugins/vstack/experimental/phase-preview/assets/check-subtraction.mjs b/plugins/vstack/experimental/phase-preview/assets/check-subtraction.mjs index d849d2c..c8242ac 100644 --- a/plugins/vstack/experimental/phase-preview/assets/check-subtraction.mjs +++ b/plugins/vstack/experimental/phase-preview/assets/check-subtraction.mjs @@ -54,17 +54,30 @@ const phaseHtml = fs.readFileSync(phasePath, 'utf8') // ── parsing ─────────────────────────────────────────────────────────────────── -const stripComments = (html) => html.replace(//g, '') +/** One pass can leave a `/g, '') + } + return out +} + +/* A closing tag may carry whitespace before its `>`. `` ends a style + block just as `` does, so the patterns below allow it — one that does + not would read the rest of the document as stylesheet. */ /** Concatenated contents of every ') + .replace(/]*>[\s\S]*?<\/script\s*>/gi, '') + .replace(/]*>[\s\S]*?<\/style\s*>/gi, '') const OPEN_TAG = /<([a-zA-Z][a-zA-Z0-9-]*)((?:"[^"]*"|'[^']*'|[^>"'])*)>/g diff --git a/plugins/vstack/experimental/spec/assets/spec-tree.html b/plugins/vstack/experimental/spec/assets/spec-tree.html index 5947e67..7df863e 100644 --- a/plugins/vstack/experimental/spec/assets/spec-tree.html +++ b/plugins/vstack/experimental/spec/assets/spec-tree.html @@ -951,7 +951,7 @@

stacks in the order things entered it: one promoted before a dialog would sit under it. Older browsers have no popover and lose nothing but the stacking. */ - try { el.showPopover() } catch {} + try { el.showPopover(); } catch {} el.classList.add('on'); clearTimeout(toastTimer); toastTimer = setTimeout(() => { @@ -1085,7 +1085,7 @@

// `defaultLang` is what the artifact was authored in — it opens that way // once, and after that the reader's own choice is the one that sticks. if (opts.defaultLang && !storedLang) lang = opts.defaultLang === 'zh' ? 'zh' : 'en'; - if (opts.lang) { lang = opts.lang === 'zh' ? 'zh' : 'en'; store.set(KEY.lang, lang) } + if (opts.lang) { lang = opts.lang === 'zh' ? 'zh' : 'en'; store.set(KEY.lang, lang); } // Not every page sends something back — the form doesn't — so the primary // action stays out of the bar unless a page asks for it. const send = $('#send'); @@ -1684,7 +1684,7 @@

try { v.json = JSON.stringify(await (await fetch(`${BRIDGE.history}/${n}?t=${BRIDGE.token}`)).json()); v.fetched = true; - } catch { syncTimeline(); return } + } catch { syncTimeline(); return; } } if (my !== navSeq || !v.json) return; // scrubbed past while it loaded doc = migrate(JSON.parse(v.json)); diff --git a/plugins/vstack/experimental/start/assets/chooser-server.mjs b/plugins/vstack/experimental/start/assets/chooser-server.mjs index 1d0b39d..a59e392 100644 --- a/plugins/vstack/experimental/start/assets/chooser-server.mjs +++ b/plugins/vstack/experimental/start/assets/chooser-server.mjs @@ -288,9 +288,12 @@ const server = http.createServer((req, res) => { fs.writeFileSync(OUT, JSON.stringify(record, null, 2)) send(res, 200, 'application/json', '{"ok":true}') + // Names come from the page, and a line break in one would read as a + // second line of output that nothing here wrote. + const oneLine = s => String(s).replace(/[\r\n]+/g, ' ') const what = record.skipDev ? 'specs & design only — development skipped' : MODE === 'existing' ? 'existing project recorded' - : `${record.pack}` + (record.addons.length ? ` + ${record.addons.join(', ')}` : ' (no add-ons)') + : oneLine(record.pack) + (record.addons.length ? ` + ${record.addons.map(oneLine).join(', ')}` : ' (no add-ons)') console.log(`\n✓ ${what}` + (record.deleting.packs.length + record.deleting.addons.length ? `\n deleting ${record.deleting.packs.length} pack(s) and ${record.deleting.addons.length} add-on(s)` + diff --git a/plugins/vstack/experimental/start/assets/chooser.html b/plugins/vstack/experimental/start/assets/chooser.html index 94e337e..6e3b291 100644 --- a/plugins/vstack/experimental/start/assets/chooser.html +++ b/plugins/vstack/experimental/start/assets/chooser.html @@ -823,7 +823,7 @@

stacks in the order things entered it: one promoted before a dialog would sit under it. Older browsers have no popover and lose nothing but the stacking. */ - try { el.showPopover() } catch {} + try { el.showPopover(); } catch {} el.classList.add('on'); clearTimeout(toastTimer); toastTimer = setTimeout(() => { @@ -957,7 +957,7 @@

// `defaultLang` is what the artifact was authored in — it opens that way // once, and after that the reader's own choice is the one that sticks. if (opts.defaultLang && !storedLang) lang = opts.defaultLang === 'zh' ? 'zh' : 'en'; - if (opts.lang) { lang = opts.lang === 'zh' ? 'zh' : 'en'; store.set(KEY.lang, lang) } + if (opts.lang) { lang = opts.lang === 'zh' ? 'zh' : 'en'; store.set(KEY.lang, lang); } // Not every page sends something back — the form doesn't — so the primary // action stays out of the bar unless a page asks for it. const send = $('#send'); diff --git a/plugins/vstack/lib/json-bridge.mjs b/plugins/vstack/lib/json-bridge.mjs index 212c745..4ff3f91 100644 --- a/plugins/vstack/lib/json-bridge.mjs +++ b/plugins/vstack/lib/json-bridge.mjs @@ -185,7 +185,9 @@ const someoneWatching = () => watchingRecently(WATCH_FILE) const HIST_DIR = path.join(BRIDGE_DIR, STEM + '.history') const HIST_INDEX = path.join(HIST_DIR, 'index.json') fs.mkdirSync(BRIDGE_DIR, { recursive: true }) -if (!fs.existsSync(SEQ_FILE)) fs.writeFileSync(SEQ_FILE, '0') +// Created only if it is not there, in one step: a second session starting at +// the same moment must not reset a counter the first one is already using. +try { fs.writeFileSync(SEQ_FILE, '0', { flag: 'wx' }) } catch {} // A verdict belongs to the round that raised it — a new link starts unsigned, // or the first waiter it arms fires on last week's approval. fs.rmSync(APPROVED_FILE, { force: true }) diff --git a/plugins/vstack/lib/shell/shell.js b/plugins/vstack/lib/shell/shell.js index cc1ac10..8ca83b8 100644 --- a/plugins/vstack/lib/shell/shell.js +++ b/plugins/vstack/lib/shell/shell.js @@ -181,7 +181,7 @@ window.VSShell = (function () { stacks in the order things entered it: one promoted before a dialog would sit under it. Older browsers have no popover and lose nothing but the stacking. */ - try { el.showPopover() } catch {} + try { el.showPopover(); } catch {} el.classList.add('on'); clearTimeout(toastTimer); toastTimer = setTimeout(() => { @@ -315,7 +315,7 @@ window.VSShell = (function () { // `defaultLang` is what the artifact was authored in — it opens that way // once, and after that the reader's own choice is the one that sticks. if (opts.defaultLang && !storedLang) lang = opts.defaultLang === 'zh' ? 'zh' : 'en'; - if (opts.lang) { lang = opts.lang === 'zh' ? 'zh' : 'en'; store.set(KEY.lang, lang) } + if (opts.lang) { lang = opts.lang === 'zh' ? 'zh' : 'en'; store.set(KEY.lang, lang); } // Not every page sends something back — the form doesn't — so the primary // action stays out of the bar unless a page asks for it. const send = $('#send'); diff --git a/plugins/vstack/lib/update-check.mjs b/plugins/vstack/lib/update-check.mjs index 0093297..be45fdc 100644 --- a/plugins/vstack/lib/update-check.mjs +++ b/plugins/vstack/lib/update-check.mjs @@ -63,11 +63,23 @@ const MARKET = 'cavalry-collective' // .claude-plugin/marketplace.json → nam const PLUGIN = 'vstack' // the marketplace entry's name const INSTALLS = path.join(os.homedir(), '.claude', 'plugins', 'installed_plugins.json') const CODEX_CACHE = path.join(os.homedir(), '.codex', 'plugins', 'cache', MARKET, PLUGIN) -const CACHE = path.join(os.tmpdir(), 'vstack-update-check.json') +// The reader's own directory, not the shared temp dir: on a multi-user machine +// anyone can create a file there first and own what this then writes to. +const CACHE_DIR = path.join(os.homedir(), '.vstack') +const CACHE = path.join(CACHE_DIR, 'update-check.json') const TTL_MS = 6 * 60 * 60 * 1000 const TIMEOUT_MS = 2500 const readJSON = f => { try { return JSON.parse(fs.readFileSync(f, 'utf8')) } catch { return null } } + +/** Merge into the cache, or carry on without it. Nothing here is worth failing + * a server start for: the cache only saves a question being asked again. */ +const writeCache = fields => { + try { + fs.mkdirSync(CACHE_DIR, { recursive: true }) + fs.writeFileSync(CACHE, JSON.stringify({ ...(readJSON(CACHE) || {}), ...fields }), { mode: 0o600 }) + } catch {} +} const short = sha => String(sha || '').slice(0, 7) /** Whatever plugin.json still declares — normally nothing, by design. */ @@ -142,9 +154,7 @@ async function ask (kind) { // Cache the answer either way: a repo that has not moved should not be // asked again every time a server starts. Merged rather than replaced — // `met` is a different question and outlives any one answer. - try { - fs.writeFileSync(CACHE, JSON.stringify({ ...(readJSON(CACHE) || {}), at: Date.now(), kind, value })) - } catch {} + writeCache({ at: Date.now(), kind, value }) return value } catch { return cached?.kind === kind ? cached.value ?? null : null @@ -171,9 +181,8 @@ const say = (key, title) => ({ pill: 'update', key, title }) * starts empty every time and a first sighting would be all there ever was. */ function firstSighting (key) { - const cache = readJSON(CACHE) || {} - if (cache.met === key) return false - try { fs.writeFileSync(CACHE, JSON.stringify({ ...cache, met: key })) } catch {} + if ((readJSON(CACHE) || {}).met === key) return false + writeCache({ met: key }) return true } @@ -188,8 +197,7 @@ function firstSighting (key) { */ export function dismissUpdate (key) { if (!key) return - const cache = readJSON(CACHE) || {} - try { fs.writeFileSync(CACHE, JSON.stringify({ ...cache, seen: String(key) })) } catch {} + writeCache({ seen: String(key) }) } /** diff --git a/plugins/vstack/skills/review/assets/harvest-reference.js b/plugins/vstack/skills/review/assets/harvest-reference.js index 320ece5..17732fd 100644 --- a/plugins/vstack/skills/review/assets/harvest-reference.js +++ b/plugins/vstack/skills/review/assets/harvest-reference.js @@ -67,7 +67,7 @@ // reports `currentColor`, which would file every text colour as a border. for (const side of ['Top', 'Bottom', 'Left']) { const w = px(c['border' + side + 'Width']); - if (w && c['border' + side + 'Style'] !== 'none') { bump('border', `${w}px ${c['border' + side + 'Color']}`, r.width); break } + if (w && c['border' + side + 'Style'] !== 'none') { bump('border', `${w}px ${c['border' + side + 'Color']}`, r.width); break; } } if (px(c.borderTopLeftRadius)) bump('radius', c.borderTopLeftRadius, area); if (c.boxShadow !== 'none') bump('shadow', c.boxShadow, area); diff --git a/plugins/vstack/skills/review/assets/review-server.mjs b/plugins/vstack/skills/review/assets/review-server.mjs index 821e8b1..227cd10 100644 --- a/plugins/vstack/skills/review/assets/review-server.mjs +++ b/plugins/vstack/skills/review/assets/review-server.mjs @@ -311,6 +311,11 @@ function saveComments (comments, subject = here()) { * still holds, and none of these ever. */ const OWNED = ['state', 'sentAt', 'deliveredAt', 'deliveredTo', 'dismissedAt'] +/** Keys that reach the prototype rather than the object. `JSON.parse` keeps + * them as ordinary own properties, so a save can carry one in, and assigning + * it would change every object in the process rather than this comment. */ +const RESERVED = ['__proto__', 'constructor', 'prototype'] + const normaliseComment = c => ({ ...c, state: c.state === 'closed' ? 'closed' : 'open', @@ -1238,6 +1243,11 @@ const send = (res, code, body, type = 'text/plain; charset=utf-8') => { } const sendJSON = (res, code, obj) => send(res, code, JSON.stringify(obj), MIME['.json']) +/** What a failed action has to say, for a reader. The message is the useful + * part and the workspace shows it; the stack behind it is noise the page has + * no use for, so only the message crosses. */ +const failureText = e => e?.message || 'The review server could not complete that.' + function readBody (req) { return new Promise((resolve, reject) => { let raw = '' @@ -1337,7 +1347,8 @@ function acceptFromReviewer (incoming) { // a draft and the reviewer may rewrite it however they like. if (!stored.sentAt) { for (const [key, value] of Object.entries(raw)) { - if (!OWNED.includes(key) && key !== 'replies') stored[key] = value + if (OWNED.includes(key) || RESERVED.includes(key) || key === 'replies') continue + stored[key] = value } } stored.replies = replies @@ -1549,7 +1560,7 @@ async function handle (req, res) { if (p === '/' || p === '/index.html') return serveWorkspace(res) if (p === '/page' && !LIVE) return serveStatic(res, FILE) if (p === '/api/project') { - try { return sendJSON(res, 200, payload()) } catch (e) { return sendJSON(res, 500, { error: String(e) }) } + try { return sendJSON(res, 200, payload()) } catch (e) { return sendJSON(res, 500, { error: failureText(e) }) } } if (p === '/api/events') { res.writeHead(200, { 'Content-Type': 'text/event-stream', 'Cache-Control': 'no-cache', Connection: 'keep-alive' }) @@ -1806,7 +1817,7 @@ async function cmdServe () { fs.rmSync(P.share(), { force: true }) const port = Number(args.port || 7788) const server = http.createServer((req, res) => { - handle(req, res).catch(e => { try { sendJSON(res, 500, { error: String(e) }) } catch {} }) + handle(req, res).catch(e => { try { sendJSON(res, 500, { error: failureText(e) }) } catch {} }) }) if (LIVE) server.on('upgrade', (req, socket, head) => { if (req.url.startsWith(BASE + '/') || req.url === BASE) return socket.destroy() diff --git a/plugins/vstack/skills/review/assets/workspace.html b/plugins/vstack/skills/review/assets/workspace.html index f01bd0a..c21516d 100644 --- a/plugins/vstack/skills/review/assets/workspace.html +++ b/plugins/vstack/skills/review/assets/workspace.html @@ -1513,7 +1513,7 @@

stacks in the order things entered it: one promoted before a dialog would sit under it. Older browsers have no popover and lose nothing but the stacking. */ - try { el.showPopover() } catch {} + try { el.showPopover(); } catch {} el.classList.add('on'); clearTimeout(toastTimer); toastTimer = setTimeout(() => { @@ -1647,7 +1647,7 @@

// `defaultLang` is what the artifact was authored in — it opens that way // once, and after that the reader's own choice is the one that sticks. if (opts.defaultLang && !storedLang) lang = opts.defaultLang === 'zh' ? 'zh' : 'en'; - if (opts.lang) { lang = opts.lang === 'zh' ? 'zh' : 'en'; store.set(KEY.lang, lang) } + if (opts.lang) { lang = opts.lang === 'zh' ? 'zh' : 'en'; store.set(KEY.lang, lang); } // Not every page sends something back — the form doesn't — so the primary // action stays out of the bar unless a page asks for it. const send = $('#send'); @@ -1803,7 +1803,9 @@

const $ = (s, r = document) => r.querySelector(s); const $$ = (s, r = document) => [...r.querySelectorAll(s)]; const clamp = (v, a, b) => Math.max(a, Math.min(b, v)); -const esc = s => String(s ?? '').replace(/[&<>]/g, c => ({ '&': '&', '<': '<', '>': '>' }[c])); +// Quotes are escaped as well as angle brackets. Most of what this builds is an +// attribute value, and a reviewer's own words end up inside one. +const esc = s => String(s ?? '').replace(/[&<>"']/g, c => ({ '&': '&', '<': '<', '>': '>', '"': '"', "'": ''' }[c])); const uid = () => 'c' + Math.random().toString(36).slice(2, 8); /* One severity: everything the reviewer writes is something to do. The mark diff --git a/plugins/vstack/skills/review/tests/update-check.mjs b/plugins/vstack/skills/review/tests/update-check.mjs index ce244c1..819148f 100644 --- a/plugins/vstack/skills/review/tests/update-check.mjs +++ b/plugins/vstack/skills/review/tests/update-check.mjs @@ -37,7 +37,8 @@ for (const part of ['lib', 'host-profiles', '.claude-plugin']) { fs.cpSync(path.join(PLUGIN, part), path.join(installed, part), { recursive: true }) } -const cache = path.join(process.env.TMPDIR, 'vstack-update-check.json') +const cache = path.join(process.env.HOME, '.vstack', 'update-check.json') +fs.mkdirSync(path.dirname(cache), { recursive: true }) const seed = extra => fs.writeFileSync(cache, JSON.stringify({ at: Date.now(), kind: 'version', value: LATEST, ...extra })) seed({}) diff --git a/plugins/vstack/skills/user-story-map/assets/story-map-template.html b/plugins/vstack/skills/user-story-map/assets/story-map-template.html index 9e3b089..002e649 100644 --- a/plugins/vstack/skills/user-story-map/assets/story-map-template.html +++ b/plugins/vstack/skills/user-story-map/assets/story-map-template.html @@ -838,7 +838,7 @@

stacks in the order things entered it: one promoted before a dialog would sit under it. Older browsers have no popover and lose nothing but the stacking. */ - try { el.showPopover() } catch {} + try { el.showPopover(); } catch {} el.classList.add('on'); clearTimeout(toastTimer); toastTimer = setTimeout(() => { @@ -972,7 +972,7 @@

// `defaultLang` is what the artifact was authored in — it opens that way // once, and after that the reader's own choice is the one that sticks. if (opts.defaultLang && !storedLang) lang = opts.defaultLang === 'zh' ? 'zh' : 'en'; - if (opts.lang) { lang = opts.lang === 'zh' ? 'zh' : 'en'; store.set(KEY.lang, lang) } + if (opts.lang) { lang = opts.lang === 'zh' ? 'zh' : 'en'; store.set(KEY.lang, lang); } // Not every page sends something back — the form doesn't — so the primary // action stays out of the bar unless a page asks for it. const send = $('#send'); @@ -1276,27 +1276,27 @@

acts.forEach(a=>{ // The activity is its name. A second line explaining it turned the // backbone into prose, and the stories under it already say what it means. - h += `
+ h += `
⠿ ⠿ -
${esc(a.name)} -
`; +
${esc(a.name)} +
`; }); h += `
`; phases.forEach((p,pi)=>{ const pc = "p"+(pi%6); - h += `
+ h += `
⠿ - ${esc(p.name)} + ${esc(p.name)} - ${esc(p.goal||"")} -
`; + ${esc(p.goal||"")} +
`; acts.forEach(a=>{ const cards = state.stories.filter(s=>s.phase===p.id && s.activity===a.id); - h += `
`; + h += `
`; cards.forEach(s=>{ h += cardHtml(s); }); - h += `
`; + h += `
`; }); - h += `
`; + h += `
`; }); h += `
`; map.innerHTML = h; @@ -1321,14 +1321,14 @@

function cardHtml(s){ const pills = s.tags.map(tagPill).join(""); const on = sel.has(s.id); - return `
+ return `
⠿ - - - + + +
-

${esc(s.text)}

+

${esc(s.text)}

${pills?`${pills}`:""}
`; } From dbdc15aeae0c81c1f08d33fb40e5e9f390f4993e Mon Sep 17 00:00:00 2001 From: DeyangChan Date: Sun, 9 Aug 2026 23:00:32 +0800 Subject: [PATCH 2/9] A draft is rebuilt from the payload, and a closing tag ends at its bracket MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copying the payload's fields onto the stored comment one name at a time let a name like __proto__ reach the prototype, and a blocklist still leaves the write arbitrary. The unsent draft is now rebuilt by spread, which creates own properties and follows no setter. An end tag carries anything up to its `>` — `` closes a script — so the phase-preview patterns read to the bracket. Co-Authored-By: Claude Opus 5 (1M context) --- .../assets/check-subtraction.mjs | 13 +++++----- .../skills/review/assets/review-server.mjs | 26 +++++++++---------- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/plugins/vstack/experimental/phase-preview/assets/check-subtraction.mjs b/plugins/vstack/experimental/phase-preview/assets/check-subtraction.mjs index c8242ac..d7ad40a 100644 --- a/plugins/vstack/experimental/phase-preview/assets/check-subtraction.mjs +++ b/plugins/vstack/experimental/phase-preview/assets/check-subtraction.mjs @@ -65,19 +65,20 @@ const stripComments = (html) => { return out } -/* A closing tag may carry whitespace before its `>`. `` ends a style - block just as `` does, so the patterns below allow it — one that does - not would read the rest of the document as stylesheet. */ +/* A closing tag ends at its `>`, not at the tag name: `` closes a + style block just as `` does, because a parser reads and discards + whatever sits between. A pattern that stops at the name misses those and + reads the rest of the document as stylesheet. */ /** Concatenated contents of every ') + .replace(/]*>[\s\S]*?<\/script\b[^>]*>/gi, '') + .replace(/]*>[\s\S]*?<\/style\b[^>]*>/gi, '') const OPEN_TAG = /<([a-zA-Z][a-zA-Z0-9-]*)((?:"[^"]*"|'[^']*'|[^>"'])*)>/g diff --git a/plugins/vstack/skills/review/assets/review-server.mjs b/plugins/vstack/skills/review/assets/review-server.mjs index 227cd10..50e6267 100644 --- a/plugins/vstack/skills/review/assets/review-server.mjs +++ b/plugins/vstack/skills/review/assets/review-server.mjs @@ -311,11 +311,6 @@ function saveComments (comments, subject = here()) { * still holds, and none of these ever. */ const OWNED = ['state', 'sentAt', 'deliveredAt', 'deliveredTo', 'dismissedAt'] -/** Keys that reach the prototype rather than the object. `JSON.parse` keeps - * them as ordinary own properties, so a save can carry one in, and assigning - * it would change every object in the process rather than this comment. */ -const RESERVED = ['__proto__', 'constructor', 'prototype'] - const normaliseComment = c => ({ ...c, state: c.state === 'closed' ? 'closed' : 'open', @@ -1328,28 +1323,31 @@ function mergeReplies (stored = [], incoming = []) { */ function acceptFromReviewer (incoming) { const comments = loadComments() - const byId = new Map(comments.map(comment => [comment.id, comment])) + const indexById = new Map(comments.map((comment, i) => [comment.id, i])) const at = new Date().toISOString() for (const raw of incoming) { if (!raw?.id) continue - const stored = byId.get(raw.id) - if (!stored) { + const index = indexById.get(raw.id) + if (index === undefined) { const { state, deliveredAt, deliveredTo, ...rest } = raw const fresh = normaliseComment({ ...rest, state: 'open', deliveredAt: null, sentAt: raw.sentAt ? at : null }) - comments.push(fresh) - byId.set(fresh.id, fresh) + indexById.set(fresh.id, comments.push(fresh) - 1) continue } + let stored = comments[index] const replies = mergeReplies(stored.replies, raw.replies) const answered = replies.length > (stored.replies || []).length && replies.at(-1)?.by === REVIEWER_ROLE // The words are frozen once they are sent; before that the comment is still // a draft and the reviewer may rewrite it however they like. if (!stored.sentAt) { - for (const [key, value] of Object.entries(raw)) { - if (OWNED.includes(key) || RESERVED.includes(key) || key === 'replies') continue - stored[key] = value - } + // Spread rather than assign field by field. A payload can carry a key + // like `__proto__`, which `JSON.parse` keeps as an ordinary property but + // assignment would follow to the prototype, changing every object here. + const { replies: _thread, ...draft } = raw + for (const owned of OWNED) delete draft[owned] + stored = { ...stored, ...draft } + comments[index] = stored } stored.replies = replies if (!stored.sentAt && raw.sentAt) stored.sentAt = at From c58dbb7f6126b6779ca75b7f7dde437173b2334d Mon Sep 17 00:00:00 2001 From: DeyangChan Date: Sun, 9 Aug 2026 23:16:48 +0800 Subject: [PATCH 3/9] The Security and Scorecard badges are shown again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both report now: the security workflow passes on main, and the Scorecard badge renders a score rather than the invalid-repo-path error it used to. The two Sonar badges stay hidden — Sonar has not analysed the repository since v5.0.0, so the gate is not computed and the rating is stale. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 1f6556a..9136fcf 100644 --- a/README.md +++ b/README.md @@ -3,15 +3,14 @@ # Visual Stack [![CI](https://github.com/Cavalry-Collective/visual-stack/actions/workflows/ci.yml/badge.svg)](https://github.com/Cavalry-Collective/visual-stack/actions/workflows/ci.yml) +[![Security](https://github.com/Cavalry-Collective/visual-stack/actions/workflows/security.yml/badge.svg)](https://github.com/Cavalry-Collective/visual-stack/actions/workflows/security.yml) +[![OpenSSF Scorecard](https://api.scorecard.dev/projects/github.com/Cavalry-Collective/visual-stack/badge)](https://scorecard.dev/viewer/?uri=github.com/Cavalry-Collective/visual-stack) [![License: MIT](https://img.shields.io/badge/license-MIT-blue.svg)](LICENSE) From 14eee583ec8e53f66423c339c1f90ba4dad91bc9 Mon Sep 17 00:00:00 2001 From: DeyangChan Date: Sun, 9 Aug 2026 23:31:34 +0800 Subject: [PATCH 4/9] A local install's bookkeeping stays out of the repo Installing from this clone as a local marketplace, which is how the plugin is tested before it ships, leaves a .orphaned_at timestamp inside plugins/vstack/. It belongs to that machine's install, not to the plugin. Co-Authored-By: Claude Opus 5 (1M context) --- .gitignore | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.gitignore b/.gitignore index 2f1be16..40a0865 100644 --- a/.gitignore +++ b/.gitignore @@ -10,5 +10,10 @@ # Recording the README demo installs playwright-core at the repo root. node_modules/ +# A host writes this into the plugin directory when it installs from this clone +# as a local marketplace, which is how the plugin is tested before it ships. It +# is that machine's install bookkeeping, not part of the plugin. +plugins/vstack/.orphaned_at + .DS_Store .env From a7c0eec7e814809620eb18a3563327f1b9a8e94f Mon Sep 17 00:00:00 2001 From: DeyangChan Date: Mon, 10 Aug 2026 00:37:38 +0800 Subject: [PATCH 5/9] A scroll the page was not ready for is tried again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both scroll steps scrolled once and then polled for the result. Until the page under review has laid out there is nothing to scroll, and a scroll made at that moment is dropped rather than queued — so an attempt that lost the race could never recover, and the step failed for its full timeout instead of retrying. Against a box that becomes scrollable after 800ms, the old shape fails after the whole 5s budget and the new one passes at 866ms. Co-Authored-By: Claude Opus 5 (1M context) --- e2e/steps/browser.steps.mjs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/e2e/steps/browser.steps.mjs b/e2e/steps/browser.steps.mjs index e227c57..0e5cf4b 100644 --- a/e2e/steps/browser.steps.mjs +++ b/e2e/steps/browser.steps.mjs @@ -271,19 +271,30 @@ async function framedBox (world, selector) { return box } +/* Both scrolls are re-applied on every attempt rather than done once and then + waited on. Until the page under review has laid out there is nothing to + scroll, and a scroll made at that moment is dropped rather than queued — so + an attempt that observes without repeating the scroll can never recover. */ + When('the reviewer scrolls the framed page to the bottom', async function () { const framed = await framedWindow(this) - await framed.evaluate(() => window.scrollTo(0, document.body.scrollHeight)) await eventually(async () => { - assert.ok(await framed.evaluate(() => window.scrollY) > 0, 'the page did not scroll') + const y = await framed.evaluate(() => { + window.scrollTo(0, document.body.scrollHeight) + return window.scrollY + }) + assert.ok(y > 0, 'the page did not scroll') }, 'the page under review scrolls in its own window') }) When('the reviewer scrolls the canvas to the bottom', async function () { const port = this.browserPage.locator('#viewportBox') - await port.evaluate(element => { element.scrollTop = element.scrollHeight }) await eventually(async () => { - assert.ok(await port.evaluate(element => element.scrollTop) > 0, 'the canvas did not scroll') + const top = await port.evaluate(element => { + element.scrollTop = element.scrollHeight + return element.scrollTop + }) + assert.ok(top > 0, 'the canvas did not scroll') }, 'the canvas scrolls') }) From 4873e70456b748a7c7afd03d4f69f580151b982c Mon Sep 17 00:00:00 2001 From: DeyangChan Date: Mon, 10 Aug 2026 00:53:20 +0800 Subject: [PATCH 6/9] A scroll waits for the layout it is scrolling Both scroll steps ran before the workspace had sized the frame to the page it had loaded. There was nothing to scroll yet, and a scroll made then is dropped rather than queued, so the step failed for its whole timeout. They now wait for the size the workspace sets once the framed page has loaded, then scroll and confirm they reached the bottom rather than merely moved. Stopping at the first movement leaves the target part way up the canvas, where the click that follows cannot reach it. Co-Authored-By: Claude Opus 5 (1M context) --- e2e/steps/browser.steps.mjs | 52 ++++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 15 deletions(-) diff --git a/e2e/steps/browser.steps.mjs b/e2e/steps/browser.steps.mjs index 0e5cf4b..5976d35 100644 --- a/e2e/steps/browser.steps.mjs +++ b/e2e/steps/browser.steps.mjs @@ -274,28 +274,50 @@ async function framedBox (world, selector) { /* Both scrolls are re-applied on every attempt rather than done once and then waited on. Until the page under review has laid out there is nothing to scroll, and a scroll made at that moment is dropped rather than queued — so - an attempt that observes without repeating the scroll can never recover. */ + an attempt that observes without repeating the scroll can never recover. + + Each one waits to be at the bottom and to have stayed there, not merely to + have moved. The bottom keeps moving while the page lays out, and a step that + stops at the first sign of movement leaves what it scrolled to off screen, + where the click that follows cannot reach it. */ + +/** The workspace sizes the frame to the page once that page has loaded, so the + canvas has no final extent before then. Waiting for the size it set is what + tells a step the layout it is about to scroll or measure has arrived. */ +async function framedPageIsFitted (world) { + await eventually(async () => { + const height = await world.browserPage.locator('#frame').evaluate(frame => + frame.contentDocument?.readyState === 'complete' ? parseFloat(frame.style.height) || 0 : 0) + assert.ok(height > 0, 'the workspace has not sized the frame to the page') + }, 'the workspace sizes the frame to the page') +} + +/** Scroll to the bottom and confirm it got there. `reach` scrolls and reports + where it landed and where the bottom is, in the same evaluation. */ +async function scrolledToTheBottom (reach, what) { + await eventually(async () => { + const { at, end } = await reach() + assert.ok(end > 0, `${what} did not scroll`) + assert.ok(Math.abs(at - end) <= 1, `${what} is not at the bottom`) + }, `${what} scrolls to the bottom`) +} When('the reviewer scrolls the framed page to the bottom', async function () { + await framedPageIsFitted(this) const framed = await framedWindow(this) - await eventually(async () => { - const y = await framed.evaluate(() => { - window.scrollTo(0, document.body.scrollHeight) - return window.scrollY - }) - assert.ok(y > 0, 'the page did not scroll') - }, 'the page under review scrolls in its own window') + await scrolledToTheBottom(() => framed.evaluate(() => { + window.scrollTo(0, document.body.scrollHeight) + return { at: window.scrollY, end: document.documentElement.scrollHeight - window.innerHeight } + }), 'the page under review') }) When('the reviewer scrolls the canvas to the bottom', async function () { + await framedPageIsFitted(this) const port = this.browserPage.locator('#viewportBox') - await eventually(async () => { - const top = await port.evaluate(element => { - element.scrollTop = element.scrollHeight - return element.scrollTop - }) - assert.ok(top > 0, 'the canvas did not scroll') - }, 'the canvas scrolls') + await scrolledToTheBottom(() => port.evaluate(element => { + element.scrollTop = element.scrollHeight + return { at: element.scrollTop, end: element.scrollHeight - element.clientHeight } + }), 'the canvas') }) When('the reviewer clicks {string} in the framed page and writes {string}', From 83c839c99f2cba5f9640e02f1a4119a4fdb010d8 Mon Sep 17 00:00:00 2001 From: DeyangChan Date: Mon, 10 Aug 2026 17:50:05 +0800 Subject: [PATCH 7/9] Review comments are handled one at a time The review server owns a single active FIFO slot. A question releases that slot, and the answered thread returns to the queue without interrupting the comment already in progress. Codex uses bounded pull and claim delivery while Claude and Grok retain push delivery under the same queue semantics. --- CHANGELOG.md | 35 ++ README.md | 4 +- docs/assets/review-lifecycle.svg | 22 +- e2e/features/linking.feature | 8 + e2e/features/sending.feature | 14 +- e2e/steps/linking.steps.mjs | 25 ++ e2e/test-plan.html | 16 +- plugins/vstack/.claude-plugin/plugin.json | 2 +- plugins/vstack/.codex-plugin/plugin.json | 2 +- plugins/vstack/contracts/host.md | 39 ++- plugins/vstack/contracts/host.schema.json | 10 +- plugins/vstack/contracts/review-loop.md | 72 ++-- plugins/vstack/hooks/round-gate.mjs | 2 +- plugins/vstack/host-profiles/claude.json | 1 + plugins/vstack/host-profiles/codex.json | 3 +- plugins/vstack/host-profiles/grok.json | 1 + plugins/vstack/lib/live-link.mjs | 45 ++- plugins/vstack/skills/review/SKILL.md | 89 ++--- .../skills/review/assets/review-server.mjs | 323 ++++++++++++++---- .../skills/review/assets/workspace.html | 53 +-- plugins/vstack/skills/review/hosts/codex.md | 42 +-- .../skills/review/references/workflow.md | 17 +- .../skills/review/tests/host-profiles.mjs | 8 +- .../skills/review/tests/review-lifecycle.mjs | 116 ++++++- .../vstack/skills/review/tests/round-gate.mjs | 7 +- 25 files changed, 689 insertions(+), 267 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eef8bca..9dd6ef6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,10 +8,45 @@ The version in `plugins/vstack/.claude-plugin/plugin.json` is what your host compares against to decide an update is available. See the release checklist in [`CONTRIBUTING.md`](CONTRIBUTING.md). +## 6.6.0 — 2026-08-10 + +**Changed** + +- **Review comments now reach the agent one at a time, in FIFO order.** New + comments wait without interrupting the comment in progress. Asking a question + releases the queue so the next ready comment can proceed; when the reviewer + answers, that thread rejoins the queue at the time of the reply. +- The workspace shows exactly one comment as in progress. Later comments and + answered threads say Queued, while questions still say that the agent is + waiting on the reviewer. + +## 6.5.0 — 2026-08-10 + +**Changed** + +- **Codex now receives review rounds through bounded foreground waits.** Its + terminal does not push a background process's output into an idle agent turn, + so keeping a second persistent shell and polling its buffer could leave a + comment marked as delivered before Codex had read it. Codex now runs a + 25-second `watch --next` call, repeats it on `IDLE`, and explicitly `claim`s a + `REVIEW` offer. Until that claim succeeds, the comments remain queued. +- **Linked means a Codex consumer is still calling back.** Each bounded wait + renews a short lease which bridges normal re-arms and expires when the turn + stops. A leftover Node process can no longer keep the workspace falsely + Linked. Claude Code and Grok keep their pushed stream watcher unchanged. +- Two Codex pulls may wait on the same review safely: both see one durable + offer, and only the first claim records delivery. A push watcher remains + exclusive and cannot be taken over by a project-wide pull. + ## 6.4.1 — 2026-08-09 **Fixed** +- **A reply written with paragraph breaks arrives with them.** A shell leaves + `\n` inside a quoted argument as two characters, so an agent's multi-paragraph + answer reached the comment thread with `\n` showing as text. `reply --text`, + `reply --option` and `publish --summary` now read `\n` as a line break. Write + `\\n` when you mean the two characters. - **A comment containing a double quote could break the page it was drawn on.** The workspace escaped `&`, `<` and `>` but not quotes, and most of what it builds is an HTML attribute — so a quote in your own words ended the attribute diff --git a/README.md b/README.md index 9136fcf..5b3caba 100644 --- a/README.md +++ b/README.md @@ -99,13 +99,13 @@ No scrolling back through the chat. No screenshot graveyard on your desktop. No ### Live Link -Each workspace is linked to one agent session. The link holds while that session is active, its heartbeat is less than 15 seconds old, and every submitted review round has been claimed. +Each workspace is linked to one agent session. The link holds while that session is active and its heartbeat is less than 15 seconds old. Comments wait in one FIFO, and only the active comment is in the agent's hands. ![The workspace page in a browser tab talks over http and SSE to the review server on 127.0.0.1. The server reads and writes a store on disk holding the state, the versions, the comments, the rounds, and the files that carry the link. The agent session watches and writes the same store.](docs/assets/live-link.svg) ### Review Lifecycle -![Your comments are submitted as one review round. The agent claims the round and reads its brief, asking for clarification when a comment is unclear. Comments sent while the round is in progress join it. Publishing is blocked until every comment has been applied, answered, or dismissed, and the published version appears in the same workspace.](docs/assets/review-lifecycle.svg) +![Comments enter one FIFO queue. The agent receives one comment at a time, so later comments never interrupt active work. Asking a question releases the queue while that thread waits for an answer, and the answered thread rejoins in arrival order. Each completed comment publishes into the same workspace.](docs/assets/review-lifecycle.svg) ## Security diff --git a/docs/assets/review-lifecycle.svg b/docs/assets/review-lifecycle.svg index 7d51393..1a82e90 100644 --- a/docs/assets/review-lifecycle.svg +++ b/docs/assets/review-lifecycle.svg @@ -1,8 +1,8 @@ - + REVIEW LIFECYCLE - A review round, from submission to the next version + One comment at a time, in arrival order @@ -19,18 +19,18 @@ - EVERY ROUND + FIFO LOOP Send - Your comments are submitted as one review round. + Comments enter one queue in arrival order. Review - The agent claims the round and reads its brief. + The agent claims only the oldest ready comment. @@ -40,12 +40,12 @@ IF A COMMENT IS UNCLEAR Question - The agent asks for clarification on that comment. + Asking releases the active slot for the next comment. Reply - Your answer is added to the comment. + Your answer rejoins the FIFO at this moment. @@ -55,18 +55,18 @@ IF YOU COMMENT MID-ROUND Send again - New comments join the current round. + New comments wait without interrupting active work. Review again - The agent reads them at the next checkpoint. + The next ready comment arrives after the active one ends. Publish - Blocked until every comment has been - applied, answered, or dismissed. + Close the active comment and publish its change. + The next FIFO item can then become active. diff --git a/e2e/features/linking.feature b/e2e/features/linking.feature index 97cfbd9..44b6240 100644 --- a/e2e/features/linking.feature +++ b/e2e/features/linking.feature @@ -14,3 +14,11 @@ Feature: The stream watcher links a session When the reviewer sends a comment "A" Then the watcher receives a REVIEW event And the workspace cannot requeue the round while the watcher lives + + Scenario: S18 — a Codex pull does not deliver until its offer is claimed + Given a page is under review with host "codex" + When the reviewer sends a comment "A" + And the agent runs a bounded pull + Then the pull offers the round without delivering it + When the agent claims the pull offer + Then the pull claim delivers the round to session "codex-test" diff --git a/e2e/features/sending.feature b/e2e/features/sending.feature index 7a0c607..13709a7 100644 --- a/e2e/features/sending.feature +++ b/e2e/features/sending.feature @@ -1,7 +1,6 @@ Feature: Page review — sending comments - A sent comment reaches the agent immediately when no round is in flight, - and queues behind the round when one is. Whatever the agent does not close - comes back on the next delivery. + A sent comment reaches the agent immediately when nothing is active. Every + delivery contains one comment, and ready comments are handled FIFO. Background: Given a page is under review @@ -26,12 +25,15 @@ Feature: Page review — sending comments And the brief lists "B" as new @round1 - Scenario: S3 — whatever is not closed comes back + Scenario: S3 — ready comments are delivered FIFO, one at a time Given the reviewer has sent comments "A" and "B" And the agent has taken delivery When the agent closes "A" and publishes "Only A" And the reviewer sends a comment "C" And the agent takes delivery - Then the delivery names 2 open comments, 1 new - And the brief lists "B" as not new + Then the delivery names 1 open comment, 1 new + And the brief lists "B" as new + When the agent closes "B" and publishes "Then B" + And the agent takes delivery + Then the delivery names 1 open comment, 1 new And the brief lists "C" as new diff --git a/e2e/steps/linking.steps.mjs b/e2e/steps/linking.steps.mjs index bb075e9..c4e8146 100644 --- a/e2e/steps/linking.steps.mjs +++ b/e2e/steps/linking.steps.mjs @@ -42,3 +42,28 @@ Then('the workspace cannot requeue the round while the watcher lives', async fun assert.equal(requeued.response.status, 409, 'nothing is taken off an agent that is listening') }) + +When('the agent runs a bounded pull', function () { + this.pull = this.cli('watch', '--all', '--next', '--timeout', '1') + assert.equal(this.pull.status, 0, this.pull.stderr) + this.pullToken = this.pull.stdout.match(/token ([0-9a-f]+)/)?.[1] + assert.ok(this.pullToken, `pull did not offer a token:\n${this.pull.stdout}`) +}) + +Then('the pull offers the round without delivering it', function () { + assert.match(this.pull.stdout, /REVIEW/) + assert.equal(this.byNote('A').deliveredAt, null, + 'terminal output nobody claimed is not a delivery') +}) + +When('the agent claims the pull offer', function () { + this.claim = this.cli('claim', ...this.subjectArgs(), '--token', this.pullToken, + '--session', 'codex-test') + assert.equal(this.claim.status, 0, this.claim.stderr) +}) + +Then('the pull claim delivers the round to session {string}', function (session) { + assert.match(this.claim.stdout, /CLAIMED/) + assert.ok(this.byNote('A').deliveredAt) + assert.equal(this.byNote('A').deliveredTo, session) +}) diff --git a/e2e/test-plan.html b/e2e/test-plan.html index 6b696e8..67c2b7a 100644 --- a/e2e/test-plan.html +++ b/e2e/test-plan.html @@ -88,13 +88,14 @@

Feature: Page review — sending comments

-
S3

Whatever is not closed comes back

@round1
-
Scenario: an unclosed comment returns on the next delivery - Given the agent was handed comments A and B in one delivery - When the agent publishes closing only A - Then B stays open - When the reviewer sends a new comment C - Then the delivery hands over B and C — B marked not new, C marked new
+
S3

Ready comments arrive FIFO, one at a time

@round1
+
Scenario: each delivery contains the oldest ready comment + Given comments A and B were sent in that order + When the agent takes delivery + Then only A is delivered and B stays queued + When the agent closes A and the reviewer sends C + Then the next delivery contains only B + And the delivery after B closes contains only C
@@ -119,6 +120,7 @@

Feature: Page review — closing, versions, threads

When the agent replies "Every row, or only overdue ones?" Then the reply is appended to A's thread with by: "agent" And A stays open + And A releases the active slot while it waits on the reviewer And `unanswered` no longer names A — a reply answers the round
diff --git a/plugins/vstack/.claude-plugin/plugin.json b/plugins/vstack/.claude-plugin/plugin.json index b045006..325e0e3 100644 --- a/plugins/vstack/.claude-plugin/plugin.json +++ b/plugins/vstack/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "vstack", "displayName": "Visual Stack", - "version": "6.4.1", + "version": "6.6.0", "description": "Stop prompting. Start pointing. Visual Stack adds a Figma-like feedback layer to Claude Code. Create a new screen from a prompt, screenshot, reference site, or design system, or open an app you already have running. Click any element and leave feedback exactly where the problem is hiding, and Claude publishes the next revision into the same workspace. Compare revisions on a timeline, preview desktop, tablet, and mobile layouts, and keep every comment attached to the element, route, and version it refers to. Wireframes are self-contained HTML and review state stays in your project.", "author": { "name": "Cavalry Collective", diff --git a/plugins/vstack/.codex-plugin/plugin.json b/plugins/vstack/.codex-plugin/plugin.json index a0f062f..97440fb 100644 --- a/plugins/vstack/.codex-plugin/plugin.json +++ b/plugins/vstack/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "vstack", - "version": "6.4.1", + "version": "6.6.0", "description": "Stop prompting. Start pointing. Visual Stack adds a Figma-like feedback layer to Codex. Create a new screen from a prompt, screenshot, reference site, or design system, or open an app you already have running. Click any element and leave feedback exactly where the problem is hiding, and Codex publishes the next revision into the same workspace. Compare revisions on a timeline, preview desktop, tablet, and mobile layouts, and keep every comment attached to the element, route, and version it refers to. Wireframes are self-contained HTML and review state stays in your project.", "author": { "name": "Cavalry Collective", diff --git a/plugins/vstack/contracts/host.md b/plugins/vstack/contracts/host.md index b45e6d9..0f19da4 100644 --- a/plugins/vstack/contracts/host.md +++ b/plugins/vstack/contracts/host.md @@ -17,6 +17,13 @@ Every Host is described by a **profile** (`host-profiles/.json`, schema | `id` | Stable key: `claude`, `codex`, `grok`, … — used as `VSTACK_HOST` / `--host` | | `name` | Human label in UI (“Claude”, “Codex”, “Grok”) | +Two capability fields describe delivery semantics rather than UI: + +| Field | Meaning | +| --- | --- | +| `capabilities.watch` | `stream` pushes events; `pull` uses bounded waits and explicit claims | +| `capabilities.turnGate` | Whether the Host can stop a turn from ending with a delivered round unanswered | + --- ## Operations @@ -34,11 +41,16 @@ Start a process that **outlives the current agent turn**. Used for - Must be stoppable via `stop`. - Stdout/stderr should remain available for diagnosis. -### `watch_stream(command)` — **required** +### Watch delivery — **required** + +A Host profile declares `capabilities.watch: stream | pull`, and its adapter +implements the matching operation. + +#### `watch_stream(command)` — `stream` Run a long-lived process whose **stdout is a line-delimited event stream**. -Each complete line is delivered to the agent as an event (without the agent -having to poll or re-arm). Used for: +Each complete line is pushed to the agent as an event without polling or +re-arming. Used for: ```bash node review-server.mjs watch --all --stream @@ -53,13 +65,30 @@ node review-server.mjs watch --all --stream proves a session is receiving the stream, which is the only claim the UI's **Linked** state is allowed to make. +#### `watch_next(command)` — `pull` + +Run a bounded foreground command which returns one event or `IDLE` before the +Host tool's own yield limit: + +```bash +node review-server.mjs watch --all --next --timeout 25 +``` + +- `REVIEW` is a durable offer and prints a token-bearing `claim` command. +- The agent runs `claim` immediately; only a successful claim records delivery. +- `IDLE` means re-run the bounded wait while the review remains open. +- A short `listening` lease is renewed while each wait is active and expires if + the Host stops calling. This, not a leftover process, proves **Linked**. +- Multiple pull consumers may see the same offer; `claim` serialises delivery. + ### `stop(handle)` — **required** -Terminate a process started by `background` or `watch_stream`. +Terminate a process started by `background` or `watch_stream`. A bounded +`watch_next` has no retained process after it returns. ### `run(command)` — **required** -Synchronous shell: `publish`, `reply`, `share`, `check`, `status`, file edits’ +Synchronous shell: `claim`, `publish`, `reply`, `share`, `check`, `status`, file edits’ supporting commands. Blocks until exit; agent reads exit code and stdout. ### `edit` — **required** diff --git a/plugins/vstack/contracts/host.schema.json b/plugins/vstack/contracts/host.schema.json index c610de1..8d3b19f 100644 --- a/plugins/vstack/contracts/host.schema.json +++ b/plugins/vstack/contracts/host.schema.json @@ -18,7 +18,7 @@ }, "capabilities": { "type": "object", - "required": ["share", "watch", "browser", "updateDetect"], + "required": ["share", "watch", "turnGate", "browser", "updateDetect"], "additionalProperties": false, "properties": { "share": { @@ -28,8 +28,12 @@ }, "watch": { "type": "string", - "enum": ["stream", "oneshot"], - "description": "Preferred watch mode. stream is required for product-quality Linked state." + "enum": ["stream", "pull"], + "description": "Delivery mode: pushed stream events, or bounded pull waits followed by an explicit claim." + }, + "turnGate": { + "type": "boolean", + "description": "Whether the Host can block a turn that tries to end with a delivered round unanswered." }, "browser": { "type": "boolean", diff --git a/plugins/vstack/contracts/review-loop.md b/plugins/vstack/contracts/review-loop.md index c141b30..c86b6a8 100644 --- a/plugins/vstack/contracts/review-loop.md +++ b/plugins/vstack/contracts/review-loop.md @@ -47,17 +47,20 @@ calls a comment done. Everything the workspace shows is derived from that list. | `closedAt` | When the agent closed it | | `replies` | `{ by, text, at }[]`, append-only. An agent reply may carry `options: [{ text, recommended }]` — answers the reviewer picks from | | `sentAt` | The reviewer let go of it. Null means it is still a draft | -| `deliveredAt` | The agent was last handed it. Null means it is still queued here | +| `deliveredAt` | The agent was last handed it. Null means it has never left the queue | | `deliveredTo` | The session the last delivery was recorded for — the `--session` id its watcher was started with. Null when the watcher carried no identity | +| `activeAt` | The comment currently in the agent's hands. At most one open comment has this set | | `dismissedAt` | The reviewer took it off the list after it had been delivered. The record stays; the workspace never shows it again | -Those two timestamps carry the whole of a comment's progress: +The delivery fields and the thread carry the whole of a comment's progress: -| State | `sentAt` | `deliveredAt` | Editable | Withdrawable | +| State | Delivery fields | Thread | Editable | Withdrawable | | --- | --- | --- | --- | --- | -| Being written | — | — | yes | yes, the record goes | -| Queued | set | — | no | yes, the record goes | -| With the agent | set | set | no | yes, the record stays behind it | +| Being written | none | any | yes | yes, the record goes | +| Queued for its first turn | `sentAt` only | any | no | yes, the record goes | +| With the agent | `sentAt`, `deliveredAt`, `activeAt` | agent has not answered this delivery | no | yes, the record stays behind it | +| Waiting on the reviewer | `sentAt`, `deliveredAt`; no `activeAt` | agent spoke last | no | yes, the record stays behind it | +| Answered and queued again | `sentAt`, `deliveredAt`; no `activeAt` | reviewer replied after delivery | no | yes, the record stays behind it | --- @@ -78,8 +81,8 @@ interrupted by a withdrawal and is not told of one. **Agent** — three verbs: -- Take delivery of the open comments (the tick). -- Reply to a comment. This never changes its state. +- Take delivery of the oldest ready comment (the tick). +- Reply to a comment. This never changes its open/closed state and releases the active slot. - Close comments, and optionally snapshot a version. --- @@ -111,15 +114,17 @@ subject present in both is read from `review/`. | --- | --- | | `state.json` | `{ name, version, file? \| app?, start? }` | | `comments.json` | Every comment for this review | -| `brief.md` | The open comments, rewritten on every delivery | +| `brief.md` | The one active comment, rewritten on every delivery | | `versions/v.html` | Frozen file, or the DOM capture for a live app | | `versions/v.meta.json` | `{ n, label, date }` | | `reviews/v/` | Only ever read: where a store filled by an older version keeps its comments | | `handshake` | A stream watcher waiting to be told its events are being read | +| `delivery-offer.json` | A pull watcher found a round; its token must be claimed before delivery is recorded | | `approved` | Sentinel: design signed off; engine shutting down | | `share` | Sentinel: reviewer wants a shareable link | | `url` | Present only while `serve` is running | | `watching` | Heartbeat while Host op `watch_stream` is active | +| `listening` | Short lease renewed by bounded `watch_next` calls; ages out when pulls stop | `serve` also records the store it is serving under the directory it was run from: `/.vstack/local/review/.serving/`, one file per live review. @@ -143,8 +148,9 @@ Host selection: `--host ` or `VSTACK_HOST=` (affects UI injection only). | Command | Contract | | --- | --- | | `serve --file …` / `serve --app …` | Long-lived via Host `background`. Binds `127.0.0.1` | -| `watch [--all] [--file …] [--stream] [--session ]` | Take delivery. Blocks until the reviewer has said something new. `--session` names the agent session each delivery binds to | +| `watch [--all] [--file …] [--stream \| --next --timeout ] [--session ]` | Push mode takes delivery; pull mode returns a durable offer or `IDLE`. `--session` names the session a stream delivery or later claim binds to | | `ack --file/name … --token ` | Answer a stream watcher's handshake. Only this arms the `watching` heartbeat | +| `claim --file/name … --token [--session ]` | Accept a pull `REVIEW` offer. Only this records delivery and writes `brief.md`; safe to retry only until another consumer wins | | `publish --file/name … [--close ids] [--label …] [--summary …]` | Close comments, snapshot a version, or both. `--summary` records the account of the round, which the workspace shows; the latest one is kept and a publish without it clears it | | `reply --file/name … --comment --text "…" [--option "…" … --recommend ]` | Append `{ by: "agent", text, at }`, with `options: [{ text, recommended }]` when options are given. The reviewer answers by pressing one, which posts those words as their reply | | `share --file/name … --url ` | Record public URL; clear the `share` sentinel | @@ -154,9 +160,9 @@ Host selection: `--host ` or `VSTACK_HOST=` (affects UI injection only). --- -## Stream events +## Watch events -One line of stdout per event (from `watch --stream`): +One line of stdout per event. Streams stay open; bounded pulls return one: | Prefix | Meaning | Agent action | | --- | --- | --- | @@ -165,26 +171,29 @@ One line of stdout per event (from `watch --stream`): | `LINKED` | The handshake was answered and at least one review is covered | — | | `UNLINKED` | The handshake was answered and no review turned up to cover | Start it again with `--file` if a review is running elsewhere | | `UNWIRED` | The handshake went unanswered; the watcher exits `3` | Start it again via `watch_stream` | -| `REVIEW` | Comments have been handed over; names how many and the brief | Read `brief.md`, apply it, `publish --close` / `reply` | +| `IDLE` | Bounded pull timed out without an event | Run `watch_next` again while the review remains open | +| `REVIEW` | Stream: one comment delivered. Pull: token offered, nothing delivered yet | Pull runs printed `claim`; then read `brief.md`, apply it, `publish --close` / `reply` | | `SHARE` | Link requested | Host `share` if capable; then `share --url` | | `APPROVED` | Sign-off; server exiting | Confirm; next pipeline stage as skill says | | `OPENED` | Another live store joined `--all` | — | | `CLOSED` | Tab/store gone | Drop; exit when none left | -A reply raises no event of its own: it is the same comment coming round again -with more said on it. +A reviewer reply makes that thread ready again. It rejoins the FIFO at the time +of the reply and waits for the active comment to finish. --- ## The loop ``` -serve (background) + watch_stream +serve (background) + watch_stream | watch_next │ ▼ reviewer comments ──Send──► comments.json │ │ - │ REVIEW event ──► brief.md (delivery recorded) + │ REVIEW event + │ push: delivery recorded + │ pull: claim ──► brief.md (delivery recorded) │ ▼ │ agent: apply · reply/close · publish │ │ @@ -197,8 +206,8 @@ reviewer comments ──Send──► comments.json Rules: 1. Only `publish --close` says a comment is done. The reviewer has no resolve. Withdrawing (rule 9) takes a comment off their list and says nothing about the work. -2. A tick hands over **every** open comment, not only the new ones, and marks which are new since the last delivery. -3. Whatever the agent does not close stays open and comes back on the next tick. There is no coverage to satisfy. +2. A tick hands over exactly one comment: the oldest ready first send or thread answer. One active comment blocks every later delivery. +3. Closing the active comment releases the queue. Replying also releases it, so the next ready comment can proceed while that thread waits on the reviewer. The reviewer's answer rejoins the FIFO at its reply time. 4. **Nothing can refuse a close.** An agent that has taken delivery can always finish, whatever the reviewer did meanwhile. 5. Closing what is already closed is a no-op, so a retried command is safe. 6. A comment's words are frozen when the reviewer sends it. The engine keeps the stored note whatever a client saves afterwards. @@ -206,19 +215,18 @@ Rules: 8. A reviewer's reply to a closed comment reopens it. An agent's reply never changes state. 9. A comment may be withdrawn at any point. Undelivered, it is deleted. Delivered, it is marked `dismissedAt` and `closed`: it leaves the workspace, no tick raises it again, and the id still resolves so the agent holding it can close it. 10. A version is a snapshot to look at. It records no comments, and no comment records a version. -11. One `watch_stream` per session is enough with `--all`. -12. Presence is proven. A stream watcher writes its `watching` heartbeat from the moment its handshake is answered, so **Linked** means a session is receiving the stream. Default window 120 s (`--handshake-timeout `). -13. Presence is per review, and per watcher. A watcher heartbeats only the stores it covers, and goes live only on an answer carrying its own token. -14. An agent that took delivery answers. A comment it was handed is answered by closing it or by replying to it. Neither is a round that stopped halfway, because no tick will raise that comment again until the reviewer writes. `unanswered` names them, and exits 1 while any remain. -15. A delivered comment goes back to the queue when nothing is listening. That is the way out of a round whose agent session died: those comments are not `unseen`, so no new watcher would ever hand them over. `deliveredAt` and `deliveredTo` are cleared and the comment is Queued again. The engine refuses this while a `watching` heartbeat is fresh, because then an agent still holds it and rule 14 applies instead. -16. A delivery binds to a session. A watcher started with `--session ` records that id on every comment it hands over, the latest delivery owns the round, and `unanswered --session ` answers for that session alone — so a Host that gates the end of a turn never holds one session's turn open for another session's round. A delivery recorded with no identity is reported only by the unfiltered form. -17. `watch --all` never covers a store whose `watching` heartbeat is fresh: that heartbeat is another watcher, and covering the review twice would deliver the same comment to two sessions. The store joins the sweep once the heartbeat is gone. A store named with `--file` is covered regardless — naming it is a deliberate takeover. - -Rule 14 is an obligation on the agent, not a refusal by the engine. Rule 3 -still holds: `publish` closes exactly what it names and accepts everything else -being left open. A Host that can gate the end of a turn is where the obligation -is enforced — see the Stop hook in `plugins/vstack/hooks/`. A Host that cannot -gets the rule as an instruction and nothing more. +11. One `watch_stream` per push session, or one repeated `watch_next` loop per pull session, is enough with `--all`. +12. Presence is proven. A stream watcher writes `watching` from the moment its handshake is answered. A pull call renews `listening` only while its bounded foreground wait is active; the lease survives the small re-arm gap and then ages out. **Linked** means at least one proof is fresh. +13. Presence is per review and per consumer. A push watcher heartbeats only the stores it covers. Pull consumers may overlap because they share one durable offer and `claim` serialises delivery. +14. An agent that took delivery answers. The active comment is answered by closing it or replying to it. Neither leaves the queue blocked by a turn that stopped halfway. `unanswered` names that comment and exits 1 while it remains active and unanswered. +15. An active comment goes back to the queue when nothing is listening. That is the way out of a turn whose agent session died. `activeAt`, `deliveredAt` and `deliveredTo` are cleared. Comments waiting on reviewer answers stay where they are. The engine refuses recovery while either push heartbeat or pull lease is fresh, because then an agent still holds the active comment and rule 14 applies instead. +16. A delivery binds to a session. A watcher started with `--session ` records that id on the comment it hands over. A later delivery of the same thread binds it to the latest session. `unanswered --session ` answers for that session alone, so a Host that gates the end of a turn never holds one session's turn open for another session's work. A delivery recorded with no identity is reported only by the unfiltered form. +17. A push or legacy one-shot `watch --all` never covers a store whose push heartbeat or pull lease is fresh. A pull `watch --next` may overlap another pull: both see the same offer and only one claim can record delivery. A store named with `--file` is covered regardless — naming it is a deliberate takeover. + +Rule 14 is an obligation on the agent and the engine's queue gate. `publish` +still closes exactly what it names. A Host that can gate the end of a turn also +enforces the obligation at the turn boundary. See the Stop hook in +`plugins/vstack/hooks/`. A Host that cannot gets the rule as an instruction. Rule 4 is the liveness property. Every dead-end this protocol has had came from a rule that could stop a round ending. diff --git a/plugins/vstack/hooks/round-gate.mjs b/plugins/vstack/hooks/round-gate.mjs index 4f09985..8478ce2 100644 --- a/plugins/vstack/hooks/round-gate.mjs +++ b/plugins/vstack/hooks/round-gate.mjs @@ -23,7 +23,7 @@ try { input = JSON.parse(Buffer.concat(chunks).toString('utf8') || '{}') } catch /* One block per turn. The gate names what is missing once; a session that means to stop with a round open — because the reviewer asked it to — must still be able to. Nothing is lost either way: an unanswered comment stays open and - comes back on the next delivery. */ + can be returned to the FIFO once that session is gone. */ if (input.stop_hook_active) process.exit(0) /* The payload names the session this Stop belongs to, and `unanswered` answers diff --git a/plugins/vstack/host-profiles/claude.json b/plugins/vstack/host-profiles/claude.json index 9880709..8f05002 100644 --- a/plugins/vstack/host-profiles/claude.json +++ b/plugins/vstack/host-profiles/claude.json @@ -4,6 +4,7 @@ "capabilities": { "share": "artifact", "watch": "stream", + "turnGate": true, "browser": true, "updateDetect": "claude-install" }, diff --git a/plugins/vstack/host-profiles/codex.json b/plugins/vstack/host-profiles/codex.json index d651eec..92df4ae 100644 --- a/plugins/vstack/host-profiles/codex.json +++ b/plugins/vstack/host-profiles/codex.json @@ -3,7 +3,8 @@ "name": "Codex", "capabilities": { "share": "copy", - "watch": "stream", + "watch": "pull", + "turnGate": false, "browser": true, "updateDetect": "codex-install" }, diff --git a/plugins/vstack/host-profiles/grok.json b/plugins/vstack/host-profiles/grok.json index 467994d..cf62852 100644 --- a/plugins/vstack/host-profiles/grok.json +++ b/plugins/vstack/host-profiles/grok.json @@ -4,6 +4,7 @@ "capabilities": { "share": "copy", "watch": "stream", + "turnGate": false, "browser": true, "updateDetect": "none" } diff --git a/plugins/vstack/lib/live-link.mjs b/plugins/vstack/lib/live-link.mjs index 6e863f5..240f37b 100644 --- a/plugins/vstack/lib/live-link.mjs +++ b/plugins/vstack/lib/live-link.mjs @@ -1,11 +1,10 @@ /* * live-link.mjs — the plumbing every live-link server shares. * - * The review server and the JSON bridge speak the same file protocol: a - * `watching` heartbeat that says an agent session is listening, a presence - * event that repeats it to the page, and atomic writes so a reader never sees - * half a file. The protocol lives here so its invariants — the heartbeat - * cadence, what counts as stale — cannot drift between engines. + * The review server and the JSON bridge speak the same file protocol: live + * push heartbeats, bounded pull leases, presence events that repeat them to the + * page, and atomic writes so a reader never sees half a file. The protocol + * lives here so its timing and write invariants cannot drift between engines. */ import fs from 'node:fs' @@ -65,12 +64,30 @@ export function injectHead (html, tag) { number: a writer beating every BEAT_MS is comfortably inside STALE_MS. */ export const WATCH_BEAT_MS = 2000 export const WATCH_STALE_MS = 15000 +/* Pull hosts prove attention one bounded wait at a time. Their lease is longer + than a normal wait (25s), so a prompt re-arm never flickers the workspace to + Unlinked; if the agent stops calling back, it expires without a process + pretending the session is still there. */ +export const PULL_LEASE_MS = 45000 /** Is a watcher alive behind this heartbeat file right now? */ export function watchingRecently (file) { try { return Date.now() - fs.statSync(file).mtimeMs < WATCH_STALE_MS } catch { return false } } +/** Is a bounded pull consumer still inside the lease its last wait proved? */ +export function leasedRecently (file) { + try { return Date.now() - fs.statSync(file).mtimeMs < PULL_LEASE_MS } catch { return false } +} + +/** Renew one pull lease without starting a timer (claim and other quick ops). */ +export function renewLease (file) { + try { + fs.mkdirSync(path.dirname(file), { recursive: true }) + fs.writeFileSync(file, String(Date.now())) + } catch {} +} + /** * Keep heartbeat files fresh until stopped. `getFiles` is read on every beat, * so a watcher whose subject list changes underneath it stays truthful. @@ -97,6 +114,24 @@ export function startHeartbeat (getFiles) { } } +/** + * Renew a pull consumer's lease while its bounded wait is active. Stopping the + * timer deliberately leaves the marker behind: its age, not process cleanup, + * is what lets the next wait bridge the small gap between tool calls and what + * makes an abandoned Codex turn become Unlinked by itself. + */ +export function startLease (getFiles) { + const renew = () => { + for (const file of getFiles()) renewLease(file) + } + const timer = setInterval(renew, WATCH_BEAT_MS) + renew() + return { + renew, + stop () { clearInterval(timer) }, + } +} + /** * Tell every SSE client who is listening, only when the answer changes. With * `keepalive`, quiet ticks still send an SSE comment so the connection is its diff --git a/plugins/vstack/skills/review/SKILL.md b/plugins/vstack/skills/review/SKILL.md index 7509531..5022f05 100644 --- a/plugins/vstack/skills/review/SKILL.md +++ b/plugins/vstack/skills/review/SKILL.md @@ -20,7 +20,7 @@ Adapters live in the `hosts/` directory beside this SKILL.md. Do not read `plugins/vstack/host-profiles/.json` instead: that JSON is UI data with no tool mapping. -**Read the adapter before §3.** Every `background`, `watch_stream`, `stop`, +**Read the adapter before §3.** Every `background`, `watch_stream` / `watch_next`, `stop`, `share`, and `browser_capture` step is fulfilled exactly as that file says. A two-way review loop. The user comments on the screen; you apply the comments, ask about anything ambiguous, and publish the next version. Two things can go under it: @@ -142,8 +142,8 @@ The page opens in **its own browser window** on the canvas — own viewport, own | **Clear all** | in the comment list footer, behind a confirm. It takes the addressed comments off the list — the same act as the per-card delete. Comments still open stay unless the reviewer ticks the box on the confirm, which is off every time it is asked | | **Link status** | a dot beside Send — linked to your session, or link lost. Nothing is said until the connection has actually answered | | **Send to {agent}** (⌘⏎) | sends straight through — no preview step — and wakes you up. Label uses the Host profile name. Greys out until something actually changes | -| **In flight** | every comment you were sent keeps an indeterminate progress bar until you publish or reply. No banner covers the page any more — the progress is on the comments it belongs to | -| **Stalled** | after a minute with nothing listening, the strip stops claiming progress and says you have stalled. **Send again** puts those comments back in the queue, and the next session to pick up is handed them. Refused while your watcher is alive, because then you still have them | +| **In flight** | one comment keeps an indeterminate progress bar until you publish or reply. Later comments remain queued, and a question waits on the reviewer without blocking the next ready comment | +| **Stalled** | after a minute with nothing listening, the strip stops claiming progress and says you have stalled. **Send again** puts the active comment back in the queue for the next session. Refused while your watcher is alive, because then you still have it | | **Addressed** | comments you closed stay in the list in their own section, each offering **Revert** or **Refine** | | **Publish a link to this wireframe** (the ▾ beside Send) | only when Host `capabilities.share` is `artifact`. Asks you to publish **the wireframe** (Host op `share`) and hand the URL back. Hidden on hosts without public share, and in a live review | | **Approve & finish** (the ▾ beside Send) | sign-off. Ends the review, closes the server, and tells you the design is settled — behind a confirm that warns how many comments are being left unapplied | @@ -166,32 +166,34 @@ accepts a status going backwards when the reviewer deliberately sent it back. ## 5 · Catch the review, and hold up your end of the conversation -After starting the server, start the watcher with Host op **`watch_stream`**. Look up the tool for -that op in your adapter — `watch_stream` delivers each line of output to you as it arrives, and it -is a different op from the `background` you used in §3. +After starting the server, start the watch operation your Host adapter names. A profile with +`capabilities.watch: stream` uses **`watch_stream`**; `pull` uses **`watch_next`**. Either is a +different op from the `background` you used in §3. ```bash -node "$SKILL/assets/review-server.mjs" watch --all --stream # or --file +node "$SKILL/assets/review-server.mjs" watch --all --stream # push Host +node "$SKILL/assets/review-server.mjs" watch --all --next --timeout 25 # pull Host ``` -Use the exact command your adapter's `watch_stream` entry gives, not the bare form above: it adds -`--session ` when your host has one, and that is what binds each delivery to you — -without it, the round you take cannot be told apart from another session's. +Use the exact command your adapter gives, not the bare forms above. It adds `--session ` when your Host has one, and that is what binds each delivery to you — without it, the +round you take cannot be told apart from another session's. -Each line of its output is one event, delivered to you as it happens, and the process keeps running, -so one watcher covers the whole session — `--all` takes in every review open in the project, -including ones opened later, and any you started from this directory whose page lives elsewhere. -It never takes over a review another session's watcher is already covering. -Run it from the same directory you started the server from; that is what ties the two together. +`--all` takes in every review open in the project, including ones opened later, and any you started +from this directory whose page lives elsewhere. It never takes over a push watcher another session +already has. Run it from the same directory you started the server from; that ties the two together. -**It opens with a `HANDSHAKE` line naming a command. Run that command straight away.** The watcher -goes live once you answer, the workspace says **Linked**, and the round events start reaching you. -Answering proves the op was fulfilled, since only a session that can run commands can answer. -Answer within two minutes; after that the watcher prints `UNWIRED` and exits, and you start it again -with the tool your adapter names for `watch_stream`. +**Push:** it opens with a `HANDSHAKE` line naming a command. Run that command straight away. The +watcher goes live once you answer. Answer within two minutes; after that it prints `UNWIRED` and +exits, and you start it again via `watch_stream`. -The page says **Linked** for as long as the watcher is answered, -and **Unlinked** in amber the rest of the time, so the reviewer always knows which one they have. +**Pull:** one foreground call returns within the adapter's timeout. On `IDLE`, call `watch_next` +again. On `REVIEW`, run the exact `CLAIM` command it prints immediately; only that command hands the +comment to you and writes `brief.md`. An unread REVIEW offer leaves the comment queued, so another +call or session can still receive it. + +The page says **Linked** while a push watcher is answered or a pull call's short consumer lease is +fresh, and **Unlinked** in amber the rest of the time. Each event is one line (full table: `contracts/review-loop.md`): @@ -201,7 +203,8 @@ Each event is one line (full table: `contracts/review-loop.md`): | **`LINKED`** | the handshake is answered and a review is under the watcher; the workspace says Linked | carry on — the loop is live | | **`UNLINKED`** | the handshake is answered, but the watcher found no review to cover, so no workspace says Linked | start it again with `--file ` if a review is already running for a page outside this directory. A serve started here after it needs nothing | | **`UNWIRED`** | the handshake went unanswered and the watcher exited | start it again with the tool your adapter names for `watch_stream` | -| **`REVIEW`** | comments have been handed to you; the line names how many are open and where the brief is | read the brief, then the steps below | +| **`IDLE`** | a bounded pull ended without an event | call `watch_next` again while the review is open | +| **`REVIEW`** | push: one comment was delivered; pull: a token was offered | pull Hosts run the printed `CLAIM` command first, then read the brief and continue below | | **`SHARE`** | they want a link to send someone | Host op `share` if capable, then §6; if the Host cannot share publicly, say so and offer a file/bundle instead | | **`APPROVED`** | the design is signed off; the server has closed itself | say it's approved, note any `openComments` deliberately left, and carry on with whatever comes next | | **`CLOSED`** | that review's tab went away | the watcher drops it and keeps watching the rest; it only stops when none are left | @@ -211,19 +214,19 @@ closing a comment is `publish --close`. ### What a delivery is -The watcher blocks until the reviewer has said something you have not been given, then hands you -**every open comment** — not only the new ones — marking which are new since last time. A comment you -do not close comes back on the next delivery, so nothing is lost by being missed. +The watch operation waits until a comment is ready and none is active. A push watcher hands it over +immediately; a pull watcher offers it and `claim` hands it over. Delivery contains **one comment**: +the oldest first send or thread answer in the FIFO. A new comment or thread answer never interrupts +the active one. While you work, **nothing will interrupt you.** New comments accumulate on the server and arrive with the next delivery; a review in flight cannot be called off. On a delivery: -1. **Read the brief** the `REVIEW` line names (`/brief.md`). It carries every open comment with +1. **Read the brief** the `REVIEW` line names (`/brief.md`). It carries the active comment with its element, place, screen size and thread. -2. **Apply every comment.** There are no priorities to sort by — if the reviewer wrote it down, it - needs doing. Locate each from its **anchor** — the element and the region it sits in — at the screen +2. **Apply that comment.** Locate it from its **anchor** — the element and the region it sits in — at the screen size it was made at, using the coordinates only to break a tie. 3. **Ask instead of guessing.** If a comment is ambiguous, reply to it — the question appears on the mark and in the comment list, where the user answers it: @@ -231,8 +234,11 @@ On a delivery: node "$SKILL/assets/review-server.mjs" reply --file "$FILE" \ --comment c7f2a1 --text "Every overdue row, or only the ones assigned to you?" ``` - The comment stays open and comes back with their answer attached. On disk the reply uses - `by: "agent"` (legacy files may say `"claude"`; treat them the same). + The comment stays open and releases the queue. Its answer comes back in FIFO order after whatever + was already ready. On disk the reply uses `by: "agent"` (legacy files may say `"claude"`; treat them the same). + + **Write a paragraph break as `\n`, or as a real line break inside the quotes.** Both reach the + reviewer as a break, in `--text`, `--option` and `--summary`. Write `\\n` for the two characters. **When the answers are a short list, offer them.** `--option`, repeated, puts them on the comment as buttons, and `--recommend ` marks the one you would take. Pressing one answers @@ -249,12 +255,10 @@ On a delivery: 5. **Close what you did, and snapshot the version:** ```bash node "$SKILL/assets/review-server.mjs" publish --file "$FILE" \ - --close c1f3k2,c9dk1 --label "Filters collapsed, overdue sorts first" \ - --summary "Filters are collapsed behind a single control, and overdue rows sort first. - I left the date column alone — say if you want it narrower too." + --close c1f3k2 --label "Filters collapsed" \ + --summary "Filters are collapsed behind a single control." ``` - Anything you do not name stays open and comes back. Publish tells you what it left open — close - those or reply asking about them, because the delivery will not raise them again on its own. + Closing or replying releases the active slot. The watcher then hands over the next ready comment. **`--label` names the version in one line. `--summary` is the account you would give in chat** — what you changed, what you decided, what you left. The workspace shows it on the @@ -265,13 +269,14 @@ On a delivery: ```bash node "$SKILL/assets/review-server.mjs" unanswered --all ``` - It exits 1 and names every comment you were handed and then said nothing about — neither closed - nor replied to. Those are the ones nothing will remind you of again, because the next delivery - only comes when the reviewer writes. Add `--session ` if your adapter names one, + It exits 1 and names the active comment when you were handed it and then said nothing about it — + neither closed nor replied. Until it is answered, the FIFO cannot move. Add + `--session ` if your adapter names one, so the answer covers your deliveries and not another session's. On Claude Code a Stop hook runs this for you, with your session id, and holds your turn open until it is clean. -7. Leave **`watch_stream` running** and say what changed in a few lines. Then wait — don't ask "shall I - continue?", the loop is the point. (Only re-arm if you used one-shot `watch` without `--stream`.) +7. Keep the adapter's **watch operation active** and say what changed in a few lines. Then wait — + don't ask "shall I continue?", the loop is the point. A pull Host immediately starts its next + bounded wait; a push Host leaves `watch_stream` running. **Closing the browser tab closes the review.** The workspace holds an SSE connection; when the last one goes and none returns within the grace period @@ -353,7 +358,7 @@ The server reverse-proxies the app, so the workspace and the app share an origin coordinate. The workspace moves to **http://localhost:7788/__review/**; every other path belongs to the app. Sockets are proxied too, so hot reload keeps working. Start it with Host op **`background`**, tell the user the -`/__review/` URL, and arm **`watch_stream`** (§5) against `.vstack/local/review//` in the +`/__review/` URL, and arm the adapter's **watch operation** (§5) against `.vstack/local/review//` in the directory you ran it from — **run every later command from that same directory**, or pass `--store`. Pass `--host` / `VSTACK_HOST` the same as a file review. diff --git a/plugins/vstack/skills/review/assets/review-server.mjs b/plugins/vstack/skills/review/assets/review-server.mjs index 50e6267..4264268 100644 --- a/plugins/vstack/skills/review/assets/review-server.mjs +++ b/plugins/vstack/skills/review/assets/review-server.mjs @@ -18,7 +18,9 @@ * * node review-server.mjs serve --file [--port 7788] [--idle-timeout 90] [--no-open] * node review-server.mjs serve --app [--name ] [--start /path] [--port 7788] - * node review-server.mjs watch --file (blocks; hands over the open comments) + * node review-server.mjs watch --file (blocks; hands over the next comment) + * node review-server.mjs watch --all --next --timeout 25 (offers one pull event or IDLE) + * node review-server.mjs claim --file --token * node review-server.mjs publish --file [--close c1,c3] [--label "…"] [--summary "…"] * node review-server.mjs reply --file --comment --text "…" * [--option "…" --option "…" [--recommend ]] @@ -30,25 +32,28 @@ * the review is of a running app. * * One list of comments, each open or closed, and the agent is the only one who - * closes. `watch` hands over every open comment and records that it went; - * whatever the agent does not close comes back on the next one. Nothing can - * refuse a close: an agent that took delivery can always finish. + * closes. A watch hands over the oldest ready comment and nothing else. Closing + * it releases the FIFO; asking a question also releases it while that thread + * waits for the reviewer. Nothing can refuse a close: an agent that took + * delivery can always finish. * * State lives in a sibling directory, out of the way of the page: * /.vstack/local/review// (live: /.vstack/local/review//) * state.json { name, version, file? | app?, start? } * comments.json every comment for this review — the whole truth - * brief.md the open comments, rewritten on every delivery + * brief.md the active comment, rewritten on every delivery * versions/v.html frozen copy of each published version * (live: the DOM as it stood when a review was sent) * versions/v.meta.json label and date — a snapshot to look at, nothing more * reviews/v/ only ever read: where a store filled by an older * version keeps its comments * handshake a stream watcher waiting to be told its events land + * delivery-offer.json a pull event waiting for an explicit claim * approved sentinel written on sign-off — the review is over * share sentinel — they want a shareable public link * url the live URL — present only while the server runs * watching heartbeat — an agent session is waiting on this review + * listening short pull-consumer lease — expires when bounded waits stop * * A serve also leaves a pointer to that store under the directory it was run * from — `/.vstack/local/review/.serving/` — so `watch --all`, run @@ -77,7 +82,10 @@ import { fileURLToPath } from 'node:url' import { checkForUpdate, currentVersion, dismissUpdate, withUpdate, withVersion } from '../../../lib/update-check.mjs' import { resolveHostId, loadHost, withHost, AGENT_ROLE, REVIEWER_ROLE } from '../../../lib/host.mjs' import { workDir, subjectDir, toolNames, LOCAL, TOOL } from '../../../lib/workdir.mjs' -import { writeAtomic, watchingRecently, startHeartbeat, startPresence, openInBrowser } from '../../../lib/live-link.mjs' +import { + writeAtomic, watchingRecently, leasedRecently, renewLease, + startHeartbeat, startLease, startPresence, openInBrowser, +} from '../../../lib/live-link.mjs' const HERE = path.dirname(fileURLToPath(import.meta.url)) @@ -110,6 +118,13 @@ function repeatedArg (flag) { return out } +/** A flag whose value is prose the reviewer reads — `--text`, `--summary`, + `--option`. A shell leaves `\n` inside a quoted argument as the two + characters, so a reply written with paragraph breaks arrives with them + unresolved and the workspace shows them as text. Resolve them here. + `\\n` is how you say the two characters instead. */ +const prose = value => String(value).replace(/\\(\\|n)/g, (_, c) => (c === 'n' ? '\n' : '\\')) + /* The agent session this process acts for — `--session `, supplied by the Host adapter. The engine never knows how a host names its sessions; it only records the identity it was given, so that a delivery binds to the session @@ -191,14 +206,19 @@ const P = { brief: () => path.join(STORE, 'brief.md'), lock: () => path.join(STORE, 'transition.lock'), handshake: () => path.join(STORE, 'handshake'), + offer: () => path.join(STORE, 'delivery-offer.json'), approved: () => path.join(STORE, 'approved'), share: () => path.join(STORE, 'share'), url: () => path.join(STORE, 'url'), /* Touched by `watch` while it runs, deleted when it stops — the heartbeat protocol in lib/live-link.mjs. */ watching: () => path.join(STORE, 'watching'), + /* A pull Host renews this only while a bounded wait is actually reaching the + agent. It ages out after the agent stops calling, unlike a background + process heartbeat that can outlive the turn consuming it. */ + listening: () => path.join(STORE, 'listening'), } -const someoneWatching = () => watchingRecently(P.watching()) +const someoneWatching = () => watchingRecently(P.watching()) || leasedRecently(P.listening()) const readJSON = (f, d = null) => { try { return JSON.parse(fs.readFileSync(f, 'utf8')) } catch { return d } } const writeJSON = (f, v) => { @@ -269,13 +289,14 @@ const saveState = s => writeJSON(P.state(), s) /* ──────────────────────── the comment list ─────────────────────── One list per review, and the only place a comment's state lives. A comment - is open or closed. Two timestamps say where it is between the reviewer and + is open or closed. Delivery fields say where it is between the reviewer and the agent: `sentAt` is the reviewer letting go of it, which also freezes its - words; `deliveredAt` is the agent taking it, after which withdrawing it + words; `deliveredAt` is the agent taking it, and `activeAt` says it owns the + single active slot. After delivery, withdrawing it leaves the record behind. `deliveredTo` names the session that took it — whichever identity the last deliverer was started with — so what a session owes is a recorded fact, not an inference from standing in the same - directory. Everything the workspace shows is derived from those. */ + directory. Everything the workspace shows is derived from those fields. */ /** * What a review is, read from its own store rather than from this process's @@ -309,7 +330,7 @@ function saveComments (comments, subject = here()) { /** Fields the protocol owns. A client may write everything else on a comment it * still holds, and none of these ever. */ -const OWNED = ['state', 'sentAt', 'deliveredAt', 'deliveredTo', 'dismissedAt'] +const OWNED = ['state', 'sentAt', 'deliveredAt', 'deliveredTo', 'activeAt', 'dismissedAt'] const normaliseComment = c => ({ ...c, @@ -318,6 +339,7 @@ const normaliseComment = c => ({ sentAt: c.sentAt || null, deliveredAt: c.deliveredAt || null, deliveredTo: c.deliveredTo || null, + activeAt: c.activeAt || null, }) /** @@ -356,18 +378,40 @@ function adoptOlderStore (store = STORE) { /** Open, and released by the reviewer — what a tick hands over. */ const deliverable = comments => comments.filter(c => c.state === 'open' && c.sentAt) -/** Has the reviewer said anything the agent has not been given yet? A comment - * it has never seen, or an answer written since it last took delivery. */ -const unseen = comment => !comment.deliveredAt || - (comment.replies || []).some(reply => reply.by === REVIEWER_ROLE && - Date.parse(reply.at || '') > Date.parse(comment.deliveredAt)) -const anythingWaiting = (subject = here()) => - deliverable(loadComments(subject)).some(unseen) +/** When a comment became ready for another turn. A thread answer rejoins the + * same FIFO at the moment the reviewer wrote it; it does not jump ahead using + * the comment's original send time. */ +function readyAt (comment) { + if (!comment.deliveredAt) return Date.parse(comment.sentAt || '') || 0 + const delivered = Date.parse(comment.deliveredAt) || 0 + return Math.min(...(comment.replies || []) + .filter(reply => reply.by === REVIEWER_ROLE && (Date.parse(reply.at || '') || 0) > delivered) + .map(reply => Date.parse(reply.at)), Infinity) +} + +/** The next ready comment in arrival order. Array order settles comments saved + * in one browser request, which deliberately receive the same timestamp. */ +function nextReady (comments) { + return comments + .map((comment, order) => ({ comment, order, at: readyAt(comment) })) + .filter(item => Number.isFinite(item.at)) + .sort((a, b) => a.at - b.at || a.order - b.order)[0]?.comment || null +} + +/** One delivery owns the agent at a time. A question releases that ownership, + * but a silent delivered comment still blocks even when it came from a store + * written before `activeAt` existed. */ +const anythingWaiting = (subject = here()) => { + const comments = deliverable(loadComments(subject)) + if (comments.some(comment => comment.activeAt || + (comment.activeAt === undefined && unanswered(comment)))) return false + return !!nextReady(comments) +} /* ───────────────────────────── the brief ───────────────────────── - The open comments, written for the agent. It is rendered here rather than in - the workspace because the workspace does not know what a delivery is: a tick - hands over everything open, whenever each of them was written. */ + The active comment, written for the agent. It is rendered here rather than + in the workspace because only a delivery decides which ready comment owns + the next turn. */ const SCREENS = [ { id: 'ultrawide', label: 'Ultrawide', width: 2560, height: 1440 }, @@ -417,7 +461,7 @@ const coversLine = comment => comment.covers.map(c => `\`<${c.tag}>\` “${c.tex function renderBrief (subject, going, fresh) { const L = [] L.push(`# ${subject.live ? 'Live UI review' : 'Wireframe review'} — ${subject.name} · v${subject.state.version || 1}`) - L.push(`${going.length} open comment(s) — every one is a must` + + L.push(`${going.length} active comment — finish it or ask before taking the next` + (fresh.size ? ` · ${fresh.size} new since you last looked` : '')) L.push('') if (subject.live) { @@ -468,42 +512,88 @@ function renderBrief (subject, going, fresh) { } } L.push('---') - L.push('Close what you have done. Anything you do not name stays open and comes back next time,') - L.push('so ask about whatever is unclear instead of guessing:') + L.push('Close this comment when it is done, or ask about what is unclear. A question releases') + L.push('the queue for the next comment; the answer rejoins it in arrival order:') L.push('```bash') - L.push(`node review-server.mjs publish ${subject.flags} --close --label "" \\`) + L.push(`node review-server.mjs publish ${subject.flags} --close --label "" \\`) L.push(` --summary "" # optional, shown in the workspace`) L.push(`node review-server.mjs reply ${subject.flags} --comment --text ""`) L.push('```') return L.join('\n') + '\n' } -/** - * Hand every open comment to the agent, and record that it went. - * - * All of them, every time — not only the new ones. A comment the agent skipped - * comes back on the next tick, so the only way to be rid of one is to close it, - * and nothing can be forgotten by being missed. What is new since the last - * delivery is marked as such, which is a hint for where to look rather than a - * filter on what arrives. - */ -function deliver (subject = here()) { +/** Hand the oldest ready comment to the agent and record that it went. New + * comments and answered threads share one FIFO, and none can interrupt the + * comment already active. */ +function deliver (subject = here(), session = SESSION) { const comments = loadComments(subject) - const going = deliverable(comments) + const open = deliverable(comments) + const blocked = open.some(comment => comment.activeAt || + (comment.activeAt === undefined && unanswered(comment))) + const next = blocked ? null : nextReady(open) + const going = next ? [next] : [] const fresh = new Set(going.filter(comment => !comment.deliveredAt).map(comment => comment.id)) const at = new Date().toISOString() - /* The latest delivery owns the round: a comment handed over again binds to - whoever took it this time, which is also how a review adopted after its - session died changes hands. A watcher given no identity records none. */ - for (const comment of going) { comment.deliveredAt = at; comment.deliveredTo = SESSION } + /* A returned thread binds to whoever takes it this time, which is also how a + review adopted after its session died changes hands. */ + for (const comment of going) { + comment.deliveredAt = at + comment.deliveredTo = session + comment.activeAt = at + } saveComments(comments, subject) fs.mkdirSync(subject.store, { recursive: true }) writeAtomic(subject.brief, renderBrief(subject, going, fresh)) return { going, fresh } } -/* Presence is the watcher: it is the loop that takes delivery, so a live - heartbeat is a session that will be handed the next comment written. */ +/** + * A pull watcher reports that a delivery is available without claiming the + * comments on the agent's behalf. Reuse one durable token until somebody + * claims it, so a watcher restart and two simultaneous waits observe the same + * event instead of manufacturing competing deliveries. + */ +function offerDelivery (subject = here()) { + const file = path.join(subject.store, 'delivery-offer.json') + const current = readJSON(file) + if (current?.token) return current + const offer = { + token: randomBytes(8).toString('hex'), + at: new Date().toISOString(), + } + writeJSON(file, offer) + return offer +} + +/** + * Complete the second half of pull delivery. Only this command records + * `deliveredAt`, which means a REVIEW line left unread in a terminal buffer can + * never strand a comment in the agent's hands. + */ +function cmdClaim () { + const token = args.token && args.token !== true ? String(args.token) : null + if (!token) { + console.error('Need --token from a REVIEW offer') + process.exit(1) + } + const offer = readJSON(P.offer()) + if (!offer || offer.token !== token) { + console.error('That delivery offer is no longer available — run watch --next again.') + process.exit(2) + } + renewLease(P.listening()) + if (!anythingWaiting()) { + fs.rmSync(P.offer(), { force: true }) + console.log('Nothing to claim — that round was already taken or withdrawn.') + return + } + const { going, fresh } = deliver(here(), SESSION) + fs.rmSync(P.offer(), { force: true }) + console.log(`CLAIMED ${going.length} open${fresh.size ? `, ${fresh.size} new` : ''} · ${P.brief()}`) +} + +/* Presence is the delivery consumer: an answered push watcher, or a bounded + pull whose lease says the foreground call is still reaching the agent. */ const agentListening = () => someoneWatching() const openComments = () => loadComments() @@ -548,7 +638,7 @@ function cmdPublish (quiet) { would say in chat about the round it just finished. The workspace shows it where the news lands, so a reviewer who is not reading the terminal still gets the account of what changed. */ - const summary = args.summary && args.summary !== true ? String(args.summary).trim() : null + const summary = args.summary && args.summary !== true ? prose(args.summary).trim() : null /* A version is a frozen copy of the page under review, and a running app has no such thing: what a capture of one produces is a likeness with its scripts stripped and half its styling missing, which is worse than not offering it. @@ -574,6 +664,9 @@ function cmdPublish (quiet) { const at = new Date().toISOString() for (const id of ids) { const comment = byId.get(id) + // Naming the active comment hands its slot back even when the reviewer + // withdrew it first and closing is consequently a no-op. + comment.activeAt = null if (comment.state === 'closed') continue comment.state = 'closed' // What was just closed is what the reviewer wants to look at; what was @@ -622,10 +715,9 @@ function cmdPublish (quiet) { snapshot ? `Published v${n}` : null, ids.length ? `closed ${ids.length} comment(s)` : null, ].filter(Boolean).join(' — ') || 'Nothing to do') - /* Said here because here is where the agent believes it has finished. The - tick will not raise these again on its own — it wakes for what the - reviewer says, and they have said it already. */ - const left = loadComments().filter(comment => comment.state === 'open' && comment.deliveredAt) + /* Said here because an active comment left unnamed still owns the FIFO and + prevents the next delivery. */ + const left = loadComments().filter(comment => comment.state === 'open' && comment.activeAt) if (left.length) { console.log(`${left.length} comment(s) you were given are still open: ${left.map(c => c.id).join(', ')}`) console.log('Close them, or reply asking about them — leaving one silently leaves it on the reviewer.') @@ -673,16 +765,16 @@ function cmdReset (quiet) { */ function cmdReply () { const id = args.comment - const text = args.text - if (!id || !text) { + if (!id || !args.text || args.text === true) { console.error('Need --comment --text "…"') process.exit(1) } + const text = prose(args.text) /* A question the reviewer answers by picking rather than by typing. The options are offered on the comment, one of them can be marked as the one you would take, and the box to type something else is still there — a choice you did not think of is the whole reason the question was asked. */ - const options = repeatedArg('option') + const options = repeatedArg('option').map(prose) const recommend = args.recommend === undefined ? 0 : Number(args.recommend) if (options.length === 1) { console.error('A choice needs at least two --option values') @@ -705,6 +797,9 @@ function cmdReply () { ? { options: options.map((option, i) => ({ text: option, recommended: i + 1 === recommend })) } : {}), }) + // A question is a completed turn on this comment. Its answer will rejoin the + // FIFO when the reviewer writes it, while the next ready comment can proceed. + target.activeAt = null saveComments(comments) console.log(`Replied to ${id} — the reviewer will see it on the comment` + (options.length ? ` with ${options.length} options to pick from` : '')) @@ -985,6 +1080,7 @@ async function cmdStream (stores, label, all, subjectFlags) { const subject = subjectOf(store) if (anythingWaiting(subject)) { const { going, fresh } = withStoreLock(() => deliver(subject), store) + if (!going.length) continue say(`REVIEW ${label(store)} · ${going.length} open` + (fresh.size ? `, ${fresh.size} new` : '') + ` · ${subject.brief}`) } @@ -999,7 +1095,7 @@ async function cmdStream (stores, label, all, subjectFlags) { if (seen.has(store)) continue /* Not covered here and heartbeating anyway: another session's watcher has it, and it joins this one only once that heartbeat is gone. */ - if (watchingRecently(inStore(store, 'watching'))) continue + if (watchingRecently(inStore(store, 'watching')) || leasedRecently(inStore(store, 'listening'))) continue stores.push(store) seen.set(store, { flags: new Set() }) say(`OPENED ${label(store)} · now watching ${stores.length} review(s)`) @@ -1019,6 +1115,72 @@ async function cmdStream (stores, label, all, subjectFlags) { } } +/** + * `watch --next` — a bounded pull for Hosts whose terminal sessions do not + * push background stdout into an idle agent turn. + * + * The command returns inside the caller's foreground tool invocation, either + * with one event or with IDLE. A REVIEW is only an offer: `claim` is the point + * at which the agent proves it received the event and delivery is recorded. + */ +async function cmdNext (stores, label, all) { + const timeout = Math.max(1, Number(args.timeout) || 25) * 1000 + const until = Date.now() + timeout + const lease = startLease(() => stores.map(store => inStore(store, 'listening'))) + const finish = (line, code = 0) => { + lease.stop() + console.log(line) + process.exit(code) + } + const stop = code => { lease.stop(); process.exit(code) } + process.on('SIGINT', () => stop(130)) + process.on('SIGTERM', () => stop(143)) + + while (Date.now() < until) { + for (const store of [...stores]) { + const at = name => inStore(store, name) + if (!fs.existsSync(at('url'))) { + fs.rmSync(at('listening'), { force: true }) + stores = stores.filter(item => item !== store) + return finish(`CLOSED ${label(store)} · the tab went away`) + } + if (fs.existsSync(at('approved'))) return finish(`APPROVED ${label(store)} · read ${at('approved')}`) + if (fs.existsSync(at('share'))) return finish(`SHARE ${label(store)} · read ${at('share')}`) + + const subject = subjectOf(store) + if (anythingWaiting(subject)) { + const offered = withStoreLock(() => { + // Another pull consumer may have claimed between the outer check and + // this lock. In that case there is no event for this call after all. + if (!anythingWaiting(subject)) return null + return offerDelivery(subject) + }, store) + if (!offered) continue + lease.renew() + lease.stop() + console.log(`REVIEW ${label(store)} · delivery offered, not yet claimed · token ${offered.token}`) + console.log(`CLAIM node "${process.argv[1]}" claim ${subject.flags} --token ${offered.token}`) + return + } + } + + if (all) { + for (const store of liveStores()) { + if (stores.includes(store)) continue + /* A push watcher really owns delivery. Pull consumers may overlap: + they see one shared offer and `claim` serialises the winner. */ + if (watchingRecently(inStore(store, 'watching'))) continue + stores.push(store) + lease.renew() + } + } + await new Promise(resolve => setTimeout(resolve, 250)) + } + finish(stores.length + ? `IDLE no review event in ${Math.round(timeout / 1000)}s` + : 'CLOSED no live reviews remain') +} + async function cmdWatch () { // `--file` may be given more than once; parseArgs keeps only the last, so // read them off the raw argv. @@ -1032,7 +1194,9 @@ async function cmdWatch () { claimed store alone — it is found again the moment its watcher stops. A store named with `--file` is covered regardless: naming it is a deliberate takeover, which is how a review is adopted from a watcher that is stuck. */ - const unclaimed = store => !watchingRecently(inStore(store, 'watching')) + const next = args.next === true || args.next === 'true' + const unclaimed = store => !watchingRecently(inStore(store, 'watching')) && + (next || !leasedRecently(inStore(store, 'listening'))) let stores = [...(all ? liveStores().filter(unclaimed) : []), ...many.map(storeFor)] // Named subjects only. Never fall back to the placeholder STORE from // `watch --all` (cwd/.vstack/local/review) — that path is not a review store, and @@ -1050,6 +1214,8 @@ async function cmdWatch () { process.on('SIGTERM', () => { stopBeating(); process.exit(143) }) touch() // the page hears about it straight away + if (next) return cmdNext(stores, label, all) + if (args.stream === true || args.stream === 'true') { // Stream mode with --all and nothing live yet: wait for a serve to appear // instead of exiting. cmdStream's OPENED path picks new stores up. @@ -1091,6 +1257,7 @@ async function cmdWatch () { const subject = subjectOf(store) if (anythingWaiting(subject)) { const { going, fresh } = withStoreLock(() => deliver(subject), store) + if (!going.length) continue return done('REVIEW', store, subject.brief, ` · ${going.length} open${fresh.size ? `, ${fresh.size} new` : ''}`) } @@ -1125,7 +1292,11 @@ function cmdStatus () { comments: loadComments().map(comment => ({ id: comment.id, state: comment.state, - where: comment.sentAt ? (comment.deliveredAt ? 'with the agent' : 'queued') : 'still being written', + where: comment.state === 'closed' ? 'closed' + : !comment.sentAt ? 'still being written' + : comment.activeAt ? 'with the agent' + : (comment.replies || []).at(-1)?.by && + (comment.replies || []).at(-1).by !== REVIEWER_ROLE ? 'waiting for the reviewer' : 'queued', note: comment.note, })), approved: fs.existsSync(P.approved()) ? readJSON(P.approved(), {}) : null, @@ -1139,17 +1310,14 @@ function cmdStatus () { /** * A comment the agent was handed and has said nothing about since. * - * A delivered comment is answered by closing it or by replying to it. One that - * has neither is a round that stopped halfway, and nothing else in the protocol - * notices: the next tick only fires when the reviewer writes again, so an - * unanswered comment sits there for as long as they stay quiet. + * An active comment is answered by closing it or by replying to it. One that + * has neither is a turn that stopped halfway and keeps the FIFO blocked. * * It is settled by comparing what the agent has said against what it was given, - * not against the delivery itself: every tick re-stamps `deliveredAt` on every - * open comment, so a comment the agent asked a question about would fall behind - * its own delivery as soon as the reviewer wrote anything at all. + * not against the latest thread line. A reviewer answer written after delivery + * is queued for a later turn rather than owed by the current one. */ -const unanswered = comment => { +function unanswered (comment) { if (comment.state !== 'open' || !comment.deliveredAt) return false const when = reply => Date.parse(reply.at || '') || 0 const delivered = Date.parse(comment.deliveredAt) @@ -1255,6 +1423,7 @@ function readBody (req) { function payload () { const state = loadState() const versions = listVersions() + const comments = loadComments() return { mode: LIVE ? 'live' : 'local', // What this server is on now. A tab opened before an update still holds the @@ -1276,7 +1445,8 @@ function payload () { review as the tab that wrote it, with nothing to reconstruct. What the reviewer took off the list is the one thing left out: the record stays on disk so the agent holding it can still close it. */ - comments: loadComments().filter(comment => !comment.dismissedAt), + comments: comments.filter(comment => !comment.dismissedAt), + activeCount: comments.filter(comment => comment.activeAt).length, shareUrl: state.shareUrl || null, shareVersion: state.shareVersion || null, sharePending: fs.existsSync(P.share()), @@ -1329,7 +1499,7 @@ function acceptFromReviewer (incoming) { if (!raw?.id) continue const index = indexById.get(raw.id) if (index === undefined) { - const { state, deliveredAt, deliveredTo, ...rest } = raw + const { state, deliveredAt, deliveredTo, activeAt, ...rest } = raw const fresh = normaliseComment({ ...rest, state: 'open', deliveredAt: null, sentAt: raw.sentAt ? at : null }) indexById.set(fresh.id, comments.push(fresh) - 1) continue @@ -1658,12 +1828,10 @@ font:14px/1.6 ui-sans-serif,system-ui,-apple-system,sans-serif;color:#667;backgr return sendJSON(res, 200, { ok: true }) }) } - /* Give a stranded round back to the queue. - A delivered comment is not `unseen`, so a watcher armed after the session - behind it died blocks and hands over nothing: the round sits where no agent - can reach it and no tick will raise it. Clearing `deliveredAt` puts those - comments back where the state table says a sent, undelivered comment - belongs, and the next tick takes them. + /* Give a stranded active comment back to the queue. A question waiting on + the reviewer is not stranded agent work and keeps its delivery record. + Clearing the active comment's delivery returns it to the FIFO for the next + session to take. Refused while a heartbeat says someone is listening, because then the round is not stranded — an agent holds it and owes an answer on it, and handing the same comment to a second session is the race. */ @@ -1671,12 +1839,20 @@ font:14px/1.6 ui-sans-serif,system-ui,-apple-system,sans-serif;color:#667;backgr return withStoreLock(() => { if (agentListening()) { return sendJSON(res, 409, { - error: 'The agent is listening — it still has these. Reply on one to ask for it back.', + error: 'The agent is listening — it still has this comment.', }) } const comments = loadComments() - const stranded = comments.filter(comment => comment.state === 'open' && comment.deliveredAt) - for (const comment of stranded) { comment.deliveredAt = null; comment.deliveredTo = null } + const stranded = comments.filter(comment => comment.state === 'open' && + (comment.activeAt || (comment.activeAt === undefined && unanswered(comment)))) + for (const comment of comments) { + if (!comment.activeAt && !stranded.includes(comment)) continue + comment.activeAt = null + if (comment.state === 'open') { + comment.deliveredAt = null + comment.deliveredTo = null + } + } saveComments(comments) touch() return sendJSON(res, 200, { ok: true, requeued: stranded.map(comment => comment.id) }) @@ -1914,6 +2090,7 @@ switch (args._) { case 'publish': withStoreLock(() => cmdPublish()); break case 'reply': withStoreLock(cmdReply); break case 'ack': withStoreLock(cmdAck); break + case 'claim': withStoreLock(cmdClaim); break case 'share': withStoreLock(cmdShare); break case 'status': cmdStatus(); break case 'unanswered': cmdUnanswered(); break @@ -1921,6 +2098,6 @@ switch (args._) { case 'watch': cmdWatch(); break case 'serve': cmdServe(); break default: - console.error(`Unknown command "${args._}". Use: serve | watch | publish | reply | ack | share | status | unanswered | reset`) + console.error(`Unknown command "${args._}". Use: serve | watch | claim | publish | reply | ack | share | status | unanswered | reset`) process.exit(1) } diff --git a/plugins/vstack/skills/review/assets/workspace.html b/plugins/vstack/skills/review/assets/workspace.html index c21516d..7a8a518 100644 --- a/plugins/vstack/skills/review/assets/workspace.html +++ b/plugins/vstack/skills/review/assets/workspace.html @@ -2127,7 +2127,7 @@

const S = { runtime: 'local', name: 'Review', fileName: 'page.html', - html: '', versions: [], comments: [], + html: '', versions: [], comments: [], activeCount: 0, version: 1, viewing: 1, historyClearedAt: null, clearingHistory: false, resetting: false, /* `mode` is what the pointer does right now — View, or one of the annotate @@ -2184,6 +2184,7 @@

S.html = data.html || ''; S.versions = data.versions || []; S.comments = data.comments || []; + S.activeCount = data.activeCount || 0; S.version = S.viewing = data.currentVersion || 1; /* Whatever the agent last said is already said by the time the page opens — the banner is for a round landing while the reviewer watches, not for one @@ -2221,21 +2222,25 @@

} /* Everything the panel says about a comment is read off the comment. A comment - is open or closed; two timestamps say where it has got to between the - reviewer and Claude. Nothing here is remembered separately, so a reload and a - second tab see the same review as the tab that wrote it. */ + is open or closed; its delivery fields and thread say where it has got to + between the reviewer and the agent. Nothing here is remembered separately, + so a reload and a second tab see the same review as the tab that wrote it. */ const isClosed = a => a.state === 'closed'; const isOpen = a => !isClosed(a); /** Let go of by the reviewer: it is Claude's to answer, and its words are set. */ const sent = a => !!a.sentAt; -/** Written and released, but Claude has not been handed it yet. */ -const isQueued = a => !!a.sentAt && !a.deliveredAt; /** Claude spoke last and nobody has answered — the one state that waits on you. */ const awaitsReply = a => isOpen(a) && isAgent((a.replies || []).at(-1)?.by); -/** In Claude's hands: it went out with a delivery and has not come back closed. - A question is the exception — the comment is open and delivered, but the move - is the reviewer's, so progress on it would say the opposite of what is true. */ -const isWorking = a => !!a.deliveredAt && isOpen(a) && !awaitsReply(a); +/** A reviewer answer is a new place in the FIFO, dated when they wrote it. */ +const hasUnseenReply = a => (a.replies || []).some(reply => !isAgent(reply.by) && + Date.parse(reply.at || '') > Date.parse(a.deliveredAt || '')); +/** Written and ready, but not the one comment currently in the agent's hands. */ +const isQueued = a => !!a.sentAt && isOpen(a) && !awaitsReply(a) && !a.activeAt && + (!a.deliveredAt || hasUnseenReply(a)); +/** In the agent's hands. The fallback keeps a round written by an older server + truthful until that already-delivered work is answered. */ +const isWorking = a => isOpen(a) && !awaitsReply(a) && + (!!a.activeAt || (a.activeAt === undefined && !!a.deliveredAt && !hasUnseenReply(a))); function loadAnnotations () { const local = S.runtime === 'artifact' ? lsGet() : null; @@ -2296,6 +2301,7 @@

try { data = await (await fetch(API + '/project')).json() } catch { return } // The link can land on its own, without the page or the review moving. adoptShare(data, true); + S.activeCount = data.activeCount || 0; VSShell.setWatching(data.watching); noteWatching(data.watching); VSShell.setServerVersion(data.version); @@ -2325,7 +2331,7 @@

return; } S.pendingUpdate = data; - // Mid-edit saves are not news — the comments being worked on already say so. + // Mid-edit saves are not news — the active comment already says so. // Only a version that has actually landed is worth interrupting for. if (bumped) { /* What Claude closed is news whether or not the reviewer brings the new @@ -2387,7 +2393,7 @@

being worked on. A reload, a second tab and the tab that pressed Send all see the same thing. */ -/* How long nothing may be listening before a round in hand is called stalled. +/* How long nothing may be listening before the active comment is called stalled. A stream watcher keeps its heartbeat up for the whole time the agent works, so a heartbeat that stopped is a session that went away rather than a slow one. The wait is what keeps a session restarting mid-round from reading as a @@ -2404,12 +2410,12 @@

/** How long nothing has been listening. Zero while something is. */ const quietFor = () => S.watching === false && S.unwatchedAt ? Date.now() - S.unwatchedAt : 0; -/** A round in hand, quiet long enough to call the session behind it gone. +/** An active comment, quiet long enough to call the session behind it gone. Everything that would animate progress asks this before claiming any. */ -const isStalled = () => quietFor() >= STALL_MS && S.ann.some(isWorking); +const isStalled = () => quietFor() >= STALL_MS && S.activeCount > 0; function renderWork () { - const working = S.ann.filter(isWorking).length; + const working = Math.max(S.ann.filter(isWorking).length, S.activeCount); const queued = S.ann.filter(a => isQueued(a) && isOpen(a)).length; const quiet = working > 0 ? quietFor() : 0; const stalled = isStalled(); @@ -2435,13 +2441,12 @@

} /** - * Give a stalled round back to the queue. + * Give a stalled active comment back to the queue. * * A comment already handed over is not something a new watcher will ever be * given, so a session that died holding one strands it where nobody can reach - * it. This drops only the record of the handover: the comments stay open and - * still say what they said, and the next session to pick up is given them like - * any other delivery. + * it. This drops only the record of the handover: the comment stays open and + * still says what it said, and the next session can take it from the FIFO. */ async function sendAgain () { const again = $('#btnAgain'); @@ -2453,7 +2458,8 @@

if (response.status === 409) { toast(T('sendAgainRefused')); return } if (!response.ok) throw new Error(`requeue failed (${response.status})`); const back = new Set((await response.json()).requeued || []); - for (const a of S.ann) if (back.has(a.id)) a.deliveredAt = null; + for (const a of S.ann) if (back.has(a.id)) { a.deliveredAt = null; a.activeAt = null } + S.activeCount = 0; toast(T('sentAgain')); render(); } catch { toast(T('sendFail')) } @@ -2500,6 +2506,7 @@

// reads as one closed long ago, and drops straight into the Earlier fold // instead of standing where the reviewer can check it. if (incoming.deliveredAt !== local.deliveredAt) { local.deliveredAt = incoming.deliveredAt; changed = true } + if (incoming.activeAt !== local.activeAt) { local.activeAt = incoming.activeAt; changed = true } if (incoming.sentAt !== local.sentAt) { local.sentAt = incoming.sentAt; changed = true } if (incoming.closedAt !== local.closedAt) { local.closedAt = incoming.closedAt; changed = true } } @@ -4763,9 +4770,9 @@

}); } -/* Send stays Send, even with a round out. Noticing something else while Claude - works is the normal case, not an interruption to be blocked — the comment - goes out and joins the round. */ +/* Send stays Send while one comment is active. Noticing something else is the + normal case; the new comment takes its place in the FIFO without interrupting + what the agent is doing. */ function renderSendButton (open) { const btn = $('#btnSend'); // Both labels, and the bar's width picks one — the long form while there is diff --git a/plugins/vstack/skills/review/hosts/codex.md b/plugins/vstack/skills/review/hosts/codex.md index 87b81d1..e52d163 100644 --- a/plugins/vstack/skills/review/hosts/codex.md +++ b/plugins/vstack/skills/review/hosts/codex.md @@ -16,9 +16,9 @@ node "$SKILL/assets/review-server.mjs" serve --file "$FILE" --port 7788 --host c | Host op | Codex tool | How | | --- | --- | --- | | `background(cmd)` | persistent shell execution (`exec_command`) | Start with a short yield and retain the returned session id. The review server must stay alive. | -| `watch_stream(cmd)` | a second persistent `exec_command`, then `write_stdin` | Run `watch --all --stream`; poll the session with an empty write, normally for 30 seconds at a time, until it emits an event. Keep polling while reviews remain open. | -| `stop(handle)` | `write_stdin` | Send Ctrl-C (`\u0003`) to the retained server or watcher session. | -| `run(cmd)` | foreground `exec_command` | Use for `publish`, `reply`, `ack`, `share`, `status`, and `unanswered`. | +| `watch_next(cmd)` | foreground `exec_command` | Run `watch --all --next --timeout 25` with a 30-second tool yield. It completes inside the call with one event or `IDLE`; call it again while reviews remain open. | +| `stop(handle)` | `write_stdin` | Send Ctrl-C (`\u0003`) to the retained review-server session. The bounded watcher leaves no process to stop. | +| `run(cmd)` | foreground `exec_command` | Use for `claim`, `publish`, `reply`, `share`, `status`, and `unanswered`. | | `edit` | `apply_patch` | Change the wireframe or application source without overwriting unrelated work. | | `share(file)` | no generic public Artifact publisher | Profile uses `capabilities.share: copy`; offer the HTML file or an offline bundle instead of inventing a URL. | | `browser_capture` | Codex Browser controls, when installed | Navigate, resize, screenshot, and run `harvest-reference.js`. If Browser is unavailable, use screenshots supplied by the user. | @@ -34,36 +34,38 @@ node "$SKILL/assets/review-server.mjs" publish --file "$FILE" --label "Initial v # persistent exec session; retain its session id node "$SKILL/assets/review-server.mjs" serve --file "$FILE" --port 7788 --host codex -# second persistent exec session; retain and poll this session id -node "$SKILL/assets/review-server.mjs" watch --all --stream +# foreground exec_command, yield 30s; it returns within 25s +node "$SKILL/assets/review-server.mjs" watch --all --next --timeout 25 -# then answer the HANDSHAKE line it prints, in the foreground -node "$SKILL/assets/review-server.mjs" ack --all --token +# REVIEW prints this exact command; run it before reading brief.md +node "$SKILL/assets/review-server.mjs" claim --file --token ``` Tell the user **http://localhost:7788/** (or `/__review/` for live `--app`). ## Events and turn lifetime -Codex does not need a product-specific Monitor tool. The streaming watcher is a -normal persistent command session: - -1. Start it with `exec_command` and keep the returned session id. -2. Poll it with an empty `write_stdin`, using a bounded wait so the user keeps - receiving progress updates. -3. Answer the `HANDSHAKE` line the stream opens with, using `run`: `ack --all --token `. The watcher goes live once you do; answer within two minutes. -4. On `REVIEW`, `REPLIED`, `SHARE`, `APPROVED`, or `CLOSED`, follow - the core skill and review-loop contract. +Codex uses bounded pull delivery because output written by a background terminal +is not proof that the current agent turn read it: + +1. Run `watch --all --next --timeout 25` as a foreground `exec_command` with a + 30-second yield. Do not retain a watcher session id. +2. `IDLE` means call the same command again. The pull lease keeps the workspace + Linked between prompt re-arms and expires if this turn stops calling. +3. `REVIEW` is an offer, not a delivery. Run the exact `CLAIM` command printed + immediately. Only a successful claim marks the comment delivered and writes + `brief.md`; then read the brief and follow the core loop. +4. On `SHARE`, `APPROVED`, or `CLOSED`, follow the core skill and review-loop contract. 5. Run `unanswered --all` before you end a turn, and settle whatever it names. Codex cannot gate the end of a turn, so rule 14 of [review-loop.md](../../../contracts/review-loop.md) is yours to keep. -6. Resume polling after each publish. Do not send the final response +6. Resume bounded waits after each publish. Do not send the final response while the review is still active; keep the Codex turn open until approval, closure, or an explicit request from the user to stop. -Use a 30-second poll in normal operation. If nothing arrives, send a brief -commentary update before continuing so the user is never left without visible -activity for more than a minute. +Two Codex sessions may wait at once. They receive the same durable offer and +`claim` serialises delivery; the loser is told to wait again. A Claude/Grok push +watcher still has exclusive ownership, so a Codex pull does not steal its review. ## Share diff --git a/plugins/vstack/skills/review/references/workflow.md b/plugins/vstack/skills/review/references/workflow.md index 884fab2..5ce1eb0 100644 --- a/plugins/vstack/skills/review/references/workflow.md +++ b/plugins/vstack/skills/review/references/workflow.md @@ -16,7 +16,7 @@ wireframes/ candidate-pipeline/ state.json { name, version, file? | app? } comments.json every comment for this review — the whole truth - brief.md the open comments, rewritten on every delivery + brief.md the active comment, rewritten on every delivery versions/v1.html frozen copy of each published version versions/v1.meta.json label and date reviews/v1/ only ever read — where an older version kept its comments @@ -135,7 +135,7 @@ WATCHING 2 review(s): wireframe, spec-tree HANDSHAKE this stream is not live until you answer it. Run now: node …/review-server.mjs ack --all --token 7f3a91 LINKED handshake answered — the workspace says Linked from here -REVIEW wireframe · 3 open, 1 new · …/.vstack/local/review/wireframe/brief.md +REVIEW wireframe · 1 open, 1 new · …/.vstack/local/review/wireframe/brief.md OPENED story-map-template · now watching 3 review(s) CLOSED spec-tree · the tab went away ``` @@ -153,7 +153,7 @@ its handshake is answered. Nothing is listening to any workspace at that point, whatever the handshake proved: start it again with `--file `. First thing after a `REVIEW`: read the `brief.md` it names. Delivery is already -recorded, so the workspace shows those comments as being worked on. Use `share --url` +recorded, so the workspace shows that comment as being worked on. Use `share --url` after publishing a link. A one-shot form (`watch` without `--stream`) still exists: it exits on the first @@ -171,8 +171,8 @@ one leaves loops polling paths that no longer exist. ## Reading the brief -`brief.md` is every open comment, grouped by screen size and rewritten on each -delivery. `comments.json` beside it is the same data structured. Each comment: +`brief.md` is the one active comment, grouped by screen size and rewritten on +each delivery. `comments.json` beside it holds the whole queue. Each comment: | field | meaning | |---|---| @@ -235,10 +235,9 @@ the desktop one. ## The conversation -The reviewer has no resolve button — `publish --close ` is the only thing -that closes a comment out, and nothing they do can refuse it. Anything you do -not name stays open and comes back on the next delivery, so a partial answer is -a normal one. +The reviewer has no resolve button — `publish --close ` is the only thing +that closes a comment out, and nothing they do can refuse it. A delivery contains +one comment. Close it or reply before the FIFO advances. They can take back a comment you have not been handed yet, but they cannot mark one done. Emptying a comment's text deletes it, so an empty comment never reaches you. diff --git a/plugins/vstack/skills/review/tests/host-profiles.mjs b/plugins/vstack/skills/review/tests/host-profiles.mjs index ebf0100..05ad200 100644 --- a/plugins/vstack/skills/review/tests/host-profiles.mjs +++ b/plugins/vstack/skills/review/tests/host-profiles.mjs @@ -16,7 +16,8 @@ assert.equal(codex.id, 'codex') assert.equal(codex.name, 'Codex') assert.deepEqual(codex.capabilities, { share: 'copy', - watch: 'stream', + watch: 'pull', + turnGate: false, browser: true, updateDetect: 'codex-install', }) @@ -40,10 +41,11 @@ for (const id of listHosts()) { assert.equal(p.id, id, `${where}: id matches filename`) assert.ok(p.name.length >= 1, `${where}: name`) const c = p.capabilities - assert.deepEqual(Object.keys(c).sort(), ['browser', 'share', 'updateDetect', 'watch'], + assert.deepEqual(Object.keys(c).sort(), ['browser', 'share', 'turnGate', 'updateDetect', 'watch'], `${where}: capabilities keys`) assert.ok(['artifact', 'copy', 'none'].includes(c.share), `${where}: share enum`) - assert.ok(['stream', 'oneshot'].includes(c.watch), `${where}: watch enum`) + assert.ok(['stream', 'pull'].includes(c.watch), `${where}: watch enum`) + assert.equal(typeof c.turnGate, 'boolean', `${where}: turnGate`) assert.equal(typeof c.browser, 'boolean', `${where}: browser`) assert.ok(['claude-install', 'codex-install', 'none'].includes(c.updateDetect), `${where}: updateDetect enum`) if (p.install) { diff --git a/plugins/vstack/skills/review/tests/review-lifecycle.mjs b/plugins/vstack/skills/review/tests/review-lifecycle.mjs index f81e14b..27a08fa 100644 --- a/plugins/vstack/skills/review/tests/review-lifecycle.mjs +++ b/plugins/vstack/skills/review/tests/review-lifecycle.mjs @@ -75,6 +75,15 @@ try { assert.equal(cli('publish', '--label', 'Initial').status, 0) await startServer() + /* ── a bounded pull completes inside one foreground call ── */ + + const idlePull = cli('watch', '--next', '--timeout', '1') + assert.equal(idlePull.status, 0, idlePull.stderr) + assert.match(idlePull.stdout, /IDLE/, 'a quiet pull returns instead of leaving a terminal session behind') + const idleLease = path.join(store, 'listening') + assert.ok(fs.existsSync(idleLease), 'the wait leaves a lease across the prompt re-arm gap') + fs.rmSync(idleLease, { force: true }) + /* ── a comment is the reviewer's until they let go of it ── */ await write([comment('c1', 'First draft')]) @@ -97,15 +106,21 @@ try { assert.equal(byId('c1').note, 'First, reworded', 'a comment already sent cannot be reworded, whatever a stale tab saves') - /* ── the tick hands over everything open ── */ + /* ── the tick hands over one comment at a time, FIFO ── */ let out = tick() assert.match(out, /REVIEW/) - assert.match(out, /2 open, 2 new/) + assert.match(out, /1 open, 1 new/) assert.match(briefText(), /### c1 · NEW/) assert.match(briefText(), /First, reworded/) - assert.match(briefText(), /--close /) + assert.doesNotMatch(briefText(), /Second/, 'the next comment does not distract from the active one') + assert.match(briefText(), /--close /) assert.ok(byId('c1').deliveredAt, 'delivery is recorded on the comment') + assert.ok(byId('c1').activeAt, 'the delivered comment owns the single active slot') + assert.equal(byId('c2').deliveredAt, null, 'the second comment remains queued') + const uninterrupted = cli('watch', '--next', '--timeout', '1') + assert.match(uninterrupted.stdout, /IDLE/, 'a waiting comment cannot raise another delivery mid-turn') + fs.rmSync(path.join(store, 'listening'), { force: true }) /* ── closing is the agent's alone, and partial by design ── */ @@ -113,10 +128,9 @@ try { assert.equal(published.status, 0) assert.equal(byId('c1').state, 'closed') assert.equal(byId('c2').state, 'open', 'a comment nobody named is still open') + assert.equal(byId('c1').activeAt, null, 'closing releases the active slot') assert.equal(JSON.parse(fs.readFileSync(path.join(store, 'state.json'))).version, 2) - // The tick wakes for what the reviewer says, so a comment left open has to be - // named where the agent believes it has finished. - assert.match(published.stdout, /1 comment\(s\) you were given are still open: c2/) + assert.doesNotMatch(published.stdout, /still open/, 'queued work is not work the agent silently left behind') assert.equal(cli('publish', '--close', 'c1').status, 0, 'closing what is closed is a no-op') assert.equal(JSON.parse(fs.readFileSync(path.join(store, 'state.json'))).version, 2, @@ -127,10 +141,20 @@ try { await write([{ ...byId('c1'), replies: [{ by: 'reviewer', text: 'Not like that', at: new Date().toISOString() }] }]) assert.equal(byId('c1').state, 'open', 'answering something called done says it is not done') out = tick() - assert.match(out, /2 open/, 'everything open goes, not only what was just said') - assert.doesNotMatch(out, /new/, 'a comment coming round again is not new') + assert.match(out, /1 open, 1 new/, 'the older queued comment keeps its place ahead of the reopened one') + assert.match(briefText(), /### c2 · NEW/) + assert.doesNotMatch(briefText(), /Not like that/) + + /* ── a question releases the slot; its answer rejoins the FIFO ── */ + + assert.equal(cli('reply', '--comment', 'c2', '--text', 'Which card?').status, 0) + assert.equal(byId('c2').activeAt, null, 'asking releases the active slot') + assert.equal(byId('c2').state, 'open', 'asking is not a state — the comment stays open') + out = tick() + assert.match(out, /1 open/, 'the next ready comment proceeds while c2 waits on the reviewer') + assert.doesNotMatch(out, /new/, 'a reopened comment is not new') assert.match(briefText(), /They replied:\*\* Not like that/) - assert.match(briefText(), /### c2/) + assert.doesNotMatch(briefText(), /### c2/) /* ── a stranded round goes back to the queue ── */ @@ -140,27 +164,24 @@ try { assert.equal(held.response.status, 409, 'nothing is taken off an agent that is listening') assert.ok(byId('c1').deliveredAt, 'and the handover stands') - /* Nothing listening: both comments are delivered and neither is unseen, so no - watcher started after this point would ever be handed them. */ + /* Nothing listening: only the active comment is stranded. The question on c2 + is still where it belongs, waiting on the reviewer. */ const note = byId('c1').note fs.rmSync(watching, { force: true }) const requeued = await post('/api/comments/requeue', {}) assert.equal(requeued.response.status, 200) - assert.deepEqual(requeued.body.requeued.sort(), ['c1', 'c2']) + assert.deepEqual(requeued.body.requeued, ['c1']) assert.equal(byId('c1').deliveredAt, null, 'only the record of the handover goes') + assert.ok(byId('c2').deliveredAt, 'a question waiting on the reviewer is not requeued') assert.equal(byId('c1').state, 'open', 'the comment itself is untouched') assert.equal(byId('c1').note, note, 'and it still says what it said') out = tick() assert.match(out, /REVIEW/, 'the next session to pick up is handed the stranded round') - // New to the session receiving them, which is the whole point of putting them - // back: the one that was given them first is gone. - assert.match(out, /2 open, 2 new/) + assert.match(out, /1 open, 1 new/, 'the recovered comment is new to the session receiving it') /* ── the thread is append-only, from either side ── */ - assert.equal(cli('reply', '--comment', 'c2', '--text', 'Which card?').status, 0) - assert.equal(byId('c2').state, 'open', 'asking is not a state — the comment stays open') // A tab that never saw the agent's question saves its own copy of the thread. await write([{ ...comment('c2', 'Second', { sentAt: byId('c2').sentAt }), @@ -169,9 +190,23 @@ try { assert.deepEqual(byId('c2').replies.map(reply => reply.by), ['agent', 'reviewer'], 'neither side can lose a line of the thread by being stale') + assert.equal(cli('publish', '--close', 'c1', '--label', 'First done').status, 0) + out = tick() + assert.match(out, /1 open/, 'the answered thread returns after the active comment finishes') + assert.match(briefText(), /They replied:\*\* The second one/) + + /* ── prose flags carry paragraph breaks a shell would not resolve ── */ + + assert.equal(cli('reply', '--comment', 'c2', + '--text', 'One.\\n\\nTwo.', + '--option', 'Split on \\\\n', '--option', 'Keep\\nboth').status, 0) + const escaped = byId('c2').replies.at(-1) + assert.equal(escaped.text, 'One.\n\nTwo.', 'an escaped break reaches the reviewer as a break') + assert.deepEqual(escaped.options.map(option => option.text), ['Split on \\n', 'Keep\nboth'], + 'a doubled backslash is how an option says the two characters') + /* ── liveness: whatever the reviewer did meanwhile, the agent can finish ── */ - tick() assert.equal(cli('publish', '--close', 'c1,c2', '--label', 'Both done').status, 0, 'nothing the reviewer does can stop the agent closing what it was given') assert.deepEqual(stored().map(item => item.state), ['closed', 'closed']) @@ -196,8 +231,11 @@ try { const gone = await request('/api/project') assert.equal(gone.body.comments.find(item => item.id === 'c5'), undefined, 'the workspace never shows it again') + assert.equal(gone.body.activeCount, 1, + 'the workspace can still recover a hidden active comment if its session dies') assert.equal(cli('publish', '--close', 'c5').status, 0, 'the agent holding it can still close what it was given') + assert.equal((await request('/api/project')).body.activeCount, 0, 'closing releases the hidden slot') /* ── the agent cannot close what it was never given ── */ @@ -211,6 +249,48 @@ try { assert.match(unknown.stderr, /c404 is not a comment on this review/) assert.equal(byId('c7').state, 'open', 'a rejected close changes nothing at all') + /* ── pull delivery is offered, then explicitly claimed ── */ + + const firstOffer = cli('watch', '--next', '--timeout', '1') + assert.equal(firstOffer.status, 0, firstOffer.stderr) + const token = firstOffer.stdout.match(/token ([0-9a-f]+)/)?.[1] + assert.ok(token, `the pull names its durable offer:\n${firstOffer.stdout}`) + assert.equal(byId('c7').deliveredAt, null, + 'writing REVIEW output is not delivery until the agent reads and claims it') + assert.equal(cli('unanswered', '--all').status, 0, + 'an unread offer cannot make the agent owe a round') + + const competingOffer = cli('watch', '--next', '--timeout', '1') + assert.match(competingOffer.stdout, new RegExp(`token ${token}`), + 'overlapping pull consumers see one shared offer') + assert.equal(byId('c7').deliveredAt, null, 'neither consumer wins merely by waiting') + + const claimed = cli('claim', '--token', token, '--session', 'pull-a') + assert.equal(claimed.status, 0, claimed.stderr) + assert.match(claimed.stdout, /CLAIMED/) + assert.ok(byId('c7').deliveredAt, 'claim is the delivery point') + assert.equal(byId('c7').deliveredTo, 'pull-a', 'claim binds delivery to its session') + assert.equal(cli('claim', '--token', token, '--session', 'pull-b').status, 2, + 'a competing claim cannot deliver the same offer twice') + + const listening = path.join(store, 'listening') + assert.ok(fs.existsSync(listening), 'the bounded pull leaves a short consumer lease') + const heldByPull = await post('/api/comments/requeue', {}) + assert.equal(heldByPull.response.status, 409, 'a fresh pull lease protects a claimed round') + + const expired = new Date(Date.now() - 60_000) + fs.utimesSync(listening, expired, expired) + const pullRequeued = await post('/api/comments/requeue', {}) + assert.equal(pullRequeued.response.status, 200, 'an abandoned pull lease ages out') + assert.deepEqual(pullRequeued.body.requeued, ['c7']) + assert.equal(byId('c7').deliveredAt, null, 'the abandoned round becomes available again') + + const nextOffer = cli('watch', '--next', '--timeout', '1') + const nextToken = nextOffer.stdout.match(/token ([0-9a-f]+)/)?.[1] + assert.ok(nextToken && nextToken !== token, 'a requeued round receives a fresh offer') + assert.equal(cli('claim', '--token', nextToken).status, 0) + assert.equal(cli('publish', '--close', 'c7', '--label', 'Seventh done').status, 0) + /* ── a store written by an older version is read where it lies ── */ const old = path.join(temp, 'old') diff --git a/plugins/vstack/skills/review/tests/round-gate.mjs b/plugins/vstack/skills/review/tests/round-gate.mjs index 0c19e1d..8430bd5 100644 --- a/plugins/vstack/skills/review/tests/round-gate.mjs +++ b/plugins/vstack/skills/review/tests/round-gate.mjs @@ -93,16 +93,15 @@ try { assert.equal(cli('reply', '--file', page, '--comment', 'c1', '--text', 'How much bigger?').status, 0) assert.equal(cli('unanswered', '--all').status, 0, 'a reply hands the round back') - // The reviewer writes about something else while c1 waits on them. Every tick - // re-stamps delivery on every open comment, so c1 is handed over again — but - // the agent has had its say on it and owes only the comment it has not. + // The reviewer writes about something else while c1 waits on them. Asking + // released the active slot, so the new comment can take its own turn. await post('/api/comments', { comments: [comment('c3', 'And centre the footer')] }) assert.match(tick().stdout, /REVIEW/, 'the new comment is handed over') const alongside = cli('unanswered', '--all') assert.equal(alongside.status, 1, 'the comment nothing has been said about is outstanding') assert.match(alongside.stdout, /c3/, 'it names that comment') assert.doesNotMatch(alongside.stdout, /c1/, - 'being handed a comment again does not unanswer the reply already on it') + 'a question waiting on the reviewer is not delivered alongside new work') assert.equal(cli('publish', '--file', page, '--close', 'c3', '--label', 'Footer centred').status, 0) // The reviewer answers. That comment is waiting for the next tick, not for From a70032148ec6edb034efb94c4684e676d0e9aaf2 Mon Sep 17 00:00:00 2001 From: DeyangChan Date: Mon, 10 Aug 2026 22:13:39 +0800 Subject: [PATCH 8/9] A bounded wait names the command that resumes it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pull Host's review loop continues only while the agent keeps calling the bounded wait, and every exit from that loop was silent about it. The wait returned `IDLE` after 25 seconds and said nothing further; a claim reported the round it handed over and nothing further. Nothing was left running to notice the calls had stopped, so the lease aged out, the workspace went Unlinked, and the comments the reviewer kept sending sat undelivered. Each of those exits now prints the command that resumes the wait, which is what the one-shot push form has always done. Arguments are quoted, so a page under a path with a space survives being copied back out. `unanswered` was the check that should have caught it and instead confirmed the mistake: with nothing ever delivered, it reported that every comment was closed or answered. It now names a live review that has comments waiting with no watcher behind it, and gives the command that starts watching again. It stays quiet while a pull lease is fresh, so the ordinary gap between calls is not reported as a fault, and its exit code is unchanged — only a round this session was handed and left unanswered blocks a turn, as rule 14 says. The lifecycle suite now runs from a directory whose name contains a space, which is what caught the quoting. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 17 ++++++ plugins/vstack/.claude-plugin/plugin.json | 2 +- plugins/vstack/.codex-plugin/plugin.json | 2 +- plugins/vstack/contracts/review-loop.md | 3 +- plugins/vstack/skills/review/SKILL.md | 6 +++ .../skills/review/assets/review-server.mjs | 52 ++++++++++++++++--- plugins/vstack/skills/review/hosts/codex.md | 9 +++- .../skills/review/tests/review-lifecycle.mjs | 33 +++++++++++- 8 files changed, 112 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9dd6ef6..4ef70c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,23 @@ The version in `plugins/vstack/.claude-plugin/plugin.json` is what your host compares against to decide an update is available. See the release checklist in [`CONTRIBUTING.md`](CONTRIBUTING.md). +## 6.7.0 — 2026-08-10 + +**Fixed** + +- **Comments no longer pile up undelivered when Codex stops polling.** Codex + receives comments through a bounded wait that returns every 25 seconds, and + the review continues only while the agent keeps calling it. Each wait now ends + by printing the exact command that resumes it, so a loop that was about to be + dropped names its own next step. Taking a comment prints the same line, since + answering one comment is not the end of the review. +- **`unanswered` no longer reports all-clear over a review nobody is watching.** + It now names a live review that has comments waiting with no watcher behind + it, and gives the command that starts watching again. Codex runs this check + before ending a turn, and it previously said nothing while a queue sat + undelivered. Comments that were never delivered still do not block the end of + a turn in Claude Code — they are reported, not gated. + ## 6.6.0 — 2026-08-10 **Changed** diff --git a/plugins/vstack/.claude-plugin/plugin.json b/plugins/vstack/.claude-plugin/plugin.json index 325e0e3..d629b4e 100644 --- a/plugins/vstack/.claude-plugin/plugin.json +++ b/plugins/vstack/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "vstack", "displayName": "Visual Stack", - "version": "6.6.0", + "version": "6.7.0", "description": "Stop prompting. Start pointing. Visual Stack adds a Figma-like feedback layer to Claude Code. Create a new screen from a prompt, screenshot, reference site, or design system, or open an app you already have running. Click any element and leave feedback exactly where the problem is hiding, and Claude publishes the next revision into the same workspace. Compare revisions on a timeline, preview desktop, tablet, and mobile layouts, and keep every comment attached to the element, route, and version it refers to. Wireframes are self-contained HTML and review state stays in your project.", "author": { "name": "Cavalry Collective", diff --git a/plugins/vstack/.codex-plugin/plugin.json b/plugins/vstack/.codex-plugin/plugin.json index 97440fb..97baba5 100644 --- a/plugins/vstack/.codex-plugin/plugin.json +++ b/plugins/vstack/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "vstack", - "version": "6.6.0", + "version": "6.7.0", "description": "Stop prompting. Start pointing. Visual Stack adds a Figma-like feedback layer to Codex. Create a new screen from a prompt, screenshot, reference site, or design system, or open an app you already have running. Click any element and leave feedback exactly where the problem is hiding, and Codex publishes the next revision into the same workspace. Compare revisions on a timeline, preview desktop, tablet, and mobile layouts, and keep every comment attached to the element, route, and version it refers to. Wireframes are self-contained HTML and review state stays in your project.", "author": { "name": "Cavalry Collective", diff --git a/plugins/vstack/contracts/review-loop.md b/plugins/vstack/contracts/review-loop.md index c86b6a8..86e46a7 100644 --- a/plugins/vstack/contracts/review-loop.md +++ b/plugins/vstack/contracts/review-loop.md @@ -155,7 +155,7 @@ Host selection: `--host ` or `VSTACK_HOST=` (affects UI injection only). | `reply --file/name … --comment --text "…" [--option "…" … --recommend ]` | Append `{ by: "agent", text, at }`, with `options: [{ text, recommended }]` when options are given. The reviewer answers by pressing one, which posts those words as their reply | | `share --file/name … --url ` | Record public URL; clear the `share` sentinel | | `status --file/name …` | Human/debug snapshot | -| `unanswered [--all] [--file/name …] [--session ]` | Comments the agent was handed and has not answered. Exits 1 while any remain. With `--session`, only deliveries recorded for that id count | +| `unanswered [--all] [--file/name …] [--session ]` | Comments the agent was handed and has not answered. Exits 1 while any remain. With `--session`, only deliveries recorded for that id count. Also names a live review that has comments waiting and no watcher behind it, at exit 0 — that queue is nobody's round yet, so it reports rather than blocks | | `reset --file/name …` | Delete every comment and version for the review, and start again at v1 | --- @@ -172,6 +172,7 @@ One line of stdout per event. Streams stay open; bounded pulls return one: | `UNLINKED` | The handshake was answered and no review turned up to cover | Start it again with `--file` if a review is running elsewhere | | `UNWIRED` | The handshake went unanswered; the watcher exits `3` | Start it again via `watch_stream` | | `IDLE` | Bounded pull timed out without an event | Run `watch_next` again while the review remains open | +| `WAIT` | Follows `IDLE` and `CLAIMED`; carries the command that resumes the bounded wait | Run it — after answering the round, in the `CLAIMED` case | | `REVIEW` | Stream: one comment delivered. Pull: token offered, nothing delivered yet | Pull runs printed `claim`; then read `brief.md`, apply it, `publish --close` / `reply` | | `SHARE` | Link requested | Host `share` if capable; then `share --url` | | `APPROVED` | Sign-off; server exiting | Confirm; next pipeline stage as skill says | diff --git a/plugins/vstack/skills/review/SKILL.md b/plugins/vstack/skills/review/SKILL.md index 5022f05..7756607 100644 --- a/plugins/vstack/skills/review/SKILL.md +++ b/plugins/vstack/skills/review/SKILL.md @@ -192,6 +192,11 @@ again. On `REVIEW`, run the exact `CLAIM` command it prints immediately; only th comment to you and writes `brief.md`. An unread REVIEW offer leaves the comment queued, so another call or session can still receive it. +A bounded call delivers nothing once it has returned, and no process is left behind to notice that +you stopped calling. Comments the reviewer sends then sit undelivered. Every call that ends prints +the command that resumes it on a `WAIT` line, including the one you claim from — answer the round, +then start the next wait. + The page says **Linked** while a push watcher is answered or a pull call's short consumer lease is fresh, and **Unlinked** in amber the rest of the time. @@ -204,6 +209,7 @@ Each event is one line (full table: `contracts/review-loop.md`): | **`UNLINKED`** | the handshake is answered, but the watcher found no review to cover, so no workspace says Linked | start it again with `--file ` if a review is already running for a page outside this directory. A serve started here after it needs nothing | | **`UNWIRED`** | the handshake went unanswered and the watcher exited | start it again with the tool your adapter names for `watch_stream` | | **`IDLE`** | a bounded pull ended without an event | call `watch_next` again while the review is open | +| **`WAIT`** | follows `IDLE` and `CLAIMED`, carrying the command that resumes the bounded wait | run it — after you have answered the round, in the `CLAIMED` case | | **`REVIEW`** | push: one comment was delivered; pull: a token was offered | pull Hosts run the printed `CLAIM` command first, then read the brief and continue below | | **`SHARE`** | they want a link to send someone | Host op `share` if capable, then §6; if the Host cannot share publicly, say so and offer a file/bundle instead | | **`APPROVED`** | the design is signed off; the server has closed itself | say it's approved, note any `openComments` deliberately left, and carry on with whatever comes next | diff --git a/plugins/vstack/skills/review/assets/review-server.mjs b/plugins/vstack/skills/review/assets/review-server.mjs index 4264268..3095b40 100644 --- a/plugins/vstack/skills/review/assets/review-server.mjs +++ b/plugins/vstack/skills/review/assets/review-server.mjs @@ -587,9 +587,15 @@ function cmdClaim () { console.log('Nothing to claim — that round was already taken or withdrawn.') return } - const { going, fresh } = deliver(here(), SESSION) + const subject = here() + const { going, fresh } = deliver(subject, SESSION) fs.rmSync(P.offer(), { force: true }) console.log(`CLAIMED ${going.length} open${fresh.size ? `, ${fresh.size} new` : ''} · ${P.brief()}`) + /* Claiming is the only way out of the bounded wait that leads somewhere else, + and answering one comment is not the end of the review. Whatever the + reviewer sends next waits until this loop is running again. */ + console.log(`WAIT answer it, then wait for the next comment:`) + console.log(` node "${process.argv[1]}" watch --all ${subject.flags} --next --timeout 25`) } /* Presence is the delivery consumer: an answered push watcher, or a bounded @@ -848,6 +854,14 @@ const storeFor = f => { } const inStore = (store, name) => path.join(store, name) +/* This exact invocation, for an agent to run again. A watch that ends is put + back only by being copied out of its own output, so an argument holding a + space has to survive the trip — a page under "My Projects" is otherwise a + command that silently watches the wrong thing. */ +const thisCommand = () => `node "${process.argv[1]}" ` + process.argv.slice(2) + .map(arg => /[\s"]/.test(arg) ? `"${arg.replace(/(["\\])/g, '\\$1')}"` : arg) + .join(' ') + /* Where a server records the store it is serving, for the benefit of a watcher that cannot walk to it. @@ -1126,6 +1140,12 @@ async function cmdStream (stores, label, all, subjectFlags) { async function cmdNext (stores, label, all) { const timeout = Math.max(1, Number(args.timeout) || 25) * 1000 const until = Date.now() + timeout + /* A bounded wait ends every few seconds, and the loop only continues because + the agent calls again. Nothing else is left running to notice that it + stopped: the lease ages out, the workspace goes Unlinked, and the comments + the reviewer keeps sending sit undelivered. So the wait ends by printing + the command that resumes it, as the one-shot form does. */ + const rearm = thisCommand() const lease = startLease(() => stores.map(store => inStore(store, 'listening'))) const finish = (line, code = 0) => { lease.stop() @@ -1176,9 +1196,10 @@ async function cmdNext (stores, label, all) { } await new Promise(resolve => setTimeout(resolve, 250)) } - finish(stores.length - ? `IDLE no review event in ${Math.round(timeout / 1000)}s` - : 'CLOSED no live reviews remain') + if (!stores.length) return finish('CLOSED no live reviews remain') + finish(`IDLE no review event in ${Math.round(timeout / 1000)}s\n` + + `WAIT the review is still open and nothing is reading it until you call again:\n` + + ` ${rearm}`) } async function cmdWatch () { @@ -1236,7 +1257,7 @@ async function cmdWatch () { the easiest thing in the world to forget, and a forgotten waiter is a review nobody is reading. So the last thing printed is the command that puts it back. Prefer `watch --stream` via Host op watch_stream. */ - const rearm = `node "${process.argv[1]}" ${process.argv.slice(2).join(' ')}` + const rearm = thisCommand() const done = (what, store, file, detail = '') => { stopBeating() console.log(`${what} ${label(store)}${detail}`) @@ -1355,10 +1376,18 @@ function cmdUnanswered () { const stores = args.all === true || args.all === 'true' ? liveStores() : [STORE] const bin = process.argv[1] let owing = 0 + const stranded = [] for (const store of stores) { const subject = subjectOf(store) const owed = loadComments(subject).filter(comment => unanswered(comment) && (!SESSION || comment.deliveredTo === SESSION)) + /* A live review with comments ready and no watcher behind it. They are not + owed by this session — nothing ever delivered them — but this is the + question an agent asks before ending its turn, and answering "nothing + outstanding" is how a dropped watch loop stays dropped. */ + if (!watchingRecently(inStore(store, 'watching')) && + !leasedRecently(inStore(store, 'listening')) && + anythingWaiting(subject)) stranded.push(subject) if (!owed.length) continue owing += owed.length const them = owed.length > 1 ? 'them' : 'it' @@ -1368,7 +1397,18 @@ function cmdUnanswered () { console.log(` node "${bin}" publish ${subject.flags} --close ${owed.map(comment => comment.id).join(',')} --label "what changed"`) console.log(` node "${bin}" reply ${subject.flags} --comment ${owed[0].id} --text "your question"`) } - if (!owing) console.log('Nothing outstanding — every comment you were handed is closed or answered.') + for (const subject of stranded) { + console.log(`Review "${subject.name}" — comments are waiting and nothing is watching for them.`) + console.log(`Start the watch operation your Host adapter names, and keep it running:`) + console.log(` node "${bin}" watch --all ${subject.flags} --stream # push Host`) + console.log(` node "${bin}" watch --all ${subject.flags} --next --timeout 25 # pull Host, called again each time it returns`) + } + if (!owing && !stranded.length) { + console.log('Nothing outstanding — every comment you were handed is closed or answered.') + } + /* Only a round this session was handed and left unanswered blocks a turn: + rule 14 of contracts/review-loop.md, which the Claude Stop hook enforces by + reading this exit code. A stranded review is reported, not blocked on. */ process.exit(owing ? 1 : 0) } diff --git a/plugins/vstack/skills/review/hosts/codex.md b/plugins/vstack/skills/review/hosts/codex.md index e52d163..29ddf01 100644 --- a/plugins/vstack/skills/review/hosts/codex.md +++ b/plugins/vstack/skills/review/hosts/codex.md @@ -51,13 +51,18 @@ is not proof that the current agent turn read it: 1. Run `watch --all --next --timeout 25` as a foreground `exec_command` with a 30-second yield. Do not retain a watcher session id. 2. `IDLE` means call the same command again. The pull lease keeps the workspace - Linked between prompt re-arms and expires if this turn stops calling. + Linked between prompt re-arms and expires if this turn stops calling. Every + bounded wait that ends prints a `WAIT` line carrying the exact command that + resumes it; nothing delivers a comment while no call is in flight. 3. `REVIEW` is an offer, not a delivery. Run the exact `CLAIM` command printed immediately. Only a successful claim marks the comment delivered and writes `brief.md`; then read the brief and follow the core loop. 4. On `SHARE`, `APPROVED`, or `CLOSED`, follow the core skill and review-loop contract. 5. Run `unanswered --all` before you end a turn, and settle whatever it names. - Codex cannot gate the end of a turn, so rule 14 of + It names two things: a round you were handed and left unanswered, which you + finish with `publish` or `reply`; and a live review with comments waiting and + nothing watching, which you settle by starting the bounded wait again. Codex + cannot gate the end of a turn, so rule 14 of [review-loop.md](../../../contracts/review-loop.md) is yours to keep. 6. Resume bounded waits after each publish. Do not send the final response while the review is still active; keep the Codex turn open until approval, diff --git a/plugins/vstack/skills/review/tests/review-lifecycle.mjs b/plugins/vstack/skills/review/tests/review-lifecycle.mjs index 27a08fa..37b3b8e 100644 --- a/plugins/vstack/skills/review/tests/review-lifecycle.mjs +++ b/plugins/vstack/skills/review/tests/review-lifecycle.mjs @@ -9,7 +9,9 @@ import { fileURLToPath } from 'node:url' const HERE = path.dirname(fileURLToPath(import.meta.url)) const SERVER = path.resolve(HERE, '../assets/review-server.mjs') -const temp = fs.mkdtempSync(path.join(os.tmpdir(), 'vstack-review-test-')) +/* A space in the path, because a project under "My Projects" is ordinary and + every command this suite prints for an agent to copy has to survive it. */ +const temp = fs.mkdtempSync(path.join(os.tmpdir(), 'vstack review test ')) const page = path.join(temp, 'page.html') const store = path.join(temp, '.vstack', 'local', 'review', 'page') const port = 18000 + (process.pid % 1000) @@ -80,6 +82,10 @@ try { const idlePull = cli('watch', '--next', '--timeout', '1') assert.equal(idlePull.status, 0, idlePull.stderr) assert.match(idlePull.stdout, /IDLE/, 'a quiet pull returns instead of leaving a terminal session behind') + assert.match(idlePull.stdout, /watch --next --timeout 1/, + 'a bounded wait ends by naming the command that resumes it — nothing else will') + assert.match(idlePull.stdout, new RegExp(`--file "${page.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}"`), + 'and quotes its arguments, so the command survives being copied') const idleLease = path.join(store, 'listening') assert.ok(fs.existsSync(idleLease), 'the wait leaves a lease across the prompt re-arm gap') fs.rmSync(idleLease, { force: true }) @@ -268,6 +274,8 @@ try { const claimed = cli('claim', '--token', token, '--session', 'pull-a') assert.equal(claimed.status, 0, claimed.stderr) assert.match(claimed.stdout, /CLAIMED/) + assert.match(claimed.stdout, /watch --all .* --next/, + 'taking a round names the wait to come back to once it is answered') assert.ok(byId('c7').deliveredAt, 'claim is the delivery point') assert.equal(byId('c7').deliveredTo, 'pull-a', 'claim binds delivery to its session') assert.equal(cli('claim', '--token', token, '--session', 'pull-b').status, 2, @@ -291,6 +299,29 @@ try { assert.equal(cli('claim', '--token', nextToken).status, 0) assert.equal(cli('publish', '--close', 'c7', '--label', 'Seventh done').status, 0) + /* ── a live review nobody is watching is named, not called all-clear ── */ + + await send([comment('c8', 'Eighth')]) + const covered = cli('unanswered', '--all') + assert.equal(covered.status, 0) + assert.match(covered.stdout, /Nothing outstanding/, + 'a fresh pull lease means the loop is still running; the wait between calls is not a fault') + + fs.rmSync(path.join(store, 'listening'), { force: true }) + const dropped = cli('unanswered', '--all') + assert.match(dropped.stdout, /comments are waiting and nothing is watching/, + 'the end-of-turn check names a review whose watch loop stopped with a queue behind it') + assert.match(dropped.stdout, /--next --timeout 25/, 'and says how to start watching again') + assert.equal(dropped.status, 0, + 'a comment this session never took delivery of does not block the end of its turn') + + const strandedOffer = cli('watch', '--next', '--timeout', '1') + const strandedToken = strandedOffer.stdout.match(/token ([0-9a-f]+)/)?.[1] + assert.equal(cli('claim', '--token', strandedToken).status, 0) + assert.match(cli('unanswered', '--all').stdout, /have not answered it/, + 'once delivered, the same comment is owed by the session that took it') + assert.equal(cli('publish', '--close', 'c8', '--label', 'Eighth done').status, 0) + /* ── a store written by an older version is read where it lies ── */ const old = path.join(temp, 'old') From bcbcc3d7dbe5b2fa863b86b9bc385e54717a33c7 Mon Sep 17 00:00:00 2001 From: DeyangChan Date: Mon, 10 Aug 2026 22:13:45 +0800 Subject: [PATCH 9/9] The README says what Codex support does not yet do Codex has no background monitor tool, so the review loop is driven by bounded polling rather than a push watcher. Someone choosing a host should know that before they install. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 5b3caba..813ef0d 100644 --- a/README.md +++ b/README.md @@ -52,7 +52,7 @@ Then run: /vstack:review Wireframe a desktop personal task manager with minimal aesthetics. ``` -### Codex +### Codex1 Install, in your terminal: @@ -67,6 +67,8 @@ Then run: $vstack:review Wireframe a desktop personal task manager with minimal aesthetics. ``` +1 Codex support is experimental. Codex does not come with a background monitor tool that allows two-way communication with Visual Stack. A deterministic polling workaround is used, but occasionally the agent stops polling prematurely. If that happens, prompt the agent to resume watching. + ## What you can do - Work in a familiar, Figma-like interface.