feat: Accept repository token for remote config OD-497 - #206
Conversation
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 1 minor |
🟢 Metrics 42 complexity · 2 duplication
Metric Results Complexity 42 Duplication 2
🟢 Coverage 62.69% diff coverage · +0.47% coverage variation
Metric Results Coverage variation ✅ +0.47% coverage variation (-0.50%) Diff coverage ✅ 62.69% diff coverage (50.00%) Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (6dfabce) 6301 1561 24.77% Head commit (b165533) 6331 (+30) 1598 (+37) 25.24% (+0.47%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#206) 67 42 62.69% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
dede01d to
045169c
Compare
045169c to
7620a83
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces the --project-token flag to support repository-specific authentication for remote configurations. While the core functionality is present, the implementation contains a logic tautology in analyze.go that renders error handling unreachable. Furthermore, the decision to default the flag to an environment variable in the global flag definition implicitly forces the CLI into 'remote mode' for any user with that variable set, which may break local workflows. Codacy results indicate that the PR is not up to standards due to insufficient diff coverage (25%). Critical safety guards in config.go and the primary mode-detection logic in domain/initFlags.go lack test coverage, which should be addressed before merging to prevent regressions.
About this PR
- The new helper method
InitFlags.HasRemoteToken()is the primary logic gate for determining if the CLI should operate in remote or local mode, yet it currently has 0% test coverage. Given its central role in the new logic, unit tests for this method are essential. - Integration-level testing for the CLI commands (
init,analyze,config) is missing for the new token flag. Several lines in the command implementation files are uncovered, indicating that the new flag's impact on command execution flow has not been verified.
Test suggestions
- Verify getRequest sets 'api-token' header and skips 'project-token' when both are provided (precedence).
- Verify getRequest sets 'project-token' header when only ProjectToken is present in InitFlags.
- Verify CLI flags correctly bind the CODACY_PROJECT_TOKEN environment variable as a default value.
- Verify InitFlags.HasRemoteToken() returns true when only ProjectToken is set.
- Verify 'config reset' correctly identifies remote mode and enforces required flags (provider/org/repo) when using --project-token.
- Verify coverage for safety guards in cmd/config.go (lines 57-61).
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify CLI flags correctly bind the CODACY_PROJECT_TOKEN environment variable as a default value.
2. Verify InitFlags.HasRemoteToken() returns true when only ProjectToken is set.
3. Verify 'config reset' correctly identifies remote mode and enforces required flags (provider/org/repo) when using --project-token.
4. Verify coverage for safety guards in cmd/config.go (lines 57-61).
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| if currentCliMode == "remote" && !tokenFlagProvided { | ||
| fmt.Println("Error: Your Codacy CLI is currently configured in 'remote' (cloud) mode.") | ||
| fmt.Println("To reset your configuration using remote settings, you must provide the --api-token, --provider, --organization, and --repository flags.") | ||
| fmt.Println("To reset your configuration using remote settings, you must provide --api-token (or --project-token) together with the --provider, --organization, and --repository flags.") | ||
| fmt.Println("Running 'config reset' without these flags is not permitted while configured for 'remote' mode.") | ||
| fmt.Println("This prevents an accidental switch to a local default configuration.") |
There was a problem hiding this comment.
🟡 MEDIUM RISK
This block implements a safety guard to prevent accidental configuration loss when the CLI is in remote mode. It currently lacks test coverage, posing a risk that future logic changes might break this protection without being detected. It is highly recommended to add a test case that verifies the 'config reset' command exits with the appropriate error when called in remote mode without tokens.
| cmd.Flags().StringVar(&flags.Organization, "organization", "", "Remote organization name to fetch configurations from Codacy. Required when api-token is provided") | ||
| cmd.Flags().StringVar(&flags.Repository, "repository", "", "Remote repository name to fetch configurations from Codacy. Required when api-token is provided") | ||
| cmd.Flags().StringVar(&flags.ApiToken, "api-token", "", "Optional Codacy account API token. If defined, configurations will be fetched from Codacy") | ||
| cmd.Flags().StringVar(&flags.ProjectToken, "project-token", os.Getenv("CODACY_PROJECT_TOKEN"), "Optional Codacy repository token (defaults to CODACY_PROJECT_TOKEN). Alternative to api-token for fetching configurations from Codacy") |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Setting a default value from an environment variable here makes the CLI assume 'remote mode' whenever CODACY_PROJECT_TOKEN is present, even if the user didn't specify the flag. This causes commands like init and config reset to fail with validation errors for missing --provider or --organization flags. It is better to check the environment variable only when the required remote context is also present.
| tests := []struct { | ||
| name string | ||
| flags domain.InitFlags | ||
| expectedApiToken string |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Follow Go naming conventions for initialisms. The field 'expectedApiToken' should be named 'expectedAPIToken' to maintain consistency with Go naming standards and other types in the project.
| }) | ||
| return nil | ||
| } else if (!cliLocalMode && initFlags.ApiToken != "") || cliLocalMode { | ||
| } else if (!cliLocalMode && initFlags.HasRemoteToken()) || cliLocalMode { |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: This condition (!cliLocalMode && initFlags.HasRemoteToken()) || cliLocalMode always evaluates to true because cliLocalMode is the negation of HasRemoteToken(). This makes the else block on line 350 unreachable dead code, including the log message on line 354. The logic should be simplified or corrected to ensure the intended fallback behavior works.
7620a83 to
342ca68
Compare
There was a problem hiding this comment.
Pull Request Overview
While this PR successfully introduces support for repository-scoped tokens, it falls short of project quality standards due to significant gaps in validation and automated testing. Specifically, the analyze and config discover commands lack the necessary validation calls (ValidateRemoteFlags), which deviates from the objective of providing clear user feedback when mandatory repository coordinates are missing.
Files such as cmd/init.go and cmd/cmdutils/flags.go are identified as high-risk due to their complexity and a total lack of test coverage on the newly introduced logic paths. Integration tests are notably absent for the command execution flows, leaving the impact of the new flags on real-world usage unverified. Additionally, several idiomatic Go linting issues were identified in error message formatting that should be corrected for consistency.
About this PR
- There is a systemic lack of integration or functional testing for the new flags within the actual command execution logic. Core files like
cmd/init.goandcmd/analyze.goshow 0% coverage on the logic branches introduced in this PR. - The
analyzeandconfig discovercommands do not callValidateRemoteFlags. Users providing only a token to these commands will likely encounter malformed request errors from the API client instead of the intended clear validation messages. This is a gap in the acceptance criteria regarding error handling.
Test suggestions
- Verify InitFlags.HasRemoteToken() returns true if either ApiToken or ProjectToken is set.
- Verify getRequest sets the correct authentication headers and respects token priority (account token wins).
- Verify ValidateRemoteFlags returns an error if a token is provided without provider, organization, or repository.
- Integration: Verify the 'init' command correctly processes the --project-token flag to fetch remote config.
- Integration: Verify the 'config reset' command correctly processes the --project-token flag.
- Integration: Verify the 'analyze' command correctly identifies remote mode when only --project-token is provided.
- Verify AddCloudFlags correctly registers all flags (api-token, project-token, provider, organization, repository) to a cobra command with expected default values.
- Write an integration test for the 'init' command that executes the Run function with different flag combinations and asserts that ValidateRemoteFlags is called.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Integration: Verify the 'init' command correctly processes the --project-token flag to fetch remote config.
2. Integration: Verify the 'config reset' command correctly processes the --project-token flag.
3. Integration: Verify the 'analyze' command correctly identifies remote mode when only --project-token is provided.
4. Verify AddCloudFlags correctly registers all flags (api-token, project-token, provider, organization, repository) to a cobra command with expected default values.
5. Write an integration test for the 'init' command that executes the Run function with different flag combinations and asserts that ValidateRemoteFlags is called.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| } | ||
|
|
||
| cliLocalMode := len(initFlags.ApiToken) == 0 | ||
| cliLocalMode := !initFlags.HasRemoteToken() |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The analyze command is missing a call to cmdutils.ValidateRemoteFlags(initFlags). This results in the command attempting to fetch repository tools with potentially empty provider, organization, or repository segments if a token is provided without coordinates.
| } | ||
|
|
||
| cliLocalMode := len(initFlags.ApiToken) == 0 | ||
| if err := cmdutils.ValidateRemoteFlags(initFlags); err != nil { |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The logic in the Run block for initCmd is uncovered. Since this controls the primary workflow of the tool, consider adding integration tests to verify that provided flags correctly trigger remote configuration vs local defaults.
| cmd.Flags().StringVar(&flags.Provider, "provider", "", "Provider (e.g., gh, bb, gl) to fetch configurations from Codacy. Required when api-token is provided") | ||
| cmd.Flags().StringVar(&flags.Organization, "organization", "", "Remote organization name to fetch configurations from Codacy. Required when api-token is provided") | ||
| cmd.Flags().StringVar(&flags.Repository, "repository", "", "Remote repository name to fetch configurations from Codacy. Required when api-token is provided") | ||
| cmd.Flags().StringVar(&flags.ApiToken, "api-token", "", "Optional Codacy account API token. If defined, configurations will be fetched from Codacy") |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The flag registration logic in AddCloudFlags is currently untested. Consider adding a unit test in flags_test.go that initializes a cobra command, calls AddCloudFlags, and verifies that all flags (api-token, project-token, provider, organization, repository) are correctly registered with their expected default values.
| return nil | ||
| } | ||
| if flags.Provider == "" || flags.Organization == "" || flags.Repository == "" { | ||
| return errors.New("Error: When using --api-token or --project-token, you must also provide --provider, --organization, and --repository flags.") |
There was a problem hiding this comment.
⚪ LOW RISK
Follow Go error string conventions: remove the 'Error: ' prefix, use lowercase, and remove the trailing period.
| return errors.New("Error: When using --api-token or --project-token, you must also provide --provider, --organization, and --repository flags.") | |
| return errors.New("when using --api-token or --project-token, you must also provide --provider, --organization, and --repository flags") |
342ca68 to
bc77fd9
Compare
There was a problem hiding this comment.
Pull Request Overview
The PR successfully implements the support for Codacy repository-specific tokens via the --project-token flag and CODACY_PROJECT_TOKEN environment variable. Codacy results are up to standards, and the core utility functions for flag resolution are well-tested.
However, there are two primary concerns that should prevent merging in the current state:
- Authentication Regressions: In
cmd/configsetup, changes have introduced logic that ignores provided flags and uses an empty struct for API calls, effectively stripping all authentication headers (both API tokens and Project tokens). - Execution Order Bug: In the
config resetcommand, the CLI checks for token presence before resolving environment variables, which will cause failures for users relying on theCODACY_PROJECT_TOKENvariable. - Test Coverage Gaps: While low-level utilities are covered, the actual command handlers (
init,analyze,config) show zero coverage for the newly introduced branches, and integration tests for high-level API client wrappers are missing.
About this PR
- There is a systemic lack of integration testing for the new flags within the actual command execution logic. Core files like
cmd/init.goandcmd/analyze.goshow 0% coverage on the logic branches introduced in this PR, meaning the end-to-end wiring of the repository token is unverified.
Test suggestions
- Found: Verify flag registration for cloud-related parameters in cmdutils.
- Found: Test ResolveProjectToken handles environment variable fallback only when coordinates are present.
- Found: Test ValidateRemoteFlags rejects tokens provided without provider/org/repo coordinates.
- Found: Verify getRequest header precedence: api-token header wins over project-token.
- Missing: Integration test: ensure 'analyze' command correctly invokes PrepareRemoteFlags (coverage check).
- Missing: Integration tests for API wrapper functions (GetRepositoryTools, GetRepositoryLanguages) to verify flag propagation.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Missing: Integration test: ensure 'analyze' command correctly invokes PrepareRemoteFlags (coverage check).
2. Missing: Integration tests for API wrapper functions (GetRepositoryTools, GetRepositoryLanguages) to verify flag propagation.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| } | ||
|
|
||
| apiTokenFlagProvided := len(configResetInitFlags.ApiToken) > 0 | ||
| tokenFlagProvided := configResetInitFlags.HasRemoteToken() |
There was a problem hiding this comment.
🔴 HIGH RISK
The remote token check is performed before PrepareRemoteFlags resolves environment variables. Reorder the logic in the Run function of configResetCmd so that cmdutils.PrepareRemoteFlags(cmd, &configResetInitFlags) is called before checking tokenFlagProvided, and then update tokenFlagProvided to reflect the resolved flags.
| } | ||
|
|
||
| patternsConfig, err := codacyclient.GetToolPatternsConfig(flags, toolUUID, true) | ||
| patternsConfig, err := codacyclient.GetToolPatternsConfig(domain.InitFlags{}, toolUUID, true) |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The flags parameter is ignored here; GetToolPatternsConfig is called with an empty struct, stripping authentication. If the goal is to prevent sending a repository-scoped ProjectToken to a global endpoint, it is safer to selectively strip that token while preserving the ApiToken.
| patternsConfig, err := codacyclient.GetToolPatternsConfig(domain.InitFlags{}, toolUUID, true) | |
| patternsConfig, err := codacyclient.GetToolPatternsConfig(flags, toolUUID, true) |
| func createToolConfigurationsForUUIDs(uuids []string, toolsConfigDir string, initFlags domain.InitFlags) error { | ||
| for _, uuid := range uuids { | ||
| patternsConfig, err := codacyclient.GetToolPatternsConfig(initFlags, uuid, true) | ||
| patternsConfig, err := codacyclient.GetToolPatternsConfig(domain.InitFlags{}, uuid, true) |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The initFlags parameter is no longer passed to GetToolPatternsConfig. By using an empty domain.InitFlags{}, the API request is made without any authentication headers (stripping both the API token and the new Project token), which will result in 401/403 errors.
bc77fd9 to
8de99ce
Compare
There was a problem hiding this comment.
Pull Request Overview
While Codacy quality standards are technically met, several critical blockers remain. Integration of PrepareRemoteFlags into the main command handlers is still missing, leaving a significant acceptance criteria gap. The previously reported logic error where authentication flags are stripped during pattern fetching in configsetup persists, which will lead to unauthorized errors for private repositories. Additionally, cmd/cmdutils/flags.go has seen a notable increase in complexity (+9) without sufficient integration-level testing to verify the orchestration of the new repository tokens.
About this PR
- The 'Manual Testing' section in the PR description is incomplete, which hinders verification. Additionally, there is a systemic lack of integration testing for the new flags within the actual command execution logic; core files like
cmd/init.goandcmd/analyze.goshow 0% coverage on the logic branches introduced in this PR.
Test suggestions
- Verify HasRemoteToken returns true for either API token or Project token
- Ensure ResolveProjectToken does not fetch from environment if coordinates are missing
- Confirm getRequest sets project-token header when ApiToken is empty
- Verify getRequest prioritizes ApiToken over ProjectToken in headers
- Validate that PrepareRemoteFlags terminates the process if a token is used without coordinates
- Ensure commands (init, analyze, config reset) correctly integrate the PrepareRemoteFlags logic
- Add integration tests for command handlers (analyze.go, init.go, config.go) to verify token orchestration
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Ensure commands (init, analyze, config reset) correctly integrate the PrepareRemoteFlags logic
2. Add integration tests for command handlers (analyze.go, init.go, config.go) to verify token orchestration
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
CI runs with a repository token silently fell back to default tool configuration on init and config reset. CODACY_PROJECT_TOKEN is the coverage reporter's variable, so it only applies once provider, organization and repository are given. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8de99ce to
b165533
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR successfully introduces support for Codacy repository tokens via the --project-token flag and CODACY_PROJECT_TOKEN environment variable. However, the implementation currently contains a major functional regression where authentication headers are stripped during the tool pattern retrieval process in cmd/configsetup. This flaw effectively prevents the CLI from operating correctly with private tool configurations or restricted environments.
Furthermore, there is a significant gap in the acceptance criteria: the config reset command lacks the required safety logic to prevent accidental transitions from remote to local mode without explicit token confirmation. This gap is coupled with a systemic lack of integration testing for the new flag's impact on command execution flow. These issues must be addressed before the PR can be considered ready for production use.
About this PR
- The acceptance criterion requiring 'config reset' to prevent switching from remote to local mode without an explicit token is currently unaddressed in the logic and missing from the test suite.
- There is a systemic lack of integration testing for the repository token flags within the command execution logic. Core workflow paths in
cmd/init.goandcmd/analyze.goshow insufficient coverage, leaving the end-to-end token propagation unverified.
1 comment outside of the diff
cmd/config.go
line 87-93🟡 MEDIUM RISK
Nitpick: The logic for creating the local Codacy directory and tools-configs directory is duplicated between 'cmd/init.go' and 'cmd/config.go'. Consider extracting this into a shared utility function to ensure consistent permission handling and directory structure.
Test suggestions
- Verify 'project-token' header is sent to the API when --project-token is provided.
- Verify 'api-token' header takes precedence when both token types are provided.
- Verify CODACY_PROJECT_TOKEN env var is adopted only when provider, organization, and repository are set.
- Verify CLI exits with an error if a token is provided without the required repository coordinates.
- Verify 'config reset' prevents switching from remote to local mode without an explicit token.
- Integration test: verify that the 'config reset' safety guard successfully prevents accidental configuration loss.
- Integration test: verify that the 'analyze' command correctly identifies the CLI mode based on token presence.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify 'config reset' prevents switching from remote to local mode without an explicit token.
2. Integration test: verify that the 'config reset' safety guard successfully prevents accidental configuration loss.
3. Integration test: verify that the 'analyze' command correctly identifies the CLI mode based on token presence.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
Changes
InitFlagsgains aProjectTokenfield and aHasRemoteToken()helper covering either token type.--project-tokenflag oninit,config reset,config discoverandanalyze, defaulting to theCODACY_PROJECT_TOKENenvironment variable.getRequestnow takes the wholeInitFlagsand sets theproject-tokenheader when no account API token is present; the account token wins when both are given.ApiToken != ""checks that decided local vs. remote mode now useHasRemoteToken(), and the related messages mention both token types.initandconfig reset.On the name
The flag is
--project-token, matching theproject-tokenHTTP header the API actually reads andthe existing
upload --project-token. Codacy's API describes the credential as a "repository APItoken" (
createRepositoryApiToken), so the naming here is deliberately the wire name rather thanthe product one — renaming the flag would split it from
uploadfor no functional gain.Manual Testing
Run against
gh/new-pedrobpereira-org/coverage-test-repo-prodwith a repository token.init --project-tokenandconfig reset --project-tokenswitch tomode: remoteand write therepository's real configuration: 4 tools (opengrep, pmd 6.55.0, pylint 4.0.5, trivy 0.72.0) and
the repo's own pylint pattern list, instead of the 8 default tools. No silent fallback.
--api-tokenand theCODACY_PROJECT_TOKENenv var both produce byte-identical output to--project-token.CODACY_PROJECT_TOKENalone, without the coordinates, leavesinit,analyzeandconfig discoverin local mode — unchanged from before this PR.--provider/--organization/--repositoryexits 1 naming all three.uploadunchanged;upload-sbomstill rejects--project-token.