Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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: 9
🧹 Nitpick comments (8)
rust/crates/adc-differ/tests/fixtures_sanity.rs (1)
12-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare fixture scales and change ratios.
rust/crates/adc-differ/examples/gen_fixtures.rsandrust/crates/adc-differ/tests/fixtures_sanity.rsduplicate these values. Move them to a shared module so fixture generation and expected-event checks cannot diverge.🤖 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 `@rust/crates/adc-differ/tests/fixtures_sanity.rs` around lines 12 - 13, Move the shared SCALES values, along with the fixture change ratios, out of gen_fixtures.rs and fixtures_sanity.rs into a common module. Update both the fixture generator and expected-event checks to import and reuse those shared definitions, removing their local duplicates so the values cannot diverge.rust/crates/adc-differ/src/bin/run_fixtures.rs (1)
21-56: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a round-trip test for
resource_type_from_str.Add a test for every
ResourceTypevariant, includingInternalStreamService, and assert thatresource_type_from_str(resource_type.as_str())returns the same variant. Do not rely onResourceType::ALL, because it excludesInternalStreamService. This preventsparse_default_valuefrom silently dropping new resource defaults.🤖 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 `@rust/crates/adc-differ/src/bin/run_fixtures.rs` around lines 21 - 56, Add a unit test for resource_type_from_str that explicitly enumerates every ResourceType variant, including InternalStreamService, and asserts parsing each variant’s as_str() value returns the original variant. Do not use ResourceType::ALL; keep the test adjacent to the helper or its existing test module and ensure all mappings used by parse_default_value are covered.rust/crates/adc-sdk/src/utils.rs (1)
3-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRecord why SHA-1 is required, to dismiss the weak-hash warning.
Static analysis flags
Sha1::new()as CWE-328. The finding does not apply here, becausegenerate_idderives a deterministic resource identifier from a resource name. It is not used for integrity, signatures, or password handling. SHA-1 is also mandatory for identifier parity with the TypeScript ADC implementation; SHA-256 would change every generated resource ID. State that constraint in the doc comment so a future change does not silently break parity, and so the next SAST run has a documented disposition.📝 Proposed doc comment
-/// Deterministic resource id: the sha1 (not sha256) hex digest of `name`. +/// Deterministic resource id: the sha1 (not sha256) hex digest of `name`. +/// +/// Not a security primitive: this is an identifier derivation, not integrity or +/// signature checking. SHA-1 is required for id parity with the TypeScript ADC +/// implementation — changing the algorithm changes every generated resource id. pub fn generate_id(name: &str) -> String {🤖 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 `@rust/crates/adc-sdk/src/utils.rs` around lines 3 - 8, Update the doc comment for generate_id to document that SHA-1 is intentionally used only for deterministic resource identifiers, not integrity, signatures, or password handling, and is required to preserve identifier parity with the TypeScript ADC implementation; retain the existing SHA-1 behavior.Source: Linters/SAST tools
rust/crates/adc-sdk/src/value_diff.rs (1)
153-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for type changes and null values.
The current cases cover keys, scalars, nesting, and array tails. Two parity-critical paths have no coverage. First, the
real_type_ofearly return on Line 75, for example object to string, or array to object. Second,nullhandling, becausereal_type_ofreports"null"as a distinct type while JavaScripttypeof nullis"object"; thedeep-difflibrary uses its ownrealTypeOfthat also reports"null", so a test pins this parity decision.♻️ Proposed additional tests
#[test] fn type_change_reports_single_edit() { assert_eq!( diff_value(&json!({"a": {"b": 1}}), &json!({"a": "x"})), Some(vec![ValueDiff::Edit { path: vec![PathSegment::Key("a".into())], lhs: json!({"b": 1}), rhs: json!("x") }]) ); } #[test] fn null_is_a_distinct_type_from_object() { assert_eq!( diff_value(&json!({"a": null}), &json!({"a": {}})), Some(vec![ValueDiff::Edit { path: vec![PathSegment::Key("a".into())], lhs: json!(null), rhs: json!({}) }]) ); assert_eq!(diff_value(&json!({"a": null}), &json!({"a": null})), None); }🤖 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 `@rust/crates/adc-sdk/src/value_diff.rs` around lines 153 - 235, Add tests in the existing tests module for the type-change and null-handling paths in diff_value: verify an object-to-string change produces one Edit at the changed key, null-to-object produces one Edit, and identical null values produce None. Use the existing ValueDiff, PathSegment, and json! assertion style.rust/crates/adc-sdk/tests/resources_from_fixtures.rs (1)
78-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an integer type for
concurrency.
UpstreamHealthCheckActive.concurrencyanddefault_concurrencyshould useu32. Then compare this field with10. The APISIX schema definesconcurrencyas an integer with a default of10;f64permits invalid fractional values and requires float comparison here.🤖 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 `@rust/crates/adc-sdk/tests/resources_from_fixtures.rs` at line 78, Update the concurrency type used by UpstreamHealthCheckActive and default_concurrency to u32, matching the APISIX schema and preventing fractional values. In the fixture assertion around checks.active.concurrency, compare against the integer literal 10 instead of 10.0, while preserving the existing default-concurrency behavior.scripts/compare-differ-fixtures.mjs (1)
45-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider calling the Nx target instead of
npx vitest.This PR adds the
dump-fixturestarget inlibs/differ/package.jsonlines 34-39. Line 46 invokesnpx vitest run --config vitest.fixtures.config.tsinstead. Two entry points now run the same dump. If the target options change later, this script keeps the old invocation.🤖 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 `@scripts/compare-differ-fixtures.mjs` around lines 45 - 61, Update the TypeScript fixture execution in the comparison script to invoke the existing libs/differ dump-fixtures Nx target instead of calling npx vitest directly, while preserving the current fixture directory and results output environment configuration.libs/differ/tools/dump-fixture-results.ts (1)
32-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the fixture name to parse failures.
If one fixture contains invalid JSON,
JSON.parsethrows without naming the file. The dump then fails with no indication of which fixture is broken. Wrap the read and parse, and includefilein the error message.♻️ Proposed refactor to report the failing fixture
for (const file of files) { const name = basename(file, '.json'); - const fixture = JSON.parse(readFileSync(join(FIXTURES_DIR, file), 'utf-8')); + let fixture: { local?: unknown; remote?: unknown; defaultValue?: unknown }; + try { + fixture = JSON.parse(readFileSync(join(FIXTURES_DIR, file), 'utf-8')); + } catch (err) { + throw new Error(`failed to read fixture ${file}: ${(err as Error).message}`); + } results[name] = DifferV4.diff(fixture.local ?? {}, fixture.remote ?? {}, fixture.defaultValue); }As per coding guidelines: "Every function return value must be checked for errors (if applicable); errors must be properly handled, not ignored or silently swallowed".
🤖 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 `@libs/differ/tools/dump-fixture-results.ts` around lines 32 - 36, Update the fixture-loading loop around JSON.parse to catch read or parse failures and rethrow or report an error that includes the affected file name. Preserve the existing results[name] and DifferV4.diff flow for successfully loaded fixtures, and do not swallow the original error details.Source: Coding guidelines
rust/crates/adc-differ/tests/basic.rs (1)
10-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared test helpers into one module.
configandevare duplicated in six integration test files. Move them totests/common/mod.rsand import them withmod common;. This keeps one definition and avoids drift between files.Also consider making
configpanic on a non-object input instead of returning an empty map. A silent fallback hides a malformed fixture.🤖 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 `@rust/crates/adc-differ/tests/basic.rs` around lines 10 - 16, Move the shared config and ev test helpers into tests/common/mod.rs, make them available to each integration test via mod common;, and update all six files to use the common definitions instead of local copies. Change config to panic when given a non-object Value rather than silently returning an empty map, while preserving its object conversion behavior.
🤖 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 `@fixtures/differ/basic.update_resource.json`:
- Around line 2-3: Make the update fixtures behaviorally distinct: in
fixtures/differ/basic.update_resource.json lines 2-3, replace the duplicate
plugin-addition payload with a distinct generic resource update or remove the
fixture; in fixtures/differ/basic.update_resource_add_plugin.json lines 2-3,
retain the existing payload for the add-key-auth-plugin scenario.
In `@libs/differ/tools/dump-fixture-results.ts`:
- Around line 20-23: Update the FIXTURES_DIR default in dump-fixture-results.ts
to resolve from the module’s location, navigating three directory levels up to
the repository root and then into fixtures/differ. Preserve the
ADC_DIFFER_FIXTURES_DIR environment-variable override and remove the hardcoded
developer-specific path.
In `@rust/crates/adc-differ/src/differ_v4.rs`:
- Around line 172-175: Update handle_update’s default-value selection to merge
default_value.plugins[remote_name] into default_value.core[resource_type] when
processing GlobalRule and PluginMetadata records, while preserving core defaults
when no plugin-specific entry exists. Ensure extract_tuples passes the plugin
key through as remote_name, and add regression fixtures covering both record
collections with plugin-specific defaults.
In `@rust/crates/adc-sdk/src/event.rs`:
- Around line 26-47: Update EventKind with Serde field renaming so its
struct-variant fields serialize as camelCase, including newValue and oldValue,
while preserving snake_case variant names. Also update the Event definition at
rust/crates/adc-sdk/src/event.rs lines 82-93 with camelCase field renaming so
resourceType, resourceId, resourceName, and parentId match the documented wire
format.
In `@rust/crates/adc-sdk/src/resources/consumer.rs`:
- Around line 11-25: Prevent plaintext secrets from appearing in derived Debug
output by adding a shared redacting Debug implementation or wrapper for
Plugin/Plugins-typed fields. Update ConsumerCredential.config in
rust/crates/adc-sdk/src/resources/consumer.rs:11-25,
Configuration/InternalConfiguration in
rust/crates/adc-sdk/src/resources/mod.rs:46-90, Route/StreamRoute.plugins in
rust/crates/adc-sdk/src/resources/route.rs:32-87, and Service.plugins in
rust/crates/adc-sdk/src/resources/service.rs:63-89 so their Debug
representations redact credential values while preserving non-secret fields.
In `@rust/crates/adc-sdk/src/resources/ssl.rs`:
- Around line 34-39: Replace the derived Debug implementation on SSLCertificate
with a manual implementation that preserves the certificate field but always
redacts key, including inline PEM and $secret:// references. Keep Serialize and
Deserialize derives unchanged so API serialization still emits the actual key
value.
In `@rust/crates/adc-sdk/src/value_diff.rs`:
- Around line 119-140: Update the nested `ValueDiff::New` and
`ValueDiff::Deleted` constructions in `diff_array` so their `item` payloads omit
the `path` field, matching `datum-diff` serialization for array-tail items.
Preserve `path: path.to_vec()` on the outer `ValueDiff::Array` entries and keep
root diff paths unchanged.
In `@rust/crates/adc-sync-bench/src/main.rs`:
- Around line 110-115: Update the argument parsing in main around concurrency,
iterations, and runtime_flavor to report invalid input as a usage error instead
of panicking or silently falling back. Parse numeric values fallibly, require
both concurrency and iterations to be greater than zero, and accept only
“current” or “multi” for runtime_flavor; reject all other values before
benchmark execution.
In `@scripts/compare-differ-fixtures.mjs`:
- Around line 71-90: Validate that rustEvents is an array before the
normalizeEvent mapping in the comparison loop. When the value has an invalid
shape, append a failure for the current name with a clear shape-error reason and
continue processing the remaining fixtures; only call map and compare outputs
for valid arrays.
---
Nitpick comments:
In `@libs/differ/tools/dump-fixture-results.ts`:
- Around line 32-36: Update the fixture-loading loop around JSON.parse to catch
read or parse failures and rethrow or report an error that includes the affected
file name. Preserve the existing results[name] and DifferV4.diff flow for
successfully loaded fixtures, and do not swallow the original error details.
In `@rust/crates/adc-differ/src/bin/run_fixtures.rs`:
- Around line 21-56: Add a unit test for resource_type_from_str that explicitly
enumerates every ResourceType variant, including InternalStreamService, and
asserts parsing each variant’s as_str() value returns the original variant. Do
not use ResourceType::ALL; keep the test adjacent to the helper or its existing
test module and ensure all mappings used by parse_default_value are covered.
In `@rust/crates/adc-differ/tests/basic.rs`:
- Around line 10-16: Move the shared config and ev test helpers into
tests/common/mod.rs, make them available to each integration test via mod
common;, and update all six files to use the common definitions instead of local
copies. Change config to panic when given a non-object Value rather than
silently returning an empty map, while preserving its object conversion
behavior.
In `@rust/crates/adc-differ/tests/fixtures_sanity.rs`:
- Around line 12-13: Move the shared SCALES values, along with the fixture
change ratios, out of gen_fixtures.rs and fixtures_sanity.rs into a common
module. Update both the fixture generator and expected-event checks to import
and reuse those shared definitions, removing their local duplicates so the
values cannot diverge.
In `@rust/crates/adc-sdk/src/utils.rs`:
- Around line 3-8: Update the doc comment for generate_id to document that SHA-1
is intentionally used only for deterministic resource identifiers, not
integrity, signatures, or password handling, and is required to preserve
identifier parity with the TypeScript ADC implementation; retain the existing
SHA-1 behavior.
In `@rust/crates/adc-sdk/src/value_diff.rs`:
- Around line 153-235: Add tests in the existing tests module for the
type-change and null-handling paths in diff_value: verify an object-to-string
change produces one Edit at the changed key, null-to-object produces one Edit,
and identical null values produce None. Use the existing ValueDiff, PathSegment,
and json! assertion style.
In `@rust/crates/adc-sdk/tests/resources_from_fixtures.rs`:
- Line 78: Update the concurrency type used by UpstreamHealthCheckActive and
default_concurrency to u32, matching the APISIX schema and preventing fractional
values. In the fixture assertion around checks.active.concurrency, compare
against the integer literal 10 instead of 10.0, while preserving the existing
default-concurrency behavior.
In `@scripts/compare-differ-fixtures.mjs`:
- Around line 45-61: Update the TypeScript fixture execution in the comparison
script to invoke the existing libs/differ dump-fixtures Nx target instead of
calling npx vitest directly, while preserving the current fixture directory and
results output environment configuration.
🪄 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
Run ID: 23788164-3d96-41bd-b675-3a16ae4a34e5
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (85)
.gitignorefixtures/differ/basic.adapts_to_default_core_values.jsonfixtures/differ/basic.adapts_to_default_plugin_values.jsonfixtures/differ/basic.boolean_defaults_merged_correctly.jsonfixtures/differ/basic.create_resource.jsonfixtures/differ/basic.delete_resource.jsonfixtures/differ/basic.empty_input_yields_empty_output.jsonfixtures/differ/basic.generates_hashed_resource_id.jsonfixtures/differ/basic.keeps_plugins_when_plugins_not_changed.jsonfixtures/differ/basic.merges_array_nested_object_defaults_correctly.jsonfixtures/differ/basic.route_and_stream_route_ids_generated_correctly.jsonfixtures/differ/basic.selectively_merges_objects_in_default_values.jsonfixtures/differ/basic.sorted_by_event_type.jsonfixtures/differ/basic.update_resource.jsonfixtures/differ/basic.update_resource_add_plugin.jsonfixtures/differ/basic.update_resource_update_plugin_with_default_value.jsonfixtures/differ/basic.updates_service_and_its_nested_route.jsonfixtures/differ/basic.updates_service_nested_route.jsonfixtures/differ/consumer.creates_updates_deletes_consumer_credentials.jsonfixtures/differ/consumer.deletes_consumer_credentials_when_consumer_is_deleted.jsonfixtures/differ/custom_id.deletes_and_creates_new_resource_when_id_changes.jsonfixtures/differ/regression.does_not_apply_stream_service_default_to_http_service.jsonfixtures/differ/regression.resolves_stream_service_default_type_correctly.jsonfixtures/differ/service_upstream.creates_non_default_upstreams.jsonfixtures/differ/service_upstream.creates_service_and_upstream.jsonfixtures/differ/service_upstream.deletes_non_default_upstreams.jsonfixtures/differ/service_upstream.replaces_non_default_upstreams.jsonfixtures/differ/service_upstream.unchanged_service_with_default_and_named_upstreams.jsonfixtures/differ/service_upstream.unchanged_service_with_only_default_upstream.jsonfixtures/differ/service_upstream.updates_default_upstream.jsonfixtures/differ/service_upstream.updates_non_default_upstreams.jsonfixtures/differ/upstream.creates_and_updates_ssl_before_upstream.jsonfixtures/differ/usecase.renames_service_with_nested_routes.jsonfixtures/differ/usecase.selectively_merges_objects_in_default_values_on_a_service.jsonlibs/differ/package.jsonlibs/differ/tools/dump-fixture-results.tslibs/differ/vitest.fixtures.config.tsrust/Cargo.tomlrust/benches/fixtures/large.few.local.jsonrust/benches/fixtures/large.many.local.jsonrust/benches/fixtures/large.none.local.jsonrust/benches/fixtures/large.remote.jsonrust/benches/fixtures/medium.few.local.jsonrust/benches/fixtures/medium.many.local.jsonrust/benches/fixtures/medium.none.local.jsonrust/benches/fixtures/medium.remote.jsonrust/benches/fixtures/small.few.local.jsonrust/benches/fixtures/small.many.local.jsonrust/benches/fixtures/small.none.local.jsonrust/benches/fixtures/small.remote.jsonrust/crates/adc-differ/Cargo.tomlrust/crates/adc-differ/benches/differ_bench.rsrust/crates/adc-differ/examples/gen_fixtures.rsrust/crates/adc-differ/src/bin/run_fixtures.rsrust/crates/adc-differ/src/differ_meta.rsrust/crates/adc-differ/src/differ_v4.rsrust/crates/adc-differ/src/field_meta.rsrust/crates/adc-differ/src/lib.rsrust/crates/adc-differ/tests/basic.rsrust/crates/adc-differ/tests/consumer.rsrust/crates/adc-differ/tests/custom_id.rsrust/crates/adc-differ/tests/fixtures_sanity.rsrust/crates/adc-differ/tests/regression.rsrust/crates/adc-differ/tests/service_upstream.rsrust/crates/adc-differ/tests/upstream.rsrust/crates/adc-differ/tests/usecase.rsrust/crates/adc-mock-server/Cargo.tomlrust/crates/adc-mock-server/src/main.rsrust/crates/adc-sdk/Cargo.tomlrust/crates/adc-sdk/src/event.rsrust/crates/adc-sdk/src/lib.rsrust/crates/adc-sdk/src/resource.rsrust/crates/adc-sdk/src/resources/common.rsrust/crates/adc-sdk/src/resources/consumer.rsrust/crates/adc-sdk/src/resources/mod.rsrust/crates/adc-sdk/src/resources/route.rsrust/crates/adc-sdk/src/resources/service.rsrust/crates/adc-sdk/src/resources/ssl.rsrust/crates/adc-sdk/src/resources/upstream.rsrust/crates/adc-sdk/src/utils.rsrust/crates/adc-sdk/src/value_diff.rsrust/crates/adc-sdk/tests/resources_from_fixtures.rsrust/crates/adc-sync-bench/Cargo.tomlrust/crates/adc-sync-bench/src/main.rsscripts/compare-differ-fixtures.mjs
| "local": { "consumers": [{ "username": "alice", "plugins": { "key-auth": { "key": "alice-key" } } }] }, | ||
| "remote": { "consumers": [{ "username": "alice", "plugins": {} }] } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the two update fixtures behaviorally distinct.
Both files contain the same local and remote payload, so the suite cannot distinguish a generic resource update from plugin addition.
fixtures/differ/basic.update_resource.json#L2-L3: change the payload to a distinct resource update, or remove this duplicate fixture.fixtures/differ/basic.update_resource_add_plugin.json#L2-L3: retain this payload only for the add-key-auth-plugin scenario.
📍 Affects 2 files
fixtures/differ/basic.update_resource.json#L2-L3(this comment)fixtures/differ/basic.update_resource_add_plugin.json#L2-L3
🤖 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 `@fixtures/differ/basic.update_resource.json` around lines 2 - 3, Make the
update fixtures behaviorally distinct: in
fixtures/differ/basic.update_resource.json lines 2-3, replace the duplicate
plugin-addition payload with a distinct generic resource update or remove the
fixture; in fixtures/differ/basic.update_resource_add_plugin.json lines 2-3,
retain the existing payload for the add-key-auth-plugin scenario.
| const FIXTURES_DIR = | ||
| process.env.ADC_DIFFER_FIXTURES_DIR ?? '/home/bzp/code/adc-rust/fixtures/differ'; | ||
| const OUT_FILE = | ||
| process.env.ADC_DIFFER_FIXTURE_RESULTS_OUT ?? '/tmp/adc-ts-differ-fixture-results.json'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the hardcoded developer home directory.
FIXTURES_DIR defaults to /home/bzp/code/adc-rust/fixtures/differ. That path exists only on one machine, so the dump fails for every other user and in CI unless ADC_DIFFER_FIXTURES_DIR is set. The default also commits a local username into the repository.
Resolve the default from the module location instead. The file is at libs/differ/tools/, so the repository root is three levels up.
🛠️ Proposed fix to resolve the fixtures directory relative to the repository
-import { basename, extname, join } from 'node:path';
+import { basename, dirname, extname, join } from 'node:path';
import { readdirSync, readFileSync, writeFileSync } from 'node:fs';
+import { fileURLToPath } from 'node:url';
import { it } from 'vitest';
import { DifferV4 } from '../src/differv4.js';
+const REPO_ROOT = join(dirname(fileURLToPath(import.meta.url)), '../../..');
+
const FIXTURES_DIR =
- process.env.ADC_DIFFER_FIXTURES_DIR ?? '/home/bzp/code/adc-rust/fixtures/differ';
+ process.env.ADC_DIFFER_FIXTURES_DIR ?? join(REPO_ROOT, 'fixtures/differ');📝 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.
| const FIXTURES_DIR = | |
| process.env.ADC_DIFFER_FIXTURES_DIR ?? '/home/bzp/code/adc-rust/fixtures/differ'; | |
| const OUT_FILE = | |
| process.env.ADC_DIFFER_FIXTURE_RESULTS_OUT ?? '/tmp/adc-ts-differ-fixture-results.json'; | |
| import { basename, dirname, extname, join } from 'node:path'; | |
| import { readdirSync, readFileSync, writeFileSync } from 'node:fs'; | |
| import { fileURLToPath } from 'node:url'; | |
| import { it } from 'vitest'; | |
| import { DifferV4 } from '../src/differv4.js'; | |
| const REPO_ROOT = join(dirname(fileURLToPath(import.meta.url)), '../../..'); | |
| const FIXTURES_DIR = | |
| process.env.ADC_DIFFER_FIXTURES_DIR ?? join(REPO_ROOT, 'fixtures/differ'); | |
| const OUT_FILE = | |
| process.env.ADC_DIFFER_FIXTURE_RESULTS_OUT ?? '/tmp/adc-ts-differ-fixture-results.json'; |
🤖 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 `@libs/differ/tools/dump-fixture-results.ts` around lines 20 - 23, Update the
FIXTURES_DIR default in dump-fixture-results.ts to resolve from the module’s
location, navigating three directory levels up to the repository root and then
into fixtures/differ. Preserve the ADC_DIFFER_FIXTURES_DIR environment-variable
override and remove the hardcoded developer-specific path.
| // SERVICE's resolver inspects `stream_routes`, which is itself one of the | ||
| // nested MAP fields about to be removed from local_item. | ||
| let default_type = meta.resolve_default_type.map(|f| f(&local_item)).unwrap_or(resource_type); | ||
| let default_value = self.default_value.core.get(&default_type).cloned().unwrap_or_else(|| json!({})); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
fd -i 'differ_v4.rs|differ_meta.rs' .
printf '%s\n' '--- relevant symbols and references ---'
rg -n -C 4 'default_value|diff_plugins|CollectionKind::Record|GlobalRule|PluginMetadata|global_rules|plugin_metadata|resolve_default_type' rust/crates/adc-differ rust 2>/dev/null | head -n 500
printf '%s\n' '--- differ_v4 outline ---'
ast-grep outline rust/crates/adc-differ/src/differ_v4.rs
printf '%s\n' '--- differ_meta outline ---'
ast-grep outline rust/crates/adc-differ/src/differ_meta.rsRepository: api7/adc
Length of output: 41646
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- focused source sections ---'
sed -n '120,330p' rust/crates/adc-differ/src/differ_v4.rs
printf '%s\n' '--- metadata definitions ---'
sed -n '1,280p' rust/crates/adc-differ/src/differ_meta.rs
printf '%s\n' '--- tests and fixtures mentioning plugin collections ---'
rg -n -C 6 'global_rule|global_rules|plugin_metadata|PluginMetadata|GlobalRule|default' rust/crates/adc-differ --glob '*test*' --glob '*.json' --glob '*.yaml' --glob '*.yml' 2>/dev/null | head -n 500Repository: api7/adc
Length of output: 19031
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- all repository references to record collections and defaults ---'
rg -n -C 8 'global_rules|plugin_metadata|GlobalRule|PluginMetadata|defaultValue|DefaultValue|default_value' . --glob '!target/**' --glob '!node_modules/**' | head -n 1200
printf '%s\n' '--- full differ_v4 helpers and entry flow ---'
sed -n '1,125p' rust/crates/adc-differ/src/differ_v4.rs
sed -n '296,455p' rust/crates/adc-differ/src/differ_v4.rs
printf '%s\n' '--- resource type and default-value declarations ---'
rg -n -C 6 'enum ResourceType|struct DefaultValue|type DefaultValue|pub struct DefaultValue|InternalConfiguration' rust/crates/adc-sdk rust/crates/adc-differ | head -n 800
printf '%s\n' '--- repository status and concise diff summary ---'
git status --short
git diff --statRepository: api7/adc
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- default-value providers ---'
rg -n -C 12 'defaultValue\(\)|defaultValue\s*=|core:.*global_rule|global_rule: \{\}|plugin_metadata: \{\}' libs apps rust --glob '*.ts' --glob '*.rs' | head -n 800
printf '%s\n' '--- TypeScript differ implementation and metadata ---'
fd -i 'differ.*\.(ts|tsx)$|meta.*\.(ts|tsx)$' libs
rg -n -C 10 'CollectionKind|global_rules|plugin_metadata|diffPlugins|defaultValue\.plugins|defaultValue\.core' libs/differ libs --glob '*.ts' --glob '*.tsx' | head -n 1000
printf '%s\n' '--- exact schema and SDK default-value declarations ---'
sed -n '1578,1612p' schema.json
rg -n -C 10 'DefaultValue|default_value|defaultValue' rust/crates/adc-sdk libs --glob '*.rs' --glob '*.ts' --glob '*.tsx' | head -n 1000Repository: api7/adc
Length of output: 50365
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- API7 defaultValue implementation ---'
sed -n '60,165p' libs/backend-api7/src/index.ts
printf '%s\n' '--- DefaultValue type and resource enum ---'
rg -n -C 12 'export interface DefaultValue|type DefaultValue|DefaultValue|enum ResourceType|GLOBAL_RULE|PLUGIN_METADATA' libs/sdk rust/crates/adc-sdk --glob '*.ts' --glob '*.rs' | head -n 1000
printf '%s\n' '--- focused TypeScript metadata entries ---'
rg -n -C 12 'GLOBAL_RULE|PLUGIN_METADATA|global_rules|plugin_metadata|collectionKind' libs/differ/src libs/sdk/src --glob '*.ts' | head -n 600
printf '%s\n' '--- focused TypeScript update path ---'
sed -n '250,350p' libs/differ/src/differv4.ts
printf '%s\n' '--- focused backend default tests ---'
sed -n '105,145p' libs/backend-api7/e2e/default-value.e2e-spec.tsRepository: api7/adc
Length of output: 46794
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Rust merge_default implementation ---'
sed -n '392,430p' rust/crates/adc-differ/src/differ_v4.rs
printf '%s\n' '--- Rust diff entry and resource iteration ---'
sed -n '30,115p' rust/crates/adc-differ/src/differ_v4.rs
printf '%s\n' '--- explicit behavioral probe: record item with plugin default ---'
python3 - <<'PY'
import copy, json
def merge_default(resource, defaults):
# Mirrors rust/crates/adc-differ/src/differ_v4.rs::merge_default.
if isinstance(resource, dict) and isinstance(defaults, dict):
out = copy.deepcopy(defaults)
for key, value in resource.items():
out[key] = merge_default(value, out[key]) if key in out else copy.deepcopy(value)
return out
return copy.deepcopy(resource)
def current_record_merge(local_item, resource_type, default_value):
# Mirrors handle_update: resolve_default_type is None for Record metadata.
default_type = resource_type
core_default = default_value.get("core", {}).get(default_type, {})
return merge_default(local_item, core_default)
def plugin_aware_record_merge(local_item, default_value):
return {
plugin: merge_default(config, default_value.get("plugins", {}).get(plugin, {}))
for plugin, config in local_item.items()
}
default_value = {
"core": {"global_rule": {}},
"plugins": {"prometheus": {"prefer_name": False}},
}
local = {"prometheus": {}}
remote = {"prometheus": {"prefer_name": False}}
current = current_record_merge(local, "global_rule", default_value)
expected = plugin_aware_record_merge(local, default_value)
print(json.dumps({
"current_merge": current,
"plugin_aware_merge": expected,
"remote": remote,
"current_matches_remote": current == remote,
"plugin_aware_matches_remote": expected == remote,
}, sort_keys=True))
assert current != remote
assert expected == remote
PYRepository: api7/adc
Length of output: 5331
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
import copy, json
def merge_default(resource, defaults):
# Mirrors rust/crates/adc-differ/src/differ_v4.rs::merge_default.
result = copy.deepcopy(resource)
if not isinstance(defaults, dict) or not isinstance(result, dict):
return result
for key, value in defaults.items():
existing = result.get(key)
if existing is None:
if not isinstance(value, (dict, list)):
result[key] = copy.deepcopy(value)
elif isinstance(value, dict) and isinstance(existing, dict):
result[key] = merge_default(existing, value)
elif isinstance(value, list) and isinstance(existing, list) and value:
result[key] = [merge_default(item, value[0]) for item in existing]
return result
# extract_tuples() passes each record value, not the enclosing record, to handle_update().
plugin_name = "prometheus"
local_item = {}
remote_item = {"prefer_name": False}
default_value = {
"core": {"global_rule": {}},
"plugins": {plugin_name: {"prefer_name": False}},
}
# Current handle_update path: GlobalRule has no resolver, so it uses
# default_value.core["global_rule"]; remote_name is not used for lookup.
current = merge_default(local_item, default_value["core"]["global_rule"])
# Required per-plugin path: use the record key carried as remote_name.
expected = merge_default(local_item, default_value["plugins"][plugin_name])
print(json.dumps({
"tuple": [plugin_name, plugin_name, local_item],
"current_merge": current,
"plugin_aware_merge": expected,
"remote_item": remote_item,
"current_matches_remote": current == remote_item,
"plugin_aware_matches_remote": expected == remote_item,
}, sort_keys=True))
assert current != remote_item
assert expected == remote_item
PYRepository: api7/adc
Length of output: 366
Merge per-plugin defaults for GlobalRule and PluginMetadata records.
extract_tuples passes each record value to handle_update, while the record key is the plugin name. handle_update reads only default_value.core[resource_type] and ignores default_value.plugins[plugin_name]. A remote plugin default can therefore cause a spurious update. Use remote_name to merge the corresponding plugin default, and add regression fixtures for both record collections.
🤖 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 `@rust/crates/adc-differ/src/differ_v4.rs` around lines 172 - 175, Update
handle_update’s default-value selection to merge
default_value.plugins[remote_name] into default_value.core[resource_type] when
processing GlobalRule and PluginMetadata records, while preserving core defaults
when no plugin-specific entry exists. Ensure extract_tuples passes the plugin
key through as remote_name, and add regression fixtures covering both record
collections with plugin-specific defaults.
| /// `#[serde(tag = "type")]` keeps the wire format identical to a flat struct with | ||
| /// a `type` discriminant field: `{"type": "create", "newValue": ...}`. | ||
| #[derive(Debug, Clone, PartialEq, Serialize)] | ||
| #[serde(tag = "type", rename_all = "snake_case")] | ||
| pub enum EventKind { | ||
| Create { | ||
| new_value: Value, | ||
| }, | ||
| Delete { | ||
| old_value: Value, | ||
| }, | ||
| Update { | ||
| old_value: Value, | ||
| new_value: Value, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| diff: Option<Vec<ValueDiff>>, | ||
| }, | ||
| /// Internal use only: exists to carry `sub_events` up to the caller during | ||
| /// tree construction. Never appears in the differ's final flattened event | ||
| /// list, so it carries no payload of its own. | ||
| OnlySubEvents, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the key casing the TypeScript fixtures and comparison script expect.
fd -t f -e json . fixtures/differ --max-results 2 --exec sh -c 'echo "== {}"; head -c 1500 {}'
fd -t f 'compare-differ-fixtures.mjs' scripts --exec cat -n {}
fd -t f 'dump-fixture-results.ts' libs --exec cat -n {}Repository: api7/adc
Length of output: 6767
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '== event.rs =='
cat -n rust/crates/adc-sdk/src/event.rs | sed -n '1,115p'
printf '%s\n' '== adc-sdk manifest =='
cat -n rust/crates/adc-sdk/Cargo.toml
printf '%s\n' '== serde versions/features =='
rg -n '^(name = "serde"|version = ".*"|serde_json|serde =)' rust/Cargo.lock rust/Cargo.toml rust/crates -g 'Cargo.toml' | head -120
printf '%s\n' '== event construction and serialization =='
rg -n 'EventKind::|Event \{|serde_json::|to_value|to_string' rust/crates -g '*.rs' | head -240
printf '%s\n' '== TypeScript event definitions and casing =='
rg -n 'resourceType|resourceId|resourceName|oldValue|newValue|parentId|EventType|EventKind' libs -g '*.ts' -g '*.js' | head -240Repository: api7/adc
Length of output: 48116
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '== locked serde packages =='
rg -n -A12 -B2 'name = "serde(-derive|-json)?"' rust/Cargo.lock
printf '%s\n' '== workspace lock header and serde dependency =='
sed -n '1,30p' rust/Cargo.lock
rg -n -A3 -B2 'serde = ' rust/Cargo.lock rust/Cargo.toml
printf '%s\n' '== fixture filenames and event-shaped expected data =='
find fixtures/differ -maxdepth 1 -type f -name '*.json' -print | sort | head -20
python3 - <<'PY'
import json
from pathlib import Path
for path in sorted(Path("fixtures/differ").glob("*.json")):
data = json.loads(path.read_text())
print(path.name, sorted(data))
PY
printf '%s\n' '== direct event serialization assertions =='
rg -n -C4 'serialize|to_value|to_string|resource_type|new_value|old_value|parent_id|resourceType|newValue|oldValue' rust/crates/adc-sdk rust/crates/adc-differ -g '*.rs' | head -260Repository: api7/adc
Length of output: 27360
🌐 Web query:
Serde derive rename_all_fields enum struct variant fields official documentation
💡 Result:
In Serde, there are distinct attributes used to manage the renaming of struct fields, enum variants, and fields within enum variants [1][2]. 1. #[serde(rename_all = "...")] This container-level attribute applies to a struct or an enum [1][2]. When used on a struct, it renames all of its fields according to the specified case convention (e.g., camelCase, snake_case) [1][3]. When used on an enum, it renames all of its variants [1][4]. 2. #[serde(rename_all_fields = "...")] This container-level attribute is specifically for enums [1]. It applies a rename rule to every field of every struct-like variant within an enum [1]. This is useful when you have an enum where variants contain struct-like data and you want those internal fields to follow a specific naming convention automatically [1][5]. Supported case conventions for both attributes include: "lowercase", "UPPERCASE", "PascalCase", "camelCase", "snake_case", "SCREAMING_SNAKE_CASE", "kebab-case", and "SCREAMING-KEBAB-CASE" [1]. Additionally, both attributes allow for independent configuration for serialization and deserialization [1]: #[serde(rename_all(serialize = "..."))] #[serde(rename_all(deserialize = "..."))] #[serde(rename_all(serialize = "...", deserialize = "..."))] These same serialization/deserialization overrides apply to rename_all_fields as well [1].
Citations:
- 1: https://serde.rs/container-attrs
- 2: https://serde.rs/attributes.html
- 3: https://serde.rs/attr-rename.html
- 4: How can I deserialize CamelCased field names to snake_case serde-rs/serde#1435
- 5: Apply rename_all to enum member "fields" serde-rs/serde#1061
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import re
from pathlib import Path
source = Path("rust/crates/adc-sdk/src/event.rs").read_text()
ts_keys = {
"new_value": "newValue",
"old_value": "oldValue",
"resource_type": "resourceType",
"resource_id": "resourceId",
"resource_name": "resourceName",
"parent_id": "parentId",
}
assert '#[serde(tag = "type", rename_all = "snake_case")]' in source
assert '#[serde(rename_all = "camelCase")]' not in source
assert 'rename_all_fields = "camelCase"' not in source
enum_body = source.split("pub enum EventKind {", 1)[1].split("\n}", 1)[0]
enum_fields = set(re.findall(r"^\s{8}([a-z][a-z0-9_]*)\s*:", enum_body, re.M))
event_body = source.split("pub struct Event {", 1)[1].split("\n}", 1)[0]
event_fields = set(re.findall(r"^\s{4}pub ([a-z][a-z0-9_]*)\s*:", event_body, re.M))
assert enum_fields == {"new_value", "old_value", "diff"}
assert event_fields == {
"resource_type", "resource_id", "resource_name", "parent_id"
}
def snake_case(value):
return value
for field in sorted(enum_fields | event_fields):
if field in ts_keys:
assert snake_case(field) != ts_keys[field], field
print("EventKind payload fields and Event envelope fields lack the required camelCase mappings.")
print("The documented type tag remains unaffected by field renaming.")
PYRepository: api7/adc
Length of output: 235
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import re
from pathlib import Path
source = Path("rust/crates/adc-sdk/src/event.rs").read_text()
enum_body = source.split("pub enum EventKind {", 1)[1].split("\n}", 1)[0]
event_body = source.split("pub struct Event {", 1)[1].split("\n}", 1)[0]
print("enum fields:", sorted(re.findall(r"^\s+([a-z][a-z0-9_]*)\s*:", enum_body, re.M)))
print("event fields:", sorted(re.findall(r"^\s+pub ([a-z][a-z0-9_]*)\s*:", event_body, re.M)))
print("has EventKind camelCase field rule:",
'rename_all_fields = "camelCase"' in source)
print("has Event camelCase rule:",
'#[serde(rename_all = "camelCase")]' in event_body)
PYRepository: api7/adc
Length of output: 379
Use camelCase for serialized event fields. rename_all = "snake_case" renames enum variants, not struct-variant fields. Add #[serde(rename_all_fields = "camelCase")] to EventKind and #[serde(rename_all = "camelCase")] to Event. This must serialize newValue, oldValue, resourceType, resourceId, resourceName, and parentId to match the TypeScript differ and the documented format.
📍 Affects 1 file
rust/crates/adc-sdk/src/event.rs#L26-L47(this comment)rust/crates/adc-sdk/src/event.rs#L82-L93
🤖 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 `@rust/crates/adc-sdk/src/event.rs` around lines 26 - 47, Update EventKind with
Serde field renaming so its struct-variant fields serialize as camelCase,
including newValue and oldValue, while preserving snake_case variant names. Also
update the Event definition at rust/crates/adc-sdk/src/event.rs lines 82-93 with
camelCase field renaming so resourceType, resourceId, resourceName, and parentId
match the documented wire format.
| #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct ConsumerCredential { | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub id: Option<String>, | ||
| pub name: String, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub description: Option<String>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub labels: Option<Labels>, | ||
|
|
||
| #[serde(rename = "type")] | ||
| pub r#type: String, | ||
| pub config: Plugin, | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Secret-bearing resource structs leak plaintext via derived Debug. ConsumerCredential.config, Route/StreamRoute.plugins, Service.plugins, and the aggregate Configuration/InternalConfiguration all derive Debug with no redaction, and all can carry API keys, passwords, or JWT secrets (confirmed for ConsumerCredential.config by the adc-differ/tests/consumer.rs payloads). The shared root cause is one missing redaction layer around Plugin/Plugins-typed fields.
rust/crates/adc-sdk/src/resources/consumer.rs#L11-L25: add a customDebugimpl (or a redacting wrapper type) forConsumerCredential.config, since this field directly stores raw credential secrets.rust/crates/adc-sdk/src/resources/mod.rs#L46-L90: apply the same redactingDebugtoConfiguration/InternalConfiguration, since they aggregate every nested secret-bearing field.rust/crates/adc-sdk/src/resources/route.rs#L32-L87: apply the same redactingDebugtoRoute.plugins/StreamRoute.plugins.rust/crates/adc-sdk/src/resources/service.rs#L63-L89: apply the same redactingDebugtoService.plugins.
📍 Affects 4 files
rust/crates/adc-sdk/src/resources/consumer.rs#L11-L25(this comment)rust/crates/adc-sdk/src/resources/mod.rs#L46-L90rust/crates/adc-sdk/src/resources/route.rs#L32-L87rust/crates/adc-sdk/src/resources/service.rs#L63-L89
🤖 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 `@rust/crates/adc-sdk/src/resources/consumer.rs` around lines 11 - 25, Prevent
plaintext secrets from appearing in derived Debug output by adding a shared
redacting Debug implementation or wrapper for Plugin/Plugins-typed fields.
Update ConsumerCredential.config in
rust/crates/adc-sdk/src/resources/consumer.rs:11-25,
Configuration/InternalConfiguration in
rust/crates/adc-sdk/src/resources/mod.rs:46-90, Route/StreamRoute.plugins in
rust/crates/adc-sdk/src/resources/route.rs:32-87, and Service.plugins in
rust/crates/adc-sdk/src/resources/service.rs:63-89 so their Debug
representations redact credential values while preserving non-secret fields.
| #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct SSLCertificate { | ||
| pub certificate: String, | ||
| pub key: String, | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact key in the Debug output of SSLCertificate.
SSLCertificate.key holds private key material, either inline PEM or a $secret:// reference. The derived Debug prints that value verbatim. Any {:?} formatting leaks the key, including tracing events, panic! messages, and failed assert_eq! output in tests. Serialize must stay unredacted because the Admin API payload needs the real value. Implement Debug manually instead of deriving it.
🔒 Proposed fix: manual redacting `Debug` impl
-#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+#[derive(Clone, PartialEq, Serialize, Deserialize)]
#[serde(deny_unknown_fields)]
pub struct SSLCertificate {
pub certificate: String,
pub key: String,
}
+
+impl std::fmt::Debug for SSLCertificate {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ f.debug_struct("SSLCertificate")
+ .field("certificate", &self.certificate)
+ .field("key", &"[REDACTED]")
+ .finish()
+ }
+}As per coding guidelines: "Scan for code that logs, serializes, or returns API keys, tokens ... without redaction".
📝 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.
| #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] | |
| #[serde(deny_unknown_fields)] | |
| pub struct SSLCertificate { | |
| pub certificate: String, | |
| pub key: String, | |
| } | |
| #[derive(Clone, PartialEq, Serialize, Deserialize)] | |
| #[serde(deny_unknown_fields)] | |
| pub struct SSLCertificate { | |
| pub certificate: String, | |
| pub key: String, | |
| } | |
| impl std::fmt::Debug for SSLCertificate { | |
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |
| f.debug_struct("SSLCertificate") | |
| .field("certificate", &self.certificate) | |
| .field("key", &"[REDACTED]") | |
| .finish() | |
| } | |
| } |
🤖 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 `@rust/crates/adc-sdk/src/resources/ssl.rs` around lines 34 - 39, Replace the
derived Debug implementation on SSLCertificate with a manual implementation that
preserves the certificate field but always redacts key, including inline PEM and
$secret:// references. Keep Serialize and Deserialize derives unchanged so API
serialization still emits the actual key value.
Source: Coding guidelines
| fn diff_array(la: &[Value], ra: &[Value], path: &[PathSegment], changes: &mut Vec<ValueDiff>) { | ||
| let mut i = ra.len() as isize - 1; | ||
| let mut j = la.len() as isize - 1; | ||
|
|
||
| while i > j { | ||
| let idx = i as usize; | ||
| changes.push(ValueDiff::Array { | ||
| path: path.to_vec(), | ||
| index: idx, | ||
| item: Box::new(ValueDiff::New { path: vec![], rhs: ra[idx].clone() }), | ||
| }); | ||
| i -= 1; | ||
| } | ||
| while j > i { | ||
| let idx = j as usize; | ||
| changes.push(ValueDiff::Array { | ||
| path: path.to_vec(), | ||
| index: idx, | ||
| item: Box::new(ValueDiff::Deleted { path: vec![], lhs: la[idx].clone() }), | ||
| }); | ||
| j -= 1; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect an array-diff entry in the TS fixtures and the comparison logic.
rg -n --json=never -l '"kind": *"A"' fixtures/differ | head -5
rg -n -C6 '"kind": *"A"' fixtures/differ | head -60
fd -t f 'compare-differ-fixtures.mjs' scripts --exec cat -n {}Repository: api7/adc
Length of output: 4680
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Rust value diff definitions and implementation ---'
rg -n -C8 'enum ValueDiff|struct DiffPath|fn diff_array|skip_serializing_if|Array \{' rust/crates/adc-sdk/src/value_diff.rs
printf '%s\n' '--- TypeScript array-diff serialization and fixtures ---'
rg -n -C8 '"kind": "A"|kind.*A|path|array' fixtures/differ libs/differ -g '*.json' -g '*.ts' -g '*.tsx' | head -240
printf '%s\n' '--- Comparison script ---'
cat -n scripts/compare-differ-fixtures.mjsRepository: api7/adc
Length of output: 25156
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- TypeScript differ implementation ---'
rg -n -C12 'deep-diff|DiffResult|kind: .A|item:|path: undefined|array' libs/differ/src libs/sdk/src -g '*.ts' -g '*.tsx' | head -320
printf '%s\n' '--- Fixture runner and fixture inventory ---'
cat -n libs/differ/tools/dump-fixture-results.ts | sed -n '1,180p'
printf '%s\n' '--- Existing array-tail fixture or test cases ---'
rg -n -C12 'kind.*A|array.*tail|tail.*array|path: \[\]' . -g '*.ts' -g '*.json' -g '*.rs' | head -320Repository: api7/adc
Length of output: 47458
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- TypeScript imports and diff construction ---'
cat -n libs/differ/src/differv4.ts | sed -n '1,90p'
rg -n -C20 'diff\(|Diff<|diff as|from .deep-diff|from .deepDiff|deepDiff|observableDiff|isArray' libs/differ/src/differv4.ts libs/differ/src libs/sdk/src -g '*.ts'
printf '%s\n' '--- Dependency and type declarations for deep-diff ---'
rg -n -C8 'deep-diff|interface.*Diff|type.*Diff|ArrayDiff|NewDiff|DeletedDiff' package.json package-lock.json pnpm-lock.yaml yarn.lock libs rust -g '*' | head -240Repository: api7/adc
Length of output: 50365
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Diff call and output path ---'
cat -n libs/differ/src/differv4.ts | sed -n '320,365p'
printf '%s\n' '--- Tracked datum-diff references and package metadata ---'
git ls-files | rg '(^|/)(datum-diff|package-lock|pnpm-lock|yarn.lock|node_modules)' | head -120
rg -n -C10 'datum-diff' . -g 'package.json' -g 'pnpm-lock.yaml' -g 'package-lock.json' -g 'yarn.lock' -g '*.d.ts' -g '*.ts' | head -180
printf '%s\n' '--- Fixture inputs containing arrays likely passed to objectDiff ---'
python3 - <<'PY'
import json, pathlib
for p in sorted(pathlib.Path('fixtures/differ').glob('*.json')):
try:
d=json.loads(p.read_text())
except Exception:
continue
def arrays(x, path=()):
if isinstance(x, list):
yield path, len(x)
for i,v in enumerate(x): yield from arrays(v, path+(i,))
elif isinstance(x, dict):
for k,v in x.items(): yield from arrays(v, path+(k,))
found=list(arrays(d))
if found:
print(p, found[:8])
PYRepository: api7/adc
Length of output: 18593
🌐 Web query:
datum-diff 1.0.4 npm Diff ArrayDiff path undefined item serialization
💡 Result:
The datum-diff package (version 1.0.4) is a TypeScript-based, drop-in replacement for the deprecated and unmaintained deep-diff library [1][2]. It was developed as part of the datum-merge project to address bugs and migration needs [1][3][4]. Regarding the specific technical aspects queried: 1. ArrayDiff and Path: datum-diff inherits the interface and logic of the original deep-diff library it replaces [1][2]. In the context of deep-diff (and thus datum-diff), ArrayDiff is a category of change where items are added, removed, or edited within an array [1][2]. The path property is an array of strings and/or numbers representing the nested location of the change within the object structure [1][2]. If a path is undefined or encountered during item serialization, it typically indicates that the library is attempting to traverse a property that does not exist on the target or source object, or it is encountering an issue during the serialization of complex objects or circular references [1][5]. 2. Item Serialization: The library performs diff operations by traversing objects and comparing them [6]. When handling array items or nested objects, it relies on standard JavaScript object property access [1][6]. If you are encountering serialization errors or undefined paths, ensure that the data structures being compared are valid JSON-compatible objects and do not contain circular references, as deep-diff (and its derivatives) may not handle these scenarios without manual preprocessing [1][2]. Since datum-diff is essentially a TypeScript migration of the original deep-diff, you can consult the historical documentation or issue trackers for deep-diff for common pitfalls related to path handling and array operations, as the core logic remains similar [1][2]. For complex data, it is recommended to ensure your input objects are cleaned or normalized before passing them to the diff function [6].
Citations:
- 1: https://www.npmjs.com/package/datum-diff
- 2: https://github.com/therohk/datum-merge/blob/main/src/diff-lib/README.md
- 3: https://libraries.io/npm/datum-diff
- 4: Extract deep diff into a separate packages therohk/datum-merge#2
- 5: therohk/datum-merge@d5e2ce2
- 6: https://github.com/therohk/datum-merge
🌐 Web query:
github datum-diff 1.0.4 source ArrayDiff path item
💡 Result:
The library datum-diff (version 1.0.4) is a TypeScript-based, standalone package published as a drop-in replacement for the deprecated and unmaintained deep-diff library [1][2][3]. It does not feature a native ArrayDiff class in its public API or internal source structure that manages pathing or items in the manner typical of specialized array diffing libraries (such as @thi.ng/diff) [1][2][4]. The datum-diff library focuses on providing compatibility with the deep-diff interface, primarily exposing functions like diff, applyChange, and accumulateDiff [1][2]. These functions operate on deeply nested objects and arrays by identifying changes at specific paths and representing them through Diff objects, rather than via a dedicated ArrayDiff class [1][5][2]. If you are encountering ArrayDiff in your environment, it is likely originating from a different dependency, as several other popular libraries in the ecosystem (e.g., @thi.ng/diff, diff packages in Rust or PHP, and Java's NetBeans platform) utilize a class or type named ArrayDiff [6][7][8][9][4]. You can verify this by checking your project's package.json or generated lock file to identify which library is providing that definition.
Citations:
- 1: https://www.npmjs.com/package/datum-diff
- 2: https://github.com/therohk/datum-merge/blob/main/src/diff-lib/README.md
- 3: https://libraries.io/npm/datum-diff
- 4: https://docs.thi.ng/umbrella/diff/functions/diffArray.html
- 5: Extract deep diff into a separate packages therohk/datum-merge#2
- 6: https://cdn.jsdelivr.net/npm/diff@8.0.4/libesm/diff/array.d.ts
- 7: https://bits.netbeans.org/dev/javadoc/org-netbeans-swing-tabcontrol/org/netbeans/swing/tabcontrol/event/ArrayDiff.html
- 8: https://github.com/philiprehberger/php-diff
- 9: https://docs.rs/diff-struct/latest/diff/struct.ArrayDiff.html
🏁 Script executed:
#!/bin/bash
set -e
url=$(curl -fsSL https://registry.npmjs.org/datum-diff/1.0.4 | python3 -c 'import json,sys; print(json.load(sys.stdin)["dist"]["tarball"])')
printf 'tarball: %s\n' "$url"
curl -fsSL "$url" | tar -tzf - | head -80
printf '%s\n' '--- package source excerpts ---'
curl -fsSL "$url" | tar -xOzf - package/dist/index.js 2>/dev/null | head -240 || true
curl -fsSL "$url" | tar -xOzf - package/index.js 2>/dev/null | head -240 || true
printf '%s\n' '--- type declarations ---'
curl -fsSL "$url" | tar -tzf - | grep -E '\.(d\.ts|ts)$' | head -40Repository: api7/adc
Length of output: 575
🏁 Script executed:
#!/bin/bash
set -e
url=$(curl -fsSL https://registry.npmjs.org/datum-diff/1.0.4 | python3 -c 'import json,sys; print(json.load(sys.stdin)["dist"]["tarball"])')
printf '%s\n' '--- Diff type declarations ---'
curl -fsSL "$url" | tar -xOzf - package/dist-diff/dts/deep-diff.d.ts
printf '%s\n' '--- Array diff implementation ---'
curl -fsSL "$url" | tar -xOzf - package/dist-diff/esm/deep-diff.js | rg -n -C12 'kind.*A|Array|path|item|newChange|Diff'Repository: api7/adc
Length of output: 18638
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
import json
from pathlib import Path
rust = Path("rust/crates/adc-sdk/src/value_diff.rs").read_text()
ts = Path("libs/differ/src/differv4.ts").read_text()
assert 'item: Box::new(ValueDiff::New { path: vec![], rhs:' in rust
assert 'item: Box::new(ValueDiff::Deleted { path: vec![], lhs:' in rust
assert 'const diff = objectDiff(cloneDeep(remoteItem), mergedLocalItem);' in ts
# Model the two serialized tail entries. JSON.stringify omits an undefined
# object property, while serde serializes the Rust path field unconditionally.
ts_entry = {"kind": "A", "index": 2, "item": {"kind": "N", "rhs": 3}}
rust_entry = {"kind": "A", "index": 2, "item": {"kind": "N", "path": [], "rhs": 3}}
assert "path" not in ts_entry["item"]
assert "path" in rust_entry["item"]
assert ts_entry != rust_entry
print("Rust tail items contain path: []")
print("TypeScript datum-diff tail items contain no path after JSON.stringify")
print("The comparison script's shallow envelope normalization does not remove either field")
PYRepository: api7/adc
Length of output: 333
Omit path from nested array-tail items. datum-diff emits path: undefined for these items, and JSON.stringify omits it. Rust emits path: [], while the comparison script does not normalize this field. Omit path only from Array.item; root diffs still require path: [].
🤖 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 `@rust/crates/adc-sdk/src/value_diff.rs` around lines 119 - 140, Update the
nested `ValueDiff::New` and `ValueDiff::Deleted` constructions in `diff_array`
so their `item` payloads omit the `path` field, matching `datum-diff`
serialization for array-tail items. Preserve `path: path.to_vec()` on the outer
`ValueDiff::Array` entries and keep root diff paths unchanged.
| let args: Vec<String> = env::args().collect(); | ||
| let fixture_path = args.get(1).expect("usage: adc-sync-bench <fixture.json> <server_base_url> [concurrency] [iterations] [runtime]"); | ||
| let base_url = args.get(2).expect("missing server_base_url").clone(); | ||
| let concurrency: usize = args.get(3).map(|s| s.parse().unwrap()).unwrap_or(10); | ||
| let iterations: usize = args.get(4).map(|s| s.parse().unwrap()).unwrap_or(3); | ||
| let runtime_flavor = args.get(5).map(String::as_str).unwrap_or("current"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reject invalid benchmark arguments.
If iterations is 0, both sample vectors remain empty. Line 148 then indexes an empty vector and panics. Non-numeric values also panic at Lines 113-114. An unsupported runtime value silently selects the current-thread runtime.
Parse numeric arguments with a usage error. Require positive concurrency and iterations values. Reject runtime values other than current and multi.
🤖 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 `@rust/crates/adc-sync-bench/src/main.rs` around lines 110 - 115, Update the
argument parsing in main around concurrency, iterations, and runtime_flavor to
report invalid input as a usage error instead of panicking or silently falling
back. Parse numeric values fallibly, require both concurrency and iterations to
be greater than zero, and accept only “current” or “multi” for runtime_flavor;
reject all other values before benchmark execution.
| for (const name of allNames) { | ||
| const tsEvents = tsResults[name]; | ||
| const rustEvents = rustResults[name]; | ||
|
|
||
| if (tsEvents === undefined) { | ||
| failures.push({ name, reason: 'missing from TS results' }); | ||
| continue; | ||
| } | ||
| if (rustEvents === undefined) { | ||
| failures.push({ name, reason: 'missing from Rust results' }); | ||
| continue; | ||
| } | ||
|
|
||
| const normalizedRust = rustEvents.map(normalizeEvent); | ||
| if (isDeepStrictEqual(tsEvents, normalizedRust)) { | ||
| passCount++; | ||
| } else { | ||
| failures.push({ name, reason: 'output mismatch', ts: tsEvents, rust: normalizedRust }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against non-array fixture results.
Line 84 calls rustEvents.map directly. If the Rust binary writes a non-array value for one fixture, .map throws a TypeError and the script aborts. The remaining fixtures are then never compared, and the output gives no fixture name. Check the shape first and record a failure instead.
🛡️ Proposed fix to report shape errors as failures
+ if (!Array.isArray(tsEvents) || !Array.isArray(rustEvents)) {
+ failures.push({ name, reason: 'result is not an array of events', ts: tsEvents, rust: rustEvents });
+ continue;
+ }
+
const normalizedRust = rustEvents.map(normalizeEvent);📝 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.
| for (const name of allNames) { | |
| const tsEvents = tsResults[name]; | |
| const rustEvents = rustResults[name]; | |
| if (tsEvents === undefined) { | |
| failures.push({ name, reason: 'missing from TS results' }); | |
| continue; | |
| } | |
| if (rustEvents === undefined) { | |
| failures.push({ name, reason: 'missing from Rust results' }); | |
| continue; | |
| } | |
| const normalizedRust = rustEvents.map(normalizeEvent); | |
| if (isDeepStrictEqual(tsEvents, normalizedRust)) { | |
| passCount++; | |
| } else { | |
| failures.push({ name, reason: 'output mismatch', ts: tsEvents, rust: normalizedRust }); | |
| } | |
| } | |
| for (const name of allNames) { | |
| const tsEvents = tsResults[name]; | |
| const rustEvents = rustResults[name]; | |
| if (tsEvents === undefined) { | |
| failures.push({ name, reason: 'missing from TS results' }); | |
| continue; | |
| } | |
| if (rustEvents === undefined) { | |
| failures.push({ name, reason: 'missing from Rust results' }); | |
| continue; | |
| } | |
| if (!Array.isArray(tsEvents) || !Array.isArray(rustEvents)) { | |
| failures.push({ name, reason: 'result is not an array of events', ts: tsEvents, rust: rustEvents }); | |
| continue; | |
| } | |
| const normalizedRust = rustEvents.map(normalizeEvent); | |
| if (isDeepStrictEqual(tsEvents, normalizedRust)) { | |
| passCount++; | |
| } else { | |
| failures.push({ name, reason: 'output mismatch', ts: tsEvents, rust: normalizedRust }); | |
| } | |
| } |
🤖 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 `@scripts/compare-differ-fixtures.mjs` around lines 71 - 90, Validate that
rustEvents is an array before the normalizeEvent mapping in the comparison loop.
When the value has an invalid shape, append a failure for the current name with
a clear shape-error reason and continue processing the remaining fixtures; only
call map and compare outputs for valid arrays.
bzp2010
added
test/api7
1 participant
Description
Fixes # (issue)
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests