Skip to content

feat(encryption): add encrypt unit test for plaintext shorter than t…#1217

Merged
MaojiaSheng merged 1 commit intovolcengine:mainfrom
baojun-zhang:encrypt-unit-test
Apr 5, 2026
Merged

feat(encryption): add encrypt unit test for plaintext shorter than t…#1217
MaojiaSheng merged 1 commit intovolcengine:mainfrom
baojun-zhang:encrypt-unit-test

Conversation

@baojun-zhang
Copy link
Copy Markdown
Collaborator

@baojun-zhang baojun-zhang commented Apr 4, 2026

Description

add encrypt unit test for plaintext shorter than the magic header

Related Issue

#1163

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test update

Changes Made

  • add encrypt unit test for plaintext shorter than the magic header
  • change to zip mode to default behavior

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested this on the following platforms:
    • Linux
    • macOS
    • Windows

Checklist

  • My code follows the project's coding style
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Screenshots (if applicable)

Additional Notes

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 4, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🏅 Score: 90
🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: Add unit test for decrypting short unencrypted plaintext

Relevant files:

  • tests/unit/crypto/test_encryptor.py

Sub-PR theme: Change zip to non-strict mode in summarizer

Relevant files:

  • openviking/utils/summarizer.py

⚡ Recommended focus areas for review

Suggestion

Changed zip(..., strict=True) to strict=False despite an explicit length check immediately before. This removes a defense-in-depth safeguard that would catch accidental mismatches if the length check were ever removed or bypassed.

for uri, temp_uri in zip(resource_uris, temp_uris, strict=False):

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 4, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Restore strict zip for safety

The code already ensures len(temp_uris) == len(resource_uris) via an explicit check.
Using strict=True in zip() adds a defensive safeguard to catch any future
regressions where the length check might be bypassed or modified.

openviking/utils/summarizer.py [59]

-for uri, temp_uri in zip(resource_uris, temp_uris, strict=False):
+for uri, temp_uri in zip(resource_uris, temp_uris, strict=True):
Suggestion importance[1-10]: 5

__

Why: The code already verifies len(temp_uris) == len(resource_uris), so strict=True adds a defensive safeguard against future regressions without breaking current behavior. This is a reasonable maintainability improvement.

Low

@baojun-zhang
Copy link
Copy Markdown
Collaborator Author

baojun-zhang commented Apr 4, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

Category **Suggestion                                                                                                                                    ** Impact
General
Restore strict zip for safety
The code already ensures len(temp_uris) == len(resource_uris) via an explicit check. Using strict=True in zip() adds a defensive safeguard to catch any future regressions where the length check might be bypassed or modified.

openviking/utils/summarizer.py [59]

-for uri, temp_uri in zip(resource_uris, temp_uris, strict=False):
+for uri, temp_uri in zip(resource_uris, temp_uris, strict=True):

Suggestion importance[1-10]: 5
__

Why: The code already verifies len(temp_uris) == len(resource_uris), so strict=True adds a defensive safeguard against future regressions without breaking current behavior. This is a reasonable maintainability improvement.

Low

use strict=False instead . cause there already have if len(temp_uris) != len(resource_uris): to prevent
and silence truncate better than interrupt the summarize processing

@MaojiaSheng MaojiaSheng merged commit 821287d into volcengine:main Apr 5, 2026
11 checks passed
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenViking project Apr 5, 2026
@baojun-zhang baojun-zhang deleted the encrypt-unit-test branch April 5, 2026 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants