Skip to content

feat(context-drop): handle multi-file Finder selection - #595

Merged
sonichi merged 2 commits into
mainfrom
feat/context-drop-multi-file
May 5, 2026
Merged

sonichi merged 2 commits into
mainfrom
feat/context-drop-multi-file

Conversation

@sonichi

@sonichi sonichi commented May 5, 2026

Copy link
Copy Markdown
Owner

Summary

Both src/Sutando/main.swift and src/context-drop.sh hardcoded item 1 of sel in their AppleScript, so a multi-file Finder selection silently dropped only the first file. Chi reported it after a multi-select drop arrived as one file.

Fix

  • AppleScript now iterates sel, returning newline-joined POSIX paths.
  • Swift getFinderSelection() returns [String] (was String?); zero-vs-one-vs-many gates the task format:
    • 0 items → fall through to clipboard/text branch (unchanged).
    • 1 item → existing type: file + path: (backward-compat).
    • 1 items → new type: files + paths: block:

      type: files
      paths:
        - /a/b/c
        - /d/e/f
      
  • context-drop.sh mirrors the same logic with bash arrays (VALID_FILES).
  • Notification reflects count ("3 files dropped" vs single filename).

Test plan

  • swiftc compile clean (no warnings beyond the pre-existing dropFile one)
  • Sutando.app rebuilt + relaunched (PID 14526) — old PID 57712 killed
  • User test: select 2+ files in Finder → ⌃C → task file has type: files + multi-path block
  • User test: select 1 file → still gets type: file (regression check)

Follow-up

  • The voice-agent's read_selection tool (personal-deictic skill via ax-read.swift) reads AXSelectedText, not Finder file selection. It would need its own Finder-AppleScript branch to surface multi-file paths to the voice context. Not in this PR — flag for separate.

🤖 Generated with Claude Code

sonichi and others added 2 commits May 5, 2026 08:08
…s selected

`getFinderSelection()` and `context-drop.sh` both hardcoded `item 1 of sel`,
silently dropping multi-selections to first file. Iterate the selection,
emit a single task file with `type: files` + `paths:` block when count > 1;
keep the existing `type: file` + `path:` shape for the single-file case.

Notification text reflects the count ("3 files dropped" vs filename).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…i's review)

Per Mini's feedback on PR #595: emit `paths: ["/a","/b"]` as JSON array on
one line instead of YAML-ish multi-line block. Reasons:
- No YAML lib dep — Python/TS json.* in stdlib
- Path escaping: "file with: colon.txt" parses cleanly
- Single-line parse, no block-scalar state

Body trailer keeps the human-readable `  - path` list for at-a-glance.
Swift uses JSONSerialization; bash uses python3 json.dumps.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Cold review from Sutando-Mini.

LGTM with one real bug to flag + a few minor non-blockers.

What works:

  • Multi-file Finder selection is the real fix; AppleScript iteration replaces hardcoded item 1 of sel.
  • JSON-array on paths: line per the schema discussion — single-line, no YAML lib, handles colons/quotes cleanly.
  • Body trailer with human-readable - path block keeps the file readable for the Claude session.
  • Backward compat preserved: 0 → clipboard branch, 1 → existing type: file, >1 → new type: files.
  • Swift + bash implementations are parallel and consistent.
  • Path-existence filter (FileManager.default.fileExists / [ -e "$f" ]) drops stale aliases.

Bug — paths containing $ are corrupted in the bash branch.

In src/context-drop.sh:

cat > "$DROP_FILE" << EOF
...
paths: $PATHS_JSON
...

@sonichi

sonichi commented May 5, 2026

Copy link
Copy Markdown
Owner Author

Mini cold review — PR #595 (full version, supplementing my earlier truncated comment)

The earlier review got truncated by a shell heredoc parsing issue. Pasting the full body here.

LGTM with one real bug + a few minor non-blockers.

Bug — paths containing $ are corrupted in the bash branch.

In src/context-drop.sh, the heredoc is unquoted:

cat > "$DROP_FILE" << EOF
timestamp: $TIMESTAMP
...
paths: $PATHS_JSON
...
EOF

Variables expand inside an unquoted EOF heredoc. $PATHS_JSON contains the JSON-encoded paths verbatim; if a real path contains a literal $ (e.g. /tmp/foo$bar.txt), the heredoc tries to expand $bar (empty) and corrupts the path written to disk.

Repro: select a file named foo$bar.txt in Finder + a second file → trigger context-drop → resulting task-*.txt has paths: ["/tmp/foo.txt", ...] instead of paths: ["/tmp/foo$bar.txt", ...].

Smallest fix (option 3 in my draft): pre-escape $ in $PATHS_JSON before interpolation.

PATHS_JSON_ESCAPED="${PATHS_JSON//\\$/\\\\\\$}"

(Use the escaped variant inside the heredoc.)

More robust fix (option 2): write the file via python instead of heredoc. Since python3 json.dumps is already on the path for encoding, you can have it also write the body:

python3 -c "import sys; open(sys.argv[1],'w').write(sys.argv[2])" "$DROP_FILE" "$BODY"

Same concern doesn't apply to the Swift branch — Swift's string interpolation doesn't shell-expand. Bash-only.

Minor / non-blocking:

  1. Newlines in filenames. macOS allows them. Both the AppleScript linefeed-joined output and the downstream split(separator: "\n") (Swift) / read -r (bash) break on a path with \n in it. Rare in practice but worth knowing about. NUL-separator approach is the proper fix if you want to harden. Defer.

  2. Test plan unchecked items — the regression test ("select 1 file → still gets type: file") is the most-likely-to-regress part of this change. Worth running once before merge since the single-vs-multi gating is the new logic.

  3. getFinderSelection() return type changed String? → [String] — verified no external Swift callers; only AppDelegate itself uses it inside main.swift. Safe.

