Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Strengthen the generated-draft import test to verify the persisted Store state, not only the returned value.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Adds synthetic planning acceptance fixtures and SQLite-backed integration tests for imports, invalid plans, identity, replay, hostile inputs, and command approval, with accompanying evidence documentation.
Changes:
- Added planning acceptance integration coverage.
- Added synthetic plan, edit, and hostile-input fixtures.
- Documented acceptance evidence and remaining dependencies.
| File | Description |
|---|---|
test/planning-acceptance.test.ts |
Planning acceptance integration coverage |
test/fixtures/planning/synthetic-plan.json |
Synthetic plan fixture |
test/fixtures/planning/synthetic-edits.json |
Synthetic suggestion fixture |
test/fixtures/planning/hostile-input.json |
Hostile prompt-input fixture |
docs/implementation/planning-acceptance.md |
Acceptance matrix and dependency documentation |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BLOCKED draft: E3 #35 requires the F-owned atomic pending-attempt cleanup prerequisite #34. Recorded real Claude/Codex authoring-output acceptance also requires D5 (#28). E3/E4 and T18 are not complete.
Adds dedicated synthetic integration fixtures for JSON/YAML import, the eight T18 broken-plan cases, stable identity, replay/sibling invalidation, SQLite reopen, malformed responses, hostile prompt input and exact command approval. Composes E2/E3 with the real Store. No fixture is represented as vendor output or isolation evidence.
Depends on E3 #35; ownership #29. Acceptance matrix and D5 handoff: docs/implementation/planning-acceptance.md.
Validated fixture head: 886ccf5. After final change and push: npm test passed 310 tests across 15 files; npm run typecheck passed; npm run test:browser passed all 43 tests. Both CI runs are green. These passing suites do not cover/fix the newly reproduced E3 completion/cleanup blocker in #34. No live provider or Docker validation performed.
Review round 1: strengthened generated-draft persistence checks: changed summary, complete current/reopened Store equality to validated output, and unchanged historical revision one. Dedicated suite remains 25 tests, all passing. Thread replied/resolved; no findings declined. Further review and completion are paused at the upstream dependency. Full E4 diff self-review after the assertion fix found no additional fixture issue.
Review lesson: existing AGENTS.md durable-versus-visible state assertions apply; no duplicate rule added. Shared store edits remain with F. Resume E3 only after atomic cleanup is supplied and its failing-before race has a passing-after regression, then refresh this stack and complete remaining E4 live-output evidence after D5. No merge performed.