feat: add :AgitateRepoVisibility - #6
Conversation
Changes an existing repository between public and private, which was
the last unchecked repository function in the README.
:AgitateRepoVisibility -v private
:AgitateRepoVisibility -v public -u acme -r agitate
The repository and owner default to whatever `origin` points at, so
inside a checkout only the visibility is needed. `util.parse_github_remote`
reads both forms git hands back, the https URL and the scp style ssh
remote, with the `.git` suffix optional in each.
Going public asks for confirmation. It publishes the repository and its
entire history to everyone, and it cannot be meaningfully undone once
the contents have been seen, forked or indexed, so it is not something
a mistyped command should be able to do. Going private is not a
disclosure and does not ask.
`-v` is required and validated. There is no sensible default for a
command whose only purpose is to set that value, and guessing one in
either direction would be wrong.
Merging the HTTP branch brought in `util.parse_github_remote` and `util.origin_repository`, which this branch had introduced its own copies of. Both were needed by more than one feature, so they now live in `util` on the shared base. Drops the duplicates and points `Visibility` at the shared versions. The behaviour and the tests are unchanged; there is simply one definition of each now instead of two.
There was a problem hiding this comment.
🟡 Changes recommended
The current implementation has a couple of correctness/safety inconsistencies (service handling consistency and owner-defaulting behavior) that should be addressed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new Repo-level command to change a GitHub repository’s visibility (public/private), including safety confirmation when making a repo public, plus service-layer support and tests for the GitHub API call.
Changes:
- Add
:AgitateRepoVisibilityuser command and core implementation with-v/-u/-rargument handling and a confirmation prompt for public visibility. - Add
service.github.set_repository_visibilityto PATCH the repo visibility via the GitHub API. - Extend the test suite and README to cover/describe the new visibility feature.
File summaries
| File | Description |
|---|---|
| tests/github_spec.lua | Adds unit tests covering the visibility PATCH request and failure reporting. |
| README.md | Documents :AgitateRepoVisibility and marks the visibility feature as implemented. |
| lua/agitate/service/github.lua | Introduces set_repository_visibility GitHub service call. |
| lua/agitate/core/repo.lua | Implements M.Visibility (argument parsing, defaults, confirmation, calling service). |
| lua/agitate/api/repo.lua | Registers the new AgitateRepoVisibility user command. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| local origin_owner, origin_repository = util.origin_repository() | ||
| local repository_name = parameters['-r'] or origin_repository | ||
| local github_username = parameters['-u'] or origin_owner or options.github_username | ||
|
|
| function M.set_repository_visibility(access_token, owner, repository, is_private, callback) | ||
| http.request({ | ||
| url = http.github_url('repos/' .. owner .. '/' .. repository), | ||
| method = 'PATCH', | ||
| token = access_token, | ||
| body = { private = is_private }, | ||
| }, function(request_ok, response) | ||
| if not request_ok then | ||
| return callback(false, response) | ||
| end | ||
|
|
||
| if response.status ~= 200 then | ||
| return callback(false, { message = M.describe_failure('change the visibility of `' .. owner .. '/' .. repository .. '`', response) }) | ||
| end | ||
|
|
||
| callback(true, response.body or {}) | ||
| end) | ||
| end |
| assert.are.equal('PATCH', captured_request.method) | ||
| assert.are.equal('https://api.github.com/repos/acme/agitate', captured_request.url) | ||
| assert.are.same({ private = true }, captured_request.body) |
…path Three review findings. The owner fell back to the configured `github_username` when `origin` could not be parsed. That contradicted the documented behaviour and, more importantly, meant `-r some-repo` run outside a checkout would silently target `<configured-user>/some-repo`. Guessing an owner is not something a command that changes who can see a repository should do. The owner now comes from `-u` or from the GitHub `origin` remote, and the command asks when it has neither. `set_repository_visibility` reimplemented the request and response handling instead of using `M.call`, and treated a 200 carrying no JSON as success by substituting an empty table. It now goes through `M.call` like every other endpoint, so a non JSON success surfaces as an error rather than as a silent no-op, and there is one definition of what a successful call means. The callback annotation claimed `GitHubNewRepoSuccessResponse`, which describes repository creation rather than this endpoint. Loosened. The spec asserted method, URL and body but never that the token reached the transport, even though an unauthenticated PATCH would 404 rather than change anything. Adds that assertion, and a case for the non JSON 200 the shared helper now rejects.
There was a problem hiding this comment.
🟡 Changes recommended
core.repo.Visibility ignores parse_args’s incomplete return value, which can silently drop missing flag values and mis-handle user input.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
| local parameters, leftover = parse_args({ | ||
| '-v', | ||
| '-r', | ||
| '-u', | ||
| }, optional_parameters) | ||
|
|
||
| if #leftover > 0 then | ||
| return agitate_error.throw('core.repo.Visibility -- Error: unrecognised arguments: ' .. table.concat(leftover, ' ')) | ||
| end |
| positionally. The repository and owner default to whatever the `origin` | ||
| remote points at, so inside a checkout you only need the visibility. Outside | ||
| one, or with a remote that is not GitHub, pass `-u` explicitly. |
…t the README Two review findings. `Visibility` ignored the `incomplete` return from `parse_args`, so `-u` with no value silently fell back to the `origin` defaults and changed the visibility of a repository the user had just tried to override. It now reports the flag that needs a value. The README said that outside a checkout, or with a non GitHub origin, you pass `-u` explicitly. The command needs both the owner and the repository in that case, since neither can come from the remote. Says both now.
There was a problem hiding this comment.
🟢 Approval recommended
The implementation is consistent with the existing service/call pattern, includes safety confirmation for risky behavior, and adds targeted automated test coverage for the new GitHub endpoint integration.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The feature is implemented with appropriate input validation, safety confirmation for public changes, and targeted service-level tests for request/response handling.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The feature appears correctly implemented with targeted service-level tests, and the remaining feedback is limited to minor clarity improvements.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
| ---@param access_token string Your GitHub PAT | ||
| ---@param owner string The user or organization owning the repository | ||
| ---@param repository string The repository name | ||
| ---@param is_private boolean The visibility to set |
| local origin_owner, origin_repository = util.origin_repository() | ||
| local repository_name = parameters['-r'] or origin_repository | ||
| local github_username = parameters['-u'] or origin_owner | ||
|
|
Inside `Visibility` the variable held the account that owns the repository, which comes from `-u` or the `origin` remote and is not necessarily the configured account. Calling it `github_username`, the name every other command uses for `options.github_username`, made the two easy to conflate, and conflating them is precisely how the unsafe fallback to the configured account got written in the first place. Also spells out what the `is_private` boolean means in `set_repository_visibility`, since "the visibility to set" left the direction of the flag to the reader.
There was a problem hiding this comment.
🟢 Approval recommended
The implementation matches the described behavior, includes safety confirmation for making repos public, and adds targeted tests for the service integration and error handling.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The change is well-scoped, matches the described behavior (including safety confirmation), and is covered by focused service-layer tests.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The implementation is consistent with existing patterns, includes safety confirmation for making repos public, and is backed by targeted service-level tests.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The feature is implemented end-to-end with service-level tests and safe UX (confirmation on public), with only a minor nargs ergonomics suggestion noted.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
| end, { | ||
| nargs = '*', | ||
| desc = 'Change the visibility of a repository between public and private', | ||
| }) |
The command was declared `nargs = '*'`, so `:AgitateRepoVisibility` with no arguments was accepted and then failed in the core with a message about `-v`. The visibility is required and deliberately has no default, so the command line can reject the empty invocation directly, matching `:AgitateBranchDelete`.
There was a problem hiding this comment.
🟢 Approval recommended
The feature is implemented end-to-end (API/service/core/command/docs) with targeted tests covering the new GitHub service behavior and failure reporting.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
`table<string>` is not valid LuaLS and describes the wrong shape: the value is `opts.fargs`, a `string[]`. And the notification split "Repository" across a conditional (`'Private r' or 'R'` followed by `'epository '`). Both have now been reported on several branches one file at a time, because each branch holds its own copy of the affected function. Applied across every file on every branch at once so they stop recurring.
There was a problem hiding this comment.
🟡 Changes recommended
The positional argument order derived from parse_args is likely to cause owner/repo swapping in real use and should be corrected (and documented) before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
| Takes `-v` for the visibility (`public` or `private`, required), `-r` for the | ||
| repository and `-u` for the owner, in that order if you pass them | ||
| positionally. The repository and owner default to whatever the `origin` | ||
| remote points at, so inside a checkout you only need the visibility. Outside | ||
| one, or with a remote that is not GitHub, pass both `-u` and `-r`. |
`parse_args` fills declared flags positionally in declaration order, and the declaration was `-v, -r, -u`. So `:AgitateRepoVisibility public acme agitate` meant the repository `acme` owned by `agitate`, the reverse of how GitHub writes it everywhere. Reordered to `-v, -u, -r`, and the README now shows the reading rather than leaving it to be inferred. Same class as the `-H` before `-B` fix on the pull request branch: whenever positional order is available, it has to match the order people already have in their heads. Breaking for anyone passing all three positionally, which the README has documented for less than a day.
There was a problem hiding this comment.
🟢 Approval recommended
The implementation is consistent with existing service/core patterns, includes appropriate safety confirmation for making repos public, and adds targeted tests for the new GitHub service endpoint.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Closes the last unchecked item under Repository functions.
Defaults come from the remote
The repository and owner default to whatever
originpoints at, so inside a checkout you only supply the visibility.util.parse_github_remotehandles both forms git returns:with the
.gitsuffix optional in each, and it correctly keeps the dot in a name likeagitate.nvim. A non-GitHub remote returns nil rather than a wrong guess, and the command then asks for-uand-rexplicitly.Going public confirms first
Making a repository public exposes it and its entire history to everyone, and it cannot be meaningfully undone once the contents have been seen, forked or indexed. That is not something a mistyped command should be able to do, so it prompts and defaults to Cancel.
Going private is not a disclosure, so it does not ask.
-vis requiredThere is no sensible default for a command whose only purpose is to set that value. Guessing in either direction would be wrong, so a missing or misspelled
-vis reported.Tests
Eight new: six on
parse_github_remote(https, https without suffix, ssh, a dotted repository name, a non-GitHub host, nil/malformed input) and two on the service call (that it PATCHes the right URL with the right body, and that it surfaces GitHub's reason on a 403).The confirmation path is not tested, for the same reason as in #4: driving a modal
vim.fn.confirmfrom a spec needs harness work beyond this feature.