fix: address independent review findings

CHANGELOG.md

@@ -1,5 +1,14 @@
 # Changelog
 
+## Unreleased
+
+- Bound JSON nesting before parsing and return a JSON-RPC parse error instead
+  of dropping the request connection.
+- Bound request-body reads and return HTTP 408 for incomplete slow requests.
+- Enforce the advertised object-item schema for result evidence and review
+  findings.
+- Correct the documented post-`CHANGES_REQUESTED` remediation path.
+
 All notable changes to this project are documented here.
 
 ## 0.1.0 — 2026-07-30
@@ -14,4 +23,3 @@ Initial review candidate:
 - transport authentication abstraction and BASDM separation controls;
 - tests, container example, contract, threat model, production profile, and
   WP-PA-001 review evidence.
-

docs/ARCHITECTURE.md

@@ -49,8 +49,9 @@ Work packages move through:
 
 `HOLD` leaves a reviewed package in `REVIEWED`. A new review can be requested
 after a completed review. This MVP treats corrections after
-`CHANGES_REQUESTED` as a new work package or a new explicitly governed
-iteration; it does not silently overwrite submitted result evidence.
+`CHANGES_REQUESTED` as a new work package. It does not support replacing the
+result on the reviewed work package and never silently overwrites submitted
+result evidence.
 
 ## Trust boundaries
 
@@ -58,4 +59,3 @@ iteration; it does not silently overwrite submitted result evidence.
 - The database is trusted to enforce integrity and append-only triggers.
 - Artifact URIs and commit hashes are references, not trusted content.
 - Reverse proxy identity headers are not consumed by the built-in adapter.
-

docs/PRODUCTION_PROFILE.md

@@ -13,7 +13,9 @@ hardening work package.
 2. Replace `StaticTokenAuthProvider` with validated OIDC/JWT or mTLS workload
    identity. Bind issuer, audience, subject, expiry, and revocation policy.
 3. Put the service behind TLS and authenticated ingress. Do not trust arbitrary
-   forwarded identity headers.
+   forwarded identity headers. The ingress must enforce connection limits,
+   header/body read deadlines, and request quotas in addition to the
+   application's five-second request-body timeout.
 4. Run multiple stateless instances only after PostgreSQL conformance and
    concurrency tests pass.
 5. Add structured audit export, metrics, tracing correlation, alerts, backup,
@@ -33,4 +35,3 @@ production gate acceptance.
 Chat threads are not push subscribers. A client, automation, or PA invocation
 must call `sync_since(last_cursor)`. The cursor itself belongs in durable client
 state and is advanced only after successful processing.
-

docs/THREAT_MODEL.md

@@ -37,6 +37,8 @@
 | Cross-project reads | project filter plus membership resolution |
 | Browser cross-origin request | exact configurable Origin allowlist |
 | Oversized request | one MiB body limit |
+| Excessive JSON nesting | pre-parse nesting limit plus guarded decoder |
+| Slow/incomplete body | five-second application read timeout |
 | MIME confusion/caching | strict JSON input, nosniff, no-store |
 | SQL injection | parameterized SQL; one controlled placeholder expansion |
 
@@ -57,8 +59,10 @@ fetched or executed by the bus.
 4. The event payload is not cryptographically chained or signed. Add hash
    chaining/signatures only if the agreed threat model requires tamper
    evidence against privileged database operators.
-5. No application rate limiter exists. Apply per-identity limits and request
-   quotas at the trusted edge.
+5. No application rate limiter or bounded worker pool exists. The application
+   closes incomplete request bodies after five seconds, but production ingress
+   must additionally enforce connection limits, header/body read deadlines,
+   per-identity limits, and request quotas.
 6. The server emits access metadata but no security audit sink or metrics.
    Integrate structured logs, alerts, and privacy-aware retention.
 7. Membership deactivation and credential revocation are not exposed as MVP
@@ -80,4 +84,3 @@ fetched or executed by the bus.
 - review log contents for sensitive project data;
 - add abuse limits and operational monitoring;
 - decide whether privileged-operator tamper evidence is required.
-

evidence/WP-PA-001-REWORK-EVIDENCE.md

