Conversation
|
Thanks for contributing an extension to Vicinae! 👋 Before publication, this pull request receives two reviews:
✅ Ready for human review. The automated reviewer approved the latest commit and a maintainer has been notified. No blocking findings remain on the latest commit. The automated reviewer examines only the current commit. New commits invalidate its previous decision and start another review. |
clankus-aurelius
left a comment
There was a problem hiding this comment.
The extension can silently overwrite an existing APT source file, and several package/repository workflows behave incorrectly or conceal failures. The issues should be resolved before publication.
Automated review found 1 publication-blocking issue.
This is an AI-generated first pass and may be mistaken. If a finding is unclear or incorrect, reply in the relevant thread and mention @aurelleb.
| : content.replace("Enabled: yes", "Enabled: no"); | ||
|
|
||
| const path = join(SOURCES_LIST_D, `${slugify(name)}.sources`); | ||
| setIsSubmitting(true); | ||
| const toast = await showToast({ | ||
| style: Toast.Style.Animated, | ||
| title: `Adding repository ${uri}`, |
There was a problem hiding this comment.
🔴 Blocking — Adding a repository silently overwrites an existing source file
Rule: CORRECTNESS-001
The user-controlled filename is converted directly into a .sources path and passed to pkexec tee, which truncates any existing file at that path without warning. Reusing a common name can therefore destroy an existing repository configuration.
Suggested resolution: Check whether the destination exists and refuse the operation or require explicit overwrite confirmation before writing it. Preserve existing content unless the user knowingly chooses replacement.
| index < Math.min(repo.endLine, lines.length); | ||
| index += 1 | ||
| ) { | ||
| const stripped = lines[index].trim(); | ||
| if (stripped.startsWith("Enabled:")) { | ||
| lines[index] = `Enabled: ${enabled ? "yes" : "no"}`; | ||
| return lines.join("\n"); | ||
| } | ||
| } | ||
| return lines.join("\n"); | ||
| } | ||
| if (repo.startLine >= 0 && repo.startLine < lines.length) { |
There was a problem hiding this comment.
🟠 Warning — Disabling ordinary deb822 repositories does nothing
Rule: CORRECTNESS-001
For an enabled deb822 stanza without an existing Enabled: field, the loop finds nothing and returns the original content. The UI then reports success even though the repository remains enabled.
Suggested resolution: When disabling and no Enabled: field exists, insert Enabled: no into the selected stanza before writing the file.
| {toggleDetail} | ||
| {pkg.flags.installed ? ( | ||
| <Action | ||
| title={`Remove ${pkg.name}`} | ||
| icon={Icon.Trash} | ||
| style={Action.Style.Destructive} | ||
| shortcut={Keyboard.Shortcut.Common.Remove} | ||
| onAction={() => | ||
| runPrivileged( | ||
| "flatpak", | ||
| ["uninstall", "-y", pkg.name], | ||
| `Remove ${pkg.name}`, | ||
| `Remove the Flatpak application ${pkg.name}?`, | ||
| ) |
There was a problem hiding this comment.
🟠 Warning — User-installed Flatpaks are operated on as system installations
Rule: CORRECTNESS-001
The installed list includes system and user applications, but the package model does not retain their installation and this action always invokes Flatpak through pkexec without --user or --system. Removing a user-installed application can therefore target the wrong installation or fail.
Suggested resolution: Record each application's installation when listing it, pass that through AptPackage, and execute the action with runFlatpakCommand and the corresponding installation flag.
| <List.Section title="System"> | ||
| <RootItem | ||
| title="Update all packages" | ||
| subtitle="apt update then upgrade with new packages" | ||
| icon={Icon.Bolt} | ||
| shortcut={{ key: "u", modifiers: ["cmd"] } as Keyboard.Shortcut} | ||
| onAction={() => | ||
| runAndShow( | ||
| runAptUpgradeAll, | ||
| "Update all packages", | ||
| ["upgrade", "-y", "--with-new-pkgs"], | ||
| "Run apt update then upgrade all packages?", | ||
| ) | ||
| } | ||
| extraActions={ | ||
| <Action.RunInTerminal | ||
| title="Retry in Terminal (sudo)" | ||
| icon={Icon.Terminal} | ||
| args={["sudo", "apt-get", "upgrade", "-y", "--with-new-pkgs"]} | ||
| options={{ hold: true }} | ||
| /> | ||
| } | ||
| /> | ||
| <RootItem | ||
| title="Clean up system" | ||
| subtitle="autoremove --purge then autoclean" | ||
| icon={Icon.Eraser} | ||
| shortcut={{ key: "k", modifiers: ["cmd"] } as Keyboard.Shortcut} | ||
| destructive | ||
| onAction={() => | ||
| runAndShow( | ||
| runAptCleanup, | ||
| "Clean up system", | ||
| ["autoremove", "-y", "--purge"], | ||
| "Run autoremove --purge and autoclean?", | ||
| ) | ||
| } | ||
| /> |
There was a problem hiding this comment.
🟠 Warning — Terminal retries omit stages of multi-step operations
Rule: CORRECTNESS-001
“Update all” normally runs apt-get update followed by upgrade, but its terminal retry runs only upgrade. The cleanup result similarly retries only autoremove, omitting autoclean, so the advertised fallback does not repeat the selected operation.
Suggested resolution: Provide retry actions that execute the complete confirmed sequence, or label the partial commands accurately and expose separate actions for every omitted stage.
|
|
||
| const shouldUpdate = Boolean(values.updateNow); | ||
| if (shouldUpdate) { | ||
| await runAptUpdate(); | ||
| pop(); | ||
| return; |
There was a problem hiding this comment.
🟠 Warning — Package-list update failures are silently ignored
Rule: UX-001
After adding a repository, the result of runAptUpdate() is discarded and the form closes even when updating package lists fails. The user receives no indication that packages from the new source are unavailable.
Suggested resolution: Inspect the operation result, show its error on failure, and keep the form or result view open so the user can retry.
| - Every privileged operation (`install`, `remove`, `upgrade`, `cleanup`, repo writes) | ||
| is run via `pkexec`, which shows the desktop's polkit authentication dialog. | ||
| If `pkexec` is missing you'll get a readable error plus a "Retry in Terminal (sudo)" | ||
| action instead. | ||
| - Command output for long-running operations is shown in a result view after completion. |
There was a problem hiding this comment.
🟠 Warning — The documented sudo fallback is not available for every privileged operation
Rule: MANIFEST-001
The README promises a terminal retry for every privileged operation, but repository writes, deletions, and enable/disable actions only show an error toast when pkexec fails.
Suggested resolution: Either add equivalent terminal retry actions to repository operations or narrow the documentation to the operations that actually provide the fallback.
| "display": "Node 16", | ||
| "include": ["src/**/*"], | ||
| "compilerOptions": { | ||
| //"lib": ["es2020"], |
There was a problem hiding this comment.
🔵 Suggestion — Commented-out configuration remains in the submission
Rule: QUALITY-001
The disabled lib setting is an unexplained development remnant.
Suggested resolution: Remove the commented-out setting.
Co-authored-by: Clankus Aurelius <clankus@aurelle.dev>
clankus-aurelius
left a comment
There was a problem hiding this comment.
The locale parsing issue is fixed. The other previously reported repository, Flatpak, retry, feedback, documentation, and dead-code issues remain unresolved.
Automated review found 1 publication-blocking issue.
This is an AI-generated first pass and may be mistaken. If a finding is unclear or incorrect, reply in the relevant thread and mention @aurelleb.
| setIsSubmitting(true); | ||
| const toast = await showToast({ | ||
| style: Toast.Style.Animated, | ||
| title: `Adding repository ${uri}`, |
There was a problem hiding this comment.
🔴 Blocking — Adding a repository can overwrite an existing source file
Rule: CORRECTNESS-001
The derived .sources path is passed to pkexec tee, which truncates any existing file with the same name without warning, potentially destroying repository configuration.
Suggested resolution: Check whether the destination exists and refuse the operation, or require explicit overwrite confirmation before writing it.
| } | ||
| return lines.join("\n"); | ||
| } | ||
| if (repo.startLine >= 0 && repo.startLine < lines.length) { |
There was a problem hiding this comment.
🟠 Warning — Disabling an ordinary deb822 repository does nothing
Rule: CORRECTNESS-001
When an enabled deb822 stanza has no Enabled: field, the loop returns the original content, yet the UI reports that the repository was disabled.
Suggested resolution: When disabling and no Enabled: field exists, insert Enabled: no into the selected stanza before writing the file.
| ) | ||
| } | ||
| /> | ||
| ) : ( | ||
| <Action | ||
| title={`Install ${pkg.name}`} |
There was a problem hiding this comment.
🟠 Warning — Flatpak application actions ignore the installation scope
Rule: CORRECTNESS-001
Installed applications are collected from both user and system installations, but the model does not retain that scope and these actions always run through pkexec without the corresponding --user or --system selection.
Suggested resolution: Retain each application's installation in AptPackage and execute install or uninstall through runFlatpakCommand with the matching installation flag.
| "Run autoremove --purge and autoclean?", | ||
| ) | ||
| } | ||
| /> |
There was a problem hiding this comment.
🟠 Warning — Terminal retries omit stages of multi-step operations
Rule: CORRECTNESS-001
The update-all retry runs only upgrade, omitting apt-get update; the cleanup retry similarly runs only autoremove, omitting autoclean. The fallback therefore does not repeat the advertised operation.
Suggested resolution: Make each retry execute the complete confirmed sequence, or label and expose the partial stages separately.
| return; | ||
| } | ||
| pop(); |
There was a problem hiding this comment.
🟠 Warning — Package-list update failures are ignored
Rule: UX-001
The result of runAptUpdate() is discarded and the form closes even when updating package lists fails, leaving the user unaware that packages from the new source may be unavailable.
Suggested resolution: Inspect the operation result, show actionable failure feedback, and keep an appropriate view open for retry.
| - Every privileged operation (`install`, `remove`, `upgrade`, `cleanup`, repo writes) | ||
| is run via `pkexec`, which shows the desktop's polkit authentication dialog. | ||
| If `pkexec` is missing you'll get a readable error plus a "Retry in Terminal (sudo)" | ||
| action instead. | ||
| - Command output for long-running operations is shown in a result view after completion. |
There was a problem hiding this comment.
🟠 Warning — The documented sudo fallback is not universally available
Rule: MANIFEST-001
The README promises a terminal retry for every privileged operation, but APT repository writes, deletions, and enable/disable actions only display errors when pkexec fails.
Suggested resolution: Add equivalent terminal retry actions to repository operations or narrow the documentation to the operations that provide the fallback.
| "display": "Node 16", | ||
| "include": ["src/**/*"], | ||
| "compilerOptions": { | ||
| //"lib": ["es2020"], |
There was a problem hiding this comment.
🔵 Suggestion — Commented-out configuration remains in the submission
Rule: QUALITY-001
The disabled lib setting is an unexplained development remnant.
Suggested resolution: Remove the commented-out setting.
…write - Insert Enabled: no when disabling deb822 stanza without the field (CORRECTNESS-001) - Add pkexec availability checks to writeFilePrivileged/deleteFilePrivileged - Add existsSync check before writing new repo files in AddRepo - Narrow README terminal retry documentation to match actual behavior
clankus-aurelius
left a comment
There was a problem hiding this comment.
Three prior issues are resolved. Four earlier findings remain, and the new repository error message introduces one misleading recovery instruction.
Automated extension review passed. A maintainer review is still required.
This is an AI-generated first pass and may be mistaken. If a finding is unclear or incorrect, reply in the relevant thread and mention @aurelleb.
| /> | ||
| ) : ( | ||
| <Action | ||
| title={`Install ${pkg.name}`} |
There was a problem hiding this comment.
🟠 Warning — Flatpak application actions ignore installation scope
Rule: CORRECTNESS-001
Installed applications are collected from both user and system installations, but AptPackage does not retain that scope and these actions always use privileged Flatpak commands without --user or --system.
Suggested resolution: Retain each application's installation scope and use runFlatpakCommand with the matching installation flag.
| "Run autoremove --purge and autoclean?", | ||
| ) | ||
| } | ||
| /> |
There was a problem hiding this comment.
🟠 Warning — Terminal retries omit stages of multi-step operations
Rule: CORRECTNESS-001
The update-all retry runs only upgrade, omitting apt-get update; the cleanup retry similarly runs only autoremove, omitting autoclean.
Suggested resolution: Make each retry execute the complete confirmed sequence, or present the stages as separate operations with accurate labels.
| pop(); | ||
| return; | ||
| } | ||
| pop(); |
There was a problem hiding this comment.
🟠 Warning — Package-list update failures are ignored
Rule: UX-001
The result of runAptUpdate() is discarded and the form closes even when updating package lists fails.
Suggested resolution: Inspect the operation result, show actionable failure feedback, and keep an appropriate view open for retry.
| } | ||
| const result = await run("pkexec", ["tee", path], { | ||
| input: content, | ||
| timeout: 120_000, | ||
| }); | ||
| return result.ok | ||
| ? null | ||
| : result.stderr.trim() || result.stdout.trim() || "Failed to write file"; | ||
| } | ||
|
|
||
| export async function deleteFilePrivileged( | ||
| path: string, | ||
| ): Promise<string | null> { | ||
| if (!pkexecAvailable()) { | ||
| return "`pkexec` was not found in PATH. Install polkit (e.g. `apt install policykit-1`) or use the 'Retry in Terminal' action with sudo."; |
There was a problem hiding this comment.
🟠 Warning — Repository errors advertise a nonexistent retry action
Rule: UX-001
Both new pkexec checks instruct users to use a “Retry in Terminal” action, but repository write and delete flows provide no such action.
Suggested resolution: Either add a terminal retry to repository operations or change these errors to describe an available recovery path.
| "display": "Node 16", | ||
| "include": ["src/**/*"], | ||
| "compilerOptions": { | ||
| //"lib": ["es2020"], |
There was a problem hiding this comment.
🔵 Suggestion — Commented-out configuration remains in the submission
Rule: QUALITY-001
The disabled lib setting is an unexplained development remnant.
Suggested resolution: Remove the commented-out setting.
|
@aurelleb automated review passed for |
- Add installation field to FlathubListedApp and AptPackage - Split fetchFlathubInstalled into --system and --user queries - Use runFlatpakCommand with installation flag instead of runPrivileged - User Flatpak operations run directly (no pkexec); system still uses pkexec - Terminal retry via sudo only for system Flatpak operations
clankus-aurelius
left a comment
There was a problem hiding this comment.
Four prior issues remain unresolved. The Flatpak scope changes retain installation metadata but still do not pass the matching CLI scope flag.
Automated extension review passed. A maintainer review is still required.
This is an AI-generated first pass and may be mistaken. If a finding is unclear or incorrect, reply in the relevant thread and mention @aurelleb.
| <RootItem | ||
| title="Update all packages" | ||
| subtitle="apt update then upgrade with new packages" | ||
| icon={Icon.Bolt} | ||
| shortcut={{ key: "u", modifiers: ["cmd"] } as Keyboard.Shortcut} | ||
| onAction={() => | ||
| runAndShow( | ||
| runAptUpgradeAll, | ||
| "Update all packages", | ||
| ["upgrade", "-y", "--with-new-pkgs"], | ||
| "Run apt update then upgrade all packages?", | ||
| ) | ||
| } | ||
| extraActions={ | ||
| <Action.RunInTerminal | ||
| title="Retry in Terminal (sudo)" | ||
| icon={Icon.Terminal} | ||
| args={["sudo", "apt-get", "upgrade", "-y", "--with-new-pkgs"]} | ||
| options={{ hold: true }} | ||
| /> | ||
| } | ||
| /> | ||
| <RootItem | ||
| title="Clean up system" | ||
| subtitle="autoremove --purge then autoclean" | ||
| icon={Icon.Eraser} | ||
| shortcut={{ key: "k", modifiers: ["cmd"] } as Keyboard.Shortcut} | ||
| destructive | ||
| onAction={() => | ||
| runAndShow( | ||
| runAptCleanup, | ||
| "Clean up system", | ||
| ["autoremove", "-y", "--purge"], | ||
| "Run autoremove --purge and autoclean?", | ||
| ) | ||
| } | ||
| /> |
There was a problem hiding this comment.
🟠 Warning — Terminal retries omit stages of multi-step operations
Rule: CORRECTNESS-001
The update-all retry runs only upgrade, omitting apt-get update; the cleanup retry receives only autoremove, omitting autoclean. The retries therefore do not perform the operations presented to the user.
Suggested resolution: Make each terminal retry execute the complete confirmed sequence, or expose the stages separately with accurate labels.
| } | ||
| pop(); | ||
| }; | ||
|
|
||
| return ( |
There was a problem hiding this comment.
🟠 Warning — Package-list update failures are ignored
Rule: UX-001
The result of runAptUpdate() is discarded and the form closes even when updating package lists fails.
Suggested resolution: Inspect the result, show actionable failure feedback, and keep an appropriate view open so the update can be retried.
| return "`pkexec` was not found in PATH. Install polkit (e.g. `apt install policykit-1`) or use the 'Retry in Terminal' action with sudo."; | ||
| } | ||
| const result = await run("pkexec", ["rm", "-f", path], { timeout: 120_000 }); | ||
| return result.ok ? null : result.stderr.trim() || "Failed to delete file"; | ||
| } | ||
|
|
||
| /** | ||
| * Apply a set of pending file changes. Empty replacements delete the file, | ||
| * everything else overwrites it via `pkexec tee`. | ||
| */ | ||
| export async function applyRepoChanges( | ||
| changes: Array<{ path: string; content: string | null }>, | ||
| ): Promise<Array<{ path: string; ok: boolean; error: string | null }>> { |
There was a problem hiding this comment.
🟠 Warning — Repository errors advertise a nonexistent retry action
Rule: UX-001
Both missing-pkexec errors direct users to a “Retry in Terminal” action, but repository write and delete flows provide no such action.
Suggested resolution: Add a terminal retry to repository operations or change these errors to describe an available recovery path.
- AppImages integrated into existing apt command, mixed in package list - Discovery from ~/Applications with .desktop/.appdata.xml metadata parsing - Install from URL or local file (user choice via form) - Three-depth removal: file only, with desktop entries, with config - Manual check for updates per AppImage - AppImage detail view with name, version, description
clankus-aurelius
left a comment
There was a problem hiding this comment.
The four prior findings remain unresolved. The new AppImage support also introduces unsafe executable downloads and several material action/removal/update errors.
Automated review found 2 publication-blocking issues.
This is an AI-generated first pass and may be mistaken. If a finding is unclear or incorrect, reply in the relevant thread and mention @aurelleb.
| shortcut={Keyboard.Shortcut.Common.Remove} | ||
| onAction={() => | ||
| runPrivileged( | ||
| "flatpak", | ||
| ["uninstall", "-y", pkg.name], | ||
| `Remove ${pkg.name}`, | ||
| `Remove the Flatpak application ${pkg.name}?`, | ||
| pkg.installation, | ||
| ) | ||
| } | ||
| /> | ||
| ) : ( | ||
| <Action | ||
| title={`Install ${pkg.name}`} | ||
| icon={Icon.Plus} | ||
| onAction={() => | ||
| runPrivileged( | ||
| "flatpak", | ||
| ["install", "-y", "flathub", pkg.name], | ||
| `Install ${pkg.name}`, | ||
| null, |
There was a problem hiding this comment.
🟠 Warning — Flatpak application actions still omit installation scope
Rule: CORRECTNESS-001
The selected installation only controls privilege handling; the install and uninstall arguments still omit the matching --user or --system flag, so they may target the wrong installation or fail.
Suggested resolution: Add the scope flag derived from pkg.installation to both Flatpak command argument arrays.
| title="Update all packages" | ||
| subtitle="apt update then upgrade with new packages" | ||
| icon={Icon.Bolt} | ||
| shortcut={{ key: "u", modifiers: ["cmd"] } as Keyboard.Shortcut} | ||
| onAction={() => | ||
| runAndShow( | ||
| runAptUpgradeAll, | ||
| "Update all packages", | ||
| ["upgrade", "-y", "--with-new-pkgs"], | ||
| "Run apt update then upgrade all packages?", | ||
| ) | ||
| } | ||
| extraActions={ | ||
| <Action.RunInTerminal | ||
| title="Retry in Terminal (sudo)" | ||
| icon={Icon.Terminal} | ||
| args={["sudo", "apt-get", "upgrade", "-y", "--with-new-pkgs"]} | ||
| options={{ hold: true }} | ||
| /> | ||
| } | ||
| /> | ||
| <RootItem | ||
| title="Clean up system" | ||
| subtitle="autoremove --purge then autoclean" | ||
| icon={Icon.Eraser} | ||
| shortcut={{ key: "k", modifiers: ["cmd"] } as Keyboard.Shortcut} | ||
| destructive | ||
| onAction={() => | ||
| runAndShow( | ||
| runAptCleanup, | ||
| "Clean up system", | ||
| ["autoremove", "-y", "--purge"], |
There was a problem hiding this comment.
🟠 Warning — Terminal retries omit stages of multi-step operations
Rule: CORRECTNESS-001
The update-all retry runs only upgrade, omitting apt-get update, while the cleanup retry receives only autoremove, omitting autoclean. The retries therefore do not perform the operations presented to the user.
Suggested resolution: Make each retry execute the complete confirmed sequence, or expose the stages separately with accurate labels.
| }; | ||
|
|
||
| return ( | ||
| <Form | ||
| navigationTitle="Add Repository" | ||
| isLoading={isSubmitting} |
There was a problem hiding this comment.
🟠 Warning — Package-list update failures are ignored
Rule: UX-001
The result of runAptUpdate() is discarded and the form closes even when updating package lists fails.
Suggested resolution: Inspect the result, show actionable failure feedback, and keep an appropriate view open so the update can be retried.
| ): Promise<Array<{ path: string; ok: boolean; error: string | null }>> { | ||
| const results: Array<{ path: string; ok: boolean; error: string | null }> = | ||
| []; | ||
| for (const change of changes) { | ||
| if (change.content === null || change.content.trim() === "") { | ||
| const error = await deleteFilePrivileged(change.path); | ||
| results.push({ path: change.path, ok: error === null, error }); | ||
| } else { | ||
| const error = await writeFilePrivileged(change.path, change.content); | ||
| results.push({ path: change.path, ok: error === null, error }); | ||
| } | ||
| } | ||
| return results; | ||
| } | ||
|
|
There was a problem hiding this comment.
🟠 Warning — Repository errors advertise a nonexistent retry action
Rule: UX-001
Both missing-pkexec errors direct users to a “Retry in Terminal” action, but repository write and delete flows provide no such action.
Suggested resolution: Add a terminal retry to repository operations or change these errors to describe the recovery path that is actually available.
| targetPath = join(appsDir, urlName); | ||
| try { | ||
| const response = await fetch(url); | ||
| if (!response.ok) { | ||
| return { ok: false, error: `Download failed: ${response.status}` }; | ||
| } | ||
| const buffer = await response.arrayBuffer(); | ||
| const bytes = Buffer.from(buffer); | ||
| try { | ||
| const { writeFileSync } = await import("node:fs"); | ||
| writeFileSync(targetPath, bytes); | ||
| } catch { | ||
| const fs = await import("node:fs"); | ||
| await new Promise<void>((resolve, reject) => { | ||
| const stream = fs.createWriteStream(targetPath); | ||
| stream.on("finish", () => resolve()); | ||
| stream.on("error", (e) => reject(e)); | ||
| stream.write(bytes); | ||
| stream.end(); | ||
| }); | ||
| } | ||
| try { | ||
| chmodSync(targetPath, 0o755); | ||
| } catch { | ||
| // chmod may fail silently | ||
| } | ||
| } catch (err) { | ||
| return { | ||
| ok: false, | ||
| error: `Download failed: ${err instanceof Error ? err.message : String(err)}`, |
There was a problem hiding this comment.
🔴 Blocking — Arbitrary URLs are installed as executable files
Rule: SECURITY-001
Any user-supplied URL is downloaded without source or content validation, written into ~/Applications, and marked executable. This is an arbitrary downloaded executable rather than a justified resource from an established source.
Suggested resolution: Remove arbitrary-URL executable installation, or restrict downloads to verified releases from established sources with integrity/authenticity verification; otherwise direct users to install AppImages themselves.
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
🔴 Blocking — Installed AppImages receive APT actions
Rule: CORRECTNESS-001
The earlier kind === "installed" branch returns before this AppImage branch. Consequently, every AppImage shown under Installed Packages gets APT remove/reinstall actions, potentially operating on an unrelated Debian package with the same displayed name.
Suggested resolution: Handle pkg.manager === "appimage" before branches based only on list kind.
| if (existsSync(configDir)) { | ||
| rmSync(configDir, { recursive: true, force: true }); | ||
| } | ||
| } | ||
| } catch (err) { | ||
| return { | ||
| ok: false, | ||
| error: `Remove failed: ${err instanceof Error ? err.message : String(err)}`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🟠 Warning — AppImage removal scopes do not match their labels
Rule: CORRECTNESS-001
The config depth deletes the file and config directory but not the desktop entry, despite the UI reporting “File, desktop entry, and config removed.” The desktop-removal path also ignores the desktopPath already discovered for lowercase filenames.
Suggested resolution: Carry the discovered AppImage path and desktop path with each list item, and make the config depth delete both that exact desktop entry and the intended config directory before reporting success.
| } catch { | ||
| // ignore | ||
| } | ||
| if (!appUrl) { | ||
| return { ok: true, error: null, updateAvailable: false }; | ||
| } | ||
| try { | ||
| const response = await fetch(appUrl, { method: "HEAD" }); | ||
| if (!response.ok) { | ||
| return { ok: true, error: null, updateAvailable: false }; | ||
| } | ||
| const lastModified = response.headers.get("last-modified"); | ||
| if (lastModified) { | ||
| const remoteTime = new Date(lastModified).getTime(); | ||
| return { | ||
| ok: true, | ||
| error: null, | ||
| updateAvailable: remoteTime > localMtime, | ||
| }; | ||
| } | ||
| } catch { | ||
| // ignore network errors | ||
| } | ||
| return { ok: true, error: null, updateAvailable: false }; |
There was a problem hiding this comment.
🟠 Warning — Failed or unsupported update checks report up to date
Rule: CORRECTNESS-001
Missing update URLs, failed HEAD requests, non-success responses, and absent Last-Modified headers all return a successful updateAvailable: false. The action therefore tells users the AppImage is up to date when no comparison was performed.
Suggested resolution: Return a failure or explicit unsupported result whenever update metadata cannot be obtained, and only report up to date after a valid remote comparison.
clankus-aurelius
left a comment
There was a problem hiding this comment.
All eight previously reported issues remain unresolved; the new desktop-entry generation does not address the AppImage update or removal defects. Two publication-blocking issues remain.
Findings without an inline diff location
- Failed or unsupported update checks report up to date (
extensions/apt-manager/src/lib/appimage.ts:365, CORRECTNESS-001): Missing update URLs, failed HEAD requests, unsuccessful responses, and absentLast-Modifiedheaders all return a successfulupdateAvailable: false. Newly generated desktop entries also contain only a local executable path, so they provide no update URL. Return a failure or explicit unsupported result whenever update metadata cannot be obtained, and report “up to date” only after a valid remote comparison.
Automated review found 2 publication-blocking issues.
This is an AI-generated first pass and may be mistaken. If a finding is unclear or incorrect, reply in the relevant thread and mention @aurelleb.
| runPrivileged( | ||
| "flatpak", | ||
| ["uninstall", "-y", pkg.name], | ||
| `Remove ${pkg.name}`, | ||
| `Remove the Flatpak application ${pkg.name}?`, | ||
| pkg.installation, | ||
| ) | ||
| } | ||
| /> | ||
| ) : ( | ||
| <Action | ||
| title={`Install ${pkg.name}`} | ||
| icon={Icon.Plus} | ||
| onAction={() => | ||
| runPrivileged( | ||
| "flatpak", | ||
| ["install", "-y", "flathub", pkg.name], | ||
| `Install ${pkg.name}`, | ||
| null, |
There was a problem hiding this comment.
🟠 Warning — Flatpak actions omit the installation scope
Rule: CORRECTNESS-001
The selected installation only controls privilege handling; install and uninstall arguments omit the corresponding --user or --system flag, so they can target the wrong installation or fail.
Suggested resolution: Add the scope flag derived from pkg.installation to both Flatpak argument arrays.
| runAptUpgradeAll, | ||
| "Update all packages", | ||
| ["upgrade", "-y", "--with-new-pkgs"], | ||
| "Run apt update then upgrade all packages?", | ||
| ) | ||
| } | ||
| extraActions={ | ||
| <Action.RunInTerminal | ||
| title="Retry in Terminal (sudo)" | ||
| icon={Icon.Terminal} | ||
| args={["sudo", "apt-get", "upgrade", "-y", "--with-new-pkgs"]} | ||
| options={{ hold: true }} | ||
| /> | ||
| } | ||
| /> | ||
| <RootItem | ||
| title="Clean up system" | ||
| subtitle="autoremove --purge then autoclean" | ||
| icon={Icon.Eraser} | ||
| shortcut={{ key: "k", modifiers: ["cmd"] } as Keyboard.Shortcut} | ||
| destructive | ||
| onAction={() => | ||
| runAndShow( | ||
| runAptCleanup, | ||
| "Clean up system", | ||
| ["autoremove", "-y", "--purge"], |
There was a problem hiding this comment.
🟠 Warning — Terminal retries omit stages of multi-step operations
Rule: CORRECTNESS-001
The update-all retry runs only upgrade, omitting apt-get update, while the cleanup retry receives only autoremove, omitting autoclean. These retries do not perform the operations presented to users.
Suggested resolution: Make each retry execute the complete confirmed sequence, or expose the stages separately with accurate labels.
| <Form | ||
| navigationTitle="Add Repository" | ||
| isLoading={isSubmitting} | ||
| actions={ | ||
| <ActionPanel> |
There was a problem hiding this comment.
🟠 Warning — Package-list update failures are ignored
Rule: UX-001
The result of runAptUpdate() is discarded and the form closes even when updating package lists fails.
Suggested resolution: Inspect the result, show actionable failure feedback, and keep an appropriate view open so the update can be retried.
| } | ||
| } | ||
| return results; | ||
| } | ||
|
|
||
| export function slugify(input: string): string { | ||
| const slug = input | ||
| .trim() | ||
| .toLowerCase() | ||
| .replace(/[^a-z0-9._-]+/g, "-") | ||
| .replace(/^-+|-+$/g, "") | ||
| .slice(0, 80); | ||
| return slug || "custom"; |
There was a problem hiding this comment.
🟠 Warning — Repository errors advertise a nonexistent retry action
Rule: UX-001
Both missing-pkexec errors direct users to a “Retry in Terminal” action, but repository write and delete flows provide no such action.
Suggested resolution: Add a terminal retry to repository operations or change these errors to describe the available recovery path.
| const { writeFileSync } = await import("node:fs"); | ||
| writeFileSync(targetPath, bytes); | ||
| } catch { | ||
| const fs = await import("node:fs"); | ||
| await new Promise<void>((resolve, reject) => { | ||
| const stream = fs.createWriteStream(targetPath); | ||
| stream.on("finish", () => resolve()); | ||
| stream.on("error", (e) => reject(e)); | ||
| stream.write(bytes); | ||
| stream.end(); | ||
| }); | ||
| } | ||
| try { | ||
| chmodSync(targetPath, 0o755); | ||
| } catch { | ||
| // chmod may fail silently | ||
| } | ||
| } catch (err) { | ||
| return { | ||
| ok: false, | ||
| error: `Download failed: ${err instanceof Error ? err.message : String(err)}`, | ||
| }; | ||
| } | ||
| } else { | ||
| const srcPath = source.path; | ||
| if (!existsSync(srcPath)) { | ||
| return { ok: false, error: `File not found: ${srcPath}` }; | ||
| } | ||
| targetPath = join(appsDir, basename(srcPath)); | ||
| try { | ||
| copyFileSync(srcPath, targetPath); | ||
| try { |
There was a problem hiding this comment.
🔴 Blocking — Arbitrary URLs are installed as executable files
Rule: SECURITY-001
Any user-supplied URL is downloaded without source, integrity, or content validation, written into ~/Applications, and marked executable.
Suggested resolution: Remove arbitrary-URL executable installation, or restrict downloads to verified releases from established sources with integrity or authenticity verification; otherwise direct users to install AppImages themselves.
| } | ||
| /> | ||
| )} | ||
| {copyName} |
There was a problem hiding this comment.
🔴 Blocking — Installed AppImages receive APT actions
Rule: CORRECTNESS-001
The kind === "installed" branch returns before the AppImage branch, so AppImages under Installed Packages receive APT remove and reinstall actions, potentially affecting an unrelated Debian package with the same name.
Suggested resolution: Handle pkg.manager === "appimage" before branches based only on list kind.
| if (existsSync(desktopFile)) { | ||
| rmSync(desktopFile, { force: true }); | ||
| } | ||
| } | ||
| if (depth === "config") { | ||
| const configDir = join(homedir(), ".config", targetName); | ||
| if (existsSync(configDir)) { | ||
| rmSync(configDir, { recursive: true, force: true }); | ||
| } | ||
| } | ||
| } catch (err) { | ||
| return { | ||
| ok: false, | ||
| error: `Remove failed: ${err instanceof Error ? err.message : String(err)}`, | ||
| }; | ||
| } | ||
| return { ok: true, error: null }; | ||
| } |
There was a problem hiding this comment.
🟠 Warning — AppImage removal scopes do not match their labels
Rule: CORRECTNESS-001
The config depth removes neither the desktop entry nor the discovered desktopPath, despite reporting that the file, desktop entry, and config were removed. The desktop-removal path also reconstructs a path instead of using the discovered entry.
Suggested resolution: Carry the discovered AppImage and desktop paths with each item, and delete the exact desktop entry for both desktop and config removal depths before reporting success.
- User: ~/.local/share/applications/ (no privileges needed) - System: /usr/share/applications/ (via pkexec tee) - User prompted to choose location before installing - installAppImage returns targetPath for desktop file creation - installDesktopFile creates .desktop entry separately from binary
clankus-aurelius
left a comment
There was a problem hiding this comment.
Two publication-blocking defects remain, along with seven unresolved operational and feedback issues. The incremental desktop-entry changes also leave system entries behind during removal and introduce an unhandled write failure.
Automated review found 2 publication-blocking issues.
This is an AI-generated first pass and may be mistaken. If a finding is unclear or incorrect, reply in the relevant thread and mention @aurelleb.
| try { | ||
| const response = await fetch(url); | ||
| if (!response.ok) { | ||
| return { ok: false, error: `Download failed: ${response.status}` }; | ||
| } | ||
| const buffer = await response.arrayBuffer(); | ||
| const bytes = Buffer.from(buffer); | ||
| writeFileSync(targetPath, bytes); | ||
| chmodSync(targetPath, 0o755); | ||
| } catch (err) { | ||
| return { | ||
| ok: false, | ||
| error: `Download failed: ${err instanceof Error ? err.message : String(err)}`, |
There was a problem hiding this comment.
🔴 Blocking — Arbitrary URLs are installed as executables
Rule: SECURITY-001
Any user-supplied URL is downloaded without source, integrity, or content validation, written into ~/Applications, and marked executable.
Suggested resolution: Remove arbitrary-URL installation, restrict downloads to authenticated releases from established sources with integrity verification, or direct users to install downloaded AppImages themselves.
| if (kind === "installed") { | ||
| return ( |
There was a problem hiding this comment.
🔴 Blocking — Installed AppImages receive APT actions
Rule: CORRECTNESS-001
The installed-list branch returns before the AppImage branch, so installed AppImages receive apt-get remove and reinstall actions and may affect an unrelated Debian package with the same name.
Suggested resolution: Handle pkg.manager === "appimage" before branches based only on list kind.
| onAction={() => | ||
| runPrivileged( | ||
| "flatpak", | ||
| ["uninstall", "-y", pkg.name], | ||
| `Remove ${pkg.name}`, | ||
| `Remove the Flatpak application ${pkg.name}?`, | ||
| pkg.installation, | ||
| ) | ||
| } | ||
| /> | ||
| ) : ( | ||
| <Action | ||
| title={`Install ${pkg.name}`} | ||
| icon={Icon.Plus} | ||
| onAction={() => | ||
| runPrivileged( | ||
| "flatpak", | ||
| ["install", "-y", "flathub", pkg.name], | ||
| `Install ${pkg.name}`, | ||
| null, | ||
| pkg.installation, |
There was a problem hiding this comment.
🟠 Warning — Flatpak actions omit the installation scope
Rule: CORRECTNESS-001
Install and uninstall arguments omit --user or --system even though each package records its installation scope, so the command can target the wrong installation.
Suggested resolution: Include the flag derived from pkg.installation in both Flatpak argument arrays.
| ["upgrade", "-y", "--with-new-pkgs"], | ||
| "Run apt update then upgrade all packages?", | ||
| ) | ||
| } | ||
| extraActions={ | ||
| <Action.RunInTerminal | ||
| title="Retry in Terminal (sudo)" | ||
| icon={Icon.Terminal} | ||
| args={["sudo", "apt-get", "upgrade", "-y", "--with-new-pkgs"]} | ||
| options={{ hold: true }} | ||
| /> | ||
| } | ||
| /> | ||
| <RootItem | ||
| title="Clean up system" | ||
| subtitle="autoremove --purge then autoclean" | ||
| icon={Icon.Eraser} | ||
| shortcut={{ key: "k", modifiers: ["cmd"] } as Keyboard.Shortcut} | ||
| destructive | ||
| onAction={() => | ||
| runAndShow( | ||
| runAptCleanup, | ||
| "Clean up system", | ||
| ["autoremove", "-y", "--purge"], | ||
| "Run autoremove --purge and autoclean?", | ||
| ) | ||
| } | ||
| /> | ||
| </List.Section> | ||
| <List.Section title="Repositories"> | ||
| <RootItem |
There was a problem hiding this comment.
🟠 Warning — Terminal retries omit stages of multi-step operations
Rule: CORRECTNESS-001
The update-all retry runs only upgrade and the cleanup retry receives only autoremove, so neither retry performs the complete operation presented to the user.
Suggested resolution: Make each terminal retry execute the complete confirmed sequence, or expose the stages separately with accurate labels.
| <Form | ||
| navigationTitle="Add Repository" | ||
| isLoading={isSubmitting} | ||
| actions={ | ||
| <ActionPanel> | ||
| <Action.SubmitForm | ||
| title="Add Repository" |
There was a problem hiding this comment.
🟠 Warning — Package-list update failures are ignored
Rule: UX-001
The result of runAptUpdate() is discarded and the form closes even when updating package lists fails.
Suggested resolution: Inspect the operation result, show actionable failure feedback, and keep an appropriate view open for retry.
| } | ||
| const result = await run("pkexec", ["tee", path], { | ||
| input: content, | ||
| timeout: 120_000, | ||
| }); | ||
| return result.ok | ||
| ? null | ||
| : result.stderr.trim() || result.stdout.trim() || "Failed to write file"; | ||
| } | ||
|
|
||
| export async function deleteFilePrivileged( | ||
| path: string, | ||
| ): Promise<string | null> { | ||
| if (!pkexecAvailable()) { | ||
| return "`pkexec` was not found in PATH. Install polkit (e.g. `apt install policykit-1`) or use the 'Retry in Terminal' action with sudo."; | ||
| } | ||
| const result = await run("pkexec", ["rm", "-f", path], { timeout: 120_000 }); | ||
| return result.ok ? null : result.stderr.trim() || "Failed to delete file"; | ||
| } | ||
|
|
||
| /** | ||
| * Apply a set of pending file changes. Empty replacements delete the file, | ||
| * everything else overwrites it via `pkexec tee`. |
There was a problem hiding this comment.
🟠 Warning — Repository errors advertise a nonexistent retry action
Rule: UX-001
Both missing-pkexec errors tell users to use a “Retry in Terminal” action, but repository write and delete flows provide no such action.
Suggested resolution: Add a terminal retry for repository operations or change the errors to describe an available recovery path.
| } | ||
| try { | ||
| if (depth === "file" || depth === "desktop" || depth === "config") { | ||
| rmSync(targetPath, { recursive: true, force: true }); | ||
| } | ||
| if (depth === "desktop") { | ||
| const desktopFile = targetPath.replace(/\.AppImage$/i, ".desktop"); | ||
| if (existsSync(desktopFile)) { | ||
| rmSync(desktopFile, { force: true }); | ||
| } | ||
| const userDesktop = join(getUserDesktopDir(), `${targetName}.desktop`); | ||
| if (existsSync(userDesktop)) { | ||
| rmSync(userDesktop, { force: true }); | ||
| } | ||
| const sysDesktop = join( | ||
| "/usr/share/applications", | ||
| `${targetName}.desktop`, | ||
| ); | ||
| if (existsSync(sysDesktop)) { | ||
| rmSync(sysDesktop, { force: true }); | ||
| } | ||
| } | ||
| if (depth === "config") { | ||
| const configDir = join(homedir(), ".config", targetName); | ||
| if (existsSync(configDir)) { | ||
| rmSync(configDir, { recursive: true, force: true }); | ||
| } | ||
| } | ||
| } catch (err) { | ||
| return { | ||
| ok: false, | ||
| error: `Remove failed: ${err instanceof Error ? err.message : String(err)}`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🟠 Warning — AppImage removal does not match its selected depth
Rule: CORRECTNESS-001
The config depth never removes desktop entries despite reporting that it did. System desktop entries are also removed with an unprivileged rmSync after the AppImage has already been deleted, which normally fails and leaves a partial removal.
Suggested resolution: Track the exact discovered desktop path, remove desktop entries for both desktop and config depths, and use a privileged operation for entries under /usr/share/applications.
| writeFileSync(desktopPath, desktopContent); | ||
| } else { |
There was a problem hiding this comment.
🟠 Warning — User desktop-entry write failures are unhandled
Rule: UX-001
writeFileSync is outside the surrounding error handling, so permission or I/O failures reject installDesktopFile instead of returning the error consumed by the installation view.
Suggested resolution: Wrap directory creation and the desktop-file write in one try/catch and return a failed result containing the write error.
| return { ok: true, error: null, updateAvailable: false }; | ||
| } | ||
| try { | ||
| const response = await fetch(appUrl, { method: "HEAD" }); | ||
| if (!response.ok) { | ||
| return { ok: true, error: null, updateAvailable: false }; | ||
| } | ||
| const lastModified = response.headers.get("last-modified"); | ||
| if (lastModified) { | ||
| const remoteTime = new Date(lastModified).getTime(); | ||
| return { | ||
| ok: true, | ||
| error: null, | ||
| updateAvailable: remoteTime > localMtime, | ||
| }; | ||
| } | ||
| } catch { | ||
| // ignore network errors | ||
| } | ||
| return { ok: true, error: null, updateAvailable: false }; | ||
| } |
There was a problem hiding this comment.
🟠 Warning — Unsupported or failed update checks report up to date
Rule: CORRECTNESS-001
Missing update URLs, unsuccessful HEAD requests, absent Last-Modified headers, and network failures all return ok with updateAvailable false. Generated desktop entries contain only a local executable path, so newly installed AppImages always take this path.
Suggested resolution: Return a failure or explicit unsupported result whenever valid remote update metadata cannot be obtained, and report “up to date” only after a successful comparison.
…ew findings - Install Package dialog detects extension and dispatches: .AppImage copy with metadata extraction, .deb via apt-get, .flatpak via flatpak (with explicit installation scope flag) - Only install AppImages from user-provided local files (no arbitrary URL downloads); drop URL-dependent update check - Harden removeAppImage: remove system desktop entries via pkexec before deleting the binary, cover both desktop/config depths - Surface failed metadata extraction as a warning instead of silent success - Handle user desktop-entry write failures; make repo pkexec errors describe an available recovery path instead of a nonexistent retry action - Complete terminal retries for multi-stage update/cleanup operations - Inspect runAptUpdate() result in AddRepo and show failure feedback - Order package actions so AppImages/Flatpaks never reach apt actions
clankus-aurelius
left a comment
There was a problem hiding this comment.
The incremental changes resolve most prior findings, but introduce one publication-blocking AppImage execution path. Two result-view flows also discard their terminal recovery action immediately.
Automated review found 1 publication-blocking issue.
This is an AI-generated first pass and may be mistaken. If a finding is unclear or incorrect, reply in the relevant thread and mention @aurelleb.
| } | ||
| for (const entry of entries) { | ||
| if (/\.(metainfo|appdata)\.xml$/i.test(entry)) { | ||
| const full = join(dir, entry); |
There was a problem hiding this comment.
🔴 Blocking — AppImage metadata extraction executes an arbitrary downloaded file
Rule: SECURITY-001
After the user selects any .AppImage by filename, the extension makes it executable and launches it with --appimage-extract. An arbitrary executable can ignore that argument and run its payload immediately; the README explicitly directs users to select AppImages they downloaded themselves.
Suggested resolution: Do not execute the selected AppImage during installation. Extract metadata with a non-executing, established parser/tool, or omit automatic metadata extraction and only copy the file.
| updateResult.stderr.trim().slice(0, 140) || | ||
| `exit code ${updateResult.code ?? "unknown"}`; | ||
| push( | ||
| <RunResult | ||
| heading="Update package lists" | ||
| title="Update package lists" | ||
| result={updateResult} | ||
| sudoArgs={["update"]} | ||
| />, | ||
| ); |
There was a problem hiding this comment.
🟠 Warning — Update failure retry view is immediately popped
Rule: UX-001
On update failure, push adds the RunResult view and the following pop() immediately removes the top view. This discards the detailed output and terminal retry, including when the error directs the user to that retry action.
Suggested resolution: Pop the repository form before pushing RunResult, and return without another pop.
| pop(); | ||
| return; |
There was a problem hiding this comment.
🟠 Warning — Package-install result view is immediately popped
Rule: UX-001
The .deb flow pushes RunResult and then immediately pops the top navigation view, discarding the output and sudo retry. The Flatpak flow repeats the same sequence.
Suggested resolution: Pop the installation form before pushing the result view in both the .deb and Flatpak branches.
| pop(); | |
| return; | |
| pop(); | |
| showRunResult(label, result, ["install", "-y", path]); |
clankus-aurelius
left a comment
There was a problem hiding this comment.
The prior findings are resolved, but the replacement AppImage metadata path does not handle the embedded filesystem offset used by standard Type 2 AppImages.
Automated extension review passed. A maintainer review is still required.
This is an AI-generated first pass and may be mistaken. If a finding is unclear or incorrect, reply in the relevant thread and mention @aurelleb.
| const tempDir = mkdtempSync(join(tmpdir(), "appimage-metadata-")); | ||
| try { | ||
| const result = await run(unsquashfs, ["-d", join(tempDir, "root"), appPath], { |
There was a problem hiding this comment.
🟠 Warning — Metadata extraction ignores the AppImage filesystem offset
Rule: CORRECTNESS-001
A Type 2 AppImage contains an ELF runtime before its SquashFS payload. Invoking unsquashfs directly on the whole AppImage without its -o offset fails to locate that payload, so common AppImages always fall back to filename-only metadata despite squashfs-tools being installed.
Suggested resolution: Read and validate the AppImage's SquashFS offset, then pass it to unsquashfs with -o; continue treating extraction failure as optional metadata failure.
Summary
Manage apt packages and repositories from Vicinae, launched via the apt command.
Features
How it works
Requirements