From 8a439e782e7d38dc4844fee225e8c19c8a0ee08c Mon Sep 17 00:00:00 2001 From: Ben Firestone Date: Fri, 1 May 2026 10:16:38 -0700 Subject: [PATCH 1/4] fix(web): preserve quick-label type when popover handles comment body MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a quick-label without a preset (Nit/Issue/Suggestion/Question) routed the user into the comment popover for body text, the picked taxonomy was dropped — every saved annotation became a generic 'comment'. The popover save now reads a held `pendingCommentType` first, falling back to the suggestedText inference only when nothing was explicitly picked. --- web/src/routes/share/[id]/+page.svelte | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/web/src/routes/share/[id]/+page.svelte b/web/src/routes/share/[id]/+page.svelte index d8f1c76..390cb43 100644 --- a/web/src/routes/share/[id]/+page.svelte +++ b/web/src/routes/share/[id]/+page.svelte @@ -54,6 +54,11 @@ let popoverMode = $state(null); let showQuickLabel = $state(false); let activeMarkId = $state(undefined); + // When a quick-label without a preset body routes the user into the + // comment popover (e.g. Nit / Issue / Question), we hold their picked + // taxonomy here so the eventual save uses it instead of falling back + // to the generic 'comment' type. + let pendingCommentType = $state(null); let client: PasteClient | undefined; @@ -139,6 +144,7 @@ activeSelection = null; popoverMode = null; showQuickLabel = false; + pendingCommentType = null; const sel = window.getSelection(); sel?.removeAllRanges(); } @@ -258,7 +264,9 @@ if (!activeSelection) return; await createComment({ body, - comment_type: suggestedText ? 'suggestion' : 'comment', + // pendingCommentType is an explicit user choice from QuickLabelPicker, + // so it wins over the suggestedText inference. + comment_type: pendingCommentType ?? (suggestedText ? 'suggestion' : 'comment'), action: 'comment', suggested_text: suggestedText, anchor: buildAnchor(activeSelection) @@ -279,7 +287,9 @@ }); clearSelection(); } else { - // No preset — open the comment popover so the user can write a body + // No preset — open the comment popover so the user can write a body. + // Remember the picked label so handlePopoverSave can preserve it. + pendingCommentType = label; popoverMode = 'comment'; } } From ffc0c6efe17cfe7676c04ec631eea95b7c62b095 Mon Sep 17 00:00:00 2001 From: Ben Firestone Date: Fri, 1 May 2026 10:46:01 -0700 Subject: [PATCH 2/4] fix(arc-paste): default-deny non-share routes at edge and in binary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit arc-paste only owns /share/, /api/paste/*, /_app/* and /robots.txt. Previously the embedded SPA fallback served the arc app shell at /, and the SPA's /api/v1/* boot probes fell through to that fallback and returned HTML — which the browser then failed to JSON.parse. Lock the surface down both at Caddy and in the Echo router so dev (no Caddy) and prod behave identically. Adds tests covering the / 404, /api/v1/* JSON 404, and the existing share-create path against the same router used in production. --- arc-paste/Caddyfile | 31 ++++++++++++++++++++--- arc-paste/main.go | 29 +++++++++++++++------ arc-paste/main_test.go | 57 ++++++++++++++++++++++++++++++++++++------ 3 files changed, 99 insertions(+), 18 deletions(-) diff --git a/arc-paste/Caddyfile b/arc-paste/Caddyfile index 1c2f792..b51daac 100644 --- a/arc-paste/Caddyfile +++ b/arc-paste/Caddyfile @@ -6,7 +6,6 @@ # To change the public hostname, edit the site address below. To change the # ACME contact, edit `email` in the global block. Reload without downtime via: # docker compose -f arc-paste/compose.yaml exec caddy caddy reload --config /etc/caddy/Caddyfile - { # Let's Encrypt sends expiry + revocation notices here. Without it, you # only learn about cert problems when the site goes down. @@ -17,9 +16,33 @@ arcpaste.company.com { # Negotiate compression with the client. zstd preferred, gzip fallback. encode zstd gzip - # arc-paste listens on the internal Docker network — the service name - # resolves via Compose's embedded DNS. - reverse_proxy arc-paste:7433 + # Default-deny edge: arc-paste only needs to expose four things to render + # a share — the share route itself, SvelteKit's hashed asset bundle, the + # paste API, and robots.txt. Anything else (the arc app shell at /, stray + # /api/v1/* probes from the SPA, /labels, /planner/*, etc.) is rejected + # before it reaches the upstream. arc-paste/main.go has matching in-binary + # guards so dev (no Caddy) behaves the same. + # + # Note: directives inside one named-matcher block are AND'd. To OR + # different matcher *kinds* (path-list vs. regex), define them separately + # and use multiple handle blocks pointing at the same upstream. + # /api/paste covers `arc share create` (POST to bare /api/paste); + # /api/paste/* covers GET/PUT/append against an existing share id. + @assets path /_app/* /api/paste /api/paste/* /robots.txt + @share path_regexp ^/share/[^/]+$ + + handle @assets { + # arc-paste listens on the internal Docker network — the service + # name resolves via Compose's embedded DNS. + reverse_proxy arc-paste:7433 + } + handle @share { + reverse_proxy arc-paste:7433 + } + + handle { + respond "Not found" 404 + } # Structured access logs to stdout so `docker compose logs caddy` # stays parseable by jq / log shippers. diff --git a/arc-paste/main.go b/arc-paste/main.go index 4e7b902..63b3cba 100644 --- a/arc-paste/main.go +++ b/arc-paste/main.go @@ -8,6 +8,7 @@ import ( "database/sql" "fmt" "log" + "net/http" "os" "path/filepath" @@ -54,9 +55,13 @@ func run() error { return fmt.Errorf("apply migrations: %w", err) } - store := pastesqlite.New(db) - handlers := paste.NewHandlers(store) + handlers := paste.NewHandlers(pastesqlite.New(db)) + return newRouter(handlers).Start(addr) +} +// newRouter wires the arc-paste HTTP surface. Extracted so tests can exercise +// route precedence (/, /api/v1/*, SPA fallback) without binding a real listener. +func newRouter(handlers *paste.Handlers) *echo.Echo { e := echo.New() e.HideBanner = true e.Use(middleware.RequestLoggerWithConfig(middleware.RequestLoggerConfig{ @@ -72,13 +77,23 @@ func run() error { e.Use(middleware.Recover()) e.Use(middleware.CORS()) - // Mount paste handlers at /api/paste - handlers.Register(e.Group("/api/paste")) + // arc-paste only owns /api/paste/*. The arc SPA shell probes /api/v1/* on + // boot; without this guard those probes fall through to the SPA fallback + // and return HTML, which the browser then fails to JSON.parse. + e.Any("/api/v1/*", func(c echo.Context) error { + return c.JSON(http.StatusNotFound, map[string]string{"error": "not found"}) + }) - // Serve embedded SPA with fallback to index.html for routing - web.RegisterSPA(e) + // Paste deployments don't serve the arc app shell — the only useful + // destination is /share/. Mirrors the Caddy edge config so dev and + // prod behave the same. + e.Any("/", func(c echo.Context) error { + return c.String(http.StatusNotFound, "Not found") + }) - return e.Start(addr) + handlers.Register(e.Group("/api/paste")) + web.RegisterSPA(e) + return e } // envOr returns the value of env variable key, or defaultVal if not set. diff --git a/arc-paste/main_test.go b/arc-paste/main_test.go index c1adb7c..a4829ad 100644 --- a/arc-paste/main_test.go +++ b/arc-paste/main_test.go @@ -7,21 +7,29 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" - "github.com/labstack/echo/v4" "github.com/sentiolabs/arc/internal/paste" pastesqlite "github.com/sentiolabs/arc/internal/paste/sqlite" _ "modernc.org/sqlite" ) -func TestArcPasteCreate(t *testing.T) { - db, _ := sql.Open("sqlite", ":memory:") - defer db.Close() - _ = pastesqlite.Apply(context.Background(), db) - e := echo.New() - paste.NewHandlers(pastesqlite.New(db)).Register(e.Group("/api/paste")) +func newTestRouter(t *testing.T) http.Handler { + t.Helper() + db, err := sql.Open("sqlite", ":memory:") + if err != nil { + t.Fatalf("open db: %v", err) + } + t.Cleanup(func() { db.Close() }) + if err := pastesqlite.Apply(context.Background(), db); err != nil { + t.Fatalf("migrate: %v", err) + } + return newRouter(paste.NewHandlers(pastesqlite.New(db))) +} +func TestArcPasteCreate(t *testing.T) { + e := newTestRouter(t) body, _ := json.Marshal(map[string]any{ "plan_blob": []byte{1, 2, 3}, "plan_iv": []byte{4, 5, 6}, @@ -35,3 +43,38 @@ func TestArcPasteCreate(t *testing.T) { t.Fatalf("expected 201, got %d", rec.Code) } } + +// arc-paste only owns /api/paste/*. The arc SPA shell calls /api/v1/projects +// on boot; without an explicit guard those requests fall through to the SPA +// fallback and return HTML, which the browser fails to JSON.parse. Return a +// clean JSON 404 so the SPA gets a parseable error instead of a crash. +func TestArcPasteRejectsArcAPIProbes(t *testing.T) { + e := newTestRouter(t) + for _, path := range []string{"/api/v1/projects", "/api/v1/workspaces", "/api/v1/anything/nested"} { + rec := httptest.NewRecorder() + e.ServeHTTP(rec, httptest.NewRequest("GET", path, nil)) + if rec.Code != http.StatusNotFound { + t.Errorf("%s: expected 404, got %d", path, rec.Code) + } + if ct := rec.Header().Get("Content-Type"); !strings.Contains(ct, "application/json") { + t.Errorf("%s: expected JSON content-type, got %q", path, ct) + } + var body map[string]string + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Errorf("%s: body is not JSON: %v (%q)", path, err, rec.Body.String()) + } + } +} + +// arc-paste deployments don't serve the arc app shell — the only useful +// destination is /share/. Returning 404 at the root mirrors the Caddy +// edge configuration so dev (localhost) and prod (arcpaste.company.com) +// behave identically. +func TestArcPasteRootIs404(t *testing.T) { + e := newTestRouter(t) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, httptest.NewRequest("GET", "/", nil)) + if rec.Code != http.StatusNotFound { + t.Fatalf("expected 404 at /, got %d (body=%q)", rec.Code, rec.Body.String()) + } +} From 3103f62c701aeb5dfabda9dc2483d78e7928db28 Mon Sep 17 00:00:00 2001 From: Ben Firestone Date: Fri, 1 May 2026 11:01:48 -0700 Subject: [PATCH 3/4] refactor(share): drop --local/--share, add --remote, print one URL by kind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `arc share create` previously printed both the reviewer URL and the author URL on every successful create. Copy-pasting the wrong line handed recipients author privileges via the &t= token, and for --local shares the printed reviewer URL was actively misleading — a localhost link isn't reachable by anyone else. Output now branches on share kind: - local -> "Preview URL (local-only — not reachable by others):" - shared -> "Author URL (keep private — open it, then use the in-page Share link button to copy a reviewer URL):" Reviewer URL is no longer printed by the CLI in either mode. The author opens the link and copies a `#k=…`-only URL via the existing in-page Share-link button (web/src/routes/share/[id]/+page.svelte:copyShareLink), which already strips &t=. Flag surface tightened: - --local was a no-op (default) -> dropped - --share renamed to --remote (cleaner reads next to --server foo, avoids the awkward "share … --share" stutter) - --server unchanged; still wins over --remote and forces shared `resolveServer` signature simplified from (local, share, override) to (remote, override). New unit tests assert the URL output for both kinds and the absence of the old "Share URL" / "send to reviewers" lines. Runbook and config doc comment updated to match. --- cmd/arc/main.go | 4 +- cmd/arc/share.go | 27 ++++--- cmd/arc/share_test.go | 146 ++++++++++++++++++++++++++++------ docs/runbooks/paste-server.md | 29 +++---- 4 files changed, 151 insertions(+), 55 deletions(-) diff --git a/cmd/arc/main.go b/cmd/arc/main.go index dda5a56..c2dcdf6 100644 --- a/cmd/arc/main.go +++ b/cmd/arc/main.go @@ -90,9 +90,9 @@ type Config struct { // 4. `git config user.name` ShareAuthor string `json:"share_author,omitempty"` // ShareServer is the default URL for the remote paste server used by - // `arc share create --share`. Lets users persistently target a private + // `arc share create --remote`. Lets users persistently target a private // arc-paste deployment instead of the public default. - // Resolution precedence in `arc share create --share`: + // Resolution precedence in `arc share create --remote`: // 1. --server flag (highest) // 2. this config field // 3. $ARC_SHARE_SERVER diff --git a/cmd/arc/share.go b/cmd/arc/share.go index a300b53..ab33329 100644 --- a/cmd/arc/share.go +++ b/cmd/arc/share.go @@ -189,7 +189,6 @@ var shareDeleteCmd = &cobra.Command{ } var ( - shareCreateLocal bool shareCreateRemote bool shareCreateServer string shareCreateAuthor string @@ -201,8 +200,7 @@ var ( ) func init() { - shareCreateCmd.Flags().BoolVar(&shareCreateLocal, "local", false, "Use the local arc-server") - shareCreateCmd.Flags().BoolVar(&shareCreateRemote, "share", false, "Use the configured remote share server") + shareCreateCmd.Flags().BoolVar(&shareCreateRemote, "remote", false, "Use the configured remote share server (precedence: --server flag > share_server in cli-config.json > $ARC_SHARE_SERVER > built-in default). Without --remote, --server, or an explicit URL, the share is created on the local arc-server.") shareCreateCmd.Flags().StringVar(&shareCreateServer, "server", "", "Server URL override (precedence: flag > share_server in cli-config.json > "+ "$ARC_SHARE_SERVER > built-in default).") @@ -234,7 +232,7 @@ func runShareCreate(cmd *cobra.Command, args []string) error { if err != nil { return err } - server, kind := resolveServer(shareCreateLocal, shareCreateRemote, shareCreateServer) + server, kind := resolveServer(shareCreateRemote, shareCreateServer) key, err := paste.GenerateKey() if err != nil { return err @@ -279,10 +277,16 @@ func runShareCreate(cmd *cobra.Command, args []string) error { return err } trimmedServer := strings.TrimRight(server, "/") - fmt.Printf("Share URL (send to reviewers):\n %s/share/%s#k=%s\n\n", - trimmedServer, resp.ID, keyB64) - fmt.Printf("Author URL (keep private — gives you Accept/Resolve):\n %s/share/%s#k=%s&t=%s\n\n", - trimmedServer, resp.ID, keyB64, resp.EditToken) + authorURL := fmt.Sprintf("%s/share/%s#k=%s&t=%s", trimmedServer, resp.ID, keyB64, resp.EditToken) + // Reviewer URL is intentionally NOT printed — copy-pasting the wrong line + // would hand a recipient author privileges (the &t= grants + // Accept/Resolve/Reject). Authors get a reviewer URL by opening the link + // below and clicking the in-page "Share link" button, which strips &t=. + if kind == shareKindLocal { + fmt.Printf("Preview URL (local-only — not reachable by others):\n %s\n\n", authorURL) + } else { + fmt.Printf("Author URL (keep private — open it, then use the in-page Share link button to copy a reviewer URL):\n %s\n\n", authorURL) + } fmt.Println("Edit token saved to the local arc keyring") return nil } @@ -489,12 +493,13 @@ func resolveAuthor(flag string) string { // // For local mode, the server URL comes from `server_url` in the CLI config // (defaulting to http://localhost:7432). The `--server` flag still wins over -// everything in either mode. -func resolveServer(_, share bool, override string) (server, kind string) { +// everything in either mode — passing a URL there forces shared mode regardless +// of `--remote`. +func resolveServer(remote bool, override string) (server, kind string) { if s := strings.TrimSpace(override); s != "" { return s, shareKindShared } - if share { + if remote { return resolveShareServer(), shareKindShared } return cliConfigServerURL(), shareKindLocal diff --git a/cmd/arc/share_test.go b/cmd/arc/share_test.go index 4c2d403..313c227 100644 --- a/cmd/arc/share_test.go +++ b/cmd/arc/share_test.go @@ -614,7 +614,12 @@ func captureStdout(t *testing.T, fn func()) string { // global `configPath` at a temp file, and from the user's git identity by // running each subtest with $PATH cleared so `git` is unavailable (the // helper falls back silently to "" on git failure). -func TestShareCreatePrintsBothURLs(t *testing.T) { +// In shared (remote) mode, `arc share create` prints exactly one URL — the +// Author URL with &t= — labeled with privacy guidance and a pointer at the +// in-page Share-link button. The standalone reviewer URL line is gone: +// printing it was a footgun (copy-pasting the wrong line gave recipients +// author privileges via the &t= token). +func TestShareCreatePrintsAuthorURLOnly(t *testing.T) { home := t.TempDir() t.Setenv("HOME", home) srv := startTestPasteServer(t) @@ -623,13 +628,13 @@ func TestShareCreatePrintsBothURLs(t *testing.T) { plan := filepath.Join(t.TempDir(), "plan.md") _ = os.WriteFile(plan, []byte("# Hello\n\nBody."), 0o600) - // Capture stdout. r, w, _ := os.Pipe() stdout := os.Stdout os.Stdout = w defer func() { os.Stdout = stdout }() - shareCreateServer = srv.URL + shareCreateServer = srv.URL // forces shared mode regardless of --remote + t.Cleanup(func() { shareCreateServer = "" }) if err := runShareCreate(shareCreateCmd, []string{plan}); err != nil { t.Fatalf("runShareCreate: %v", err) } @@ -637,18 +642,25 @@ func TestShareCreatePrintsBothURLs(t *testing.T) { out, _ := io.ReadAll(r) output := string(out) - if !strings.Contains(output, "Share URL") { - t.Errorf("expected 'Share URL' label, got: %s", output) - } if !strings.Contains(output, "Author URL") { t.Errorf("expected 'Author URL' label, got: %s", output) } - if !strings.Contains(output, "send to reviewers") { - t.Errorf("expected reviewer guidance, got: %s", output) - } if !strings.Contains(output, "keep private") { t.Errorf("expected privacy guidance for author URL, got: %s", output) } + if !strings.Contains(output, "Share link button") { + t.Errorf("expected pointer at the in-page Share-link button, got: %s", output) + } + // Reviewer-URL artifacts from the old dual-print MUST be gone. + if strings.Contains(output, "Share URL") { + t.Errorf("'Share URL' label leaked back into output: %s", output) + } + if strings.Contains(output, "send to reviewers") { + t.Errorf("'send to reviewers' guidance leaked back into output: %s", output) + } + if strings.Contains(output, "Preview URL") { + t.Errorf("'Preview URL' label appeared in shared-mode output: %s", output) + } if strings.Contains(output, "Edit token: ") { t.Errorf("raw 'Edit token: ' line should not appear in output: %s", output) } @@ -656,13 +668,96 @@ func TestShareCreatePrintsBothURLs(t *testing.T) { t.Errorf("expected pointer to shares.json, got: %s", output) } - // Author URL must contain &t=. + // Exactly one /share/ line should appear, and it must carry &t=. + shareLines := 0 + hasToken := false for line := range strings.SplitSeq(output, "\n") { - if strings.Contains(line, "/share/") && strings.Contains(line, "&t=") { - return + if strings.Contains(line, "/share/") { + shareLines++ + if strings.Contains(line, "&t=") { + hasToken = true + } } } - t.Errorf("expected an author URL line with &t=, got: %s", output) + if shareLines != 1 { + t.Errorf("expected exactly one /share/ line, got %d. output: %s", shareLines, output) + } + if !hasToken { + t.Errorf("expected the single share line to carry &t=, got: %s", output) + } +} + +// In local (default) mode, `arc share create` labels the URL "Preview URL" +// and notes it's local-only — printing a "Share URL" / "Author URL" label +// would suggest the link is shareable, but a localhost URL can't be opened +// by anyone else. +func TestShareCreatePrintsPreviewURLForLocal(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + srv := startTestPasteServer(t) + defer srv.Close() + + // Point the local CLI config at the test server so cliConfigServerURL() + // returns srv.URL during this test. The shared/remote escape hatches + // (--server flag, --remote) are deliberately left clear. + origConfigPath := configPath + t.Cleanup(func() { configPath = origConfigPath }) + dir := t.TempDir() + configPath = filepath.Join(dir, "cli-config.json") + if err := os.WriteFile(configPath, []byte(`{"server_url":"`+srv.URL+`"}`), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + + plan := filepath.Join(t.TempDir(), "plan.md") + _ = os.WriteFile(plan, []byte("# Hello\n\nBody."), 0o600) + + r, w, _ := os.Pipe() + stdout := os.Stdout + os.Stdout = w + defer func() { os.Stdout = stdout }() + + // shareCreateRemote is false and shareCreateServer is empty → local mode. + shareCreateRemote = false + shareCreateServer = "" + if err := runShareCreate(shareCreateCmd, []string{plan}); err != nil { + t.Fatalf("runShareCreate: %v", err) + } + _ = w.Close() + out, _ := io.ReadAll(r) + output := string(out) + + if !strings.Contains(output, "Preview URL") { + t.Errorf("expected 'Preview URL' label, got: %s", output) + } + if !strings.Contains(output, "local-only") { + t.Errorf("expected 'local-only' guidance, got: %s", output) + } + if strings.Contains(output, "Author URL") { + t.Errorf("'Author URL' label should NOT appear for local shares, got: %s", output) + } + if strings.Contains(output, "Share URL") { + t.Errorf("'Share URL' label should NOT appear, got: %s", output) + } + + // Even for local, the printed URL is the author-token URL — it's the + // preview the author opens themselves. Just confirm exactly one /share/ + // line and that it carries &t=. + shareLines := 0 + hasToken := false + for line := range strings.SplitSeq(output, "\n") { + if strings.Contains(line, "/share/") { + shareLines++ + if strings.Contains(line, "&t=") { + hasToken = true + } + } + } + if shareLines != 1 { + t.Errorf("expected exactly one /share/ line, got %d. output: %s", shareLines, output) + } + if !hasToken { + t.Errorf("expected the preview URL line to carry &t=, got: %s", output) + } } func TestResolveAuthor(t *testing.T) { @@ -749,8 +844,7 @@ func TestResolveServer(t *testing.T) { name string config string // share_server in cli-config.json; "" omits the field env string // ARC_SHARE_SERVER value; "" clears it - share bool - local bool + remote bool flag string wantURL string wantKind string @@ -759,38 +853,38 @@ func TestResolveServer(t *testing.T) { { name: "flag wins over everything", config: "https://from-config.example", env: "https://from-env.example", - share: true, flag: "https://from-flag.example", + remote: true, flag: "https://from-flag.example", wantURL: "https://from-flag.example", wantKind: shareKindShared, }, { // The override flag is intentionally global — it forces shared mode - // regardless of which boolean flags are set, mirroring the previous - // behavior. Locked in here so it doesn't silently drift. - name: "flag wins even without --share", - config: "https://from-config.example", - local: true, flag: "https://from-flag.example", wantNoEnv: true, + // even without --remote, mirroring the previous behavior. Locked in + // here so it doesn't silently drift. + name: "flag wins even without --remote", + config: "https://from-config.example", + flag: "https://from-flag.example", wantNoEnv: true, wantURL: "https://from-flag.example", wantKind: shareKindShared, }, { name: "config wins over env when no flag", config: "https://from-config.example", env: "https://from-env.example", - share: true, + remote: true, wantURL: "https://from-config.example", wantKind: shareKindShared, }, { name: "env wins when config empty", - env: "https://from-env.example", share: true, + env: "https://from-env.example", remote: true, wantURL: "https://from-env.example", wantKind: shareKindShared, }, { name: "falls back to built-in default", - share: true, + remote: true, wantURL: builtinDefault, wantKind: shareKindShared, }, { name: "flag whitespace is trimmed and treated as empty", config: "https://from-config.example", - share: true, flag: " ", + remote: true, flag: " ", wantURL: "https://from-config.example", wantKind: shareKindShared, }, } @@ -801,7 +895,7 @@ func TestResolveServer(t *testing.T) { if !tc.wantNoEnv { t.Setenv("ARC_SHARE_SERVER", tc.env) } - got, kind := resolveServer(tc.local, tc.share, tc.flag) + got, kind := resolveServer(tc.remote, tc.flag) if got != tc.wantURL || kind != tc.wantKind { t.Errorf("resolveServer = (%q, %q), want (%q, %q)", got, kind, tc.wantURL, tc.wantKind) } diff --git a/docs/runbooks/paste-server.md b/docs/runbooks/paste-server.md index 3e416ea..18a3d16 100644 --- a/docs/runbooks/paste-server.md +++ b/docs/runbooks/paste-server.md @@ -58,12 +58,9 @@ Validate the shared review feature. - Resolve / accept / reject EOF -./bin/arc share create /tmp/test-plan.md --local +./bin/arc share create /tmp/test-plan.md # Output: -# Share URL (send to reviewers): -# http://localhost:7432/share/#k= -# -# Author URL (keep private — gives you Accept/Resolve): +# Preview URL (local-only — not reachable by others): # http://localhost:7432/share/#k=&t= # # Edit token saved to ~/.arc/shares.json @@ -129,10 +126,10 @@ The author's resolution events still apply: their UI is gated on `authorToken` ( Walk these in order to confirm the new auth UX end-to-end: -1. **Create a share.** `arc share create --local` — confirm output prints both Share URL and Author URL, no raw `Edit token: ` line, and a "saved to ~/.arc/shares.json" pointer. +1. **Create a share.** `arc share create ` — confirm output prints a single `Preview URL` line (containing `&t=`), no raw `Edit token: ` line, and a "saved to ~/.arc/shares.json" pointer. 2. **Reprint the author URL.** `arc share show --author-url` — single line, contains `&t=`. -3. **Author URL flow.** Open the Author URL in a fresh browser profile. Header chip shows ` · author` immediately, no modal. Accept / Resolve buttons visible on existing comments. -4. **Reviewer URL flow.** Open the Share URL (without `&t=`) in a different fresh profile. No chip in the header, no "Sign in" button. Select text → click Comment in the toolbar → name modal opens → save a name → comment posts → chip now shows the name. +3. **Author URL flow.** Open the Preview URL in a fresh browser profile. Header chip shows ` · author` immediately, no modal. Accept / Resolve buttons visible on existing comments. +4. **Reviewer URL flow.** From the author's browser tab, click the **Share link** button in the page header — clipboard now holds the bare reviewer URL (`#k=…` only, no `&t=`). Open that copied URL in a different fresh profile. No chip in the header, no "Sign in" button. Select text → click Comment in the toolbar → name modal opens → save a name → comment posts → chip now shows the name. 5. **Rename via chip.** Click the chip on the reviewer side. Modal opens prefilled with the saved name (text selected). Edit, save → chip updates. 6. **Author opens the bare share URL.** Open the share URL (without `&t=`) on the author's browser. They are now in reviewer mode (no Accept buttons). Reopen via the Author URL → author mode restored. @@ -166,14 +163,14 @@ Make sure DNS for `arcpaste.company.com` points to the host and ports 80/443 are ```bash # Terminal 2 (Docker/Caddy stack) -./bin/arc share create /tmp/test-plan.md --share --server https://arcpaste.company.com -# → Share URL: https://arcpaste.company.com/share/#k= +./bin/arc share create /tmp/test-plan.md --server https://arcpaste.company.com +# → Author URL: https://arcpaste.company.com/share/#k=&t= # If you launched the standalone binary directly instead, use: -# ./bin/arc share create /tmp/test-plan.md --share --server http://localhost:7433 +# ./bin/arc share create /tmp/test-plan.md --server http://localhost:7433 ``` -That URL is what you'd send to a real reviewer (Slack, email, etc.). It contains everything they need: the share id and the decryption key in the fragment. +The CLI prints only the Author URL — keep it private. To send a reviewer link (Slack, email, etc.), open the Author URL in your browser and click the **Share link** button in the page header; that copies a `#k=…`-only URL with no `&t=` token. ### Pull the comments back @@ -192,7 +189,7 @@ To exercise the actual "remote" code paths (cross-origin, no shared filesystem), ./bin/arc-paste # From your laptop: -./bin/arc share create plan.md --share --server https://share.example.com +./bin/arc share create plan.md --server https://share.example.com ``` For a persistent default, set `share_server` in `~/.arc/cli-config.json`: @@ -205,7 +202,7 @@ For a persistent default, set `share_server` in `~/.arc/cli-config.json`: } ``` -Then `arc share create plan.md --share` will pick it up without any flag or env var. The full precedence is `--server flag → share_server in cli-config.json → $ARC_SHARE_SERVER → https://arcplanner.sentiolabs.io`. +Then `arc share create plan.md --remote` will pick it up without any flag or env var. The full precedence is `--server flag → share_server in cli-config.json → $ARC_SHARE_SERVER → https://arcplanner.sentiolabs.io`. ## 4. End-to-end via the brainstorm skill @@ -217,8 +214,8 @@ In a new Claude Code session, the agent-nexus brainstorm skill update can be exe When the skill reaches step 6, it should now offer three options via `AskUserQuestion`: -- **Local review** → invokes `arc share create --local` -- **Shared review** → invokes `arc share create --share` +- **Local review** → invokes `arc share create` (local is the default) +- **Shared review** → invokes `arc share create --remote` - **Save for later** → no server registration Step 7 (review loop) uses `arc share approve` and `arc share pull` instead of the legacy `arc plan *` commands. From 8a34fea45105e1436066dc150964c7fd4413f6de Mon Sep 17 00:00:00 2001 From: Ben Firestone Date: Fri, 1 May 2026 12:11:11 -0700 Subject: [PATCH 4/4] chore: fix lint errors - gofmt cmd/arc/share_test.go (struct field alignment) - wrap two over-120-char lines in cmd/arc/share.go (revive line-length-limit) - handle db.Close() error explicitly in arc-paste/main_test.go (revive unhandled-error) --- arc-paste/main_test.go | 2 +- cmd/arc/share.go | 8 ++++++-- cmd/arc/share_test.go | 6 +++--- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/arc-paste/main_test.go b/arc-paste/main_test.go index a4829ad..6aeabc4 100644 --- a/arc-paste/main_test.go +++ b/arc-paste/main_test.go @@ -21,7 +21,7 @@ func newTestRouter(t *testing.T) http.Handler { if err != nil { t.Fatalf("open db: %v", err) } - t.Cleanup(func() { db.Close() }) + t.Cleanup(func() { _ = db.Close() }) if err := pastesqlite.Apply(context.Background(), db); err != nil { t.Fatalf("migrate: %v", err) } diff --git a/cmd/arc/share.go b/cmd/arc/share.go index ab33329..de7f570 100644 --- a/cmd/arc/share.go +++ b/cmd/arc/share.go @@ -200,7 +200,10 @@ var ( ) func init() { - shareCreateCmd.Flags().BoolVar(&shareCreateRemote, "remote", false, "Use the configured remote share server (precedence: --server flag > share_server in cli-config.json > $ARC_SHARE_SERVER > built-in default). Without --remote, --server, or an explicit URL, the share is created on the local arc-server.") + shareCreateCmd.Flags().BoolVar(&shareCreateRemote, "remote", false, + "Use the configured remote share server (precedence: --server flag > share_server in "+ + "cli-config.json > $ARC_SHARE_SERVER > built-in default). Without --remote, --server, "+ + "or an explicit URL, the share is created on the local arc-server.") shareCreateCmd.Flags().StringVar(&shareCreateServer, "server", "", "Server URL override (precedence: flag > share_server in cli-config.json > "+ "$ARC_SHARE_SERVER > built-in default).") @@ -285,7 +288,8 @@ func runShareCreate(cmd *cobra.Command, args []string) error { if kind == shareKindLocal { fmt.Printf("Preview URL (local-only — not reachable by others):\n %s\n\n", authorURL) } else { - fmt.Printf("Author URL (keep private — open it, then use the in-page Share link button to copy a reviewer URL):\n %s\n\n", authorURL) + fmt.Printf("Author URL (keep private — open it, then use the in-page "+ + "Share link button to copy a reviewer URL):\n %s\n\n", authorURL) } fmt.Println("Edit token saved to the local arc keyring") return nil diff --git a/cmd/arc/share_test.go b/cmd/arc/share_test.go index 313c227..3a7efe4 100644 --- a/cmd/arc/share_test.go +++ b/cmd/arc/share_test.go @@ -860,9 +860,9 @@ func TestResolveServer(t *testing.T) { // The override flag is intentionally global — it forces shared mode // even without --remote, mirroring the previous behavior. Locked in // here so it doesn't silently drift. - name: "flag wins even without --remote", - config: "https://from-config.example", - flag: "https://from-flag.example", wantNoEnv: true, + name: "flag wins even without --remote", + config: "https://from-config.example", + flag: "https://from-flag.example", wantNoEnv: true, wantURL: "https://from-flag.example", wantKind: shareKindShared, }, {