feat(bot): answer questions from docs and related posts#4743
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
robertsLando
left a comment
There was a problem hiding this comment.
Deep review — 8 specialist passes (correctness, security, performance, DRY, design, tests, operability, readability)
Verdict: Needs work — one Blocker, then this is a good change. The approach is sound and the port is faithful; this is a "fix the Blocker and the port artifacts" review, not a rework.
Strengths
The .cjs migration is complete and correct, and it fixes live breakage: root package.json is "type": "module", so today's require(".../bot-scripts/index.js") in zwave-js-bot_comment.yml resolves as ESM — the existing bot commands are almost certainly broken on master right now. All 13 call sites were updated, and post-release-notes.js is genuine ESM and correctly left alone.
The Z-Wave JS UI adaptation is real, not a copy-paste: all 18 expectedFiles in docsAnswersEvalCases.json resolve to actual files under docs/, the q-a category slug and documentation label match this repo, and the deleted workflow's UI-vs-driver classifier is preserved verbatim (same prompt, model, token, trigger types) behind a correct if: github.event_name == 'issues' guard. Using the Actions cache as the vector store — no external DB, no new service, no runtime deps — is the right complexity level for a repo-scoped RAG bot.
Top 3 risks
- Raw LLM output is posted as the bot with no sanitization, on a path any anonymous user can trigger by opening an issue. The mitigation (
neutralizeMentions) ships in this very PR — it's just applied to the digest, not the public comment. - The docs index can never be invalidated by a code change.
INDEX_VERSIONis effectively a no-op, and a stale-schema index is served indefinitely. - The failure modes that matter all end in a green check. Missing index → steps skipped → job succeeds → bot silently stops answering, and nobody finds out.
Themes
- Port artifacts — upstream constants that were correct for
zwave-jsand are wrong here. TheEXCLUDED_USERSone is worth calling out directly: as written, @robertsLando's own issues get auto-answered by the bot. - The docs path is the poor cousin of the posts path —
postsIndex.cjshas a version-validating loader;docsIndex.cjshas none. The same asymmetry drives three separate findings. - Silent green — cache misses, API outages, and quota exhaustion are all indistinguishable from a healthy run.
Findings not anchored to a diff line
- No
timeout-minutes:on any new job.embedBatchedthrottles 4.5s/request andmodelsRequestretries 5× with up to 60s backoff, so a degraded Models API leaves nightly jobs spinning toward the 6h default. - No README for the subsystem. 13+ new files and no entry point explaining the
buildDocsIndex → answerFromDocs → collectDocsFeedback → updateDocsFeedbackIssuepipeline, how to rebuild an index locally, or how to add an eval case. A short.github/bot-scripts/README.mdwould pay for itself the first time someone else touches this. answerFromDocs.cjs:506-551—main()inlines ~45 lines of comment-body string composition (nested ternaries, conditional separators, metadata JSON) directly in the orchestration function, obscuring its actual control flow. AcomposeComment()helper would mirror the existingbuildDocsAnswerSection/buildRelatedPostsSectiondecomposition.- Duplicated tracking-issue logic —
updateDocsFeedbackIssue.cjsandupdateEvalTrackingIssue.cjsboth find-or-create a marker-identified tracking issue, via two different API layers (rawfetchovergithubApi.cjsvs the injected octokit) and two different idempotency schemes, invoked from the same workflow two different ways (nodevsgithub-script). Only one of them paginates — which is the bug flagged atupdateEvalTrackingIssue.cjs:27.
FYI — checked and not held against this PR
BOT_TOKENon the anonymousissues: [opened, edited]path, and the missingpermissions:block oncheck-issue-repo, are both pre-existing — I diffed against the deletedzwave-js-bot_issue.yml, which already did exactly this. Not a regression. But since the file is being rewritten anyway, a top-levelpermissions: {}with per-job grants is nearly free.- Floating action tags (
checkout@v7,cache@v6, …) in workflows holding a PAT are a supply-chain exposure, but that's this repo's existing convention everywhere — out of scope here.
Coverage
All 8 specialists run, on security-critical model tiering. Cross-lens agreement on the EXCLUDED_USERS port artifact (2 specialists), the docs-index invalidation gap (3), and the duplicate-comment risk (3). Every finding below was verified against the PR branch before submission.
Follow-up (non-blocking): can users ask the bot a question on demand?Reading through the triggers, the docs bot is purely event-driven — Practically that means the bot gets exactly one shot per post: it answers when the issue is opened, and there's no way to ask a follow-up, re-ask after clarifying the question, or summon it on an existing thread. Given that the retrieval quality is genuinely good (18/18 and 8/8 hit@5), that feels like leaving value on the table — a lot of support questions get sharper after the first round of back-and-forth, which is exactly when the bot is currently silent. Is an I can see two reasons to be cautious, so I'm asking rather than proposing:
Happy to leave this as a separate issue if you'd rather keep this PR a faithful port — just wanted to flag it while the design is fresh. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Addressed the unanchored findings as follows:
All inline findings have an individual reply with the fixing commit or agreed disposition. |
Coverage Report for CI Build 29485812596Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage remained the same at 7.994%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Followup questions are currently not intended due to possibility for abuse. If it looks like we need them, we can reconsider. |
robertsLando
left a comment
There was a problem hiding this comment.
Reconciliation re-review at 2d74596f
Thanks for the thorough remediation — this addresses the prior review point-by-point. Verified each fix against the tree with per-cluster passes:
- 16 of 18 findings verified fixed — confidence validation,
EXCLUDED_USERSderivation, comment/discussion pagination, error propagation, REST+GraphQL retry/backoff, reaction dedup, tracking-issue pagination, infra-vs-quality outcomes, digest fence-escaping + bounding, workflow concurrency, and scheduled-job fail-on-missing-index all confirmed present and correct. I've resolved those threads. - 2 test findings partially landed — 108 deterministic tests now gate PRs (great), but two gaps remain; see the still-open test threads.
- All 5 WONTFIX dispositions accepted — kill switch (mitigated by warn-on-missing-index, confirmed), full-index rewrite, duplicate embedding, BM25/RRF literals, triple-declared top-K. Reasonable calls for this repo's scale.
Remaining items are line-anchored below and on the three still-open threads. None block merge on their own, but the entity-encoded mention residual is worth closing before treating the sanitizer as the security boundary.
Address follow-up review feedback on PR #4743. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
.cjsso they load correctly with this repository'stype: moduleconfigurationZ-Wave JS UI adaptations
https://zwave-js.github.io/zwave-js-ui/#for documentation linksq-acategorydocumentationlabel for the feedback digest