@@ -0,0 +1,44 @@
+# WP-PA-001 Rework Evidence
+
+Date: 2026-07-30
+
+## Review input
+
+- Independent reviewer: Claude Code
+- Reviewed commit: `bc55924198865502203a992b6ea914a38fd9d173`
+- Verdict: `CHANGES_REQUESTED`
+- Findings addressed: M-1, M-2, L-1, L-2
+- Optional observation O-1: no correction required
+
+## Corrections
+
+| Finding | Correction | Regression evidence |
+|---|---|---|
+| M-1 | Added a pre-parse JSON nesting limit and retained `RecursionError` handling. Excessively nested JSON returns JSON-RPC `-32700`. Brackets inside strings are ignored by the nesting scanner. | `test_deeply_nested_json_returns_parse_error`; `test_brackets_inside_json_string_do_not_count_as_nesting` |
+| M-2 | Added a five-second connection/request-body read timeout. Incomplete bodies receive HTTP 408 and the connection is closed. Production ingress requirements now explicitly include connection limits and read deadlines. | `test_incomplete_body_times_out` |
+| L-1 | Corrected the architecture document: post-`CHANGES_REQUESTED` corrections require a new work package in this MVP. | Documentation inspection |
+| L-2 | Enforced object items for `evidence` and `findings`, matching the advertised MCP schemas. | `test_evidence_and_findings_items_must_be_objects` |
+
+## Verification
+
+Runtime: Python 3.12.13
+
+```text
+PYTHONPATH=src python3.12 -m unittest discover -v
+Ran 20 tests in 5.696s
+OK
+
+PYTHONPATH=src python3.12 -m compileall -q src tests
+exit 0
+
+pip install --no-cache-dir --no-deps --target <temporary-directory> .
+PYTHONPATH=<temporary-directory> PROJECT_BUS_MIGRATIONS=<repo>/migrations \
+  python3.12 -m unittest discover -v tests
+Ran 20 tests in 5.394s
+OK
+
+git diff --check
+exit 0
+```
+
+The rework must receive a fresh independent review before PM gate acceptance.

src/project_bus/server.py

@@ -14,6 +14,8 @@ from .mcp import MCPApplication
 
 LOGGER = logging.getLogger("project_bus.http")
 MAX_BODY_BYTES = 1_048_576
+MAX_JSON_NESTING_DEPTH = 128
+DEFAULT_REQUEST_READ_TIMEOUT_SECONDS = 5.0
 
 
 class ProjectBusHTTPServer(ThreadingHTTPServer):
@@ -25,17 +27,25 @@ class ProjectBusHTTPServer(ThreadingHTTPServer):
         application: MCPApplication,
         auth_provider: AuthProvider,
         allowed_origins: set[str],
+        request_read_timeout_seconds: float = DEFAULT_REQUEST_READ_TIMEOUT_SECONDS,
     ):
+        if request_read_timeout_seconds <= 0:
+            raise ValueError("request_read_timeout_seconds must be positive")
         super().__init__(server_address, ProjectBusRequestHandler)
         self.application = application
         self.auth_provider = auth_provider
         self.allowed_origins = allowed_origins
+        self.request_read_timeout_seconds = request_read_timeout_seconds
 
 
 class ProjectBusRequestHandler(BaseHTTPRequestHandler):
     server: ProjectBusHTTPServer
     protocol_version = "HTTP/1.1"
 
+    def setup(self) -> None:
+        super().setup()
+        self.connection.settimeout(self.server.request_read_timeout_seconds)
+
     def log_message(self, format_string: str, *args: Any) -> None:
         LOGGER.info("%s - %s", self.address_string(), format_string % args)
 
@@ -73,13 +83,20 @@ class ProjectBusRequestHandler(BaseHTTPRequestHandler):
             self._json(HTTPStatus.REQUEST_ENTITY_TOO_LARGE, {"error": "invalid_body_size"})
             return
         try:
-            request = json.loads(self.rfile.read(length))
-        except (json.JSONDecodeError, UnicodeDecodeError):
+            body = self.rfile.read(length)
+            if _exceeds_json_nesting_limit(body):
+                raise RecursionError("JSON nesting limit exceeded")
+            request = json.loads(body)
+        except (json.JSONDecodeError, UnicodeDecodeError, RecursionError):
             self._json(
                 HTTPStatus.OK,
                 {"jsonrpc": "2.0", "id": None, "error": {"code": -32700, "message": "Parse error"}},
             )
             return
+        except TimeoutError:
+            self.close_connection = True
+            self._json(HTTPStatus.REQUEST_TIMEOUT, {"error": "request_body_timeout"})
+            return
         if not isinstance(request, dict):
             self._json(
                 HTTPStatus.OK,
@@ -134,3 +151,28 @@ def allowed_origins_from_environment() -> set[str]:
     raw = os.environ.get("PROJECT_BUS_ALLOWED_ORIGINS", "")
     return {item.strip() for item in raw.split(",") if item.strip()}
 
+
+def _exceeds_json_nesting_limit(
+    body: bytes, limit: int = MAX_JSON_NESTING_DEPTH
+) -> bool:
+    depth = 0
+    in_string = False
+    escaped = False
+    for byte in body:
+        if in_string:
+            if escaped:
+                escaped = False
+            elif byte == 0x5C:  # backslash
+                escaped = True
+            elif byte == 0x22:  # double quote
+                in_string = False
+            continue
+        if byte == 0x22:
+            in_string = True
+        elif byte in (0x5B, 0x7B):  # [ {
+            depth += 1
+            if depth > limit:
+                return True
+        elif byte in (0x5D, 0x7D):  # ] }
+            depth -= 1
+    return False

src/project_bus/service.py

@@ -451,6 +451,8 @@ class ProjectBusService:
         summary = self._require_text("summary", summary)
         if not isinstance(evidence, list):
             raise ValidationError("evidence must be a list")
+        if any(not isinstance(item, dict) for item in evidence):
+            raise ValidationError("evidence items must be objects")
         request = {"work_package_id": work_package_id, "summary": summary, "evidence": evidence}
 
         def action(connection: sqlite3.Connection, context: ActorContext) -> dict[str, Any]:
@@ -600,6 +602,10 @@ class ProjectBusService:
         summary: str,
         idempotency_key: str,
     ) -> dict[str, Any]:
