Skip to content

docs: add REVIEW.md#147

Open
vincent-k2026 wants to merge 1 commit into
mainfrom
krabat/docs/review-guidelines
Open

docs: add REVIEW.md#147
vincent-k2026 wants to merge 1 commit into
mainfrom
krabat/docs/review-guidelines

Conversation

@vincent-k2026

Copy link
Copy Markdown
Collaborator

Adds a REVIEW.md with salt-specific review checks, supplementing the centralized baseline rubric:

  • Serde round-trips must be exercised by a test (no true-by-construction assertions that pass on a broken encoder).
  • No dead configuration — a documented, set-but-never-read manifest key is a silent no-op for operators.
  • Documented local flows must run on stock macOS bash 3.2 (no mapfile; quote empty-array expansions).

The centralized pr-review action reads this file automatically.

@github-actions

Copy link
Copy Markdown

Mutation testing - PASS

Nothing to test: no viable mutants were generated.

@github-actions

Copy link
Copy Markdown

Performance Benchmark Comparison

Compared 5 benchmark(s) against the latest main baseline.

Detailed Comparison
Benchmark Baseline Throughput (Kelem/s) New Throughput (Kelem/s) Change
update 10000 KVs/1 threads 75.69 75.31 -0.50%
update 10000 KVs/2 threads 140.86 142.81 +1.38%
update 10000 KVs/4 threads 263.83 264.56 +0.28%
update 10000 KVs/8 threads 444.87 445.04 +0.04%
update 10000 KVs/16 threads 603.09 601.20 -0.31%

Comment thread REVIEW.md

## Tooling and scope

- **Documented local flows must run on stock macOS bash 3.2.** The `just`/shell entry points people actually run can't assume bash ≥ 4: no `mapfile`, and always quote empty-array expansions (an unguarded `"${arr[@]}"` throws `unbound variable` under `set -u`).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

1. The bash rule prescribes the wrong fix — and the review bot will apply it literally (REVIEW.md:12)

"always quote empty-array expansions (an unguarded "${arr[@]}" throws unbound variable under set -u)"

  • Quoting is not the remedy. On bash 3.2–4.3, an empty "${arr[@]}" trips set -u whether or not it's quoted (bash 4.4 fixed this). The rule is internally inconsistent: the failing example it shows is already quoted.
  • The correct fix is the guard idiom, which this repo already uses and names correctly — scripts/mutation_test.sh:92 says "the nounset-safe ${arr[@]+...} idiom" and line 110 uses ${exclude_args[@]+"${exclude_args[@]}"}.
  • This is worse than a normal doc nit because the file is machine-consumed guidance: a reviewer bot told "always quote" would approve "${arr[@]}" — exactly the pattern that breaks on stock macOS — and could flag the correct guard as redundant.

Suggested replacement: "…no mapfile, and guard possibly-empty array expansions with the nounset-safe ${arr[@]+"${arr[@]}"} idiom — under set -u, bash ≤ 4.3 treats an empty "${arr[@]}" as unbound even when quoted (see scripts/mutation_test.sh)."

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

2. Minor: "just/shell entry points" (REVIEW.md:12) — there is no justfile anywhere in this repo; local flows are plain scripts (scripts/mutation_test.sh, scripts/mutation_gate.py). Fine as future-proofing, but if it's a leftover from another repo's REVIEW.md, drop "just/" so reviewers don't hunt for recipes that don't exist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants