Skip to content

fix(agent-isolation): scope touch dismissals to each operation - #1894

Open
leninworld wants to merge 2 commits into
apache:mainfrom
leninworld:fix-touch-overlay-dismissals-issue-1834
Open

leninworld wants to merge 2 commits into
apache:mainfrom
leninworld:fix-touch-overlay-dismissals-issue-1834

Conversation

@leninworld

Copy link
Copy Markdown
Contributor

This PR fixes issue #1834.

Summary

  • Give wrapper and hook watchers the same real signer PID, so dismissing one signature does not immediately open another reminder. Preserve the wrapper as lifetime owner, with a conservative owner-PID fallback if publication fails.
  • Clear an empty SSH-authentication dismissal after observed idle, under the existing window lease. Preserve nonempty signer dismissals and the existing TTL.
  • Add six real-process/socket regression cases, serialize the existing software-key signing test with its shared process lock, and regenerate the required setup fingerprint.

Type of change

  • Python package (tools/*/ with pyproject.toml)
  • Cross-cutting (RFC, AGENTS.md, sandbox, privacy-LLM)
  • Other: shell runtime

Test plan

  • Both reported faults fail assertions on the original script; all six new regression cases pass after the fix.
  • uv run --frozen prek run --all-files --skip workspace-pytest passes, including workspace Ruff, formatting, mypy, links and spec checks.
  • Complete affected/dependent pytest selection: agent-isolation 487 passed / 8 skipped, plugins 114 passed, setup-preflight 408 passed / 7 skipped — 1,009 passed, 15 skipped, zero failures.

Linux ARM64, Python 3.12.12, frozen workspace lock. The skips cover unavailable GUI/Tk, zsh, ambient Node, private-home and trusted-Python scenarios. No physical hardware or macOS GUI validation is claimed. Scoped test-file Ruff reports the same 45 existing diagnostics before and after, with none added.

Summary of retained Docker validation results

RFC-AI-0004 compliance

  • Sandbox — existing runtime-directory, watcher-ownership and window-lease boundaries preserved.

Gen-AI disclosure

Generative AI was used to prepare the code, tests and this description with Codex (desktop version 26.1013.40934), independently reviewed with Claude Desktop 4.29.0 (Opus 5.5). I have personally read and understood the final diff.

Linked issues

Closes #1834.

Share real signer identity across wrapper and hook watchers. Clear only an
empty completed-authentication dismissal while holding the window lease.
Keep conservative fallback and owner cleanup, and cover both regressions.

The existing software-key signing test now takes the shared process lock;
the setup fingerprint is regenerated by its required hook.

Fixes apache#1834

Generated-by: Codex (desktop 26.1013.40934)
Generated-by: Claude Desktop 4.29.0 (Opus 5.5 review)
@github-actions github-actions Bot added family:setup setup-* skills family:tools tools/* capability:platform Framework / agent substrate skills (install, verify, doctor, override, status, setup bootstrap) substrate:sandbox Tool substrate: agent isolation, egress control, settings audit labels Oct 28, 2026
Comment thread tools/agent-isolation/tests/test_gpg_touch_overlay.py Fixed
Generated-by: Codex (desktop 26.1016.61520)
Generated-by: Claude Desktop 4.29.0 (Opus 5.5 review)

@Kaap10 Kaap10 left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @leninworld for the solid PR. This cleanly resolves the two precision issues identified in #1834.

The implementation is well reasoned and thorough:

  1. Shared Signer Identity Across Watchers:
    Publishing the child shell PID via an intermediate exec wrapper ensures both wrapped and hook watchers observe the exact same signer PID, eliminating the duplicate window cycle when an agent session and a terminal sign concurrently.

  2. Conservative Publication Fallback:
    In signer_pids(), falling back to $MAGPIE_GPG_TOUCH_PARENT if $MAGPIE_GPG_TOUCH_SIGNER_FILE is unreadable or fails to publish guarantees that a publication issue never suppresses a touch prompt or aborts the signature.

  3. Lease-Protected Empty Dismissal Cleanup:
    Clearing only empty dismissals under the window lease when socket connections drop back to baseline resolves the second SSH authentication prompt suppression without endangering concurrent nonempty signer dismissals.

  4. Test Suite & Spec Integrity:
    The six new regression tests provide comprehensive coverage across both faults, including abnormal process termination and obstructed context paths. The serialization of test_git_signs_a_commit_through_the_wrapper with process_named prevents test flakiness, and FRAMEWORK_FINGERPRINT is correctly synchronized with the modified script.

All checks in CI pass cleanly (1,009 tests passed, 0 failures; prek formatting, Ruff, MyPy, and license checks green).

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sharing the real signer PID between the wrapped and hook watchers is clean and well tested; the empty-dismissal cleanup should fire only on the watcher's own blocked-to-idle transition, since as written a freshly armed idle watcher can delete another watcher's dismissal of an SSH authentication still in flight.

GitHub has not yet computed whether this branch merges cleanly with its base.

an empty dismissal is cleared when the watcher observes the authentication end (tools/agent-isolation/gpg-touch-overlay.sh:1018)

An empty ssh-authentication dismissal is cleared when
the watcher observes the authentication end, under the shared window lease;
a nonempty signer dismissal is preserved (#1834).

if [[ -e $DISMISSED_MARKER ]] && _lease_acquire; then
    if dismissed="$(cat "$DISMISSED_MARKER" 2>/dev/null)" && [[ -z $dismissed ]]; then
        rm -f "$DISMISSED_MARKER" 2>/dev/null

DISMISSED_MARKER is one file per host ($RUNTIME_DIR/dismissed), but baseline is each watcher's own socket count at start. So this branch runs for any watcher that is idle by its own baseline, not only the one that saw the authentication end. A hook watcher armed while an SSH authentication is already in flight counts that connection in its baseline, is idle on its first poll, takes the lease (released by the dismissing watcher after show_overlay) and deletes the empty marker; the watcher still blocked on that authentication then fails dismissal_holds and draws a second window for a touch the operator already dismissed. It fails safe (an extra click), but it brings back the duplicate-window case this PR fixes, and on main an idle watcher never touched the marker.

Clearing only on this watcher's own blocked-to-idle transition would match the spec line: set a flag in the blocked branch, run the lease-guarded removal in the idle branch only when it is set, then reset it. A regression test with watcher B shown and dismissed while a client connection stays open, and watcher A armed during that connection, asserting one showing and a surviving marker, would pin it.

Flagged by both the primary and adversarial reviewers.


This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The findings
below are observations, not blockers; an Apache Magpie
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.

More on how Apache Magpie handles maintainer review:
Contributing guide.

# not another one starting within the TTL. The existing lease
# protects the check and removal from a concurrent signer
# publishing a nonempty dismissal. Leave those markers alone.
if [[ -e $DISMISSED_MARKER ]] && _lease_acquire; then

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an empty dismissal is cleared when the watcher observes the authentication end

An empty ssh-authentication dismissal is cleared when
the watcher observes the authentication end, under the shared window lease;
a nonempty signer dismissal is preserved (#1834).

DISMISSED_MARKER is one file per host ($RUNTIME_DIR/dismissed), but baseline is each watcher's own socket count at start. So this branch runs for any watcher that is idle by its own baseline, not only the one that saw the authentication end. A hook watcher armed while an SSH authentication is already in flight counts that connection in its baseline, is idle on its first poll, takes the lease (released by the dismissing watcher after show_overlay) and deletes the empty marker; the watcher still blocked on that authentication then fails dismissal_holds and draws a second window for a touch the operator already dismissed. It fails safe (an extra click), but it brings back the duplicate-window case this PR fixes, and on main an idle watcher never touched the marker.

Clearing only on this watcher's own blocked-to-idle transition would match the spec line: set a flag in the blocked branch, run the lease-guarded removal in the idle branch only when it is set, then reset it. A regression test with watcher B shown and dismissed while a client connection stays open, and watcher A armed during that connection, asserting one showing and a surviving marker, would pin it.

Flagged by both the primary and adversarial reviewers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI-generated response (Codex), reviewed by Claude.

Thanks, agreed: cleanup needs to follow this watcher's own blocked-to-idle transition, rather than any poll that is idle relative to its startup baseline.

The prepared follow-up adds a separate transition flag and a real two-watcher/socket regression for the case you described, including a check that the dismissal survives while the original authentication remains in flight. It will also merge current main to resolve the setup-fingerprint conflict. These changes are prepared locally only: validation and the regenerated fingerprint are still pending, and nothing has been pushed yet. I'll keep this thread open until the tested revision is published.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

capability:platform Framework / agent substrate skills (install, verify, doctor, override, status, setup bootstrap) family:setup setup-* skills family:tools tools/* substrate:sandbox Tool substrate: agent isolation, egress control, settings audit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(agent-isolation): touch-overlay dismissals do not identify their signature precisely

4 participants