Hand a build's fetch the named paths in a shared request, so no build module reads them from a manifest (issue 1758, ADR 0499 §4, ADR 0495 §1)
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.
This commit is contained in:
@@ -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 <owner>/<name>, 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, <owner>/<name>", 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, "../")
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user