fix: kill stale GPG agents before key generation#895
Merged
Conversation
Before generating GPG keys, kill any stale gpg-agent processes that might be holding locks on the key database. This helps prevent 'timeout while waiting on locked key database' errors. The issue occurs when another GPG process (possibly from another application) holds a lock on the key database, causing key generation to fail with timeout errors.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdd ResolvedGpgconfCommand and Pass::resolveGpgconfCommand to derive the appropriate gpgconf invocation from a configured gpg path; use it in GenerateGPGKeys to kill stale gpg-agent via a new Executor::executeBlocking overload that accepts an environment. Add tests and update Estonian/Russian translations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Pass
participant Executor
participant gpgconf as "gpgconf (host/WSL)"
participant gpg_agent as "gpg-agent"
User->>Pass: Request GenerateGPGKeys(...)
Pass->>Pass: resolveGpgconfCommand(gpgPath)
Pass->>Executor: executeBlocking(env, resolved.program, resolved.arguments + ["--kill","gpg-agent"])
Executor->>gpgconf: start process (prefix + --kill gpg-agent)
gpgconf->>gpg_agent: request termination
gpg_agent-->>gpgconf: exit/ack
gpgconf-->>Executor: exit code
Executor-->>Pass: return status
Pass-->>User: continue generation or report failure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Coverage is 20.115% — fix/gpg-keygen-timeout-v2 into main. No base build found for |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pass.cpp`:
- Around line 211-212: The two calls using Executor::executeBlocking with
gpgPath are invoking the wrong binary; change the calls so that gpgconf is
invoked directly instead of using gpg with extra args — replace the
Executor::executeBlocking(gpgPath, {"--kill", "gpg-agent"}) and
Executor::executeBlocking(gpgPath, {"gpgconf", "--kill", "gpg-agent"}) usages
with Executor::executeBlocking(gpgconfPath, {"--kill", "gpg-agent"}) (or
otherwise resolve the path to the gpgconf binary and call
Executor::executeBlocking with that path) so the actual command run is "gpgconf
--kill gpg-agent".
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed418b76-00c8-4253-ac13-df21554dbc40
📒 Files selected for processing (1)
src/pass.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pass.cpp`:
- Around line 209-212: Derive the gpgconf executable path from the resolved
gpgPath instead of calling the hardcoded "gpgconf": use
QtPassSettings::getGpgExecutable() (gpgPath) and build gpgconf by taking the
directory of gpgPath (via QFileInfo) and appending "gpgconf", verify it
exists/is executable and pass that full path to Executor::executeBlocking
instead of the literal "gpgconf", falling back to "gpgconf" if not found; also
add `#include` <QFileInfo> if not already included.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8fd06d57-4411-4886-b807-d71884d27cac
📒 Files selected for processing (1)
src/pass.cpp
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/pass.cpp (1)
212-219:⚠️ Potential issue | 🟠 MajorHandle WSL/Windows
gpgExecutableforms when resolvinggpgconf.At Line 213-217, path derivation assumes
gpgPathis a native filesystem path. In this codebase,gpgPathcan be"wsl gpg2"(fromsrc/util.cpp), so fallback at Line 217 may run hostgpgconfinstead of WSLgpgconf, leaving the locked WSL agent untouched. Also, Windows sibling resolution should account for.exe.Proposed fix
QString gpgPath = QtPassSettings::getGpgExecutable(); if (!gpgPath.isEmpty()) { - QFileInfo gpgInfo(gpgPath); - QString gpgconfPath = - gpgInfo.absolutePath() + QDir::separator() + "gpgconf"; - if (!QFileInfo::exists(gpgconfPath) || - !QFileInfo(gpgconfPath).isExecutable()) { - gpgconfPath = "gpgconf"; - } - Executor::executeBlocking(gpgconfPath, {"--kill", "gpg-agent"}); + QString gpgconfApp = "gpgconf"; + if (gpgPath.startsWith("wsl ")) { + gpgconfApp = "wsl gpgconf"; + } else { + QFileInfo gpgInfo(gpgPath); + QString candidate = + gpgInfo.absolutePath() + QDir::separator() + "gpgconf"; +#ifdef Q_OS_WIN + candidate += ".exe"; +#endif + if (QFileInfo::exists(candidate) && QFileInfo(candidate).isExecutable()) { + gpgconfApp = candidate; + } + } + Executor::executeBlocking(gpgconfApp, {"--kill", "gpg-agent"}); }Use this script to confirm the concern in current code:
#!/bin/bash set -euo pipefail echo "Check WSL-form gpg executable assignment:" rg -n -C3 'ret = wslCommand|QStringLiteral\("wsl "\)' src/util.cpp echo echo "Check current gpgconf resolution in GenerateGPGKeys:" rg -n -C5 'GenerateGPGKeys|gpgconfPath|startsWith\("wsl "' src/pass.cpp echo echo "Check whether .exe handling exists in this block:" rg -n -C3 'gpgconfPath|Q_OS_WIN' src/pass.cppExpected: WSL assignment exists in
src/util.cpp, while currentsrc/pass.cppblock has nowslbranch and no Windows.exehandling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pass.cpp` around lines 212 - 219, The code assumes gpgPath is a native filesystem path (gpgPath, gpgconfPath, QFileInfo) so on WSL forms like "wsl gpg2" it will try to run host "gpgconf" instead of the WSL one and also doesn't consider Windows ".exe" siblings; fix GenerateGPGKeys by detecting command prefixes and platform: if gpgPath startsWith("wsl ") or otherwise contains a space treat the prefix (e.g., "wsl") as the launcher and build gpgconfPath by prefixing that launcher (e.g., "wsl gpgconf"); on Windows, when deriving sibling gpgconf use the same base name but with ".exe" (check for gpgconf.exe and isExecutable via QFileInfo) before falling back to plain "gpgconf"; then call Executor::executeBlocking with the constructed launcher-aware gpgconfPath.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/pass.cpp`:
- Around line 212-219: The code assumes gpgPath is a native filesystem path
(gpgPath, gpgconfPath, QFileInfo) so on WSL forms like "wsl gpg2" it will try to
run host "gpgconf" instead of the WSL one and also doesn't consider Windows
".exe" siblings; fix GenerateGPGKeys by detecting command prefixes and platform:
if gpgPath startsWith("wsl ") or otherwise contains a space treat the prefix
(e.g., "wsl") as the launcher and build gpgconfPath by prefixing that launcher
(e.g., "wsl gpgconf"); on Windows, when deriving sibling gpgconf use the same
base name but with ".exe" (check for gpgconf.exe and isExecutable via QFileInfo)
before falling back to plain "gpgconf"; then call Executor::executeBlocking with
the constructed launcher-aware gpgconfPath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: d8980ab5-c91f-4992-b892-bbc0a4e17a92
📒 Files selected for processing (1)
src/pass.cpp
annejan
approved these changes
Apr 7, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #895 +/- ##
==========================================
+ Coverage 25.58% 25.80% +0.21%
==========================================
Files 38 38
Lines 3170 3205 +35
==========================================
+ Hits 811 827 +16
- Misses 2359 2378 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Contributor
ReviewThe fix looks solid. A few observations: pass.cpp changes:
Test additions:
Minor suggestions:
Overall, LGTM. The change addresses issue #815 by more reliably terminating stale GPG agents. |
- Add resolveGpgconfCommand() helper for path resolution - Handle WSL wrappers (wsl gpg2, wsl -d Ubuntu gpg2) - Detect shell commands (wsl sh -c) and fallback to PATH - Check .exe extension on Windows - Add unit tests for all path cases - Resolves CodeRabbit review finding
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/auto/executor/tst_executor.cpp`:
- Around line 164-170: The test tst_executor::executeBlockingGpgKillAgent
assumes gpgconf is on PATH; update it to detect missing gpgconf and skip the
test when absent (like the gpgSupportsEd25519 test does): before calling
Executor::executeBlocking("gpgconf", ...), use Qt's findExecutable
(QStandardPaths::findExecutable("gpgconf")) or equivalent to check presence and
call QSKIP("gpgconf not available") if not found, so the test only runs when
gpgconf is available.
- Around line 199-202: The test currently assumes a system /usr/bin/gpgconf
exists; instead make it filesystem-independent by creating a temporary
directory, writing two dummy files named "gpg2" and "gpgconf" there, set both
executable (QFile::setPermissions or QFileDevice + QProcess::start to touch),
and call Pass::resolveGpgconfCommand with the temporary "gpg2" path and assert
it returns the sibling "gpgconf"; update the test in tst_executor.cpp to remove
the hardcoded "/usr/bin/gpg2" and clean up the temp files after the assertion.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: f63efa75-af6d-431c-9b38-997aeb6b686e
📒 Files selected for processing (3)
src/pass.cppsrc/pass.htests/auto/executor/tst_executor.cpp
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* Translated using Weblate (Estonian) Currently translated at 100.0% (215 of 215 strings) Translation: QtPass/QtPass Translate-URL: https://hosted.weblate.org/projects/qtpass/qtpass/et/ * Translated using Weblate (Russian) Currently translated at 98.1% (211 of 215 strings) Translation: QtPass/QtPass Translate-URL: https://hosted.weblate.org/projects/qtpass/qtpass/ru/ --------- Co-authored-by: Priit Jõerüüt <jrthwlate@users.noreply.hosted.weblate.org> Co-authored-by: Сергей <asvmail.as@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pass.cpp`:
- Around line 257-260: The blocking gpgconf call uses Executor::executeBlocking
which creates a new QProcess without the instance GNUPGHOME, so gpgconf --kill
gpg-agent may hit the wrong socket; replace the
Executor::executeBlocking(gpgconfCommand, {"--kill", "gpg-agent"}) call with the
instance method Pass::executeWrapper(...) (or otherwise pass the instance
executor environment) so the same environment set by
Pass::init()/Pass::updateEnv() (including GNUPGHOME) is used for the gpgconf
--kill invocation; keep the same command built by resolveGpgconfCommand(gpgPath)
and the same arguments ("--kill", "gpg-agent") when switching to executeWrapper.
- Around line 209-218: The WSL handling in the function that currently returns a
flattened command string (used by GenerateGPGKeys() and passed into
Executor::executeBlocking()/startProcessBlocking()) breaks argument boundaries;
change the implementation so WSL invocations are returned as a program plus
argument list (e.g., keep "wsl" as the program and return the remaining parts as
a QStringList of args, with parts.last() replaced by "gpgconf" when appropriate)
instead of parts.join(' '), update the caller
(GenerateGPGKeys()/Executor::executeBlocking()) to accept program + args (or a
QStringList) and pass them into startProcessBlocking() without collapsing them
into a single quoted argument so flags like -d and subsequent commands remain
separate.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 620f2bb8-8eb4-4aab-9a76-119cb477b792
📒 Files selected for processing (5)
localization/localization_et_EE.tslocalization/localization_ru_RU.tssrc/pass.cppsrc/pass.htests/auto/executor/tst_executor.cpp
- Return ResolvedGpgconfCommand struct instead of flattened string - Preserve WSL arguments as separate items to avoid shell quoting issues - Use same environment (GNUPGHOME) as key generation for gpgconf --kill - Add executeBlocking overload that takes environment - Make Unix path test filesystem-independent using temp directory
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/executor.cpp`:
- Around line 284-305: The new Executor::executeBlocking(const QStringList&,
QString, const QStringList&, QString*, QString*) skips the existing blocking
contract (waitForStarted(), exitStatus() checks and UTF-8 decoding) used by the
other executeBlocking overload; update this overload to call the same
success/failure path: after startProcessBlocking(...) call waitForStarted() and
check QProcess::ExitStatus similarly to the other executeBlocking
implementation, use the same UTF-8 decoding logic when populating
process_out/process_err, and return the same error/success codes as the other
overload so launch failures, crashes and captured output are handled
identically.
In `@src/pass.cpp`:
- Around line 212-220: The current WSL handling in src/pass.cpp replaces the
final token with "gpgconf" even when the command is a shell wrapper (e.g., "wsl
/bin/sh -c gpg2"), causing later arguments like "--kill gpg-agent" to be
dropped; update the logic that examines first and parts (the branch handling
first == "wsl" || first == "wsl.exe") to detect common shell-wrapper patterns
(presence of shell names like "sh", "bash" anywhere in parts and shell flags
such as "-c", "-lc", "--command", or an intervening "/bin/sh" token) before the
code that rewrites parts.last(); when such a pattern is found, return early
(e.g., return {"gpgconf", {}} or another appropriate early-return) instead of
removing/appending tokens so GenerateGPGKeys that appends "--kill gpg-agent"
won’t leave arguments outside the shell -c invocation.
- Around line 204-209: The function signature for ResolvedGpgconfCommand
Pass::resolveGpgconfCommand should use a trailing return type to satisfy
modernize-use-trailing-return-type, and the call to QProcess::splitCommand must
be guarded for Qt versions older than 5.15: change the declaration of
resolveGpgconfCommand to use a trailing return type (e.g., auto
Pass::resolveGpgconfCommand(...) -> ResolvedGpgconfCommand) and wrap the use of
QProcess::splitCommand inside an `#if` QT_VERSION >= QT_VERSION_CHECK(5,15,0) /
`#else` block so that for older Qt you fall back to a compatible split routine (or
simple QStringList containing the original gpgPath) while keeping the existing
early-return branch intact.
In `@tests/auto/executor/tst_executor.cpp`:
- Around line 220-241: The test assumes Unix sibling names but the resolver
Pass::resolveGpgconfCommand prefers gpgconf.exe on Windows; update the test to
be OS-aware by either guarding the whole block to run only on non-Windows (e.g.,
exclude Q_OS_WIN) or, when running on Windows, create and set executable
permissions for "gpgconf.exe" (and assert result.program equals that path)
instead of "gpgconf"; reference the Pass::resolveGpgconfCommand call in the test
and adjust the created filename and assertion accordingly so the expectation
matches platform-specific resolver behavior.
- Around line 164-176: Update tst_executor::executeBlockingGpgKillAgent to also
call the env-aware overload Executor::executeBlocking(env, ...) so the GNUPGHOME
propagation is exercised: create a controlled QProcessEnvironment (or
QHash<QString, QString> / QStringList depending on overload) named testEnv that
sets GNUPGHOME to a known path (e.g., a temp dir or "TEST_GNUPGHOME"), invoke
Executor::executeBlocking(testEnv, "gpgconf", {"--kill", "gpg-agent"},
QString(), &output, &err), and add a QVERIFY/QCOMPARE asserting the returned
result is 0 (or skip if not available) to ensure the env-taking overload behaves
the same as the legacy call. Ensure the new assertion is adjacent to the
existing legacy invoke in tst_executor::executeBlockingGpgKillAgent so
regressions in env propagation are caught.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: ca126ea3-80c7-4c6c-8692-705946e02138
📒 Files selected for processing (5)
src/executor.cppsrc/executor.hsrc/pass.cppsrc/pass.htests/auto/executor/tst_executor.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pass.cpp (1)
204-209:⚠️ Potential issue | 🟠 MajorGuard the Qt 5.15-only API here and match the repo's trailing-return rule.
The signature already trips
modernize-use-trailing-return-type, and unless the minimum supported Qt was raised to 5.15, the unguardedQProcess::splitCommand()call is inconsistent with the compatibility branches still present at Lines 293-299.Minimal syntax fix
-ResolvedGpgconfCommand Pass::resolveGpgconfCommand(const QString &gpgPath) { +auto Pass::resolveGpgconfCommand(const QString &gpgPath) + -> ResolvedGpgconfCommand {Verify whether the build still targets pre-5.15 Qt; if it does, this helper needs the same version guard as the other 5.15-only code in this file.
#!/bin/bash set -euo pipefail printf 'Helper definition and splitCommand usage:\n' sed -n '204,210p' src/pass.cpp printf '\nQt 5.15-specific guards already present in this file:\n' rg -n 'QProcess::splitCommand|QT_VERSION_CHECK\(5, 15, 0\)' src/pass.cpp || true printf '\nQt version constraints from build files:\n' rg -n 'find_package\(Qt|QT_VERSION_CHECK|QT_MINIMUM_VERSION|greaterThan\(QT_MAJOR_VERSION' \ -g 'CMakeLists.txt' -g '*.pro' -g '*.pri' -g '*.cmake' || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pass.cpp` around lines 204 - 209, The helper Pass::resolveGpgconfCommand uses the Qt 5.15-only QProcess::splitCommand unsafely and its signature should follow the repo's trailing-return rule; change the function declaration to use a trailing return (e.g. auto Pass::resolveGpgconfCommand(const QString &gpgPath) -> ResolvedGpgconfCommand) and wrap the QProcess::splitCommand call with a Qt version guard (`#if` QT_VERSION >= QT_VERSION_CHECK(5,15,0) / `#else` / `#endif`), providing a compatible fallback (e.g. a simple QStringList fallback such as treating gpgPath as a single entry or a basic space-split) inside the else branch so builds targeting pre-5.15 Qt continue to compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pass.cpp`:
- Around line 216-220: The check should match the basename of the last token,
not the raw token string; replace parts.last().startsWith("gpg") with a basename
check using QFileInfo(parts.last()).fileName().startsWith("gpg") so wrappers
like "wsl /usr/bin/gpg2" are detected; keep the same removal/append logic
(parts.removeLast(); parts.append("gpgconf"); return {parts.first(),
parts.mid(1)}).
---
Duplicate comments:
In `@src/pass.cpp`:
- Around line 204-209: The helper Pass::resolveGpgconfCommand uses the Qt
5.15-only QProcess::splitCommand unsafely and its signature should follow the
repo's trailing-return rule; change the function declaration to use a trailing
return (e.g. auto Pass::resolveGpgconfCommand(const QString &gpgPath) ->
ResolvedGpgconfCommand) and wrap the QProcess::splitCommand call with a Qt
version guard (`#if` QT_VERSION >= QT_VERSION_CHECK(5,15,0) / `#else` / `#endif`),
providing a compatible fallback (e.g. a simple QStringList fallback such as
treating gpgPath as a single entry or a basic space-split) inside the else
branch so builds targeting pre-5.15 Qt continue to compile.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7b39836e-ab59-465a-954c-6c8794346328
📒 Files selected for processing (1)
src/pass.cpp
- Use trailing return type to match repo style - Add Qt 5.15 version guard for QProcess::splitCommand
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/pass.cpp (1)
217-225:⚠️ Potential issue | 🟠 MajorReturn a complete kill command for shell-backed WSL setups.
This still breaks common WSL forms like
wsl /bin/sh -c gpg2andwsl bash -lc gpg2: the fullgpgconf --kill gpg-agentpayload has to stay inside the-cargument, but here--kill/gpg-agentare appended afterwards. The same branch also misseswsl /usr/bin/gpg2because it matches the raw token instead of its basename. In both cases the WSL-side agent is left running, so the original lock timeout can still reproduce. Consider making the resolver return the complete kill invocation for shell-backed commands and matching the target executable by basename.Also applies to: 266-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pass.cpp` around lines 217 - 225, The WSL branch incorrectly appends "--kill gpg-agent" outside a shell -c argument and matches the full token rather than the basename, leaving the agent running; update the logic in the block that checks first == "wsl" / "wsl.exe" (and the analogous block at 266-268) to (1) detect shell-backed invocations by checking if the second token is a shell (startsWith "sh", "bash", or their paths) and if a "-c" / "-lc" style argument is present, then return a single command where the shell's -c argument contains the full payload "gpgconf --kill gpg-agent" (so the entire kill invocation stays inside the -c string), and (2) when matching the target executable token use its basename (strip path like "/usr/bin/gpg2") to compare against "gpg" / "gpg2" and only then replace the executable with "gpgconf" (or adjust args) for non-shell cases; update the return shapes accordingly where the code currently returns {"gpgconf", {}} or mutates parts (parts.first, parts.mid, removeLast/append) so the produced argv executes the kill on the WSL side correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/pass.cpp`:
- Around line 217-225: The WSL branch incorrectly appends "--kill gpg-agent"
outside a shell -c argument and matches the full token rather than the basename,
leaving the agent running; update the logic in the block that checks first ==
"wsl" / "wsl.exe" (and the analogous block at 266-268) to (1) detect
shell-backed invocations by checking if the second token is a shell (startsWith
"sh", "bash", or their paths) and if a "-c" / "-lc" style argument is present,
then return a single command where the shell's -c argument contains the full
payload "gpgconf --kill gpg-agent" (so the entire kill invocation stays inside
the -c string), and (2) when matching the target executable token use its
basename (strip path like "/usr/bin/gpg2") to compare against "gpg" / "gpg2" and
only then replace the executable with "gpgconf" (or adjust args) for non-shell
cases; update the return shapes accordingly where the code currently returns
{"gpgconf", {}} or mutates parts (parts.first, parts.mid, removeLast/append) so
the produced argv executes the kill on the WSL side correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7ddabc56-5a6c-4103-8132-18f0ea5a99e5
📒 Files selected for processing (2)
src/pass.cppsrc/pass.h
Use QFileInfo to extract basename from path when checking for gpg binary, and preserve the directory path when constructing gpgconf command. Add test for WSL with full path case.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
tests/auto/executor/tst_executor.cpp (2)
228-249:⚠️ Potential issue | 🟠 MajorMake the temp-dir absolute-path case match the Windows resolver branch.
Under
Q_OS_WIN,Pass::resolveGpgconfCommand(const QString &gpgPath)looks forgpgconf.exefirst. This block only creates and asserts an extensionlessgpgconf, so Windows runs will fall back to"gpgconf"and fail the assertion.♻️ Suggested fix
- QString gpgPath = tempDir.filePath("gpg2"); - QString gpgconfPath = tempDir.filePath("gpgconf"); + QString gpgPath = tempDir.filePath( +#ifdef Q_OS_WIN + "gpg2.exe" +#else + "gpg2" +#endif + ); + QString gpgconfPath = tempDir.filePath( +#ifdef Q_OS_WIN + "gpgconf.exe" +#else + "gpgconf" +#endif + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/auto/executor/tst_executor.cpp` around lines 228 - 249, The test creates a temp gpgconf file without a .exe extension, which mismatches the Windows resolver behavior in Pass::resolveGpgconfCommand that prefers "gpgconf.exe"; update the test to create a platform-matching gpgconf name (on Windows create "gpgconf.exe", on other OS keep "gpgconf") or create both names so the resolver finds the expected sibling; modify the gpgconfPath variable/creation logic in the test block (the QString gpgconfPath and subsequent QFile gpgconfFile setup) to choose/add the ".exe" suffix when Q_OS_WIN so the assertion comparing result.program to gpgconfPath passes.
164-176: 🧹 Nitpick | 🔵 TrivialExercise the env-aware overload that the production fix uses.
Pass::GenerateGPGKeys(QString batch)now goes throughExecutor::executeBlocking(env, gpgconf.program, killArgs), but this regression test still only covers the legacy overload. AGNUPGHOMEpropagation regression would not be caught here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/auto/executor/tst_executor.cpp` around lines 164 - 176, The test currently calls the legacy Executor::executeBlocking overload and must exercise the env-aware overload used by Pass::GenerateGPGKeys; update tst_executor::executeBlockingGpgKillAgent to create a QProcessEnvironment (e.g. QProcessEnvironment::systemEnvironment() or a tailored env containing GNUPGHOME) and call Executor::executeBlocking(env, gpgconf.program, {"--kill","gpg-agent"}, QString(), &output, &err) instead, keeping the same result checks and QSKIP behavior.src/pass.cpp (1)
217-235:⚠️ Potential issue | 🟠 MajorBroaden the WSL shell-wrapper guard before rewriting the command.
Line 218 only catches
wsl sh .... Inputs likewsl /bin/sh -c gpg2andwsl bash -lc gpg2still flow into theparts.last()rewrite, soPass::GenerateGPGKeys(QString batch)ends up runningwsl ... -c gpgconf --kill gpg-agentwith--kill gpg-agentoutside the shell command. The stale-agent cleanup still fails for those valid wrapper forms.♻️ Suggested fix
if (first == "wsl" || first == "wsl.exe") { - if (parts.size() >= 2 && parts.at(1).startsWith("sh")) { + bool shellWrapped = false; + for (int i = 1; i < parts.size(); ++i) { + const QString token = QFileInfo(parts.at(i)).fileName(); + if (token == "sh" || token == "bash") { + shellWrapped = parts.contains("-c") || parts.contains("-lc") || + parts.contains("--command"); + if (shellWrapped) { + break; + } + } + } + if (shellWrapped) { return {"gpgconf", {}}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pass.cpp` around lines 217 - 235, The guard that detects WSL shell wrappers is too narrow (only checks parts.at(1).startsWith("sh")), causing commands like "wsl /bin/sh -c ..." or "wsl bash -lc ..." to fall through and break the gpgconf rewrite; update the condition in the WSL branch (the block using variables first and parts) to detect common shell wrappers more broadly (e.g. match "/bin/sh", "sh", "sh.exe", "bash", "bash.exe" and also detect "-c" or "-lc" usage) and return {"gpgconf", {}} for those cases; adjust the check in the same if (first == "wsl" || first == "wsl.exe") block (used by Pass::GenerateGPGKeys) so any recognized shell wrapper path or shell + "-c"/"-lc" combination prevents the subsequent parts.last() gpg->gpgconf rewrite.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/pass.cpp`:
- Around line 217-235: The guard that detects WSL shell wrappers is too narrow
(only checks parts.at(1).startsWith("sh")), causing commands like "wsl /bin/sh
-c ..." or "wsl bash -lc ..." to fall through and break the gpgconf rewrite;
update the condition in the WSL branch (the block using variables first and
parts) to detect common shell wrappers more broadly (e.g. match "/bin/sh", "sh",
"sh.exe", "bash", "bash.exe" and also detect "-c" or "-lc" usage) and return
{"gpgconf", {}} for those cases; adjust the check in the same if (first == "wsl"
|| first == "wsl.exe") block (used by Pass::GenerateGPGKeys) so any recognized
shell wrapper path or shell + "-c"/"-lc" combination prevents the subsequent
parts.last() gpg->gpgconf rewrite.
In `@tests/auto/executor/tst_executor.cpp`:
- Around line 228-249: The test creates a temp gpgconf file without a .exe
extension, which mismatches the Windows resolver behavior in
Pass::resolveGpgconfCommand that prefers "gpgconf.exe"; update the test to
create a platform-matching gpgconf name (on Windows create "gpgconf.exe", on
other OS keep "gpgconf") or create both names so the resolver finds the expected
sibling; modify the gpgconfPath variable/creation logic in the test block (the
QString gpgconfPath and subsequent QFile gpgconfFile setup) to choose/add the
".exe" suffix when Q_OS_WIN so the assertion comparing result.program to
gpgconfPath passes.
- Around line 164-176: The test currently calls the legacy
Executor::executeBlocking overload and must exercise the env-aware overload used
by Pass::GenerateGPGKeys; update tst_executor::executeBlockingGpgKillAgent to
create a QProcessEnvironment (e.g. QProcessEnvironment::systemEnvironment() or a
tailored env containing GNUPGHOME) and call Executor::executeBlocking(env,
gpgconf.program, {"--kill","gpg-agent"}, QString(), &output, &err) instead,
keeping the same result checks and QSKIP behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7694efcc-674c-4e1f-b541-bdc7f7ce9a64
📒 Files selected for processing (2)
src/pass.cpptests/auto/executor/tst_executor.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/auto/executor/tst_executor.cpp`:
- Around line 247-261: The assertion message for the Pass::resolveGpgconfCommand
check is platform-misleading and the QFile::setPermissions(..., ExeOwner) calls
are unnecessary on Windows; update the assertion text (referencing the result
check using result.program == gpgconfPath) to a platform-neutral message like
"Should derive sibling gpgconf path" and either remove or guard the
QFile::setPermissions(gpgPath, ...) and QFile::setPermissions(gpgconfPath, ...)
calls (or comment them) so they are not relied upon on Windows where
QFileInfo::isExecutable() uses file extensions rather than permission bits.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: acb161c8-f088-4ebd-83c7-9c046e7b9deb
📒 Files selected for processing (1)
tests/auto/executor/tst_executor.cpp
- Add waitForStarted() check with debug output - Add exitStatus() check for crash detection - Use decodeAssumingUtf8() for output decoding - Return -1 on failure to match existing pattern
This was referenced Apr 7, 2026
This was referenced Apr 17, 2026
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
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--kill gpg-agent(older GPG) andgpgconf --kill gpg-agent(newer GPG)Fixes #815
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Localization