jpmschweitzer/settled-reach
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat(ci): sprint 10 — perf baseline + validation fix #35

Closed
jpmschweitzer wants to merge 0 commits from ci into main
pull from: ci
merge into: :main
Conversation 3 Commits - Files Changed -
jpmschweitzer commented 2026-02-18 12:44:57 +01:00
Owner
Copy Link
Copy Source

Summary

Sprint 10 CI deliverables:

  • #499 — Performance baseline tooling: make perf-baseline runs full plugin stack tick benchmark (50 measured ticks, 5 warmup), captures per-tick timing, entity counts, process RSS, and shadowcast benchmarks. Outputs structured JSON to tests/perf/baseline.json. Supports --compare mode for regression detection (>20% threshold). First baseline: mean 366µs, p95 526µs (0.5% of D-026 100ms budget).

  • #515 — Bidirectional relationship check fix: Check 9 in tooling/validate-content tested target in npc_rels which missed NPCs with no relationship entries. Fixed to target in self.npcs. Advisory warning, non-blocking.

Test plan

  • make perf-baseline runs and produces tests/perf/baseline.json
  • make perf-baseline --compare detects regressions against saved baseline
  • make pre-pr-validate passes with 0 errors, 21 advisory warnings
  • Hoshe QA review passed both tickets
