Skip to content

feat(permissions): explain and gate macOS Documents/Desktop access#142

Open
songcarver wants to merge 2 commits into
mainfrom
claude/awesome-jang-6b704a
Open

feat(permissions): explain and gate macOS Documents/Desktop access#142
songcarver wants to merge 2 commits into
mainfrom
claude/awesome-jang-6b704a

Conversation

@songcarver
Copy link
Copy Markdown
Contributor

Summary

  • First install on macOS: prints a rationale and probes ~/Documents + ~/Desktop so both TCC prompts appear with on-screen context instead of incidentally mid-backfill.
  • Existing users: re-running cadence install no longer introduces a surprise Desktop prompt — the new ~/Desktop probe is gated on a marker written at first install or via the new opt-in command.
  • New opt-in command: cadence permissions request-desktop for users who want to enable Desktop scanning later.
  • Linux / Windows: no TCC equivalent, so the rationale and marker are #[cfg]-skipped and ~/Desktop is simply added to the scan list unconditionally — an additive improvement, no new prompts.

Test plan

  • Fresh macOS install: rationale banner + both TCC dialogs fire; install completes on grant and on deny.
  • Re-run install with bootstrap marker already present: no rationale, no new prompts.
  • Simulated existing user (last-version-bootstrap seeded): no Desktop prompt during install.
  • cadence permissions request-desktop on a simulated existing user: Desktop prompt fires, marker written.
  • Deny-both path: install completes cleanly, backfill does not crash, just returns no repos under gated folders.
  • Linux spot-check: no rationale printed, no prompt, ~/Desktop/<repo> is found by backfill.
  • cadence permissions --help and cadence permissions request-desktop --help read cleanly on all platforms.

Manual test script is in docs/plans/tcc-folder-permissions-handoff.md.

🤖 Generated with Claude Code

On macOS, the first filesystem access under `~/Documents` or `~/Desktop`
triggers a TCC permission prompt. Today those prompts fire incidentally
during recovery backfill without any on-screen context for the user.

This change surfaces the prompts deliberately:

- First install on macOS: print a short rationale and probe both folders
  so the system dialogs appear while the explanation is visible. Marker
  file `~/.cadence/cli/desktop-access-requested` records that the user
  has been asked.
- Existing users (re-install / upgrade): no new prompts — the marker's
  absence is silently treated as "not yet opted in," and `~/Desktop`
  stays out of the repo scan list until the user runs the new
  `cadence permissions request-desktop` command.
- Linux/Windows: no TCC equivalent, so the rationale, probes, and marker
  are all `#[cfg]`-skipped. `desktop_access_requested()` returns `true`
  unconditionally there, which additively adds `~/Desktop` and
  `~/Desktop/GitHub` to the scan list on those platforms.

First-install detection reuses the existing `last-version-bootstrap`
marker — absence of that file means the machine has never completed a
Cadence install before.

Tests cover the marker round-trip, first-install detection, Desktop
gating before and after the marker is written, and clap parsing of the
new subcommand. Marker-specific tests are `#[cfg(target_os = "macos")]`;
non-macOS test coverage asserts the no-op paths.

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

@cadence cadence Bot left a comment

Choose a reason for hiding this comment

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

⚠️Mergeable, but macOS handoff needs review

TCC prompt flow was added; code aligned but macOS handoff drifted.

@songcarver you can check your private coaching feedback here.

If this review was useful, please react with 👍 below. Otherwise, react with 👎.

