[21377] feat(servicenow): expand knowledge base retrieval in search-knowledge-base#21391
Open
ashwins01 wants to merge 2 commits into
Open
[21377] feat(servicenow): expand knowledge base retrieval in search-knowledge-base#21391ashwins01 wants to merge 2 commits into
ashwins01 wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📝 WalkthroughWalkthroughAdds ServiceNow actions and app methods for retrieving knowledge articles, attachments, and knowledge bases. Knowledge-base search gains filtering and updated response handling. Existing action versions and the ServiceNow package version are incremented. ChangesServiceNow knowledge capabilities
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Action
participant ServiceNowApp
participant ServiceNowAPI
participant FileSystem
Action->>ServiceNowApp: getKnowledgeArticle(articleId, fields)
ServiceNowApp->>ServiceNowAPI: Fetch article
ServiceNowAPI-->>ServiceNowApp: Article response
ServiceNowApp-->>Action: Return article
Action->>ServiceNowApp: getKnowledgeArticleAttachment(articleSysId, attachmentSysId)
ServiceNowApp->>ServiceNowAPI: Fetch attachment as arraybuffer
ServiceNowAPI-->>ServiceNowApp: Attachment content
ServiceNowApp-->>Action: Return attachment content
Action->>FileSystem: Write attachment under /tmp
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@components/servicenow/actions/get-article-attachment/get-article-attachment.mjs`:
- Around line 47-48: Update the file path construction near the writeFile call
to sanitize this.fileName with path.basename before joining it with /tmp, while
retaining attachmentSysId as the fallback when no file name is provided. Ensure
the resulting path always remains directly inside /tmp.
In `@components/servicenow/actions/get-article/get-article.mjs`:
- Around line 21-26: Centralize the shared fields prop in servicenow.app.mjs
under propDefinitions, preserving its current type, label, description, and
optional behavior. In
components/servicenow/actions/get-article/get-article.mjs:21-26 and
components/servicenow/actions/search-knowledge-base/search-knowledge-base.mjs:36,
remove the inline definitions and reference the shared prop with propDefinition:
[servicenow, "fields"].
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb005987-23c6-4e58-a0d4-5105c5e7fb74
📒 Files selected for processing (28)
components/servicenow/actions/add-item-to-cart/add-item-to-cart.mjscomponents/servicenow/actions/check-order-status/check-order-status.mjscomponents/servicenow/actions/checkout-cart/checkout-cart.mjscomponents/servicenow/actions/create-table-record/create-table-record.mjscomponents/servicenow/actions/delete-cart-item/delete-cart-item.mjscomponents/servicenow/actions/delete-table-record/delete-table-record.mjscomponents/servicenow/actions/empty-cart/empty-cart.mjscomponents/servicenow/actions/find-users/find-users.mjscomponents/servicenow/actions/get-article-attachment/get-article-attachment.mjscomponents/servicenow/actions/get-article/get-article.mjscomponents/servicenow/actions/get-catalog-item-variables/get-catalog-item-variables.mjscomponents/servicenow/actions/get-current-user/get-current-user.mjscomponents/servicenow/actions/get-record-counts-by-field/get-record-counts-by-field.mjscomponents/servicenow/actions/get-table-record-by-id/get-table-record-by-id.mjscomponents/servicenow/actions/get-table-records/get-table-records.mjscomponents/servicenow/actions/list-knowledge-bases/list-knowledge-bases.mjscomponents/servicenow/actions/list-tables/list-tables.mjscomponents/servicenow/actions/order-catalog-item/order-catalog-item.mjscomponents/servicenow/actions/search-catalog-items/search-catalog-items.mjscomponents/servicenow/actions/search-knowledge-base/search-knowledge-base.mjscomponents/servicenow/actions/search-records-by-keyword/search-records-by-keyword.mjscomponents/servicenow/actions/submit-cart-order/submit-cart-order.mjscomponents/servicenow/actions/submit-record-producer/submit-record-producer.mjscomponents/servicenow/actions/update-table-record/update-table-record.mjscomponents/servicenow/actions/view-cart/view-cart.mjscomponents/servicenow/common/constants.mjscomponents/servicenow/package.jsoncomponents/servicenow/servicenow.app.mjs
Comment on lines
+21
to
+26
| fields: { | ||
| type: "string[]", | ||
| label: "Fields", | ||
| description: "Additional `kb_knowledge` fields to return under `fields`. Example: `short_description`, `sys_class_name`.", | ||
| optional: true, | ||
| }, |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Centralize the duplicated fields prop in the app file.
The fields prop is defined inline in multiple components, duplicating its definition and configuration. As per coding guidelines and path instructions, any prop used by more than one component must be defined in the app file's propDefinitions and referenced via propDefinition.
Move the fields prop configuration into servicenow.app.mjs and update these components to reference it:
components/servicenow/actions/get-article/get-article.mjs#L21-L26: Replace the inline definition withpropDefinition: [servicenow, "fields"].components/servicenow/actions/search-knowledge-base/search-knowledge-base.mjs#L36-L36: Replace the inlinefieldsprop definition with the samepropDefinitionreference.
As per learnings, relocating a prop from an inline definition to a shared propDefinition does not warrant a major version bump provided the prop key and behavior remain intact.
📍 Affects 2 files
components/servicenow/actions/get-article/get-article.mjs#L21-L26(this comment)components/servicenow/actions/search-knowledge-base/search-knowledge-base.mjs#L36-L36
🤖 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 `@components/servicenow/actions/get-article/get-article.mjs` around lines 21 -
26, Centralize the shared fields prop in servicenow.app.mjs under
propDefinitions, preserving its current type, label, description, and optional
behavior. In components/servicenow/actions/get-article/get-article.mjs:21-26 and
components/servicenow/actions/search-knowledge-base/search-knowledge-base.mjs:36,
remove the inline definitions and reference the shared prop with propDefinition:
[servicenow, "fields"].
Sources: Coding guidelines, Path instructions, Learnings
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@components/servicenow/actions/get-article-attachment/get-article-attachment.mjs`:
- Around line 47-48: Update the filename selection before the writeFile call to
reject basename results of "..", ".", or an empty string, falling back to
attachmentSysId for those values. Preserve the existing path.basename behavior
for safe filenames and continue using the resulting name with path.join("/tmp",
...) in the attachment action.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2843a474-7e66-47b4-929f-f99eca810334
📒 Files selected for processing (4)
components/servicenow/actions/get-article-attachment/get-article-attachment.mjscomponents/servicenow/actions/get-article/get-article.mjscomponents/servicenow/actions/search-knowledge-base/search-knowledge-base.mjscomponents/servicenow/servicenow.app.mjs
Comment on lines
+47
to
+48
| const filePath = path.join("/tmp", path.basename(this.fileName || this.attachmentSysId)); | ||
| await fs.promises.writeFile(filePath, content); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node - <<'NODE'
import path from "node:path";
for (const name of ["..", ".", "/", "../../file.pdf"]) {
console.log(name, "=>", path.join("/tmp", path.basename(name)));
}
NODERepository: PipedreamHQ/pipedream
Length of output: 219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' components/servicenow/actions/get-article-attachment/get-article-attachment.mjs | cat -nRepository: PipedreamHQ/pipedream
Length of output: 2584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the action file and related attachment methods/props.
rg -n "fileName|attachmentSysId|writeFile|path.join\(\"/tmp\"" components/servicenow -g '*.mjs'
# Show the target file with line numbers.
sed -n '1,220p' components/servicenow/actions/get-article-attachment/get-article-attachment.mjs | cat -nRepository: PipedreamHQ/pipedream
Length of output: 3437
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "fileName|attachmentSysId|writeFile|path.join\\(\"/tmp\"" components/servicenow -g '*.mjs'
echo "----"
sed -n '1,220p' components/servicenow/actions/get-article-attachment/get-article-attachment.mjs | cat -nRepository: PipedreamHQ/pipedream
Length of output: 3442
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file =="
sed -n '1,220p' components/servicenow/actions/get-article-attachment/get-article-attachment.mjs | cat -n
echo "== related references =="
rg -n "fileName|attachmentSysId|get-article-attachment|Get Article Attachment" components/servicenow -g '*.mjs'Repository: PipedreamHQ/pipedream
Length of output: 3587
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== similar tmp write patterns =="
rg -n 'path\.join\("/tmp"|writeFile\(filePath|basename\(this\.' components/servicenow -g '*.mjs'
echo "== target action outline =="
ast-grep outline components/servicenow/actions/get-article-attachment/get-article-attachment.mjs --view expandedRepository: PipedreamHQ/pipedream
Length of output: 626
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba components/servicenow/actions/get-article-attachment/get-article-attachment.mjs | sed -n '1,220p'Repository: PipedreamHQ/pipedream
Length of output: 199
Guard against unsafe attachment filenames. path.basename() can still return .., ., or "", which makes path.join("/tmp", ...) target / or /tmp and fail on write; fall back to attachmentSysId for those cases.
🤖 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
`@components/servicenow/actions/get-article-attachment/get-article-attachment.mjs`
around lines 47 - 48, Update the filename selection before the writeFile call to
reject basename results of "..", ".", or an empty string, falling back to
attachmentSysId for those values. Preserve the existing path.basename behavior
for safe filenames and continue using the resulting name with path.join("/tmp",
...) in the attachment action.
michelle0927
approved these changes
Jul 15, 2026
michelle0927
left a comment
michelle0927
left a comment
Collaborator
There was a problem hiding this comment.
LGTM! Ready for QA.
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
2 participants
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
Closes #21377
0.0.1->0.1.0): new optional Knowledge Base IDs prop, mapping to the documentedkbquery param.content. Accepts asys_idor a KB number, per the docs./tmpand returns the path.kb_knowledge_baseso users can find the sys_ids that scope a search..fieldson Search Knowledge Base was serialized as repeated query params (paramsSerializer: { indexes: null }). The API documentsfieldsas a comma-separated string, and its responsemetaechoes back"fields": "short_description"for the comma form. Changed to.join(",")._makeRequestreturnsresponse.result, which isundefinedfor the attachment endpoint (it returns a file, not a{ result }envelope). Split out_makeRawRequest;_makeRequestnow delegates to it. No behavior change for existing callers.Checklist
Please check the following items before your PR can be reviewed:
Versioning
0.0.1for new ones)package.json's version updatedNew app
If this is a new app, please submit an app integration request - the PR will only be reviewed after the app is integrated.
CodeRabbit review
After the PR is opened, and if new changes are pushed, CodeRabbit will automatically review it. Do not 'mark as resolved' CodeRabbit's comments, but reply to them instead, whether you agree (and update the PR accordingly) or disagree.
Summary by CodeRabbit