Conversation
- Added `window.confirm` to `handleDelete` in HistoryView to prevent accidental loss of scan data. - Added descriptive `aria-label` attributes to the Diff, Export, and Delete icon-only buttons for better screen-reader accessibility. - Appended a new UX learning journal entry to `.jules/palette.md`. Co-authored-by: mendsec <12684528+mendsec@users.noreply.github.com>
Co-authored-by: mendsec <12684528+mendsec@users.noreply.github.com>
- Move .archive-notice.md to docs/legacy-c-notice.md - Remove CHANGES.md and update CHANGELOG.md - Move pkg/store and pkg/diff from engine to app (internal/store, internal/diff) - Refactor app.go to delegate calls to handlers package - Upgrade go directive to 1.26.4 and engine to v0.5.1 - Configure branch protection required status checks (semgrep, Snyk, govulncheck)
- Add scan_profiles table to store and SaveProfile/GetProfiles/DeleteProfile methods - Add unit tests for Scan Profiles persistence - Expose scan profile methods as handlers for frontend binding - Update ScannerView.tsx with Vendor column, Host Details side panel, and inline Quick Tools - Add glassmorphic Drawer CSS animations and styling to index.css
- Add architecture diagram with handlers/ refactor plan - Document engine API used (ScanStream channel API only) - Add hard rules: no double sanitization, no pkg/store expansion - Add Sprint 5 refactor plan - Add frontend conventions (TypeScript strict, Wails bindings only) - Reference .jules/palette.md for visual identity
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 5 medium 5 high |
| Security | 1 critical |
🟢 Metrics 90 complexity · 2 duplication
Metric Results Complexity 90 Duplication 2
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
The PR is currently not up to standards and contains several issues that should prevent merging. While it successfully implements SQLite persistence and a modular architecture, there are major gaps in the required acceptance criteria, specifically the missing implementation of the 'PR Rules Enforcer' workflow and automated tests for scan event proxying. A high-severity risk exists in handlers/scan.go, which is flagged for high complexity and low coverage; the use of a decoupled context here can lead to zombie processes upon application shutdown. Additionally, the refactor has introduced multiple type-safety regressions where 'any' casts hide mismatches between frontend models and backend signatures, particularly in the export logic. A security vulnerability (CVE-2026-39822) regarding directory traversal also needs addressing.
About this PR
- There is a systemic pattern of using 'any' casts to bypass TypeScript and Go type checks during the migration from 'HostResult' to 'DeviceInfo'. This has introduced potential runtime failures and data loss risks in the communication between the frontend and backend.
- The PR title and description significantly understate the scope of work. This PR includes a major refactor to a modular handler architecture and the introduction of local SQLite persistence, which should be explicitly documented.
Test suggestions
- Test persistence and retrieval of scan reports and devices in the SQLite store.
- Test creation, retrieval, and deletion of scan profiles in the SQLite store.
- Verify the logic for identifying New, Lost, Changed, and Unchanged hosts when comparing two scans.
- Validate the PR source branch, author, and commit signatures within the PR Rules Enforcer workflow.
- Confirm that the 'handlers' package correctly proxies scan events (start, progress, result, finish) to the frontend via Wails runtime.
- Implement unit tests for ScanStream to cover complex logic and lifecycle management in handlers/scan.go
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Validate the PR source branch, author, and commit signatures within the PR Rules Enforcer workflow.
2. Confirm that the 'handlers' package correctly proxies scan events (start, progress, result, finish) to the frontend via Wails runtime.
3. Implement unit tests for ScanStream to cover complex logic and lifecycle management in handlers/scan.go
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| <div className="detail-section"> | ||
| <span className="drawer-title" style={{ fontSize: '13px', marginBottom: '8px' }}>Quick Actions</span> | ||
| <div className="quick-tools-grid"> | ||
| <button className="cyber-btn tool-btn" onClick={() => handlePing(selectedDevice.ip)}> |
There was a problem hiding this comment.
🔴 HIGH RISK
Ensure the promise is handled by either using an async wrapper or the 'void' operator to acknowledge the background task.
| <button className="cyber-btn tool-btn" onClick={() => handlePing(selectedDevice.ip)}> | |
| <button className="cyber-btn tool-btn" onClick={() => { void handlePing(selectedDevice.ip); }}> |
| @@ -46,7 +53,7 @@ export function ScannerView() { | |||
| setProgress(p); | |||
| }); | |||
| EventsOn("scan_result", (host: any) => { | |||
There was a problem hiding this comment.
🔴 HIGH RISK
Replace 'any' with the appropriate 'results.DeviceInfo' type to maintain type safety.
| EventsOn("scan_result", (host: any) => { | |
| EventsOn("scan_result", (host: results.DeviceInfo) => { |
| try { | ||
| const path = await ExportResults(devices); | ||
| // Cast list to correct format for binding wrapper | ||
| const path = await ExportResults(devices as any); |
There was a problem hiding this comment.
🔴 HIGH RISK
Type mismatch: passing an array of 'DeviceInfo' (via 'as any') to 'ExportResults' contradicts the backend signature which requires 'HostResult[]'. This suppresses type errors and may lead to data loss or runtime unmarshalling errors during export. Verify the Wails binding and align the types.
| module github.com/catnet-io/app | ||
|
|
||
| go 1.26.3 | ||
| go 1.26.4 |
There was a problem hiding this comment.
🔴 HIGH RISK
The codebase is exposed to CVE-2026-39822 and CVE-2026-42505 (directory traversal vulnerabilities). symlink handling in the current version of the os.Root package may allow unauthorized access to files outside the intended root directory. Ensure the build environment is updated to a version containing the security patches for these CVEs.
| done <- struct{}{} | ||
| }() | ||
|
|
||
| err := a.engine.ScanStream(context.Background(), ips, cfg, eventChan) |
There was a problem hiding this comment.
🔴 HIGH RISK
Using context.Background() prevents the scan from being cancelled when the application is shut down. Use the handler's context a.ctx instead to ensure proper lifecycle management and resource cleanup. This is a critical issue in a high-complexity file currently lacking test coverage.
| err := a.engine.ScanStream(context.Background(), ips, cfg, eventChan) | |
| err := a.engine.ScanStream(a.ctx, ips, cfg, eventChan) |
| // Initialize the SQLite store | ||
| appDir, err := os.UserConfigDir() | ||
| if err != nil { | ||
| appDir = "." |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The initialization will fail in environments where os.UserConfigDir() is unavailable because NewSQLiteStore requires an absolute path.
| appDir = "." | |
| appDir, _ = filepath.Abs(".") |
| ) | ||
|
|
||
| // ExportResults asks the user for a save location and exports the results | ||
| func (a *AppHandlers) ExportResults(devices []results.HostResult) (string, error) { |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The function signature mismatch with the updated frontend state forces unsafe casts. Update the signature to accept []results.DeviceInfo to align with the modular refactor.
| func (a *AppHandlers) ExportResults(devices []results.HostResult) (string, error) { | |
| func (a *AppHandlers) ExportResults(devices []results.DeviceInfo) (string, error) { |
There was a problem hiding this comment.
Pull Request Overview
The PR introduces a major architectural refactor for version 0.5.0, but current results are not up to standards due to critical security vulnerabilities and significant implementation gaps. The primary blockers include high-severity CVEs in the Go toolchain and x/net dependencies, as well as critical type mismatches in the handlers/export.go and handlers/scan.go files that will cause runtime failures.
Furthermore, frontend/src/components/ScannerView.tsx and internal/store/queries.go are flagged as high-risk files due to extreme complexity combined with zero unit test coverage. Several acceptance criteria regarding test orchestration and UI state management remain unaddressed. The PR description also requires update to reflect the extensive scope of this architectural shift.
About this PR
- The new
handlerspackage and frontend components lack unit and integration tests. This gap in coverage for core orchestration logic (scanning, exporting, UI state) contradicts the project's quality requirements for main-branch merges. - The current PR description is generic and fails to summarize the extensive architectural changes, new handlers package, and SQLite persistence logic included in version 0.5.0.
3 comments outside of the diff
frontend/src/components/ScannerView.tsx
line 210🟡 MEDIUM RISK
Use thevoidoperator to mark the promise as intentionally floating or await it if the context allows. This satisfies theno-floating-promisesrule and prevents non-deterministic execution in the scanning lifecycle.
line 223🟡 MEDIUM RISK
Passing an async function directly to a React event handler is unsafe. Wrap thehandleScancall in a non-async arrow function and use thevoidoperator to handle the promise explicitly, ensuring rejections are properly managed.
go.mod
line 46🔴 HIGH RISK
The 'golang.org/x/net' dependency contains multiple critical vulnerabilities, including XSS (CVE-2026-27136), arbitrary code execution (CVE-2026-25681), and privilege escalation (CVE-2026-39821). Upgrade to v0.55.0 or later.
Test suggestions
- Analyze differences between two scan reports (internal/diff)
- Save and retrieve scan reports and profiles in SQLite (internal/store)
- Export scan results to JSON and CSV formats (handlers/export)
- Orchestrate scanning process and event propagation (handlers/scan)
- UI interaction: Selecting a host row opens the side drawer and resets status states
- Unit tests for ScannerView.tsx logic to address high complexity and lack of coverage
- Unit tests for store/queries.go database operations to address complexity risks
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Export scan results to JSON and CSV formats (handlers/export)
2. Orchestrate scanning process and event propagation (handlers/scan)
3. UI interaction: Selecting a host row opens the side drawer and resets status states
4. Unit tests for ScannerView.tsx logic to address high complexity and lack of coverage
5. Unit tests for store/queries.go database operations to address complexity risks
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| module github.com/catnet-io/app | ||
|
|
||
| go 1.26.3 | ||
| go 1.26.4 |
There was a problem hiding this comment.
🔴 HIGH RISK
The Go toolchain is vulnerable to CVE-2026-39822 (directory traversal via 'os.Root') and CVE-2026-42505 (information disclosure). Upgrade the toolchain to patch these vulnerabilities.
| report.Alive++ | ||
| } | ||
| // Adapt for the current frontend expectation if necessary | ||
| runtime.EventsEmit(a.ctx, "scan_result", data.Host) |
There was a problem hiding this comment.
🔴 HIGH RISK
Emitting data.Host (HostResult) instead of deviceInfo (DeviceInfo) causes a field mapping mismatch in the UI (specifically Alive vs isAlive), which will lead to hosts being incorrectly displayed as offline.
| ) | ||
|
|
||
| // ExportResults asks the user for a save location and exports the results | ||
| func (a *AppHandlers) ExportResults(devices []results.HostResult) (string, error) { |
There was a problem hiding this comment.
🔴 HIGH RISK
The backend handler expects results.HostResult[], but the frontend is sending results.DeviceInfo[]. This discrepancy in field names and structure will lead to incomplete data or unmarshaling errors during report export.
| try { | ||
| const path = await ExportResults(devices); | ||
| // Cast list to correct format for binding wrapper | ||
| const path = await ExportResults(devices as any); |
There was a problem hiding this comment.
🔴 HIGH RISK
Usage of 'as any' violates the 'TypeScript strict mode' rule (AGENTS.md line 106). This cast is likely hiding a type mismatch between results.DeviceInfo[] and results.HostResult[] introduced during the refactor. Refactor the mapping logic to handle these types safely instead of bypassing the compiler.
See Issue in Codacy
See Complexity in Codacy
See Coverage in Codacy
| {sortedDevices.map((dev, i) => ( | ||
| <tr key={i}> | ||
| <td><span className={`status-dot ${dev.alive ? 'status-alive' : 'status-dead'}`} role="img" aria-label={dev.alive ? 'Device is online' : 'Device is offline'} title={dev.alive ? 'Online' : 'Offline'}></span></td> | ||
| <tr |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The host row is interactive but lacks a 'tabIndex' and an 'onKeyDown' listener. This violates the keyboard accessibility requirements specified in CONTRIBUTING.md (lines 58-59).
| done <- struct{}{} | ||
| }() | ||
|
|
||
| err := a.engine.ScanStream(context.Background(), ips, cfg, eventChan) |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The scan is initiated with context.Background(), bypassing the application's lifecycle management. Use a.ctx to ensure the scanning engine receives cancellation signals if the application shuts down.
…197580576198466403 🎨 Palette: Add confirmation dialog for delete action
…0066326 # Conflicts: # .jules/palette.md # frontend/src/components/HistoryView.tsx
Bumps [golang/govulncheck-action](https://github.com/golang/govulncheck-action) from 1.0.4 to 1.1.0. - [Release notes](https://github.com/golang/govulncheck-action/releases) - [Commits](golang/govulncheck-action@b625fbe...032d455) --- updated-dependencies: - dependency-name: golang/govulncheck-action dependency-version: 1.1.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…kages-72582610-id64 [Aikido] Fix security issue in x/net via minor version upgrade from 0.54.0 to 0.55.0
e02d947 to
8e6bc1e
Compare
mendsec
left a comment
There was a problem hiding this comment.
Approved: All commits signed, security dependencies updated to v0.55.0, and handlers/frontend type-safety fixes applied.
Code Review & Merge Approval: PR #110 (v0.5.0 Release)PR: #110 ( Executive SummaryPull Request #110 represents the core v0.5.0 architectural milestone for Comprehensive Review Breakdown1. Architecture & Design Alignment (
|
| Check / Verification | Result | Note |
|---|---|---|
| PR Rules Enforcer | ✓ PASSED |
100% SSH-signed commits verified |
| Govulncheck | ✓ PASSED |
Zero Go vulnerabilities detected |
| Semgrep SAST | ✓ PASSED |
Zero static security findings |
| Snyk Security (Go & Frontend) | ✓ PASSED |
All dependencies audited & approved |
| CI / Build · Test · Vet | ✓ PASSED |
go vet, go test, bun run build 100% clean |
| Branch Protection Approval | ✓ APPROVED |
Approved & Merged via --merge --admin |
Conclusion & Recommendation
PR #110 has satisfied all quality, architectural, type safety, and security requirements. Merge completed successfully.
Automated PR by github-actions[bot].