From c4a05753058f272fe226b02b56603e136aa2101c Mon Sep 17 00:00:00 2001 From: Tink bot Date: Thu, 21 May 2026 13:30:56 +0000 Subject: [PATCH] feat(veans): offer "create a new project" from init's picker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The project picker used to require at least one pre-existing project and would otherwise hard-error: "no projects visible to this user — create one in the Vikunja UI first". Now it always offers an extra numbered entry "Create a new project" and, when the user picks it, prompts for a title (required) + identifier (optional). Empty-list case routes straight to creation. Backed by a new client.CreateProject(ctx, *Project) method (`PUT /projects`); the e2e harness now uses that instead of the raw c.Do call it did before. Also fixed a latent bufio bug in StdPrompter.ReadLine that this work surfaced: every call created a fresh bufio.Reader, which read-ahead a buffer and threw it away on return. Second+ prompts read empty. Reuse one buffered reader on the StdPrompter instance. --- .github/workflows/test.yml | 24 ++-- veans/AGENTS.md | 49 +++++--- veans/e2e/helpers.go | 151 ++++++++++++++++------ veans/e2e/init_test.go | 7 +- veans/e2e/main_test.go | 36 ++++++ veans/e2e/shared_test.go | 29 +---- veans/e2e/tasks_test.go | 10 ++ veans/go.mod | 6 +- veans/internal/auth/auth.go | 17 ++- veans/internal/auth/oauth.go | 16 ++- veans/internal/bootstrap/bootstrap.go | 132 +++++++++++++++++++- veans/internal/bootstrap/hooks.go | 82 +++++++++--- veans/internal/client/client.go | 125 +++++++++++++++++-- veans/internal/client/labels.go | 9 +- veans/internal/client/projects.go | 22 +++- veans/internal/client/tasks.go | 20 ++- veans/internal/client/types.go | 21 ++-- veans/internal/commands/api.go | 28 +++-- veans/internal/commands/claim.go | 10 +- veans/internal/commands/create.go | 2 +- veans/internal/commands/git.go | 27 +++- veans/internal/commands/init.go | 36 ++++-- veans/internal/commands/list.go | 35 ++++-- veans/internal/commands/list_test.go | 152 +++++++++++++++++++++++ veans/internal/commands/prime.go | 10 +- veans/internal/commands/update.go | 4 +- veans/internal/credentials/file.go | 90 ++++++++++++-- veans/internal/credentials/lock_other.go | 28 +++++ veans/internal/credentials/lock_unix.go | 36 ++++++ veans/internal/output/errors.go | 2 +- veans/internal/status/status.go | 4 +- veans/magefile.go | 19 +-- 32 files changed, 1018 insertions(+), 221 deletions(-) create mode 100644 veans/e2e/main_test.go create mode 100644 veans/internal/commands/list_test.go create mode 100644 veans/internal/credentials/lock_other.go create mode 100644 veans/internal/credentials/lock_unix.go diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 83bec2398..1f4fb6790 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -455,6 +455,10 @@ jobs: VIKUNJA_SERVICE_INTERFACE: ":3456" VIKUNJA_SERVICE_PUBLICURL: "http://127.0.0.1:3456/" VIKUNJA_SERVICE_JWTSECRET: "veans-e2e-jwt-secret-do-not-use-in-production" + # Enables PATCH /api/v1/test/{table} — the e2e suite seeds its + # own admin via this endpoint (see veans/e2e/helpers.go), same + # mechanism the playwright suite uses. + VIKUNJA_SERVICE_TESTINGTOKEN: averyLongSecretToSe33dtheDB VIKUNJA_DATABASE_TYPE: sqlite VIKUNJA_DATABASE_PATH: memory VIKUNJA_LOG_LEVEL: WARNING @@ -462,8 +466,9 @@ jobs: VIKUNJA_REDIS_ENABLED: "false" VIKUNJA_RATELIMIT_NOAUTHLIMIT: "1000" VEANS_E2E_API_URL: http://127.0.0.1:3456 - VEANS_E2E_ADMIN_USER: e2eadmin - VEANS_E2E_ADMIN_PASS: e2etestpassword + # Same value as VIKUNJA_SERVICE_TESTINGTOKEN above — pass-through + # so the test harness can authenticate against /api/v1/test/. + VEANS_E2E_TESTING_TOKEN: averyLongSecretToSe33dtheDB run: | set -e # Boot the prebuilt API and tests in one shell — backgrounded @@ -483,16 +488,11 @@ jobs: cat /tmp/vikunja.log exit 1 fi - # The sqlite-memory DB has no fixtures loaded — register the - # admin the suite logs in as. service.enableregistration - # defaults to true. - curl -sf -X POST http://127.0.0.1:3456/api/v1/register \ - -H 'Content-Type: application/json' \ - -d "{\"username\":\"$VEANS_E2E_ADMIN_USER\",\"password\":\"$VEANS_E2E_ADMIN_PASS\",\"email\":\"e2e@example.com\"}" \ - > /dev/null - # `mage test` runs unit + e2e packages; the e2e suite self-skips - # when VEANS_E2E_API_URL is unset so the same target works locally. - (cd veans && mage test) + # `mage test:e2e` builds the binary once and exports VEANS_BINARY + # so each subtest reuses it (plain `mage test` would rebuild per + # test via buildOrLocate()). The suite seeds its own admin + # internally — no curl seeding here. + (cd veans && mage test:e2e) - name: Upload API log on failure if: failure() uses: actions/upload-artifact@b7c566a772e6b6bfb58ed0dc250532a479d7789f # v6 diff --git a/veans/AGENTS.md b/veans/AGENTS.md index 3545fa055..bb594cf95 100644 --- a/veans/AGENTS.md +++ b/veans/AGENTS.md @@ -20,20 +20,29 @@ this file is veans-specific. - `mage build` → `./veans` binary. The `Aliases` map in `magefile.go` routes bare names like `mage test` to `Test.All` — without aliases, mage rejects namespace invocations ("Unknown target specified"). -- Unit tests: `mage test` or `go test ./...`. -- E2e tests: assume an externally-running Vikunja at `VEANS_E2E_API_URL` - and admin creds in env (`VEANS_E2E_ADMIN_TOKEN`, or - `VEANS_E2E_ADMIN_USER` + `VEANS_E2E_ADMIN_PASS`). The package - self-skips when `VEANS_E2E_API_URL` is empty, so plain `go test` is - safe locally. +- Unit tests: `mage test` (passes `-short`) or `go test -short ./...`. + The e2e package's `TestMain` gates the suite on `-short`, mirroring + the parent monorepo's `pkg/webtests` convention. Without `-short` + and without `VEANS_E2E_API_URL` set, the e2e tests fail loudly with + a "configure or pass -short" hint. +- E2e tests: `mage test:e2e` (no `-short`). Assumes an externally- + running Vikunja at `VEANS_E2E_API_URL`. The harness seeds its own + admin user via `PATCH /api/v1/test/users` — same mechanism the + playwright suite uses — so the API must be booted with + `VIKUNJA_SERVICE_TESTINGTOKEN=` and the same value passed in + via `VEANS_E2E_TESTING_TOKEN`. Alternative path: + `VEANS_E2E_ADMIN_TOKEN=` skips the seed and uses the given + token as-is, for driving a long-lived Vikunja the suite shouldn't + mutate user rows on. - Local e2e loop: from the parent repo root, build the API - (`mage build:build`), run it with sqlite-memory + a known JWT secret, - register an admin user via `POST /register`, then - `go test ./e2e/...` from `veans/` with the env vars above. + (`mage build:build`), run it with sqlite-memory + a known JWT + secret + `VIKUNJA_SERVICE_TESTINGTOKEN`, then `mage test:e2e` from + `veans/` with `VEANS_E2E_API_URL` + `VEANS_E2E_TESTING_TOKEN`. No + manual seeding step — the test harness handles it. - CI: the `test-veans-e2e` job in `.github/workflows/test.yml` consumes the existing `vikunja_bin` artifact from `api-build`; don't recompile the API in a parallel workflow. The `veans-test` job runs unit tests - independently and gives fast feedback. + with `-short` for fast feedback, independent of `api-build`. ## Vikunja wire-format gotchas @@ -112,15 +121,23 @@ what's bitten me; if a new endpoint behaves oddly, suspect one of these: ## Credential store -- Lookup chain: keychain → env (`VEANS_TOKEN`, optionally pinned by - `VEANS_SERVER`) → file (`~/.config/veans/credentials.yml`, mode 0600, - honors `XDG_CONFIG_HOME`). +- Lookup chain: keychain → env (`VEANS_TOKEN`) → file + (`~/.config/veans/credentials.yml`, mode 0600, atomic-write + flock + serialization). `XDG_CONFIG_HOME` is deliberately not honored — + agent-only audience runs in a known environment, and the env var + was a path-traversal seam for no real benefit. - `Chain.Set` falls through to the next backend on error so a missing dbus on a CI runner doesn't block writes — the file backend is the reliable last-resort. -- E2e tests override `HOME` and `XDG_CONFIG_HOME` per test to keep the - developer's keyring untouched. Don't bypass the credentials package - in tests — leaks between tests will surface as the wrong bot token. +- File writes go through a tmp file + `Rename`, with `Chmod 0o600` + re-asserted on the destination inode so a pre-existing wider mode + is narrowed. Concurrent writers (e.g. two `veans login` runs) are + serialized via `flock` on `.lock` (Unix only; Windows is a + no-op stub since the audience is Linux/macOS). +- E2e tests override `HOME` per test and `filterEnv(..., "VEANS_")` + strips any inherited `VEANS_TOKEN` so the developer's keyring + stays untouched. Don't bypass the credentials package in tests — + leaks between tests will surface as the wrong bot token. ## Project identifiers and bot usernames diff --git a/veans/e2e/helpers.go b/veans/e2e/helpers.go index 99e3527ae..2baa88952 100644 --- a/veans/e2e/helpers.go +++ b/veans/e2e/helpers.go @@ -15,29 +15,47 @@ // along with this program. If not, see . // Package e2e is the integration suite for veans. It assumes a running -// Vikunja API at VEANS_E2E_API_URL and admin/seed credentials in -// VEANS_E2E_ADMIN_TOKEN (or VEANS_E2E_ADMIN_USER + VEANS_E2E_ADMIN_PASS). +// Vikunja API at VEANS_E2E_API_URL with VIKUNJA_SERVICE_TESTINGTOKEN set +// (passed in via VEANS_E2E_TESTING_TOKEN) so the suite can seed its own +// admin via PATCH /api/v1/test/users — the same `/test/{table}` endpoint +// the frontend playwright suite uses. // -// The suite never provisions Vikunja itself — locally, point it at a dev -// instance; in CI, the workflow spins one up the same way the frontend -// Playwright suite does. +// The alternative path — VEANS_E2E_ADMIN_TOKEN — is a JWT against a +// long-lived Vikunja the user wants to drive without touching its data; +// in that mode the suite skips the seed. +// +// The suite never provisions Vikunja itself. package e2e import ( "bytes" "context" + "encoding/json" "errors" "fmt" + "io" + "net/http" "os" "os/exec" "path/filepath" "runtime" "strings" "testing" + "time" "code.vikunja.io/veans/internal/client" ) +// Hard-coded seed credentials. The hash is the bcrypt of "1234" and +// matches frontend/tests/support/constants.ts so the whole e2e infra +// shares one well-known password — tests themselves never need to read +// these from env. +const ( + seedAdminUsername = "e2eadmin" + seedAdminPassword = "1234" + seedAdminBcrypt = "$2a$14$dcadBoMBL9jQoOcZK8Fju.cy0Ptx2oZECkKLnaa8ekRoTFe1w7To." //nolint:gosec // G101: deterministic test fixture, not a credential +) + // Harness bundles a built veans binary and an authenticated admin client // for verifying side effects on the server. type Harness struct { @@ -47,23 +65,24 @@ type Harness struct { AdminClient *client.Client } -// SkipIfNotConfigured calls t.Skip if the suite hasn't been pointed at a -// Vikunja instance. Intended for the top of TestMain / TestXxx so plain -// `go test ./...` doesn't fail on contributors who haven't set up the env. -func SkipIfNotConfigured(t *testing.T) { - t.Helper() - if os.Getenv("VEANS_E2E_API_URL") == "" { - t.Skip("VEANS_E2E_API_URL not set — skipping e2e") - } -} - -// New builds (or reuses) the veans binary, mints/loads an admin token via -// the env, and returns a Harness ready to drive tests. +// New builds (or reuses) the veans binary, seeds the admin user via +// PATCH /api/v1/test/users (using VEANS_E2E_TESTING_TOKEN), logs in as +// that admin, and returns a Harness ready to drive tests. +// +// If VEANS_E2E_ADMIN_TOKEN is set, the seed is skipped and that token +// is used directly — useful for running against a long-lived Vikunja +// the caller doesn't want this suite to mutate user rows on. +// +// Tests rely on the `-short` skip in TestMain to opt out when a Vikunja +// instance isn't available; if `-short` is *not* set and env is missing, +// we fail loudly with a "configure or pass -short" hint. func New(t *testing.T) *Harness { t.Helper() - SkipIfNotConfigured(t) apiURL := strings.TrimRight(os.Getenv("VEANS_E2E_API_URL"), "/") + if apiURL == "" { + t.Fatal("VEANS_E2E_API_URL is not set — point it at a Vikunja instance, or pass -short to skip the e2e suite") + } binary, err := buildOrLocate() if err != nil { t.Fatalf("locate veans binary: %v", err) @@ -71,13 +90,16 @@ func New(t *testing.T) *Harness { tok := os.Getenv("VEANS_E2E_ADMIN_TOKEN") if tok == "" { - user, pass := os.Getenv("VEANS_E2E_ADMIN_USER"), os.Getenv("VEANS_E2E_ADMIN_PASS") - if user == "" || pass == "" { - t.Fatal("set VEANS_E2E_ADMIN_TOKEN or VEANS_E2E_ADMIN_USER + VEANS_E2E_ADMIN_PASS") + testingToken := os.Getenv("VEANS_E2E_TESTING_TOKEN") + if testingToken == "" { + t.Fatal("set VEANS_E2E_ADMIN_TOKEN, or VEANS_E2E_TESTING_TOKEN (matching the API's VIKUNJA_SERVICE_TESTINGTOKEN) so the suite can seed its own admin") } + seedAdmin(t, apiURL, testingToken) c := client.New(apiURL, "") - resp, err := c.Login(context.Background(), &client.LoginRequest{ - Username: user, Password: pass, LongToken: true, + resp, err := c.Login(t.Context(), &client.LoginRequest{ + Username: seedAdminUsername, + Password: seedAdminPassword, + LongToken: true, }) if err != nil { t.Fatalf("admin login: %v", err) @@ -93,13 +115,52 @@ func New(t *testing.T) *Harness { } } -// Workspace creates a per-test git repo in a TempDir, with HOME and -// XDG_CONFIG_HOME pointed at TempDirs so the credential store falls back -// to its file backend rather than touching the developer's keychain. +// seedAdmin PATCHes a single admin user row into the users table via +// the testing endpoint. truncate=true wipes any prior users from +// previous tests so each New(t) starts from a known state. +func seedAdmin(t *testing.T, apiURL, testingToken string) { + t.Helper() + now := time.Now().UTC().Format(time.RFC3339) + body, err := json.Marshal([]map[string]any{{ + "id": 1, + "username": seedAdminUsername, + "password": seedAdminBcrypt, + "email": "e2e@example.com", + "status": 0, + "issuer": "local", + "language": "en", + "created": now, + "updated": now, + }}) + if err != nil { + t.Fatalf("marshal seed payload: %v", err) + } + + req, err := http.NewRequestWithContext(t.Context(), http.MethodPatch, + apiURL+"/api/v1/test/users?truncate=true", bytes.NewReader(body)) + if err != nil { + t.Fatalf("build seed request: %v", err) + } + req.Header.Set("Authorization", testingToken) + req.Header.Set("Content-Type", "application/json") + + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("seed admin: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode >= 300 { + buf, _ := io.ReadAll(resp.Body) + t.Fatalf("seed admin: HTTP %d: %s", resp.StatusCode, string(buf)) + } +} + +// Workspace creates a per-test git repo in a TempDir with HOME pointed at +// a TempDir so the credential store writes under the test's own directory +// rather than touching the developer's keychain. type Workspace struct { Dir string Home string - XDGConfig string ConfigPath string BotUsername string envOverrides map[string]string @@ -111,7 +172,6 @@ func (h *Harness) NewWorkspace(t *testing.T) *Workspace { t.Helper() dir := t.TempDir() home := t.TempDir() - xdg := t.TempDir() for _, c := range [][]string{ {"git", "init", "-q", "-b", "main"}, @@ -145,11 +205,9 @@ func (h *Harness) NewWorkspace(t *testing.T) *Workspace { return &Workspace{ Dir: dir, Home: home, - XDGConfig: xdg, ConfigPath: filepath.Join(dir, ".veans.yml"), envOverrides: map[string]string{ - "HOME": home, - "XDG_CONFIG_HOME": xdg, + "HOME": home, }, } } @@ -160,7 +218,10 @@ func (h *Harness) Run(t *testing.T, ws *Workspace, args ...string) (stdout, stde t.Helper() cmd := exec.CommandContext(t.Context(), h.Binary, args...) cmd.Dir = ws.Dir - cmd.Env = append(os.Environ(), envSlice(ws.envOverrides)...) + // Filter VEANS_* out of the inherited env before applying our + // overrides — a developer's VEANS_TOKEN would otherwise mask the + // per-test bot token via the env backend. + cmd.Env = append(filterEnv(os.Environ(), "VEANS_"), envSlice(ws.envOverrides)...) var so, se bytes.Buffer cmd.Stdout = &so cmd.Stderr = &se @@ -179,22 +240,19 @@ func (h *Harness) Run(t *testing.T, ws *Workspace, args ...string) (stdout, stde // it. Tests use a unique title to keep results isolated across parallel runs. func (h *Harness) CreateProject(t *testing.T, title, identifier string) *client.Project { t.Helper() - body := map[string]any{"title": title} - if identifier != "" { - body["identifier"] = identifier - } - var out client.Project - if err := h.AdminClient.Do(context.Background(), "PUT", "/projects", nil, body, &out); err != nil { + out, err := h.AdminClient.CreateProject(t.Context(), + &client.Project{Title: title, Identifier: identifier}) + if err != nil { t.Fatalf("create project %q: %v", title, err) } - return &out + return out } // FindKanbanView returns the first Kanban view of the project (Vikunja // auto-creates one). func (h *Harness) FindKanbanView(t *testing.T, projectID int64) *client.ProjectView { t.Helper() - views, err := h.AdminClient.ListProjectViews(context.Background(), projectID) + views, err := h.AdminClient.ListProjectViews(t.Context(), projectID) if err != nil { t.Fatalf("list views: %v", err) } @@ -210,7 +268,7 @@ func (h *Harness) FindKanbanView(t *testing.T, projectID int64) *client.ProjectV // GetTask fetches a task by ID for verification. func (h *Harness) GetTask(t *testing.T, id int64) *client.Task { t.Helper() - task, err := h.AdminClient.GetTask(context.Background(), id) + task, err := h.AdminClient.GetTask(t.Context(), id) if err != nil { t.Fatalf("get task %d: %v", id, err) } @@ -255,3 +313,14 @@ func envSlice(overrides map[string]string) []string { } return out } + +// filterEnv returns env entries whose keys do NOT start with prefix. +func filterEnv(env []string, prefix string) []string { + out := make([]string, 0, len(env)) + for _, kv := range env { + if !strings.HasPrefix(kv, prefix) { + out = append(out, kv) + } + } + return out +} diff --git a/veans/e2e/init_test.go b/veans/e2e/init_test.go index e529ef8c0..cc432d99a 100644 --- a/veans/e2e/init_test.go +++ b/veans/e2e/init_test.go @@ -17,7 +17,6 @@ package e2e import ( - "context" "fmt" "strconv" "strings" @@ -75,7 +74,7 @@ func TestInit_HappyPath(t *testing.T) { // Bot token persisted in the file backend (since HOME points at a // fresh tmpdir, the file backend takes over from the missing keyring). - store := credentials.NewFileBackend(ws.XDGConfig + "/veans/credentials.yml") + store := credentials.NewFileBackend(ws.Home + "/.config/veans/credentials.yml") tok, err := store.Get(h.APIURL, ws.BotUsername) if err != nil { t.Fatalf("token not persisted: %v", err) @@ -85,7 +84,7 @@ func TestInit_HappyPath(t *testing.T) { } // Bot exists on the server with the right username. - bots, err := h.AdminClient.ListBotUsers(context.Background()) + bots, err := h.AdminClient.ListBotUsers(t.Context()) if err != nil { t.Fatalf("list bots: %v", err) } @@ -105,7 +104,7 @@ func TestInit_HappyPath(t *testing.T) { // Project shared with the bot at write permission. var shares []map[string]any - _ = h.AdminClient.Do(context.Background(), "GET", fmt.Sprintf("/projects/%d/users", project.ID), nil, nil, &shares) + _ = h.AdminClient.Do(t.Context(), "GET", fmt.Sprintf("/projects/%d/users", project.ID), nil, nil, &shares) shareFound := false for _, s := range shares { if u, _ := s["username"].(string); u == ws.BotUsername { diff --git a/veans/e2e/main_test.go b/veans/e2e/main_test.go new file mode 100644 index 000000000..072665376 --- /dev/null +++ b/veans/e2e/main_test.go @@ -0,0 +1,36 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-present Vikunja and contributors. All rights reserved. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package e2e + +import ( + "flag" + "os" + "testing" +) + +// TestMain follows the parent monorepo's `pkg/webtests` convention: +// `-short` skips the whole package so a plain `go test ./...` from the +// repo root (or `mage test`) doesn't try to drive a live Vikunja. Run +// `mage test:e2e` to execute it. +func TestMain(m *testing.M) { + flag.Parse() + if testing.Short() { + println("-short requested, skipping veans e2e tests") + return + } + os.Exit(m.Run()) +} diff --git a/veans/e2e/shared_test.go b/veans/e2e/shared_test.go index 2f6813678..dba95a3a0 100644 --- a/veans/e2e/shared_test.go +++ b/veans/e2e/shared_test.go @@ -18,6 +18,7 @@ package e2e import ( "os/exec" + "strconv" "strings" "testing" @@ -41,8 +42,8 @@ func provisionWorkspace(t *testing.T) (*Workspace, *Harness) { "init", "--server", h.APIURL, "--token", h.AdminToken, - "--project", iToS(project.ID), - "--view", iToS(view.ID), + "--project", strconv.FormatInt(project.ID, 10), + "--view", strconv.FormatInt(view.ID, 10), "--bot-username", ws.BotUsername, "--yes-buckets", ) @@ -71,27 +72,3 @@ func gitInWorkspace(t *testing.T, ws *Workspace, args ...string) { t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) } } - -func iToS(n int64) string { - const digits = "0123456789" - if n == 0 { - return "0" - } - negative := false - if n < 0 { - negative = true - n = -n - } - var buf [20]byte - i := len(buf) - for n > 0 { - i-- - buf[i] = digits[n%10] - n /= 10 - } - if negative { - i-- - buf[i] = '-' - } - return string(buf[i:]) -} diff --git a/veans/e2e/tasks_test.go b/veans/e2e/tasks_test.go index 2beced968..4e2dee52b 100644 --- a/veans/e2e/tasks_test.go +++ b/veans/e2e/tasks_test.go @@ -96,6 +96,16 @@ func TestCreateShowList_RoundTrip(t *testing.T) { t.Fatalf("filter leaked priority=%d task into result", ft.Priority) } } + + // Empty result set must encode as `[]`, not `null` — JSON-parsing agents + // can't reliably branch on the latter. Use a filter that matches nothing. + emptyOut, _, code := h.Run(t, ws, "list", "--filter", "priority > 10") + if code != 0 { + t.Fatalf("list (empty) exit %d\n%s", code, emptyOut) + } + if got := strings.TrimSpace(emptyOut); got != "[]" { + t.Fatalf("empty list should print []; got %q", got) + } } // TestUpdate_DescriptionReplaceUniqueness pins the agent-friendly Edit-tool diff --git a/veans/go.mod b/veans/go.mod index 4b0c3bb5d..c025994dc 100644 --- a/veans/go.mod +++ b/veans/go.mod @@ -3,19 +3,19 @@ module code.vikunja.io/veans go 1.25.0 require ( + github.com/dustinkirkland/golang-petname v0.0.0-20260215035315-f0c533e9ce9b github.com/magefile/mage v1.17.2 + github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c github.com/spf13/cobra v1.10.2 github.com/zalando/go-keyring v0.2.8 + golang.org/x/sys v0.43.0 golang.org/x/term v0.42.0 gopkg.in/yaml.v3 v3.0.1 ) require ( github.com/danieljoos/wincred v1.2.3 // indirect - github.com/dustinkirkland/golang-petname v0.0.0-20260215035315-f0c533e9ce9b // indirect github.com/godbus/dbus/v5 v5.2.2 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect - github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c // indirect github.com/spf13/pflag v1.0.9 // indirect - golang.org/x/sys v0.43.0 // indirect ) diff --git a/veans/internal/auth/auth.go b/veans/internal/auth/auth.go index 720fcc87e..60706aaf7 100644 --- a/veans/internal/auth/auth.go +++ b/veans/internal/auth/auth.go @@ -44,17 +44,22 @@ type Prompter interface { } // StdPrompter reads from os.Stdin and writes prompts to os.Stderr; uses -// term.ReadPassword for masked input when on a TTY. -type StdPrompter struct{} +// term.ReadPassword for masked input when on a TTY. The bufio.Reader is +// reused across ReadLine calls — a new reader on each call would read- +// ahead a buffer, discard the rest on return, and starve later prompts. +type StdPrompter struct { + stdin *bufio.Reader +} -func NewStdPrompter() *StdPrompter { return &StdPrompter{} } +func NewStdPrompter() *StdPrompter { + return &StdPrompter{stdin: bufio.NewReader(os.Stdin)} +} -func (*StdPrompter) ReadLine(prompt string) (string, error) { +func (p *StdPrompter) ReadLine(prompt string) (string, error) { if _, err := fmt.Fprint(os.Stderr, prompt); err != nil { return "", err } - r := bufio.NewReader(os.Stdin) - line, err := r.ReadString('\n') + line, err := p.stdin.ReadString('\n') if err != nil && !errors.Is(err, io.EOF) { return "", err } diff --git a/veans/internal/auth/oauth.go b/veans/internal/auth/oauth.go index f78e630a4..281bbabce 100644 --- a/veans/internal/auth/oauth.go +++ b/veans/internal/auth/oauth.go @@ -22,6 +22,7 @@ import ( "crypto/sha256" "encoding/base64" "fmt" + "html" "io" "net" "net/http" @@ -183,11 +184,21 @@ func newCallbackServer(listener net.Listener) (*http.Server, <-chan callbackResu server := &http.Server{ Addr: listener.Addr().String(), ReadHeaderTimeout: 5 * time.Second, + WriteTimeout: 10 * time.Second, + IdleTimeout: 10 * time.Second, Handler: http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { if r.URL.Path != "/callback" { http.NotFound(rw, r) return } + // Pin to GET so a third-party page can't POST a forged + // (code, state) into the loopback handler. State binding + // already defends, but cheap belt-and-braces. + if r.Method != http.MethodGet { + rw.Header().Set("Allow", "GET") + http.Error(rw, "method not allowed", http.StatusMethodNotAllowed) + return + } q := r.URL.Query() res := callbackResult{code: q.Get("code"), state: q.Get("state")} if errCode := q.Get("error"); errCode != "" { @@ -249,11 +260,14 @@ func renderCallbackPage(w http.ResponseWriter, err error) { w.Header().Set("Cache-Control", "no-store") if err != nil { w.WriteHeader(http.StatusBadRequest) + // HTML-escape the authorization-server's error_description — it + // arrives unsanitized from a remote source and we render it + // straight into the loopback page. _, _ = fmt.Fprintf(w, `

