fix: comprehensive code-audit fixes (perf, concurrency, error-handling, security, tests, CI)#90
Merged
Merged
Conversation
… CI (wave 1) From a full-codebase multi-perspective audit. Wave 1 = the safe, high-value, mechanical fixes (concurrency refactors + large refactors deferred). Performance: - store: add idx_earnings_platform_created on earnings(platform, created_at) — the fastest-growing table had zero indexes; ListDailyBalances/ListLatestEarnings/ PurgeOldData full-scanned it under a single SQLite connection. - app: GetAppState fetches ListLatestEarnings/ListDeployments once and threads them into computeEarningsSummary/notifications (was 3x/2x per call). Error handling: - store: RecordEvent no longer swallows its INSERT error (it sinks all health events; a silent failure masked crash-loops) — logs on failure. - native: updatePID logs a registry-persist failure, like persistRunning. - frontend: renderSettings/renderFleet/removeFleetDevice/hydrateWizardForm and clipboard copy now try/catch their awaited bound calls (were silent dead tabs / empty forms / unhandled rejections). Consistency / correctness: - runtime: unify the two divergent memory parsers into one canonical parseMemoryBytes (fractional + overflow-guarded); the native path now shares the Docker grammar, removing the fail-open-on-fractional divergence. Deletes the duplicate parseMemBytes + its test. - frontend: escapeHtml all slug interpolations consistently. Test: - fix flaky TestNativeSupervisorStopsAfterMaxRestarts (5s deadline -> the repo's waitFor 15s poll; ~4s consumed by real fork/exec under -race intermittently reddened CI). CI / security: - pin go-version 1.26.5 in both workflows (closes crypto/tls GO-2026-5856, reachable via Docker Logs). - desktop-release: add persist-credentials:false; fix the Windows-signing `if` (referenced step-level env, always false, so signing never ran). Verified: go build ./... + GOOS=windows build + go vet ./... + go test -race ./... + frontend tsc && vite build all green.
📝 WalkthroughWalkthroughThe PR pins Go tooling in CI and desktop releases, reuses fetched application data, hardens frontend actions, improves runtime memory parsing and persistence logging, updates supervisor timing tests, and adds store logging and an earnings index. ChangesDelivery workflow updates
Application state and frontend resilience
Runtime and store behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #90 +/- ##
==========================================
- Coverage 74.74% 74.70% -0.05%
==========================================
Files 15 14 -1
Lines 3271 3261 -10
==========================================
- Hits 2445 2436 -9
+ Misses 652 651 -1
Partials 174 174
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
metrics.go (1)
73-78: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDuplicate
ListLatestEarningsfetch in the metrics path.
renderMetricsfetches earnings at line 75 forcomputeEarningsSummary, thenrenderStoreMetrics(line 101) fetches the same data again. The PR objective is to avoid repeated fetches — thread the earnings slice intorenderStoreMetricsto eliminate the second query.♻️ Proposed refactor
func (a *App) renderMetrics() string { mw := &metricsWriter{} // ... var summary EarningsSummary if a.store != nil { - summary = a.computeEarningsSummary(a.store.ListLatestEarnings()) + earnings := a.store.ListLatestEarnings() + summary = a.computeEarningsSummary(earnings) + a.renderStoreMetrics(mw, earnings) } else { summary = a.computeEarningsSummary(nil) } // ... - if a.store != nil { - a.renderStoreMetrics(mw) - } return mw.String() } -func (a *App) renderStoreMetrics(mw *metricsWriter) { +func (a *App) renderStoreMetrics(mw *metricsWriter, latest []store.EarningsRecord) { // -- Per-platform latest balance / error -- - latest := a.store.ListLatestEarnings() mw.family("cashpilot_service_balance", ...)🤖 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 `@metrics.go` around lines 73 - 78, renderMetrics currently fetches earnings separately from renderStoreMetrics, causing duplicate ListLatestEarnings queries. Store the fetched earnings slice once in renderMetrics, pass it to computeEarningsSummary and renderStoreMetrics, and update renderStoreMetrics’ signature and callers to use the provided slice instead of querying a.store again.frontend/src/main.ts (1)
1317-1317: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueRaw slug in CSS attribute selector after escaping in HTML.
The
data-form-slugattribute is set withescapeHtml(service.slug)(line 1242), buthydrateWizardFormuses the rawservice.slugin the CSS selector string. For normal slugs this works because the DOM attribute value equals the raw slug after HTML parsing. If a slug ever contained a"character, the selector would break. Slugs are controlled identifiers so this is theoretical, but worth noting for consistency.🤖 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 `@frontend/src/main.ts` at line 1317, Update hydrateWizardForm to safely handle service.slug when constructing the data-form-slug CSS selector, using appropriate CSS attribute-value escaping or a DOM lookup that avoids interpolating the raw slug; keep it consistent with the escaped value assigned by escapeHtml when rendering the attribute..github/workflows/desktop-release.yml (1)
29-29: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider cache-poisoning risk on
setup-gocache.zizmor flags
setup-gowithcache: trueas potentially vulnerable to cache poisoning in public repos. If this repository is public, a attacker could submit a PR that poisons the Go module cache. Evaluate whethercache: trueis acceptable given the repo's visibility, or considercache: falsewith explicitgo mod downloadif this is a concern.🤖 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/desktop-release.yml at line 29, Review the setup-go configuration in the desktop release workflow and assess whether its enabled module caching is safe for this repository’s visibility and PR model. If cache poisoning is a concern, disable setup-go caching and add an explicit go mod download step; otherwise document or retain the justified configuration.Source: Linters/SAST tools
🤖 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/ci.yml:
- Line 25: Update the Go version configuration used by the CI workflow and its
corresponding release configuration to use the documented auto-patching selector
“1.26.x” instead of the exact “1.26.5” pin, ensuring the values remain
consistent with docs/SECURITY-NOTES.md.
In @.github/workflows/desktop-release.yml:
- Line 63: Replace the secrets.WINDOWS_SIGN_CERT reference in the step’s if
condition with the corresponding env.WINDOWS_SIGN_CERT variable already defined
for the step, while preserving the Windows runner check.
In `@internal/runtime/runtime.go`:
- Around line 685-692: Update the overflow check in the unit-parsing conversion
logic to use a greater-than-or-equal comparison against math.MaxInt64, ensuring
the exact 2^63 boundary is rejected before int64 conversion. Locate the bytes
calculation and guard in the relevant parsing function and change the condition
from > to >=.
---
Nitpick comments:
In @.github/workflows/desktop-release.yml:
- Line 29: Review the setup-go configuration in the desktop release workflow and
assess whether its enabled module caching is safe for this repository’s
visibility and PR model. If cache poisoning is a concern, disable setup-go
caching and add an explicit go mod download step; otherwise document or retain
the justified configuration.
In `@frontend/src/main.ts`:
- Line 1317: Update hydrateWizardForm to safely handle service.slug when
constructing the data-form-slug CSS selector, using appropriate CSS
attribute-value escaping or a DOM lookup that avoids interpolating the raw slug;
keep it consistent with the escaped value assigned by escapeHtml when rendering
the attribute.
In `@metrics.go`:
- Around line 73-78: renderMetrics currently fetches earnings separately from
renderStoreMetrics, causing duplicate ListLatestEarnings queries. Store the
fetched earnings slice once in renderMetrics, pass it to computeEarningsSummary
and renderStoreMetrics, and update renderStoreMetrics’ signature and callers to
use the provided slice instead of querying a.store again.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d16c4f6e-8f55-4e1d-9c3e-132bc7753784
📒 Files selected for processing (15)
.github/workflows/ci.yml.github/workflows/desktop-release.ymlapp.goapp_test.gofrontend/src/main.tsinternal/runtime/native.gointernal/runtime/native_test.gointernal/runtime/resource_limits.gointernal/runtime/resource_limits_linux.gointernal/runtime/resource_limits_test.gointernal/runtime/resource_limits_windows.gointernal/runtime/runtime.gointernal/runtime/runtime_test.gointernal/store/store.gometrics.go
💤 Files with no reviewable changes (2)
- internal/runtime/resource_limits_test.go
- internal/runtime/resource_limits.go
| - uses: actions/setup-go@v6 | ||
| with: | ||
| go-version: '1.26' | ||
| go-version: '1.26.5' |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚖️ Poor tradeoff
Pinning to exact 1.26.5 contradicts the documented auto-patch strategy.
docs/SECURITY-NOTES.md:10-14 states CI/release builds use go-version: '1.26' / '1.26.x' to automatically pull each Go security patch. Pinning to the exact 1.26.5 means future patches (e.g., 1.26.6) won't be picked up without a manual workflow update, degrading the security posture the docs describe. Either update the docs to reflect the intentional pinning strategy, or use '1.26.x' to retain auto-patching.
Also applies to: 56-56
🤖 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/ci.yml at line 25, Update the Go version configuration
used by the CI workflow and its corresponding release configuration to use the
documented auto-patching selector “1.26.x” instead of the exact “1.26.5” pin,
ensuring the values remain consistent with docs/SECURITY-NOTES.md.
|
|
||
| - name: Sign Windows binaries | ||
| if: runner.os == 'Windows' && env.WINDOWS_SIGN_CERT != '' | ||
| if: runner.os == 'Windows' && secrets.WINDOWS_SIGN_CERT != '' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
secrets context is not allowed in step if conditionals — this will fail.
actionlint confirms: the secrets context is unavailable in if expressions. GitHub Actions only allows env, github, inputs, job, matrix, needs, runner, steps, strategy, and vars in conditionals. The step already maps WINDOWS_SIGN_CERT into env at line 65 — reference that instead.
🔧 Fix: use env instead of secrets in the if condition
- name: Sign Windows binaries
- if: runner.os == 'Windows' && secrets.WINDOWS_SIGN_CERT != ''
+ if: runner.os == 'Windows' && env.WINDOWS_SIGN_CERT != ''
env:
WINDOWS_SIGN_CERT: ${{ secrets.WINDOWS_SIGN_CERT }}
WINDOWS_SIGN_CERT_PASSWORD: ${{ secrets.WINDOWS_SIGN_CERT_PASSWORD }}📝 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
| if: runner.os == 'Windows' && secrets.WINDOWS_SIGN_CERT != '' | |
| - name: Sign Windows binaries | |
| if: runner.os == 'Windows' && env.WINDOWS_SIGN_CERT != '' | |
| env: | |
| WINDOWS_SIGN_CERT: ${{ secrets.WINDOWS_SIGN_CERT }} | |
| WINDOWS_SIGN_CERT_PASSWORD: ${{ secrets.WINDOWS_SIGN_CERT_PASSWORD }} |
🧰 Tools
🪛 actionlint (1.7.12)
[error] 63-63: context "secrets" is not allowed here. available contexts are "env", "github", "inputs", "job", "matrix", "needs", "runner", "steps", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🤖 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/desktop-release.yml at line 63, Replace the
secrets.WINDOWS_SIGN_CERT reference in the step’s if condition with the
corresponding env.WINDOWS_SIGN_CERT variable already defined for the step, while
preserving the Windows runner check.
Source: Linters/SAST tools
Comment on lines
+685
to
+692
| // Guard against overflowing a 64-bit byte count before the float->int64 | ||
| // conversion, which would otherwise silently produce an unspecified/wrapped | ||
| // result for very large mantissas or unit multipliers (e.g. "1e300t"). | ||
| bytes := value * float64(multiplier) | ||
| if bytes > math.MaxInt64 { | ||
| return 0, fmt.Errorf("%q overflows a 64-bit byte count", s) | ||
| } | ||
| return int64(bytes), nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that float64(math.MaxInt64) == 2^63 and int64(2^63) overflows.
cat <<'EOF' | go run -
package main
import (
"fmt"
"math"
)
func main() {
f := float64(math.MaxInt64)
fmt.Printf("float64(MaxInt64) = %v\n", f)
fmt.Printf("float64(MaxInt64) == 2^63: %v\n", f == float64(1<<63))
i := int64(f)
fmt.Printf("int64(float64(MaxInt64)) = %d (MinInt64=%d, overflow=%v)\n", i, math.MinInt64, i == math.MinInt64)
// Simulate "8388608t"
bytes := 8388608.0 * float64(int64(1)<<40)
fmt.Printf("8388608t bytes = %v\n", bytes)
fmt.Printf("bytes > math.MaxInt64: %v\n", bytes > math.MaxInt64)
fmt.Printf("bytes >= math.MaxInt64: %v\n", bytes >= math.MaxInt64)
fmt.Printf("int64(bytes) = %d\n", int64(bytes))
}
EOFRepository: GeiserX/CashPilot-Desktop
Length of output: 186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== source =="
sed -n '675,700p' internal/runtime/runtime.go
echo
echo "== go verifier =="
tmpdir="$(mktemp -d)"
cat >"$tmpdir/main.go" <<'EOF'
package main
import (
"fmt"
"math"
)
func main() {
f := float64(math.MaxInt64)
fmt.Printf("float64(MaxInt64)=%.0f\n", f)
fmt.Printf("float64(MaxInt64)==2^63:%v\n", f == float64(uint64(1)<<63))
fmt.Printf("float64(MaxInt64)>MaxInt64:%v\n", f > math.MaxInt64)
fmt.Printf("float64(MaxInt64)>=MaxInt64:%v\n", f >= math.MaxInt64)
fmt.Printf("int64(float64(MaxInt64))=%d\n", int64(f))
bytes := 8388608.0 * float64(int64(1)<<40)
fmt.Printf("8388608t bytes=%.0f\n", bytes)
fmt.Printf("bytes>MaxInt64:%v\n", bytes > math.MaxInt64)
fmt.Printf("bytes>=MaxInt64:%v\n", bytes >= math.MaxInt64)
fmt.Printf("int64(bytes)=%d\n", int64(bytes))
}
EOF
go run "$tmpdir/main.go"
rm -rf "$tmpdir"Repository: GeiserX/CashPilot-Desktop
Length of output: 1406
Handle the exact 2^63 boundary too. bytes > math.MaxInt64 still lets 8388608t through because math.MaxInt64 rounds to 2^63 as a float64; use >= so the boundary case is rejected.
🤖 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/runtime/runtime.go` around lines 685 - 692, Update the overflow
check in the unit-parsing conversion logic to use a greater-than-or-equal
comparison against math.MaxInt64, ensuring the exact 2^63 boundary is rejected
before int64 conversion. Locate the bytes calculation and guard in the relevant
parsing function and change the condition from > to >=.
…ts facade drift (wave 2) - runtime: NativeProcessProvider.List samples each alive process's CPU/mem concurrently (goroutine per entry, own preallocated slice index, 8-slot semaphore, WaitGroup join) instead of sequentially at ~200ms each — mirrors DockerProvider.statsForContainers. Sort order + entryIsOurs liveness gate preserved. - app: collectAll's per-service collect loop is now a bounded worker pool (collectConcurrency=6) — was strictly sequential. Single-flight guard, per-service emitError (never aborts batch), post-loop ordering (wg.Wait before sampleHealth/purge), and ctx-cancel-without-goroutine-leak all preserved. Store writes serialize on the single SQLite connection. - app_test: TestCollectAllSingleFlight rewritten — asserted max-concurrent-collects==1, which measured the old SEQUENTIAL collect, not single-flight. Now asserts the real property: of 8 concurrent collectAll calls only one body runs (3 collects total, not 8x3) and the run's parallelism stays within collectConcurrency. - frontend: add the 3 missing bindings (StartService — called by main.ts — plus ListServices, GetService) to the hand-maintained wails.d.ts, which had drifted from Go. Verified: build + GOOS=windows build + vet + go test -race ./... (root + runtime, no data race) + frontend tsc && vite build.
…sibling
- collectors: standardize the 6 lowercase error-string outliers to the user-facing
Product-Capitalized house style (these surface in earnings.error + toasts).
- runtime: substitute is now a single-pass regexp replacer (compiled once at package
scope) instead of accumulating strings.ReplaceAll over random map order — a value
containing a literal ${OTHER} is no longer re-expanded. Known/unknown/non-var
semantics preserved.
- security: extend the Fleet Bind Address help to warn that a non-loopback bind also
serves the UNAUTHENTICATED /metrics endpoint (earnings/health) to the LAN.
- test: add resource_limits_windows_test.go (//go:build windows) — the Windows Job
Object path had zero coverage; runs on windows-latest at release. Invalid-pid error
path + nil/no-Process guard, no risk to the test process.
- test: fix the sibling flaky test TestNativeSupervisorRecordsCrashAndRestartEvents
(same 5s deadline the wave-1 fix left untouched) -> waitFor(15s) poll.
Verified: build + GOOS=windows build/vet (compiles the windows test) + go vet ./... +
go test -race ./internal/collectors/ ./internal/runtime/ . all green.
GeiserX
changed the title
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.
Fixes from a full-codebase multi-perspective audit (
/review-code!— 8 auditors). Delivered in 3 verified waves on this branch. Overall health went from a first-pass 7/10 toward closing every HIGH-severity actionable finding; the only items left are genuine architecture decisions / a preventive security refactor that warrant their own PRs (listed at the bottom).Wave 1 — safe, high-value, mechanical
idx_earnings_platform_created(the fastest-growing table had zero indexes; every scan blocked the single SQLite conn).GetAppStatefetches earnings/deployments once (was 3×/2×).MemLimitwhile Docker failed closed.RecordEvent/updatePIDlog their swallowed errors; 5 frontendtry/catchgaps (silent dead tabs / empty forms / unhandled rejections) + consistent slugescapeHtml.TestNativeSupervisorStopsAfterMaxRestarts(actively reddening CI — failed live during the audit).go-version 1.26.5(crypto/tls advisory);persist-credentials:false; fix the Windows-signingif:that never fired.Wave 2 — concurrency perf (HIGH)
Listnow samples native-process stats concurrently (goroutine per entry, 8-slot semaphore, own slice index) — was O(N×200ms) blocking on every dashboard click; mirrors the Docker path.collectAllis now a bounded worker pool (6) — was serial (a few slow providers stretched a cycle to minutes). Single-flight, per-service error isolation, post-loop ordering, ctx-cancel-safety all preserved.StartService— called by main.ts —ListServices,GetService) to the hand-maintainedwails.d.tsthat had drifted from Go.Wave 3 — nits, security warning, test gaps
substituteis now a single-pass regexp (no re-expansion of${...}inside a value); Fleet-Bind help now warns about the unauthenticated/metricsLAN exposure.resource_limits_windows_test.go(//go:build windows, runs on windows-latest at release) for the previously-untested Job-Object path; fixed the sibling flaky test.Verification (all local, macOS)
go build ./...+GOOS=windows go build/vet(compiles the Windows test) +go vet ./...+go test -race ./...(all 10 packages) + frontendtsc && vite build— all green.Deferred — with rationale, not dropped (need their own PR / your call)
state.jsonstores service creds in plaintext — defeats the AES-GCM at-rest design vs backups/sync. Fix crosses the deliberateruntime↔storedecoupling (inject a sealer / re-resolve from the encrypted store on relaunch) and touches the core relaunch flow, so it deserves a dedicated, well-tested PR (no credential-bearing native service ships today, so it is preventive).Provider/ContainerInfoto a runtime-neutral shape (churns thedeploymentsschema + frontend); collector self-registration (retire the central dispatch map); extractcomputeEarningsSummary→internal/summary(now param-testable after wave 1, so lower ROI). These change public contracts/schema and are best decided explicitly.