cargo build --release
```

Then, on **macOS**, test each scenario by simulating a fresh or existing user via `HOME` overrides so you don't touch your real state:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Verification drifted

Claude wrote the macOS handoff around HOME=$FRESH_HOME, but Cadence resolves the probed folders from HOME (src/agents/mod.rs:156-159, used by src/permissions.rs:83-96 and src/git.rs:435-456). That redirects the checks to temp directories, and the matched session did not show any proof that these steps still trigger the real Documents/Desktop TCC dialogs you wanted to validate. Ask for one explicit macOS check of the actual prompt path before you rely on this handoff as the acceptance script.

Prompt for AI Agent
claude -r '4f954fdc-f73b-4f07-945f-f98d440e2a1c' 'Original request/outcome summary: TCC prompt flow was added; code aligned but macOS handoff drifted.

Cadence flagged a possible Verification issue at `docs/plans/tcc-folder-permissions-handoff.md`:30.

Concern:
Claude wrote the macOS handoff around `HOME=$FRESH_HOME`, but Cadence resolves the probed folders from `HOME` (`src/agents/mod.rs:156-159`, used by `src/permissions.rs:83-96` and `src/git.rs:435-456`). That redirects the checks to temp directories, and the matched session did not show any proof that these steps still trigger the real Documents/Desktop TCC dialogs you wanted to validate. Ask for one explicit macOS check of the actual prompt path before you rely on this handoff as the acceptance script.

Please verify this against the current branch and do one of the following:
1. Explain why this change was required, citing the exact request, repo guidance, or session evidence that justified it, or
2. Make the smallest change needed to bring the branch back in line with the request.

If this is a runtime-sensitive change, include the exact local verification command and output in your response.'

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.

Good catch. The original plan used HOME=$(mktemp -d) throughout, which isolates state but also redirects home_dir() to a tempdir — and those paths are not TCC-protected, so the 'expected dialog' steps would never have fired a real prompt.

Updated in 09df67e: the plan now splits into two explicit tracks:

  • Code-path tests (throwaway HOME) — exercise the rationale banner, marker write/read, install ordering. No dialogs expected.
  • Real-dialog tests — operate against the real HOME with tccutil reset SystemPolicy{Documents,Desktop}Folder <terminal-bundle-id> plus deletion of the bootstrap marker to re-trigger the first-install branch. These validate the actual macOS prompts.

Also added backup/restore instructions for the real-HOME steps so live Cadence state is not lost.

…tions

The earlier plan used HOME=$(mktemp -d) throughout, which isolates
state but redirects home_dir() to tempdir paths that macOS does not
TCC-protect — so the "expected dialog" steps would never have fired a
real system prompt. PR review flagged this before merge.

Reorganized into two explicit tracks:

- Code-path tests (throwaway HOME) exercise the rationale banner,
  marker write/read, and install ordering. No dialogs expected.
- Real-dialog tests operate against the user's real HOME with a
  tccutil reset on the terminal's bundle ID, plus removal of the
  bootstrap marker to re-trigger the first-install branch. These
  validate the actual macOS prompts.

Added explicit backup/restore guidance for the real-HOME steps so the
tester does not lose live Cadence state.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread src/bootstrap.rs
Comment on lines +531 to 540
if is_first_install().await.unwrap_or(false)
&& let Err(err) = permissions::prompt_first_install_folder_access().await
{
output::note(&format!(
"Could not record folder access preference ({err})"
));
}

let include_recovery_backfill = should_run_current_version_recovery_backfill(true).await?;
let outcome = execute_bootstrap(BootstrapOptions {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The cadence permissions request-desktop command unnecessarily triggers a full bootstrap process because Command::Permissions is missing from the automatic bootstrap exclusion list.
Severity: MEDIUM

Suggested Fix

Add Command::Permissions to the exclusion list in the should_run_automatic_current_version_bootstrap function. This will prevent the automatic bootstrap from running for this command, aligning its behavior with other lightweight setup commands like Install and Uninstall.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/bootstrap.rs#L531-L540

Potential issue: The `should_run_automatic_current_version_bootstrap` function does not
exclude `Command::Permissions`. Consequently, running the `cadence permissions
request-desktop` command triggers a full, potentially heavyweight version bootstrap
process. This is unintended for a lightweight command designed for managing permissions
on existing installations and can lead to unnecessary operations like recovery backfill
scanning, causing significant delays.

Did we get this right? 👍 / 👎 to inform future reviews.

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