Skip to content

Generate protobuf and offline wire artifacts from canonical UFC fixtures - #47

Open
typotter wants to merge 11 commits into
mainfrom
typo/rules-fixture-encodings
Open

typotter wants to merge 11 commits into
mainfrom
typo/rules-fixture-encodings

Conversation

@typotter

@typotter typotter commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

The canonical rules corpus is JSON, while browser offline initialization consumes a stringified ConfigurationWire containing protobuf rules. Fixture consumers need reproducible encodings tied to the unchanged source and evaluation cases.

Changes

  • Add a resource catalogue and local script that invokes the production converter, then serializes its protobuf with the public core SDK API.
  • Version raw protobuf, offline wire, provenance hashes and converted/omitted flag inventories in the fixture repository.
  • Gitignore ufc.generated.json, report.json and parity.json. Local validation checks JSON preservation when these diagnostics exist; clean-checkout validation does not require them.
  • Preserve unknown protobuf fields through the public wire serializer, using browser byte containers for content-equivalent round-trip checks.
  • Add integrity, repeat-generation and tooling regression checks, including SDK metadata projection and a deterministic evaluation clock. Evaluation mismatches are diagnostic and never determine which flags are encoded.

Overall flow and this PR

PR 1–4 in this diagram identify workstream slices, not GitHub PR numbers.

Orange marks the components introduced or changed by this PR. Other nodes provide end-to-end context. Double-bordered nodes are files; the three labelled local diagnostics are gitignored. Dashed delivery to the online rules endpoint is future work.

flowchart TB
  subgraph DD["External converter"]
    Converter("PR 1: local production-codec adapter<br/>Attempt every flag; warn and omit failures<br/>Compare generated JSON with original")
  end
  subgraph Fixtures["ffe-system-test-data"]
    UFC[["ufc-config.json<br/>Canonical source — unchanged"]]
    Cases[["evaluation-cases/*.json<br/>Original expectations — unchanged"]]
    Generate("PR 2: local generation script<br/>Invoke converter; use public wire serializer")
    PB[["ufc.pb<br/>Versioned raw rules protobuf"]]
    Wire[["configuration-wire.json<br/>Versioned offline wire text"]]
    Manifest[["manifest.json<br/>Hashes + converted/omitted flag keys"]]
    Parity[["parity.json<br/>Local / gitignored evaluation diagnostics"]]
    JSON[["ufc.generated.json<br/>Local / gitignored"]]
    Report[["report.json<br/>Local / gitignored<br/>Omissions + JSON comparison"]]
    Checks("PR 2: integrity and repeat-generation checks<br/>Parity reports differences; never selects flags")
    Precomputed[["PR 3: precomputed response fixtures<br/>Add required createdAt metadata<br/>Values and expectations unchanged"]]
  end
  UFC --> Converter
  Generate -. "invokes" .-> Converter
  Converter --> PB
  Converter --> JSON
  Converter --> Report
  PB --> Generate --> Wire
  Generate --> Manifest
  Generate --> Parity
  Cases --> Parity
  UFC --> Checks
  Manifest --> Checks
  PB --> Checks
  Wire --> Checks
  JSON -. "when present" .-> Checks
  Report -. "when present" .-> Checks
  subgraph FLEX["RUM FIT / FLEX"]
    Version("PR 4: browser SDK 2.0.0 / core 3.0.1")
    Load("PR 4: rules loader<br/>Verify hashes; retain original cases<br/>Explicit conversion-omission skips")
    Init("PR 4: InitProvider.configuration_wire<br/>Opaque serialized configuration via IPC")
    Provider("PR 4: public parser + DatadogCoreProvider<br/>Set configuration; await OpenFeature registration")
    Eval("PR 4: typed evaluation + context updates<br/>Normal OpenFeature client")
    OfflineProof("PR 4: lifecycle proof<br/>Browser network disabled before init<br/>All five types; zero attempted requests")
    Conformance("PR 4: full corpus runner<br/>Original value, reason and error assertions")
    FlagServer("Existing FlagsServer<br/>Serve precomputed responses verbatim")
    OnlineProvider("Existing online provider path<br/>Uses upgraded SDK in PR 4")
    Regression("Existing precomputed regression tests")
  end
  Wire --> Load
  Manifest --> Load
  Cases --> Load
  Load --> Init --> Provider --> Eval
  Version --> Provider
  Version --> OnlineProvider
  Eval --> OfflineProof
  Eval --> Conformance
  Precomputed --> FlagServer --> OnlineProvider --> Regression
  Future("Future: online rules endpoint<br/>and SDK rules fetch helper")
  PB -. "future delivery" .-> Future
  classDef context fill:#f3f4f6,stroke:#6b7280,color:#111827;
  classDef artifact fill:#dbeafe,stroke:#2563eb,stroke-width:2px,color:#111827;
  classDef target fill:#ffedd5,stroke:#ea580c,stroke-width:3px,color:#7c2d12;
  classDef future fill:#fff,stroke:#9ca3af,stroke-dasharray:5 5,color:#4b5563;
  class Converter,Generate,Checks,Load,Init,Provider,Eval,OfflineProof,Conformance,FlagServer,OnlineProvider,Regression,Version context;
  class UFC,Cases,PB,Wire,Manifest,Precomputed artifact;
  class JSON,Report,Parity context;
  class Future future;
  class Generate,PB,Wire,Manifest,Parity,JSON,Report,Checks target;
Loading

Decisions

  • JSON-to-protobuf evaluation disparities are allowed and tracked in the local parity report as progress towards parity. They never block generation, select passing flags or rewrite corpus expectations. Malformed diagnostic files fail integrity checks; well-formed reports containing mismatches do not.

  • Canonical UFC and authored evaluation cases stay unchanged. Conversion attempts every flag; no normalization, reason insertion, expectation rewrite or authored flag-selection list is applied.

  • Converter provenance is recorded as one opaque source digest. The source inventory remains local and is supplied separately during generation. This PR targets main independently of the timestamp fix in #46 and dependent-flags work.

  • Raw protobuf supports future online delivery; the wire file is the offline artifact. CI regeneration and converter distribution are deferred.

  • October 28 validation: generation, repeat-generation, integrity and schema checks plus eight Node tests pass. Protobuf and offline wire bytes are unchanged by the provenance update. All 63 current core flags are attempted; 45 convert and 18 are explicitly omitted. New fixture fields are preserved without normalisation.

  • Historical October 23 raw-core diagnostics exercised 343 cases and reported 282 differing evaluations; this count predates the metadata and evaluation-clock corrections. These remain permitted diagnostics, not a claim of corpus conformance. FLEX separately runs all 367 authored core cases, retaining missing-flag negative tests and explicitly reporting conversion omissions.

Checklist

  • JSON files are valid (no syntax errors)
  • New test cases reference existing flags in config/ufc-config.json
  • New flags have corresponding test cases in evaluation-cases/
  • Expected results match the flag configuration logic

Template checklist retained for review. This PR adds no flags or evaluation cases; the canonical file is currently at the repository root. JSON/artifact validation passed on October 28. Existing expectations remain unchanged, and protobuf conformance differences remain documented failures.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Integrity validation can accept incorrect flag inventories and malformed local diagnostics, weakening the artifact contract for consumers.

2 open issues
1 file not reviewed
  • tools/ufc/package-lock.json: Generated file
What changed in this PR

Adds reproducible protobuf and offline wire artifacts derived from the canonical UFC fixtures, without changing authored configurations or evaluation expectations.

Changes:

  • Adds a resource catalogue, production-converter source lock, and pinned SDK serialization tooling.
  • Records artifact hashes and converted/omitted flag inventories, with local evaluation diagnostics.
  • Adds integrity validation, regeneration comparisons, and tooling regression tests.
File Description
tools/​ufc/​validate-rules-schema.py Validates the resource catalogue schema.
tools/​ufc/​validate-rules-resources.mjs Checks artifact integrity and available diagnostics.
tools/​ufc/​rules-resources.test.mjs Adds tooling and integrity regression tests.
tools/​ufc/​README.md Documents generation, validation, and artifact contracts.
tools/​ufc/​parity.mjs Compares evaluations across configuration encodings.
tools/​ufc/​package.json Defines commands and pins the core SDK.
tools/​ufc/​package-lock.json Locks serializer dependencies.
tools/​ufc/​generate.mjs Converts fixtures, packages wire data, and records provenance.
tools/​ufc/​converter-lock.json Pins converter source revision and hashes.
schemas/​rules-resource-catalog.schema.json Defines the catalogue format.
rules-resources/​generated/​manifest.json Records hashes, provenance, and flag inventories.
rules-resources/​generated/​canonical-evaluator/​configuration-wire.json Provides serialized configuration for offline consumers.
rules-resources/​catalog.json Registers the canonical UFC resource.
.gitignore Excludes local dependencies and generated diagnostics.

Review effort: Balanced

Comment thread tools/ufc/validate-rules-resources.mjs Resolved
Comment thread tools/ufc/validate-rules-resources.mjs Resolved

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The promised versioned parity report conflicts with the implemented local-only diagnostic contract and needs to be reconciled.

No open issues 🎉

2 resolved since last review
1 file not reviewed
  • tools/ufc/package-lock.json: Generated file

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Cross-repository converter regeneration and SDK serialization round trips need human verification beyond the available static and hash checks.

No open issues 🎉

1 file not reviewed
  • tools/ufc/package-lock.json: Generated file

Review effort: Balanced

@typotter

Copy link
Copy Markdown
Contributor Author

Following up on the latest Copilot request for converter regeneration and SDK serialization verification: these were exercised locally for the current fixture revision.

  • Ran the merged production converter, repeated generation, and passed artifact integrity and catalogue schema validation.
  • All six fixture tooling regression tests passed, including preservation of unknown protobuf fields across Buffer/Uint8Array representations.
  • Loaded the committed ConfigurationWire through @datadog/openfeature-browser 2.0.0 and @datadog/flagging-core 3.0.1 in Chromium using FLEX #221's actual fixture pin, without fixture or SDK-source overrides.
  • The offline lifecycle test passed all five value types, context changes, reinitialisation and invalid configuration handling, with zero attempted requests after networking was disabled.

This proves generation and offline consumption, not full evaluator parity. The unchanged core corpus reports 64 passes, 283 failures and 20 explicit conversion omissions. Conversion and evaluation disparities remain diagnostic; generated JSON and reports stay gitignored. Authored UFC and expectations remain unchanged.

Please reassess the implementation in light of this runtime evidence; any remaining concrete concerns should remain open.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Source-provenance validation needs correction, and converter reproducibility and SDK round trips still require independent verification.

1 open issue
1 file not reviewed
  • tools/ufc/package-lock.json: Generated file

Review effort: Balanced

Comment thread tools/ufc/validate-rules-resources.mjs Resolved

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Hash integrity is verified, but production-converter regeneration and SDK round-trip behavior still need independent confirmation.

No open issues 🎉

1 resolved since last review
1 file not reviewed
  • tools/ufc/package-lock.json: Generated file

Review effort: Balanced

@typotter
typotter requested a review from sameerank October 26, 2026 17:15
@typotter
typotter marked this pull request as ready for review October 26, 2026 17:15
@typotter
typotter requested a review from a team as a code owner October 26, 2026 17:15
Comment thread tools/ufc/parity.mjs Resolved
Comment thread tools/ufc/parity.mjs Resolved

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The parity comparator reports false failures when absent serial IDs are compared with explicit null expectations.

1 open issue
1 file not reviewed
  • tools/ufc/package-lock.json: Generated file

Review effort: Balanced

Comment thread tools/ufc/parity.mjs Resolved

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

External-converter reproducibility and SDK round-trip behavior still require human verification.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Prevent clock assertion errors from being discarded

tools/​ufc/​rules-resources.test.mjs:151

parity() catches evaluation exceptions and records them as { encoding, error }, but this filter discards those entries. A failed clock assertion in the flags getter can therefore be ignored after observed has incremented, letting the regression pass without verifying clock behavior. Assert that there are no error-bearing mismatches before filtering for metadata, then regenerate the manifest's tooling hashes.

1 resolved since last review
1 file not reviewed
  • tools/ufc/package-lock.json: Generated file

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

End-to-end regeneration and SDK round-trip behavior still require verification with the external converter and pinned dependencies.

No open issues 🎉

1 file not reviewed
  • tools/ufc/package-lock.json: Generated file

Review effort: Balanced

