Repository navigation
Conversation
… Roam page Open a page that holds a proof kit and the runner loads that PR's CI build of the Discourse Graph extension, opens the kit pinned in the right sidebar, and plays its cases when you press Run. Kits that change the graph before load ask first. A finished run writes its verdicts, build and commit to the page. With URL loading the build gets this extension's extensionAPI (settings under a dg: prefix); from roam/js it gets a browser-storage stand-in.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Roam prototype previewsThe preview deployment is ready. Paste a URL below into Load Developer Extensions from URL in Roam: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7345ab41a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const mine = this.env.buildFor === this.rootUid; | ||
| const wanted = config.build ?? (config.pr && loaded && mine ? loaded.branch : null); |
There was a problem hiding this comment.
When a build is already loaded and the user navigates to a different kit that specifies only pr::, mine is false, so wanted becomes null. No mismatch is reported and the panel offers Run instead of Reload, causing the second PR's cases to execute against the first PR's build. Treat any PR-owned by another kit as requiring a reload, even before its branch is resolved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fccfbf4. buildStatus treats a pr:: kit as matching only a build loaded for it or for the same PR; on another kit's build the panel shows "this page wants PR #n", offers Reload and keeps Run off. Covered in tests/run-gate.test.ts.
| if (!live) { | ||
| const run = button(state ? "Run again" : "Run", "run", { className: view.setup ? "" : "primary", title: "Run every case on this page" }); | ||
| run.disabled = Boolean(view.error) || Boolean(view.setup) || Boolean(view.build.loading); | ||
| box.append(run); |
There was a problem hiding this comment.
If GitHub lookup, build fetching, importing, or readiness fails—or another Discourse Graph copy is already active—env.buildError is shown only as a warning and view.error remains false, so this condition still enables Run once loading ends. The cases then exercise no build or the unrelated installed build and write a completed run log, which can produce invalid proof results; requested-build kits should not be runnable until the matching build has loaded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fccfbf4. A kit that names a build can't run while that build failed to load, is loading, waits for setup, or belongs to another kit; Run is disabled with the reason as its title, and start() refuses with the same reason.
| private readRecord(): RunRecord | null { | ||
| try { | ||
| const record = JSON.parse(localStorage.getItem(this.storageKey) ?? "null") as RunRecord | null; | ||
| return record && this.kit && record.kitHash === hash(this.kit.kit) ? record : null; | ||
| } catch { |
There was a problem hiding this comment.
A saved record includes the original commit, but readRecord validates only the kit hash. If a branch advances between a partial run and a reload, Resume merges verdicts from the old commit with newly executed cases and writeRunLog attributes the combined result to the current commit, corrupting the proof record. Accept the record only when its commit matches the currently loaded build.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fccfbf4. recordFits accepts a saved run only when both the kit hash and the loaded commit match, so Resume never mixes verdicts across commits.
| this.env.beforeLoad = await applyBeforeLoad(plan, context); | ||
| this.trace(`before-load fixtures: ${this.env.beforeLoad.map((item) => `${item.id} ${item.outcome}`).join(", ") || "none"}`); | ||
| try { | ||
| this.trace(`loading ${branch}`); | ||
| this.env.build = await loadBuild({ |
There was a problem hiding this comment.
When a before-load fixture throws, applyBeforeLoad records a failed outcome, but finishLoad immediately starts the extension anyway. These fixtures establish flags and node types that the extension reads only at startup, so the resulting run uses the wrong initial state and cannot repair it in-session; a failed outcome should stop the load and remain retryable rather than merely becoming a warning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fccfbf4. When a before-load fixture fails, the build isn't loaded: retryPlan puts the failed fixtures back behind Set up and load, and the panel shows what failed.
| while (!dgReady()) { | ||
| if (Date.now() > deadline) throw new Error(`The build from ${branch} ran but DG didn't report ready in ${timeout / 1000}s.`); | ||
| await sleep(200); |
There was a problem hiding this comment.
If extension.onload completes but dgReady() never becomes true, this throw exits before a LoadedBuild is assigned, so the caller cannot invoke extension.onunload; commands, the patched palette, CSS, and any partial DG state remain active despite the reported load failure. Clean up the extension/API/CSS on every post-import failure before propagating the error.
AGENTS.md reference: AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fccfbf4. Every failure after import (onload throwing or readiness timing out) runs the same cleanup as unload: the build's onunload, the commands added for it, the palette hook and the stylesheet.
From the review: - A kit that names a build (build:: or pr::) runs only on that build, loaded for it. Another kit's build, a failed load, a load in progress or pending setup now keep Run off and say why, so no run log credits a build the cases didn't run on. - A failed before-load fixture stops the load and offers Set up and load again for the fixtures that failed, instead of starting DG in the wrong state. - Resume only picks up a partial run on the commit it started on. - A build that fails after import, including one that never reports ready, is unloaded with its commands, palette hook and stylesheet.
A CI build talks to the hosted database and signs itself in when it loads with sync or node sharing on, creating the graph's space the first time. The runner now reads which database a build uses before planning setup, leaves the kits' local-database sign-in to a hosted build, and after load waits for DG's session. A kit that needs the database can't run until DG is signed in, and the panel says why when it isn't.
…tabase CI compiles PR builds against the production database. A kit that needs a database, or that has sync or node sharing on (or turns them on in setup), would put the test graph on it. For those kits the runner now refuses to load a build that isn't on a local database and says why.
…panel, agent tools - Kits that need a database run on the PR's CI build pointed at the proof database: the runner swaps the build's Supabase URL and key for the proof database's and Discourse Graph's website API for the embeddings stub, and refuses the build if anything hosted is left. Connect this machine starts the proof database through a dg-proof:// link. This replaces local builds. - The page helpers talk to the proof database and sign in the way create-space would. - Kit pages carry QA decisions, areas and a surface section; Run plays the approved cases. - The panel shows what a kit needs before Run as a checklist, each unmet need with the button that meets it, and during a run the page's own case blocks; Retry runs a failed step as its block reads now. - Agent tools on Roam's AI API (extensionAPI.ai.addTool); Ask your agent opens one on the kit. - dg-baseline@2 turns sync and node sharing off, so UI-only kits run on CI builds for anyone.
…r names the keys The runner copied the proof database's keys into a SUPABASE_* map itself, which the public-artifact check reads as an assigned credential. The helper on the tester's machine, which holds the keys, now sends that map as env, and the page passes it on.
Any developer sets a machine up once from this folder: `pnpm helper setup --dg <discourse-graph checkout>`. After that a kit page's Connect starts the proof database and hands the runner its address and the kits' keys; Disconnect stops what Connect started. - helper/ runs on plain Node 22 (type stripping), beside the extension and not in it: the proof database (its own Supabase project, dg-proof, on 553xx, from the checkout's migrations; Docker when Docker answers, native processes otherwise), an embeddings stub in Node, the server the page asks, and the CLI. - Setup registers dg-proof:// links: xdg on Linux, a small AppleScript app on macOS. - The checklist's setup hint names the command.
…e README - Windows: dg-proof:// links register under the user's own registry classes (no admin), the launcher starts the helper in its own minimized window, and the proof database runs in Docker. The Supabase CLI now runs through its own Node launcher and npm through a shell there, since Windows won't run .cmd shims directly. - README: "Getting set up" says what a developer does once (turn the runner on, set the machine up with one command, and what that needs), what to press on a kit page, and what to check when it doesn't work. - The checklist's setup hint points at that section.
…oat the controls over dialogs
…s and cases collapsed
…e when a person uses the page mid-step
…296 and eng-2329 kits
… run off the kit page
…NG-2176 kit's Query step
…NG-2329 kit wording and case 15's checks
…re Run, with skip, edit and add; left-out cases named in results
…son; notes and left-out reasons in the PR result; Stop in every state; added cases are proposals
1 participant
What
A developer-extension prototype that tests a Discourse Graph pull request from a Roam page. The page holds a proof kit: the PR it proves (
pr:: 1506) and its cases, written as steps a person could follow with each action folded under its step. Opening the page loads that PR's CI build fromdiscoursegraphs.com/releases/roam/<branch>/, opens the kit pinned in the right sidebar, and Run plays the cases: clicks, typing, keys and palette commands in the page, each case's check, and Pass/Fail for cases done by hand. A finished run writes its verdicts, build, commit, time and runner to the page.Try it
proof/roam-selftest-pron test-3-graph checks the runner itself on PR #1506's build.How it behaves
extensionAPI, with its settings under adg:prefix, so Roam cleans up its commands on unload. From roam/js it gets a browser-storage stand-in.Checked
pnpm test,pnpm buildandpnpm prepare:artifactspass on Node 22. The prototype adds 25 tests: every kit round-trips through blocks, plus the selector engine, the build loader'sextensionAPIhand-off, and before-load planning.ac09d76) with the runner'sextensionAPIpassed on. On theproof/eng-2348kit, it stopped at Set up and load (sync and sharing flags) without writing anything.Notes for review
createButtonObserveris replaced with a small observer, because importing it from theroamjs-components/dombarrel adds about 850 KB (refractor and xregexp).evalwhen a person presses Run, the same trust a graph's roam/js asks for. That also means an admin can't install it for everyone.src/coreandsrc/roamare copied from Sid's proof tooling, whose Playwright runner shares the kit format, machine, recipes and baselines.src/index.ts, the README and the tests belong to this prototype.