From 7cf73ed76b03cd62075e0fc61eb8d5d764c7906e Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 22 Oct 2026 17:29:58 +0200 Subject: [PATCH] =?UTF-8?q?Hand=20a=20build's=20fetch=20the=20named=20path?= =?UTF-8?q?s=20in=20a=20shared=20request,=20so=20no=20build=20module=20rea?= =?UTF-8?q?ds=20them=20from=20a=20manifest=20(issue=201758,=20ADR=200499?= =?UTF-8?q?=20=C2=A74,=20ADR=200495=20=C2=A71)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit buildstep.FetchRequest is what the builder writes and the build modules read: the repository, commit, ref, base and, for a module naming its paths, its directory and those paths from the indexer's kept inputs. --- go/buildstep/request.go | 115 +++++++++++++++++++++++++++++++++++ go/buildstep/request_test.go | 57 +++++++++++++++++ 2 files changed, 172 insertions(+) create mode 100644 go/buildstep/request.go create mode 100644 go/buildstep/request_test.go diff --git a/go/buildstep/request.go b/go/buildstep/request.go new file mode 100644 index 0000000..c0189dc --- /dev/null +++ b/go/buildstep/request.go @@ -0,0 +1,115 @@ +package buildstep + +import ( + "fmt" + "path" + "regexp" + "strings" +) + +var ( + requestCommitOK = regexp.MustCompile(`^[0-9a-f]{40}$`) + requestRepositoryOK = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._-]*/[A-Za-z0-9][A-Za-z0-9._-]*$`) +) + +// FetchRequest is what a build module's fetch tool is asked: the repository and the full commit, the pull request's +// ref when only its head carries the commit, the trunk commit to merge onto for a merge preview, and, for a build of a +// module whose every artifact names its paths, that module's directory and its named paths (novox/hq ADR 0499 clause 4). +// +// **The paths come from the builder, not from a manifest** (ADR 0495 clause 1; the lead's decision L8 of R4e): the +// builder fills Dir and Paths from the indexer's Module value (Path, and KeptInputs2.Paths' keys), and the build module +// gives the build and test steps a workspace holding those paths, the manifest and its lock files. A request without +// them builds from the module's directory, as before. +type FetchRequest struct { + Repository string `json:"repository"` + Commit string `json:"commit"` + Ref string `json:"ref,omitempty"` + Base string `json:"base,omitempty"` + Dir string `json:"dir,omitempty"` + Paths []string `json:"paths,omitempty"` +} + +// FetchRequestProperties are the JSON schema properties of FetchRequest's dir and paths, for a fetch tool's Input. +func FetchRequestProperties() map[string]any { + return map[string]any{ + "dir": map[string]any{"type": "string", "description": "the module's directory in the repository (\".\" for its root), with paths"}, + "paths": map[string]any{"type": "array", "items": map[string]any{"type": "string"}, + "description": "the paths the module names, relative to dir, from the indexer's kept inputs: the build sees only these, the manifest and its lock files"}, + } +} + +// Args is r as a tool call's arguments. +func (r FetchRequest) Args() map[string]any { + args := map[string]any{"repository": r.Repository, "commit": r.Commit} + for k, v := range map[string]string{"ref": r.Ref, "base": r.Base, "dir": r.Dir} { + if v != "" { + args[k] = v + } + } + if len(r.Paths) > 0 { + args["paths"] = append([]string(nil), r.Paths...) + } + return args +} + +// ParseFetchRequest reads a fetch tool's arguments; arguments it does not know (a build module's own) are left to the +// caller. It refuses a repository that is not /, a commit or a base that is not 40 hex digits, paths with no +// module directory or a directory with no path, a directory leaving the repository and a path leaving the directory, +// naming each. +func ParseFetchRequest(args map[string]any) (FetchRequest, error) { + var r FetchRequest + r.Repository, _ = args["repository"].(string) + r.Commit, _ = args["commit"].(string) + r.Ref, _ = args["ref"].(string) + r.Base, _ = args["base"].(string) + r.Dir, _ = args["dir"].(string) + switch v := args["paths"].(type) { + case nil: + case []string: + r.Paths = append([]string(nil), v...) + case []any: + for _, p := range v { + s, ok := p.(string) + if !ok { + return FetchRequest{}, fmt.Errorf("paths is a list of strings") + } + r.Paths = append(r.Paths, s) + } + default: + return FetchRequest{}, fmt.Errorf("paths is a list of strings") + } + return r, r.Check() +} + +// Check is what ParseFetchRequest refuses. +func (r FetchRequest) Check() error { + if !requestRepositoryOK.MatchString(r.Repository) || strings.Contains(r.Repository, "..") { + return fmt.Errorf("%q is not a repository, /", r.Repository) + } + if !requestCommitOK.MatchString(r.Commit) { + return fmt.Errorf("%q is not a full commit (40 hex digits)", r.Commit) + } + if r.Base != "" && !requestCommitOK.MatchString(r.Base) { + return fmt.Errorf("%q is not a trunk commit (40 hex digits)", r.Base) + } + switch { + case len(r.Paths) > 0 && r.Dir == "": + return fmt.Errorf("the request names paths and names no module directory (dir) they are relative to") + case len(r.Paths) == 0 && r.Dir != "": + return fmt.Errorf("the request names a module directory and no path") + } + if r.Dir != "" && (outside(r.Dir) || strings.HasPrefix(r.Dir, "/")) { + return fmt.Errorf("the module directory %q leaves the repository", r.Dir) + } + for _, p := range r.Paths { + if p == "" || strings.HasPrefix(p, "/") || outside(p) || path.Clean(p) == "." { + return fmt.Errorf("the path %q leaves the module's directory or is not a path in it", p) + } + } + return nil +} + +func outside(p string) bool { + c := path.Clean(p) + return c == ".." || strings.HasPrefix(c, "../") +} diff --git a/go/buildstep/request_test.go b/go/buildstep/request_test.go new file mode 100644 index 0000000..6e13c8f --- /dev/null +++ b/go/buildstep/request_test.go @@ -0,0 +1,57 @@ +package buildstep + +import ( + "reflect" + "strings" + "testing" +) + +// A build's fetch is handed the paths of a module that names them (novox/hq ADR 0499 clause 4): the builder fills them +// from the indexer's kept inputs and the build module reads them from the call, never from the manifest (ADR 0495 +// clause 1). The request survives the trip through a tool call's arguments. +func TestAFetchRequestCarriesTheNamedPathsThroughTheToolCall(t *testing.T) { + req := FetchRequest{Repository: "novox/mesh-sdk", Commit: strings.Repeat("a", 40), Ref: "refs/pull/7/head", + Dir: ".", Paths: []string{"package.json", "src"}} + args := req.Args() + // A tool call's arguments arrive as JSON: lists of any. + args["paths"] = []any{"package.json", "src"} + got, err := ParseFetchRequest(args) + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(got, req) { + t.Fatalf("got %+v, want %+v", got, req) + } + plain, err := ParseFetchRequest(map[string]any{"repository": "novox/x", "commit": strings.Repeat("b", 40), + "base": strings.Repeat("c", 40), "beside": []any{}}) + if err != nil || plain.Paths != nil || plain.Base != strings.Repeat("c", 40) { + t.Fatalf("a request without paths: %+v, %v", plain, err) + } + + ok := map[string]any{"repository": "novox/x", "commit": strings.Repeat("b", 40)} + for _, c := range []struct { + set map[string]any + says string + }{ + {map[string]any{"repository": "novox"}, "is not a repository"}, + {map[string]any{"repository": "novox/../x"}, "is not a repository"}, + {map[string]any{"commit": "abc"}, "is not a full commit"}, + {map[string]any{"base": "abc"}, "is not a trunk commit"}, + {map[string]any{"paths": []any{"src"}}, "names no module directory"}, + {map[string]any{"dir": "modules/a", "paths": []any{"../b"}}, "leaves the module's directory"}, + {map[string]any{"dir": "../a", "paths": []any{"src"}}, "leaves the repository"}, + {map[string]any{"dir": "modules/a", "paths": []any{3}}, "paths is a list of strings"}, + {map[string]any{"dir": "modules/a"}, "names a module directory and no path"}, + } { + args := map[string]any{} + for k, v := range ok { + args[k] = v + } + for k, v := range c.set { + args[k] = v + } + if _, err := ParseFetchRequest(args); err == nil || !strings.Contains(err.Error(), c.says) { + t.Errorf("%v: %v, wanted a refusal saying %q", c.set, err, c.says) + } + } +}