Add step436 review comparison view and tests

editor/CMakeLists.txt

@@ -2803,4 +2803,13 @@ target_link_libraries(step435_test PRIVATE
     tree_sitter_javascript tree_sitter_typescript
     tree_sitter_java tree_sitter_rust tree_sitter_go)
 
+add_executable(step436_test tests/step436_test.cpp)
+target_include_directories(step436_test PRIVATE src)
+target_link_libraries(step436_test PRIVATE
+    nlohmann_json::nlohmann_json
+    unofficial::tree-sitter::tree-sitter
+    tree_sitter_python tree_sitter_cpp tree_sitter_elisp
+    tree_sitter_javascript tree_sitter_typescript
+    tree_sitter_java tree_sitter_rust tree_sitter_go)
+
 # Step 12: Dear ImGui shell scaffolding created (main.cpp exists but not built due to dependencies)

editor/src/ReviewComparisonView.h

@@ -0,0 +1,118 @@
+#pragma once
+
+#include "WorkflowTaskDetail.h"
+
+#include <optional>
+#include <string>
+#include <vector>
+
+struct ReviewDiffLine {
+    char kind = ' '; // ' ' unchanged, '-' removed, '+' added, '~' modified
+    std::string left;
+    std::string right;
+    std::string note;
+};
+
+struct ReviewComparisonModel {
+    std::string itemId;
+    std::string title;
+    std::string status;
+    std::string leftLabel;
+    std::string rightLabel;
+    std::vector<ReviewDiffLine> diff;
+    std::vector<std::string> changeNotes;
+    bool canApprove = false;
+    bool canReject = false;
+    std::string approveShortcut;
+    std::string rejectShortcut;
+};
+
+class ReviewComparisonView {
+public:
+    static std::optional<ReviewComparisonModel> build(const WorkflowState& wf,
+                                                      const std::string& itemId) {
+        auto detail = WorkflowTaskDetail::build(wf, itemId);
+        if (!detail.has_value()) return std::nullopt;
+
+        ReviewComparisonModel m;
+        m.itemId = detail->itemId;
+        m.title = detail->title;
+        m.status = detail->status;
+        m.leftLabel = "skeleton";
+        m.rightLabel = "generated";
+        m.diff = buildDiffLines(detail->skeletonCode, detail->generatedCode);
+        m.changeNotes = buildChangeNotes(*detail);
+        m.canApprove = (detail->status == WI_REVIEW);
+        m.canReject = (detail->status == WI_REVIEW);
+        m.approveShortcut = approveShortcut();
+        m.rejectShortcut = rejectShortcut();
+        return m;
+    }
+
+    static bool approve(WorkflowState& wf, const std::string& itemId,
+                        const std::string& reviewer = "human") {
+        return WorkflowTaskDetail::approve(wf, itemId, reviewer);
+    }
+
+    static bool reject(WorkflowState& wf, const std::string& itemId,
+                       const std::string& feedback,
+                       const std::string& reviewer = "human") {
+        return WorkflowTaskDetail::reject(wf, itemId, feedback, reviewer);
+    }
+
+    static std::string approveShortcut() { return "Ctrl+Shift+A"; }
+
+    static std::string rejectShortcut() { return "Ctrl+Shift+R"; }
+
+private:
+    static std::vector<std::string> splitLines(const std::string& text) {
+        std::vector<std::string> lines;
+        std::string cur;
+        for (char c : text) {
+            if (c == '\n') {
+                lines.push_back(cur);
+                cur.clear();
+            } else {
+                cur.push_back(c);
+            }
+        }
+        if (!cur.empty()) lines.push_back(cur);
+        return lines;
+    }
+
+    static std::vector<ReviewDiffLine> buildDiffLines(const std::string& left,
+                                                      const std::string& right) {
+        auto l = splitLines(left);
+        auto r = splitLines(right);
+        size_t n = l.size() > r.size() ? l.size() : r.size();
+        std::vector<ReviewDiffLine> out;
+        for (size_t i = 0; i < n; ++i) {
+            std::string ls = i < l.size() ? l[i] : "";
+            std::string rs = i < r.size() ? r[i] : "";
+            if (i >= l.size()) {
+                out.push_back({'+', "", rs, "added"});
+            } else if (i >= r.size()) {
+                out.push_back({'-', ls, "", "removed"});
+            } else if (ls == rs) {
+                out.push_back({' ', ls, rs, "unchanged"});
+            } else {
+                out.push_back({'~', ls, rs, "modified"});
+            }
+        }
+        return out;
+    }
+
+    static std::vector<std::string> buildChangeNotes(const TaskDetailView& detail) {
+        std::vector<std::string> notes;
+        if (!detail.routingExplanation.empty()) {
+            notes.push_back("routing: " + detail.routingExplanation);
+        }
+        if (!detail.rejectionFeedback.empty()) {
+            notes.push_back("latest-feedback: " + detail.rejectionFeedback);
+        }
+        for (const auto& t : detail.annotationTags) {
+            notes.push_back("annotation: " + t);
+        }
+        return notes;
+    }
+};

