Prepare Roundtable release fixes#14
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
agent-mafei
left a comment
agent-mafei
left a comment
There was a problem hiding this comment.
Code Review Summary — agent-mafei
Verdict: ✅ Approve
I reviewed the PR locally on 5c72049, read the diff against main, and reran local quality gates/tests. This changeset is solid and ready once the PR leaves draft status.
Review evidence
ruff check src tests✅ruff format --check src tests✅mypy src✅pytest -q✅ (285 passed)node --check src/roundtable/web/server.mjs && node --check src/roundtable/web/viewer.js✅
✅ Looks Good
- Smart default change:
create_discussion(web=True)remains the product default, but viewer startup is now best-effort and non-blocking. - Good security improvement: owner-secret gating on
/api/:token/revoke. - Password hashing is now cross-runtime via PBKDF2, reducing forced
bcryptjsload paths. - Node/web startup strategy is more robust: healthy-server reuse → direct
node server.mjs→npm install --omit=devfallback → optional PM2. - CI is tighter: consolidated quality gates, build/wheel smoke, and release preflight.
💡 Suggestions (1)
| # | File | Suggestion |
|---|---|---|
| 1 | src/roundtable/web/server.mjs:1016 |
Consider documenting or guarding the /api/:token/revoke disk write, since Python also merges discussion.json under lock. Not blocking for this PR. |
Notes
- PR is still a draft. If you want this merged, remove draft status after review.
Reviewed by agent-mafei
| }); | ||
|
|
||
| router.post("/api/:token/revoke", (req, res, params) => { | ||
| router.post("/api/:token/revoke", async (req, res, params) => { |
There was a problem hiding this comment.
💡 Suggestion: This /api/:token/revoke route writes discussion.json with a tmp-file write+rename, while the Python side merges cross-process changes under a lock.
If both sides can touch the same file, consider either adding a lock/merge path here or documenting that Node only performs targeted revocation writes while Python owns the primary merge path. Not blocking.
| @@ -174,6 +190,7 @@ result = rt.init( | |||
| | 📊 **收敛追踪** | 自动计算每轮共识度（convergence score），量化讨论进展 | | |||
There was a problem hiding this comment.
✅ Looks good: The new web_status / web_error / web_help documentation is clear and matches the implementation. Helpful addition for operators debugging viewer startup.
| @@ -187,6 +194,9 @@ def create_discussion( | |||
| "speech_order": disc.speech_order, | |||
| "status": disc.status, | |||
| "web_url": web_url, | |||
There was a problem hiding this comment.
✅ Looks good: Surfacing web_status, web_error, and web_help in the create-discussion result is the right call. It keeps the main discussion flow intact while giving callers actionable diagnostics.
| except OSError: | ||
| return False | ||
|
|
||
| def _is_roundtable_server_ready(self, port: int) -> bool: |
There was a problem hiding this comment.
✅ Looks good: The startup fallback strategy is much better now. Reusing an existing healthy server first, then direct node server.mjs, then npm install --omit=dev, and only then optional PM2 should avoid the earlier hard dependency on global tooling.
Review Summary — agent-mafeiStatus: ✅ Approve
Main takeaway: the web viewer default is now much healthier. Viewer startup is best-effort, diagnostics are clearer, and the fallback path no longer depends on PM2 as the primary strategy. One follow-up suggestion
This PR is still a draft. If you want it merged, remove draft status after review; Boss should still do the final merge. Reviewed by agent-mafei |
agent-mafei
left a comment
agent-mafei
left a comment
There was a problem hiding this comment.
Code Review Summary — agent-mafei
Verdict: ✅ Approve
Reviewed the latest PR #14 head commit 76ed2a0 (Guard web revoke disk writes). The new delta addresses the prior revoke-write race by re-reading the latest discussion.json, merging only revoked_token_hashes, validating the snapshot signature before rename, and preserving unrelated disk updates.
Verified locally
python3 -m ruff check src tests— passedpython3 -m ruff format --check src tests— passedpython3 -m mypy src— passedpython3 -m pytest -q— passed (280 passed, 1 skipped)node --check src/roundtable/web/server.mjs— passednpm run check— passed
Looks Good
- Revoke endpoint now avoids overwriting concurrent Python-side updates.
- Added regression coverage for preserving
dispatch_summaryand appendedspeechesduring revoke. - Release/package metadata includes web assets and npm lockfiles for wheel/sdist installs.
- Web startup is best-effort/non-blocking, matching
create_discussion(web=True)default behavior.
No merge blockers from my side.
2 participants
Summary
create_discussion(web=True)as the default while making Web Viewer startup best-effort and non-blocking2.1.0Verification
/usr/local/bin/python3.11 -m ruff check src tests/usr/local/bin/python3.11 -m ruff format --check src tests/usr/local/bin/python3.11 -m mypy src/usr/local/bin/python3.11 -m pytest -q(283 passed, 2 skipped)node --check src/roundtable/web/server.mjs && npm run checkpython -m buildpython -m twine check dist/*import roundtable, demo--no-web, and packaged Web/template/skill resourcesPUPPETEER_SKIP_DOWNLOAD=trueNotes
Local machine default Node is
v14.18.2, so fullscripts/release/preflight-check.shshould be rerun in CI or a Node 18+ release environment.