M10: Mediated MCP, harness import, and container deploy path#15
Open
huronat wants to merge 11 commits into
Open
M10: Mediated MCP, harness import, and container deploy path#15huronat wants to merge 11 commits into
huronat wants to merge 11 commits into
Conversation
Expose forensic trace tools over MCP, add authenticated HTTP transport, role/scope policy, and redaction for uploads and remote responses so campaign agents can query synty without raw bucket access. Import harness/Devin NDJSON, honor CODEX_HOME/CLAUDE_CONFIG_DIR, and ship Dockerfile plus Helm for supervisor-driven track/mcp. Validation: 250 default and 250 mcp-http scenarios. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesCore data and access
Deployment and release
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP_HTTP
participant Dispatcher
participant MCP_Server
Client->>MCP_HTTP: POST /mcp with bearer token and protocol headers
MCP_HTTP->>MCP_HTTP: Validate origin, content type, rate, and body limits
MCP_HTTP->>Dispatcher: Queue semantic or analysis request
Dispatcher->>MCP_Server: Execute scoped MCP tool
MCP_Server-->>Dispatcher: Redacted MCP response
Dispatcher-->>MCP_HTTP: Response or timeout status
MCP_HTTP-->>Client: JSON-RPC HTTP response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.dockerignore (1)
1-8: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winExclude local credentials from the Docker build context.
.env,.aws/, and private-key files can still be sent to a remote Docker builder. Add explicit exclusions for common secret locations and key formats.Proposed fix
.git target tmp corpus .synty +.env +.env.* +.aws +*.pem +*.key +*.p12 +*.pfx docs/*.gif evals/runs*🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.dockerignore around lines 1 - 8, Update the Docker ignore configuration to exclude local credential sources by adding entries for .env files, the .aws/ directory, and private-key files such as *.pem and *.key. Preserve all existing exclusions.src/trace.rs (1)
1222-1267: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
resolve()leaks unscoped trace ids through ambiguity errors, bypassingReadScope.
resolve()matches candidates acrossjobs/turns/spans/session_events/eventswith no knowledge of the caller'sReadScope. When a short prefix is ambiguous, itbail!s with up to 8 matching ids drawn from the entire store.show_text/compare_text(lines 980-1050) callresolve(..)?beforeresolved_allowed(..)runs, so this error propagates straight to the caller — including MCP clients bound to a restrictedReadScope(e.g.synty_trace_show/synty_trace_compare). A scoped client can therefore enumerate ids (and thus the existence of sessions/turns/jobs) outside its granted campaigns/roles/repos simply by probing short prefixes, which the exact-match path (single candidate, still scope-checked afterward viaresolved_allowed) does not otherwise allow.Thread
scopeintoresolve()and filter candidates (reusingscope_allows) before buildingbases, so the ambiguity list — and the match count itself — never includes out-of-scope ids.🔒 Proposed fix sketch
-fn resolve(store: &TraceStore, query: &str) -> Result<Resolved> { +fn resolve(store: &TraceStore, query: &str, scope: Option<&crate::policy::ReadScope>) -> Result<Resolved> { if query.chars().count() < 4 { bail!("trace id prefixes must be at least 4 characters"); } let mut bases: HashMap<String, Resolved> = HashMap::new(); for (i, j) in store.jobs.iter().enumerate() { - if id_matches(&j.id, query) { + if id_matches(&j.id, query) && scope_allows(store, &j.session_id, &j.repo, &j.source, scope) { bases.insert(j.id.clone(), Resolved::Job(i)); } } // ... same guard added for turns/spans/session_events/eventsAnd update the two call sites (
show_text,compare_text) to passscopethrough toresolve.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/trace.rs` around lines 1222 - 1267, Update resolve to accept the caller’s ReadScope and apply scope_allows to every job, turn, span, session event, and event candidate before inserting it into bases, ensuring ambiguity counts and listed IDs contain only permitted records. Update show_text and compare_text to pass their scope into resolve while preserving the existing post-resolution resolved_allowed checks.src/mcp.rs (1)
254-283: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winScope
synty_topicsandsynty_recentto the fullReadScope
topics_textandrecent_textonly filter by repo, so campaign/role/source restrictions are ignored here. Either carry those fields ontoUnit/Sessionand filter them too, or disable these tools whenever the scope narrows beyond repos.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp.rs` around lines 254 - 283, The topics_text and recent_text handlers currently enforce only repository access, ignoring other ReadScope restrictions such as campaign, role, and source. Update these handlers to apply the complete ReadScope to returned topics and units, carrying required scope fields onto Unit/Session if necessary; otherwise reject or disable both tools whenever the scope is narrower than repository-only.
🧹 Nitpick comments (5)
src/ingest.rs (1)
301-318: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a scenario test asserting campaign/role/backend reach
Doc.meta.
docs_from_eventsnow stampscampaign_id,campaign_role, andbackendonto emitted docs fromSessionMaps. These meta fields become the index columns thatReadScopefilters on insearch.rs, so mis-propagation would silently weaken scope enforcement. No existingsession_docstest asserts these fields survive fromsession_startinto the produced docs.Consider extending a
session_docstest with asession_startcarryingrollup_dim/campaign_role/backendand asserting the resultingDoc.meta.As per coding guidelines: "Every behavioral change must include a scenario-style unit test based on user expectations, following the existing
#[cfg(test)]conventions."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ingest.rs` around lines 301 - 318, Extend an existing session_docs scenario test to include a session_start event with rollup_dim, campaign_role, and backend values, then assert the emitted Doc.meta contains the corresponding campaign_id, campaign_role, and backend fields. Keep the test within the existing #[cfg(test)] conventions and verify values propagate through docs_from_events and SessionMaps.Source: Coding guidelines
src/import.rs (1)
114-116: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist
crate::config::load()out of the NDJSON loop.Config::load()reads and parsessrc/config.rson every call, so the repo allowlist check does repeated file I/O for every imported row.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/import.rs` around lines 114 - 116, Move the crate::config::load() call out of the NDJSON row-processing loop and load the configuration once before iteration begins. Reuse the resulting capture_repos allowlist in the existing event repository check, preserving the current filtering behavior.src/main.rs (1)
271-376: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new CLI surface in the user-facing docs.
README.mdanddocs/design.mdstill need the expandedimport/mcp/initflags (--bind,--listen-public,--scope,--redaction,--campaign,--no-autostart, etc.);AGENTS.mdalready carries the general reconciliation rule.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.rs` around lines 271 - 376, Document the expanded Import, Init, and Mcp CLI surfaces in README.md and docs/design.md, including flags such as --bind, --listen-public, --scope, --redaction, --campaign, and --no-autostart. Keep the documentation aligned with the command definitions and existing AGENTS.md reconciliation guidance.Source: Coding guidelines
src/sync.rs (1)
309-328: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test covering redacted uploads.
redact_event_linesis a new, security-relevant behavior (uploads now redact secrets/PII by default), but no test in this diff exercises it — e.g. asserting aBearer …token in a payload doesn't survivepush_events_scoped/redact_event_lineswithProfile::Standard, and thatProfile::Offround-trips raw bytes unchanged.
As per coding guidelines, "Every behavioral change must include a scenario-style unit test based on user expectations, following the existing#[cfg(test)]conventions."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sync.rs` around lines 309 - 328, Add a scenario-style unit test under the existing #[cfg(test)] conventions for redact_event_lines, using Profile::Standard to verify a Bearer token is removed from uploaded event payloads and Profile::Off to verify raw bytes round-trip unchanged. Exercise the behavior through push_events_scoped where practical, while retaining direct coverage of redact_event_lines if that is the established test pattern.Source: Coding guidelines
src/policy.rs (1)
38-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepo/source/campaign/role scope-matching logic is reimplemented in five places.
ReadScopedoesn't expose a single reusable predicate, so the same security-relevant AND-chain (and, separately, the campaign/role extraction from an event) is hand-copied inpolicy.rs,view.rs, and twice more intrace.rs. That's a maintainability risk today and a real risk of future scope-bypass if one copy is updated (e.g. a new scope dimension) and the others aren't.
src/policy.rs#L38-L43: extract the repo/source/campaign/role AND-chain used byallows_eventinto a small reusable method (e.g.ReadScope::allows(repo, source, campaign, role)), and makecampaign()/role()(L57-66)pub(crate)so other modules can reuse the extraction too.src/trace.rs#L219-L228: call the sharedpub(crate)campaign/role extraction helper frompolicy.rsinstead of re-deriving it fromev.rollup_dim/ev.payloadinline.src/trace.rs#L877-L891: havescope_allowscall the newReadScope::allows(...)helper instead of re-inlining the AND-chain.src/trace.rs#L1207-L1218: have theResolved::Sessionarm delegate toscope_allows(or the sharedReadScope::allows(...)) instead of a third inline copy of the same chain.src/view.rs#L417-L423: have thedocs.retain(...)closure call the same sharedReadScope::allows(...)helper instead of re-inlining the chain againstdoc.meta.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/policy.rs` around lines 38 - 43, Centralize scope matching in src/policy.rs lines 38-43 by adding a reusable ReadScope::allows(repo, source, campaign, role) method, updating allows_event to use it, and making campaign() and role() pub(crate). In src/trace.rs lines 219-228, reuse those extraction helpers; in lines 877-891 and 1207-1218, delegate to ReadScope::allows or scope_allows; and in src/view.rs lines 417-423, use ReadScope::allows in the docs.retain closure, removing each duplicated AND-chain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/helm/synty/templates/builder.yaml`:
- Around line 27-30: Keep shell line continuations inside the corresponding
optional Helm branches so empty values cannot join separate commands. Update
deploy/helm/synty/templates/builder.yaml lines 27-30 to conditionally emit the
AWS-profile continuation and keep synty build separate; apply the same
conditional continuation handling for AWS, campaign, and role options in
deploy/helm/synty/templates/tracker.yaml lines 29-42; apply it to the
AWS-profile option in deploy/helm/synty/templates/mcp.yaml lines 28-31 so exec
synty mcp remains a separate command.
- Around line 17-19: Add pod securityContext.fsGroup set to 10001 before
serviceAccountName in deploy/helm/synty/templates/builder.yaml (lines 17-19),
deploy/helm/synty/templates/tracker.yaml (lines 19-21), and
deploy/helm/synty/templates/mcp.yaml (lines 19-20), ensuring the shared
/var/synty volume is writable by the non-root runtime.
In `@deploy/helm/synty/templates/mcp.yaml`:
- Around line 31-36: Update the MCP endpoint configuration around the `exec
synty mcp` command to require TLS or mTLS rather than exposing plaintext
`--listen-public` HTTP. Add configurable certificate/client-auth settings and
source selectors for the Service and NetworkPolicy, ensuring ingress is not
permitted from every source. Do not enable the public endpoint by default until
both encrypted transport and caller restrictions are enforced.
In `@docs/design.md`:
- Line 171: Update the MCP policy sentence near “policy and optional read scope
bound mediated clients” to use grammatical wording, such as “policy and optional
read scope bind mediated clients” or “are bound to mediated clients,” while
preserving the intended meaning.
- Around line 335-339: Update the bucket-backplane description in the
surrounding design documentation to list only local, S3, and GCS as storage
backends; describe MCP-HTTP separately as an optional HTTP transport, and ensure
the fleet-wide encode-once and collaborative-build claims apply only to the
actual backplane.
In `@src/mcp_http.rs`:
- Around line 16-39: Update run’s incoming request loop to handle errors from
handle locally: log each per-request failure and continue accepting subsequent
requests instead of propagating it with ?. Tighten origin_allowed so HTTPS
origins are accepted only when they are loopback or explicitly present in the
configured allowlist, rejecting arbitrary HTTPS sites.
In `@src/track.rs`:
- Around line 157-158: Add scenario-style tests under the existing #[cfg(test)]
section covering Tracker::new propagation of non-empty Opts.campaign and
Opts.role into Stream metadata, and synthetic session-end events retaining the
configured campaign while asserting the event ID. Replace or extend the manually
constructed Stream and empty-campaign cases so both paths would fail if the
corresponding plumbing regressed.
- Around line 454-456: Update the environment override handling in the code
calling roots_with_overrides so CLAUDE_CONFIG_DIR and CODEX_HOME values are
considered unset when empty. Filter empty strings before passing
claude_home.as_deref() and codex_home.as_deref(), preserving non-empty overrides
and allowing fallback to the corresponding home-based defaults.
- Around line 348-351: Update the event payload construction in the relevant
tracking flow so campaign_role is written onto every envelope, not only
session_start events. Preserve the existing non-empty-role guard and continue
sourcing the value from self.campaign_role, ensuring later role-scoped events
retain the role used by ReadScope::allows_event.
---
Outside diff comments:
In @.dockerignore:
- Around line 1-8: Update the Docker ignore configuration to exclude local
credential sources by adding entries for .env files, the .aws/ directory, and
private-key files such as *.pem and *.key. Preserve all existing exclusions.
In `@src/mcp.rs`:
- Around line 254-283: The topics_text and recent_text handlers currently
enforce only repository access, ignoring other ReadScope restrictions such as
campaign, role, and source. Update these handlers to apply the complete
ReadScope to returned topics and units, carrying required scope fields onto
Unit/Session if necessary; otherwise reject or disable both tools whenever the
scope is narrower than repository-only.
In `@src/trace.rs`:
- Around line 1222-1267: Update resolve to accept the caller’s ReadScope and
apply scope_allows to every job, turn, span, session event, and event candidate
before inserting it into bases, ensuring ambiguity counts and listed IDs contain
only permitted records. Update show_text and compare_text to pass their scope
into resolve while preserving the existing post-resolution resolved_allowed
checks.
---
Nitpick comments:
In `@src/import.rs`:
- Around line 114-116: Move the crate::config::load() call out of the NDJSON
row-processing loop and load the configuration once before iteration begins.
Reuse the resulting capture_repos allowlist in the existing event repository
check, preserving the current filtering behavior.
In `@src/ingest.rs`:
- Around line 301-318: Extend an existing session_docs scenario test to include
a session_start event with rollup_dim, campaign_role, and backend values, then
assert the emitted Doc.meta contains the corresponding campaign_id,
campaign_role, and backend fields. Keep the test within the existing
#[cfg(test)] conventions and verify values propagate through docs_from_events
and SessionMaps.
In `@src/main.rs`:
- Around line 271-376: Document the expanded Import, Init, and Mcp CLI surfaces
in README.md and docs/design.md, including flags such as --bind,
--listen-public, --scope, --redaction, --campaign, and --no-autostart. Keep the
documentation aligned with the command definitions and existing AGENTS.md
reconciliation guidance.
In `@src/policy.rs`:
- Around line 38-43: Centralize scope matching in src/policy.rs lines 38-43 by
adding a reusable ReadScope::allows(repo, source, campaign, role) method,
updating allows_event to use it, and making campaign() and role() pub(crate). In
src/trace.rs lines 219-228, reuse those extraction helpers; in lines 877-891 and
1207-1218, delegate to ReadScope::allows or scope_allows; and in src/view.rs
lines 417-423, use ReadScope::allows in the docs.retain closure, removing each
duplicated AND-chain.
In `@src/sync.rs`:
- Around line 309-328: Add a scenario-style unit test under the existing
#[cfg(test)] conventions for redact_event_lines, using Profile::Standard to
verify a Bearer token is removed from uploaded event payloads and Profile::Off
to verify raw bytes round-trip unchanged. Exercise the behavior through
push_events_scoped where practical, while retaining direct coverage of
redact_event_lines if that is the established test pattern.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 97be4b96-3315-468a-9d4f-85ad4e063341
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
.dockerignore.github/workflows/release.ymlAGENTS.mdCargo.tomlDockerfileREADME.mddeploy/helm/synty/Chart.yamldeploy/helm/synty/templates/_helpers.tpldeploy/helm/synty/templates/builder.yamldeploy/helm/synty/templates/configmap.yamldeploy/helm/synty/templates/mcp.yamldeploy/helm/synty/templates/pvc.yamldeploy/helm/synty/templates/serviceaccount.yamldeploy/helm/synty/templates/tracker.yamldeploy/helm/synty/values.yamldocs/design.mdsrc/config.rssrc/event.rssrc/fleet.rssrc/import.rssrc/ingest.rssrc/init.rssrc/main.rssrc/mcp.rssrc/mcp_http.rssrc/policy.rssrc/redact.rssrc/search.rssrc/sync.rssrc/trace.rssrc/track.rssrc/tui.rssrc/up.rssrc/view.rs
Make scope enforcement fail closed across 12 tools, stabilize canonical imports and atomic sync, and bound authenticated HTTP execution. Add native multi-arch ECR publishing, read-only IRSA/Helm deployment, and PR checks covering 271 default plus 275 cloud/MCP scenarios.
Require certificate-backed HTTPS and source-restricted NetworkPolicy rules before Helm enables remote MCP. Filter trace identifiers before ambiguity reporting, centralize scope predicates, and preserve campaign roles on every event. Add scenario coverage for scoped ids, metadata propagation, upload redaction, tracker construction, synthetic ends, TLS serving, and CLI parsing. The default and shipped feature matrices pass 274 and 278 tests respectively.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/mcp_http.rs (1)
262-266: 🩺 Stability & Availability | 🔵 TrivialNo body read timeout on this public listener.
read_to_end(...)runs before the dispatcher timeout, so a slow client can occupy one of theMAX_IN_FLIGHTslots until the upload completes. If--listen-publicremains exposed, enforce client-body timeouts at a proxy or another listener that supports connection read deadlines.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp_http.rs` around lines 262 - 266, Update the public-listener request handling around the body read in the relevant handler to enforce a client-body read timeout before dispatching, using a proxy or listener mechanism that supports connection read deadlines. Ensure slow uploads cannot hold a MAX_IN_FLIGHT slot indefinitely, while preserving the existing MAX_BODY_BYTES limit and 413 response.src/bucket/cloud.rs (1)
280-303: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePartial writes are invisible on batch failure.
buffer_unordereddrains every future before the loop maps errors, so if one object's PUT fails, other objects in the same batch may already have been created — but the caller only sees an aggregateErrwith no indication of how many succeeded. Since writes are write-once/idempotent, this isn't corrupting, but callers retrying on error will assume nothing happened. Consider surfacing the partialcreatedcount alongside the error if callers ever need to reason about retry cost.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bucket/cloud.rs` around lines 280 - 303, Update put_many_if_absent to retain the total number of successful PUTs when any result fails, rather than returning immediately from the result loop. Process all collected results, count every created object, record the first error with its key, then return an error that includes the aggregate created count; otherwise return the count as today.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Around line 99-108: Update the “Check immutable architecture tag” step to pass
the version output through the step’s env block, then reference that environment
variable in the shell command instead of interpolating
steps.version.outputs.version directly. Preserve the existing ECR repository and
matrix suffix behavior while ensuring the tag-derived value is not treated as
shell syntax.
In `@CONTRIBUTING.md`:
- Around line 59-61: Update the container publishing description in
CONTRIBUTING.md to state that the version tag publishes a verified
multi-architecture Linux manifest containing both amd64 and arm64 images,
matching the behavior of the release.yml image-index job. Remove the inaccurate
claim that it publishes only an immutable linux/amd64 container while preserving
the registry path and deployment instructions.
In `@deploy/helm/synty/templates/builder.yaml`:
- Around line 42-44: Remove the conditional AWS_PROFILE environment-variable
injection from the builder pod template in
deploy/helm/synty/templates/builder.yaml lines 42-44 and the MCP pod template in
deploy/helm/synty/templates/mcp.yaml lines 57-59. Leave AWS workloads to use
their role chain and do not replace the removed configuration with another
profile mechanism.
---
Nitpick comments:
In `@src/bucket/cloud.rs`:
- Around line 280-303: Update put_many_if_absent to retain the total number of
successful PUTs when any result fails, rather than returning immediately from
the result loop. Process all collected results, count every created object,
record the first error with its key, then return an error that includes the
aggregate created count; otherwise return the count as today.
In `@src/mcp_http.rs`:
- Around line 262-266: Update the public-listener request handling around the
body read in the relevant handler to enforce a client-body read timeout before
dispatching, using a proxy or listener mechanism that supports connection read
deadlines. Ensure slow uploads cannot hold a MAX_IN_FLIGHT slot indefinitely,
while preserving the existing MAX_BODY_BYTES limit and 413 response.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2608913d-e47f-4634-8345-87a7e95ef76d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
.github/workflows/ci.yml.github/workflows/release.ymlAGENTS.mdCONTRIBUTING.mdCargo.tomlDockerfileREADME.mddeploy/aws/ecr-publisher.yamldeploy/aws/mcp-reader.yamldeploy/helm/synty/Chart.yamldeploy/helm/synty/templates/_helpers.tpldeploy/helm/synty/templates/builder.yamldeploy/helm/synty/templates/mcp.yamldeploy/helm/synty/templates/tracker.yamldeploy/helm/synty/values-sie-gw-cfg.yamldeploy/helm/synty/values.yamldocs/design.mdscripts/mcp-http-smoke.shsrc/bucket.rssrc/bucket/cloud.rssrc/config.rssrc/fleet.rssrc/import.rssrc/index.rssrc/ingest.rssrc/init.rssrc/main.rssrc/mcp.rssrc/mcp_http.rssrc/policy.rssrc/qwen.rssrc/redact.rssrc/search.rssrc/store.rssrc/sync.rssrc/tui.rssrc/units.rssrc/view.rs
🚧 Files skipped from review as they are similar to previous changes (13)
- deploy/helm/synty/Chart.yaml
- src/fleet.rs
- Dockerfile
- Cargo.toml
- deploy/helm/synty/templates/_helpers.tpl
- src/config.rs
- src/search.rs
- src/init.rs
- deploy/helm/synty/templates/tracker.yaml
- src/ingest.rs
- src/import.rs
- src/main.rs
- src/mcp.rs
Align the local override, Cargo MSRV, CI and release jobs, and container builder on Rust 1.97.0 so every supported build path uses the same compiler. Rust 1.97 passes 274 default tests and 278 shipped-feature tests. Docker resolves rust:1.97-bookworm and reports no Dockerfile check warnings.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/helm/synty/templates/mcp.yaml (1)
145-146: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
egress: - {}allows unrestricted outbound traffic. WithpolicyTypes: [Ingress, Egress], this is allow-all egress; the chart only exposesnetworkPolicy.ingressFrom, so there’s no way to scope outbound DNS/S3 access yet. Add explicit egress rules and chart values for the required destinations/ports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/synty/templates/mcp.yaml` around lines 145 - 146, Replace the unrestricted egress rule in the chart’s NetworkPolicy with explicit destination and port rules for the required DNS and S3 access. Add corresponding configurable chart values, wire them into the egress rules, and preserve the existing network policy behavior for ingress.
🧹 Nitpick comments (1)
scripts/mcp-http-smoke.sh (1)
93-100: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winValidate the generated certificate instead of using
-k.
-kmakes this TLS check accept any certificate. Generate a certificate with a SAN for127.0.0.1and use--cacert "$work/tls.crt"so the smoke test validates the trust path too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/mcp-http-smoke.sh` around lines 93 - 100, Update the TLS setup and health-check curl in the smoke test: generate the certificate with a subject alternative name for 127.0.0.1, then replace curl’s -k option with --cacert "$work/tls.crt" so the check validates the generated certificate and trust path.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/helm/synty/templates/mcp.yaml`:
- Around line 8-14: Strengthen the ingress validation in the
networkPolicy.ingressFrom template loop so podSelector and namespaceSelector
cannot be empty or contain empty matchLabels/matchExpressions. Require each
selector to include at least one non-empty matchLabels entry or matchExpressions
constraint before rendering MCP ingress, while preserving the existing rejection
of empty entries and unrestricted namespace selectors.
---
Outside diff comments:
In `@deploy/helm/synty/templates/mcp.yaml`:
- Around line 145-146: Replace the unrestricted egress rule in the chart’s
NetworkPolicy with explicit destination and port rules for the required DNS and
S3 access. Add corresponding configurable chart values, wire them into the
egress rules, and preserve the existing network policy behavior for ingress.
---
Nitpick comments:
In `@scripts/mcp-http-smoke.sh`:
- Around line 93-100: Update the TLS setup and health-check curl in the smoke
test: generate the certificate with a subject alternative name for 127.0.0.1,
then replace curl’s -k option with --cacert "$work/tls.crt" so the check
validates the generated certificate and trust path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5de73af-21a6-4d1a-a659-a5cb80783a99
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.dockerignore.github/workflows/ci.yml.github/workflows/release.ymlCargo.tomlDockerfileREADME.mddeploy/helm/synty/templates/mcp.yamldeploy/helm/synty/values-sie-gw-cfg.yamldeploy/helm/synty/values.yamldocs/design.mdrust-toolchain.tomlscripts/mcp-http-smoke.shsrc/ingest.rssrc/main.rssrc/mcp_http.rssrc/policy.rssrc/sync.rssrc/trace.rssrc/track.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- .dockerignore
- deploy/helm/synty/values-sie-gw-cfg.yaml
- Dockerfile
- Cargo.toml
- .github/workflows/ci.yml
- .github/workflows/release.yml
- deploy/helm/synty/values.yaml
- README.md
- docs/design.md
- src/mcp_http.rs
- src/policy.rs
- src/sync.rs
- src/ingest.rs
- src/main.rs
- src/track.rs
- src/trace.rs
Replace blocking HTTP body reads with Hyper/Rustls deadlines that cap 64 connections, 32 in-flight requests, and 4 MiB bodies. Reject unrestricted Helm caller selectors and leave Kubernetes AWS credentials on the workload role chain. Drain cloud batch writes before reporting partial failure, harden tag-derived shell input, and reconcile multi-architecture docs. Validation: 274 default tests, 280 shipped-feature tests, HTTP/HTTPS smoke, Helm lint/render, and Dockerfile check.
Join the v0.2.4 release and fleet batch-transfer history into the mediated-access branch without rewriting the PR commits. The overlapping cloud batch implementation keeps partial-success accounting from d549336. The resolved tree is unchanged from the tested PR head: 274 default tests and 280 shipped-feature tests passed before the merge.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CONTRIBUTING.md (1)
54-54: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument
mcp-httpfor the macOS release too.The release workflow builds
macos-14withmetal,s3,gcs,mcp-http, but this paragraph lists onlymetal,s3,gcsfor macOS.Based on the supplied
.github/workflows/release.ymlcontext, the macOS matrix includesmcp-http.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CONTRIBUTING.md` at line 54, Update the macOS release feature list in the relevant CONTRIBUTING.md paragraph to include mcp-http alongside metal,s3,gcs, matching the macos-14 release workflow; leave the Ubuntu feature listing unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bucket/cloud.rs`:
- Around line 473-488: Update the failure test around Cloud::put_many_if_absent
to use a deterministic failing ObjectStore wrapper and invoke the public
batch-put method instead of calling summarize_batch_puts directly. Assert the
returned partial-progress error includes the successful-create count, object
name, and injected backend error, while retaining the existing helper test only
as supplemental coverage.
---
Outside diff comments:
In `@CONTRIBUTING.md`:
- Line 54: Update the macOS release feature list in the relevant CONTRIBUTING.md
paragraph to include mcp-http alongside metal,s3,gcs, matching the macos-14
release workflow; leave the Ubuntu feature listing unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 881f8e0c-9c8b-4245-8b30-5c1bb95a9f7f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/ci.yml.github/workflows/release.ymlCONTRIBUTING.mdCargo.tomlREADME.mddeploy/helm/synty/templates/builder.yamldeploy/helm/synty/templates/mcp.yamldeploy/helm/synty/templates/tracker.yamldeploy/helm/synty/values-sie-gw-cfg.yamldeploy/helm/synty/values.yamlsrc/bucket/cloud.rssrc/mcp_http.rs
💤 Files with no reviewable changes (2)
- deploy/helm/synty/templates/builder.yaml
- deploy/helm/synty/templates/tracker.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/workflows/ci.yml
- deploy/helm/synty/values-sie-gw-cfg.yaml
- deploy/helm/synty/templates/mcp.yaml
- README.md
- deploy/helm/synty/values.yaml
- .github/workflows/release.yml
- src/mcp_http.rs
Publish deterministic format-2 session, tool, fleet, and bounded trace projections inside immutable builds so MCP readers never scan the raw lake. Fail readiness closed until those projections exist, serialize heavy analysis calls, and constrain object-store egress.\n\nStream the 657 MiB document store, exercise default, no-LLM, cloud, HTTP, and Helm paths, and preserve all 104,512 live event identities while bounding literal trace evidence.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/helm/synty/templates/mcp.yaml`:
- Around line 8-15: Strengthen the validation around
networkPolicy.objectStoreCidrs so CIDR combinations cannot recreate unrestricted
internet egress, including split IPv4 or IPv6 /0 ranges such as paired /1
prefixes. Reject overly broad prefixes or validate the configured list against
the allowed object-store endpoint ranges before the NetworkPolicy egress rules
are rendered; preserve the existing required-list and exact /0 checks.
In `@src/eval.rs`:
- Around line 220-225: Add a scenario-style unit test in the existing
#[cfg(test)] block for the default-feature, extractive-only configuration,
invoking run_names with a representative bucket and asserting it returns an
error containing the actionable “llm” feature message. Keep the test gated
consistently with the non-llm implementation and preserve the existing command
contract.
In `@src/index.rs`:
- Around line 34-48: Ensure index::run and ingest publish and consume one
immutable ingest generation: stage docs, projections, and related artifacts in a
generation directory, atomically publish its pointer, and resolve all inputs and
build outputs from that same generation before repointing. Update src/index.rs
lines 34-48 to read a single generation snapshot, src/index.rs lines 195-205 to
write artifacts selected from that snapshot, and src/ingest.rs lines 145-162 to
stage and atomically publish the generation; alternatively, hold the shared
ingest/index lock across the complete build. Add a scenario-style test covering
concurrent ingest and indexing, verifying no mixed-generation documents,
embeddings, metadata, or snapshots are published.
In `@src/trace.rs`:
- Around line 211-255: Validate all snapshot reference values in
TraceSnapshot::into_store before constructing and rebuilding TraceStore,
ensuring event span indexes, span start_event/end_event values, job
span_indexes, and turn event_indexes/span_indexes are within their corresponding
collection bounds. Return a descriptive anyhow error for any invalid reference,
while preserving valid snapshot loading.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1276baa2-d4a3-4cec-bde5-a78cbcd14ae4
📒 Files selected for processing (24)
.github/workflows/ci.ymlCONTRIBUTING.mdREADME.mddeploy/helm/synty/templates/mcp.yamldeploy/helm/synty/values-sie-gw-cfg.yamldeploy/helm/synty/values.yamldocs/design.mdscripts/mcp-http-smoke.shsrc/bucket/cloud.rssrc/eval.rssrc/fleet.rssrc/index.rssrc/ingest.rssrc/main.rssrc/mcp.rssrc/mcp_http.rssrc/qwen.rssrc/readmodel.rssrc/related.rssrc/sync.rssrc/trace.rssrc/units.rssrc/view.rssrc/words.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- deploy/helm/synty/values-sie-gw-cfg.yaml
- CONTRIBUTING.md
- .github/workflows/ci.yml
- scripts/mcp-http-smoke.sh
- deploy/helm/synty/values.yaml
- README.md
- docs/design.md
- src/sync.rs
- src/mcp.rs
- src/main.rs
Serialize ingest replacement with index publication so documents, metadata, embeddings, and both projections come from one generation. Validate every trace reference before reconstruction and reject object-store CIDR sets that can recreate split default routes. Exercise 280 default, 259 extractive-only, and 286 shipped-feature scenarios.
Add an explicit read-only index mode that reuses fleet embeddings while keeping misses and the completed read-model local. The live bucket covered 4,621 of 10,800 retained documents without changing the MCP reader role. Exercise 281 default, 261 extractive-only, and 288 shipped-feature scenarios.
Keep chart-created PVCs across Helm upgrades and point sie-gw-cfg at the verified 50 GiB replacement after the live 20 GiB gp2 claim reached 100% with 15 MiB free during shard 5. CI asserts the retention annotation; Helm lint and both default and cluster-specific renders pass.
Emit full native session and topic ids from agent-facing lists and ambiguity errors. The live sie-gw-cfg corpus contained two sessions under the printed 019f7d1e prefix, so synty_show could not consume its own search result directly. Validation: 283 default, 262 extractive-only, 289 shipped-feature scenarios, plus HTTP/TLS smoke.
Contributor
|
Does this add a server in front of the bucket for auth? Given that this is used in a trusted environment, why do we need that? |
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.
Summary
synty_trace_*tools on MCP alongside the existing read surface, with role/tool policy (primary/investigator/validator/operator) and optional JSON read scopes (repos, campaigns, roles, sources).--http, featuremcp-http, bearer token + loopback-by-default) so remote campaign agents can query without raw bucket credentials.off/standard/mcp_safe),synty importfor harness/Devin NDJSON, campaign/role stamps,$CODEX_HOME/$CLAUDE_CONFIG_DIRroots,--no-autostartfor supervisors, plus Dockerfile and Helm (tracker / builder / MCP).Why
Campaign harness agents need a mediated memory/forensics path into synty: structured recall + execution traces, without giving every agent write/raw-bucket trust. This keeps synty as the telemetry/memory layer while a separate harness owns orchestration policy.
Test plan
cargo test --locked— 250 passedcargo test --locked --features mcp-http— 250 passedsynty mcp --role investigatorand calltools/list/synty_trace_listsynty mcp --http --token …hit/healthand/mcpwith Bearer authsynty import --format harnessdry-run on a sample NDJSON fileCODEX_HOME=/tmp/codex synty track --source codex --drypath resolution (sessions under override)synty init … --no-autostartskips launchd/systemd installDockerfileandhelm templatethe chart with a bucket + MCP secretMade with Cursor
Summary by CodeRabbit
New Features
importfor Harness/Devin/envelope NDJSON with filtering, quarantine, deduplication, and dry-run.Security
Documentation