diff --git a/docs/architecture/migration-progress.md b/docs/architecture/migration-progress.md index 765e89fc2..e5403bf83 100644 --- a/docs/architecture/migration-progress.md +++ b/docs/architecture/migration-progress.md @@ -5,13 +5,13 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block ## Current Context - Issue: [`rustfs/backlog#660`](https://github.com/rustfs/backlog/issues/660) -- Branch: `overtrue/arch-migration-guardrails` -- Baseline: `upstream/main` at `61f0dfbc40f748be313be84d834d8259cf3e19c9` -- PR type for this branch: `docs-only` +- Branch: `overtrue/arch-migration-ci-gates` +- Baseline: `upstream/main` at `f00898d0703ac94a4a215a54b6666c747ebe6622` +- PR type for this branch: `ci-gate` - Runtime behavior changes: none - Rust code changes: none -- Repository metadata changes: `.gitignore` now allows tracking only - `docs/architecture/` under the otherwise ignored `docs` tree. +- Script changes: stabilize `scripts/check_layer_dependencies.sh` against the + current accepted baseline. ## Phase 0 Tasks @@ -25,11 +25,11 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block allowed PR type per PR. - [x] `G-004` Define re-export and wrapper policy. - Acceptance: temporary compatibility code must use `RUSTFS_COMPAT_TODO`. -- [~] `G-005` Add dependency direction guard. - - Current branch: documentation only. - - Next PR: add a `ci-gate` check for forbidden dependency edges. +- [x] `G-005` Add dependency direction guard. + - Acceptance: `./scripts/check_layer_dependencies.sh` passes on current + `upstream/main` while still rejecting new unaccepted layer dependencies. - [~] `G-006` Create migration loss-prevention checks. - - Current branch: documentation only. + - Current branch: keeps the existing layer guard stable. - Next PR: add checks for public re-export, route matrix, and storage trait coverage before pure moves. - [x] `G-009` Enforce pre-push three-expert review. @@ -37,48 +37,56 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block quality/architecture, migration-preservation, and testing/verification review before push. - [~] `TEST-PRTYPE-001` Check PR type enum consistency. - - Current branch: documentation only. + - Current branch: not in scope. - Next PR: add a mechanical check that all migration docs use the same PR type vocabulary. ## Next PRs -1. `ci-gate`: extend `scripts/check_layer_dependencies.sh` or add a nearby check - for architecture-migration guardrails. -2. `docs-only` or `test-only`: capture startup timeline and admin route-action +1. `docs-only` or `test-only`: capture startup timeline and admin route-action snapshot. -3. `docs-only`: inventory `ecstore::config::{Config, KV, KVS}` consumers. +2. `docs-only`: inventory `ecstore::config::{Config, KV, KVS}` consumers. +3. `ci-gate`: add focused checks for PR type vocabulary and temporary + compatibility marker/register consistency. ## Pre-Push Review Log | Expert | Status | Notes | |---|---|---| -| Quality/architecture | pass | Staged diff is scoped to docs, `.gitignore`, and the root architecture index; no over-abstraction or target drift found | -| Migration preservation | pass | No runtime files changed; notify/audit and IAM/KMS no-drift bullets were added after review feedback | -| Testing/verification | pass-with-nonblocking-follow-up | Docs-only verification is sufficient; suggested commands are recorded below | +| Quality/architecture | pass | Final review confirmed the guard scope, baseline logic, and storage target classification; the segment-level `storage::ecfs` / `storage::s3_api` follow-up passed | +| Migration preservation | pass | Final review confirmed no Rust, runtime, storage hot-path, or global-state logic changes and no missing `RUSTFS_COMPAT_TODO` marker | +| Testing/verification | pass | Final review accepted the full gate, focused guard checks, stale-baseline check, and negative baseline-removal check as sufficient for this `ci-gate` PR | ## Verification Notes Passed: -- `git diff --cached --check` -- `git diff --cached --name-only -- '*.rs' 'Cargo.toml' 'Cargo.lock' '.github/**' 'scripts/**' 'Makefile' 'Justfile'` -- `git diff --cached --exit-code -- '*.rs' 'Cargo.toml' 'Cargo.lock' '.github/**' 'scripts/**' 'Makefile' 'Justfile'` -- `printf '%s\n' docs/architecture/overview.md docs/foo.md docs/other/file.md | git check-ignore -v --stdin --no-index` -- `git rev-parse upstream/main` -- `git log -1 --format='%H %s' upstream/main` +- `./scripts/check_layer_dependencies.sh` - `./scripts/check_metrics_migration_refs.sh` - -Known unrelated baseline issue: - -- `./scripts/check_layer_dependencies.sh` currently fails on `upstream/main` - because the script output and `scripts/layer-dependency-baseline.txt` format are - out of sync. Keep that fix in the next `ci-gate` PR. +- `bash -n scripts/check_layer_dependencies.sh` +- `make pre-commit` +- temporary negative check that adds an unaccepted reverse dependency to a copied + fixture and confirms the guard fails +- temporary negative check that adds single-line root grouped + `use crate::{admin::bad};` and confirms the guard fails +- temporary negative check that adds multiline root grouped + `use crate::{ ... }` and confirms the guard fails +- temporary negative check that adds root grouped alias + `use crate::{admin as admin_alias};` and confirms the guard fails +- temporary negative check that adds grouped `self` imports and confirms the + guard fails +- temporary negative check that adds a stale baseline row and confirms the guard + fails +- temporary negative check that removes the accepted + `crate::storage::ecfs::FS` baseline row for `rustfs/src/protocols/client.rs` + and confirms the guard fails +- `git diff --check` +- focused shell review of changed scripts ## Handoff Notes - Keep Phase 0 PRs small. Do not start Config, Storage API, Runtime, or ECStore - movement inside this docs branch. + movement inside this `ci-gate` branch. - Keep CI checks in a separate `ci-gate` PR so the PR type rule remains enforceable. - Do not add temporary compatibility code without a matching `RUSTFS_COMPAT_TODO()` marker and cleanup-register entry. diff --git a/scripts/check_layer_dependencies.sh b/scripts/check_layer_dependencies.sh index 141bf0685..32f41fc07 100755 --- a/scripts/check_layer_dependencies.sh +++ b/scripts/check_layer_dependencies.sh @@ -27,6 +27,7 @@ classify_source_layer() { classify_target_layer() { local path="$1" local root="${path%%::*}" + local storage_path case "$root" in app) @@ -36,7 +37,9 @@ classify_target_layer() { printf 'interface' ;; storage) - if [[ "$path" == ecfs* ]] || [[ "$path" == s3_api* ]]; then + storage_path="${path#storage::}" + if [[ "$storage_path" == "ecfs" ]] || [[ "$storage_path" == ecfs::* ]] || + [[ "$storage_path" == "s3_api" ]] || [[ "$storage_path" == s3_api::* ]]; then printf 'interface' else printf 'infra' @@ -65,6 +68,168 @@ layer_rank() { esac } +normalize_import_group_item() { + local prefix="$1" + local item="$2" + local path nested_prefix nested_items + + item="$(sed -E 's/^[[:space:]]+//; s/[[:space:]]+$//; s/[[:space:]]+as[[:space:]].*$//' <<<"$item")" + item="$(sed -E 's/[[:space:]]//g' <<<"$item")" + prefix="$(sed -E 's/[[:space:]]//g' <<<"$prefix")" + + [[ -z "$item" ]] && return 0 + if [[ "$item" == "self" ]]; then + prefix="${prefix%::}" + [[ -n "$prefix" ]] && printf '%s\n' "$prefix" + return 0 + fi + + if [[ "$item" == *"{"* ]]; then + nested_prefix="${item%%\{*}" + nested_items="${item#*\{}" + nested_items="${nested_items%\}*}" + nested_prefix="${nested_prefix%::}" + if [[ -n "$prefix" ]]; then + nested_prefix="${prefix}::${nested_prefix}" + fi + normalize_import_group "$nested_prefix" "$nested_items" + return 0 + fi + + if [[ -n "$prefix" ]]; then + path="${prefix}::${item}" + else + path="$item" + fi + + if [[ "$path" == *"::*" ]]; then + path="${path%::*}" + fi + while [[ "$path" == *"::" ]]; do + path="${path%::}" + done + + [[ -n "$path" ]] && printf '%s\n' "$path" +} + +normalize_import_group() { + local prefix="$1" + local group="$2" + local item="" char depth=0 i + + for ((i = 0; i < ${#group}; i++)); do + char="${group:i:1}" + case "$char" in + "{") + depth=$((depth + 1)) + ;; + "}") + depth=$((depth - 1)) + ;; + ",") + if (( depth == 0 )); then + normalize_import_group_item "$prefix" "$item" + item="" + continue + fi + ;; + esac + item+="$char" + done + + normalize_import_group_item "$prefix" "$item" +} + +normalize_import_path() { + local text="$1" + local path + + path="$(sed -E 's/.*use[[:space:]]+crate::([^;]+);?.*/\1/' <<<"$text")" + if [[ -z "$path" ]] || [[ "$path" == "$text" ]]; then + return 0 + fi + + path="$(sed -E 's/^[[:space:]]+//; s/[[:space:]]+$//' <<<"$path")" + if [[ "$path" == *"{"* ]]; then + local prefix group + + prefix="" + group="$path" + if [[ "$path" != \{* ]]; then + prefix="${path%%\{*}" + group="${path#*\{}" + else + group="${path#\{}" + fi + group="${group%\}*}" + prefix="${prefix%::}" + normalize_import_group "$prefix" "$group" + return 0 + fi + + path="$(sed -E 's/[[:space:]]+as[[:space:]].*$//' <<<"$path")" + path="$(sed -E 's/[[:space:]]//g' <<<"$path")" + + if [[ "$path" == *"::*" ]]; then + path="${path%::*}" + fi + while [[ "$path" == *"::" ]]; do + path="${path%::}" + done + + printf '%s\n' "$path" +} + +emit_crate_use_statements() { + (cd "$ROOT_DIR" && rg --files -g '*.rs' rustfs/src | while IFS= read -r file; do + perl -0777 -ne ' + while (/\buse\s+crate::.*?;/sg) { + my $statement = $&; + my $line = substr($_, 0, $-[0]) =~ tr/\n//; + $line += 1; + $statement =~ s/\s+/ /g; + print "$ARGV:$line:$statement\n"; + } + ' "$file" + done) +} + +normalize_baseline_file() { + local input="$1" + local output="$2" + local line status first second third + + : >"$output" + + while IFS= read -r line; do + [[ -z "$line" ]] && continue + [[ "$line" == \#* ]] && continue + + IFS='|' read -r status first second third _ <<<"$line" + case "$status" in + dep) + if [[ -n "$first" ]] && [[ -n "$second" ]] && [[ -n "$third" ]]; then + printf 'dep|%s|%s|%s\n' "$first" "$second" "$third" >>"$output" + fi + ;; + cycle) + if [[ -n "$first" ]]; then + printf 'cycle|%s\n' "$first" >>"$output" + fi + ;; + accepted | todo) + if [[ "$first" == "cycle" ]] && [[ -n "$second" ]]; then + printf 'cycle|%s\n' "$second" >>"$output" + elif [[ -n "$first" ]] && [[ -n "$second" ]] && [[ -n "$third" ]]; then + printf 'dep|%s|%s|%s\n' "$first" "$second" "$third" >>"$output" + fi + ;; + esac + done <"$input" + + sort -u -o "$output" "$output" +} + TMP_DIR="$(mktemp -d)" trap 'rm -rf "$TMP_DIR"' EXIT @@ -86,28 +251,28 @@ while IFS= read -r line; do continue fi - import_path="$(sed -E 's/.*use[[:space:]]+crate::([^;{ ]+).*/\1/' <<<"$text")" - if [[ -z "$import_path" ]] || [[ "$import_path" == "$text" ]]; then - continue - fi + while IFS= read -r import_path; do + if [[ -z "$import_path" ]]; then + continue + fi - target_layer="$(classify_target_layer "$import_path")" - if [[ "$target_layer" == "unknown" ]]; then - continue - fi + target_layer="$(classify_target_layer "$import_path")" + if [[ "$target_layer" == "unknown" ]]; then + continue + fi - if [[ "$source_layer" != "$target_layer" ]]; then - printf '%s->%s\n' "$source_layer" "$target_layer" >>"$EDGES_RAW" - fi + if [[ "$source_layer" != "$target_layer" ]]; then + printf '%s->%s\n' "$source_layer" "$target_layer" >>"$EDGES_RAW" + fi - source_rank="$(layer_rank "$source_layer")" - target_rank="$(layer_rank "$target_layer")" + source_rank="$(layer_rank "$source_layer")" + target_rank="$(layer_rank "$target_layer")" - if (( source_rank < target_rank )); then - printf 'dep|%s|%s->%s|crate::%s\n' "$file" "$source_layer" "$target_layer" "$import_path" >>"$VIOLATIONS_RAW" - printf '%s:%s reverse dependency %s->%s via crate::%s\n' "$file" "$lineno" "$source_layer" "$target_layer" "$import_path" - fi -done < <(cd "$ROOT_DIR" && rg -n --no-heading -g '*.rs' 'use[[:space:]]+crate::' rustfs/src) + if (( source_rank < target_rank )); then + printf 'dep|%s|%s->%s|crate::%s\n' "$file" "$source_layer" "$target_layer" "$import_path" >>"$VIOLATIONS_RAW" + fi + done < <(normalize_import_path "$text") +done < <(emit_crate_use_statements) sort -u "$VIOLATIONS_RAW" >"${TMP_DIR}/violations_sorted.txt" @@ -138,15 +303,24 @@ if [[ ! -f "$BASELINE_FILE" ]]; then exit 1 fi -sort -u "$BASELINE_FILE" >"${TMP_DIR}/baseline_sorted.txt" +normalize_baseline_file "$BASELINE_FILE" "${TMP_DIR}/baseline_sorted.txt" NEW_ITEMS="${TMP_DIR}/new_items.txt" comm -13 "${TMP_DIR}/baseline_sorted.txt" "$CURRENT_BASELINE" >"$NEW_ITEMS" +STALE_ITEMS="${TMP_DIR}/stale_items.txt" +comm -23 "${TMP_DIR}/baseline_sorted.txt" "$CURRENT_BASELINE" >"$STALE_ITEMS" + if [[ -s "$NEW_ITEMS" ]]; then echo "Layer dependency guard failed: new reverse dependencies or cycles detected" cat "$NEW_ITEMS" exit 1 fi +if [[ -s "$STALE_ITEMS" ]]; then + echo "Layer dependency guard failed: stale baseline entries detected" + cat "$STALE_ITEMS" + exit 1 +fi + echo "Layer dependency guard passed (no new reverse dependencies/cycles)." diff --git a/scripts/layer-dependency-baseline.txt b/scripts/layer-dependency-baseline.txt index 8787d0c92..dc78fa8c9 100644 --- a/scripts/layer-dependency-baseline.txt +++ b/scripts/layer-dependency-baseline.txt @@ -5,15 +5,56 @@ # Since they live inside one Cargo crate, Rust doesn't enforce separation. # The list is maintained for architectural awareness during code review. # -# Format: status|source_file|direction|imported_symbol +# Format for dependency entries: +# status|source_file|direction|imported_symbol|reason +# +# Format for conceptual cycles: +# status|cycle|direction_pair|reason # # Status: # accepted - reviewed and intentionally allowed # todo - should be resolved in a future refactor -accepted|rustfs/src/main.rs|infra->app|crate::app::context::*|main wires all layers +accepted|rustfs/src/app/bucket_usecase.rs|app->interface|crate::admin::handlers::site_replication::site_replication_bucket_meta_hook|bucket use-case calls site replication hooks +accepted|rustfs/src/app/bucket_usecase.rs|app->interface|crate::admin::handlers::site_replication::site_replication_delete_bucket_hook|bucket use-case calls site replication hooks +accepted|rustfs/src/app/bucket_usecase.rs|app->interface|crate::admin::handlers::site_replication::site_replication_make_bucket_hook|bucket use-case calls site replication hooks +accepted|rustfs/src/app/bucket_usecase.rs|app->interface|crate::storage::s3_api::bucket::ListObjectVersionsParams|bucket use-case uses current S3 API bucket helper types +accepted|rustfs/src/app/bucket_usecase.rs|app->interface|crate::storage::s3_api::bucket::ListObjectsV2Params|bucket use-case uses current S3 API bucket helper types +accepted|rustfs/src/app/bucket_usecase.rs|app->interface|crate::storage::s3_api::bucket::build_list_buckets_output|bucket use-case uses current S3 API bucket response helpers +accepted|rustfs/src/app/bucket_usecase.rs|app->interface|crate::storage::s3_api::bucket::build_list_object_versions_output|bucket use-case uses current S3 API bucket response helpers +accepted|rustfs/src/app/bucket_usecase.rs|app->interface|crate::storage::s3_api::bucket::build_list_objects_output|bucket use-case uses current S3 API bucket response helpers +accepted|rustfs/src/app/bucket_usecase.rs|app->interface|crate::storage::s3_api::bucket::build_list_objects_v2_output|bucket use-case uses current S3 API bucket response helpers +accepted|rustfs/src/app/bucket_usecase.rs|app->interface|crate::storage::s3_api::bucket::parse_list_object_versions_params|bucket use-case uses current S3 API bucket query parser +accepted|rustfs/src/app/bucket_usecase.rs|app->interface|crate::storage::s3_api::bucket::parse_list_objects_v2_params|bucket use-case uses current S3 API bucket query parser +accepted|rustfs/src/app/bucket_usecase.rs|app->interface|crate::storage::s3_api::common::rustfs_owner|bucket use-case uses current S3 API owner helper +accepted|rustfs/src/app/lifecycle_transition_api_test.rs|app->interface|crate::storage::ecfs::FS|app test exercises ECFS through current interface path +accepted|rustfs/src/app/multipart_usecase.rs|app->interface|crate::storage::s3_api::multipart::ListMultipartUploadsParams|multipart use-case uses current S3 API multipart helper types +accepted|rustfs/src/app/multipart_usecase.rs|app->interface|crate::storage::s3_api::multipart::build_list_multipart_uploads_output|multipart use-case uses current S3 API multipart response helpers +accepted|rustfs/src/app/multipart_usecase.rs|app->interface|crate::storage::s3_api::multipart::build_list_parts_output|multipart use-case uses current S3 API multipart response helpers +accepted|rustfs/src/app/multipart_usecase.rs|app->interface|crate::storage::s3_api::multipart::parse_list_multipart_uploads_params|multipart use-case uses current S3 API multipart query parser +accepted|rustfs/src/app/multipart_usecase.rs|app->interface|crate::storage::s3_api::multipart::parse_list_parts_params|multipart use-case uses current S3 API multipart query parser +accepted|rustfs/src/app/multipart_usecase.rs|app->interface|crate::storage::s3_api::multipart::parse_upload_part_number|multipart use-case uses current S3 API multipart part parser +accepted|rustfs/src/app/object_usecase.rs|app->interface|crate::storage::ecfs|object use-case uses current ECFS interface helpers +accepted|rustfs/src/app/object_usecase.rs|app->interface|crate::storage::s3_api::multipart::parse_list_parts_params|object use-case uses current S3 API multipart query parser +accepted|rustfs/src/init.rs|infra->interface|crate::admin|init wires the admin module during process setup +accepted|rustfs/src/protocols/client.rs|infra->interface|crate::storage::ecfs::FS|protocol client delegates to the current ECFS S3 implementation accepted|rustfs/src/server/audit.rs|infra->app|crate::app::context::resolve_server_config|config resolution uses global AppContext accepted|rustfs/src/server/event.rs|infra->app|crate::app::context::resolve_server_config|config resolution uses global AppContext accepted|rustfs/src/server/http.rs|infra->interface|crate::admin|HTTP server routes to admin handlers accepted|rustfs/src/server/layer.rs|infra->interface|crate::admin::console::is_console_path|pure path predicate for routing -accepted|rustfs/src/storage/objects/put_object.rs|infra->app|crate::app::context::resolve_bucket_metadata_handle|metadata resolution uses global AppContext +accepted|rustfs/src/server/layer.rs|infra->interface|crate::admin::handlers::health::HealthProbe|HTTP layer builds health responses +accepted|rustfs/src/server/layer.rs|infra->interface|crate::admin::handlers::health::build_health_response_parts|HTTP layer builds health responses +accepted|rustfs/src/startup_iam.rs|infra->app|crate::app::context::AppContext|startup wires IAM bootstrap through AppContext +accepted|rustfs/src/startup_iam.rs|infra->app|crate::app::context::get_global_app_context|startup wires IAM bootstrap through AppContext +accepted|rustfs/src/startup_iam.rs|infra->app|crate::app::context::init_global_app_context|startup wires IAM bootstrap through AppContext +accepted|rustfs/src/storage/ecfs_extend.rs|infra->interface|crate::storage::ecfs::ListObjectUnorderedQuery|storage extension uses current ECFS interface query type +accepted|rustfs/src/storage/ecfs_test.rs|infra->interface|crate::storage::ecfs::FS|storage tests exercise current ECFS interface path +accepted|rustfs/src/storage/ecfs_test.rs|infra->interface|crate::storage::ecfs::validate_object_lock_configuration_input|storage tests exercise current ECFS object-lock validator +accepted|rustfs/src/storage/ecfs_test.rs|infra->interface|crate::storage::s3_api::common::rustfs_initiator|storage tests use current S3 API initiator helper +accepted|rustfs/src/storage/ecfs_test.rs|infra->interface|crate::storage::s3_api::common::rustfs_owner|storage tests use current S3 API owner helper +accepted|rustfs/src/storage/rpc/node_service.rs|infra->interface|crate::admin::service::config::reload_dynamic_config_runtime_state|node RPC reloads admin runtime state +accepted|rustfs/src/storage/rpc/node_service.rs|infra->interface|crate::admin::service::config::reload_runtime_config_snapshot|node RPC reloads admin runtime state +accepted|rustfs/src/storage/rpc/node_service.rs|infra->interface|crate::admin::service::site_replication::reload_site_replication_runtime_state|node RPC reloads admin runtime state +accepted|cycle|app<->infra|existing intra-crate cross-layer references +accepted|cycle|app<->interface|existing intra-crate cross-layer references +accepted|cycle|infra<->interface|existing intra-crate cross-layer references