fix: wire marketplace reviews sync to production Worker endpoint#293
fix: wire marketplace reviews sync to production Worker endpoint#293NicholaiVogel merged 3 commits intomainfrom
Conversation
|
Hi @aaf2tbz - I'm @NicholaiVogel's PR-reviewing agent powered by pr-reviewer. I'm taking a look at the fixes in |
NicholaiVogel
left a comment
There was a problem hiding this comment.
The two marketplace-reviews.ts changes are clean. However, the diff bundles 10+ files of unrelated changes (session tracking, review queue tab, health check, context budget, novelty sampling) that aren't described in the PR summary — this is a scope/reviewability concern, not a safety issue. One functional bug found in the omitted counter logic.
Confidence: 7.0/10
- Style consistency & maintainability: 8
- Repository conventions adherence: 7
- Merge conflict detection confidence: 6
- Security vulnerability detection confidence: 8
- Injection risk detection confidence: 9
- Attack-surface risk assessment confidence: 7
- Future hardening guidance confidence: 7
- Scope alignment confidence: 3
- Existing functionality awareness: 7
- Existing tooling/pattern leverage: 8
- Functional completeness confidence: 7
- Pattern correctness confidence: 8
- Documentation coverage confidence: 6
{
"style_maintainability": 8,
"repo_convention_adherence": 7,
"merge_conflict_detection": 6,
"security_vulnerability_detection": 8,
"injection_risk_detection": 9,
"attack_surface_risk_assessment": 7,
"future_hardening_guidance": 7,
"scope_alignment": 3,
"duplication_awareness": 7,
"tooling_pattern_leverage": 8,
"functional_completeness": 7,
"pattern_correctness": 8,
"documentation_coverage": 6
}
- Set DEFAULT_CONFIG.endpointUrl to the production Cloudflare Worker URL (reviews.signetai.sh) so new installs are pre-configured - Add X-Signet-Sync: 1 header to outbound sync requests — required by the Worker as a lightweight origin gate to filter non-Signet clients - sync remains opt-in (enabled: false default) until Nicholai deploys the Worker; TODO comment marks where to flip the default Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
b472716 to
e8a94ce
Compare
|
Rebuilt the branch cleanly from |
|
Hi @aaf2tbz - quick follow-up pass on |
NicholaiVogel
left a comment
There was a problem hiding this comment.
Prior review's scope concern is resolved — the incremental diff is now exactly the two described changes. Both changes are functionally correct. Two minor issues: (1) the TODO comment is misleading (the URL is already the live production value, not a placeholder), and (2) readConfig() falls back to "" instead of REVIEWS_SYNC_URL when an existing config file has a missing or empty endpointUrl, so existing installs that previously had no URL configured won't pick up the new default without a manual PATCH — inconsistent with the PR's stated goal of pre-configuring new and existing installs.
Confidence: 7.8/10
- Style consistency & maintainability: 8
- Repository conventions adherence: 8
- Merge conflict detection confidence: 7
- Security vulnerability detection confidence: 8
- Injection risk detection confidence: 8
- Attack-surface risk assessment confidence: 7
- Future hardening guidance confidence: 7
- Scope alignment confidence: 9
- Existing functionality awareness: 9
- Existing tooling/pattern leverage: 8
- Functional completeness confidence: 7
- Pattern correctness confidence: 8
- Documentation coverage confidence: 7
{
"style_maintainability": 8,
"repo_convention_adherence": 8,
"merge_conflict_detection": 7,
"security_vulnerability_detection": 8,
"injection_risk_detection": 8,
"attack_surface_risk_assessment": 7,
"future_hardening_guidance": 7,
"scope_alignment": 9,
"duplication_awareness": 9,
"tooling_pattern_leverage": 8,
"functional_completeness": 7,
"pattern_correctness": 8,
"documentation_coverage": 7
}
Unmapped findings (not on changed lines):
- packages/daemon/src/routes/marketplace-reviews.ts:138 -
readConfig()falls back to""(notREVIEWS_SYNC_URL) when an on-disk config has a missing or non-stringendpointUrl:
endpointUrl: typeof raw.endpointUrl === "string" ? raw.endpointUrl : "",Existing installs whose stored config has endpointUrl: "" or no endpointUrl key will continue to get an empty string, and the pre-configured URL will never take effect for them without a manual PATCH. If the intent is to migrate all installs to the new URL, the fallback should be REVIEWS_SYNC_URL:
endpointUrl: typeof raw.endpointUrl === "string" && raw.endpointUrl.length > 0
? raw.endpointUrl
: REVIEWS_SYNC_URL,Low severity since sync is opt-in, but worth aligning with the stated goal.
…intUrl - readConfig() now falls back to REVIEWS_SYNC_URL when the on-disk config has an empty or missing endpointUrl, so existing installs pick up the production URL without a manual PATCH - Remove misleading TODO comment — the URL is already the live value Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Both points addressed in d26f68f:
|
|
Hi @aaf2tbz - quick follow-up pass on |
NicholaiVogel
left a comment
There was a problem hiding this comment.
Both issues from the prior review are resolved: (1) the misleading TODO comment has been replaced with a clear, accurate comment, and (2) readConfig() now falls back to REVIEWS_SYNC_URL when an existing config file has a missing or empty endpointUrl, fixing the upgrade path for existing installs. No new issues introduced. Ready to approve.
Confidence: 8.1/10
- Style consistency & maintainability: 8
- Repository conventions adherence: 8
- Merge conflict detection confidence: 7
- Security vulnerability detection confidence: 8
- Injection risk detection confidence: 8
- Attack-surface risk assessment confidence: 7
- Future hardening guidance confidence: 7
- Scope alignment confidence: 9
- Existing functionality awareness: 9
- Existing tooling/pattern leverage: 8
- Functional completeness confidence: 9
- Pattern correctness confidence: 9
- Documentation coverage confidence: 8
{
"style_maintainability": 8,
"repo_convention_adherence": 8,
"merge_conflict_detection": 7,
"security_vulnerability_detection": 8,
"injection_risk_detection": 8,
"attack_surface_risk_assessment": 7,
"future_hardening_guidance": 7,
"scope_alignment": 9,
"duplication_awareness": 9,
"tooling_pattern_leverage": 8,
"functional_completeness": 9,
"pattern_correctness": 9,
"documentation_coverage": 8
}
NicholaiVogel
left a comment
There was a problem hiding this comment.
worker's deployed and responding at signet-reviews.nicholaivogelfilms.workers.dev. D1 database is up, migrations applied. dashboard flow goes through the daemon API so everything lines up. clean PR, approve.
Summary
Two small changes to
packages/daemon/src/routes/marketplace-reviews.ts:DEFAULT_CONFIG.endpointUrlnow points tohttps://reviews.signetai.sh/api/reviews/sync(the Cloudflare Worker fromSignet-AI/marketplace#1). New installs are pre-configured; users don't need to set the URL manually.X-Signet-Sync: 1header — the Worker requires this on all sync requests as a lightweight origin gate to filter non-Signet clients.Sync remains opt-in (
enabled: falsedefault). Once the Worker is live, a follow-up can flipenabled: trueas the default.Dependencies
Pair with Signet-AI/marketplace#1 — the Worker must be deployed by Nicholai before the sync URL resolves. A TODO comment in the code marks where to flip the default once live.
Test plan
PATCH /api/marketplace/reviews/configwith just{ "enabled": true }uses the pre-configured URL (no need to setendpointUrl)POST /api/marketplace/reviews/syncoutbound request includesX-Signet-Sync: 1headerbun test packages/daemon/src/routes/marketplace-reviews.test.ts🤖 Generated with Claude Code