-
Notifications
You must be signed in to change notification settings - Fork 10
.pr_agent_accepted_suggestions
| PR 84 (2026-07-16) |
[correctness] Uninstaller list lacks icons
Uninstaller list lacks icons
The Software Uninstaller UI renders app name/version/size but never renders the `iconPath` metadata, so application icons are not displayed. This violates the requirement to display icons alongside enumerated applications where available.The uninstaller results include iconPath (parsed from DisplayIcon) but the renderer never displays it, so the UI misses the required app icons.
The compliance requirement expects icons to be shown where available for each enumerated app.
- src/ui/js/pages/tools.js[238-243]
- src/scripts/safeScripts/uninstallUtils.js[12-20]
[correctness] Leftover scan skips registry
Leftover scan skips registry
The leftover scan implementation only searches filesystem locations (InstallLocation/AppData/ProgramData) and does not scan for leftover registry keys. This does not meet the requirement to scan both registry keys and AppData folders after uninstall.The leftover scanning flow only looks for leftover folders and does not implement any scanning for leftover registry keys.
Compliance requires scanning for both leftover registry keys and AppData folders (with fuzzy matching).
- src/scripts/safeScripts/uninstallUtils.js[32-66]
- src/scripts/safeScripts/uninstallerReport.js[49-53]
[security] Protected root bypass
Protected root bypass
isProtected() only checks prefixes that include a trailing separator, so an exact protected root path (e.g. "C:\\Windows" or "/usr") does not match and can slip past the delete guard used by deleteFiles/removeLeftovers. This weakens the safety boundary for destructive filesystem operations.isProtected() builds protected prefixes with a trailing path separator and then uses startsWith(prefix) against path.resolve(targetPath). This fails to treat the exact protected root directory as protected (because resolved paths typically have no trailing separator), allowing dangerous inputs like C:\Windows (no trailing \) or /usr (no trailing /) to pass the guard.
This guard is used as the primary safety gate before deletion in multiple safeScripts.
- src/scripts/safeScripts/protectedPaths.js[5-42]
- src/scripts/safeScripts/deleteFiles.js[18-22]
- src/scripts/safeScripts/removeLeftovers.js[20-24]
- Normalize both the protected roots and the target path in a path-aware way.
- Ensure the check treats both:
- exact match of the protected root, and
- any descendant of the protected root as protected.
- Normalize target to
normalizedTarget = normalize(resolvedTarget). - For each protected root
root(without forcing a trailing separator), treat protected when:-
normalizedTarget === root, OR -
normalizedTarget.startsWith(root + sep)wheresepis\on Windows and/on POSIX.
-
[maintainability] Hardcoded uninstaller UI strings
Hardcoded uninstaller UI strings
Several newly-added renderer strings for the uninstaller flow are hardcoded (e.g., leftover section label and prompts/alerts) instead of being sourced from i18n keys. This violates the requirement to replace hardcoded UI strings with i18n keys so locales don’t fall back to raw English text.The uninstaller UI introduces new user-facing strings that are not in src/i18n/locales/en.json and are not translated via window.I18n.t(...).
Compliance expects renderer UI strings to be sourced from i18n keys, with safe fallback behavior.
- src/ui/js/pages/tools.js[224-246]
- src/ui/js/pages/tools.js[375-385]
- src/ui/js/pages/tools.js[428-446]
- src/i18n/locales/en.json[1-20]
- src/i18n/locales/*.json[1-20]
[correctness] Unknown platform treated Linux
Unknown platform treated Linux
getProvider() returns the Linux provider for any unrecognized platform string, which can yield incorrect support decisions and messaging on unsupported platforms. A base provider exists for this case but is not used.getProvider(platform) falls back to the Linux provider (linux) for unknown platform values. This misclassifies unsupported platforms and produces Linux-specific labels/messages instead of a generic unknown-platform response.
A base provider is already defined specifically for unknown/unsupported platforms.
- src/platform/index.js[17-19]
- src/platform/base.js[3-11]
- Import
baseinsrc/platform/index.js. - Change fallback to
PROVIDERS[platform] || base. - (Optional) Add/adjust a unit test to assert unknown platform uses base provider.
[reliability] Abort listener not removed
Abort listener not removed
WorkerManager.runTask() adds an AbortSignal 'abort' listener but never removes it when the task settles normally, so a long-lived/reused signal can accumulate listeners and retain task closures. This can lead to memory growth and MaxListeners warnings over time.WorkerManager.runTask() attaches an AbortSignal listener with { once: true }, but only aborting removes the listener. If the task completes (success/failure/timeout) before abort, the listener remains attached to the signal and retains closures.
This can matter when callers reuse a single AbortController.signal across multiple tasks or keep signals alive for a long time.
- src/core/workerManager.js[15-73]
- Track whether a listener was attached.
- In
finish(), ifsignalexists, callsignal.removeEventListener('abort', onAbort)(guarded) before resolving/rejecting. - Optionally, also remove it in the
signal.abortedearly-return path after callingonAbort()(defensive).
- Add a test that reuses one
AbortController.signalacross multiple successful tasks and asserts listener count does not grow (or at least doesn’t warn).
[reliability] Placeholder test missing placeholder
Placeholder test missing placeholder
The i18n test intended to validate placeholder replacement does not include any placeholder in the translation value, so it doesn’t actually test replacement behavior. This falls short of the required test coverage for placeholder replacement.The placeholder replacement test asserts a string with no {placeholder} tokens, so it never exercises the replacement code path.
Compliance requires tests for placeholder replacement behavior.
- tests/i18n.test.js[45-47]
- src/i18n/index.js[109-120]
- src/i18n/locales/en.json[1-20]
[maintainability] Settings i18n fallback ignored
Settings i18n fallback ignored
The Settings page helper t(key,fallback) returns window.I18n.t(key) unconditionally when i18n exists, so if a key is missing from both the active and English catalogs, the UI will display the raw key instead of the provided fallback. This makes future/partial translations degrade poorly compared to Tools page behavior.In Settings, t(key, fallback) does not check whether window.I18n.t(key) returned a real translation vs returning the key itself. When the catalogs lack a key, I18n.t() returns the key string, and the Settings page will render that instead of the provided English fallback.
i18nClient.t() returns the key on miss; Tools page already guards against this by checking translated !== key.
- src/ui/js/pages/settings.js[23-40]
- src/ui/js/i18nClient.js[29-33]
- src/ui/js/pages/tools.js[15-21]
- Implement the same pattern as Tools:
- call
const translated = window.I18n.t(key) - if
translated && translated !== keyreturn it - else return
fallback || key
- call
| PR 75 (2026-07-15) |
[correctness] Aurora contrast variable missing
Aurora contrast variable missing
`:root[data-theme="aurora"]` does not set `--accent-primary-contrast`, so `.btn-primary` inherits the default `#fff` text color while Aurora sets a very light `--accent-primary` (`#7dd3fc`), making primary button labels hard to read when Aurora is active.The Aurora theme defines a light --accent-primary but does not override --accent-primary-contrast. Since .btn-primary uses color: var(--accent-primary-contrast), the default #fff remains in effect, leading to poor contrast on primary buttons under Aurora.
- Base
:rootsets--accent-primary-contrast: #fff. -
.btn-primaryuses--accent-primary-contrastfor its text color. - Aurora sets
--accent-primary: #7dd3fcbut doesn’t override the contrast variable.
- src/ui/css/style.css[199-213]
Add an Aurora-specific contrast value, e.g.:
| PR 65 (2026-07-14) |
[correctness] Sticky cancel flag
Sticky cancel flag
ClamAVEngine.abortCurrentScan() sets cancelRequested=true even when no subprocess is killed, so the next clamscan/freshclam close handler can incorrectly treat a normal completion as user-canceled. This is reachable because ScanEngine.abortScan() calls abortCurrentScan() regardless of whether a scan is actually running.ClamAVEngine.abortCurrentScan() sets a global cancelRequested flag unconditionally. If abortCurrentScan() is invoked while no activeScanProcess/activeUpdateProcess exists (or kill fails), the flag remains set and is later “consumed” by whichever subprocess closes next, misreporting cancellations.
-
ScanEngine.abortScan()callsclamEngine.abortCurrentScan()even ifabortControlleris null (idle cancel). - Both
updateDefinitions()andscanFile()check the same globalcancelRequestedin their'close'handlers.
- src/security/ClamAVEngine.js[123-139]
- src/security/ClamAVEngine.js[246-288]
- src/security/ClamAVEngine.js[296-307]
- src/security/ScanEngine.js[255-262]
- Only set cancellation state when there is an active process to cancel, and clear it if nothing was canceled.
- Prefer per-process/per-operation cancellation flags (e.g.,
cancelScanRequestedandcancelUpdateRequested, or track cancellation by process identity) so scan and definition update cancellation cannot interfere with each other. - Ensure flags are reset deterministically when cancel is requested but no process exists.
[correctness] HeuristicEngine contract mismatch
HeuristicEngine contract mismatch
New tests require HeuristicEngine.analyze() to return `{ score, signals }` (and to emit entropy/PE-header signals), but the current implementation returns only `{ score: 0 }`, so the test suite (and intended feature behavior) cannot pass. This is a direct mismatch between newly-added test expectations and the shipped implementation.tests/heuristicEngine.test.js asserts that HeuristicEngine.analyze() returns { score: number, signals: [] } and produces entropy/PE-header related signals. The current src/security/HeuristicEngine.js is still a stub and does not return signals at all.
The PR adds tests that codify the expected contract and behaviors. As-is, engine.analyze() cannot satisfy these assertions.
- tests/heuristicEngine.test.js[20-54]
- src/security/HeuristicEngine.js[1-4]
- Implement
HeuristicEngine.analyze(filePath)to always return{ score, signals }. - Ensure the large-file (>50MB) path returns
{ score: 0, signals: [] }exactly. - Add the entropy scoring + PE-header mismatch detection required by the tests (or adjust tests only if the feature is intentionally out-of-scope, but then remove/replace these tests).
[correctness] ReputationEngine methods missing
ReputationEngine methods missing
New tests call `addHash/removeHash/listHashes` and expect stored verdicts, but the current ReputationEngine only implements `checkHash()` and always returns null, so tests will throw and the feature cannot work. Additionally, the database initialization does not create the required persistence table, so even a partial implementation would not be able to store entries.tests/reputationEngine.test.js expects a ReputationEngine supporting addHash, removeHash, listHashes, and checkHash backed by SQLite persistence. The current src/security/ReputationEngine.js is still a stub (only checkHash() returning null), and the DB schema initialization does not define a reputation hashes table.
The PR adds unit tests that immediately exercise the missing APIs; these will fail at runtime with engine.addHash is not a function.
- tests/reputationEngine.test.js[22-56]
- src/security/ReputationEngine.js[1-4]
- src/core/database.js[17-126]
- Implement
addHash(hash, verdict, note?),removeHash(hash),listHashes(), and updatecheckHash(hash)to query persisted rows. - Add a DB migration/init step to create the backing table (and any needed indices/constraints) without breaking existing DBs.
- Validate SHA-256 inputs and verdict enum so tests like “rejects invalid hash input” pass.
[reliability] Flaky test timing/randomness
Flaky test timing/randomness
scanCancellation.test.js relies on a fixed 20ms delay before aborting, which can race on slower CI runners and attempt to cancel before the fake engine is actually waiting. heuristicEngine.test.js also generates entropy test data via Math.random(), which is avoidably non-deterministic for a unit test.Two new tests have avoidable flakiness sources:
-
scanCancellation.test.jsuses a fixedsetTimeout(..., 20)to wait for scan startup. -
heuristicEngine.test.jsusesMath.random()to generate a fixture.
Fixed sleeps are timing-sensitive under load; randomness is non-deterministic and can mask regressions.
- tests/scanCancellation.test.js[46-57]
- tests/heuristicEngine.test.js[33-40]
- Replace the fixed 20ms sleep with a bounded wait that polls until
fakeClam.pendingResolveis set (or use an explicit synchronization primitive/event). - Replace the entropy fixture with deterministic high-entropy bytes (e.g., fill with
buf[i] = i % 256repeated) to guarantee a stable Shannon entropy outcome.
| PR 64 (2026-07-14) |
[reliability] No PDF export tests
No PDF export tests
The added test suite does not include any test that generates a PDF from an existing HTML report or verifies the returned output path for PDF export. This violates the requirement to have automated coverage for successful PDF generation and predictable missing-report behavior.Add automated tests that validate PDF export behavior: successful PDF generation from an existing HTML report, returning the output path, and clean failure when the HTML report is missing.
Current tests cover CSV serialization and only a PDF-related path validation helper; they do not exercise PDF generation (webContents.printToPDF()) or the report:exportPDF IPC handler behavior.
- tests/reportExport.test.js[1-87]
- src/security/reportExport.js[54-70]
- src/main/ipcHandlers.js[526-541]
[security] Path prefix check bypass
Path prefix check bypass
The new `shell:openPath` handler restricts files using `resolved.startsWith(root)`, which can be bypassed by sibling paths like `~/.soterios/scan-reports-evil/...` that share the same string prefix but are outside the allowed directory. The same unsafe prefix logic is also used by `isPathInScanReportsDir`, weakening the intended export/open path boundary.Directory membership checks are implemented with string prefix matching (startsWith), which is not a safe way to confirm a path is contained within an allowed directory. This permits bypass with similarly-prefixed sibling directories (e.g., scan-reports-evil).
This affects the new IPC shell:openPath and also the helper isPathInScanReportsDir used by report export handlers.
- src/main/ipcHandlers.js[656-664]
- src/security/reportExport.js[11-14]
- Replace
startsWithchecks with a path-aware containment check:
const rel = path.relative(root, resolved);- Allow only if
rel && !rel.startsWith('..' + path.sep) && rel !== '..' && !path.isAbsolute(rel)(and/orrel.split(path.sep)[0] !== '..'). - Consider allowing
resolved === rootas well.
- (Optional but stronger) Use
fs.realpathSync/fs.promises.realpathon bothrootandresolvedbefore comparison to reduce symlink-escape risk. - Centralize logic in one helper and reuse it in both IPC and export paths.
[correctness] Quarantine CSV inference wrong
Quarantine CSV inference wrong
`isThreatQuarantined()` infers per-threat quarantine state from the overall scan `report.status === 'completed'`, so any scan marked `failed`/`canceled` (due to unrelated errors) exports `quarantined=false` even when quarantine for that threat likely succeeded. This produces systematically incorrect CSV output for partially successful scans.CSV export derives quarantined from the scan-level report.status, which is not a per-threat signal. A scan can be failed/canceled for non-quarantine reasons while still successfully quarantining threats found earlier.
ScanEngine sets status based on errors.length and adds many non-quarantine errors (e.g., scan canceled, scan failed for a target). Quarantine failure errors are only a subset.
- src/security/reportExport.js[22-30]
- src/security/ScanEngine.js[170-179]
- src/security/ScanEngine.js[183-185]
- src/security/ScanEngine.js[193-205]
Option A (minimal, keeps boolean):
- Change
isThreatQuarantinedfallback to: - If there is a matching "Failed to quarantine " error =>
false - Else =>
trueThis avoids using scan-level status as a proxy. Option B (more correct, structured): - Persist an explicit
threat.quarantinedboolean into the saved report JSON when quarantine succeeds/fails, and have CSV export use that field (no inference from free-form errors).
[security] PDF export runs active HTML
PDF export runs active HTML
`generatePdfFromHtml()` loads the report HTML into a hidden BrowserWindow with only `sandbox:true`, which still allows JavaScript execution in the renderer during export. Even if current reports are generated without scripts, disabling JS and explicitly setting isolation flags would reduce risk from tampered report files and unnecessary active content.The PDF export window loads local HTML and permits renderer JavaScript by default; this is unnecessary for printing a static report and increases exposure if the report file is modified.
Main window uses explicit hardening flags (contextIsolation true, nodeIntegration false, sandbox true). The PDF window currently sets only sandbox.
- src/security/reportExport.js[54-63]
- src/main/main.js[413-418]
Update the export BrowserWindow to explicitly harden webPreferences, e.g.:
contextIsolation: truenodeIntegration: falsesandbox: true-
javascript: false(preferred if the report HTML does not require JS) Also consider denying new windows/navigation viasetWindowOpenHandlerand/orwill-navigateguards for defense-in-depth.
[reliability] Unhandled JSON.parse in getScanReport
Unhandled JSON.parse in getScanReport
`DatabaseService.getScanReport()` parses `target_paths`/`details` without guarding JSON.parse, so malformed/corrupt DB values will throw synchronously. The new export IPC handlers call `getScanReport()` outside their try/catch blocks, causing export requests to fail unexpectedly without the intended error response.getScanReport() can throw on invalid JSON in persisted columns. Export IPC handlers call it before entering their try/catch, so the error bypasses their structured { success:false, error } responses.
This is a new method introduced for export.
- src/core/database.js[179-187]
- src/main/ipcHandlers.js[526-534]
- src/main/ipcHandlers.js[543-551]
- In
getScanReport, wrap JSON.parse calls in try/catch and fall back to safe defaults ([]/{}) or return a structured error. - Additionally (or alternatively), move
db.getScanReport(...)inside the try/catch blocks in bothreport:exportPDFandreport:exportCSVhandlers to ensure consistent error handling.
| PR 63 (2026-07-14) |
[maintainability] Missing `Network Reports` tool
Missing `Network Reports` tool
The new `System Tools` wiki page lists several utilities but omits `network reports`, which is required to be documented as part of the System Tools feature set. This makes the System Tools documentation incomplete relative to the compliance checklist.The docs/wiki/System-Tools.md page does not document network reports, which is explicitly required in the System Tools documentation.
PR Compliance ID 7 requires the System Tools page to cover: disk usage, temporary file cleanup, startup items, browser cache, Windows services, network reports, and large file finder. The current table omits network reports.
- docs/wiki/System-Tools.md[7-15]
[correctness] Wiki sync steps broken
Wiki sync steps broken
The sync snippet in docs/wiki/Home.md never changes into the cloned `Soterios.wiki` directory before running `git commit`/`git push`, so the commands will run in the wrong repository (or fail). It also uses `git commit -am`, which won’t stage newly-copied pages, preventing a successful wiki sync.docs/wiki/Home.md includes a “Syncing to GitHub Wiki” bash snippet that (1) doesn’t cd into the cloned wiki repo before committing/pushing and (2) uses git commit -am, which won’t stage newly copied markdown pages.
This is a maintainer-facing workflow; following the documented steps as-is can result in commits happening in the wrong repo or no wiki pages being committed.
- docs/wiki/Home.md[26-36]
Update the snippet to something like:
[correctness] Broken internal doc link
Broken internal doc link
docs/wiki/Troubleshooting.md links to `Privacy-and-Security` (no extension), but the actual repo file added is `Privacy-and-Security.md`, so the link will be dead when browsing the version-controlled docs in-repo. This makes navigation inconsistent with the rest of the wiki source (which uses `.md` links).docs/wiki/Troubleshooting.md references [Privacy and Security](Privacy-and-Security) but the repository file name is Privacy-and-Security.md, so the repo-rendered link target does not match the file.
Most other links in docs/wiki/Home.md use .md extensions, so this appears to be an inconsistency/typo.
- docs/wiki/Troubleshooting.md[70-75]
Change the link to:
| PR 62 (2026-07-14) |
[correctness] Toasts ignore rose theme
Toasts ignore rose theme
After this PR, `Api.applyTheme()` allows and persists the `rose` theme, but desktop notification toasts resolve toast colors via `TOAST_THEMES[themeName] || TOAST_THEMES.dark`, and `TOAST_THEMES` has no `rose` entry. As a result, when the app is set to Rose, notification toasts will render using the dark toast palette instead of Rose.Desktop notification toasts use a hardcoded TOAST_THEMES map in src/main/main.js. When the user's saved theme is rose, showNotification() passes themeName='rose' into toastHtml(), which then falls back to TOAST_THEMES.dark because TOAST_THEMES.rose is missing.
This PR adds rose to the allowed theme list and Settings dropdown, so users can now persist ui.theme = 'rose' and trigger the fallback path in the toast renderer.
Add a rose entry to TOAST_THEMES (and consider adding midnight as well since it’s also selectable but not present in the toast map). Use colors aligned with the CSS theme variables.
- src/main/main.js[142-191]
- src/main/main.js[197-200]
- src/main/main.js[294-328]
| PR 61 (2026-07-14) |
[correctness] Primary button low-contrast
Primary button low-contrast
The monochrome theme sets `--accent-primary`/`--accent-primary-dark` to very light grays, but `.btn-primary` hardcodes white text, making primary button labels nearly unreadable in monochrome. This affects key actions like the Settings “Apply Theme” button.In the monochrome theme, the primary button background is driven by --accent-primary (very light gray) while .btn-primary forces color: #fff, creating insufficient contrast.
- Monochrome defines a light
--accent-primary. -
.btn-primaryusesvar(--accent-primary)for the background and hardcodes white text.
- src/ui/css/style.css[149-163]
- src/ui/css/style.css[646-656]
- Introduce a variable such as
--accent-primary-contrast(or--text-on-accent) and set it per theme. - Update
.btn-primary { color: var(--accent-primary-contrast); }. - For monochrome, set
--accent-primary-contrast: #0a0a0a(or similar dark gray) to restore readability.
[correctness] Monochrome retains colored accents
Monochrome retains colored accents
The monochrome theme defines grayscale `--accent-success/--accent-warning/--accent-danger`, but multiple UI elements use hardcoded colored backgrounds/glows (green/orange/red/blue), so monochrome will still display colored accents. This causes inconsistent status styling (e.g., gray foreground with a still-colored background/glow).Monochrome sets grayscale status accents, but several components render hue via hardcoded RGBA/hex values, so the UI is not actually monochrome when that theme is active.
At least these are hardcoded today:
- Status icon backgrounds and glows (safe/warning/danger/info)
- Brand logo gradient includes a fixed blue
- Success button uses fixed green gradients
- src/ui/css/style.css[149-163]
- src/ui/css/style.css[214-227]
- src/ui/css/style.css[548-579]
- src/ui/css/style.css[658-668]
Pick one:
-
Theme-specific overrides (minimal churn): Add
:root[data-theme="monochrome"] .status-icon.safe { ... }etc. to replace the hardcoded colored RGBA backgrounds/glows with grayscale values (or derived from the grayscale--accent-*variables). -
Generalize via CSS variables (more scalable): Replace the hardcoded RGBA/hex literals in
.status-icon.*,.brand-logo, and.btn-successwith variables (e.g.,--status-safe-bg,--status-safe-glow,--brand-secondary,--btn-success-bg1/bg2) and define them per theme (including monochrome).
[correctness] Toasts ignore monochrome
Toasts ignore monochrome
The UI now allows persisting the `monochrome` theme, but custom Electron toast notifications use a fixed `TOAST_THEMES` map that has no monochrome entry, so they fall back to the dark palette. This produces inconsistent theming between the app UI and notifications.monochrome is now an allowed/persisted theme in the UI, but notification toasts look up colors by themeName in TOAST_THEMES and fall back to dark when the key is missing.
- The main process reads
ui.themefrom DB for notifications. -
toastHtml()usesTOAST_THEMES[themeName] || TOAST_THEMES.dark.
- src/ui/js/api.js[1-16]
- src/main/main.js[142-200]
- src/main/main.js[294-329]
- Add a
monochromeentry toTOAST_THEMES(and ideally addmidnighttoo, since it’s also a supported UI theme). - Optionally normalize legacy theme aliases in the main process (similar to
Api.applyTheme) before indexing intoTOAST_THEMESso older stored values don’t silently fall back.
| PR 60 (2026-07-14) |
[correctness] Toast theme fallback
Toast theme fallback
After adding 'bumblebee' to the allowed themes, it can be persisted as `ui.theme`, but main-process toast notifications only define palettes for a subset of themes so `bumblebee` toasts will render using the dark fallback palette. This causes notifications to not match the selected app theme whenever a toast is shown.When the user selects the new bumblebee theme, it is saved to ui.theme. Main-process toast notifications read ui.theme and try to theme the toast window using a TOAST_THEMES map, but bumblebee is not present, so toasts fall back to the dark palette.
- The renderer now persists
ui.theme = 'bumblebee'. - The main process uses
dbRef.getSetting('ui.theme', 'dark')and resolvesTOAST_THEMES[themeName] || TOAST_THEMES.dark.
- Add a
bumblebeeentry to theTOAST_THEMESmap (and optionally also addmidnightfor completeness, since it is also a selectable theme). - src/main/main.js[142-191]
- src/main/main.js[294-328]
- (optional) src/main/main.js[197-200]