Remove the log_feedback tool from agami-core - #92
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Removes the log_feedback MCP tool and its supporting contracts/ports/adapters so agami-core’s MCP surface is limited to the four product tools (list_datasources, get_datasource_schema, get_prompt_examples, execute_sql). This simplifies the runtime activity logging surface area while keeping the existing DB schema (including the feedback table) intact.
Changes:
- Deleted the
log_feedbacktool implementation and removed it from the sharedTOOLSregistry. - Removed feedback-related contracts and
ActivitySink/sink implementations for recording feedback. - Updated tests and documentation to reflect the 4-tool MCP surface and the updated runtime log formats.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_tools_registry.py | Updates the expected tool surface to 4 tools and moves log_feedback into dropped-tools assertions. |
| tests/test_ports.py | Updates FileActivitySink test coverage to remove feedback logging expectations. |
| tests/test_oauth_server.py | Updates expected tools/list results to remove log_feedback. |
| tests/test_mcp_http.py | Updates HTTP transport tool-surface expectations to 4 tools. |
| tests/test_mcp_harness.py | Updates stdio transport tool-surface expectations to remove log_feedback. |
| tests/test_db_activity_sink.py | Removes DB-path tests for log_feedback and adjusts module description accordingly. |
| tests/test_contracts.py | Removes roundtrip tests for feedback-related contracts and sink record shapes. |
| tests/test_admin_activity.py | Updates tool iteration to exclude log_feedback when checking common schema properties. |
| packages/agami-core/src/tools.py | Removes tool_log_feedback, feedback logging helpers/constants, and the log_feedback registry entry. |
| packages/agami-core/src/store.py | Updates module docstring to reflect query logging only (no feedback logging). |
| packages/agami-core/src/semantic_model/curate.py | Updates comment wording to avoid referring to a “feedback generator” in this context. |
| packages/agami-core/src/ports.py | Removes record_feedback from the ActivitySink protocol and updates docs accordingly. |
| packages/agami-core/src/oss_adapters.py | Simplifies FileActivitySink to only log query executions; removes feedback path handling. |
| packages/agami-core/src/model_store.py | Removes DbActivitySink.record_feedback and updates class docstring. |
| packages/agami-core/src/contracts.py | Removes LogFeedback* and FeedbackRecord contracts; updates top-level module docs to “4 product tools”. |
| docs/mcp-server.md | Removes log_feedback from the documented tool list. |
| docs/format-spec.md | Removes feedback from the documented query_log.jsonl record format. |
| docs/credentials.md | Updates the BigQuery credential example key name to service_account_path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
28
to
30
| | `get_datasource_schema` | Return the semantic model: the subject-area index, full per-table detail for requested `dataset_names`, plus `ORGANIZATION.md` / `USER_MEMORY.md`. | | ||
| | `get_prompt_examples` | Return the curated `examples.yaml` few-shot library. | | ||
| | `execute_sql` | Run **one read-only** `SELECT` / `WITH...SELECT` locally and return `{columns, rows, row_count, ...}`. DML/DDL/multi-statement are rejected. | |
Contributor
Author
There was a problem hiding this comment.
Fixed — the intro now reads "Four tools" (this was a spelled-out "Five" my count-sweep missed). Pushed.
Comment on lines
233
to
237
| tools = {t["name"] for t in by_id[2]["result"]["tools"]} | ||
| assert tools == { | ||
| "list_datasources", "get_datasource_schema", | ||
| "get_prompt_examples", "execute_sql", "log_feedback", | ||
| "get_prompt_examples", "execute_sql", | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Already addressed — the comment now reads "4 product tools" (fixed in the review pass before this Copilot run landed).
Comment on lines
402
to
407
| assert names == { | ||
| "list_datasources", | ||
| "get_datasource_schema", | ||
| "get_prompt_examples", | ||
| "execute_sql", | ||
| "log_feedback", | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Already addressed — the comment now reads "4 product tools" (fixed in the earlier review pass).
Comment on lines
20
to
25
| PRODUCT_TOOLS = { | ||
| "list_datasources", | ||
| "get_datasource_schema", | ||
| "get_prompt_examples", | ||
| "execute_sql", | ||
| "log_feedback", | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Good catch — the docstring was already updated to "4", and I have now also renamed the test test_http_tools_list_is_the_same_five → ..._same_four. Pushed.
ashwin-agami
force-pushed
the
remove-log-feedback-tool
branch
from
July 7, 2026 10:14
8ee56ac to
67087bd
Compare
Subtractive refactor of the MCP tool surface: drops the log_feedback tool, its FeedbackRecord / LogFeedbackResult contracts, the DbActivitySink.record_feedback sink, and the port / adapter / file-sink wiring, so no dangling symbol remains. The runtime feedback table is left in place (unused). Tests and the MCP + format docs are updated to match; the model-review 'Generate feedback for Claude' back-channel is a separate feature and is untouched.
ashwin-agami
force-pushed
the
remove-log-feedback-tool
branch
from
July 7, 2026 10:22
67087bd to
afd5448
Compare
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
What
Subtractive refactor of the MCP tool surface — removes the
log_feedbacktool and everything wired to it:tool_log_feedback, thelog_feedbackentry in theTOOLSregistry,_record_feedback, and theFEEDBACK_LOGconstant.LogFeedbackRequest,LogFeedbackResult,FeedbackRecord.DbActivitySink.record_feedback.ActivitySink.record_feedbackmember, theFileActivitySinkfeedback path/param, and stray references, so no dangling symbol or import remains.The MCP surface is now the 4 product tools (
list_datasources,get_datasource_schema,get_prompt_examples,execute_sql).Left intentionally in place
feedbacktable —migrations/core/002_runtime.sqlis untouched; the table is still created (empty, unused). Dropping it is destructive and out of scope.test_schema.pystill asserts it exists.query_executionsand its sink — unchanged.Not touched (different feature — flagging to avoid confusion)
The
/agami-modeldashboard's "Generate feedback for Claude" back-channel (parsed byparse_model_feedback.py) is a separate feature that happens to share the word "feedback." Its tests (test_model_feedback.py) and its docs (usage.md,trust-layer.md) are deliberately left as-is.Verification
rg -i "feedback" packages/agami-core/src-> zero hits.TOOLSno longer containslog_feedback; imports resolve clean.