Skip to content

fierce-pike-491 - #1025

Merged
briansrls merged 3 commits into
mainfrom
session/fierce-pike-491
Apr 27, 2026
Merged

briansrls merged 3 commits into
mainfrom
session/fierce-pike-491

Conversation

@briansrls

Copy link
Copy Markdown
Contributor

Opened from session-dashboard for session fierce-pike-491.

@briansrls
briansrls marked this pull request as ready for review April 27, 2026 15:55
@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: cursor / composer-2
  • Commit: ddd30e76 · Trigger: schedule
  • Thinking: 59s wall

Findings

  • src/v3/compiler/tests/integration/test_runner_test.rs:536–547 — TESTING.md (anti-pattern: pinning error message text; brittle coupling to suite order). The updated assertions match ClaimResult::Fail via reason.contains("…") and assume results[0]–results[4] align with the suite claim order. TESTING.md calls out assert!(…contains("missing field"))-style checks as implementation-driven and discourages multi-claim coupling; here the same pattern is applied to runner-produced strings. NON-BLOCKING — behavior is still covered, but rewording a ClaimResult::Fail string or reordering claims will break the test without a semantic change.

Verdict

APPROVE_WITH_COMMENTS — The runner changes wire PB census predicates to real checks (GENERATED_FILES, embedded SG-0 census, stage snapshots, explicit fixed-point double compile) and remove the NYI stub in a fail-closed way; nothing here looks like a substrate or invariant breach for the implementation layer. The only rubric-aligned nit is the new integration assertions above; tightening them later (e.g. match on structured ClaimResult or stable claim names) would align better with TESTING.md.

Exploratory (optional)

  • test_runner.rs:2408–2413: The branch that fails when expected_hand_authored_test is non-empty reports “hand-authored test file(s) outside generated paths,” which reads like a path-set check, while the condition is “census list count ≠ 0.” Worth clarifying the message if this predicate stays bundled.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review metadata

  • Provider / model: codex / unknown
  • Commit: ddd30e76 · Trigger: schedule
  • Thinking: 101s wall

⚠️ Review blocked because gh pr diff 1025 --repo gunb-ai/gunbc cannot connect to api.github.com and no allowed authoritative PR diff source is available.

@briansrls
briansrls merged commit 8e55bcc into main Apr 27, 2026
8 checks passed
@briansrls

Copy link
Copy Markdown
Contributor Author

Follow-up on the scheduled Codex review at 2026-04-27T16:45:16Z: I re-ran PR inspection successfully after GitHub connectivity recovered. That review item was blocked and did not contain a code finding to fix.\n\nWhile verifying the PR, I also checked the earlier review note about brittle PB census assertions. That finding was valid; the branch now has follow-up commit 83d0fd7 replacing index-ordered / message-substring assertions with claim-name lookup and ClaimResult variant checks. The active follow-up PR for this branch is #1042.\n\nVerification: cargo fmt --check passed; git diff --check passed; focused cargo test was attempted with a 90s timeout but did not complete while compiling v3-compiler.

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.

1 participant