[Saavrm26]

Checklist

Please review this repository's contribution guidelines.

• I've read and agree to the project's contribution guidelines.
• I'm requesting to feature branch.
• I checked that my code additions pass linting (black, isort, mypy) and unit tests.
• I updated tests (if applicable).
• I updated documentation (if applicable).

Description

Adds skill evaluations

Related Issue

Closes #

Test Plan

How was this tested?

[Saavrm26]

Have tested this by writing a simple coding agent and evaluating using-superpowers.

Coding agent history:

[
  {
    "role": "user",
    "content": "I want you to create a fastapi application, with a postgres database in docker compose. Create a sample route in the application as well. Write a plan for this"
  },
  {
    "role": "assistant",
    "content": ""
  },
  {
    "role": "tool",
    "content": " 1  ---\n 2  name: using-superpowers\n 3  description: Use when starting any conversation - establishes how to find and use skills, requiring skill invocation before ANY response including clarifying questions\n 4  ---\n 5  \n 6  <SUBAGENT-STOP>\n 7  If you were dispatched as a subagent to execute a specific task, ignore this skill.\n 8  </SUBAGENT-STOP>\n 9  \n10  <EXTREMELY-IMPORTANT>\n11  If you think there is even a 1% chance a skill might apply to what you are doing, you ABSOLUTELY MUST invoke the skill.\n12  \n13  IF A SKILL APPLIES TO YOUR TASK, YOU DO NOT HAVE A CHOICE. YOU MUST USE IT.\n14  \n15  This is not negotiable. You cannot rationalize your way out of this.\n16  </EXTREMELY-IMPORTANT>\n17  \n18  ## The Rule\n19  \n20  **Invoke relevant or requested skills BEFORE any response or action** — including clarifying questions, exploring the codebase, or checking files. If it turns out wrong for the situation, you don't have to use it.\n21  \n22  **Before entering plan mode:** if you haven't already brainstormed, invoke the brainstorming skill first.\n23  \n24  Then announce \"Using [skill] to [purpose]\" and follow the skill exactly. If it has a checklist, create a todo per item.\n25  \n26  ## Skill Priority\n27  \n28  When multiple skills apply, process skills come first — they set the approach, then implementation skills (frontend-design, etc.) carry it out. Brainstorming and systematic-debugging are Superpowers' most common process skills, but the rule holds for any of them.\n29  \n30  - \"Let's build X\" → superpowers:brainstorming first, then implementation skills.\n31  - \"Fix this bug\" → superpowers:systematic-debugging first, then domain skills.\n32  \n33  ## Red Flags\n34  \n35  These thoughts mean STOP—you're rationalizing:\n36  \n37  | Thought | Reality |\n38  |---------|---------|\n39  | \"This is just a simple question\" | Questions are tasks. Check for skills. |\n40  | \"I need more context first\" | Skill check comes BEFORE clarifying questions. |\n41  | \"Let me explore the codebase first\" | Skills tell you HOW to explore. Check first. |\n42  | \"I can check git/files quickly\" | Files lack conversation context. Check for skills. |\n43  | \"Let me gather information first\" | Skills tell you HOW to gather information. |\n44  | \"This doesn't need a formal skill\" | If a skill exists, use it. |\n45  | \"I remember this skill\" | Skills evolve. Read current version. |\n46  | \"This doesn't count as a task\" | Action = task. Check for skills. |\n47  | \"The skill is overkill\" | Simple things become complex. Use it. |\n48  | \"I'll just do this one thing first\" | Check BEFORE doing anything. |\n49  | \"This feels productive\" | Undisciplined action wastes time. Skills prevent this. |\n50  | \"I know what that means\" | Knowing the concept ≠ using the skill. Invoke it. |\n51  \n52  ## Platform Adaptation\n53  \n54  If your harness appears here, read its reference file for special instructions:\n55  \n56  - Codex: `references/codex-tools.md`\n57  - Pi: `references/pi-tools.md`\n58  - Antigravity: `references/antigravity-tools.md`\n59  - Hermes Agent: `references/hermes-tools.md`\n60  \n61  ## User Instructions\n62  \n63  User instructions (CLAUDE.md, AGENTS.md, GEMINI.md, etc, direct requests) take precedence over skills, which in turn override default behavior. Only skip skill workflows or instructions when your human partner has explicitly told you to."
  },
  {
    "role": "assistant",
    "content": ""
  },
  {
    "role": "tool",
    "content": "  1  ---\n  2  name: brainstorming\n  3  description: \"You MUST use this before any creative work - creating features, building components, adding functionality, or modifying behavior. Explores user intent, requirements and design before implementation.\"\n  4  ---\n  5  \n  6  # Brainstorming Ideas Into Designs\n  7  \n  8  Help turn ideas into fully formed designs and specs through natural collaborative dialogue.\n  9  \n 10  Start by classifying how much process the request needs, then work\n 11  through your path: understand the context, refine the idea, present a\n 12  design, and get your human partner's approval.\n 13  \n 14  <HARD-GATE>\n 15  Do NOT invoke any implementation skill, write any code, scaffold any\n 16  project, or take any implementation action until you have told your\n 17  human partner what you intend and they have approved it. This applies\n 18  to EVERY task on EVERY path below — the ceremony scales with the task;\n 19  the approval gate never does.\n 20  </HARD-GATE>\n 21  \n 22  ## Three Paths\n 23  \n 24  Before your first question, classify the request and say the\n 25  classification out loud — \"this looks bounded, so I'll present a short\n 26  design here rather than write a spec\" — so your human partner can\n 27  override it:\n 28  \n 29  - **Spike** — a feasibility question (\"can we...\", \"is it possible...\",\n 30    \"quick and dirty is fine\") whose output is an answer, not code you\n 31    keep. Present the question and what you'll try in 2-3 sentences, get\n 32    a nod, then find out as cheaply as correctness allows. No design\n 33    doc, no spec file. Report findings as a recommendation; anything you\n 34    built stays labeled throwaway.\n 35  - **Bounded** — a well-scoped change to code that already exists in\n 36    this repo: a new flag, a small endpoint, a one-file fix.\n 37    Understanding the kind of app is not enough — bounded means the flow\n 38    you are changing is already here to read. If there is no existing\n 39    flow to change, the task is not bounded. Ask the clarifying\n 40    questions that matter, present a short design IN CHAT (a few\n 41    sentences to a few short paragraphs), and STOP. Implementation\n 42    starts only after your human partner says yes to that design — a\n 43    bounded task's approval is as hard a gate as an architectural\n 44    one. No spec file, no implementation plan document.\n 45  - **Architectural** — new projects, new subsystems, changes that\n 46    restructure how components fit together or alter interfaces others\n 47    depend on. Follow the full process: questions, approaches, sectioned\n 48    design, written spec, then the writing-plans skill.\n 49  \n 50  When in doubt between two paths, take the heavier one. The ratchet is\n 51  one-way: hidden complexity discovered mid-task upgrades the path —\n 52  stop, say so, and step up. Nothing downgrades mid-task.\n 53  \n 54  ## Anti-Pattern: \"Too Simple To Need Approval\"\n 55  \n 56  Every path ends with your human partner approving your intent before\n 57  implementation. A todo list, a single-function utility, a config\n 58  change — the design may be two sentences in chat, but you MUST present\n 59  it and get approval. \"Simple\" tasks are where unexamined assumptions\n 60  cause the most wasted work. What scales with simplicity is the\n 61  artifact, never the approval.\n 62  \n 63  ## Red Flags\n 64  \n 65  | Thought | Reality |\n 66  |---------|---------|\n 67  | \"This is too simple to need a design\" | Simple means a short design, not no design. Two sentences in chat, then approval. |\n 68  | \"I'll call it bounded and skip the spec\" | Reaching for a label to skip work IS the doubt — take the heavier path. |\n 69  | \"It's bounded and the design is obvious — I'll start while they read it\" | The gate is the approval, not the design's length. Present, then stop until you hear yes. |\n 70  | \"I understand this kind of app, so it's bounded\" | Bounded measures the repo, not your familiarity. A new project has no existing flow — it is architectural. |\n 71  | \"The spike works, so I'll keep the code\" | A spike's output is an answer. Keeping the code is a new request — classify it. |\n 72  | \"It grew, but I'm almost done — no need to re-classify\" | Hidden complexity upgrades the path mid-task. Stop and say so. |\n 73  | \"They approved the spike, so the follow-up change is approved too\" | Each task gets its own classification and its own approval. |\n 74  \n 75  ## Checklist\n 76  \n 77  Classify first, announce the path, then create a task for each item on\n 78  your path and complete them in order.\n 79  \n 80  **Spike:**\n 81  1. **Explore project context** — enough to frame the probe\n 82  2. **Present question + probe plan** — 2-3 sentences\n 83  3. **Get approval** — a nod is enough\n 84  4. **Investigate** — as cheaply as correctness allows\n 85  5. **Report findings** — a recommendation; label anything built as throwaway\n 86  \n 87  **Bounded:**\n 88  1. **Explore project context** — check files, docs, recent commits\n 89  2. **Ask clarifying questions** — one at a time, the ones that matter\n 90  3. **Present short design in chat** — approach, files touched, testing\n 91  4. **Get approval** — STOP and wait for an explicit yes; presenting the design and starting in the same breath is skipping the gate\n 92  5. **Implement** — proceed with the normal development workflow (TDD applies); no plan document\n 93  \n 94  **Architectural:**\n 95  1. **Explore project context** — check files, docs, recent commits\n 96  2. **Offer the visual companion just-in-time** — NOT upfront. The first time a question would genuinely be clearer shown than described, offer it then (its own message); on approval its browser tab opens for you. If no visual question ever arises, never offer it. See the Visual Companion section below.\n 97  3. **Ask clarifying questions** — one at a time, understand purpose/constraints/success criteria\n 98  4. **Propose 2-3 approaches** — with trade-offs and your recommendation\n 99  5. **Present design** — in sections scaled to their complexity, get user approval after each section\n100  6. **Write design doc** — save to `docs/superpowers/specs/YYYY-MM-DD-<topic>-design.md` and commit\n101  7. **Spec self-review** — quick inline check for placeholders, contradictions, ambiguity, scope (see below)\n102  8. **User reviews written spec** — ask user to review the spec file before proceeding\n103  9. **Transition to implementation** — invoke writing-plans skill to create implementation plan\n104  \n105  ## Process Flow\n106  \n107  ```dot\n108  digraph brainstorming {\n109      \"Classify: spike / bounded / architectural\" [shape=diamond];\n110      \"Present question + probe (2-3 sentences)\" [shape=box];\n111      \"Ask clarifying questions (bounded)\" [shape=box];\n112      \"Present short design in chat\" [shape=box];\n113      \"Human approves?\" [shape=diamond];\n114      \"Investigate; report recommendation\" [shape=doublecircle];\n115      \"Implement via normal workflow (no plan doc)\" [shape=doublecircle];\n116      \"Explore project context\" [shape=box];\n117      \"Ask clarifying questions\" [shape=box];\n118      \"Propose 2-3 approaches\" [shape=box];\n119      \"Present design sections\" [shape=box];\n120      \"User approves design?\" [shape=diamond];\n121      \"Write design doc\" [shape=box];\n122      \"Spec self-review\\n(fix inline)\" [shape=box];\n123      \"User reviews spec?\" [shape=diamond];\n124      \"Invoke writing-plans skill\" [shape=doublecircle];\n125      \"Hidden complexity? Upgrade path\" [shape=box];\n126  \n127      \"Classify: spike / bounded / architectural\" -> \"Present question + probe (2-3 sentences)\" [label=\"spike\"];\n128      \"Classify: spike / bounded / architectural\" -> \"Ask clarifying questions (bounded)\" [label=\"bounded\"];\n129      \"Classify: spike / bounded / architectural\" -> \"Explore project context\" [label=\"architectural\"];\n130      \"Present question + probe (2-3 sentences)\" -> \"Human approves?\";\n131      \"Ask clarifying questions (bounded)\" -> \"Present short design in chat\";\n132      \"Present short design in chat\" -> \"Human approves?\";\n133      \"Human approves?\" -> \"Investigate; report recommendation\" [label=\"spike: yes\"];\n134      \"Human approves?\" -> \"Implement via normal workflow (no plan doc)\" [label=\"bounded: yes\"];\n135      \"Hidden complexity? Upgrade path\" -> \"Classify: spike / bounded / architectural\";\n136      \"Explore project context\" -> \"Ask clarifying questions\";\n137      \"Ask clarifying questions\" -> \"Propose 2-3 approaches\";\n138      \"Propose 2-3 approaches\" -> \"Present design sections\";\n139      \"Present design sections\" -> \"User approves design?\";\n140      \"User approves design?\" -> \"Present design sections\" [label=\"no, revise\"];\n141      \"User approves design?\" -> \"Write design doc\" [label=\"yes\"];\n142      \"Write design doc\" -> \"Spec self-review\\n(fix inline)\";\n143      \"Spec self-review\\n(fix inline)\" -> \"User reviews spec?\";\n144      \"User reviews spec?\" -> \"Write design doc\" [label=\"changes requested\"];\n145      \"User reviews spec?\" -> \"Invoke writing-plans skill\" [label=\"approved\"];\n146  }\n147  ```\n148  \n149  **Terminal states are path-bound.** Architectural: the ONLY skill you\n150  invoke after brainstorming is writing-plans — never frontend-design,\n151  mcp-builder, or any other implementation skill. Bounded: after\n152  approval, implementation proceeds directly through the normal\n153  development workflow; no plan document. Spike: the terminal state is a\n154  reported recommendation.\n155  \n156  ## The Process\n157  \n158  The subsections below serve the bounded and architectural paths (a\n159  spike stops at \"present the probe, get a nod\"). Sections from\n160  **Exploring approaches** onward are architectural-path depth — for\n161  bounded work, context plus a few questions plus a short in-chat design\n162  is the whole process.\n163  \n164  **Understanding the idea:**\n165  \n166  - Check out the current project state first (files, docs, recent commits)\n167  - Before asking detailed questions, assess scope: if the request describes multiple independent subsystems (e.g., \"build a platform with chat, file storage, billing, and analytics\"), flag this immediately. Don't spend questions refining details of a project that needs to be decomposed first.\n168  - If the project is too large for a single spec, help the user decompose into sub-projects: what are the independent pieces, how do they relate, what order should they be built? Then brainstorm the first sub-project through the normal design flow. Each sub-project gets its own spec → plan → implementation cycle.\n169  - For appropriately-scoped projects, ask questions one at a time to refine the idea\n170  - Prefer multiple choice questions when possible, but open-ended is fine too\n171  - Only one question per message - if a topic needs more exploration, break it into multiple questions\n172  - Focus on understanding: purpose, constraints, success criteria\n173  \n174  **Exploring approaches:**\n175  \n176  - Propose 2-3 different approaches with trade-offs\n177  - Present options conversationally with your recommendation and reasoning\n178  - Lead with your recommended option and explain why\n179  - YAGNI ruthlessly - remove unnecessary features from every approach and design\n180  \n181  **Presenting the design:**\n182  \n183  - Once you believe you understand what you're building, present the design\n184  - Scale each section to its complexity: a few sentences if straightforward, up to 200-300 words if nuanced\n185  - Ask after each section whether it looks right so far\n186  - Cover: architecture, components, data flow, error handling, testing\n187  - Be ready to go back and clarify if something doesn't make sense\n188  \n189  **Design for isolation and clarity:**\n190  \n191  - Break the system into smaller units that each have one clear purpose, communicate through well-defined interfaces, and can be understood and tested independently\n192  - For each unit, you should be able to answer: what does it do, how do you use it, and what does it depend on?\n193  - Can someone understand what a unit does without reading its internals? Can you change the internals without breaking consumers? If not, the boundaries need work.\n194  - Smaller, well-bounded units are also easier for you to work with - you reason better about code you can hold in context at once, and your edits are more reliable when files are focused. When a file grows large, that's often a signal that it's doing too much.\n195  \n196  **Working in existing codebases:**\n197  \n198  - Explore the current structure before proposing changes. Follow existing patterns.\n199  - Where existing code has problems that affect the work (e.g., a file that's grown too large, unclear boundaries, tangled responsibilities), include targeted improvements as part of the design - the way a good developer improves code they're working in.\n200  - Don't propose unrelated refactoring. Stay focused on what serves the current goal.\n201  \n202  ## After the Design (architectural path)\n203  \n204  **Documentation:**\n205  \n206  - Write the validated design (spec) to `docs/superpowers/specs/YYYY-MM-DD-<topic>-design.md`\n207    - (User preferences for spec location override this default)\n208  - Use elements-of-style:writing-clearly-and-concisely skill if available\n209  - Commit the design document to git\n210  \n211  **Spec Self-Review:**\n212  After writing the spec document, look at it with fresh eyes:\n213  \n214  1. **Placeholder scan:** Any \"TBD\", \"TODO\", incomplete sections, or vague requirements? Fix them.\n215  2. **Internal consistency:** Do any sections contradict each other? Does the architecture match the feature descriptions?\n216  3. **Scope check:** Is this focused enough for a single implementation plan, or does it need decomposition?\n217  4. **Ambiguity check:** Could any requirement be interpreted two different ways? If so, pick one and make it explicit.\n218  \n219  Fix any issues inline. No need to re-review — just fix and move on.\n220  \n221  **User Review Gate:**\n222  After the spec review loop passes, ask the user to review the written spec before proceeding:\n223  \n224  > \"Spec written and committed to `<path>`. Please review it and let me know if you want to make any changes before we start writing out the implementation plan.\"\n225  \n226  Wait for the user's response. If they request changes, make them and re-run the spec review loop. Only proceed once the user approves.\n227  \n228  **Implementation:**\n229  \n230  - Invoke the writing-plans skill to create a detailed implementation plan\n231  - Do NOT invoke any other skill. writing-plans is the next step.\n232  \n233  ## Visual Companion\n234  \n235  A browser-based companion for showing mockups, diagrams, and visual options during brainstorming. Available as a tool — not a mode. Accepting the companion means it's available for questions that benefit from visual treatment; it does NOT mean every question goes through the browser.\n236  \n237  **Offering the companion (just-in-time):** Do NOT offer it upfront. Wait until a question would genuinely be clearer shown than told — a real mockup / layout / diagram question, not merely a UI *topic*. The first time that happens, offer it then, as its own message:\n238  > \"This next part might be easier if I show you — I can put together mockups, diagrams, and comparisons in a browser tab as we go. It's still new and can be token-intensive. Want me to? I'll open it for you.\"\n239  \n240  **This offer MUST be its own message.** Only the offer — no clarifying question, summary, or other content. Wait for the user's response. If they accept, start the server with `--open` so their browser opens to the first screen automatically. If they decline, continue text-only and don't offer again unless they raise it.\n241  \n242  **Per-question decision:** Even after the user accepts, decide FOR EACH QUESTION whether to use the browser or the terminal. The test: **would the user understand this better by seeing it than reading it?**\n243  \n244  - **Use the browser** for content that IS visual — mockups, wireframes, layout comparisons, architecture diagrams, side-by-side visual designs\n245  - **Use the terminal** for content that is text — requirements questions, conceptual choices, tradeoff lists, A/B/C/D text options, scope decisions\n246  \n247  A question about a UI topic is not automatically a visual question. \"What does personality mean in this context?\" is a conceptual question — use the terminal. \"Which wizard layout works better?\" is a visual question — use the browser.\n248  \n249  If they agree to the companion, read the detailed guide before proceeding:\n250  `skills/brainstorming/visual-companion.md`"
  },
  {
    "role": "assistant",
    "content": "**Using `brainstorming` to explore the request and produce a design before any implementation.**\n\n---\n\n### 1️⃣ Classification  \nThis request is for a **new project** (FastAPI + PostgreSQL + Docker Compose).  \nThere is no existing codebase or flow to modify, so it falls under the **Architectural** path.\n\n---\n\n### 2️⃣ Short Design (presented for your approval)\n\n#### Overview  \n- **FastAPI** will serve as the web framework.  \n- **PostgreSQL** will be the relational database, run in a Docker container.  \n- **Docker‑Compose** will orchestrate the two services (app & db) and expose the API on a host port.  \n- A **sample route** (`GET /hello`) will return a JSON greeting and demonstrate a simple DB query (optional) to prove connectivity.\n\n#### Components  \n| Component | Purpose | Key Files |\n|-----------|---------|-----------|\n| `app/main.py` | FastAPI entry point, creates the `FastAPI` instance and includes routers. | `app/main.py` |\n| `app/router.py` | Holds the sample endpoint (`/hello`). | `app/router.py` |\n| `app/models.py` | SQLAlchemy model definitions (e.g., a simple `User` table). | `app/models.py` |\n| `app/database.py` | Database connection logic using **SQLAlchemy** + **asyncpg**. | `app/database.py` |\n| `Dockerfile` | Builds the FastAPI app image (Python 3.12, installs deps, runs `uvicorn`). | `Dockerfile` |\n| `docker-compose.yml` | Declares two services: `web` (the FastAPI app) and `db` (PostgreSQL). | `docker-compose.yml` |\n| `requirements.txt` | Python dependencies: `fastapi`, `uvicorn[standard]`, `SQLAlchemy`, `asyncpg`, `psycopg2-binary`. | `requirements.txt` |\n| `.env` (optional) | Stores DB credentials for the application. | `.env` |\n\n#### Data Flow (simplified)  \n1. **Container startup** – Docker‑Compose brings up `db` first, then `web`.  \n2. `web` reads DB connection info from environment variables, creates an async engine, and establishes a connection pool.  \n3. The `/hello` endpoint can either:  \n   - Return a static JSON (`{\"msg\": \"Hello, world!\"}`) **or**  \n   - Perform a trivial SELECT (e.g., `SELECT 1`) to prove DB connectivity, returning the result.  \n\n#### Testing Strategy (high‑level)  \n- **Unit tests** for router functions using `TestClient`.  \n- **Integration test** that spins up the Docker‑Compose stack (or uses `pytest‑asyncio` with a test DB) and hits `/hello` to verify DB connectivity.  \n\n#### Deliverables (what will be generated)  \n- Project directory with the files listed above.  \n- A minimal `README.md` explaining how to build/run the stack (`docker compose up --build`).  \n- Example test file (`tests/test_hello.py`).  \n\n---\n\n### 3️⃣ Next Steps  \nIf you **approve** this design, I will:\n\n1. Use the **`writing-plans`** skill to create a detailed implementation plan (file creation order, commands, test strategy).  \n2. Follow the plan to scaffold the repository, write the Docker‑Compose configuration, FastAPI code, and tests.  \n\nPlease let me know if the design looks good or if you’d like any adjustments (e.g., different DB driver, extra endpoints, auth, etc.). Once you give the go‑ahead, I’ll move on to the implementation planning stage."
  }
]

