Conversation
|
| # | File | Issue | Violates |
|---|---|---|---|
| 1 | src/edictum/telemetry.py:56 |
edictum.calls.denied renamed to edictum.calls.blocked without a transition period — breaking change for downstream OTel consumers |
CLAUDE.md — Breaking changes and backward compatibility |
| 2 | src/edictum/server/audit_sink.py:137, src/edictum/gate/audit_buffer.py:162,356 |
"rules" + "_evaluated" string concatenation deliberately evades check-terminology.py's linter check — the linter rule itself is inverted but the workaround is the wrong fix |
CLAUDE.md — Pre-Merge Verification |
Details
1. src/edictum/telemetry.py:56 — OTel metric name rename breaks downstream consumers
The OTel counter name changed from edictum.calls.denied to edictum.calls.blocked. OTel metric names are part of the observable public API surface: anyone with a Prometheus scrape config, Grafana dashboard, alert rule, or OTel Collector processor that references edictum.calls.denied will silently stop counting blocked calls after upgrading. The counter just disappears — no error, no warning, no data.
This is a correctness bug today: customers in production who have built dashboards or alerts on edictum.calls.denied will have a broken monitoring graph the moment they upgrade. The method record_denial() also retains its old name while now incrementing the blocked counter, adding a naming inconsistency.
The rename itself is correct from a terminology standpoint (aligns with the denied → blocked standard). The problem is the absence of a transition mechanism.
Suggested fix: Emit both metric names for at least one release cycle, then drop the old one with a deprecation note in the changelog:
def _setup_metrics(self):
if not self._meter:
return
self._blocked_counter = self._meter.create_counter(
"edictum.calls.blocked",
description="Number of blocked tool calls",
)
# Deprecated alias — emits the same value. Remove in v0.19.0.
self._denied_counter = self._meter.create_counter(
"edictum.calls.denied",
description="[Deprecated] Use edictum.calls.blocked",
)
self._allowed_counter = self._meter.create_counter(
"edictum.calls.allowed",
description="Number of allowed tool calls",
)
def record_denial(self, tool_call: Any, reason: str | None = None) -> None:
if _HAS_OTEL and self._meter:
attrs = {"tool.name": tool_call.tool_name}
self._blocked_counter.add(1, attrs)
self._denied_counter.add(1, attrs) # deprecated alias2. src/edictum/server/audit_sink.py:137, src/edictum/gate/audit_buffer.py:162,356 — Linter evasion via string concatenation masks an inverted linter rule
The PR introduces wire_rules_key = "rules" + "_evaluated" in three places instead of the literal string "rules_evaluated". The reason is documented implicitly: scripts/check-terminology.py has a BANNED_PATTERNS entry that flags "rules_evaluated" and suggests replacing it with "contracts_evaluated". But the linter rule is inverted — the tests (assert payload["rules_evaluated"] == ... in both test_audit_sink.py:130 and test_audit_buffer.py:302,337) confirm rules_evaluated is the correct current wire key and contracts_evaluated is the old internal attribute name on AuditEvent.
Evading the linter with string concatenation is the wrong fix. It means:
- Future developers who write
"rules_evaluated"directly will see a spurious linter failure and may change it to the wrong name, silently breaking the server wire format. - The linter's confidence in its own rules is undermined — if some violations are intentional workarounds, reviewers can't trust any linter output.
- The obfuscation pattern (
"rules" + "_evaluated") will confuse anyone searching the codebase for the wire field name.
Suggested fix: Fix the root cause — correct the inverted rule in scripts/check-terminology.py and remove the string concatenation:
# In scripts/check-terminology.py, remove or correct this line:
# WRONG (currently):
(re.compile(r'"\brules_evaluated\b"'), '"contracts_evaluated"', "old field name"),
# The wire key IS rules_evaluated. The OLD internal field is contracts_evaluated.
# If the goal is to prevent raw use of the internal field name in wire format code:
(re.compile(r'"contracts_evaluated"'), '"rules_evaluated" (wire key) or contracts_evaluated attribute', "use wire key"),Then revert the obfuscation in all three files to use the literal string:
# src/edictum/server/audit_sink.py
payload["rules_evaluated"] = deepcopy(getattr(event, "contracts_evaluated", []))
# src/edictum/gate/audit_buffer.py (build_audit_event)
evaluated_count = (
getattr(evaluation_result, "contracts_evaluated", getattr(evaluation_result, "rules_evaluated", 0))
if evaluation_result
else 0
)
# src/edictum/gate/audit_buffer.py (_to_console_event)
contracts_evaluated = raw.get("contracts_evaluated") or raw.get("rules_evaluated", [])
event["rules_evaluated"] = contracts_evaluatedReview details
| Files reviewed | 41 |
| Checks applied | Tier boundary · Code quality · Terminology · Security · Adapter parity · Governance consistency · API design · Breaking changes |
| Issues found | 2 |
| Criteria sources | CLAUDE.md · .docs-style-guide.md · code-reviewer.md |
Files
- ✏️
ARCHITECTURE.md(modified) - ✏️
README.md(modified) - ✏️
SECURITY.md(modified) - ✏️
src/edictum/__init__.py(modified) - ✏️
src/edictum/_factory.py(modified) - ✏️
src/edictum/_guard.py(modified) - ✏️
src/edictum/_runner.py(modified) - ✏️
src/edictum/_server_factory.py(modified) - ✏️
src/edictum/adapters/agno.py(modified) - ✏️
src/edictum/adapters/claude_agent_sdk.py(modified) - ✏️
src/edictum/adapters/crewai.py(modified) - ✏️
src/edictum/adapters/google_adk.py(modified) - ✏️
src/edictum/adapters/langchain.py(modified) - ✏️
src/edictum/adapters/nanobot.py(modified) - ✏️
src/edictum/adapters/openai_agents.py(modified) - ✏️
src/edictum/adapters/semantic_kernel.py(modified) - ✏️
src/edictum/audit.py(modified) - ✏️
src/edictum/builtins.py(modified) - ✏️
src/edictum/envelope.py(modified) - ✏️
src/edictum/gate/__init__.py(modified) - ✏️
src/edictum/gate/audit_buffer.py(modified) - ✏️
src/edictum/gate/check.py(modified) - ✏️
src/edictum/gate/config.py(modified) - ✏️
src/edictum/gate/formats/claude_code.py(modified) - ✏️
src/edictum/gate/formats/copilot_cli.py(modified) - ✏️
src/edictum/gate/formats/cursor.py(modified) - ✏️
src/edictum/gate/formats/gemini_cli.py(modified) - ✏️
src/edictum/gate/formats/opencode.py(modified) - ✏️
src/edictum/otel.py(modified) - ✏️
src/edictum/pipeline.py(modified) - ✏️
src/edictum/rules.py(modified) - ✏️
src/edictum/server/audit_sink.py(modified) - ✏️
src/edictum/server/backend.py(modified) - ✏️
src/edictum/session.py(modified) - ✏️
src/edictum/telemetry.py(modified) - ✏️
src/edictum/workflow/evaluator_exec.py(modified) - ✏️
src/edictum/yaml_engine/templates/coding-assistant-base.yaml(modified) - ✏️
src/edictum/yaml_engine/templates/devops-agent.yaml(modified) - ✏️
src/edictum/yaml_engine/templates/file-agent.yaml(modified) - ✏️
src/edictum/yaml_engine/templates/nanobot-agent.yaml(modified) - ✏️
src/edictum/yaml_engine/templates/research-agent.yaml(modified)
| self._denied_counter = self._meter.create_counter( | ||
| "edictum.calls.denied", | ||
| description="Number of denied tool calls", | ||
| "edictum.calls.blocked", |
There was a problem hiding this comment.
Renaming edictum.calls.denied to edictum.calls.blocked silently breaks any downstream consumer (Prometheus alert, Grafana dashboard, OTel Collector rule) that references the old metric name. The counter disappears on upgrade with no warning.
The rename is terminology-correct but needs a transition. Emit both names for one release cycle:
self._blocked_counter = self._meter.create_counter(
"edictum.calls.blocked",
description="Number of blocked tool calls",
)
# Deprecated alias — remove in v0.19.0
self._denied_counter = self._meter.create_counter(
"edictum.calls.denied",
description="[Deprecated] Use edictum.calls.blocked",
)Then in record_denial() increment both counters, and add a deprecation note to the changelog.
| event_workflow = getattr(event, "workflow", None) | ||
| if isinstance(event_workflow, dict): | ||
| workflow = event_workflow | ||
| wire_rules_key = "rules" + "_evaluated" |
There was a problem hiding this comment.
wire_rules_key = "rules" + "_evaluated" is obfuscating the literal "rules_evaluated" to avoid a false positive from scripts/check-terminology.py. The linter's BANNED_PATTERNS entry is inverted — it treats "rules_evaluated" as the old field name and suggests "contracts_evaluated", but tests at test_audit_sink.py:130 and test_audit_buffer.py:302,337 confirm rules_evaluated is the correct current wire key.
This workaround is the wrong fix. Future developers writing "rules_evaluated" directly will get a spurious linter failure and may change it to "contracts_evaluated", silently breaking the server wire format.
Fix the root cause: Correct (or remove) the inverted rule in scripts/check-terminology.py and use the literal string here:
payload["rules_evaluated"] = deepcopy(getattr(event, "contracts_evaluated", []))| rules = _contracts_to_dicts(evaluation_result) | ||
| pe = getattr(evaluation_result, "policy_error", False) if evaluation_result else False | ||
| evaluated_count = getattr(evaluation_result, "rules_evaluated", 0) if evaluation_result else 0 | ||
| legacy_count_attr = "rules" + "_evaluated" |
There was a problem hiding this comment.
server/audit_sink.py:137)
legacy_count_attr = "rules" + "_evaluated" obfuscates "rules_evaluated" to avoid check-terminology.py's inverted linter rule. The correct fix is to repair the linter rule, not work around it. See the companion comment on server/audit_sink.py:137 for full context and the suggested fix.
Summary
Verification