Safety fix :Add explicit safety warnings for pickle-based recording workflows

[apfine]

I propose the following changes to use of .pkl in order to ensure the safety of the user's system.

Summary

This PR keeps pickle support in the recording workflow, but makes its security implications explicit by warning users when pickle-based recordings are loaded or saved.

Changes

• emit a UserWarning when loading .pkl recordings in AgentViewer
• emit a UserWarning when saving recordings with format="pickle" in SimulationRecorder.save()
• preserve existing pickle support for trusted local workflows
• keep JSON behavior unchanged
• update recording tests to assert warning-based behavior

Why

Pickle deserialization can execute arbitrary code when loading untrusted files. While pickle may still be useful for trusted local workflows and backward compatibility, it should not appear to be a risk-free default path.

This change keeps user choice while making the security tradeoff explicit at the point of use.

Validation

• full repository test suite passed
• targeted recording test suite passed
• repo-wide Ruff checks passed

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 71374c47-4f75-4850-a40c-d8395d407b55

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[IlamaranMagesh]

Just curious, removing a feature entirely, is it a viable option? I understand .pkl's case but can we not provide an alternate option? And moreover, the user is aware and responsible for loading the file, since the .pkl file is created using one of the available methods we provide. We can technically make the .pkl safer but in my opinion, it's not recommended as the data community is moving away from .pkl.

The alternate export feature requires a separate discussioon, since other options like safetensors or parquet need some extra logics and dependency.

Happy to hear your views.

[apfine]

Just curious, removing a feature entirely, is it a viable option? I understand .pkl's case but can we not provide an alternate option? And moreover, the user is aware and responsible for loading the file, since the .pkl file is created using one of the available methods we provide. We can technically make the .pkl safer but in my opinion, it's not recommended as the data community is moving away from .pkl.

The alternate export feature requires a separate discussioon, since other options like safetensors or parquet need some extra logics and dependency.

Happy to hear your views.

I see what you are saying about choice.

But as of now we provide pickle as a first-class supported path which signal legitimacy and safety , we can keep it but we should include noticable warnings to probable consequences of using it .

The issue is if this module is being used in a confidential system one can infiltrate the system by somehow deserializing their malicious .pkl through our current methods , and we offer no protection to it as of now .

So all I wanna say is we should keep pickle as an advanced choice rather than being first in the row .

We can work on this version rather than removing .pkl.

I think it would be a better choice .

Thankyou @IlamaranMagesh for your views.

Would be happy to hear your views !!

[apfine]

@IlamaranMagesh I have tried to implement a UserWarning system if a user uses .pkl.

[IlamaranMagesh]

Thanks @apfine. @colinfrisch, thoughts?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.66%. Comparing base (a719dac) to head (54b96a9).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #181      +/-   ##
==========================================
+ Coverage   90.64%   90.66%   +0.01%     
==========================================
  Files          19       19              
  Lines        1540     1543       +3     
==========================================
+ Hits         1396     1399       +3     
  Misses        144      144              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[apfine]

@BhoomiAgrawal12 @wang-boyu @EwoutH
Would be happy to hear your thoughts !!

[IlamaranMagesh]

I have reviewed the changes and shared my thoughts in the main conversation thread above. Overall, it looks good. I'll let the maintainers take the call.

[apfine]

I have reviewed the changes and shared my thoughts in the main conversation thread above. Overall, it looks good. I'll let the maintainers take the call.

Thankyou ,

I request maintainers to give some feedback @EwoutH @BhoomiAgrawal12 @khushiiagrawal

Thankyou for your valuable time !!