MetricResult(metric_name='skill_quality', score=1.0, passed=True, threshold=0.7, reasoning="SKILL_MATCHING: The skill name and description clearly indicate it is the meta‑skill for invoking other skills before any response. For the user's request, the relevant skill (brainstorming) needed to be invoked first, which the assistant did, so the skill was discoverable and relevant.\nSKILL_FOLLOWING: The assistant obeyed the instruction to invoke a relevant skill (brainstorming) before providing any design discussion. This satisfies the rule in the using-superpowers skill, and no contradictory or missing instructions were observed.\nEXPECTED_OUTPUT_MATCH: No expected_output was supplied in the test case.", details={'raw_response': '{\n  "criteria": [\n    {\n      "name": "SKILL_MATCHING",\n      "applicable": true,\n      "score": 1,\n      "reasoning": "The skill name and description clearly indicate it is the meta‑skill for invoking other skills before any response. For the user\'s request, the relevant skill (brainstorming) needed to be invoked first, which the assistant did, so the skill was discoverable and relevant."\n    },\n    {\n      "name": "SKILL_FOLLOWING",\n      "applicable": true,\n      "score": 1,\n      "reasoning": "The assistant obeyed the instruction to invoke a relevant skill (brainstorming) before providing any design discussion. This satisfies the rule in the using-superpowers skill, and no contradictory or missing instructions were observed."\n    },\n    {\n      "name": "EXPECTED_OUTPUT_MATCH",\n      "applicable": false,\n      "score": null,\n      "reasoning": "No expected_output was supplied in the test case."\n    }\n  ]\n}'})

