SK-3037 run contract tests for flowvault, and gate on the public API only

.github/workflows/contract-tests.yml

@@ -5,12 +5,24 @@ on:
     branches:
       - main
       - release/*
+      - flowvault-release/*
 
 jobs:
   contract-tests:
-    name: Contract Tests
+    # One job per module so a break in one is reported against that module by name,
+    # and both still run even when the other fails.
+    name: Contract Tests (${{ matrix.module }})
     runs-on: ubuntu-latest
 
+    strategy:
+      fail-fast: false
+      matrix:
+        include:
+          - module: skyvault
+            artifact: skyflow-java
+          - module: flowvault
+            artifact: skyflow-flowvault-java
+
     permissions:
       contents: read
       pull-requests: write
@@ -29,29 +41,31 @@ jobs:
           cache: 'maven'
 
       - name: Verify API surface snapshot
-        run: mvn -B install -pl common,skyvault -am -DskipTests -Dmaven.javadoc.skip=true -Dgpg.skip=true
+        run: mvn -B install -pl common,${{ matrix.module }} -am -DskipTests -Dmaven.javadoc.skip=true -Dgpg.skip=true
 
       - name: Show API surface diff
         if: failure()
         run: |
-          echo "### API surface changes detected ###"
-          echo "See skyvault/target/japicmp/default-cli.diff for the full comparison against skyvault/api-report/skyflow-java.baseline.jar."
-          echo "If this change is intentional, run scripts/contract-snapshot-update.sh and commit the updated baseline jar."
+          echo "### API surface changes detected in ${{ matrix.module }} ###"
+          echo "Compared against ${{ matrix.module }}/api-report/${{ matrix.artifact }}.baseline.jar."
+          echo "If this change is intentional, run:"
+          echo "    scripts/contract-snapshot-update.sh ${{ matrix.module }}"
+          echo "and commit the updated baseline jar."
           echo ""
-          cat skyvault/target/japicmp/default-cli.diff || true
+          cat ${{ matrix.module }}/target/japicmp/default-cli.diff || true
 
       - name: Upload API surface diff on failure
         if: failure()
         uses: actions/upload-artifact@v4
         with:
-          name: api-surface-diff
-          path: skyvault/target/japicmp/**
+          name: api-surface-diff-${{ matrix.module }}
+          path: ${{ matrix.module }}/target/japicmp/**
           retention-days: 7
 
       # The step above only shows a diff when the CURRENT build differs from the
       # committed baseline - once someone runs contract-snapshot-update.sh and
       # commits the refreshed baseline jar, that check goes green and shows nothing.
-      # A reviewer looking at a green PR that touches api-report/skyflow-java.baseline.jar
+      # A reviewer looking at a green PR that touches api-report/*.baseline.jar
       # (a binary file) would otherwise have no way to see WHAT was just approved as
       # the new contract. These steps explicitly diff the OLD committed baseline
       # (from the PR's base branch) against the NEW committed baseline (from this PR)
@@ -61,40 +75,74 @@ jobs:
         if: always() && github.event.pull_request
         run: |
           git fetch origin "${{ github.event.pull_request.base.ref }}" --depth=1
-          if git diff --name-only "origin/${{ github.event.pull_request.base.ref }}" HEAD -- skyvault/api-report/skyflow-java.baseline.jar | grep -q .; then
+          BASELINE="${{ matrix.module }}/api-report/${{ matrix.artifact }}.baseline.jar"
+          if ! git diff --name-only "origin/${{ github.event.pull_request.base.ref }}" HEAD -- "$BASELINE" | grep -q .; then
+            echo "changed=false" >> "$GITHUB_OUTPUT"
+          elif git cat-file -e "origin/${{ github.event.pull_request.base.ref }}:$BASELINE" 2>/dev/null; then
             echo "changed=true" >> "$GITHUB_OUTPUT"
           else
-            echo "changed=false" >> "$GITHUB_OUTPUT"
+            # Added by this PR rather than modified: the module is getting its
+            # first baseline. git diff reports an addition as a change, but there
+            # is no old snapshot to `git show`, so a plain "true" here would send
+            # the next step into `git show <base>:<path>` and exit 128.
+            echo "changed=new" >> "$GITHUB_OUTPUT"
           fi
 
       - name: Diff old vs new contract baseline
-        if: always() && steps.baseline-diff-check.outputs.changed == 'true'
+        if: always() && (steps.baseline-diff-check.outputs.changed == 'true' || steps.baseline-diff-check.outputs.changed == 'new')
         run: |
+          BASELINE="${{ matrix.module }}/api-report/${{ matrix.artifact }}.baseline.jar"
+
+          if [ "${{ steps.baseline-diff-check.outputs.changed }}" = "new" ]; then
+            {
+              echo "\`$BASELINE\` is **new in this PR** - \`${{ matrix.module }}\` had no committed baseline before, so there is nothing to diff against."
+              echo ""
+              echo "This snapshot becomes the approved contract: every later PR is compared against it, and any incompatible change fails the \`Contract Tests (${{ matrix.module }})\` job until someone regenerates it deliberately. Review it as the starting point, not as a change."
+            } > /tmp/contract-baseline-diff.md
+            cat /tmp/contract-baseline-diff.md
+            exit 0
+          fi
+
           curl -sL -o /tmp/japicmp-cli.jar "https://repo.maven.apache.org/maven2/com/github/siom79/japicmp/japicmp/0.26.0/japicmp-0.26.0-jar-with-dependencies.jar"
-          mvn -q -B dependency:build-classpath -pl skyvault -Dmdep.outputFile=/tmp/skyvault-classpath.txt -Dmaven.javadoc.skip=true -Dgpg.skip=true
-          git show "origin/${{ github.event.pull_request.base.ref }}:skyvault/api-report/skyflow-java.baseline.jar" > /tmp/old-baseline.jar
+          mvn -q -B dependency:build-classpath -pl ${{ matrix.module }} -Dmdep.outputFile=/tmp/module-classpath.txt -Dmaven.javadoc.skip=true -Dgpg.skip=true
+          git show "origin/${{ github.event.pull_request.base.ref }}:$BASELINE" > /tmp/old-baseline.jar
 
+          # Same allowlist the poms gate on, so the comment shows the contract and
+          # nothing else. Keep these in sync with the <includes> in the module poms.
           java -jar /tmp/japicmp-cli.jar \
             -o /tmp/old-baseline.jar \
-            -n skyvault/api-report/skyflow-java.baseline.jar \
+            -n "$BASELINE" \
             -a protected \
-            -e "com.skyflow.generated.*;com.skyflow.utils.*" \
-            --old-classpath "$(cat /tmp/skyvault-classpath.txt)" \
-            --new-classpath "$(cat /tmp/skyvault-classpath.txt)" \
+            -i "com.skyflow.Skyflow;com.skyflow.config;com.skyflow.enums;com.skyflow.errors;com.skyflow.serviceaccount.util;com.skyflow.vault.audit;com.skyflow.vault.bin;com.skyflow.vault.connection;com.skyflow.vault.controller;com.skyflow.vault.data;com.skyflow.vault.detect;com.skyflow.vault.tokens" \
+            --old-classpath "$(cat /tmp/module-classpath.txt)" \
+            --new-classpath "$(cat /tmp/module-classpath.txt)" \
             -m \
+            --ignore-missing-classes \
             --markdown > /tmp/contract-baseline-diff.md || true
 
           cat /tmp/contract-baseline-diff.md
 
       - name: Comment contract baseline change on PR
-        if: always() && steps.baseline-diff-check.outputs.changed == 'true'
+        if: always() && (steps.baseline-diff-check.outputs.changed == 'true' || steps.baseline-diff-check.outputs.changed == 'new')
         uses: actions/github-script@v7
+        env:
+          BASELINE_STATE: ${{ steps.baseline-diff-check.outputs.changed }}
         with:
           script: |
             const fs = require('fs');
+            const module = '${{ matrix.module }}';
+            const artifact = '${{ matrix.artifact }}';
             const summary = fs.readFileSync('/tmp/contract-baseline-diff.md', 'utf8');
-            const marker = '<!-- contract-baseline-diff -->';
-            const body = `${marker}\n## Contract baseline change detected\n\nThis PR updates \`skyvault/api-report/skyflow-java.baseline.jar\` (the approved public API contract). Here is exactly what it changes, comparing the baseline on \`${{ github.event.pull_request.base.ref }}\` against the baseline committed in this PR:\n\n${summary}`;
+            // per-module marker so the two matrix jobs update their own comment
+            const marker = `<!-- contract-baseline-diff:${module} -->`;
+            const isNew = process.env.BASELINE_STATE === 'new';
+            const heading = isNew
+              ? `## Contract baseline added (\`${module}\`)`
+              : `## Contract baseline change detected (\`${module}\`)`;
+            const preamble = isNew
+              ? `This PR adds \`${module}/api-report/${artifact}.baseline.jar\`, the approved public API contract for this module.`
+              : `This PR updates \`${module}/api-report/${artifact}.baseline.jar\` (the approved public API contract). Here is exactly what it changes, comparing the baseline on \`${{ github.event.pull_request.base.ref }}\` against the baseline committed in this PR:`;
+            const body = `${marker}\n${heading}\n\n${preamble}\n\n${summary}`;
             const { data: comments } = await github.rest.issues.listComments({
               owner: context.repo.owner,
               repo: context.repo.repo,

flowvault/pom.xml

@@ -77,6 +77,96 @@
                     </execution>
                 </executions>
             </plugin>
+            <plugin>
+                <!-- Comparison-only jar (classifier "with-common") merging com.skyflow:common,
+                     mirroring skyvault. The published artifact is untouched: shadedArtifactAttached
+                     keeps the main jar as it was. This exists so japicmp compares the whole surface
+                     a consumer actually gets, since classes a customer imports (Credentials,
+                     SkyflowException, BearerToken) live in common and would otherwise read as
+                     missing - japicmp only sees what is physically inside the jars it is given. -->
+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-shade-plugin</artifactId>
+                <version>3.6.0</version>
+                <executions>
+                    <execution>
+                        <id>shade-for-japicmp</id>
+                        <phase>package</phase>
+                        <goals>
+                            <goal>shade</goal>
+                        </goals>
+                        <configuration>
+                            <shadedArtifactAttached>true</shadedArtifactAttached>
+                            <shadedClassifierName>with-common</shadedClassifierName>
+                            <artifactSet>
+                                <includes>
+                                    <include>com.skyflow:common</include>
+                                </includes>
+                            </artifactSet>
+                        </configuration>
+                    </execution>
+                </executions>
+            </plugin>
+            <plugin>
+                <groupId>com.github.siom79.japicmp</groupId>
+                <artifactId>japicmp-maven-plugin</artifactId>
+                <version>0.26.0</version>
+                <configuration>
+                    <!-- No published version is pulled. "Old" is the committed contract snapshot,
+                         a jar built from the last code state a human deliberately approved as the
+                         contract. Regenerate with scripts/contract-snapshot-update.sh flowvault
+                         after an intentional public API change, then commit the updated jar. -->
+                    <oldVersion>
+                        <file>
+                            <path>${project.basedir}/api-report/skyflow-flowvault-java.baseline.jar</path>
+                        </file>
+                    </oldVersion>
+                    <newVersion>
+                        <file>
+                            <path>${project.build.directory}/${project.build.finalName}-with-common.jar</path>
+                        </file>
+                    </newVersion>
+                    <parameter>
+                        <accessModifier>protected</accessModifier>
+                        <onlyModified>true</onlyModified>
+                        <ignoreMissingClasses>true</ignoreMissingClasses>
+                        <breakBuildOnBinaryIncompatibleModifications>true</breakBuildOnBinaryIncompatibleModifications>
+                        <breakBuildOnSourceIncompatibleModifications>true</breakBuildOnSourceIncompatibleModifications>
+                        <!-- An allowlist, not a blocklist: the contract is exactly what a customer
+                             imports. Everything unlisted - com.skyflow.generated.*, com.skyflow.logs,
+                             com.skyflow.utils.* - is internal and free to change without a baseline
+                             update. Note japicmp compares signatures only, never method bodies, so
+                             the controller entries below gate its method signatures, not how they
+                             are implemented.
+
+                             com.skyflow.VaultClient is deliberately NOT listed. Every one of its
+                             members is protected - generated-client getters, HTTP config, bearer-token
+                             refresh - so it has no public API, and it only ever showed up here
+                             because accessModifier=protected sweeps protected members in. Its
+                             subclass (VaultController) is final, so no customer can subclass their
+                             way to a protected member either. Please do not add it back. -->
+                        <includes>
+                            <include>com.skyflow.Skyflow</include>
+                            <include>com.skyflow.config</include>
+                            <include>com.skyflow.enums</include>
+                            <include>com.skyflow.errors</include>
+                            <include>com.skyflow.serviceaccount.util</include>
+                            <include>com.skyflow.vault.controller</include>
+                            <include>com.skyflow.vault.data</include>
+                        </includes>
+                        <skipXmlReport>false</skipXmlReport>
+                        <skipHtmlReport>false</skipHtmlReport>
+                    </parameter>
+                </configuration>
+                <executions>
+                    <execution>
+                        <id>default-cli</id>
+                        <phase>verify</phase>
+                        <goals>
+                            <goal>cmp</goal>
+                        </goals>
+                    </execution>
+                </executions>
+            </plugin>
         </plugins>
     </build>
 

flowvault/src/test/java/com/skyflow/vault/data/RequestContextTests.java

@@ -1,11 +1,15 @@
 package com.skyflow.vault.data;
 
 import com.skyflow.enums.CustomHeaderKey;
+
+import java.util.Map;
 import org.junit.Assert;
 import org.junit.Test;
 
 /**
- * Batch position on the interceptor context. Without it every batch of a bulk call presents an
+ * The interceptor context: its operation, its custom headers, and the batch position.
+ *
+ * <p>Batch position matters without it every batch of a bulk call presents an
  * identical context, so a caller cannot tag them apart — no per-batch correlation id, no
  * "batch 3 of 12" logging.
  */
