Skip to content

Pin which layer refuses an evaluation-corpus pack path #120

Description

@kikashy

Goal

Add table-driven tests for EvaluationPack in internal/artifacts/artifacts.go.

Why

func (s *Set) EvaluationPack(relative string) ([]byte, error) {
	name, ok := strings.CutPrefix(relative, "packs/")
	if !ok || name == "" || strings.ContainsRune(name, '/') || !strings.HasSuffix(name, ".json") {
		return nil, fmt.Errorf("not an evaluation-corpus pack path: %s", relative)
	}
	return s.Read(path.Join("evaluation", "packs", name))
}

The argument is a pack value read out of the bundled evaluation manifest (internal/evaluation/corpus.go:376), and the comment above the function states the intent: a manifest must not be able to reach an artifact outside the corpus. Two independent layers hold that up — this grammar, and Read's check that the resolved path is one the artifact lock records:

func (s *Set) Read(relative string) ([]byte, error) {
	if _, ok := s.files[relative]; !ok {
		return nil, fmt.Errorf("artifact is not recorded in lock: %s", relative)
	}

The grammar is load-bearing on its own. Delete just strings.ContainsRune(name, '/') and packs/../../schema.json stops being refused: path.Join cleans the traversal away, the result is schema.json, the lock records it, and the call hands back all 14,268 bytes of the specification schema.

The two layers refuse different sets, and the split is not accidental. Today:

"packs/nested/a.json"      -> not an evaluation-corpus pack path
"packs/../../schema.json"  -> not an evaluation-corpus pack path
"packs/a.txt"              -> not an evaluation-corpus pack path
"packs/A.json"             -> artifact is not recorded in lock: evaluation/packs/A.json
"packs/..json"             -> artifact is not recorded in lock: evaluation/packs/..json

packs/A.json clears the grammar and is stopped only by the lock — which is the interesting half, because the importer that writes the corpus is stricter than the reader: validateEvaluationPackPath (tools/sync-spec-artifacts/main.go:331) requires ^packs/[a-z0-9][a-z0-9-]*\.json$. The reader deliberately does not restate the writer's rule and leans on the lock for the difference.

One rejection is tested, at artifacts_test.go:165, and it asserts only that an error came back — and its comment attributes that refusal to the wrong layer, because packs/../../schema.json never reaches the lock. Neither error message is asserted anywhere in the repository, so nothing today can tell the two layers apart, and removing the lock check entirely would leave that assertion green.

Scope

  • Change internal/artifacts/artifacts_test.go only.
  • Load the set with Load(EvaluatorDraftVersion). DraftVersion bundles no evaluation corpus at all.
  • Table-driven with t.Run per case; each case names the layer it expects and asserts the message that layer produces.
    • grammar refusals: no packs/ prefix (schema.json); packs/ alone; a nested path; a .. traversal; a non-.json suffix (packs/a.txt); a leading /.
    • lock refusals: well-formed names the corpus does not contain, including packs/A.json and packs/..json.
    • one accepted case: packs/data-request-intake-triage.json, the corpus pack the existing test already reads, asserting non-empty bytes.
  • Assert that no rejection returns bytes, the way TestExampleReturnsBytesAndRejectsUnknownNames already does for Example.
  • Do not change EvaluationPack, Read, the lock, or any bundled artifact.

Acceptance criteria

  • Each clause of the grammar has a rejected case, and the lock layer has at least two.
  • Every case asserts which of the two messages came back, so a rejection by the wrong layer fails.
  • No rejection returns bytes.
  • Mutation check, run locally and reported in the pull request: delete !strings.HasSuffix(name, ".json") from the guard. packs/a.txt then falls through to the lock instead — the whole suite still passes with that mutation today, so a test that asks only "did it fail?" cannot see it, and only a test that names the layer does. Restore the file afterwards and confirm git status is clean.
  • env GO111MODULE=on go test ./internal/artifacts and env GO111MODULE=on go test ./... pass; gofmt -l . prints nothing; env GO111MODULE=on go vet ./... is clean.
  • Every commit includes a DCO sign-off created with git commit -s.

Contributor learning

What defence in depth looks like in a few lines of Go — two independent checks, each refusing a different set — and why a test that asks only "did it fail?" cannot tell them apart, or notice when one of them stops running.

Metadata

Metadata

Assignees

No one assigned

    Labels

    goPull requests that update go codegood first issueGood for newcomers

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions