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

fix(simulation): Sprint 30 server — filter fix, corridor-status, skill update #108

Closed
jpmschweitzer wants to merge 0 commits from server into main
pull from: server
merge into: :main
Conversation 3 Commits - Files Changed -
jpmschweitzer commented 2026-04-04 23:12:32 +02:00
Owner
Copy Link
Copy Source

Summary

  • #762: Fix habitable_planet_count filter to accept both breathable and standard atmosphere values — previously all committed systems reported 0 habitable planets
  • #744: Add corridor-status subcommand to atlas CLI — shows remaining unfinished systems grouped by geographic sector and hop distance
  • chore: Add git-state verification warning to pr-push skill (process improvement from Sprint 30)

Files changed

  • server/src/bin/atlas.rs — atmosphere filter fix + corridor-status command
  • server/Cargo.lock — version bump to 0.1.29
  • CHANGELOG.md — Sprint 30 entries
  • .claude/skills/pr-push/SKILL.md — process guard against uncommitted-work false positives

Test plan

  • cargo test --lib — 1147 tests pass
  • cargo check --bin atlas — compiles clean
  • Clippy + rustfmt pass
  • GDScript parse check passes
## Summary - **#762**: Fix `habitable_planet_count` filter to accept both `breathable` and `standard` atmosphere values — previously all committed systems reported 0 habitable planets - **#744**: Add `corridor-status` subcommand to atlas CLI — shows remaining unfinished systems grouped by geographic sector and hop distance - **chore**: Add git-state verification warning to pr-push skill (process improvement from Sprint 30) ## Files changed - `server/src/bin/atlas.rs` — atmosphere filter fix + corridor-status command - `server/Cargo.lock` — version bump to 0.1.29 - `CHANGELOG.md` — Sprint 30 entries - `.claude/skills/pr-push/SKILL.md` — process guard against uncommitted-work false positives ## Test plan - [x] `cargo test --lib` — 1147 tests pass - [x] `cargo check --bin atlas` — compiles clean - [x] Clippy + rustfmt pass - [x] GDScript parse check passes
jpmschweitzer added 3 commits 2026-04-04 23:12:32 +02:00
fix(simulation): Sprint 30 server — atmosphere filter fix + corridor-status cmd 8a98cfa7fc 
#762: Update habitable_planet_count filter to accept both "breathable"
and "standard" atmosphere values via matches! macro. Previously only
matched "breathable", causing all committed systems to report 0
habitable planets.

#744: Add corridor-status subcommand to atlas CLI. Shows remaining
unfinished systems grouped by geographic_sector and hop_distance,
with plain text table output and totals.

Cargo.lock updated for v0.1.29.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chore(meta): update changelog ef15d0afa3 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chore(skills): add git-state verification warning to pr-push skill 6763c30d43 
Sprint 30 showed that grepping source files does not distinguish
committed code from uncommitted agent work. Added mandatory
git status + git diff --stat check from repo root before accepting
"already done" claims, and a warning about CWD path resolution.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-04-04 23:46:23 +02:00
Author
Owner
Copy Link
Copy Source

Review: server -> main (type: code) — Sprint 30

Reviewers: Hoshe (code quality), Tyre (architecture)

Process note: No runtime smoke test mentioned in commits.

Execution Issues (changes requested)

# File Severity Issue
1 server/src/bin/atlas.rs:2006 warning cmd_corridor_status uses INNER JOIN on system_gates — systems with no gate record silently excluded. Every other query uses LEFT JOIN. Will undercount remaining systems and could report false "all done." Fix: LEFT JOIN system_gates g ON s.system_id = g.system_id
2 server/src/bin/atlas.rs:2006 warning After fixing to LEFT JOIN, hop_distance_from_gateway can be NULL — creates a NULL group-by bucket. Display code handles it ("?") but needs verification after the fix.
3 server/src/bin/atlas.rs warning corridor-status emits plain-text ASCII table while every other command emits JSON. Breaks jq composability. Add --json flag or document human-readable-only output.

Ticket #752 (settings.db)

Tyre confirmed this was already implemented in a prior sprint. If so, mark the ticket done and note it in the PR description — the current state is confusing (listed as Sprint 30 server work but absent from the diff).

Suggestions (non-blocking)

# File Severity Issue
1 server/src/bin/atlas.rs:914,946 suggestion generate_body_matrix still emits atmosphere: "breathable" — new proposals won't match the "standard" convention in committed systems. Align default in follow-up.
2 server/src/bin/atlas.rs:1-17 suggestion Doc header doesn't list corridor-status usage example
3 CHANGELOG.md suggestion Doesn't note previously-committed systems may have stale habitable_planet_count = 0 needing re-commit

Verdict: CHANGES REQUESTED

Fix the LEFT JOIN bug (#1) and address the output format inconsistency (#3), then re-request review.

## Review: server -> main (type: code) — Sprint 30 Reviewers: Hoshe (code quality), Tyre (architecture) **Process note:** No runtime smoke test mentioned in commits. --- ### Execution Issues (changes requested) | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `server/src/bin/atlas.rs:2006` | warning | `cmd_corridor_status` uses INNER JOIN on `system_gates` — systems with no gate record silently excluded. Every other query uses LEFT JOIN. Will undercount remaining systems and could report false "all done." Fix: `LEFT JOIN system_gates g ON s.system_id = g.system_id` | | 2 | `server/src/bin/atlas.rs:2006` | warning | After fixing to LEFT JOIN, `hop_distance_from_gateway` can be NULL — creates a NULL group-by bucket. Display code handles it (`"?"`) but needs verification after the fix. | | 3 | `server/src/bin/atlas.rs` | warning | `corridor-status` emits plain-text ASCII table while every other command emits JSON. Breaks `jq` composability. Add `--json` flag or document human-readable-only output. | ### Ticket #752 (settings.db) Tyre confirmed this was already implemented in a prior sprint. If so, mark the ticket done and note it in the PR description — the current state is confusing (listed as Sprint 30 server work but absent from the diff). ### Suggestions (non-blocking) | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `server/src/bin/atlas.rs:914,946` | suggestion | `generate_body_matrix` still emits `atmosphere: "breathable"` — new proposals won't match the `"standard"` convention in committed systems. Align default in follow-up. | | 2 | `server/src/bin/atlas.rs:1-17` | suggestion | Doc header doesn't list `corridor-status` usage example | | 3 | `CHANGELOG.md` | suggestion | Doesn't note previously-committed systems may have stale `habitable_planet_count = 0` needing re-commit | ### Verdict: CHANGES REQUESTED Fix the LEFT JOIN bug (#1) and address the output format inconsistency (#3), then re-request review.
jpmschweitzer added 1 commit 2026-04-04 23:50:20 +02:00
fix(simulation): address PR #108 review — LEFT JOIN, JSON output, atmosphere default 16b7fe0bb5 
Review fixes:
- corridor-status: INNER JOIN → LEFT JOIN so systems without gate
  records are included in counts instead of silently excluded
- corridor-status: output as JSON (serde_json) matching all other
  atlas commands, instead of plain-text ASCII table
- generate_body_matrix: emit atmosphere "standard" instead of
  "breathable" to match committed-system conventions
- Doc header: add corridor-status usage example
- CHANGELOG: note stale habitable_planet_count in pre-fix systems

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-04-04 23:50:44 +02:00
Author
Owner
Copy Link
Copy Source

Review response

All 3 warnings and 3 suggestions addressed in 16b7fe0b:

Warnings fixed

  1. LEFT JOIN — cmd_corridor_status now uses LEFT JOIN system_gates so systems without gate records are counted (was INNER JOIN, silently excluding them)
  2. NULL hop handling — already handled by Option<i32> → "?" display, now verified with LEFT JOIN in place
  3. JSON output — corridor-status now emits serde_json::to_string_pretty matching all other atlas commands (was plain-text ASCII table)

Suggestions addressed

  1. "breathable" → "standard" — generate_body_matrix now emits atmosphere: "standard" for both inhabited and uninhabited habitable planets, matching committed-system conventions
  2. Doc header — added corridor-status usage example
  3. CHANGELOG — added note that pre-fix systems may have stale habitable_planet_count = 0 needing re-commit

#752 (settings.db) clarification

Ticket #752 was already implemented in a prior sprint — SettingsStore::open() uses Connection::open() (auto-creates) + CREATE TABLE IF NOT EXISTS, and server/settings.db is in .gitignore. No code changes needed; ticket marked done. Absent from diff because nothing was changed.

## Review response All 3 warnings and 3 suggestions addressed in `16b7fe0b`: ### Warnings fixed 1. **LEFT JOIN** — `cmd_corridor_status` now uses `LEFT JOIN system_gates` so systems without gate records are counted (was INNER JOIN, silently excluding them) 2. **NULL hop handling** — already handled by `Option<i32>` → `"?"` display, now verified with LEFT JOIN in place 3. **JSON output** — corridor-status now emits `serde_json::to_string_pretty` matching all other atlas commands (was plain-text ASCII table) ### Suggestions addressed 1. **"breathable" → "standard"** — `generate_body_matrix` now emits `atmosphere: "standard"` for both inhabited and uninhabited habitable planets, matching committed-system conventions 2. **Doc header** — added `corridor-status` usage example 3. **CHANGELOG** — added note that pre-fix systems may have stale `habitable_planet_count = 0` needing re-commit ### #752 (settings.db) clarification Ticket #752 was already implemented in a prior sprint — `SettingsStore::open()` uses `Connection::open()` (auto-creates) + `CREATE TABLE IF NOT EXISTS`, and `server/settings.db` is in `.gitignore`. No code changes needed; ticket marked done. Absent from diff because nothing was changed.
jpmschweitzer closed this pull request 2026-04-04 23:57:29 +02:00
jpmschweitzer commented 2026-04-04 23:57:46 +02:00
Author
Owner
Copy Link
Copy Source

Review Round 2: APPROVED

All 3 execution issues from round 1 resolved:

  1. LEFT JOIN — cmd_corridor_status now uses LEFT JOIN system_gates with NULL hop handling
  2. JSON output — corridor-status now emits serde_json::to_string_pretty (both normal and empty paths)
  3. Atmosphere default — generate_body_matrix changed to "standard", filter still accepts both

Merged to main via fast-forward.

## Review Round 2: APPROVED All 3 execution issues from round 1 resolved: 1. **LEFT JOIN** — `cmd_corridor_status` now uses `LEFT JOIN system_gates` with NULL hop handling 2. **JSON output** — corridor-status now emits `serde_json::to_string_pretty` (both normal and empty paths) 3. **Atmosphere default** — `generate_body_matrix` changed to `"standard"`, filter still accepts both Merged to main via fast-forward.

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#108