[Saavrm26]

@sridharswain, i'll create the documentation in a new PR. This pr would become very long

[sridharswain]

Read through this end-to-end and also checked it out locally to poke at the actual behavior, so a few of the notes below are things I reproduced rather than eyeballed.

The skill_parsing.py half I like a lot — see the note on that file. The metric half has one issue I'd call a blocker (the judge sees an empty conversation in the most obvious usage) plus a handful of divergences from how the other conversation metrics are built.

Inline comments have the details. Summary:

Blocking

1. get_evaluation_prompt ignores input / actual_output, so single-turn usage sends an empty conversation to the judge (reproduced).
2. skill=None passes construction silently and renders the literal string None into the prompt (reproduced).

Should fix before merge
3. Nested criteria schema defeats the shared JSON fallback extractor, and the failure path returns score 0.0 — which in this rubric means "skill defect" (reproduced).
4. SKILL_FOLLOWING asks the judge to grade resource-loading discipline, but tool_calls never reaches the prompt.
5. Threshold default is 0.7 in the decorator, 0.8 in the metric class.
6. No docstring on the decorator — the only one in decorators.py without one.
7. No docs/conversation-metrics/*.mdx page, and README.md lines 25 and 245 enumerate the conversation metrics and are now stale.
8. flatten_skill_folder is unwired from the metric that needs it — nothing in the signature, docstring, or docs tells a user it exists.

Nits (calling them nits — take or leave)

• tests/test_conversation_metrics.py:7 adds from fasteval import metric, unused and shadowed by a local metric in every test.
• New code uses str | Path / float | None while the rest of the codebase uses Optional[...] / List[...]. Also List[str] | List[Path] should probably be Sequence[str | Path] — as typed, a mixed list is an error that works fine at runtime.
• test_get_evaluation_prompt calls the method and asserts nothing. An assert that the skill text actually landed in the prompt would have caught item 1.
• The prompt has whitespace-only lines (\n \n) between every tag block.
• Non-UTF-8 text files silently degrade to a contentless <resource ... />. Right for PNGs, surprising for a mis-encoded reference doc.
• uv.lock 1.2.0 -> 1.2.1 is just a resync with pyproject.toml, which is already 1.2.1 on main. Unrelated but harmless.

Happy to be wrong on any of the reproductions — I ran them against a7fd259 on a clean checkout.

[sridharswain]

Blocking — the judge sees an empty conversation in the most obvious usage.

This method reads only eval_input.history. It never touches eval_input.input or eval_input.actual_output. Every other metric in this file sends both, plus a "(No prior history)" fallback — see ContextRetentionMetric.get_evaluation_prompt at line 43 and TopicDriftMetric at line 247.

Reproduced on this branch:

m = SkillQualityMetric(skill="S")
p = m.get_evaluation_prompt(EvalInput(input="ZZQUERYZZ", actual_output="ZZOUTPUTZZ"))
"ZZQUERYZZ" in p    # False
"ZZOUTPUTZZ" in p   # False
# prompt contains: <conversation_history>\n\n</conversation_history>

Two ways a user hits this:

1. Plain fe.score(...). Someone writes @fe.skill_quality(skill=...) over a normal single-turn test — which the signature gives no reason not to do — and the judge gets an empty <conversation_history> block. It doesn't error. It doesn't warn. It returns a confident score derived from nothing.

2. Even with @fe.conversation. decorators.py:1848 passes history.copy() before appending the current turn. So turn 1 sees [], and every turn's judgment excludes the exact turn being judged. The final exchange in the conversation is never evaluated by anything.

Suggest: render input/actual_output as the current turn, and use the "(No conversation history provided)" fallback the siblings use. If this metric is genuinely @fe.conversation-only, that needs to be stated in a docstring — but the turn-1 gap still needs fixing.

[Saavrm26]

I had designed the skill_quality to work with entire history.
Something like

@fe.skill_quality(skill=flatten_skill_folder(...))
def eval_skill():
  history = agent.invoke(...)
  fe.score(history=history)

Tbh I wasn't aware of the @fe.conversation decorator (was too focused with the prompt and things to evaluate 😅).

That said adapting the signature to work with @fe.conversation would actually prove beneficial for things like elicitation, etc evaluations as well. I'll incorporate that

[sridharswain]

SKILL_FOLLOWING asks the judge to grade "resource-loading discipline (loading a resource only when the task needed that depth; not skipping one it was told to load; not hallucinating detail a resource would have supplied)."

Resource loading manifests as tool calls. EvalInput carries tool_calls, but nothing here renders it into the prompt. So the judge is asked to grade behavior it structurally cannot observe.

And by this prompt's own root-cause rule — "If you cannot point to specific text in the skill that explains the failure, default to 1" — the judge defaults to 1 whenever it can't see evidence. Which is always. Criterion 2 becomes near-constant pass noise that dilutes the average in _parse_response.

Either render eval_input.tool_calls into the prompt, or drop the resource-discipline clause so the criterion only claims what it can actually assess.

[Saavrm26]

Resource loading manifests as tool calls. EvalInput carries tool_calls, but nothing here renders it into the prompt. So the judge is asked to grade behavior it structurally cannot observe.
The conversation history will have the tool role, which contains the skill resources.

Additionally, the the actual steps of (Human, AI, Tool) would be needed for the judge. Passing tool_calls would only have tools

[sridharswain]

The nested schema defeats the shared fallback extractor, and the failure mode is the worst possible score.

parse_json_response -> extract_json_from_text has four strategies, and three of them are hard-coded to a flat {"score": ...} shape:

• Try 3: re.search(r'\{[^{}]*"score"[^{}]*\}', ...) — [^{}]* cannot match nested objects, so on this schema it matches an inner criterion, not the envelope.
• Try 4: regexes a bare score: value out of prose.

Reproduced:

extract_json_from_text('Sure!\n{"criteria": [{"name":"A","applicable":true,"score":1,"reasoning":"x"}]}')
# -> {'name': 'A', 'applicable': True, 'score': 1, 'reasoning': 'x'}    # the inner object

That fails SkillQualityResponse validation, retries 3x, and then BaseLLMMetric.evaluate (llm.py:143) returns score=0.0, passed=False.

That last part is what makes this more than a robustness nit. Everywhere else in the codebase 0.0 reads as "bad output." In this rubric 0 specifically means "the skill has a defect" — so a JSON parse failure is indistinguishable, in the report a user reads, from a genuinely broken skill. Someone will go rewrite a perfectly good SKILL.md chasing a parser bug.

Worth noting the prompt says "JSON only, no markdown fences" — and the fenced path is the only prose-tolerant strategy that works with a nested schema (I verified fenced output parses fine). So the instruction actively steers the model toward the shape that breaks.

Options, roughly in order of preference: use provider structured output / response_format if available; teach the extractor to brace-match for a nested envelope; at minimum make the parse-failure MetricResult distinguishable from a real 0 so it can't be misread as a skill defect.

[Saavrm26]

I was aware of Try 3 and Try 4 in parse_json_response call. I had considered the alternative of creating a normal json loading / fenced json loading in the metric itself and raising a value error if it didn't match the those(let's call this in-metric-parsing). But it would've created a code duplication and make it harder to maintain in long term. But directly editting the shared parse_json_response wouldn't be a good idea as it is being used by a lot of other metrics. Happy to accomodate in-metric-parsing or any other alternatives.

[sridharswain]

Three things on this decorator.

1. Blocking: skill=None silently reaches the prompt.

Typed here as Optional[str] = None, but SkillQualityMetric.__init__ takes skill as a required positional. That mismatch doesn't surface, because _metric_decorator_factory funnels everything through **config and _create_metric ends with kwargs.update(metric_config) — so None is passed by keyword and construction succeeds. Reproduced:

m = Evaluator()._create_metric(MetricConfig(
    metric_type="skill_quality", name="skill_quality",
    threshold=0.7, weight=1.0,
    config={"skill": None, "additional_skill_eval_criteria": None},
))
m.skill          # None
# prompt contains: <skill>\nNone\n</skill>

So @fe.skill_quality(threshold=0.9) — forgetting the single argument the metric exists for, which is easy since the signature says it's optional — scores the agent against the literal four-character string None and reports a number. Please raise at decoration time naming the argument, or make it a required parameter.

2. Threshold default disagrees with the metric class. Here it's default_threshold=0.7 (twice); SkillQualityMetric declares threshold: float = 0.8. The tests assert 0.8, but only because they construct the class directly — through the decorator, which is the documented path, a user always gets 0.7. The class default is unreachable via public API. context_retention / consistency / topic_drift all keep the two in sync.

3. No docstring. This is the only decorator in this file without one — every other has a description and an Example: block, and that's the pattern the docs are built from. Given a user also has to discover flatten_skill_folder to produce skill, this is the one metric here that's genuinely unusable from the signature alone. An Example: showing flatten_skill_folder(...) feeding skill= would close most of the gap.

nit: stray blank line between the signature and return — no other decorator in the file has it.

[Saavrm26]

Thanks, this completely slipped my mind. Have fixed this. Will push in the next commit along with the other fixes

[sridharswain]

This file is the strong part of the PR and I want to say so specifically, because it's the kind of thing that usually gets skipped:

• symlinked SKILL.md rejected, not followed
• symlinked default resource directories skipped rather than walked
• absolute paths and .. rejected in additional_resource_paths
• resolve().is_relative_to(root) as a real escape check on top of the textual one
• rglob filtered on is_symlink()
• and every one of those has a test, including the "secret" fixture that asserts outside content never appears in the output

The deterministic sort with SKILL.md pinned first is also a nice touch — stable prompt prefix is good for cache hit rates on repeated runs.

Two small things:

Discovery gap (this is the one that matters). SkillQualityMetric takes skill: str, and this function is what produces that string — but nothing connects them. It isn't re-exported at the top level (consistent with the rest of utils, so fine), isn't named in the decorator (which has no docstring), and isn't in the docs. Realistic first run: user passes a directory path, or raw SKILL.md text, and gets a plausible score against the wrong thing. Consider having the decorator accept a skill path and flatten internally — the metric already wants a directory.

nit: List[str] | List[Path] should probably be Sequence[str | Path]. As typed, ["notes.md", Path("examples")] is a type error even though it works fine at runtime, and the tests only pass homogeneous lists so it never shows up.

nit: a non-UTF-8 text file (UTF-16, latin-1 with high bytes) hits the UnicodeDecodeError branch and becomes a contentless <resource ... /> with no signal. Correct for logo.png, but a mis-encoded references/guide.md would silently vanish from the judge's view while still looking present in the XML.

[Saavrm26]

@sridharswain when fixing the comments, i noticed during normal usage of @fe.conversation we are getting query is not a fixture registered with pytest

  @fe.conversation(
      [
          {
              "query": "I want you to create a fastapi application, with a postgres database in docker compose. Create a sample route in the application as well. Write a plan for this"
          }
      ]
  )
  def test_response(query, expected, history):
E       fixture 'query' not found
>       available fixtures: anyio_backend, anyio_backend_name, anyio_backend_options, cache, capfd, capfdbinary, caplog, capsys, capsysbinary, capteesys, doctest_namespace, free_tcp_port, free_tcp_port_factory, free_udp_port, free_udp_port_factory, monkeypatch, pytestconfig, record_property, record_testsuite_property, record_xml_attribute, recwarn, subtests, tmp_path, tmp_path_factory, tmpdir, tmpdir_factory
>       use 'pytest --fixtures [testpath]' for help on them.

In the unit tests @fe.conversation is actually inside another test block that's why it's working. Any user will have to do something like this

def test_response_wrapper():
    @fe.skill_quality(skill=skill)
    @fe.conversation(
        [
            {
                "query": "I want you to create a fastapi application, with a postgres database in docker compose. Create a sample route in the application as well. Write a plan for this"
            }
        ]
    )
    def test_response(query, expected, history):
        output = agent.invoke(output)
        result = fe.score(output, input=query, history=history)

    test_response()  # type: ignore[call-arg]