@typotter
typotter requested a review from sameerank October 27, 2026 21:59
Comment thread tools/ufc/generate.mjs Outdated
await fs.mkdir(output);
try {
const manifest = { schemaVersion: 1, catalogSha256: sha256(catalogBytes), converter: { ...identity, source: sourceIdentity }, generatorInputs: {}, serializer: '@datadog/flagging-core@3.0.1', resources: [] };
for (const directory of ['tools/ufc', 'schemas', 'evaluation-cases']) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Comment from Claude working with sameerank:

[P1] Hash only the files that determine the committed bytes. The current scope makes the manifest stale on routine edits, including changes already on main.

This loop hashes every file under tools/ufc/, schemas/ and evaluation-cases/, except node_modules. validate-rules-resources.mjs then requires each hash and the exact file set. But ufc.pb and configuration-wire.json depend only on ufc-config.json, the converter, generate.mjs and the serializer pin. Evaluation cases feed only the gitignored parity.json.

main (#56) already changed three evaluation-case files. After a merge of main, validation fails, although none of these edits affects the protobuf or wire bytes. Regeneration needs the converter binary, its expected digest and a matching source checkout. The same failure happens after a precomputed-schema edit, a README edit, a parity.mjs edit, or a stray .DS_Store in one of these directories.

Reproduction at e3809e9
$ git merge origin/main        # 1ae3eda, clean merge
$ node tools/ufc/validate-rules-resources.mjs
evaluation-cases/test-case-disabled-flag.json changed: regenerate rules resources

$ git reset --hard e3809e9 && touch tools/ufc/.DS_Store
$ node tools/ufc/validate-rules-resources.mjs
Generator input membership changed
Suggested patch (tested)
diff --git a/tools/ufc/generate.mjs b/tools/ufc/generate.mjs
index bf2857a..e3d1d6c 100644
--- a/tools/ufc/generate.mjs
+++ b/tools/ufc/generate.mjs
@@ -13,6 +13,8 @@ export const root = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '
 export const sha256 = data => createHash('sha256').update(data).digest('hex');
 const json = value => JSON.stringify(value, null, 2) + '\n';
 const stable = value => JSON.stringify(value, (_, item) => item && typeof item === 'object' && !Array.isArray(item) ? Object.fromEntries(Object.entries(item).sort(([a], [b]) => a.localeCompare(b))) : item);
+// Only files that determine committed protobuf/wire bytes; evaluation cases feed local diagnostics only.
+export const generatorInputFiles = ['tools/ufc/converter-lock.json', 'tools/ufc/generate.mjs', 'tools/ufc/package-lock.json', 'tools/ufc/package.json'];
 export async function sourcePath(relative, base = root) {
   if (typeof relative !== 'string' || path.isAbsolute(relative) || relative.split(/[\\/]/).includes('..')) throw new Error(`Unsafe source path: ${relative}`);
   const resolved = await fs.realpath(path.join(base, relative));
@@ -91,13 +93,7 @@ export async function generate({ converter, converterSha256, converterSource, ch
   await fs.mkdir(output);
   try {
     const manifest = { schemaVersion: 1, catalogSha256: sha256(catalogBytes), converter: { ...identity, source: sourceIdentity }, generatorInputs: {}, serializer: '@datadog/flagging-core@3.0.1', resources: [] };
-    for (const directory of ['tools/ufc', 'schemas', 'evaluation-cases']) {
-      for (const relative of await filesUnder(path.join(base, directory), true)) {
-        if (relative.startsWith('node_modules/')) continue;
-        const file = `${directory}/${relative}`;
-        manifest.generatorInputs[file] = sha256(await fs.readFile(await sourcePath(file, base)));
-      }
-    }
+    for (const file of generatorInputFiles) manifest.generatorInputs[file] = sha256(await fs.readFile(await sourcePath(file, base)));
     const seen = new Set();
     for (const resource of catalog.resources) {
       if (!/^[a-z0-9]+(?:-[a-z0-9]+)*$/.test(resource.id) || seen.has(resource.id)) throw new Error(`Invalid or duplicate resource id: ${resource.id}`);
diff --git a/tools/ufc/validate-rules-resources.mjs b/tools/ufc/validate-rules-resources.mjs
index bd59915..e8fdc6f 100644
--- a/tools/ufc/validate-rules-resources.mjs
+++ b/tools/ufc/validate-rules-resources.mjs
@@ -4,7 +4,7 @@ import assert from 'node:assert/strict';
 import { promises as fs } from 'node:fs';
 import path from 'node:path';
 import { fileURLToPath } from 'node:url';
-import { root, sha256, packageWire, sourcePath } from './generate.mjs';
+import { root, sha256, packageWire, sourcePath, generatorInputFiles } from './generate.mjs';
 import { parity } from './parity.mjs';
 const { configurationFromRulesBinary } = createRequire(import.meta.url)('@datadog/flagging-core/rules-based');
 export async function validate(base = root) {
@@ -15,20 +15,7 @@ export async function validate(base = root) {
   for (const [relative, expected] of Object.entries(manifest.generatorInputs)) {
     assert.equal(sha256(await fs.readFile(await sourcePath(relative, base))), expected, `${relative} changed: regenerate rules resources`);
   }
-  // Detect additions/deletions as well as changes to recorded generator inputs.
-  async function inputFiles(directory, prefix) {
-    const result = [];
-    for (const entry of await fs.readdir(directory, { withFileTypes: true })) {
-      if (entry.name === 'node_modules') continue;
-      const relative = `${prefix}/${entry.name}`;
-      assert.ok(!entry.isSymbolicLink(), `Input symlink forbidden: ${relative}`);
-      if (entry.isDirectory()) result.push(...await inputFiles(path.join(directory, entry.name), relative));
-      else result.push(relative);
-    }
-    return result;
-  }
-  const expectedInputs = [...new Set(catalog.resources.map(x => x.source))];
-  for (const directory of ['tools/ufc', 'schemas', 'evaluation-cases']) expectedInputs.push(...await inputFiles(path.join(base, directory), directory));
+  const expectedInputs = [...new Set([...catalog.resources.map(x => x.source), ...generatorInputFiles])];
   assert.deepEqual(Object.keys(manifest.generatorInputs).sort(), expectedInputs.sort(), 'Generator input membership changed');
   assert.deepEqual(manifest.converter.source, JSON.parse(await fs.readFile(path.join(base, 'tools/ufc/converter-lock.json'))));
   const expectedFiles = ['manifest.json'];

Also update tools/ufc/README.md:35, which says the manifest hashes "tooling, schemas, cases". Then regenerate the manifest.

I applied this patch and recomputed the generatorInputs hashes. I did not run the converter. Results:

  • All 7 tests pass, and the validator passes.
  • After a merge of origin/main, the validator still passes. It also passes with a stray tools/ufc/.DS_Store.
  • Positive controls still fail: an edit to generate.mjs or ufc-config.json gives "changed: regenerate rules resources".

catalogSha256 and sourceSha256 still cover the catalogue and the source. I found no reader of these hashes in this repository other than the generator, the validator and the tests. If the downstream loader reads generatorInputs, please check it before you apply this change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Narrowed the manifest to artifact-producing inputs in 3171711, retaining catalogue, source and artifact integrity checks. Case, README, schema and stray-file changes no longer require artifact regeneration; generator and source changes still invalidate it. Updated the companion FLEX loader to discover cases independently of manifest hashes. All 9 tooling tests and 25 loader tests pass; protobuf/wire bytes are unchanged.

🤖written by a bot, edited by a human

Comment thread tools/ufc/README.md
wire string itself, without extra JSON quoting. Public pinned
`@datadog/flagging-core@3.0.1` APIs decode protobuf, serialize wire, and parse it
again. Parser failure, empty configurations, changed semantic payload and unstable
wire serialization fail generation. No timestamps, ETags or split reasons are added.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Comment from Claude working with sameerank:

[P2] Please state that protobuf and wire evaluation reports reason: "UNKNOWN" for every evaluation that selects a split. Consider a manifest field for it too, so that consumers can skip reason assertions explicitly.

This sentence is correct, but a consumer cannot infer its effect. No split in ufc-config.json has a reason, so every split in ufc.pb has reason 0. The @datadog/flagging-core 3.0.1 protobuf evaluator maps that value to "UNKNOWN". The JSON evaluator derives the reason, so the original JSON passes the same reason assertions.

This is also most of the parity count: 273 of the 282 differing cases differ only in reason.

Parity breakdown at e3809e9

I ran parity() on the committed artifacts: 343 cases exercised, 282 failures.

Mismatch (binary and wire each) Cases
reason expected TARGETING_MATCH / SPLIT / STATIC / DEFAULT / ERROR, actual UNKNOWN 279
reason expected ERROR, actual DEFAULT 3
errorCode (expected PARSE_ERROR) 6
value 3
features (fails on all four encodings) 3

273 cases fail only on reason. In 3 cases the only other mismatch is features, which the SDK does not return. The remaining 6 cases are malformed flags that convert, but they lose the expected PARSE_ERROR.

The SDK code is resolutionReason(split) in cjs/evaluation/evaluateProtobufConfiguration.js. It maps split.reason 1–4 to TARGETING_MATCH, SPLIT, STATIC and DEFAULT, and returns 'UNKNOWN' for any other value.

Suggested wording
No timestamps, ETags or split reasons are added. The canonical splits have no
`reason`, so protobuf and wire evaluation reports reason `UNKNOWN` for every
evaluation that selects a split. Consumers cannot assert the canonical reason for
those results against these artifacts.

A manifest field, for example "splitReasons": "absent", would let a loader skip reason assertions without hard-coding this knowledge. A longer-term option is for the converter to derive the reasons the way the JSON evaluator does.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified reason preservation and the missing-reason limitation in 3171711. The converter already preserves all four supported reasons supplied in JSON; I verified each through conversion and SDK protobuf decoding. A missing reason becomes REASON_UNSPECIFIED (0), which the @datadog/flagging-core 3.0.1 protobuf evaluator maps to UNKNOWN. We retain the canonical reason expectations and report the resulting UNKNOWN differences without inventing missing reasons.

🤖written by a bot, edited by a human

Comment thread tools/ufc/parity.mjs Outdated
const actual = { ...sdkResult };
actual.serialId = sdkResult.flagMetadata?.__dd_split_serial_id ?? null;
actual.doLog = sdkResult.flagMetadata?.doLog;
for (const [field,expected] of Object.entries(test.result)) if (!isDeepStrictEqual(actual[field],expected)) mismatches.push({encoding,field,expected,actual:actual[field] ?? null});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Comment from Claude working with sameerank:

[P2] Compare only fields that the SDK returns, and do not count missing-flag cases as conversion omissions.

  • This line compares every key under result. Four cases in test-case-split-features.json expect features, but @datadog/flagging-core 3.0.1 does not return that field. In the 3 exercised cases, features fails on all four encodings, including the original JSON.
  • Line 33 adds every flag that is not in the binary to omittedCases. That includes flags that are not in the source at all: flag-that-does-not-exist (3 cases) and unknown-opt-out-flag (1 case). So the FLAG_NOT_FOUND path is never exercised through binary and wire.
Suggested patch (tested)
diff --git a/tools/ufc/parity.mjs b/tools/ufc/parity.mjs
index 940b55b..7a0825e 100644
--- a/tools/ufc/parity.mjs
+++ b/tools/ufc/parity.mjs
@@ -8,6 +8,8 @@ const { evaluate, evaluateRulesBasedConfiguration } = require('@datadog/flagging
 const { configurationFromRulesBinary, configurationFromString } = require('@datadog/flagging-core/rules-based');
 const types = { BOOLEAN: 'boolean', INTEGER: 'number', NUMERIC: 'number', STRING: 'string', JSON: 'object' };
 const logger = { debug() {}, info() {}, warn() {}, error() {} };
+// flagging-core 3.0.1 does not expose split features; compare only fields the SDK returns.
+const sdkFields = new Set(['value', 'reason', 'errorCode', 'serialId', 'doLog']);
 export function parity(root, binary, wire, generated, source) {
   const configs = { binary: configurationFromRulesBinary(binary), wire: configurationFromString(wire) };
   const cases = readdirSync(resolve(root, 'evaluation-cases')).filter(x => x.endsWith('.json')).sort().flatMap(file => JSON.parse(readFileSync(resolve(root, 'evaluation-cases', file))).map((value,index) => ({id:`evaluation-cases/${file}#${index}`,value})));
@@ -30,7 +32,7 @@ export function parity(root, binary, wire, generated, source) {
       static now() { return fixedTime; }
     };
     for (const {id,value:test} of cases) {
-      if (!included.has(test.flag)) { omittedCases.push({id,flag:test.flag}); continue; }
+      if (!included.has(test.flag) && Object.hasOwn(source.flags, test.flag)) { omittedCases.push({id,flag:test.flag}); continue; }
       const context = {...test.attributes,targetingKey:test.targetingKey};
       const calls = {
         original: () => evaluateRulesBasedConfiguration(source,types[test.variationType],test.flag,test.defaultValue,context,logger),
@@ -47,7 +49,7 @@ export function parity(root, binary, wire, generated, source) {
           const actual = { ...sdkResult };
           actual.serialId = sdkResult.flagMetadata?.__dd_split_serial_id ?? null;
           actual.doLog = sdkResult.flagMetadata?.doLog;
-          for (const [field,expected] of Object.entries(test.result)) if (!isDeepStrictEqual(actual[field],expected)) mismatches.push({encoding,field,expected,actual:actual[field] ?? null});
+          for (const [field,expected] of Object.entries(test.result)) if (sdkFields.has(field) && !isDeepStrictEqual(actual[field],expected)) mismatches.push({encoding,field,expected,actual:actual[field] ?? null});
         } catch(error) { mismatches.push({encoding,error:error.message}); }
       }
       results.push({id,flag:test.flag,passed:!mismatches.length,mismatches});

Results with this patch:

  • Exercised cases: 343 → 347. Omitted cases: 24 → 20.
  • The 4 missing-flag cases pass on all four encodings.
  • No case has a mismatch on the original JSON any more. Before the patch, 3 cases had one, from features.
  • The 3 features cases now fail only on reason, on binary and wire.

parity.mjs is currently a hashed generator input, so this edit also needs a manifest regeneration. With the input-scope change from the generate.mjs comment, it does not. With this patch, the input-scope change and the new sourcePath test applied, 8/8 tests and the validator pass.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied the supported-field projection and missing-key distinction in 3171711. Deliberately absent keys now run across all four encodings; only source flags omitted by conversion are excluded. The report exercises 347 cases and omits 20. Added regression coverage and documented that features are outside this evaluator API's observable result fields. All 9 tooling tests pass; corpus expectations are unchanged.

🤖written by a bot, edited by a human

Comment thread tools/ufc/rules-resources.test.mjs Outdated
Comment on lines +90 to +93
test('source paths cannot leave repository', async () => {
await assert.rejects(sourcePath('../ufc-config.json'));
await assert.rejects(sourcePath('/etc/passwd'));
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Comment from Claude working with sameerank:

[P3] This test passes even when the guard it tests is removed. Please probe with paths that exist, and match the error message.

With the line-17 guard removed, both probes still reject, because fs.realpath cannot find the path. ../ufc-config.json does not exist above the repository root, and path.join(base, '/etc/passwd') resolves inside the repository. When I deleted the traversal check at generate.mjs:17, this test still passed. No existing test exercises the symlink-escape rejection at generate.mjs:19.

Suggested test (tested)
test('source paths reject traversal, absolute paths and symlink escapes', async () => temporary(async directory => {
  const base = path.join(directory, 'repo');
  await fs.mkdir(base);
  await fs.writeFile(path.join(directory, 'outside.json'), '{}');
  await fs.writeFile(path.join(base, 'inside.json'), '{}');
  await fs.symlink(path.join(directory, 'outside.json'), path.join(base, 'link.json'));
  // Match guard messages so filesystem errors cannot satisfy these assertions.
  await assert.rejects(sourcePath('../outside.json', base), /Unsafe source path/);
  await assert.rejects(sourcePath(path.join(base, 'inside.json'), base), /Unsafe source path/);
  await assert.rejects(sourcePath('link.json', base), /Source escapes repository/);
  assert.equal(await sourcePath('inside.json', base), await fs.realpath(path.join(base, 'inside.json')));
}));

Results:

  • It passes on the PR code.
  • With generate.mjs:17 deleted, it fails, and the existing test still passes.
  • With generate.mjs:19 deleted, it also fails.

rules-resources.test.mjs is currently a hashed generator input, so a new test also needs a manifest regeneration, unless you narrow the input scope first.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added existing-file traversal and absolute-path checks, symlink-escape coverage, and valid-path resolution in 3171711. Each rejection asserts the intended guard error. Removing either guard independently makes the test fail; after restoring both guards, all 9 tooling tests pass.

🤖written by a bot, edited by a human

@typotter
typotter requested a review from sameerank October 30, 2026 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants