Plan C Phase 6: deduplicate instrument.clj fdef registry#94
Merged
Conversation
Replace three parallel symbol lists in `src/github/copilot_sdk/instrument.clj` with a single registry populated at fdef-declaration time: * New private `register-fdef!` macro delegates to `s/fdef` and records the fully-qualified symbol in a private `registered-fdefs` atom. * `instrument-all!` and `unstrument-all!` derive their target list from the registry — no more parallel symbol lists. * All 98 hand-written fdefs migrated from `s/fdef` to `register-fdef!`. * Macro fail-fast at macroexpansion time: rejects unqualified symbols and alias-qualified symbols (e.g. `client/foo` where `client` is an `(:require ... :as client)` alias). The latter would otherwise silently leave an instrumentation gap because `'~sym` records the unresolved alias while `s/fdef` registers under the resolved namespace. Workflow change: adding a public API fn now requires editing exactly one place (the `register-fdef!` form) instead of three. AGENTS.md updated. Net diff: instrument.clj 689 → 527 lines (~162 lines removed, no behavior change). `bb test` passes 188/605 0 failures. Reviewed in parallel by Claude Opus 4.7, GPT-5.5 (extra-high reasoning), and GPT-5.3-Codex (extra-high reasoning). Round 1 surfaced one Medium finding (alias-qualified guard gap, also raised by rubber-duck pre-impl) which was fixed; round 2 confirmed no remaining issues. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the duplicated “three parallel lists” maintenance pattern in instrument.clj by introducing a single registry-backed macro for declaring clojure.spec fdefs, and updating docs/changelog accordingly.
Changes:
- Add a private
register-fdef!macro +registered-fdefsregistry to record fdef symbols at definition time. - Migrate all existing public API fdefs to
register-fdef!and simplify(un)instrument-all!to use the registry. - Update contributor instructions and add an Unreleased changelog entry documenting the workflow change.
Show a summary per file
| File | Description |
|---|---|
src/github/copilot_sdk/instrument.clj |
Introduces registry + macro; replaces hard-coded instrument/unstrument symbol vectors with registry-derived set. |
CHANGELOG.md |
Documents the instrumentation workflow change under Unreleased. |
.github/copilot-instructions.md |
Updates the “adding new public functions” workflow to use register-fdef! (single edit). |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 0
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
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.
Plan C Phase 6: deduplicate
instrument.cljfdef registryEliminates the three-parallel-list maintenance burden in
src/github/copilot_sdk/instrument.clj.Problem
Adding a public API function previously required edits in three places:
(s/fdef sym ...)declaration ininstrument.cljstest/instrumentininstrument-all!stest/unstrumentinunstrument-all!This triplication was documented in
AGENTS.mdas a known weakness.Drift between the three lists would silently leave an instrumentation gap
for the missing symbol.
Solution
A single private
register-fdef!macro records each fdef into a singleprivate registry; both
(un)instrument-all!derive from it.All 98 hand-written fdefs were mechanically migrated from
s/fdeftoregister-fdef!. The trailing(instrument-all!)call still works becauseall 98
register-fdef!forms run (and populate the atom) before it.Net effect
instrument.clj: 689 → 527 lines (~162 lines of pure repetition removed)stest/instrumentisidentical to the old hard-coded vector
Defense in depth
The macro fail-fast at macroexpansion time:
client/foowhereclientis a(:require ... :as client)alias).s/fdefresolves aliases atregistration time, but
'~symrecords the unresolved symbol — sostest/instrumentwould receive a symbol that doesn't match any var.This was caught by both the rubber-duck critique (pre-impl) and the
GPT-5.5 round-1 review.
Validation
bb test: 188 tests, 605 assertions, 0 failures, 0 errorsbb ci:full(on the codegen-pipeline branch where this work wasinitially developed before being moved to its own branch): exit 0
and GPT-5.3-Codex (extra-high reasoning). Round 1: one Medium finding
(alias guard) raised by GPT-5.5; fixed. Round 2: clean.
Files changed
src/github/copilot_sdk/instrument.clj— registry + macro + 98 form rewrites + simplified (un)instrument-all!.github/copilot-instructions.md— workflow updated from 4 steps to 3 (single edit replaces "add to fdef list AND instrument-all! AND unstrument-all!")CHANGELOG.md— Unreleased entry under "### Changed (instrumentation)"Plan C progress
This is Phase 6 of Plan C. Phase 1+3+3.5 (codegen pipeline + three-tier
wire/coerce/idiom architecture) is in #93. Phase 7 (mock v3 coverage) and
Phase 5 (RPC fdef codegen — blocked on upstream
api.schema.json) remain.This branch is independent of #93 — it's branched directly from
mainsoit can land independently.