Add native API versioning and compatibility guards - #126
Conversation
![semgrep-code-snapchat[bot]](https://avatars.githubusercontent.com/u/40505220?s=60&v=4){kind=small}
| @@ -0,0 +1,2 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <manifest xmlns:android="http://schemas.android.com/apk/res/android" package="com.snap.valdi.empty" /> | |||
There was a problem hiding this comment.
Semgrep identified an issue in your code:
This literal might contain a Snapchat internal reference that should not be committed to open-source repositories.
Fix: Please replace / remove the string to avoid committing it to open-source repositories.
To resolve this comment:
✨ Commit fix suggestion
- Replace the internal package name in the manifest with a neutral open-source identifier that does not include
.snap, for example changepackage="com.snap.valdi.empty"to something likepackage="com.example.valdi.empty". - Keep the new package value consistent with the module’s intended public namespace so Android tooling still treats it as the app/package identifier.
- Alternatively, if this manifest is only a placeholder or test fixture, remove the
packageattribute entirely when it is not required by the file’s purpose.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by internal-sensitive-strings.
You can view more details about this finding in the Semgrep AppSec Platform.
There was a problem hiding this comment.
This is a false positive: com.snap.valdi is Valdi's existing public Android package namespace, already used throughout this open-source repository, rather than an internal-only Snapchat reference.
github-actions
Bot
added
area/runtime
Sensitive Files Detected🔧 Build rules — Affects build rules for all Valdi consumers. This is an automated notice. A maintainer will review after import. |
|
| Test Suite | Result |
|---|---|
| Snapshot Tests | ✅ success |
| API Surface Check | ✅ success |
| macOS: C++ & Platform Tests | ✅ success |
| valdi_web Integration Test | ❌ failure |
| Valdi Smoke Tests | ✅ success |
| Linux: Build & Export | ✅ success |
| Linux: Hotreload Smoke | ✅ success |
| Linux: Registry Validation | ✅ success |
| Linux: C++ Tests | ✅ success |
| Linux: Build Compiler | ✅ success |
| Linux: Module Tests | ✅ success |
Some tests failed. Please check the workflow logs for details.
🚀 Bazel remote cache is now enabled - future builds will be faster!
Workflow: Valdi CI
scottthompsonsc
left a comment
scottthompsonsc
left a comment
There was a problem hiding this comment.
Notes from reading through the versioning validator. Three findings, all about @Version propagation and guard narrowing. None look covered by VersioningValidator.spec.ts, so I may be misreading the intended semantics in places. Flagging as questions rather than blockers.
| for (const declaration of declarations) { | ||
| const version = this.getVersion(declaration); | ||
| if (version !== undefined) { | ||
| return version; | ||
| } | ||
| } | ||
|
|
||
| return undefined; |
There was a problem hiding this comment.
A member's @Version is read without its container's, so uses of it are not guarded
getVersionFromSymbol resolves the required version by calling getVersion(declaration) on the member declaration itself. For a member with no annotation of its own that returns undefined, even when the containing interface or class carries @Version(N), so validatePropertyAccess and getRequiredVersionForCall see no requirement and allow the access unguarded.
// @Version(5)
export interface Renderer {
draw(): void;
}
function render(r: Renderer) {
r.draw(); // no diagnostic, but this only exists at apiVersion >= 5
}This is the direction that fails unsafely: the call compiles and then hits a missing native implementation on an older runtime. validateContainerDeclaration already propagates the container version to heritage clauses and to property members, so the propagation exists for declaration sites but not for use sites.
Is container-level @Version meant to be inherited by members? If so this lookup needs to walk to the containing declaration. If not, it might be worth rejecting @Version on a container outright, so the annotation cannot read as covering its members.
Separately, the loop returns the first declaration that has a version rather than the maximum across declarations, which would matter for merged interface declarations.
There was a problem hiding this comment.
Fixed in fe9a48f. getVersionFromSymbol now checks every declaration and merges each member annotation with its containing class, interface, or enum version. Unannotated members inherit the container requirement at use sites, and merged declarations use the highest required version. Added regression coverage for interface members, class members, guarded accesses, and merged declarations.
| if (ts.isMethodDeclaration(node) || ts.isGetAccessorDeclaration(node) || ts.isSetAccessorDeclaration(node)) { | ||
| return this.getVersion(node); | ||
| } |
There was a problem hiding this comment.
Class methods do not inherit their class's @Version during signature validation
getDeclarationVersion returns getVersion(node) for a method or accessor without consulting the parent, and validateContainerDeclaration skips function-like members, so a class method is only ever validated against its own annotation.
// @Version(5)
export class Foo {
bar(): SomeV5Type { ... } // "Type 'SomeV5Type' requires @Version(5) on the containing declaration"
}effectiveDeclarationVersion resolves to undefined here, so validateSignature reports a diagnostic for a signature the class's own version already covers, and every method has to repeat the annotation.
Interface methods are not affected, since isFunctionLikeDeclaration is false for MethodSignature and they keep going through validateContainerDeclaration to pick up memberVersion. That asymmetry between class and interface methods looks unintended.
There was a problem hiding this comment.
Fixed in fe9a48f. Class methods, constructors, accessors, instance field initializers, arrow-function fields, and nested callback signatures now inherit their enclosing class version for signature and body validation; explicit newer member annotations still win. Eager static initializers and static blocks intentionally do not inherit that version because they execute when the module loads. Added focused regression coverage for each case.
| this.validateCallExpression(node, currentVersion); | ||
| } | ||
|
|
||
| ts.forEachChild(node, (child) => this.visit(child, currentVersion)); |
There was a problem hiding this comment.
Version guards only narrow inside if statements, so && and ternary guards fall through
visitVersionCondition handles && short-circuiting, but its only caller is visitIfStatement. Everywhere else visit reaches this forEachChild and descends into both sides of a &&, and both branches of a ternary, with the outer currentVersion, so the guard is never applied.
const canDraw = isVersionAtLeast(42) && model.v42Prop;
return isVersionAtLeast(42) && model.v42Prop;
{isVersionAtLeast(42) && <NewComponent />}
isVersionAtLeast(42) ? newApi() : oldApi();The JSX form is the one I would expect to hurt most, since {cond && <Component />} is the usual way to render conditionally. Every isVersionAtLeast case in VersioningValidator.spec.ts is an if condition, so this reads as untested rather than intentional.
Routing && binary expressions through visitVersionCondition from visit, plus a ts.isConditionalExpression case that visits whenTrue with the merged version, would cover these. !isVersionAtLeast(N) followed by an early return is a related gap.
There was a problem hiding this comment.
Fixed in fe9a48f. Guard narrowing now follows && and || short-circuit order, standalone expressions, conditional JSX, both ternary branches including negated guards, nested conditions, and early-return or throw control flow. Added positive and negative coverage for guard ordering, insufficient versions, JSX, callbacks, and nested guards. The full companion suite passes: 458 tests.
…pi-versioning # Conflicts: # compiler/compiler/Compiler/Sources/Processors/NativeCodeGenerationManager.swift
beaucollins
left a comment
beaucollins
left a comment
There was a problem hiding this comment.
There's additional issues our internal review process is flagging. I tried to remove the ones you've already dismissed.
I had it create a PR with a sample test failure for the @ExportModule detection problem it is flagging.
| */ | ||
| export function isVersionAtLeast(version: number): boolean { | ||
| return version === PLACEHOLDER_VERSION || runtime.apiVersion >= version; | ||
| } |
There was a problem hiding this comment.
Mirrored from the internal import review (unresolved static-analysis finding); drafted for triage.
🟡 isVersionAtLeast will return false for version 0 on older native runtimes (bug · medium)
When newly compiled JS runs on an older native runtime (e.g., via OTA updates), runtime.apiVersion will be undefined since older binaries do not expose this property.
Because undefined >= 0 evaluates to false in JavaScript, isVersionAtLeast(0) will incorrectly return false on older runtimes. If version 0 represents the baseline API (available before versioning was introduced), this guard could skip executing logic for baseline APIs that actually exist on the older runtime.
Consider defaulting to 0 when runtime.apiVersion is missing to correctly identify older runtimes as baseline version 0.
Agentic Prompt
Paste the following into your favorite agent to work on a fix for you
I'm not sure if the following code review comment is correct. Verify it and if it's valid, then fix it. If there are existing tests for the affected code, update or add test cases to cover the fix and prevent regression. In client/src/open_source/src/valdi_modules/src/valdi/valdi_core/src/CompilerIntrinsics.ts, the issue is: When newly compiled JS runs on an older native runtime (e.g., via OTA updates), `runtime.apiVersion` will be `undefined` since older binaries do not expose this property.
Because `undefined >= 0` evaluates to `false` in JavaScript, `isVersionAtLeast(0)` will incorrectly return `false` on older runtimes. If version `0` represents the baseline API (available before versioning was introduced), this guard could skip executing logic for baseline APIs that actually exist on the older runtime.
Consider defaulting to `0` when `runtime.apiVersion` is missing to correctly identify older runtimes as baseline version `0`.
Verification
Verified via:
code_search — confirmed export function isVersionAtLeast(version: number): boolean is the only definition.
Impact evidence:
Unbound missing-property access on older runtimes causing baseline feature logic guarded by isVersionAtLeast(0) to incorrectly evaluate to false and skip execution.
Suggested fix:
| } | |
| export function isVersionAtLeast(version: number): boolean { | |
| return version === PLACEHOLDER_VERSION || (runtime.apiVersion ?? 0) >= version; | |
| } |
Help us improve our AI reviews using reactions: 👍 Helpful feedback • 👎 Not useful • 😕 Poor suggested fix
There was a problem hiding this comment.
I think it's pedantic also it's arguable whether isVersionAtLeast(0) should return true if the apiVersion is not provided
| const bodyVersion = | ||
| this.nativeApiMinVersion === undefined | ||
| ? declaredVersion ?? currentVersion | ||
| : this.mergeVersions(currentVersion, declaredVersion); |
There was a problem hiding this comment.
Mirrored from the internal import review (unresolved static-analysis finding); drafted for triage.
🟡 Inconsistent version merging for function bodies (bug · medium)
When a function with an explicit @Version annotation is declared inside an isVersionAtLeast() block, its body should run in the maximum of the block's current version and the function's declared version.
Due to this conditional logic, when nativeApiMinVersion is undefined, the body drops back to the function's explicit declaredVersion (ignoring the higher currentVersion established by the isVersionAtLeast block). This causes false positive validation errors if the body uses APIs that require the block's higher version.
Using this.mergeVersions handles the undefined cases correctly and consistently guarantees the maximum version regardless of whether a workspace minimum is configured.
Agentic Prompt
Paste the following into your favorite agent to work on a fix for you
I'm not sure if the following code review comment is correct. Verify it and if it's valid, then fix it. If there are existing tests for the affected code, update or add test cases to cover the fix and prevent regression. In client/src/open_source/compiler/companion/src/VersioningValidator.ts, the issue is: When a function with an explicit `@Version` annotation is declared inside an `isVersionAtLeast()` block, its body should run in the maximum of the block's current version and the function's declared version.
Due to this conditional logic, when `nativeApiMinVersion` is `undefined`, the body drops back to the function's explicit `declaredVersion` (ignoring the higher `currentVersion` established by the `isVersionAtLeast` block). This causes false positive validation errors if the body uses APIs that require the block's higher version.
Using `this.mergeVersions` handles the `undefined` cases correctly and consistently guarantees the maximum version regardless of whether a workspace minimum is configured.
Verification
Impact evidence:
Verified by reading the mergeVersions implementation which gracefully handles undefined while correctly taking the maximum when both versions are defined. This inconsistency drops the isVersionAtLeast guard context inside explicitly versioned functions.
Suggested fix:
| : this.mergeVersions(currentVersion, declaredVersion); | |
| const bodyVersion = this.mergeVersions(currentVersion, declaredVersion); |
Help us improve our AI reviews using reactions: 👍 Helpful feedback • 👎 Not useful • 😕 Poor suggested fix
There was a problem hiding this comment.
Addressed locally in 2847fbe (push pending because PushPatrol blocked the branch update). Function bodies now always use this.mergeVersions(currentVersion, declaredVersion), preserving a stronger surrounding isVersionAtLeast() guard even when nativeApiMinVersion is unset. Added a regression test for an @Version(42) function inside an isVersionAtLeast(43) block; the complete companion suite passes (460 tests).
|
|
||
| if (ts.isPropertyAccessExpression(node) && !this.isCalleePropertyAccess(node)) { | ||
| this.validatePropertyAccess(node, currentVersion); | ||
| } |
There was a problem hiding this comment.
Mirrored from the internal import review (unresolved static-analysis finding); drafted for triage.
🟠 ElementAccessExpression (bracket notation) bypasses native API version validation (bug · high)
When a property is accessed using bracket notation (e.g., model['subtitle']), the AST node is a ts.ElementAccessExpression rather than a ts.PropertyAccessExpression. The visit method currently lacks a handler for this expression type, meaning that any versioned property accessed via string or numeric literals completely bypasses the @Version guard validation.
This can cause runtime crashes on older clients if developers use bracket notation to access newer native API fields, as the compiler will not enforce the required isVersionAtLeast guards. We should evaluate the argument expression's type and validate it if it resolves to a literal string or number.
Agentic Prompt
Paste the following into your favorite agent to work on a fix for you
I'm not sure if the following code review comment is correct. Verify it and if it's valid, then fix it. If there are existing tests for the affected code, update or add test cases to cover the fix and prevent regression. In client/src/open_source/compiler/companion/src/VersioningValidator.ts, the issue is: When a property is accessed using bracket notation (e.g., `model['subtitle']`), the AST node is a `ts.ElementAccessExpression` rather than a `ts.PropertyAccessExpression`. The `visit` method currently lacks a handler for this expression type, meaning that any versioned property accessed via string or numeric literals completely bypasses the `@Version` guard validation.
This can cause runtime crashes on older clients if developers use bracket notation to access newer native API fields, as the compiler will not enforce the required `isVersionAtLeast` guards. We should evaluate the argument expression's type and validate it if it resolves to a literal string or number.
Verification
Verified via:
Code inspection showing ts.isElementAccessExpression is absent from the visit traversal method.
Impact evidence:
Bypassing version validation using bracket notation is a common idiom to evade strict type checks in TypeScript, which translates here to bypassing runtime safety checks for native APIs. A crash on older clients lacking the API is unrecoverable.
Suggested fix:
| } | |
| if (ts.isPropertyAccessExpression(node) && !this.isCalleePropertyAccess(node)) { | |
| this.validatePropertyAccess(node, currentVersion); | |
| } | |
| if (ts.isElementAccessExpression(node)) { | |
| const argumentType = this.typeChecker.getTypeAtLocation(node.argumentExpression); | |
| if (argumentType.isStringLiteral() || argumentType.isNumberLiteral()) { | |
| const propertyNameText = String(argumentType.value); | |
| const sourceType = this.typeChecker.getTypeAtLocation(node.expression); | |
| const symbol = this.typeChecker.getPropertyOfType(sourceType, propertyNameText); | |
| const requiredVersion = this.getVersionFromSymbol(symbol); | |
| if (requiredVersion !== undefined) { | |
| this.validateVersionedUse(node.argumentExpression, currentVersion, requiredVersion, `Property '${propertyNameText}'`); | |
| } | |
| } | |
| } |
Help us improve our AI reviews using reactions: 👍 Helpful feedback • 👎 Not useful • 😕 Poor suggested fix
There was a problem hiding this comment.
yes this limitation is on purpose because the bracket syntax cannot reliably be typed checked since the given value is rarely just a string constant (otherwise you'd use the element access syntax). As such I prefer to just make it officially not checked for bracket access rather than adding a check that is almost never useful and is inconsistent.
| const shouldDumpAllExportedSymbols = | ||
| !!sourceFile.fileName.match(/\.vue\.ts(x)?$/g) || hasExportModuleAnnotation(rootNodes[0]); | ||
| !!sourceFile.fileName.match(/\.vue\.ts(x)?$/g) || | ||
| hasExportModuleAnnotation(rootNodes[0].leadingComments?.text ?? ''); |
There was a problem hiding this comment.
Mirrored from the internal import review (unresolved static-analysis finding); drafted for triage.
🟡 Inconsistent @ExportModule file detection between AST.ts and VersioningValidator.ts (bug · medium)
In VersioningValidator.ts, sourceFileHasExportModuleAnnotation determines if a file is an export module by scanning every statement in the file for an @ExportModule annotation. However, in AST.ts, shouldDumpAllExportedSymbols only checks the leading comments of rootNodes[0].
If the @ExportModule annotation happens to be placed on a later statement in the file, VersioningValidator will incorrectly treat all exported declarations as native contracts (applying nativeApiMinVersion constraints to them), but AST.ts will fail to actually dump them. This creates a mismatch where TypeScript code is strictly validated as a native API, but the corresponding native bindings are never actually generated.
AST.ts should align with the new VersioningValidator behavior by scanning all statements for the annotation.
Agentic Prompt
Paste the following into your favorite agent to work on a fix for you
I'm not sure if the following code review comment is correct. Verify it and if it's valid, then fix it. If there are existing tests for the affected code, update or add test cases to cover the fix and prevent regression. In client/src/open_source/compiler/companion/src/AST.ts, the issue is: In `VersioningValidator.ts`, `sourceFileHasExportModuleAnnotation` determines if a file is an export module by scanning **every statement** in the file for an `@ExportModule` annotation. However, in `AST.ts`, `shouldDumpAllExportedSymbols` only checks the leading comments of `rootNodes[0]`.
If the `@ExportModule` annotation happens to be placed on a later statement in the file, `VersioningValidator` will incorrectly treat all exported declarations as native contracts (applying `nativeApiMinVersion` constraints to them), but `AST.ts` will fail to actually dump them. This creates a mismatch where TypeScript code is strictly validated as a native API, but the corresponding native bindings are never actually generated.
`AST.ts` should align with the new `VersioningValidator` behavior by scanning all statements for the annotation.
Verification
Verified via:
Code inspection showing hasExportModuleAnnotation checks only rootNodes[0] in AST.ts while VersioningValidator checks statements.some(...).
Suggested fix:
| hasExportModuleAnnotation(rootNodes[0].leadingComments?.text ?? ''); | |
| const shouldDumpAllExportedSymbols = | |
| !!sourceFile.fileName.match(/\.vue\.ts(x)?$/g) || | |
| sourceFile.statements.some(statement => hasExportModuleAnnotation(getNodeComments(statement)?.text ?? '')); |
Help us improve our AI reviews using reactions: 👍 Helpful feedback • 👎 Not useful • 😕 Poor suggested fix
Regression test pinning this first-node-only behavior: #146 (compiler/companion/src/AST.spec.ts → "only treats @ExportModule as file-wide when it leads the first root node"). Captures current behavior; the fix here would make both detectors agree.
There was a problem hiding this comment.
Addressed locally in 2847fbe (push pending), but in the opposite direction from the suggestion: VersioningValidator now recognizes @ExportModule only on the first statement, matching dumpRootNodes. The Swift annotation manager already requires @ExportModule at the top of the file, so changing AST.ts to accept it anywhere would broaden the existing contract. Added a regression covering first-versus-later placement; all 460 companion tests and the full openai-xplat native-API metadata build pass.
| const bodyVersion = | ||
| this.nativeApiMinVersion === undefined | ||
| ? declaredVersion ?? currentVersion | ||
| : this.mergeVersions(currentVersion, declaredVersion); |
There was a problem hiding this comment.
Mirrored from the internal import review (unresolved static-analysis finding); drafted for triage.
🟡 Function bodies drop surrounding version guards when workspace minimum is unconfigured (bug · medium)
When nativeApiMinVersion is undefined, the function body's version drops back to its explicit declaredVersion because of the ?? operator (declaredVersion ?? currentVersion).
If a function with @Version(42) is declared inside an isVersionAtLeast(43) block, currentVersion (43) is completely ignored, causing the function body to falsely fail validation when using APIs requiring version 43.
The surrounding control flow guard provides a hard runtime guarantee regardless of the workspace baseline, so mergeVersions should be used unconditionally.
Agentic Prompt
Paste the following into your favorite agent to work on a fix for you
I'm not sure if the following code review comment is correct. Verify it and if it's valid, then fix it. If there are existing tests for the affected code, update or add test cases to cover the fix and prevent regression. In client/src/open_source/compiler/companion/src/VersioningValidator.ts, the issue is: When `nativeApiMinVersion` is undefined, the function body's version drops back to its explicit `declaredVersion` because of the `??` operator (`declaredVersion ?? currentVersion`).
If a function with `@Version(42)` is declared inside an `isVersionAtLeast(43)` block, `currentVersion` (43) is completely ignored, causing the function body to falsely fail validation when using APIs requiring version 43.
The surrounding control flow guard provides a hard runtime guarantee regardless of the workspace baseline, so `mergeVersions` should be used unconditionally.
Verification
Impact evidence:
Will cause false positive compiler errors that block compilation when developers correctly guard a lower-versioned function within a higher-version conditional check.
Suggested fix:
| : this.mergeVersions(currentVersion, declaredVersion); | |
| const bodyVersion = this.mergeVersions(currentVersion, declaredVersion); |
Help us improve our AI reviews using reactions: 👍 Helpful feedback • 👎 Not useful • 😕 Poor suggested fix
There was a problem hiding this comment.
Addressed by the same local fix in 2847fbe (pending push). The body version now unconditionally merges the enclosing version with the function's declared version, and the new regression covers @Version(42) nested inside isVersionAtLeast(43).
|
thanks @beaucollins will take a look at the rest of the comments |
3 participants
Description
This change adds support for runtime versioning. It is designed to allow TS code to safely drift apart from its underlying native runtime.
The key concepts are as follow:
@Version()and an arbitrary int versionisVersionAtLeast().isVersionAtLeast()Type of Change
Testing
bazel test //...)Testing Details
Checklist
Related Issues
Additional Context