Repository navigation
fix(session): opt-in direct AT-SPI start when its activation is denied - #95
Conversation
…nied On Fedora with SELinux enforcing, a virtual session had no accessibility: the policy denies the D-Bus activation of at-spi-bus-launcher from kwin-mcp's private dbus-run-session bus (fedora-selinux/selinux-policy#3385), and the registry's activation from the accessibility bus is denied the same way, so AT-SPI2 queries failed (AT-SPI2 worker exited). When the activation fails, the wrapper now starts the launcher and then the registry directly, from the Exec= lines of the same service files, and waits up to 5 s of elapsed time for each to own its bus name. They run unconfined, as the desktop session's own launcher does. session_start reports the fallback in its Warning line. If the direct start cannot complete, the warning is the previous one and the half-started launcher is stopped. The direct start adds at most 17 s to a startup whose activation already failed.
|
Issue agent stopped automated processing of this pull request and needs an operator.
Reason: To retry after fixing the cause, an operator calls the bridge op Every push to this pull request is reviewed automatically. To request another review without pushing, after replying to the findings, comment |
haechibot
Bot
added
the
agent:needs-attention
|
@haechibot review |
![haechibot[bot]](https://avatars.githubusercontent.com/in/5118831?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: GREEN (PR-ready: true) reviewed at 7080337. No blocking findings.
The fallback recovers from simulated activation denial, makes KCalc visible through AT-SPI, and removes the half-started bus when registry startup fails. Independent runtime, test, and challenger reviews found no actionable regression.
Verified
- Ran the current source in an existing Debian KWin test image with Python 3.13.5: all four focused accessibility-bus lifecycle tests passed, covering healthy activation, fallback recovery, failed address retrieval, and registry-failure cleanup.
- The activation-denied regression test fails with merge-base af20dd9's session implementation: it reports accessibility unavailable instead of recovering.
- All reported CI checks passed; publishing was skipped. GitHub reports the unchanged head as mergeable.
Not verified
- Fedora with SELinux enforcing was not exercised locally; the local denial was simulated.
- The fresh installed-package Docker build was interrupted. Local execution used the current source mounted into an existing test image.
Every push to this pull request is reviewed automatically. To request another review without pushing, after replying to the findings, comment @haechibot review. The same comment restarts a review that stopped with an error. The pull request author or a maintainer can request it.
bulgasaribot
Bot
removed
the
agent:needs-attention
# Conflicts: # CHANGELOG.md
|
Issue agent stopped automated processing of this pull request and needs an operator.
Reason: To retry after fixing the cause, an operator calls the bridge op Every push to this pull request is reviewed automatically. To request another review without pushing, after replying to the findings, comment |
haechibot
Bot
added
the
agent:needs-attention
# Conflicts: # README.md # src/kwin_mcp/session.py
The startup bound comment claimed the wrapper always reports before the 60 s parent deadline, but its steps (including the 30 s KWin bus-name wait and the switch Set) can add up past it. The cleanup and activation comments now cover the direct start of the launcher and registry.
There was a problem hiding this comment.
Verdict: GREEN (PR-ready: false: E2E checks pending) re-reviewed at caee67a. No blocking findings.
Previous findings
None. The previous review approved 7080337 without findings; there are no review threads to answer.
New findings
None. The fallback correctly integrates with the accessibility switch: successful recovery enables IsEnabled on the private bus before apps launch; failed recovery skips that write. Independent review found no actionable regression.
Verified
- Re-ran all four focused accessibility-bus lifecycle tests against this head's source in an existing Debian KWin test image: 4 passed. This includes the new IsEnabled assertion, KCalc accessibility after activation denial, healthy activation, and cleanup after fallback failure.
- Lint, type-check, build, and documentation checks passed. GitHub reports this head as mergeable. Seven E2E jobs remain in progress.
Not verified
Fedora with SELinux enforcing and a fresh installed-package build were not exercised locally; local tests simulated activation denial and mounted the current source into an existing runtime image.
Every push to this pull request is reviewed automatically. To request another review without pushing, after replying to the findings, comment @haechibot review. The same comment restarts a review that stopped with an error. The pull request author or a maintainer can request it.
bulgasaribot
Bot
removed
the
agent:needs-attention
# Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Verdict: GREEN (PR-ready: false: E2E CI failing/pending) re-reviewed at 8151930. No blocking findings in the diff.
Previous findings
None. Both prior reviews approved without findings, and there are no review threads to answer.
New findings
None. This head merges upstream main. The fallback implementation and lifecycle tests are unchanged from caee67a; independent review found no conflict with the upstream accessibility window-matching changes.
Verified
- Re-ran the four focused accessibility-bus lifecycle tests against this head's source in an existing Debian KWin runtime: 4 passed, including accessibility-switch enablement, KCalc visibility after activation denial, and failure cleanup.
- Lint, type-check, build, and documentation checks passed. GitHub reports the head as mergeable.
- One openSUSE amd64 E2E job failed at Build the KWin test image; its quality checks and tests were skipped. Other E2E jobs remain in progress.
Not verified
The image-build failure's cause is unknown: GitHub CLI could not retrieve its logs while the workflow remained in progress. Fedora with SELinux enforcing and a fresh installed-package build were not exercised locally; local tests used simulated denial and mounted source.
Every push to this pull request is reviewed automatically. To request another review without pushing, after replying to the findings, comment @haechibot review. The same comment restarts a review that stopped with an error. The pull request author or a maintainer can request it.
|
Thanks for this, @superbiche. The Fedora diagnosis is clear, and I can see how much it hurts to have no accessibility in virtual sessions there. I'm holding off on merging for now, for two reasons. 1. It goes around an SELinux denial. When activation is refused, the fallback runs 2. Your latest commits ( |
…D-Bus does The direct start of the AT-SPI launcher and registry runs them in kwin-mcp's own SELinux domain instead of the gnome_atspi_t domain the activation would give them. It now runs only with KWIN_MCP_ATSPI_DIRECT_START=1; by default a failed activation is reported as before, and the warning names the opt-in. The launcher and registry commands were word-split by bash, so a quoted path with a space or a quoted argument broke. kwin_mcp.dbus_service_file ports dbus-daemon's service file reader (desktop-file unescaping, then _dbus_shell_parse_argv), and the wrapper runs the resulting argv as bash arrays. Its tests carry argv observed from dbus-daemon 1.16 activation. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…unch # Conflicts: # CHANGELOG.md
The direct start read any service file at the expected path. dbus-daemon ignores a file over 128 KiB or not valid UTF-8, and activates a service by its Name= key, so a file without the requested Name never starts it. The direct start now applies the same rules. The opt-in is read once per wrapper build. Co-Authored-By: Claude Opus 5.5 <[email protected]>
There was a problem hiding this comment.
Verdict: GREEN (PR-ready: false: E2E CI pending) re-reviewed at 1d3228e. No blocking findings.
Previous findings
Prior bot reviews contained no findings, and there are no review threads to answer. The maintainer's subsequent comment raised two concerns:
- Closed: automatic direct startup after an SELinux denial. The fallback now requires KWIN_MCP_ATSPI_DIRECT_START=1 in the server environment; session extra_env cannot enable it. Documentation explains the domain change.
- Closed: incorrect Exec= splitting. Service files are now parsed into argument arrays, preserving quoted paths and arguments without shell expansion. Invalid service files are rejected.
New findings
None. Independent parser and lifecycle reviews found no actionable defect.
Verified
- Ran the parser tests and focused lifecycle tests against this head's source in an existing Debian KWin runtime: 44 passed. Coverage includes quoted commands, malformed service files, default-off behavior, opt-in recovery with KCalc visibility, accessibility-switch enablement, and failure cleanup.
- Lint, type-check, build, and documentation CI checks passed. GitHub reports this head as mergeable; seven E2E jobs remain in progress.
Not verified
Fedora with SELinux enforcing and a fresh installed-package build were not exercised locally. Local execution used mounted source and simulated activation denial; the parser fixtures' claimed upstream observations were not independently reproduced against dbus-daemon.
Every push to this pull request is reviewed automatically. To request another review without pushing, after replying to the findings, comment @haechibot review. The same comment restarts a review that stopped with an error. The pull request author or a maintainer can request it.
|
Thanks for the careful look. Both points are fair, and both are addressed in f589ab1 and 1d3228e, with main (0.11.0) merged in b13e2ee. 1. Opt-in. The direct start now runs only with On the desktop's own launcher: on a stock Fedora 44 KDE install (VM, selinux-policy-targeted 44.11, enforcing), My Fedora 44 Plasma workstation shows the same labels. With the opt-in, the directly started launcher and registry get exactly these labels, and they are gone after 2. Testing on 1d3228e:
@superbiche · user · drafted with Claude Opus 5.5, reviewed before posting; the voice and decisions are mine. |
superbiche
changed the title
|
@isac322 the review of
Reply on the pull request, then comment |
|
The original default-on fallback and Exec parsing concerns are technically addressed. I am retaining the documented operator-only opt-in and keeping this pull request unmerged. This pass covers fixes and branch updates only; merge authorization remains a separate maintainer decision. Please evaluate technical code readiness separately from permission to merge. No hold release, merge, or check bypass is requested. @haechibot review |
There was a problem hiding this comment.
Verdict: OPEN re-reviewed at 12db7f0. One new P2 ownership defect needs correction; the maintainer's merge hold remains a separate open item.
Previous findings
- Closed: P2 unbounded failed-bootstrap cleanup.
src/kwin_mcp/session.py:1197-1214now shares a 3-second TERM grace and a 2-second KILL grace across owned jobs and performs no unbounded wait. - Closed: P2 service lookup outside the startup watchdog.
session.py:1124-1129constructs only the helper pathname; filesystem acquisition runs in children at1234,1240, after failed activation at1254-1260. - Closed technically: default-on startup after an SELinux denial. The server must explicitly set the exact opt-in value
1atsession.py:1124; application environment overrides cannot enable it. - Closed technically: incorrect
Exec=splitting.src/kwin_mcp/dbus_service_file.py:63-232follows D-Bus desktop-value and argv parsing, and282executes the resulting argv directly. - Closed: acceptance of the documented operator-only opt-in. isac322, whose repository admin permission was verified, explicitly retained it in the latest maintainer response.
- Open: the same response expressly retains the merge hold and reserves merge authorization for a separate decision.
The change supplies an operator-selected workaround for the distribution policy denial; it does not change SELinux policy. The two previous fixes are supported by independently challenged source-path arguments. A final Bash-option probe exposed a new ownership hazard.
Fix hazards
- This head applies the fix suggested in the cleanup finding. The grace periods are bounded, but the new ownership argument assumes Bash job control is off (
session.py:1178-1179) without enforcing it. Inherited monitor mode changes helper process groups and native jobspec signaling; this produced the new finding below. Explicitly disabling monitor mode before any background launch adds no blocking I/O or lock. - This head applies the fix suggested in the lookup finding. All new stat/open/read operations are child-side (
dbus_service_file.py:235-262), outside the registry lock and parent wrapper construction. Spawn registration remains serialized with close (session.py:294,317-329), but the newly added helpers inherit that registered group only with job control disabled. Individual D-Bus reply limits do not cover connection/authentication; the outer handshake deadline at1047-1048remains the final caller bound.
New findings
- P2
src/kwin_mcp/session.py:1234: inheritedSHELLOPTS=monitorenables job control inbash -c. The new launcher/registry helpers then enter separate process groups, while the registry tracks only the wrapper group (725-731) and the wrapper trap tracks only KWin (1146-1150). If startup is cancelled before direct-start rollback, a TERM-ignoring helper can survive group teardown._build_envpreserves this option (1364), and1178only comments that job control should be off. The native cleanup's dead-member safety proof also depends on its non-job-control branch. Explicitly runset +mat wrapper startup, before any background jobs, and cover cancellation with inherited monitor mode. Evidence: the targeted Bash probe below, plus the independently checked ownership path. This is a hazard of the fixes linked above.
Open items
- Maintainer hold by isac322 remains in place; merge authorization is reserved for a separate maintainer decision.
Verified
- Reviewed the full merge-base diff and complete changed files against
mainat dfcc4ea. The pinned head includes that base, and the worktree is clean. - Ran targeted probes in an official
bash:5.2scratch container withdocker run --rm --network none. Under default options, the literal role-job marker was preserved, a TERM-ignoring job disappeared after the observed 3-second grace, and a deliberately stale numeric target left a neighbor alive after bounded polling. With literalSHELLOPTS=monitor, Bash reported monitor on and the background child had a different process group from the shell. The control withset +mreported monitor off and both shared the shell's group. These are Bash probes, not complete KWin session tests. - Compared parser branches with upstream D-Bus 1.16.2 source and cleanup ownership with Bash 5.2 primary source. Read the linked SELinux issue and its scope correction.
- Applied base conventions from
CLAUDE.md,CONTRIBUTING.md,.claude/agents/docs-seo.md,.claude/skills/check-docs-seo/SKILL.md,.claude/skills/release-notes/SKILL.md,.claude/positioning.yml, the documentation hook/settings, PR template, all three workflow definitions,docs/design/*.md, andROADMAP.md. Module/test inventories, release notes and the Conventional Commit title are synchronized. The change adds no tool or shared numeric registry entry.
Not verified
Fedora with SELinux enforcing, GUI accessibility, live NFS/FUSE stalls, kernel D-state disappearance, and long-running suites were not exercised locally. Their runtime claims were assessed through the specific source paths and independent challenges below. No end-to-end suite or PR container image was built or run, and CI results did not determine this verdict.
Lenses
- correctness, always: normal activation remains at
session.py:1254; the exact opt-in is1124; successful recovery enables accessibility at1272-1274; failed recovery preserves the warning at1264-1266. Launcher preflight and registry reacquisition aredbus_service_file.py:271-279. These paths address both previous defects by argument+challenger(/root/wrapper_12db); cancellation ownership under inherited monitor mode is the new finding. The new stdlib script supports the declared Python 3.12 minimum. - concurrency/lifecycle, triggered by child creation and rollback at
session.py:1234,1240,1245: spawn/close serialization is294,317-329; TERM/KILL grace gaps are1200-1210; final group cleanup is767-789. No same-marker job replaces an exited role during rollback, and pending traps cannot run inside native kill's resolution gap. Default-mode reap/PID handling was covered by argument+challenger(/root/bootstrap_12db). The inherited-monitor cancellation near miss was confirmed by the Bash probe and the independent challenge; it is reported as the new finding. - concurrency/lifecycle, triggered by file acquisition and exec in
dbus_service_file.py:44,282: the acquired descriptor is validated at48-50and closed in58-60on every path; the helper adds no lock, thread, named artifact, or closeable shared registry. Path replacement is checked against the opened descriptor. Exec preserves the child's PID/group; whether that group is registered is addressed by the new finding. Covered by argument+challenger(/root/wrapper_12db). - input, triggered by the new server flag and helper roles:
session.py:1124leaves unset, empty, zero, boolean-looking and spaced values off. Public MCP/CLI applicationenvreaches app launch atcore.py:618, after startup; internal session overrides atsession.py:1413do not change the server gate. Helper argv accepts exactly one fixed role atdbus_service_file.py:268-269. Lookup defaults/skips and fail-closed permission/I/O behavior are237-262; size, UTF-8, Name and Exec checks are221-232. Covered by the independent input audit and argument+challenger(/root/wrapper_12db), including denied-first-root and invalidated-registry near misses. - injection, triggered by generated Bash at
session.py:1173and serviceExec=: the helper argv is encoded once withshlex.join, and quoted array expansions at1234,1240preserve literal arguments.-I -Sat1127suppresses Python startup customization;dbus_service_file.py:282-286uses direct execv without PATH lookup or implicit shell fallback. Child stdout is discarded before the handshake reader (session.py:1234,1240,1084-1095). Covered by argument+challenger(/root/wrapper_12db). - math, triggered by byte and time bounds:
dbus_service_file.py:26,51,54-56permits 131072 bytes and reads at most one extra sentinel byte to reject concurrent growth.session.py:1201,1207,1219uses seconds; reply limits at1221,1237use milliseconds. Whole-second rounding can shorten a grace by less than one second; cumulative waits and setup stalls remain subject to1048. No coordinate conversion or caller-provided numeric bound is introduced. Covered by the Bash probe and argument+challenger(/root/wrapper_12db). - test audit, triggered by all five changed test files: lifecycle tests check no parent lookup (
test_session_lifecycle.py:665-703), lookup stalls (708-768), registry invalidation (771-809), actual bus ownership/accessibility (942-1007), literal argv (1210-1274), and TERM-resistant rollback before fixture recovery (1289-1439). Exit assertions check original PID/start-time identities, pipe EOF, group disappearance and neighbor survival before finally cleanup (test_exit_cleanup.py:665-746). Parser, acquisition and execution-security tests check byte boundaries, same-fd replacement, literal arguments and inert hostile markers (test_dbus_service_file.py:54-135,test_atspi_service_acquisition.py:67-729,test_atspi_exec_security.py:159-523). Source analysis shows these assertions distinguish the corresponding missing fixes; predicates provide synchronization and no new type/lint suppressions were added. - docs/convention, always:
README.md:439-445,CHANGELOG.md:12-17,CONTRIBUTING.md:167-184,334-343, anddocker/README.md:194-208describe server-only opt-in, lazy lookup and confinement limits. Their registered-group ownership claim requires the monitor-mode fix above. The helper and three new tests appear in the inventories (CONTRIBUTING.md:236,308-310;docker/README.md:141-142,148). Applicable base sync triggers were checked; tool count remains 35.
Considered
- [concurrency/lifecycle]
session.py:1212a helper already sent SIGKILL during rollback resumes and executes; dismissed: argument+challenger(/root/bootstrap_12db), a pending fatal signal prevents return to user code. Cancellation that misses the helper's group is separately reported above. - [input]
dbus_service_file.py:46regular-to-FIFO replacement blocks acquisition; dismissed: argument+challenger(/root/wrapper_12db), nonblocking open and the same-fd regular check reject the replacement before read. - [math]
dbus_service_file.py:54growth after fstat permits unbounded allocation; dismissed: argument+challenger(/root/wrapper_12db), read(limit+1) caps bytes and the length check rejects growth. - [injection]
dbus_service_file.py:282parsed executable names or metacharacters trigger implicit shell evaluation; dismissed: argument+challenger(/root/wrapper_12db), literal execv bypasses shell dispatch and ENOEXEC has no fallback. - [concurrency/lifecycle]
dbus_service_file.py:271lazy acquisition still blocks the server before its watchdog; dismissed: argument+challenger(/root/wrapper_12db), acquisition is reachable only through the post-activation-failure child, while parent construction is lexical.
Every push to this pull request is reviewed automatically. To request another review without pushing, after replying to the findings, comment @haechibot review. The same comment restarts a review that stopped with an error. The pull request author or a maintainer can request it.
|
@isac322 the review of
Reply on the pull request, then comment |
|
The original default-on fallback and Exec parsing concerns are technically addressed. I am retaining the documented operator-only opt-in and keeping this pull request unmerged. This pass covers fixes and branch updates only; merge authorization remains a separate maintainer decision. Please evaluate technical code readiness separately from permission to merge. No hold release, merge, or check bypass is requested. @haechibot review |
There was a problem hiding this comment.
Verdict: OPEN re-reviewed at 27a43bd. The monitor ownership finding is fixed. One new parser correctness finding remains, alongside the retained maintainer hold.
Previous findings
- Closed: unbounded failed-bootstrap cleanup.
src/kwin_mcp/session.py:1199shares bounded TERM/KILL grace periods and performs no unconditional wait. - Closed: eager service-file lookup outside the startup watchdog.
src/kwin_mcp/session.py:1124constructs only a lexical helper command; filesystem acquisition runs in owned children at1236,1242after failed activation. - Closed: inherited monitor mode separates helpers from the registered process group.
src/kwin_mcp/session.py:1138disables job control before activation and every background launch. New cancellation assertions check actual child PGIDs, identities, pipe EOF and unrelated neighbors before recovery cleanup. - Closed technically: default-on confinement workaround. The fallback requires exactly
KWIN_MCP_ATSPI_DIRECT_START=1in the server environment atsrc/kwin_mcp/session.py:1124. - Closed technically: the original quoted-path and argument splitting defect. Arguments are parsed and executed directly through
src/kwin_mcp/dbus_service_file.py:210,282. The whole-file validation mismatch below is a separate finding. - Open: the maintainer hold. isac322's repository admin permission was verified; the comment explicitly retains the hold.
The change provides an operator-selected workaround for denied activation, while normal activation retains its existing path. It does not fix the upstream SELinux policy issue. Technical code readiness still requires the new parser correction below; merge authorization remains a separate maintainer decision.
New findings
Blocking
- P2
src/kwin_mcp/dbus_service_file.py:224: reject embedded NUL throughout the service file. Python's UTF-8 decode accepts NUL, and comments/localized entries skip value validation at104,113. Consequently the direct helper executes a file that D-Bus rejects entirely. A focused comparison with D-Bus 1.16.2 reproducedServiceUnknownand no execution for both ignored-line cases, while this head's actual helper executed their commands. Reject NUL before parsing and add both malformed-file cases. This check belongs on the already bounded byte buffer and requires no extra filesystem operations or locks.
Fix hazards
This head applies the fix suggested in the monitor ownership finding. Disabling monitor mode before spawning preserves the registered group across the launch-to-$! assignment window. It adds no lock or I/O. The earlier cleanup and lookup fixes retain bounded rollback and child-side acquisition. Parent containment uses the complete handshake deadline, including setup stalls; individual D-Bus reply timeouts alone do not provide that bound.
Open items
- Maintainer hold by isac322 remains in place; merge authorization is reserved for a separate maintainer decision.
The documented operator-only opt-in is retained as requested. I recommend correcting whole-file validation while keeping the existing hold; this review requests no merge or hold release.
Verified
- Read the full merge-base diff and all changed files at this head against base
dfcc4ea50a327a056e7cfc896d7314f5114d70d9, including all five changed test modules. Read PR history, review threads and the linked upstream issue. - A targeted Bash 5.2 probe ran with
docker run --rm --network none -e SHELLOPTS=monitor <probe-image>. The control created separate groups; the current prologue kept the helper in the wrapper group. Rollback removed a TERM-resistant owned role in 3 seconds, preserved its neighbor and did not signal a deliberately stale numeric target with no matching job. - A targeted Python 3.12 / D-Bus 1.16.2 comparison ran with
docker run --rm --network none <probe-image>. A valid service file executed through both paths. With NUL in a comment or localized entry, D-Bus rejected the file and the current direct helper executed it. - Repository conventions applied from the base branch:
CLAUDE.md,CONTRIBUTING.md, the PR template,.claudedocumentation guidance/settings/hook, workflow commands,docs/designandROADMAP.md. README and Docker warnings match the implementation; both test inventories include the new modules. No tool/version counter or generated-plugin synchronization is triggered. The PR title follows the Conventional Commit convention.
Not verified
- Full KWin/GUI suites, installed-wheel matrices and Fedora SELinux behavior were not run locally. Their behavior was assessed through current source paths and independently challenged arguments. The verdict does not depend on CI results.
- Actual hung network filesystems and uninterruptible processes were not exercised. Group ownership and deadline coverage were checked by source arguments; Bash rollback was exercised separately. Arbitrary operator startup scripts or service executables can deliberately change process ownership and remain trusted configuration.
Lenses
-
correctness: denied activation and healthy/default-off paths at
session.py:1256; launcher preflight and registry revalidation atdbus_service_file.py:271; original quoted argv handling at124,175,282. Covered by source arguments, independent challenges and the real D-Bus comparison. Whole-file NUL rejection is the new finding. -
concurrency/lifecycle, session module:
session.py:317guards spawn/registration against close;405,543preserve leaders through escalation;725,1138,1236contain launch/registration windows;1047bounds complete setup;1183,1199use native job signals and shared grace periods. Covered by the Bash probe and argument+challenger(/root/docs_8279). -
concurrency/lifecycle, helper module:
dbus_service_file.py:44,48,58assign, validate and finally close the acquired descriptor;282preserves PID/group through exec. There is no shared registry or lock interval. Covered by argument+challenger(/root/wrapper_12db). -
input/injection: server-only exact opt-in at
session.py:1124; isolated interpreter and quoted helper array at1127,1175; fixed role argv atdbus_service_file.py:268; root/error ordering at237,243,254; bounded UTF-8/name/Exec validation at221; direct exec at282. Covered by argument+challenger(/root/wrapper_12db) and the parser comparison, with the NUL exception reported. -
math/bounds:
dbus_service_file.py:26,51,54,55,221cover exact byte limits and growth after metadata inspection;session.py:1199,1221distinguish shared rollback graces from readiness polling and the outer deadline. Covered by the Bash probe and argument+challenger(/root/wrapper_12db). -
test audit:
test_exit_cleanup.py:607,705,739,748establish the actual installed-server option, child identity and PGID, then verify exit/EOF/group removal and neighbor survival;test_session_lifecycle.py:1006,1032,1217,1458cover success, healthy/default-off controls and rollback. Parser/acquisition/security tests check exact argv, descriptor races, markers and ownership before fixture recovery. Source audit, with the missing whole-file NUL cases identified. -
docs/convention: always triggered;
README.md:439,CHANGELOG.md:12,CONTRIBUTING.md:334anddocker/README.md:193agree on server opt-in, ownership, service roots and rollback limits; both test inventories list the new modules. Current title and multi-commit squash convention agree. Covered by the current full documentation/base-convention audit.
Considered
- [lifecycle]
session.py:1210a helper already sent SIGKILL resumes from stalled I/O and executes later; dismissed: argument+challenger(/root/bootstrap_12db), a delivered fatal signal prevents return to user code. Kernel task disappearance is not claimed; group containment remains registered. - [filesystem]
dbus_service_file.py:44replacement by a FIFO blocks acquisition; dismissed: argument+challenger(/root/wrapper_12db), nonblocking open and same-descriptor fstat reject the FIFO before read. - [bounds]
dbus_service_file.py:54growth after stat causes unbounded reads; dismissed: argument+challenger(/root/wrapper_12db), the fixed limit-plus-one read and post-read length check reject growth. - [injection]
dbus_service_file.py:282metacharacters, builtins, PATH or ENOEXEC invoke a shell; dismissed: argument+challenger(/root/wrapper_12db), execv uses literal argv/path and returns failure on OSError. - [parsing]
dbus_service_file.py:148escaped quotes, comment newlines or empty tokens alter argv; dismissed: argument+challenger(/root/wrapper_12db), odd-backslash tracking, two-stage unquoting and comment/final-token rules match the compared D-Bus branches. - [lifecycle]
dbus_service_file.py:271lookup stalls the parent before ownership/deadline establishment; dismissed: argument+challenger(/root/wrapper_12db), lookup is reached only in a helper child after failed activation. - [lifecycle]
session.py:1236a signal before role PID assignment leaks the helper; dismissed: argument+challenger(/root/docs_8279), the pre-registered group already contains the role with monitor disabled. - [lifecycle]
session.py:1193a stale numeric PID signals an unrelated process; dismissed: probe, numeric polling with an absent role marker left a live unrelated target untouched and completed within the shared grace bounds. - [lifecycle]
session.py:1194a role exits during native jobspec resolution and its recycled PID is signaled; dismissed: argument+challenger(/root/docs_8279), Bash's non-monitor branch skips completed job members; native reaping retains this ordinary async job in the resolution-to-signal gap.
Every push to this pull request is reviewed automatically. To request another review without pushing, after replying to the findings, comment @haechibot review. The same comment restarts a review that stopped with an error. The pull request author or a maintainer can request it.
|
@haechibot review Please re-review current head The maintainer merge hold remains unchanged. This requests technical re-review only, not merge or hold-release authorization. |
|
Issue agent stopped automated processing of this pull request and needs an operator.
Reason: To retry after fixing the cause, an operator calls the bridge op Every push to this pull request is reviewed automatically. To request another review without pushing, after replying to the findings, comment |
haechibot
Bot
added
the
agent:needs-attention
|
@haechibot review The previous review stopped with a model-provider stream error and did not submit a review. Please retry technical review of unchanged head The maintainer merge hold remains unchanged. This is not authorization to merge or release the hold. |
There was a problem hiding this comment.
Verdict: OPEN re-reviewed at ba77b8c. No remaining technical findings; the maintainer's merge hold remains in place.
Previous findings
- Closed: default-on direct activation workaround.
src/kwin_mcp/session.py:1124requires the exact server-side opt-in; session arguments cannot enable it. - Closed: quoted service commands losing argument boundaries.
src/kwin_mcp/dbus_service_file.py:210parses argv and:282executes it literally. The fresh D-Bus comparison preserved a quoted executable path and--label="two words". - Closed: unbounded failed-bootstrap cleanup (thread).
src/kwin_mcp/session.py:1199shares bounded TERM/KILL grace periods without an unconditional wait. - Closed: eager service lookup outside the startup watchdog (thread). Lookup now runs in owned helper children at
src/kwin_mcp/dbus_service_file.py:271,:272and:277. - Closed: inherited monitor mode separating helpers from session ownership (thread).
src/kwin_mcp/session.py:1138disables it before spawning helpers. - Closed: NUL bytes in ignored service-file lines (thread).
src/kwin_mcp/dbus_service_file.py:221rejects them before decoding or skipping comments/localized entries. Regression inputs attests/e2e/test_dbus_service_file.py:106and:110place the NUL after valid Name/Exec entries. - Open: the maintainer hold. isac322's latest request explicitly retains it.
The code addresses the earlier technical concerns. This remains an operator-controlled workaround for denied activation, rather than a fix to Fedora's SELinux policy. Technical review does not release the retained hold.
New findings
None.
Fix hazards
This head applies the fix suggested in the NUL finding. The new scan operates on the already bounded byte buffer, adds no I/O or lock, and precedes all parser branches. Removing the narrower value check is safe because both executable roles pass through this guard; UTF-8 decoding and the escape map cannot introduce NUL afterward. The fresh execution comparison below verifies both roles.
Open items
- Maintainer hold by isac322 remains in place; merge authorization is reserved for a separate maintainer decision.
Verified
- Re-read the complete merge-base diff and all changed files against main
6d047425ece3ed9b338c6b28257286ea0ec2b6be, including prior findings and discarded hypotheses. The merged uv_build upper-bound change matches main and does not interact with the AT-SPI runtime path. - Fresh targeted probe on official Python 3.12 with D-Bus 1.16.2:
docker run --rm --network none kwin-pr95-parser-probe:ba77-retry-quoted. Both prior helper roles executed service files containing NUL in comments/localized entries; both current roles rejected them without recorder execution, matching native D-Bus rejection. Valid files and both NUL-free controls preserved exact argv, including the executable path containing spaces. The native control records execution, not successful bus-name acquisition. - Fresh Bash 5.2 probe:
docker run --rm --network none -e SHELLOPTS=monitor kwin-pr95-bash-probe:ba77-retry. The control separated background process groups; the probe using the wrapper's currentset +mkept its helper in the owned group. Escalation removed a TERM-resistant helper in three seconds, preserved a neighbor, and did not signal a stale numeric target lacking the role marker. - Applied base conventions from
CLAUDE.md,CONTRIBUTING.md,.claude/**, the PR template, CI workflow commands,docs/design/*.mdandROADMAP.md. Checked documentation triggers, new module/test inventories, the unchanged 35-tool count, conventional title/commit subjects and absence of added type/lint suppressions.
Not verified
Local GUI/E2E suites, SELinux enforcement, other distribution matrices and kernel-uninterruptible filesystem I/O were not exercised. Their results do not determine this verdict. Lifecycle and filesystem boundaries below use independently challenged source arguments where the targeted probes do not exercise them. No public tool parameter or geometry behavior changes, so CLI-schema and coordinate probes were not applicable.
Lenses
- Correctness: denied activation triggers the opt-in path at
src/kwin_mcp/session.py:1256; healthy activation skips it. Both service roles validate argv before execution atsrc/kwin_mcp/dbus_service_file.py:271and:277. Python 3.12 and native D-Bus behavior are covered by the fresh parser probe. - Input/injection: exact server-environment opt-in at
src/kwin_mcp/session.py:1124; isolated Python invocation at:1127; quoted Bash arrays and discarded helper stdout at:1236and:1242; fixed role validation atsrc/kwin_mcp/dbus_service_file.py:268; literalexecvat:282. Quoting/NUL controls use the fresh probe; escape, Unicode, no-shell and no-PATH arguments were checked by argument+challenger(/root/wrapper_12db). - Filesystem/lifecycle, helper: same-descriptor regular-file validation at
src/kwin_mcp/dbus_service_file.py:48, nonblocking open at:46, bounded growth detection at:54, guaranteed descriptor closure at:60, fail-closed root selection at:237, launcher pair preflight at:271and registry reread at:277. Blocking metadata/read operations occur only in owned children. Covered by argument+challenger(/root/wrapper_12db). - Concurrency/lifecycle, session: spawn/registration share the close lock at
src/kwin_mcp/session.py:317; wrapper ownership is established at:725; helper group inheritance at:1138; setup/auth and lookup stalls are bounded externally by the monotonic handshake deadline at:1048and group teardown at:767. Rollback at:1194uses owned jobs, shared grace periods and no unconditional wait. Spawn-to-PID-assignment and jobspec signal windows use argument+challenger(/root/docs_8279); inherited monitor mode and TERM resistance use the fresh Bash probe. - Time arithmetic:
src/kwin_mcp/session.py:1203,:1209and:1221use shared seconds-based deadlines. Integer-second precision can shorten the effective grace period by less than a second; an in-progress D-Bus connection/authentication can overrun the nominal five-second role interval. The independent parent's sixty-second handshake deadline at:1048bounds startup waiting. Covered by argument+challenger(/root/wrapper_12db) and argument+challenger(/root/docs_8279). - Test audit:
tests/e2e/test_dbus_service_file.py:106exercises the previously accepted ignored-line cases; acquisition and execution-security tests assert exact argv, reached race paths and descriptor identity.tests/e2e/test_session_lifecycle.py:708,:943,:1061,:1092and:1392check lookup stalls, ownership, default-off behavior, environment isolation and rollback before fixture recovery.tests/e2e/test_exit_cleanup.py:606verifies actual TERM-resistant execution, open stdin, pipe EOF, original process identities and neighbor survival. Fresh complete source/diff audit found no vacuous assertions or new suppressions. - Docs/convention:
README.md:439,:441,:443and:445cover the opt-in, trusted roots, bounded rollback, ownership limits and SELinux domain.CONTRIBUTING.md:236and:308, plusdocker/README.md:141,:142and:148, include the helper and all new tests;CHANGELOG.md:21records whole-file NUL rejection. These match the base documentation triggers; no new synchronization omission.
Considered
- [input]
src/kwin_mcp/dbus_service_file.py:221ignored-line NUL permits execution of a file native D-Bus rejects; dismissed: probe rejected both malformed variants in both current roles while the prior helper executed them; NUL-free twins executed. - [injection]
src/kwin_mcp/dbus_service_file.py:282a builtin name, shell metacharacter or executable without a shebang gains shell execution; dismissed: argument+challenger(/root/wrapper_12db) literal execv performs no PATH lookup or ENOEXEC shell fallback; argv is never reparsed by Bash. - [input]
src/kwin_mcp/dbus_service_file.py:122and:224quoting or Unicode decoding diverges from D-Bus; dismissed: probe covers quoted-path/argument controls; argument+challenger(/root/wrapper_12db) checked quote/backslash/comment state transitions and UTF-8 scalar restrictions against D-Bus 1.16.2 source. - [filesystem]
src/kwin_mcp/dbus_service_file.py:46a regular file replaced by a FIFO blocks acquisition; dismissed: argument+challenger(/root/wrapper_12db) O_NONBLOCK precedes same-descriptor fstat rejection; slower filesystem I/O is confined to the owned child and parent deadline. - [filesystem]
src/kwin_mcp/dbus_service_file.py:54growth after fstat creates an unbounded read; dismissed: argument+challenger(/root/wrapper_12db) limit-plus-one read and post-read size rejection bound bytes, with descriptor closure in finally. - [input]
src/kwin_mcp/dbus_service_file.py:63removing the value-level NUL check bypasses validation; dismissed: argument+challenger(/root/wrapper_12db) every executable ingress passes the whole-file guard and subsequent decoding/escapes cannot synthesize NUL. - [concurrency/lifecycle]
src/kwin_mcp/session.py:1127lookup stalls before the watchdog starts; dismissed: argument+challenger(/root/docs_8279) parent construction is lexical, while acquisition occurs only in background helpers after activation failure. - [concurrency/lifecycle]
src/kwin_mcp/session.py:1236a signal before PID assignment leaks a helper; dismissed: argument+challenger(/root/docs_8279) the assignment is not atomic, but group ownership predates spawning; parent failure/shutdown escalates against that group even if the wrapper trap waits. - [concurrency/lifecycle]
src/kwin_mcp/session.py:1194recycled numeric PID receives rollback signals; dismissed: probe preserves the stale-marker neighbor; argument+challenger(/root/docs_8279) jobspec lookup unmasks SIGCHLD before kill_pid remasks. Completion in that gap retains the ordinary async job record, so PALIVE skips its dead member; removal before lookup yields INVALID_JOB. Trapped TERM/INT/HUP remain pending in this builtin. No replacement role job is launched during rollback. - [concurrency/lifecycle]
src/kwin_mcp/session.py:1215a kernel-stalled helper resumes execution after failed-bootstrap cleanup; dismissed: argument+challenger(/root/docs_8279) pending SIGKILL prevents return to userspace after the stall, while ownership remains with the private group. This does not promise immediate kernel task disappearance.
Every push to this pull request is reviewed automatically. To request another review without pushing, after replying to the findings, comment @haechibot review. The same comment restarts a review that stopped with an error. The pull request author or a maintainer can request it.
bulgasaribot
Bot
removed
the
agent:needs-attention
…ch-2 # Conflicts: # CHANGELOG.md
…ch-2 # Conflicts: # CHANGELOG.md
|
@haechibot review Please review the technical correctness of head The built wheels installed into Debian arm64 (KWin 6.3.6) and Fedora arm64 (KWin 6.7.5) images passed the full These local results do not substitute for current-head GitHub Actions. Real SELinux-enforcing Fedora, live NFS/FUSE, PID-reuse stress and GPU-backed ScreenShot2 remain unverified. Please judge technical correctness separately from merge authorization: merging, closing, auto-merge and releases remain unauthorized. |
There was a problem hiding this comment.
Verdict: OPEN re-reviewed at 7347b07. No remaining technical findings; the maintainer's merge hold remains in place.
Previous findings
- Closed: default-on direct activation workaround.
src/kwin_mcp/session.py:1124requires the exact server-side opt-in; a session or application environment cannot enable it. - Closed: quoted service commands losing argument boundaries.
src/kwin_mcp/dbus_service_file.py:208-210parses argv and:282executes it literally. - Closed: unbounded failed-bootstrap cleanup (thread).
src/kwin_mcp/session.py:1199-1215uses shared TERM/KILL grace periods without an unconditional wait. - Closed: eager service lookup outside the startup watchdog (thread). Lookup runs in owned helper children at
src/kwin_mcp/dbus_service_file.py:271-277, after activation fails. - Closed: inherited monitor mode separating helpers from session ownership (thread).
src/kwin_mcp/session.py:1138disables it before any background launch. - Closed: NUL bytes in ignored service-file lines (thread).
src/kwin_mcp/dbus_service_file.py:221-222rejects them before decoding or line parsing; regression inputs are attests/e2e/test_dbus_service_file.py:107and:111. - Open: the maintainer hold. isac322's latest request explicitly retains it.
The merge with main adds the unrelated spoken-name feature and expands the E2E time budget. Those changes do not share the direct-start runtime path. Technical review does not release the retained hold.
New findings
None.
Fix hazards
The direct-start implementation is unchanged from the previous reviewed head. The whole-file NUL guard operates on the already bounded byte buffer, adds no I/O or lock, and precedes every parser branch. Both executable roles pass through it; strict UTF-8 decoding and the escape map cannot introduce a NUL afterward.
Open items
- Maintainer hold by isac322 remains in place; merge authorization is reserved for a separate maintainer decision.
Verified
- Re-read the complete diff against main
3655841d6af72913f984c94650a24bef3e3ed23eand all changed files at this head, including the merged-main interaction. - In a fresh official Python 3.12 container with D-Bus 1.16.2, the current helper rejected NUL in comments and localized entries for both launcher and registry roles with no execution. Native D-Bus rejected the same files. Valid files and NUL-free controls preserved the quoted executable path and exact
--label=two wordsargv. A comment/newline control also matched native D-Bus. - Source and independent challenge review covered helper ownership,
set +m, service lookup and D-Bus setup under the parent handshake deadline, jobspec signaling, PID-reuse boundaries, bounded rollback, literalexecv, file-size and descriptor limits, and cleanup after close. - The linked Fedora SELinux issue remains open and distinguishes the observed name-acquisition denial from unresolved complete private AT-SPI support; this PR documents an opt-in workaround rather than claiming an upstream policy fix.
- Applied base conventions from
CLAUDE.md,CONTRIBUTING.md,docker/README.md,.claude/**, the PR template, workflows, design documents andROADMAP.md. Documentation inventories, the 37-tool count from main, opt-in warnings, changelog entries and the E2E timeout change are synchronized.
Not verified
Live GUI/E2E suites, SELinux enforcement, other distribution matrices, live NFS/FUSE behavior and kernel-uninterruptible filesystem I/O were not exercised locally. They do not determine this verdict. The lifecycle and filesystem conclusions use source arguments independently challenged against the relevant interleavings and boundaries.
Lenses
- Correctness: denied activation reaches the opt-in path at
src/kwin_mcp/session.py:1256-1269; healthy activation skips it. Both service roles validate argv atsrc/kwin_mcp/dbus_service_file.py:271-282. - Input/injection: exact server-environment opt-in at
src/kwin_mcp/session.py:1124; isolated helper invocation at:1127; quoted Bash arrays at:1236and:1242; fixed role validation atsrc/kwin_mcp/dbus_service_file.py:268; literalexecvat:282. Current-container controls cover quoting, comments, newline handling and NUL rejection; escape, Unicode, no-shell and no-PATH boundaries were checked by argument+challenger(/root/fresh_parser_docs). - Filesystem/lifecycle, helper: same-descriptor regular-file validation at
src/kwin_mcp/dbus_service_file.py:48-50, nonblocking open at:46, bounded growth detection at:54-56, descriptor closure at:58-60, fail-closed root selection at:237-263, launcher preflight at:271and registry reread at:277. Blocking metadata and reads occur only in owned children, covered by argument+challenger(/root/fresh_lifecycle). - Concurrency/lifecycle, session: spawn and registration share the close lock at
src/kwin_mcp/session.py:317-329; wrapper ownership is established at:725-731; helper group inheritance at:1138; setup/auth and lookup stalls are bounded externally by the monotonic handshake deadline at:1048and group teardown at:767-789. Rollback at:1199-1216uses owned jobs, shared grace periods and no unconditional wait. Signal windows and PID reuse were checked by argument+challenger(/root/fresh_lifecycle). - Time arithmetic:
src/kwin_mcp/session.py:1203,:1209and:1221use bounded shared deadlines. Integer-second precision can shorten a grace period by less than a second; an in-progress D-Bus call can exceed the nominal five-second role interval, while the parent's sixty-second handshake deadline remains the outer bound. - Test audit: the changed service-file tests exercise ignored-line NUL cases and parser controls. Acquisition, execution-security, lifecycle and exit-cleanup tests assert exact argv, descriptor identity, process groups, owned child identities, pipe EOF and neighbor survival before recovery cleanup. No new vacuous assertion or suppression was introduced.
- Docs/convention:
README.md:444-448,CONTRIBUTING.md:237and:313-315,docker/README.md:141-142and:148, andCHANGELOG.md:24document the opt-in, helper, limits, test inventory and NUL behavior. The workflow's.github/workflows/e2e.yml:55,91timeout change is internally consistent.
Considered
- [input]
src/kwin_mcp/dbus_service_file.py:221ignored-line NUL could execute a file native D-Bus rejects; dismissed: current-container probe rejected both malformed variants in both roles, while NUL-free twins executed. - [injection]
src/kwin_mcp/dbus_service_file.py:282a builtin, shell metacharacter or no-shebang executable could gain shell execution; dismissed: argument+challenger(/root/fresh_parser_docs) literalexecvperforms no PATH lookup or ENOEXEC shell fallback. - [input]
src/kwin_mcp/dbus_service_file.py:122and:224quoting or Unicode decoding could diverge from D-Bus; dismissed: current-container probe covers quoted arguments and comment/newline behavior, and argument+challenger(/root/fresh_parser_docs) checked quote state and UTF-8 scalar rules against D-Bus 1.16.2. - [filesystem]
src/kwin_mcp/dbus_service_file.py:46a regular file replaced by a FIFO could block acquisition; dismissed: argument+challenger(/root/fresh_lifecycle) O_NONBLOCK precedes same-descriptor fstat rejection, and slower I/O remains under the parent deadline. - [filesystem]
src/kwin_mcp/dbus_service_file.py:54growth after fstat could make reads unbounded; dismissed: argument+challenger(/root/fresh_lifecycle) limit-plus-one reads and post-read validation bound bytes, with closure infinally. - [concurrency/lifecycle]
src/kwin_mcp/session.py:1127lookup could stall before the watchdog; dismissed: argument+challenger(/root/fresh_lifecycle) parent construction is lexical and acquisition occurs only in children after activation failure. - [concurrency/lifecycle]
src/kwin_mcp/session.py:1236a signal before PID assignment could leak a helper; dismissed: argument+challenger(/root/fresh_lifecycle) group ownership predates spawning and parent teardown escalates the owned group. - [concurrency/lifecycle]
src/kwin_mcp/session.py:1194a recycled numeric PID could receive rollback signals; dismissed: argument+challenger(/root/fresh_lifecycle) termination uses native jobspec markers, while numeric probes only control polling. - [concurrency/lifecycle]
src/kwin_mcp/session.py:1215a kernel-stalled helper could resume after cleanup; dismissed: argument+challenger(/root/fresh_lifecycle) a pending SIGKILL prevents return to userspace, while the owned group remains available for final teardown.
Every push to this pull request is reviewed automatically. To request another review without pushing, after replying to the findings, comment @haechibot review. The same comment restarts a review that stopped with an error. The pull request author or a maintainer can request it.
There was a problem hiding this comment.
Verdict: OPEN re-reviewed at 7347b07. No maintainer answer has changed the open item since the previous review; the technical verdict and open items are unchanged.
Previous findings
- Closed: default-on direct activation workaround. The exact server-side opt-in is enforced at
src/kwin_mcp/session.py:1124. - Closed: quoted
Exec=argument splitting and literal execution atsrc/kwin_mcp/dbus_service_file.py:208-210and:282. - Closed: unbounded failed-bootstrap cleanup at
src/kwin_mcp/session.py:1199-1215. - Closed: parent-side service lookup outside the watchdog; lookup is child-owned at
src/kwin_mcp/dbus_service_file.py:271-277. - Closed: inherited monitor-mode ownership;
set +mruns before background launches atsrc/kwin_mcp/session.py:1138. - Closed: ignored-line NUL acceptance; the whole-file guard is at
src/kwin_mcp/dbus_service_file.py:221-222. - Open: isac322's explicit maintainer merge hold remains in place.
New findings
None.
Verified
The PR head is still 7347b07, all review threads are resolved, and the merge with main has no interaction with the direct-start runtime path. The previous full review at the current head covered parser parity, NUL rejection, helper ownership, filesystem bounds, startup deadlines, rollback signaling, cleanup, test coverage and documentation. No new technical evidence or code change requires reopening a finding.
Open items
- Maintainer hold by isac322 remains in place; merge authorization is reserved for a separate maintainer decision.
Every push to this pull request is reviewed automatically. To request another review without pushing, after replying to the findings, comment @haechibot review. The same comment restarts a review that stopped with an error. The pull request author or a maintainer can request it.
isac322
left a comment
isac322
left a comment
There was a problem hiding this comment.
Verdict: GREEN. PR-ready: false. Reviewed 7347b07aed2f241fbf541c2601ea33eaa4b41c84 against main 3655841d6af72913f984c94650a24bef3e3ed23e. No P0–P3 technical findings remain, and all six earlier findings are closed. The required issue-agent/review check is still failing; its latest review identifies the retained maintainer merge hold as the only open item.
Verified: all seven native E2E matrix jobs passed on this head, as did lint, typing, build, documentation and semantic-title checks. The built wheels, installed into each image, passed the full suite on Debian arm64 and Fedora arm64: 1,113 collected, 1,112 passed, 0 failures, 0 errors and one existing opt-in ScreenShot2 test skipped on each. On Debian arm64, installed CLI checks confirmed server-only opt-in under controlled first-activation denial. Five cases against native dbus-daemon 1.16.2 and both installed helper roles on Debian arm64 confirmed rejection of NUL-bearing comments/localized entries and exact execution of the valid control and both NUL-free variants. The PR's 12-file scope and main's spoken-name feature are preserved.
Not verified: real SELinux-enforcing persistent denial, live NFS/FUSE stalls, prompt cleanup of processes stuck in uninterruptible I/O, actual PID reuse, and GPU-backed ScreenShot2. Direct startup remains an operator-selected workaround with the documented confinement trade-off. This technical approval does not authorize merging, closing, auto-merge or a release; the maintainer hold remains.
|
Thanks for diagnosing the Fedora SELinux activation denial and putting together the initial fix and parser tests, @superbiche! The opt-in boundary, service-file validation, and process-group lifecycle are all verified on @haechibot review |
There was a problem hiding this comment.
Verdict: GREEN re-reviewed at 7347b07aed2f241fbf541c2601ea33eaa4b41c84. No blocking findings remain.
Previous findings
- Closed: default-on direct activation workaround; exact server-side opt-in at
src/kwin_mcp/session.py:1124. - Closed: quoted
Exec=parsing and literal execution atsrc/kwin_mcp/dbus_service_file.py:208-210and:282. - Closed: unbounded failed-bootstrap cleanup at
src/kwin_mcp/session.py:1199-1215. - Closed: parent-side service lookup outside the watchdog; lookup is child-owned at
src/kwin_mcp/dbus_service_file.py:271-277. - Closed: inherited monitor-mode ownership;
set +mruns before background launches atsrc/kwin_mcp/session.py:1138. - Closed: ignored-line NUL acceptance; the whole-file guard is at
src/kwin_mcp/dbus_service_file.py:221-222. - Closed: maintainer merge hold, explicitly released by isac322 in comment 6182279371.
New findings
None.
Verified
The full 12-file diff was reviewed against main at merge-base 3655841d6af72913f984c94650a24bef3e3ed23e. The direct-start path is opt-in, service-file commands are validated and executed literally, startup lookup remains inside the child watchdog, process groups are owned and cleaned up with bounded TERM/KILL handling, and the parser rejects NUL-containing files. An isolated container probe compared the helper with native D-Bus behavior for quoted arguments, comments, localized entries, malformed input, and NUL-free controls. The base-branch CLAUDE.md and CONTRIBUTING.md conventions were also checked.
Not verified
Live SELinux-enforcing Fedora behavior, live NFS/FUSE races, PID-reuse stress, and GPU-backed ScreenShot2 behavior were not reproduced here; no code-level issue remains from those environmental limits.
Lenses
Security and input validation, quoting and argument execution, startup deadlines, process-group lifecycle and rollback, filesystem races and bounds, documentation consistency, and interaction with the merged main changes were reviewed.
Considered
- [lifecycle]
src/kwin_mcp/session.py:1199-1215rollback could leave a failed bootstrap behind — dismissed: argument+challenger(/root/challenger) bounded group signaling and cleanup cover the failure window. - [filesystem]
src/kwin_mcp/dbus_service_file.py:230-277service-file replacement or non-regular-file races could bypass validation — dismissed: argument+challenger(/root/final_challenge) regular-file, size, and descriptor checks constrain the path. - [input]
src/kwin_mcp/dbus_service_file.py:221-222ignored comments or localized entries could bypass NUL rejection — dismissed: probe whole-file rejection matched native D-Bus behavior.
Every push to this pull request is reviewed automatically. To request another review without pushing, after replying to the findings, comment @haechibot review. The same comment restarts a review that stopped with an error. The pull request author or a maintainer can request it.
Rebased onto origin/main (ac3b821, kwin-mcp 0.12.0 + #111) and integrated every 0.12.0 capability and bug fix into the 29-tool 1.0 surface: - Locator.match ('substring' | 'exact' | 'spoken') unifies exact-name (#109) and spoken-name (#110, spoken_name.py) matching across element_query, waits, and element_act strict locators. - element_act accepts '|'-separated fallback action names (e.g. 'Press|click'), deduplicates candidate elements, and reports the invoked action (#109). - session.py and dbus_service_file.py port opt-in KWIN_MCP_ATSPI_DIRECT_START=1 direct AT-SPI startup for SELinux-enforcing hosts (#95), without re-triggering D-Bus activation from core.py when activation failed. - geometry.py ports native equality no-op, related-window traversal stop, and global X11 group-transient checks (#103) while keeping check_only and all window kinds. - cli.py rejects unknown boolean literals before session dispatch (#109). - Ported unit/E2E suites, QA trace entries (QA-SES-026..028, QA-ELEM-045..046, QA-CLI-009), mutants M83..M86, CHANGELOG 0.12.0 history, and docs.
… 1.0) (#106) * feat!: replace the 37-tool surface with 29 orthogonal tools (kwin-mcp 1.0) * feat: sync 1.0 orthogonal surface with 0.12.0 and #111 on main Rebased onto origin/main (ac3b821, kwin-mcp 0.12.0 + #111) and integrated every 0.12.0 capability and bug fix into the 29-tool 1.0 surface: - Locator.match ('substring' | 'exact' | 'spoken') unifies exact-name (#109) and spoken-name (#110, spoken_name.py) matching across element_query, waits, and element_act strict locators. - element_act accepts '|'-separated fallback action names (e.g. 'Press|click'), deduplicates candidate elements, and reports the invoked action (#109). - session.py and dbus_service_file.py port opt-in KWIN_MCP_ATSPI_DIRECT_START=1 direct AT-SPI startup for SELinux-enforcing hosts (#95), without re-triggering D-Bus activation from core.py when activation failed. - geometry.py ports native equality no-op, related-window traversal stop, and global X11 group-transient checks (#103) while keeping check_only and all window kinds. - cli.py rejects unknown boolean literals before session dispatch (#109). - Ported unit/E2E suites, QA trace entries (QA-SES-026..028, QA-ELEM-045..046, QA-CLI-009), mutants M83..M86, CHANGELOG 0.12.0 history, and docs. * fix(docs): align element_act fallback syntax in docs/skills and add 0.12 arg remedies Address review findings on 6d3895c: - Align SKILL.md, README.md, and CHANGELOG.md with the MCP string schema action='Press|click' (noting the in-process Python engine also accepts a sequence). - Add unknown-argument migration remedies for exact_name, spoken_name, and fallback_action_names in server.py and test_unknown_args.py. - Date 1.0.0 as 2026-10-15 in CHANGELOG.md and update M01-M86 range comments. * test(e2e): align session_start warning and Locator.match default in e2e assertions * docs: remove AI-ceremony files, dead design docs, and non-English prose in docs - Delete the self-referential .claude/ docs-seo loop (docs-seo.md, positioning.yml,\n docs-seo-trigger.sh, settings.json, check-docs-seo/SKILL.md), the one-off\n .github/social-preview-prompt.md, obsolete docs/design/* and\n docs/ai-agent-integration.md (folding unique tips into README.md and\n CONTRIBUTING.md), and the one-off scripts/gen_qa_trace.py scaffold.\n- Keep the deterministic checks in scripts/check_docs_seo.py and\n .github/workflows/docs-seo.yml while dropping the .claude/ prompt checks and\n PR comment bot.\n- Tighten CLAUDE.md, ROADMAP.md, CONTRIBUTING.md, README.md, SKILL.md, and\n plugin/README.md, and replace non-English examples in docs/skills with pure\n English descriptions. * fix(docker): drop removed docs/ directory from E2E image COPY
Summary
On Fedora with SELinux enforcing, virtual sessions had no accessibility: the policy denies the D-Bus activation of
at-spi-bus-launcherfrom kwin-mcp's privatedbus-run-sessionbus (fedora-selinux/selinux-policy#3385), and the registry's activation from the accessibility bus is denied the same way.wait_for_elementandfind_ui_elementsthen failed withAT-SPI2 query failed: AT-SPI2 worker exited.With
KWIN_MCP_ATSPI_DIRECT_START=1in the server's environment, when the activation fails, the session wrapper starts the launcher and then the registry directly and waits up to 5 s for each to own its bus name. Their commands come from theExec=lines of the same D-Bus service files (searched onXDG_DATA_DIRS), read and split into arguments the way dbus-daemon does (kwin_mcp/dbus_service_file.py), and run as bash arrays. Started this way they run in kwin-mcp's own SELinux domain rather thangnome_atspi_t, which is why it is opt-in; the desktop session's own launcher, started bysystemd --user, runs asunconfined_ttoo.session_startreports the direct start in itsWarning:line. If the direct start cannot complete, the previous warning is kept and the half-started launcher is stopped.Without the opt-in, a failed activation starts nothing, as before, and the warning now names the variable. Distros where activation works are unaffected.
Motivation
Fedora 44 KDE is a common kwin-mcp host, and its virtual sessions had no working
accessibility_tree,find_ui_elementsorwait_for_element.How to Test
scripts/run-e2e-docker.shon 1d3228e: quality step passed; 673 passed, 1 skipped.tests/e2e/test_dbus_service_file.py: the parsed argv for quoting, escapes, comments and malformed files, each expectation observed from dbus-daemon 1.16.2 activation on a private bus.tests/e2e/test_session_lifecycle.py:--label="two words", receives exactly that argv;session_startreported the activation failure naming the variable, andwait_for_elementfailed withAT-SPI2 worker exited (status -6), as onmain;Checklist
uv run ruff check .passesuv run ruff format --check .passesuv run ty checkpasses (ty check src testswith the locked ty)@superbiche · user · drafted with Claude Opus 5.5, reviewed before posting; the voice and decisions are mine.