editor/tests/step436_test.cpp

@@ -0,0 +1,173 @@
+// Step 436: Review Comparison View Tests (12 tests)
+
+#include "ReviewComparisonView.h"
+
+#include <iostream>
+
+static int passed = 0, failed = 0;
+#define TEST(name) { std::cout << "  " << #name << "... "; }
+#define PASS() { std::cout << "PASS\n"; ++passed; }
+#define FAIL(msg) { std::cout << "FAIL: " << msg << "\n"; ++failed; }
+#define CHECK(cond, msg) if (!(cond)) { FAIL(msg); return; } else {}
+
+static WorkflowState sampleReviewState() {
+    WorkflowState wf("review");
+    WorkItem wi;
+    wi.id = "r1";
+    wi.nodeId = "node-review";
+    wi.nodeName = "computePath";
+    wi.bufferId = "src/path.cpp";
+    wi.status = WI_REVIEW;
+    wi.workerType = "llm";
+    wi.priority = "high";
+    wi.contextWidth = "file";
+    wi.reviewRequired = true;
+    wi.result.generatedCode = "void computePath() {\n  int x = 1;\n}\n";
+    wi.rejectionFeedback = "add bounds check";
+    wf.queue.enqueue(wi);
+    return wf;
+}
+
+void test_build_returns_model_for_existing_item() {
+    TEST(build_returns_model_for_existing_item);
+    auto wf = sampleReviewState();
+    auto model = ReviewComparisonView::build(wf, "r1");
+    CHECK(model.has_value(), "model should exist");
+    CHECK(model->itemId == "r1", "item id mismatch");
+    PASS();
+}
+
+void test_build_returns_null_for_missing_item() {
+    TEST(build_returns_null_for_missing_item);
+    auto wf = sampleReviewState();
+    auto model = ReviewComparisonView::build(wf, "missing");
+    CHECK(!model.has_value(), "missing item should return nullopt");
+    PASS();
+}
+
+void test_diff_contains_modified_line_entries() {
+    TEST(diff_contains_modified_line_entries);
+    auto wf = sampleReviewState();
+    auto model = ReviewComparisonView::build(wf, "r1");
+    CHECK(model.has_value(), "expected model");
+    bool hasModified = false;
+    for (const auto& d : model->diff) if (d.kind == '~') hasModified = true;
+    CHECK(hasModified, "diff should contain modified line");
+    PASS();
+}
+
+void test_review_status_enables_approve_and_reject() {
+    TEST(review_status_enables_approve_and_reject);
+    auto wf = sampleReviewState();
+    auto model = ReviewComparisonView::build(wf, "r1");
+    CHECK(model.has_value(), "expected model");
+    CHECK(model->canApprove, "review item should allow approve");
+    CHECK(model->canReject, "review item should allow reject");
+    PASS();
+}
+
+void test_shortcuts_match_spec() {
+    TEST(shortcuts_match_spec);
+    CHECK(ReviewComparisonView::approveShortcut() == "Ctrl+Shift+A", "approve shortcut mismatch");
+    CHECK(ReviewComparisonView::rejectShortcut() == "Ctrl+Shift+R", "reject shortcut mismatch");
+    PASS();
+}
+
+void test_model_includes_shortcuts() {
+    TEST(model_includes_shortcuts);
+    auto wf = sampleReviewState();
+    auto model = ReviewComparisonView::build(wf, "r1");
+    CHECK(model.has_value(), "expected model");
+    CHECK(model->approveShortcut == "Ctrl+Shift+A", "approve shortcut missing");
+    CHECK(model->rejectShortcut == "Ctrl+Shift+R", "reject shortcut missing");
+    PASS();
+}
+
+void test_change_notes_include_routing() {
+    TEST(change_notes_include_routing);
+    auto wf = sampleReviewState();
+    auto model = ReviewComparisonView::build(wf, "r1");
+    CHECK(model.has_value(), "expected model");
+    bool hasRouting = false;
+    for (const auto& n : model->changeNotes) if (n.find("routing:") == 0) hasRouting = true;
+    CHECK(hasRouting, "routing note expected");
+    PASS();
+}
+
+void test_change_notes_include_rejection_feedback() {
+    TEST(change_notes_include_rejection_feedback);
+    auto wf = sampleReviewState();
+    auto model = ReviewComparisonView::build(wf, "r1");
+    CHECK(model.has_value(), "expected model");
+    bool hasFeedback = false;
+    for (const auto& n : model->changeNotes) if (n.find("latest-feedback:") == 0) hasFeedback = true;
+    CHECK(hasFeedback, "feedback note expected");
+    PASS();
+}
+
+void test_approve_transitions_item_to_complete() {
+    TEST(approve_transitions_item_to_complete);
+    auto wf = sampleReviewState();
+    bool ok = ReviewComparisonView::approve(wf, "r1", "architect");
+    CHECK(ok, "approve should succeed");
+    auto updated = wf.queue.getItem("r1");
+    CHECK(updated.has_value(), "item should exist");
+    CHECK(updated->status == WI_COMPLETE, "status should be complete");
+    PASS();
+}
+
+void test_reject_transitions_item_to_ready() {
+    TEST(reject_transitions_item_to_ready);
+    auto wf = sampleReviewState();
+    bool ok = ReviewComparisonView::reject(wf, "r1", "needs tests", "architect");
+    CHECK(ok, "reject should succeed");
+    auto updated = wf.queue.getItem("r1");
+    CHECK(updated.has_value(), "item should exist");
+    CHECK(updated->status == WI_READY, "status should be ready after rejection");
+    PASS();
+}
+
+void test_reject_requires_feedback() {
+    TEST(reject_requires_feedback);
+    auto wf = sampleReviewState();
+    bool ok = ReviewComparisonView::reject(wf, "r1", "", "architect");
+    CHECK(!ok, "reject should fail without feedback");
+    PASS();
+}
+
+void test_non_review_item_disables_actions() {
+    TEST(non_review_item_disables_actions);
+    auto wf = sampleReviewState();
+    auto item = wf.queue.getItem("r1");
+    CHECK(item.has_value(), "item missing");
+    WorkItem updated = *item;
+    updated.status = WI_COMPLETE;
+    CHECK(wf.queue.updateItem("r1", updated), "update failed");
+
+    auto model = ReviewComparisonView::build(wf, "r1");
+    CHECK(model.has_value(), "expected model");
+    CHECK(!model->canApprove, "approve should be disabled");
+    CHECK(!model->canReject, "reject should be disabled");
+    PASS();
+}
+
+int main() {
+    std::cout << "Step 436: Review Comparison View Tests\n";
+
+    test_build_returns_model_for_existing_item();         // 1
+    test_build_returns_null_for_missing_item();           // 2
+    test_diff_contains_modified_line_entries();           // 3
+    test_review_status_enables_approve_and_reject();      // 4
+    test_shortcuts_match_spec();                          // 5
+    test_model_includes_shortcuts();                      // 6
+    test_change_notes_include_routing();                  // 7
+    test_change_notes_include_rejection_feedback();       // 8
+    test_approve_transitions_item_to_complete();          // 9
+    test_reject_transitions_item_to_ready();              // 10
+    test_reject_requires_feedback();                      // 11
+    test_non_review_item_disables_actions();              // 12
+
+    std::cout << "\nResults: " << passed << "/" << (passed + failed)
+              << " passed\n";
+    return failed == 0 ? 0 : 1;
+}

progress.md

@@ -5129,6 +5129,51 @@ state into editor views, with status icon/color mapping and task-detail routes.
   - `editor/src/MCPServer.h` (`1940` > `600`)
   - `editor/src/HeadlessAgentRPCHandler.h` (`2768` > `600`)
 
+### Step 436: Review Comparison View
+**Status:** PASS (12/12 tests)
+
+Added a review-comparison model that produces side-by-side diff rows and
+integrates approve/reject actions with keyboard shortcut metadata.
+
+**Files created:**
+- `editor/src/ReviewComparisonView.h` — review diff model support:
+  - workflow item -> comparison model projection
+  - skeleton/generated side labels
+  - line-oriented diff rows (added/removed/modified/unchanged)
+  - contextual change notes (routing, feedback, annotations)
+  - action availability flags keyed to review status
+  - approve/reject wrappers delegated to workflow detail logic
+  - keyboard shortcut constants (`Ctrl+Shift+A`, `Ctrl+Shift+R`)
+- `editor/tests/step436_test.cpp` — 12 tests covering:
+  1. model generation for existing item
+  2. missing-item null handling
+  3. modified-line diff detection
+  4. review-state action enablement
+  5. shortcut constant mapping
+  6. shortcut propagation into model
+  7. routing note inclusion
+  8. rejection-feedback note inclusion
+  9. approve transition to complete
+  10. reject transition to ready
+  11. reject-feedback requirement
+  12. non-review action disablement
+
+**Files modified:**
+- `editor/CMakeLists.txt` — `step436_test` target
+
+**Verification run:**
+- `step436_test` — PASS (12/12) new step coverage
+- `step435_test` — PASS (12/12) regression coverage
+- `step434_test` — PASS (12/12) regression coverage
+
+**Architecture gate check:**
+- `editor/src/ReviewComparisonView.h` within header-size limit (`118` <= `600`)
+- `editor/tests/step436_test.cpp` within test-file size guidance (`173` lines)
+- Legacy oversized headers persist:
+  - `editor/src/ast/Serialization.h` (`1427` > `600`)
+  - `editor/src/MCPServer.h` (`1940` > `600`)
+  - `editor/src/HeadlessAgentRPCHandler.h` (`2768` > `600`)
+
 # Roadmap Planning — Sprints 12-25+
 
 ## Status: Planning Complete (Sprints 12-19 detailed, 20-25 in roadmap.md)