feat: Harden Hashing Consistency - #2011
Open
NeaguGeorgiana23 wants to merge 3 commits into
Open
Conversation
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
dosubot
Bot
added
the
size:L
✅ Deploy Preview for polite-licorice-3db33c canceled.
|
📝 WalkthroughWalkthroughFractional evaluation now validates explicit and implicit hashing inputs, normalizes values recursively, encodes them with deterministic CBOR, and hashes the bytes with Murmur3. Distribution parsing and weighted variant selection remain unchanged. Unit test expectations are updated to match the new hashing results. Go module dependencies for CBOR are added, and integration tests exclude fractional v2 scenarios. ChangesFractional evaluation hashing
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Evaluate
participant InputParser
participant ValueNormalizer
participant DeterministicCBOR
participant Murmur3
participant DistributionSelector
Evaluate->>InputParser: Parse explicit or [flagKey, targetingKey] input
InputParser->>ValueNormalizer: Normalize validated values
ValueNormalizer->>DeterministicCBOR: Encode normalized values
DeterministicCBOR->>Murmur3: Hash encoded bytes
Murmur3-->>Evaluate: Return 32-bit hash
Evaluate->>DistributionSelector: Select weighted variant
DistributionSelector-->>Evaluate: Return variant or nil
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/pkg/evaluator/fractional.go (1)
117-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate the duplicated encode-and-parse-distributions steps.
The explicit-input branch (Lines 140-155) and the implicit-input branch (Lines 157-186) each independently call
encodeDeterministicCBOR, thenparseFractionalEvaluationDistributions, then return the same three values. Only the value passed toencodeDeterministicCBORdiffers between branches. Extract the hashing-input selection into a single variable, then perform the encode-and-parse-distributions step once.♻️ Proposed refactor to remove duplication
- // If first element is a non-array type, use it as explicit hashing input. - if _, isArray := valuesArray[0].([]any); !isArray { - hashingInput := valuesArray[0] - valuesArray = valuesArray[1:] - - bytesToHash, err := encodeDeterministicCBOR(hashingInput) - if err != nil { - return nil, nil, fmt.Errorf("flag %q: failed to encode hashing input: %w", flagKey, err) - } - - feDistributions, err := parseFractionalEvaluationDistributions(valuesArray, data, logger, flagKey) - if err != nil { - return nil, nil, err - } - - return bytesToHash, feDistributions, nil - } - - // First element is an array ([]any), meaning no explicit hashing input was provided. - // We fall back to implicit targetingKey rules. - rawTargetingKey, exists := dataMap[targetingKeyKey] - if !exists || rawTargetingKey == nil { - return nil, nil, fmt.Errorf("flag %q: bucketing value not supplied and no targetingKey in context", flagKey) - } - - targetingKey, isString := rawTargetingKey.(string) - if !isString { - return nil, nil, fmt.Errorf("flag %q: targetingKey is not a string", flagKey) - } - - if targetingKey == "" { - return nil, nil, fmt.Errorf("flag %q: targetingKey is empty", flagKey) - } - - // Build 2-element array [flagKey, targetingKey] and encode to CBOR. - implicitInput := []any{flagKey, targetingKey} - bytesToHash, err := encodeDeterministicCBOR(implicitInput) - if err != nil { - return nil, nil, fmt.Errorf("flag %q: failed to encode implicit targetingKey: %w", flagKey, err) - } - - feDistributions, err := parseFractionalEvaluationDistributions(valuesArray, data, logger, flagKey) - if err != nil { - return nil, nil, err - } - - return bytesToHash, feDistributions, nil + var hashingInput any + + // If first element is a non-array type, use it as explicit hashing input. + if _, isArray := valuesArray[0].([]any); !isArray { + hashingInput = valuesArray[0] + valuesArray = valuesArray[1:] + } else { + // First element is an array ([]any), meaning no explicit hashing input was provided. + // We fall back to implicit targetingKey rules. + rawTargetingKey, exists := dataMap[targetingKeyKey] + if !exists || rawTargetingKey == nil { + return nil, nil, fmt.Errorf("flag %q: bucketing value not supplied and no targetingKey in context", flagKey) + } + + targetingKey, isString := rawTargetingKey.(string) + if !isString { + return nil, nil, fmt.Errorf("flag %q: targetingKey is not a string", flagKey) + } + + if targetingKey == "" { + return nil, nil, fmt.Errorf("flag %q: targetingKey is empty", flagKey) + } + + hashingInput = []any{flagKey, targetingKey} + } + + bytesToHash, err := encodeDeterministicCBOR(hashingInput) + if err != nil { + return nil, nil, fmt.Errorf("flag %q: failed to encode hashing input: %w", flagKey, err) + } + + feDistributions, err := parseFractionalEvaluationDistributions(valuesArray, data, logger, flagKey) + if err != nil { + return nil, nil, err + } + + return bytesToHash, feDistributions, nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/pkg/evaluator/fractional.go` around lines 117 - 186, Refactor parseFractionalEvaluationData so the explicit and implicit branches only select a shared hashing-input value: use the first value for explicit input, or construct the [flagKey, targetingKey] fallback after validating targetingKey. Move encodeDeterministicCBOR and parseFractionalEvaluationDistributions into one common path after that selection, preserving the existing validation and error context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/pkg/evaluator/fractional.go`:
- Around line 62-110: Update the positive float64 range check in normalizeValue
to use a strict less-than comparison against float64(math.MaxUint64), ensuring
values at 2^64 or above remain float64 and are never converted to uint64 out of
range. Preserve the existing handling for valid in-range integral values and
negative values.
- Around line 138-147: Add a fractional evaluation test covering an explicit
hashing input represented as raw []byte, exercising the non-array branch around
valuesArray and encodeDeterministicCBOR. Assert the evaluation result and
hashing behavior match the expected fractional assignment, while preserving
existing coverage for other explicit input types.
---
Nitpick comments:
In `@core/pkg/evaluator/fractional.go`:
- Around line 117-186: Refactor parseFractionalEvaluationData so the explicit
and implicit branches only select a shared hashing-input value: use the first
value for explicit input, or construct the [flagKey, targetingKey] fallback
after validating targetingKey. Move encodeDeterministicCBOR and
parseFractionalEvaluationDistributions into one common path after that
selection, preserving the existing validation and error context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 98abdea1-2b2e-49f2-81e6-6ba2ecf01956
📒 Files selected for processing (1)
core/pkg/evaluator/fractional.go
Comment on lines
+62
to
+110
| func normalizeValue(val any) any { | ||
| switch v := val.(type) { | ||
| case float64: | ||
| if math.IsNaN(v) || math.IsInf(v, 0) { | ||
| return v | ||
| } | ||
| if v == math.Trunc(v) { | ||
| if v >= 0 && v <= float64(math.MaxUint64) { | ||
| return uint64(v) | ||
| } | ||
| if v < 0 && v >= float64(math.MinInt64) { | ||
| return int64(v) | ||
| } | ||
| } | ||
| return v | ||
| case float32: | ||
| return normalizeValue(float64(v)) | ||
| case int: | ||
| if v >= 0 { | ||
| return uint64(v) | ||
| } | ||
| return int64(v) | ||
| case int64: | ||
| if v >= 0 { | ||
| return uint64(v) | ||
| } | ||
| return v | ||
| case uint: | ||
| return uint64(v) | ||
| case uint32: | ||
| return uint64(v) | ||
| case uint64: | ||
| return v | ||
| case map[string]any: | ||
| res := make(map[string]any, len(v)) | ||
| for k, item := range v { | ||
| res[k] = normalizeValue(item) | ||
| } | ||
| return res | ||
| case []any: | ||
| res := make([]any, len(v)) | ||
| for i, item := range v { | ||
| res[i] = normalizeValue(item) | ||
| } | ||
| return res | ||
| default: | ||
| return v | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the positive-float64 boundary check to avoid implementation-defined conversion.
float64(math.MaxUint64) cannot represent math.MaxUint64 exactly. It rounds up to 2^64, since the nearest representable float64 values near this magnitude are spaced 2048 apart. This makes the check v <= float64(math.MaxUint64) true when v == 2^64. Converting a float64 value of 2^64 to uint64 at Line 70 is then implementation-defined, per the Go language specification, because 2^64 is out of range for uint64.
This directly conflicts with the PR goal of consistent hashing "across providers, languages, compilers, and platforms," since the exact result of this conversion can differ by compiler or architecture for values at this boundary.
Use a strict < comparison so values at or above 2^64 fall through and stay as float64 (still deterministic, just not canonicalized to an integer at this extreme edge).
🐛 Proposed fix for the boundary check
if v == math.Trunc(v) {
- if v >= 0 && v <= float64(math.MaxUint64) {
+ if v >= 0 && v < float64(math.MaxUint64) {
return uint64(v)
}
if v < 0 && v >= float64(math.MinInt64) {
return int64(v)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func normalizeValue(val any) any { | |
| switch v := val.(type) { | |
| case float64: | |
| if math.IsNaN(v) || math.IsInf(v, 0) { | |
| return v | |
| } | |
| if v == math.Trunc(v) { | |
| if v >= 0 && v <= float64(math.MaxUint64) { | |
| return uint64(v) | |
| } | |
| if v < 0 && v >= float64(math.MinInt64) { | |
| return int64(v) | |
| } | |
| } | |
| return v | |
| case float32: | |
| return normalizeValue(float64(v)) | |
| case int: | |
| if v >= 0 { | |
| return uint64(v) | |
| } | |
| return int64(v) | |
| case int64: | |
| if v >= 0 { | |
| return uint64(v) | |
| } | |
| return v | |
| case uint: | |
| return uint64(v) | |
| case uint32: | |
| return uint64(v) | |
| case uint64: | |
| return v | |
| case map[string]any: | |
| res := make(map[string]any, len(v)) | |
| for k, item := range v { | |
| res[k] = normalizeValue(item) | |
| } | |
| return res | |
| case []any: | |
| res := make([]any, len(v)) | |
| for i, item := range v { | |
| res[i] = normalizeValue(item) | |
| } | |
| return res | |
| default: | |
| return v | |
| } | |
| } | |
| func normalizeValue(val any) any { | |
| switch v := val.(type) { | |
| case float64: | |
| if math.IsNaN(v) || math.IsInf(v, 0) { | |
| return v | |
| } | |
| if v == math.Trunc(v) { | |
| if v >= 0 && v < float64(math.MaxUint64) { | |
| return uint64(v) | |
| } | |
| if v < 0 && v >= float64(math.MinInt64) { | |
| return int64(v) | |
| } | |
| } | |
| return v | |
| case float32: | |
| return normalizeValue(float64(v)) | |
| case int: | |
| if v >= 0 { | |
| return uint64(v) | |
| } | |
| return int64(v) | |
| case int64: | |
| if v >= 0 { | |
| return uint64(v) | |
| } | |
| return v | |
| case uint: | |
| return uint64(v) | |
| case uint32: | |
| return uint64(v) | |
| case uint64: | |
| return v | |
| case map[string]any: | |
| res := make(map[string]any, len(v)) | |
| for k, item := range v { | |
| res[k] = normalizeValue(item) | |
| } | |
| return res | |
| case []any: | |
| res := make([]any, len(v)) | |
| for i, item := range v { | |
| res[i] = normalizeValue(item) | |
| } | |
| return res | |
| default: | |
| return v | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/pkg/evaluator/fractional.go` around lines 62 - 110, Update the positive
float64 range check in normalizeValue to use a strict less-than comparison
against float64(math.MaxUint64), ensuring values at 2^64 or above remain float64
and are never converted to uint64 out of range. Preserve the existing handling
for valid in-range integral values and negative values.
Comment on lines
+138
to
+147
|
|
||
| // If first element is a non-array type, use it as explicit hashing input. | ||
| if _, isArray := valuesArray[0].([]any); !isArray { | ||
| hashingInput := valuesArray[0] | ||
| valuesArray = valuesArray[1:] | ||
|
|
||
| bytesToHash, err := encodeDeterministicCBOR(hashingInput) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("flag %q: failed to encode hashing input: %w", flagKey, err) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether raw []byte explicit hashing inputs are covered by tests.
rg -n -B2 -A10 '\[\]byte' core/pkg/evaluator/fractional_test.goRepository: open-feature/flagd
Length of output: 7019
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== fractional.go relevant section =="
sed -n '120,175p' core/pkg/evaluator/fractional.go
echo
echo "== relevant tests mentioning fractional explicit non-array inputs =="
rg -n -B5 -A12 'fractional"|"cat"|"email"|non-array|explicit|raw|[]byte|bool|float64' core/pkg/evaluator/fractional_test.go | sed -n '1,220p'
echo
echo "== deterministic encoding support and tests for []byte =="
rg -n -B3 -A8 'encodeDeterministicCBOR|non-string|bytes|case.*\[\]byte|[]byte|byte\(' core/pkg/evaluator -g '*.go'
echo
echo "== change summary for fractional files in this PR =="
git diff --stat -- core/pkg/evaluator/fractional.go core/pkg/evaluator/fractional_test.go || true
git diff --numstat -- core/pkg/evaluator/fractional.go core/pkg/evaluator/fractional_test.go || trueRepository: open-feature/flagd
Length of output: 2606
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== encoder tests around explicit byte coverage =="
rg -n -B5 -A15 'encodeDeterministicCBOR|non-string|bytes|[]byte|byte\(|blob|BinaryMarshaler' core/pkg/evaluator -g '*.go'
echo
echo "== fractional tests around explicit non-array inputs and primitives =="
rg -n -B8 -A18 '"fractional"|non-array|explicit|[]byte|bool|float64|email|cat' core/pkg/evaluator/fractional_test.go | sed -n '1,260p'
echo
echo "== deterministic encoder implementation =="
fd -a '.*encoding.*|.*fractional.*|.*encoder.*' core/pkg/evaluator | sed -n '1,60p'
rg -n -B4 -A20 'func encodeDeterministicCBOR|case "\[\]byte"|case.*byte' core/pkg -g '*.go'
echo
echo "== deterministic tests directly passing []byte =="
rg -n -B6 -A12 'TestEncode|encodeDeterministicCBOR|[]byte\(' core/pkg -g '*test.go'Repository: open-feature/flagd
Length of output: 396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== deterministic encoder implementation =="
rg -n -B4 -A30 'func encodeDeterministicCBOR|case "[]byte"|case \[\]byte' core/pkg -g '*.go'
echo
echo "== deterministic tests directly passing []byte =="
rg -n -B6 -A14 'encodeDeterministicCBOR|[]byte\(' core/pkg -g '*test.go'
echo
echo "== fractional tests around explicit non-array inputs and primitives =="
rg -n -B8 -A18 '"fractional"|email|cat|non-array|bool|float64' core/pkg/evaluator/fractional_test.go | sed -n '1,260p'Repository: open-feature/flagd
Length of output: 364
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== deterministic implementation =="
rg -n -B4 -A35 'func encodeDeterministicCBOR|case "\[\]byte"|case \[\]byte' core/pkg -g '*.go'
echo
echo "== deterministic tests directly passing []byte =="
rg -n -B6 -A14 'encodeDeterministicCBOR|[]byte\(' core/pkg -g '*test.go'
echo
echo "== fractional tests around explicit non-array inputs and primitives =="
rg -n -B8 -A18 '"fractional"|email|cat|non-array|bool|float64' core/pkg/evaluator/fractional_test.go | sed -n '1,260p'
echo
echo "== deterministic test coverage summary for []byte input =="
python3 - <<'PY'
from pathlib import Path
for p in Path('core/pkg').rglob('*_test.go'):
txt = p.read_text(errors='replace').splitlines()
for i,line in enumerate(txt,1):
if 'encodeDeterministicCBOR' in line:
start=max(1,i-4); end=min(len(txt),i+14)
print(f'--- {p}:{i} ---')
for j in range(start,end+1):
print(f'{j}: {txt[j-1]}')
PYRepository: open-feature/flagd
Length of output: 3286
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== deterministic tests directly passing []byte =="
rg -n -B6 -A14 'encodeDeterministicCBOR|\\[\\]byte\\(' core/pkg -g '*test.go' || true
echo
echo "== deterministic encoding implementation =="
sed -n '90,120p' core/pkg/evaluator/fractional.go
echo
rg -n -B3 -A25 'func normalizeValue|case string|case uint' core/pkg/evaluator/fractional.go -g '*.go'
echo
echo "== fractional tests around explicit non-array inputs and primes =="
rg -n -B8 -A18 '"fractional"|email|cat|non-array|bool|float64' core/pkg/evaluator/fractional_test.go | sed -n '1,260p'
echo
echo "== deterministic encoder raw []byte coverage =="
python3 - <<'PY'
from pathlib import Path
for p in Path('core/pkg').rglob('*_test.go'):
txt=p.read_text(errors='replace').splitlines()
for i,line in enumerate(txt,1):
if 'encodeDeterministicCBOR' in line:
print(f'--- {p}:{i} ---')
for j in range(max(1,i-3), min(len(txt),i+14)+1):
print(f'{j}: {txt[j-1]}')
PYRepository: open-feature/flagd
Length of output: 11194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== fractional tests mentioning bool or float non-array literals =="
rg -n -B8 -A18 'bool|float64|non-array|error message|invalid|non-string' core/pkg/evaluator/fractional_test.go | sed -n '1,260p'
echo
echo "== deterministic encoder test coverage =="
python3 - <<'PY'
from pathlib import Path
for p in Path('core/pkg').rglob('*_test.go'):
txt=p.read_text(errors='replace').splitlines()
for i,line in enumerate(txt,1):
if 'encodeDeterministicCBOR' in line or 'normalizeValue' in line:
print(f'--- {p}:{i} ---')
for j in range(max(1,i-3), min(len(txt),i+14)+1):
print(f'{j}: {txt[j-1]}')
PY
echo
echo "== deterministic tests mentioning []byte =="
rg -n -B5 -A12 'byte\(|[]byte' -g '*test.go' | sed -n '1,140p'Repository: open-feature/flagd
Length of output: 4292
Add coverage for raw []byte explicit fractional inputs.
encodeDeterministicCBOR passes a Go-native []byte through unchanged before CBOR marshaling, and this path is not exercised by the direct encoder tests. Add a raw []byte explicit input case for fractional evaluation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/pkg/evaluator/fractional.go` around lines 138 - 147, Add a fractional
evaluation test covering an explicit hashing input represented as raw []byte,
exercising the non-array branch around valuesArray and encodeDeterministicCBOR.
Assert the evaluation result and hashing behavior match the expected fractional
assignment, while preserving existing coverage for other explicit input types.
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
Signed-off-by: NeaguGeorgiana23 <neagugeorgiana@google.com>
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/pkg/evaluator/fractional_test.go (1)
140-141: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd regression cases for non-string explicit hashing inputs.
The valid explicit inputs in these cases resolve to strings. The implicit cases use string
flagKeyandtargetingKeycomponents. The tests do not cover raw[]byte, numeric, boolean, or nested array/map inputs. An implementation that still stringifies these values can pass every updated assertion. Add cases through the same evaluator path and pin them to known variant or CBOR/Murmur3 vectors.Also applies to: 150-151, 170-171, 190-191, 200-201, 210-211, 298-299, 417-418, 441-443, 657-658, 687-688
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/pkg/evaluator/fractional_test.go` around lines 140 - 141, Add regression cases in the evaluator table-driven tests covering explicit raw []byte, numeric, boolean, and nested array/map inputs, plus implicit non-string flagKey/targetingKey components. Exercise each through the existing evaluator path and assert known variant or CBOR/Murmur3 vector results so stringification implementations fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/pkg/evaluator/fractional_test.go`:
- Around line 441-443: Rename the test case surrounding the expectedVariant,
expectedValue, and expectedReason fields to describe the new behavior: an
explicit hashing input resolving to nil causes an error and the resolver returns
the default variant. Remove wording that says nil or missing custom variables
are ignored and parsing continues.
In `@test-harness`:
- Line 1: Update the test-harness submodule checkout to commit
82ba89ec8db498fa51368e558e4d87642d9e93c4, ensuring that commit is fetched and
available locally before validating or merging the updated gitlink.
---
Nitpick comments:
In `@core/pkg/evaluator/fractional_test.go`:
- Around line 140-141: Add regression cases in the evaluator table-driven tests
covering explicit raw []byte, numeric, boolean, and nested array/map inputs,
plus implicit non-string flagKey/targetingKey components. Exercise each through
the existing evaluator path and assert known variant or CBOR/Murmur3 vector
results so stringification implementations fail.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fc0aa70f-bc8c-4541-a7e3-e194b0786bf2
⛔ Files ignored due to path filters (1)
test/integration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
core/pkg/evaluator/fractional_test.gotest-harnesstest/integration/go.modtest/integration/integration_test.go
Comment on lines
+441
to
+443
| expectedVariant: redVariant, | ||
| expectedValue: redHex, | ||
| expectedReason: model.DefaultReason, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rename the test case to match the new fallback behavior.
The assertion at Line 441-443 now expects model.DefaultReason. The test name at Line 421 still says the parser should “ignore nil/missing custom variables and continue”. The parser now returns an error when the explicit hashing input resolves to nil, and the resolver returns the default variant. Rename the case to describe the new behavior.
Proposed test name
- "missing email - parser should ignore nil/missing custom variables and continue": {
+ "missing explicit bucket-by value returns default variant": {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/pkg/evaluator/fractional_test.go` around lines 441 - 443, Rename the
test case surrounding the expectedVariant, expectedValue, and expectedReason
fields to describe the new behavior: an explicit hashing input resolving to nil
causes an error and the resolver returns the default variant. Remove wording
that says nil or missing custom variables are ignored and parsing continues.
| @@ -1 +1 @@ | |||
| Subproject commit df6a84377aa29d022659020807825cdcb191f87f | |||
| Subproject commit 82ba89ec8db498fa51368e558e4d87642d9e93c4 | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
old_commit="$(git rev-parse HEAD^:test-harness)"
new_commit="$(git rev-parse HEAD:test-harness)"
git -C test-harness diff --stat "$old_commit" "$new_commit"
git -C test-harness diff "$old_commit" "$new_commit"Repository: open-feature/flagd
Length of output: 332
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- submodule entry ---"
git ls-files -s test-harness || true
echo "--- .gitmodules test-harness ---"
git config -f .gitmodules --get-regexp 'submodule\.test-harness' || true
echo "--- HEAD and HEAD^ submodule pointers in index ---"
git show HEAD:test-harness 2>&1 | head -20 || true
git show HEAD^:test-harness 2>&1 | head -20 || true
echo "--- any local test-harness refs/commits ---"
git rev-parse HEAD:test-harness HEAD^:test-harness 2>&1 || true
git rev-parse test-harness/test-harness 2>&1 | sed 's/^/rev-parse: /' || trueRepository: open-feature/flagd
Length of output: 1190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- .gitmodules test-harness full object ---"
python3 - <<'PY'
import subprocess
try:
out = subprocess.check_output(["git", "config", "-f", ".gitmodules", "-l"], text=True)
for line in out.splitlines():
if "test-harness" in line or "submodule" in line:
print(line)
except subprocess.CalledProcessError as e:
print(e.stderr or e.output)
PY
echo "--- try fetch test-harness commits from canonical URL ---"
git ls-remote https://github.com/open-feature/test-harness.git 82ba89ec8db498fa51368e558e4d87642d9e93c4 HEAD main master 2>&1 | sed -n '1,80p' || true
echo "--- HEAD and HEAD^ gitlink object info ---"
git cat-file -e HEAD:test-harness && git cat-file -p HEAD:test-harness | sed -n '1,5p' || true
git cat-file -e HEAD^:test-harness && git cat-file -p HEAD^:test-harness | sed -n '1,5p' || true
echo "--- compare old/new gitlink if both resolve ---"
old="$(git cat-file -e HEAD^:test-harness >/dev/null 2>&1 && git rev-parse HEAD^:test-harness)"
new="$(git cat-file -e HEAD:test-harness >/dev/null 2>&1 && git rev-parse HEAD:test-harness)"
set +e
if [ -n "${old:-}" ] && [ -n "${new:-}" ]; then
echo "$old -> $new"
else
echo "old=$old new=$new"
fiRepository: open-feature/flagd
Length of output: 1106
Fix the test-harness submodule checkout before merge.
The updated gitlink exists on the upstream HEAD and main ref, but the required 82ba89ec8db498fa51368e558e4d87642d9e93c4 object is not available in the current clone. This makes branch checkout/validation fail until submodule content can be fetched.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-harness` at line 1, Update the test-harness submodule checkout to commit
82ba89ec8db498fa51368e558e4d87642d9e93c4, ensuring that commit is fetched and
available locally before validating or merging the updated gitlink.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR
Implements the fractional-non-string-rand-units architecture decision by replacing string-concatenation hashing with deterministic CBOR encoding and supporting non-string explicit hashing inputs in fractional evaluation.
murmur3.StringSum32) with byte-based Murmur3 hashing (murmur3.Sum32) over deterministic CBOR-encoded payloads (github.com/fxamacker/cbor/v2).normalizeValueandencodeDeterministicCBOR) so numeric integers, maps, and slices are consistently typed and canonically encoded across platforms.[flagKey, targetingKey]as a deterministic CBOR array instead of concatenating strings.null, or when an implicittargetingKeyis missing, non-string, or empty.Fixes #1737