Skip to content

Validate review-summary edge cases - #17

Merged
mchwang merged 6 commits into
mainfrom
codex/validate-review-edge-cases
Sep 24, 2026
Merged

mchwang merged 6 commits into
mainfrom
codex/validate-review-edge-cases

Conversation

@mchwang

@mchwang mchwang commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

PR #9's final summary retained two moderate concerns and one styling nit despite reporting no findings. Validate all three before the real-issue experiment without inferring defects from summary shorthand.

Results:

  • Manual reassignment after retry.ts → renamed.ts keeps both removed old-path and added new-path segments in scope.
  • A literal declared filename *.txt is not confused with a decoy a.txt; planting rejects the history when the literal file disappears.
  • Accepted changes render with the existing muted treatment and explicit Accepted outside plan label. A proposed error-color assertion failed because the summary supplied no expected alternative; no new color semantics were inferred.

No production defect reproduced, so this PR changes no runtime behavior. It adds focused real-Git validation fixtures for the two behavioral concerns and commits the evidence record required before issue #3's human experiment.

Validation on exact pushed head 2708ae9: typecheck, 190 unit/integration tests, and 39 browser tests passed locally; both required CI runs passed.

Review-lesson audit

  • Originating PR Add the local plan-indexed review screen #9 summary-only concerns: handled by the existing Review readiness rule requiring reproduction or a concrete follow-up issue; issue Validate review-summary edge cases before the real-issue experiment #10 and this evidence PR are that follow-up.
  • PR Validate review-summary edge cases #17 round 1, rename segment selection: fixed by asserting -/+ operations and resolving the removed side through oldPath and added side through path before reassignment. Captured by the new Review readiness rule requiring summary-concern fixtures to assert the disputed intermediate representation.
  • PR Validate review-summary edge cases #17 round 1, requested literal-pathspec production fix: declined because direct Git controls prove ls-tree treats *.txt literally and rejects :(glob) magic as unsupported. The fixture now asserts those command semantics before the plant outcome. This command-specific false premise needs no production change and is covered by the same new fixture-evidence rule.
  • PR Validate review-summary edge cases #17 round 2, claimed retained P3 fixture item: declined because the fixture replaces plan.items with a P1-only array. It now asserts the exact item IDs before import; typecheck and all focused assertions pass. This false intermediate-state premise is covered by the same new fixture-evidence rule.
  • PR Validate review-summary edge cases #17 round 3: no findings. Its overview asked for the final in-scope assertion already added in the following commit.
  • PR Validate review-summary edge cases #17 round 4, ambiguous validation commit wording: accepted. The evidence record now labels 5a2685c as the baseline and points exact-head results to this PR. A new rule requires evidence records to distinguish baselines, checkpoints, and validated heads.
  • PR Validate review-summary edge cases #17 round 5 on exact head 2708ae9: no findings; approval recommended.
  • All three review threads are resolved. The review loop converged after five rounds.

Deferred: issue #3's manual assignment and paired human go/no-go experiment remain pending.

Closes #10.

Copilot AI lite review requested due to automatic review settings September 24, 2026 00:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Two moderate validation issues and one documentation nit remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 Medium severity

Open (2)
What changed in this PR

Adds focused test fixtures and documentation for review-summary edge cases without intended runtime changes.

Changes:

  • Tests rename reassignment behavior.
  • Tests literal filename planting.
  • Records styling validation and experiment status.
File Summary Review status
test/​review.test.ts Rename reassignment fixture Changes required: assert both path/oldPath and operations.
test/​plant.test.ts Literal path planting fixture Changes required: fix literal pathspec handling.
docs/​experiments/​review-summary-edge-cases.md Validation evidence record Nit: accurately describe rename segment fields.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread test/plant.test.ts
Comment thread test/review.test.ts Outdated
Copilot AI review requested due to automatic review settings September 24, 2026 00:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

A critical fixture issue blocks the rename/reassignment assertions in test/review.test.ts.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (2)

Comment thread test/review.test.ts
Copilot AI review requested due to automatic review settings September 24, 2026 00:28

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Add a final assertion confirming both renamed-file candidates remain in scope.

Review effort: Lite
Findings: None

Resolved since last review (1)

Copilot AI review requested due to automatic review settings September 24, 2026 00:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

No blocking issues remain; the sole documentation nit is non-blocking.

Review effort: Lite
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Low severity Evidence record references a different validation commit

docs/​experiments/​review-summary-edge-cases.md:3

The evidence record says it was validated from 5a2685c, but the PR metadata reports validation on the exact pushed head c4d580b. This makes the record point at a different tree than the claimed validation; update the recorded head (or explicitly explain why an earlier checkpoint is intentional).

Copilot AI review requested due to automatic review settings September 24, 2026 00:43

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

All reviewed concerns are covered by passing focused validation and no runtime issues remain.

Review effort: Lite
Findings: None

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

All reviewed concerns are addressed, with no production behavior changes.

Review effort: Lite
Findings: None

@mchwang
mchwang merged commit 442147d into main Sep 24, 2026
4 checks passed
@mchwang
mchwang deleted the codex/validate-review-edge-cases branch September 24, 2026 00:48
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.

Validate review-summary edge cases before the real-issue experiment

2 participants