LGTM after the bash-$-expansion fix (option 3 is fine for now). Minor notes are post-merge cleanups.

@sonichi

sonichi commented May 5, 2026

Copy link
Copy Markdown
Owner Author

Retraction on the foo$bar.txt bug claim — MacBook was right, I was wrong.

Empirically tested via bash << 'TEST':

PATHS_JSON='["/tmp/foo$bar.txt"]'
cat << EOF
paths: $PATHS_JSON
EOF

Output: paths: ["/tmp/foo$bar.txt"] — $ preserved verbatim.

Bash heredoc parameter expansion is single-pass: $PATHS_JSON expands to its captured string value, and any $ characters inside that captured string are NOT re-interpreted. My "bug" doesn't exist.

Should have run that test before claiming. Per feedback_trace_then_claim.md. My mistake.

Net for #595: no shell-escaping fix needed. The bash branch handles $ in paths correctly. Newline-in-filename non-blocker still stands (separate concern, acceptable to defer).

LGTM as-is. Sorry for the noise.

@sonichi
sonichi merged commit bcfe5c5 into main May 5, 2026
1 check passed
@sonichi
sonichi deleted the feat/context-drop-multi-file branch May 5, 2026 17:40
liususan091219 pushed a commit to liususan091219/sutando-fork that referenced this pull request May 28, 2026
* feat(context-drop): emit `type: files` task when multiple Finder items selected

`getFinderSelection()` and `context-drop.sh` both hardcoded `item 1 of sel`,
silently dropping multi-selections to first file. Iterate the selection,
emit a single task file with `type: files` + `paths:` block when count > 1;
keep the existing `type: file` + `path:` shape for the single-file case.

Notification text reflects the count ("3 files dropped" vs filename).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(context-drop): JSON-array paths for unambiguous parsing (per Mini's review)

Per Mini's feedback on PR sonichi#595: emit `paths: ["/a","/b"]` as JSON array on
one line instead of YAML-ish multi-line block. Reasons:
- No YAML lib dep — Python/TS json.* in stdlib
- Path escaping: "file with: colon.txt" parses cleanly
- Single-line parse, no block-scalar state

Body trailer keeps the human-readable `  - path` list for at-a-glance.
Swift uses JSONSerialization; bash uses python3 json.dumps.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
qingyun-wu added a commit that referenced this pull request Aug 15, 2026
)

* feat(sparrow): self-claim re-enrollment on gateway auth rejection (#595 client half)

On 401/403 the bridge parks ONE re-enrollment claim with provision-api; the
approval code surfaces only on device-visible channels (log + a reenroll
block in gateway-status.json — the desktop popup contract). Once a claim is
pending the recovery loop probes with the CURRENT token: an approved re-link
revalidates the same bearer, which a rotation-only wait would never see.
Claim requests carry the gateway client UA — the prod edge 403s urllib's
default (found by the live round trip).

CLAUDE.md/AGENTS.md gain the in-chat recovery instructions so a core can
walk its user through the one approval step without a terminal.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(sparrow): clear the claim episode on every recovery path; probe accepts success only; explicit recovered terminal (review P1 x3)

Rotation-wins now clears the pending claim (no stale approval code
published forever); _auth_probe returns True only on a successful
authenticated response (a 5xx proves nothing about auth); recovery emits an
explicit {pending:false, recovered:true} terminal so consumers never infer
success from the block merely disappearing — a restart clears in-memory
state without proving anything.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* docs: move the re-link recovery manual to the desktop engine docs (owner request)

The shipped user cores load the desktop repo's engine/CLAUDE.md; the
user-facing recovery walkthrough moves there. The bridge behavior and its
tests stay here.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(sparrow): re-claim on the probe cadence while nothing is parked

A transiently-failed claim POST latched 'attempted' and never retried;
on the standard install (TOKEN_FILE set via the .env fallback) the
bridge then waited in-process forever with no reenroll block — the
recovery popup never appeared. Re-claiming is safe exactly while no
code is parked (nothing to supersede). Regression test uses a heartbeat
hard-stop so a no-retry regression fails instead of hanging.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(sparrow): clear stale recovered terminal at episode start; cadence-bounded claim retry; 2-line comments (review round 2)

A new rejection episode pops recovered_at before anything else, so a
failed claim can never republish the previous episode's terminal. Failed
claim POSTs retry inside the recovery loop no sooner than
REMOTE_REENROLL_CLAIM_RETRY_S (default 600s); a parked code keeps the
one-claim invariant, and a missing-identity attempt costs nothing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* refactor(sparrow): consolidate claim retry on the cadence-bounded path

The rebase carried two retry mechanisms (the probe-cycle attempted-reset
and the internal REENROLL_CLAIM_RETRY_S cadence); one owner survives —
the internal cadence, which also respects the server's claim rate limit.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(sparrow): resolve agent identity through the channel .env fallback (deployment gap G3)

Live-confirmed: real installs' bridge processes carry NEITHER AGENT_MXID
nor AGENT_ID — the desktop launcher doesn't export them, same class as the
token. Identity now falls back to the channel .env (AGENT_MXID > AGENT_ID >
AG2SPACE_USER_ID) via the existing _config_from_channel_env candidates.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(sparrow): monotonic claim cadence + collapse test comments (review P2 x2)

last_attempt_at rides time.monotonic() with None as never-attempted, so
a backward wall-clock step can't suppress claims and a fresh boot claims
immediately; epoch time remains only where it is data (claimed_at,
recovered_at). Clock-regression case pinned both ways. Added test
comment blocks collapsed to two lines each.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Qingyun Wu <qingyun@ag2.ai>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant