feat: contributor capture middleware

[NikkiL-Snyk]

Description

This PR implements contributor capturing middleware which captures project IDs, associated them with interaction IDs and passes them to a singleton sink. A future PR will implement a post invoke hook to collect data from the sync and call the contributor emitter, as well as checking the feature flag included in a constant in this PR before enabling the sink.

Checklist

• Tests added and all succeed (make test)
• Regenerated mocks, etc. (make generate)
• Linted (make lint)
• Test your changes work for the CLI - done using feat/contributor-capture-wiring (in draft, enables capture and wires the middleware output to the contributor emitter), and have run against the real commands we're intending to capture data for

---

Note

Medium Risk
Adds new network middleware that inspects request/response bodies, which could affect CLI network behavior if body handling or panic recovery has bugs; however it is gated by a nil sink provider and includes safeguards like body restoration and panic recovery.

Overview
Adds an HTTP round-trip middleware that captures Snyk entity IDs from product API traffic and stores them in a per-interaction sink for later contributor publishing.

The middleware recognizes monitor, IaC share, test-create/test-components, and AI BOM upload endpoints. It extracts project IDs from responses (and revision IDs from AI BOM upload requests), associates them with the snyk-interaction-id header, and records them via a Sink. A new internal/contributors package provides the singleton sink and feature-flag/config constants; capture is currently disabled until contributors.Enable() is called.

The middleware is inserted into the default network round-tripper chain in pkg/networking/networking.go. It peeks request/response bodies with a 64 KiB limit and restores them so normal request flow is unaffected, and it recovers from panics to avoid breaking CLI commands.

^{Reviewed by Cursor Bugbot for commit aac9a35. Bugbot is set up for automated code reviews on this repo. Configure here.}

[snyk-io]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues
✅	Secrets	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[snyk-io]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[cursor]

IaC drops extra projects

Medium Severity

recordProjects calls RecordEntity for every parsed project ID under the same interaction ID, but the production sink keeps only the first entity per interaction and ignores later calls. IaC share responses with multiple projects therefore lose all but one captured ID, and which project survives depends on map iteration order.

Additional Locations (1)

• internal/contributors/sink.go#L44-L53

 

^{Reviewed by Cursor Bugbot for commit 14c221d. Configure here.}

[jonnyowenpowell]

This is intentional, the project is used a reference to find the target, and since all the projects share the same target, we can pick any of them.

[cursor]

Wrong create-test report JSON path

High Severity

parseCreateTestPublishReport now reads data.attributes.configuration.output.report, but create-test bodies in this repo use data.attributes.config.publish_report (see TestAttributesCreate / TestConfiguration in the generated test API client). With --report, the flag stays false, pending tests are never registered, and contributor capture never runs for the components polling flow.

 

^{Reviewed by Cursor Bugbot for commit 4316392. Configure here.}

[jonnyowenpowell]

I have tested this. As of today, the requests sent by the commands we are capturing use the hidden endpoint, and this JSON path.

[cursor]

Pending cleared before capture

Medium Severity

In the test components flow, captureIfPendingTest removes the test ID from pendingTestsSingleton before calling recordProjects. If recording panics or otherwise fails, the pending flag is already gone, so later component polls will not capture the project ID even though the response was successful.

 

^{Reviewed by Cursor Bugbot for commit a8d9f89. Configure here.}

[jonnyowenpowell]

There will be no later "component polls" because it's not a polling API - the call happens exactly once after test status polling finishes.

[basti-snyk]

I'd clean it up. Right now, IDEs may be out of scope, but they would run into exactly this kind of error, where a long running, re-used process needs to be tidy.

[jonnyowenpowell]

In response to review bot:

Broken Seeker Contract

Not true, it never broke it, it just made it potentially take a less optimized path, and it appears the bot is looking at stale data, there's now extra work to restore seeking, with a test.

Public API Layering Violation

It's fine that the middleware isn't usable outside this module. We can move it to internal, but it separates it from all the other middleware. Regardless, small change, happy to do it if a maintainer agrees with 🤖.

Memory Leak in Singletons

Memory leak is a dramatic word for this, even if it is technically accurate. You'd need thousands of tests before you got to 1MB of memory - it's a non-issue, and implementing a TTL is overengineering.

Hot-path Performance Issue

This happens once for every HTTP request and adds ns, which will be statistically insignificant compared to request durations. Again, this is rather dramatic.

[basti-snyk]

[Should Fix] pendingTestsSingleton.ids is only cleared on the success path. captureIfPendingTest returns early at line 212 when extract(body) finds no matching entity IDs, so clearPendingTest (line 215) is never reached for a create-test flow that never gets a matching components response. Since this is process-global state (var pendingTestsSingleton, line 31) with no eviction on that path, entries accumulate for the life of the process. It's harmless for a short-lived CLI run, but this repo's AGENTS.md explicitly calls out avoiding package-level mutable state for hosts that reuse a process across many invocations (IDE/MCP), so it's worth closing before this middleware is wired up more broadly.

Fix: clear the pending-test entry on any outcome for a tracked test ID (not just a successful capture), or replace the singleton with a bounded/expiring cache.

— AI review

[jonnyowenpowell]

Clearing it on any outcome is not correct, because we don't know when we're observing the final call related to that test, so we may clear it early and miss the one containing the project ID.

As I describe above, I think adding a TTL is over-engineering and it's not possible to replace the singleton because there is no workflow scope to attach to in the middleware.

[basti-snyk]

TTL wouldn't be good, I agree. How about attaching to the context?

[jonnyowenpowell]

It's possible, but only by making changes to set this up in engineImpl - and relies on code outside of this codebase not changing how it handles contexts, which isn't great.

[jonnyowenpowell]

Also, the changes actually require attaching it to a context that hangs around for the lifetime of the process anyway, so it doesn't solve test IDs sticking around after they're not required.

[basti-snyk]

[Suggestion] None of the parse functions in this file (parseMonitorProjectIDs, parseIaCShareProjectIDs, parseComponentsProjectIDs, etc.) log anything when parsing fails — they just return an empty result. If one of these API response shapes changes, capture stops silently with no signal anywhere that something broke.

Probably better as a follow-up than expanding this PR: a debug-level log with the endpoint kind on parse failure would make a schema drift visible.

— AI review

[jonnyowenpowell]

This mechanism is essentially invisible to users of the CLI, so I don't think a debug log is the right call - we won't see it anyway. What I plan to do (in the final PR for this work) is introduce data for analytics to use which indicates whether a capture occurred, because that is reported and can be analyzed.

[basti-snyk]

Could also be an error log - this is the only way to analyze things in the CLI, so error logging is essential. In every single support case, we require the debug level logs for this reason.

[jonnyowenpowell]

The point is that this is invisible to users of the CLI, and so it's very unlikely for them to ever bring us the logs. Nevertheless, I can include this in a follow up which wires this up and adds the analytics.

[basti-snyk]

[Suggestion] RecordEntity is called synchronously from RoundTrip, so a slow Sink implementation would add latency to every captured request. The current implementation (internal/contributors/sink.go) is just a mutex-protected in-memory map write, so this is harmless today, but the interface itself doesn't say implementations must be fast/non-blocking.

Probably better as a follow-up than expanding this PR: a doc comment on the interface stating the non-blocking expectation would prevent a future implementation from violating it silently.

— AI review

[jonnyowenpowell]

Added the comment.

[snyk-pr-review-bot]

PR Reviewer Guide 🔍

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Broken Retry Contract 🟠 [major] For bodies exceeding maxCaptureBodyBytes, peekAndRestoreBody returns a stitchedReadCloser which only implements io.ReadCloser. However, pkg/networking/middleware/retry_middleware.go (see cross-file context) relies on bodies satisfying io.ReadSeeker to perform efficient retries without re-buffering to memory. By returning a non-seeker, this middleware forces the retry layer to either fail or fall back to expensive re-buffering for any request larger than 64KB. if int64(len(buf)) <= maxBytes { _ = body.Close() return buf, seekableReadCloser{bytes.NewReader(buf)}, nil } return nil, stitch(buf, body), fmt.Errorf("%w (%d bytes)", errBodyTooLarge, maxBytes) } Memory Leak 🟠 [major] The pendingTestsSingleton map tracks test IDs globally. Entries are added in markTestPending but only removed in completeRequestCapture when a poll to EndpointTestComponents successfully returns a project ID. In long-running host applications like IDEs, if a test fails, is cancelled, or the user never completes the polling cycle, the UUID remains in the map indefinitely, leading to a permanent memory leak. pendingTestsSingleton.mu.Lock() pendingTestsSingleton.ids[testID] = struct{}{} pendingTestsSingleton.mu.Unlock() }

📚 Repository Context Analyzed This review considered 35 relevant code sections from 14 files (average relevance: 0.97) internal/apiclients/contributors_ingest/client.go (lines 1-167) pkg/analytics/instrumentation_collector.go (lines 1-249) pkg/networking/middleware/response.go (lines 1-211) pkg/local_workflows/doctor_workflow/diagnosis/correlation.go (lines 1-259) internal/contributors/emitter.go (lines 1-108) pkg/analytics/analytics.go (lines 1-250) internal/apiclients/contributors_ingest/2024-10-15/contributors_ingest.go (2 segments, lines 1-191) ... and 7 more file(s)

🤖 Repository instructions applied (from AGENTS.md)

[cursor]

Cursor Bugbot has reviewed your changes and found 2 potential issues.

There are 5 total unresolved issues (including 3 from previous reviews).

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit aac9a35. Configure here.}

[cursor]

AI BOM captures before success

Medium Severity

For EndpointAIBomUpload, beginRequestCapture writes the revision ID to the sink before RoundTrip runs. Monitor, IaC share, and test flows only record after a successful 2xx response. Failed uploads, HTTP errors, and transport failures can still emit contributor data for uploads that never completed.

Additional Locations (1)

• pkg/networking/middleware/contributor_capture/contributor_capture.go#L66-L68

 

^{Reviewed by Cursor Bugbot for commit aac9a35. Configure here.}

[jonnyowenpowell]

This is intentional, the data is only used later when corresponding entities exist in the platform which happens on success. If a response failure occurs, we don't know if a test started or not.

[cursor]

Pending tests map never evicted

Low Severity

Process-global pendingTestsSingleton.ids is only removed in clearPendingTest after a components response yields a project ID. Early returns on non-2xx components responses, successful responses with no parseable project ID, or a create-test flow that never completes components leave entries in the map for the process lifetime.

Additional Locations (1)

• pkg/networking/middleware/contributor_capture/contributor_capture.go#L145-L147

 

^{Reviewed by Cursor Bugbot for commit aac9a35. Configure here.}