Add credential-proxy disclosure for skill command dispatch#142
Merged
dgarson merged 2 commits intodgarson/forkfrom Mar 3, 2026
Merged
Add credential-proxy disclosure for skill command dispatch#142dgarson merged 2 commits intodgarson/forkfrom
dgarson merged 2 commits intodgarson/forkfrom
Conversation
dgarson
commented
Feb 24, 2026
Owner
Author
dgarson
left a comment
There was a problem hiding this comment.
Summary
This PR adds explicit credential-proxy disclosure for non-owner skill command dispatch:
- deterministic tool-dispatch replies now prepend a disclosure warning for non-owners
- model-routed skill rewrites now instruct the model to disclose owner-credential/proxy usage
- adds regression coverage for non-owner deterministic tool dispatch
What looks good
- Disclosure is correctly gated by
!command.senderIsOwner - Message wording is clear and specific about OAuth/API token proxy risk
- Deterministic dispatch test validates disclosure appears and tool output is preserved
Concerns
-
PR scope overlap with #141
- This PR currently includes
src/agents/pi-embedded-helpers/errors.ts+ related tests, which are the PII-redaction changes tracked separately in PR #141. - Keeping both in this PR makes review/merge sequencing noisier and increases cherry-pick/conflict risk.
- This PR currently includes
-
Test coverage gap (non-blocking)
- No explicit test here for the owner path (ensuring no disclosure) and no direct assertion for the model-routed disclosure instruction branch.
Suggestions
- Rebase/split this PR to include only credential-disclosure changes (
get-reply-inline-actions*) unless intentionally stacked on #141. - Add small tests for:
- owner deterministic dispatch => no disclosure
- model-routed rewrite for non-owner => disclosure instruction included
Blocking issues before merge
- Please resolve/clarify the PR scope overlap with #141 (stack or split).
- If this is intentionally stacked, note that explicitly in the PR description.
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
1 participant
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.
Summary\n- add explicit disclosure text when a non-owner triggers a skill command with deterministic tool dispatch\n- add disclosure guidance to model-routed skill command rewrites for non-owner senders\n- add regression test covering non-owner disclosure on tool-dispatched skill commands\n\n## Why\nMitigates OAuth/API token proxy risk by making credential use transparent when skill commands may route requests through owner-configured credentials.\n\n## Testing\n- pnpm vitest src/auto-reply/reply/get-reply-inline-actions.credential-disclosure.test.ts\n- pnpm vitest src/auto-reply/reply/get-reply-inline-actions.skip-when-config-empty.test.ts