feat(registry): publish buttons to the hosted registry (#276 client) - #217
Merged
Conversation
📝 WalkthroughWalkthroughThe PR extends ChangesRegistry Publishing Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/store/publish.go (1)
66-106: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject symlinks and other non-regular files while loading the bundle.
This helper reads every non-directory entry with
os.ReadFile. That follows symlinks and can block on FIFOs/devices, so a crafted button directory can leak arbitrary local files to the registry or hangbuttons publish.Suggested fix
for _, e := range entries { if e.IsDir() { continue // skip pressed/ — run history isn't part of the artifact } + if e.Type()&os.ModeSymlink != 0 { + return nil, fmt.Errorf("button %q contains symlink %q; only regular files are allowed", localName, e.Name()) + } + info, err := e.Info() + if err != nil { + return nil, err + } + if !info.Mode().IsRegular() { + return nil, fmt.Errorf("button %q contains non-regular file %q; only regular files are allowed", localName, e.Name()) + } // `#nosec` G304 -- dir is config.ButtonDir (rooted, slugified) + an enumerated entry. data, err := os.ReadFile(filepath.Join(dir, e.Name())) if err != nil { return nil, err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/store/publish.go` around lines 66 - 106, The loadLocalBundle helper currently accepts every non-directory entry and passes it to os.ReadFile, which allows symlinks and other non-regular files to be followed or read. Update the loop over entries in loadLocalBundle to inspect each entry’s file type before reading, and only allow regular files while skipping or rejecting symlinks, FIFOs, devices, and other special files. Keep the existing button.json handling and Bundle construction unchanged, but make sure any invalid entry causes a clear error rather than being read.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/publish.go`:
- Around line 54-60: The publish flow is forwarding publishKind straight into
HTTPPublisher without local validation, so invalid values only fail after the
registry request. Add an early validation step in the publish path in
cmd/publish.go, before constructing the store.HTTPPublisher or making the POST,
so only the supported button/drawer kinds are accepted. If the kind is invalid,
return a local publishConfigError/VALIDATION_ERROR using the existing
publishKind handling in the publish command.
In `@internal/store/http_publish.go`:
- Around line 101-107: The archive header in the file-writing path is
hard-coding a single mode, which breaks the required permissions split for
published buttons. Update the tar header creation in the archive-building logic
to derive Mode per entry using a helper such as archiveModeFor, and make sure
button.json and any *.history.json files use 0600 while code/content files use
0700 instead of one blanket 0644 setting.
---
Outside diff comments:
In `@internal/store/publish.go`:
- Around line 66-106: The loadLocalBundle helper currently accepts every
non-directory entry and passes it to os.ReadFile, which allows symlinks and
other non-regular files to be followed or read. Update the loop over entries in
loadLocalBundle to inspect each entry’s file type before reading, and only allow
regular files while skipping or rejecting symlinks, FIFOs, devices, and other
special files. Keep the existing button.json handling and Bundle construction
unchanged, but make sure any invalid entry causes a clear error rather than
being read.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c38cd96e-e064-436f-a5d4-cf87e300b160
📒 Files selected for processing (4)
cmd/publish.gointernal/store/http_publish.gointernal/store/http_publish_test.gointernal/store/publish.go
Comment on lines
+54
to
+60
| if reg := strings.TrimRight(os.Getenv("BUTTONS_REGISTRY_URL"), "/"); reg != "" { | ||
| key := registryWriteKey() | ||
| if key == "" { | ||
| return publishConfigError("registry write key not set: run `buttons batteries set REGISTRY_WRITE_KEY <key>` (or set $BUTTONS_BAT_REGISTRY_WRITE_KEY)") | ||
| } | ||
| return err | ||
| pub := &store.HTTPPublisher{BaseURL: reg, Key: key, Kind: publishKind} | ||
| return renderPublish(func() (*store.PublishResult, error) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate --kind before the registry POST.
The CLI advertises button | drawer, but publishKind is forwarded to HTTPPublisher unchanged. A typo currently turns into a remote publish failure instead of a local VALIDATION_ERROR.
Suggested fix
// Otherwise the hosted registry.
if reg := strings.TrimRight(os.Getenv("BUTTONS_REGISTRY_URL"), "/"); reg != "" {
+ switch publishKind {
+ case "button", "drawer":
+ default:
+ return publishConfigError(`invalid --kind: must be "button" or "drawer"`)
+ }
key := registryWriteKey()
if key == "" {
return publishConfigError("registry write key not set: run `buttons batteries set REGISTRY_WRITE_KEY <key>` (or set $BUTTONS_BAT_REGISTRY_WRITE_KEY)")
}
pub := &store.HTTPPublisher{BaseURL: reg, Key: key, Kind: publishKind}Also applies to: 117-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/publish.go` around lines 54 - 60, The publish flow is forwarding
publishKind straight into HTTPPublisher without local validation, so invalid
values only fail after the registry request. Add an early validation step in the
publish path in cmd/publish.go, before constructing the store.HTTPPublisher or
making the POST, so only the supported button/drawer kinds are accepted. If the
kind is invalid, return a local publishConfigError/VALIDATION_ERROR using the
existing publishKind handling in the publish command.
Comment on lines
+101
to
+107
| hdr := &tar.Header{ | ||
| Name: path.Join(wrapper, n), | ||
| Mode: 0o644, | ||
| Size: int64(len(data)), | ||
| ModTime: time.Unix(0, 0).UTC(), | ||
| Typeflag: tar.TypeReg, | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Don't hard-code 0644 for every archived file.
This archive becomes the source for registry installs, so button.json and executable content lose the repo's required mode split. If extraction preserves tar metadata, published buttons will ship with overly permissive JSON and non-executable code files.
Suggested fix
+ mode := archiveModeFor(n)
hdr := &tar.Header{
Name: path.Join(wrapper, n),
- Mode: 0o644,
+ Mode: mode,
Size: int64(len(data)),
ModTime: time.Unix(0, 0).UTC(),
Typeflag: tar.TypeReg,
}func archiveModeFor(name string) int64 {
if name == "button.json" || strings.HasSuffix(name, ".history.json") {
return 0o600
}
return 0o700
}As per coding guidelines, internal/**/*.go: Set file permissions to 0700 for data directories and code files, and 0600 for spec/history JSON files.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| hdr := &tar.Header{ | |
| Name: path.Join(wrapper, n), | |
| Mode: 0o644, | |
| Size: int64(len(data)), | |
| ModTime: time.Unix(0, 0).UTC(), | |
| Typeflag: tar.TypeReg, | |
| } | |
| mode := archiveModeFor(n) | |
| hdr := &tar.Header{ | |
| Name: path.Join(wrapper, n), | |
| Mode: mode, | |
| Size: int64(len(data)), | |
| ModTime: time.Unix(0, 0).UTC(), | |
| Typeflag: tar.TypeReg, | |
| } | |
| func archiveModeFor(name string) int64 { | |
| if name == "button.json" || strings.HasSuffix(name, ".history.json") { | |
| return 0o600 | |
| } | |
| return 0o700 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/store/http_publish.go` around lines 101 - 107, The archive header in
the file-writing path is hard-coding a single mode, which breaks the required
permissions split for published buttons. Update the tar header creation in the
archive-building logic to derive Mode per entry using a helper such as
archiveModeFor, and make sure button.json and any *.history.json files use 0600
while code/content files use 0700 instead of one blanket 0644 setting.
Source: Coding guidelines
bobakemamian
force-pushed
the
feat/publish-to-registry
branch
from
June 29, 2026 17:14
098376f to
2471b2b
Compare
The write-side mirror of HTTPSource. `buttons publish @desk/name` pushes a local button to the registry configured via $BUTTONS_REGISTRY_URL, not only to a local --source dir. - HTTPPublisher (internal/store/http_publish.go): tars the bundle, hashes the tarball, POSTs it bearer-authed with the write key. tarGz is the inverse of untarGz (wrap + sort + zero mtimes) so the artifact round-trips to install. - publish.go: shared loadLocalBundle; PublishToRegistry + splitScoped resolve @desk/name to the bare on-disk button under a scoped registry identity. - cmd/publish: picks registry vs local the way install picks HTTPSource vs LocalSource — $BUTTONS_REGISTRY_URL + the REGISTRY_WRITE_KEY battery. - scoped names ride as two path segments (/v1/buttons/@desk/name/...) via a shared scopedPath helper — no %2F-encoded single segment. - keep the concrete registry host out of this (public) repo: help/comment examples use placeholders; the real endpoint + ops live in the registry repo. - tests: publish->fetch round-trip through both real clients, version immutability (409), auth failure, tarGz<->untarGz losslessness, splitScoped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bobakemamian
force-pushed
the
feat/publish-to-registry
branch
from
June 29, 2026 17:42
2471b2b to
f4937bd
Compare
bobakemamian
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/store/http_publish.go`:
- Around line 70-77: Close the HTTP response body in httpPublish immediately
after p.httpClient().Do(req) succeeds so every path releases resources,
including the non-OK status path that currently returns through registryError
without closing resp.Body. Update the publish flow in the httpPublish function
to defer resp.Body.Close right after the Do call and keep the existing status
handling and error wrapping intact.
- Line 58: The registry publish request in HTTPPublisher.Publish still uses
http.NewRequest without a context, so update it to use
http.NewRequestWithContext instead; since Publish has no context parameter yet,
thread context.Background() into the request as the minimal lint-safe fix. Use
the HTTPPublisher.Publish method and the request creation callsite to locate the
change, and keep the existing endpoint, method, and tarball body behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a551592-04b1-4aeb-85af-cfca5e40a488
📒 Files selected for processing (6)
cmd/install.gocmd/publish.gointernal/store/http_publish.gointernal/store/http_publish_test.gointernal/store/http_source.gointernal/store/publish.go
✅ Files skipped from review due to trivial changes (1)
- cmd/install.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/publish.go
- internal/store/publish.go
- internal/store/http_publish_test.go
| } | ||
| endpoint := strings.TrimRight(p.BaseURL, "/") + | ||
| fmt.Sprintf("/v1/buttons/%s/%s", scopedPath(b.Name), url.PathEscape(b.Version)) | ||
| req, err := http.NewRequest(http.MethodPost, endpoint, bytes.NewReader(tarball)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'http\.NewRequest\(' internal/store/http_publish.goRepository: autonoco/buttons
Length of output: 412
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file outline ==\n'
ast-grep outline internal/store/http_publish.go --view expanded || true
printf '\n== relevant context ==\n'
cat -n internal/store/http_publish.go | sed -n '1,220p'
printf '\n== publisher interface search ==\n'
rg -n 'type .*Publisher|interface .*Publisher|NewRequestWithContext|NewRequest\(' internal -g '*.go'Repository: autonoco/buttons
Length of output: 7374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== publisher interface ==\n'
cat -n internal/store/publish.go
printf '\n== publish callers ==\n'
rg -n 'Publish\(' internal -g '*.go'
printf '\n== request-with-context usage in store package ==\n'
rg -n 'NewRequestWithContext|NewRequest\(' internal/store -g '*.go'Repository: autonoco/buttons
Length of output: 7710
Use NewRequestWithContext for registry publish requests. HTTPPublisher.Publish still has no context to thread through, so context.Background() is the minimal lint-safe stopgap here.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 58-58: net/http.NewRequest must not be called. use net/http.NewRequestWithContext
(noctx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/store/http_publish.go` at line 58, The registry publish request in
HTTPPublisher.Publish still uses http.NewRequest without a context, so update it
to use http.NewRequestWithContext instead; since Publish has no context
parameter yet, thread context.Background() into the request as the minimal
lint-safe fix. Use the HTTPPublisher.Publish method and the request creation
callsite to locate the change, and keep the existing endpoint, method, and
tarball body behavior unchanged.
Source: Linters/SAST tools
Comment on lines
+70
to
+77
| resp, err := p.httpClient().Do(req) | ||
| if err != nil { | ||
| return fmt.Errorf("registry %s: %w", p.BaseURL, err) | ||
| } | ||
| if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated { | ||
| return registryError("publish "+b.Name+"@"+b.Version, resp) | ||
| } | ||
| _ = resp.Body.Close() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Close resp.Body on all response paths.
The body is only closed after a successful status; failed publishes return through registryError before this file closes it. Defer the close immediately after Do succeeds.
Proposed fix
resp, err := p.httpClient().Do(req)
if err != nil {
return fmt.Errorf("registry %s: %w", p.BaseURL, err)
}
+ defer resp.Body.Close()
if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated {
return registryError("publish "+b.Name+"@"+b.Version, resp)
}
- _ = resp.Body.Close()
return nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| resp, err := p.httpClient().Do(req) | |
| if err != nil { | |
| return fmt.Errorf("registry %s: %w", p.BaseURL, err) | |
| } | |
| if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated { | |
| return registryError("publish "+b.Name+"@"+b.Version, resp) | |
| } | |
| _ = resp.Body.Close() | |
| resp, err := p.httpClient().Do(req) | |
| if err != nil { | |
| return fmt.Errorf("registry %s: %w", p.BaseURL, err) | |
| } | |
| defer resp.Body.Close() | |
| if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated { | |
| return registryError("publish "+b.Name+"@"+b.Version, resp) | |
| } | |
| return nil |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/store/http_publish.go` around lines 70 - 77, Close the HTTP response
body in httpPublish immediately after p.httpClient().Do(req) succeeds so every
path releases resources, including the non-OK status path that currently returns
through registryError without closing resp.Body. Update the publish flow in the
httpPublish function to defer resp.Body.Close right after the Do call and keep
the existing status handling and error wrapping intact.
This was referenced Jul 1, 2026
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.
What
The write-side twin of the install loop:
buttons publish @desk/namepushes alocal button to the registry configured via
$BUTTONS_REGISTRY_URL— the mirrorof
buttons install @desk/name. Closes the client half of #276.Until now
buttons publishonly wrote to a local--sourcedirectory.How
internal/store/http_publish.go) — inverse of HTTPSource:tarGz the bundle (wrap + sort + zero mtimes so untarGz round-trips losslessly),
hash the tarball, POST it bearer-authed with the write key. Satisfies Publisher.
loadLocalBundle;PublishToRegistry+splitScopedresolve
@desk/nameto the bare on-disk button under the scoped registry identity.HTTPSource vs LocalSource (
$BUTTONS_REGISTRY_URL+ the write-key battery).(
/v1/buttons/@desk/name/...) via a sharedscopedPath, no%2Fencoding.concrete registry endpoint and publish/deploy ops live in the (private) registry
repo. Help text + comment examples use placeholders.
Test plan
go test ./...green. New store tests: publish→fetch round-trip through both realclients (content hash matches install's pin), duplicate-version 409, auth failure,
requires-name+version,
tarGz↔untarGz,splitScoped.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
buttons publishnow supports publishing from either a local folder or a configured registry.@desk/nameand a new--kindoption for publishing different item types.Bug Fixes