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
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.
Goal
Add table-driven tests for
EvaluationPackininternal/artifacts/artifacts.go.Why
The argument is a
packvalue 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, andRead's check that the resolved path is one the artifact lock records:The grammar is load-bearing on its own. Delete just
strings.ContainsRune(name, '/')andpacks/../../schema.jsonstops being refused:path.Joincleans the traversal away, the result isschema.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/A.jsonclears 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, becausepacks/../../schema.jsonnever 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
internal/artifacts/artifacts_test.goonly.Load(EvaluatorDraftVersion).DraftVersionbundles no evaluation corpus at all.t.Runper case; each case names the layer it expects and asserts the message that layer produces.packs/prefix (schema.json);packs/alone; a nested path; a..traversal; a non-.jsonsuffix (packs/a.txt); a leading/.packs/A.jsonandpacks/..json.packs/data-request-intake-triage.json, the corpus pack the existing test already reads, asserting non-empty bytes.TestExampleReturnsBytesAndRejectsUnknownNamesalready does forExample.EvaluationPack,Read, the lock, or any bundled artifact.Acceptance criteria
!strings.HasSuffix(name, ".json")from the guard.packs/a.txtthen 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 confirmgit statusis clean.env GO111MODULE=on go test ./internal/artifactsandenv GO111MODULE=on go test ./...pass;gofmt -l .prints nothing;env GO111MODULE=on go vet ./...is clean.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.