Skip to content

Conversation

@MasterPtato
Copy link
Contributor

No description provided.

@vercel
Copy link

vercel bot commented Nov 14, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Error Error Nov 17, 2025 7:18pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 17, 2025 7:18pm
rivet-inspector Ignored Ignored Preview Nov 17, 2025 7:18pm
rivet-site Ignored Ignored Preview Nov 17, 2025 7:18pm

Copy link
Contributor Author

MasterPtato commented Nov 14, 2025


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Nov 14, 2025

PR Review: Add Debug Scripts

Summary

This PR adds debugging utilities to help visualize and analyze Rivet engine logs. The main additions are:

  1. logfmt_to_gantt.ts - Converts logfmt logs to Mermaid Gantt charts
  2. debug_mermaid.ts - Binary search tool for debugging problematic Mermaid gantt entries
  3. Moves decode_audit_entry.js to the debug directory

Code Quality & Best Practices

Positive Aspects

  • Well-structured code: Both scripts have clear separation of concerns with dedicated helper functions
  • Good documentation: Usage instructions in comments at the top of files
  • Type safety: Proper TypeScript interfaces for LogEntry and Task
  • Robust parsing: Handles ANSI color codes, quoted values, and edge cases in logfmt parsing
  • Flexible filtering: Time-based filtering with support for both absolute timestamps and time-of-day

Issues & Suggestions

1. Potential bugs in logfmt_to_gantt.ts

Line 505, 508, 513: Potential runtime error when workflow_id is undefined

const wfId = task.metadata.workflow_id.substring(0, 8);  // Could crash if undefined

The code assumes workflow_id exists for workflow and activity tasks in non-split mode, but only checks for it in signal tasks (line 513). Should add null checks or use optional chaining:

const wfId = task.metadata.workflow_id?.substring(0, 8) || 'unknown';

Line 225: No validation that entries array is non-empty before accessing entries[0].ts

const startTime = entries[0].ts;

While there's a check at line 171, subsequent filtering (lines 177-208) could leave the array empty. The empty check at line 204 should be moved after the time filtering logic.

2. Logic issues

Lines 188-191, 194-199: Inefficient array mutation

entries.splice(0, entries.length, ...entries.filter(...));

This creates a copy of the filtered array and splices it back. Better to use simple reassignment:

const filtered = entries.filter(...);
entries.length = 0;
entries.push(...filtered);

Or better yet, avoid mutation entirely and use a new variable.

Lines 317-323: Operation matching logic is fragile

for (const [k, task] of operationTasks.entries()) {
    if (k.startsWith(entry.operation_name) && !task.end) {
        task.end = entry.ts;
        break;
    }
}

This matches by string prefix, which could incorrectly match operations with similar names (e.g., "operation" would match "operation_call"). Should use exact name matching with timestamp-based pairing.

3. Error handling

Missing error handling for:

  • File I/O operations (lines 157, 552) - Should catch and handle ENOENT, EACCES errors
  • Invalid date parsing (line 59) - new Date(value) could return Invalid Date
  • Fractional second parsing (line 64) - parseFloat could return NaN

Recommended pattern:

try {
    const content = readFileSync(inputFile, 'utf-8');
} catch (error) {
    console.error(`Failed to read file ${inputFile}:`, error.message);
    process.exit(1);
}

4. Performance considerations

Line 317-323: Nested loop for operation matching - O(n²) complexity if many operations
Consider using a Map with operation name as key to track pending operations for O(1) lookup.

Line 188-199: Creating new arrays for filtering when could filter once and store result

5. Code style issues

Hard-coded file paths: Lines 11, 145, 146 use /tmp/ paths. Consider:

  • Making these configurable via environment variables
  • Using os.tmpdir() for cross-platform compatibility
  • Adding validation that paths exist/are writable

Magic numbers:

  • Line 95: substring(0, 50) - Should be a named constant
  • Line 518: 0.01 - Should be a named constant like MIN_DURATION_THRESHOLD_MS

Inconsistent string concatenation: Mix of template literals and string concatenation

Security Concerns