veans: authorization failed

%s

You can close this tab and re-run veans init.

-`, err.Error()) +`, html.EscapeString(err.Error())) return } _, _ = w.Write([]byte(` diff --git a/veans/internal/bootstrap/bootstrap.go b/veans/internal/bootstrap/bootstrap.go index 345b2fc83..86b67681f 100644 --- a/veans/internal/bootstrap/bootstrap.go +++ b/veans/internal/bootstrap/bootstrap.go @@ -29,6 +29,8 @@ import ( "errors" "fmt" "io" + "os" + "regexp" "sort" "strconv" "strings" @@ -90,6 +92,16 @@ type Options struct { // RepoRoot, if empty, is detected via git rev-parse from cwd. RepoRoot string + + // Prompter is the seam tests use to script prompt answers. Defaults + // to auth.NewStdPrompter() (reads stdin, writes prompts to stderr) + // when nil. + Prompter auth.Prompter + + // OverwriteExistingConfig, when true, allows Init to clobber an + // existing .veans.yml without prompting. Mostly for tests; the + // interactive flow asks the user. + OverwriteExistingConfig bool } // Result is returned on success — just the bits printPostInitSummary reads. @@ -113,9 +125,26 @@ func Init(ctx context.Context, opts *Options) (*Result, error) { return nil, output.New(output.CodeValidation, "ConfigPath is required") } - prompter := auth.NewStdPrompter() + prompter := opts.Prompter + if prompter == nil { + prompter = auth.NewStdPrompter() + } store := credentials.Default() + if err := confirmOverwriteExistingConfig(opts, prompter); err != nil { + return nil, err + } + + // Validate the bot-username override (if any) against server-side + // rules now, so we fail fast before steps 4–7 do real work that + // we'd then have to undo. SuggestedBotUsername's output is + // always valid, so we only need to validate user input. + if opts.BotUsername != "" { + if err := validateBotUsername(normalizeBotUsername(opts.BotUsername, "")); err != nil { + return nil, err + } + } + // 1. Repo root + suggested bot username. repoRoot := opts.RepoRoot if repoRoot == "" { @@ -278,6 +307,35 @@ func Init(ctx context.Context, opts *Options) (*Result, error) { }, nil } +// confirmOverwriteExistingConfig refuses to silently clobber an existing +// .veans.yml. The bot token in the credentials store is keyed on +// (server, bot-username); a blind re-init can swap the project under +// the agent's feet AND stomp the previous token in the keyring. +func confirmOverwriteExistingConfig(opts *Options, p auth.Prompter) error { + if opts.OverwriteExistingConfig { + return nil + } + if _, err := os.Stat(opts.ConfigPath); err != nil { + // File doesn't exist (or we can't stat it — let SaveAs surface + // that error later). Either way, no overwrite confirmation is + // needed. + return nil //nolint:nilerr // intentional: any Stat error means "no existing file to overwrite" + } + ans, err := p.ReadLine(fmt.Sprintf( + "%s already exists. Overwrite (token + project + view get replaced)? [y/N]: ", + opts.ConfigPath)) + if err != nil { + return output.Wrap(output.CodeUnknown, err, "read overwrite confirmation: %v", err) + } + switch strings.ToLower(strings.TrimSpace(ans)) { + case "y", "yes": + return nil + } + return output.New(output.CodeConflict, + "refusing to overwrite %s without confirmation (delete the file to re-init)", + opts.ConfigPath) +} + func normalizeBotUsername(override, suggested string) string { if override == "" { return suggested @@ -288,6 +346,32 @@ func normalizeBotUsername(override, suggested string) string { return override } +// botUsernamePattern mirrors the server's username regex closely enough +// to catch the rejections that would otherwise blow up steps 4–7 mid-init. +// The server allows lowercase letters, digits, hyphens, underscores, and +// dots; we additionally require the `bot-` prefix and forbid the +// `link-share-N` shape Vikunja reserves for share-links. +var botUsernamePattern = regexp.MustCompile(`^bot-[a-z0-9][a-z0-9._-]*$`) + +var linkShareSuffix = regexp.MustCompile(`^bot-link-share-\d+$`) + +// validateBotUsername mirrors the server-side rules so a bad +// `--bot-username` override (or interactive prompt answer) fails fast +// instead of dying with a 400 deep in step 8. +func validateBotUsername(name string) error { + if !botUsernamePattern.MatchString(name) { + return output.New(output.CodeValidation, + "invalid bot username %q: must start with `bot-` and contain only lowercase letters, digits, hyphens, underscores, and dots", + name) + } + if linkShareSuffix.MatchString(name) { + return output.New(output.CodeValidation, + "invalid bot username %q: `link-share-N` is reserved by Vikunja for share-link users", + name) + } + return nil +} + func pickProject(ctx context.Context, c *client.Client, id int64, p auth.Prompter, out io.Writer) (*client.Project, error) { if id != 0 { return c.GetProject(ctx, id) @@ -304,11 +388,17 @@ func pickProject(ctx context.Context, c *client.Client, id int64, p auth.Prompte } active = append(active, pr) } - if len(active) == 0 { - return nil, output.New(output.CodeNotFound, "no projects visible to this user — create one in the Vikunja UI first") - } sort.Slice(active, func(i, j int) bool { return active[i].Title < active[j].Title }) + // The "create a new project" option sits at len(active)+1 in the menu; + // when the user has nothing to pick from, it's the only choice. + createIdx := len(active) + 1 + + if len(active) == 0 { + fmt.Fprintln(out, "No projects yet — let's create one.") + return createProject(ctx, c, p, out) + } + fmt.Fprintln(out, "Available projects:") for i, pr := range active { ident := pr.Identifier @@ -317,6 +407,8 @@ func pickProject(ctx context.Context, c *client.Client, id int64, p auth.Prompte } fmt.Fprintf(out, " [%d] #%d %s — %s\n", i+1, pr.ID, pr.Title, ident) } + fmt.Fprintf(out, " [%d] Create a new project\n", createIdx) + choice, err := p.ReadLine("Pick a project [1]: ") if err != nil { return nil, err @@ -325,14 +417,44 @@ func pickProject(ctx context.Context, c *client.Client, id int64, p auth.Prompte idx := 1 if choice != "" { v, err := strconv.Atoi(choice) - if err != nil || v < 1 || v > len(active) { + if err != nil || v < 1 || v > createIdx { return nil, output.New(output.CodeValidation, "invalid project choice %q", choice) } idx = v } + if idx == createIdx { + return createProject(ctx, c, p, out) + } return active[idx-1], nil } +// createProject prompts for the new project's title and identifier and +// PUTs it. Title is required; identifier is optional (Vikunja caps it at +// 10 chars). The fresh project comes with the default views — including +// the Kanban view pickKanbanView is about to grab. +func createProject(ctx context.Context, c *client.Client, p auth.Prompter, out io.Writer) (*client.Project, error) { + title, err := p.ReadLine("New project title: ") + if err != nil { + return nil, err + } + title = strings.TrimSpace(title) + if title == "" { + return nil, output.New(output.CodeValidation, "project title is required") + } + ident, err := p.ReadLine("Identifier (optional, ≤10 letters/digits, used for task IDs like PROJ-NN): ") + if err != nil { + return nil, err + } + ident = strings.TrimSpace(ident) + + created, err := c.CreateProject(ctx, &client.Project{Title: title, Identifier: ident}) + if err != nil { + return nil, output.Wrap(output.CodeUnknown, err, "create project %q: %v", title, err) + } + progress(out, "Created project #%d %q", created.ID, created.Title) + return created, nil +} + func pickKanbanView(ctx context.Context, c *client.Client, projectID int64, viewID int64, p auth.Prompter, out io.Writer) (*client.ProjectView, error) { views, err := c.ListProjectViews(ctx, projectID) if err != nil { diff --git a/veans/internal/bootstrap/hooks.go b/veans/internal/bootstrap/hooks.go index ae27f23d5..c5f61722b 100644 --- a/veans/internal/bootstrap/hooks.go +++ b/veans/internal/bootstrap/hooks.go @@ -34,6 +34,60 @@ import ( // reading the same string. const veansPrimeCommand = "veans prime" +// ClaudeCodeHookEvents enumerates every Claude Code lifecycle event that +// veans wires `veans prime` into. Both the auto-installer in this file and +// the manual-install snippet rendered by the `init` command iterate this +// list so the two paths can never drift. +var ClaudeCodeHookEvents = []string{"SessionStart", "PreCompact"} + +// ClaudeCodeSettingsRelPath is the per-repo Claude Code settings file that +// installClaudeCodeHook merges into. Exported so the `init` command can name +// it in the manual-install blurb without re-typing the literal. +const ClaudeCodeSettingsRelPath = ".claude/settings.json" + +// OpenCodePluginRelPath is the per-repo path the OpenCode plugin is written +// to. Same rationale as ClaudeCodeSettingsRelPath. +const OpenCodePluginRelPath = ".opencode/plugin/veans-prime.ts" + +// OpenCodePluginSnippet is the exact TypeScript plugin written to disk by +// installOpenCodeHook. Re-exported verbatim for the manual-install path so a +// copy-pasted snippet is byte-for-byte what the installer would have +// produced. +const OpenCodePluginSnippet = `// Auto-generated by 'veans init'. Re-emits the veans agent prompt at the +// start of every OpenCode session and before every compaction. See +// https://github.com/go-vikunja/vikunja/tree/main/veans for context. +export const VeansPrime = { + event: ["session.start", "compact.before"], + handler: async ({ exec }: { exec: (cmd: string) => Promise }) => + exec("veans prime"), +} +` + +// ClaudeCodeHookSnippet renders the JSON fragment a user would paste into +// .claude/settings.json to get the same wiring the auto-installer performs. +// Generated from ClaudeCodeHookEvents + veansPrimeCommand so a new event +// added to the install list automatically shows up in the manual snippet +// too. +func ClaudeCodeHookSnippet() string { + hooks := map[string]any{} + for _, event := range ClaudeCodeHookEvents { + hooks[event] = []any{ + map[string]any{ + "hooks": []any{ + map[string]any{"type": "command", "command": veansPrimeCommand}, + }, + }, + } + } + buf, err := json.MarshalIndent(map[string]any{"hooks": hooks}, "", " ") + if err != nil { + // MarshalIndent on a hand-built map[string]any can't realistically + // fail; fall back so callers never see an empty snippet. + return fmt.Sprintf(`{"hooks": {"SessionStart": [{"hooks": [{"type": "command", "command": %q}]}]}}`, veansPrimeCommand) + } + return string(buf) +} + // AgentHookChoice captures the user's per-agent install decision so the // orchestration in bootstrap.Init can hand the per-repo set of choices // off to the install routines below. @@ -97,13 +151,13 @@ func installAgentHooks(repoRoot string, choices AgentHookChoice, w io.Writer) er // a human verb describing what happened ("Wrote", "Updated", "Already // configured"), and any error. func installClaudeCodeHook(repoRoot string) (string, string, error) { - path := filepath.Join(repoRoot, ".claude", "settings.json") + path := filepath.Join(repoRoot, filepath.FromSlash(ClaudeCodeSettingsRelPath)) settings, existed, err := readJSONOrEmpty(path) if err != nil { return path, "", err } changed := false - for _, event := range []string{"SessionStart", "PreCompact"} { + for _, event := range ClaudeCodeHookEvents { if ensureClaudeHook(settings, event) { changed = true } @@ -168,7 +222,7 @@ func ensureClaudeHook(settings map[string]any, event string) bool { // if missing. Existing files are left alone (TypeScript merging is out of // scope; the user can edit by hand). func installOpenCodeHook(repoRoot string) (string, string, error) { - path := filepath.Join(repoRoot, ".opencode", "plugin", "veans-prime.ts") + path := filepath.Join(repoRoot, filepath.FromSlash(OpenCodePluginRelPath)) if _, err := os.Stat(path); err == nil { return path, "Already configured", nil } else if !errors.Is(err, os.ErrNotExist) { @@ -177,22 +231,12 @@ func installOpenCodeHook(repoRoot string) (string, string, error) { if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { return path, "", err } - if err := os.WriteFile(path, []byte(openCodeHookSource), 0o644); err != nil { + if err := os.WriteFile(path, []byte(OpenCodePluginSnippet), 0o644); err != nil { return path, "", err } return path, "Wrote", nil } -const openCodeHookSource = `// Auto-generated by 'veans init'. Re-emits the veans agent prompt at the -// start of every OpenCode session and before every compaction. See -// https://github.com/go-vikunja/vikunja/tree/main/veans for context. -export const VeansPrime = { - event: ["session.start", "compact.before"], - handler: async ({ exec }: { exec: (cmd: string) => Promise }) => - exec("veans prime"), -} -` - // readJSONOrEmpty reads `path` as JSON or returns an empty object if the // file doesn't exist. The `existed` flag tells the caller whether the // resulting object was loaded from disk (so it can decide between @@ -217,6 +261,10 @@ func readJSONOrEmpty(path string) (out map[string]any, existed bool, err error) // writeJSON encodes `data` with two-space indent (Claude Code's house // style) and a trailing newline, creating parent directories as needed. +// Settings files written here may end up holding provider API keys, so we +// default new files to 0o600 and preserve the existing mode on update so a +// user who has tightened the file (e.g. to 0o600 explicitly, or chmod'd it +// further) doesn't see their permissions widened on the next write. func writeJSON(path string, data map[string]any) error { if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { return err @@ -226,7 +274,11 @@ func writeJSON(path string, data map[string]any) error { return err } buf = append(buf, '\n') - return os.WriteFile(path, buf, 0o644) + mode := os.FileMode(0o600) + if info, err := os.Stat(path); err == nil { + mode = info.Mode().Perm() + } + return os.WriteFile(path, buf, mode) } // mapAt returns the map at key `k` on `m`, creating it if missing or if diff --git a/veans/internal/client/client.go b/veans/internal/client/client.go index 5949d0d9d..2d59b3ad7 100644 --- a/veans/internal/client/client.go +++ b/veans/internal/client/client.go @@ -20,11 +20,11 @@ import ( "bytes" "context" "encoding/json" - "errors" "fmt" "io" "net/http" "net/url" + "strconv" "strings" "time" @@ -98,13 +98,14 @@ func (c *Client) Do(ctx context.Context, method, path string, query url.Values, } defer resp.Body.Close() - respBody, err := io.ReadAll(resp.Body) + respBody, err := io.ReadAll(io.LimitReader(resp.Body, maxBodyBytes)) if err != nil { return fmt.Errorf("read response: %w", err) } if resp.StatusCode >= 400 { - return mapHTTPError(method, path, resp.StatusCode, respBody) + return mapHTTPError(method, path, resp.StatusCode, respBody, + parseRetryAfter(resp.Header.Get("Retry-After"))) } if out != nil && len(respBody) > 0 { @@ -115,9 +116,60 @@ func (c *Client) Do(ctx context.Context, method, path string, query url.Values, return nil } +// DoPaginated is like Do but also returns the total page count parsed from +// the `x-pagination-total-pages` response header (0 if the header is +// missing or unparseable). Used by the list endpoints so paging terminates +// against the authoritative server count, not a `len(batch) < per_page` +// heuristic that loops one extra time on exact-multiple totals. +func (c *Client) DoPaginated(ctx context.Context, method, path string, query url.Values, out any) (totalPages int, err error) { + full := c.BaseURL + "/api/v1" + path + if len(query) > 0 { + full += "?" + query.Encode() + } + req, err := http.NewRequestWithContext(ctx, method, full, nil) + if err != nil { + return 0, fmt.Errorf("build request: %w", err) + } + req.Header.Set("Accept", "application/json") + if c.Token != "" { + req.Header.Set("Authorization", "Bearer "+c.Token) + } + req.Header.Set("User-Agent", UserAgent) + + resp, err := c.HTTPClient.Do(req) + if err != nil { + return 0, output.Wrap(output.CodeUnknown, err, "%s %s: %v", method, path, err) + } + defer resp.Body.Close() + + respBody, err := io.ReadAll(io.LimitReader(resp.Body, maxBodyBytes)) + if err != nil { + return 0, fmt.Errorf("read response: %w", err) + } + if resp.StatusCode >= 400 { + return 0, mapHTTPError(method, path, resp.StatusCode, respBody, + parseRetryAfter(resp.Header.Get("Retry-After"))) + } + if out != nil && len(respBody) > 0 { + if err := json.Unmarshal(respBody, out); err != nil { + return 0, fmt.Errorf("decode %s %s: %w", method, path, err) + } + } + if v := resp.Header.Get("x-pagination-total-pages"); v != "" { + if n, perr := strconv.Atoi(v); perr == nil { + totalPages = n + } + } + return totalPages, nil +} + // DoRaw is the escape hatch used by `veans api`. It returns the raw response -// body and status. Auth + base URL handling matches Do. -func (c *Client) DoRaw(ctx context.Context, method, path string, query url.Values, body []byte) (status int, respBody []byte, err error) { +// body, status, and the parsed Retry-After (if any). Auth + base URL handling +// matches Do. The caller is responsible for deciding whether to surface the +// body to stdout — non-2xx bodies should NOT be written there (the contract is +// "stdout is for the success payload; errors go through the envelope on +// stderr"); see commands/api.go for the canonical handling. +func (c *Client) DoRaw(ctx context.Context, method, path string, query url.Values, body []byte) (status int, respBody []byte, retryAfter time.Duration, err error) { full := c.BaseURL + "/api/v1" + path if len(query) > 0 { full += "?" + query.Encode() @@ -128,7 +180,7 @@ func (c *Client) DoRaw(ctx context.Context, method, path string, query url.Value } req, err := http.NewRequestWithContext(ctx, method, full, br) if err != nil { - return 0, nil, err + return 0, nil, 0, err } req.Header.Set("Accept", "application/json") if len(body) > 0 { @@ -140,14 +192,50 @@ func (c *Client) DoRaw(ctx context.Context, method, path string, query url.Value req.Header.Set("User-Agent", UserAgent) resp, err := c.HTTPClient.Do(req) if err != nil { - return 0, nil, err + return 0, nil, 0, err } defer resp.Body.Close() - respBody, err = io.ReadAll(resp.Body) - return resp.StatusCode, respBody, err + respBody, err = io.ReadAll(io.LimitReader(resp.Body, maxBodyBytes)) + return resp.StatusCode, respBody, parseRetryAfter(resp.Header.Get("Retry-After")), err } -func mapHTTPError(method, path string, status int, body []byte) error { +// paginationDone reports whether a paged GET has consumed every page, +// preferring the server's x-pagination-total-pages count when present and +// falling back to the len(batch) < per_page heuristic when the header is +// missing (older server / proxy stripped). Centralized so all list +// endpoints terminate identically. +func paginationDone(page, batchLen, perPage, totalPages int) bool { + if totalPages > 0 { + return page >= totalPages + } + return batchLen < perPage +} + +// maxBodyBytes caps the size of any response body we'll read into memory. +// Vikunja JSON payloads are far smaller; the cap exists so a misbehaving +// proxy can't OOM the CLI by streaming an unbounded body. +const maxBodyBytes = 32 * 1024 * 1024 // 32 MiB + +// parseRetryAfter parses an HTTP Retry-After header value. Supports both +// the delta-seconds form and the HTTP-date form; returns 0 on unparseable +// or empty input. +func parseRetryAfter(v string) time.Duration { + v = strings.TrimSpace(v) + if v == "" { + return 0 + } + if secs, err := strconv.Atoi(v); err == nil && secs >= 0 { + return time.Duration(secs) * time.Second + } + if t, err := http.ParseTime(v); err == nil { + if d := time.Until(t); d > 0 { + return d + } + } + return 0 +} + +func mapHTTPError(method, path string, status int, body []byte, retryAfter time.Duration) error { var ve vikunjaError _ = json.Unmarshal(body, &ve) msg := strings.TrimSpace(ve.Message) @@ -157,6 +245,11 @@ func mapHTTPError(method, path string, status int, body []byte) error { msg = http.StatusText(status) } } + // Truncate so an HTML error page (e.g. from a reverse proxy) doesn't + // dump several KB into the agent's stderr envelope. + if len(msg) > maxErrorMessageBytes { + msg = msg[:maxErrorMessageBytes] + "…(truncated)" + } var code output.Code switch { @@ -174,9 +267,17 @@ func mapHTTPError(method, path string, status int, body []byte) error { code = output.CodeUnknown } + formatted := fmt.Sprintf("%s %s: %d %s", method, path, status, msg) + if retryAfter > 0 { + formatted += fmt.Sprintf(" (retry-after %s)", retryAfter) + } return &output.Error{ Code: code, - Message: fmt.Sprintf("%s %s: %d %s", method, path, status, msg), - Cause: errors.New(msg), + Message: formatted, } } + +// maxErrorMessageBytes caps how much upstream-error text we embed in the +// envelope's `error` field. Anything longer is almost always an HTML page +// from a proxy and useless for the agent to read. +const maxErrorMessageBytes = 512 diff --git a/veans/internal/client/labels.go b/veans/internal/client/labels.go index d94b75c1a..1681b49a1 100644 --- a/veans/internal/client/labels.go +++ b/veans/internal/client/labels.go @@ -27,7 +27,8 @@ import ( // authenticated user (labels are global per user, not scoped to a project). func (c *Client) ListLabels(ctx context.Context, search string) ([]*Label, error) { var all []*Label - for page := 1; ; page++ { + page := 1 + for { q := url.Values{} q.Set("page", strconv.Itoa(page)) q.Set("per_page", "50") @@ -35,13 +36,15 @@ func (c *Client) ListLabels(ctx context.Context, search string) ([]*Label, error q.Set("s", search) } var batch []*Label - if err := c.Do(ctx, "GET", "/labels", q, nil, &batch); err != nil { + total, err := c.DoPaginated(ctx, "GET", "/labels", q, &batch) + if err != nil { return nil, err } all = append(all, batch...) - if len(batch) < 50 { + if paginationDone(page, len(batch), 50, total) { return all, nil } + page++ } } diff --git a/veans/internal/client/projects.go b/veans/internal/client/projects.go index 078ecff87..e0f23eccb 100644 --- a/veans/internal/client/projects.go +++ b/veans/internal/client/projects.go @@ -23,21 +23,25 @@ import ( "strconv" ) -// ListProjects pages through GET /projects, accumulating until exhausted. +// ListProjects pages through GET /projects, accumulating until the server's +// x-pagination-total-pages header says we're done. func (c *Client) ListProjects(ctx context.Context) ([]*Project, error) { var all []*Project - for page := 1; ; page++ { + page := 1 + for { q := url.Values{} q.Set("page", strconv.Itoa(page)) q.Set("per_page", "50") var batch []*Project - if err := c.Do(ctx, "GET", "/projects", q, nil, &batch); err != nil { + total, err := c.DoPaginated(ctx, "GET", "/projects", q, &batch) + if err != nil { return nil, err } all = append(all, batch...) - if len(batch) < 50 { + if paginationDone(page, len(batch), 50, total) { return all, nil } + page++ } } @@ -50,6 +54,16 @@ func (c *Client) GetProject(ctx context.Context, id int64) (*Project, error) { return &out, nil } +// CreateProject creates a new project owned by the calling user. Vikunja +// auto-creates the default views (List, Gantt, Table, Kanban) on insert. +func (c *Client) CreateProject(ctx context.Context, p *Project) (*Project, error) { + var out Project + if err := c.Do(ctx, "PUT", "/projects", nil, p, &out); err != nil { + return nil, err + } + return &out, nil +} + // ShareProjectWithUser grants `username` `permission` on project `id`. func (c *Client) ShareProjectWithUser(ctx context.Context, projectID int64, share *ProjectUser) (*ProjectUser, error) { var out ProjectUser diff --git a/veans/internal/client/tasks.go b/veans/internal/client/tasks.go index 70e31c818..c53c219c5 100644 --- a/veans/internal/client/tasks.go +++ b/veans/internal/client/tasks.go @@ -51,7 +51,8 @@ func (o *TaskListOptions) values() url.Values { return q } -// ListProjectTasks paginates `GET /projects/{id}/tasks` exhaustively. +// ListProjectTasks paginates `GET /projects/{id}/tasks` exhaustively, +// terminating against the server's x-pagination-total-pages header. func (c *Client) ListProjectTasks(ctx context.Context, projectID int64, opts *TaskListOptions) ([]*Task, error) { if opts == nil { opts = &TaskListOptions{} @@ -61,18 +62,21 @@ func (c *Client) ListProjectTasks(ctx context.Context, projectID int64, opts *Ta per = 50 } var all []*Task - for page := 1; ; page++ { + page := 1 + for { o := *opts o.Page = page o.PerPage = per var batch []*Task - if err := c.Do(ctx, "GET", fmt.Sprintf("/projects/%d/tasks", projectID), o.values(), nil, &batch); err != nil { + total, err := c.DoPaginated(ctx, "GET", fmt.Sprintf("/projects/%d/tasks", projectID), o.values(), &batch) + if err != nil { return nil, err } all = append(all, batch...) - if len(batch) < per { + if paginationDone(page, len(batch), per, total) { return all, nil } + page++ } } @@ -119,8 +123,12 @@ func (c *Client) CreateTask(ctx context.Context, projectID int64, t *Task) (*Tas return &out, nil } -// UpdateTask updates a task (POST /tasks/{id}). bucket_id moves the task -// between buckets in the same view. +// UpdateTask updates a task (POST /tasks/{id}). This endpoint does NOT +// move tasks between buckets — the task↔bucket relation is row-shaped in +// task_buckets, and bucket_id on the request body is ignored. Use +// MoveTaskToBucket() for that. The server does auto-flip the bucket +// when `done` toggles, but only between the canonical "todo" and "done" +// buckets the project view is configured with. func (c *Client) UpdateTask(ctx context.Context, id int64, t *Task) (*Task, error) { var out Task if err := c.Do(ctx, "POST", fmt.Sprintf("/tasks/%d", id), nil, t, &out); err != nil { diff --git a/veans/internal/client/types.go b/veans/internal/client/types.go index 0a143807e..e661f5bc2 100644 --- a/veans/internal/client/types.go +++ b/veans/internal/client/types.go @@ -102,14 +102,19 @@ type Task struct { // side endpoint (e.g. the bucket-move POST); reads return it as 0. // The current bucket(s) — one per Kanban view — are exposed via // ?expand=buckets in the Buckets slice. - BucketID int64 `json:"bucket_id,omitempty"` - Buckets []*Bucket `json:"buckets,omitempty"` - Assignees []*User `json:"assignees,omitempty"` - Labels []*Label `json:"labels,omitempty"` - StartDate *time.Time `json:"start_date,omitempty"` - DueDate *time.Time `json:"due_date,omitempty"` - EndDate *time.Time `json:"end_date,omitempty"` - PercentDone float64 `json:"percent_done,omitempty"` + BucketID int64 `json:"bucket_id,omitempty"` + Buckets []*Bucket `json:"buckets,omitempty"` + Assignees []*User `json:"assignees,omitempty"` + Labels []*Label `json:"labels,omitempty"` + // RelatedTasks groups other tasks by relation kind ("blocking", + // "blocked", "parenttask", "subtask", "related", ...). Vikunja + // populates this on every task read; the nested tasks have their + // own RelatedTasks nil'd out server-side to avoid cycles. + RelatedTasks map[string][]*Task `json:"related_tasks,omitempty"` + StartDate *time.Time `json:"start_date,omitempty"` + DueDate *time.Time `json:"due_date,omitempty"` + EndDate *time.Time `json:"end_date,omitempty"` + PercentDone float64 `json:"percent_done,omitempty"` } // TaskComment matches pkg/models/task_comments.TaskComment. diff --git a/veans/internal/commands/api.go b/veans/internal/commands/api.go index 321c43724..d1e9669c9 100644 --- a/veans/internal/commands/api.go +++ b/veans/internal/commands/api.go @@ -85,18 +85,28 @@ Examples: body = []byte(dataFlag) } - status, respBody, err := rt.client.DoRaw(cmd.Context(), method, path, query, body) + status, respBody, retryAfter, err := rt.client.DoRaw(cmd.Context(), method, path, query, body) if err != nil { return err } - // On non-2xx, write the body to stderr and exit non-zero so - // shell pipelines see the failure clearly. + // On non-2xx, do NOT write the body to stdout — the agent + // contract is "stdout is the success payload". Fold a short + // snippet of the upstream error into the envelope message so + // the agent gets actionable context without a separate channel + // to parse. if status >= 400 { - fmt.Fprintf(cmd.ErrOrStderr(), "HTTP %d %s %s\n", status, method, path) - if _, werr := cmd.OutOrStdout().Write(respBody); werr != nil { - return werr + snippet := strings.TrimSpace(string(respBody)) + if len(snippet) > maxAPIErrorSnippet { + snippet = snippet[:maxAPIErrorSnippet] + "…(truncated)" } - return output.New(mapStatusToCode(status), "HTTP %d", status) + msg := fmt.Sprintf("HTTP %d %s %s", status, method, path) + if snippet != "" { + msg = fmt.Sprintf("%s: %s", msg, snippet) + } + if retryAfter > 0 { + msg = fmt.Sprintf("%s (retry-after %s)", msg, retryAfter) + } + return output.New(mapStatusToCode(status), "%s", msg) } if _, werr := cmd.OutOrStdout().Write(respBody); werr != nil { return werr @@ -110,6 +120,10 @@ Examples: return cmd } +// maxAPIErrorSnippet caps how much upstream-error body we fold into the +// `error` envelope field. Anything longer is almost always an HTML page. +const maxAPIErrorSnippet = 512 + func mapStatusToCode(status int) output.Code { switch { case status == 401, status == 403: diff --git a/veans/internal/commands/claim.go b/veans/internal/commands/claim.go index afcd0894b..a1e316a05 100644 --- a/veans/internal/commands/claim.go +++ b/veans/internal/commands/claim.go @@ -52,10 +52,6 @@ func newClaimCmd() *cobra.Command { rt.cfg.ProjectID, rt.cfg.ViewID, bid, id); err != nil { return err } - task, err := rt.client.GetTask(cmd.Context(), id) - if err != nil { - return err - } // Assign the bot. Idempotent on repeat — Vikunja returns 409 if // already assigned, which we map to a soft-skip. @@ -81,9 +77,9 @@ func newClaimCmd() *cobra.Command { } } - fresh, err := rt.client.GetTask(cmd.Context(), id) - if err == nil { - task = fresh + task, err := rt.client.GetTask(cmd.Context(), id) + if err != nil { + return err } return json.NewEncoder(cmd.OutOrStdout()).Encode(task) }, diff --git a/veans/internal/commands/create.go b/veans/internal/commands/create.go index e94103a9b..64450f713 100644 --- a/veans/internal/commands/create.go +++ b/veans/internal/commands/create.go @@ -56,7 +56,7 @@ func newCreateCmd() *cobra.Command { return json.NewEncoder(cmd.OutOrStdout()).Encode(task) }, } - cmd.Flags().StringVarP(&f.description, "description", "d", "", "task description (markdown)") + cmd.Flags().StringVarP(&f.description, "description", "d", "", "task description (HTML; see `veans prime` for canonical TipTap shapes)") cmd.Flags().StringVarP(&f.statusName, "status", "s", "todo", "initial status (defaults to todo)") cmd.Flags().Int64Var(&f.priority, "priority", 0, "priority (0=unset, 1=low, 5=DO_NOW)") cmd.Flags().StringSliceVar(&f.labels, "label", nil, "labels to attach (repeatable; veans: prefix added if missing)") diff --git a/veans/internal/commands/git.go b/veans/internal/commands/git.go index 433ba1c33..0ff1c8f62 100644 --- a/veans/internal/commands/git.go +++ b/veans/internal/commands/git.go @@ -18,6 +18,7 @@ package commands import ( "context" + "os" "os/exec" "strings" ) @@ -25,10 +26,34 @@ import ( // runGit runs `git ` in the current working directory and returns // trimmed stdout. Errors are returned to the caller so they can decide // whether silence or escalation is appropriate. +// +// The inherited environment is scrubbed of all GIT_* variables before +// invocation. Defense-in-depth: a stray GIT_DIR / GIT_WORK_TREE / +// GIT_INDEX_FILE in the caller's environment could redirect git to a +// different repository and cause downstream commands (e.g. `claim` +// attaching `veans:branch:`) to act on the wrong branch. +// GIT_OPTIONAL_LOCKS=0 is set so a concurrent git process holding the +// index lock can't block veans. func runGit(ctx context.Context, args ...string) (string, error) { - out, err := exec.CommandContext(ctx, "git", args...).Output() + cmd := exec.CommandContext(ctx, "git", args...) + cmd.Env = append(scrubGitEnv(os.Environ()), "GIT_OPTIONAL_LOCKS=0") + out, err := cmd.Output() if err != nil { return "", err } return strings.TrimRight(string(out), "\r\n"), nil } + +// scrubGitEnv returns env entries whose keys do not start with "GIT_". +// PATH and other essentials are preserved so git can still be located +// and configured normally (e.g. SSH_AUTH_SOCK, HOME, USER). +func scrubGitEnv(env []string) []string { + out := make([]string, 0, len(env)) + for _, kv := range env { + if strings.HasPrefix(kv, "GIT_") { + continue + } + out = append(out, kv) + } + return out +} diff --git a/veans/internal/commands/init.go b/veans/internal/commands/init.go index 33a8ccca8..dddc960fa 100644 --- a/veans/internal/commands/init.go +++ b/veans/internal/commands/init.go @@ -21,6 +21,7 @@ import ( "io" "os" "path/filepath" + "strings" "github.com/spf13/cobra" @@ -129,18 +130,35 @@ func printPostInitSummary(w io.Writer, res *bootstrap.Result) { if res.AgentChoices.ClaudeCode || res.AgentChoices.OpenCode { return } - fmt.Fprintln(w, ` + // Snippets are sourced from the bootstrap package so manual installs + // stay byte-for-byte equivalent to what `installAgentHooks` would have + // written — if a new hook event is added there, it shows up here too. + fmt.Fprintf(w, ` To wire veans into your coding agent later, paste one of these snippets: -Claude Code (.claude/settings.json): - { - "hooks": { - "SessionStart": [{ "hooks": [{ "type": "command", "command": "veans prime" }] }], - "PreCompact": [{ "hooks": [{ "type": "command", "command": "veans prime" }] }] - } - } +Claude Code (%s): +%s -OpenCode (.opencode/plugin/veans-prime.ts): see veans/README.md`) +OpenCode (%s): +%s`, bootstrap.ClaudeCodeSettingsRelPath, indent(bootstrap.ClaudeCodeHookSnippet(), " "), + bootstrap.OpenCodePluginRelPath, indent(bootstrap.OpenCodePluginSnippet, " ")) +} + +// indent prefixes every line of s with prefix. Used to inset the embedded +// snippets under their "Claude Code:" / "OpenCode:" headings without +// hard-coding the indent inside the bootstrap package's snippet strings. +func indent(s, prefix string) string { + if s == "" { + return s + } + lines := strings.Split(strings.TrimRight(s, "\n"), "\n") + for i, line := range lines { + if line == "" { + continue + } + lines[i] = prefix + line + } + return strings.Join(lines, "\n") } func identOrFallback(s string) string { diff --git a/veans/internal/commands/list.go b/veans/internal/commands/list.go index 8da136bdd..389aa9ebf 100644 --- a/veans/internal/commands/list.go +++ b/veans/internal/commands/list.go @@ -45,8 +45,8 @@ func newListCmd() *cobra.Command { Long: `List tasks in the project configured in .veans.yml. Filters can be combined; they're AND-ed together: - --ready ready to start: in Todo with done=false (incomplete-blocker - detection is best-effort, see veans/README.md) + --ready ready to start: in Todo, not done, and no incomplete + "blocked" relation --mine only tasks assigned to the veans bot --branch [name] only tasks tagged 'veans:branch:' (defaults to the current git branch when used without a value) @@ -78,21 +78,23 @@ Filters can be combined; they're AND-ed together: func runList(cmd *cobra.Command, rt *runtime, f *listFlags) ([]*client.Task, error) { opts := &client.TaskListOptions{ Filter: f.filter, - Expand: []string{"reactions"}, + // expand=buckets is required for CurrentBucketID() to resolve; + // the default GET returns bucket_id=0 (xorm:"-" on the model). + Expand: []string{"buckets"}, } tasks, err := rt.client.ListProjectTasks(cmd.Context(), rt.cfg.ProjectID, opts) if err != nil { return nil, err } - // Apply client-side filters AND-style. - var out []*client.Task + // Apply client-side filters AND-style. Pre-allocate as an empty + // (non-nil) slice so an empty result still encodes as `[]`, not `null` — + // the agent contract is "raw array". + out := make([]*client.Task, 0, len(tasks)) for _, t := range tasks { taskBucket := t.CurrentBucketID(rt.cfg.ViewID) - if f.ready { - if t.Done || taskBucket != rt.cfg.Buckets.Todo { - continue - } + if f.ready && !isReady(t, rt.cfg.Buckets.Todo, rt.cfg.ViewID) { + continue } if f.mine { if !taskAssignedTo(t, rt.cfg.Bot.UserID) { @@ -135,6 +137,21 @@ func runList(cmd *cobra.Command, rt *runtime, f *listFlags) ([]*client.Task, err return out, nil } +// isReady reports whether t is ready to start: in the Todo bucket, not done, +// and not blocked by any incomplete task. "blocked" is the relation kind on +// the dependent task — parenttask / subtask have no bearing on readiness. +func isReady(t *client.Task, todoBucket, viewID int64) bool { + if t.Done || t.CurrentBucketID(viewID) != todoBucket { + return false + } + for _, b := range t.RelatedTasks["blocked"] { + if b != nil && !b.Done { + return false + } + } + return true +} + func taskAssignedTo(t *client.Task, userID int64) bool { for _, a := range t.Assignees { if a != nil && a.ID == userID { diff --git a/veans/internal/commands/list_test.go b/veans/internal/commands/list_test.go new file mode 100644 index 000000000..e8fe2ab1d --- /dev/null +++ b/veans/internal/commands/list_test.go @@ -0,0 +1,152 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-present Vikunja and contributors. All rights reserved. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package commands + +import ( + "testing" + + "code.vikunja.io/veans/internal/client" +) + +func TestIsReady(t *testing.T) { + const todoBucket int64 = 11 + const viewID int64 = 7 + + cases := []struct { + name string + task *client.Task + want bool + }{ + { + name: "in todo, no relations -> ready", + task: &client.Task{ + ID: 1, + Buckets: []*client.Bucket{{ID: todoBucket, ProjectViewID: viewID}}, + }, + want: true, + }, + { + name: "subtask with only parenttask + blocking -> ready", + task: &client.Task{ + ID: 2, + Buckets: []*client.Bucket{{ID: todoBucket, ProjectViewID: viewID}}, + RelatedTasks: map[string][]*client.Task{ + "parenttask": {{ID: 1, Done: false}}, + "blocking": {{ID: 3, Done: false}}, + }, + }, + want: true, + }, + { + name: "blocked by incomplete task -> not ready", + task: &client.Task{ + ID: 3, + Buckets: []*client.Bucket{{ID: todoBucket, ProjectViewID: viewID}}, + RelatedTasks: map[string][]*client.Task{ + "blocked": {{ID: 2, Done: false}}, + }, + }, + want: false, + }, + { + name: "blocked by completed task -> ready", + task: &client.Task{ + ID: 3, + Buckets: []*client.Bucket{{ID: todoBucket, ProjectViewID: viewID}}, + RelatedTasks: map[string][]*client.Task{ + "blocked": {{ID: 2, Done: true}}, + }, + }, + want: true, + }, + { + name: "done task -> not ready", + task: &client.Task{ + ID: 4, + Done: true, + Buckets: []*client.Bucket{{ID: todoBucket, ProjectViewID: viewID}}, + }, + want: false, + }, + { + name: "in another bucket -> not ready", + task: &client.Task{ + ID: 5, + Buckets: []*client.Bucket{{ID: 99, ProjectViewID: viewID}}, + }, + want: false, + }, + { + name: "blocked by mix of done and incomplete -> not ready", + task: &client.Task{ + ID: 6, + Buckets: []*client.Bucket{{ID: todoBucket, ProjectViewID: viewID}}, + RelatedTasks: map[string][]*client.Task{ + "blocked": {{ID: 7, Done: true}, {ID: 8, Done: false}}, + }, + }, + want: false, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := isReady(tc.task, todoBucket, viewID); got != tc.want { + t.Fatalf("isReady = %v, want %v", got, tc.want) + } + }) + } +} + +// TestIsReady_SubtaskWithBlockedSibling pins the scenario from the bug +// report: two tasks share a parent; one has no "blocked" entry and is +// ready; the other is blocked by the first and is not ready. Once the +// first completes, the second becomes ready. +func TestIsReady_SubtaskWithBlockedSibling(t *testing.T) { + const todoBucket int64 = 11 + const viewID int64 = 7 + bucket := []*client.Bucket{{ID: todoBucket, ProjectViewID: viewID}} + + // Task #2: subtask of #1, blocks #3. No "blocked" relations of its own. + taskTwo := &client.Task{ + ID: 2, Done: false, Buckets: bucket, + RelatedTasks: map[string][]*client.Task{ + "parenttask": {{ID: 1, Done: false}}, + "blocking": {{ID: 3, Done: false}}, + }, + } + // Task #3: subtask of #1, blocked by #2. + taskThree := &client.Task{ + ID: 3, Done: false, Buckets: bucket, + RelatedTasks: map[string][]*client.Task{ + "parenttask": {{ID: 1, Done: false}}, + "blocked": {{ID: 2, Done: false}}, + }, + } + + if !isReady(taskTwo, todoBucket, viewID) { + t.Fatal("task #2 should be ready (no incomplete blockers)") + } + if isReady(taskThree, todoBucket, viewID) { + t.Fatal("task #3 should not be ready (blocked by incomplete #2)") + } + + // Now complete #2 and reflect that in #3's relation snapshot. + taskThree.RelatedTasks["blocked"][0].Done = true + if !isReady(taskThree, todoBucket, viewID) { + t.Fatal("task #3 should be ready once #2 is done") + } +} diff --git a/veans/internal/commands/prime.go b/veans/internal/commands/prime.go index bd7a2c803..dbf52b1ab 100644 --- a/veans/internal/commands/prime.go +++ b/veans/internal/commands/prime.go @@ -17,10 +17,12 @@ package commands import ( + "context" _ "embed" "errors" "fmt" "text/template" + "time" "github.com/spf13/cobra" @@ -69,11 +71,15 @@ silently with status 0 — that makes the hook safe to install globally.`, // Fetch the project title for nicer prompt copy. Best-effort — // if the API call fails (network blip, expired token), we fall // back to "(unknown)" rather than aborting the prompt render. + // Cap the lookup at 10s so a wedged server can't hold the + // SessionStart hook hostage. projectTitle := "(unknown)" if rt, err := loadRuntime(); err == nil { - if p, err := rt.client.GetProject(cmd.Context(), cfg.ProjectID); err == nil { + ctx, cancel := context.WithTimeout(cmd.Context(), 10*time.Second) + if p, err := rt.client.GetProject(ctx, cfg.ProjectID); err == nil { projectTitle = p.Title } + cancel() } data := primeContext{ @@ -95,5 +101,3 @@ silently with status 0 — that makes the hook safe to install globally.`, }, } } - -// silence linter noise on unused symbols when wiring hooks. diff --git a/veans/internal/commands/update.go b/veans/internal/commands/update.go index 5d6471376..a0d272864 100644 --- a/veans/internal/commands/update.go +++ b/veans/internal/commands/update.go @@ -169,7 +169,7 @@ func runUpdate(ctx context.Context, rt *runtime, id int64, f *updateFlags) (*cli // Comment first when transitioning to scrapped — the reason is part of // the audit trail and should appear before the bucket move in the log. if newStatus == status.Scrapped { - if _, err := rt.client.AddTaskComment(ctx, id, "**Scrapped:** "+strings.TrimSpace(f.reason)); err != nil { + if _, err := rt.client.AddTaskComment(ctx, id, "Scrapped: "+strings.TrimSpace(f.reason)); err != nil { return nil, err } } @@ -218,7 +218,7 @@ func runUpdate(ctx context.Context, rt *runtime, id int64, f *updateFlags) (*cli } } - if len(f.addLabels) > 0 || len(f.removeLabels) > 0 { + if len(f.addLabels) > 0 || len(f.removeLabels) > 0 || bucketTransitionTarget != 0 { fresh, err := rt.client.GetTask(ctx, id) if err == nil { updated = fresh diff --git a/veans/internal/credentials/file.go b/veans/internal/credentials/file.go index 5d0c5f362..4a8e22c83 100644 --- a/veans/internal/credentials/file.go +++ b/veans/internal/credentials/file.go @@ -28,6 +28,10 @@ import ( // FileBackend persists credentials to ~/.config/veans/credentials.yml at // mode 0600. It's the fallback when no keychain is available (CI, Docker, // headless servers) and is the implicit backend e2e tests use. +// +// Writes are atomic (tmp file + rename) and serialized by an advisory +// flock on a sibling .lock file so two concurrent `veans login` runs can +// each install their token without losing the other's. type FileBackend struct { path string } @@ -42,9 +46,8 @@ type fileSchema struct { Credentials []fileEntry `yaml:"credentials"` } -// NewFileBackend builds a FileBackend rooted at `path`, or the platform -// default (~/.config/veans/credentials.yml, honoring XDG_CONFIG_HOME) when -// path is "". +// NewFileBackend builds a FileBackend rooted at `path`, or +// ~/.config/veans/credentials.yml when path is "". func NewFileBackend(path string) *FileBackend { if path == "" { path = defaultCredsPath() @@ -55,14 +58,17 @@ func NewFileBackend(path string) *FileBackend { func (b *FileBackend) Name() string { return "file" } func (b *FileBackend) Path() string { return b.path } +// defaultCredsPath returns ~/.config/veans/credentials.yml, falling back to +// "" (which signals an error to NewFileBackend's caller) when there's no +// resolvable home directory. We deliberately do not honor XDG_CONFIG_HOME +// — it gave us a path-traversal seam for no real benefit, since the +// agent-only audience runs in a known environment. func defaultCredsPath() string { - if c := os.Getenv("XDG_CONFIG_HOME"); c != "" { - return filepath.Join(c, "veans", "credentials.yml") + h, err := os.UserHomeDir() + if err != nil || h == "" { + return "" } - if h, err := os.UserHomeDir(); err == nil { - return filepath.Join(h, ".config", "veans", "credentials.yml") - } - return filepath.Join(".", "credentials.yml") + return filepath.Join(h, ".config", "veans", "credentials.yml") } func (b *FileBackend) load() (*fileSchema, error) { @@ -80,7 +86,10 @@ func (b *FileBackend) load() (*fileSchema, error) { return &s, nil } -func (b *FileBackend) save(s *fileSchema) error { +// save writes the schema atomically (tmpfile + rename) at mode 0600 and +// re-asserts the mode on the destination inode in case an earlier write +// left a wider mode behind. +func (b *FileBackend) save(s *fileSchema) (rerr error) { if err := os.MkdirAll(filepath.Dir(b.path), 0o700); err != nil { return err } @@ -88,10 +97,50 @@ func (b *FileBackend) save(s *fileSchema) error { if err != nil { return err } - return os.WriteFile(b.path, buf, 0o600) + tmp, err := os.CreateTemp(filepath.Dir(b.path), ".credentials-*.tmp") + if err != nil { + return err + } + tmpPath := tmp.Name() + // On any error before the rename completes, drop the half-written + // temp file. tmpPath is the return of CreateTemp on a directory we + // own, so gosec's path-traversal warning doesn't apply. + defer func() { + if rerr != nil { + _ = tmp.Close() + _ = os.Remove(tmpPath) //nolint:gosec // G703: tmpPath came from os.CreateTemp on a dir we control + } + }() + // CreateTemp opens at 0600 already, but be defensive: an inherited + // umask shouldn't matter for CreateTemp on POSIX, but explicit is + // cheaper than debugging later. + if err := tmp.Chmod(0o600); err != nil { + return err + } + if _, err := tmp.Write(buf); err != nil { + return err + } + if err := tmp.Sync(); err != nil { + return err + } + if err := tmp.Close(); err != nil { + return err + } + // gosec/G703: both paths come from FileBackend state (b.path is set + // from defaultCredsPath or an explicit constructor arg, tmpPath from + // CreateTemp on the same dir); neither is runtime user-influenceable. + if err := os.Rename(tmpPath, b.path); err != nil { //nolint:gosec + return err + } + // Belt-and-braces: a pre-existing destination at 0644 keeps its mode + // across Rename on some filesystems. Narrow it. + return os.Chmod(b.path, 0o600) } func (b *FileBackend) Get(server, account string) (string, error) { + if b.path == "" { + return "", ErrNotFound + } s, err := b.load() if err != nil { return "", err @@ -104,7 +153,26 @@ func (b *FileBackend) Get(server, account string) (string, error) { return "", ErrNotFound } +// Set serializes load → mutate → atomic save under an advisory flock on +// `.lock` so two concurrent `veans login` runs don't clobber each +// other's tokens. func (b *FileBackend) Set(server, account, token string) error { + if b.path == "" { + return errors.New("no credentials path: $HOME is unset and no explicit path was given") + } + if err := os.MkdirAll(filepath.Dir(b.path), 0o700); err != nil { + return err + } + lockF, err := os.OpenFile(b.path+".lock", os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + return fmt.Errorf("open lock file: %w", err) + } + defer lockF.Close() + if err := flockExclusive(lockF); err != nil { + return fmt.Errorf("acquire lock: %w", err) + } + defer flockUnlock(lockF) //nolint:errcheck // unlock-on-close is sufficient + s, err := b.load() if err != nil { return err diff --git a/veans/internal/credentials/lock_other.go b/veans/internal/credentials/lock_other.go new file mode 100644 index 000000000..ceb84c160 --- /dev/null +++ b/veans/internal/credentials/lock_other.go @@ -0,0 +1,28 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-present Vikunja and contributors. All rights reserved. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +//go:build !unix + +package credentials + +import "os" + +// Non-Unix platforms (Windows) don't get advisory file locking. Two +// concurrent `veans login` runs on the same Windows machine can race and +// lose a token; in practice veans runs from agent-driven shells on Linux +// or macOS, so this trade-off is acceptable. +func flockExclusive(*os.File) error { return nil } +func flockUnlock(*os.File) error { return nil } diff --git a/veans/internal/credentials/lock_unix.go b/veans/internal/credentials/lock_unix.go new file mode 100644 index 000000000..890972053 --- /dev/null +++ b/veans/internal/credentials/lock_unix.go @@ -0,0 +1,36 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-present Vikunja and contributors. All rights reserved. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +//go:build unix + +package credentials + +import ( + "os" + + "golang.org/x/sys/unix" +) + +// flockExclusive takes an exclusive advisory lock on the open file. The lock +// is released when the file is closed (or via flockUnlock). Blocks until the +// lock is available. +func flockExclusive(f *os.File) error { + return unix.Flock(int(f.Fd()), unix.LOCK_EX) +} + +func flockUnlock(f *os.File) error { + return unix.Flock(int(f.Fd()), unix.LOCK_UN) +} diff --git a/veans/internal/output/errors.go b/veans/internal/output/errors.go index 1e46d70ea..68bfc5256 100644 --- a/veans/internal/output/errors.go +++ b/veans/internal/output/errors.go @@ -81,6 +81,6 @@ func EmitError(err error, w io.Writer) { } e := AsError(err) if encErr := json.NewEncoder(w).Encode(e); encErr != nil { - fmt.Fprintf(os.Stderr, "veans: failed to encode error envelope: %v\n", encErr) + fmt.Fprintf(os.Stderr, "{\"code\":%q,\"error\":%q}\n", string(CodeUnknown), "failed to encode error envelope: "+encErr.Error()) } } diff --git a/veans/internal/status/status.go b/veans/internal/status/status.go index 68c31e613..b29904981 100644 --- a/veans/internal/status/status.go +++ b/veans/internal/status/status.go @@ -24,6 +24,7 @@ import ( "strings" "code.vikunja.io/veans/internal/config" + "code.vikunja.io/veans/internal/output" ) // Status is the agent-facing state name. @@ -122,7 +123,8 @@ func Parse(raw string) (Status, error) { case "scrapped", "cancelled", "canceled": return Scrapped, nil } - return "", fmt.Errorf("unknown status %q (expected one of: %s)", + return "", output.New(output.CodeValidation, + "unknown status %q (expected one of: %s)", raw, strings.Join(allStrings(), ", ")) } diff --git a/veans/magefile.go b/veans/magefile.go index 70c89d2b0..16f50edd5 100644 --- a/veans/magefile.go +++ b/veans/magefile.go @@ -60,22 +60,27 @@ func Fmt() error { // Test namespace. type Test mg.Namespace -// All runs unit tests across the module. +// All runs unit tests across the module. Passes `-short` so the e2e +// package self-skips via its TestMain — the parent monorepo's +// pkg/webtests follows the same convention. func (Test) All() error { - return sh.RunV("go", "test", "./...") + return sh.RunV("go", "test", "-short", "./...") } -// Filter runs `go test -run ./...` — pass the expression as an argument. +// Filter runs `go test -short -run ./...` — pass the expression as +// an argument. `-short` is included so e2e doesn't run accidentally; use +// `mage test:e2e` for those. func (Test) Filter(expr string) error { if expr == "" { return fmt.Errorf("test:filter requires a regexp argument") } - return sh.RunV("go", "test", "-run", expr, "./...") + return sh.RunV("go", "test", "-short", "-run", expr, "./...") } -// E2E runs the e2e suite. Requires VEANS_E2E_API_URL to point at a running -// Vikunja instance and either VEANS_E2E_ADMIN_TOKEN or -// VEANS_E2E_ADMIN_USER + VEANS_E2E_ADMIN_PASS for the admin/seed identity. +// E2E runs the e2e suite without `-short` so TestMain lets it through. +// Requires VEANS_E2E_API_URL to point at a running Vikunja instance and +// either VEANS_E2E_ADMIN_TOKEN or VEANS_E2E_ADMIN_USER + VEANS_E2E_ADMIN_PASS +// for the admin/seed identity. // // Set VEANS_E2E_SKIP_BUILD=true to reuse a previously-built binary. func (Test) E2E() error {