Skip to content

Fix #276: [milestone Milestone 7 ] Implement real-time evidence streaming with EvidencePublisher for live monitor... - #277

Open
telleroutlook wants to merge 1 commit into
mainfrom
claude/issue-276
Open

Fix #276: [milestone Milestone 7 ] Implement real-time evidence streaming with EvidencePublisher for live monitor...#277
telleroutlook wants to merge 1 commit into
mainfrom
claude/issue-276

Conversation

@telleroutlook

Copy link
Copy Markdown
Collaborator

Fixes #276

Generated by claude-bot-go worker.

…ming with `EvidencePublisher` for live monitor...
@telleroutlook

Copy link
Copy Markdown
Collaborator Author

Summary

This PR implements real-time evidence streaming with EvidencePublisher and OTLP transport; the code is well-designed with proper error handling, lifecycle management, and comprehensive tests

Severity

none

Blocking findings

None.

Verdict

✅ Approved

Merge risk

low

Audit

  • model: opus
  • effort: high
  • tokens: input=0, output=0
{
  "approved": true,
  "severity": "none",
  "summary": "This PR implements real-time evidence streaming with EvidencePublisher and OTLP transport; the code is well-designed with proper error handling, lifecycle management, and comprehensive tests",
  "findings": [],
  "merge_risk": "low",
  "Model": "opus",
  "model": "claude-cli",
  "effort": "high",
  "tokens": {
    "input_tokens": 0,
    "output_tokens": 0
  },
  "same_verdict_count": 1
}

@telleroutlook

Copy link
Copy Markdown
Collaborator Author

Summary

Potential resource leak and race condition in evidencePublisher

Severity

medium

Blocking findings

  1. packages/otel-exporter/src/evidenceOtlpTransport.ts:311 Resource leak: timeout timer not cleared if fetch throws. The clearTimeout(timer) call is inside the try block after await fetch(), but if fetch() throws, the timer is never cleared, causing a resource leak. Move clearTimeout() into a finally block.
  2. packages/aep/src/evidencePublisher.ts:218 Race condition: close() can be called while #poll() is still executing. The close() method sets #closed=true and stops the timer, but if a poll is in progress, it will continue and attempt to publish to the now-closed stream. While EvidenceStream.publish() will likely handle this gracefully, the design allows overlapping close/poll operations. The close() method should await completion of any in-flight poll before proceeding.
  3. packages/aep/src/evidencePublisher.ts:145 The publish() method increments #errors counter in the catch block but then re-throws the error, making the error counter increment meaningless (the counter is only observable via stats() if the error is caught higher up). Either don't re-throw or document that errors from publish() are always fatal and the counter is for internal tracking only.
  4. packages/otel-exporter/src/evidenceOtlpTransport.ts:93 No validation that record.actions is a non-empty array. If an AEPRecord with empty actions is published, aepRecordToOtlpSpans returns an empty spans array, resulting in an OTLP payload with no spans being POSTed. While this might be valid, it's worth considering whether empty evidence records should be filtered earlier or explicitly handled.
  5. packages/otel-exporter/src/evidenceOtlpTransport.ts:217 The ensureHex function silently truncates IDs that are too long, which could result in non-unique trace/span IDs if the input ID has unique significant bits beyond the target length. This could cause trace collisions in production. Consider throwing an error or logging a warning when truncation occurs.

Verdict

❌ Rejected

Merge risk

medium

Audit

  • model: opus
  • effort: high
  • tokens: input=0, output=0
{
  "approved": false,
  "severity": "medium",
  "summary": "Potential resource leak and race condition in evidencePublisher",
  "findings": [
    {
      "file": "packages/otel-exporter/src/evidenceOtlpTransport.ts",
      "line": 311,
      "issue": "Resource leak: timeout timer not cleared if fetch throws. The clearTimeout(timer) call is inside the try block after await fetch(), but if fetch() throws, the timer is never cleared, causing a resource leak. Move clearTimeout() into a finally block.",
      "kind": "blocker"
    },
    {
      "file": "packages/aep/src/evidencePublisher.ts",
      "line": 218,
      "issue": "Race condition: close() can be called while #poll() is still executing. The close() method sets #closed=true and stops the timer, but if a poll is in progress, it will continue and attempt to publish to the now-closed stream. While EvidenceStream.publish() will likely handle this gracefully, the design allows overlapping close/poll operations. The close() method should await completion of any in-flight poll before proceeding.",
      "kind": "blocker"
    },
    {
      "file": "packages/aep/src/evidencePublisher.ts",
      "line": 145,
      "issue": "The publish() method increments #errors counter in the catch block but then re-throws the error, making the error counter increment meaningless (the counter is only observable via stats() if the error is caught higher up). Either don't re-throw or document that errors from publish() are always fatal and the counter is for internal tracking only.",
      "kind": "suggestion"
    },
    {
      "file": "packages/otel-exporter/src/evidenceOtlpTransport.ts",
      "line": 93,
      "issue": "No validation that record.actions is a non-empty array. If an AEPRecord with empty actions is published, aepRecordToOtlpSpans returns an empty spans array, resulting in an OTLP payload with no spans being POSTed. While this might be valid, it's worth considering whether empty evidence records should be filtered earlier or explicitly handled.",
      "kind": "suggestion"
    },
    {
      "file": "packages/otel-exporter/src/evidenceOtlpTransport.ts",
      "line": 217,
      "issue": "The ensureHex function silently truncates IDs that are too long, which could result in non-unique trace/span IDs if the input ID has unique significant bits beyond the target length. This could cause trace collisions in production. Consider throwing an error or logging a warning when truncation occurs.",
      "kind": "suggestion"
    }
  ],
  "merge_risk": "medium",
  "Model": "opus",
  "model": "claude-cli",
  "effort": "high",
  "tokens": {
    "input_tokens": 0,
    "output_tokens": 0
  },
  "same_verdict_count": 1
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[milestone Milestone 7 ] Implement real-time evidence streaming with EvidencePublisher for live monitor...

1 participant