diff --git a/.cursor/bugbot-cmd.md b/.cursor/bugbot-cmd.md index 420939013..4299fbf43 100644 --- a/.cursor/bugbot-cmd.md +++ b/.cursor/bugbot-cmd.md @@ -27,3 +27,16 @@ Prefer `outputformat.PrintJSONEnvelope` for structured output so the payload is ## Pagination Safety Validate that pagination never crosses host boundaries. + +## Repeated Flag Shapes Register Through a Shared pflag.Value + +A flag shape that recurs across commands, or carries the same validation requirement wherever +it appears (a count, a duration, an ID format), must register through one shared `pflag.Value` +(see `internal/countflags`, `cmd/internal/pollflags`) — not a fresh ad hoc check hand-rolled in +each command's `RunE`. + +## Sentinel Defaults Are a Documented Exception + +A flag where a value like `0` means "unset, use a default" (a port, a replica count) must not +be forced through a shared validator built for a different flag's semantics — but the exception +must be called out. diff --git a/.cursor/bugbot-design.md b/.cursor/bugbot-design.md index 7e96812e5..9d19089dd 100644 --- a/.cursor/bugbot-design.md +++ b/.cursor/bugbot-design.md @@ -43,3 +43,8 @@ Extract utility functions only after the same code appears in 2+ independent pla ## Scope Discipline: Foundational PRs Foundational PRs should do architectural work without bundling unrelated refactorings. + +## Document Intentional Asymmetry Between Look-Alike Siblings + +When two structurally similar functions/phases behave differently on failure by design, say so +inline — otherwise a later refactor "fixes" the asymmetry and breaks the reason it existed. diff --git a/.cursor/bugbot-errors.md b/.cursor/bugbot-errors.md index 41f445437..6005af55a 100644 --- a/.cursor/bugbot-errors.md +++ b/.cursor/bugbot-errors.md @@ -30,3 +30,14 @@ one shared sentinel — not two phrasings that differ by which path reached the An error whose only consumer is a boolean — file-type predicates, "is this ours" checks — must not be phrased as user guidance. Someone will later "align" it with a real message and put a remedy into a string nothing prints. + +## Terminal Errors Must Name the Exact Recovery Command + +A refusal in a terminal/blocked state must name the exact command that unblocks it, not "check +the logs" — and that command must not itself dead-end into another unexplained error. + +## Retry Safety Must Be Classified by Response Shape, Not Status Code Alone + +A status code that's sometimes-safe to retry (502/504) must not be retried using the same rule +as one that's always safe (503) — check whether the response proves the request was actually +received before assuming a retry can't double-submit. diff --git a/.cursor/bugbot-package-design.md b/.cursor/bugbot-package-design.md index bcf62a26e..2dd153cdf 100644 --- a/.cursor/bugbot-package-design.md +++ b/.cursor/bugbot-package-design.md @@ -20,3 +20,19 @@ Document how functions fail, especially for streaming, long-running, and resourc Intentional limitations must be tracked with JIRA issues. However in package documentation or in the codebase, do NOT list issues by number. Just describe the limitation and the intended future work to address it. + +## A New API Wrapper Extends drapi, Never Re-Implements It + +A package adding typed request/response shapes or a caller-specific error envelope over a +DataRobot API surface (see `internal/drapi/filesapi`, `internal/workload/apiclient`) must build +entirely on `drapi`'s verb functions, `EndpointURL`, and `HTTPError`. If the new package +constructs its own `http.Client`, re-derives a timeout default, or defines a second error type +shaped like `HTTPError`, that's a sign it's duplicating `drapi` rather than extending it. + +## Non-DataRobot-API Calls Do Not Belong in drapi + +A request that isn't to the authenticated DataRobot platform API — a local OAuth-redirect +handshake, a plugin download, a third-party registry fetch — must not be routed through +`drapi`'s verb functions or forced through `AuthorizeRequest`/`URLMatchesConfiguredBase`. Those +exist to protect a bearer token that has no business being attached to an unrelated origin; a +plain, purpose-built client for that call is correct, not a gap. diff --git a/.cursor/bugbot-resources.md b/.cursor/bugbot-resources.md index d6e8ae987..c2f2230c7 100644 --- a/.cursor/bugbot-resources.md +++ b/.cursor/bugbot-resources.md @@ -27,3 +27,8 @@ Rollback and restore operations must be idempotent. ## File Permissions & Attributes Preserved File modes, symlinks, and extended attributes must be preserved through backup and restore cycles. + +## Duplicated Constants Need a Canonical-Source Comment + +When an import cycle forces a second copy of a shared constant (timeout, retry count), comment +it as a duplicate pointing at the canonical definition — so the two don't drift silently. diff --git a/.cursor/bugbot-security.md b/.cursor/bugbot-security.md index a52f5c2e2..930c83fcf 100644 --- a/.cursor/bugbot-security.md +++ b/.cursor/bugbot-security.md @@ -12,6 +12,22 @@ Write tests that attempt to override security constraints and verify they fail. Validate input at every package boundary — don't assume upstream packages provide safe data. +Worked example: `docs/development/drapi-client.md` — before attaching a bearer token to +a server-supplied URL (pagination cursor, live endpoint), verify it with +`drapi.URLMatchesConfiguredBase()` first. + +## Local Callback Listeners Must Validate Request Provenance + +A listener bound to localhost for an OAuth-style handoff is reachable by any open web page — +localhost has no same-origin restriction. It must check something a page can't forge +(`Sec-Fetch-Dest`, a nonce) before treating a request as authentic, not just accept whatever +hits the port. + +## Security Mitigation Claims Must Name the Residual Bypass + +A doc or comment describing a security fix must scope its claim to the exact vectors it blocks +and name what still gets through — not claim a blanket guarantee the fix doesn't provide. + ## Validate Integration Points Explicitly Verify inter-package contracts with integration tests, not just assumptions. diff --git a/.cursor/bugbot-validation.md b/.cursor/bugbot-validation.md index 5cb23279a..f1e744f1a 100644 --- a/.cursor/bugbot-validation.md +++ b/.cursor/bugbot-validation.md @@ -35,3 +35,8 @@ Validation tests must cover both success and failure paths. ## Validation Error Messages Validation errors must name the field that failed and explain why. + +## Heuristic Detection Must Degrade, Never Refuse + +A check built on heuristics (missing project markers, ambiguous environment) must warn or +prompt, never hard-block — heuristics have false positives a hard refusal can't recover from. diff --git a/AGENTS.md b/AGENTS.md index d3d44f409..5993bda25 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -167,6 +167,14 @@ For full details, see [docs/development/configuration.md](docs/development/confi - **To make a key persistable**, add it to `config.PersistableKeys` and have the write site call `config.UpdateConfigFile("my-key")`. +## Auth & API Client Conventions + +For OAuth/browser login internals, see [docs/development/authentication.md](docs/development/authentication.md). +For wiring in a new authenticated DataRobot API call—timeout clamping, +the `HTTPError`/`errors.As` contract, and the origin-safety check before +attaching credentials to a server-supplied URL—see +[docs/development/drapi-client.md](docs/development/drapi-client.md). + ## Code Review Guidelines All PRs are reviewed against **bugbot rules** in [.cursor/BUGBOT.md](.cursor/BUGBOT.md). Rules are organized by risk level: diff --git a/docs/development/README.md b/docs/development/README.md index c7848605d..35ce52f64 100644 --- a/docs/development/README.md +++ b/docs/development/README.md @@ -25,6 +25,7 @@ If you're new to developing the CLI, start here: - **[Tool registry](tool-registry.md)**—how install-hint strategies and manager detection work; how to add a new tool or package manager. - **[Plugins](plugins.md)**—develop and test CLI plugins, understand the plugin system architecture. - **[Remote plugins](remote-plugins.md)**—create and distribute remote plugins, plugin registry management. +- **[drapi client conventions](drapi-client.md)**—how to wire in a new authenticated API call: timeout clamping, the `HTTPError`/`errors.As` contract, and origin-safety checks before attaching credentials to a server-supplied URL. - **[Workload state directory validation](workload-wapi-validation.md)**—reference for `validator/v10` tags and cross-field rules for workload local state. - **[Editing user-owned files](editing-user-files.md)**—rules for in-place edits to `.datarobot.yaml` and `.env`: comment preservation, round-trip symmetry, and when a refusal beats a rewrite. - **[Releasing](releasing.md)**—release process, versioning strategy, and GoReleaser configuration. diff --git a/docs/development/authentication.md b/docs/development/authentication.md index 52def4ea0..c72381924 100644 --- a/docs/development/authentication.md +++ b/docs/development/authentication.md @@ -171,3 +171,7 @@ at `0644` by an older CLI would otherwise stay world-readable forever. When attaching credentials to an absolute URL returned by a server response, first check it with `drapi.URLMatchesConfiguredBase()`. URLs built locally with `config.GetEndpointURL()` already stay on the configured DataRobot host. + +For how the bearer token is attached to outgoing requests once you have +it—and the timeout/error-handling conventions for any new authenticated +API call—see [drapi client conventions](drapi-client.md). diff --git a/docs/development/drapi-client.md b/docs/development/drapi-client.md new file mode 100644 index 000000000..76f9f983e --- /dev/null +++ b/docs/development/drapi-client.md @@ -0,0 +1,74 @@ +# drapi client conventions + +`internal/drapi` is the one shared HTTP client every authenticated call to +the DataRobot API is expected to build on. This page is the dev-doc mirror +of `internal/drapi/doc.go` (`go doc ./internal/drapi` for the canonical +source) — read this first when wiring in a new API call; read the package +doc when you need the full detail. + +## Always build on the verb functions + +Use `drapi.Get`, `drapi.Post`, `drapi.Patch`, or `drapi.Delete` (or their +`*JSON` variants) with a URL from `drapi.EndpointURL()` / +`config.GetEndpointURL()`. Never construct your own `http.Client` or set +the `Authorization` header by hand — `AuthorizeRequest` (`internal/drapi/auth.go`) +is called internally by every verb function and sets the bearer token, +`User-Agent`, and the optional API-consumer trace header consistently. + +If you must build your own `*http.Request` (multipart uploads, custom error +decoding), use `drapi.Do(req, timeout)` rather than duplicating +`NewHTTPClient(t).Do(req)` — it applies the same timeout clamp described +below. + +## Timeouts + +Every verb function takes an optional trailing `time.Duration`. Omit it, or +pass `<=0`, and `NewHTTPClient` clamps it to `DefaultClientTimeout` (30s) +internally — the clamp cannot be bypassed by a caller. This was tightened +specifically so a stray `0` or negative duration could never produce +an unbounded request. + +## The `HTTPError` / `errors.As` contract + +A non-2xx response becomes a `*drapi.HTTPError{StatusCode, URL, Detail, Body}`, +unpackable via `errors.As`. Roughly 32 call sites across the codebase depend +on this contract holding — treat it as load-bearing when touching +`internal/drapi`. + +`Body` is the raw, uninterpreted response body (different DataRobot APIs +disagree on their error envelope shape); `Detail` is best-effort and +populated by the caller — see `internal/workload/apiclient.LiftDetail` for +the pattern of parsing a FastAPI `{"detail": ...}` envelope into `Detail`. + +**Known gap, don't copy this as the pattern**: `Get()` currently builds a +bare `&HTTPError{StatusCode, URL}` on failure instead of calling +`ErrFromResp()` the way `Post`, `Patch`, and `Delete` do — so a `GET` +failure carries no `Body`/`Detail` today, unlike every other verb. This is +a known asymmetry, not an intentional design choice; a fix is a reasonable +follow-up, but a new caller should not assume `Get()` failures carry the +same diagnostic detail as the others until it's closed. + +## Origin safety + +Before attaching the bearer token to any URL your code did not build +locally — a server-returned pagination cursor, a workload's live `endpoint` +URL — call `drapi.URLMatchesConfiguredBase(rawURL)` first and skip +authorization when it doesn't match. Attaching a CLI credential to an +arbitrary server-supplied URL would leak it to whatever origin that URL +points at. + +Two worked examples: + +- `AssertNextOnSameHost` (`internal/drapi/pagination.go`) guards a paginated + list response's `Next` cursor before the client follows it. +- `endpointCheckAuthForURL` (`internal/workload/up/endpoint_check.go`) + authenticates a post-deploy endpoint check only when the URL is the + DataRobot-hosted workload gateway path *and* matches the configured base; + a direct/customer-container URL always stays anonymous, so the CLI token + never lands in a customer container's access log (RAPTOR-19741). + +## See also + +- [Authentication](authentication.md) — the OAuth-style login flow that + produces the token these conventions consume. +- `go doc ./internal/drapi` — the full package doc. diff --git a/docs/development/structure.md b/docs/development/structure.md index 4f2b3116a..bf5d3cb6b 100644 --- a/docs/development/structure.md +++ b/docs/development/structure.md @@ -15,9 +15,11 @@ cli/ │ ├── self/ # Self-management commands │ ├── start/ # Application startup │ ├── task/ # Task commands -│ └── templates/ # Template management +│ ├── templates/ # Template management +│ └── workload/ # Workload lifecycle commands (feature-gated) ├── internal/ # Private application code │ ├── assets/ # Embedded assets +│ ├── auth/ # Authentication logic (OAuth-style browser flow) │ ├── cli/ # CLI infrastructure (GatedCommand, etc.) │ ├── config/ # Configuration management │ ├── copier/ # Template copying utilities @@ -30,7 +32,8 @@ cli/ │ ├── task/ # Task discovery and execution │ ├── telemetry/ # Anonymous usage analytics (Amplitude) │ ├── tools/ # Tool prerequisites -│ └── version/ # Version information +│ ├── version/ # Version information +│ └── workload/ # Workload manifest, wizard, deploy, and sync engine ├── tui/ # Terminal UI components │ ├── banner.go # Banner display │ ├── interrupt.go # Interrupt handling @@ -63,8 +66,9 @@ Contains all CLI command implementations using the Cobra framework. Each subdire #### Example - `cmd/auth/cmd.go`: The auth command group -- `cmd/auth/login.go`: The login subcommand -- `cmd/auth/logout.go`: The logout subcommand +- `cmd/auth/login/cmd.go`: The login subcommand +- `cmd/auth/logout/cmd.go`: The logout subcommand +- ... ### internal/ @@ -88,7 +92,24 @@ DataRobot API client implementation for: See the package doc (`go doc ./internal/drapi`) for the timeout-override convention and the `HTTPError`/`errors.As` contract new commands should -follow when wiring in a call. +follow when wiring in a call, or [drapi client conventions](drapi-client.md) +for the same content as a dev doc. + +#### auth/ + +OAuth-style browser login flow (`auth.EnsureAuthenticated`, used as a +`PreRunE` gate on any command that needs credentials) plus the +credential-verification and interactive URL-picker logic behind +`dr auth login`/`set-url`. See +[Authentication](authentication.md) for the full flow. + +#### workload/ + +Manifest parsing/compilation, the `dr workload config` setup wizard, the +`dr workload up` deploy orchestrator, and the code-sync engine behind +`dr artifact code sync`. A dedicated architecture guide is in review with +the Workload API team; until it lands, `internal/workload/*/doc.go` (e.g. +`go doc ./internal/workload/up`) is the best available reference. #### envbuilder/ @@ -230,20 +251,28 @@ Configuration is managed through Viper and stored in: - `~/.config/datarobot/drconfig.yaml`: Global configuration and authentication tokens -Access configuration through the `internal/config` package: +Access configuration through `internal/config` and its `viperx` wrapper +(the only supported way to read/write viper state — `spf13/viper` cannot be +imported directly outside `internal/config`): ```go -import "github.com/datarobot/cli/internal/config" +import ( + "github.com/datarobot/cli/internal/config" + "github.com/datarobot/cli/internal/config/viperx" +) // Get configuration values -apiKey := config.GetAPIKey() -endpoint := config.GetEndpoint() +apiKey := viperx.GetString(config.DataRobotAPIKey) +endpoint := config.GetBaseURL() -// Set configuration values -config.SetAPIKey("new-key") -config.SaveConfig() +// Persist a value — only keys listed in config.PersistableKeys are written +viperx.Set(config.DataRobotURL, "https://app.datarobot.com") +config.UpdateConfigFile(config.DataRobotURL) ``` +See [Configuration](configuration.md) for the full contract (persistable +keys, env-var binding, redaction rules). + ## Testing structure Tests are colocated with the code they test: