diff --git a/CHANGELOG.md b/CHANGELOG.md index da962874..a89ccff9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 MSYS2 bash truncating quoted command arguments near 8 KiB, including long `agent_judge` prompts. Linux sandbox targets retain the configured inline threshold even when skill-up runs on a Windows host. +- Accept relative `--output-dir` paths for `agent_judge` cases. The judge context + host directory is now resolved to an absolute path before materialization, so a + relative output directory no longer fails with a misleading "escapes context + dir" error. Path traversal remains rejected. ### Added - Support explicit `judge.engine` configuration independent of the evaluated agent, diff --git a/internal/judge/context_materializer.go b/internal/judge/context_materializer.go index c907a123..98eeb51d 100644 --- a/internal/judge/context_materializer.go +++ b/internal/judge/context_materializer.go @@ -205,9 +205,23 @@ func resolveJudgeContext(cfg *config.JudgeContextConfig) effectiveJudgeContext { func judgeContextHostDir(artifactDir string) (string, error) { if artifactDir == "" { - return os.MkdirTemp("", "skill-up-judge-context-*") + // MkdirTemp honors TMPDIR, which may itself be relative; normalize the + // result so MaterializedContext.Dir is absolute in every branch. + dir, err := os.MkdirTemp("", "skill-up-judge-context-*") + if err != nil { + return "", err + } + abs, err := filepath.Abs(dir) + if err != nil { + _ = os.RemoveAll(dir) + return "", err + } + return abs, nil } - return filepath.Join(filepath.Dir(artifactDir), "context"), nil + // Normalize here so MaterializedContext.Dir is absolute regardless of + // whether --output-dir was given as a relative path. Downstream writes + // must not depend on the process working directory. + return filepath.Abs(filepath.Join(filepath.Dir(artifactDir), "context")) } func materializeText(ctx context.Context, rt runtime.Runtime, mc *MaterializedContext, key, fileName, content, mode string, maxBytes int) error { @@ -348,6 +362,8 @@ func safeMaterialRelPath(fileName string) (string, error) { return clean, nil } +// safeMaterialHostPath joins rel under root and rejects paths that escape it. +// root must be absolute; judgeContextHostDir normalizes it at the source. func safeMaterialHostPath(root, rel string) (string, error) { hostPath := filepath.Join(root, rel) cleanRoot := filepath.Clean(root) diff --git a/internal/judge/context_materializer_test.go b/internal/judge/context_materializer_test.go index 944b79ab..d1b92374 100644 --- a/internal/judge/context_materializer_test.go +++ b/internal/judge/context_materializer_test.go @@ -184,3 +184,90 @@ func assertMaterialPath(t *testing.T, materials []ContextMaterialManifest, key, } t.Fatalf("material %q not found in %#v", key, materials) } + +func TestSafeMaterialHostPath(t *testing.T) { + root := filepath.Join(t.TempDir(), "out", "ctx") + + for _, tc := range []struct { + name string + rel string + want string + }{ + {"plain file", "final_message.txt", filepath.Join(root, "final_message.txt")}, + {"nested file", filepath.Join("attachments", "01-diff.txt"), filepath.Join(root, "attachments", "01-diff.txt")}, + } { + t.Run(tc.name, func(t *testing.T) { + got, err := safeMaterialHostPath(root, tc.rel) + if err != nil { + t.Fatalf("safeMaterialHostPath(%q, %q): %v", root, tc.rel, err) + } + if got != tc.want { + t.Fatalf("safeMaterialHostPath(%q, %q) = %q, want %q", root, tc.rel, got, tc.want) + } + }) + } + + t.Run("traversal", func(t *testing.T) { + if _, err := safeMaterialHostPath(root, "../escape.txt"); err == nil { + t.Fatal("traversal must be rejected") + } + }) +} + +func TestMaterializeJudgeContext_RelativeArtifactDir(t *testing.T) { + // Regression: a relative --output-dir flows down as a relative artifact + // dir. The context host dir must still be absolute so downstream writes + // never depend on the process working directory. + t.Chdir(t.TempDir()) + artifactDir := filepath.Join("reports", "iteration-1", "case-1", "with_skill", "outputs", "judge", "run") + if err := os.MkdirAll(artifactDir, 0o755); err != nil { + t.Fatalf("mkdir artifact dir: %v", err) + } + + mc, err := MaterializeJudgeContext(context.Background(), &mockJudgeTestRuntime{}, nil, Input{ + FinalMessage: "final answer", + }, artifactDir) + if err != nil { + t.Fatalf("MaterializeJudgeContext: %v", err) + } + + wantDir, err := filepath.Abs(filepath.Join("reports", "iteration-1", "case-1", "with_skill", "outputs", "judge", "context")) + if err != nil { + t.Fatalf("resolve expected context dir: %v", err) + } + if !filepath.IsAbs(mc.Dir) { + t.Fatalf("mc.Dir = %q, want an absolute path", mc.Dir) + } + if mc.Dir != wantDir { + t.Fatalf("mc.Dir = %q, want %q", mc.Dir, wantDir) + } + if _, err := os.Stat(filepath.Join(mc.Dir, "final_message.txt")); err != nil { + t.Fatalf("final message material missing: %v", err) + } +} + +func TestMaterializeJudgeContext_EmptyArtifactDirUsesAbsoluteTempDir(t *testing.T) { + // Use a relative TMPDIR: with an absolute one the pre-fix code already + // returned an absolute path and this regression test would pass vacuously. + t.Chdir(t.TempDir()) + if err := os.MkdirAll("reltmp", 0o755); err != nil { + t.Fatalf("mkdir relative TMPDIR: %v", err) + } + t.Setenv("TMPDIR", "reltmp") + t.Setenv("TMP", "reltmp") + t.Setenv("TEMP", "reltmp") + + mc, err := MaterializeJudgeContext(context.Background(), &mockJudgeTestRuntime{}, nil, Input{ + FinalMessage: "final answer", + }, "") + if err != nil { + t.Fatalf("MaterializeJudgeContext: %v", err) + } + t.Cleanup(func() { _ = os.RemoveAll(mc.Dir) }) + if !filepath.IsAbs(mc.Dir) { + t.Fatalf("mc.Dir = %q, want an absolute path", mc.Dir) + } + if _, err := os.Stat(filepath.Join(mc.Dir, "final_message.txt")); err != nil { + t.Fatalf("final message material missing: %v", err) + } +}