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..6aeabc4 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()) + } +} 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..de7f570 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,10 @@ 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 +235,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 +280,17 @@ 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 +497,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..3a7efe4 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/") { + 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 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/") && 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 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", + // 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", - local: true, flag: "https://from-flag.example", wantNoEnv: true, + 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. 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'; } }