feat(security): land Engram 10/10 wave 4 foundations - #192
Conversation
Record integration HEAD 2cfab45, Todo 24 human acceptance scope, successful release dry-run 29286930522 with publication jobs skipped, SQLite deferral, and the Todo 39 HTTP body/timeout env vars in the advertised-surface inventory.
Update the PDF worker packaging checker and unit suite for the Wave 4 release.yml shape: locked multi-binary build, deterministic artifact scripts with Linux-only worker packaging, and Homebrew formula updater.
Keep release dry-run 29286930522 bound to 2cfab45, record the PDF packaging contract alignment, and advance live-state Last commit with integration HEAD.
|
Connected to Huly®: ENGRA-228 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Engram Performance
Details
| Benchmark suite | Current: 667ab03 | Previous: 962655a | Ratio |
|---|---|---|---|
entity_extractor_new/default |
3538 ns/iter (± 95) |
2322 ns/iter (± 7) |
1.52 |
entity_extraction/extract_mixed |
379866 ns/iter (± 8869) |
249155 ns/iter (± 482) |
1.52 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Engram Performance'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.15.
| Benchmark suite | Current: 667ab03 | Previous: 962655a | Ratio |
|---|---|---|---|
entity_extractor_new/default |
3538 ns/iter (± 95) |
2322 ns/iter (± 7) |
1.52 |
entity_extraction/extract_mixed |
379866 ns/iter (± 8869) |
249155 ns/iter (± 482) |
1.52 |
This comment was automatically generated by workflow using github-action-benchmark.
limaronaldo
left a comment
limaronaldo
left a comment
There was a problem hiding this comment.
Maintainability review — changes requested (posted as COMMENT: GitHub forbids REQUEST_CHANGES on self-authored PRs)
Author action still required before merge. Treat this as a formal request-changes review.
Behavior/security direction is largely right (WS isolation, HTTP timeout/auth ordering, dry-run publish guards, quality budgets). This review is not a functional rubber-stamp; it is a structural quality bar.
P0 blockers
src/realtime/server.rs713 → 1325 — crosses the 1k-line threshold hard. Origin policy, metrics/guards, socket lifecycle, and ~636 lines of tests all landed in one module.sdks/python/engram_client/client.py996 → 1055 — MCP decode path pushes the client god-file over 1k for a transport-boundary fix that should live in a small helper module..with_workspace(...)scatter — isolation is a model invariant, but enforcement is “remember to chain a builder at every emit site.” One missed call is a silent cross-workspace leak.
P1
- HTTP resource env parse soft-defaults; WS resource env parse fail-closes. Pick one policy (prefer fail-closed / shared helper).
enforce_mcp_auth/enforce_sse_authare copy-paste twins that will drift.- Release/live verify scripts and SDK live drivers multiply near-clones without a shared lib — high leverage follow-up after P0.
Minimum before approve
- Decompose
server.rs(origin / metrics / socket / tests out). - Make workspace required on memory realtime events or a single
broadcast_memory_*API. - Extract MCP decode from
client.pyunder 1k lines. - Align HTTP env parse with WS fail-closed (or one shared helper).
I will re-review after the judo split. I will not approve “it works” growth of the realtime god-file.
| /// Aggregate resource counters. They intentionally contain no principal, | ||
| /// credential, workspace, or message data. | ||
| #[derive(Default)] | ||
| struct RealtimeMetrics { |
There was a problem hiding this comment.
P0 — file size / god-module growth. server.rs went 713 → 1325 lines. Metrics + drop guards belong in something like src/realtime/metrics.rs, not more surface on the already-busy transport module.
This is a presumptive blocker: please decompose before merge so product code in server.rs is clearly under ~1k (ideally ~400–500), with tests extracted.
| allowed_origins.contains(origin) | ||
| } | ||
|
|
||
| fn parse_origin_allowlist(raw: Option<&str>) -> Result<HashSet<String>, String> { |
There was a problem hiding this comment.
Code-judo: extract origin policy. parse_origin_allowlist + origin_is_allowed are a self-contained policy surface (scheme/authority, no wildcards, multi-Origin reject). They do not need to live next to socket select loops.
Suggested split:
src/realtime/origin.rs # parse + allow checks
src/realtime/auth.rs # principal_can_subscribe / websocket_principal
src/realtime/socket.rs # handle_socket + write/close timeouts
src/realtime/server.rs # router + RealtimeServer only
config.rs already shows the right pattern — follow it.
|
|
||
| /// Handle an individual WebSocket connection | ||
| async fn handle_socket(socket: WebSocket, manager: RealtimeManager, principal: TransportPrincipal) { | ||
| async fn handle_socket( |
There was a problem hiding this comment.
Parameter explosion / lifecycle ownership. handle_socket now takes manager + principal + workspace + config + metrics + permit. That is a signal the connection lifecycle wants its own type (ConnectionSession { permit, metrics_guard, registration_guard, ... }) in a dedicated module rather than a longer free function in the god file.
Keep the Drop-guard idea — it is the right ownership model — but move it out of server.rs.
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
~636 lines of mod tests in-tree roughly double the product surface of this file. Move to src/realtime/server_tests.rs (or integration tests under tests/) as part of the decomposition that brings this file back under 1k.
Leaving the suite here is how the 1k threshold was crossed even if the product logic alone might still fit.
| } | ||
|
|
||
| /// Attach the authoritative workspace used by realtime transports. | ||
| pub fn with_workspace(mut self, workspace: impl Into<String>) -> Self { |
There was a problem hiding this comment.
P0 — wrong abstraction for a security invariant. Optional .with_workspace(...) means isolation is a call-site discipline, not a type invariant. One missed chain on a future emit site = silent cross-workspace delivery.
Code-judo (prefer one):
- Require workspace on constructors:
memory_created(id, preview, workspace). - Or
RealtimeManager::broadcast_memory_created(&Memory)that always copiesmemory.workspace. - Or make
workspacenon-optional on every memory event variant.
Please delete the “remember to call the builder” class of bugs.
| )); | ||
| manager.broadcast( | ||
| RealtimeEvent::memory_created(memory.id, memory.content.clone()) | ||
| .with_workspace(memory.workspace.clone()), |
There was a problem hiding this comment.
Spaghetti growth at emit sites. Same .with_workspace(memory.workspace.clone()) pattern is copy-pasted across create / facts / update / delete. That is special-case branching bolted onto every handler instead of pushing the invariant into the event/broadcast API.
If the event constructor requires workspace (or a single broadcast helper owns it), these chains disappear entirely — including the delete path’s (ids, workspace) plumbing.
| parse_positive_value(name, &value, default, zero) | ||
| } | ||
|
|
||
| fn parse_positive_value<T>(name: &str, value: &str, default: T, zero: T) -> T |
There was a problem hiding this comment.
P1 — inconsistent fail modes. Invalid/zero ENGRAM_HTTP_MAX_BODY_BYTES / ENGRAM_HTTP_REQUEST_TIMEOUT_MS warn and soft-default. WS resource env parse in RealtimeConfig::from_env fail-closes and refuses to listen.
Two security-resource models in one product is a maintainability and ops hazard. Prefer one shared parse policy — ideally fail-closed like WS — rather than silent defaults that paper over misconfig.
| .with_state(state) | ||
| } | ||
|
|
||
| async fn enforce_mcp_auth( |
There was a problem hiding this comment.
Duplicate auth middleware. enforce_mcp_auth and enforce_sse_auth are the same principal check with different unauthorized response shapes. They will drift.
Collapse to one middleware parameterized by failure response (or a tiny helper that returns the unauthorized Response), and keep the timeout middleware separate. Layering order itself is good — do not lose that.
| return self._decode_tool_result(result.get("result", {})) | ||
|
|
||
| @staticmethod | ||
| def _decode_tool_result(result: Any) -> Any: |
There was a problem hiding this comment.
P0 — file crossed 1k (996 → 1055). MCP CallToolResult decoding + error unwrapping is real product logic and does not need to live on the god-client.
Extract to e.g. sdks/python/engram_client/mcp_result.py (decode_tool_result, error_message) and keep EngramClient._call thin. The decode behavior is fine; the layering is not.
Decompose realtime transport into origin/metrics/auth/socket modules and extract tests so server.rs is under 500 lines. Require workspace on memory RealtimeEvent constructors to remove the .with_workspace footgun. Extract Python MCP decode/post helpers and EngramError, bring client.py under 1k. Make HTTP body/timeout env parsing fail closed like WebSocket limits, and collapse MCP/SSE auth middleware onto one shared principal gate.
Maintainability P0 remediation (follow-up to review)Addressed the structural review on
Local verification: realtime unit suite, HTTP security suite, security_config fail-closed tests, pre-commit clippy. CI noteRequired gates on this SHA were green (Test ubuntu, Security Gate, Clippy, Format, SDK live contracts, etc.). Happy to re-review / merge once CI is fully green (or if Benchmark remains noisy, we can treat it as non-blocking for this PR and keep the deterministic |
1 participant
Summary
Lands Engram 10/10 Wave 4 on
main: SDK live package contracts, retrieval/performance budget gates, reproducible release attestation, gated SDK publication workflows, and realtime/HTTP resource bounds.Todos integrated
Todo 24 human escalation
Two consecutive independent-review FAILs were remediated in
6a42f00. Human owner respondedautorizadowith scope limited to accepting/integrating that remediated SHA. Not publication approval for any channel.Release dry-run evidence
29271674475failed closed (unsupportedengram-server --versionsmoke).2cfab45aligned smoke with shipped CLIs.2cfab4563d5da43932c1cc3aa6741eeea6b487ea.Closeout on this PR
make cigreen).docs/harness/reviews/2026-07-13-engram-10-of-10-wave4-post.md→REVIEW_VERDICT: PASS2026-07-13T23:20:58Zpass.Explicit non-claims
Test plan
make cipass (no exclusions)REVIEW_VERDICT: PASS