@@ -53,4 +57,63 @@ public void testHeadersRemainUnmodifiable() {
             Assert.assertTrue(true);
         }
     }
+
+    // ── operation and headers (moved here with the class, from common) ──────────
+    @Test
+    public void testGetOperationReturnsConstructorValue() {
+        RequestContext context = new RequestContext("INSERT");
+
+        Assert.assertEquals("INSERT", context.getOperation());
+    }
+
+    @Test
+    public void testNullOperation() {
+        RequestContext context = new RequestContext(null);
+
+        Assert.assertNull(context.getOperation());
+    }
+
+    @Test
+    public void testGetHeadersReturnsEmptyMapByDefault() {
+        RequestContext context = new RequestContext("INSERT");
+
+        Assert.assertTrue(context.getHeaders().isEmpty());
+    }
+
+    @Test
+    public void testAddHeaderIsReflectedInGetHeaders() {
+        RequestContext context = new RequestContext("INSERT");
+        context.addHeader(CustomHeaderKey.SKYFLOW_ACCOUNT_ID, "account-id-value");
+
+        Map<CustomHeaderKey, String> headers = context.getHeaders();
+
+        Assert.assertEquals(1, headers.size());
+        Assert.assertEquals("account-id-value", headers.get(CustomHeaderKey.SKYFLOW_ACCOUNT_ID));
+    }
+
+    @Test
+    public void testAddHeaderOverwritesExistingValueForSameKey() {
+        RequestContext context = new RequestContext("INSERT");
+        context.addHeader(CustomHeaderKey.SKYFLOW_ACCOUNT_ID, "first-value");
+        context.addHeader(CustomHeaderKey.SKYFLOW_ACCOUNT_ID, "second-value");
+
+        Assert.assertEquals(1, context.getHeaders().size());
+        Assert.assertEquals("second-value", context.getHeaders().get(CustomHeaderKey.SKYFLOW_ACCOUNT_ID));
+    }
+
+    @Test
+    public void testAddMultipleDistinctHeaders() {
+        RequestContext context = new RequestContext("DETOKENIZE");
+        context.addHeader(CustomHeaderKey.SKYFLOW_ACCOUNT_ID, "account-id-value");
+        context.addHeader(CustomHeaderKey.SKYFLOW_ACCOUNT_NAME, "account-name-value");
+
+        Assert.assertEquals(2, context.getHeaders().size());
+    }
+
+    @Test(expected = UnsupportedOperationException.class)
+    public void testGetHeadersReturnsUnmodifiableMap() {
+        RequestContext context = new RequestContext("INSERT");
+
+        context.getHeaders().put(CustomHeaderKey.REQUEST_ID_HEADER, "request-id-value");
+    }
 }