You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
New pull-requests/prs command (src/commands/pull-requests.ts), registered in src/index.ts — the plural counterpart to pull-request (which shows a single PR by number).
-q, --search-text <text> and -b, --branch <name> map to AnalysisService.listRepositoryPullRequests's textQuery/targetBranch params (added server-side in OD-376). No API client regen needed — 57.3.9 (already pinned from the OD-296/OD-397 work this branch is based on) already has both params.
[provider] [org] [repo] auto-detect from the git remote via resolveRepoArgs, same as repository/ls/directories.
-n, --limit <n> (default 100, max 1000) with the same paginate-to-limit loop as findings.
Table columns reuse the exact same shared helpers as repository's "Open Pull Requests" table (buildGateStatus, formatStandards, formatPrIssues, formatPrCoverage, formatDelta) — Branch column shows targetBranch (the dimension --branch filters on) rather than originBranch.
The endpoint's own search param (Merged vs. last-updated classification) is deliberately not exposed — different axis, out of scope per the ticket.
Why
OD-378: expose the OD-376 API filters (text query + branch scope) as a proper list command, mirroring issues/repositories.
Testing
10 new tests in src/commands/pull-requests.test.ts — full suite passes (516 → see this PR's base for baseline).
tsc --noEmit fully clean (also fixed a real package-lock.json/update-notifier drift that was causing a pre-existing, unrelated tsc/ts-node error — update-notifier was declared in package.json but missing from node_modules/lockfile).
Manually verified against a real repo (gh codacy codacy-website, built via npm run build, using a stored codacy login credential): --branch main → 0 results (repo's PRs all target master), --branch master → matches the unfiltered count, --search-text "AI" → narrows to exactly the one matching title, --output json shape correct, and auto-detect from the git remote works from inside a real checkout. Satisfies the ticket's manual-verify step.
Surfaces advisoryInformation (advisory ID, vulnerable functions, published
date) across issue, issues, pull-request --issue, finding, and findings:
compact one-liners on list/card views, full blocks on detail views.
finding skips its own block when a linked Codacy issue already renders the
same data via printIssueCodeContext, so SCA/dependency findings (which have
no linked issue) are the case this closes out, now that SrmItem carries
advisoryInformation directly (server-side, API 57.3.9).
New `pull-requests`/`prs` command lists PRs for a repository with the same
analysis-gated columns as repository's Open Pull Requests table; -q/--search-text
and -b/--branch map to the API's textQuery/targetBranch params from OD-376.
Also fixes package-lock.json drift (update-notifier was declared in
package.json but missing from node_modules/lockfile, breaking ts-node runs).
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces the pull-requests command (aliased as prs) but currently fails to meet quality standards. The main implementation file, src/commands/pull-requests.ts, exhibits high cyclomatic complexity (13) because the action handler is performing too many responsibilities, including argument resolution, pagination logic, and multi-format rendering.
Furthermore, there is significant code duplication; both the table definition and the pagination loop have been copied from other commands (repository.ts and findings.ts) rather than being extracted into shared utilities. These quality issues must be addressed to bring the PR up to standards. While the functional requirements are met, an architectural misalignment was noted where the JSON output contains fields not visible in the console table.
About this PR
There is a systemic pattern of duplicating core logic (table formatting and pagination) across CLI commands. To improve maintainability, please prioritize extracting these into shared utilities within src/utils/ rather than duplicating them for every new command.
Test suggestions
Verify pull-requests command is correctly registered in the main CLI entry point.
Test listing pull requests using auto-detected repository credentials from the git remote.
Verify the --search-text option correctly passes the query to the API service.
Verify the --branch option correctly passes the target branch filter to the API service.
Test pagination logic by fetching multiple pages up to a defined --limit.
Verify that --output json produces the correct whitelisted fields via pickDeep.
Confirm the table view correctly truncates and sanitizes PR titles and branch names.
Ensure a clear message is displayed when the API returns an empty list of pull requests.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
The pull-requests action handler is handling argument resolution, pagination logic, and data formatting for multiple output types. This has led to a cyclomatic complexity of 13 and a length of 63 lines, exceeding recommended thresholds.
Consider refactoring the pull-requests command action handler in src/commands/pull-requests.ts. Extract the pagination logic into a reusable helper function and move the logic for rendering the JSON vs. Table output into separate dedicated functions.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: The table definition and population logic for pull requests are duplicated from the repository command (src/commands/repository.ts). This redundancy makes the UI harder to keep consistent. Consider extracting this logic into a shared helper in src/utils/formatting.ts that can be used by both commands.
The reason will be displayed to describe this comment to others. Learn more.
⚪ LOW RISK
Suggestion: The JSON output includes fields (owner.name, status, originBranch) that are not present in the console table. To align with the architectural pattern described in AGENTS.md, the JSON projection should match the visible console fields.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Codacy analysis indicates the PR is up to standards; however, the addition of 18 delta clones persists a systemic pattern of logic duplication previously identified. No security flaws or high-severity logic bugs were found that should prevent merging. It is recommended to address the logic gap in the pagination warning to ensure users are correctly notified when results are truncated, even if the API does not provide a total count.
About this PR
This PR continues a pattern of duplicating core logic for pagination and table formatting across CLI commands. While the PR is up to standards on Codacy, please prioritize the extraction of these patterns into shared utilities within src/utils/ as suggested in previous review cycles to maintain architectural consistency.
Test suggestions
Fetch and display pull requests with default parameters (auto-detected repo)
Filter results using --search-text and verify API mapping
Filter results using --branch and verify API mapping
Perform multi-page pagination using cursors until --limit is reached
Output PR list in JSON format with filtered fields
Display pagination warning when total server results exceed the fetched limit
Handle empty result sets with a 'No pull requests found' message
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the branch locally: tsc --noEmit is clean and the full suite passes (517 tests, not 516 — trivial), CI is green, and the API wiring is correct — the 8 positional args line up exactly with the generated listRepositoryPullRequests signature, and the 100-item pageSize cap is required (limitParam maximum is 100 in the swagger). Sanitization also genuinely works end-to-end: a title carrying a raw ESC byte comes out as ^[[2K…, verified at runtime.
Six things to address, in rough priority order — inline. The first two are the ones I'd want fixed before merge; the rest are cheap consistency fixes that are much easier now than after the command ships.
The reason will be displayed to describe this comment to others. Learn more.
Silent truncation: this warning never fires when the API omits pagination.total.
total is optional in PaginationInfo. When the response doesn't carry it, the loop on L131 exits as soon as items.length >= limit even though cursor is still set, and then L135's total ??= items.length makes this guard false. Result: we print Found N pull requests with no "Showing the first N results" hint, and the user has no idea results were dropped.
Verified against a mocked response { data: [pr, pr], pagination: { cursor: "next" } } with --limit 2 — no warning printed.
cursor is still in scope and still truthy at this point, so:
Suggested change
if(total>items.length){
if(total>items.length||cursor){
The same bug exists in findings.ts:356, which is where this loop was copied from — worth fixing there in this PR too while we're in here. (Extracting a shared pagination helper is out of scope.)
The reason will be displayed to describe this comment to others. Learn more.
This should pass "last-updated" rather than undefined.
Leaving search unset means the endpoint returns closed PRs mixed in with the open ones, and the table has no Status column and no way to tell them apart. I confirmed a PR with status: "Merged" renders identically to an open one, and the word "Merged" never appears anywhere in the output.
Note that repository.ts:151-154 deliberately filters status === "open" || "Open" client-side before rendering this exact same table — that's the existing workaround for the same problem.
Suggest a --state option rather than hard-coding it:
open (default when omitted) → search = "last-updated"
closed → search = "merged" — but name the CLI value closed, notmerged: that classification also returns closed-but-not-merged PRs, so "merged" isn't factual.
SPECS/commands/pull-requests.md currently claims the default is "Open status", which isn't true with search=undefined — that line needs updating alongside this.
The reason will be displayed to describe this comment to others. Learn more.
This whitelist can't reproduce the ✓ column the table shows.
formatStandards() computes ✓ from coverage.isUpToStandards and quality.isUpToStandards, and explicitly ignores this top-level isUpToStandards (see the doc comment at formatting.ts:507). Neither of those two fields is whitelisted, so a JSON consumer sees isUpToStandards: false with no way to derive what the table actually rendered. Gate colouring for Issues/Coverage/Complexity/Duplication comes from quality.resultReasons / coverage.resultReasons, also absent.
That breaks the pickDeep convention in AGENTS.md ("only includes fields that correspond to what's shown in the console table/card output") and diverges from pull-request.ts:1283-1284, which whitelists the whole coverage and quality objects.
At minimum add coverage.isUpToStandards and quality.isUpToStandards; add the two resultReasons if you want full gate parity with the table.
The reason will be displayed to describe this comment to others. Learn more.
Both flags diverge from names already established elsewhere in the CLI.
--search-text → prefer -q, --search <text>. findings.ts:198 and patterns.ts:176 already use exactly -q, --search <text> for the same concept (repositories/ls use -s, --search). --search-text introduces a fourth spelling of the same idea.
-b, --branch → prefer --base <name>. ls, directories and issues all use -b, --branch to mean "the analysed branch of the repo"; here it means the PR's target branch, so the same flag would carry two different concepts across commands. --base also matches gh pr list --base, which is what people will reach for.
Brand-new command, so renaming costs nothing right now.
Don't forget the pagination hint text on L173 and the addHelpText examples on L86-87.
The reason will be displayed to describe this comment to others. Learn more.
repository.ts:179 shows originBranch under an identically-named "Branch" header; this shows targetBranch. Same column label, two different values depending on which command you ran.
Better: render the pair the way the web UI does — <origin> → <target> — and rename the header on L33 to match. That also makes it self-evident which branch --base filters on.
The reason will be displayed to describe this comment to others. Learn more.
isAnalysing should be surfaced here.
It's fetched and whitelisted into the JSON output (L142) but never rendered in the table, so a PR that's mid-analysis shows stale numbers behind a hard red ✗ with no indication that the analysis hasn't finished — misleading on the column people read first.
utils/formatting.ts already has formatAnalysisStatus() (used by pull-request.ts) for exactly this.
Fixes a pagination-warning bug where a live cursor went unreported
whenever the API omitted `total` (same latent bug fixed in findings),
adds proper --state open/closed filtering (previously hardcoded,
mixing closed PRs into results), tightens the JSON whitelist, renames
-q/--search-text and -b/--branch to -q/--search and -B/--base to match
existing CLI conventions, shows origin -> target in the Branches
column, and surfaces isAnalysing via a shared formatStandards() fix.
Responds to review comments on PR #35.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
The pull-requests command is correctly implemented and satisfies the core acceptance criteria, including auto-detection of repository context and support for server-side filtering. Codacy analysis indicates the PR is up to standards.
There is a minor discrepancy between the PR documentation and the implementation regarding flag naming: the code uses --base (-B) while the description suggested --branch (-b). Additionally, while the command functional requirements are met, the implementation exhibits high cyclomatic complexity (23) in the main action handler and significant duplication in test setup. Addressing the suggested modularization of JSON output and state validation will improve the robustness of the command.
About this PR
The implementation uses --base (-B) for branch filtering, but the PR description specified --branch (-b). While --base is more accurate for PR contexts, ensure documentation is aligned.
Test suggestions
Command lists PRs for a repository using auto-detected git context
Filtering results by search text (textQuery API param)
Filtering results by target branch (targetBranch API param)
Filtering results by state (open/closed classification)
Pagination logic follows cursors and respects the --limit boundary
Analysis status helper shows dim '⋯' when isAnalysing is true
JSON output correctly filters fields to match the table display
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: Include the result reasons in the JSON output to allow consumers to see which specific gates passed or failed, maintaining parity with the colored columns in the table output.
Try running the following prompt in your coding agent:
Add "quality.resultReasons" and "coverage.resultReasons" to the pickDeep whitelist in printPullRequestsJson in src/commands/pull-requests.ts.
Complexity rendered as no data on every PR: the pull-request endpoints
return `quality.deltaComplexity` and omit the flat top-level
`deltaComplexity` (while still sending a top-level `deltaClonesCount`).
New shared `prQualityMetric()` reads the nested `quality` value first,
falling back to the flat field — also applied to `repository`'s Open PR
table and `pull-request`'s Analysis section, which had the same bug.
Table changes for `pull-requests`:
- `✓` moved to the first column
- metric order matches `repositories`: issues, complexity, duplication,
coverage
- Coverage column dropped entirely when no listed PR carries a coverage
value (repos without coverage return `diffCoverage.cause` and no
numbers on any PR)
- missing metrics render as a dim `-` instead of `N/A`, matching
`formatStandards`/`formatCountCell`/`formatCoverageCell`
JSON output gains `quality.resultReasons`/`coverage.resultReasons`
(addresses the Codacy review comment — they drive the per-metric gate
coloring) plus the `quality.*` metric mirrors the table renders.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`formatPrIssues` rendered a zero fixed-issue count as `-0`, which reads
as a negative number. A zero count now renders as a bare `0` — the sign
says nothing when nothing was added or fixed. Same rule
`pull-request.ts`'s Files table and `formatFileDelta` already follow,
and `formatDelta` already did for complexity/duplication.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
alerizzo
left a comment
What
pull-requests/prscommand (src/commands/pull-requests.ts), registered insrc/index.ts— the plural counterpart topull-request(which shows a single PR by number).-q, --search-text <text>and-b, --branch <name>map toAnalysisService.listRepositoryPullRequests'stextQuery/targetBranchparams (added server-side in OD-376). No API client regen needed —57.3.9(already pinned from the OD-296/OD-397 work this branch is based on) already has both params.[provider] [org] [repo]auto-detect from the git remote viaresolveRepoArgs, same asrepository/ls/directories.-n, --limit <n>(default 100, max 1000) with the same paginate-to-limit loop asfindings.repository's "Open Pull Requests" table (buildGateStatus,formatStandards,formatPrIssues,formatPrCoverage,formatDelta) — Branch column showstargetBranch(the dimension--branchfilters on) rather thanoriginBranch.searchparam (Merged vs. last-updated classification) is deliberately not exposed — different axis, out of scope per the ticket.Why
OD-378: expose the OD-376 API filters (text query + branch scope) as a proper list command, mirroring
issues/repositories.Testing
src/commands/pull-requests.test.ts— full suite passes (516 → see this PR's base for baseline).tsc --noEmitfully clean (also fixed a realpackage-lock.json/update-notifierdrift that was causing a pre-existing, unrelated tsc/ts-node error —update-notifierwas declared inpackage.jsonbut missing fromnode_modules/lockfile).gh codacy codacy-website, built vianpm run build, using a storedcodacy logincredential):--branch main→ 0 results (repo's PRs all targetmaster),--branch master→ matches the unfiltered count,--search-text "AI"→ narrows to exactly the one matching title,--output jsonshape correct, and auto-detect from the git remote works from inside a real checkout. Satisfies the ticket's manual-verify step.Related
feature/source-id-issues-OD-296(codacy-cloud-cli#34)