Skip to content

V2#264

Open
vishnu-deepsource wants to merge 63 commits intomasterfrom
v2
Open

V2#264
vishnu-deepsource wants to merge 63 commits intomasterfrom
v2

Conversation

@vishnu-deepsource
Copy link
Contributor

No description provided.

vishnu-deepsource and others added 7 commits January 28, 2026 15:17
Move utility packages from root `utils/` and `configvalidator/` into
organized `internal/` subdirectories:
- utils/prompt.go → internal/cli/prompt/
- utils/colors.go → internal/cli/style/
- utils/cmd_validator.go → internal/cli/args/
- utils/fetch_oidc_token.go → internal/oidc/
- utils/remote_resolver.go → internal/vcs/
- utils/fetch_remote.go → internal/vcs/remotes.go
- configvalidator/* → internal/configvalidator/
- utils/fetch_analyzers_transformers.go → internal/configdata/

Add shell completion support via internal/cli/completion package.

Update all import paths across commands and services to reflect new
structure. This improves code organization and follows Go's internal
package conventions for better encapsulation.
- Add new `auth whoami` command to display authenticated user info
- Add new `issues browse` command for opening issues in browser
- Add GetViewer API method to fetch authenticated user details
- Refactor output messages to use pterm.Print* instead of pterm.Info
  for consistency across auth, config, issues, and repo commands
- Update dependencies (pterm, testify, bubbletea, lipgloss)
- Add `runs list` command to display analysis runs for a repository
- Add `runs issues` command to show issues for a specific run
- Add GetAnalysisRuns and GetRunIssues API methods to client
- Add GraphQL schema and queries for runs and run issues
- Refactor whoami command to use boxed output format
- Remove issues browse command
- Add runs domain types and query helpers
Update all Go dependencies to their latest versions including:
- github.com/spf13/cobra: v1.5.0 → v1.10.2
- github.com/spf13/viper: v1.7.1 → v1.21.0
- github.com/getsentry/sentry-go: v0.6.0 → v0.41.0
- github.com/stretchr/testify: v1.8.4 → v1.11.1
- github.com/fatih/color: v1.12.0 → v1.18.0
- github.com/google/go-cmp: v0.5.5 → v0.6.0

Also includes improvements to issues display:
- Add terminal width-aware table rendering with column wrapping
- Improve issue location formatting (single line vs range)
- Add color-coded severity formatting
- Add JSON export capability for run issues with --json and --output-file flags
Add support for filtering issues by analyzer, category, severity, code, and path.
Filters can be specified multiple times to match any of the provided values.

New flags:
- --analyzer: filter by analyzer shortcode
- --category: filter by issue category
- --severity: filter by severity level
- --code: filter by issue code
- --path: filter by file path (supports partial matching)

Also refactor runs command to accept commit-oid argument and wire up
issue flags via AddRunIssueFlags for better flag reusability.
- Replace Makefile with justfile, add VERSION file for version tracking
- Replace goreleaser release pipeline with build-and-deploy workflow
- Add install script template
- Move version/ package to buildinfo/
- Swap DataDog/zstd (cgo) for klauspost/compress/zstd (pure Go)
- Replace deprecated io/ioutil usage with os and io
- Bump Go version to 1.25 in CI
- Update SARIF schema URL to official OASIS source
- Add TCP readiness check for mock server in tests
- Update README with auth docs and current command list
- Drop config generate/validate commands and supporting packages
- Drop issues list command and related services
- Remove configvalidator, configdata, and issues service packages
- Update root command registrations and dependencies
- Clean up justfile test targets
- Add top-level issues, metrics, and vulnerabilities commands with SDK queries
- Add transparent token refresh in GraphQL client (removes need for auth refresh)
- Remove version, auth refresh, and auth whoami commands
- Add UserError type to skip user-correctable errors from Sentry
- Improve Sentry setup with panic recovery, release tagging
- Add YAML output format and source filter for run issues
- Auto-open browser on login instead of waiting for user input
- Update GraphQL schema to use issues instead of occurrences
- Use --version flag instead of version subcommand
- Clean up error messages and stale code
- Rename `repo` to `repository` and `runs` to `analysis`
- Rename `--run` flag to `--commit` across issues, metrics, and vulnerabilities
- Update default hostname from deepsource.io to deepsource.com
- Add `human` output format as new default, keep `table` as explicit option
- Add `--output-file`, `--verbose`, `--analyzer` filter, and `--limit` flags
- Remove legacy YAML config support and debug logging infrastructure
- Add `GetEnabledAnalyzers` API endpoint and repository analyzers command
- Fix report service using Errorf instead of Printf for info messages
- Rename "human" output format to "pretty" in issues, metrics, and vulnerabilities
- Simplify issues table: remove boxed style, drop wrapText, conditionally show SOURCE column
- Humanize displayed values (severity, status, reachability, ecosystem)
- Add ECOSYSTEM column to vulnerabilities table
- Use helper functions for formatting location, severity, and analyzer names
- Rename telemetry → sentry across adapters, interfaces, and container
- Introduce cmddeps.Deps struct for injectable config, client, and stdout
- Add NewCmd*WithDeps constructors to all commands for testability
- Add --output json flag to analysis command
- Add golden-file-based tests for analysis, issues, metrics, vulnerabilities,
  repo status, repo analyzers, and auth status commands
- Add NewWithGraphQLClient and NewTestService test helpers
- Always show auth URL in login flow regardless of browser open result
- Update justfile with new test paths
- Change table headers from ALL CAPS to Title Case across all commands
- Normalize status labels to Title Case in analysis output
- Rename default output format from "table" to "pretty", keeping "table" as alias
- Add "pretty" to shell completion candidates
- Introduce buildinfo app identity vars (AppName, ConfigDirName, KeychainSvc, KeychainKey) with prod defaults
- Override identity at startup when buildMode=dev via ldflags, giving dev builds their own binary name, config dir, and keychain entries
- Update build-and-deploy workflow to pass buildMode and use dynamic binary names per environment
- Replace hardcoded "deepsource" strings with buildinfo vars in config, keychain, and root command
- Remove shell completion generation (gen-completions.sh deleted, references stripped from workflows and Homebrew formula)
- Add just install step to CI workflow
- Rename deploy -> deploy-prod in justfile, add build alias
- Add the three info-level progress lines that the report service now
  emits (preparing, compression check, uploading)
- Update report golden file with info-level progress lines
- Fix case mismatches in OIDC tests (failed -> Failed, can not -> cannot, provider -> Provider)
- Add environment check to codesign, notarize, and verify signing steps
- Only run Apple signing workflow when building for prod
- GitHub Actions blocks job outputs that match secret values
- Move bucket resolution from resolve-env outputs to deploy step
- Reference secrets directly in deploy job based on environment
- Point dev base_url to cli.deepsource.one subdomain instead of deepsource.one/cli
- Drop cli/ key prefix for dev R2 uploads since the subdomain serves the bucket root
- Fix manifest path in install script (/manifest.json, not /build/manifest.json)
func ResolveLatestRun(
ctx context.Context,
client *deepsource.Client,
remote *vcs.RemoteData,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function parameter vcs.RemoteData is unused


The parameter remote of type *vcs.RemoteData is declared in the function signature but not used anywhere in the function body, which can confuse maintainers or hint at logical errors. It increases the interface complexity without purpose.
Replace the unused parameter name with an underscore _ or remove it entirely if not needed to clarify the function's intent and prevent confusion.

func ResolveLatestRunForBranch(
ctx context.Context,
client *deepsource.Client,
remote *vcs.RemoteData,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused function parameter risks confusion or bugs


The parameter remote of type *vcs.RemoteData is declared but not used within the function, which can indicate unfinished refactoring or potential bugs due to ignored inputs. This can confuse readers or maintainers about the purpose of the parameter.
Replace the unused remote parameter with an underscore _ if it is intentionally unused, or remove it entirely to clarify function intent and avoid unnecessary code clutter.

Comment on lines 73 to 81
for _, sha := range commits {
run, err := client.GetRunByCommit(ctx, sha)
if err != nil {
continue
}
if run != nil && run.Status == "SUCCESS" && run.BranchName == branchName {
return run, nil
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple API calls are made in a loop instead of a batch request


This implementation makes one API call per commit for up to 50 commits, which can be slow and inefficient, especially on repositories with many commits on a branch. The previous paginated approach was more performant as it fetched multiple runs in a single API call. This change introduces a significant performance regression.

Consider reverting to a paginated approach or implementing a new API endpoint that can check multiple commit SHAs in a single request to reduce the number of network round trips.

Comment on lines 75 to 76
if err != nil {
continue
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error from client.GetRunByCommit is ignored inside a loop


The error from client.GetRunByCommit is ignored within the loop. If there is a persistent issue such as an invalid auth token or a network problem, the function will loop through all commits, fail silently on each API call, and then return a misleading "no analysis runs found" error. The actual root cause of the failure is lost.

Consider capturing and returning the last encountered error. This will provide more meaningful feedback to the user and make debugging significantly easier.

- Remove --output-file flag and table output format from all commands
- Rework human output: issues grouped by category, vulns by severity tables, metrics by key sections
- Return run status from ResolveLatestRun, handle in-progress and timed-out analysis runs
- Add grouped help/usage with categorized flag sections for all subcommands
- Organize root command into auth, repository, and analysis groups
- Add coverage percentage columns to changeset stats table
- Simplify report card display formatting
- Move spf13/pflag to direct dependency
- Show repo slug and branch/commit/PR info when no results are found
- Extract scopeLabel() into a reusable method in issues and reportcard
- Applies to issues, metrics, report-card, runs, and vulnerabilities commands
- Drop unused `remote *vcs.RemoteData` param from ResolveLatestRun and
  ResolveLatestRunForBranch, update all callers
- Rename unused function parameters to `_` across tests and noop impls
- Use http.NoBody instead of nil for empty HTTP request bodies
- Set MinVersion TLS 1.2 on the skip-verify transport in report service
- Print issue.Description in gray between the title and location lines
- Only shown when -v/--verbose flag is passed and description is non-empty
- Remove --output yaml option from all commands (issues, metrics, report, reportcard, analyzers, status, vulnerabilities)
- Remove yaml struct tags from all type definitions
- Delete YAML golden files and associated tests
- Move gopkg.in/yaml.v3 to indirect dependency
- Add description field to JSON issue output
- Remove leftover table format from report command completions
- Detect ADS remotes via dev.azure.com and visualstudio.com URL patterns
- Parse org name from HTTPS URLs (both dev.azure.com and legacy visualstudio.com)
- Parse org name from SSH URLs (git@ssh.dev.azure.com and vs-ssh variants)
- Guard against empty remote map to surface a clear error with --repo hint
- Add test cases covering all four ADS URL formats
- Move color/formatting helpers (severity, grade, run status) into
  internal/cli/style and add Errorf/Infof/Warnf/Successf badge printers
  that accept io.Writer
- Replace direct pterm calls with writer-based output in all commands
  (issues, runs, metrics, vulnerabilities, reportcard, auth, repo)
- Add Stderr field to cmddeps.Deps
- Auto-detect branch and walk commit history in runs list when --repo
  is not provided
- Clean up report command: rewrite help text, add flag completions for
  --analyzer and --key, add grouped usage layout
- Strip "DeepSource | Error |" prefix from all error messages in report
  service
- Change primary command name from "repository" to "repo" (keep alias)
- Update --repo flag descriptions to show provider/owner/name format
- Make auth logout/status output capturable via injected writer
- Add tests: logout success, auth status output, issues run-in-progress
  and timeout, runs empty list and auto-detect, reportcard no-runs,
  remotes empty input, cmdutil resolve_run and reportcard unit tests
}

// TestToReportCardJSON_AllFields verifies a fully-populated ReportCard converts correctly.
func TestToReportCardJSON_AllFields(t *testing.T) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High cyclomatic complexity reduces code clarity


The function TestToReportCardJSON_AllFields has cyclomatic complexity higher than recommended thresholds, indicating multiple decision points that increase difficulty in understanding and maintaining the code. This complexity can lead to more bugs and harder testing.

Refactor the function by splitting it into smaller, simpler functions and reducing nested control flow to lower cyclomatic complexity and improve maintainability.

}

// getCommitLog returns recent commit SHAs for a branch using git log.
func getCommitLog(branch string) ([]string, error) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Struct fields differing only by capitalization cause confusion


Naming struct fields or methods with names that differ only by capitalization makes the code confusing and error-prone. Developers might mistake one field for another, leading to bugs or incorrect data handling.
Rename one of the conflicting fields or methods to have a distinct and clear name, avoiding case-only differences like in the example where buildSystem and BuildSystem coexist.

- Replace per-commit GetRunByCommit lookups with GetAnalysisRuns bulk API
  using server-side branch filtering — eliminates git log dependency
- Auto-detect open PRs for the current branch and use PR-scoped endpoints
  for issues, metrics, vulnerabilities, and reportcard commands
- Add GetPRForBranch GraphQL query (pullRequests by branch, first: 1)
- Replace CommitLogFunc with RemoteFunc in Deps struct since commit
  history is no longer needed for run resolution
- Add internal/debug package gated by DEBUG=1 env var, with logging
  across GraphQL client, remote resolver, config manager, and report service
- Update all command tests to use bulk API and PR detection mocks
- Wrap HTTP transport to surface non-2xx responses as errors instead of
  silently decoding them as empty data
- Detect HTTP 401 as token expiry alongside "Signature has expired"
- Normalize legacy host aliases (deepsource.io, app.deepsource.com) to
  deepsource.com before building API URLs
- Update "no runs found" error to suggest --pr flag
- Add tests for transport, hostname normalization, and URL generation
- Normalize enum filters so hyphens become underscores (bug-risk -> BUG_RISK)
- Replace bare fmt.Println/Printf with Fprintln/Fprintf through opts.stdout()
  in issues, reportcard, and vulnerabilities commands
- Add unit test for normalizeEnumValue and integration test for hyphenated
  category filter with --commit flag
- Wire pterm table renders in reportcard, metrics, and runs through
  the io.Writer instead of defaulting to stdout
- Pass writer into outputChangesetStats and printFooter in metrics
- Pass writer into showRunsTable in runs
- Consistent with the testability pattern used across other commands
- Add ErrNotLoggedIn, ErrTokenExpired, ErrAuthTimeout constructors to internal/errors
- Add WrapAPIError that surfaces auth errors instead of burying them under generic API errors
- Add IsAuthError method on ErrorCode for auth error detection
- Replace scattered inline auth error strings across login, logout, status, config, graphqlclient, runs, and repo service
- Update tests to match new consistent error wording
…ctor commands

- Verify tokens against server in auth login and auth status instead of only checking local expiry
- Warn about unpushed commits when issues are shown for an auto-detected branch
- ResolveLatestRunForBranch now skips runs without a report card, falls back to most recent
- Extract helper functions from large Run() methods in issues, metrics, vulnerabilities, reportcard, runs, and report service
- Add HasUnpushedCommitsFunc to Deps for testability
- Fix CODE_PATH env var leak in report tests
- Use http.NoBody in transport tests
- Add auth status server-rejected and unpushed commits test cases
}
}()

os.Exit(run())
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`os.Exit` skips deferred statements causing resource leaks


Calling os.Exit on line 69 terminates the program immediately without executing any deferred functions, which can lead to resource leaks or incomplete cleanup operations in the enclosing function.

Replace direct os.Exit calls with controlled return codes and defer os.Exit in a top-level defer function to ensure deferred cleanup executes before program termination.

@@ -63,11 +76,37 @@ func NewCmdLogin() *cobra.Command {

// Run executes the auth command and starts the login flow if not already authenticated
func (opts *LoginOptions) Run() (err error) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`func Run` has a cyclomatic complexity of 16 with "high" risk


A function with high cyclomatic complexity can be hard to understand and
maintain. Cyclomatic complexity is a software metric that measures the number of
independent paths through a function. A higher cyclomatic complexity indicates
that the function has more decision points and is more complex.


// Mock client that returns an auth error from GetViewer
mock := graphqlclient.NewMockClient()
mock.QueryFunc = func(_ context.Context, query string, _ map[string]any, result any) error {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function parameter `result` is unused


The function assigned to mock.QueryFunc declares result as a parameter but does not use it anywhere, which may indicate unfinished code or an oversight that can confuse readers or cause maintenance issues. Leaving unused parameters reduces code clarity and may lead to undetected bugs.
Rename the unused result parameter to _ or remove it entirely if it is unnecessary to clearly indicate it is unused and improve code readability.

}

// TestToReportCardJSON_AllFields verifies a fully-populated ReportCard converts correctly.
func TestToReportCardJSON_AllFields(t *testing.T) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function has high cyclomatic complexity reducing maintainability


The function TestToReportCardJSON_AllFields has cyclomatic complexity above the recommended threshold, which means it contains many branching paths and decision points. This complexity makes it harder to understand, maintain, and test, increasing the likelihood of bugs.

Refactor TestToReportCardJSON_AllFields by breaking it into smaller functions or simplifying complex logic. This reduces branching and improves readability and maintainability.

Comment on lines +101 to +107
if clientErr == nil {
_, verifyErr := client.GetViewer(context.Background())
if verifyErr != nil {
// Server rejected the token — treat as expired
opts.TokenExpired = true
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error `clientErr` from `deepsource.New` is ignored, causing an incorrect authentication flow.


The error clientErr returned from deepsource.New is not handled. If client creation fails, for example due to a malformed hostname in the configuration, the program silently continues and bypasses the server-side token validation. This can mask configuration issues and prevent the user from being prompted to re-authenticate when necessary.

The error should be handled. A robust approach would be to treat a client creation failure as a validation failure by setting opts.TokenExpired = true, which would trigger the re-authentication flow.

- Interactive TTY users can choose to wait for analysis or view last completed results
- Non-interactive environments auto-fallback to the last completed run
- Simplified ResolveLatestRunForBranch to delegate to resolveRunFromCommits
- Reordered issue output to show severity tag before issue text
- Added WaitOrFallback and ResolveLatestCompletedRun helpers with tests
- Removed stale reportcard-fallback test fixtures, added new status-based ones
- Set autoDetectedBranch when resolving PR for branch in issues, metrics,
  and vulnerabilities commands so auto-detected context is preserved
- Change Infof style from colored "→" prefix to plain "Note:" with a
  leading blank line for better readability
- Update tests to match new Infof format and add missing
  HasUnpushedCommitsFunc mock
- Remove analyzer name tag from issue lines, show only severity and text
- Return branch name as scope label when commit OID is empty
- Remove unused analyzerDisplayName function
- Wait for in-progress analysis runs before fetching PR issues on
  auto-detected branches
- Sort issues by severity (critical > major > minor) within each
  file group in human output
- Fix double-space between severity tag and issue text
- Skip unpushed commits warning when there are no issues to show
- Update test mock to include analysis runs query
- Show interactive scope picker (default branch / PR / commit) when no
  issues are found on the auto-detected branch, so the user can quickly
  retry without re-running the command
- Extend local-changes warning to detect uncommitted changes alongside
  unpushed commits, with distinct messages for each combination
- Add HasUncommittedChanges helper in cmdutil and wire injectable funcs
  through Deps for testability
- Fix summary line indentation in human output
- Add tests for all new paths and two new empty-response golden files
- When issues list is empty on an auto-detected branch, check if the
  latest run is still in-progress before showing the scope menu
- Catches cases where resolveIssues bypassed WaitOrFallback (e.g. PR
  path ignoring FALLBACK status)
- Interactive mode offers wait/fallback choice, non-interactive falls
  back to last completed run automatically
- Add tests for safety net in both interactive and non-interactive modes

// Mock client that returns an auth error from GetViewer
mock := graphqlclient.NewMockClient()
mock.QueryFunc = func(_ context.Context, query string, _ map[string]any, result any) error {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function parameter `result` declared but not used


The function literal assigned to mock.QueryFunc declares result as a parameter but does not use it anywhere, which can confuse maintainers and indicate incomplete refactoring or implementation.
Rename unused parameters to _ or remove them entirely if not needed to clarify intent and prevent false positive flags from static analysis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants