Connector aware approvals - #1
Closed
CodeInfinity1 wants to merge 19 commits into
Closed
Conversation
Skip approval for read-only tools whose connector is already authorized, grant privacy-gated reads once per runtime session, and surface remaining approvals as native OS notifications with Approve/Reject actions on both macOS and Windows. Design only; no implementation yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Avoids reaching into PolicyEngine._connected_cache from tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- current_session_id: catch UnicodeDecodeError in addition to OSError (session file with invalid UTF-8 now safely returns instead of crashing) - purge_other_sessions: add empty-id guard to prevent accidental table wipe (matches pattern used in record/has methods) Fixes review findings in stram/safety/grants.py:22 and stram/safety/grants.py:86 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Read-only tools whose connector is authorized skip approval. Read-only tools with no connector (clipboard, screenshot) honour a session grant. BLOCKED is still evaluated first; all connector lookups fail closed. connected_lookup is an injectable seam so tests never touch private state. The per-instance connected cache keeps permissions_snapshot from opening the connector database once per tool per call. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pass config to all four PolicyEngine construction sites, mint a session id on API server start, and record a session grant after approving a read-only tool that has no connector. Mark the five privacy-gated reads read_only. Risk levels and requires_approval values are unchanged throughout. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Session "ask once" grants were handed out too freely: - a skipped/failed/invalid approval earned a grant (dry_run screenshot captured nothing yet unlocked prompt-free capture); require ActionStatus.SUCCEEDED, and reuse the executor's tool map instead of rebuilding default_tools - grants ignored arguments, so approving os_observe_ui with include_values=false authorized include_values=true; grants now store the approved tool_input as canonical JSON and only cover calls that ask for nothing more (legacy tool_grants tables are dropped on open) - the session id file outlived the server, so stale grants applied days later from the CLI; clear_session() now runs in server_close() - start_session ran before the socket bound, wiping another server's session on a failed bind; it now runs after construction Tests: the positive grant test used a schema-invalid input and asserted the bug as correct; the provider_id test was vacuous (no tool sets provider_id). Both now use registered fake read-only tools, plus regressions for the skipped-execution and broadened-argument cases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fix round 1 left two Critical holes in the read-only fast path.
Subset argument matching was unsound: every gated tool resolves an omitted
argument to a default that is broader than any explicit value, so a call that
drops a key asks for MORE, not less. Approving screenpipe_search with
content_type="ocr" and a start_time then re-sending only {"query": ...} skipped
the prompt and searched audio transcripts across the entire recorded history.
Same for os_clipboard_read max_chars (50 -> 4000) and os_observe_ui
max_elements (5 -> 40). Relatedly, all() over an empty dict is vacuously true,
so os_observe_ui {} (required=[]) matched any prior grant for that tool.
Coverage is now exact equality of the canonical argument set, which collapses
has() to one indexed SELECT against the existing primary key. A None tool input
returns False explicitly rather than being coerced to {}.
The session also outlived the server. clear_session only ran from
KeyboardInterrupt; macOS sends SIGTERM and Windows kills the process tree, so
the session file survived and grants stayed live into the next run. Record the
owning pid and treat a dead owner as no session (covers the uncatchable kill),
and install a SIGTERM handler so the clean shutdown path actually runs.
Finally, guard the grant-recording block in approve_pending_action: it runs
after mark_executed, and its store constructor can execute migration DDL, so a
locked database could fail a request whose action had already succeeded. Not
recording a grant only costs another prompt.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Quitting either app left `python -m stram serve` running: macOS had no
applicationWillTerminate at all and Windows only stopped the process from
the Stop button. The orphan's pid stays alive, so the session-liveness probe
reports a live session and its read-only capture grants keep being honoured
while the app looks quit. Two orphaned daemons and an orphaned collectors-loop
were found reparented to pid 1 on a machine with no app running.
- macOS: NSApplicationDelegateAdaptor whose applicationWillTerminate calls a
new AppViewModel.stopChildProcesses(), which toggleAgentProcess() now shares
so the quit and manual paths cannot drift. Stops the native collector too.
- LocalAgentProcess.stop() now waits for the child to exit (3s poll, SIGKILL
backstop) instead of signalling and dropping the reference, so the daemon is
gone rather than merely asked to go.
- Windows: wire _agentProcess.Stop() to the window's Closed event. Uncompiled;
Windows cannot be built on this machine.
Also harden two fail-closed paths in the grant store: bound the pid before
signalling and widen the except so OverflowError cannot escape
current_session_id, and give record() the same None-input guard has() has so a
grant recorded from an unknown input can never be matched by a later {} call.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace the deny-list in _github_path_segment with an allowlist over
[A-Za-z0-9_.-]. The deny-list missed percent-encoded dots (repo="%2e%2e/user"
reached the wire literally and normalizes to /user/issues at any decoding edge)
and URL delimiters: ConnectorHttpClient._url is plain concatenation, so
ref="abc?foo=bar" turned a check-runs read into GET /repos/o/n/commits/abc with
an attacker-chosen query string, bypassing the per_page clamp, while ref="abc#"
truncated the path and dropped /check-runs. The audit log recorded the
pre-truncation path either way. The explicit "."/".." check stays so v1.0.0 and
owner/name.js keep working.
github_checks_list asked for ("repo", "workflow"), but check-runs is a read and
workflow is a write scope; users who declined it got PermissionError on every
call. Now ("repo",).
Add OverflowError to the per_page clamp: json.loads accepts Infinity and
int(float('inf')) raises.
Assert on ToolResult.summary in the connector-failure tests so they pin the
except (ValueError, PermissionError) clause rather than passing via the generic
handler, and cover the previously untested missing-scopes path.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A parked run only showed a generic "Waiting for permission" spinner in chat, and refreshApprovals() had no timer, so a new approval could go unseen until the user manually opened the Permissions page. Add a notifier that raises a native banner with Approve and Reject buttons, plus the poll it needs to fire on. approveSelected/rejectSelected now share the one decision path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Four review findings on the connector-aware approval notifications: - notify() now returns whether the request actually reached the system, and the poll only marks a token notified when it did. Previously a token seen before the async authorization grant landed was marked forever, so an approval that persisted across a restart never raised a banner. - decideApproval tracks its token in decidingTokens (cleared via defer on both paths) and the poll skips those, so a tick that overlaps a decision cannot re-notify a request the user just approved. - The poll fetches approvals itself and returns on failure instead of going through refreshApprovals(), which swallows errors into an empty list. A daemon restart or a wake from sleep no longer withdraws a live banner and then re-alerts on recovery. refreshApprovals() is unchanged for its other callers. - The notification delegate and categories now register from the existing StramAppDelegate.applicationDidFinishLaunching instead of after bootstrap(), so a response that launched the app is not dropped. A one-slot pendingResponse queue holds a launch-time tap until configure() supplies the handlers, then drains it. Also: notify() gained the isAvailable bundle-identifier guard the other entry points have, and a denied authorization now surfaces a notice pointing at the Permissions page. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…oval banner `os.kill(pid, 0)` is not a liveness probe on Windows: CPython opens the target with PROCESS_ALL_ACCESS and calls TerminateProcess for any signal that is not CTRL_C_EVENT / CTRL_BREAK_EVENT, sig=0 included. `_session_owner_alive` ran it on the daemon's own pid on every policy evaluation of a read-only tool, so the daemon terminated itself, silently. `_pid_status` in the files tool had the same latent bug and would have killed a background process it was only querying. Both now go through `stram.process.pid_alive`, which short-circuits self, never signals on Windows, and reports anything unknown as dead. Session grants gain a 5-minute TTL: every session-granted tool reads ambient state, so exact-argument matching bounds the request but not the disclosure. The macOS approval banner showed `PolicyDecision.reason`, a per-risk-class constant, while its Approve button decided immediately. It now carries the tool, risk, the user's request and a truncated view of the arguments. The four GitHub read tools become MEDIUM + requires_approval so the connected-provider policy rule decides instead of being bypassed. Also: drop the pointless SIGTERM handler restore in run_api_server, stop re-nagging every launch about denied notifications, and correct the spec and plan to what actually shipped (Windows notifications are descoped). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…per limit ApprovalNotifier.requestAuthorization's completion closure was written inside a @MainActor-isolated method, so Swift 6 inferred MainActor isolation onto it. UNUserNotificationCenter always calls that closure back on a background queue, so the runtime isolation check failed on every launch (EXC_BREAKPOINT/SIGTRAP, ~seconds after launch, invisible to swift build). Fixed with an explicit @sendable annotation. Found and fixed only by live-launching a properly signed build and reading the resulting crash report -- swift build alone never caught this, since it's a dynamic check, not a compile-time one. Separately confirmed and documented in the spec: native notification delivery cannot be verified from any local dev build on this macOS version. Ad-hoc signing, a locally-trusted self-signed certificate, and a Gatekeeper manual allowlist (sudo spctl --add, now removed by Apple entirely) all fail identically -- Gatekeeper requires Developer ID signing plus notarization, which only the existing release pipeline (script/package_macos.sh) produces. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR makes Stram’s approval system “connector aware” by introducing read-only/provider metadata on tools, adding session-scoped grants for privacy-sensitive reads, and wiring PolicyEngine to consider connector connection state and per-session grants. It also adds four connector-backed GitHub read tools to avoid falling back to shell access, adds native macOS approval notifications + daemon shutdown hygiene, and includes a migration so legacy janus.sqlite3 agent state survives the rename to agent.sqlite3.
Changes:
- Add
Tool.read_only/Tool.provider_id, propagate through aliases, and extendPolicyEngineto waive prompts for connected-provider reads and session-granted privacy reads. - Add session/grant persistence (
stram/safety/grants.py), server session lifecycle wiring, and grant recording on successful approvals. - Add connector-backed GitHub read tools (
github_*_list) and macOS approval notification polling/handling.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_tools.py | Adds coverage for Tool defaults/alias metadata, GitHub read-tool metadata, and connected/disconnected policy behavior. |
| tests/test_tool_grants.py | New unit tests for session IDs, grant matching semantics, TTL, and PID liveness behavior. |
| tests/test_policy.py | Expands policy tests for connector-aware approvals and session grants. |
| tests/test_approval_queue.py | Verifies grants are recorded only for successful non-provider read-only tools. |
| tests/test_agent.py | Adds regression test for legacy agent DB/file/table migration. |
| stram/tools/workflow/implementation.py | Uses PolicyEngine(config) in workflow execution. |
| stram/tools/os_control/implementation.py | Marks privacy-sensitive read tools as read_only=True. |
| stram/tools/github/implementation.py | Introduces GitHubReadTool and registers four connector-backed read tools. |
| stram/tools/files/implementation.py | Switches PID probing to pid_alive() for cross-platform safety. |
| stram/tools/external/implementation.py | Marks screenpipe_search as read_only=True. |
| stram/tools/browser/live_tools.py | Marks live screenshot tool as read_only=True. |
| stram/tools/base.py | Adds read_only / provider_id fields to the Tool dataclass. |
| stram/tools/init.py | Ensures _ToolAlias preserves read-only/provider metadata. |
| stram/safety/policy.py | Adds constructor-injected config, connector connectivity checks, and session-grant checks. |
| stram/safety/permissions.py | Uses PolicyEngine(normalized) for connector-aware snapshots. |
| stram/safety/grants.py | Implements session identity + SQLite-backed session grant store with TTL. |
| stram/runtime.py | Wires PolicyEngine(config) and records session grants after successful approvals. |
| stram/process.py | Adds pid_alive() helper with Windows-safe behavior. |
| stram/orchestrator.py | Constructs executor with PolicyEngine(self.config). |
| stram/connectors/providers/manifests.py | Adds new GitHub read tools to connector tool hints. |
| stram/api.py | Starts/clears approval-grant session for the API server; handles SIGTERM via KeyboardInterrupt path. |
| stram/agent/store.py | Migrates legacy janus.sqlite3 files and janus_activations table to new agent naming. |
| docs/superpowers/specs/2026-08-06-connector-aware-approvals-design.md | Design doc capturing shipped deltas and threat model notes. |
| docs/superpowers/plans/2026-08-06-connector-aware-approvals.md | Implementation plan retained for traceability. |
| CHANGELOG.md | Notes persistence of local agent state across Janus→Agent rename. |
| apps/windows/Stram.App/MainWindow.xaml.cs | Stops spawned daemon on window close to avoid orphaned sessions/grants. |
| apps/macos/Sources/StramMacApp.swift | Adds app delegate for early notification delegate registration and daemon shutdown on quit. |
| apps/macos/Sources/LocalAgentProcess.swift | Ensures daemon is actually terminated (and SIGKILL backstop) on stop. |
| apps/macos/Sources/AppViewModel.swift | Adds approval polling/backoff + notification decision handling; centralizes child-process stop. |
| apps/macos/Sources/ApprovalNotifier.swift | Implements UNUserNotificationCenter integration with Approve/Reject actions and safe actor handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
1248
to
+1253
| def _pid_status(pid: int) -> str: | ||
| # pid_alive, not os.kill(pid, 0): on Windows that call terminates the process | ||
| # this function is only meant to be querying. | ||
| if pid <= 0: | ||
| return "unknown" | ||
| try: | ||
| os.kill(pid, 0) | ||
| except ProcessLookupError: | ||
| return "exited" | ||
| except PermissionError: | ||
| return "unknown" | ||
| return "running" | ||
| return "running" if pid_alive(pid) else "exited" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Describe the change, the user-visible problem it solves, and why this approach fits Stram's model-led, schema-driven architecture.
Related issue
Fixes #
Type of change
Real behavior proof
List exact commands, desktop actions, or live workflow steps you ran after the change.
Observed result:
Not tested:
Safety and privacy impact
Cross-platform impact
Checklist