diff --git a/docs/adr/0002-dependency-failure-tree.md b/docs/adr/0002-dependency-failure-tree.md new file mode 100644 index 00000000..d98cbe64 --- /dev/null +++ b/docs/adr/0002-dependency-failure-tree.md @@ -0,0 +1,72 @@ +# ADR-0002 — Render Project command failures as a dependency tree + +**Date:** 2026-06-14 +**Status:** Accepted + +## Context + +This decision was originally captured as a design spec on 2026-03-13 and later promoted to an ADR. + +When `lets` ran a **Project command** with a `depends` chain and a dependency failed, the error output only named the innermost failing command: + +```text +failed to run command 'lint': exit status 1 +``` + +Users could not see which parent commands triggered that failure or how deep in the **Dependency chain** execution stopped. The same ambiguity existed for single-command failures because the error carried no structured command context. + +The fix needed to work for both serial and parallel command execution paths without changing the child process exit code that `lets` returns. + +## Decision + +Carry command-chain context through executor errors with a `DependencyError` type in `internal/executor`. + +`DependencyError` stores: + +- `Chain []string`: the failing **Dependency chain**, outermost-first, for example `[]string{"deploy", "build", "lint"}`. +- `Err error`: the original execution error. + +It delegates `Error()` and `Unwrap()` to the original error, and its `ExitCode()` preserves the exit code from the innermost `ExecuteError` when one exists. + +Add a `prependToChain(name, err)` helper at command boundaries: + +- If `err` is already a `DependencyError`, return a new `DependencyError` with `name` prepended and the original cause preserved. +- Otherwise, wrap `err` in a single-node `DependencyError`. + +Both `execute()` and `executeParallel()` call `prependToChain` for command-scoped error paths: + +- command initialization (`initCmd`) +- dependency execution (`executeDepends`) +- command script execution (`runCmd`) +- persisted-checksum writes (`persistChecksum`) + +The root **Init script** is intentionally excluded because it is not tied to a **Project command** name. + +Render the chain in the CLI error renderer as a themed `command tree:` section, with the failing leaf annotated: + +```text +command tree: + └─ deploy + └─ build + └─ lint <-- failed here +``` + +For non-terminal output, keep the plain error text path. + +## Consequences + +- **Positive:** Users see the full command context for dependency failures and single-command failures. +- **Positive:** The original error remains unwrap-able, so existing error handling and exit-code propagation continue to work. +- **Positive:** Serial and parallel executor paths share the same command-boundary wrapping rule. +- **Neutral:** Command execution failures now surface as `DependencyError` values at higher layers. +- **Neutral:** Failures inside a parallel `cmd` array are attributed to the owning **Project command**, not to an individual shell fragment. +- **Negative:** The error renderer now depends on executor-specific error structure to show the command tree. + +## Related implementation + +- `internal/executor/dependency_error.go` +- `internal/executor/executor.go` +- `internal/cmd/error.go` +- `internal/executor/dependency_error_test.go` +- `internal/cmd/help_golden_test.go` +- `tests/dependency_failure_tree.bats` diff --git a/docs/adr/0003-remote-configs.md b/docs/adr/0003-remote-configs.md new file mode 100644 index 00000000..a737f622 --- /dev/null +++ b/docs/adr/0003-remote-configs.md @@ -0,0 +1,70 @@ +# ADR-0003 — Support Remote configs with local caching + +**Date:** 2026-06-14 +**Status:** Accepted + +## Context + +This decision was originally captured as a design spec on 2026-06-13 and later promoted to an ADR. It originated from Issue [#351](https://github.com/lets-cli/lets/issues/351). + +`lets` could load a local **Project config** from `lets.yaml`, but users also wanted to run reusable project workflows published at a URL: + +```bash +lets -c https://example.com/lets.yaml build +``` + +A **Remote config** needed different behavior from a local file: + +- avoid re-downloading unchanged configs on every invocation +- let users force a refresh when the remote source changes +- preserve the invocation directory as the **Work dir** for commands +- share HTTP download behavior with **Remote mixins** instead of duplicating it +- fail safely when the network is unavailable + +## Decision + +Treat a root `--config` / `-c` value that starts with `http://` or `https://` as a **Remote config**. + +Add a root `--no-cache` flag. For remote sources, this asks lets to re-download instead of using an existing cached copy. The same no-cache choice is passed through config loading so **Remote mixins** refresh consistently with **Remote configs**. + +Cache downloaded Remote configs at: + +```text +~/.config/lets/remote-configs/.yaml +``` + +The URL hash is the cache identity. Cache writes are atomic: write a sibling temp file, set file mode, then rename into place. + +Remote config loading follows this flow: + +1. Without `--no-cache`, use the cache if present. +2. Otherwise download the URL, write it to the cache, then load the cached file. +3. If the download fails and a cache file exists, warn and fall back to the cached file. +4. If the download fails with no cache file, return an error. + +Load the cached YAML through the same config validation and setup path as local Project configs, but set the config **Work dir** and `.lets` directory from the invocation CWD. Commands from a Remote config therefore run from where the user invoked `lets`, unless a command declares `work_dir`. + +Extract shared HTTP download behavior into `internal/fetch` so Remote configs and Remote mixins use the same implementation. `fetch.Download` accepts only `text/plain`, `text/yaml`, `text/x-yaml`, `application/yaml`, and `application/x-yaml`; reports non-2xx responses; supports progress observers; and is cancellable through context. + +If `LETS_CONFIG_DIR` is set while using a Remote config URL, warn and ignore it because discovery relative to a local config directory does not apply. + +## Consequences + +- **Positive:** Users can publish and consume shared Project configs by URL. +- **Positive:** Cached configs make repeated invocations fast and provide an offline fallback. +- **Positive:** `--no-cache` gives users a direct way to refresh Remote configs and Remote mixins. +- **Positive:** Remote config downloads and Remote mixin downloads share one fetch implementation and progress-reporting path. +- **Neutral:** A cached file that no longer parses produces a normal config error with guidance to retry using `--no-cache`. +- **Neutral:** The invocation CWD, not the cache directory, defines the default Work dir for Remote config commands. +- **Negative:** Remote configs are executable project workflow definitions; users must trust the URL they run. +- **Negative:** The cache is keyed only by URL. Different content at the same URL replaces the prior cached config when refreshed. + +## Related implementation + +- `internal/cli/cli.go` +- `internal/cmd/root.go` +- `internal/config/load.go` +- `internal/config/config/mixin.go` +- `internal/fetch/fetch.go` +- `internal/config/load_test.go` +- `internal/fetch/fetch_test.go` diff --git a/docs/docs/changelog.md b/docs/docs/changelog.md index 3f5d27a7..7f0776eb 100644 --- a/docs/docs/changelog.md +++ b/docs/docs/changelog.md @@ -5,6 +5,7 @@ title: Changelog ## [Unreleased](https://github.com/lets-cli/lets/releases/tag/v0.0.X) +* `[Docs]` Convert design specs into ADRs. * `[Added]` Add `lets self config path` and `lets self config edit` for user settings. Issue [#370](https://github.com/lets-cli/lets/issues/370) * `[Added]` Show interactive download progress for remote configs and remote mixins. Issue [#360](https://github.com/lets-cli/lets/issues/360) * `[Fixed]` Make `--no-cache` re-download remote mixins for local and remote configs. Issue [#365](https://github.com/lets-cli/lets/issues/365) diff --git a/docs/plans/2026-03-14-dependency-failure-tree.md b/docs/plans/2026-03-14-dependency-failure-tree.md index ebadb0b9..b19d04b1 100644 --- a/docs/plans/2026-03-14-dependency-failure-tree.md +++ b/docs/plans/2026-03-14-dependency-failure-tree.md @@ -8,7 +8,7 @@ **Tech Stack:** Go stdlib `errors`, `fmt`, `io`, `strings`; `github.com/fatih/color` v1.16.0 (already in `go.mod`) for red highlight; `testing` package for unit tests; bats-core + bats-assert for integration tests. -**Spec:** `docs/specs/2026-03-13-dependency-failure-tree-design.md` +**ADR:** `docs/adr/0002-dependency-failure-tree.md` --- diff --git a/docs/specs/2026-03-13-dependency-failure-tree-design.md b/docs/specs/2026-03-13-dependency-failure-tree-design.md deleted file mode 100644 index d479a164..00000000 --- a/docs/specs/2026-03-13-dependency-failure-tree-design.md +++ /dev/null @@ -1,184 +0,0 @@ -# Dependency Failure Tree - -**Date:** 2026-03-13 -**Status:** Design approved — ready for implementation - -## Problem - -When `lets` runs a command with a `depends` chain and a dependency fails, the current error output only names the immediate failing command: - -``` -failed to run command 'lint': exit status 1 -``` - -There is no indication of how deep in the dependency chain the failure occurred or which parent commands triggered it. - -## Goal - -Print an indented tree of the full dependency chain whenever a command fails, so the user immediately knows where in the chain execution stopped. This applies to all failures — with or without a `depends` chain. - -``` - deploy - build - lint <-- failed here - -ERRO[0000] failed to run command 'lint': exit status 1 -``` - -## Design - -### Data Model - -A new `DependencyError` type in `internal/executor/dependency_error.go`: - -```go -type DependencyError struct { - Chain []string // outermost-first: ["deploy", "build", "lint"] - Err error // the original ExecuteError -} - -func (e *DependencyError) Error() string { return e.Err.Error() } - -func (e *DependencyError) ExitCode() int { - var exitErr *ExecuteError - if errors.As(e.Err, &exitErr) { - return exitErr.ExitCode() - } - return 1 -} -``` - -A `prependToChain(name string, err error) error` helper: -- If `err` is already a `*DependencyError`: prepend `name` to `Chain` and return the same error. -- Otherwise: return `&DependencyError{Chain: []string{name}, Err: err}`. - -### Chain Construction - -Chain building happens in `execute()` and `executeParallel()`. **All error return paths** in both functions call `prependToChain` before returning. This includes errors from: -- `initCmd` -- `executeDepends` -- `runCmd` (each iteration) -- `persistChecksum` - -**Call stack trace for `deploy → build → lint` (lint fails):** - -1. `Execute(deploy)` → `execute(deploy)` → `executeDepends` calls `Execute(build)` → -2. `Execute(build)` → `execute(build)` → `executeDepends` calls `Execute(lint)` → -3. `Execute(lint)` → `execute(lint)` → `executeDepends` (empty, returns nil) → `runCmd(lint)` → returns `ExecuteError` -4. `execute(lint)` calls `prependToChain("lint", ExecuteError)` → creates `DependencyError{Chain: ["lint"], Err: ExecuteError}` -5. Bubbles up to `execute(build)` via `executeDepends`; calls `prependToChain("build", DependencyError)` → `DependencyError{Chain: ["build", "lint"], Err: ExecuteError}` -6. Bubbles up to `execute(deploy)` via `executeDepends`; calls `prependToChain("deploy", DependencyError)` → `DependencyError{Chain: ["deploy", "build", "lint"], Err: ExecuteError}` - -**Single-command failure (no depends):** `runCmd` fails in `execute`, which calls `prependToChain("lint", ExecuteError)` → `DependencyError{Chain: ["lint"], Err: ExecuteError}`. Renders as a single-node tree. - -**Global `init` script failure:** The `Execute()` method runs `cfg.Init` before dispatching to `execute`/`executeParallel`. If this fails, it returns the raw error — no tree is shown. This is intentional: the global init is not tied to any command name. - -### Rendering - -```go -// PrintDependencyTree writes an indented tree of the dependency chain to w. -// The failing node (last in chain) is annotated in red. -// fatih/color v1.16.0 automatically respects NO_COLOR. -func PrintDependencyTree(e *DependencyError, w io.Writer) -``` - -- 2 spaces of indentation per depth level (level 0 = 2 spaces, level 1 = 4 spaces, …). -- The last node gets ` <-- failed here` in red via `fatih/color`. `fatih/color` v1.16.0 automatically respects `NO_COLOR`. - -Example — 3-level chain: - -``` - deploy - build - lint <-- failed here -``` - -Example — single node: - -``` - lint <-- failed here -``` - -### Error Handler in main.go - -Tree is printed **before** `log.Error` so context appears above the error line: - -```go -if err := rootCmd.ExecuteContext(ctx); err != nil { - var depErr *executor.DependencyError - if errors.As(err, &depErr) { - executor.PrintDependencyTree(depErr, os.Stderr) - } - log.Error(err.Error()) - os.Exit(getExitCode(err, 1)) -} -``` - -`DependencyError` implements `ExitCode()` so `getExitCode` propagates the correct exit code from the innermost failing process. All execution errors — from both `execute` and `executeParallel` — bubble up through this single handler. - -Complete example of final stderr output for the 3-level chain: - -``` - deploy - build - lint <-- failed here -ERRO[0000] failed to run command 'lint': exit status 1 -``` - -### Files Changed - -| File | Change | -|------|--------| -| `internal/executor/dependency_error.go` | New: `DependencyError` type, `prependToChain` helper, `PrintDependencyTree` | -| `internal/executor/executor.go` | Call `prependToChain` on all error return paths in `execute()` and `executeParallel()` | -| `cmd/lets/main.go` | Detect `*DependencyError` and call `PrintDependencyTree` before `log.Error` | - -### Out of Scope - -- Global `init` script failures (no command name to display). -- Parallel `cmd` arrays within a single command (not `depends`). -- No changes to debug logging or `ExecLogger`. -- No new CLI flags. - -## Testing - -### Unit Tests - -In `internal/executor/dependency_error_test.go`: - -- `prependToChain` with non-`DependencyError` input → creates single-node `DependencyError`. -- `prependToChain` with existing `DependencyError` → prepends correctly. -- `DependencyError.ExitCode()` propagates exit code from inner `ExecuteError`. -- `DependencyError.ExitCode()` returns 1 when inner error has no exit code. -- `PrintDependencyTree` output matches expected indentation for 1, 2, 3-node chains. - -### Bats Integration Test - -`tests/dependency_failure_tree.bats` with fixture `tests/dependency_failure_tree/lets.yaml`: - -```yaml -shell: bash -commands: - deploy: - depends: [build] - cmd: echo done - build: - depends: [lint] - cmd: echo done - lint: - cmd: exit 1 -``` - -Tests run with `NO_COLOR=1` to avoid ANSI escape codes in assertions: - -```bash -@test "dependency failure tree shows full chain" { - cd "$BATS_TEST_DIRNAME/dependency_failure_tree" - run env NO_COLOR=1 lets deploy - assert_failure - assert_line --index 0 " deploy" - assert_line --index 1 " build" - assert_line --index 2 --partial " lint" - assert_line --index 2 --partial "failed here" -} -``` diff --git a/docs/specs/2026-06-13-remote-config-design.md b/docs/specs/2026-06-13-remote-config-design.md deleted file mode 100644 index 7465b168..00000000 --- a/docs/specs/2026-06-13-remote-config-design.md +++ /dev/null @@ -1,87 +0,0 @@ ---- -name: remote-config-design -description: Design for lets -c https://... remote config fetching with caching (issue #351) ---- - -# Remote Config Design - -**Issue:** [#351](https://github.com/lets-cli/lets/issues/351) - -## Summary - -Allow `lets -c https://url` to download a remote `lets.yaml`, cache it locally, and run commands from it with the invocation directory as the working directory. Add `--no-cache` flag to force re-download. - -## Architecture - -### New flag: `--no-cache` - -Added to `initRootFlags` in `internal/cmd/root.go` and parsed in `parseRootFlags` in `internal/cli/cli.go`, following the same pattern as `--debug` and `--init`. - -### URL detection in `cli.go` - -After parsing root flags, if `rootFlags.config` starts with `http://` or `https://`, call `config.LoadRemote(url, noCache, version)` instead of `config.Load(...)`. The existing `Load` path is untouched. - -### New `internal/fetch` package - -Extract HTTP download + content-type validation from `RemoteMixin.download()` into `internal/fetch/fetch.go`: - -```go -func Download(ctx context.Context, url string) ([]byte, error) -``` - -Same 15-minute timeout and content-type whitelist (`text/plain`, `text/yaml`, `text/x-yaml`, `application/yaml`, `application/x-yaml`). `RemoteMixin` becomes a thin wrapper calling `fetch.Download`. This eliminates duplication. - -### New `LoadRemote` in `internal/config/load.go` - -```go -func LoadRemote(url string, noCache bool, version string) (*config.Config, error) -``` - -- Cache path: `util.LetsUserDir()` + `remote-configs/.yaml` - - Resolves to `~/.config/lets/remote-configs/.yaml` -- Creates `~/.config/lets/remote-configs/` if needed -- Working directory: `os.Getwd()` (CWD at invocation time), passed as `configDir` to `Load` - -## Data Flow - -### Normal flow (no `--no-cache`) - -1. Cache file exists → load directly from cached path, skip HTTP -2. Cache missing → download via `fetch.Download` → persist → load from cached path - -### `--no-cache` flow - -1. Attempt download → persist (overwriting cache) → load -2. Download fails + cache exists → `log.Warnf("failed to refresh remote config, using cached version: %s", err)` → load from cache -3. Download fails + no cache → return error - -### Working directory - -`LoadRemote` calls `Load(cachedPath, cwd, version)` where `cwd = os.Getwd()`. Commands in the remote config execute in the invocation directory unless they specify `work_dir`. - -## Files Changed - -| File | Change | -|------|--------| -| `internal/fetch/fetch.go` | New — extracted `Download` func | -| `internal/fetch/fetch_test.go` | New — unit tests for fetch | -| `internal/config/config/mixin.go` | Refactor `download()` to use `fetch.Download` | -| `internal/config/load.go` | Add `LoadRemote` | -| `internal/config/load_test.go` | Add `LoadRemote` tests | -| `internal/cli/cli.go` | URL detection, call `LoadRemote`, thread `noCache` | -| `internal/cmd/root.go` | Add `--no-cache` flag | - -## Testing - -### `internal/fetch/fetch_test.go` -- Valid YAML content-type → success -- Unsupported content-type → error -- 404 → error -- Non-2xx → error - -### `internal/config/load_test.go` -- `LoadRemote` with `httptest.NewServer` serving valid YAML → loads, caches, runs from CWD -- Cache hit: serve once, call twice, assert server receives only one request -- `--no-cache` refresh: serve two different configs, assert second call gets new content -- `--no-cache` + server down + cache exists: assert warning logged + falls back to cache -- `--no-cache` + server down + no cache: assert error returned