release: v0.13.5 (triage) — bug fixes + event vocabulary backport (#74, #95, #97, #98, #124)#128
Merged
Conversation
Logs the architectural suggestion received during PR #93 review as a v1.0.0-candidate RFC. Decision blocked on multi-machine/team-sync roadmap call; if not on the roadmap, META_LEDGER + the existing CHANGEFEED on compliance_check already provide ~80% of the cited benefits. Issue #97 carries the full analysis, the proposed v0.14.0 wedge (extend CHANGEFEED to all mutation-bearing tables), and the open questions for the maintainer. This entry is the single-line BACKLOG index reference. Refs #97
…v0.14.0) (#95) Privacy-first observability foundation. Authored via QorLogic SDLC (plan → audit → implement → substantiate). Builds on the dev branch post-merge with main's v0.13.x telemetry refactor. Closes #39 — Local-only counter sink at ~/.bicameral/counters.jsonl. Records only {tool_name, delta=1, ts}; mode 0o600 on POSIX; thread-safe; no network egress. Always-on alongside the network relay (counters are local introspection, distinct from outbound telemetry). Kill-switch: BICAMERAL_LOCAL_COUNTERS=0. New module local_counters.py with increment(tool_name) and read_counters() API. Closes #42 — bicameral.usage_summary MCP tool. Aggregates ingest/bind call counts (from #39's counters file) plus decision counts by status (from ledger) and cosmetic-drift percentage (from compliance_check verdicts) over a configurable window. Returns counts and floats only — no event rows, no user content. New module handlers/usage_summary.py. Adjacent to #39: consent.py — owns ~/.bicameral/consent.json, telemetry_allowed() predicate (single source of truth gating the relay), and notify_if_first_run() non-blocking notice. Marker has acknowledged_via field distinguishing "wizard" from "first_boot_notice" for future audit. POLICY_VERSION constant re-fires the notice for everyone if the telemetry policy ever changes. telemetry.send_event: - now uses consent.telemetry_allowed() as the single gating predicate - always increments the local counter before the relay path (wrapped in try/except — failure cannot affect the caller or the relay) setup_wizard._select_telemetry: - writes the consent marker on every answer (wizard, non-interactive default, both) - raises OSError on marker write failure — guarantees a "no" answer cannot silently leave telemetry on server.serve_stdio: - calls consent.notify_if_first_run() once at startup, never blocking CI: BICAMERAL_SKIP_CONSENT_NOTICE=1 added to test job env. tests/conftest.py: session-scoped autouse fixture reroutes ~/.bicameral/ to a per-session tmp dir; stdlib only. Tests: 23 pass, 1 skipped (POSIX-only file mode). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a telemetry consent system, local per-user append-only counters, a new MCP tool Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Consent
participant Counters as LocalCounters
participant Telemetry
participant Ledger
participant Relay as RemoteRelay
Client->>Server: Start
Server->>Consent: notify_if_first_run()
Consent-->>Server: (optional first-run notice)
Client->>Server: send_event(skill)
Server->>Telemetry: send_event(skill)
Telemetry->>Counters: increment(skill)
Counters-->>Telemetry: (logged)
Telemetry->>Consent: telemetry_allowed()
Consent-->>Telemetry: allowed / denied
alt allowed
Telemetry->>Relay: POST event
Relay-->>Telemetry: response
else denied
Telemetry-->>Server: (suppress relay)
end
Client->>Server: call_tool("bicameral.usage_summary", days)
Server->>Counters: read_counters()
Server->>Ledger: query decisions/compliance since cutoff
Ledger-->>Server: grouped rows
Server-->>Client: usage summary payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server.py (1)
88-107:⚠️ Potential issue | 🔴 CriticalAdd
bicameral.usage_summarytoEXPECTED_TOOL_NAMES.The tool is registered in
list_tools()but missing from the expected tool names list. The smoke test will fail at line 1074 with a mismatch error when it compares the actual tools returned bylist_tools()againstEXPECTED_TOOL_NAMES.Proposed fix
EXPECTED_TOOL_NAMES = [ "bicameral.link_commit", "bicameral.ingest", "bicameral.bind", "bicameral.update", "bicameral.reset", "bicameral.preflight", "bicameral.judge_gaps", "bicameral.resolve_compliance", "bicameral.ratify", "bicameral.resolve_collision", "bicameral.history", "bicameral.dashboard", "bicameral.skill_begin", "bicameral.skill_end", "bicameral.feedback", + "bicameral.usage_summary", "validate_symbols", "get_neighbors", "extract_symbols", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` around lines 88 - 107, The EXPECTED_TOOL_NAMES list is missing "bicameral.usage_summary", causing the smoke test comparing list_tools() output to fail; update the EXPECTED_TOOL_NAMES constant to include "bicameral.usage_summary" alongside the other entries (e.g., near "bicameral.feedback") so the set of expected tool names matches what list_tools() registers.
🧹 Nitpick comments (2)
server.py (1)
1107-1114: 💤 Low valueConsider logging the swallowed exception instead of silent
pass.Static analysis flags this as
try-except-pass(S110). Whilenotify_if_first_run()is documented to never raise and has internal exception handling, the outer defensive catch here silently discards any unexpected import or module-level errors. A debug log would aid troubleshooting without blocking startup.♻️ Proposed improvement
try: from consent import notify_if_first_run notify_if_first_run() - except Exception: - pass + except Exception as exc: + import logging + logging.getLogger(__name__).debug("[serve_stdio] consent notice failed: %s", exc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` around lines 1107 - 1114, The try/except around the consent import and notify_if_first_run() silently swallows errors; change it to catch Exception as e and emit a debug-level log including the exception details (e.g. logger.debug("notify_if_first_run failed", exc_info=True) or similar) before continuing so unexpected import/module errors aren’t hidden — update the block that imports from consent and calls notify_if_first_run() to log the exception instead of using a bare pass.handlers/usage_summary.py (1)
24-28: 💤 Low value
error_rateis declared but never computed.The docstring lists
error_rateas part of the return schema, but it remains0.0throughout the handler. Consider either implementing the computation or removing it from the schema/docstring if it's planned for a future iteration.Also applies to: 43-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@handlers/usage_summary.py` around lines 24 - 28, The docstring lists error_rate but the variable error_rate in handlers/usage_summary.py is left at 0.0; either compute it from existing error/total counters or remove it from the returned schema and docstring. Locate the function that builds and returns the tuple/list including period_days, ingest_calls, bind_calls_total, decisions_*, and error_rate (search for the return containing those names), then if an error counter exists (e.g., error_count, errors, ingest_errors) compute error_rate = error_count / max(1, total_attempts) using an appropriate denominator (total calls/decisions) and replace the constant 0.0 with that value; otherwise remove error_rate from the returned structure and update the docstring/schema list to no longer include error_rate. Ensure the change keeps the return shape consistent with callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 68-70: The "Closes" section in CHANGELOG.md currently contains a
line that begins with "#39, `#42`." which is parsed as a heading and triggers
markdownlint MD018; replace that line with plain text such as "Closes: `#39`,
`#42`." (or escape the hashes) so the entry remains the same meaning but no longer
starts with '#' and the linter/docs checks pass.
In `@handlers/usage_summary.py`:
- Around line 84-100: The query is filtering for a verdict value
"cosmetic_autopass" that doesn't exist in compliance_check.verdict, so
cosmetic_drift_pct will always be zero; fix by either (A) adding
"cosmetic_autopass" to the verdict enum in ledger/schema.py so client.query("...
WHERE verdict IN ['drifted','cosmetic_autopass'] ...") becomes valid, or (B)
change the usage_summary query to compute cosmetic from the correct field/table
(e.g., an autopass boolean or a separate column) instead of verdict — update the
client.query call and the cosmetic = ... calculation accordingly so
base["cosmetic_drift_pct"] is derived from real data.
In `@local_counters.py`:
- Around line 79-85: The current parsing loop only catches JSONDecodeError so a
bad UTF-8 decode or non-dict JSON can escape and abort aggregation; wrap
raw.decode("utf-8") and json.loads(...) in a try/except that also catches
UnicodeDecodeError and json.JSONDecodeError and continues on error, then ensure
the parsed rec is a dict (e.g., check isinstance(rec, dict)) before using
rec.get to avoid AttributeError/TypeError, and only apply counts[tool] += delta
after validating tool is str and delta is int (or coerce/validate delta) so
malformed lines are skipped without stopping the file processing.
In `@tests/conftest.py`:
- Around line 19-44: The session autouse fixture _isolate_consent_state runs too
late for import-time path constants; instead, move the environment override into
a pytest startup hook (implement pytest_configure or pytest_sessionstart) so
HOME/USERPROFILE/BICAMERAL_SKIP_CONSENT_NOTICE are set before test modules are
imported, and restore the original env in the complementary shutdown hook
(pytest_unconfigure or pytest_sessionfinish). In the configure hook create a
temporary directory (e.g., via tempfile.mkdtemp) and save the original env
values in pytestconfig or a module-level variable, set os.environ["HOME"],
os.environ["USERPROFILE"], and os.environ["BICAMERAL_SKIP_CONSENT_NOTICE"]="1"
there, and in the unconfigure hook restore saved values and remove the temp
directory; remove or convert the _isolate_consent_state fixture accordingly so
no late-setting fixture remains.
In `@tests/test_consent_notice.py`:
- Around line 178-200: The test comment claims "Counter should still increment
locally" but no assertion verifies this; update the
test_telemetry_send_event_blocked_when_consent_disabled function to either
remove that misleading comment or add an assertion against the local_counters
module to confirm the local counter increment after calling telemetry.send_event
(e.g., import local_counters and assert the appropriate counter/key has
increased compared to before calling telemetry.send_event), referencing the test
function name, the telemetry.send_event call, and the local_counters module to
locate where to add the check.
---
Outside diff comments:
In `@server.py`:
- Around line 88-107: The EXPECTED_TOOL_NAMES list is missing
"bicameral.usage_summary", causing the smoke test comparing list_tools() output
to fail; update the EXPECTED_TOOL_NAMES constant to include
"bicameral.usage_summary" alongside the other entries (e.g., near
"bicameral.feedback") so the set of expected tool names matches what
list_tools() registers.
---
Nitpick comments:
In `@handlers/usage_summary.py`:
- Around line 24-28: The docstring lists error_rate but the variable error_rate
in handlers/usage_summary.py is left at 0.0; either compute it from existing
error/total counters or remove it from the returned schema and docstring. Locate
the function that builds and returns the tuple/list including period_days,
ingest_calls, bind_calls_total, decisions_*, and error_rate (search for the
return containing those names), then if an error counter exists (e.g.,
error_count, errors, ingest_errors) compute error_rate = error_count / max(1,
total_attempts) using an appropriate denominator (total calls/decisions) and
replace the constant 0.0 with that value; otherwise remove error_rate from the
returned structure and update the docstring/schema list to no longer include
error_rate. Ensure the change keeps the return shape consistent with callers.
In `@server.py`:
- Around line 1107-1114: The try/except around the consent import and
notify_if_first_run() silently swallows errors; change it to catch Exception as
e and emit a debug-level log including the exception details (e.g.
logger.debug("notify_if_first_run failed", exc_info=True) or similar) before
continuing so unexpected import/module errors aren’t hidden — update the block
that imports from consent and calls notify_if_first_run() to log the exception
instead of using a bare pass.
🪄 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: 593f462f-bc2a-4083-b54f-163f9b6f0f4e
📒 Files selected for processing (13)
.github/workflows/test-mcp-regression.ymlCHANGELOG.mdconsent.pydocs/BACKLOG.mdhandlers/usage_summary.pylocal_counters.pyserver.pysetup_wizard.pytelemetry.pytests/conftest.pytests/test_consent_notice.pytests/test_local_counters.pytests/test_usage_summary.py
Comment on lines
+68
to
+70
| ### Closes | ||
|
|
||
| #39, #42. |
There was a problem hiding this comment.
Fix issue-reference line that is parsed as a heading.
At Line 70, #39, #42. triggers markdownlint MD018 (missing space after #). Reword to plain text like Closes: #39, #42. (or escape the hashes) so lint/docs checks stay green.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 70-70: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` around lines 68 - 70, The "Closes" section in CHANGELOG.md
currently contains a line that begins with "#39, `#42`." which is parsed as a
heading and triggers markdownlint MD018; replace that line with plain text such
as "Closes: `#39`, `#42`." (or escape the hashes) so the entry remains the same
meaning but no longer starts with '#' and the linter/docs checks pass.
Comment on lines
+84
to
+100
| # Cosmetic drift: count compliance_check verdicts of cosmetic_autopass | ||
| # over total drift verdicts in the window. | ||
| try: | ||
| cc_rows = await client.query( | ||
| "SELECT verdict, count() AS n FROM compliance_check " | ||
| f"WHERE checked_at > <datetime>'{cutoff}' " | ||
| "AND verdict IN ['drifted', 'cosmetic_autopass'] GROUP BY verdict" | ||
| ) | ||
| cc_counts = { | ||
| r.get("verdict"): int(r.get("n", 0)) for r in (cc_rows or []) | ||
| } | ||
| cosmetic = cc_counts.get("cosmetic_autopass", 0) | ||
| drift_total = cosmetic + cc_counts.get("drifted", 0) | ||
| if drift_total > 0: | ||
| base["cosmetic_drift_pct"] = round(cosmetic / drift_total, 4) | ||
| except Exception as exc: | ||
| logger.debug("[usage_summary] cosmetic_drift query failed: %s", exc) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the compliance_check schema and check if cosmetic_autopass appears anywhere
echo "=== Schema definition for compliance_check verdict ==="
rg -n "verdict.*IN\s*\[" ledger/
echo ""
echo "=== All occurrences of cosmetic_autopass ==="
rg -n "cosmetic_autopass" --type py
echo ""
echo "=== compliance_check table definition ==="
rg -n -A5 "DEFINE TABLE compliance_check" ledger/Repository: BicameralAI/bicameral-mcp
Length of output: 1496
🏁 Script executed:
cat -n ledger/schema.py | sed -n '185,200p'Repository: BicameralAI/bicameral-mcp
Length of output: 1266
🏁 Script executed:
sed -n '70,90p' tests/test_usage_summary.pyRepository: BicameralAI/bicameral-mcp
Length of output: 1000
🏁 Script executed:
rg -n "cosmetic" ledger/schema.py -B2 -A2Repository: BicameralAI/bicameral-mcp
Length of output: 51
🏁 Script executed:
sed -n '185,205p' ledger/schema.pyRepository: BicameralAI/bicameral-mcp
Length of output: 1562
cosmetic_autopass is not a valid compliance_check.verdict value.
The schema at ledger/schema.py:190-191 restricts verdict to ['compliant', 'drifted', 'not_relevant']. Querying for 'cosmetic_autopass' will always return 0 rows, making cosmetic_drift_pct permanently stuck at 0.0.
Either add 'cosmetic_autopass' to the allowed verdict values in the schema, or track cosmetic drift using a different field/table.
🧰 Tools
🪛 Ruff (0.15.12)
[error] 88-90: Possible SQL injection vector through string-based query construction
(S608)
[warning] 99-99: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@handlers/usage_summary.py` around lines 84 - 100, The query is filtering for
a verdict value "cosmetic_autopass" that doesn't exist in
compliance_check.verdict, so cosmetic_drift_pct will always be zero; fix by
either (A) adding "cosmetic_autopass" to the verdict enum in ledger/schema.py so
client.query("... WHERE verdict IN ['drifted','cosmetic_autopass'] ...") becomes
valid, or (B) change the usage_summary query to compute cosmetic from the
correct field/table (e.g., an autopass boolean or a separate column) instead of
verdict — update the client.query call and the cosmetic = ... calculation
accordingly so base["cosmetic_drift_pct"] is derived from real data.
Comment on lines
+79
to
+85
| rec = json.loads(raw.decode("utf-8")) | ||
| except json.JSONDecodeError: | ||
| continue | ||
| tool = rec.get("tool") | ||
| delta = rec.get("delta", 1) | ||
| if isinstance(tool, str) and isinstance(delta, int): | ||
| counts[tool] += delta |
There was a problem hiding this comment.
Per-line decode/shape failures can stop aggregation early.
At Line 79, only JSONDecodeError is handled. A bad UTF-8 line or non-object JSON can fall through and trigger the outer exception path, skipping the rest of the file.
Suggested hardening
- try:
- rec = json.loads(raw.decode("utf-8"))
- except json.JSONDecodeError:
- continue
- tool = rec.get("tool")
- delta = rec.get("delta", 1)
+ try:
+ rec = json.loads(raw.decode("utf-8"))
+ except (UnicodeDecodeError, json.JSONDecodeError):
+ continue
+ if not isinstance(rec, dict):
+ continue
+ tool = rec.get("tool")
+ delta = rec.get("delta", 1)
if isinstance(tool, str) and isinstance(delta, int):
counts[tool] += delta🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@local_counters.py` around lines 79 - 85, The current parsing loop only
catches JSONDecodeError so a bad UTF-8 decode or non-dict JSON can escape and
abort aggregation; wrap raw.decode("utf-8") and json.loads(...) in a try/except
that also catches UnicodeDecodeError and json.JSONDecodeError and continues on
error, then ensure the parsed rec is a dict (e.g., check isinstance(rec, dict))
before using rec.get to avoid AttributeError/TypeError, and only apply
counts[tool] += delta after validating tool is str and delta is int (or
coerce/validate delta) so malformed lines are skipped without stopping the file
processing.
Comment on lines
+19
to
+44
| @pytest.fixture(scope="session", autouse=True) | ||
| def _isolate_consent_state(tmp_path_factory): | ||
| """Reroute ~/.bicameral/ to a per-session tmp dir and skip the consent | ||
| notice by default (issue #39). | ||
|
|
||
| Tests that explicitly exercise the consent-notice path unset | ||
| BICAMERAL_SKIP_CONSENT_NOTICE within the test body. Stdlib only — no | ||
| third-party fixture plugin. | ||
| """ | ||
| home = tmp_path_factory.mktemp("bicameral_home") | ||
| saved = { | ||
| k: os.environ.get(k) | ||
| for k in ("HOME", "USERPROFILE", "BICAMERAL_SKIP_CONSENT_NOTICE") | ||
| } | ||
| os.environ["HOME"] = str(home) | ||
| os.environ["USERPROFILE"] = str(home) | ||
| os.environ["BICAMERAL_SKIP_CONSENT_NOTICE"] = "1" | ||
| try: | ||
| yield home | ||
| finally: | ||
| for k, v in saved.items(): | ||
| if v is None: | ||
| os.environ.pop(k, None) | ||
| else: | ||
| os.environ[k] = v | ||
|
|
There was a problem hiding this comment.
Session autouse fixture applies too late for import-time home-path constants.
At Line 19, this fixture runs after test-module import/collection. Modules that compute paths at import time (e.g., consent.py and local_counters.py) can already be pinned to the real home dir before HOME/USERPROFILE are overridden, which breaks isolation and can leak files outside tmp test dirs.
Please move the env override to an earlier pytest hook (pytest_configure/pytest_sessionstart) and restore in pytest_unconfigure/pytest_sessionfinish.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/conftest.py` around lines 19 - 44, The session autouse fixture
_isolate_consent_state runs too late for import-time path constants; instead,
move the environment override into a pytest startup hook (implement
pytest_configure or pytest_sessionstart) so
HOME/USERPROFILE/BICAMERAL_SKIP_CONSENT_NOTICE are set before test modules are
imported, and restore the original env in the complementary shutdown hook
(pytest_unconfigure or pytest_sessionfinish). In the configure hook create a
temporary directory (e.g., via tempfile.mkdtemp) and save the original env
values in pytestconfig or a module-level variable, set os.environ["HOME"],
os.environ["USERPROFILE"], and os.environ["BICAMERAL_SKIP_CONSENT_NOTICE"]="1"
there, and in the unconfigure hook restore saved values and remove the temp
directory; remove or convert the _isolate_consent_state fixture accordingly so
no late-setting fixture remains.
Comment on lines
+178
to
+200
| def test_telemetry_send_event_blocked_when_consent_disabled( | ||
| tmp_path: Path, monkeypatch: pytest.MonkeyPatch | ||
| ) -> None: | ||
| """telemetry.send_event suppresses relay when consent says disabled.""" | ||
| monkeypatch.setenv("HOME", str(tmp_path)) | ||
| monkeypatch.setenv("USERPROFILE", str(tmp_path)) | ||
| monkeypatch.delenv("BICAMERAL_TELEMETRY", raising=False) | ||
| consent = _reload_consent() | ||
| consent.write_consent(telemetry=False, via="wizard") | ||
|
|
||
| import importlib | ||
| import telemetry | ||
| importlib.reload(telemetry) | ||
|
|
||
| # Patch the network path; if relay was attempted, this would be called. | ||
| sent = [] | ||
| monkeypatch.setattr(telemetry, "_send_bg", lambda payload: sent.append(payload)) | ||
| telemetry.send_event("0.13.3", skill="bicameral-ingest", duration_ms=100) | ||
| # Counter should still increment locally. | ||
| import local_counters | ||
| importlib.reload(local_counters) | ||
| # Relay was NOT called (consent denied). | ||
| assert sent == [] |
There was a problem hiding this comment.
Incomplete assertion: counter increment not verified.
The comment on line 196 states "Counter should still increment locally" but lines 197-198 only reload local_counters without asserting anything. Either remove the misleading comment or add the actual counter verification.
💚 Proposed fix to add the missing assertion
telemetry.send_event("0.13.3", skill="bicameral-ingest", duration_ms=100)
- # Counter should still increment locally.
- import local_counters
- importlib.reload(local_counters)
# Relay was NOT called (consent denied).
assert sent == []Or if local counter verification is intended:
telemetry.send_event("0.13.3", skill="bicameral-ingest", duration_ms=100)
- # Counter should still increment locally.
- import local_counters
- importlib.reload(local_counters)
+ # Verify counter incremented locally despite relay being blocked.
+ import local_counters
+ importlib.reload(local_counters)
+ counts = local_counters.read_counters()
+ assert counts.get("bicameral-ingest", 0) > 0, "local counter should increment even when relay disabled"
# Relay was NOT called (consent denied).
assert sent == []📝 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.
Suggested change
| def test_telemetry_send_event_blocked_when_consent_disabled( | |
| tmp_path: Path, monkeypatch: pytest.MonkeyPatch | |
| ) -> None: | |
| """telemetry.send_event suppresses relay when consent says disabled.""" | |
| monkeypatch.setenv("HOME", str(tmp_path)) | |
| monkeypatch.setenv("USERPROFILE", str(tmp_path)) | |
| monkeypatch.delenv("BICAMERAL_TELEMETRY", raising=False) | |
| consent = _reload_consent() | |
| consent.write_consent(telemetry=False, via="wizard") | |
| import importlib | |
| import telemetry | |
| importlib.reload(telemetry) | |
| # Patch the network path; if relay was attempted, this would be called. | |
| sent = [] | |
| monkeypatch.setattr(telemetry, "_send_bg", lambda payload: sent.append(payload)) | |
| telemetry.send_event("0.13.3", skill="bicameral-ingest", duration_ms=100) | |
| # Counter should still increment locally. | |
| import local_counters | |
| importlib.reload(local_counters) | |
| # Relay was NOT called (consent denied). | |
| assert sent == [] | |
| def test_telemetry_send_event_blocked_when_consent_disabled( | |
| tmp_path: Path, monkeypatch: pytest.MonkeyPatch | |
| ) -> None: | |
| """telemetry.send_event suppresses relay when consent says disabled.""" | |
| monkeypatch.setenv("HOME", str(tmp_path)) | |
| monkeypatch.setenv("USERPROFILE", str(tmp_path)) | |
| monkeypatch.delenv("BICAMERAL_TELEMETRY", raising=False) | |
| consent = _reload_consent() | |
| consent.write_consent(telemetry=False, via="wizard") | |
| import importlib | |
| import telemetry | |
| importlib.reload(telemetry) | |
| # Patch the network path; if relay was attempted, this would be called. | |
| sent = [] | |
| monkeypatch.setattr(telemetry, "_send_bg", lambda payload: sent.append(payload)) | |
| telemetry.send_event("0.13.3", skill="bicameral-ingest", duration_ms=100) | |
| # Relay was NOT called (consent denied). | |
| assert sent == [] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_consent_notice.py` around lines 178 - 200, The test comment claims
"Counter should still increment locally" but no assertion verifies this; update
the test_telemetry_send_event_blocked_when_consent_disabled function to either
remove that misleading comment or add an assertion against the local_counters
module to confirm the local counter increment after calling telemetry.send_event
(e.g., import local_counters and assert the appropriate counter/key has
increased compared to before calling telemetry.send_event), referencing the test
function name, the telemetry.send_event call, and the local_counters module to
locate where to add the check.
…vcrt) (#80) Issue #74: ``events/writer.py:16`` had a top-level ``import fcntl``, which is Unix-only. On Windows the import failed at module load, which collapsed any test session that imported (directly or transitively) ``events.writer`` — including all 17 ephemeral authoritative tests and a long tail of ingest-using tests. Fix: - Replace the top-level ``import fcntl`` with a platform-conditional block that imports either ``fcntl`` (POSIX) or ``msvcrt`` (Windows) and defines ``_lock_exclusive`` / ``_unlock`` helpers with matching semantics. - POSIX path uses ``fcntl.flock(LOCK_EX/LOCK_UN)`` — unchanged behaviour. - Windows path locks byte 0 with ``msvcrt.locking(LK_LOCK/LK_UNLCK, 1)`` so concurrent writers serialize on a shared mutex byte. The actual append happens via ``open(..., "ab")`` which on Windows seeks to EOF per write — the byte-0 lock is the serialization primitive, not a region lock. - Both branches use ``# pragma: no cover`` for the inactive platform. Tests: - ``tests/test_event_writer.py`` — new, 7 tests: - module imports cleanly on the current platform (regression for the original ImportError) - lock helpers exist and are callable - ``write()`` produces a parseable JSONL line - consecutive writes release the lock (would deadlock if leaked) - locking byte 0 on a previously-empty file works (Windows msvcrt edge case) - platform-specific dispatch checks (``test_windows_uses_msvcrt`` / ``test_posix_uses_fcntl``, mutually skipped) Verified on Windows: 6/6 active tests pass. Ephemeral authoritative suite went from 0/17 collectable to 15/17 passing (the remaining 2 are pre-existing V2 promotion gaps unrelated to fcntl). No POSIX behaviour change.
Wires the missing decision-status events into the existing JSONL + materializer pipeline so the shipped event vocabulary matches the v0 architecture description (decision_ratified, decision_superseded alongside the existing ingest/bind/link_commit events). Changes: - ledger/adapter.py: add `apply_ratify(decision_id, signoff)` and `apply_supersede(new_id, old_id, ...)` to SurrealDBLedgerAdapter. Both methods are idempotent so the materializer can replay them safely. They wrap the existing inline UPDATE + project + supersedes helpers — no behavioral change for solo mode. - events/team_adapter.py: add wrappers that emit `decision_ratified.completed` and `decision_superseded.completed` events before delegating to the inner adapter. Event payloads carry `canonical_id` (UUIDv5 from description + source_type + source_ref) so cross-author replay can resolve to the peer's local row even though SurrealDB-generated decision ids are per-DB. - events/materializer.py: replay cases for the two new event types. Each looks up the local decision row by canonical_id; warns and skips if not found (out-of-order replay across authors). - handlers/ratify.py: route through `ledger.apply_ratify` instead of inline UPDATE + project_decision_status + update_decision_status. Pre-write idempotency check (early return when state already matches) is unchanged. - handlers/resolve_collision.py: route through `ledger.apply_supersede` for the supersede branch. Edge creation + frozen-signoff merge moves into the adapter so it's reachable from replay. - ledger/queries.py: new `get_canonical_id(client, decision_id)` and `find_decision_by_canonical_id(client, canonical_id)` helpers. Tests: - tests/test_team_event_replay.py (new) — three round-trip tests: ratify, supersede (with edge replay), and ingest regression. Each ingests through team adapter A, then connects a fresh team adapter B pointing at the same JSONL log + a fresh memory:// inner DB and a fresh watermark. Asserts state in B matches what A wrote. - tests/test_preflight_id_plumbing.py — updated the ratify mock to match the new `ledger.apply_ratify` shape. Out of scope (deferred to future PRs): compliance_checked event (Phase 4 uses CHANGEFEED), CHANGEFEED extension to code_subject / subject_identity / binds_to / code_region (schema migration), SHA256 chain (strictly v1). Closes part of #97. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit e6d4b8f) Adaptation: ledger/queries.py — kept only #97's two new helpers (get_canonical_id, find_decision_by_canonical_id); auto-merge had inadvertently bundled the #77 update_decision_level block (PR #107, decision_level classifier, v0.16.0) which is a missing prerequisite on triage and not part of e6d4b8f's actual diff for this file Adaptation: handlers/ratify.py — kept only e6d4b8f's import-list change (drop update_decision_status); dropped the auto-merged preflight_telemetry import which is a missing prerequisite on triage (#65 preflight telemetry capture loop) and not referenced by the cherry-picked body Skip: tests/test_preflight_id_plumbing.py — kept triage's prior deletion of this file; e6d4b8f's update to its ratify mock is moot here
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@events/materializer.py`:
- Around line 97-147: The code advances the per-author watermark
(new_offsets[author] = f.tell()) even when canonical-ID resolution fails for
events like "decision_ratified.completed" and "decision_superseded.completed",
which causes permanently dropped replay; instead, when
find_decision_by_canonical_id returns None for either
local_id/local_new/local_old, stop processing further events for that author (do
not "continue" past the unresolved dependent event) and leave
new_offsets[author] pointing to the file position before the unresolved event so
the event can be retried later; modify the handling in the branches that use
find_decision_by_canonical_id (the decision_ratified.completed and
decision_superseded.completed cases), replacing the current continue behavior
with logic that records the current file offset (use f.tell() or the saved
offset before reading the event) and breaks out of the per-author processing
loop so apply_ratify/apply_supersede are only called when canonical resolution
succeeds.
In `@events/team_adapter.py`:
- Around line 150-158: The code calls get_canonical_id(...) which can return
None but still emits events (e.g., the decision_ratified.completed write block
using canonical_id and the analogous block at lines 176-183), producing
non-resolvable replay events; change the flow to fail-fast: after awaiting
get_canonical_id(self._inner._client, decision_id) check if canonical_id is None
and if so raise a clear exception (or log and raise) instead of calling
self._writer.write, otherwise proceed to write the event; apply the same
None-check-and-fail behavior to the other occurrence that writes events so no
event is emitted with a missing canonical_id.
In `@ledger/adapter.py`:
- Around line 1131-1133: The code interpolates decision_id and old_id directly
into SurrealQL (calls to self._client.query using f"UPDATE {decision_id} ..."
and similar at the later block), creating an injection risk; fix by validating
these identifier variables against a strict whitelist/regex (e.g. allow only
alphanumerics, hyphen, underscore and the expected namespace separator) before
interpolation, raising an exception if validation fails, and only then build the
query string (or use a safe quoting helper) so no unvalidated input reaches
f"...{decision_id}..." or f"...{old_id}..."; update both occurrences (the UPDATE
using decision_id and the other block referenced) and add unit tests for invalid
ids.
- Around line 1165-1172: The code uses **old_signoff when building the new
signoff but old_signoff (from rows[0].get("signoff")) can be non-dict; before
calling self._client.execute, ensure old_signoff is a dict (e.g., if not
isinstance(old_signoff, dict): old_signoff = {}) so dict unpack won't raise;
update the block around rows, old_signoff and the call to _client.execute to
coerce or replace non-dict signoff values with an empty dict before doing
**old_signoff.
In `@ledger/queries.py`:
- Around line 952-959: The get_canonical_id function currently interpolates
decision_id directly into SurrealQL which is unsafe; validate decision_id using
the module's existing _validated_record_id helper before using it in the query
(call _validated_record_id(decision_id) and use that validated value in the
LedgerClient.query call), then proceed to run the SELECT canonical_id query and
return the result as before; update references in get_canonical_id to use the
validated id and keep behavior of returning a string or None.
In `@tests/test_team_event_replay.py`:
- Around line 95-97: Replace f-string SQL interpolation in
tests/test_team_event_replay.py (calls to inner_a._client.query and any similar
uses like the queries referencing decision_id_a/decision_id_b) with
parameterized queries using the query(...) vars parameter (e.g., "SELECT signoff
FROM $id LIMIT 1", vars={"id": decision_id_a}) so the ledger client’s parameter
binding is used; alternatively, if you intentionally deem the interpolation safe
for this test, add a contextual "# noqa: S608" comment next to the offending
inner_a._client.query call(s) explaining why SQL injection is not possible in
this test, but prefer the vars-based refactor to satisfy Ruff S608.
🪄 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: 06bd9fbc-ca61-4090-b1ea-38203150f8b0
📒 Files selected for processing (9)
events/materializer.pyevents/team_adapter.pyevents/writer.pyhandlers/ratify.pyhandlers/resolve_collision.pyledger/adapter.pyledger/queries.pytests/test_event_writer.pytests/test_team_event_replay.py
Comment on lines
+97
to
147
| elif etype == "decision_ratified.completed": | ||
| # Resolve canonical_id → local decision_id; the | ||
| # event was emitted by a peer whose local | ||
| # decision_id is meaningless in this DB. | ||
| from ledger.queries import find_decision_by_canonical_id | ||
|
|
||
| local_id = await find_decision_by_canonical_id( | ||
| inner_adapter._client, | ||
| payload.get("canonical_id", ""), | ||
| ) | ||
| if local_id is None: | ||
| logger.warning( | ||
| "[materializer] skipping decision_ratified — " | ||
| "canonical_id %r not found locally (ingest event missing or out-of-order)", | ||
| payload.get("canonical_id"), | ||
| ) | ||
| continue | ||
| await inner_adapter.apply_ratify( | ||
| local_id, | ||
| payload.get("signoff", {}), | ||
| ) | ||
| replayed += 1 | ||
| elif etype == "decision_superseded.completed": | ||
| from ledger.queries import find_decision_by_canonical_id | ||
|
|
||
| local_new = await find_decision_by_canonical_id( | ||
| inner_adapter._client, | ||
| payload.get("new_canonical_id", ""), | ||
| ) | ||
| local_old = await find_decision_by_canonical_id( | ||
| inner_adapter._client, | ||
| payload.get("old_canonical_id", ""), | ||
| ) | ||
| if local_new is None or local_old is None: | ||
| logger.warning( | ||
| "[materializer] skipping decision_superseded — " | ||
| "canonical_id resolution failed (new=%r old=%r)", | ||
| payload.get("new_canonical_id"), | ||
| payload.get("old_canonical_id"), | ||
| ) | ||
| continue | ||
| await inner_adapter.apply_supersede( | ||
| new_id=local_new, | ||
| old_id=local_old, | ||
| signer=payload.get("signer", ""), | ||
| signoff_note=payload.get("signoff_note", ""), | ||
| superseded_at=payload.get("superseded_at", ""), | ||
| session_id=payload.get("session_id", ""), | ||
| ) | ||
| replayed += 1 | ||
| new_offsets[author] = f.tell() |
There was a problem hiding this comment.
Don’t advance watermark past unresolved canonical-ID events.
When canonical lookup fails, the event is skipped but the file offset still advances. That permanently drops replay for out-of-order dependencies.
🔧 Suggested approach (stop at first unresolved dependent event)
- with open(path, "rb") as f:
- f.seek(start)
- for raw in f:
+ with open(path, "rb") as f:
+ f.seek(start)
+ while True:
+ line_pos = f.tell()
+ raw = f.readline()
+ if not raw:
+ break
try:
event = json.loads(raw)
except json.JSONDecodeError:
continue
...
- if local_id is None:
+ if local_id is None:
logger.warning(...)
- continue
+ f.seek(line_pos)
+ break
...
- if local_new is None or local_old is None:
+ if local_new is None or local_old is None:
logger.warning(...)
- continue
+ f.seek(line_pos)
+ break
...
new_offsets[author] = f.tell()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@events/materializer.py` around lines 97 - 147, The code advances the
per-author watermark (new_offsets[author] = f.tell()) even when canonical-ID
resolution fails for events like "decision_ratified.completed" and
"decision_superseded.completed", which causes permanently dropped replay;
instead, when find_decision_by_canonical_id returns None for either
local_id/local_new/local_old, stop processing further events for that author (do
not "continue" past the unresolved dependent event) and leave
new_offsets[author] pointing to the file position before the unresolved event so
the event can be retried later; modify the handling in the branches that use
find_decision_by_canonical_id (the decision_ratified.completed and
decision_superseded.completed cases), replacing the current continue behavior
with logic that records the current file offset (use f.tell() or the saved
offset before reading the event) and breaks out of the per-author processing
loop so apply_ratify/apply_supersede are only called when canonical resolution
succeeds.
Comment on lines
+150
to
+158
| canonical_id = await get_canonical_id(self._inner._client, decision_id) | ||
| self._writer.write( | ||
| "decision_ratified.completed", | ||
| { | ||
| "canonical_id": canonical_id, | ||
| "decision_id": decision_id, | ||
| "signoff": signoff, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Fail fast if canonical IDs are missing before writing replay events.
get_canonical_id(...) can return None, but those values are emitted anyway. That produces non-resolvable events for peers.
🔧 Suggested fix
async def apply_ratify(self, decision_id: str, signoff: dict) -> str:
await self._ensure_ready()
canonical_id = await get_canonical_id(self._inner._client, decision_id)
+ if not canonical_id:
+ raise ValueError(f"Missing canonical_id for decision {decision_id}")
self._writer.write(
"decision_ratified.completed",
{
"canonical_id": canonical_id,
...
new_canonical = await get_canonical_id(self._inner._client, new_id)
old_canonical = await get_canonical_id(self._inner._client, old_id)
+ if not new_canonical or not old_canonical:
+ raise ValueError(
+ f"Missing canonical_id(s) for supersede: new={new_id} old={old_id}"
+ )
self._writer.write(
"decision_superseded.completed",Also applies to: 176-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@events/team_adapter.py` around lines 150 - 158, The code calls
get_canonical_id(...) which can return None but still emits events (e.g., the
decision_ratified.completed write block using canonical_id and the analogous
block at lines 176-183), producing non-resolvable replay events; change the flow
to fail-fast: after awaiting get_canonical_id(self._inner._client, decision_id)
check if canonical_id is None and if so raise a clear exception (or log and
raise) instead of calling self._writer.write, otherwise proceed to write the
event; apply the same None-check-and-fail behavior to the other occurrence that
writes events so no event is emitted with a missing canonical_id.
Comment on lines
+1131
to
+1133
| await self._client.query( | ||
| f"UPDATE {decision_id} SET signoff = $signoff", | ||
| {"signoff": signoff}, |
There was a problem hiding this comment.
Validate record IDs before interpolating into SurrealQL.
decision_id/old_id are inserted directly into query strings. That keeps an injection surface open if malformed IDs reach this layer.
🔧 Suggested hardening
+import re
+
+_DECISION_ID_RE = re.compile(r"^decision:[A-Za-z0-9_-]+$")
+
+def _validated_decision_id(value: str) -> str:
+ v = str(value or "")
+ if not _DECISION_ID_RE.fullmatch(v):
+ raise ValueError(f"Invalid decision id: {v!r}")
+ return v
...
async def apply_ratify(self, decision_id: str, signoff: dict) -> str:
await self._ensure_connected()
+ did = _validated_decision_id(decision_id)
await self._client.query(
- f"UPDATE {decision_id} SET signoff = $signoff",
+ f"UPDATE {did} SET signoff = $signoff",
{"signoff": signoff},
)
...
async def apply_supersede(...):
await self._ensure_connected()
+ oid = _validated_decision_id(old_id)
...
- rows = await self._client.query(f"SELECT signoff FROM {old_id} LIMIT 1")
+ rows = await self._client.query(f"SELECT signoff FROM {oid} LIMIT 1")
...
- f"UPDATE {old_id} SET signoff = $s",
+ f"UPDATE {oid} SET signoff = $s",Also applies to: 1163-1169
🧰 Tools
🪛 Ruff (0.15.12)
[error] 1132-1132: Possible SQL injection vector through string-based query construction
(S608)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ledger/adapter.py` around lines 1131 - 1133, The code interpolates
decision_id and old_id directly into SurrealQL (calls to self._client.query
using f"UPDATE {decision_id} ..." and similar at the later block), creating an
injection risk; fix by validating these identifier variables against a strict
whitelist/regex (e.g. allow only alphanumerics, hyphen, underscore and the
expected namespace separator) before interpolation, raising an exception if
validation fails, and only then build the query string (or use a safe quoting
helper) so no unvalidated input reaches f"...{decision_id}..." or
f"...{old_id}..."; update both occurrences (the UPDATE using decision_id and the
other block referenced) and add unit tests for invalid ids.
Comment on lines
+1165
to
+1172
| if rows and isinstance(rows[0], dict): | ||
| old_signoff = rows[0].get("signoff") or {} | ||
| await self._client.execute( | ||
| f"UPDATE {old_id} SET signoff = $s", | ||
| { | ||
| "s": { | ||
| **old_signoff, | ||
| "state": "superseded", |
There was a problem hiding this comment.
Guard old_signoff type before dict unpack.
old_signoff = rows[0].get("signoff") can be non-dict on bad/legacy data; **old_signoff would then raise at runtime.
🔧 Suggested fix
- old_signoff: dict = {}
+ old_signoff: dict = {}
if rows and isinstance(rows[0], dict):
- old_signoff = rows[0].get("signoff") or {}
+ raw_signoff = rows[0].get("signoff")
+ old_signoff = raw_signoff if isinstance(raw_signoff, dict) else {}🧰 Tools
🪛 Ruff (0.15.12)
[error] 1168-1168: Possible SQL injection vector through string-based query construction
(S608)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ledger/adapter.py` around lines 1165 - 1172, The code uses **old_signoff when
building the new signoff but old_signoff (from rows[0].get("signoff")) can be
non-dict; before calling self._client.execute, ensure old_signoff is a dict
(e.g., if not isinstance(old_signoff, dict): old_signoff = {}) so dict unpack
won't raise; update the block around rows, old_signoff and the call to
_client.execute to coerce or replace non-dict signoff values with an empty dict
before doing **old_signoff.
Comment on lines
+952
to
+959
| async def get_canonical_id( | ||
| client: LedgerClient, | ||
| decision_id: str, | ||
| ) -> str | None: | ||
| """Return the canonical_id for a local decision row, or None.""" | ||
| rows = await client.query( | ||
| f"SELECT canonical_id FROM {decision_id} LIMIT 1", | ||
| ) |
There was a problem hiding this comment.
Harden get_canonical_id with record-ID validation.
This helper directly interpolates decision_id into SurrealQL. Validate it first (same module already has _validated_record_id).
🔧 Suggested fix
async def get_canonical_id(
client: LedgerClient,
decision_id: str,
) -> str | None:
"""Return the canonical_id for a local decision row, or None."""
+ did = _validated_record_id(decision_id, "decision")
rows = await client.query(
- f"SELECT canonical_id FROM {decision_id} LIMIT 1",
+ f"SELECT canonical_id FROM {did} LIMIT 1",
)🧰 Tools
🪛 Ruff (0.15.12)
[error] 958-958: Possible SQL injection vector through string-based query construction
(S608)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ledger/queries.py` around lines 952 - 959, The get_canonical_id function
currently interpolates decision_id directly into SurrealQL which is unsafe;
validate decision_id using the module's existing _validated_record_id helper
before using it in the query (call _validated_record_id(decision_id) and use
that validated value in the LedgerClient.query call), then proceed to run the
SELECT canonical_id query and return the result as before; update references in
get_canonical_id to use the validated id and keep behavior of returning a string
or None.
Comment on lines
+95
to
+97
| rows = await inner_a._client.query( | ||
| f"SELECT signoff FROM {decision_id_a} LIMIT 1" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Confirm Ruff config includes S608 for this repo/tests
fd -i 'pyproject.toml|ruff.toml|.ruff.toml|setup.cfg' \
| xargs -r rg -n 'S608|flake8-bandit|select|extend-select|ignore|per-file-ignores'
# 2) Enumerate string-formatted query callsites in this test
rg -nP 'query\(\s*f"' tests/test_team_event_replay.py
# 3) Check existing repo precedent for suppressing S608 in tests
rg -nP --iglob '*.py' 'noqa:\s*S608'Repository: BicameralAI/bicameral-mcp
Length of output: 703
🏁 Script executed:
# Read the test file to check lines mentioned in the review
cat -n tests/test_team_event_replay.py | sed -n '90,170p'Repository: BicameralAI/bicameral-mcp
Length of output: 3907
🏁 Script executed:
# Check the complete Ruff configuration to see if S608 is actually enforced
cat ruff.tomlRepository: BicameralAI/bicameral-mcp
Length of output: 1184
🏁 Script executed:
cat -n tests/test_team_event_replay.py | sed -n '188,196p'Repository: BicameralAI/bicameral-mcp
Length of output: 550
🏁 Script executed:
# Search for query helper functions or methods
rg -n 'def.*query|parameterized.*query' --type py
# Check if there are any existing noqa: S608 patterns in test files
rg -n 'noqa.*S608' --type pyRepository: BicameralAI/bicameral-mcp
Length of output: 614
🏁 Script executed:
# Check the ledger client query method signature and usage
cat -n ledger/client.py | sed -n '85,110p'
# Check if query() method supports parameterization/prepared statements
rg -A 5 'async def query' ledger/client.pyRepository: BicameralAI/bicameral-mcp
Length of output: 1796
Resolve S608 violations by using parameterized queries instead of f-string interpolation
Lines 96, 108, 143, 154, 163, and 192 use f-string SQL construction, which triggers Ruff S608 (SQL injection risk) and will block CI. The query() method in ledger/client.py supports parameterized queries via the vars parameter—use that instead of string interpolation. For example:
Example refactor
# Before:
rows = await inner_a._client.query(
f"SELECT signoff FROM {decision_id_a} LIMIT 1"
)
# After:
rows = await inner_a._client.query(
"SELECT signoff FROM $id LIMIT 1",
vars={"id": decision_id_a}
)Alternatively, add # noqa: S608 with a comment explaining why string interpolation is safe in this test context (if you prefer keeping the current form).
🧰 Tools
🪛 Ruff (0.15.12)
[error] 96-96: Possible SQL injection vector through string-based query construction
(S608)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_team_event_replay.py` around lines 95 - 97, Replace f-string SQL
interpolation in tests/test_team_event_replay.py (calls to inner_a._client.query
and any similar uses like the queries referencing decision_id_a/decision_id_b)
with parameterized queries using the query(...) vars parameter (e.g., "SELECT
signoff FROM $id LIMIT 1", vars={"id": decision_id_a}) so the ledger client’s
parameter binding is used; alternatively, if you intentionally deem the
interpolation safe for this test, add a contextual "# noqa: S608" comment next
to the offending inner_a._client.query call(s) explaining why SQL injection is
not possible in this test, but prefer the vars-based refactor to satisfy Ruff
S608.
4 tasks
…hook
Phase 0a — Decompose server.py:cli_main (92 LOC → 15 LOC orchestrator
+ _register_subparsers (16 LOC) + _dispatch (29 LOC)). Razor-compliant.
Phase 0 — Promote cli/branch_scan.py:_invoke_link_commit to shared
cli/_link_commit_runner.py module. Pure refactor under existing
test_branch_scan_cli.py coverage.
Phase 1 — Register link_commit CLI subcommand:
- cli/link_commit_cli.py (29 LOC) — JSON-to-stdout default, --quiet
flag, always exits 0 (graceful skip on no-ledger or handler error).
- server.py — subparser registration in _register_subparsers + dispatch
branch in _dispatch.
- tests/test_link_commit_cli.py (6 tests) — argparse defaults, output
shape, --quiet, no-ledger graceful skip, handler-exception graceful
skip.
Phase 2 — Harden post-commit hook:
- setup_wizard.py:_GIT_POST_COMMIT_HOOK now writes stderr to
${HOME}/.bicameral/hook-errors.log (was /dev/null), surfaces a
one-line summary on stderr, always exits 0. > truncates the file
on each run so successful commits auto-clear stale errors. F-2
remediation per audit v2.
- tests/test_hook_command_registration.py (3 tests) — smoke that
walks every bicameral-mcp <cmd> in installed hooks and asserts
CLI registration + dispatch coverage. Original #124 bug class is
now caught at PR time.
Phase 3 — CHANGELOG [Unreleased] Fixed entry.
Validation: 20 passed, 1 skipped (Windows chmod). ruff check + format
+ mypy clean. Manual smoke: link_commit --help renders.
Plan v2 PASS at META_LEDGER #21 (chain 86225d49). Implementation
sealed at META_LEDGER #22 (chain e83d674c).
Closes #124.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
(cherry picked from commit 431e202)
Adaptation: server.py — kept dev's _register_subparsers / _dispatch helper extraction (#124 phase 0a refactor) so test_hook_command_registration.py introspection works; omitted dev's branch-scan subparser registration and the setup --with-push-hook flag (both are #48 prerequisites missing on triage)
Adaptation: server.py:_dispatch — kept dev's setup → branch-scan → link_commit dispatch chain shape; dropped branch-scan dispatch case (cli/branch_scan.py is a missing prerequisite from #48 on triage); kept link_commit dispatch case (the actual #124 fix)
Adaptation: tests/test_hook_command_registration.py — dropped _GIT_PRE_PUSH_HOOK import + the test_pre_push_hook_command_is_registered test (pre-push hook is from #48, not on triage); test_all_hook_commands_have_dispatch_branches scoped to _GIT_POST_COMMIT_HOOK only; test_post_commit_hook_command_is_registered (the canonical #124 regression test) is preserved
Skip: cli/branch_scan.py — kept triage's prior absence of this file (added by #48); the cherry-pick wanted to refactor it
Skip: docs/META_LEDGER.md — kept triage's HEAD chain state; e6d4b8f's META_LEDGER #21/#23 entries are dev's chain, not triage's
Skip: CHANGELOG.md — kept triage's HEAD; v0.X.Y triage release narrative goes in PR #128 per DEV_CYCLE §10.5.4
jinhongkuan
changed the title
jinhongkuan
added
the
flow:release
9 tasks
jinhongkuan
pushed a commit
that referenced
this pull request
Apr 30, 2026
Triage release per DEV_CYCLE §10.5. Restores Guided-mode post-commit hook behavior (#124) and ships event vocabulary extension for cross-author replay (#97), alongside earlier carry-forward fixes (#74 Windows ingest, #95 telemetry counters + first-boot consent, #98 RFC docs). Full triage provenance and §10.5.3 adaptation notes in PR #128. CHANGELOG headline reworked: replaces the cherry-picked v0.14.0 dev-side heading with a v0.13.5 triage heading covering all 5 commits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
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
2 participants
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.
Release v0.13.5 (triage)
North star
This release advances the v0 Architecture (Notion). Triage releases (per DEV_CYCLE.md §10.5) ship the subset of `dev` work that's user-facing, isolated, and §10.5.1-eligible — letting fast iteration on `dev` coexist with deliberate user-visible delivery on `main`.
Headline
Restores Guided-mode post-commit hook behavior (#124) and ships event vocabulary extension for cross-author replay (#97), alongside earlier carry-forward fixes (#74 Windows ingest, #95 telemetry counters, #98 RFC docs).
Cherry-picked commits
Per DEV_CYCLE §10.5.4, every triage release PR enumerates each cherry-picked commit with provenance:
The two §10.5-era commits (`5f60eed`, pending-`#134`) carry full `(cherry picked from commit ...)` provenance and `Adaptation:` trailers in their commit messages. The three pre-§10.5 commits are sunk-cost divergence per §10.5.3 ("Do not rewrite history on `triage-from-dev` to fix the divergence — it is a published branch") — going forward every cherry-pick re-converges the audit trail.
Per-issue narrative
bicameral-mcp link_commit HEADis not a registered subcommand #124 (post-commit hook silent no-op) — the hook installed by `bicameral-mcp setup` (Guided mode) called `bicameral-mcp link_commit HEAD`, but `link_commit` was never registered as a CLI subcommand. Every commit since the hook was added silently failed. This release registers the subcommand and hardens the hook to write stderr to `${HOME}/.bicameral/hook-errors.log` and surface a one-line summary. Architectural caveat: `link_commit` syncs ledger state but does NOT auto-resolve drift. Pending-compliance indicators still need `resolve_compliance` to clear them (caller-driven, by design — selection over generation per CLAUDE.md). Active Claude Code sessions get tool-call auto-sync independently; this fix matters most for out-of-session committers.import fcntlbreaks Windows (blocks ALL ingest-using tests) #74 (Windows ingest crash) — `events/writer.py` top-level `import fcntl` blocked all ingest-using tests on Windows. Cross-platform shim (POSIX fcntl + Windows msvcrt) lands here.Schema
No schema migrations. Schema-migrating dev work (decision_level classifier #77/v0.16.0, governance contracts v0.17.0, preflight telemetry #65) is intentionally held out — not triage-eligible per §10.5.1 without migration coordination on main.
Breaking changes
None in the API/data sense. The post-commit hook in #124 newly surfaces failures on stderr (was silent); observable behavior change for users who had silent failures, but no API/data change. Existing installs that had been silently failing will start surfacing errors — interpret these as the system telling you something was already broken, not a new regression.
Documentation
Test plan
Pre-existing carry-overs (not new regressions)
Two preflight test files fail at collection time on triage at HEAD due to missing dev-only symbols:
Both predate this release. Carrying forward — they're a separate issue tied to triage's pre-§10.5 divergence, not introduced by anything in this PR.
Sequencing
feat(#124): backport CLI link_commit registration to triage (cherry-pick of 431e202 with §10.5.3 adaptations) #134 (`triage/124-link-commit-cli → triage-from-dev`) merges first → adds `feat(post-commit hook silently no-ops —— ✅ landed as `bb76ad5`bicameral-mcp link_commit HEADis not a registered subcommand #124)` SHA to `triage-from-dev`References
🤖 Generated with Claude Code