P2-1: HTTP client core — auth, base URL, error mapping #21

New issue

Closed

opened 2026-09-10 17:40:45 +00:00 by Cordy · 1 comment

Cordy commented

2026-09-10 17:40:45 +00:00

Owner

First task of phase 2. Depends on the phase-1-engine milestone being complete.

Goal

The plumbing every WebDAV verb sits on: base URL joining, authentication, timeouts, and turning HTTP status codes into typed Go errors the engine and UI can act on.

Files

Create: internal/remote/client.go, internal/remote/errors.go, internal/remote/client_test.go

Produces

type Config struct {
	BaseURL  string        // e.g. https://cairn.example.org/dav/
	Username string
	Password string        // app password on OIDC instances; directory password on LDAP/local
	Timeout  time.Duration
	UserAgent string
}

type Client struct{ /* … */ }
func New(cfg Config) (*Client, error)
func (c *Client) do(method, path string, body io.Reader, hdr http.Header) (*http.Response, error)

Typed errors — the engine branches on these

var (
	ErrUnauthorized  = errors.New("remote: unauthorized")        // 401
	ErrForbidden     = errors.New("remote: forbidden")           // 403
	ErrNotFound      = errors.New("remote: not found")           // 404
	ErrConflict      = errors.New("remote: conflict")            // 409
	ErrLocked        = errors.New("remote: locked")              // 423
	ErrQuotaExceeded = errors.New("remote: quota exceeded")      // 507
)

507 matters most. Cairn returns it when a quota is exhausted. It must surface as a clear, actionable state in the UI — never a retry loop, which would hammer the server forever with no chance of success.

401 matters second. It means the app password was revoked; the client must stop and ask for re-authentication rather than retrying.

Steps

Write failing tests using httptest.NewServer:
- base URL join handles trailing/missing slashes and percent-encodes path segments (spaces, #, +, non-ASCII)
- basic auth header is set
- each status above maps to the right sentinel error (assert with errors.Is)
- a 500 returns a non-sentinel error carrying the status and body snippet
Run them; confirm they fail.
Implement. Use url.PathEscape per segment, not on the whole path — escaping / would break it.
Run tests; confirm they pass.
Commit: git commit -s -m "feat(remote): HTTP client core with typed errors"

Acceptance criteria

Every sentinel error is produced by a test against a fake server.
Paths containing spaces and non-ASCII characters round-trip correctly.
No credential is ever written to a log line.

First task of phase 2. Depends on the `phase-1-engine` milestone being complete. ## Goal The plumbing every WebDAV verb sits on: base URL joining, authentication, timeouts, and turning HTTP status codes into typed Go errors the engine and UI can act on. ## Files - Create: `internal/remote/client.go`, `internal/remote/errors.go`, `internal/remote/client_test.go` ## Produces ```go type Config struct { BaseURL string // e.g. https://cairn.example.org/dav/ Username string Password string // app password on OIDC instances; directory password on LDAP/local Timeout time.Duration UserAgent string } type Client struct{ /* … */ } func New(cfg Config) (*Client, error) func (c *Client) do(method, path string, body io.Reader, hdr http.Header) (*http.Response, error) ``` ## Typed errors — the engine branches on these ```go var ( ErrUnauthorized = errors.New("remote: unauthorized") // 401 ErrForbidden = errors.New("remote: forbidden") // 403 ErrNotFound = errors.New("remote: not found") // 404 ErrConflict = errors.New("remote: conflict") // 409 ErrLocked = errors.New("remote: locked") // 423 ErrQuotaExceeded = errors.New("remote: quota exceeded") // 507 ) ``` **507 matters most.** Cairn returns it when a quota is exhausted. It must surface as a clear, actionable state in the UI — **never** a retry loop, which would hammer the server forever with no chance of success. **401 matters second.** It means the app password was revoked; the client must stop and ask for re-authentication rather than retrying. ## Steps - [ ] Write failing tests using `httptest.NewServer`: - base URL join handles trailing/missing slashes and percent-encodes path segments (spaces, `#`, `+`, non-ASCII) - basic auth header is set - each status above maps to the right sentinel error (assert with `errors.Is`) - a 500 returns a non-sentinel error carrying the status and body snippet - [ ] Run them; confirm they fail. - [ ] Implement. Use `url.PathEscape` per segment, not on the whole path — escaping `/` would break it. - [ ] Run tests; confirm they pass. - [ ] Commit: `git commit -s -m "feat(remote): HTTP client core with typed errors"` ## Acceptance criteria - Every sentinel error is produced by a test against a fake server. - Paths containing spaces and non-ASCII characters round-trip correctly. - No credential is ever written to a log line.

Cordy added this to the phase-2-integration milestone

2026-09-10 17:40:45 +00:00

Cordy added the

remote

label

2026-09-10 17:40:45 +00:00

Cordy commented

2026-09-11 02:42:55 +00:00

Author

Owner

Done

3ab6299d6a — feat(remote): HTTP client core with typed errors
b8a1dddd68 — fix(remote): bound Client.Timeout to dial/TLS/headers, not the body; never leak BaseURL credentials

What was built

internal/remote/client.go + errors.go: Config, Client, New, unexported do, the six sentinel errors, StatusError, per-segment url.PathEscape join in resolve.
404 is mapped once at the status layer to satisfy both errors.Is(err, ErrNotFound) and errors.Is(err, fs.ErrNotExist) (P2-R3).
Fix round: Timeout now bounds only dial/TLS/response-header wait (cloned *http.Transport), not the body read; New rejects a BaseURL with userinfo and both String() methods redact via url.URL.Redacted.

Tests

client_test.go: sentinel-error mapping (401/403/404/409/423/507) against httptest.Server with errors.Is; base-URL join + percent-encoding (space, #, +, non-ASCII) verified against both decoded r.URL.Path and raw r.RequestURI; Basic-auth header; unmapped 500; credential-redaction tests; two new timeout tests (header stall vs. a slow steady body longer than Timeout).
CI run 19 (ci.yml): http://192.168.10.245/Cordy/cairn-desktop/actions/runs/19 — green, arm64 runner, internal/remote 92.1% coverage, package total 91.2%.

Acceptance criteria

Every sentinel error produced by a test against a fake server — TestDoMapsStatusToSentinelErrors, all six statuses.
Paths with spaces/non-ASCII round-trip correctly — TestDoJoinsBaseURLAndPercentEncodesSegments.
No credential ever written to a log line — Config.String/Client.String redact password and any BaseURL userinfo; review finding F2 (a BaseURL-embedded password leaking through both Stringers and New's parse error) fixed in b8a1ddd with dedicated regression tests.

Rulings

P2-R1: push to main after clean review; close only once every Actions run for the pushed commit is green; comment+close, never edit issue bodies; go.mod stays go 1.25, go mod tidy byte-identical.
P2-R3: Client.Get/Delete/Move on a missing path must satisfy both errors.Is(err, ErrNotFound) and errors.Is(err, fs.ErrNotExist) — done once in sentinelForStatus.
P2-R8: retry/error classification lives in internal/remote; internal/sync never imports net/net/http — out of scope for this issue, StatusError.Status left exposed for later.
P2-R13: the remote sync root is a WebDAV collection URL (Config.BaseURL); Cairn's default per-user home is <server>/dav/home/.
progress.md CV1: pushed only after clean re-review; closed only when the pushed head's Actions run is green.
progress.md CV2: RED reproduced independently from adc10aa + client_test.go alone (build failed on undefined Client/New/Config).
progress.md CV3: go vet/gofmt/go mod tidy/whole-suite re-verified independently on host, windows/amd64 and linux/arm64.
progress.md CV4: no live-cairnd test needed for #21 — do() sends Basic auth pre-emptively and maps a challenge-less 401 by status alone; runtime confirmation deferred to #29.

Deferred

F3–F7 (Minor, task-21-review.md) deferred to the final review: error-body draining/comment, stricter BaseURL validation and the string-rebuild in resolve, redirect policy, test-gap hardening (exact non-ASCII/percent-literal encoding), and making the bare ErrNotFound sentinel itself satisfy fs.ErrNotExist.

Implemented and reviewed by Claude (subagent-driven), landed on main after review and green CI.

**Done** - http://192.168.10.245/Cordy/cairn-desktop/commit/3ab6299d6a46ad4511d604718ee7057f978f496b — feat(remote): HTTP client core with typed errors - http://192.168.10.245/Cordy/cairn-desktop/commit/b8a1dddd683f98ffcb5ae1063dee5de6fec946c4 — fix(remote): bound Client.Timeout to dial/TLS/headers, not the body; never leak BaseURL credentials **What was built** - `internal/remote/client.go` + `errors.go`: `Config`, `Client`, `New`, unexported `do`, the six sentinel errors, `StatusError`, per-segment `url.PathEscape` join in `resolve`. - 404 is mapped once at the status layer to satisfy both `errors.Is(err, ErrNotFound)` and `errors.Is(err, fs.ErrNotExist)` (P2-R3). - Fix round: `Timeout` now bounds only dial/TLS/response-header wait (cloned `*http.Transport`), not the body read; `New` rejects a `BaseURL` with userinfo and both `String()` methods redact via `url.URL.Redacted`. **Tests** - `client_test.go`: sentinel-error mapping (401/403/404/409/423/507) against `httptest.Server` with `errors.Is`; base-URL join + percent-encoding (space, `#`, `+`, non-ASCII) verified against both decoded `r.URL.Path` and raw `r.RequestURI`; Basic-auth header; unmapped 500; credential-redaction tests; two new timeout tests (header stall vs. a slow steady body longer than `Timeout`). - CI run 19 (ci.yml): http://192.168.10.245/Cordy/cairn-desktop/actions/runs/19 — green, arm64 runner, `internal/remote` 92.1% coverage, package total 91.2%. **Acceptance criteria** - Every sentinel error produced by a test against a fake server — `TestDoMapsStatusToSentinelErrors`, all six statuses. - Paths with spaces/non-ASCII round-trip correctly — `TestDoJoinsBaseURLAndPercentEncodesSegments`. - No credential ever written to a log line — `Config.String`/`Client.String` redact password and any `BaseURL` userinfo; review finding F2 (a `BaseURL`-embedded password leaking through both Stringers and `New`'s parse error) fixed in b8a1ddd with dedicated regression tests. **Rulings** - P2-R1: push to main after clean review; close only once every Actions run for the pushed commit is green; comment+close, never edit issue bodies; `go.mod` stays `go 1.25`, `go mod tidy` byte-identical. - P2-R3: `Client.Get`/`Delete`/`Move` on a missing path must satisfy both `errors.Is(err, ErrNotFound)` and `errors.Is(err, fs.ErrNotExist)` — done once in `sentinelForStatus`. - P2-R8: retry/error classification lives in `internal/remote`; `internal/sync` never imports `net`/`net/http` — out of scope for this issue, `StatusError.Status` left exposed for later. - P2-R13: the remote sync root is a WebDAV collection URL (`Config.BaseURL`); Cairn's default per-user home is `<server>/dav/home/`. - progress.md CV1: pushed only after clean re-review; closed only when the pushed head's Actions run is green. - progress.md CV2: RED reproduced independently from `adc10aa` + `client_test.go` alone (build failed on undefined `Client`/`New`/`Config`). - progress.md CV3: `go vet`/`gofmt`/`go mod tidy`/whole-suite re-verified independently on host, windows/amd64 and linux/arm64. - progress.md CV4: no live-cairnd test needed for #21 — `do()` sends Basic auth pre-emptively and maps a challenge-less 401 by status alone; runtime confirmation deferred to #29. **Deferred** - F3–F7 (Minor, `task-21-review.md`) deferred to the final review: error-body draining/comment, stricter `BaseURL` validation and the string-rebuild in `resolve`, redirect policy, test-gap hardening (exact non-ASCII/percent-literal encoding), and making the bare `ErrNotFound` sentinel itself satisfy `fs.ErrNotExist`. _Implemented and reviewed by Claude (subagent-driven), landed on main after review and green CI._

Cordy closed this issue

2026-09-11 02:43:00 +00:00

Cordy referenced this issue

2026-09-11 05:11:54 +00:00

P2-3: GET, PUT, MKCOL, DELETE, MOVE — complete the Remote interface #23