feat(chisel): Phase B — eval harness + canonical queries - #212
Open
jpb33333 wants to merge 2 commits into
Open
Conversation
Production-discipline regression net: each tool / workflow ships a canonical_queries.yaml beside its manifest declaring user_query → expected_plan (tool order + arg includes/excludes) and optional expected_prose (substring includes/excludes per plan §11.6 — no regex). The eval harness runs them through the planner and reports per-query pass/fail. Framework (pebble/chisel/eval.py): - Pydantic shapes for canonical_queries.yaml — strict (extra=forbid), id format validated, expected_plan/expected_prose optional - load_canonical_queries walks tools/ + workflows/, raises with the source path on malformed files - assert_plan: positional step match, arg_includes (subset), arg_excludes (forbidden keys), step_count tolerance for extra steps - assert_prose: substring includes/excludes (deliberately not regex) - run_plan_eval drives one query through Planner.plan(); skip_reason short-circuits with passed=True so the report stays signal-rich - format_results: PASS/FAIL/SKIP summary with per-failure detail CLI: - chisel eval [--unit NAME] [--tag LABEL] - ANTHROPIC_API_KEY absent → schema-validates and exits 0 without burning tokens (useful for local dev + offline CI smoke) - API key present → calls live planner, exits 1 on any failure Fixtures (planner-only assertions; full-pipeline prose checks deferred to the gated CI run per plan §11.12): - search_crm: 2 NL queries - get_record: 1 explicit-id query - aggregate_pipeline_views: 3 NL queries - generate_chart, request_human_review: skip_reason — both downstream of upstream state that single-query planner eval can't deterministically provoke - weekly_pipeline_review: NL path covered; /pipeline slash path documented as bypass-planner (build_plan.py is unit-tested) Tests (21 cases): - canonical_queries.yaml schema validation (id format, extra=forbid, empty tool rejection) - assert_plan: empty-expected pass, arg_includes happy + miss-key + value-mismatch, arg_excludes forbidden-key, step_count, wrong-tool, extra-step tolerance - assert_prose: includes + excludes pass/fail - loader: real chisel root discovery, malformed file → ValueError with file path - run_plan_eval: stub planner happy / planner-error / skip paths - format_results rendering CI workflow file deferred — repo currently has no .github/workflows/. Adding eval-gate.yml without aligning to the team's CI surface (Buildkite? external runner?) is premature; tests run locally via pytest and the CLI is the canonical entry point in the meantime. Suite: 668 pass (647 + 21 new).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, push a new commit or reopen this pull request to trigger a review.
…pse maps
Six-pass review surfaced ~135 lines of dead/speculative code and a few
overbuilt abstractions. This commit removes them, collapses the slash/
intent/plan-builder triple-map into one ``WorkflowEntry``, and moves
autoload to chisel-package import so production wiring doesn't depend
on streaming.py being imported first.
Behavior preserved (664 pass, was 668 — net -4 = removed tests for
deleted features, +2 for new convention).
Tier 1 — correctness:
- Autoload now runs at ``pebble.chisel`` import; ``streaming.py`` no
longer needs the explicit ``_chisel.autoload()`` call. Eliminates the
import-order fragility where router.py could see empty maps if loaded
before streaming.
- WorkflowManifest auto-fills ``dispatch_intent = workflow_<name>`` if
omitted. Closes the slash-without-intent silent-dispatch gap.
- Lints run at autoload time and surface in ``AutoloadReport.lint_warnings``
(advisory — registration still proceeds). ``chisel validate`` exits
non-zero when any handler under the loaded tree has a lint hit.
- schema.py: drop the dead ``setdefault`` + unconditional-overwrite
pair; collapse two deepcopies into one in-place mutator.
Tier 2 — kill dead code (~210 lines removed):
- Drop ``ToolManifest.eval_fixtures`` / ``lint_overrides`` / ``scope``
/ ``requires_permission`` / ``output_kind`` (no reader anywhere).
- Drop ``WorkflowManifest.requires_permission`` / ``scope``.
- Collapse ``FixedCost | VariableCost`` union → flat ``cost_estimate_usd:
float``. No tool uses variable-cost; YAML is simpler.
- Delete ``pebble/chisel/rbac.py`` entirely. Sprint-12 RBAC isn't
wired anywhere; resurrect when it lands.
- Delete ``lints._check_no_env_in_run`` — trivially evadable via
``from os import environ``, load-bearing only if enforced robustly.
- Delete ``schema.assert_strict`` — test-only sugar; tests assert the
invariant directly.
- Delete ``lints.lint_handler_module`` ``overrides=`` arg + manifest's
``lint_overrides`` field. No production handler used them.
- Delete ``AutoloadReport.ok()`` method — one caller, inlined.
Tier 3 — collapse abstractions:
- Replace ``slash_command_map`` + ``slash_to_intent`` + ``dispatch_workflow``
+ ``build_workflow_plan`` with two lookups: ``lookup_slash(slash) ->
WorkflowEntry | None`` and ``lookup_intent(intent) -> WorkflowEntry |
None``. ``WorkflowEntry`` is a frozen dataclass carrying name +
dispatch_intent + slash_command + build_plan. One shape, one
vocabulary, no reverse-scans.
- Slim ``HandlerContext`` from 40 lines of per-property forwarders to
a 12-line ``__getattr__`` delegate. ``ctx.user_email`` /
``ctx.http_client`` still work; ``ctx.cite()`` and ``ctx.citations``
unchanged.
- Refactor adapter to use a ``_fail()`` helper, halving the
ToolResult construction repetition.
Tier 4 — DX:
- conftest autouse fixture drops the redundant post-yield autoload
call; saves ~50ms per test across 664 tests.
- ``chisel validate`` (no path) now lints every loaded handler by
default, not just the autoload errors. Default behaviour matches
the documented purpose.
Manifests updated: 5 tool manifests + weekly_pipeline_review/workflow.yaml
lose ``output_kind`` / ``scope`` / nested ``cost_estimate: {fixed: ...}``
in favor of flat ``cost_estimate_usd: 0.0``.
Total: 22 files changed, 340 insertions, 654 deletions (net -314).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase B of the Chisel plan — production-discipline regression net. Each tool / workflow ships a
canonical_queries.yamlbeside its manifest;chisel evalruns them through the planner and reports pass/fail with per-step diff.Stacked on #211 (Phase A). Base branch is
feat/pebble-chisel-scoping; rebase todevafter #211 lands.What's in the PR
pebble/chisel/eval.py— Pydantic schema, loader,assert_plan/assert_prose,run_plan_eval,format_resultschisel evalCLI subcommand (filters by--unitand--tag)pebble/tests/test_chisel_eval.py)Design calls
.github/workflows/directory; droppingeval-gate.ymlblind to the team's actual CI surface (Buildkite? external runner?) felt premature. Tests run via pytest and the CLI; we can add the gate once we know where it should live.skip_reasonrather than excluded — they document the gap (downstream-of-state queries, slash-bypass-planner) and stay visible for the future gated CI job.Test plan
pytest pebble/tests/test_chisel_eval.py)python -m pebble.chisel.cli eval→ schema-validation passesANTHROPIC_API_KEY=... python -m pebble.chisel.cli eval(reviewer runs locally)What's next
Phase C — GUI builder under
financial_forecasting/frontend-v2/src/pages/chisel/(3–4 wks).🤖 Generated with Claude Code