ACE-095: delete the dead GovernancePolicy port and its seam - #178
Merged
Conversation
GovernancePolicy was declared as a port and wired into Adapters, but no core call site ever evaluated it: the sole caller of .evaluate() repo-wide was the test that asserted the warn-only adapter returns allowed=True. A port with one implementation and zero consumers is not a seam, it is a placeholder that every construction site has to keep feeding. Removes the GovernancePolicy Protocol, the GovernanceVerdict value type, the WarnOnlyGovernancePolicy default adapter, and the Adapters.governance field, plus all four construction sites. The warn-only test is deleted rather than adapted -- there is nothing left to exercise. Nothing on the execute_sql enforcement path is touched; enforcement was never routed through this port. execute_sql.py is vendored, so plugins/agami/lib/execute_sql.py is regenerated to match (its only change is a docstring word).
ports.Adapters is public API of the published agami-core package, so a dropped constructor keyword belongs in a Removed section even though no downstream in this repo constructs it. Keep-a-Changelog + SemVer.
Adapters is frozen but not kw_only, so removing the 4th field is not only a dropped keyword: the 4th positional slot was governance and is now executor. A consumer building it positionally still constructs successfully and only fails at query time, when the guarded path calls .execute() on what it thinks is an executor. That is worth a sentence in the changelog, since a silent rebind is harder to diagnose than a TypeError. Constructing by keyword is the fix, so kw_only=True is not added here. The two docstring counts were wrong in the same direction: four ports remain, not three. oss_adapters covers three of them because Executor's default lives in execute_sql (that module ships in the plugin mirror and cannot import oss_adapters), and test_ports covers the same three because Executor is tested in test_ah012_executor_seam.py.
There was a problem hiding this comment.
Pull request overview
This PR removes the unused GovernancePolicy port seam (and its verdict/value types + default OSS adapter) from agami-core, eliminating an inert interface that had no product call sites and was only exercised by a single test. This simplifies the adapter surface area while keeping the execute_sql enforcement chokepoint unchanged (aside from docstring wording).
Changes:
- Deleted
GovernancePolicy/GovernanceVerdictand removedAdapters.governance, updating all construction sites accordingly. - Removed the OSS default
WarnOnlyGovernancePolicyadapter and deleted the sole test that exercised.evaluate(). - Updated docstrings and changelog notes to reflect the new “4 ports” shape and the executor default-location rationale (including the plugin mirror).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_tool_extension_seam.py | Drops governance-related imports/assertions and stops passing governance= when constructing Adapters. |
| tests/test_ports.py | Removes governance protocol/value-type coverage and deletes the warn-only governance behavior test. |
| tests/test_ah012_executor_seam.py | Removes the unused governance passthrough when building Adapters for executor seam coverage. |
| packages/agami-core/src/ports.py | Deletes GovernancePolicy/GovernanceVerdict and removes Adapters.governance, updating docs to “four ports”. |
| packages/agami-core/src/oss_adapters.py | Removes WarnOnlyGovernancePolicy and updates module docstring to clarify executor default location. |
| packages/agami-core/src/mcp_http.py | Updates default adapter wiring to no longer import/construct governance; keeps executor default wiring intact. |
| packages/agami-core/src/execute_sql.py | Docstring wording update to reflect “other three ports” after governance removal. |
| plugins/agami/lib/execute_sql.py | Mirrors the same executor docstring wording update in the vendored plugin copy. |
| CHANGELOG.md | Adds an Unreleased “Removed” entry documenting the API change and warning about positional Adapters(...) construction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
145
to
149
| """The port adapters, bundled so ``mcp_http.create_app`` takes them as one argument. | ||
|
|
||
| A consumer builds this with its own implementations of the ports (its own ``OrgResolver``, | ||
| ``AuthProvider``, ``ActivitySink``, ``GovernancePolicy``, and optionally an ``Executor``); | ||
| ``AuthProvider``, ``ActivitySink``, and optionally an ``Executor``); | ||
| passing ``adapters=None`` to ``create_app`` uses the OSS defaults (``mcp_http.default_adapters``). |
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.
Spec: ACE-095
Summary
GovernancePolicy,GovernanceVerdictandWarnOnlyGovernancePolicywere wired into the HTTPadapter set, but
.evaluate()was never called in the product. Its only caller was a single test.The seam was built for the graded, tiered verdict the governance principles removed, and its
successor spec is abandoned, so nothing was holding it open. Left in place, it invites the next
reader to implement against a port that enforces nothing.
This deletes the port, its value type, its OSS default adapter, the
Adapters.governancefield,and every construction site and test that referenced them.
The removed code was inert. Verified against the base commit:
Adapters.governancehad exactly tworeaders repo-wide, both in tests, and
.evaluate()had exactly one caller, the test deleted here.No product call site read the field or invoked the port, so nothing that was enforced before is
unenforced now. The single
execute_sqlenforcement chokepoint is untouched apart from onedocstring word.
Changes
Deleted
ports.py: theGovernancePolicyProtocol and theGovernanceVerdictdataclass.oss_adapters.py: theWarnOnlyGovernancePolicydefault adapter.ports.py: thegovernancefield onAdapters, plus all four construction sites that passed it(
mcp_http.default_adapters(), two intest_tool_extension_seam.py, one intest_ah012_executor_seam.py). Three passed the type; the fourth passedbase.governance, so agrep for the symbol names alone misses it.
fieldfrom thedataclassesimport inports.py, whose only use wasGovernanceVerdict.warnings.Test contract change
tests/test_ports.py::test_warn_only_governance_never_blocksis deleted, not adapted. It wasthe sole caller of
.evaluate(); with the port gone it has no contract left to assert. Recordedin the spec's
## Decisionsper the rule that a deleted test is a contract change.governance=kwarg. They cover the tool-extensionand executor seams, not governance, and still assert their original behavior (adapter defaults,
sentinel identity, the 403 path, injected-executor vs
BUILTIN_EXECUTOR).Docs
execute_sql.py(mirror regenerated; the two copies are byte-identical).oss_adapters.pynow explains why the fourth port's default lives elsewhere:Executor'sBUILTIN_EXECUTORsits inexecute_sql.pybecause that module ships in the stdlib-lean pluginmirror, which cannot import
oss_adapters.CHANGELOG.mdgains aRemovedentry. It flags thatAdaptersis not keyword-only, so aconsumer that constructed it positionally must re-check the call: the 4th positional slot was
governanceand is nowexecutor, which still constructs but fails later at query time.Checklist
ports.py::GovernancePolicy,ports.py::GovernanceVerdictandoss_adapters.py::WarnOnlyGovernancePolicyare gone; grep-clean inpackages/agami-core/src.ports.py::Adaptersloses itsgovernancefield, and all four construction sites stoppassing it.
.evaluate()is deleted, not adapted.uv run dev.py checkgreen (2412 passed, 4 skipped; ruff clean, gitleaks clean).docs/,README.md, anySKILL.md,pyproject.tomlor deploy config.plugins/agami/lib/execute_sql.pybyte-identical to source).execute_sqlenforcement chokepoint untouched; no gate weakened.🤖 Generated with Claude Code