diff --git a/cmd/modelith/main.go b/cmd/modelith/main.go index 43ee877..d6ea0f1 100644 --- a/cmd/modelith/main.go +++ b/cmd/modelith/main.go @@ -3,15 +3,18 @@ package main import ( + "context" "encoding/json" "errors" "fmt" "io" "io/fs" "os" + "os/signal" "path/filepath" "runtime/debug" "strings" + "syscall" "time" "github.com/spf13/cobra" @@ -71,10 +74,18 @@ func buildVersion() string { var errBlocking = errors.New("blocking findings") func main() { - if err := rootCmd().Execute(); err != nil { - if !errors.Is(err, errBlocking) { - fmt.Fprintln(os.Stderr, "error:", err) + ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + defer stop() + + if err := rootCmd().ExecuteContext(ctx); err != nil { + if errors.Is(err, errBlocking) { + os.Exit(1) + } + if ctx.Err() != nil { + fmt.Fprintln(os.Stderr, "interrupted") + os.Exit(130) } + fmt.Fprintln(os.Stderr, "error:", err) os.Exit(1) } } @@ -86,6 +97,9 @@ func rootCmd() *cobra.Command { SilenceUsage: true, SilenceErrors: true, Version: buildVersion(), + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + return cmd.Context().Err() + }, } root.AddCommand(lintCmd(), renderCmd(), schemaCmd(), depsCmd()) return root @@ -109,25 +123,35 @@ that use the network; lint and render never do.`), } func depsImportCmd() *cobra.Command { - var ref string + var ( + ref string + timeout time.Duration + ) cmd := &cobra.Command{ Use: "import [dir]", Short: "Vendor a model from another repository", Long: strings.TrimSpace(` Vendor a model from another repository into this one. - is the address of the file as it appears in a browser on github.com. The -copy is written into [dir] (the working directory by default) with a provenance -header recording where it came from, and is verified against that header by -every later lint. + is the address of the file as it appears in a browser. Both github.com +and dev.azure.com are supported. The copy is written into [dir] (the working +directory by default) with a provenance header recording where it came from, +and is verified against that header by every later lint. + +For GitHub, fetching is delegated to the gh CLI, which must be installed and +authenticated. For Azure DevOps, fetching is delegated to the az CLI; run +'az login' first. + +Each fetch is bounded by --timeout (default 60s, 0 disables the bound): a hung +CLI fails fast instead of stalling the import. -Fetching is delegated to the gh CLI, which must be installed and authenticated. The imports list of the model that will reference this copy is yours to edit; this command prints the entry to add.`), Example: strings.TrimSpace(` modelith deps import https://github.com/acme/billing/blob/main/docs/payments.modelith.yaml modelith deps import https://github.com/acme/billing/blob/main/docs/payments.modelith.yaml docs/ - modelith deps import --ref v2.1.0 https://github.com/acme/billing/blob/main/docs/payments.modelith.yaml`), + modelith deps import --ref v2.1.0 https://github.com/acme/billing/blob/main/docs/payments.modelith.yaml + modelith deps import "https://dev.azure.com/myorg/myproject/_git/myrepo?path=docs/payments.modelith.yaml&version=GBmain"`), Args: cobra.RangeArgs(1, 2), RunE: func(cmd *cobra.Command, args []string) error { dir := "." @@ -143,10 +167,11 @@ this command prints the entry to add.`), } res, err := deps.Import(cmd.Context(), deps.Options{ - URL: args[0], - Dir: dir, - Ref: ref, - Now: time.Now(), + URL: args[0], + Dir: dir, + Ref: ref, + Now: time.Now(), + Timeout: timeout, }) if err != nil { return err @@ -161,6 +186,8 @@ this command prints the entry to add.`), }, } cmd.Flags().StringVar(&ref, "ref", "", "ref to fetch, overriding the one in the URL (a tag pins the copy)") + cmd.Flags().DurationVar(&timeout, "timeout", 60*time.Second, + "abandon a delegated fetch (gh/az) that exceeds this duration; 0 disables the bound") return cmd } @@ -218,6 +245,7 @@ func printTrustWarning(errOut io.Writer) { } func depsCheckCmd() *cobra.Command { + var timeout time.Duration cmd := &cobra.Command{ Use: "check ...", Short: "Report which vendored copies have fallen behind their origin", @@ -236,14 +264,18 @@ A copy pinned to a tag is reported as up to date for as long as that tag points where it did; modelith does not look for newer releases. Every line names the ref it was checked against. -Fetching is delegated to the gh CLI, which must be installed and authenticated. -Nothing is written.`), +Fetching is delegated to the gh CLI for github.com origins and the az CLI for +dev.azure.com ones, each of which must be installed and authenticated. Nothing +is written. + +Each fetch is bounded by --timeout (default 60s, 0 disables the bound): a hung +CLI fails fast instead of stalling the run.`), Example: strings.TrimSpace(` modelith deps check docs/payments.modelith.yaml modelith deps check docs/*.modelith.yaml`), Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - reports, err := deps.Check(cmd.Context(), deps.CheckOptions{Paths: args}) + reports, err := deps.Check(cmd.Context(), deps.CheckOptions{Paths: args, Timeout: timeout}) // A run that got nowhere has nothing to summarise, and "checked 0 // vendored copies" above the reason would read as the outcome. blocking := len(reports) > 0 && printCheckReports(cmd.OutOrStdout(), cmd.ErrOrStderr(), reports) @@ -256,11 +288,16 @@ Nothing is written.`), return nil }, } + cmd.Flags().DurationVar(&timeout, "timeout", 60*time.Second, + "abandon a delegated fetch (gh/az) that exceeds this duration; 0 disables the bound") return cmd } func depsUpdateCmd() *cobra.Command { - var ref string + var ( + ref string + timeout time.Duration + ) cmd := &cobra.Command{ Use: "update ...", Short: "Bring vendored copies forward to what their origins serve", @@ -281,7 +318,11 @@ imports list, and it does not lint: an item a copy used to define may have been renamed or removed upstream, and modelith lint is what reports that from the importing model's seat. -Fetching is delegated to the gh CLI, which must be installed and authenticated.`), +Fetching is delegated to the gh CLI for github.com origins and the az CLI for +dev.azure.com ones, each of which must be installed and authenticated. + +Each fetch is bounded by --timeout (default 60s, 0 disables the bound): a hung +CLI fails fast instead of stalling the run.`), Example: strings.TrimSpace(` modelith deps update docs/payments.modelith.yaml modelith deps update docs/*.modelith.yaml @@ -289,9 +330,10 @@ Fetching is delegated to the gh CLI, which must be installed and authenticated.` Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { reports, err := deps.Update(cmd.Context(), deps.UpdateOptions{ - Paths: args, - Ref: ref, - Now: time.Now(), + Paths: args, + Ref: ref, + Now: time.Now(), + Timeout: timeout, }) // See depsCheckCmd: nothing reached means nothing to summarise. blocking := len(reports) > 0 && printUpdateReports(cmd.OutOrStdout(), cmd.ErrOrStderr(), reports) @@ -305,6 +347,8 @@ Fetching is delegated to the gh CLI, which must be installed and authenticated.` }, } cmd.Flags().StringVar(&ref, "ref", "", "re-pin the copy to this ref (one file only)") + cmd.Flags().DurationVar(&timeout, "timeout", 60*time.Second, + "abandon a delegated fetch (gh/az) that exceeds this duration; 0 disables the bound") return cmd } diff --git a/cmd/modelith/main_test.go b/cmd/modelith/main_test.go index 67ff3ba..5c9b987 100644 --- a/cmd/modelith/main_test.go +++ b/cmd/modelith/main_test.go @@ -2,12 +2,14 @@ package main import ( "bytes" + "context" "encoding/json" "errors" "os" "path/filepath" "strings" "testing" + "time" "github.com/stacklok/modelith/internal/deps" "github.com/stacklok/modelith/internal/lint" @@ -23,7 +25,7 @@ func run(t *testing.T, args ...string) (string, error) { root.SetOut(&buf) root.SetErr(&buf) root.SetArgs(args) - err := root.Execute() + err := root.ExecuteContext(context.Background()) return buf.String(), err } @@ -840,3 +842,59 @@ func TestSchemaOutputsValidJSON(t *testing.T) { t.Fatalf("schema output is not valid JSON: %v", err) } } + +// TestDepsImportTimeoutFlagParses pins that --timeout is accepted with a +// duration and with 0 (the explicit opt-out), and that its default is 60s. +// The import fails on the unsupported host before any fetch, so no gh/az runs +// and the test needs no network. +func TestDepsImportTimeoutFlagParses(t *testing.T) { + dir := t.TempDir() + cmd := depsImportCmd() + if d, err := cmd.Flags().GetDuration("timeout"); err != nil || d != 60*time.Second { + t.Fatalf("default --timeout = %v (%v), want 60s", d, err) + } + + for _, tc := range []struct{ name, val string }{ + {"a duration", "5s"}, + {"an explicit opt-out", "0"}, + } { + t.Run(tc.name, func(t *testing.T) { + want, _ := time.ParseDuration(tc.val) + cmd := depsImportCmd() + var buf bytes.Buffer + cmd.SetOut(&buf) + cmd.SetErr(&buf) + cmd.SetArgs([]string{"--timeout", tc.val, + "https://gitlab.com/acme/billing/-/blob/main/m.modelith.yaml", dir}) + err := cmd.ExecuteContext(context.Background()) + if err == nil { + t.Fatal("expected the unsupported-host error, got nil") + } + if strings.Contains(err.Error(), "unknown flag") { + t.Fatalf("--timeout %s was rejected: %v", tc.val, err) + } + if !strings.Contains(err.Error(), "github.com/stacklok/modelith/issues") { + t.Fatalf("want the unsupported-host error, got: %v", err) + } + if got, _ := cmd.Flags().GetDuration("timeout"); got != want { + t.Errorf("--timeout parsed as %v, want %v", got, want) + } + }) + } +} + +func TestExecuteContextInterrupted(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + root := rootCmd() + var buf bytes.Buffer + root.SetOut(&buf) + root.SetErr(&buf) + root.SetArgs([]string{"schema"}) + + err := root.ExecuteContext(ctx) + if err == nil { + t.Fatal("expected error when executing with canceled context, got nil") + } +} diff --git a/docs/07-cli.md b/docs/07-cli.md index 8f220f7..c5996fc 100644 --- a/docs/07-cli.md +++ b/docs/07-cli.md @@ -99,34 +99,42 @@ semantics, and refresh behavior. ### `modelith deps import [dir]` -Fetches a GitHub model and writes a vendored copy to `dir`, or the working -directory when omitted. The filename comes from the origin. It requires an -installed, authenticated [`gh`](https://cli.github.com) CLI and prints the -`imports:` entry to add; it does not edit your model. +Fetches a GitHub or Azure DevOps model and writes a vendored copy to `dir`, or +the working directory when omitted. The filename comes from the origin. GitHub +imports require an installed, authenticated [`gh`](https://cli.github.com) CLI. +Azure DevOps imports require the [Azure CLI](https://learn.microsoft.com/en-us/cli/azure/) +and an `az login` session. The command prints the `imports:` entry to add; it +does not edit your model. | Argument / flag | Meaning | |---|---| -| `` | The address of the file as it appears in a browser on github.com. | +| `` | The browser URL of a file on github.com or dev.azure.com. | | `[dir]` | Destination directory. | | `--ref` | Ref to fetch, overriding the ref in the URL. A tag pins the copy. | +| `--timeout` | Maximum duration for each delegated `gh` or `az` fetch. Defaults to `60s`; `0` disables the limit. | -When a branch or tag contains `/`, pass `--ref` only when it names that same ref in -the URL: it tells modelith where the ref ends and the file path begins. For -example, use `--ref release/v2` with a URL containing -`/blob/release/v2/docs/payments.modelith.yaml`. For an ordinary single-segment -ref in the URL, a different `--ref` works. But `--ref` cannot both select a -different ref and disambiguate a URL whose ref itself contains `/`; in that -ambiguous case, copy the browser URL for the file at the target ref. +GitHub browser URLs can be ambiguous when a branch or tag contains `/`. Pass +`--ref` only when it names that same ref in the URL: it tells modelith where the +ref ends and the file path begins. For example, use `--ref release/v2` with a +URL containing `/blob/release/v2/docs/payments.modelith.yaml`. For an ordinary +single-segment ref in the URL, a different `--ref` works. But `--ref` cannot +both select a different ref and disambiguate a URL whose ref contains `/`; in +that case, copy the browser URL for the file at the target ref. ```sh modelith deps import https://github.com/acme/billing/blob/main/docs/payments.modelith.yaml docs/ +modelith deps import "https://dev.azure.com/acme/billing/_git/models?path=docs/payments.modelith.yaml&version=GBmain" docs/ ``` ### `modelith deps check ...` Checks vendored copies against their origins and exits non-zero when a copy is stale or cannot be reached. It writes nothing and skips files without provenance -headers. +headers. Copies from github.com and dev.azure.com are both checked. + +| Flag | Default | Description | +|---|---|---| +| `--timeout` | `60s` | Maximum duration for each delegated `gh` or `az` fetch; `0` disables the limit. | ```sh modelith deps check docs/*.modelith.yaml @@ -134,9 +142,14 @@ modelith deps check docs/*.modelith.yaml ### `modelith deps update [--ref ] ...` -Updates vendored copies from their origins. `--ref` re-pins one copy to a tag or -branch; it accepts exactly one file. The command does not edit `imports:` or -lint the result. +Updates vendored copies from their origins, for copies from github.com and +dev.azure.com alike. `--ref` re-pins one copy to a tag or branch; it accepts +exactly one file. The command does not edit `imports:` or lint the result. + +| Flag | Default | Description | +|---|---|---| +| `--ref` | header's ref | Re-pin one copy to this ref (a tag or branch). One file only. | +| `--timeout` | `60s` | Maximum duration for each delegated `gh` or `az` fetch; `0` disables the limit. | ```sh modelith deps update docs/*.modelith.yaml diff --git a/docs/10-vendoring.md b/docs/10-vendoring.md index 531fa74..239a514 100644 --- a/docs/10-vendoring.md +++ b/docs/10-vendoring.md @@ -11,12 +11,20 @@ another one defines — but only across files that are already in your repository. **Vendoring** is how a model from *somewhere else* gets there: you fetch a copy, commit it, and modelith records where it came from. +Authenticate with the CLI for the host before your first import: + ```sh -modelith deps import https://github.com/acme/billing/blob/main/docs/payments.modelith.yaml docs/ +gh auth login # GitHub +az login # Azure DevOps ``` -That URL is the address of the file as it appears in your browser on -github.com — open the model on GitHub and copy the address bar. +Then copy the model's browser URL and import it. GitHub and Azure DevOps use +slightly different URL shapes: + +```sh +modelith deps import https://github.com/acme/billing/blob/main/docs/payments.modelith.yaml docs/ +modelith deps import "https://dev.azure.com/acme/billing/_git/models?path=docs/payments.modelith.yaml&version=GBmain" docs/ +``` ## What you get @@ -43,6 +51,7 @@ never has one, and never meets any of this. | `vendored` | That this file is a copy. Nothing enforces it; it is there so a person or an agent about to edit the file stops. | | `fetch` | How to get it again. `git` today. | | `origin`, `path`, `ref` | Where it came from and what to track. A tag in `ref` pins the copy; a branch follows it. | +| `ref-type` | Azure DevOps only: the kind of ref `ref` names — `branch`, `tag`, or `commit`. Omitted for GitHub, whose API resolves an untyped ref, and when the type is not yet known, in which case a refresh resolves it. | | `commit` | The commit that last touched *this file* at that ref — so it does not move when unrelated commits land. | | `imported` | When you fetched it. | | `digest` | SHA-256 of the file with the header lines removed, so stamping the header does not change it. | @@ -195,6 +204,21 @@ that matched none of your copies does not read as good news. To find them: git grep -l '# modelith-vendored' ``` +:::note[Azure DevOps copies record how pinned they are] + +An Azure DevOps URL carries a version *type* (`GB` branch, `GT` tag, `GC` +commit), which a branch and a tag sharing a name would answer differently. The +header records it as `modelith-ref-type`, so a later `deps check` or `deps +update` asks for the same typed version the import did. There is no "let the API +decide": Azure DevOps reads an untyped request as a *branch*, so a copy pinned +to a tag or a commit would 404. When the type is not named — a bare `version=`, +a `--ref` override, or a header written before the key existed — modelith +resolves it before fetching: a git object id is a commit, and anything else is +looked up among the repository's branches and then its tags. GitHub has no such +key: its API resolves an untyped ref on its own. + +::: + ### Two ways to track a model Which one you are on is whatever `# modelith-ref:` records. @@ -271,15 +295,15 @@ already solves. ## Requirements and limits -- **`gh` must be installed and authenticated.** modelith implements no network - transport of its own; it delegates to the [GitHub - CLI](https://cli.github.com), which already solves authentication for private - and internal repositories. -- **GitHub only, for now.** A URL on another host is an error that asks you to - [open an issue](https://github.com/stacklok/modelith/issues). That is not a - brush-off: the header records *how* it was fetched, so adding another - transport is straightforward — what is missing is a real user to build it - for, and an issue is how you become one. +- **Install the CLI for the host you import from.** For GitHub, install and + authenticate the [GitHub CLI](https://cli.github.com). For Azure DevOps, + install the [Azure CLI](https://learn.microsoft.com/en-us/cli/azure/) and run + `az login`. modelith delegates authentication to these tools. +- **Import and refresh from github.com or dev.azure.com.** An Azure DevOps URL + has the form + `https://dev.azure.com///_git/?path=&version=GB`. + To request another host, [open an + issue](https://github.com/stacklok/modelith/issues). - **`lint` and `render` never touch the network**, whatever you pass them ([ADR-0011](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0011-network-boundary.md)). Everything under `modelith deps` is opt-in, and nothing else fetches. @@ -291,5 +315,6 @@ already solves. The design and its trade-offs are [ADR-0010](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0010-cross-model-references-by-vendoring.md), [ADR-0015](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0015-vendoring-is-a-whole-file-copy.md), +[ADR-0016](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0016-staleness-is-a-content-comparison.md), and -[ADR-0016](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0016-staleness-is-a-content-comparison.md). +[ADR-0019](https://github.com/stacklok/modelith/blob/main/project-docs/adr/0019-azure-devops-as-a-second-transport.md). diff --git a/internal/deps/deps.go b/internal/deps/deps.go index a1ca8c4..36b94f1 100644 --- a/internal/deps/deps.go +++ b/internal/deps/deps.go @@ -1,8 +1,8 @@ // Package deps acquires a model from another repository and stamps it as a // vendored copy. // -// It implements no network transport of its own. Every fetch is delegated to -// the gh CLI, executed as an argv array and never through a shell, so this +// Every fetch is delegated to an external CLI — gh for GitHub, az for Azure +// DevOps — executed as an argv array and never through a shell, so this // binary holds no TLS configuration and no credentials (ADR-0011). package deps @@ -23,6 +23,14 @@ import ( "github.com/stacklok/modelith/internal/provenance" ) +// Host identifies the source-code platform a Source was parsed from. +type Host string + +const ( + HostGitHub Host = "github" + HostADO Host = "azure-devops" +) + // Runner runs an external command and returns its standard output. It is the // seam the gh calls go through, so Import is testable without a network. type Runner interface { @@ -32,26 +40,15 @@ type Runner interface { // ExecRunner runs commands with os/exec. type ExecRunner struct{} -// Run executes name with args and returns its standard output. Standard error -// is folded into the returned error, because gh reports why it refused there. -func (ExecRunner) Run(ctx context.Context, name string, args ...string) ([]byte, error) { - // nolint:gosec // G204 flags a variable command and arguments, which is - // what a transport seam is. The command is the literal "gh" at both call - // sites; the arguments are literals plus an endpoint assembled from a URL - // that ParseSource has already validated, with each path segment and query - // value escaped and traversal segments rejected outright (escaping a - // segment cannot neutralise a segment that *is* a traversal, so ParseSource - // refuses those rather than passing them on). Nothing here comes from a - // model file, and there is no - // shell: exec passes an argv array, so a metacharacter is a byte in an - // argument rather than syntax (ADR-0010). - cmd := exec.CommandContext(ctx, name, args...) - var stderr strings.Builder - cmd.Stderr = &stderr +// runCommand executes cmd and folds its outcome into an error, because the +// CLI reports why it refused on standard error. It is the shared tail of the +// platform-specific Run methods in exec_unix.go and exec_windows.go, so the +// error text cannot drift between platforms. +func runCommand(cmd *exec.Cmd, name string, args []string, stderr *strings.Builder) ([]byte, error) { out, err := cmd.Output() if err != nil { if errors.Is(err, exec.ErrNotFound) { - return nil, unusable{fmt.Errorf("%s is not installed — modelith delegates fetching to it; install it from https://cli.github.com and run `%s auth login`", name, name)} + return nil, unusable{fmt.Errorf("%s is not installed — modelith delegates fetching to it%s", name, installHint(name))} } if msg := strings.TrimSpace(stderr.String()); msg != "" { failed := fmt.Errorf("%s %s: %s", name, strings.Join(args, " "), msg) @@ -65,18 +62,67 @@ func (ExecRunner) Run(ctx context.Context, name string, args ...string) ([]byte, return out, nil } -// unauthenticated reports whether gh refused for want of credentials rather -// than because of anything about the request. +// installHint returns the rest of the "not installed" message for a given CLI. +func installHint(name string) string { + switch name { + case "gh": + return "; install it from https://cli.github.com and run `gh auth login`" + case "az": + return "; install it from https://aka.ms/azure-cli and run `az login`" + } + return "" +} + +// timeoutRunner bounds each delegated command to timeout. It is a decorator on +// the Runner seam: the deadline is enforced per command, so a slow-but-working +// content fetch does not consume the commit fetch's budget. +type timeoutRunner struct { + inner Runner + timeout time.Duration +} + +// Run derives a per-command deadline from the caller's context and abandons the +// command when it expires. The error names the command and the bound and tells +// the user how to adjust it; it deliberately does not echo the argv, so a URI +// cannot leak into logs through this path. +func (r timeoutRunner) Run(ctx context.Context, name string, args ...string) ([]byte, error) { + ctx, cancel := context.WithTimeout(ctx, r.timeout) + defer cancel() + out, err := r.inner.Run(ctx, name, args...) + // ErrWaitDelay is the deadline's last line of defense: the direct child was + // killed, but a helper it spawned held the pipe past WaitDelay, so Wait + // abandoned it. It is the deadline surfacing, so report it as one. + // + // The deadline's main line is ctx.Err(), asked for a *deadline* specifically: + // when the bound fires, ExecRunner SIGKILLs the process group and cmd.Output + // returns *exec.ExitError ("signal: killed") — os/exec prefers the process's + // own error over the context's, so errors.Is cannot see the deadline through + // it (issue #3). The context still distinguishes a fired deadline from a + // caller canceling the fetch (e.g. Ctrl+C); only the former is a timeout the + // user can raise with --timeout. And err must be non-nil: a command that + // completed successfully just before the bound must not have its output + // discarded because the deadline ticked over in the same instant. + if err != nil && (errors.Is(ctx.Err(), context.DeadlineExceeded) || errors.Is(err, exec.ErrWaitDelay)) { + return nil, fmt.Errorf("%s did not finish within %s — the fetch was abandoned (raise it with --timeout if this is a slow but legitimate fetch)", name, r.timeout) + } + return out, err +} + +// unauthenticated reports whether a delegated CLI refused for want of +// credentials rather than because of anything about the request. // -// It matches gh's own text because gh is a separate program: it reports this on -// stderr and exits 1, with no typed error to unwrap and no distinguishing exit -// code. The three needles are the ones gh's binary actually carries — "gh auth -// login" appears in every logged-out message, "GH_TOKEN" in the two automation -// ones, and HTTP 401 is a token that exists and is refused. Reading it wrong -// costs a batch that stops when it could have continued, or one that repeats -// the same paragraph per file. +// It matches the CLI's own text because each is a separate program: it reports +// this on stderr and exits non-zero, with no typed error to unwrap and no +// distinguishing exit code. For gh, "gh auth login" appears in every logged-out +// message and "GH_TOKEN" in the two automation ones; for az, a missing or +// expired session prints "az login" (e.g. "Please run 'az login'…") and the +// AADSTS diagnostics carry an "AADSTS…" code. HTTP 401 is a token that exists +// and is refused, whichever CLI sent it. Reading it wrong costs a batch that +// stops when it could have continued, or one that repeats the same paragraph +// per file — and an unclassified az would do the latter, since a batch keys on +// ErrToolUnavailable to decide whether to stop. func unauthenticated(msg string) bool { - for _, needle := range []string{"gh auth login", "GH_TOKEN", "HTTP 401"} { + for _, needle := range []string{"gh auth login", "GH_TOKEN", "az login", "AADSTS", "HTTP 401"} { if strings.Contains(msg, needle) { return true } @@ -94,27 +140,25 @@ func (unusable) Is(target error) bool { return target == ErrToolUnavailable } // Source is a model file in another repository, as an origin URL parsed into // the parts a fetch and a later refresh both need. type Source struct { - Origin string // https://github.com/owner/repo - Owner string - Repo string - Ref string - Path string // path within the repository + Host Host // HostGitHub or HostADO + Origin string // https://github.com/owner/repo or https://dev.azure.com/org/project/_git/repo + Owner string // GitHub owner, or ADO organization + Project string // ADO project; empty for GitHub + Repo string + Ref string + RefType string // ADO version type: "branch", "tag", or "commit"; empty for GitHub + Path string // path within the repository } -// ParseSource reads a GitHub blob URL — the address of the file as it appears -// in a browser — into its parts. A non-empty ref overrides the one in the URL, -// and when it is the ref the URL names, it is also what disambiguates a branch -// whose name contains a slash from the path that follows it. It cannot do both -// at once: overriding with a *different* ref leaves the split to be taken at the -// first segment, which splitHint explains when the fetch that follows fails. +// ParseSource reads a GitHub or Azure DevOps blob URL — the address of the file +// as it appears in a browser — into its parts. A non-empty ref overrides the +// one in the URL. func ParseSource(raw, ref string) (Source, error) { u, err := url.Parse(strings.TrimSpace(raw)) if err != nil { return Source{}, fmt.Errorf("%q is not a URL: %w", raw, err) } - // A host is case-insensitive, and a browser hands back the "www." form as - // readily as the bare one; neither is a different site. - host := strings.TrimPrefix(strings.ToLower(u.Host), "www.") + host := normalizeHost(u.Host) // The Raw button hands back this host, so it is an easy thing to paste. The // model is on GitHub and the address just names a different view of it, so // sending the reader off to ask for another host to be supported would be @@ -124,41 +168,48 @@ func ParseSource(raw, ref string) (Source, error) { "%q is a raw file URL, and modelith wants the page you see when you open the file on github.com — the same address with /blob/ in it. Open the file there and copy the address bar", raw) } - if host != "github.com" { + + switch { + case host == "github.com": + return parseGitHubSource(u, ref) + case host == "dev.azure.com": + return parseADOSource(u, ref) + case strings.HasSuffix(host, ".visualstudio.com"): + org := strings.TrimSuffix(host, ".visualstudio.com") + return Source{}, fmt.Errorf( + "%q is a legacy visualstudio.com URL — Azure DevOps moved to dev.azure.com. Open the file in your browser on dev.azure.com (e.g. https://dev.azure.com/%s) and import that address instead", + raw, org) + default: return Source{}, fmt.Errorf( - "modelith can currently fetch only from github.com, and %q is on %q. Support for other hosts is not written yet because nobody has needed it — if you do, please open an issue at %s saying where your models live", + "modelith can currently fetch only from github.com and dev.azure.com, and %q is on %q. Support for other hosts is not written yet because nobody has needed it — if you do, please open an issue at %s saying where your models live", raw, u.Host, issuesURL) } - // A URL copied from the browser often carries ?plain=1 and a #L12 anchor. - // Neither is part of the file's address. +} + +// parseGitHubSource parses a GitHub blob URL: +// +// https://github.com///blob// +func parseGitHubSource(u *url.URL, ref string) (Source, error) { parts := strings.Split(strings.Trim(u.Path, "/"), "/") if len(parts) < 5 || parts[2] != "blob" { return Source{}, fmt.Errorf( "%q is not a GitHub file URL — it should look like https://github.com///blob//, which is the address you get by opening the file on github.com", - raw) + u.String()) } - // A dot segment or an empty one is not part of any address github.com - // serves, and url.Parse does not remove one. It has to be rejected here - // rather than escaped later: escapePath escapes the characters within a - // segment, which leaves a segment that *is* a traversal exactly as it was, - // and the endpoint fetchContent builds would then leave the repository's - // contents namespace. for _, p := range parts { if p == "" || p == "." || p == ".." { return Source{}, fmt.Errorf( "%q has a %q path segment, which is not part of a file's address on github.com — copy the address bar from the file's page rather than assembling the URL by hand", - raw, p) + u.String(), p) } } src := Source{ + Host: HostGitHub, Owner: parts[0], Repo: parts[1], Origin: "https://github.com/" + parts[0] + "/" + parts[1], } rest := strings.Join(parts[3:], "/") - // A ref may contain slashes ("release/v2"), and the URL gives no way to tell - // where it ends. Splitting at the first segment is right for the common - // case; an explicit ref settles the rest. switch { case ref != "" && strings.HasPrefix(rest, ref+"/"): src.Ref, src.Path = ref, strings.TrimPrefix(rest, ref+"/") @@ -169,7 +220,76 @@ func ParseSource(raw, ref string) (Source, error) { } } if src.Path == "" { - return Source{}, fmt.Errorf("%q names no file inside the repository", raw) + return Source{}, fmt.Errorf("%q names no file inside the repository", u.String()) + } + return src, nil +} + +// parseADOSource parses an Azure DevOps blob URL: +// +// https://dev.azure.com///_git/?path=&version=GB +// +// The version parameter prefix indicates: GB=GitBranch, GT=GitTag, GC=GitCommit. +func parseADOSource(u *url.URL, ref string) (Source, error) { + // Path: ///_git/ + parts := strings.Split(strings.Trim(u.Path, "/"), "/") + if len(parts) < 4 || parts[2] != "_git" { + return Source{}, fmt.Errorf( + "%q is not an Azure DevOps file URL — it should look like https://dev.azure.com///_git/?path=&version=GB, which is the address you get by opening the file on dev.azure.com", + u.String()) + } + for _, p := range parts { + if p == "" || p == "." || p == ".." { + return Source{}, fmt.Errorf( + "%q has a %q path segment, which is not part of a file's address on dev.azure.com — copy the address bar from the file's page rather than assembling the URL by hand", + u.String(), p) + } + } + + q := u.Query() + filePath := strings.TrimPrefix(strings.TrimSpace(q.Get("path")), "/") + if filePath == "" { + return Source{}, fmt.Errorf("%q names no file inside the repository — it needs a ?path= query parameter", u.String()) + } + for _, p := range strings.Split(filePath, "/") { + if p == "" || p == "." || p == ".." || strings.Contains(p, "\\") || strings.TrimSpace(p) != p { + return Source{}, fmt.Errorf( + "%q has a %q path segment, which is not part of a file's address on dev.azure.com — copy the address bar from the file's page rather than assembling the URL by hand", + u.String(), p) + } + } + + version := q.Get("version") + var urlRef, refType string + switch { + case strings.HasPrefix(version, "GB"): + urlRef, refType = version[2:], "branch" + case strings.HasPrefix(version, "GT"): + urlRef, refType = version[2:], "tag" + case strings.HasPrefix(version, "GC"): + urlRef, refType = version[2:], "commit" + case version != "": + urlRef = version + } + + src := Source{ + Host: HostADO, + Owner: parts[0], + Project: parts[1], + Repo: parts[3], + Origin: "https://dev.azure.com/" + parts[0] + "/" + parts[1] + "/_git/" + parts[3], + Path: filePath, + Ref: urlRef, + RefType: refType, + } + if ref != "" { + src.Ref = ref + src.RefType = "" // not named by the URL; the fetch path resolves it + } + if src.Ref == "" { + return Source{}, fmt.Errorf( + "%q has no version parameter — add &version=GB to the URL, or pass --ref to pin a specific ref", + u.String()) } return src, nil } @@ -178,7 +298,7 @@ const issuesURL = "https://github.com/stacklok/modelith/issues" // Options are the inputs to Import. type Options struct { - // URL is the GitHub blob URL of the model to vendor. + // URL is the GitHub or Azure DevOps blob URL of the model to vendor. URL string // Dir is the directory to write into. Empty means the working directory. Dir string @@ -186,6 +306,10 @@ type Options struct { Ref string // Now stamps the header's imported date, in local time. Now time.Time + // Timeout bounds each delegated command (gh, az) individually. Zero means + // no bound: a hung CLI becomes a fast, actionable error instead of a + // silent wait. + Timeout time.Duration // Run is the command seam; nil uses ExecRunner. Run Runner } @@ -221,11 +345,28 @@ func Import(ctx context.Context, opts Options) (*Result, error) { if runner == nil { runner = ExecRunner{} } + if opts.Timeout > 0 { + runner = timeoutRunner{inner: runner, timeout: opts.Timeout} + } + // An ADO source whose type the URL did not name — a bare version=, or a + // --ref override — has to be resolved before it is fetched, because the API + // does not infer the type: an untyped request is read as a branch. + if err := resolveADORefType(ctx, runner, &src); err != nil { + return nil, err + } - content, err := fetchContent(ctx, runner, src) + // One dispatch, not two: the transport is chosen from the source's host, so + // adding a host does not mean threading a branch through here (and the + // refresh path uses the same pair). + content, err := fetchContentFor(ctx, runner, src) + if err != nil { + return nil, fmt.Errorf("fetching content: %w%s", err, splitHint(src, err)) + } + commit, err := fetchCommitFor(ctx, runner, src) if err != nil { - return nil, fmt.Errorf("%w%s", err, splitHint(src, err)) + return nil, fmt.Errorf("fetching commit: %w", err) } + if provenance.Present(content) { return nil, fmt.Errorf( "%s carries a %s line, so modelith reads it as somebody else's copy rather than a model's home. If it is a copy, vendor it from the origin its header names instead, so this repository tracks the model's home. If it is not — the line is an ordinary comment that happens to use modelith's reserved prefix at column zero — it has to be indented or removed at the origin before this file can be vendored", @@ -245,17 +386,13 @@ func Import(ctx context.Context, opts Options) (*Result, error) { return nil, fmt.Errorf("%s is not a domain model — it %s, not \"DomainModel\"", opts.URL, declares) } - commit, err := fetchCommit(ctx, runner, src) - if err != nil { - return nil, err - } - h := &provenance.Header{ Vendored: provenance.Banner, Fetch: "git", Origin: src.Origin, Path: src.Path, Ref: src.Ref, + RefType: recordedRefType(src), Commit: commit, Imported: opts.Now.Format("2006-01-02"), Digest: provenance.Digest(content), @@ -327,17 +464,18 @@ func guardTarget(target string, src Source) (replaced bool, err error) { } // splitHint explains the one way ParseSource can be wrong about a URL it -// accepted. A browse URL gives no way to tell where a ref containing a slash -// ends and the path begins, so the split is taken at the first segment; a failed -// fetch is where that guess surfaces, as a 404 that looks like the file is -// simply not there. +// accepted. For GitHub sources, a browse URL gives no way to tell where a ref +// containing a slash ends and the path begins, so the split is taken at the +// first segment; a failed fetch is where that guess surfaces, as a 404. ADO +// URLs carry the ref in a query parameter, so this ambiguity does not arise. // -// It is offered only for that failure. A missing gh, a rejected credential, an -// unreachable network — none of those say anything about the URL, and adding a -// paragraph about ref splitting to them would send the reader after the wrong -// problem. A single-segment path had nothing to lose to the ref, so it gets no -// hint either. +// It is offered only for that failure. A missing tool, a rejected credential, +// an unreachable network — none of those say anything about the URL. func splitHint(src Source, err error) string { + // The ref/path ambiguity is GitHub-specific; ADO URLs use query params. + if src.Host == HostADO { + return "" + } if !strings.Contains(src.Path, "/") || !isNotFound(err) { return "" } @@ -346,16 +484,129 @@ func splitHint(src Source, err error) string { src.Ref, src.Path) } -// isNotFound reports whether err is gh saying the endpoint does not exist. -// -// It matches gh's text because gh is a separate program: it reports the status -// on stderr and exits 1, so there is no typed error to unwrap. Reading it wrong -// costs a hint that should not have printed, or one that should have. +// isNotFound reports whether err says the endpoint does not exist. func isNotFound(err error) bool { msg := err.Error() return strings.Contains(msg, "404") || strings.Contains(msg, "Not Found") } +// fetchContentFor fetches the file's bytes through the transport its host uses: +// the gh CLI for GitHub, the az CLI for Azure DevOps. +func fetchContentFor(ctx context.Context, runner Runner, src Source) ([]byte, error) { + if src.Host == HostADO { + return fetchContentADO(ctx, runner, src) + } + return fetchContent(ctx, runner, src) +} + +// fetchCommitFor resolves the commit that last touched the file, through the +// transport its host uses. +func fetchCommitFor(ctx context.Context, runner Runner, src Source) (string, error) { + if src.Host == HostADO { + return fetchCommitADO(ctx, runner, src) + } + return fetchCommit(ctx, runner, src) +} + +// recordedRefType is the ref-type a provenance header records for src. It is +// the ADO version type — branch, tag, or commit — and empty for GitHub, whose +// API resolves an untyped ref on its own and whose headers predate this key. +// +// It is empty rather than "auto" when the type is unknown, which only happens +// if a caller skipped resolveADORefType; leaving the key out makes a later +// refresh resolve it, so the omission is self-healing rather than a lie about +// the API inferring something it does not infer. +func recordedRefType(src Source) string { + if src.Host != HostADO { + return "" + } + return adoVersionType(src) +} + +// resolveADORefType fills in src.RefType when the source names no type, by +// asking the API what the ref is. It is a no-op for GitHub, for an ADO source +// whose URL already carried a GB/GT/GC prefix, and for one whose header +// recorded a type. +// +// It must run before any ADO fetch: the API does not infer a version's type. +// An untyped request is read as a branch (verified against 7.1/7.1-preview.1/ +// 7.2-preview.1: omitting versionDescriptor.versionType on a commit sha fails +// with "The version descriptor > could not be resolved"), so a +// copy pinned to a tag or a commit cannot be fetched while the type is unknown. +func resolveADORefType(ctx context.Context, runner Runner, src *Source) error { + if src.Host != HostADO || adoVersionType(*src) != "" { + return nil + } + // A git object id names a commit and nothing else, so it needs no call. + if isCommitSHA(src.Ref) { + src.RefType = "commit" + return nil + } + // Otherwise ask the refs API. A ref name and a tag name can collide, and a + // version is one or the other; a branch is preferred because that is what + // the API itself defaults to, and a tag is reachable from the URL's GT form. + branch, err := adoRefExists(ctx, runner, *src, "heads/"+src.Ref) + if err != nil { + return err + } + if branch { + src.RefType = "branch" + return nil + } + tag, err := adoRefExists(ctx, runner, *src, "tags/"+src.Ref) + if err != nil { + return err + } + if tag { + src.RefType = "tag" + return nil + } + return fmt.Errorf( + "%q is neither a branch, a tag, nor a commit in %s/%s, so its type cannot be resolved. Check the ref, or name the type in the URL's version= parameter (GB, GT, or GC)", + src.Ref, src.Owner, src.Repo) +} + +// adoRefExists reports whether the repository has the named ref. The filter is +// the ref name without its "refs/" prefix — "heads/main", "tags/v1.0.0" — which +// is what the API's filter parameter matches (verified: filter=heads/main +// returns refs/heads/main, while filter=main returns nothing). +func adoRefExists(ctx context.Context, runner Runner, src Source, ref string) (bool, error) { + uri := fmt.Sprintf( + "https://dev.azure.com/%s/%s/_apis/git/repositories/%s/refs?filter=%s&api-version=7.1", + url.PathEscape(src.Owner), url.PathEscape(src.Project), + url.PathEscape(src.Repo), url.QueryEscape(ref)) + out, err := runner.Run(ctx, "az", "rest", "--method", "get", + "--resource", adoResourceID, "--uri", uri, "--query", "value[].name", "-o", "tsv") + if err != nil { + return false, err + } + for _, line := range strings.Split(strings.TrimSpace(string(out)), "\n") { + if strings.TrimSpace(line) == "refs/"+ref { + return true, nil + } + } + return false, nil +} + +// isCommitSHA reports whether s is a git object id: 40 hex digits (SHA-1) or 64 +// (SHA-256). ADO accepts such a value as versionDescriptor.version with a commit +// type, and nothing else. +func isCommitSHA(s string) bool { + switch len(s) { + case 40, 64: + default: + return false + } + for _, r := range s { + switch { + case r >= '0' && r <= '9', r >= 'a' && r <= 'f', r >= 'A' && r <= 'F': + default: + return false + } + } + return true +} + // fetchContent returns the file's bytes. The raw media type asks the API for // the content itself rather than a JSON envelope carrying it base64-encoded, so // nothing here has to decode. @@ -397,6 +648,29 @@ func fetchCommit(ctx context.Context, runner Runner, src Source) (string, error) // different model's by another. func normOrigin(o string) string { return strings.TrimSuffix(o, "/") } +// normalizeHost puts a URL host in the form a comparison uses: a host is +// case-insensitive, and a browser hands back the "www." form as readily as the +// bare one, so neither names a different site. ParseSource dispatches on this +// form, and any other caller reasoning about an origin's host must agree with +// it — comparing a raw host against "github.com" would refuse "www.github.com" +// and accept nothing a dispatch would. +func normalizeHost(host string) string { + return strings.TrimPrefix(strings.ToLower(host), "www.") +} + +// originHost reports the host an origin URL names, in the normalized form +// normalizeHost produces, or "" when the origin does not parse. It is for +// callers that hold only the recorded origin — refresh, deciding whether it can +// reach the address it is about to rebuild — and must not guess the host by +// string inspection. +func originHost(origin string) string { + u, err := url.Parse(strings.TrimSpace(origin)) + if err != nil { + return "" + } + return normalizeHost(u.Host) +} + // escapePath escapes each segment of a repository path, leaving the separators // alone so the API still sees a path. func escapePath(p string) string { @@ -406,3 +680,89 @@ func escapePath(p string) string { } return strings.Join(segments, "/") } + +// --- Azure DevOps transport (az rest) --- + +// adoResourceID is the Azure DevOps application ID that az rest needs to +// request an AAD token with the correct audience when the URL alone doesn't +// let it derive one. +const adoResourceID = "499b84ac-1321-427f-aa17-267ca6975798" + +// adoVersionType returns the versionDescriptor.versionType for a Source. When +// the version prefix was not one of the known three (GB/GT/GC), the API +// endpoint cannot infer the ref type, so an unknown type is resolved before +// the fetch rather than passed through as an empty versionType. +func adoVersionType(src Source) string { + switch src.RefType { + case "branch", "tag", "commit": + return src.RefType + } + return "" +} + +// fetchContentADO fetches the file content from Azure DevOps by delegating to +// `az rest`. Auth is handled by the az CLI. +// +// download=true is what makes the response the file's bytes. The Items API +// defaults it to false, in which case the body is a JSON GitItem describing the +// item — metadata the model parser would then reject. With it, the body is the +// raw content and --output-file writes those bytes verbatim. +// +// The body is written to a temporary file with --output-file rather than taken +// from stdout: az rest appends a newline when it prints a body to stdout, so the +// stdout form is not byte-identical to the origin file — it drifts a trailing +// newline into the vendored copy and its digest (ADR-0019). +func fetchContentADO(ctx context.Context, runner Runner, src Source) ([]byte, error) { + uri := fmt.Sprintf( + "https://dev.azure.com/%s/%s/_apis/git/repositories/%s/items?path=%s&versionDescriptor.version=%s&download=true&api-version=7.1", + url.PathEscape(src.Owner), url.PathEscape(src.Project), + url.PathEscape(src.Repo), url.QueryEscape(src.Path), + url.QueryEscape(src.Ref)) + if vt := adoVersionType(src); vt != "" { + uri += "&versionDescriptor.versionType=" + url.QueryEscape(vt) + } + + tmp, err := os.CreateTemp("", "modelith-ado-*") + if err != nil { + return nil, fmt.Errorf("creating a temp file for the fetch: %w", err) + } + name := tmp.Name() + if err := tmp.Close(); err != nil { + _ = os.Remove(name) + return nil, fmt.Errorf("closing the temp file for the fetch: %w", err) + } + defer func() { _ = os.Remove(name) }() + + if _, err := runner.Run(ctx, "az", "rest", "--method", "get", + "--resource", adoResourceID, "--uri", uri, + "--output-file", name); err != nil { + return nil, err + } + return os.ReadFile(name) +} + +// fetchCommitADO returns the commit that last touched the file at the given +// ref by delegating to `az rest`. Auth is handled by the az CLI. +func fetchCommitADO(ctx context.Context, runner Runner, src Source) (string, error) { + uri := fmt.Sprintf( + "https://dev.azure.com/%s/%s/_apis/git/repositories/%s/commits?searchCriteria.itemPath=%s&searchCriteria.itemVersion.version=%s&$top=1&api-version=7.1", + url.PathEscape(src.Owner), url.PathEscape(src.Project), + url.PathEscape(src.Repo), url.QueryEscape(src.Path), + url.QueryEscape(src.Ref)) + if vt := adoVersionType(src); vt != "" { + uri += "&searchCriteria.itemVersion.versionType=" + url.QueryEscape(vt) + } + + out, err := runner.Run(ctx, "az", "rest", "--method", "get", + "--resource", adoResourceID, "--uri", uri, + "--query", "value[0].commitId", "-o", "tsv") + if err != nil { + return "", err + } + sha := strings.TrimSpace(string(out)) + if sha == "" || sha == "null" { + return "", fmt.Errorf("%s/%s/_git/%s has no commit touching %q at %q", + src.Owner, src.Project, src.Repo, src.Path, src.Ref) + } + return sha, nil +} diff --git a/internal/deps/deps_test.go b/internal/deps/deps_test.go index 560bcdb..09177a4 100644 --- a/internal/deps/deps_test.go +++ b/internal/deps/deps_test.go @@ -3,8 +3,10 @@ package deps import ( "context" "fmt" + "net/url" "os" "path/filepath" + "runtime" "strings" "testing" "time" @@ -12,11 +14,10 @@ import ( "github.com/stacklok/modelith/internal/provenance" ) -// fakeRunner answers the gh calls Import makes from a map keyed by the API -// endpoint. A hand-written fake rather than a mock: it behaves like gh, -// answering the two endpoints it knows and failing the way gh fails on -// anything else, so a test cannot accidentally assert a response the real -// command could never produce. +// fakeRunner answers the gh/az calls Import makes. It behaves like gh or az, +// answering the endpoints it knows and failing the way they fail on anything +// else, so a test cannot accidentally assert a response the real command could +// never produce. type fakeRunner struct { content string sha string @@ -24,6 +25,8 @@ type fakeRunner struct { calls [][]string // fail, when set, is returned for any call whose endpoint contains it. fail string + // ado sets whether to answer as az rest (ADO) instead of gh api. + ado bool // unusable, when set, makes calls to matching endpoints fail because gh itself // cannot be used. unusable string @@ -31,6 +34,14 @@ type fakeRunner struct { func (f *fakeRunner) Run(_ context.Context, name string, args ...string) ([]byte, error) { f.calls = append(f.calls, append([]string{name}, args...)) + + if f.ado { + return f.runAz(args) + } + return f.runGh(args) +} + +func (f *fakeRunner) runGh(args []string) ([]byte, error) { endpoint := args[len(args)-1] for _, a := range args { if strings.HasPrefix(a, "repos/") { @@ -52,6 +63,168 @@ func (f *fakeRunner) Run(_ context.Context, name string, args ...string) ([]byte return nil, fmt.Errorf("gh: unexpected endpoint %q", endpoint) } +func (f *fakeRunner) runAz(args []string) ([]byte, error) { + var uri, resource string + for i, a := range args { + if a == "--uri" && i+1 < len(args) { + uri = args[i+1] + } + if a == "--resource" && i+1 < len(args) { + resource = args[i+1] + } + } + // The ADO resource ID must be present. + if resource != "499b84ac-1321-427f-aa17-267ca6975798" { + return nil, fmt.Errorf("az: expected --resource 499b84ac-1321-427f-aa17-267ca6975798, got %q", resource) + } + if f.fail != "" && strings.Contains(uri, f.fail) { + return nil, fmt.Errorf("az: HTTP 404: Not Found (%s)", uri) + } + switch { + case strings.Contains(uri, "/refs"): + // The refs API, used to resolve a ref whose type the URL did not name. + // The filter is the ref name without its "refs/" prefix ("heads/main"). + filter := "" + if i := strings.Index(uri, "filter="); i >= 0 { + rest := uri[i+len("filter="):] + if j := strings.IndexByte(rest, '&'); j >= 0 { + rest = rest[:j] + } + if v, err := url.QueryUnescape(rest); err == nil { + filter = v + } + } + if _, ok := adoRefs[filter]; ok { + return []byte("refs/" + filter + "\n"), nil + } + return nil, nil + case strings.Contains(uri, "/items"): + // Mirror the API's version handling, including its default: an omitted + // versionType is read as a *branch*, not inferred. A copy pinned to a + // tag or commit therefore 404s while the type is unset — the real + // behaviour this models, so a test can catch a missing resolution. + vt, ver := adoVersionParams(uri) + if vt == "" { + vt = "branch" + } + if !adoVersionKnown(vt, ver, f.sha) { + return nil, fmt.Errorf("az: TF401175: The version descriptor <%s: %s> could not be resolved to a version (https://dev.azure.com/x/_apis/git/repositories/y/items)", adoTypeLabel(vt), ver) + } + // The real fetch writes the body to --output-file (az rest appends a + // newline when printing a body to stdout, which would drift the + // vendored copy — see fetchContentADO). The fake must mirror that + // contract, so a test cannot pass while asserting an argv the real + // command would not produce. + var outPath string + for i, a := range args { + if a == "--output-file" && i+1 < len(args) { + outPath = args[i+1] + } + } + if outPath == "" { + return nil, fmt.Errorf("az: content fetch must use --output-file, got %q", args) + } + // The Items API returns the file's bytes only with download=true; + // without it the body is a JSON GitItem, which the model parser then + // rejects. Mirroring that here turns the omission into a test failure + // rather than a surprise against a real endpoint. + body := []byte(f.content) + if !strings.Contains(uri, "download=true") { + body = []byte(`{"objectId":"` + f.sha + `","gitObjectType":"blob","path":"/payments.modelith.yaml"}`) + } + if err := os.WriteFile(outPath, body, 0o644); err != nil { + return nil, err + } + return body, nil + case strings.Contains(uri, "/commits"): + // The commit endpoint must not shell-escape $top. + if strings.Contains(uri, "\\$top") { + return nil, fmt.Errorf("az: $top is shell-escaped, but ExecRunner uses argv (no shell)") + } + // The commits query is typed the same way the items request is, so the + // same resolution rule applies. + vt, ver := adoVersionParams(uri) + if vt == "" { + vt = "branch" + } + if !adoVersionKnown(vt, ver, f.sha) { + return nil, fmt.Errorf("az: TF401175: the commits query could not resolve <%s: %s>", adoTypeLabel(vt), ver) + } + return []byte(f.sha + "\n"), nil + } + return nil, fmt.Errorf("az: unexpected uri %q", uri) +} + +// adoRefs are the refs the fake repository serves, keyed the way the refs API's +// filter names them. +var adoRefs = map[string]bool{ + "heads/main": true, + "tags/v1.0.0": true, + "heads/release/v2": true, +} + +// adoVersionParams reads the version and its type out of an ADO URI. The items +// endpoint names them versionDescriptor.version[Type]; the commits endpoint, +// searchCriteria.itemVersion.version[Type]. +func adoVersionParams(uri string) (vt, ver string) { + get := func(key string) string { + i := strings.Index(uri, key+"=") + if i < 0 { + return "" + } + rest := uri[i+len(key)+1:] + if j := strings.IndexByte(rest, '&'); j >= 0 { + rest = rest[:j] + } + v, err := url.QueryUnescape(rest) + if err != nil { + return "" + } + return v + } + for _, k := range []string{"versionDescriptor.versionType", "searchCriteria.itemVersion.versionType"} { + if v := get(k); v != "" { + vt = v + break + } + } + for _, k := range []string{"versionDescriptor.version", "searchCriteria.itemVersion.version"} { + if v := get(k); v != "" { + ver = v + break + } + } + return vt, ver +} + +// adoVersionKnown reports whether the fake repository can resolve a typed +// version. A commit is any value equal to the fixture sha. +func adoVersionKnown(vt, ver, sha string) bool { + switch vt { + case "commit": + return ver == sha + case "branch", "tag": + prefix := "heads/" + if vt == "tag" { + prefix = "tags/" + } + return adoRefs[prefix+ver] + } + return false +} + +func adoTypeLabel(vt string) string { + switch vt { + case "branch": + return "Branch" + case "tag": + return "Tag" + case "commit": + return "Commit" + } + return vt +} + const upstream = `# yaml-language-server: $schema=https://modelith.sh/schema/domain-model/v1.json kind: DomainModel version: v1 @@ -80,6 +253,7 @@ func TestParseSource(t *testing.T) { name: "a browser blob URL", raw: blobURL, want: Source{ + Host: HostGitHub, Origin: "https://github.com/acme/billing", Owner: "acme", Repo: "billing", Ref: "main", Path: "docs/payments.modelith.yaml", }, @@ -88,6 +262,7 @@ func TestParseSource(t *testing.T) { name: "a query and anchor are not part of the address", raw: blobURL + "?plain=1#L12", want: Source{ + Host: HostGitHub, Origin: "https://github.com/acme/billing", Owner: "acme", Repo: "billing", Ref: "main", Path: "docs/payments.modelith.yaml", }, @@ -97,6 +272,7 @@ func TestParseSource(t *testing.T) { raw: blobURL, ref: "v2.1.0", want: Source{ + Host: HostGitHub, Origin: "https://github.com/acme/billing", Owner: "acme", Repo: "billing", Ref: "v2.1.0", Path: "docs/payments.modelith.yaml", }, @@ -108,6 +284,7 @@ func TestParseSource(t *testing.T) { raw: "https://github.com/acme/billing/blob/release/v2/docs/payments.modelith.yaml", ref: "release/v2", want: Source{ + Host: HostGitHub, Origin: "https://github.com/acme/billing", Owner: "acme", Repo: "billing", Ref: "release/v2", Path: "docs/payments.modelith.yaml", }, @@ -118,6 +295,7 @@ func TestParseSource(t *testing.T) { name: "the host is matched without regard to case or a www prefix", raw: "https://WWW.GitHub.com/acme/billing/blob/main/docs/payments.modelith.yaml", want: Source{ + Host: HostGitHub, Origin: "https://github.com/acme/billing", Owner: "acme", Repo: "billing", Ref: "main", Path: "docs/payments.modelith.yaml", }, @@ -189,6 +367,48 @@ func importInto(t *testing.T, dir string, r *fakeRunner, url string) (*Result, e }) } +// TestADR_0019_RefTypeIsRecordedOnlyWhereItIsNeeded pins the one header change +// ADR-0019 makes. Azure DevOps takes a version's *type* alongside its value and +// does not infer one (an untyped request is read as a branch), so an ADO import +// resolves and records it. GitHub's API resolves an untyped ref on its own, so a +// GitHub header must not gain the key: that is what keeps a header +// written before this change byte-identical to one written after it, with no +// migration. +func TestADR_0019_RefTypeIsRecordedOnlyWhereItIsNeeded(t *testing.T) { + t.Parallel() + + ghDir := t.TempDir() + gh, err := importInto(t, ghDir, &fakeRunner{content: upstream, sha: sha}, blobURL) + if err != nil { + t.Fatal(err) + } + if gh.Header.RefType != "" { + t.Errorf("a GitHub header carries a ref type (%q); its API needs none, and the key must be omitted", gh.Header.RefType) + } + if got := readImportFile(t, gh.Path); strings.Contains(got, "# modelith-ref-type:") { + t.Errorf("a GitHub copy was stamped with a ref-type line:\n%s", got) + } + + adoDir := t.TempDir() + ado, err := importAdoInto(t, adoDir, adoRunner(adoContent, adoCommit), adoBlobURL) + if err != nil { + t.Fatal(err) + } + if ado.Header.RefType != "branch" { + t.Errorf("an ADO header recorded ref type %q, want %q for a GB URL", ado.Header.RefType, "branch") + } +} + +// readImportFile reads a file a test just wrote, failing on error. +func readImportFile(t *testing.T, path string) string { + t.Helper() + b, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + return string(b) +} + func TestImport_StampsAVerifiableCopy(t *testing.T) { t.Parallel() @@ -483,7 +703,7 @@ func TestImport_RefusesUnknownReservedPrefixAtTarget(t *testing.T) { func TestSplitHint(t *testing.T) { t.Parallel() - src := Source{Ref: "main", Path: "docs/payments.modelith.yaml"} + src := Source{Host: HostGitHub, Ref: "main", Path: "docs/payments.modelith.yaml"} cases := []struct { name string src Source @@ -496,7 +716,7 @@ func TestSplitHint(t *testing.T) { {"a forbidden repository", src, fmt.Errorf("gh: HTTP 403: Forbidden"), false}, {"an unreachable network", src, fmt.Errorf("dial tcp: lookup api.github.com: no such host"), false}, {"a single-segment path has nothing to lose to the ref", - Source{Ref: "main", Path: "payments.modelith.yaml"}, fmt.Errorf("gh: HTTP 404: Not Found"), false}, + Source{Host: HostGitHub, Ref: "main", Path: "payments.modelith.yaml"}, fmt.Errorf("gh: HTTP 404: Not Found"), false}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -618,3 +838,841 @@ func TestImport_ReplacesAnEarlierCopy(t *testing.T) { t.Error("the replaced copy carries more than one header") } } + +// --- Azure DevOps tests --- + +const adoBlobURL = "https://dev.azure.com/myorg/myproject/_git/myrepo?path=docs/payments.modelith.yaml&version=GBmain" + +const adoContent = `# yaml-language-server: $schema=https://modelith.sh/schema/domain-model/v1.json +kind: DomainModel +version: v1 +title: Payments +enums: + PaymentMethod: + values: + - name: card +` + +const adoCommit = "4f2c1e9c8b3ad0e5f71b2c9a6d4e8f30ab5c7d21" + +func adoRunner(content, sha string) *fakeRunner { + return &fakeRunner{content: content, sha: sha, ado: true} +} + +func importAdoInto(t *testing.T, dir string, r *fakeRunner, url string) (*Result, error) { + t.Helper() + return Import(context.Background(), Options{ + URL: url, + Dir: dir, + Now: time.Date(2026, 7, 27, 12, 0, 0, 0, time.Local), + Run: r, + }) +} + +func TestParseSource_ADO(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + raw string + ref string + want Source + wantErr string + }{ + { + name: "a browser ADO blob URL with GB branch", + raw: adoBlobURL, + want: Source{ + Host: HostADO, + Origin: "https://dev.azure.com/myorg/myproject/_git/myrepo", + Owner: "myorg", + Project: "myproject", + Repo: "myrepo", + Ref: "main", + RefType: "branch", + Path: "docs/payments.modelith.yaml", + }, + }, + { + name: "an explicit ref overrides the one in the URL", + raw: adoBlobURL, + ref: "release/v2", + want: Source{ + Host: HostADO, + Origin: "https://dev.azure.com/myorg/myproject/_git/myrepo", + Owner: "myorg", + Project: "myproject", + Repo: "myrepo", + Ref: "release/v2", + RefType: "", + Path: "docs/payments.modelith.yaml", + }, + }, + { + name: "an explicit ref leaves RefType unset for the fetch path to resolve", + raw: "https://dev.azure.com/myorg/myproject/_git/myrepo?path=docs/payments.modelith.yaml&version=GBmain", + ref: "v1.0.0", + want: Source{ + Host: HostADO, + Origin: "https://dev.azure.com/myorg/myproject/_git/myrepo", + Owner: "myorg", + Project: "myproject", + Repo: "myrepo", + Ref: "v1.0.0", + RefType: "", + Path: "docs/payments.modelith.yaml", + }, + }, + { + name: "GT tag prefix", + raw: "https://dev.azure.com/myorg/myproject/_git/myrepo?path=docs/payments.modelith.yaml&version=GTv1.0.0", + want: Source{ + Host: HostADO, + Origin: "https://dev.azure.com/myorg/myproject/_git/myrepo", + Owner: "myorg", + Project: "myproject", + Repo: "myrepo", + Ref: "v1.0.0", + RefType: "tag", + Path: "docs/payments.modelith.yaml", + }, + }, + { + name: "GC commit prefix", + raw: "https://dev.azure.com/myorg/myproject/_git/myrepo?path=docs/payments.modelith.yaml&version=GC" + adoCommit, + want: Source{ + Host: HostADO, + Origin: "https://dev.azure.com/myorg/myproject/_git/myrepo", + Owner: "myorg", + Project: "myproject", + Repo: "myrepo", + Ref: adoCommit, + RefType: "commit", + Path: "docs/payments.modelith.yaml", + }, + }, + { + name: "an anchor is stripped", + raw: adoBlobURL + "&_a=contents", + want: Source{ + Host: HostADO, + Origin: "https://dev.azure.com/myorg/myproject/_git/myrepo", + Owner: "myorg", + Project: "myproject", + Repo: "myrepo", + Ref: "main", + RefType: "branch", + Path: "docs/payments.modelith.yaml", + }, + }, + { + name: "leading slash in path query parameter is stripped", + raw: "https://dev.azure.com/myorg/myproject/_git/myrepo?path=/docs/payments.modelith.yaml&version=GBmain", + want: Source{ + Host: HostADO, + Origin: "https://dev.azure.com/myorg/myproject/_git/myrepo", + Owner: "myorg", + Project: "myproject", + Repo: "myrepo", + Ref: "main", + RefType: "branch", + Path: "docs/payments.modelith.yaml", + }, + }, + { + name: "no path query parameter", + raw: "https://dev.azure.com/myorg/myproject/_git/myrepo?version=GBmain", + wantErr: "names no file inside the repository", + }, + { + name: "not a _git URL", + raw: "https://dev.azure.com/myorg/myproject/_wiki/wikis", + wantErr: "not an Azure DevOps file URL", + }, + { + name: "a traversal segment in URL path is rejected", + raw: "https://dev.azure.com/myorg/../_git/myrepo?path=docs/payments.modelith.yaml&version=GBmain", + wantErr: `has a ".." path segment`, + }, + { + name: "double slash in path query parameter is rejected", + raw: "https://dev.azure.com/myorg/myproject/_git/myrepo?path=docs//payments.modelith.yaml&version=GBmain", + wantErr: `has a "" path segment`, + }, + { + name: "trailing slash in path query parameter is rejected", + raw: "https://dev.azure.com/myorg/myproject/_git/myrepo?path=docs/payments.modelith.yaml/&version=GBmain", + wantErr: `has a "" path segment`, + }, + { + name: "relative dot traversal in path query parameter is rejected", + raw: "https://dev.azure.com/myorg/myproject/_git/myrepo?path=docs/./payments.modelith.yaml&version=GBmain", + wantErr: `has a "." path segment`, + }, + { + name: "parent dot traversal in path query parameter is rejected", + raw: "https://dev.azure.com/myorg/myproject/_git/myrepo?path=docs/../../secret.yaml&version=GBmain", + wantErr: `has a ".." path segment`, + }, + { + name: "backslash in path segment is rejected", + raw: "https://dev.azure.com/myorg/myproject/_git/myrepo?path=docs\\payments.modelith.yaml&version=GBmain", + wantErr: `has a "docs\\payments.modelith.yaml" path segment`, + }, + { + name: "untrimmed whitespace in path segment is rejected", + raw: "https://dev.azure.com/myorg/myproject/_git/myrepo?path=docs/ payments.modelith.yaml&version=GBmain", + wantErr: `has a " payments.modelith.yaml" path segment`, + }, + { + name: "legacy visualstudio.com URL offers migration hint to dev.azure.com", + raw: "https://myorg.visualstudio.com/myproject/_git/myrepo?path=docs/payments.modelith.yaml&version=GBmain", + wantErr: `is a legacy visualstudio.com URL — Azure DevOps moved to dev.azure.com`, + }, + { + name: "no version parameter and no --ref", + raw: "https://dev.azure.com/myorg/myproject/_git/myrepo?path=docs/payments.modelith.yaml", + wantErr: "has no version parameter", + }, + { + name: "a bare version with no prefix leaves RefType to be resolved", + raw: "https://dev.azure.com/myorg/myproject/_git/myrepo?path=docs/payments.modelith.yaml&version=main", + want: Source{ + Host: HostADO, + Origin: "https://dev.azure.com/myorg/myproject/_git/myrepo", + Owner: "myorg", + Project: "myproject", + Repo: "myrepo", + Ref: "main", + RefType: "", + Path: "docs/payments.modelith.yaml", + }, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := ParseSource(tc.raw, tc.ref) + if tc.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("want an error containing %q, got %v", tc.wantErr, err) + } + return + } + if err != nil { + t.Fatal(err) + } + if got != tc.want { + t.Errorf("ParseSource() = %+v, want %+v", got, tc.want) + } + }) + } +} + +func TestImport_ADO_StampsAVerifiableCopy(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + r := adoRunner(adoContent, adoCommit) + res, err := importAdoInto(t, dir, r, adoBlobURL) + if err != nil { + t.Fatal(err) + } + + written, err := os.ReadFile(res.Path) + if err != nil { + t.Fatal(err) + } + if got, want := res.Path, filepath.Join(dir, "payments.modelith.yaml"); got != want { + t.Errorf("wrote %s, want %s", got, want) + } + if res.Replaced { + t.Error("reported replacing a file that did not exist") + } + + h, problems := provenance.Parse(written) + if len(problems) != 0 { + t.Fatalf("the stamped copy has header problems: %+v", problems) + } + want := provenance.Header{ + Vendored: provenance.Banner, + Fetch: "git", + Origin: "https://dev.azure.com/myorg/myproject/_git/myrepo", + Path: "docs/payments.modelith.yaml", + Ref: "main", + RefType: "branch", + Commit: adoCommit, + Imported: "2026-07-27", + Digest: provenance.Digest([]byte(adoContent)), + } + if *h != want { + t.Errorf("stamped header = %+v, want %+v", *h, want) + } + if ok, got := h.Verify(written); !ok { + t.Errorf("the freshly written copy does not verify: computed %s", got) + } + + if !strings.HasPrefix(string(written), "# yaml-language-server:") { + t.Error("the editor directive is no longer the first line") + } + if !strings.Contains(string(written), " PaymentMethod:\n") { + t.Error("the model content did not survive the stamp") + } +} + +func TestImport_ADO_CallsAzWithTheExpectedEndpoints(t *testing.T) { + t.Parallel() + + r := adoRunner(adoContent, adoCommit) + if _, err := importAdoInto(t, t.TempDir(), r, adoBlobURL); err != nil { + t.Fatal(err) + } + if len(r.calls) != 2 { + t.Fatalf("want 2 az calls, got %d: %+v", len(r.calls), r.calls) + } + // Find the --uri and --resource arguments in each call. + var foundContent, foundCommit, foundResource int + for _, call := range r.calls { + for i, a := range call { + if a == "--resource" && i+1 < len(call) && call[i+1] == adoResourceID { + foundResource++ + } + if a == "--uri" && i+1 < len(call) { + uri := call[i+1] + if strings.Contains(uri, "/items") { + foundContent++ + // Without download=true the Items API returns a JSON + // GitItem, and the import cannot parse the model out of it. + if !strings.Contains(uri, "download=true") { + t.Errorf("the items request omits download=true, so the API returns JSON metadata rather than the model: %s", uri) + } + } + if strings.Contains(uri, "/commits") { + foundCommit++ + } + } + } + } + if foundResource != 2 { + t.Errorf("want 2 --resource flags, got %d", foundResource) + } + if foundContent == 0 { + t.Error("no az rest call with /items endpoint") + } + if foundCommit == 0 { + t.Error("no az rest call with /commits endpoint") + } +} + +// TestImport_ADO_TagUsesVersionTypeTag pins that a GT (tag) URL produces +// versionType=tag in the API call, not the hardcoded "branch". +func TestImport_ADO_TagUsesVersionTypeTag(t *testing.T) { + t.Parallel() + + r := adoRunner(adoContent, adoCommit) + tagURL := "https://dev.azure.com/myorg/myproject/_git/myrepo?path=docs/payments.modelith.yaml&version=GTv1.0.0" + if _, err := importAdoInto(t, t.TempDir(), r, tagURL); err != nil { + t.Fatal(err) + } + for _, call := range r.calls { + for i, a := range call { + if a == "--uri" && i+1 < len(call) { + uri := call[i+1] + if strings.Contains(uri, "/items") && !strings.Contains(uri, "versionType=tag") { + t.Errorf("tag URL should produce versionType=tag, got %q", uri) + } + if strings.Contains(uri, "/commits") && !strings.Contains(uri, "versionType=tag") { + t.Errorf("tag URL should produce versionType=tag in commits call, got %q", uri) + } + } + } + } +} + +// TestImport_ADO_CommitUsesVersionTypeCommit pins that a GC (commit) URL +// produces versionType=commit in the API call. +func TestImport_ADO_CommitUsesVersionTypeCommit(t *testing.T) { + t.Parallel() + + r := adoRunner(adoContent, adoCommit) + commitURL := "https://dev.azure.com/myorg/myproject/_git/myrepo?path=docs/payments.modelith.yaml&version=GC" + adoCommit + if _, err := importAdoInto(t, t.TempDir(), r, commitURL); err != nil { + t.Fatal(err) + } + for _, call := range r.calls { + for i, a := range call { + if a == "--uri" && i+1 < len(call) { + uri := call[i+1] + if strings.Contains(uri, "/items") && !strings.Contains(uri, "versionType=commit") { + t.Errorf("commit URL should produce versionType=commit, got %q", uri) + } + } + } + } +} + +// TestImport_ADO_OverrideRefOmitsVersionType pins that when --ref overrides +// the URL's ref, the value is left untyped for the fetch path to resolve. +// TestImport_ADO_OverrideRefResolvesTheType pins that a --ref override on an ADO +// URL is typed before it is fetched. The override names a ref the URL's GB/GT/GC +// prefix does not describe, and the API does not infer a type — an untyped +// request is read as a branch — so the type is resolved from the repository's +// refs. Here the override names a tag, so the request must ask for a *tag*, not +// fall through to the API's branch default. +func TestImport_ADO_OverrideRefResolvesTheType(t *testing.T) { + t.Parallel() + + r := adoRunner(adoContent, adoCommit) + res, err := Import(context.Background(), Options{ + URL: adoBlobURL, // names GBmain, overridden below + Dir: t.TempDir(), + Ref: "v1.0.0", // a tag in the fake repository + Now: time.Date(2026, 7, 27, 12, 0, 0, 0, time.Local), + Run: r, + }) + if err != nil { + t.Fatal(err) + } + if res.Header.RefType != "tag" { + t.Errorf("the header recorded ref-type %q, want %q — the override named a tag", res.Header.RefType, "tag") + } + + var sawRefs, sawTypedFetch bool + for _, call := range r.calls { + for i, a := range call { + if a != "--uri" || i+1 >= len(call) { + continue + } + uri := call[i+1] + if strings.Contains(uri, "/refs?") { + sawRefs = true + continue + } + if !strings.Contains(uri, "version=v1.0.0") { + t.Errorf("a fetch did not use the override value in version=: %q", uri) + continue + } + if !strings.Contains(uri, "versionType=tag") { + t.Errorf("a fetch did not ask for the resolved type (tag): %q", uri) + continue + } + sawTypedFetch = true + } + } + if !sawRefs { + t.Error("the type was never resolved with a refs lookup") + } + if !sawTypedFetch { + t.Error("no fetch carried the resolved versionType") + } +} + +// TestImport_ADO_OverrideRefCommitNeedsNoRefsLookup pins the cheap half of +// resolution: a git object id names a commit and nothing else, so an override +// that is one is typed without asking the refs API. +func TestImport_ADO_OverrideRefCommitNeedsNoRefsLookup(t *testing.T) { + t.Parallel() + + r := adoRunner(adoContent, adoCommit) + res, err := Import(context.Background(), Options{ + URL: adoBlobURL, + Dir: t.TempDir(), + Ref: adoCommit, + Now: time.Date(2026, 7, 27, 12, 0, 0, 0, time.Local), + Run: r, + }) + if err != nil { + t.Fatal(err) + } + if res.Header.RefType != "commit" { + t.Errorf("the header recorded ref-type %q, want %q", res.Header.RefType, "commit") + } + for _, call := range r.calls { + for i, a := range call { + if a == "--uri" && i+1 < len(call) && strings.Contains(call[i+1], "/refs?") { + t.Errorf("a commit sha was resolved with a needless refs lookup: %q", call[i+1]) + } + } + } +} + +func TestImport_ADO_RejectsAlreadyVendored(t *testing.T) { + t.Parallel() + + r := adoRunner("# modelith-origin: https://dev.azure.com/other/proj/_git/repo\n"+adoContent, adoCommit) + _, err := importAdoInto(t, t.TempDir(), r, adoBlobURL) + if err == nil || !strings.Contains(err.Error(), "reads it as somebody else's copy") { + t.Fatalf("want 'reads it as somebody else's copy', got %v", err) + } +} + +func TestImport_ADO_Rejections(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + runner *fakeRunner + url string + wantErr string + }{ + { + name: "a file that is not a domain model", + runner: adoRunner("kind: SomethingElse\nversion: v1\n", adoCommit), + url: adoBlobURL, + wantErr: `it declares kind "SomethingElse"`, + }, + { + name: "a file with no kind at all", + runner: adoRunner("title: Payments\n", adoCommit), + url: adoBlobURL, + wantErr: "it declares no kind", + }, + { + name: "a path with no commits at that ref (empty commit sha)", + runner: adoRunner(adoContent, ""), + url: adoBlobURL, + wantErr: "no commit touching", + }, + { + name: "a path with no commits at that ref (null commit sha)", + runner: adoRunner(adoContent, "null"), + url: adoBlobURL, + wantErr: "no commit touching", + }, + { + name: "az refusing the fetch", + runner: &fakeRunner{content: adoContent, sha: adoCommit, ado: true, fail: "/items"}, + url: adoBlobURL, + wantErr: "HTTP 404", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + _, err := importAdoInto(t, dir, tc.runner, tc.url) + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("want an error containing %q, got %v", tc.wantErr, err) + } + }) + } +} + +// TestImport_ADO_RejectsEmptyRef pins that an ADO URL without a version +// parameter and no --ref is rejected with a clear message, rather than +// silently stamping a header with an empty ref. +func TestImport_ADO_RejectsEmptyRef(t *testing.T) { + t.Parallel() + + url := "https://dev.azure.com/myorg/myproject/_git/myrepo?path=docs/payments.modelith.yaml" + _, err := Import(context.Background(), Options{ + URL: url, + Dir: t.TempDir(), + Now: time.Date(2026, 7, 27, 12, 0, 0, 0, time.Local), + Run: adoRunner(adoContent, adoCommit), + }) + if err == nil { + t.Fatal("want an error for a URL with no version parameter, got nil") + } + if !strings.Contains(err.Error(), "has no version parameter") { + t.Errorf("want 'has no version parameter' in error, got: %v", err) + } +} + +func TestSplitHint_ADO(t *testing.T) { + t.Parallel() + + src := Source{ + Host: HostADO, + Project: "myproject", + Ref: "main", + Path: "docs/payments.modelith.yaml", + } + if got := splitHint(src, fmt.Errorf("HTTP 404: Not Found")); got != "" { + t.Errorf("splitHint for ADO source should be empty, got %q", got) + } +} + +// --- Timeout decorator tests --- + +// runnerFunc adapts a func to the Runner interface for tests. +type runnerFunc func(ctx context.Context, name string, args ...string) ([]byte, error) + +func (f runnerFunc) Run(ctx context.Context, name string, args ...string) ([]byte, error) { + return f(ctx, name, args...) +} + +// hungRunner blocks every call until the context Import gave it expires, the +// way a stalled az/gh process behaves under exec.CommandContext. +type hungRunner struct{ calls int } + +func (h *hungRunner) Run(ctx context.Context, name string, args ...string) ([]byte, error) { + h.calls++ + <-ctx.Done() + return nil, ctx.Err() +} + +// ctxRecordingRunner records the context each call received, so a test can +// assert what Import delivered: a deadline when Timeout is set, the caller's +// bare ctx when it is zero. +type ctxRecordingRunner struct { + fakeRunner + ctxs []context.Context +} + +func (r *ctxRecordingRunner) Run(ctx context.Context, name string, args ...string) ([]byte, error) { + r.ctxs = append(r.ctxs, ctx) + return r.fakeRunner.Run(ctx, name, args...) +} + +// TestTimeoutRunner_ReturnsAClearErrorOnDeadline pins the decorator's contract: +// a hung command becomes an error that names the command and the bound and says +// how to raise it, and that never echoes the argv — defense in depth, since the +// argv can carry URIs a caller would not want duplicated into error logs. +func TestTimeoutRunner_ReturnsAClearErrorOnDeadline(t *testing.T) { + t.Parallel() + + inner := runnerFunc(func(ctx context.Context, name string, args ...string) ([]byte, error) { + <-ctx.Done() + return nil, ctx.Err() + }) + r := timeoutRunner{inner: inner, timeout: 10 * time.Millisecond} + + _, err := r.Run(context.Background(), "az", "rest", "--uri", "https://dev.azure.com/secret/org/_apis/items") + if err == nil { + t.Fatal("expected a timeout error, got nil") + } + for _, want := range []string{"az", "10ms", "--timeout"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the error does not name %q: %v", want, err) + } + } + for _, leaked := range []string{"rest", "--uri", "dev.azure.com/secret"} { + if strings.Contains(err.Error(), leaked) { + t.Errorf("the error echoes the argv (%q): %v", leaked, err) + } + } +} + +// TestTimeoutRunner_ReportsAKilledChildAsTheBound pins the issue #3 fix: when +// the bound fires, ExecRunner SIGKILLs the process group and cmd.Output +// returns *exec.ExitError ("signal: killed"), which errors.Is against +// DeadlineExceeded / ErrWaitDelay does not match. The friendly message must +// fire anyway — and must not echo the argv, the URI the command was fetching. +func TestTimeoutRunner_ReportsAKilledChildAsTheBound(t *testing.T) { + t.Parallel() + if runtime.GOOS == "windows" { + t.Skip("process groups are unix-only") + } + + r := timeoutRunner{inner: ExecRunner{}, timeout: 200 * time.Millisecond} + start := time.Now() + _, err := r.Run(context.Background(), "sleep", "30") + if err == nil { + t.Fatal("expected a timeout error, got nil") + } + for _, want := range []string{"sleep", "did not finish within", "--timeout"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the error does not name %q: %v", want, err) + } + } + for _, leaked := range []string{"signal: killed", "sleep 30"} { + if strings.Contains(err.Error(), leaked) { + t.Errorf("the error leaks %q: %v", leaked, err) + } + } + if elapsed := time.Since(start); elapsed > 10*time.Second { + t.Fatalf("Run blocked %v after the bound — the deadline did not fire", elapsed) + } +} + +// TestTimeoutRunner_ParentCancelIsNotReportedAsTimeout pins the review finding +// on 3f88c53: a caller canceling the context (e.g. a future Ctrl+C handler) +// must not be relabeled as a timeout. The bound is deliberately far away, so +// only the parent cancel can stop the child — the error path is otherwise +// identical to a timeout (process-group SIGKILL, *exec.ExitError), and the +// context is what tells the two apart: context.Canceled is not a deadline the +// user can raise with --timeout. +func TestTimeoutRunner_ParentCancelIsNotReportedAsTimeout(t *testing.T) { + t.Parallel() + if runtime.GOOS == "windows" { + t.Skip("process groups are unix-only") + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go func() { + time.Sleep(100 * time.Millisecond) + cancel() + }() + + r := timeoutRunner{inner: ExecRunner{}, timeout: time.Minute} + start := time.Now() + _, err := r.Run(ctx, "sleep", "30") + if err == nil { + t.Fatal("expected an error, got nil") + } + for _, misleading := range []string{"did not finish within", "--timeout"} { + if strings.Contains(err.Error(), misleading) { + t.Errorf("a caller cancel is misreported as a timeout (%q): %v", misleading, err) + } + } + if elapsed := time.Since(start); elapsed > 10*time.Second { + t.Fatalf("Run blocked %v after the cancel — the child was not killed", elapsed) + } +} + +// TestImport_TimeoutIsPerCommand pins that the bound applies to each command +// individually, not to the whole import: a whole-import budget could let the +// first call consume everything and fail on the second. A per-command deadline +// fails at the first hung call, which is the one the user is waiting on. +func TestImport_TimeoutIsPerCommand(t *testing.T) { + t.Parallel() + + hung := &hungRunner{} + _, err := Import(context.Background(), Options{ + URL: blobURL, + Dir: t.TempDir(), + Now: time.Date(2026, 7, 27, 12, 0, 0, 0, time.Local), + Run: hung, + Timeout: 20 * time.Millisecond, + }) + if err == nil { + t.Fatal("expected a timeout error, got nil") + } + if !strings.Contains(err.Error(), "gh did not finish within") { + t.Errorf("the error should name the first command (gh), got: %v", err) + } + if !strings.Contains(err.Error(), "--timeout") { + t.Errorf("the error should say how to adjust the bound, got: %v", err) + } + if hung.calls != 1 { + t.Errorf("the first hung call consumed the whole import; got %d calls, want 1", hung.calls) + } +} + +// TestImport_ADO_TimeoutBoundsTheAzCall pins the fix this feature exists for: +// a stalled az rest becomes a fast, actionable error naming az, not a silent +// 75s wait on the commits-with-path query. +func TestImport_ADO_TimeoutBoundsTheAzCall(t *testing.T) { + t.Parallel() + + hung := &hungRunner{} + _, err := Import(context.Background(), Options{ + URL: adoBlobURL, + Dir: t.TempDir(), + Now: time.Date(2026, 7, 27, 12, 0, 0, 0, time.Local), + Run: hung, + Timeout: 20 * time.Millisecond, + }) + if err == nil { + t.Fatal("expected a timeout error, got nil") + } + if !strings.Contains(err.Error(), "az did not finish within") { + t.Errorf("the error should name az, got: %v", err) + } + if hung.calls != 1 { + t.Errorf("the first az call should fail alone; got %d calls", hung.calls) + } +} + +// TestImport_ZeroTimeoutPassesTheCallerContextThrough pins that Options.Timeout +// zero (the default) leaves the runner's context untouched: no deadline is +// added, so the decorator is a no-op and existing behavior is unchanged. +func TestImport_ZeroTimeoutPassesTheCallerContextThrough(t *testing.T) { + t.Parallel() + + rec := &ctxRecordingRunner{fakeRunner: fakeRunner{content: upstream, sha: sha}} + if _, err := Import(context.Background(), Options{ + URL: blobURL, + Dir: t.TempDir(), + Now: time.Date(2026, 7, 27, 12, 0, 0, 0, time.Local), + Run: rec, + Timeout: 0, + }); err != nil { + t.Fatal(err) + } + if len(rec.ctxs) != 2 { + t.Fatalf("want 2 calls, got %d", len(rec.ctxs)) + } + for i, ctx := range rec.ctxs { + if _, ok := ctx.Deadline(); ok { + t.Errorf("call %d received a deadline; Timeout 0 must pass the caller's ctx through", i) + } + } +} + +// TestImport_WithTimeoutBoundsEachCall pins that Timeout > 0 applies the +// decorator to every delegated command, and that a command that finishes within +// the bound still succeeds. +func TestImport_WithTimeoutBoundsEachCall(t *testing.T) { + t.Parallel() + + rec := &ctxRecordingRunner{fakeRunner: fakeRunner{content: upstream, sha: sha}} + if _, err := Import(context.Background(), Options{ + URL: blobURL, + Dir: t.TempDir(), + Now: time.Date(2026, 7, 27, 12, 0, 0, 0, time.Local), + Run: rec, + Timeout: time.Minute, + }); err != nil { + t.Fatal(err) + } + for i, ctx := range rec.ctxs { + d, ok := ctx.Deadline() + if !ok { + t.Errorf("call %d received no deadline; Timeout %v should bound each call", i, time.Minute) + continue + } + if time.Until(d) > time.Minute { + t.Errorf("call %d deadline %v is later than the %v bound", i, d, time.Minute) + } + } +} + +// TestExecRunner_KillsTheWholeProcessGroup pins the fix for the macOS stall +// (issue #2): CommandContext's default kill sends SIGKILL only to the direct +// child, so a CLI that spawned a helper leaves that helper holding the stdout +// pipe — Wait then blocks past the deadline. Killing the process group closes +// the pipe and the call returns. Here the direct child (sh) is waiting on a +// backgrounded sleep: killing only sh would leave sleep holding the pipe for +// its full 30s. +func TestExecRunner_KillsTheWholeProcessGroup(t *testing.T) { + t.Parallel() + if runtime.GOOS == "windows" { + t.Skip("process groups are unix-only") + } + + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + + start := time.Now() + _, err := (ExecRunner{}).Run(ctx, "sh", "-c", "sleep 30 & wait") + if err == nil { + t.Fatal("expected an error, got nil") + } + if elapsed := time.Since(start); elapsed > 10*time.Second { + t.Fatalf("Run blocked %v after the deadline — the process group was not killed", elapsed) + } +} + +// TestExecRunner_WaitDelayBoundsAPipeHeldByAStrayChild pins the belt-and- +// braces half of the fix: even if a helper escapes the process group, the +// stdout pipe it inherited cannot hold Wait hostage past WaitDelay. The shell +// exits immediately; the backgrounded sleep inherits the stdout pipe and keeps +// it open for 30s. WaitDelay must make Run return long before that. +func TestExecRunner_WaitDelayBoundsAPipeHeldByAStrayChild(t *testing.T) { + t.Parallel() + if runtime.GOOS == "windows" { + t.Skip("process groups are unix-only") + } + + start := time.Now() + _, err := (ExecRunner{}).Run(context.Background(), "sh", "-c", "sleep 30 &") + if err == nil { + t.Fatal("expected an error (WaitDelay abandoned the held pipe), got nil") + } + if elapsed := time.Since(start); elapsed > 10*time.Second { + t.Fatalf("Run blocked %v on a pipe held by a stray child", elapsed) + } +} diff --git a/internal/deps/exec_unix.go b/internal/deps/exec_unix.go new file mode 100644 index 0000000..d31bb67 --- /dev/null +++ b/internal/deps/exec_unix.go @@ -0,0 +1,44 @@ +//go:build !windows + +package deps + +import ( + "context" + "os/exec" + "strings" + "syscall" + "time" +) + +// Run executes name with args and returns its standard output. Standard error +// is folded into the returned error, because the CLI reports why it refused there. +// +// The child runs in its own process group and is killed as a group on context +// cancellation. CommandContext alone kills only the direct child, and a CLI +// like az that spawns a helper (token refresh, keychain) can leave that helper +// holding the stdout/stderr pipe — Wait then blocks forever past the deadline, +// which is the macOS stall this guards against. WaitDelay bounds that last +// wait regardless, so a helper that escaped the group still cannot hold Wait +// hostage. +func (ExecRunner) Run(ctx context.Context, name string, args ...string) ([]byte, error) { + // nolint:gosec // G204 flags a variable command and arguments, which is + // what a transport seam is. The commands are the literals "gh" and "az" at + // the call sites; the arguments are literals plus endpoints assembled from + // URLs that ParseSource has already validated. Nothing here comes from a + // model file, and there is no shell: exec passes an argv array. + cmd := exec.CommandContext(ctx, name, args...) + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + cmd.Cancel = func() error { + if cmd.Process == nil { + return nil + } + // The negative pid names the process group, so a helper the CLI + // spawned dies with it instead of keeping the pipe open past the + // deadline. ESRCH is fine — the process may already be gone. + return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + } + cmd.WaitDelay = 5 * time.Second + var stderr strings.Builder + cmd.Stderr = &stderr + return runCommand(cmd, name, args, &stderr) +} diff --git a/internal/deps/exec_windows.go b/internal/deps/exec_windows.go new file mode 100644 index 0000000..25c55ac --- /dev/null +++ b/internal/deps/exec_windows.go @@ -0,0 +1,30 @@ +//go:build windows + +package deps + +import ( + "context" + "os/exec" + "strings" + "time" +) + +// Run executes name with args and returns its standard output. Standard error +// is folded into the returned error, because the CLI reports why it refused there. +// +// Windows has no POSIX process groups, so the group kill that !windows builds +// use has no equivalent here: CommandContext still terminates the direct child +// on cancellation, and WaitDelay bounds the wait even if a helper the CLI +// spawned keeps the pipe open. +func (ExecRunner) Run(ctx context.Context, name string, args ...string) ([]byte, error) { + // nolint:gosec // G204 flags a variable command and arguments, which is + // what a transport seam is. The commands are the literals "gh" and "az" at + // the call sites; the arguments are literals plus endpoints assembled from + // URLs that ParseSource has already validated. Nothing here comes from a + // model file, and there is no shell: exec passes an argv array. + cmd := exec.CommandContext(ctx, name, args...) + cmd.WaitDelay = 5 * time.Second + var stderr strings.Builder + cmd.Stderr = &stderr + return runCommand(cmd, name, args, &stderr) +} diff --git a/internal/deps/refresh.go b/internal/deps/refresh.go index 5dcc50a..e3221e9 100644 --- a/internal/deps/refresh.go +++ b/internal/deps/refresh.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io/fs" + "net/url" "os" "strings" "time" @@ -13,11 +14,12 @@ import ( "github.com/stacklok/modelith/internal/provenance" ) -// ErrToolUnavailable marks a failure of gh itself rather than of the request: -// it is not installed, or it holds no usable credentials. Every file in a run -// would fail it identically, so a batch stops on it instead of repeating the -// same paragraph once per copy. -var ErrToolUnavailable = errors.New("gh is unavailable") +// ErrToolUnavailable marks a failure of the delegated CLI itself rather than of +// the request: it is not installed, or it holds no usable credentials. Every +// file in a run would fail it identically, so a batch stops on it instead of +// repeating the same paragraph once per copy. It is not GitHub-specific — a +// copy fetched through az fails it the same way. +var ErrToolUnavailable = errors.New("the delegated CLI is unavailable") // State is one vendored copy measured against its origin. // @@ -100,6 +102,10 @@ type CheckOptions struct { // Paths are the files to check. One that carries no provenance header is // skipped, because the glob a user passes to lint holds their own models too. Paths []string + // Timeout bounds each delegated command (gh, az) individually. Zero means + // no bound. A check that reaches many copies is exactly where a hung CLI + // would otherwise stall a whole CI run, so the bound belongs here too. + Timeout time.Duration // Run is the command seam; nil uses ExecRunner. Run Runner } @@ -113,6 +119,9 @@ type UpdateOptions struct { Ref string // Now stamps a refreshed header's imported date, in local time. Now time.Time + // Timeout bounds each delegated command (gh, az) individually. Zero means + // no bound. + Timeout time.Duration // Run is the command seam; nil uses ExecRunner. Run Runner } @@ -122,10 +131,10 @@ type UpdateOptions struct { // CI and against a read-only checkout. // // A per-file failure lands in that file's Report and the run continues. A -// non-nil error means the run stopped before it could measure a file because gh -// was unusable while fetching its content. +// non-nil error means the run stopped before it could measure a file because +// the delegated CLI (gh or az) was unusable while fetching its content. func Check(ctx context.Context, opts CheckOptions) ([]Report, error) { - return survey(ctx, surveyOptions{paths: opts.Paths, run: opts.Run}) + return survey(ctx, surveyOptions{paths: opts.Paths, timeout: opts.Timeout, run: opts.Run}) } // Update brings each vendored copy forward to what its origin serves now. @@ -140,15 +149,16 @@ func Update(ctx context.Context, opts UpdateOptions) ([]Report, error) { "--ref re-pins one copy and %d files were named — a single ref across several origins names a different version in each. Run it once per copy", len(opts.Paths)) } - return survey(ctx, surveyOptions{paths: opts.Paths, ref: opts.Ref, now: opts.Now, run: opts.Run, write: true}) + return survey(ctx, surveyOptions{paths: opts.Paths, ref: opts.Ref, now: opts.Now, timeout: opts.Timeout, run: opts.Run, write: true}) } type surveyOptions struct { - paths []string - ref string - now time.Time - run Runner - write bool + paths []string + ref string + now time.Time + timeout time.Duration + run Runner + write bool } func survey(ctx context.Context, opts surveyOptions) ([]Report, error) { @@ -156,6 +166,9 @@ func survey(ctx context.Context, opts surveyOptions) ([]Report, error) { if runner == nil { runner = ExecRunner{} } + if opts.timeout > 0 { + runner = timeoutRunner{inner: runner, timeout: opts.timeout} + } reports := make([]Report, 0, len(opts.paths)) for _, p := range opts.paths { rep, err := visit(ctx, runner, p, opts) @@ -213,8 +226,22 @@ func visit(ctx context.Context, runner Runner, path string, opts surveyOptions) rep.Err = err return rep, nil } + // An ADO ref whose type the header did not record, or one a --ref re-pin + // renamed, has to be resolved before the fetch: the API does not infer the + // type, so an untyped request is read as a branch and a tag- or + // commit-pinned copy would 404. + if err := resolveADORefType(ctx, runner, &src); err != nil { + // A refs lookup goes through the CLI too, so an absent or unauthenticated + // az has to stop the batch here exactly as it would on a fetch — the + // resolver would otherwise report it as one file's problem, once per copy. + if errors.Is(err, ErrToolUnavailable) { + return rep, err + } + rep.Err = err + return rep, nil + } - upstream, err := fetchContent(ctx, runner, src) + upstream, err := fetchContentFor(ctx, runner, src) if err != nil { if errors.Is(err, ErrToolUnavailable) { return rep, err @@ -235,7 +262,7 @@ func visit(ctx context.Context, runner Runner, path string, opts surveyOptions) // The commit is reporting, not verdict: resolve it only when the origin // actually moved, so a clean check costs one call per copy (ADR-0016). if st.Moved() { - commit, err := fetchCommit(ctx, runner, src) + commit, err := fetchCommitFor(ctx, runner, src) if err != nil { // Not rep.Err: this copy was reached and measured, and the // verdict that measurement produced is what check exists to @@ -275,7 +302,7 @@ func visit(ctx context.Context, runner Runner, path string, opts surveyOptions) } // A refresh: a new version, a new pin, or both. - commit, err := fetchCommit(ctx, runner, src) + commit, err := fetchCommitFor(ctx, runner, src) if err != nil { if errors.Is(err, ErrToolUnavailable) { return rep, err @@ -285,6 +312,11 @@ func visit(ctx context.Context, runner Runner, path string, opts surveyOptions) } next := *h next.Ref = ref + // The ref type is not copied from the old header: a repin changes what the + // ref names, and an untyped source may have had its type resolved by a refs + // lookup during this run. It is recorded from what src now names, and is + // empty for GitHub, which keeps its header shape. + next.RefType = recordedRefType(src) next.Commit = commit next.Imported = opts.now.Format("2006-01-02") next.Digest = provenance.Digest(upstream) @@ -306,12 +338,14 @@ func write(path string, content []byte, h *provenance.Header) error { // sourceFromHeader rebuilds the fetch address from what a header records. // -// It reassembles the blob URL and hands it back to ParseSource rather than -// picking the origin apart here, so a hand-written header gets the same -// treatment a typed one does: the host check, and the dot-segment rejection -// that escapePath depends on to keep an endpoint inside the repository's -// contents namespace. Passing ref explicitly is what lets a ref containing a -// slash split correctly, which is knowable here and is not from a URL alone. +// The host decides the shape: a github.com origin is reassembled into a blob URL +// and handed back to ParseSource, and a dev.azure.com origin into the typed ADO +// address adoSourceFromHeader builds. Going through ParseSource rather than +// picking the origin apart here means a hand-written header gets the same +// treatment a typed one does: the host check, and the dot-segment rejection that +// escapePath depends on to keep an endpoint inside the repository's contents +// namespace. Passing ref explicitly is what lets a ref containing a slash split +// correctly, which is knowable here and is not from a URL alone. // // The ref and path go back in escaped. What a header records is what ParseSource // decoded out of a URL, so handing them over raw would ask url.Parse to read a @@ -326,7 +360,62 @@ func sourceFromHeader(h *provenance.Header, ref string) (Source, error) { "modelith cannot refresh a copy fetched with %q — this build knows how to fetch %s", h.Fetch, strings.Join(provenance.Methods(), ", ")) } - return ParseSource(fmt.Sprintf("%s/blob/%s/%s", normOrigin(h.Origin), escapePath(ref), escapePath(h.Path)), ref) + switch originHost(h.Origin) { + case "github.com": + return ParseSource(fmt.Sprintf("%s/blob/%s/%s", normOrigin(h.Origin), escapePath(ref), escapePath(h.Path)), ref) + case "dev.azure.com": + return adoSourceFromHeader(h, ref) + default: + return Source{}, fmt.Errorf( + "cannot be refreshed: it was vendored from %q, and deps check and deps update reach github.com and dev.azure.com origins only. To take a newer version, import it again from its origin (`modelith deps import `) — that overwrites this copy with the origin's current file. If you need refresh for another host, please open an issue at %s", + h.Origin, issuesURL) + } +} + +// adoSourceFromHeader rebuilds an Azure DevOps fetch address from what a header +// records. The origin carries the organization, project, and repository; path +// and ref name the item and version; and ref-type — recorded at import — names +// the kind of ref, so a refresh asks for the same typed version the import did. +// The API does not infer a missing type (it reads one as a branch), so a header +// without the key leaves the source untyped for resolveADORefType to fill in. +func adoSourceFromHeader(h *provenance.Header, ref string) (Source, error) { + u, err := url.Parse(normOrigin(h.Origin)) + if err != nil { + return Source{}, fmt.Errorf("%q is not a URL: %w", h.Origin, err) + } + q := url.Values{} + q.Set("path", h.Path) + // The prefix encodes the ref's type. On a plain refresh the recorded type + // still describes the same ref, so it is reused. A --ref override, or a + // header written before the key existed, names a ref whose type is not + // recorded: the prefix is dropped and the type resolved by a refs lookup + // (resolveADORefType), because the API does not infer it — an untyped + // request is read as a branch, so a tag would fail as a branch of that name. + prefix := adoVersionPrefix(h.RefType) + if ref != h.Ref { + prefix = "" + } + q.Set("version", prefix+ref) + u.RawQuery = q.Encode() + // The ref type rides in the version prefix, so ParseSource is not asked to + // override the ref: an override would reset the type, which would discard + // what the header recorded. + return ParseSource(u.String(), "") +} + +// adoVersionPrefix maps a recorded ref-type to the URL's version prefix. An +// empty type yields no prefix, which leaves the source untyped for +// resolveADORefType to fill in — the API itself would read it as a branch. +func adoVersionPrefix(refType string) string { + switch refType { + case "branch": + return "GB" + case "tag": + return "GT" + case "commit": + return "GC" + } + return "" } // checkFetched applies to a refetch the same two refusals import applies to a diff --git a/internal/deps/refresh_test.go b/internal/deps/refresh_test.go index cadcaf7..fcc3658 100644 --- a/internal/deps/refresh_test.go +++ b/internal/deps/refresh_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "os" + "path/filepath" "strings" "testing" "time" @@ -83,6 +84,220 @@ enums: - name: bank-transfer ` +// vendoredFromADO writes a copy vendored from Azure DevOps — an origin this +// build's refresh path cannot reach (see sourceFromHeader) — and hands back its +// path. It is built by the real Import, like vendored, so the fixture is a copy +// a user could have rather than one hand-assembled to suit an assertion. +func vendoredFromADO(t *testing.T) string { + t.Helper() + r := adoRunner(adoContent, adoCommit) + res, err := Import(context.Background(), Options{ + URL: adoBlobURL, Dir: t.TempDir(), Now: importedAt, Run: r, + }) + if err != nil { + t.Fatalf("building the ADO fixture: %v", err) + } + return res.Path +} + +// TestRefresh_ADOCopyIsFirstClass pins that a copy vendored from Azure DevOps is +// checked and updated like a GitHub one. The header records the ref type, so +// refresh rebuilds the same typed request the import made — which is not +// equivalent to a refs lookup when a branch and a tag share a name. +func TestRefresh_ADOCopyIsFirstClass(t *testing.T) { + t.Parallel() + + t.Run("an unmoved origin is up to date", func(t *testing.T) { + t.Parallel() + path := vendoredFromADO(t) + rep := check(t, adoRunner(adoContent, adoCommit), path)[0] + if rep.Err != nil { + t.Fatalf("unexpected error: %v", rep.Err) + } + if rep.Stale() { + t.Error("reported stale against an origin serving the same bytes") + } + if rep.State.Ref != "main" { + t.Errorf("checked against ref %q, want the header's %q", rep.State.Ref, "main") + } + }) + + t.Run("a moved origin is stale and names the new commit", func(t *testing.T) { + t.Parallel() + path := vendoredFromADO(t) + rep := check(t, adoRunner(moved, laterSHA), path)[0] + if !rep.Stale() { + t.Fatal("reported up to date against an origin serving different bytes") + } + if rep.Commit != laterSHA { + t.Errorf("Commit = %q, want the origin's %q", rep.Commit, laterSHA) + } + }) + + t.Run("update brings the copy forward and keeps the ref type", func(t *testing.T) { + t.Parallel() + path := vendoredFromADO(t) + rep := update(t, adoRunner(moved, laterSHA), "", path)[0] + if !rep.Written { + t.Fatalf("update did not write the copy: %v", rep.Err) + } + after := readFile(t, path) + for _, want := range []string{ + "# modelith-ref-type: branch", + "# modelith-ref: main", + "# modelith-commit: " + laterSHA, + } { + if !strings.Contains(after, want) { + t.Errorf("the refreshed copy does not contain %q:\n%s", want, after) + } + } + }) + + t.Run("the request is typed from the header, not re-resolved", func(t *testing.T) { + t.Parallel() + path := vendoredFromADO(t) + r := adoRunner(adoContent, adoCommit) + check(t, r, path) + if len(r.calls) == 0 { + t.Fatal("the check made no az call") + } + if uri := azURI(r.calls[0]); !strings.Contains(uri, "versionDescriptor.versionType=branch") { + t.Errorf("refresh did not rebuild the typed request: %s", uri) + } + }) +} + +// azURI returns the --uri argument of an az rest call, or "" if there is none. +func azURI(call []string) string { + for i, a := range call { + if a == "--uri" && i+1 < len(call) { + return call[i+1] + } + } + return "" +} + +// vendoredOnHost writes a copy whose header names an arbitrary origin, so a test +// can exercise the refresh dispatch for a host this build has no transport for. +func vendoredOnHost(t *testing.T, origin, path, ref string) string { + t.Helper() + content := []byte(upstream) + h := &provenance.Header{ + Vendored: provenance.Banner, + Fetch: "git", + Origin: origin, + Path: path, + Ref: ref, + Commit: sha, + Imported: "2026-07-27", + Digest: provenance.Digest(content), + } + file := filepath.Join(t.TempDir(), "payments.modelith.yaml") + if err := os.WriteFile(file, provenance.Stamp(content, h), 0o644); err != nil { + t.Fatal(err) + } + return file +} + +// TestRefresh_RefusesAnUnknownHost pins that a copy from a host this build has no +// transport for is refused per file — naming the origin and the remedy — rather +// than being misread as a GitHub address and failing on something unrelated. +func TestRefresh_RefusesAnUnknownHost(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + run func(t *testing.T, r Runner, path string) Report + }{ + {"check", func(t *testing.T, r Runner, path string) Report { + return check(t, r, path)[0] + }}, + {"update", func(t *testing.T, r Runner, path string) Report { + return update(t, r, "", path)[0] + }}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + path := vendoredOnHost(t, "https://gitlab.com/acme/billing", "docs/payments.modelith.yaml", "main") + before := readFile(t, path) + + // A runner that would answer, and answer "moved", were the refusal + // absent: a check that proceeded would mark the copy stale, and an + // update would rewrite it. Neither may happen. + r := &fakeRunner{content: moved, sha: laterSHA} + rep := tc.run(t, r, path) + + if rep.Err == nil { + t.Fatal("a copy on an unreachable host was refreshed without an error") + } + for _, want := range []string{"gitlab.com", issuesURL} { + if !strings.Contains(rep.Err.Error(), want) { + t.Errorf("the refusal does not mention %q:\n%v", want, rep.Err) + } + } + if rep.State != nil { + t.Error("a copy this build cannot reach was measured anyway") + } + if got := readFile(t, path); got != before { + t.Error("the copy was rewritten despite the refusal") + } + if len(r.calls) != 0 { + t.Errorf("the refusal still called out %d time(s): %v", len(r.calls), r.calls) + } + }) + } +} + +// TestRefresh_AnUnknownHostDoesNotStopTheRun pins that the refusal above is a +// per-file Report and not a batch abort: a repository holding both an +// unknown-host copy and a GitHub one still gets a verdict on the GitHub copy. +func TestRefresh_AnUnknownHostDoesNotStopTheRun(t *testing.T) { + t.Parallel() + + ghPath, r := vendored(t, upstream) + otherPath := vendoredOnHost(t, "https://gitlab.com/acme/billing", "docs/payments.modelith.yaml", "main") + + reports := check(t, r, otherPath, ghPath) + if len(reports) != 2 { + t.Fatalf("got %d reports, want one per file", len(reports)) + } + if reports[0].Err == nil { + t.Error("the unknown-host copy was not refused") + } + if got := reports[1]; got.Err != nil { + t.Errorf("the GitHub copy was not judged: %v", got.Err) + } else if got.State == nil || got.Stale() { + t.Errorf("the GitHub copy got no clean verdict: %+v", got) + } +} + +// TestSurvey_ResolutionAbortsOnUnusableCLI pins that resolving a ref type goes +// through the same stop-the-batch rule as a fetch. The lookup needs the CLI, so +// an absent or unauthenticated az has to end the run rather than be reported +// once per copy as if each file were separately at fault. +func TestSurvey_ResolutionAbortsOnUnusableCLI(t *testing.T) { + t.Parallel() + + path := vendoredFromADO(t) + // Drop the key, so the check has to resolve the type before it can fetch. + without := strings.Replace(readFile(t, path), "# modelith-ref-type: branch\n", "", 1) + if err := os.WriteFile(path, []byte(without), 0o644); err != nil { + t.Fatal(err) + } + + r := &unusableRunner{} + reports, err := Check(context.Background(), CheckOptions{Paths: []string{path}, Run: r}) + if !errors.Is(err, ErrToolUnavailable) { + t.Fatalf("Check returned %v, want an ErrToolUnavailable from the refs lookup", err) + } + if len(reports) != 0 { + t.Errorf("got %d reports, want none — the run was abandoned before judging: %+v", len(reports), reports) + } + if r.calls != 1 { + t.Errorf("the run made %d calls after the CLI proved unusable, want 1", r.calls) + } +} + func TestCheck_ReportsWhetherTheOriginMoved(t *testing.T) { t.Parallel() @@ -255,10 +470,13 @@ func (u *unusableRunner) Run(context.Context, string, ...string) ([]byte, error) return nil, unusable{errors.New("gh is not installed — modelith delegates fetching to it")} } -// TestUnauthenticatedMatchesGhsOwnText pins the needles against the messages gh -// actually emits, quoted from its binary. gh reports this on stderr and exits 1, -// so there is no typed error to unwrap and nothing else to key on. -func TestUnauthenticatedMatchesGhsOwnText(t *testing.T) { +// TestUnauthenticatedMatchesTheClisOwnText pins the needles against the +// messages the delegated CLIs actually emit. Each reports this on stderr and +// exits non-zero, so there is no typed error to unwrap and nothing else to key +// on. az matters here as much as gh: a batch keys on ErrToolUnavailable to +// decide whether to stop, so an unclassified az would repeat the same +// paragraph once per copy. +func TestUnauthenticatedMatchesTheClisOwnText(t *testing.T) { t.Parallel() cases := []struct { @@ -266,12 +484,16 @@ func TestUnauthenticatedMatchesGhsOwnText(t *testing.T) { msg string want bool }{ - {"logged out entirely", "To get started with GitHub CLI, please run: gh auth login", true}, - {"a workflow with no token", "gh: To use GitHub CLI in a GitHub Actions workflow, set the GH_TOKEN environment variable.", true}, - {"automation with no token", "gh: To use GitHub CLI in automation, set the GH_TOKEN environment variable.", true}, - {"a token that is refused", "gh: Bad credentials (HTTP 401)", true}, - {"a file that is not there", "gh: HTTP 404: Not Found (https://api.github.com/repos/a/b/contents/c)", false}, - {"a repository that is private", "gh: HTTP 403: Forbidden", false}, + {"gh logged out entirely", "To get started with GitHub CLI, please run: gh auth login", true}, + {"gh workflow with no token", "gh: To use GitHub CLI in a GitHub Actions workflow, set the GH_TOKEN environment variable.", true}, + {"gh automation with no token", "gh: To use GitHub CLI in automation, set the GH_TOKEN environment variable.", true}, + {"gh token that is refused", "gh: Bad credentials (HTTP 401)", true}, + {"az with no session", "Please run 'az login' to setup account.", true}, + {"az with an expired session", "ERROR: AADSTS700082: The refresh token has expired due to inactivity.", true}, + {"az token that is refused", "az: Bad credentials (HTTP 401)", true}, + {"gh file that is not there", "gh: HTTP 404: Not Found (https://api.github.com/repos/a/b/contents/c)", false}, + {"gh repository that is private", "gh: HTTP 403: Forbidden", false}, + {"az resource that is not there", "az: 404 Not Found: the item does not exist", false}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -283,6 +505,218 @@ func TestUnauthenticatedMatchesGhsOwnText(t *testing.T) { } } +// TestSurvey_TimeoutBoundsEachDelegatedCall pins that deps check and deps +// update honour --timeout the way deps import does. Without it a stalled gh or +// az wedges a scheduled CI check indefinitely, which is the whole reason the +// bound exists on the import path. +func TestSurvey_TimeoutBoundsEachDelegatedCall(t *testing.T) { + t.Parallel() + + t.Run("check passes the bound through", func(t *testing.T) { + t.Parallel() + a, _ := vendored(t, upstream) + r := &hangRunner{} + start := time.Now() + reports, err := Check(context.Background(), CheckOptions{ + Paths: []string{a}, Timeout: 200 * time.Millisecond, Run: r, + }) + if err != nil { + t.Fatalf("Check aborted: %v", err) + } + if elapsed := time.Since(start); elapsed > 5*time.Second { + t.Errorf("the bound did not hold: took %s", elapsed) + } + if len(reports) != 1 || reports[0].Err == nil { + t.Fatalf("a hung CLI produced no per-file error: %+v", reports) + } + if !strings.Contains(reports[0].Err.Error(), "did not finish within") { + t.Errorf("the failure does not read as a timeout: %v", reports[0].Err) + } + if r.calls != 1 { + t.Errorf("the hung call was made %d times, want 1", r.calls) + } + }) + + t.Run("update passes the bound through", func(t *testing.T) { + t.Parallel() + a, _ := vendored(t, upstream) + r := &hangRunner{} + reports, err := Update(context.Background(), UpdateOptions{ + Paths: []string{a}, Timeout: 200 * time.Millisecond, Run: r, + }) + if err != nil { + t.Fatalf("Update aborted: %v", err) + } + if len(reports) != 1 || reports[0].Err == nil { + t.Fatalf("a hung CLI produced no per-file error: %+v", reports) + } + if !strings.Contains(reports[0].Err.Error(), "did not finish within") { + t.Errorf("the failure does not read as a timeout: %v", reports[0].Err) + } + }) + + t.Run("no bound means no decoration", func(t *testing.T) { + t.Parallel() + a, _ := vendored(t, upstream) + r := &fakeRunner{content: upstream, sha: sha} + if _, err := Check(context.Background(), CheckOptions{Paths: []string{a}, Run: r}); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) +} + +// hangRunner blocks until the context is done, then reports why — a stand-in +// for a CLI that never answers. +type hangRunner struct{ calls int } + +func (h *hangRunner) Run(ctx context.Context, _ string, _ ...string) ([]byte, error) { + h.calls++ + <-ctx.Done() + return nil, ctx.Err() +} + +// TestRefresh_ADORepinDropsTheRecordedType pins the bug this fix repairs: a +// copy imported as an ADO branch and then re-pinned to a tag must not carry the +// old type prefix. "GBv1.0.0" asks Azure DevOps for a branch of that name, so +// the request 404s and the copy cannot be re-pinned at all. The header must +// also stop claiming the ref is a branch. +// TestRefresh_ADORepinRetypesTheRef pins the repair of the repin bug and the +// reason the repair is a resolution rather than an omission. A branch-typed copy +// is re-pinned to a tag: the old type must not be reused (that asks ADO for a +// branch named after the tag), and the request must not simply drop the type +// either — an untyped ADO request is read as a branch, so it would fail the same +// way. The type has to be looked up and the header rewritten to match. +func TestRefresh_ADORepinRetypesTheRef(t *testing.T) { + t.Parallel() + + path := vendoredFromADO(t) + if got := readFile(t, path); !strings.Contains(got, "# modelith-ref-type: branch") { + t.Fatalf("the fixture is not a branch-typed ADO copy:\n%s", got) + } + + r := adoRunner(adoContent, adoCommit) + rep := update(t, r, "v1.0.0", path)[0] + if rep.Err != nil { + t.Fatalf("repin failed: %v", rep.Err) + } + + // Every fetch the repin made must name the resolved type. An untyped one + // would be read as a branch and 404, which the fake models. + var typed bool + for _, call := range r.calls { + uri := azURI(call) + switch { + case uri == "": + case strings.Contains(uri, "/refs?"): + case strings.Contains(uri, "versionType=branch"): + t.Errorf("the repin kept the old branch type onto a tag: %s", uri) + case strings.Contains(uri, "versionType=tag"): + typed = true + default: + t.Errorf("a repin fetch carried no versionType, which ADO reads as a branch: %s", uri) + } + } + if !typed { + t.Error("no repin fetch asked for the resolved tag type") + } + + after := readFile(t, path) + for _, want := range []string{"# modelith-ref: v1.0.0", "# modelith-ref-type: tag"} { + if !strings.Contains(after, want) { + t.Errorf("the re-pinned copy does not contain %q:\n%s", want, after) + } + } + if strings.Contains(after, "# modelith-ref-type: branch") { + t.Error("the header still claims the ref is a branch after a tag repin") + } +} + +// TestRefresh_ADORepinToACommitTypesItAsACommit covers the other repin shape: +// a value that is a git object id is resolved without a refs lookup and fetched +// as a commit. +func TestRefresh_ADORepinToACommitTypesItAsACommit(t *testing.T) { + t.Parallel() + + path := vendoredFromADO(t) + r := adoRunner(adoContent, adoCommit) + rep := update(t, r, adoCommit, path)[0] + if rep.Err != nil { + t.Fatalf("repin to a commit failed: %v", rep.Err) + } + var sawCommit bool + for _, call := range r.calls { + uri := azURI(call) + if strings.Contains(uri, "versionType=commit") { + sawCommit = true + } + if strings.Contains(uri, "/refs?") { + t.Errorf("a commit sha was resolved with a needless refs lookup: %s", uri) + } + } + if !sawCommit { + t.Error("the repin did not ask for a commit") + } + if after := readFile(t, path); !strings.Contains(after, "# modelith-ref-type: commit") { + t.Errorf("the header did not record the commit type:\n%s", after) + } +} + +// TestRefresh_ADOHeaderWithoutRefTypeResolvesIt pins the compatibility path: a +// copy imported before the ref-type key existed has no type recorded, so a +// refresh has to resolve it — and must not issue an untyped request, which ADO +// would read as a branch. +func TestRefresh_ADOHeaderWithoutRefTypeResolvesIt(t *testing.T) { + t.Parallel() + + path := vendoredFromADO(t) + // Drop the key, as a header written by an earlier build would. + without := strings.Replace(readFile(t, path), "# modelith-ref-type: branch\n", "", 1) + if err := os.WriteFile(path, []byte(without), 0o644); err != nil { + t.Fatal(err) + } + + r := adoRunner(adoContent, adoCommit) + rep := check(t, r, path)[0] + if rep.Err != nil { + t.Fatalf("check failed on a header with no ref-type: %v", rep.Err) + } + var typed bool + for _, call := range r.calls { + uri := azURI(call) + if strings.Contains(uri, "/refs?") { + continue + } + if !strings.Contains(uri, "versionType=") { + t.Errorf("the fetch was untyped, which ADO reads as a branch: %s", uri) + } + typed = true + } + if !typed { + t.Error("no typed fetch was made") + } +} + +// TestRefresh_ADOBareRefreshKeepsTheRecordedType pins the other half: a refresh +// that does not re-pin must keep asking for the same typed version the import +// did, because the API's default is not equivalent when a branch and a tag share a +// name. +func TestRefresh_ADOBareRefreshKeepsTheRecordedType(t *testing.T) { + t.Parallel() + + path := vendoredFromADO(t) + r := adoRunner(moved, laterSHA) + rep := update(t, r, "", path)[0] + if rep.Err != nil { + t.Fatalf("refresh failed: %v", rep.Err) + } + if uri := azURI(r.calls[0]); !strings.Contains(uri, "versionType=branch") { + t.Errorf("a bare refresh dropped the recorded type: %s", uri) + } + if after := readFile(t, path); !strings.Contains(after, "# modelith-ref-type: branch") { + t.Errorf("a bare refresh did not keep the recorded type:\n%s", after) + } +} + // TestADR_0016_ACurrentCopyIsNotRewritten pins the property that makes update // safe to run habitually over a glob: it produces a diff only where something // changed. It also pins what imported: means — a date that moved here would be @@ -589,6 +1023,7 @@ func TestSourceFromHeader_RoundTripsWhatParseSourceDecoded(t *testing.T) { t.Fatal(err) } want := Source{ + Host: HostGitHub, Origin: "https://github.com/acme/billing", Owner: "acme", Repo: "billing", Ref: tc.ref, Path: tc.path, } diff --git a/internal/provenance/provenance.go b/internal/provenance/provenance.go index d707444..59eef98 100644 --- a/internal/provenance/provenance.go +++ b/internal/provenance/provenance.go @@ -43,14 +43,29 @@ type Header struct { Origin string Path string Ref string + // RefType is the kind of ref Ref names, for a host whose API distinguishes + // them: "branch", "tag", or "commit". It is optional and omitted for a host, + // such as GitHub, whose API resolves an untyped ref on its own — so a header + // written before this key existed still parses, and a GitHub header keeps + // its shape. It is also omitted when the type is not known, which the layer + // that fetches then resolves; there is no value meaning "let the origin + // decide", because the Azure DevOps API does not. + RefType string Commit string Imported string Digest string } +// refTypes is the closed set a ref-type value may name. There is deliberately +// no "auto": the Azure DevOps API does not infer a version's type — an untyped +// request is read as a branch — so a value meaning "let the API decide" would +// be a claim the API does not honour. An unknown type is *omitted*, and the +// layer that fetches resolves it. +var refTypes = []string{"branch", "tag", "commit"} + // keyOrder is the order Format writes the keys in, and the set of keys that // exist at all: a line naming anything else is a Problem. -var keyOrder = []string{"vendored", "fetch", "origin", "path", "ref", "commit", "imported", "digest"} +var keyOrder = []string{"vendored", "fetch", "origin", "path", "ref", "ref-type", "commit", "imported", "digest"} // commonKeys are required whatever the fetch method is. methodKeys are the ones // each method requires on top, so adding a method means declaring what it @@ -86,6 +101,8 @@ func (h *Header) field(key string) *string { return &h.Path case "ref": return &h.Ref + case "ref-type": + return &h.RefType case "commit": return &h.Commit case "imported": @@ -204,6 +221,10 @@ func (h *Header) validate(seen map[string]int) []Problem { problems = append(problems, Problem{seen["digest"], fmt.Sprintf( "provenance digest %q is not in the form sha256:<64 hex digits>", h.Digest)}) } + if h.RefType != "" && !slices.Contains(refTypes, h.RefType) { + problems = append(problems, Problem{seen["ref-type"], fmt.Sprintf( + "provenance ref-type %q is not one of %s", h.RefType, quotedList(refTypes))}) + } return problems } diff --git a/internal/provenance/provenance_test.go b/internal/provenance/provenance_test.go index 794ba6e..2f9b799 100644 --- a/internal/provenance/provenance_test.go +++ b/internal/provenance/provenance_test.go @@ -15,6 +15,7 @@ const vendored = `# yaml-language-server: $schema=https://modelith.sh/schema/dom # modelith-origin: https://github.com/stacklok/some-repo # modelith-path: docs/payments.modelith.yaml # modelith-ref: main +# modelith-ref-type: branch # modelith-commit: 4f2c1e9c8b3ad0e5f71b2c9a6d4e8f30ab5c7d21 # modelith-imported: 2026-07-27 # modelith-digest: sha256:0000000000000000000000000000000000000000000000000000000000000000 @@ -67,6 +68,7 @@ func TestParse_Valid(t *testing.T) { Origin: "https://github.com/stacklok/some-repo", Path: "docs/payments.modelith.yaml", Ref: "main", + RefType: "branch", Commit: "4f2c1e9c8b3ad0e5f71b2c9a6d4e8f30ab5c7d21", Imported: "2026-07-27", Digest: "sha256:" + strings.Repeat("0", 64), @@ -100,6 +102,7 @@ func TestParse_Problems(t *testing.T) { "origin": "https://github.com/stacklok/some-repo", "path": "docs/payments.modelith.yaml", "ref": "main", + "ref-type": "branch", "commit": "4f2c1e9", "imported": "2026-07-27", "digest": "sha256:" + strings.Repeat("0", 64), @@ -149,9 +152,15 @@ func TestParse_Problems(t *testing.T) { { name: "a malformed digest names the shape", src: header(map[string]string{"digest": "sha256:nope"}), - wantLine: 9, + wantLine: 10, contains: "sha256:<64 hex digits>", }, + { + name: "a ref-type outside the closed set is reported", + src: header(map[string]string{"ref-type": "sausage"}), + wantLine: 7, + contains: `provenance ref-type "sausage" is not one of`, + }, { name: "a provenance line below the model content is misplaced", src: "# modelith-fetch: git\n" + plain + "# modelith-origin: https://github.com/stacklok/some-repo\n", diff --git a/project-docs/adr/0015-vendoring-is-a-whole-file-copy.md b/project-docs/adr/0015-vendoring-is-a-whole-file-copy.md index bb0ffe9..d981a56 100644 --- a/project-docs/adr/0015-vendoring-is-a-whole-file-copy.md +++ b/project-docs/adr/0015-vendoring-is-a-whole-file-copy.md @@ -5,6 +5,12 @@ and verified offline against a SHA-256 of its own bytes. Vendoring does not recurse, the fetch is `gh`-only, and the trust warning prints rather than blocks. Supersedes ADR-0010's **Digest** section; the rest of ADR-0010 stands. +> **Status: Accepted, partially superseded by [ADR-0019](0019-azure-devops-as-a-second-transport.md).** +> ADR-0019 makes Azure DevOps a second origin, which supersedes the +> **`gh` is the only transport** decision below. Everything else in this record +> stands unchanged; the superseded paragraph is kept as the record of what was +> decided at the time. + ## Context ADR-0010 specified vendoring in full before any of it was built. Implementing diff --git a/project-docs/adr/0019-azure-devops-as-a-second-transport.md b/project-docs/adr/0019-azure-devops-as-a-second-transport.md new file mode 100644 index 0000000..22e5913 --- /dev/null +++ b/project-docs/adr/0019-azure-devops-as-a-second-transport.md @@ -0,0 +1,119 @@ +# Azure DevOps is a second transport for vendoring + +Vendoring can fetch from Azure DevOps (`dev.azure.com`) as well as GitHub, +delegating each host to its own CLI — `gh api` for GitHub, `az rest` for Azure +DevOps — and `deps check` / `deps update` work for both origins. Supersedes +ADR-0015's **`gh` is the only transport** decision; every other ADR-0015 +decision stands. + +## Context + +ADR-0015 designated `gh` as the sole transport and made a non-GitHub origin an +error that asks the user to file an issue, on ADR-0007's bar that a second +transport waits for a real user. That user arrived: canonical domain models +hosted in private Azure DevOps Git repositories. ADR-0010 foresaw the slot — +`fetch: git` names what the origin *is*, not which binary fetched it — so no +header migration and no schema change are involved. + +The hard constraint is ADR-0011: the binary holds no HTTP client, no TLS +configuration, and no credential handling; network work is delegated to an +external CLI, run as an argv array with no shell. + +## Decision + +**Delegate to `az rest`, not to an HTTP client, and not to `curl`.** Where +GitHub hands off to `gh api`, Azure DevOps hands off to `az rest`. `az` +resolves its own token from the user's `az login` session and performs the +request internally, so the binary never sees a credential. An earlier revision +of this work read a token from `az account get-access-token` and passed it to +`curl`; it was reverted, because routing a credential through the binary's +argument list breaks the no-credential property ADR-0011 keeps, and no `curl` +invocation — argv, stdin, or config file — restores it. + +**The AAD audience is passed explicitly.** `az rest` cannot derive an Azure +DevOps audience from a `dev.azure.com` URL, so requests carry +`--resource 499b84ac-1321-427f-aa17-267ca6975798`, the well-known Microsoft +first-party application ID for Azure DevOps. That is a fact about the `az` CLI, +not configuration modelith invents. + +**The Items API is asked for bytes.** The content fetch passes `download=true`. +Without it the endpoint returns a JSON `GitItem` describing the item rather +than its content, which the model parser then rejects; with it the body is the +file. Content is written to a temp file with `--output-file` and read back, +because `az rest` appends a newline when it prints a body to stdout — one byte +that would move the copy's digest off canonical. The bytes then match the origin +by construction rather than by trusting a printer. + +**The header gains one optional key, `ref-type`, and an unknown type is +resolved rather than inferred.** An Azure DevOps version has a *type* — `GB` a +branch, `GT` a tag, `GC` a commit — and the API takes the type alongside the +value. The API does **not** infer a missing type: an untyped +`versionDescriptor.version` is read as a **branch**, so a copy pinned to a tag +or a commit cannot be fetched while the type is unset (verified against +api-version 7.1, 7.1-preview.1, and 7.2-preview.1: a 40-hex value with no type +fails with "The version descriptor could not be resolved"). So an +ADO import records `# modelith-ref-type:` as `branch`, `tag`, or `commit`; the +key is optional and *omitted* for GitHub, whose API resolves an untyped ref on +its own, so a GitHub header written before the key existed is byte-identical to +one written now and needs no migration. When the type is not named — a bare +`version=`, a `--ref` override, or a header from before the key existed — it is +**resolved before the fetch**: a git object id (40 or 64 hex) is a commit, and +anything else is looked up with the refs API under `refs/heads/` and then +`refs/tags/`. A branch is preferred on a name collision, because that is what +the API itself defaults to and a tag is reachable from the URL's `GT` form. An +unknown value is an error, like an unknown `fetch:` method, which matches +ADR-0015's closed-set posture pre-release. There is deliberately no value +meaning "let the origin decide": the origin does not decide. + +**Refresh is first-class for both hosts.** `deps check` and `deps update` +dispatch on the origin's host. A `github.com` origin is rebuilt into a blob URL +as before; a `dev.azure.com` origin is rebuilt into the typed ADO address from +`origin` (organization, project, repository), `path`, `ref`, and `ref-type`. The +content and commit fetchers dispatch the same way, so a copy from either host is +checked and updated rather than only imported. A `--ref` re-pin drops the +recorded type, which is then resolved afresh — a branch re-pinned to a tag must +not ask for a branch named after the tag, and an untyped request would be read +as one — and the header's `ref-type` is rewritten from what was resolved. A copy from a host with no transport is +still refused per file — a `Report`, not a run abort, so a mixed run still +judges its reachable copies — with an error naming the origin. + +**A delegated CLI failure stops a batch, whoever the CLI is.** An absent or +unauthenticated `gh` or `az` fails every copy in a run identically, so the +"unusable" classification keys on both CLIs' own text (`gh auth login`, +`GH_TOKEN`, `az login`, `AADSTS`, HTTP 401) rather than on GitHub's alone. + +**Process lifecycle is bounded on both platforms.** A delegated command that +spawns helpers inheriting its pipes can hold `Wait()` open past a context +deadline. On Unix the whole process group is killed (`Setpgid` plus a +negative-PID `SIGKILL`), with `cmd.WaitDelay` bounding the pipe drain if a +helper escapes the group; on Windows, which has no POSIX process groups, the +direct child is killed and `WaitDelay` still bounds the wait. A `--timeout` +decorator gives each delegated command its own deadline on `deps import`, +`deps check`, and `deps update`, and reports a fired deadline separately from a +caller's cancel (e.g. Ctrl+C). The Windows bound is soft by up to `WaitDelay` +(5s), which is an accepted platform cost rather than a documented flag +behaviour. + +## Consequences + +- `deps import`, `deps check`, and `deps update` accept `github.com` and + `dev.azure.com` origins. +- `az` is a runtime prerequisite for Azure DevOps origins, the way `gh` is for + GitHub; the "not installed" hint names the right CLI for the one that is + missing. +- No schema change. `ref-type` is a header comment, not schema, so + `TestSchemaStructSync` is not in play; `fetch: git` still records the same + origin URL shape (`https://dev.azure.com///_git/`), so an + ADO copy is verified offline against its digest exactly like a GitHub one. +- A legacy `*.visualstudio.com` URL is refused at parse time with a pointer to + its `dev.azure.com` address, since the host moved. +- Pinned by `TestImport_ADO_StampsAVerifiableCopy` and the rest of the + `TestImport_ADO_*` set (import end to end, the GB/GT/GC prefixes, the `--ref` + override, and `download=true` on the items request); + `TestRefresh_ADOCopyIsFirstClass`, `TestRefresh_ADORepinDropsTheRecordedType`, + and `TestRefresh_ADOBareRefreshKeepsTheRecordedType` (refresh dispatch, the + repin prefix, and the recorded type); `TestRefresh_RefusesAnUnknownHost` (a + host with no transport); `TestExecRunner_KillsTheWholeProcessGroup` and + `TestExecRunner_WaitDelayBoundsAPipeHeldByAStrayChild` (the process + lifecycle); and the `TestTimeoutRunner_*` and + `TestSurvey_TimeoutBoundsEachDelegatedCall` sets (the deadline).