feat(xtest): improve TDF test stability and add ec-wrapped schema support#412
feat(xtest): improve TDF test stability and add ec-wrapped schema support#412dmihalcik-virtru wants to merge 3 commits intoopentdf:mainfrom
Conversation
Bump pyright from 1.1.380 to 1.1.408 and alphabetize dev dependency ordering in pyproject.toml. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: David Mihalcik <dmihalcik@virtru.com>
Replace busy-wait polling with condition-based wakeup using threading.Condition. Improve clock skew handling with explicit timezone support. Add frozenset constants for audit types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: David Mihalcik <dmihalcik@virtru.com>
…port Use explicit RSA attribute mapping to prevent base_key interference. Add ec-wrapped to manifest schema type enum. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: David Mihalcik <dmihalcik@virtru.com>
Summary of ChangesHello @dmihalcik-virtru, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on improving the stability and robustness of the TDF test suite by introducing explicit RSA key mapping for encryption operations, thereby mitigating potential flakiness caused by default key configurations. Concurrently, it expands the TDF manifest schema to support Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the stability of TDF tests by using an explicit RSA attribute mapping, preventing interference from base_key configurations. It also enhances the audit log assertion framework by replacing polling with a more efficient threading.Condition-based notification system, which should make tests faster and more reliable. Additionally, it adds support for "ec-wrapped" in the manifest schema.
My review has identified a few areas for improvement:
- There is some redundant code in the timezone conversion logic.
- A test case has a misleading name and docstring that does not match its implementation.
- There is a style issue with an import statement being placed inside a function.
Overall, these are great improvements for test stability and efficiency. Addressing the identified issues will further improve the code quality.
| if collection_time.tzinfo is None: | ||
| # Assume local time, convert to UTC | ||
| collection_utc = collection_time.astimezone(UTC) | ||
| else: | ||
| collection_utc = collection_time.astimezone(UTC) |
There was a problem hiding this comment.
This if/else block is redundant as both branches execute the same code: collection_utc = collection_time.astimezone(UTC). The datetime.astimezone() method already handles both naive (assuming local timezone) and aware datetimes correctly, making the original implementation more concise. This redundant logic is also repeated at line 365.
collection_utc = collection_time.astimezone(UTC)| def tamper_policy_binding(manifest: tdfs.Manifest) -> tdfs.Manifest: | ||
| pb = manifest.encryptionInformation.keyAccess[0].policyBinding | ||
| if isinstance(pb, tdfs.PolicyBinding): | ||
| import base64 | ||
|
|
||
| h = pb.hash | ||
| altered = base64.b64encode(b"tampered" + base64.b64decode(h)[:8]) | ||
| pb.hash = str(altered) | ||
| else: | ||
| import base64 | ||
|
|
||
| altered = base64.b64encode(b"tampered" + base64.b64decode(pb)[:8]) | ||
| manifest.encryptionInformation.keyAccess[0].policyBinding = str(altered) | ||
| return manifest |
There was a problem hiding this comment.
The import base64 statement is used twice inside this local function. According to PEP 8, imports should be at the top of the module. This import was previously at the top of the file and was removed in this PR. Please move it back to the top-level imports for better code style and to avoid repeated imports.
1 participant
Summary
base_keyinterferenceattribute_default_rsaparameter andattr_valuessupport throughoutdo_encrypt_with"ec-wrapped"to manifest schema type enumParent PRs
fix/xtest-audit-framework)Test plan
cd xtest && uv run ruff check . && uv run pyrightuv run pytest test_tdfs.py --sdks go -v --no-audit-logs🤖 Generated with Claude Code
Part of stacked PR series decomposing
chore/the-claudiest-day-tmux