fix(ingest): bundle devil's-advocate followups #230 + #232 + #233#238
Merged
Conversation
…233) Plan for the three devil's-advocate followups from PR #229's review pass. All three touch handlers/ingest.py middleware + adjacent surfaces; bundling into one PR keeps the test-isolation hoist (#233) atomic with the boundary-detail scrub (#230) and the truthy+malformed_payload widenings (#232). Audit: round 1 PASS (L1, mechanical follow-ups; positive A04 contribution from the malformed_payload translation closing a fail-open path). Substrate observations folded into implementation per audit guidance: - Use private cross-module import `from context import _GUIDED_MODE_TRUTHY` with explicit comment (avoid widening scope to public alias). - Hoist BOTH `_reset_rate_limit_registry` AND `_ensure_rate_limit_enabled` to conftest.py (the second autouse fixture has identical future-flake risk). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lose malformed_payload fail-open + hoist fixtures (#230 #232 #233) #230 Finding 1 — scrub session_id from `_IngestRefused('rate_limit_exceeded').detail`: - handlers/ingest.py:_check_rate_limit drops `session {uuid}` segment from the refusal-detail; emits `bucket empty (burst=N, refill=R/s)` only. - The raw session UUID is process-fingerprinting state that surrounding telemetry writers hash via per-install salt; emitting it raw at the MCP boundary (which the agent then relays into operator-visible context) is inconsistent with that posture. #232 Finding 1 — widen truthy vocabulary on rate-limit env-var disable: - handlers/ingest.py imports `_GUIDED_MODE_TRUTHY` from context (private cross-module use; explicit comment in source; the alternative — public alias or hoist — would widen scope unnecessarily). - `BICAMERAL_INGEST_RATE_LIMIT_DISABLE` now accepts 1/true/yes/on case-insensitively, matching the canonical truthy frozenset. #232 Finding 2 — translate non-JSON-serializable payloads to `malformed_payload`: - handlers/ingest.py:_check_payload_size wraps `json.dumps(...)` in try/except `(ValueError, TypeError, RecursionError)`, raises `_IngestRefused('malformed_payload', ...)` with the underlying exception type in detail. Closes a pre-existing OWASP A04 fail-open path: a circular-ref or non-serializable payload would previously leak ValueError past the gate to the MCP boundary's generic exception path. - server.py:_ACTION_FOR_REFUSAL_REASON adds operator-actionable guidance for `malformed_payload`. #233 — hoist autouse fixtures to conftest.py: - `_reset_rate_limit_registry` and `_ensure_rate_limit_enabled` (companion fixture flagged by audit substrate observation 2) move from tests/test_ingest_rate_limit.py to tests/conftest.py. Eliminates implicit-fixture-dependency flake risk for any future ingest-touching test in any file. Tests: - 7 new functional tests across the two findings (parametrized truthy variants, circular-ref, RecursionError, TypeError, scrubbed-detail shape lock). - Existing assertions updated to verify scrubbed detail (no UUID leak). - 113/113 ingest+server tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ing 2) The token-bucket gate slows BURST consumption per session but does not bound aggregate throughput. Default config admits ~70 ingests/60s and 1 MiB/s sustained — ~86 GiB/day worst case under a runaway agent (model regression, prompt-injection-hijacked re-ingest cycle, dev-time infinite-loop bug). Not a security crisis (size cap bounds per-payload damage; in-process registry resets on server restart) but an operator-side disaster for ledger-writer churn + disk pressure. Stricter aggregate enforcement (sliding-window cross-session bound) is deferred to the team-server-activation track which has cross-developer correlation needs that single-session token-bucket cannot satisfy. Documented in: - docs/research-brief-compliance-audit-2026-05-06.md § 2.4 LLM-08 walk - plan-216-ingest-size-and-rate-limit.md boundaries.limitations - handlers/ingest.py module-level docstring (ships with code via prior commit) No code change in this commit. Closes #230 Finding 2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Knapp-Kevin
had a problem deploying
to
recording-approval
GitHub Actions
Failure
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
4 tasks
Merged
3 tasks
Knapp-Kevin
added a commit
to Knapp-Kevin/bicameral-mcp
that referenced
this pull request
May 21, 2026
…icameralAI#225 + BicameralAI#226) Plan for the three compliance-posture stance declarations: - BicameralAI#220 / MCP-01: MCP host UX dependency (OWASP LLM-07) - BicameralAI#225 / NIST-RMF-01 + AI-ACT-02: prohibited-uses declaration - BicameralAI#226 / SOC2-02: availability stance (operator-run-only) All three bundle naturally because they share docs/policies/ + a single README cross-reference section. Pure-doc surface fully disjoint from in-flight code PRs (BicameralAI#237, BicameralAI#238, BicameralAI#239) — safe as a parallel PR. Audit: round 1 PASS (L1, doc-only). Doctrine interpretation locked: for markdown policy artifacts, the unit IS the document content; read_text() + assert "<commitment>" in content is genuine unit invocation per qor/references/doctrine-test-functionality.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 27, 2026
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
1 participant
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.
Summary
Bundle of three devil's-advocate followups from PR #229's review pass. All touch
handlers/ingest.pymiddleware + adjacent surfaces. Closes:_IngestRefused('rate_limit_exceeded').detailat the MCP boundary (Finding 1) + document aggregate-rate worst case (Finding 2)BICAMERAL_INGEST_RATE_LIMIT_DISABLE(Finding 1) + close circular-ref fail-open path viamalformed_payloadtranslation (Finding 2)_reset_rate_limit_registryautouse fixture totests/conftest.py(plus_ensure_rate_limit_enabledcompanion per audit substrate)Plan / Audit
plan-A-ingest-followups-230-232-233.mdmalformed_payloadtranslation closing a pre-existing fail-open pathWhat ships
handlers/ingest.py:_check_rate_limitsession {uuid}from_IngestRefused.detail; emitbucket empty (burst=N, refill=R/s)onlyhandlers/ingest.py:_check_rate_limit== "1"with_GUIDED_MODE_TRUTHYmembership (1/true/yes/on case-insensitive)handlers/ingest.py:_check_payload_sizejson.dumpsintry/except (ValueError, TypeError, RecursionError)→_IngestRefused("malformed_payload", ...)server.py:_ACTION_FOR_REFUSAL_REASON"malformed_payload"→ operator-actionable guidancetests/conftest.py_reset_rate_limit_registryand_ensure_rate_limit_enabledautouse fixtureshandlers/ingest.pymodule docstring +docs/research-brief-compliance-audit-2026-05-06.md§ 2.4 +plan-216-ingest-size-and-rate-limit.mdboundariesTest plan
tests/test_ingest_rate_limit.py— assertions updated to verify scrubbed detail (no UUID leak); 1 new parametrized testtest_check_rate_limit_disabled_via_truthy_variantscovering 1/true/yes/on (case-insensitive)tests/test_ingest_size_limit.py— 3 new tests covering circular-ref, RecursionError, TypeError →malformed_payloadtranslationruff check+ruff format --checkcleanOWASP A04 positive contribution
The
_check_payload_sizetry/except CLOSES a pre-existing fail-open path: a circular-reference or non-serializable payload would previously leakValueErrorpast the gate to the MCP boundary's generic exception handler. After this PR, all such payloads translate to_IngestRefused("malformed_payload")at the same MCP boundary as the other refusals — consistent with the existing error envelope.Scope discipline
_GUIDED_MODE_TRUTHYprivate import is documented inline)_normalize_payloadoverage (76 LOC) andhandlers/ingest.pyfile size (695 LOC) are unaffected; explicitly out of scope per plan boundaries🤖 Generated with Claude Code