adding a /cherryhelp Slack bot - #26
Open
CherryCIC wants to merge 2 commits into
Open
Conversation
Limit indexed root docs to the exact approved filenames so unrelated README files are not pulled into the bot index. Share secret-line redaction across local, GitHub and Notion sources, and add regression coverage for the approved path rules. Tested from bots/cherry-help with npm ci, npm test, npm run build and npm run build-index.
📝 WalkthroughWalkthroughThis PR introduces Changescherry-help Bot Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
CherryCIC
marked this pull request as ready for review
May 7, 2026 15:44
![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: 95a941c4f0
ℹ️ 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".
| @@ -0,0 +1,9 @@ | |||
| const SENSITIVE_LINE_PATTERN = | |||
| /\b(xoxb|xapp|sk_live|sk_test|ghp_|github_pat_|password\s*=|secret\s*=|token\s*=|api[_\s-]?key\s*=|client[_\s-]?secret\s*=|signing[_\s-]?secret\s*=)\b/i; | |||
There was a problem hiding this comment.
The redaction regex misses common credential lines like SLACK_BOT_TOKEN=..., GITHUB_PAT=..., and github_pat_... because \b treats _ as a word character, so boundaries before token/secret and after prefixes like ghp_ do not match. As a result, approved docs can be indexed with real secrets intact if someone pastes env-style keys into markdown or text files, which defeats the bot’s documented safety guard.
Useful? React with 👍 / 👎.
|
|
||
| const SECRET_PATTERNS = [ | ||
| /\b(slack|github|notion|stripe|firebase|sendcloud)?\s*(token|secret|password|credential|api\s*key|client\s*secret|signing\s*secret|verification\s*token)\b/i, | ||
| /\b(xoxb|xapp|sk_live|sk_test|ghp_|github_pat_)\b/i, |
There was a problem hiding this comment.
Sensitive-request detection misses typical secret query forms such as what is SLACK_BOT_TOKEN or strings containing ghp_.../github_pat_... for the same word-boundary reason (_ is a word char), so these prompts are not refused by isSensitiveRequest. In those cases the bot can proceed to retrieval and potentially return credential-bearing snippets instead of the refusal message.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
bots/cherry-help/.env.example (1)
9-9: ⚡ Quick winQuote the multi-word value on Line 9 for portability across different .env loading methods.
VOLUNTEER_LEAD_LABEL=the volunteer leadrequires quotes because unquoted values with spaces are word-split by shell-based.envloaders (e.g.,source .env), causing command execution errors, and dotenv requires quotes for reliable parsing. UseVOLUNTEER_LEAD_LABEL="the volunteer lead"instead.Proposed change
-VOLUNTEER_LEAD_LABEL=the volunteer lead +VOLUNTEER_LEAD_LABEL="the volunteer lead"🤖 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 `@bots/cherry-help/.env.example` at line 9, The environment example defines a multi-word value without quotes which can be split by shell-based .env loaders; update the VOLUNTEER_LEAD_LABEL entry to wrap the value in double quotes (change VOLUNTEER_LEAD_LABEL=the volunteer lead to VOLUNTEER_LEAD_LABEL="the volunteer lead") so dotenv and shell sourcing parse it as a single value.bots/cherry-help/src/sources/notion.ts (1)
92-121: ⚡ Quick winPrefer
plain_textwhen extracting Notion rich text.At Line 94 and Line 115, using
part.text.contentskips non-textrich_text items.plain_textpreserves more content for indexing.Proposed fix
- if (!isRecord(part) || !isRecord(part.text) || typeof part.text.content !== 'string') { + if (!isRecord(part) || typeof part.plain_text !== 'string') { return ''; } - return part.text.content; + return part.plain_text;- if (!isRecord(part) || !isRecord(part.text) || typeof part.text.content !== 'string') { + if (!isRecord(part) || typeof part.plain_text !== 'string') { return ''; } - return part.text.content; + return part.plain_text;🤖 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 `@bots/cherry-help/src/sources/notion.ts` around lines 92 - 121, The code is extracting Notion rich text using part.text.content which misses non-text rich_text items; update both mappings (the anonymous richText->string mapper and the getPageTitle function) to prefer part.plain_text (check isRecord(part) && typeof part.plain_text === 'string') and fall back to '' if missing, leaving existing isRecord checks intact so the title and rich text extraction use plain_text for indexing.
🤖 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 `@bots/cherry-help/Procfile`:
- Line 1: The Procfile's worker entry currently uses "worker: npm start" which
assumes the build artifact exists (dist/src/app.js); update the Procfile so the
worker boot runs the build first or invokes a start script that performs the
build (e.g., run the build command before starting or use a start:prod script
that builds), ensuring the "worker: npm start" invocation is replaced with a
command that guarantees npm run build (or equivalent) completes prior to
launching the app so the dist artifacts exist.
In `@bots/cherry-help/src/app.ts`:
- Around line 77-83: handle the case where handleQuestion returns null by
sending an explicit ephemeral response instead of doing nothing; after calling
handleQuestion (the variable reply), check if reply is null and call
respond(...) with mrkdwn: true, response_type: 'ephemeral' and a short message
like "This slash command is not enabled in this channel" so users get actionable
feedback; update the block around the respond call that currently only executes
when reply is truthy to also handle the null case and return afterwards.
In `@bots/cherry-help/src/retrieval/index.ts`:
- Around line 95-98: The map over parsed.chunks currently assumes chunk.tokens
exists and is an array, causing crashes for older/manual index files; update the
mapping in the parsed.chunks processing (the arrow function that spreads
...chunk and sets tokens) to check Array.isArray(chunk.tokens) &&
chunk.tokens.length > 0 before using it, otherwise call tokenise(chunk.text) to
produce tokens; reference the parsed.chunks.map callback, the chunk.tokens
property, and the tokenise function when making this change.
In `@bots/cherry-help/src/sources/github.ts`:
- Around line 66-91: The loop over repoSpecs currently lets any API error abort
the entire fetch flow; wrap the per-repo processing (the body that calls
fetchDefaultBranch, fetchRepoTree, fetchRepoFile and processes tree items) in a
try/catch so failures for one repo are caught, logged (include
repoSpec.owner/repo and the error), and the code continues to the next repo,
ensuring documents.push still only happens for successfully fetched files;
update the block containing fetchDefaultBranch, fetchRepoTree, fetchRepoFile and
the tree iteration to be enclosed in the try/catch and skip to the next repo on
error.
- Around line 161-173: The githubRequest function lacks a request timeout and
can hang; wrap the fetch in an AbortController, start a timer (e.g., 10s or
configurable) that calls controller.abort() on timeout, pass controller.signal
to fetch, and clear the timer after fetch completes; ensure that when aborted
you throw a clear error (or rethrow the fetch error) so callers like
fetchGitHubDocs fail fast instead of hanging.
In `@bots/cherry-help/src/sources/notion.ts`:
- Around line 17-37: The loop that calls notion.pages.retrieve and readBlocks
should be made resilient: inside fetchNotionDocs wrap the per-page work
(notion.pages.retrieve, getPageTitle, readBlocks, redactSensitiveLines and the
documents.push) in a try/catch so a thrown error for one page is caught, logged
(including pageId and the error), and processing continues to the next page
without rethrowing; ensure you only push to documents when the try succeeds and
keep the rest of the function unchanged.
In `@bots/cherry-help/src/sources/safety.ts`:
- Around line 1-2: SENSITIVE_LINE_PATTERN uses \b word boundaries which fail for
tokens containing underscores and similar characters; update the
SENSITIVE_LINE_PATTERN constant (and ensure redactSensitiveLines() continues to
use it) to a regex that does not rely on \b and instead matches the common
secret prefixes and assignment forms (e.g., ghp_, github_pat_, sk_live_,
sk_test_, sk_*, api[_\s-]?key\s*=, password\s*=, secret\s*=, token\s*=,
client[_\s-]?secret, signing[_\s-]?secret, xoxb, xapp) using boundaries
expressed via lookarounds or start/whitespace/quote checks so strings like
"ghp_abcd1234", "github_pat_ABC_def", "sk_live_abc123", "password = mypassword",
and "api_key = abc123" are matched and redacted.
---
Nitpick comments:
In `@bots/cherry-help/.env.example`:
- Line 9: The environment example defines a multi-word value without quotes
which can be split by shell-based .env loaders; update the VOLUNTEER_LEAD_LABEL
entry to wrap the value in double quotes (change VOLUNTEER_LEAD_LABEL=the
volunteer lead to VOLUNTEER_LEAD_LABEL="the volunteer lead") so dotenv and shell
sourcing parse it as a single value.
In `@bots/cherry-help/src/sources/notion.ts`:
- Around line 92-121: The code is extracting Notion rich text using
part.text.content which misses non-text rich_text items; update both mappings
(the anonymous richText->string mapper and the getPageTitle function) to prefer
part.plain_text (check isRecord(part) && typeof part.plain_text === 'string')
and fall back to '' if missing, leaving existing isRecord checks intact so the
title and rich text extraction use plain_text for indexing.
🪄 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 Plus
Run ID: 63f2bdf2-578a-48ad-a876-ace8aee215fc
⛔ Files ignored due to path filters (1)
bots/cherry-help/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
bots/cherry-help/.env.examplebots/cherry-help/.gitignorebots/cherry-help/Procfilebots/cherry-help/README.mdbots/cherry-help/content/volunteer-faq.mdbots/cherry-help/jest.config.jsbots/cherry-help/package.jsonbots/cherry-help/scripts/build-index.tsbots/cherry-help/src/answer.tsbots/cherry-help/src/app.tsbots/cherry-help/src/retrieval/index.tsbots/cherry-help/src/sources/github.tsbots/cherry-help/src/sources/notion.tsbots/cherry-help/src/sources/safety.tsbots/cherry-help/src/types.tsbots/cherry-help/tests/bot.test.tsbots/cherry-help/tsconfig.json
| @@ -0,0 +1 @@ | |||
| worker: npm start | |||
There was a problem hiding this comment.
Worker start path can fail without a prior build artifact.
On Line 1, npm start assumes dist/src/app.js already exists. If the deploy step doesn’t run npm run build, worker boot will fail.
Safer Procfile command
-worker: npm start
+worker: npm run build && npm start📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| worker: npm start | |
| worker: npm run build && npm start |
🤖 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 `@bots/cherry-help/Procfile` at line 1, The Procfile's worker entry currently
uses "worker: npm start" which assumes the build artifact exists
(dist/src/app.js); update the Procfile so the worker boot runs the build first
or invokes a start script that performs the build (e.g., run the build command
before starting or use a start:prod script that builds), ensuring the "worker:
npm start" invocation is replaced with a command that guarantees npm run build
(or equivalent) completes prior to launching the app so the dist artifacts
exist.
Comment on lines
+77
to
+83
| if (reply) { | ||
| await respond({ | ||
| mrkdwn: true, | ||
| response_type: 'ephemeral', | ||
| text: reply.text | ||
| }); | ||
| } |
There was a problem hiding this comment.
Return explicit feedback for disallowed slash-command channels.
When handleQuestion returns null, Line 77 skips respond, so users see no actionable feedback. For slash commands, this should return an ephemeral “not enabled in this channel” message.
Suggested fix
- if (reply) {
- await respond({
- mrkdwn: true,
- response_type: 'ephemeral',
- text: reply.text
- });
- }
+ await respond({
+ mrkdwn: true,
+ response_type: 'ephemeral',
+ text: reply?.text ?? 'This command is not enabled in this channel.'
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (reply) { | |
| await respond({ | |
| mrkdwn: true, | |
| response_type: 'ephemeral', | |
| text: reply.text | |
| }); | |
| } | |
| await respond({ | |
| mrkdwn: true, | |
| response_type: 'ephemeral', | |
| text: reply?.text ?? 'This command is not enabled in this channel.' | |
| }); |
🤖 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 `@bots/cherry-help/src/app.ts` around lines 77 - 83, handle the case where
handleQuestion returns null by sending an explicit ephemeral response instead of
doing nothing; after calling handleQuestion (the variable reply), check if reply
is null and call respond(...) with mrkdwn: true, response_type: 'ephemeral' and
a short message like "This slash command is not enabled in this channel" so
users get actionable feedback; update the block around the respond call that
currently only executes when reply is truthy to also handle the null case and
return afterwards.
Comment on lines
+192
to
+194
| if (config.allowedChannelIds.size === 0) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Fail closed for channel access in production.
Line 192 currently allows all channels when ALLOWED_CHANNEL_IDS is unset. That’s a risky default for an internal-doc bot and can leak answers to unintended channels if config is missing.
Suggested fix
export function loadConfig(env: NodeJS.ProcessEnv = process.env): RuntimeConfig {
const botMode = env.BOT_MODE?.trim() || 'retrieve_only';
+ const nodeEnv = env.NODE_ENV ?? 'development';
+ const allowedChannelIds = new Set(parseList(env.ALLOWED_CHANNEL_IDS ?? ''));
if (botMode !== 'retrieve_only') {
throw new Error('Only BOT_MODE=retrieve_only is supported.');
}
+
+ if (nodeEnv === 'production' && allowedChannelIds.size === 0) {
+ throw new Error('ALLOWED_CHANNEL_IDS must be set in production.');
+ }
return {
- allowedChannelIds: new Set(parseList(env.ALLOWED_CHANNEL_IDS ?? '')),
+ allowedChannelIds,
botMode,
indexPath: env.INDEX_PATH ?? path.join(process.cwd(), 'data', 'index.json'),
- nodeEnv: env.NODE_ENV ?? 'development',
+ nodeEnv,
volunteerLeadLabel: env.VOLUNTEER_LEAD_LABEL?.trim() || 'the volunteer lead'
};
}Also applies to: 33-39
Comment on lines
+95
to
+98
| chunks: parsed.chunks.map((chunk) => ({ | ||
| ...chunk, | ||
| tokens: chunk.tokens.length > 0 ? chunk.tokens : tokenise(chunk.text) | ||
| })) |
There was a problem hiding this comment.
Guard against missing tokens in loaded index JSON.
At Line 97, chunk.tokens.length assumes tokens exists and is an array. Add a runtime guard to avoid load-time crashes on older or manually edited index files.
Proposed fix
- tokens: chunk.tokens.length > 0 ? chunk.tokens : tokenise(chunk.text)
+ tokens: Array.isArray(chunk.tokens) && chunk.tokens.length > 0 ? chunk.tokens : tokenise(chunk.text)🤖 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 `@bots/cherry-help/src/retrieval/index.ts` around lines 95 - 98, The map over
parsed.chunks currently assumes chunk.tokens exists and is an array, causing
crashes for older/manual index files; update the mapping in the parsed.chunks
processing (the arrow function that spreads ...chunk and sets tokens) to check
Array.isArray(chunk.tokens) && chunk.tokens.length > 0 before using it,
otherwise call tokenise(chunk.text) to produce tokens; reference the
parsed.chunks.map callback, the chunk.tokens property, and the tokenise function
when making this change.
Comment on lines
+66
to
+91
| for (const repoSpec of repoSpecs) { | ||
| const ref = repoSpec.ref ?? (await fetchDefaultBranch(repoSpec, token)); | ||
| const tree = await fetchRepoTree(repoSpec, ref, token); | ||
|
|
||
| for (const item of tree) { | ||
| if (item.type !== 'blob' || !isApprovedDocPath(item.path) || isBlockedPath(item.path)) { | ||
| continue; | ||
| } | ||
|
|
||
| if (item.size && item.size > MAX_FILE_SIZE_BYTES) { | ||
| continue; | ||
| } | ||
|
|
||
| const text = await fetchRepoFile(repoSpec, item.path, ref, token); | ||
| documents.push({ | ||
| priority: 2, | ||
| source: { | ||
| kind: 'github', | ||
| title: `${repoSpec.owner}/${repoSpec.repo}`, | ||
| path: item.path, | ||
| url: `https://github.com/${repoSpec.owner}/${repoSpec.repo}/blob/${ref}/${item.path}` | ||
| }, | ||
| text: redactSensitiveLines(text) | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "github.ts" -type f
</function_calls>Repository: Cherry-CIC/cherry-Backend
Length of output: 239
🏁 Script executed:
cat -n bots/cherry-help/src/sources/github.ts | head -100
</function_calls>Repository: Cherry-CIC/cherry-Backend
Length of output: 3752
🏁 Script executed:
tail -n +100 bots/cherry-help/src/sources/github.tsRepository: Cherry-CIC/cherry-Backend
Length of output: 3600
Handle per-repo API failures without aborting all GitHub indexing.
At lines 67–79, any API error currently fails the entire fetchGitHubDocs run. Wrap each repo fetch flow in try/catch and continue to the next repo.
Proposed fix
for (const repoSpec of repoSpecs) {
- const ref = repoSpec.ref ?? (await fetchDefaultBranch(repoSpec, token));
- const tree = await fetchRepoTree(repoSpec, ref, token);
+ try {
+ const ref = repoSpec.ref ?? (await fetchDefaultBranch(repoSpec, token));
+ const tree = await fetchRepoTree(repoSpec, ref, token);
- for (const item of tree) {
- if (item.type !== 'blob' || !isApprovedDocPath(item.path) || isBlockedPath(item.path)) {
- continue;
- }
+ for (const item of tree) {
+ if (item.type !== 'blob' || !isApprovedDocPath(item.path) || isBlockedPath(item.path)) {
+ continue;
+ }
- if (item.size && item.size > MAX_FILE_SIZE_BYTES) {
- continue;
- }
+ if (item.size && item.size > MAX_FILE_SIZE_BYTES) {
+ continue;
+ }
- const text = await fetchRepoFile(repoSpec, item.path, ref, token);
- documents.push({
- priority: 2,
- source: {
- kind: 'github',
- title: `${repoSpec.owner}/${repoSpec.repo}`,
- path: item.path,
- url: `https://github.com/${repoSpec.owner}/${repoSpec.repo}/blob/${ref}/${item.path}`
- },
- text: redactSensitiveLines(text)
- });
+ const text = await fetchRepoFile(repoSpec, item.path, ref, token);
+ documents.push({
+ priority: 2,
+ source: {
+ kind: 'github',
+ title: `${repoSpec.owner}/${repoSpec.repo}`,
+ path: item.path,
+ url: `https://github.com/${repoSpec.owner}/${repoSpec.repo}/blob/${ref}/${item.path}`
+ },
+ text: redactSensitiveLines(text)
+ });
+ }
+ } catch {
+ continue;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const repoSpec of repoSpecs) { | |
| const ref = repoSpec.ref ?? (await fetchDefaultBranch(repoSpec, token)); | |
| const tree = await fetchRepoTree(repoSpec, ref, token); | |
| for (const item of tree) { | |
| if (item.type !== 'blob' || !isApprovedDocPath(item.path) || isBlockedPath(item.path)) { | |
| continue; | |
| } | |
| if (item.size && item.size > MAX_FILE_SIZE_BYTES) { | |
| continue; | |
| } | |
| const text = await fetchRepoFile(repoSpec, item.path, ref, token); | |
| documents.push({ | |
| priority: 2, | |
| source: { | |
| kind: 'github', | |
| title: `${repoSpec.owner}/${repoSpec.repo}`, | |
| path: item.path, | |
| url: `https://github.com/${repoSpec.owner}/${repoSpec.repo}/blob/${ref}/${item.path}` | |
| }, | |
| text: redactSensitiveLines(text) | |
| }); | |
| } | |
| } | |
| for (const repoSpec of repoSpecs) { | |
| try { | |
| const ref = repoSpec.ref ?? (await fetchDefaultBranch(repoSpec, token)); | |
| const tree = await fetchRepoTree(repoSpec, ref, token); | |
| for (const item of tree) { | |
| if (item.type !== 'blob' || !isApprovedDocPath(item.path) || isBlockedPath(item.path)) { | |
| continue; | |
| } | |
| if (item.size && item.size > MAX_FILE_SIZE_BYTES) { | |
| continue; | |
| } | |
| const text = await fetchRepoFile(repoSpec, item.path, ref, token); | |
| documents.push({ | |
| priority: 2, | |
| source: { | |
| kind: 'github', | |
| title: `${repoSpec.owner}/${repoSpec.repo}`, | |
| path: item.path, | |
| url: `https://github.com/${repoSpec.owner}/${repoSpec.repo}/blob/${ref}/${item.path}` | |
| }, | |
| text: redactSensitiveLines(text) | |
| }); | |
| } | |
| } catch { | |
| continue; | |
| } | |
| } |
🤖 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 `@bots/cherry-help/src/sources/github.ts` around lines 66 - 91, The loop over
repoSpecs currently lets any API error abort the entire fetch flow; wrap the
per-repo processing (the body that calls fetchDefaultBranch, fetchRepoTree,
fetchRepoFile and processes tree items) in a try/catch so failures for one repo
are caught, logged (include repoSpec.owner/repo and the error), and the code
continues to the next repo, ensuring documents.push still only happens for
successfully fetched files; update the block containing fetchDefaultBranch,
fetchRepoTree, fetchRepoFile and the tree iteration to be enclosed in the
try/catch and skip to the next repo on error.
Comment on lines
+161
to
+173
| async function githubRequest<T>(url: string, token: string): Promise<T> { | ||
| const response = await fetch(url, { | ||
| headers: { | ||
| Accept: 'application/vnd.github+json', | ||
| Authorization: `Bearer ${token}`, | ||
| 'X-GitHub-Api-Version': '2022-11-28', | ||
| 'User-Agent': 'cherry-help-bot' | ||
| } | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`GitHub request failed with status ${response.status}.`); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the file to see the actual code
cat -n bots/cherry-help/src/sources/github.ts | head -200Repository: Cherry-CIC/cherry-Backend
Length of output: 7208
🏁 Script executed:
# Search for timeout patterns in the codebase
rg -i "timeout|abortcontroller" --type ts --type js | head -20Repository: Cherry-CIC/cherry-Backend
Length of output: 522
🏁 Script executed:
# Search for fetch calls to see how they're handled elsewhere
rg "fetch\(" --type ts --type js -A 3 | head -50Repository: Cherry-CIC/cherry-Backend
Length of output: 363
🏁 Script executed:
# Check package.json for Node.js version
cat package.json | grep -A 5 '"engines"'Repository: Cherry-CIC/cherry-Backend
Length of output: 51
🏁 Script executed:
# Read package.json to check Node.js version
cat package.json | head -30Repository: Cherry-CIC/cherry-Backend
Length of output: 883
🏁 Script executed:
# Check if there's any error handling or retry logic in the calling functions
sed -n '56,94p' bots/cherry-help/src/sources/github.tsRepository: Cherry-CIC/cherry-Backend
Length of output: 1230
Add timeout to GitHub API calls to prevent indefinite hangs.
The fetch call at line 162 has no timeout, so index building can hang indefinitely if any GitHub API request stalls. The sequential calls in fetchGitHubDocs (lines 67–79) mean a single hung request blocks the entire process.
Use AbortController with a timeout to ensure requests fail fast:
Proposed fix
async function githubRequest<T>(url: string, token: string): Promise<T> {
- const response = await fetch(url, {
- headers: {
- Accept: 'application/vnd.github+json',
- Authorization: `Bearer ${token}`,
- 'X-GitHub-Api-Version': '2022-11-28',
- 'User-Agent': 'cherry-help-bot'
- }
- });
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 10_000);
+ let response: Response;
+ try {
+ response = await fetch(url, {
+ signal: controller.signal,
+ headers: {
+ Accept: 'application/vnd.github+json',
+ Authorization: `Bearer ${token}`,
+ 'X-GitHub-Api-Version': '2022-11-28',
+ 'User-Agent': 'cherry-help-bot'
+ }
+ });
+ } catch (error) {
+ if (error instanceof Error && error.name === 'AbortError') {
+ throw new Error('GitHub request timed out.');
+ }
+ throw error;
+ } finally {
+ clearTimeout(timeout);
+ }
if (!response.ok) {
throw new Error(`GitHub request failed with status ${response.status}.`);
}🤖 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 `@bots/cherry-help/src/sources/github.ts` around lines 161 - 173, The
githubRequest function lacks a request timeout and can hang; wrap the fetch in
an AbortController, start a timer (e.g., 10s or configurable) that calls
controller.abort() on timeout, pass controller.signal to fetch, and clear the
timer after fetch completes; ensure that when aborted you throw a clear error
(or rethrow the fetch error) so callers like fetchGitHubDocs fail fast instead
of hanging.
Comment on lines
+17
to
+37
| for (const pageId of pageIds) { | ||
| const page = await notion.pages.retrieve({ page_id: pageId }); | ||
| const title = getPageTitle(page); | ||
| const blocks = await readBlocks(notion, pageId); | ||
| const text = redactSensitiveLines(blocks.join('\n\n')).trim(); | ||
|
|
||
| if (!text) { | ||
| continue; | ||
| } | ||
|
|
||
| documents.push({ | ||
| priority: 3, | ||
| source: { | ||
| kind: 'notion', | ||
| title, | ||
| path: pageId, | ||
| url: getPageUrl(page) | ||
| }, | ||
| text | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd notion.ts --type fRepository: Cherry-CIC/cherry-Backend
Length of output: 108
🏁 Script executed:
cat -n bots/cherry-help/src/sources/notion.tsRepository: Cherry-CIC/cherry-Backend
Length of output: 4344
Isolate Notion page failures so one bad page does not fail the entire source ingest.
Currently, any Notion API error at line 18 (notion.pages.retrieve) or line 20 (readBlocks) will abort the entire fetchNotionDocs function, causing all pages to fail. Wrap each page fetch and read in a try/catch block to skip failed pages and continue processing remaining ones.
Proposed fix
for (const pageId of pageIds) {
- const page = await notion.pages.retrieve({ page_id: pageId });
- const title = getPageTitle(page);
- const blocks = await readBlocks(notion, pageId);
- const text = redactSensitiveLines(blocks.join('\n\n')).trim();
+ try {
+ const page = await notion.pages.retrieve({ page_id: pageId });
+ const title = getPageTitle(page);
+ const blocks = await readBlocks(notion, pageId);
+ const text = redactSensitiveLines(blocks.join('\n\n')).trim();
- if (!text) {
- continue;
- }
+ if (!text) {
+ continue;
+ }
- documents.push({
- priority: 3,
- source: {
- kind: 'notion',
- title,
- path: pageId,
- url: getPageUrl(page)
- },
- text
- });
+ documents.push({
+ priority: 3,
+ source: {
+ kind: 'notion',
+ title,
+ path: pageId,
+ url: getPageUrl(page)
+ },
+ text
+ });
+ } catch {
+ continue;
+ }
}🤖 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 `@bots/cherry-help/src/sources/notion.ts` around lines 17 - 37, The loop that
calls notion.pages.retrieve and readBlocks should be made resilient: inside
fetchNotionDocs wrap the per-page work (notion.pages.retrieve, getPageTitle,
readBlocks, redactSensitiveLines and the documents.push) in a try/catch so a
thrown error for one page is caught, logged (including pageId and the error),
and processing continues to the next page without rethrowing; ensure you only
push to documents when the try succeeds and keep the rest of the function
unchanged.
Comment on lines
+1
to
+2
| const SENSITIVE_LINE_PATTERN = | ||
| /\b(xoxb|xapp|sk_live|sk_test|ghp_|github_pat_|password\s*=|secret\s*=|token\s*=|api[_\s-]?key\s*=|client[_\s-]?secret\s*=|signing[_\s-]?secret\s*=)\b/i; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import re
pat = re.compile(r'\b(xoxb|xapp|sk_live|sk_test|ghp_|github_pat_|password\s*=|secret\s*=|token\s*=|api[_\s-]?key\s*=|client[_\s-]?secret\s*=|signing[_\s-]?secret\s*=)\b', re.I)
samples = [
"GITHUB_TOKEN=ghp_abcd1234",
"GITHUB_TOKEN=github_pat_ABC_def",
"STRIPE_SECRET=sk_live_abc123",
"SLACK_BOT_TOKEN=xoxb-123456",
]
for s in samples:
print(f"{s} -> matched={bool(pat.search(s))}")
PYRepository: Cherry-CIC/cherry-Backend
Length of output: 251
🏁 Script executed:
find . -type f -name "safety.ts" | head -20Repository: Cherry-CIC/cherry-Backend
Length of output: 110
🏁 Script executed:
cat -n ./bots/cherry-help/src/sources/safety.ts | head -20Repository: Cherry-CIC/cherry-Backend
Length of output: 492
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import re
# Original pattern
original = re.compile(r'\b(xoxb|xapp|sk_live|sk_test|ghp_|github_pat_|password\s*=|secret\s*=|token\s*=|api[_\s-]?key\s*=|client[_\s-]?secret\s*=|signing[_\s-]?secret\s*=)\b', re.I)
# Proposed pattern from review
proposed = re.compile(r'(?:xoxb-|xapp-|sk_(?:live|test)_[A-Za-z0-9]+|ghp_[A-Za-z0-9]+|github_pat_[A-Za-z0-9_]+|\b(?:password|secret|token|api[_\s-]?key|client[_\s-]?secret|signing[_\s-]?secret)\s*=)', re.I)
samples = [
"GITHUB_TOKEN=ghp_abcd1234",
"GITHUB_TOKEN=github_pat_ABC_def",
"STRIPE_SECRET=sk_live_abc123",
"SLACK_BOT_TOKEN=xoxb-123456",
"password = mypassword",
"api_key = abc123",
]
print("Original Pattern Results:")
for s in samples:
result = bool(original.search(s))
print(f" {s:40} -> {result}")
print("\nProposed Pattern Results:")
for s in samples:
result = bool(proposed.search(s))
print(f" {s:40} -> {result}")
PYRepository: Cherry-CIC/cherry-Backend
Length of output: 739
Critical: redaction regex misses common secret formats.
The SENSITIVE_LINE_PATTERN on lines 1–2 fails to match secrets with common formats because \b word boundaries don't work correctly when followed by word characters like underscores. Testing confirms the pattern fails to match:
ghp_abcd1234(GitHub personal access token)github_pat_ABC_def(GitHub PAT)sk_live_abc123(Stripe secret key)password = mypassword(password assignments)api_key = abc123(API key assignments)
Since redactSensitiveLines() uses this pattern to filter out sensitive lines, these secrets will not be redacted. Replace the pattern with:
-const SENSITIVE_LINE_PATTERN =
- /\b(xoxb|xapp|sk_live|sk_test|ghp_|github_pat_|password\s*=|secret\s*=|token\s*=|api[_\s-]?key\s*=|client[_\s-]?secret\s*=|signing[_\s-]?secret\s*=)\b/i;
+const SENSITIVE_LINE_PATTERN =
+ /(?:xoxb-|xapp-|sk_(?:live|test)_[A-Za-z0-9]+|ghp_[A-Za-z0-9]+|github_pat_[A-Za-z0-9_]+|\b(?:password|secret|token|api[_\s-]?key|client[_\s-]?secret|signing[_\s-]?secret)\s*=)/i;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const SENSITIVE_LINE_PATTERN = | |
| /\b(xoxb|xapp|sk_live|sk_test|ghp_|github_pat_|password\s*=|secret\s*=|token\s*=|api[_\s-]?key\s*=|client[_\s-]?secret\s*=|signing[_\s-]?secret\s*=)\b/i; | |
| const SENSITIVE_LINE_PATTERN = | |
| /(?:xoxb-|xapp-|sk_(?:live|test)_[A-Za-z0-9]+|ghp_[A-Za-z0-9]+|github_pat_[A-Za-z0-9_]+|\b(?:password|secret|token|api[_\s-]?key|client[_\s-]?secret|signing[_\s-]?secret)\s*=)/i; |
🤖 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 `@bots/cherry-help/src/sources/safety.ts` around lines 1 - 2,
SENSITIVE_LINE_PATTERN uses \b word boundaries which fail for tokens containing
underscores and similar characters; update the SENSITIVE_LINE_PATTERN constant
(and ensure redactSensitiveLines() continues to use it) to a regex that does not
rely on \b and instead matches the common secret prefixes and assignment forms
(e.g., ghp_, github_pat_, sk_live_, sk_test_, sk_*, api[_\s-]?key\s*=,
password\s*=, secret\s*=, token\s*=, client[_\s-]?secret, signing[_\s-]?secret,
xoxb, xapp) using boundaries expressed via lookarounds or start/whitespace/quote
checks so strings like "ghp_abcd1234", "github_pat_ABC_def", "sk_live_abc123",
"password = mypassword", and "api_key = abc123" are matched and redacted.
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.
Summary
Describe the change in plain language.
Checklist
npm run buildnpm test -- --runInBand.env.exampleif config changedTesting
List the checks you ran and the result.
Risk
Call out anything reviewers should watch closely, especially around auth, Stripe, Sendcloud, and order creation.
Summary by CodeRabbit
New Features
/cherryhelp), mentions, and direct messages with cited sources.Documentation
Tests