[REVIEW] CODERABBIT REVIEW - #1
Open
Kawixh wants to merge 11 commits into
Open
Conversation
Implement the milestone 6 global cache foundation with cache layout creation, content-addressed built-in rule caching, source metadata, cache update/clean commands, JSON reports, and offline scan cache preparation. Also complete maintainer matching for local rules by extracting maintainer-style metadata from package manifests and wiring it into rule and blocklist findings.
Implement milestone 07 threat source plumbing with OSV, npm registry metadata, cached OpenSSF malicious package records, cached GitHub Advisory records, offline cache reads, source status reporting, and required-source failure handling. Wire threat findings into scan snapshots and reports while preserving local policy findings from the previous rules milestone.
Add a bounded JavaScript analyzer for encoded payload recovery, sink-flow detection, decoded payload caching, and scan finding integration. Extend finding evidence with decoded payload metadata and cover analyzer behavior with unit and scan integration tests.
Document the Malox Homebrew tap publishing flow, install and upgrade commands, tap token setup, and first-release troubleshooting. Add a milestone for non-blocking latest-release checks that preserve scan execution and JSON output.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (10)
AGENTS.md (1)
7-7: ⚡ Quick winConsider referencing specific verification tools for clarity.
Line 7 instructs to "run go build, test and code verification, format commands," but docs/go-code-guidelines.md (lines 944-954) specifies the exact tools that should be run: gofmt, goimports, go vet, golangci-lint, go test, go test -race, and govulncheck. Adding a reference to that section would provide clearer guidance and ensure consistency.
📝 Proposed refinement for line 7
-always run go build, test and code verification, format commands to ensure that the code is free of syntax errors, follows the Go code style guidelines, and passes all tests before finalizing the implementation. This will help maintain code quality and ensure that the implementation is robust and reliable. +always run go build, test and code verification (gofmt, goimports, go vet, golangci-lint, go test, go test -race, govulncheck), and format commands as specified in docs/go-code-guidelines.md to ensure that the code is free of syntax errors, follows the Go code style guidelines, and passes all tests before finalizing the implementation. This will help maintain code quality and ensure that the implementation is robust and reliable.🤖 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 `@AGENTS.md` at line 7, The instruction on line 7 of the AGENTS.md file references generic "go build, test and code verification, format commands" without specifying the exact verification tools to use. Update this instruction to reference the specific tools documented in the go-code-guidelines section (gofmt, goimports, go vet, golangci-lint, go test, go test -race, and govulncheck) to provide clearer and more consistent guidance. Add an explicit reference to the go-code-guidelines documentation where these tools are detailed to help developers know exactly which commands to execute.Source: Coding guidelines
.github/workflows/release.yml (1)
20-23: ⚡ Quick winConsider security hardening for Actions checkout.
Setting
persist-credentials: falseprevents the workflow from persisting GitHub credentials to disk, reducing risk if artifacts are compromised.🔒 Suggested hardening
- name: Checkout uses: actions/checkout@v6 with: fetch-depth: 0 + persist-credentials: false🤖 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 @.github/workflows/release.yml around lines 20 - 23, The actions/checkout@v6 step is missing the persist-credentials security hardening setting. Add persist-credentials: false to the with section of the checkout action to prevent GitHub credentials from being persisted to disk, which reduces the risk of credential exposure if workflow artifacts are compromised.Source: Linters/SAST tools
internal/node/deno.go (2)
107-120: ⚡ Quick winSimplify the redundant prefix check.
The static analysis tool correctly identified that lines 112-114 contain a redundant check. The
strings.TrimPrefixfunction already handles the case where the prefix is absent (it's a no-op), so the conditional is unnecessary.♻️ Proposed simplification
if value == "" { return "", "" } - if strings.HasPrefix(value, "npm:") { - value = strings.TrimPrefix(value, "npm:") - } + value = strings.TrimPrefix(value, "npm:") idx := strings.LastIndex(value, "@")🤖 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 `@internal/node/deno.go` around lines 107 - 120, The parseDenoPackageKey function contains a redundant conditional check before calling strings.TrimPrefix. The strings.TrimPrefix function already handles the case where the prefix is not present (it returns the original string unchanged), so the if statement checking strings.HasPrefix is unnecessary. Remove the conditional on lines that check for "npm:" prefix and simply call strings.TrimPrefix directly without the condition, keeping the same logic but eliminating the redundant check.Source: Linters/SAST tools
115-119: Add test coverage for scoped package parsing.The logic correctly handles scoped npm packages like
@scope/pkg@1.0.0by finding the last@character (the version delimiter, not the scope's@). For@scope/pkgwithout a version,idx <= 0returns the package name as-is with an empty version. However, the test suite only covers non-scoped packages (left-pad@1.3.0). Adding explicit test cases for scoped packages would strengthen confidence in this behavior.🤖 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 `@internal/node/deno.go` around lines 115 - 119, Add test cases to verify that scoped npm packages are correctly parsed by the package name and version splitting logic. The current tests only cover non-scoped packages like "left-pad@1.3.0", but the code correctly handles scoped packages using LastIndex to find the last "@" character (the version delimiter). Add test cases for: scoped packages with versions like "`@scope/pkg`@1.0.0" (should split into "`@scope/pkg`" and "1.0.0"), and scoped packages without versions like "`@scope/pkg`" (should return "`@scope/pkg`" with empty version). These tests will validate that the idx <= 0 check and bounds checking correctly handle the scope prefix without incorrectly treating the scope's "@" as a version delimiter.internal/node/bun.go (1)
21-30: ⚡ Quick winConsider adding context cancellation support for lockfile parsing.
The
parseBunLockfunction performs I/O-like operations (JSON parsing, iteration over potentially large maps) but doesn't accept acontext.Contextparameter. For large lockfiles, this could lead to uninterruptible operations. Consider accepting a context and checkingctx.Err()at loop boundaries.Based on learnings, always pursue the goal and play devil's advocate: if a malicious or extremely large lockfile is encountered, the scanner should be able to cancel gracefully.
🤖 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 `@internal/node/bun.go` around lines 21 - 30, The parseBunLock function lacks context cancellation support which is necessary for handling potentially large or malicious lockfiles that could cause uninterruptible operations. Modify the parseBunLock function signature to accept a context.Context as the first parameter, then add context cancellation checks by calling ctx.Err() at appropriate points throughout the function, particularly at loop boundaries where iteration over potentially large maps occurs, to ensure the operation can be gracefully cancelled when needed.Source: Learnings
internal/node/inventory.go (1)
341-357: 💤 Low valuePotential redundant sort in deduplication.
dedupeDependenciescallsslices.SortFuncon line 345, butBuild()also callssortInventory()on line 113, which sorts dependencies again on line 296. SincededupeDependenciesis called beforesortInventory(line 115 before line 113), the second sort is necessary. However, if the deduped output is already sorted, you could document this invariant or avoid the extra work.💡 Alternative: Document the sorting contract
If
dedupeDependenciesguarantees sorted output, add a comment:func dedupeDependencies(deps []Dependency) []Dependency { + // Returns dependencies in sorted order after deduplication. if len(deps) == 0 {Then potentially skip re-sorting in
sortInventorywhen that invariant is known.🤖 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 `@internal/node/inventory.go` around lines 341 - 357, The dedupeDependencies function sorts dependencies using slices.SortFunc before deduplicating them, and this sorted output is then passed to sortInventory which sorts again, causing redundant work. Add a comment to the dedupeDependencies function documenting that it guarantees sorted output as a postcondition, so the contract is clear that the returned slice is always sorted by the compareDependency ordering. This makes the invariant explicit and allows for potential optimization of the redundant sort in sortInventory.internal/scan/types.go (1)
29-46: ⚡ Quick winConsider adding JSON struct tags to serialized types.
The
Snapshotstruct and several others (File,SkipReason,SkippedFile,SkippedDirectory,Issue,Summary) are part of the scan snapshot schema that will be serialized to JSON (as evidenced bySchemaVersion = "malox.scan.snapshot.v1"and context showing this being written). OnlyThreatSourceStatushas JSON tags. Without explicit tags, field names will serialize using Go's default casing, which may not match your intended schema.📋 Example: Add JSON tags for consistent serialization
type Snapshot struct { - SchemaVersion string - ScannerVersion string - ScanID string + SchemaVersion string `json:"schema_version"` + ScannerVersion string `json:"scanner_version"` + ScanID string `json:"scan_id"` // ... continue for all fieldsApply similar tags to
File,SkipReason,SkippedFile,SkippedDirectory,Issue, andSummary.🤖 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 `@internal/scan/types.go` around lines 29 - 46, The Snapshot struct and related structs (File, SkipReason, SkippedFile, SkippedDirectory, Issue, and Summary) are serialized to JSON but lack explicit JSON struct tags on their fields. Add json struct tags to all exported fields in these structs, using the ThreatSourceStatus struct as a reference for the tagging pattern already implemented. Apply consistent JSON tag naming (typically snake_case) across all fields in Snapshot and the other mentioned structs to ensure the serialized JSON output matches the intended malox.scan.snapshot.v1 schema format.internal/threat/osv.go (1)
199-215: 💤 Low valueConsider adding overflow protection in version parsing.
The manual digit parsing at lines 206-210 could theoretically overflow
intfor very long digit sequences (e.g., maliciously crafted version strings with hundreds of digits). While OSV data is curated and this is unlikely in practice, usingstrconv.Atoior capping the iteration count would be more defensive.♻️ Optional refactor using strconv
func versionParts(version string) []int { version = strings.TrimPrefix(version, "v") version, _, _ = strings.Cut(version, "-") chunks := strings.Split(version, ".") out := make([]int, 0, len(chunks)) for _, chunk := range chunks { - var value int - for _, r := range chunk { - if r < '0' || r > '9' { - break - } - value = value*10 + int(r-'0') - } + // Extract leading digits + var i int + for i = 0; i < len(chunk); i++ { + if chunk[i] < '0' || chunk[i] > '9' { + break + } + } + digits := chunk[:i] + value, _ := strconv.Atoi(digits) // Ignore error, default to 0 out = append(out, value) } return out }🤖 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 `@internal/threat/osv.go` around lines 199 - 215, The versionParts function manually parses digits into an int value using a loop that multiplies by 10 and adds each digit without overflow protection, which could overflow for maliciously crafted version strings with very long digit sequences. Replace the manual digit parsing loop (the for loop iterating over rune r in chunk) with strconv.Atoi to safely convert the chunk string to an integer, or alternatively add a cap on the iteration count to prevent excessive accumulation of the value variable.internal/threat/threat.go (2)
368-375: 💤 Low valueUnchecked error from deferred
resp.Body.Close().The deferred
resp.Body.Close()at line 369 does not check the returned error. While Close errors on HTTP response bodies are often safe to ignore (the response has been read), explicitly acknowledging the ignored error improves code clarity.♻️ Proposed fix to explicitly ignore Close error
func readHTTPResponse(resp *http.Response) ([]byte, error) { - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() data, err := io.ReadAll(io.LimitReader(resp.Body, 10<<20)) if err != nil { return nil, err } return data, nil }🤖 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 `@internal/threat/threat.go` around lines 368 - 375, In the readHTTPResponse function, the deferred resp.Body.Close() call ignores the returned error without explicitly acknowledging it. Replace the simple defer statement with a deferred anonymous function that captures and explicitly ignores the error returned by Close(), using the blank identifier pattern such as defer func() { _ = resp.Body.Close() }() to make it clear that the error is intentionally ignored.Source: Linters/SAST tools
344-366: ⚖️ Poor tradeoffLimited retry logic only covers 5xx errors.
The retry loop at lines 345-366 uses
for attempt := range 2(attempts 0 and 1, so one retry). However, it only retries on 5xx server errors (line 356-359) and not on transient network errors (connection failures, timeouts). Network errors at line 347 are captured but break immediately without retry.Consider whether the current retry strategy aligns with the resilience goals for threat-intelligence queries. If transient network failures should be retried, the logic would need to distinguish retryable errors.
🤖 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 `@internal/threat/threat.go` around lines 344 - 366, The retry logic in the loop starting with `for attempt := range 2` does not properly distinguish between transient and permanent errors. Currently, network errors from `opts.httpClient().Do(req)` and readHTTPResponse() are caught and retried, but the strategy should be more selective—only truly transient errors like timeouts or connection resets should trigger a retry. Review the error handling at the err assignment and readErr assignment points, and add type assertions or error checking to identify transient network errors using appropriate error checking methods, while allowing permanent errors to fail immediately without retry.
🤖 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 @.github/workflows/release.yml:
- Around line 5-7: The release workflow is missing test execution before
publishing releases. Add a new step in the .github/workflows/release.yml
workflow that runs `go test` to validate code quality. Position this test step
after the "Set up Go" step and before the "Select next version" step to ensure
tests pass before any release builds or publications occur.
In @.goreleaser.yml:
- Around line 47-63: The brews configuration in the goreleaser.yml file has
inconsistent repository ownership that conflicts with documentation references.
Change the owner field in the brews section from darckorp to Kawixh, and update
the corresponding homepage URL from https://github.com/darckorp/malox to
https://github.com/Kawixh/malox to ensure the Homebrew formula is published to
the correct repository and displays the correct project homepage.
- Around line 5-7: The before hooks section in .goreleaser.yml currently only
contains the `go mod tidy` hook but does not execute tests as required by the
coding guidelines. Add a test execution hook (such as `go test ./...`) as an
additional entry in the before hooks list under the hooks key to ensure tests
are run before the release build is created.
In `@internal/cache/cache.go`:
- Line 619: The parseTime function call on the fetchedAt assignment is silently
discarding the error return value using the blank identifier, which means
malformed timestamp data in source.FetchedAt will result in a zero time value
with no indication of failure. Modify the code to capture both return values
from parseTime, check if an error was returned, and either log the error, return
an error from the containing function, or otherwise handle the parse failure
appropriately to prevent silent data corruption.
- Around line 97-103: The snapshot verification in the LoadSnapshot method only
checks that the ScanID matches after reloading, but does not verify the
integrity of the entire snapshot structure. To strengthen the verification and
detect potential corruption from JSON marshaling/unmarshaling issues, add a
comprehensive comparison beyond the ScanID check. This could include comparing
the full snapshot object byte-level (by re-marshaling to JSON and comparing) or
computing a checksum of the entire snapshot before writing and comparing it
after loading. Replace or supplement the current ScanID-only comparison with a
full snapshot integrity check to catch silent corruption in other fields like
timestamps or nested structures.
In `@internal/node/jsanalysis/jsanalysis.go`:
- Around line 916-923: The boundary check before reading 2 hex digits in the hex
escape handling logic contains an off-by-one error. The condition `i+1 <=
len(source)` only validates that position `i+1` is within bounds, but reading at
positions `i` and `i+1` requires ensuring both indices are valid. Change the
condition from `i+1 <= len(source)` to `i+2 <= len(source)` to properly validate
that there are 2 characters available to read starting from position `i`.
In `@internal/threat/threat.go`:
- Around line 335-342: The `ctx` parameter in the `do` function is shadowed
before being used, as a new timeout context is created from `req.Context()`
instead of the passed `ctx`. Fix this by modifying the context.WithTimeout call
to use the passed `ctx` parameter as the parent context instead of
`req.Context()`. This ensures the passed context parameter is properly respected
as the base context for creating the timeout context, which may carry important
deadlines or values from the caller.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 20-23: The actions/checkout@v6 step is missing the
persist-credentials security hardening setting. Add persist-credentials: false
to the with section of the checkout action to prevent GitHub credentials from
being persisted to disk, which reduces the risk of credential exposure if
workflow artifacts are compromised.
In `@AGENTS.md`:
- Line 7: The instruction on line 7 of the AGENTS.md file references generic "go
build, test and code verification, format commands" without specifying the exact
verification tools to use. Update this instruction to reference the specific
tools documented in the go-code-guidelines section (gofmt, goimports, go vet,
golangci-lint, go test, go test -race, and govulncheck) to provide clearer and
more consistent guidance. Add an explicit reference to the go-code-guidelines
documentation where these tools are detailed to help developers know exactly
which commands to execute.
In `@internal/node/bun.go`:
- Around line 21-30: The parseBunLock function lacks context cancellation
support which is necessary for handling potentially large or malicious lockfiles
that could cause uninterruptible operations. Modify the parseBunLock function
signature to accept a context.Context as the first parameter, then add context
cancellation checks by calling ctx.Err() at appropriate points throughout the
function, particularly at loop boundaries where iteration over potentially large
maps occurs, to ensure the operation can be gracefully cancelled when needed.
In `@internal/node/deno.go`:
- Around line 107-120: The parseDenoPackageKey function contains a redundant
conditional check before calling strings.TrimPrefix. The strings.TrimPrefix
function already handles the case where the prefix is not present (it returns
the original string unchanged), so the if statement checking strings.HasPrefix
is unnecessary. Remove the conditional on lines that check for "npm:" prefix and
simply call strings.TrimPrefix directly without the condition, keeping the same
logic but eliminating the redundant check.
- Around line 115-119: Add test cases to verify that scoped npm packages are
correctly parsed by the package name and version splitting logic. The current
tests only cover non-scoped packages like "left-pad@1.3.0", but the code
correctly handles scoped packages using LastIndex to find the last "@" character
(the version delimiter). Add test cases for: scoped packages with versions like
"`@scope/pkg`@1.0.0" (should split into "`@scope/pkg`" and "1.0.0"), and scoped
packages without versions like "`@scope/pkg`" (should return "`@scope/pkg`" with
empty version). These tests will validate that the idx <= 0 check and bounds
checking correctly handle the scope prefix without incorrectly treating the
scope's "@" as a version delimiter.
In `@internal/node/inventory.go`:
- Around line 341-357: The dedupeDependencies function sorts dependencies using
slices.SortFunc before deduplicating them, and this sorted output is then passed
to sortInventory which sorts again, causing redundant work. Add a comment to the
dedupeDependencies function documenting that it guarantees sorted output as a
postcondition, so the contract is clear that the returned slice is always sorted
by the compareDependency ordering. This makes the invariant explicit and allows
for potential optimization of the redundant sort in sortInventory.
In `@internal/scan/types.go`:
- Around line 29-46: The Snapshot struct and related structs (File, SkipReason,
SkippedFile, SkippedDirectory, Issue, and Summary) are serialized to JSON but
lack explicit JSON struct tags on their fields. Add json struct tags to all
exported fields in these structs, using the ThreatSourceStatus struct as a
reference for the tagging pattern already implemented. Apply consistent JSON tag
naming (typically snake_case) across all fields in Snapshot and the other
mentioned structs to ensure the serialized JSON output matches the intended
malox.scan.snapshot.v1 schema format.
In `@internal/threat/osv.go`:
- Around line 199-215: The versionParts function manually parses digits into an
int value using a loop that multiplies by 10 and adds each digit without
overflow protection, which could overflow for maliciously crafted version
strings with very long digit sequences. Replace the manual digit parsing loop
(the for loop iterating over rune r in chunk) with strconv.Atoi to safely
convert the chunk string to an integer, or alternatively add a cap on the
iteration count to prevent excessive accumulation of the value variable.
In `@internal/threat/threat.go`:
- Around line 368-375: In the readHTTPResponse function, the deferred
resp.Body.Close() call ignores the returned error without explicitly
acknowledging it. Replace the simple defer statement with a deferred anonymous
function that captures and explicitly ignores the error returned by Close(),
using the blank identifier pattern such as defer func() { _ = resp.Body.Close()
}() to make it clear that the error is intentionally ignored.
- Around line 344-366: The retry logic in the loop starting with `for attempt :=
range 2` does not properly distinguish between transient and permanent errors.
Currently, network errors from `opts.httpClient().Do(req)` and
readHTTPResponse() are caught and retried, but the strategy should be more
selective—only truly transient errors like timeouts or connection resets should
trigger a retry. Review the error handling at the err assignment and readErr
assignment points, and add type assertions or error checking to identify
transient network errors using appropriate error checking methods, while
allowing permanent errors to fail immediately without retry.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 37f290fc-1e08-4f19-b36f-2f1a5f6dee2b
⛔ Files ignored due to path filters (7)
go.sumis excluded by!**/*.sumtestdata/node/bun/bun.lockis excluded by!**/*.locktestdata/node/deno/deno.lockis excluded by!**/*.locktestdata/node/npm/node_modules/left-pad/package.jsonis excluded by!**/node_modules/**testdata/node/npm/package-lock.jsonis excluded by!**/package-lock.jsontestdata/node/pnpm/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltestdata/node/yarn/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (70)
.github/workflows/release.yml.gitignore.goreleaser.yml.release-bump.exampleAGENTS.mdbin/maloxdocs/homebrew-distribution.mddocs/milestones/11-homebrew-distribution-and-update-checks.mddocs/milestones/README.mdgo.modinternal/app/app.gointernal/app/app_test.gointernal/app/help.gointernal/app/parse.gointernal/cache/cache.gointernal/cache/cache_test.gointernal/cache/global.gointernal/cache/global_test.gointernal/config/config.gointernal/config/config_test.gointernal/diff/diff.gointernal/diff/diff_test.gointernal/fileid/fileid.gointernal/fileid/fileid_test.gointernal/node/bun.gointernal/node/deno.gointernal/node/inventory.gointernal/node/inventory_test.gointernal/node/jsanalysis/jsanalysis.gointernal/node/jsanalysis/jsanalysis_test.gointernal/node/manifest.gointernal/node/npm.gointernal/node/owner.gointernal/node/pnpm.gointernal/node/purl.gointernal/node/purl_test.gointernal/node/types.gointernal/node/yaml.gointernal/node/yarn.gointernal/report/cache.gointernal/report/diff.gointernal/report/diff_test.gointernal/report/rules.gointernal/report/scan.gointernal/report/scan_test.gointernal/rules/defaults/allowlist-template.jsoninternal/rules/defaults/blocklist-template.jsoninternal/rules/defaults/builtin-rules.jsoninternal/rules/evaluate.gointernal/rules/load.gointernal/rules/match.gointernal/rules/rules_test.gointernal/rules/semver.gointernal/rules/test.gointernal/rules/types.gointernal/scan/scan.gointernal/scan/scan_test.gointernal/scan/types.gointernal/threat/npm.gointernal/threat/osv.gointernal/threat/threat.gointernal/threat/threat_test.gotestdata/node/bun/package.jsontestdata/node/deno/deno.jsontestdata/node/npm/.malox/indexes/files.jsonltestdata/node/npm/.malox/latest.jsontestdata/node/npm/.malox/scans/2026-06-17T12-44-05.188532000Z.jsontestdata/node/npm/package.jsontestdata/node/pnpm/package.jsontestdata/node/yarn/package.json
Comment on lines
+5
to
+7
| branches: | ||
| - main | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add test execution before releasing.
The workflow builds and publishes releases but does not run tests. As per coding guidelines, go test should run before release to ensure code quality.
🧪 Suggested test step
Add a test step after "Set up Go" and before "Select next version":
- name: Set up Go
uses: actions/setup-go@v6
with:
go-version-file: go.mod
cache: true
+ - name: Run tests
+ run: go test -v ./...
+
- name: Select next version
id: version🤖 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 @.github/workflows/release.yml around lines 5 - 7, The release workflow is
missing test execution before publishing releases. Add a new step in the
.github/workflows/release.yml workflow that runs `go test` to validate code
quality. Position this test step after the "Set up Go" step and before the
"Select next version" step to ensure tests pass before any release builds or
publications occur.
Source: Coding guidelines
Comment on lines
+5
to
+7
| before: | ||
| hooks: | ||
| - go mod tidy |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Consider adding test execution to before hooks.
The before hooks run go mod tidy but do not execute tests. As per coding guidelines, tests should run before building releases.
🧪 Suggested test hook
before:
hooks:
- go mod tidy
+ - go test ./...🤖 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 @.goreleaser.yml around lines 5 - 7, The before hooks section in
.goreleaser.yml currently only contains the `go mod tidy` hook but does not
execute tests as required by the coding guidelines. Add a test execution hook
(such as `go test ./...`) as an additional entry in the before hooks list under
the hooks key to ensure tests are run before the release build is created.
Source: Coding guidelines
Comment on lines
+47
to
+63
| brews: | ||
| - name: malox | ||
| ids: | ||
| - malox | ||
| repository: | ||
| owner: darckorp | ||
| name: homebrew-tap | ||
| branch: main | ||
| token: "{{ .Env.HOMEBREW_TAP_GITHUB_TOKEN }}" | ||
| directory: Formula | ||
| homepage: "https://github.com/darckorp/malox" | ||
| description: "Fast cross-platform terminal security scanner for open source projects" | ||
| license: "MIT" | ||
| install: | | ||
| bin.install "malox" | ||
| test: | | ||
| system "#{bin}/malox", "--version" |
There was a problem hiding this comment.
Critical: Repository owner and homepage mismatch.
The Homebrew configuration uses owner: darckorp and homepage: "https://github.com/darckorp/malox", but all documentation consistently references Kawixh:
- Line 19 in homebrew-distribution.md:
Kawixh/homebrew-tap - Line 86 in homebrew-distribution.md:
Kawixh/malox - Line 11 in milestone doc:
Kawixh/homebrew-tap
This mismatch will cause the formula to be published to the wrong repository and the homepage link to be incorrect.
🔧 Proposed fix
brews:
- name: malox
ids:
- malox
repository:
- owner: darckorp
+ owner: Kawixh
name: homebrew-tap
branch: main
token: "{{ .Env.HOMEBREW_TAP_GITHUB_TOKEN }}"
directory: Formula
- homepage: "https://github.com/darckorp/malox"
+ homepage: "https://github.com/Kawixh/malox"
description: "Fast cross-platform terminal security scanner for open source projects"
license: "MIT"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| brews: | |
| - name: malox | |
| ids: | |
| - malox | |
| repository: | |
| owner: darckorp | |
| name: homebrew-tap | |
| branch: main | |
| token: "{{ .Env.HOMEBREW_TAP_GITHUB_TOKEN }}" | |
| directory: Formula | |
| homepage: "https://github.com/darckorp/malox" | |
| description: "Fast cross-platform terminal security scanner for open source projects" | |
| license: "MIT" | |
| install: | | |
| bin.install "malox" | |
| test: | | |
| system "#{bin}/malox", "--version" | |
| brews: | |
| - name: malox | |
| ids: | |
| - malox | |
| repository: | |
| owner: Kawixh | |
| name: homebrew-tap | |
| branch: main | |
| token: "{{ .Env.HOMEBREW_TAP_GITHUB_TOKEN }}" | |
| directory: Formula | |
| homepage: "https://github.com/Kawixh/malox" | |
| description: "Fast cross-platform terminal security scanner for open source projects" | |
| license: "MIT" | |
| install: | | |
| bin.install "malox" | |
| test: | | |
| system "#{bin}/malox", "--version" |
🤖 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 @.goreleaser.yml around lines 47 - 63, The brews configuration in the
goreleaser.yml file has inconsistent repository ownership that conflicts with
documentation references. Change the owner field in the brews section from
darckorp to Kawixh, and update the corresponding homepage URL from
https://github.com/darckorp/malox to https://github.com/Kawixh/malox to ensure
the Homebrew formula is published to the correct repository and displays the
correct project homepage.
Comment on lines
+97
to
+103
| verified, err := s.LoadSnapshot(ctx, snapshot.ScanID) | ||
| if err != nil { | ||
| return fmt.Errorf("verify scan snapshot: %w", err) | ||
| } | ||
| if verified.ScanID != snapshot.ScanID { | ||
| return fmt.Errorf("verify scan snapshot: got scan id %q, want %q", verified.ScanID, snapshot.ScanID) | ||
| } |
There was a problem hiding this comment.
Verification only checks ScanID match, not full snapshot integrity.
The write verification reloads the snapshot and confirms only that ScanID matches. If JSON marshaling/unmarshaling introduced silent corruption in other fields (e.g., truncated timestamps, dropped nested structures), this check would not detect it.
🛡️ Proposed enhancement to strengthen verification
Consider adding a checksum or byte-level comparison:
verified, err := s.LoadSnapshot(ctx, snapshot.ScanID)
if err != nil {
return fmt.Errorf("verify scan snapshot: %w", err)
}
- if verified.ScanID != snapshot.ScanID {
- return fmt.Errorf("verify scan snapshot: got scan id %q, want %q", verified.ScanID, snapshot.ScanID)
+ // Verify by comparing the serialized form to catch any marshaling issues
+ verifiedData, err := marshalSnapshot(verified)
+ if err != nil {
+ return fmt.Errorf("verify scan snapshot: %w", err)
+ }
+ if !bytes.Equal(data, verifiedData) {
+ return fmt.Errorf("verify scan snapshot: marshaled data mismatch")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| verified, err := s.LoadSnapshot(ctx, snapshot.ScanID) | |
| if err != nil { | |
| return fmt.Errorf("verify scan snapshot: %w", err) | |
| } | |
| if verified.ScanID != snapshot.ScanID { | |
| return fmt.Errorf("verify scan snapshot: got scan id %q, want %q", verified.ScanID, snapshot.ScanID) | |
| } | |
| verified, err := s.LoadSnapshot(ctx, snapshot.ScanID) | |
| if err != nil { | |
| return fmt.Errorf("verify scan snapshot: %w", err) | |
| } | |
| // Verify by comparing the serialized form to catch any marshaling issues | |
| verifiedData, err := marshalSnapshot(verified) | |
| if err != nil { | |
| return fmt.Errorf("verify scan snapshot: %w", err) | |
| } | |
| if !bytes.Equal(data, verifiedData) { | |
| return fmt.Errorf("verify scan snapshot: marshaled data mismatch") | |
| } |
🤖 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 `@internal/cache/cache.go` around lines 97 - 103, The snapshot verification in
the LoadSnapshot method only checks that the ScanID matches after reloading, but
does not verify the integrity of the entire snapshot structure. To strengthen
the verification and detect potential corruption from JSON
marshaling/unmarshaling issues, add a comprehensive comparison beyond the ScanID
check. This could include comparing the full snapshot object byte-level (by
re-marshaling to JSON and comparing) or computing a checksum of the entire
snapshot before writing and comparing it after loading. Replace or supplement
the current ScanID-only comparison with a full snapshot integrity check to catch
silent corruption in other fields like timestamps or nested structures.
| } | ||
| out := make([]scan.ThreatSourceStatus, 0, len(d.ThreatSources)) | ||
| for _, source := range d.ThreatSources { | ||
| fetchedAt, _ := parseTime(source.FetchedAt) |
There was a problem hiding this comment.
Silent error suppression when parsing FetchedAt timestamp.
Line 619 uses fetchedAt, _ := parseTime(source.FetchedAt) which silently ignores parse errors. If source.FetchedAt contains malformed timestamp data, this will produce a zero time without any warning or validation failure.
🛡️ Proposed fix to log or handle parse errors
for _, source := range d.ThreatSources {
- fetchedAt, _ := parseTime(source.FetchedAt)
+ fetchedAt, err := parseTime(source.FetchedAt)
+ if err != nil && source.FetchedAt != "" {
+ // Log warning or return error depending on policy
+ return scan.ThreatSourceStatus{}, err
+ }
out = append(out, scan.ThreatSourceStatus{🤖 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 `@internal/cache/cache.go` at line 619, The parseTime function call on the
fetchedAt assignment is silently discarding the error return value using the
blank identifier, which means malformed timestamp data in source.FetchedAt will
result in a zero time value with no indication of failure. Modify the code to
capture both return values from parseTime, check if an error was returned, and
either log the error, return an error from the containing function, or otherwise
handle the parse failure appropriately to prevent silent data corruption.
Comment on lines
+916
to
+923
| if i+1 <= len(source) { | ||
| if decoded, ok := parseHexRunes(source, i, 2); ok { | ||
| value.WriteRune(decoded) | ||
| raw.WriteString(string(source[i : i+2])) | ||
| i += 2 | ||
| column += 2 | ||
| } | ||
| } |
There was a problem hiding this comment.
Off-by-one error in hex escape validation.
Line 916 checks i+1 <= len(source) before reading 2 hex digits at positions i and i+1, but this allows reading source[i+1] when i+1 == len(source), which is out of bounds. The check should be i+2 <= len(source).
🐛 Proposed fix
case 'x':
- if i+1 <= len(source) {
+ if i+2 <= len(source) {
if decoded, ok := parseHexRunes(source, i, 2); ok {🤖 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 `@internal/node/jsanalysis/jsanalysis.go` around lines 916 - 923, The boundary
check before reading 2 hex digits in the hex escape handling logic contains an
off-by-one error. The condition `i+1 <= len(source)` only validates that
position `i+1` is within bounds, but reading at positions `i` and `i+1` requires
ensuring both indices are valid. Change the condition from `i+1 <= len(source)`
to `i+2 <= len(source)` to properly validate that there are 2 characters
available to read starting from position `i`.
Comment on lines
+335
to
+342
| func do(ctx context.Context, opts Options, req *http.Request) ([]byte, http.Header, error) { | ||
| timeout := opts.Timeout | ||
| if timeout <= 0 { | ||
| timeout = defaultTimeout | ||
| } | ||
| ctx, cancel := context.WithTimeout(req.Context(), timeout) | ||
| defer cancel() | ||
| req = req.WithContext(ctx) |
There was a problem hiding this comment.
Context parameter shadowed before use.
The ctx parameter at line 335 is never used before being overwritten at line 340. The function creates a new timeout context from req.Context() instead. This triggers staticcheck SA4009 (argument overwritten before first use).
🔧 Proposed fix to use the passed context parameter
func do(ctx context.Context, opts Options, req *http.Request) ([]byte, http.Header, error) {
timeout := opts.Timeout
if timeout <= 0 {
timeout = defaultTimeout
}
- ctx, cancel := context.WithTimeout(req.Context(), timeout)
+ timeoutCtx, cancel := context.WithTimeout(ctx, timeout)
defer cancel()
- req = req.WithContext(ctx)
+ req = req.WithContext(timeoutCtx)
var lastErr errorThis ensures the passed ctx parameter is respected as the parent context for the timeout.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func do(ctx context.Context, opts Options, req *http.Request) ([]byte, http.Header, error) { | |
| timeout := opts.Timeout | |
| if timeout <= 0 { | |
| timeout = defaultTimeout | |
| } | |
| ctx, cancel := context.WithTimeout(req.Context(), timeout) | |
| defer cancel() | |
| req = req.WithContext(ctx) | |
| func do(ctx context.Context, opts Options, req *http.Request) ([]byte, http.Header, error) { | |
| timeout := opts.Timeout | |
| if timeout <= 0 { | |
| timeout = defaultTimeout | |
| } | |
| timeoutCtx, cancel := context.WithTimeout(ctx, timeout) | |
| defer cancel() | |
| req = req.WithContext(timeoutCtx) | |
| var lastErr error |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 335-335: SA4009: argument ctx is overwritten before first use
(staticcheck)
[error] 340-340: SA4009(related information): assignment to ctx
(staticcheck)
🤖 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 `@internal/threat/threat.go` around lines 335 - 342, The `ctx` parameter in the
`do` function is shadowed before being used, as a new timeout context is created
from `req.Context()` instead of the passed `ctx`. Fix this by modifying the
context.WithTimeout call to use the passed `ctx` parameter as the parent context
instead of `req.Context()`. This ensures the passed context parameter is
properly respected as the base context for creating the timeout context, which
may carry important deadlines or values from the caller.
Source: Linters/SAST tools
Store project snapshots as compact gzip files and resolve the latest scan through a small pointer document. Preserve legacy JSON reads, remove the redundant file index, and cap retained history with a configurable limit.
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
1 participant
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 by CodeRabbit
Release Notes
New Features
Documentation