feat(layer-3): behavioral probes + LLM semantic judge - #2
Conversation
Schema checks can't see a lying server, and most real MCP tools declare no outputSchema at all. Layer 3 closes both gaps: - [[probes]] in covenant.toml: safe read-only example calls. snapshot fingerprints each response's type shape (never values) into the lock; check re-runs them and classifies drift with the unchanged Layer 0 classifier (responses are output-side by definition), location "behavior". - covenant check --judge ([judge] extra, anthropic): compares baseline sample vs live response for semantic drift a shape can't see (dollars->cents). Verdicts are advisory DEGRADED, never BREAKING - a probabilistic detector must not trigger quarantine. - Example server levers: COVENANT_BEHAVIOR_DRIFT=1 (body-only rename, schema identical - probes catch BREAKING, CI dogfoods it) and COVENANT_SEMANTIC_DRIFT=1 (balance x100 - judge territory). 112 tests (22 new), ruff + strict mypy clean, lock stays byte-deterministic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds a "Layer 3" behavioral probe system to Covenant: probes fingerprint live tool responses at snapshot/check time, an optional ChangesBehavioral probes and judge feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Probes
participant Judge
participant Report
User->>CLI: covenant check --judge
CLI->>Probes: run_probes + diff_probes
Probes-->>CLI: probe changes (behavior)
CLI->>Judge: judge_probe(baseline, live)
Judge-->>CLI: Verdict(drift, reason)
CLI->>Report: render(changes)
Report-->>User: OK / BREAKING / DEGRADED
Related Issues: None referenced in the diff. Related PRs: None referenced in the diff. Suggested labels: feature, cli, tests, documentation Suggested reviewers: Mhemd139 Poem:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (4)
covenant/cli.py (1)
51-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing-probe error message can't disambiguate probes sharing a tool name.
missingis built fromp.toolalone; two probes on the same tool with differentargsthat are both missing render as a duplicated, indistinguishable tool name in the error text.♻️ Include args for disambiguation
- missing = [p.tool for p in cfg.probes if probe_key(p.tool, p.args) not in base_by] + missing = [f"{p.tool} {p.args}" for p in cfg.probes if probe_key(p.tool, p.args) not in base_by]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@covenant/cli.py` around lines 51 - 57, The missing-probe error in the baseline check is ambiguous because `missing` in `covenant.cli` only collects `p.tool`, so probes using the same tool with different args collapse to the same name in the message. Update the probe-identification logic around `probe_key`, `base_by`, and the `CovenantError` construction so the error text includes enough probe detail to distinguish entries by both tool and args, and make sure the missing list is built from that disambiguated representation.examples/mcp_server.py (1)
106-111: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePrefer
round()overint()for cents conversion.
int(t["amount_usd"] * 100)truncates toward zero; floating-point multiplication can yield values like420999.9999999999for otherwise-exact cent amounts, silently losing a cent. Current hardcoded samples happen to avoid this, but the pattern is fragile.♻️ Proposed fix
- {"txn_id": t["txn_id"], "amount_cents": int(t["amount_usd"] * 100), + {"txn_id": t["txn_id"], "amount_cents": round(t["amount_usd"] * 100), "merchant": t["merchant"]}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/mcp_server.py` around lines 106 - 111, The cents conversion in the BEHAVIOR_DRIFT normalization block is using truncation, which can silently drop a cent for floating-point values. Update the txns transformation in examples/mcp_server.py to use rounding when computing amount_cents from amount_usd, and keep the change local to the existing behavior-drift handling so the txn_id, amount_cents, and merchant mapping in that comprehension remains intact.tests/test_cli.py (1)
84-92: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnchecked
snapshotinvocations weaken the failure-path assertions.In both
test_probe_missing_from_baseline_errors(Line 87) andtest_judge_without_probes_errors(Line 96), thesnapshotinvoke's result is discarded. If snapshot itself failed (e.g. bad server command),checkcould still return exit code 2 for an unrelated reason, letting these tests pass without exercising the intended "probe not in baseline" / "--judge needs probes" error paths.✅ Proposed fix
def test_probe_missing_from_baseline_errors(tmp_path, monkeypatch): monkeypatch.chdir(tmp_path) _write_toml(tmp_path) - runner.invoke(app, ["snapshot"]) + r = runner.invoke(app, ["snapshot"]) + assert r.exit_code == 0, r.output _write_toml(tmp_path, '\n[[probes]]\ntool = "get_weather"\nargs = { city = "Haifa" }\n') r = runner.invoke(app, ["check"]) assert r.exit_code == 2 assert "snapshot" in r.output def test_judge_without_probes_errors(tmp_path, monkeypatch): monkeypatch.chdir(tmp_path) - runner.invoke(app, ["snapshot", "--server", SERVER]) + r = runner.invoke(app, ["snapshot", "--server", SERVER]) + assert r.exit_code == 0, r.output r = runner.invoke(app, ["check", "--server", SERVER, "--judge"]) assert r.exit_code == 2Also applies to: 94-98
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cli.py` around lines 84 - 92, The tests are discarding the result of the initial app.invoke("snapshot") call, so they may pass even if snapshot failed for an unrelated reason. Update test_probe_missing_from_baseline_errors and test_judge_without_probes_errors to assert the snapshot invocation succeeded before running check, using the existing runner.invoke(app, ["snapshot"]) call site in these test functions. This keeps the failure-path assertions focused on the intended probe/baseline and --judge validation behavior.pyproject.toml (1)
60-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
anthropic>=0.40in one place.devstill duplicates the same dependency already declared injudge; makedevdepend oncovenant-mcp[judge]instead of pinning it again.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` at line 60, The `dev` extra is duplicating the `anthropic>=0.40` dependency that is already declared under `judge`; update the dependency definition so `dev` depends on `covenant-mcp[judge]` instead of listing `anthropic` again, and keep the single source of truth in the `judge` extra.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@covenant/cli.py`:
- Around line 32-44: The snapshot path in _snapshot_probes() can let a
ValueError from fingerprint() escape, which bypasses the CLI’s CovenantError
handling. Wrap the fingerprint(r["response"]) call (or the whole record-building
block in snapshot()/ _snapshot_probes()) and convert any fingerprint-related
ValueError into a CovenantError with a clean message, so snapshot() still exits
via the existing one-line error path and code 2. Use _snapshot_probes,
fingerprint, and snapshot as the key places to update.
- Around line 111-130: The probe diff path can let raw fingerprint parsing
failures escape `check()`, so update `_check_probes()`/`diff_probes()` to catch
`ValueError` from `fingerprint()` and re-raise it as `CovenantError` like
`judge_probe()` does. Keep the handling close to the `diff_probes()` call site
so `check()`’s existing `CovenantError` exit path is used for non-JSON probe
results instead of a stack trace.
- Around line 47-81: The probe-judging logic in _check_probes is collapsing
drift tracking to tool name instead of probe identity, which can cause unrelated
probes for the same tool to be skipped; change shape_drifted to track full probe
identity via probe_key for each Change and only skip the matching probe in the
live loop. Also replace the direct base_by[probe_key(...)] lookup with a safe
.get() check and raise a CovenantError when the probe key is missing, so
mismatched echoed args from run_probes do not crash _check_probes unexpectedly.
In `@covenant/config.py`:
- Around line 80-82: The judge config parsing in load_config is unsafe because
`data.get("judge")` may be a non-dict and the chained `.get("model")` can raise
a raw AttributeError instead of ConfigError. Add the same shape guard used in
`_parse_probes` by checking that the `[judge]` value is a dict before reading
`model`, and raise a ConfigError with a clear message when it is not. Keep the
existing `judge_model` validation logic, but make sure malformed `judge` entries
are converted into clean config errors rather than uncaught exceptions.
In `@covenant/diff.py`:
- Around line 245-267: Handle malformed baseline probe records in the probe
comparison logic so a missing fingerprint does not raise a raw KeyError. Update
the baseline lookup and fingerprint access in the change-building loop in
covenant/diff.py to validate bp before using bp["fingerprint"], and route the
failure through the existing CovenantError path used by covenant check. Keep the
fix localized around the probe comparison code that uses base_by, probe_key, and
fingerprint.
In `@covenant/introspect.py`:
- Around line 91-104: The per-probe failure handling in _run_probes is too
generic because exceptions from session.call_tool are later surfaced as a
ConnectionError, hiding which probe/tool failed and making config mistakes look
like connectivity issues. Update the probe execution path in _run_probes and the
caller in run_probes to catch per-tool exceptions, preserve the failing probe’s
tool/args context, and propagate or record that specific error so snapshot/check
can report a probe/config problem instead of a generic server connection
failure.
- Around line 91-104: The probe execution path in _run_probes (and the shared
run_probes flow) calls session.call_tool without any timeout, so a stalled
server can block snapshot/check indefinitely. Update the call to enforce a
timeout by passing a default/configurable read_timeout_seconds or by wrapping
the tool invocation in asyncio.wait_for, and make sure the timeout is applied
consistently wherever probes are executed.
- Around line 91-104: The _run_probes flow currently executes every Probe.tool
without verifying it is read-only, so a misconfigured snapshot/check probe can
mutate state. Add a runtime guard in _run_probes (or earlier in the
Probe/_tool_to_dict pipeline) to reject any probe whose tool is not marked
read-only, using the existing Probe and _resolve_result/_session flow to keep
the check centralized.
In `@covenant/judge/__init__.py`:
- Around line 76-80: The verdict parsing in the judge module is coercing
`data["drift"]` with `bool(...)`, which can turn string values like "false" into
a true drift result. Update the `Verdict` construction in
`covenant/judge/__init__.py` to require `drift` to already be a real boolean and
reject any non-boolean value by raising `TypeError` or otherwise letting the
existing exception handling convert it to `CovenantError`. Keep the
`json.loads`/`Verdict(...)` flow in the same parsing block so invalid model
output fails loudly rather than being coerced.
In
`@docs/superpowers/specs/2026-07-03-covenant-layer3-behavioral-probes-design.md`:
- Around line 34-36: The array fingerprinting behavior in the spec leaves a
blind spot in diff_probes() for collection-returning probes, since a uniform
array that later becomes heterogeneous is still treated as a bare array and the
drift is missed. Update the documentation around the array-shape rules to
explicitly mark this as a non-goal, or add a regression test covering the array
fingerprinting behavior so the limitation is captured; use the existing
array/list shape language in the spec to locate the affected section.
In `@examples/mcp_server.py`:
- Around line 103-112: get_transactions currently returns an unparameterized
dict, which violates the strict mypy settings; update the return type on
get_transactions to a concrete typed mapping or a TypedDict that matches the
response shape. Define and use a named response type for the
account_id/transactions payload, and make sure the txns transformation in
get_transactions still fits that declared shape.
In `@README.md`:
- Line 97: The README wording in the covenant snapshot/check description is
outdated and says the lock stores only a fingerprint and never values; update
that sentence to reflect the new snapshot format that also persists a sample
response in covenant.lock.json. Adjust the text around the covenant snapshot and
covenant check description so it accurately describes what is retained, while
keeping the explanation of fingerprint/shape drift and behavior severity
consistent.
---
Nitpick comments:
In `@covenant/cli.py`:
- Around line 51-57: The missing-probe error in the baseline check is ambiguous
because `missing` in `covenant.cli` only collects `p.tool`, so probes using the
same tool with different args collapse to the same name in the message. Update
the probe-identification logic around `probe_key`, `base_by`, and the
`CovenantError` construction so the error text includes enough probe detail to
distinguish entries by both tool and args, and make sure the missing list is
built from that disambiguated representation.
In `@examples/mcp_server.py`:
- Around line 106-111: The cents conversion in the BEHAVIOR_DRIFT normalization
block is using truncation, which can silently drop a cent for floating-point
values. Update the txns transformation in examples/mcp_server.py to use rounding
when computing amount_cents from amount_usd, and keep the change local to the
existing behavior-drift handling so the txn_id, amount_cents, and merchant
mapping in that comprehension remains intact.
In `@pyproject.toml`:
- Line 60: The `dev` extra is duplicating the `anthropic>=0.40` dependency that
is already declared under `judge`; update the dependency definition so `dev`
depends on `covenant-mcp[judge]` instead of listing `anthropic` again, and keep
the single source of truth in the `judge` extra.
In `@tests/test_cli.py`:
- Around line 84-92: The tests are discarding the result of the initial
app.invoke("snapshot") call, so they may pass even if snapshot failed for an
unrelated reason. Update test_probe_missing_from_baseline_errors and
test_judge_without_probes_errors to assert the snapshot invocation succeeded
before running check, using the existing runner.invoke(app, ["snapshot"]) call
site in these test functions. This keeps the failure-path assertions focused on
the intended probe/baseline and --judge validation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: caef019f-6868-4aa5-9b8c-c0cedd747cb8
📒 Files selected for processing (23)
.github/workflows/ci.ymlCLAUDE.mdREADME.mdcovenant.lock.jsoncovenant.tomlcovenant/cli.pycovenant/config.pycovenant/contract.pycovenant/diff.pycovenant/fingerprint.pycovenant/introspect.pycovenant/judge/__init__.pycovenant/report.pydocs/BUILD_LOG.mddocs/superpowers/specs/2026-07-03-covenant-layer3-behavioral-probes-design.mdexamples/mcp_server.pypyproject.tomltests/test_cli.pytests/test_contract.pytests/test_fingerprint.pytests/test_introspect.pytests/test_judge.pytests/test_probes.py
| def _snapshot_probes(cfg: Config) -> list[JsonDict]: | ||
| """Run the configured probes and build their baseline records.""" | ||
| records: list[JsonDict] = [] | ||
| for r in run_probes(cfg, cfg.probes): | ||
| if r["is_error"]: | ||
| raise CovenantError(f"probe {r['tool']} failed at snapshot: {r['error']}") | ||
| records.append({ | ||
| "tool": r["tool"], | ||
| "args": r["args"], | ||
| "fingerprint": fingerprint(r["response"]), | ||
| "sample": r["response"], | ||
| }) | ||
| return records |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Uncaught ValueError from fingerprint() leaks a stack trace.
fingerprint() raises ValueError for non-JSON-serializable values. That call happens inside snapshot()'s try block, but snapshot() only catches CovenantError. If a probed tool ever returns a value fingerprint() can't classify (e.g. bytes, a tuple), this propagates as an unhandled exception with a full stack trace instead of the required clean one-line error + exit code 2.
As per coding guidelines, "CLI errors must be converted from CovenantError into one clean line with exit code 2; never emit a stack trace or swallow the error."
🛡️ Proposed fix
def _snapshot_probes(cfg: Config) -> list[JsonDict]:
"""Run the configured probes and build their baseline records."""
records: list[JsonDict] = []
for r in run_probes(cfg, cfg.probes):
if r["is_error"]:
raise CovenantError(f"probe {r['tool']} failed at snapshot: {r['error']}")
+ try:
+ fp = fingerprint(r["response"])
+ except ValueError as e:
+ raise CovenantError(f"probe {r['tool']}: {e}") from e
records.append({
"tool": r["tool"],
"args": r["args"],
- "fingerprint": fingerprint(r["response"]),
+ "fingerprint": fp,
"sample": r["response"],
})
return records📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _snapshot_probes(cfg: Config) -> list[JsonDict]: | |
| """Run the configured probes and build their baseline records.""" | |
| records: list[JsonDict] = [] | |
| for r in run_probes(cfg, cfg.probes): | |
| if r["is_error"]: | |
| raise CovenantError(f"probe {r['tool']} failed at snapshot: {r['error']}") | |
| records.append({ | |
| "tool": r["tool"], | |
| "args": r["args"], | |
| "fingerprint": fingerprint(r["response"]), | |
| "sample": r["response"], | |
| }) | |
| return records | |
| def _snapshot_probes(cfg: Config) -> list[JsonDict]: | |
| """Run the configured probes and build their baseline records.""" | |
| records: list[JsonDict] = [] | |
| for r in run_probes(cfg, cfg.probes): | |
| if r["is_error"]: | |
| raise CovenantError(f"probe {r['tool']} failed at snapshot: {r['error']}") | |
| try: | |
| fp = fingerprint(r["response"]) | |
| except ValueError as e: | |
| raise CovenantError(f"probe {r['tool']}: {e}") from e | |
| records.append({ | |
| "tool": r["tool"], | |
| "args": r["args"], | |
| "fingerprint": fp, | |
| "sample": r["response"], | |
| }) | |
| return records |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@covenant/cli.py` around lines 32 - 44, The snapshot path in
_snapshot_probes() can let a ValueError from fingerprint() escape, which
bypasses the CLI’s CovenantError handling. Wrap the fingerprint(r["response"])
call (or the whole record-building block in snapshot()/ _snapshot_probes()) and
convert any fingerprint-related ValueError into a CovenantError with a clean
message, so snapshot() still exits via the existing one-line error path and code
2. Use _snapshot_probes, fingerprint, and snapshot as the key places to update.
Source: Coding guidelines
| def _check_probes( | ||
| cfg: Config, base_tools: list[JsonDict], base_probes: list[JsonDict], judge: bool | ||
| ) -> list[Change]: | ||
| """Re-run the probes, diff fingerprints, and (optionally) judge semantics.""" | ||
| base_by = {probe_key(p["tool"], p.get("args")): p for p in base_probes} | ||
| missing = [p.tool for p in cfg.probes if probe_key(p.tool, p.args) not in base_by] | ||
| if missing: | ||
| raise CovenantError( | ||
| f"probe(s) not in baseline: {', '.join(missing)} - " | ||
| "re-run `covenant snapshot --force`" | ||
| ) | ||
| live = run_probes(cfg, cfg.probes) | ||
| changes = diff_probes(base_probes, live) | ||
| if not judge: | ||
| return changes | ||
| from .judge import judge_probe # the [judge] extra is optional; import on use | ||
|
|
||
| descriptions = {t["name"]: t.get("description") for t in base_tools} | ||
| shape_drifted = {c.tool for c in changes} | ||
| for r in live: | ||
| if r["is_error"] or r["tool"] in shape_drifted: | ||
| continue # errors and shape drift are already reported; judge only clean shapes | ||
| sample = base_by[probe_key(r["tool"], r["args"])].get("sample") | ||
| verdict = judge_probe( | ||
| r["tool"], descriptions.get(r["tool"]), r["args"], | ||
| sample, r["response"], model=cfg.judge_model, | ||
| ) | ||
| if verdict.drift: | ||
| changes.append(Change( | ||
| r["tool"], "behavior", None, "semantic_drift", "degraded", | ||
| f"probe {r['tool']}: semantic drift suspected - {verdict.reason}", | ||
| note="LLM-judge verdict - review manually", | ||
| )) | ||
| return changes | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
shape_drifted filters by tool name, not probe identity — can wrongly skip judging a clean probe.
probe_key (tool + canonical args) is the documented identity of a probe, so covenant.toml can legitimately hold multiple probes for the same tool with different args. shape_drifted = {c.tool for c in changes} collapses this to tool name only: if any probe on tool foo has shape drift or errors, every live probe for foo (including a distinct, genuinely clean probe with different args) is skipped from judging at Line 67-68.
Separately, base_by[probe_key(r["tool"], r["args"])] at Line 69 indexes directly rather than using .get() — if run_probes's echoed args ever fail to exactly match the config's canonicalized args, this raises an unhandled KeyError instead of a clean CovenantError.
♻️ Proposed fix — key by probe identity, not tool name
- changes = diff_probes(base_probes, live)
+ changes = diff_probes(base_probes, live)
if not judge:
return changes
from .judge import judge_probe # the [judge] extra is optional; import on use
descriptions = {t["name"]: t.get("description") for t in base_tools}
- shape_drifted = {c.tool for c in changes}
for r in live:
- if r["is_error"] or r["tool"] in shape_drifted:
- continue # errors and shape drift are already reported; judge only clean shapes
- sample = base_by[probe_key(r["tool"], r["args"])].get("sample")
+ if r["is_error"]:
+ continue
+ bp = base_by.get(probe_key(r["tool"], r["args"]))
+ if bp is None or fingerprint(r["response"]) != bp.get("fingerprint"):
+ continue # errors and shape drift are already reported; judge only clean shapes
+ sample = bp.get("sample")
verdict = judge_probe(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _check_probes( | |
| cfg: Config, base_tools: list[JsonDict], base_probes: list[JsonDict], judge: bool | |
| ) -> list[Change]: | |
| """Re-run the probes, diff fingerprints, and (optionally) judge semantics.""" | |
| base_by = {probe_key(p["tool"], p.get("args")): p for p in base_probes} | |
| missing = [p.tool for p in cfg.probes if probe_key(p.tool, p.args) not in base_by] | |
| if missing: | |
| raise CovenantError( | |
| f"probe(s) not in baseline: {', '.join(missing)} - " | |
| "re-run `covenant snapshot --force`" | |
| ) | |
| live = run_probes(cfg, cfg.probes) | |
| changes = diff_probes(base_probes, live) | |
| if not judge: | |
| return changes | |
| from .judge import judge_probe # the [judge] extra is optional; import on use | |
| descriptions = {t["name"]: t.get("description") for t in base_tools} | |
| shape_drifted = {c.tool for c in changes} | |
| for r in live: | |
| if r["is_error"] or r["tool"] in shape_drifted: | |
| continue # errors and shape drift are already reported; judge only clean shapes | |
| sample = base_by[probe_key(r["tool"], r["args"])].get("sample") | |
| verdict = judge_probe( | |
| r["tool"], descriptions.get(r["tool"]), r["args"], | |
| sample, r["response"], model=cfg.judge_model, | |
| ) | |
| if verdict.drift: | |
| changes.append(Change( | |
| r["tool"], "behavior", None, "semantic_drift", "degraded", | |
| f"probe {r['tool']}: semantic drift suspected - {verdict.reason}", | |
| note="LLM-judge verdict - review manually", | |
| )) | |
| return changes | |
| def _check_probes( | |
| cfg: Config, base_tools: list[JsonDict], base_probes: list[JsonDict], judge: bool | |
| ) -> list[Change]: | |
| """Re-run the probes, diff fingerprints, and (optionally) judge semantics.""" | |
| base_by = {probe_key(p["tool"], p.get("args")): p for p in base_probes} | |
| missing = [p.tool for p in cfg.probes if probe_key(p.tool, p.args) not in base_by] | |
| if missing: | |
| raise CovenantError( | |
| f"probe(s) not in baseline: {', '.join(missing)} - " | |
| "re-run `covenant snapshot --force`" | |
| ) | |
| live = run_probes(cfg, cfg.probes) | |
| changes = diff_probes(base_probes, live) | |
| if not judge: | |
| return changes | |
| from .judge import judge_probe # the [judge] extra is optional; import on use | |
| descriptions = {t["name"]: t.get("description") for t in base_tools} | |
| for r in live: | |
| if r["is_error"]: | |
| continue | |
| bp = base_by.get(probe_key(r["tool"], r["args"])) | |
| if bp is None or fingerprint(r["response"]) != bp.get("fingerprint"): | |
| continue # errors and shape drift are already reported; judge only clean shapes | |
| sample = bp.get("sample") | |
| verdict = judge_probe( | |
| r["tool"], descriptions.get(r["tool"]), r["args"], | |
| sample, r["response"], model=cfg.judge_model, | |
| ) | |
| if verdict.drift: | |
| changes.append(Change( | |
| r["tool"], "behavior", None, "semantic_drift", "degraded", | |
| f"probe {r['tool']}: semantic drift suspected - {verdict.reason}", | |
| note="LLM-judge verdict - review manually", | |
| )) | |
| return changes |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@covenant/cli.py` around lines 47 - 81, The probe-judging logic in
_check_probes is collapsing drift tracking to tool name instead of probe
identity, which can cause unrelated probes for the same tool to be skipped;
change shape_drifted to track full probe identity via probe_key for each Change
and only skip the matching probe in the live loop. Also replace the direct
base_by[probe_key(...)] lookup with a safe .get() check and raise a
CovenantError when the probe key is missing, so mismatched echoed args from
run_probes do not crash _check_probes unexpectedly.
| @app.command() | ||
| def check( | ||
| server: str | None = _server_opt, | ||
| strict: bool = typer.Option(False, "--strict", help="Fail on degraded changes too."), | ||
| json_out: bool = typer.Option(False, "--json", help="Emit changes as JSON."), | ||
| judge: bool = typer.Option( | ||
| False, "--judge", | ||
| help="Judge probe responses for semantic drift with an LLM (needs ANTHROPIC_API_KEY).", | ||
| ), | ||
| ) -> None: | ||
| """Diff the live server against the baseline and classify every change.""" | ||
| try: | ||
| cfg = load_config(server_override=server) | ||
| _, base_tools = read_baseline(cfg.baseline_path) | ||
| _, base_tools, base_probes = read_baseline(cfg.baseline_path) | ||
| current = introspect(cfg) | ||
| changes = diff_tools(base_tools, current) | ||
| if cfg.probes: | ||
| changes += _check_probes(cfg, base_tools, base_probes, judge) | ||
| elif judge: | ||
| raise CovenantError("--judge needs [[probes]] in covenant.toml to judge") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm all exceptions raised inside judge_probe's call chain are CovenantError
ast-grep run --pattern 'def _complete($$$) { $$$ }' --lang python covenant/judge/__init__.py
rg -n -A5 'def _complete' covenant/judge/__init__.pyRepository: Mhemd139/Covenant
Length of output: 715
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== covenant/cli.py outline =='
ast-grep outline covenant/cli.py --match check --view expanded || true
echo
echo '== covenant/judge/__init__.py outline =='
ast-grep outline covenant/judge/__init__.py --view expanded || true
echo
echo '== relevant function bodies =='
sed -n '1,220p' covenant/judge/__init__.py | cat -n
echo
echo '== diff/fingerprint/probe references =='
rg -n -A4 -B4 'def (diff_probes|fingerprint|judge_probe|_complete)|CovenantError|raise .*from e' covenant/judge/__init__.py covenant/cli.pyRepository: Mhemd139/Covenant
Length of output: 12636
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== covenant/diff.py outline =='
ast-grep outline covenant/diff.py --view expanded || true
echo
echo '== covenant/fingerprint.py outline =='
ast-grep outline covenant/fingerprint.py --view expanded || true
echo
echo '== relevant bodies in diff.py =='
sed -n '1,260p' covenant/diff.py | cat -n
echo
echo '== relevant bodies in fingerprint.py =='
sed -n '1,240p' covenant/fingerprint.py | cat -n
echo
echo '== _snapshot_probes context in cli.py =='
sed -n '1,90p' covenant/cli.py | cat -nRepository: Mhemd139/Covenant
Length of output: 19596
Wrap fingerprint() errors before they escape check()
judge_probe() already turns API and verdict parsing failures into CovenantError, but diff_probes() still calls fingerprint() directly. A non-JSON probe result will raise ValueError and bypass the check() error handler, producing a stack trace instead of the clean exit-2 path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@covenant/cli.py` around lines 111 - 130, The probe diff path can let raw
fingerprint parsing failures escape `check()`, so update
`_check_probes()`/`diff_probes()` to catch `ValueError` from `fingerprint()` and
re-raise it as `CovenantError` like `judge_probe()` does. Keep the handling
close to the `diff_probes()` call site so `check()`’s existing `CovenantError`
exit path is used for non-JSON probe results instead of a stack trace.
| judge_model = (data.get("judge") or {}).get("model") | ||
| if judge_model is not None and not isinstance(judge_model, str): | ||
| raise ConfigError("[judge].model must be a string") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unguarded .get() on [judge] can crash with a raw AttributeError instead of ConfigError.
If [judge] isn't a table (e.g. a malformed judge = "x" in covenant.toml), data.get("judge") returns a non-dict truthy value, so (data.get("judge") or {}).get("model") calls .get on a non-dict and raises AttributeError. _parse_probes guards against this shape mismatch with isinstance(entry, dict) checks (Line 45-47), but the judge section has no equivalent guard. An uncaught AttributeError would propagate past load_config as a stack trace rather than a clean ConfigError, which conflicts with the CLI's error-handling contract.
🐛 Proposed fix
- judge_model = (data.get("judge") or {}).get("model")
+ judge_section = data.get("judge") or {}
+ if not isinstance(judge_section, dict):
+ raise ConfigError("[judge] must be a table")
+ judge_model = judge_section.get("model")As per coding guidelines, "CLI errors must be converted from CovenantError into one clean line with exit code 2; never emit a stack trace or swallow the error" — this bug's root cause in load_config would defeat that guarantee.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| judge_model = (data.get("judge") or {}).get("model") | |
| if judge_model is not None and not isinstance(judge_model, str): | |
| raise ConfigError("[judge].model must be a string") | |
| judge_section = data.get("judge") or {} | |
| if not isinstance(judge_section, dict): | |
| raise ConfigError("[judge] must be a table") | |
| judge_model = judge_section.get("model") | |
| if judge_model is not None and not isinstance(judge_model, str): | |
| raise ConfigError("[judge].model must be a string") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@covenant/config.py` around lines 80 - 82, The judge config parsing in
load_config is unsafe because `data.get("judge")` may be a non-dict and the
chained `.get("model")` can raise a raw AttributeError instead of ConfigError.
Add the same shape guard used in `_parse_probes` by checking that the `[judge]`
value is a dict before reading `model`, and raise a ConfigError with a clear
message when it is not. Keep the existing `judge_model` validation logic, but
make sure malformed `judge` entries are converted into clean config errors
rather than uncaught exceptions.
Source: Coding guidelines
| base_by = {probe_key(p["tool"], p.get("args")): p for p in baseline} | ||
| changes: list[Change] = [] | ||
| for lp in live: | ||
| bp = base_by.get(probe_key(lp["tool"], lp.get("args"))) | ||
| if bp is None: | ||
| continue | ||
| tool = str(lp["tool"]) | ||
| if lp.get("is_error"): | ||
| changes.append(Change( | ||
| tool, "behavior", None, "probe_errored", "degraded", | ||
| f"probe {tool}: live call returned an error - {lp.get('error')}", | ||
| )) | ||
| continue | ||
| base_fp, live_fp = bp["fingerprint"], fingerprint(lp["response"]) | ||
| if _type_set(base_fp) == {"object"} and _type_set(live_fp) == {"object"}: | ||
| raw = _diff_object(tool, "output", base_fp, live_fp) | ||
| else: | ||
| raw = _diff_field(tool, "output", "response", base_fp, live_fp) | ||
| changes += [ | ||
| replace(c, location="behavior", message=f"probe {tool}: {c.message}") | ||
| for c in raw | ||
| ] | ||
| return changes |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map relevant functions/symbols in covenant/diff.py, then inspect targeted ranges.
ast-grep outline covenant/diff.py --view expanded || true
printf '\n---\n'
sed -n '1,340p' covenant/diff.py
printf '\n=== SEARCH fingerprint / CovenantError / check handling ===\n'
rg -n "def fingerprint|fingerprint\\(|CovenantError|check\\b|probe_errored|probe_key|Change\\(" covenant -SRepository: Mhemd139/Covenant
Length of output: 16656
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '=== covenant/fingerprint.py ===\n'
sed -n '1,220p' covenant/fingerprint.py
printf '\n=== covenant/cli.py (check path) ===\n'
sed -n '1,220p' covenant/cli.py
printf '\n=== baseline / fingerprint key searches ===\n'
rg -n '"fingerprint"|probe_errored|snapshot\(' covenant -SRepository: Mhemd139/Covenant
Length of output: 9590
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '=== covenant/contract.py ===\n'
sed -n '1,260p' covenant/contract.py
printf '\n=== covenant/errors.py ===\n'
sed -n '1,120p' covenant/errors.py
printf '\n=== read_baseline / write_baseline usages ===\n'
rg -n "read_baseline\\(|write_baseline\\(" covenant -SRepository: Mhemd139/Covenant
Length of output: 4624
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '=== covenant/introspect.py ===\n'
sed -n '1,260p' covenant/introspect.py
printf '\n=== covenant/config.py (probe shape) ===\n'
sed -n '1,220p' covenant/config.py
printf '\n=== search for non-JSON-native probe responses ===\n'
rg -n "run_probes|response.*object|response.*json|is_error|json.loads|json.dumps|result" covenant -SRepository: Mhemd139/Covenant
Length of output: 12806
🌐 Web query:
Model Context Protocol CallToolResult structuredContent JSON compatible text content response semantics
💡 Result:
In the Model Context Protocol (MCP), the CallToolResult object is used to return the output of a tool invocation [1]. It supports two primary fields for conveying results: content and structuredContent [2][3]. The content field is for unstructured results and consists of an array of content blocks (e.g., text, images, audio, or resource links) [2]. This field is the primary mechanism for conveying human-readable or multimodal information to the model [4]. The structuredContent field is an optional JSON value (which can be an object, array, string, number, boolean, or null) designed to provide machine-readable output [2][4][3]. If a tool definition includes an outputSchema, any structuredContent returned by the server must conform to that schema [2][3]. Clients are encouraged to validate this content against the defined schema [2][3]. For backwards compatibility, it is recommended that if a tool returns structuredContent, it should also provide a serialized version of that data within a TextContent block inside the content array [5][2][6]. In summary, the semantics are as follows: - content: Used for unstructured, multimodal, or human-readable data [2][4]. - structuredContent: Used for machine-readable data, ideally validated against a tool's outputSchema [2][3]. - Backwards compatibility: Best practice is to include a serialized copy of the structured data in the content field so that clients unaware of the structuredContent field can still access the data [5][2].
Citations:
- 1: https://github.com/modelcontextprotocol/csharp-sdk/blob/main/src/ModelContextProtocol.Core/Protocol/CallToolResult.cs
- 2: https://modelcontextprotocol.io/specification/2025-11-25/server/tools.md
- 3: https://modelcontextprotocol.io/specification/draft/server/tools
- 4: https://kotlin.sdk.modelcontextprotocol.io/kotlin-sdk-core/io.modelcontextprotocol.kotlin.sdk.types/-call-tool-result/index.html
- 5: What is the expected usage of `structuredContent` vs `content` in `CallToolResult`? modelcontextprotocol/modelcontextprotocol#1563
- 6: https://modelcontextprotocol.io/specification/2025-06-18/server/tools
Guard missing probe fingerprints in baseline records
covenant/introspect.py already normalizes live probe responses, but bp["fingerprint"] can still raise a raw KeyError if a baseline probe record is malformed. Handle that path here so covenant check returns a clean CovenantError instead of a traceback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@covenant/diff.py` around lines 245 - 267, Handle malformed baseline probe
records in the probe comparison logic so a missing fingerprint does not raise a
raw KeyError. Update the baseline lookup and fingerprint access in the
change-building loop in covenant/diff.py to validate bp before using
bp["fingerprint"], and route the failure through the existing CovenantError path
used by covenant check. Keep the fix localized around the probe comparison code
that uses base_by, probe_key, and fingerprint.
| async def _run_probes(config: Config, probes: list[Probe]) -> list[JsonDict]: | ||
| records: list[JsonDict] = [] | ||
| async with _session(config) as session: | ||
| for probe in probes: | ||
| result = await session.call_tool(probe.tool, probe.args) | ||
| response, is_error, error = _resolve_result(result) | ||
| records.append({ | ||
| "tool": probe.tool, | ||
| "args": probe.args, | ||
| "response": response, | ||
| "is_error": is_error, | ||
| "error": error, | ||
| }) | ||
| return records |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Per-probe failures are reported as generic connection errors.
If session.call_tool raises (e.g., unknown tool name, invalid arguments, protocol error from the server) partway through the loop, run_probes swallows it into ConnectionError(f"could not probe MCP server ({target}): {e}") — losing which probe/tool actually failed. For a config-level mistake (typo'd tool name, wrong args) this misleadingly reads as a connectivity problem rather than a probe/config issue, making covenant snapshot/check failures harder to diagnose.
🔍 Suggested fix
async with _session(config) as session:
for probe in probes:
- result = await session.call_tool(probe.tool, probe.args)
+ try:
+ result = await session.call_tool(probe.tool, probe.args)
+ except Exception as e:
+ raise ConnectionError(f"probe {probe.tool} failed: {e}") from e
response, is_error, error = _resolve_result(result)Also applies to: 118-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@covenant/introspect.py` around lines 91 - 104, The per-probe failure handling
in _run_probes is too generic because exceptions from session.call_tool are
later surfaced as a ConnectionError, hiding which probe/tool failed and making
config mistakes look like connectivity issues. Update the probe execution path
in _run_probes and the caller in run_probes to catch per-tool exceptions,
preserve the failing probe’s tool/args context, and propagate or record that
specific error so snapshot/check can report a probe/config problem instead of a
generic server connection failure.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== File outline ==\n'
ast-grep outline covenant/introspect.py --view expanded || true
printf '\n== Relevant lines in covenant/introspect.py ==\n'
cat -n covenant/introspect.py | sed -n '1,220p'
printf '\n== Search for call_tool usages and timeout handling ==\n'
rg -n "call_tool\\(|read_timeout_seconds|timedelta\\(" covenant . || true
printf '\n== Search for MCP session type / docs references ==\n'
rg -n "read_timeout_seconds|call_tool" . || trueRepository: Mhemd139/Covenant
Length of output: 6997
🌐 Web query:
MCP Python SDK session.call_tool read_timeout_seconds default hangs forever docs
💡 Result:
In the Model Context Protocol (MCP) Python SDK, the session.call_tool method does not have a default timeout that automatically terminates requests, which can lead to situations where the client hangs indefinitely if the server fails to respond [1][2]. While the call_tool method signature accepts an optional read_timeout_seconds parameter (often as a timedelta object), it is not always enforced by default across all transport layers or internal states, leading to the reported "hang forever" behavior [1][3][2]. Key technical insights regarding this behavior: 1. Lack of Mandatory Default: There is no global, enforced default timeout for call_tool requests in older versions or specific transport implementations (like early WebSocket connectors), meaning the call relies on the underlying connection remaining active [1][2][4]. 2. Transport Limitations: Even if a transport (such as SSE) has a configured read timeout, it may not propagate effectively to the specific call_tool request, causing the client to wait indefinitely for an asyncio.Future that never resolves [1][2][5]. 3. Recommended Workaround: Developers are advised to wrap session.call_tool calls in an explicit asyncio.wait_for block to ensure that tool execution is bounded by a concrete timeout, allowing the application to handle failures or trigger reconnections gracefully [2]. Example of the recommended pattern: import asyncio from datetime import timedelta try: # Use asyncio.wait_for to enforce a strict timeout result = await asyncio.wait_for( session.call_tool("tool_name", arguments={}), timeout=30.0) except asyncio.TimeoutError: # Handle the timeout, e.g., by logging or reconnecting print("Tool call timed out") Recent updates to the SDK (e.g., in v2 and associated patches) have focused on improving timeout propagation, ensuring that read_timeout_seconds is respected by connectors and correctly interrupting pending requests [5][6]. Always check the specific version of the SDK you are using, as older versions were more susceptible to these indefinite hangs [2][6].
Citations:
- 1: Client hangs forever modelcontextprotocol/python-sdk#1789
- 2: cannot get response from await session.call_tool() modelcontextprotocol/python-sdk#262
- 3: SDKs and other middleware SHOULD allow these timeouts to be configured on a per-request basis. modelcontextprotocol/python-sdk#600
- 4: Python WebSocketConnector.call_tool is incompatible with MCPSession read_timeout_seconds mcp-use/mcp-use#1731
- 5: fix(websocket): respect read_timeout_seconds for tool calls mcp-use/mcp-use#1773
- 6: [v2] ClientSession runs on JSONRPCDispatcher; BaseSession removed modelcontextprotocol/python-sdk#2838
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== covenant/introspect.py relevant excerpt =='
nl -ba covenant/introspect.py | sed -n '70,150p'
echo
echo '== Timeout-related references in covenant/introspect.py =='
rg -n "read_timeout_seconds|call_tool|timeout|timedelta" covenant/introspect.py || true
echo
echo '== MCP session/call_tool references elsewhere in repo =='
rg -n "read_timeout_seconds|call_tool" covenant || trueRepository: Mhemd139/Covenant
Length of output: 241
Add a timeout to probe tool calls. session.call_tool(...) here has no read_timeout_seconds (and the same path is used by run_probes), so a stalled server can hang snapshot/check indefinitely. Pass a default/configurable timeout or wrap the call in asyncio.wait_for.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@covenant/introspect.py` around lines 91 - 104, The probe execution path in
_run_probes (and the shared run_probes flow) calls session.call_tool without any
timeout, so a stalled server can block snapshot/check indefinitely. Update the
call to enforce a timeout by passing a default/configurable read_timeout_seconds
or by wrapping the tool invocation in asyncio.wait_for, and make sure the
timeout is applied consistently wherever probes are executed.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for any readOnlyHint / annotation-based guard on probe execution
rg -n "readOnlyHint|annotations" covenant/ tests/Repository: Mhemd139/Covenant
Length of output: 1110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file list =="
git ls-files covenant/introspect.py covenant/config.py covenant/cli.py covenant/fingerprint.py covenant/_types.py
echo
echo "== outlines =="
ast-grep outline covenant/introspect.py --view expanded || true
echo
ast-grep outline covenant/config.py --view expanded || true
echo
ast-grep outline covenant/cli.py --view expanded || true
echo
ast-grep outline covenant/fingerprint.py --view expanded || true
echo
echo "== targeted reads =="
for f in covenant/introspect.py covenant/config.py covenant/cli.py covenant/fingerprint.py; do
echo "--- $f ---"
wc -l "$f"
done
echo
echo "== covenant/introspect.py relevant slice =="
sed -n '1,220p' covenant/introspect.py
echo
echo "== covenant/config.py relevant slice =="
sed -n '1,260p' covenant/config.py
echo
echo "== covenant/cli.py relevant slice =="
sed -n '1,260p' covenant/cli.py
echo
echo "== covenant/fingerprint.py relevant slice =="
sed -n '1,260p' covenant/fingerprint.pyRepository: Mhemd139/Covenant
Length of output: 19013
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== find readOnlyHint and probe-related validation =="
rg -n "readOnlyHint|read_only|read-only|probe|Probe|tool.*readonly|readonly.*tool" covenant tests/
echo
echo "== locate schema_hash =="
rg -n "schema_hash|description" covenantRepository: Mhemd139/Covenant
Length of output: 12280
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== covenant/introspect.py =="
sed -n '1,220p' covenant/introspect.py
echo
echo "== covenant/config.py =="
sed -n '1,260p' covenant/config.py
echo
echo "== covenant/cli.py =="
sed -n '1,240p' covenant/cli.py
echo
echo "== covenant/fingerprint.py =="
sed -n '1,220p' covenant/fingerprint.py
echo
echo "== search for any read-only gating or tool metadata shaping =="
rg -n "readOnlyHint|readonly|read-only|annotations|tool.*dict|_tool_to_dict|call_tool|list_tools|probes" covenant tests/Repository: Mhemd139/Covenant
Length of output: 24770
Reject non-read-only probes. _run_probes will execute any configured tool during snapshot/check, but Probe and _tool_to_dict carry no readOnlyHint, so a misconfigured probe can still mutate the server. Add a runtime read-only check or thread the annotation into the probe pipeline.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@covenant/introspect.py` around lines 91 - 104, The _run_probes flow currently
executes every Probe.tool without verifying it is read-only, so a misconfigured
snapshot/check probe can mutate state. Add a runtime guard in _run_probes (or
earlier in the Probe/_tool_to_dict pipeline) to reject any probe whose tool is
not marked read-only, using the existing Probe and _resolve_result/_session flow
to keep the check centralized.
Source: Coding guidelines
| try: | ||
| data = json.loads(raw) | ||
| return Verdict(drift=bool(data["drift"]), reason=str(data["reason"])) | ||
| except (json.JSONDecodeError, KeyError, TypeError) as e: | ||
| raise CovenantError(f"judge returned an unparseable verdict: {raw!r}") from e |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
bool(data["drift"]) can mint a false positive from a stringy verdict.
If the model replies with a quoted value (e.g. {"drift": "false"}), bool("false") is True, so a non-drift verdict is reported as drift (a spurious DEGRADED). Given the module's loud-failure philosophy, reject a non-boolean drift instead of coercing it — TypeError is already caught and converted to a clean CovenantError.
🛡️ Proposed fix
data = json.loads(raw)
- return Verdict(drift=bool(data["drift"]), reason=str(data["reason"]))
+ drift = data["drift"]
+ if not isinstance(drift, bool):
+ raise TypeError("drift must be a JSON boolean")
+ return Verdict(drift=drift, reason=str(data["reason"]))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| data = json.loads(raw) | |
| return Verdict(drift=bool(data["drift"]), reason=str(data["reason"])) | |
| except (json.JSONDecodeError, KeyError, TypeError) as e: | |
| raise CovenantError(f"judge returned an unparseable verdict: {raw!r}") from e | |
| try: | |
| data = json.loads(raw) | |
| drift = data["drift"] | |
| if not isinstance(drift, bool): | |
| raise TypeError("drift must be a JSON boolean") | |
| return Verdict(drift=drift, reason=str(data["reason"])) | |
| except (json.JSONDecodeError, KeyError, TypeError) as e: | |
| raise CovenantError(f"judge returned an unparseable verdict: {raw!r}") from e |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@covenant/judge/__init__.py` around lines 76 - 80, The verdict parsing in the
judge module is coercing `data["drift"]` with `bool(...)`, which can turn string
values like "false" into a true drift result. Update the `Verdict` construction
in `covenant/judge/__init__.py` to require `drift` to already be a real boolean
and reject any non-boolean value by raising `TypeError` or otherwise letting the
existing exception handling convert it to `CovenantError`. Keep the
`json.loads`/`Verdict(...)` flow in the same parsing block so invalid model
output fails loudly rather than being coerced.
| - `list` → array; `items` kept only when every element fingerprints identically, else bare | ||
| array. Accepted limitation: a uniform array going heterogeneous fingerprints as bare and | ||
| is not flagged — probe tools that return uniform collections. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline covenant/fingerprint.py --view expanded
rg -n -C3 '\b(list|array|items|fingerprint)\b' covenant/fingerprint.py covenant/diff.py tests/test_fingerprint.py tests/test_probes.pyRepository: Mhemd139/Covenant
Length of output: 11741
🏁 Script executed:
sed -n '240,320p' covenant/diff.py
printf '\n---\n'
sed -n '1,120p' tests/test_fingerprint.py
printf '\n---\n'
sed -n '1,220p' tests/test_probes.pyRepository: Mhemd139/Covenant
Length of output: 6592
🏁 Script executed:
sed -n '1,140p' covenant/diff.py
printf '\n---\n'
python3 - <<'PY'
from covenant.fingerprint import fingerprint
from covenant.diff import diff_probes
baseline = [{
"tool": "probe",
"args": {},
"fingerprint": fingerprint([1, 2]),
"sample": [1, 2],
}]
live = [{
"tool": "probe",
"args": {},
"response": [1, "x"],
}]
print("baseline_fp:", baseline[0]["fingerprint"])
print("live_fp:", fingerprint(live[0]["response"]))
print("changes:", diff_probes(baseline, live))
PYRepository: Mhemd139/Covenant
Length of output: 5781
Document the array-shape blind spot or add coverage
A uniform array that later becomes heterogeneous is fingerprinted as bare {"type":"array"}, so diff_probes() will miss that drift entirely. Add a regression test if collection-returning probes are in scope, or mark this as an explicit non-goal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@docs/superpowers/specs/2026-07-03-covenant-layer3-behavioral-probes-design.md`
around lines 34 - 36, The array fingerprinting behavior in the spec leaves a
blind spot in diff_probes() for collection-returning probes, since a uniform
array that later becomes heterogeneous is still treated as a bare array and the
drift is missed. Update the documentation around the array-shape rules to
explicitly mark this as a non-goal, or add a regression test covering the array
fingerprinting behavior so the limitation is captured; use the existing
array/list shape language in the spec to locate the affected section.
| def get_transactions(account_id: str) -> dict: | ||
| """List recent transactions for an account.""" | ||
| txns = _TRANSACTIONS.get(account_id, []) | ||
| if BEHAVIOR_DRIFT: # the response body changes; the declared (loose) schema does not | ||
| txns = [ | ||
| {"txn_id": t["txn_id"], "amount_cents": int(t["amount_usd"] * 100), | ||
| "merchant": t["merchant"]} | ||
| for t in txns | ||
| ] | ||
| return {"account_id": account_id, "transactions": txns} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check mypy strict config and whether examples/ is included/excluded.
rg -n 'strict|disallow_any_generics|mypy' pyproject.toml
rg -n 'exclude|files' pyproject.toml -A3 | rg -n 'mypy' -B3Repository: Mhemd139/Covenant
Length of output: 235
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect mypy config around strict mode and overrides.
sed -n '70,120p' pyproject.toml
# Inspect the target file around the function.
sed -n '90,120p' examples/mcp_server.py
# Find any mypy-related overrides or file exclusions that mention examples/.
rg -n 'examples/|exclude|files|strict|disallow_any_generics|disable_error_code|ignore_errors' pyproject.toml -A4 -B4Repository: Mhemd139/Covenant
Length of output: 1796
Type the return value here
get_transactions returns a bare dict, which will fail this repo's strict mypy checks. Use a parameterized mapping or a TypedDict for the response shape.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/mcp_server.py` around lines 103 - 112, get_transactions currently
returns an unparameterized dict, which violates the strict mypy settings; update
the return type on get_transactions to a concrete typed mapping or a TypedDict
that matches the response shape. Define and use a named response type for the
account_id/transactions payload, and make sure the txns transformation in
get_transactions still fits that declared shape.
| args = { account_id = "acct-001" } | ||
| ``` | ||
|
|
||
| `covenant snapshot` runs each probe and stores its response **fingerprint** — the type shape of what actually came back, never the values, which legitimately change — in the lock. `covenant check` re-runs the probes and classifies shape drift with the same severity model, at location `behavior`: |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Document that probe snapshots persist a sample response.
Line 97 says the lock stores only the fingerprint and "never the values," but the new snapshot format also persists a sample response. That misstates retention and can mislead users about what lands in covenant.lock.json.
Suggested wording
-`covenant snapshot` runs each probe and stores its response **fingerprint** — the type shape of what actually came back, never the values, which legitimately change — in the lock.
+`covenant snapshot` runs each probe and stores its response **fingerprint** plus a sample response in the lock; use only safe, read-only probes and avoid sensitive outputs.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `covenant snapshot` runs each probe and stores its response **fingerprint** — the type shape of what actually came back, never the values, which legitimately change — in the lock. `covenant check` re-runs the probes and classifies shape drift with the same severity model, at location `behavior`: | |
| `covenant snapshot` runs each probe and stores its response **fingerprint** plus a sample response in the lock; use only safe, read-only probes and avoid sensitive outputs. `covenant check` re-runs the probes and classifies shape drift with the same severity model, at location `behavior`: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 97, The README wording in the covenant snapshot/check
description is outdated and says the lock stores only a fingerprint and never
values; update that sentence to reflect the new snapshot format that also
persists a sample response in covenant.lock.json. Adjust the text around the
covenant snapshot and covenant check description so it accurately describes what
is retained, while keeping the explanation of fingerprint/shape drift and
behavior severity consistent.
gemini-* models use GOOGLE_API_KEY via google-genai; everything else stays
on the Anthropic SDK. No new config: [judge].model already existed.
Fixes a real Python 3.14 hazard found running the judge live: a chained
`genai.Client().models.generate_content(...)` lets the temporary Client be
garbage-collected mid-call, and the SDK's __del__ closes its httpx
transport ("Cannot send a request, as the client has been closed").
Both providers now hold the client in a local for the duration of the call.
Verified live with GOOGLE_API_KEY: COVENANT_SEMANTIC_DRIFT=1 check --judge
--strict flags "balance_usd ... 100x larger than the baseline" (DEGRADED,
exit 1); clean check --judge stays 0. 114 tests, ruff + strict mypy clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fix: CodeRabbit review fixes for Layer 3 (missed by the #2 merge)
feat(layer-3): behavioral probes + LLM semantic judge
fix: CodeRabbit review fixes for Layer 3 (missed by the #2 merge)
1 participant
What
Layer 3 of the roadmap, end-to-end. Two detectors for drift that schema diffing cannot see:
[[probes]]incovenant.tomlare safe, read-only example calls.covenant snapshotruns them and stores each response's fingerprint (type shape, never values) plus a sample in the lock.covenant checkre-runs them and classifies shape drift with the unchanged Layer 0 classifier — responses are output-side by definition, so the direction principle applies verbatim (lost field = BREAKING, scalar retype = DEGRADED). Rendered at locationbehavior.covenant check --judge(optional[judge]extra) sends baseline sample + live response to an LLM for meaning-level drift (a balance quietly rescaled to cents). Verdicts are advisory DEGRADED, never BREAKING: a probabilistic detector must not trigger quarantine.Why it matters
Most real MCP tools declare no
outputSchemaat all — Layer 0 has nothing to diff there. Probes fingerprint what tools actually return, so a server whose schema says one thing and whose body does another is caught. This closes the caveat the clean-check note has carried since Layer 0.Demo levers (examples/mcp_server.py)
COVENANT_BEHAVIOR_DRIFT=1— body-only field rename, schema identical → probe catches BREAKING, exit 1. CI dogfoods this on every push.COVENANT_SEMANTIC_DRIFT=1— balance ×100, schema and shape identical → only--judgesees it.Verified
covenant check0 ·COVENANT_DRIFT=11 (now caught in schema and body) ·COVENANT_BEHAVIOR_DRIFT=11 (schema check alone would pass)docs/superpowers/specs/2026-07-03-covenant-layer3-behavioral-probes-design.md🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation