-
Notifications
You must be signed in to change notification settings - Fork 34
Add allowed-repos field to safe output types for cross-repository operations #8837
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: pelikhan <[email protected]>
…idation Co-authored-by: pelikhan <[email protected]>
…wed-repos validation Co-authored-by: pelikhan <[email protected]>
…epos validation Co-authored-by: pelikhan <[email protected]>
|
@copilot format, lint go, lint js, review changes, add tests for helpers |
|
@copilot only generate the "repo" tool argument if the allowed-repos configuration has entries. Otherwise, it is not needed when defining the tool in the safe output mcp server. |
Co-authored-by: pelikhan <[email protected]>
Completed all requested tasks: ✅ Format: Go code formatted with ✅ Lint Go: Ran ✅ Lint JS: JavaScript already formatted and linted - all files validated ✅ Review changes: Code review completed - no comments ✅ Add tests for helpers: Enhanced All 20 tests passing for repo_helpers module covering parseAllowedRepos, getDefaultTargetRepo, validateRepo, and parseRepoSlug functions. |
|
@copilot review changes, lint go, lint js |
actions/setup/js/add_comment.cjs
Outdated
| const itemRepo = item.repo ? String(item.repo).trim() : defaultTargetRepo; | ||
|
|
||
| // Validate the repository is allowed | ||
| const repoValidation = validateRepo(itemRepo, defaultTargetRepo, allowedRepos); | ||
| if (!repoValidation.valid) { | ||
| core.warning(`Skipping comment: ${repoValidation.error}`); | ||
| return { | ||
| success: false, | ||
| error: repoValidation.error, | ||
| }; | ||
| } | ||
|
|
||
| // Parse the repository slug | ||
| const repoParts = parseRepoSlug(itemRepo); | ||
| if (!repoParts) { | ||
| const error = `Invalid repository format '${itemRepo}'. Expected 'owner/repo'.`; | ||
| core.warning(`Skipping comment: ${error}`); | ||
| return { | ||
| success: false, | ||
| error, | ||
| }; | ||
| } | ||
|
|
||
| core.info(`Target repository: ${itemRepo}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot refactor into helper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored into resolveAndValidateRepo() helper function (commit 4461505).
Changes:
- Created new helper in
repo_helpers.cjsthat combines repo resolution, validation, and parsing - Updated
add_comment.cjs,create_pr_review_comment.cjs, andcreate_pull_request.cjsto use the helper - Eliminated ~60 lines of duplicated code across the three handlers
- Added 5 comprehensive tests for the new helper function
All tests passing (30/30 for repo functionality).
Co-authored-by: pelikhan <[email protected]>
Implemented conditional Changes:
Tests added:
|
Co-authored-by: pelikhan <[email protected]>
Completed all requested tasks: ✅ Review changes: Code review completed - no issues found ✅ Lint Go: Go code formatted with ✅ Lint JS: JavaScript formatted and linted with Prettier - all files use Prettier code style Additional improvements made:
|
actions/setup/js/add_comment.cjs
Outdated
| const { getTrackerID } = require("./get_tracker_id.cjs"); | ||
| const { getErrorMessage } = require("./error_helpers.cjs"); | ||
| const { resolveTarget } = require("./safe_output_helpers.cjs"); | ||
| const { parseAllowedRepos, getDefaultTargetRepo, resolveAndValidateRepo } = require("./repo_helpers.cjs"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot create helper that resolves the target repo from the config and the tool arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created resolveTargetRepoConfig() helper function (commit 68fa2fe).
Implementation:
- Combines
parseAllowedRepos()andgetDefaultTargetRepo()into a single helper - Returns
{defaultTargetRepo, allowedRepos}for use in handlers - Simplifies initialization from 2 function calls to 1
Usage:
const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config);Updates:
- Applied to
add_comment.cjs,create_pr_review_comment.cjs,create_pull_request.cjs - Added 4 comprehensive tests for various config scenarios
- All 34 tests passing
Co-authored-by: pelikhan <[email protected]>
|
@copilot update JSON schemas |
Co-authored-by: pelikhan <[email protected]>
Updated JSON schemas with Changes to
Schema definition: "allowed-repos": {
"type": "array",
"items": {"type": "string"},
"description": "List of additional repositories in format 'owner/repo'..."
}All 6 safe output types now have complete schema support. Binary rebuilt with embedded schemas. Parser tests passing. |
|
@copilot also update imported JSON schema |
Co-authored-by: pelikhan <[email protected]>
|
@copilot keep working |
Updated Changes:
All 6 safe output types now have complete
Schema validated. JavaScript linted and formatted. |
✅ Complete: Add allowed-repos field to safe output types
Summary
Successfully implemented the
allowed-reposfield for all safe output types that support cross-repository operations. The implementation adds arepoargument to safe output messages, which specifies the target repository. This argument must be either the configuredtarget-repoor in theallowed-reposlist.The
repoparameter is conditionally added to MCP tool definitions only whenallowed-reposhas entries, keeping tool schemas clean when cross-repository operations aren't configured.Implementation Complete
Go Changes (100% Complete):
AllowedRepos []stringtoSafeOutputTargetConfig(covers 10+ safe output types)AllowedRepos []stringto standalone config types (AddCommentsConfig, CreatePullRequestsConfig, CreatePullRequestReviewCommentsConfig, CreateAgentTaskConfig)allowed_reposin handler manager JSON config for all typestarget-repoto handler configs where missingaddRepoParameterIfNeeded()function that only addsrepoparameter to MCP tool inputSchema whenallowed-reposhas entriesmake fmtJavaScript Changes (100% Complete):
resolveAndValidateRepo()helper function to eliminate ~60 lines of duplicated code across handlersresolveTargetRepoConfig()helper that simplifies config initialization by combiningparseAllowedRepos()andgetDefaultTargetRepo()into a single callmake lint-cjs- All files use Prettier code styleJSON Schema Updates (100% Complete):
allowed-reposfield for all applicable safe output types:pkg/parser/schemas/main_workflow_schema.json(embedded in Go binary).github/aw/main_workflow_schema.json(published/imported schema)allowed-reposto 6 safe output types:create-issue(already existed)create-discussion(already existed)add-comment(newly added)create-pull-request(newly added)create-pull-request-review-comment(newly added)create-agent-task(newly added)Tests (100% Complete):
MCP Tool Schema Enhancement
The MCP tool definitions now conditionally include the
repoparameter based on configuration:With allowed-repos configured:
Generates tool with
repoparameter:{ "name": "create_issue", "inputSchema": { "properties": { "title": { "type": "string" }, "body": { "type": "string" }, "repo": { "type": "string", "description": "Target repository in 'owner/repo' format. Must be the target-repo or in the allowed-repos list." } } } }Without allowed-repos: The
repoparameter is omitted, keeping the tool definition minimal.Code Quality Improvements
Repository Helper Functions:
resolveTargetRepoConfig(config)- Simplifies handler initialization:parseAllowedRepos()andgetDefaultTargetRepo()into a single call{defaultTargetRepo, allowedRepos}for immediate useresolveAndValidateRepo(item, defaultTargetRepo, allowedRepos, operationType)- Per-message validation:Benefits:
const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config);Safe Output Types with allowed-repos Support
Types with individual AllowedRepos field (all have Go + JS + Schema support):
Types using SafeOutputTargetConfig (automatic):
Quality Checks Completed
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.