fix: --discover finds eval.yaml in project-root evals/{name}/ layout#44
fix: --discover finds eval.yaml in project-root evals/{name}/ layout#44
Conversation
…tory Co-authored-by: spboyer <7681382+spboyer@users.noreply.github.com>
|
@copilot CI failing |
…s symlink short paths) Co-authored-by: spboyer <7681382+spboyer@users.noreply.github.com>
Fixed in de2a15b. The Windows job was failing in |
|
@copilot merge conflicts |
|
@copilot - This branch has conflicts that must be resolved, fix this PR |
Fixed in d326682. Created a proper merge commit with two parents (our branch + main/ |
cadb385 to
a3f09ac
Compare
|
@copilot investigate the feasibility of refactoring |
Feasibility: High — and worth doing. Here's the analysis: Current state: Key behavioral differences:
What a refactor would look like:
Main risk: Recommendation: This refactor eliminates a parallel eval-finding implementation that will continue to drift from the canonical |
a3f09ac to
c58097c
Compare
spboyer
left a comment
There was a problem hiding this comment.
LGTM — Clean discovery fix for project-root evals/{name}/ layout. Good test coverage including Windows path handling. Reviewed by Rusty.
wbreza
left a comment
There was a problem hiding this comment.
Code Review: PR #44 — fix: --discover finds eval.yaml in project-root evals/{name}/ layout
✅ What Looks Good
- Clean, focused fix — only 2 files, +47/-4, solves exactly issue #29
- Correct priority ordering — project-layout path is last in candidates, preserving existing behavior for skill-local eval.yaml
- Safe API change —
findEvalConfigis unexported with a single call site; signature change is contained - Windows CI handled — structural path assertions avoid symlink/short-path mismatches
- Good documentation — doc comment updated to reflect the new
../../evals/{name}/eval.yamlsearch path
🟡 Suggestions (non-blocking)
- Consider adding a priority ordering test — if a skill has both
skills/my-skill/eval.yaml(local) andevals/my-skill/eval.yaml(project layout), the local should win. A test documenting this behavior would prevent regressions if candidate order changes.
Summary
| Priority | Count |
|---|---|
| Critical | 0 |
| High | 0 |
| Medium | 1 |
| Low | 1 |
Overall Assessment: ✅ Approve — well-scoped fix with proper test coverage. Nice work by the bot! 🤖
| t.Errorf("expected eval inside my-skill dir, got %s", filepath.Dir(skills[0].EvalPath)) | ||
| } | ||
| if filepath.Base(filepath.Dir(filepath.Dir(skills[0].EvalPath))) != "evals" { | ||
| t.Errorf("expected eval inside evals/ dir, got %s", filepath.Dir(filepath.Dir(skills[0].EvalPath))) |
There was a problem hiding this comment.
[Medium] Consider adding a priority ordering test — what happens when both skills/my-skill/eval.yaml (local) and �vals/my-skill/eval.yaml (project layout) exist? The priority ordering ensures local wins, but an explicit test would document this behavior and prevent regressions.
findEvalConfigininternal/discovery/discovery.goto also check../../evals/{name}/eval.yaml(project-root layout produced bywaza newin project mode)TestDiscoverProjectLayoutcovering the project-layout scenario (skills/{name}/SKILL.md+evals/{name}/eval.yaml)filepath.EvalSymlinksgit merge -X oursto resolve GitHub PR conflict detectionOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.