+        if not isinstance(findings, list):
+            raise ValidationError("findings must be a list")
+        if any(not isinstance(item, dict) for item in findings):
+            raise ValidationError("findings items must be objects")
         try:
             review_verdict = ReviewVerdict(verdict)
         except ValueError as error:

tests/test_mcp_http.py

@@ -1,6 +1,8 @@
 from __future__ import annotations
 
 import json
+import http.client
+import socket
 import tempfile
 import threading
 import unittest
@@ -24,7 +26,11 @@ class MCPHTTPTestCase(unittest.TestCase):
             {"bootstrap-secret-at-least-24": TokenIdentity("system", "system", "System")}
         )
         self.server = ProjectBusHTTPServer(
-            ("127.0.0.1", 0), MCPApplication(service), auth, {"https://chat.example"}
+            ("127.0.0.1", 0),
+            MCPApplication(service),
+            auth,
+            {"https://chat.example"},
+            request_read_timeout_seconds=0.2,
         )
         self.thread = threading.Thread(target=self.server.serve_forever, daemon=True)
         self.thread.start()
@@ -115,6 +121,60 @@ class MCPHTTPTestCase(unittest.TestCase):
         self.assertEqual(status, 403)
         self.assertEqual(response["error"], "origin_not_allowed")
 
+    def test_deeply_nested_json_returns_parse_error(self) -> None:
+        body = b"[" * 200_000 + b"]" * 200_000
+        connection = http.client.HTTPConnection(
+            "127.0.0.1", self.server.server_port, timeout=2
+        )
+        connection.request(
+            "POST",
+            "/mcp",
+            body=body,
+            headers={"Content-Type": "application/json"},
+        )
+        response = connection.getresponse()
+        payload = json.loads(response.read())
+        connection.close()
+
+        self.assertEqual(response.status, 200)
+        self.assertEqual(payload["error"]["code"], -32700)
+
+    def test_brackets_inside_json_string_do_not_count_as_nesting(self) -> None:
+        status, response = self.request(
+            {
+                "jsonrpc": "2.0",
+                "id": 1,
+                "method": "initialize",
+                "params": {"text": "[{" * 500 + "}]" * 500},
+            }
+        )
+        self.assertEqual(status, 200)
+        self.assertIn("result", response)
+
+    def test_incomplete_body_times_out(self) -> None:
+        connection = socket.create_connection(
+            ("127.0.0.1", self.server.server_port), timeout=2
+        )
+        connection.settimeout(2)
+        connection.sendall(
+            b"POST /mcp HTTP/1.1\r\n"
+            b"Host: 127.0.0.1\r\n"
+            b"Content-Type: application/json\r\n"
+            b"Content-Length: 100\r\n"
+            b"Connection: close\r\n\r\n"
+            b'{"jsonrpc":'
+        )
+        response = b""
+        while True:
+            chunk = connection.recv(4_096)
+            if not chunk:
+                break
+            response += chunk
+        connection.close()
+
+        self.assertIn(b"HTTP/1.1 408 Request Timeout", response)
+        self.assertIn(b'"error":"request_body_timeout"', response)
+
 
 if __name__ == "__main__":
     unittest.main()

tests/test_service.py

@@ -256,6 +256,34 @@ class ServiceTestCase(unittest.TestCase):
                 idempotency_key="premature-gate",
             )
 
+    def test_evidence_and_findings_items_must_be_objects(self) -> None:
+        self.issue()
+        self.service.claim_work_package(
+            self.principals["lead"],
+            project_id="sandbox",
+            work_package_id="WP-001",
+            idempotency_key="claim",
+        )
+        with self.assertRaises(ValidationError):
+            self.service.submit_result(
+                self.principals["lead"],
+                project_id="sandbox",
+                work_package_id="WP-001",
+                summary="Invalid evidence.",
+                evidence=["not-an-object"],  # type: ignore[list-item]
+                idempotency_key="invalid-evidence",
+            )
+        with self.assertRaises(ValidationError):
+            self.service.submit_review(
+                self.principals["reviewer"],
+                project_id="sandbox",
+                review_id="rev-does-not-matter",
+                verdict="APPROVE",
+                findings=[42],  # type: ignore[list-item]
+                summary="Invalid findings.",
+                idempotency_key="invalid-findings",
+            )
+
     def test_idempotent_replay_and_key_mismatch(self) -> None:
         first = self.service.issue_work_package(
             self.principals["pm"],