chore(policy):SP-3822 mark policy threads as fixed when policy check … #53
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughPolicy checks now manage Azure DevOps PR thread statuses: new THREAD_STATUS enum and helpers fetch/update PR threads and mark SCANOSS-related threads as fixed when copyleft, undeclared, or dependency-track checks pass. Minor version and changelog bumps included. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Policy Runner
participant PolicyCore as PolicyCheck (helpers)
participant DevOps as Azure DevOps API
participant PR as PR Threads Service
Runner->>PolicyCore: run policy scan
alt scan success (code == 0)
PolicyCore->>DevOps: getPreviousThreads()
DevOps->>PR: GET threads filtered by SCANOSS marker
PR-->>DevOps: thread list
loop for each matching thread
PolicyCore->>DevOps: updateThreadStatus(threadId, THREAD_STATUS.fixed)
DevOps->>PR: PATCH thread status -> fixed
PR-->>DevOps: ack
end
DevOps-->>PolicyCore: threads resolved
PolicyCore-->>Runner: return success
else scan failure
PolicyCore-->>Runner: return error (no thread resolution)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 10-12: The changelog entry header "## [1.5.0] - 2026-01-19" has a
future date; verify whether the 1.5.0 release has actually occurred and if not,
remove the hard date and place the "1.5.0" entry under an "Unreleased" section
(or change the header to "## [Unreleased]" and keep the added bullet about
marking policy threads as fixed), then stamp the actual release date only when
cutting the release and update the header back to "## [1.5.0] - YYYY-MM-DD".
In `@codescantask/policies/copyleft-policy-check.ts`:
- Line 101: Typo in the success message: update the call to this.success in
copyleft-policy-check (the line that currently passes '### :white_check_mark:
Policy Pass \n #### Not copyleft Licenses were found') to use the corrected text
"No copyleft licenses were found" so the message reads '### :white_check_mark:
Policy Pass \n #### No copyleft licenses were found' when invoking this.success.
In `@codescantask/policies/policy-check.ts`:
- Around line 253-273: resolvePolicyThreads currently returns after updating the
first matching thread and doesn't handle errors from updateThreadStatus; change
it to attempt to resolve all matching threads by replacing the early return with
a break (so you finish checking the rest of thread.comments and continue
iterating threads) and wrap each call to updateThreadStatus(thread.id,
THREAD_STATUS.fixed) in a try-catch to make it best-effort (catch/log the error
and continue) while still using getPreviousThreads() and the scanossMarker built
from this.checkName.
🧹 Nitpick comments (2)
codescantask/package.json (1)
16-16: Consider movingsinonto devDependencies.
sinonis a mocking library typically used only for testing. Having it independenciesmeans it will be included in production bundles unnecessarily.♻️ Suggested fix
"dependencies": { "axios": "^1.7.2", - "azure-pipelines-task-lib": "^4.12.1", - "sinon": "^18.0.0" + "azure-pipelines-task-lib": "^4.12.1" }, "devDependencies": { "@types/mocha": "^10.0.6", "@types/node": "^20.12.12", "@types/q": "^1.5.8", "@types/sinon": "^17.0.3", "mocha": "^10.4.0", + "sinon": "^18.0.0", "sync-request": "^6.1.0", "ts-node": "^10.9.2", "typescript": "^5.6.3" }codescantask/policies/copyleft-policy-check.ts (1)
100-104: Consider error handling forresolvePolicyThreads().The
updateThreadStatus()method in the basePolicyCheckclass (line 240-251) callsaxios.patch()without error handling. If this API call fails (e.g., network error or authentication failure), the error will propagate up throughresolvePolicyThreads()and potentially fail the task despite the policy check passing. This affects all three policy checks that callresolvePolicyThreads(): copyleft-policy-check.ts, undeclared-policy-check.ts, and dep-track-policy-check.ts.The most effective fix would be to add error handling directly in
updateThreadStatus()to match the pattern used in other API-calling methods in the same class (e.g.,addComment(),getPreviousThreads()). Alternatively, wrap the call at each call site:♻️ Suggested improvement
if (results.code === 0) { await this.success('### :white_check_mark: Policy Pass \n #### Not copyleft Licenses were found', undefined); - await this.resolvePolicyThreads(); + try { + await this.resolvePolicyThreads(); + } catch (error) { + tl.warning(`Failed to resolve policy threads: ${error}`); + } return; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
codescantask/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
CHANGELOG.mdcodescantask/package.jsoncodescantask/policies/copyleft-policy-check.tscodescantask/policies/dep-track-policy-check.tscodescantask/policies/policy-check.tscodescantask/policies/undeclared-policy-check.tscodescantask/task.jsonvss-extension-dev.jsonvss-extension.json
🔇 Additional comments (12)
vss-extension-dev.json (1)
5-5: LGTM!Dev manifest version bump aligns with the new policy thread resolution feature.
vss-extension.json (1)
5-5: LGTM!Production manifest version bump to 1.5.0 is consistent with related package files and appropriate for the new feature.
codescantask/task.json (1)
10-14: LGTM!Task version bump to 1.5.0 is consistent across all package manifests.
codescantask/package.json (1)
3-3: LGTM!Package version bump to 1.5.0 is consistent with task.json and vss-extension.json.
codescantask/policies/dep-track-policy-check.ts (1)
258-260: Good addition to resolve prior policy threads after success.codescantask/policies/undeclared-policy-check.ts (1)
78-82: Success-path thread resolution looks good.codescantask/policies/policy-check.ts (6)
29-29: No concerns here.
37-48: THREAD_STATUS enum aligns with Azure DevOps thread status API.
129-145: Centralizing thread retrieval is a clean improvement.
152-154: Nice reuse of getPreviousThreads in cleanup path.
182-213: Thread status propagation on comment creation is solid.
233-251: Helper for thread status updates looks good.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
agustingroh
force-pushed
the
chore/SP-3822-close-resolved-issues
branch
from
92735dd to
1e19c1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@codescantask/policies/policy-check.ts`:
- Line 29: The import of resolve from "node:dns" is unused; remove the unused
import (the resolve symbol) from the top of the file so the import line no
longer references resolve, ensuring no other code references the resolve
identifier in policy-check.ts (search for resolve and delete the import if
unused).
In `@codescantask/policies/undeclared-policy-check.ts`:
- Around line 78-82: The success message passed to this.success contains a
grammar typo: replace "Not undeclared components were found" with "No undeclared
components were found" in the branch where results.code === 0; keep the
surrounding logic (the tl.debug call and the subsequent await
this.resolvePolicyThreads() invocation) unchanged so the policy passes and
threads are resolved as intended.
♻️ Duplicate comments (1)
codescantask/policies/policy-check.ts (1)
253-277: LGTM - Previous review concerns addressed.The implementation now includes:
- Try-catch around
updateThreadStatusfor best-effort handling (lines 267-271)breakinstead ofreturnto continue processing all matching threads (line 272)This ensures individual thread update failures don't halt the process, and all matching SCANOSS threads are resolved.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
codescantask/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
CHANGELOG.mdcodescantask/package.jsoncodescantask/policies/copyleft-policy-check.tscodescantask/policies/dep-track-policy-check.tscodescantask/policies/policy-check.tscodescantask/policies/undeclared-policy-check.tscodescantask/task.jsonvss-extension-dev.jsonvss-extension.json
✅ Files skipped from review due to trivial changes (1)
- vss-extension-dev.json
🚧 Files skipped from review as they are similar to previous changes (6)
- vss-extension.json
- codescantask/task.json
- codescantask/policies/copyleft-policy-check.ts
- codescantask/package.json
- CHANGELOG.md
- codescantask/policies/dep-track-policy-check.ts
🔇 Additional comments (6)
codescantask/policies/policy-check.ts (5)
37-48: LGTM!The
THREAD_STATUSenum is well-documented with a reference to the Azure DevOps API documentation, and the values correctly map to the supported thread statuses.
129-145: LGTM - Good error handling pattern.The method gracefully handles failures by logging and returning an empty array, allowing callers to continue. The buildReason gate is consistent with other methods.
Minor nit: the indentation inside the try block (lines 131-139) appears inconsistent with the rest of the file.
151-180: LGTM!Good refactoring to use
getPreviousThreads()for better code reuse. The error handling appropriately logs a warning without failing the task.
182-218: LGTM - Minor observation on redundant status update.The thread status is set both in the POST payload (line 197) and via a subsequent PATCH call (line 212). This may be intentional if the Azure DevOps API requires the PATCH to properly persist the status. If this is confirmed behavior, consider adding a brief comment explaining why both are needed.
233-251: LGTM!The method is well-documented and focused. Error handling is appropriately delegated to callers, which both wrap the call in try-catch blocks.
codescantask/policies/undeclared-policy-check.ts (1)
24-24: Minor formatting change.Import formatting adjusted. No functional impact.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
agustingroh
force-pushed
the
chore/SP-3822-close-resolved-issues
branch
from
1e19c1d to
79f71b9
Compare
agustingroh
force-pushed
the
chore/SP-3822-close-resolved-issues
branch
from
79f71b9 to
d0ebf9b
Compare
What's Changed
Added
Summary by CodeRabbit
New Features
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.