From 34b38069b9bf0415f945b908244764f7ef67ad1f Mon Sep 17 00:00:00 2001 From: blevins darrin Date: Sat, 26 Sep 2026 20:12:57 +0800 Subject: [PATCH 1/8] feat(deps): add Azure DevOps support to deps import Vendoring could fetch only from github.com, because it delegated to `gh`. Canonical domain models also live in private Azure DevOps Git repositories, which is the live case ADR-0007's bar asked for before a second transport. `ParseSource` now reads a `dev.azure.com` blob URL alongside a github.com one: https://dev.azure.com///_git/?path=...&version=GB The GB/GT/GC version prefixes map to the API's branch/tag/commit version type, `--ref` overrides the URL's ref and lets the API auto-detect the type, a URL with no version is refused before a header can be stamped, and a legacy `*.visualstudio.com` URL is intercepted with a pointer to its `dev.azure.com` address. The fetch still delegates to an external CLI over an argv array with no shell, so modelith holds no HTTP client, TLS configuration, or credential handling (ADR-0011). Azure DevOps delegates to `az rest`, which resolves its own token from the user's `az login` session; requests carry the well-known Azure DevOps resource ID so `az` asks for the right audience, and content is written with `--output-file` because `az rest` appends a newline to a raw body on stdout, which would move the copy's digest off canonical. A delegated command that spawns helpers inheriting its pipes can hold Wait() open past a deadline, so on Unix the process group is killed and cmd.WaitDelay bounds the pipe drain; Windows, which has no POSIX process groups, kills the direct child under the same WaitDelay bound. A --timeout decorator gives each command its own deadline and reports a fired deadline separately from a caller's cancel. `deps check` and `deps update` still reach the origin through gh, so a copy vendored from Azure DevOps is refused up front with an error naming the host and the remedy rather than failing as a malformed URL. Recorded as an amendment to ADR-0015, which had named `gh` as the only transport. Refs #44. Signed-off-by: blevins darrin --- cmd/modelith/main.go | 55 +- cmd/modelith/main_test.go | 60 +- docs/10-vendoring.md | 32 +- internal/deps/deps.go | 386 ++++++-- internal/deps/deps_test.go | 866 +++++++++++++++++- internal/deps/exec_unix.go | 44 + internal/deps/exec_windows.go | 30 + internal/deps/refresh.go | 12 + internal/deps/refresh_test.go | 93 ++ .../0015-vendoring-is-a-whole-file-copy.md | 82 ++ 10 files changed, 1561 insertions(+), 99 deletions(-) create mode 100644 internal/deps/exec_unix.go create mode 100644 internal/deps/exec_windows.go diff --git a/cmd/modelith/main.go b/cmd/modelith/main.go index 43ee877..09c249d 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 } 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/10-vendoring.md b/docs/10-vendoring.md index 531fa74..9701366 100644 --- a/docs/10-vendoring.md +++ b/docs/10-vendoring.md @@ -195,6 +195,18 @@ that matched none of your copies does not read as good news. To find them: git grep -l '# modelith-vendored' ``` +:::note[Refresh reaches github.com only, for now] + +`deps check` and `deps update` fetch through `gh`, which speaks only GitHub, so a +copy vendored from Azure DevOps cannot be refreshed by this version. Both +commands report it against the copy's own line — naming the host and the +remedy — rather than trying and failing obscurely. To take a newer version of an +ADO copy in the meantime, import it again; that overwrites the copy with the +origin's current file. If you need refresh for another host, please +[open an issue](https://github.com/stacklok/modelith/issues). + +::: + ### Two ways to track a model Which one you are on is whatever `# modelith-ref:` records. @@ -271,15 +283,25 @@ 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 +- **`gh` or `az` must be installed and authenticated.** modelith implements no + network transport of its own; it delegates to the [GitHub + CLI](https://cli.github.com) or the [Azure + CLI](https://learn.microsoft.com/en-us/cli/azure/), which already solve + authentication for private and internal repositories. +- **Both github.com and dev.azure.com are supported.** The URL is the address of + the file as it appears in a browser. For Azure DevOps that means a `_git` URL + with `?path=...&version=GB` — open the file on dev.azure.com and copy + the address bar, exactly as for GitHub. If you need another host, please [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. +- **`deps check` and `deps update` are github.com only, for now.** They reach the + origin through `gh`, so an Azure DevOps copy can be imported and linted but + not refreshed; re-import it to take a newer version. Both commands say so in + the copy's own line rather than failing obscurely, and refresh for another + host is follow-up work — [open an + issue](https://github.com/stacklok/modelith/issues) if you need it. - **`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. diff --git a/internal/deps/deps.go b/internal/deps/deps.go index a1ca8c4..e585bd2 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,6 +62,52 @@ func (ExecRunner) Run(ctx context.Context, name string, args ...string) ([]byte, return out, nil } +// 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 gh refused for want of credentials rather // than because of anything about the request. // @@ -94,27 +137,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 +165,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 +217,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 = "" // let the ADO API auto-detect the ref type + } + 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 +295,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 +303,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 +342,35 @@ 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} + } - content, err := fetchContent(ctx, runner, src) - if err != nil { - return nil, fmt.Errorf("%w%s", err, splitHint(src, err)) + var content []byte + var commit string + + if src.Host == HostADO { + // ADO fetch delegates to the az CLI. + content, err = fetchContentADO(ctx, runner, src) + if err != nil { + return nil, fmt.Errorf("fetching content: %w%s", err, splitHint(src, err)) + } + commit, err = fetchCommitADO(ctx, runner, src) + if err != nil { + return nil, fmt.Errorf("fetching commit: %w", err) + } + } else { + // GitHub fetch delegates to the gh CLI. + content, err = fetchContent(ctx, runner, src) + if err != nil { + return nil, fmt.Errorf("fetching content: %w%s", err, splitHint(src, err)) + } + commit, err = fetchCommit(ctx, runner, src) + if err != nil { + 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,11 +390,6 @@ 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", @@ -327,17 +467,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,11 +487,7 @@ 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") @@ -397,6 +534,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 +566,85 @@ 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 accepts an empty versionType and auto-detects the ref. +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. +// +// The body is written to a temporary file with --output-file and read back +// rather than taken from stdout: az rest appends a newline when it prints a +// raw 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 +// (the ADR-0015 amendment on the Azure DevOps transport). The --output-file form +// is the exact API response body. +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&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..e73d1ef 100644 --- a/internal/deps/deps_test.go +++ b/internal/deps/deps_test.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "strings" "testing" "time" @@ -12,11 +13,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 +24,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 +33,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 +62,63 @@ 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, "/items"): + // Validate that a known version prefix produces the right versionType. + // GB→branch, GT→tag, GC→commit. When the prefix is absent or unknown, + // versionType is omitted (the API auto-detects). + if strings.Contains(uri, "versionType") { + if !strings.Contains(uri, "versionType=branch") && + !strings.Contains(uri, "versionType=tag") && + !strings.Contains(uri, "versionType=commit") { + return nil, fmt.Errorf("az: unexpected versionType in %q", uri) + } + } + // The real fetch writes the body to --output-file (az rest appends a + // newline when printing a raw 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) + } + if err := os.WriteFile(outPath, []byte(f.content), 0o644); err != nil { + return nil, err + } + return []byte(f.content), 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)") + } + return []byte(f.sha + "\n"), nil + } + return nil, fmt.Errorf("az: unexpected uri %q", uri) +} + const upstream = `# yaml-language-server: $schema=https://modelith.sh/schema/domain-model/v1.json kind: DomainModel version: v1 @@ -80,6 +147,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 +156,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 +166,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 +178,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 +189,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", }, @@ -483,7 +555,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 +568,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 +690,783 @@ 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 resets RefType so the API auto-detects", + 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 uses the auto-detect path", + 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", + 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++ + } + 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 API call omits versionType so ADO auto-detects. +func TestImport_ADO_OverrideRefOmitsVersionType(t *testing.T) { + t.Parallel() + + r := adoRunner(adoContent, adoCommit) + // URL has GBmain (branch), but --ref overrides to a tag-like value. + url := adoBlobURL + _, err := Import(context.Background(), Options{ + URL: url, + Dir: t.TempDir(), + Ref: "v1.0.0", + Now: time.Date(2026, 7, 27, 12, 0, 0, 0, time.Local), + Run: r, + }) + if 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, "versionType") { + t.Errorf("--ref override should omit versionType, got %q", uri) + } + if !strings.Contains(uri, "version=v1.0.0") { + t.Errorf("--ref override should use the override value in version=, got %q", uri) + } + } + } + } +} + +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..e714d75 100644 --- a/internal/deps/refresh.go +++ b/internal/deps/refresh.go @@ -326,6 +326,18 @@ 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(), ", ")) } + // A copy vendored from another host cannot be refreshed by this build: check + // and update both reach the origin through gh, which speaks only GitHub, and + // the address rebuilt below is a GitHub shape ("//blob//"). + // Refusing here, where the address is assembled, keeps the error about the + // gap rather than about a URL the user never wrote — the GitHub-shaped URL + // handed to ParseSource would instead fail as an ADO URL missing its ?path= + // query, which reads as a mistake in the header. + if host := originHost(h.Origin); host != "" && host != "github.com" { + return Source{}, fmt.Errorf( + "cannot be refreshed: it was vendored from %s, and deps check and deps update fetch through gh, which speaks only GitHub. To take a newer version, import it again from its origin (`modelith deps import `) — that overwrites this copy with the origin's current file. Refresh for hosts other than github.com is not written yet; if you need it, please open an issue at %s", + host, issuesURL) + } return ParseSource(fmt.Sprintf("%s/blob/%s/%s", normOrigin(h.Origin), escapePath(ref), escapePath(h.Path)), ref) } diff --git a/internal/deps/refresh_test.go b/internal/deps/refresh_test.go index cadcaf7..1103acd 100644 --- a/internal/deps/refresh_test.go +++ b/internal/deps/refresh_test.go @@ -83,6 +83,98 @@ 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_RefusesAnOriginItCannotReach pins the ADO refresh limit: deps +// check and deps update reach the origin through gh, which speaks only GitHub, +// so a copy vendored from Azure DevOps cannot be refreshed. The refusal has to +// be a per-file Report that names the host and points at the remedy, has to +// measure nothing and write nothing, and must not call out at all — the origin +// is on a host this build has no transport for. +func TestRefresh_RefusesAnOriginItCannotReach(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 := vendoredFromADO(t) + 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("an ADO copy was refreshed without an error") + } + for _, want := range []string{"dev.azure.com", "gh", 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_ADORefusalDoesNotStopTheRun pins that the refusal above is a +// per-file Report and not a batch abort: a repository holding both an ADO copy +// and a GitHub one still gets a verdict on the GitHub copy, which is the whole +// reason the gap is reported per file rather than raised as a run error. +func TestRefresh_ADORefusalDoesNotStopTheRun(t *testing.T) { + t.Parallel() + + ghPath, r := vendored(t, upstream) + adoPath := vendoredFromADO(t) + + reports := check(t, r, adoPath, ghPath) + if len(reports) != 2 { + t.Fatalf("got %d reports, want one per file", len(reports)) + } + if reports[0].Err == nil { + t.Error("the ADO 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) + } +} + func TestCheck_ReportsWhetherTheOriginMoved(t *testing.T) { t.Parallel() @@ -589,6 +681,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/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..f17e925 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,10 @@ 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. +> **Amended 2026-09-26.** The **`gh` is the only transport** decision below is +> superseded — Azure DevOps is now a second origin for `deps import`. Everything +> else in this record stands unchanged; see the Amendment at the end. + ## Context ADR-0010 specified vendoring in full before any of it was built. Implementing @@ -143,3 +147,81 @@ step is the real gate. case for that rule, since it is where someone else's prose lands in your published document, but it shares no code with this change and is a visibly broken document rather than a privilege boundary. + +## Amendment — Azure DevOps as a second transport (2026-09-26) + +This amendment supersedes the **`gh` is the only transport** decision above. +Nothing else in this record changes: the content digest, the header shape, the +suppression rules, the non-recursive fetch, and the print-don't-block warning +all stand. + +ADR-0007 set the bar for a second transport at "a real user exists". That user +arrived: canonical domain models hosted in private Azure DevOps Git +repositories (`dev.azure.com`). `deps import` now accepts a `dev.azure.com` blob +URL alongside a `github.com` one. + +**Delegation, not a transport.** The fetch still hands off to an external CLI, +executed as an argv array with no shell, so `modelith` acquires no HTTP client, +no TLS configuration, and no credential handling (ADR-0011). Where GitHub +delegates to `gh api`, Azure DevOps delegates to `az rest`. Authentication is +whatever the user's own CLI session is — `gh auth login`, `az login` — and the +binary never sees a token. An earlier revision of this work fetched through +`curl` with a token read from `az account get-access-token`; 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 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. + +**Byte fidelity is a fetch-path concern.** `az rest` appends a newline when it +prints a raw body to stdout, which by itself moves the copy's digest off +canonical. Content is therefore written to a temp file with `--output-file` and +read back, so the vendored bytes match the origin by construction rather than by +trusting a printer. `gh` needs no such step; it returns the raw body unchanged. + +**The header keeps its shape.** `fetch: git` still names what the origin *is*, +and the recorded `origin` is the repository URL +(`https://dev.azure.com///_git/`), so no header migration is +needed and 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 the `dev.azure.com` address, since the host moved. + +**Process lifecycle is bounded.** 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, and the resulting message distinguishes a fired +deadline from a caller's cancel (e.g. Ctrl+C) rather than reporting both as a +timeout. + +**Scope: import today, refresh later.** This amendment covers `deps import`. +`deps check` and `deps update` reach the origin through `gh` and rebuild a +GitHub-shaped address from the header, so a copy vendored from Azure DevOps +cannot yet be refreshed; such a copy is refused up front, with an error naming +the host and the remedy, rather than failing as a malformed URL. Extending +refresh to a second host means rebuilding an ADO address from what a header +records — which does not include the ADO `project` or the `version=` prefix — +so it is recorded here as follow-up work, not as a claim this amendment makes. + +Pinned by `TestImport_ADO_StampsAVerifiableCopy` and the rest of the +`TestImport_ADO_*` set (import end to end, the ref-type prefixes, and the +`--ref` override), `TestExecRunner_KillsTheWholeProcessGroup` and +`TestExecRunner_WaitDelayBoundsAPipeHeldByAStrayChild` (the process lifecycle), +the `TestTimeoutRunner_*` set (the deadline message), and +`TestRefresh_RefusesAnOriginItCannotReach` (the refresh scope limit). + +### Amendment consequences + +- `deps import` accepts `github.com` and `dev.azure.com` URLs. +- `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. +- Offline `lint` verifies an Azure DevOps copy against its digest identically to + a GitHub one. +- `deps check` and `deps update` remain `github.com`-only and refuse an Azure + DevOps copy with an actionable error. From 589318afd68c78a8242181bb4cdbeca967f184d3 Mon Sep 17 00:00:00 2001 From: Joe Beda Date: Sat, 26 Sep 2026 12:23:26 -0700 Subject: [PATCH 2/8] docs: document Azure DevOps vendoring Signed-off-by: Joe Beda --- docs/07-cli.md | 41 +++++++++++++++++++--------------- docs/10-vendoring.md | 53 +++++++++++++++++++------------------------- 2 files changed, 46 insertions(+), 48 deletions(-) diff --git a/docs/07-cli.md b/docs/07-cli.md index 8f220f7..eb47b4d 100644 --- a/docs/07-cli.md +++ b/docs/07-cli.md @@ -99,34 +99,39 @@ 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. +Checks GitHub-origin 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. This version cannot refresh Azure DevOps copies; +import the file again from its browser URL to replace one. ```sh modelith deps check docs/*.modelith.yaml @@ -134,9 +139,9 @@ 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 GitHub-origin 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. Import an Azure DevOps copy again to replace it. ```sh modelith deps update docs/*.modelith.yaml diff --git a/docs/10-vendoring.md b/docs/10-vendoring.md index 9701366..ee9688e 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 @@ -195,15 +203,11 @@ that matched none of your copies does not read as good news. To find them: git grep -l '# modelith-vendored' ``` -:::note[Refresh reaches github.com only, for now] +:::note[Refresh reaches GitHub only] -`deps check` and `deps update` fetch through `gh`, which speaks only GitHub, so a -copy vendored from Azure DevOps cannot be refreshed by this version. Both -commands report it against the copy's own line — naming the host and the -remedy — rather than trying and failing obscurely. To take a newer version of an -ADO copy in the meantime, import it again; that overwrites the copy with the -origin's current file. If you need refresh for another host, please -[open an issue](https://github.com/stacklok/modelith/issues). +`deps check` and `deps update` use `gh`, so they cannot refresh a copy imported +from Azure DevOps. To take a newer version, import the file again from its +Azure DevOps browser URL. The import replaces the existing copy. ::: @@ -283,25 +287,14 @@ already solves. ## Requirements and limits -- **`gh` or `az` must be installed and authenticated.** modelith implements no - network transport of its own; it delegates to the [GitHub - CLI](https://cli.github.com) or the [Azure - CLI](https://learn.microsoft.com/en-us/cli/azure/), which already solve - authentication for private and internal repositories. -- **Both github.com and dev.azure.com are supported.** The URL is the address of - the file as it appears in a browser. For Azure DevOps that means a `_git` URL - with `?path=...&version=GB` — open the file on dev.azure.com and copy - the address bar, exactly as for GitHub. If you need another host, please - [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. -- **`deps check` and `deps update` are github.com only, for now.** They reach the - origin through `gh`, so an Azure DevOps copy can be imported and linted but - not refreshed; re-import it to take a newer version. Both commands say so in - the copy's own line rather than failing obscurely, and refresh for another - host is follow-up work — [open an - issue](https://github.com/stacklok/modelith/issues) if you need it. +- **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 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. From 7f28d9c635571b5a7db6868f19a6954d0e32f6f7 Mon Sep 17 00:00:00 2001 From: blevins darrin Date: Sun, 27 Sep 2026 05:29:29 +0800 Subject: [PATCH 3/8] fix(deps): fetch ADO bytes with download=true and make refresh first-class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the review on the pull request, and closes the import-only scope the earlier revision recorded as follow-up. download=true: fetchContentADO built the Items API request without it, and the endpoint defaults it to false — so the body was a JSON GitItem describing the item, which the model parser then rejected. The fake runner now mirrors that contract (returning the metadata body unless download=true is present), so the omission is a test failure rather than a surprise against a real endpoint, and the endpoint test asserts the parameter. refresh: sourceFromHeader now dispatches on the origin host, rebuilding a dev.azure.com origin into a typed ADO address and a github.com one into a blob URL as before; fetchContent/fetchCommit became fetchContentFor/fetchCommitFor, which the refresh path shares with Import. A copy from a host with no transport is still refused per file, naming the origin. ref-type: the header gains an optional modelith-ref-type key. An ADO version has a type — GB a branch, GT a tag, GC a commit — and letting the API infer it is not equivalent when a branch and a tag share a name. Import records branch, tag, or commit, or auto for an unprefixed URL or a --ref override. The key is omitted for GitHub, so a GitHub header written earlier is byte-identical to one written now, and provenance validates the value against the closed set. Docs and the ADR-0015 amendment are updated accordingly, and fold in the maintainer's docs commit. Signed-off-by: blevins darrin --- docs/07-cli.md | 13 +- docs/10-vendoring.md | 16 ++- internal/deps/deps.go | 87 ++++++++---- internal/deps/deps_test.go | 20 ++- internal/deps/refresh.go | 75 +++++++--- internal/deps/refresh_test.go | 132 +++++++++++++++--- internal/provenance/provenance.go | 17 ++- internal/provenance/provenance_test.go | 11 +- .../0015-vendoring-is-a-whole-file-copy.md | 61 +++++--- 9 files changed, 326 insertions(+), 106 deletions(-) diff --git a/docs/07-cli.md b/docs/07-cli.md index eb47b4d..97ae533 100644 --- a/docs/07-cli.md +++ b/docs/07-cli.md @@ -128,10 +128,9 @@ modelith deps import "https://dev.azure.com/acme/billing/_git/models?path=docs/p ### `modelith deps check ...` -Checks GitHub-origin 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. This version cannot refresh Azure DevOps copies; -import the file again from its browser URL to replace one. +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. Copies from github.com and dev.azure.com are both checked. ```sh modelith deps check docs/*.modelith.yaml @@ -139,9 +138,9 @@ modelith deps check docs/*.modelith.yaml ### `modelith deps update [--ref ] ...` -Updates GitHub-origin 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. Import an Azure DevOps copy again to replace it. +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. ```sh modelith deps update docs/*.modelith.yaml diff --git a/docs/10-vendoring.md b/docs/10-vendoring.md index ee9688e..3247ca4 100644 --- a/docs/10-vendoring.md +++ b/docs/10-vendoring.md @@ -51,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`, `commit`, or `auto` — so a refresh rebuilds the same typed request. Omitted for GitHub, whose API resolves an untyped ref. | | `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. | @@ -203,11 +204,15 @@ that matched none of your copies does not read as good news. To find them: git grep -l '# modelith-vendored' ``` -:::note[Refresh reaches GitHub only] +:::note[Azure DevOps copies record how pinned they are] -`deps check` and `deps update` use `gh`, so they cannot refresh a copy imported -from Azure DevOps. To take a newer version, import the file again from its -Azure DevOps browser URL. The import replaces the existing copy. +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 rather than letting the +API infer one. An unprefixed URL, or a `--ref` override, records `auto`, which +is what leaves the inference to the API. GitHub has no such key: its API +resolves an untyped ref on its own. ::: @@ -291,7 +296,8 @@ already solves. 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 from github.com or dev.azure.com.** An Azure DevOps URL has the form +- **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). diff --git a/internal/deps/deps.go b/internal/deps/deps.go index e585bd2..be02ed2 100644 --- a/internal/deps/deps.go +++ b/internal/deps/deps.go @@ -346,29 +346,16 @@ func Import(ctx context.Context, opts Options) (*Result, error) { runner = timeoutRunner{inner: runner, timeout: opts.Timeout} } - var content []byte - var commit string - - if src.Host == HostADO { - // ADO fetch delegates to the az CLI. - content, err = fetchContentADO(ctx, runner, src) - if err != nil { - return nil, fmt.Errorf("fetching content: %w%s", err, splitHint(src, err)) - } - commit, err = fetchCommitADO(ctx, runner, src) - if err != nil { - return nil, fmt.Errorf("fetching commit: %w", err) - } - } else { - // GitHub fetch delegates to the gh CLI. - content, err = fetchContent(ctx, runner, src) - if err != nil { - return nil, fmt.Errorf("fetching content: %w%s", err, splitHint(src, err)) - } - commit, err = fetchCommit(ctx, runner, src) - if err != nil { - return nil, fmt.Errorf("fetching commit: %w", err) - } + // 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("fetching commit: %w", err) } if provenance.Present(content) { @@ -396,6 +383,7 @@ func Import(ctx context.Context, opts Options) (*Result, error) { 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), @@ -493,6 +481,41 @@ func isNotFound(err error) bool { 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: the ADO +// version type when the URL's GB/GT/GC prefix named one, "auto" when the ADO +// API is left to infer it (an unprefixed version=, or a --ref override), and +// empty for GitHub — whose API resolves an untyped ref on its own, and whose +// headers predate this key. Recording it is what lets a later refresh rebuild +// the same typed request, which is not equivalent to auto-detection when a +// branch and a tag share a name. +func recordedRefType(src Source) string { + if src.Host != HostADO { + return "" + } + if vt := adoVersionType(src); vt != "" { + return vt + } + return "auto" +} + // 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. @@ -588,15 +611,19 @@ func adoVersionType(src Source) string { // fetchContentADO fetches the file content from Azure DevOps by delegating to // `az rest`. Auth is handled by the az CLI. // -// The body is written to a temporary file with --output-file and read back -// rather than taken from stdout: az rest appends a newline when it prints a -// raw 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 -// (the ADR-0015 amendment on the Azure DevOps transport). The --output-file form -// is the exact API response body. +// 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 (the ADR-0015 amendment on the +// Azure DevOps transport). 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&api-version=7.1", + "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)) diff --git a/internal/deps/deps_test.go b/internal/deps/deps_test.go index e73d1ef..1ed4293 100644 --- a/internal/deps/deps_test.go +++ b/internal/deps/deps_test.go @@ -92,7 +92,7 @@ func (f *fakeRunner) runAz(args []string) ([]byte, error) { } } // The real fetch writes the body to --output-file (az rest appends a - // newline when printing a raw body to stdout, which would drift the + // 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. @@ -105,10 +105,18 @@ func (f *fakeRunner) runAz(args []string) ([]byte, error) { if outPath == "" { return nil, fmt.Errorf("az: content fetch must use --output-file, got %q", args) } - if err := os.WriteFile(outPath, []byte(f.content), 0o644); err != nil { + // 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 []byte(f.content), nil + return body, nil case strings.Contains(uri, "/commits"): // The commit endpoint must not shell-escape $top. if strings.Contains(uri, "\\$top") { @@ -952,6 +960,7 @@ func TestImport_ADO_StampsAVerifiableCopy(t *testing.T) { 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)), @@ -992,6 +1001,11 @@ func TestImport_ADO_CallsAzWithTheExpectedEndpoints(t *testing.T) { 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++ diff --git a/internal/deps/refresh.go b/internal/deps/refresh.go index e714d75..676290d 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" @@ -214,7 +215,7 @@ func visit(ctx context.Context, runner Runner, path string, opts surveyOptions) 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 +236,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 +276,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 @@ -306,12 +307,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,19 +329,51 @@ 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(), ", ")) } - // A copy vendored from another host cannot be refreshed by this build: check - // and update both reach the origin through gh, which speaks only GitHub, and - // the address rebuilt below is a GitHub shape ("//blob//"). - // Refusing here, where the address is assembled, keeps the error about the - // gap rather than about a URL the user never wrote — the GitHub-shaped URL - // handed to ParseSource would instead fail as an ADO URL missing its ?path= - // query, which reads as a mistake in the header. - if host := originHost(h.Origin); host != "" && host != "github.com" { + 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 %s, and deps check and deps update fetch through gh, which speaks only GitHub. To take a newer version, import it again from its origin (`modelith deps import `) — that overwrites this copy with the origin's current file. Refresh for hosts other than github.com is not written yet; if you need it, please open an issue at %s", - host, issuesURL) + "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) } - return ParseSource(fmt.Sprintf("%s/blob/%s/%s", normOrigin(h.Origin), escapePath(ref), escapePath(h.Path)), ref) +} + +// 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 +// rather than letting the API infer one, which a branch and a tag sharing a name +// would answer differently. +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) + q.Set("version", adoVersionPrefix(h.RefType)+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 to auto-detect, which + // would discard what the header recorded. + return ParseSource(u.String(), "") +} + +// adoVersionPrefix maps a recorded ref-type to the URL's version prefix, and an +// empty or "auto" type to none — which is what leaves the ADO API to infer it. +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 1103acd..12820d5 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" @@ -99,13 +100,109 @@ func vendoredFromADO(t *testing.T) string { return res.Path } -// TestRefresh_RefusesAnOriginItCannotReach pins the ADO refresh limit: deps -// check and deps update reach the origin through gh, which speaks only GitHub, -// so a copy vendored from Azure DevOps cannot be refreshed. The refusal has to -// be a per-file Report that names the host and points at the remedy, has to -// measure nothing and write nothing, and must not call out at all — the origin -// is on a host this build has no transport for. -func TestRefresh_RefusesAnOriginItCannotReach(t *testing.T) { +// 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 auto-detection 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 auto-detected", 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 { @@ -121,7 +218,7 @@ func TestRefresh_RefusesAnOriginItCannotReach(t *testing.T) { } { t.Run(tc.name, func(t *testing.T) { t.Parallel() - path := vendoredFromADO(t) + 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 @@ -131,9 +228,9 @@ func TestRefresh_RefusesAnOriginItCannotReach(t *testing.T) { rep := tc.run(t, r, path) if rep.Err == nil { - t.Fatal("an ADO copy was refreshed without an error") + t.Fatal("a copy on an unreachable host was refreshed without an error") } - for _, want := range []string{"dev.azure.com", "gh", issuesURL} { + 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) } @@ -151,22 +248,21 @@ func TestRefresh_RefusesAnOriginItCannotReach(t *testing.T) { } } -// TestRefresh_ADORefusalDoesNotStopTheRun pins that the refusal above is a -// per-file Report and not a batch abort: a repository holding both an ADO copy -// and a GitHub one still gets a verdict on the GitHub copy, which is the whole -// reason the gap is reported per file rather than raised as a run error. -func TestRefresh_ADORefusalDoesNotStopTheRun(t *testing.T) { +// 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) - adoPath := vendoredFromADO(t) + otherPath := vendoredOnHost(t, "https://gitlab.com/acme/billing", "docs/payments.modelith.yaml", "main") - reports := check(t, r, adoPath, ghPath) + 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 ADO copy was not refused") + 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) diff --git a/internal/provenance/provenance.go b/internal/provenance/provenance.go index d707444..ae8baae 100644 --- a/internal/provenance/provenance.go +++ b/internal/provenance/provenance.go @@ -43,14 +43,23 @@ 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", or "auto" when the origin's own API is + // left to infer it. 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. + RefType string Commit string Imported string Digest string } +// refTypes is the closed set a ref-type value may name. +var refTypes = []string{"branch", "tag", "commit", "auto"} + // 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 +95,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 +215,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 f17e925..36ea0e3 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 @@ -182,12 +182,11 @@ canonical. Content is therefore written to a temp file with `--output-file` and read back, so the vendored bytes match the origin by construction rather than by trusting a printer. `gh` needs no such step; it returns the raw body unchanged. -**The header keeps its shape.** `fetch: git` still names what the origin *is*, -and the recorded `origin` is the repository URL -(`https://dev.azure.com///_git/`), so no header migration is -needed and 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 the `dev.azure.com` address, since the host moved. +**`fetch: git` still names what the origin is.** The recorded `origin` is the +repository URL (`https://dev.azure.com///_git/`), so an ADO +copy is verified offline against its digest exactly like a GitHub one, and a +legacy `*.visualstudio.com` URL is refused at parse time with a pointer to the +`dev.azure.com` address, since the host moved. **Process lifecycle is bounded.** A delegated command that spawns helpers inheriting its pipes can hold `Wait()` open past a context deadline. On Unix the @@ -199,29 +198,49 @@ command its own deadline, and the resulting message distinguishes a fired deadline from a caller's cancel (e.g. Ctrl+C) rather than reporting both as a timeout. -**Scope: import today, refresh later.** This amendment covers `deps import`. -`deps check` and `deps update` reach the origin through `gh` and rebuild a -GitHub-shaped address from the header, so a copy vendored from Azure DevOps -cannot yet be refreshed; such a copy is refused up front, with an error naming -the host and the remedy, rather than failing as a malformed URL. Extending -refresh to a second host means rebuilding an ADO address from what a header -records — which does not include the ADO `project` or the `version=` prefix — -so it is recorded here as follow-up work, not as a claim this amendment makes. +**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, +and `--output-file` writes those bytes verbatim. + +**The header gains one optional key, `ref-type`.** An Azure DevOps version has a +*type* — `GB` a branch, `GT` a tag, `GC` a commit — and `az`'s API takes the type +alongside the value. Letting the API infer it is not equivalent: a branch and a +tag may share a name, and the two answer differently. So an ADO import records +`# modelith-ref-type:` as `branch`, `tag`, or `commit`, or `auto` when the URL +left the type unprefixed or `--ref` overrode it — the case where inference is +what was asked for. The key is optional and *omitted* for GitHub, whose API +resolves an untyped ref on its own: a header written before the key existed +still parses, and no GitHub header changes shape. + +**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, using +the `origin` (organization, project, repository), `path`, `ref`, and the new +`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 copy from a host +this build has no transport for is still refused per file — as a `Report`, so a +run that also holds reachable copies still judges them — with an error naming the +origin rather than failing as a malformed URL. Pinned by `TestImport_ADO_StampsAVerifiableCopy` and the rest of the -`TestImport_ADO_*` set (import end to end, the ref-type prefixes, and the -`--ref` override), `TestExecRunner_KillsTheWholeProcessGroup` and +`TestImport_ADO_*` set (import end to end, the ref-type prefixes, the `--ref` +override, and `download=true` on the items request), +`TestRefresh_ADOCopyIsFirstClass` (check and update dispatch to `az rest` and +keep the recorded ref type), `TestRefresh_RefusesAnUnknownHost` (a host with no +transport), `TestExecRunner_KillsTheWholeProcessGroup` and `TestExecRunner_WaitDelayBoundsAPipeHeldByAStrayChild` (the process lifecycle), -the `TestTimeoutRunner_*` set (the deadline message), and -`TestRefresh_RefusesAnOriginItCannotReach` (the refresh scope limit). +and the `TestTimeoutRunner_*` set (the deadline message). ### Amendment consequences -- `deps import` accepts `github.com` and `dev.azure.com` URLs. +- `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. - Offline `lint` verifies an Azure DevOps copy against its digest identically to a GitHub one. -- `deps check` and `deps update` remain `github.com`-only and refuse an Azure - DevOps copy with an actionable error. +- A header for a non-GitHub origin carries `modelith-ref-type:`, and one for + GitHub does not — so a GitHub header written by an earlier release is + byte-identical to one written now. From 1cf94aa834806492e89fc2706a32d28ff460f5d0 Mon Sep 17 00:00:00 2001 From: blevins darrin Date: Sun, 27 Sep 2026 06:21:25 +0800 Subject: [PATCH 4/8] fix(deps): repair ADO re-pin, classify az auth failures, bound check/update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the architecture review of 7f28d9c. Re-pin (blocker). adoSourceFromHeader applied the recorded ref-type prefix to the overridden ref, so re-pinning a branch-typed copy to a tag asked Azure DevOps for a branch named after the tag: "version=v1.0.0&versionType=branch" 404s, and the copy could not be re-pinned at all. The prefix is now dropped when --ref changes the ref, matching what an import override already does, and recordedRefType replaces the type in the header — a bare refresh keeps the recorded type, a re-pin records auto. Pinned by TestRefresh_ADORepinDropsTheRecordedType and TestRefresh_ADOBareRefreshKeepsTheRecordedType, the first of which fails on the old code with the header still claiming a branch. az authentication. unauthenticated() matched only gh's text, so an expired az session was not classified as ErrToolUnavailable and a batch repeated the same paragraph once per copy instead of stopping. It now also matches az login and AADSTS, and the sentinel reads "the delegated CLI is unavailable" rather than naming gh. --timeout on check and update. The bound existed only on import, so a stalled gh or az wedged a scheduled CI check indefinitely. CheckOptions and UpdateOptions carry a Timeout that decorates the runner in survey, and both commands expose --timeout; their help text no longer claims gh is the only delegated CLI. Signed-off-by: blevins darrin --- cmd/modelith/main.go | 33 +++++-- internal/deps/deps.go | 23 ++--- internal/deps/refresh.go | 53 +++++++++--- internal/deps/refresh_test.go | 159 +++++++++++++++++++++++++++++++--- 4 files changed, 227 insertions(+), 41 deletions(-) diff --git a/cmd/modelith/main.go b/cmd/modelith/main.go index 09c249d..d6ea0f1 100644 --- a/cmd/modelith/main.go +++ b/cmd/modelith/main.go @@ -245,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", @@ -263,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) @@ -283,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", @@ -308,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 @@ -316,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) @@ -332,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/internal/deps/deps.go b/internal/deps/deps.go index be02ed2..120a540 100644 --- a/internal/deps/deps.go +++ b/internal/deps/deps.go @@ -108,18 +108,21 @@ func (r timeoutRunner) Run(ctx context.Context, name string, args ...string) ([] return out, err } -// unauthenticated reports whether gh refused for want of credentials rather -// than because of anything about the request. +// 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 } diff --git a/internal/deps/refresh.go b/internal/deps/refresh.go index 676290d..add51d5 100644 --- a/internal/deps/refresh.go +++ b/internal/deps/refresh.go @@ -14,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. // @@ -101,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 } @@ -114,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 } @@ -126,7 +134,7 @@ type UpdateOptions struct { // non-nil error means the run stopped before it could measure a file because gh // 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. @@ -141,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) { @@ -157,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) @@ -286,6 +298,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 even a bare refresh may have had its type inferred, so the + // record is rebuilt from what the fetch actually resolved to. It 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) @@ -354,7 +371,17 @@ func adoSourceFromHeader(h *provenance.Header, ref string) (Source, error) { } q := url.Values{} q.Set("path", h.Path) - q.Set("version", adoVersionPrefix(h.RefType)+ref) + // 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 names a + // ref whose type the header cannot know — a branch may be re-pinned to a + // tag, and "GBv1.0.0" would ask ADO for a branch that does not exist — so + // the prefix is dropped and the API is left to infer it, exactly as an + // import with --ref does. + 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 to auto-detect, which diff --git a/internal/deps/refresh_test.go b/internal/deps/refresh_test.go index 12820d5..3d64541 100644 --- a/internal/deps/refresh_test.go +++ b/internal/deps/refresh_test.go @@ -443,10 +443,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 { @@ -454,12 +457,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) { @@ -471,6 +478,138 @@ 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. +func TestRefresh_ADORepinDropsTheRecordedType(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) + } + + // The request must carry no versionType at all: the override names a ref + // whose type the header cannot know, so the API is left to infer it. + if len(r.calls) == 0 { + t.Fatal("the repin made no az call") + } + for _, call := range r.calls { + if uri := azURI(call); strings.Contains(uri, "versionType") { + t.Errorf("the repin pinned a type the header could not know: %s", uri) + } + } + + after := readFile(t, path) + for _, want := range []string{"# modelith-ref: v1.0.0", "# modelith-ref-type: auto"} { + 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_ADOBareRefreshKeepsTheRecordedType pins the other half: a refresh +// that does not re-pin must keep asking for the same typed version the import +// did, because auto-detection 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 From fd84d9c5c9ce17cace39ee38537faa3f694a0f7a Mon Sep 17 00:00:00 2001 From: blevins darrin Date: Sun, 27 Sep 2026 06:27:27 +0800 Subject: [PATCH 5/8] docs: correct stale gh-only wording left by the multi-CLI change Two references still named gh as the only delegated CLI: Check's doc comment (saying the run stops when "gh was unusable") and the CLI reference, which documented --timeout for deps import but not for the deps check and deps update commands it now bounds. Signed-off-by: blevins darrin --- docs/07-cli.md | 9 +++++++++ internal/deps/refresh.go | 4 ++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/docs/07-cli.md b/docs/07-cli.md index 97ae533..c5996fc 100644 --- a/docs/07-cli.md +++ b/docs/07-cli.md @@ -132,6 +132,10 @@ 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. 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 ``` @@ -142,6 +146,11 @@ 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 modelith deps update --ref v2.2.0 docs/payments.modelith.yaml diff --git a/internal/deps/refresh.go b/internal/deps/refresh.go index add51d5..ca25bd3 100644 --- a/internal/deps/refresh.go +++ b/internal/deps/refresh.go @@ -131,8 +131,8 @@ 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, timeout: opts.Timeout, run: opts.Run}) } From 2121713c3024cdffa12e73b86c14a990ff284624 Mon Sep 17 00:00:00 2001 From: blevins darrin Date: Sun, 27 Sep 2026 06:33:20 +0800 Subject: [PATCH 6/8] docs(adr): record the Azure DevOps transport as ADR-0019, not an amendment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR-0015 is upstream-owned and the repo rule is that decisions are never edited in place — a new ADR supersedes the old one. The previous revision appended a 101-line amendment to ADR-0015, which broke that rule and read as a fork edit to an upstream record. ADR-0015 is now byte-identical to upstream except a status note pointing at the superseding record, and the decision lives in its own ADR-0019. The note keeps what ADR-0015 decided at the time while telling a reader where the transport decision moved. Also adds TestADR_0019_ (pinning that the new ref-type key is recorded for ADO and omitted for GitHub), cites ADR-0019 from docs/10-vendoring, and repoints the deps.go comment. Signed-off-by: blevins darrin --- docs/10-vendoring.md | 3 +- internal/deps/deps.go | 3 +- internal/deps/deps_test.go | 42 +++++++ .../0015-vendoring-is-a-whole-file-copy.md | 105 +---------------- ...0019-azure-devops-as-a-second-transport.md | 110 ++++++++++++++++++ 5 files changed, 160 insertions(+), 103 deletions(-) create mode 100644 project-docs/adr/0019-azure-devops-as-a-second-transport.md diff --git a/docs/10-vendoring.md b/docs/10-vendoring.md index 3247ca4..f3ef555 100644 --- a/docs/10-vendoring.md +++ b/docs/10-vendoring.md @@ -312,5 +312,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 120a540..af9037e 100644 --- a/internal/deps/deps.go +++ b/internal/deps/deps.go @@ -622,8 +622,7 @@ func adoVersionType(src Source) string { // 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 (the ADR-0015 amendment on the -// Azure DevOps transport). +// 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", diff --git a/internal/deps/deps_test.go b/internal/deps/deps_test.go index 1ed4293..a60bcdf 100644 --- a/internal/deps/deps_test.go +++ b/internal/deps/deps_test.go @@ -269,6 +269,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 +// letting the API infer it is not equivalent when a branch and a tag share a +// name, so an ADO import 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() 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 36ea0e3..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,9 +5,11 @@ 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. -> **Amended 2026-09-26.** The **`gh` is the only transport** decision below is -> superseded — Azure DevOps is now a second origin for `deps import`. Everything -> else in this record stands unchanged; see the Amendment at the end. +> **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 @@ -147,100 +149,3 @@ step is the real gate. case for that rule, since it is where someone else's prose lands in your published document, but it shares no code with this change and is a visibly broken document rather than a privilege boundary. - -## Amendment — Azure DevOps as a second transport (2026-09-26) - -This amendment supersedes the **`gh` is the only transport** decision above. -Nothing else in this record changes: the content digest, the header shape, the -suppression rules, the non-recursive fetch, and the print-don't-block warning -all stand. - -ADR-0007 set the bar for a second transport at "a real user exists". That user -arrived: canonical domain models hosted in private Azure DevOps Git -repositories (`dev.azure.com`). `deps import` now accepts a `dev.azure.com` blob -URL alongside a `github.com` one. - -**Delegation, not a transport.** The fetch still hands off to an external CLI, -executed as an argv array with no shell, so `modelith` acquires no HTTP client, -no TLS configuration, and no credential handling (ADR-0011). Where GitHub -delegates to `gh api`, Azure DevOps delegates to `az rest`. Authentication is -whatever the user's own CLI session is — `gh auth login`, `az login` — and the -binary never sees a token. An earlier revision of this work fetched through -`curl` with a token read from `az account get-access-token`; 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 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. - -**Byte fidelity is a fetch-path concern.** `az rest` appends a newline when it -prints a raw body to stdout, which by itself moves the copy's digest off -canonical. Content is therefore written to a temp file with `--output-file` and -read back, so the vendored bytes match the origin by construction rather than by -trusting a printer. `gh` needs no such step; it returns the raw body unchanged. - -**`fetch: git` still names what the origin is.** The recorded `origin` is the -repository URL (`https://dev.azure.com///_git/`), so an ADO -copy is verified offline against its digest exactly like a GitHub one, and a -legacy `*.visualstudio.com` URL is refused at parse time with a pointer to the -`dev.azure.com` address, since the host moved. - -**Process lifecycle is bounded.** 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, and the resulting message distinguishes a fired -deadline from a caller's cancel (e.g. Ctrl+C) rather than reporting both as a -timeout. - -**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, -and `--output-file` writes those bytes verbatim. - -**The header gains one optional key, `ref-type`.** An Azure DevOps version has a -*type* — `GB` a branch, `GT` a tag, `GC` a commit — and `az`'s API takes the type -alongside the value. Letting the API infer it is not equivalent: a branch and a -tag may share a name, and the two answer differently. So an ADO import records -`# modelith-ref-type:` as `branch`, `tag`, or `commit`, or `auto` when the URL -left the type unprefixed or `--ref` overrode it — the case where inference is -what was asked for. The key is optional and *omitted* for GitHub, whose API -resolves an untyped ref on its own: a header written before the key existed -still parses, and no GitHub header changes shape. - -**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, using -the `origin` (organization, project, repository), `path`, `ref`, and the new -`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 copy from a host -this build has no transport for is still refused per file — as a `Report`, so a -run that also holds reachable copies still judges them — with an error naming the -origin rather than failing as a malformed URL. - -Pinned by `TestImport_ADO_StampsAVerifiableCopy` and the rest of the -`TestImport_ADO_*` set (import end to end, the ref-type prefixes, the `--ref` -override, and `download=true` on the items request), -`TestRefresh_ADOCopyIsFirstClass` (check and update dispatch to `az rest` and -keep the recorded ref type), `TestRefresh_RefusesAnUnknownHost` (a host with no -transport), `TestExecRunner_KillsTheWholeProcessGroup` and -`TestExecRunner_WaitDelayBoundsAPipeHeldByAStrayChild` (the process lifecycle), -and the `TestTimeoutRunner_*` set (the deadline message). - -### Amendment 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. -- Offline `lint` verifies an Azure DevOps copy against its digest identically to - a GitHub one. -- A header for a non-GitHub origin carries `modelith-ref-type:`, and one for - GitHub does not — so a GitHub header written by an earlier release is - byte-identical to one written now. 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..ed7f72a --- /dev/null +++ b/project-docs/adr/0019-azure-devops-as-a-second-transport.md @@ -0,0 +1,110 @@ +# 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`.** 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. Letting the API infer it is not equivalent: a branch and a +tag may share a name, and the two answer differently. So an ADO import records +`# modelith-ref-type:` as `branch`, `tag`, or `commit`, or `auto` when the URL +left the type unprefixed or `--ref` overrode it — the case where inference is +what was asked for. 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. An unknown +value is an error, like an unknown `fetch:` method, which matches ADR-0015's +closed-set posture pre-release. + +**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 so the API infers the new ref's type — a branch re-pinned to a tag +must not ask for a branch named after the tag — and the header's `ref-type` is +rewritten from what the fetch 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). From bfc6258d62f67c9f284b681b6a1c919d4fcaf2cf Mon Sep 17 00:00:00 2001 From: blevins darrin Date: Sun, 27 Sep 2026 07:03:26 +0800 Subject: [PATCH 7/8] fix(deps): resolve an ADO ref type instead of letting the API infer one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verified against real endpoints (az 2.88.0): the Azure DevOps Git Items API does not infer a version's type. An untyped versionDescriptor.version is read as a *branch*, so a 40-hex commit fails with "The version descriptor could not be resolved" — reproduced on api-version 7.1, 7.1-preview.1, and 7.2-preview.1, and equivalently for a tag name. The refs filter shows the same default: filter=heads/main returns refs/heads/main, filter=main returns nothing. That makes the "auto" value a fiction. Nothing is inferred, so a copy pinned to a tag or a commit could not be fetched while its type was unset — which covered both override paths (--ref on import and on update) and any header written before the key existed. Dropping the type on a repin, the previous revision of this fix, only helped the tag -> branch direction; branch -> tag resolved to the same branch-typed lookup and still 404ed. So the type is now resolved before the fetch when it is not named: a git object id (40 or 64 hex) is a commit, and anything else is looked up under refs/heads/ and then refs/tags/. A branch wins a name collision, because that is the API's own default and a tag is reachable from the URL's GT form. The header's ref-type value set drops "auto": an unknown type is omitted, and a refresh resolves it, so an older header is self-healing rather than rejected. The az fake now models all of this — the refs endpoint, the versionType default to branch, and a 404 for an unresolvable typed version — because the previous fake let the untyped request pass, which is how the bug reached a real endpoint. Signed-off-by: blevins darrin --- docs/10-vendoring.md | 13 +- internal/deps/deps.go | 113 ++++++++-- internal/deps/deps_test.go | 202 +++++++++++++++--- internal/deps/refresh.go | 33 +-- internal/deps/refresh_test.go | 104 +++++++-- internal/provenance/provenance.go | 18 +- ...0019-azure-devops-as-a-second-transport.md | 31 ++- 7 files changed, 430 insertions(+), 84 deletions(-) diff --git a/docs/10-vendoring.md b/docs/10-vendoring.md index f3ef555..239a514 100644 --- a/docs/10-vendoring.md +++ b/docs/10-vendoring.md @@ -51,7 +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`, `commit`, or `auto` — so a refresh rebuilds the same typed request. Omitted for GitHub, whose API resolves an untyped ref. | +| `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. | @@ -209,10 +209,13 @@ git grep -l '# modelith-vendored' 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 rather than letting the -API infer one. An unprefixed URL, or a `--ref` override, records `auto`, which -is what leaves the inference to the API. GitHub has no such key: its API -resolves an untyped ref on its own. +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. ::: diff --git a/internal/deps/deps.go b/internal/deps/deps.go index af9037e..36b94f1 100644 --- a/internal/deps/deps.go +++ b/internal/deps/deps.go @@ -284,7 +284,7 @@ func parseADOSource(u *url.URL, ref string) (Source, error) { } if ref != "" { src.Ref = ref - src.RefType = "" // let the ADO API auto-detect the ref type + src.RefType = "" // not named by the URL; the fetch path resolves it } if src.Ref == "" { return Source{}, fmt.Errorf( @@ -348,6 +348,12 @@ func Import(ctx context.Context, opts Options) (*Result, error) { 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 + } // 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 @@ -502,21 +508,103 @@ func fetchCommitFor(ctx context.Context, runner Runner, src Source) (string, err return fetchCommit(ctx, runner, src) } -// recordedRefType is the ref-type a provenance header records for src: the ADO -// version type when the URL's GB/GT/GC prefix named one, "auto" when the ADO -// API is left to infer it (an unprefixed version=, or a --ref override), and -// empty for GitHub — whose API resolves an untyped ref on its own, and whose -// headers predate this key. Recording it is what lets a later refresh rebuild -// the same typed request, which is not equivalent to auto-detection when a -// branch and a tag share a name. +// 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 "" } - if vt := adoVersionType(src); vt != "" { - return vt + 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 "auto" + return true } // fetchContent returns the file's bytes. The raw media type asks the API for @@ -602,7 +690,8 @@ 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 accepts an empty versionType and auto-detects the ref. +// 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": diff --git a/internal/deps/deps_test.go b/internal/deps/deps_test.go index a60bcdf..260e273 100644 --- a/internal/deps/deps_test.go +++ b/internal/deps/deps_test.go @@ -3,6 +3,7 @@ package deps import ( "context" "fmt" + "net/url" "os" "path/filepath" "runtime" @@ -80,17 +81,35 @@ func (f *fakeRunner) runAz(args []string) ([]byte, error) { return nil, fmt.Errorf("az: HTTP 404: Not Found (%s)", uri) } switch { - case strings.Contains(uri, "/items"): - // Validate that a known version prefix produces the right versionType. - // GB→branch, GT→tag, GC→commit. When the prefix is absent or unknown, - // versionType is omitted (the API auto-detects). - if strings.Contains(uri, "versionType") { - if !strings.Contains(uri, "versionType=branch") && - !strings.Contains(uri, "versionType=tag") && - !strings.Contains(uri, "versionType=commit") { - return nil, fmt.Errorf("az: unexpected versionType in %q", uri) + 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 @@ -122,11 +141,90 @@ func (f *fakeRunner) runAz(args []string) ([]byte, error) { 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 @@ -811,7 +909,7 @@ func TestParseSource_ADO(t *testing.T) { }, }, { - name: "an explicit ref resets RefType so the API auto-detects", + 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{ @@ -937,7 +1035,7 @@ func TestParseSource_ADO(t *testing.T) { wantErr: "has no version parameter", }, { - name: "a bare version with no prefix uses the auto-detect path", + 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, @@ -1114,33 +1212,85 @@ func TestImport_ADO_CommitUsesVersionTypeCommit(t *testing.T) { } // TestImport_ADO_OverrideRefOmitsVersionType pins that when --ref overrides -// the URL's ref, the API call omits versionType so ADO auto-detects. -func TestImport_ADO_OverrideRefOmitsVersionType(t *testing.T) { +// 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) - // URL has GBmain (branch), but --ref overrides to a tag-like value. - url := adoBlobURL - _, err := Import(context.Background(), Options{ - URL: url, + res, err := Import(context.Background(), Options{ + URL: adoBlobURL, // names GBmain, overridden below Dir: t.TempDir(), - Ref: "v1.0.0", + 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) { - uri := call[i+1] - if strings.Contains(uri, "versionType") { - t.Errorf("--ref override should omit versionType, got %q", uri) - } - if !strings.Contains(uri, "version=v1.0.0") { - t.Errorf("--ref override should use the override value in version=, got %q", uri) - } + 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]) } } } diff --git a/internal/deps/refresh.go b/internal/deps/refresh.go index ca25bd3..3ec50cf 100644 --- a/internal/deps/refresh.go +++ b/internal/deps/refresh.go @@ -226,6 +226,14 @@ 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 { + rep.Err = err + return rep, nil + } upstream, err := fetchContentFor(ctx, runner, src) if err != nil { @@ -299,9 +307,9 @@ 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 even a bare refresh may have had its type inferred, so the - // record is rebuilt from what the fetch actually resolved to. It is empty - // for GitHub, which keeps its header shape. + // 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") @@ -372,11 +380,11 @@ func adoSourceFromHeader(h *provenance.Header, ref string) (Source, error) { 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 names a - // ref whose type the header cannot know — a branch may be re-pinned to a - // tag, and "GBv1.0.0" would ask ADO for a branch that does not exist — so - // the prefix is dropped and the API is left to infer it, exactly as an - // import with --ref does. + // 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 = "" @@ -384,13 +392,14 @@ func adoSourceFromHeader(h *provenance.Header, ref string) (Source, error) { 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 to auto-detect, which - // would discard what the header recorded. + // 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, and an -// empty or "auto" type to none — which is what leaves the ADO API to infer it. +// 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": diff --git a/internal/deps/refresh_test.go b/internal/deps/refresh_test.go index 3d64541..593790f 100644 --- a/internal/deps/refresh_test.go +++ b/internal/deps/refresh_test.go @@ -103,7 +103,7 @@ func vendoredFromADO(t *testing.T) string { // 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 auto-detection when a branch and a tag share a name. +// equivalent to a refs lookup when a branch and a tag share a name. func TestRefresh_ADOCopyIsFirstClass(t *testing.T) { t.Parallel() @@ -153,7 +153,7 @@ func TestRefresh_ADOCopyIsFirstClass(t *testing.T) { } }) - t.Run("the request is typed from the header, not auto-detected", func(t *testing.T) { + 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) @@ -553,7 +553,13 @@ func (h *hangRunner) Run(ctx context.Context, _ string, _ ...string) ([]byte, er // 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. -func TestRefresh_ADORepinDropsTheRecordedType(t *testing.T) { +// 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) @@ -567,19 +573,28 @@ func TestRefresh_ADORepinDropsTheRecordedType(t *testing.T) { t.Fatalf("repin failed: %v", rep.Err) } - // The request must carry no versionType at all: the override names a ref - // whose type the header cannot know, so the API is left to infer it. - if len(r.calls) == 0 { - t.Fatal("the repin made no az call") - } + // 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 { - if uri := azURI(call); strings.Contains(uri, "versionType") { - t.Errorf("the repin pinned a type the header could not know: %s", uri) + 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: auto"} { + 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) } @@ -589,9 +604,74 @@ func TestRefresh_ADORepinDropsTheRecordedType(t *testing.T) { } } +// 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 auto-detection is not equivalent when a branch and a tag share a +// 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() diff --git a/internal/provenance/provenance.go b/internal/provenance/provenance.go index ae8baae..59eef98 100644 --- a/internal/provenance/provenance.go +++ b/internal/provenance/provenance.go @@ -44,18 +44,24 @@ type Header struct { Path string Ref string // RefType is the kind of ref Ref names, for a host whose API distinguishes - // them: "branch", "tag", or "commit", or "auto" when the origin's own API is - // left to infer it. 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. + // 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. -var refTypes = []string{"branch", "tag", "commit", "auto"} +// 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. 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 index ed7f72a..c2d6677 100644 --- a/project-docs/adr/0019-azure-devops-as-a-second-transport.md +++ b/project-docs/adr/0019-azure-devops-as-a-second-transport.md @@ -44,17 +44,26 @@ 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`.** 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. Letting the API infer it is not equivalent: a branch and a -tag may share a name, and the two answer differently. So an ADO import records -`# modelith-ref-type:` as `branch`, `tag`, or `commit`, or `auto` when the URL -left the type unprefixed or `--ref` overrode it — the case where inference is -what was asked for. 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. An unknown -value is an error, like an unknown `fetch:` method, which matches ADR-0015's -closed-set posture pre-release. +**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 From 52a5544fbb934516fb348e6625d5df3fdb012d54 Mon Sep 17 00:00:00 2001 From: blevins darrin Date: Sun, 27 Sep 2026 07:05:54 +0800 Subject: [PATCH 8/8] fix(deps): stop the batch when the ref-type lookup finds the CLI unusable A refs lookup goes through az like any fetch, so an absent or unauthenticated session there must end the run rather than be reported once per copy as if each file were separately at fault. The comment wording that still implied the API infers a type is corrected in the ADR, refresh.go, and a test. Signed-off-by: blevins darrin --- internal/deps/deps_test.go | 8 +++--- internal/deps/refresh.go | 12 ++++++--- internal/deps/refresh_test.go | 27 +++++++++++++++++++ ...0019-azure-devops-as-a-second-transport.md | 6 ++--- 4 files changed, 43 insertions(+), 10 deletions(-) diff --git a/internal/deps/deps_test.go b/internal/deps/deps_test.go index 260e273..09177a4 100644 --- a/internal/deps/deps_test.go +++ b/internal/deps/deps_test.go @@ -368,10 +368,10 @@ 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 -// letting the API infer it is not equivalent when a branch and a tag share a -// name, so an ADO import 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 +// 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) { diff --git a/internal/deps/refresh.go b/internal/deps/refresh.go index 3ec50cf..e3221e9 100644 --- a/internal/deps/refresh.go +++ b/internal/deps/refresh.go @@ -231,6 +231,12 @@ func visit(ctx context.Context, runner Runner, path string, opts surveyOptions) // 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 } @@ -369,9 +375,9 @@ func sourceFromHeader(h *provenance.Header, ref string) (Source, error) { // 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 -// rather than letting the API infer one, which a branch and a tag sharing a name -// would answer differently. +// 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 { diff --git a/internal/deps/refresh_test.go b/internal/deps/refresh_test.go index 593790f..fcc3658 100644 --- a/internal/deps/refresh_test.go +++ b/internal/deps/refresh_test.go @@ -271,6 +271,33 @@ func TestRefresh_AnUnknownHostDoesNotStopTheRun(t *testing.T) { } } +// 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() 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 index c2d6677..22e5913 100644 --- a/project-docs/adr/0019-azure-devops-as-a-second-transport.md +++ b/project-docs/adr/0019-azure-devops-as-a-second-transport.md @@ -71,9 +71,9 @@ 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 so the API infers the new ref's type — a branch re-pinned to a tag -must not ask for a branch named after the tag — and the header's `ref-type` is -rewritten from what the fetch resolved. A copy from a host with no transport is +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.