Desktop: Phase 3 (step 1) broker serves the static reads (plate, queue) - #70
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a new ChangesPhase 3 broker: local store reads for
Sequence Diagram(s)sequenceDiagram
participant Client
participant Broker
participant store_js as store.js
participant FastAPI
rect rgba(100, 149, 237, 0.5)
Note over Client,store_js: Token-gated local serve (/api/plate, /api/queue)
Client->>Broker: GET /api/plate?t=TOKEN
Broker->>Broker: tokenOk check via timingSafeEqual
alt bad/missing token
Broker-->>Client: 401 {"error":"Unauthorized"}
else store read error
Broker->>store_js: plate(sopDir)
store_js-->>Broker: throws
Broker-->>Client: 500 {"error":"..."}
else success
Broker->>store_js: plate(sopDir)
store_js->>store_js: open state.db read-only, query waiting tasks
store_js-->>Broker: rows[]
Broker-->>Client: 200 payload
end
end
rect rgba(144, 238, 144, 0.5)
Note over Client,FastAPI: Forwarded routes (unchanged)
Client->>Broker: GET /api/runs
Broker->>FastAPI: proxy request (filtered headers + Host guard)
FastAPI-->>Client: upstream response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
Adversarial self-review hardening (folded into the latest push). No security holes (token gate equivalent to FastAPI's, missing-token fails closed, no path-bypass of the gate). Two P2 fixes:
|
zkann
force-pushed
the
desktop-broker-reads
branch
from
June 17, 2026 22:59
0e315f4 to
6e7183b
Compare
![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: 0e315f48f9
ℹ️ 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".
| // | ||
| // Each reader mirrors its Python counterpart exactly; the parity test (store == FastAPI) is the gate. | ||
|
|
||
| const { DatabaseSync } = require('node:sqlite') |
There was a problem hiding this comment.
When the desktop app is launched with the pinned Electron 32.3.3 runtime, this top-level import runs while loading main.js -> broker.js -> store.js; Electron 32 embeds Node 20.18.1, but node:sqlite is only available in newer Node releases, so the require fails before the broker, tray, or window can start. The local node --test path can pass under a newer system Node, but npm start uses Electron's older embedded Node, so the shipped desktop shell is broken until this uses a dependency/runtime Electron actually provides or Electron is upgraded accordingly.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Correct and shipping-critical, fixed. Electron 32 embeds Node 20.18, but node:sqlite needs Node >= 22.5, so require("node:sqlite") would have crashed the shell at startup (my node --test passed only because it ran under system Node 24). Fix: bumped the Electron pin to ^42.4.1 (embeds Node 24.16). Verified in the actual Electron runtime headlessly: ELECTRON_RUN_AS_NODE=1 electron -e "require(\"node:sqlite\")" -> OK, and store.js loads with its plate/queue exports. No native module / ABI rebuild needed (the reason for keeping node:sqlite over better-sqlite3). Documented the Node>=22.5 / Electron>=42 requirement in store.js.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
desktop/broker.test.js (1)
136-160: ⚡ Quick winExtend broker integration coverage to include the
/api/queueserved path.This test validates
/api/platelocal serving, but/api/queueis also broker-served and currently lacks broker-level served-vs-forwarded coverage.Suggested addition
test('serves /api/plate from the store (token-gated), forwards unknown paths', async () => { const d = fs.mkdtempSync(path.join(os.tmpdir(), 'smbos-broker-')) fs.writeFileSync(path.join(d, '.dashboard-token'), 'tok') + fs.mkdirSync(path.join(d, 'queue')) + fs.writeFileSync( + path.join(d, 'queue', 'q.md'), + '---\nstatus: queued\nsop: nightly\nproject: /tmp/acme\n---\nbody', + ) const db = new DatabaseSync(path.join(d, 'state.db')) db.exec(`CREATE TABLE task (id INTEGER PRIMARY KEY, domain TEXT, kind TEXT, subject TEXT, status TEXT, priority INTEGER DEFAULT 0, source_ref TEXT, created_at TEXT, updated_at TEXT)`) db.prepare("INSERT INTO task(id,domain,kind,subject,status,created_at,updated_at) VALUES(1,'ops','x','on plate','waiting','t','t')").run() db.close() let forwarded = false const upstream = http.createServer((req, res) => { forwarded = true; res.end('up') }) const upPort = await listen(upstream) const broker = createBroker({ targetPort: upPort, sopDir: d }) const brPort = await listen(broker) // served + no token -> 401, never forwarded assert.equal((await request(brPort, '/api/plate')).status, 401) // served + valid token -> answered from the store const ok = await request(brPort, '/api/plate?t=tok') assert.equal(ok.status, 200) assert.deepEqual(JSON.parse(ok.body).plate.map((r) => r.subject), ['on plate']) + const q = await request(brPort, '/api/queue?t=tok') + assert.equal(q.status, 200) + assert.deepEqual(JSON.parse(q.body).queue, [{ file: 'q.md', sop: 'nightly', project: 'acme' }]) assert.equal(forwarded, false, 'a served read never hits the upstream')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/broker.test.js` around lines 136 - 160, The test function "serves /api/plate from the store (token-gated), forwards unknown paths" currently only validates token-gated serving and forwarding behavior for the /api/plate endpoint, but the broker also serves /api/queue locally and needs the same coverage. Add additional assertions after the existing /api/plate test cases (and before the upstream.close() and broker.close() calls) that mirror the same test pattern for /api/queue: verify it returns 401 without a token, returns 200 with a valid token, and confirms that forwarded remains false when served from the local store.desktop/store.test.js (1)
21-30: ⚡ Quick winAdd an explicit fixture for the
id ASCtie-break inplateordering.The current case validates priority/time ordering, but it does not exercise equal-
priority+ equal-created_atrows, so theid ASCcontract in the test name isn’t actually asserted.Proposed test delta
seedTasks(d, [ { id: 1, subject: 'a', status: 'waiting', priority: 0, created_at: '2026-01-02' }, { id: 2, subject: 'b', status: 'in_flight', priority: 9 }, // not waiting -> excluded { id: 3, subject: 'c', status: 'waiting', priority: 5, created_at: '2026-01-03' }, { id: 4, subject: 'd', status: 'waiting', priority: 0, created_at: '2026-01-01' }, + { id: 5, subject: 'e', status: 'waiting', priority: 0, created_at: '2026-01-01' }, ]) - assert.deepEqual(store.plate(d).map((r) => r.subject), ['c', 'd', 'a']) // c(prio5), then prio0 oldest-first d,a + assert.deepEqual(store.plate(d).map((r) => r.subject), ['c', 'd', 'e', 'a'])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/store.test.js` around lines 21 - 30, The test name for the plate function indicates it should order by priority desc, then created_at asc, then id asc, but the current seedTasks call does not include any rows with equal priority and equal created_at values. Add additional task fixtures to the seedTasks array that have matching priority and created_at but different ids, then update the assert.deepEqual assertion to verify that these equal-priority, equal-created_at rows are correctly ordered by id in ascending order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@desktop/store.js`:
- Line 8: The import statement on line 8 uses the `node:sqlite` built-in module,
which is not available in Node.js 20.16.0 that ships with Electron 32.0.0
(requires Node.js 22.5.0+). Replace the `require('node:sqlite')` import with a
compatible SQLite library such as `better-sqlite3` or `sqlite3` that works with
Node.js 20, adjusting the usage of DatabaseSync to match the API of the chosen
alternative library, or alternatively upgrade to Electron 33 or later which
bundles Node.js 22+.
---
Nitpick comments:
In `@desktop/broker.test.js`:
- Around line 136-160: The test function "serves /api/plate from the store
(token-gated), forwards unknown paths" currently only validates token-gated
serving and forwarding behavior for the /api/plate endpoint, but the broker also
serves /api/queue locally and needs the same coverage. Add additional assertions
after the existing /api/plate test cases (and before the upstream.close() and
broker.close() calls) that mirror the same test pattern for /api/queue: verify
it returns 401 without a token, returns 200 with a valid token, and confirms
that forwarded remains false when served from the local store.
In `@desktop/store.test.js`:
- Around line 21-30: The test name for the plate function indicates it should
order by priority desc, then created_at asc, then id asc, but the current
seedTasks call does not include any rows with equal priority and equal
created_at values. Add additional task fixtures to the seedTasks array that have
matching priority and created_at but different ids, then update the
assert.deepEqual assertion to verify that these equal-priority, equal-created_at
rows are correctly ordered by id in ascending order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70e1cd0d-f091-4b21-92e2-c7c9442ffa64
📒 Files selected for processing (6)
desktop/broker.jsdesktop/broker.test.jsdesktop/main.jsdesktop/package.jsondesktop/store.jsdesktop/store.test.js
zkann
force-pushed
the
desktop-broker-reads
branch
from
June 17, 2026 23:04
6e7183b to
1eaa975
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
desktop/broker.test.js (1)
136-160: ⚡ Quick winAdd broker integration coverage for
/api/queuelocal serving.This integration test validates
/api/plateonly; Phase 3 serving also includes/api/queue, so wiring/token-gate behavior for that route is currently unverified here.Possible extension (same test or sibling test)
// served + valid token -> answered from the store const ok = await request(brPort, '/api/plate?t=tok') assert.equal(ok.status, 200) assert.deepEqual(JSON.parse(ok.body).plate.map((r) => r.subject), ['on plate']) assert.equal(forwarded, false, 'a served read never hits the upstream') + + // served queue route should also be local + token-gated + fs.mkdirSync(path.join(d, 'queue')) + fs.writeFileSync(path.join(d, 'queue', 'q.md'), '---\nstatus: queued\nsop: weekly\n---\nbody') + const q = await request(brPort, '/api/queue?t=tok') + assert.equal(q.status, 200) + assert.deepEqual(JSON.parse(q.body).queue, [{ file: 'q.md', sop: 'weekly', project: '' }]) + assert.equal(forwarded, false, 'served queue read never hits the upstream')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/broker.test.js` around lines 136 - 160, The test only validates `/api/plate` local serving and token-gating behavior, but Phase 3 also serves `/api/queue` which lacks verification here. Add test assertions to verify `/api/queue` follows the same token-gate and local-serving pattern: first insert a queue record into the task table in the test setup, then add requests to `/api/queue` without a token (expecting 401), with a valid token (expecting 200 with the queue data from store), and verify it does not forward to upstream when served locally, mirroring the existing `/api/plate` test assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@desktop/broker.test.js`:
- Around line 162-174: The test 'a served read is DENIED when the token file is
missing/empty (fails closed)' verifies that invalid/missing tokens are denied
with 401 status, but it does not verify that the upstream server was never
contacted. Modify the upstream server creation to track whether it received any
requests (for example, by incrementing a counter or setting a flag in the
request handler), then add assertions after the two denial checks to verify that
the upstream server was never contacted, proving the broker failed closed
without forwarding to upstream.
In `@desktop/store.test.js`:
- Around line 21-30: The test claims to verify `id` ordering as a tie-breaker in
its name but the test data does not include any rows with equal priority and
created_at values, so the id sorting regression cannot be caught. Add additional
task fixtures to the seedTasks call that have matching priority and created_at
values but different id values, then update the corresponding assertion in the
store.plate call to verify that these tied tasks are correctly ordered by id in
ascending order. This will ensure that the id ASC tie-breaking logic is actually
exercised and validated in the test.
---
Nitpick comments:
In `@desktop/broker.test.js`:
- Around line 136-160: The test only validates `/api/plate` local serving and
token-gating behavior, but Phase 3 also serves `/api/queue` which lacks
verification here. Add test assertions to verify `/api/queue` follows the same
token-gate and local-serving pattern: first insert a queue record into the task
table in the test setup, then add requests to `/api/queue` without a token
(expecting 401), with a valid token (expecting 200 with the queue data from
store), and verify it does not forward to upstream when served locally,
mirroring the existing `/api/plate` test assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0fc20449-84f9-457d-98ab-482bc0982191
📒 Files selected for processing (6)
desktop/broker.jsdesktop/broker.test.jsdesktop/main.jsdesktop/package.jsondesktop/store.jsdesktop/store.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- desktop/package.json
- desktop/main.js
- desktop/broker.js
The broker now ANSWERS /api/plate and /api/queue itself, reading the SQLite work-state (node:sqlite, built-in -- no native dep) and the queue files directly, instead of forwarding them to FastAPI. It owns the token gate for what it serves (FastAPI's check never runs on a broker-served response) and keeps the Host guard. Everything else still forwards unchanged. Scope: parity-verified against the live FastAPI (broker output == FastAPI output, byte for byte). settings/procedures/pending are deferred to later increments -- the parity check caught that /api/settings reads an environment-detected terminal and serializes a float budget, so it isn't a pure static read yet. The liveness-bearing reads (inflight/runs) and the SSE live mirror keep forwarding until the Phase 5 native layer migrates the flock/pid liveness. - desktop/store.js: plate() (SQLite) + queue() (files), each mirroring its Python reader. - desktop/broker.js: a SERVED map routes those two; token gate + 401; rest forwards. - tests: store unit tests + a broker served-vs-forwarded test (16 total). A live parity check against FastAPI confirms exact-match output. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
zkann
force-pushed
the
desktop-broker-reads
branch
from
June 17, 2026 23:06
1eaa975 to
6e19d7c
Compare
This was referenced Jun 17, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Start of Phase 3 of the switchover: the broker stops forwarding the static reads and answers them itself from the SQLite work-state + plain files.
What moved
/api/plate(SQLitetasktable) and/api/queue(queue.mdfiles) are now served by the broker viadesktop/store.js(node:sqlite, built into Node — no native module).Scope / honesty
/api/plateand/api/queueoutput matches byte for byte.settings/procedures/pendingfollow in later increments. The parity check caught that/api/settingsreads an environment-detected terminal (iterm) and serializes a float budget, so it isn't a pure static read — it stays forwarded rather than ship a subtly-divergent port.inflight,runs) and the SSE live mirror keep forwarding until the Phase 5 native layer migrates the flock/pid liveness (a real architectural coupling, not an oversight).Tests
node --test: 16 (store unit tests + a broker served-vs-forwarded test). Plus the live FastAPI parity check above.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores