Skip to content

fix: gate github_clone and github_pull behind approval - #488

Open
rkfshakti wants to merge 1 commit into
andrewyng:mainfrom
rkfshakti:fix/github-clone-pull-approval-gate
Open

fix: gate github_clone and github_pull behind approval#488
rkfshakti wants to merge 1 commit into
andrewyng:mainfrom
rkfshakti:fix/github-clone-pull-approval-gate

Conversation

@rkfshakti

Copy link
Copy Markdown

Both github_clone and github_pull were registered with kind="read" in TOOL_DEFS, which caused approval_for_tool() to return False and override the approval=True flag set at the call site in integration_tools.py.

The permission engine reads requires_approval from the tool metadata to classify risk: False maps to RiskClass.READ, which is_consequential() returns False for, so the engine auto-allows the call without ever prompting the user. Yet both tools write to disk — github_clone creates a new directory and populates it with a full repository, and github_pull fast-forwards an existing clone to the latest upstream commits. Their own schema descriptions say "Requires user approval," and the _attach call passes approval=True, but none of that matters because the registry kind wins.

What makes this particularly hard to notice is that the connector list API (tool_dicts in tool_defs.py) always sets requires_approval: True in its response, so the UI shows both tools as gated. The runtime silently disagrees — the UI says "approval required," the engine says "low risk, go ahead."

This is the same class of bug as #399 (browser_open_url registered as kind="read"), just in a different connector. The fix is the same: reclassify both tools as kind="write" so the §36 kind→approval mapping correctly gates them.

Changes:

  • coworker/connectors/tool_defs.py: github_clone and github_pull changed from kind="read" to kind="write"
  • tests/test_send_target_resolution.py: regression test asserting both tools now have requires_approval is True

All 1015 tests pass, ruff clean.

Both tools were registered with kind="read" in TOOL_DEFS, so
approval_for_tool() returned False and overrode the approval=True
set at the call site. The permission engine then classified them
as READ (requires_approval=False → RiskClass.READ), auto-allowing
them without ever prompting the user — even though both write to
disk (clone creates a new directory, pull fast-forwards an existing
repo) and their own descriptions say "Requires user approval".

The connector list API (tool_dicts) always reports
requires_approval=True, so the UI showed them as gated while the
runtime silently bypassed the gate — a mismatch that made the bug
invisible to users.

Reclassify both as kind="write" so the §36 kind→approval mapping
correctly gates them.
@rkfshakti

Copy link
Copy Markdown
Author

Hi maintainers — just a friendly nudge on this one. The fix gates github_clone and github_pull behind approval so those operations require explicit consent. I'm excited to see it land. Would appreciate a review when you have a moment. Thanks!

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