Low Risk Issues

  1. Arbitrary file paths (lines 145-146): Scripts accept user-provided file paths without validation. Could potentially:

    • Read sensitive files if run with elevated permissions
    • Write to arbitrary locations

    Recommendation: Validate that paths are within expected directories or use a safelist.

  2. Regular expression complexity (lines 40, 51): ANSI regex and logfmt regex should be tested for ReDoS vulnerabilities, though current patterns appear safe.

  3. Process.exit() calls: Multiple calls without cleanup could leave resources in inconsistent state if extended in the future.

Performance Considerations

  1. Memory usage: Large log files are loaded entirely into memory (line 157). For very large logs (>100MB), consider streaming or chunked processing.

  2. O(n²) iteration: The operation matching logic could be optimized as mentioned above.

  3. String operations: Multiple string replacements in sanitizeForMermaid could be combined into a single pass with a custom function.

Test Coverage

Missing tests: No tests are included for these utilities. Recommended test cases:

  • Parsing various logfmt formats
  • Handling malformed log entries
  • Time filtering edge cases
  • Empty log files
  • Very large log files
  • Invalid Mermaid characters in workflow/activity names

Additional Observations

Positive

  • The sub-millisecond precision tracking is well-implemented
  • Signal send/receive duration calculation is clever
  • The split-by-workflow feature provides useful flexibility

Minor improvements

  1. debug_mermaid.ts line 38: Using Math.max(...sectionStarts.filter(s => s < 10)) could fail if no sections exist or all are >= 10. Should handle edge case.

  2. Type definitions: Consider moving interfaces to a shared types file if these scripts will be extended.

  3. Configuration: Consider using a config file or environment variables for defaults instead of hard-coded values.

Overall Assessment

Recommendation: Approve with minor fixes

These are useful debugging tools that will help with development and troubleshooting. The code is generally well-written but has some potential runtime errors that should be fixed before merging:

Must fix:

  • Add null checks for workflow_id access (logfmt_to_gantt.ts:505, 508)
  • Move empty array check after time filtering logic

Should fix:

  • Add error handling for file I/O
  • Validate date parsing results
  • Fix inefficient array mutation pattern

Nice to have:

  • Add tests
  • Extract magic numbers to constants
  • Consider streaming for large files
  • Use cross-platform temp directory resolution

@claude
Copy link

claude bot commented Nov 17, 2025

Code Review

Thank you for adding these debug utilities! These tools will be valuable for debugging and analyzing Rivet engine execution.

Positives

  • Well-documented with clear usage instructions
  • Practical utility for visualizing workflow execution
  • Good TypeScript setup with proper package configuration
  • Comprehensive logfmt parser handling ANSI colors and fractional seconds

Code Quality Issues

1. Potential null pointer exception (logfmt_to_gantt.ts:505, 508)
Lines 505 and 508 access task.metadata.workflow_id.substring(0, 8) without checking if workflow_id exists. Line 513 handles this correctly for signals with optional chaining. Should use: task.metadata.workflow_id?.substring(0, 8) || 'unknown'

2. Inconsistent array mutation (logfmt_to_gantt.ts:188, 194)
Using entries.splice(0, entries.length, ...entries.filter(...)) is unusual. Consider more idiomatic approaches.

3. Non-unique task keys (logfmt_to_gantt.ts:303)
Using timestamp in key could cause collisions if operations start at the same millisecond. Consider adding a counter.

4. Inefficient operation matching (logfmt_to_gantt.ts:317-323)
Iterating through all operations to find a match could match the wrong operation if multiple with same name are running.

Performance Considerations

  • Large file handling: readFileSync loads entire file into memory. Consider streaming for large logs or add memory warning
  • Quadratic complexity (debug_mermaid.ts:38): Math.max with spread and filter creates unnecessary intermediate arrays

Best Practices

  • Hard-coded /tmp paths should be configurable
  • Missing error handling around file operations
  • metadata: Record<string, any> uses any - consider unknown or specific types
  • service field defined but never used
  • Magic numbers like 50 should be constants

Recommendations

  1. Add null safety checks for workflow_id access (critical)
  2. Add basic error handling around file I/O
  3. Document expected log format
  4. Consider --help flag support

Summary: Useful debugging tools with clean structure. Main concerns are null safety (critical) and large file handling. Recommend approval with suggested improvements for null safety.

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.

2 participants