## Summary Sprint 10 CI deliverables: - **#499 — Performance baseline tooling**: `make perf-baseline` runs full plugin stack tick benchmark (50 measured ticks, 5 warmup), captures per-tick timing, entity counts, process RSS, and shadowcast benchmarks. Outputs structured JSON to `tests/perf/baseline.json`. Supports `--compare` mode for regression detection (>20% threshold). First baseline: mean 366µs, p95 526µs (0.5% of D-026 100ms budget). - **#515 — Bidirectional relationship check fix**: Check 9 in `tooling/validate-content` tested `target in npc_rels` which missed NPCs with no relationship entries. Fixed to `target in self.npcs`. Advisory warning, non-blocking. ## Test plan - [x] `make perf-baseline` runs and produces `tests/perf/baseline.json` - [x] `make perf-baseline --compare` detects regressions against saved baseline - [x] `make pre-pr-validate` passes with 0 errors, 21 advisory warnings - [x] Hoshe QA review passed both tickets
jpmschweitzer added 3 commits 2026-02-18 12:44:57 +01:00
fix(ci): correct bidirectional relationship check target (#515) 76efc979b9 
Check 9 tested `target in npc_rels` which missed NPCs with no
relationship entries. Changed to `target in self.npcs` so the
advisory warning fires correctly for all NPCs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat(ci): add performance baseline tooling (#499) 08052c6459 
Adds `make perf-baseline` — boots the full server plugin stack with
real content, measures 50 ticks (5 warmup), captures per-tick timing,
entity counts, and process RSS. Includes shadowcast benchmarks. Saves
structured JSON to tests/perf/baseline.json for regression detection.
Supports --compare mode (>20% threshold).

First baseline: mean 366µs, p95 526µs (0.5% of D-026 100ms budget).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chore(meta): update changelog c1d7c072ae 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-18 13:03:12 +01:00
Author
Owner
Copy Link
Copy Source

Review: ci -> main (type: code)

Hoshe (Code Quality): REQUEST_CHANGES

Well-structured tooling, but concerns about the Rust benchmark's protocol assumptions.

# File Severity Issue
1 server/tests/perf_bench.rs:91-133 critical TCP handshake/tick protocol race — if BridgePlugin does receive_inputs before send_snapshot, neither side advances. Needs verification against BridgePlugin contract.
2 tooling/perf-baseline:87-110 warning Shadowcast output parser is order-dependent (assumes Symmetric before Recursive) with no guard.
3 Makefile:2 warning perf-baseline not in .PHONY declaration.
4 server/tests/perf_bench.rs:188 suggestion p95 calculation is actually p96 with 50 samples (off-by-one).
5 tooling/perf-baseline suggestion --compare exits 0 with no output when baseline file doesn't exist.

Tyre (Architecture): APPROVE

Architecturally sound, well-integrated with D-026 tick budget constraints.

# File Severity Issue
1 tooling/perf-baseline:21 suggestion TICK_BUDGET_US assumes 10 tps floor — make assumption explicit in comment.
2 server/tests/perf_bench.rs:159 suggestion Assertion too weak — could pass with 6 snapshots.
3 tests/perf/baseline.json suggestion Baseline references ci-branch commit hash.

Verdict: CHANGES REQUESTED

## Review: ci -> main (type: code) ### Hoshe (Code Quality): REQUEST_CHANGES Well-structured tooling, but concerns about the Rust benchmark's protocol assumptions. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `server/tests/perf_bench.rs:91-133` | critical | TCP handshake/tick protocol race — if BridgePlugin does `receive_inputs` before `send_snapshot`, neither side advances. Needs verification against BridgePlugin contract. | | 2 | `tooling/perf-baseline:87-110` | warning | Shadowcast output parser is order-dependent (assumes Symmetric before Recursive) with no guard. | | 3 | `Makefile:2` | warning | `perf-baseline` not in `.PHONY` declaration. | | 4 | `server/tests/perf_bench.rs:188` | suggestion | p95 calculation is actually p96 with 50 samples (off-by-one). | | 5 | `tooling/perf-baseline` | suggestion | `--compare` exits 0 with no output when baseline file doesn't exist. | ### Tyre (Architecture): APPROVE Architecturally sound, well-integrated with D-026 tick budget constraints. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `tooling/perf-baseline:21` | suggestion | `TICK_BUDGET_US` assumes 10 tps floor — make assumption explicit in comment. | | 2 | `server/tests/perf_bench.rs:159` | suggestion | Assertion too weak — could pass with 6 snapshots. | | 3 | `tests/perf/baseline.json` | suggestion | Baseline references ci-branch commit hash. | ### Verdict: CHANGES REQUESTED
jpmschweitzer added 1 commit 2026-02-18 13:07:53 +01:00
fix(ci): address PR #35 review comments d0596d2763 
- Document non-blocking receive contract in perf_bench.rs docstring,
  confirming no TCP deadlock race (Hoshe #1, critical)
- Make shadowcast parser order-independent — flush on new config header
  instead of requiring Recursive after Symmetric (Hoshe #2)
- Fix p95 calculation: use floor(0.95*(N-1)) nearest-rank instead of
  ceil(0.95*N)-1 which was off-by-one at N=50 (Hoshe #4)
- Error on --compare when no baseline file exists (Hoshe #5)
- Add D-031 10tps assumption comment to TICK_BUDGET_US (Tyre #1)
- Strengthen snapshot assertion: require warmup + half measurement
  window instead of warmup + 1 (Tyre #2)
- Regenerate baseline with corrected p95 (356µs, was 526µs)

Hoshe #3 (.PHONY) was already addressed — perf-baseline is in the
.PHONY declaration on Makefile line 10.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-18 13:12:54 +01:00
Author
Owner
Copy Link
Copy Source

Review comments addressed in d0596d2. All 5 Hoshe items resolved (critical TCP race documented as non-blocking contract, parser made order-independent, p95 off-by-one fixed, --compare now errors without baseline). All 3 Tyre suggestions applied (10tps assumption comment, stronger assertion requiring 30+ snapshots, baseline regenerated). Hoshe #3 (.PHONY) was already present on Makefile line 10.

Review comments addressed in d0596d2. All 5 Hoshe items resolved (critical TCP race documented as non-blocking contract, parser made order-independent, p95 off-by-one fixed, --compare now errors without baseline). All 3 Tyre suggestions applied (10tps assumption comment, stronger assertion requiring 30+ snapshots, baseline regenerated). Hoshe #3 (.PHONY) was already present on Makefile line 10.
jpmschweitzer commented 2026-02-18 13:23:09 +01:00
Author
Owner
Copy Link
Copy Source

Re-Review: ci -> main (type: code)

Hoshe (Code Quality): APPROVE

All 8 previous issues confirmed fixed. Two new suggestion-level items.

# File Severity Issue
1 tooling/perf-baseline:compare_baselines() suggestion Shadowcast results saved but never compared in --compare mode — regression won't be detected.
2 tooling/perf-baseline:run_shadowcast_benchmark() suggestion Partial config block (header without timings) silently appended — guard with completeness check.

Tyre (Architecture): APPROVE

All 3 previous suggestions confirmed fixed. Two new suggestion-level items.

# File Severity Issue
1 tooling/perf-baseline:compare_baselines() suggestion RSS memory captured but never compared — D-041 budget of ~6MB not enforced by --compare.
2 tooling/perf-baseline:compare_baselines() suggestion Shadowcast timing data saved but no comparison logic — asymmetric guard coverage.

Verdict: APPROVED

## Re-Review: ci -> main (type: code) ### Hoshe (Code Quality): APPROVE All 8 previous issues confirmed fixed. Two new suggestion-level items. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `tooling/perf-baseline:compare_baselines()` | suggestion | Shadowcast results saved but never compared in `--compare` mode — regression won't be detected. | | 2 | `tooling/perf-baseline:run_shadowcast_benchmark()` | suggestion | Partial config block (header without timings) silently appended — guard with completeness check. | ### Tyre (Architecture): APPROVE All 3 previous suggestions confirmed fixed. Two new suggestion-level items. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `tooling/perf-baseline:compare_baselines()` | suggestion | RSS memory captured but never compared — D-041 budget of ~6MB not enforced by `--compare`. | | 2 | `tooling/perf-baseline:compare_baselines()` | suggestion | Shadowcast timing data saved but no comparison logic — asymmetric guard coverage. | ### Verdict: APPROVED
jpmschweitzer closed this pull request 2026-02-18 18:26:36 +01:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
jpmschweitzer (Jeroen Schweitzer)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: jpmschweitzer/settled-reach#35