From 33ba6c938e36152763a361d5c27e0ec1904c2fec Mon Sep 17 00:00:00 2001
From: Lakshman Patel <Lakshmanp230@gmail.com>
Date: Tue, 15 Sep 2026 23:47:36 +0530
Subject: [PATCH] refactor: dedup diff walking, typed tool inputs, TUI case
 extraction, lock drift check

- engine/diff: delete byte-identical splitLinesStaging/computeLCSStaging in
  favor of the shared splitLines/computeLCS; fold local editEntry into the
  shared edit type. Add tested ParseUnifiedAddedLines/ParseHunkNewStart and
  route cmd/pr.go analyzeDiff through it (added-line numbers now follow hunk
  headers exactly; old loop was off by one after context lines).
- tool: add generic DecodeInput[T] for consistent input errors; migrate
  SleepTool and AskUserQuestionTool to typed Execute + Schema-backed
  Parameters with parity tests.
- cmd: extract 11 terminal Update cases (no shared-tail dependency) into
  cmd/chat_update_system.go as methods; dispatch stays one line per case.
  KeyMsg/WindowSize/spinner.Tick fall through to the shared tail and stay.
- plugin: add SkillsLock.Verify/VerifyInstalledSkills (missing file, hash
  mismatch, unpinned entry); surface capped warnings in rho doctor. No
  signature scheme: manifests have no signature fields and installs already
  gate on folder trust + manifest presence + threat scan.
---
 cmd/chat_update.go                        | 140 ++-----------------
 cmd/chat_update_system.go                 | 163 ++++++++++++++++++++++
 cmd/diagnostics.go                        |  15 ++
 cmd/pr.go                                 |  45 +-----
 cmd/pr_test.go                            |  11 ++
 internal/engine/diff/diff_staging.go      |  84 +++--------
 internal/engine/diff/diff_staging_test.go |   8 +-
 internal/engine/diff/unified.go           |  81 +++++++++++
 internal/engine/diff/unified_test.go      | 103 ++++++++++++++
 internal/plugin/skillslock.go             |  50 +++++++
 internal/plugin/skillslock_test.go        |  47 +++++++
 internal/tool/ask_user.go                 |  45 +++---
 internal/tool/ask_user_test.go            |  21 +++
 internal/tool/sleep.go                    |  32 +++--
 internal/tool/sleep_test.go               |  72 ++++++++++
 internal/tool/typed.go                    |  22 +++
 16 files changed, 667 insertions(+), 272 deletions(-)
 create mode 100644 cmd/chat_update_system.go
 create mode 100644 internal/engine/diff/unified.go
 create mode 100644 internal/engine/diff/unified_test.go
 create mode 100644 internal/tool/sleep_test.go
 create mode 100644 internal/tool/typed.go

diff --git a/cmd/chat_update.go b/cmd/chat_update.go
index 075cb66a8..c4cfc8da8 100644
--- a/cmd/chat_update.go
+++ b/cmd/chat_update.go
@@ -137,105 +137,23 @@ func (m chatModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
 
 	switch msg := msg.(type) {
 	case sessionSaveResultMsg:
-		if msg.err != nil {
-			m.recordWALError(msg.err)
-			return m, nil
-		}
-		// Remove the WAL only when no append happened after the snapshot.
-		// Doing this in the background save closure could race with a new
-		// submission and lose messages that were not in the saved session.
-		if msg.id == m.sessionID && msg.seq == m.walSeq && m.wal != nil {
-			if err := m.wal.Remove(); err != nil {
-				m.recordWALError(err)
-			} else {
-				m.wal = nil
-			}
-		}
-		return m, nil
+		return m.updateSessionSaveResult(msg)
 	case tea.FocusMsg:
-		m.viewDirty = true
-		m.updateViewportContent()
-		if focus := m.ensurePromptInputFocus(); focus != nil {
-			return m, focus
-		}
-		return m, nil
-
+		return m.updateFocusMsg(msg)
 	case tea.BlurMsg:
-		if m.uiFocus == focusPrompt && !m.configOpen && !m.useConfigInput {
-			m.input.Blur()
-		}
-		// Track that the terminal lost focus during a turn so we can notify
-		// the user when the agent completes.
-		if m.waiting {
-			m.backgrounded = true
-		}
-		m.viewDirty = true
-		m.updateViewportContent()
-		return m, nil
-
+		return m.updateBlurMsg(msg)
 	case tea.InterruptMsg:
-		// External SIGINT delivered while the terminal is not in raw mode
-		// (e.g. `kill -INT`, tmux/screen `prefix` + ctrl+c). Bubble Tea would
-		// otherwise exit without saving the session.
-		return m.quitModel()
-
+		return m.updateInterruptMsg(msg)
 	case tea.QuitMsg:
-		// SIGTERM (e.g. `kill <pid>`, terminal close on some platforms).
-		// Exit through the same save-and-cleanup path as Ctrl+C.
-		return m.quitModel()
-
+		return m.updateQuitMsg(msg)
 	case promptKeepAliveMsg:
-		if m.uiFocus == focusPrompt && !m.configOpen && !m.useConfigInput {
-			if !m.input.Focused() {
-				m.viewDirty = true
-				m.updateViewportContent()
-				return m, tea.Batch(promptKeepAliveCmd(), m.input.Focus())
-			}
-		}
-		return m, promptKeepAliveCmd()
-
+		return m.updatePromptKeepAliveMsg(msg)
 	case statusLeftPRsMsg:
-		// Async PR lookup result — only apply if we're still on the same branch.
-		if m.statusLeftBranch == msg.branch {
-			m.statusLeftPRs = msg.nums
-			m.viewDirty = true
-			m.updateViewportContent()
-		}
-		return m, nil
-
+		return m.updateStatusLeftPRsMsg(msg)
 	case eyeBlinkTickMsg:
-		if m.showWelcomeBanner() {
-			m.eyeFrame = 1
-			m.rebuildWelcomeCache()
-			if len(m.messages) > 0 && m.messages[0].role == "welcome" {
-				m.messages[0].content = m.welcomeCache
-			}
-			m.viewDirty = true
-			m.updateViewportContent()
-			return m, tea.Batch(eyeBlinkTickCmd(), eyeFrameNextCmd(2, 60*time.Millisecond))
-		}
-		return m, eyeBlinkTickCmd()
-
+		return m.updateEyeBlinkTickMsg(msg)
 	case eyeFrameNextMsg:
-		if m.showWelcomeBanner() {
-			m.eyeFrame = msg.frame
-			m.rebuildWelcomeCache()
-			if len(m.messages) > 0 && m.messages[0].role == "welcome" {
-				m.messages[0].content = m.welcomeCache
-			}
-			m.viewDirty = true
-			m.updateViewportContent()
-			switch msg.frame {
-			case 2:
-				return m, eyeFrameNextCmd(3, 100*time.Millisecond)
-			case 3:
-				return m, eyeFrameNextCmd(0, 60*time.Millisecond)
-			}
-		} else {
-			m.eyeFrame = 0
-		}
-		return m, nil
-
+		return m.updateEyeFrameNextMsg(msg)
 	case tea.MouseMsg:
 		if m.mouseEnabled() {
 			if m.configOpen {
@@ -264,45 +182,9 @@ func (m chatModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
 		return m, tea.Batch(cmds...)
 
 	case processArrowTickMsg:
-		// A matching seq means no newer arrow keypress has arrived since this
-		// tick was armed, so the burst is over — clear the flag unconditionally.
-		// Without this, a burst's final keypress (which arms no tick of its
-		// own) would leave arrowBurstActive stuck true forever, silently
-		// swallowing every subsequent keystroke (see applyPromptArrowKey).
-		if m.arrowSeq == msg.seq {
-			m.arrowBurstActive = false
-		}
-		if m.pendingArrow != nil && m.arrowSeq == msg.seq {
-			msgToProcess := *m.pendingArrow
-			m.pendingArrow = nil
-			m.arrowBurstActive = false
-			m.processingGenuineArrow = true
-			next, cmd := m.Update(msgToProcess)
-			if nextModel, ok := next.(chatModel); ok {
-				nextModel.processingGenuineArrow = false
-				return nextModel, cmd
-			}
-			return next, cmd
-		}
-		return m, nil
-
+		return m.updateProcessArrowTickMsg(msg)
 	case tea.PasteMsg:
-		if m.configOpen {
-			var cmd tea.Cmd
-			m.configInput, cmd = m.configInput.Update(msg)
-			m.viewDirty = true
-			m.updateViewportContent()
-			return m, cmd
-		}
-		if m.uiFocus == focusPrompt {
-			var cmd tea.Cmd
-			m.input, cmd = m.input.Update(msg)
-			m.viewDirty = true
-			m.updateViewportContent()
-			return m, cmd
-		}
-		return m, nil
-
+		return m.updatePasteMsg(msg)
 	case tea.KeyMsg:
 		s := msg.String()
 		if (s == "up" || s == "down") && !m.processingGenuineArrow {
diff --git a/cmd/chat_update_system.go b/cmd/chat_update_system.go
new file mode 100644
index 000000000..1d64032e7
--- /dev/null
+++ b/cmd/chat_update_system.go
@@ -0,0 +1,163 @@
+package cmd
+
+import (
+	"time"
+
+	tea "charm.land/bubbletea/v2"
+)
+
+// System-message handlers extracted from chatModel.Update. Each
+// handles one terminal message type (every path returns); the dispatch
+// switch in Update keeps only the one-line call.
+
+func (m chatModel) updateSessionSaveResult(msg sessionSaveResultMsg) (tea.Model, tea.Cmd) {
+	if msg.err != nil {
+		m.recordWALError(msg.err)
+		return m, nil
+	}
+	// Remove the WAL only when no append happened after the snapshot.
+	// Doing this in the background save closure could race with a new
+	// submission and lose messages that were not in the saved session.
+	if msg.id == m.sessionID && msg.seq == m.walSeq && m.wal != nil {
+		if err := m.wal.Remove(); err != nil {
+			m.recordWALError(err)
+		} else {
+			m.wal = nil
+		}
+	}
+	return m, nil
+}
+
+func (m chatModel) updateFocusMsg(msg tea.FocusMsg) (tea.Model, tea.Cmd) {
+	m.viewDirty = true
+	m.updateViewportContent()
+	if focus := m.ensurePromptInputFocus(); focus != nil {
+		return m, focus
+	}
+	return m, nil
+}
+
+func (m chatModel) updateBlurMsg(msg tea.BlurMsg) (tea.Model, tea.Cmd) {
+	if m.uiFocus == focusPrompt && !m.configOpen && !m.useConfigInput {
+		m.input.Blur()
+	}
+	// Track that the terminal lost focus during a turn so we can notify
+	// the user when the agent completes.
+	if m.waiting {
+		m.backgrounded = true
+	}
+	m.viewDirty = true
+	m.updateViewportContent()
+	return m, nil
+}
+
+func (m chatModel) updateInterruptMsg(msg tea.InterruptMsg) (tea.Model, tea.Cmd) {
+	// External SIGINT delivered while the terminal is not in raw mode
+	// (e.g. `kill -INT`, tmux/screen `prefix` + ctrl+c). Bubble Tea would
+	// otherwise exit without saving the session.
+	return m.quitModel()
+}
+
+func (m chatModel) updateQuitMsg(msg tea.QuitMsg) (tea.Model, tea.Cmd) {
+	// SIGTERM (e.g. `kill <pid>`, terminal close on some platforms).
+	// Exit through the same save-and-cleanup path as Ctrl+C.
+	return m.quitModel()
+}
+
+func (m chatModel) updatePromptKeepAliveMsg(msg promptKeepAliveMsg) (tea.Model, tea.Cmd) {
+	if m.uiFocus == focusPrompt && !m.configOpen && !m.useConfigInput {
+		if !m.input.Focused() {
+			m.viewDirty = true
+			m.updateViewportContent()
+			return m, tea.Batch(promptKeepAliveCmd(), m.input.Focus())
+		}
+	}
+	return m, promptKeepAliveCmd()
+}
+
+func (m chatModel) updateStatusLeftPRsMsg(msg statusLeftPRsMsg) (tea.Model, tea.Cmd) {
+	// Async PR lookup result — only apply if we're still on the same branch.
+	if m.statusLeftBranch == msg.branch {
+		m.statusLeftPRs = msg.nums
+		m.viewDirty = true
+		m.updateViewportContent()
+	}
+	return m, nil
+}
+
+func (m chatModel) updateEyeBlinkTickMsg(msg eyeBlinkTickMsg) (tea.Model, tea.Cmd) {
+	if m.showWelcomeBanner() {
+		m.eyeFrame = 1
+		m.rebuildWelcomeCache()
+		if len(m.messages) > 0 && m.messages[0].role == "welcome" {
+			m.messages[0].content = m.welcomeCache
+		}
+		m.viewDirty = true
+		m.updateViewportContent()
+		return m, tea.Batch(eyeBlinkTickCmd(), eyeFrameNextCmd(2, 60*time.Millisecond))
+	}
+	return m, eyeBlinkTickCmd()
+}
+
+func (m chatModel) updateEyeFrameNextMsg(msg eyeFrameNextMsg) (tea.Model, tea.Cmd) {
+	if m.showWelcomeBanner() {
+		m.eyeFrame = msg.frame
+		m.rebuildWelcomeCache()
+		if len(m.messages) > 0 && m.messages[0].role == "welcome" {
+			m.messages[0].content = m.welcomeCache
+		}
+		m.viewDirty = true
+		m.updateViewportContent()
+		switch msg.frame {
+		case 2:
+			return m, eyeFrameNextCmd(3, 100*time.Millisecond)
+		case 3:
+			return m, eyeFrameNextCmd(0, 60*time.Millisecond)
+		}
+	} else {
+		m.eyeFrame = 0
+	}
+	return m, nil
+}
+
+func (m chatModel) updateProcessArrowTickMsg(msg processArrowTickMsg) (tea.Model, tea.Cmd) {
+	// A matching seq means no newer arrow keypress has arrived since this
+	// tick was armed, so the burst is over — clear the flag unconditionally.
+	// Without this, a burst's final keypress (which arms no tick of its
+	// own) would leave arrowBurstActive stuck true forever, silently
+	// swallowing every subsequent keystroke (see applyPromptArrowKey).
+	if m.arrowSeq == msg.seq {
+		m.arrowBurstActive = false
+	}
+	if m.pendingArrow != nil && m.arrowSeq == msg.seq {
+		msgToProcess := *m.pendingArrow
+		m.pendingArrow = nil
+		m.arrowBurstActive = false
+		m.processingGenuineArrow = true
+		next, cmd := m.Update(msgToProcess)
+		if nextModel, ok := next.(chatModel); ok {
+			nextModel.processingGenuineArrow = false
+			return nextModel, cmd
+		}
+		return next, cmd
+	}
+	return m, nil
+}
+
+func (m chatModel) updatePasteMsg(msg tea.PasteMsg) (tea.Model, tea.Cmd) {
+	if m.configOpen {
+		var cmd tea.Cmd
+		m.configInput, cmd = m.configInput.Update(msg)
+		m.viewDirty = true
+		m.updateViewportContent()
+		return m, cmd
+	}
+	if m.uiFocus == focusPrompt {
+		var cmd tea.Cmd
+		m.input, cmd = m.input.Update(msg)
+		m.viewDirty = true
+		m.updateViewportContent()
+		return m, cmd
+	}
+	return m, nil
+}
diff --git a/cmd/diagnostics.go b/cmd/diagnostics.go
index 199267771..ce913295c 100644
--- a/cmd/diagnostics.go
+++ b/cmd/diagnostics.go
@@ -13,6 +13,7 @@ import (
 	"time"
 
 	rhoconfig "github.com/GrayCodeAI/rho/internal/config"
+	"github.com/GrayCodeAI/rho/internal/plugin"
 	"github.com/GrayCodeAI/rho/internal/resilience/health"
 	"github.com/GrayCodeAI/rho/internal/session"
 	"github.com/GrayCodeAI/rho/internal/storage"
@@ -98,6 +99,20 @@ func doctorReport(settings rhoconfig.Settings) string {
 	} else {
 		b.WriteString("Installed skills: none (install with `rho skills install`)\n")
 	}
+	// Warn when installed skills drifted from the lockfile pins (content
+	// changed or SKILL.md removed after install). Warn-only: local edits are
+	// legitimate, they just mean the install is no longer reproducible.
+	if drift := plugin.VerifyInstalledSkills("user"); len(drift) > 0 {
+		b.WriteString("Skills lock drift:\n")
+		const maxDriftLines = 5
+		for i, line := range drift {
+			if i >= maxDriftLines {
+				b.WriteString(fmt.Sprintf("  - ... and %d more (reinstall to re-pin)\n", len(drift)-maxDriftLines))
+				break
+			}
+			b.WriteString("  - " + line + "\n")
+		}
+	}
 
 	b.WriteString(fmt.Sprintf("Configured MCP servers: %d\n", len(settings.MCPServers)+len(mcpServers)))
 	b.WriteString(fmt.Sprintf("Built-in tools: %d\n", len(allTools())))
diff --git a/cmd/pr.go b/cmd/pr.go
index d8f3d1e21..9aaf067b4 100644
--- a/cmd/pr.go
+++ b/cmd/pr.go
@@ -8,6 +8,7 @@ import (
 	"strconv"
 	"strings"
 
+	enginediff "github.com/GrayCodeAI/rho/internal/engine/diff"
 	"github.com/spf13/cobra"
 )
 
@@ -299,47 +300,15 @@ type finding struct {
 }
 
 // analyzeDiff performs a basic static analysis pass over a unified diff.
+// File/line tracking is shared with the engine diff package; the heuristic
+// checks in checkLine stay here. Note: added-line numbers now follow the
+// hunk headers exactly (context and added lines advance the new-file
+// counter); the previous inline walker was off by one after context lines.
 func analyzeDiff(diff string) []finding {
 	var findings []finding
 
-	lines := strings.Split(diff, "\n")
-	currentFile := ""
-	currentLine := 0
-
-	for _, line := range lines {
-		// Track current file
-		if strings.HasPrefix(line, "+++ b/") {
-			currentFile = strings.TrimPrefix(line, "+++ b/")
-			currentLine = 0
-			continue
-		}
-
-		// Track line numbers from hunk headers
-		if strings.HasPrefix(line, "@@") {
-			// Parse @@ -a,b +c,d @@
-			parts := strings.Split(line, "+")
-			if len(parts) >= 2 {
-				numStr := strings.Split(parts[1], ",")[0]
-				if n, err := strconv.Atoi(numStr); err == nil {
-					currentLine = n
-					continue
-				}
-			}
-		}
-
-		// Only analyze added lines
-		if !strings.HasPrefix(line, "+") || strings.HasPrefix(line, "+++") {
-			if strings.HasPrefix(line, "+") || strings.HasPrefix(line, " ") {
-				currentLine++
-			}
-			continue
-		}
-
-		added := line[1:]
-		currentLine++
-
-		// Check for common issues
-		if f := checkLine(added, currentFile, currentLine); f != nil {
+	for _, added := range enginediff.ParseUnifiedAddedLines(diff) {
+		if f := checkLine(added.Text, added.File, added.Line); f != nil {
 			findings = append(findings, *f)
 		}
 	}
diff --git a/cmd/pr_test.go b/cmd/pr_test.go
index e7bbd2557..5c34c4541 100644
--- a/cmd/pr_test.go
+++ b/cmd/pr_test.go
@@ -118,3 +118,14 @@ func TestRequireGH(t *testing.T) {
 	// Just verify it doesn't panic — gh may or may not be installed
 	_ = err
 }
+
+func TestAnalyzeDiff_CorrectedLineNumbers(t *testing.T) {
+	diff := "diff --git a/main.go b/main.go\n--- a/main.go\n+++ b/main.go\n@@ -1,2 +1,3 @@\n package main\n+// TODO: fix\n+import \"log\"\n"
+	findings := analyzeDiff(diff)
+	if len(findings) != 1 {
+		t.Fatalf("analyzeDiff() found %d issues, want 1", len(findings))
+	}
+	if findings[0].file != "main.go" || findings[0].line != 2 {
+		t.Errorf("finding = %+v, want file main.go line 2", findings[0])
+	}
+}
diff --git a/internal/engine/diff/diff_staging.go b/internal/engine/diff/diff_staging.go
index e36301f32..983b4284b 100644
--- a/internal/engine/diff/diff_staging.go
+++ b/internal/engine/diff/diff_staging.go
@@ -300,7 +300,7 @@ func (sa *StagingArea) buildApprovedContent(change *StagedChange) string {
 	}
 
 	// Partial apply: reconstruct content applying only approved hunks
-	originalLines := splitLinesStaging(change.Original)
+	originalLines := splitLines(change.Original)
 	var result []string
 	origIdx := 0
 
@@ -347,10 +347,10 @@ func (sa *StagingArea) buildApprovedContent(change *StagedChange) string {
 
 // computeStagedHunks computes hunks between original and modified content.
 func computeStagedHunks(original, modified string) []StagedHunk {
-	oldLines := splitLinesStaging(original)
-	newLines := splitLinesStaging(modified)
+	oldLines := splitLines(original)
+	newLines := splitLines(modified)
 
-	lcs := computeLCSStaging(oldLines, newLines)
+	lcs := computeLCS(oldLines, newLines)
 
 	// Build edit script
 	type editOp struct {
@@ -449,94 +449,44 @@ func computeStagedHunks(original, modified string) []StagedHunk {
 	return hunks
 }
 
-// splitLinesStaging splits content into lines, handling empty input.
-func splitLinesStaging(s string) []string {
-	if s == "" {
-		return nil
-	}
-	lines := strings.Split(s, "\n")
-	if len(lines) > 0 && lines[len(lines)-1] == "" {
-		lines = lines[:len(lines)-1]
-	}
-	return lines
-}
-
-// computeLCSStaging returns the longest common subsequence of two string slices.
-func computeLCSStaging(a, b []string) []string {
-	m, n := len(a), len(b)
-	dp := make([][]int, m+1)
-	for i := range dp {
-		dp[i] = make([]int, n+1)
-	}
-	for i := 1; i <= m; i++ {
-		for j := 1; j <= n; j++ {
-			if a[i-1] == b[j-1] {
-				dp[i][j] = dp[i-1][j-1] + 1
-			} else if dp[i-1][j] >= dp[i][j-1] {
-				dp[i][j] = dp[i-1][j]
-			} else {
-				dp[i][j] = dp[i][j-1]
-			}
-		}
-	}
-
-	lcs := make([]string, 0, dp[m][n])
-	i, j := m, n
-	for i > 0 && j > 0 {
-		if a[i-1] == b[j-1] {
-			lcs = append(lcs, a[i-1])
-			i--
-			j--
-		} else if dp[i-1][j] >= dp[i][j-1] {
-			i--
-		} else {
-			j--
-		}
-	}
-	for left, right := 0, len(lcs)-1; left < right; left, right = left+1, right-1 {
-		lcs[left], lcs[right] = lcs[right], lcs[left]
-	}
-	return lcs
-}
-
 // unifiedDiffStaging produces a unified diff between old and new content.
 func unifiedDiffStaging(old, new, path string) string {
-	oldLines := splitLinesStaging(old)
-	newLines := splitLinesStaging(new)
+	oldLines := splitLines(old)
+	newLines := splitLines(new)
 
 	var b strings.Builder
 	b.WriteString(fmt.Sprintf("--- a/%s\n", path))
 	b.WriteString(fmt.Sprintf("+++ b/%s\n", path))
 
-	lcs := computeLCSStaging(oldLines, newLines)
+	lcs := computeLCS(oldLines, newLines)
 
-	type editEntry struct {
+	type edit struct {
 		op   byte
 		line string
 	}
 
-	var edits []editEntry
+	var edits []edit
 	oi, ni, li := 0, 0, 0
 	for li < len(lcs) {
 		for oi < len(oldLines) && oldLines[oi] != lcs[li] {
-			edits = append(edits, editEntry{'-', oldLines[oi]})
+			edits = append(edits, edit{'-', oldLines[oi]})
 			oi++
 		}
 		for ni < len(newLines) && newLines[ni] != lcs[li] {
-			edits = append(edits, editEntry{'+', newLines[ni]})
+			edits = append(edits, edit{'+', newLines[ni]})
 			ni++
 		}
-		edits = append(edits, editEntry{' ', lcs[li]})
+		edits = append(edits, edit{' ', lcs[li]})
 		oi++
 		ni++
 		li++
 	}
 	for oi < len(oldLines) {
-		edits = append(edits, editEntry{'-', oldLines[oi]})
+		edits = append(edits, edit{'-', oldLines[oi]})
 		oi++
 	}
 	for ni < len(newLines) {
-		edits = append(edits, editEntry{'+', newLines[ni]})
+		edits = append(edits, edit{'+', newLines[ni]})
 		ni++
 	}
 
@@ -633,10 +583,10 @@ func unifiedDiffStaging(old, new, path string) string {
 
 // countAddsDels counts added and deleted lines between old and new content.
 func countAddsDels(old, new string) (int, int) {
-	oldLines := splitLinesStaging(old)
-	newLines := splitLinesStaging(new)
+	oldLines := splitLines(old)
+	newLines := splitLines(new)
 
-	lcs := computeLCSStaging(oldLines, newLines)
+	lcs := computeLCS(oldLines, newLines)
 	dels := len(oldLines) - len(lcs)
 	adds := len(newLines) - len(lcs)
 	return adds, dels
diff --git a/internal/engine/diff/diff_staging_test.go b/internal/engine/diff/diff_staging_test.go
index 36ff4370e..0b2d7dcd6 100644
--- a/internal/engine/diff/diff_staging_test.go
+++ b/internal/engine/diff/diff_staging_test.go
@@ -628,7 +628,7 @@ func TestApplyAllSkipsRejected(t *testing.T) {
 	}
 }
 
-func TestSplitLinesStaging(t *testing.T) {
+func TestSplitLines(t *testing.T) {
 	tests := []struct {
 		input    string
 		expected []string
@@ -640,14 +640,14 @@ func TestSplitLinesStaging(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		got := splitLinesStaging(tt.input)
+		got := splitLines(tt.input)
 		if len(got) != len(tt.expected) {
-			t.Errorf("splitLinesStaging(%q) len = %d, want %d", tt.input, len(got), len(tt.expected))
+			t.Errorf("splitLines(%q) len = %d, want %d", tt.input, len(got), len(tt.expected))
 			continue
 		}
 		for i := range got {
 			if got[i] != tt.expected[i] {
-				t.Errorf("splitLinesStaging(%q)[%d] = %q, want %q", tt.input, i, got[i], tt.expected[i])
+				t.Errorf("splitLines(%q)[%d] = %q, want %q", tt.input, i, got[i], tt.expected[i])
 			}
 		}
 	}
diff --git a/internal/engine/diff/unified.go b/internal/engine/diff/unified.go
new file mode 100644
index 000000000..2781ee06f
--- /dev/null
+++ b/internal/engine/diff/unified.go
@@ -0,0 +1,81 @@
+package diff
+
+import (
+	"strconv"
+	"strings"
+)
+
+// AddedLine is one added line of a unified diff with its new-file location.
+type AddedLine struct {
+	File string
+	Line int
+	Text string
+}
+
+// ParseHunkNewStart extracts the new-file start line from a hunk header like
+// "@@ -a,b +c,d @@". ok is false when no parseable number is present, and the
+// caller must fall through to normal line handling.
+func ParseHunkNewStart(header string) (int, bool) {
+	parts := strings.Split(header, "+")
+	if len(parts) < 2 {
+		return 0, false
+	}
+	numStr := strings.Split(parts[1], ",")[0]
+	n, err := strconv.Atoi(numStr)
+	if err != nil {
+		return 0, false
+	}
+	return n, true
+}
+
+// ParseUnifiedAddedLines walks a unified diff and returns every added line
+// with its file path and new-file line number.
+//
+// File tracking follows "+++ b/<path>" headers; hunk headers reset the
+// counter to the header's new-file start. Context (" ") and added ("+")
+// lines advance the new-file counter; removed lines, "---"/"+++" headers,
+// and "diff --git" markers do not. A malformed hunk header leaves the counter
+// untouched and is itself skipped as a non-body line.
+func ParseUnifiedAddedLines(diff string) []AddedLine {
+	var out []AddedLine
+	currentFile := ""
+	currentLine := 0
+
+	for _, line := range strings.Split(diff, "\n") {
+		if strings.HasPrefix(line, "+++ b/") {
+			currentFile = strings.TrimPrefix(line, "+++ b/")
+			currentLine = 0
+			continue
+		}
+		if strings.HasPrefix(line, "---") || strings.HasPrefix(line, "+++") ||
+			strings.HasPrefix(line, "diff --git") {
+			continue
+		}
+
+		if strings.HasPrefix(line, "@@") {
+			if n, ok := ParseHunkNewStart(line); ok {
+				currentLine = n - 1
+				continue
+			}
+			// Malformed header: not a body line, keep counting state.
+			continue
+		}
+
+		switch {
+		case strings.HasPrefix(line, "+"):
+			currentLine++
+			out = append(out, AddedLine{File: currentFile, Line: currentLine, Text: line[1:]})
+		case strings.HasPrefix(line, " "):
+			currentLine++
+		case line == "":
+			// A bare empty line is an empty context line (git renders
+			// context line "" for an empty source line); it occupies a
+			// new-file line.
+			currentLine++
+		default:
+			// Removed ("-") and unknown lines do not advance the counter.
+		}
+	}
+
+	return out
+}
diff --git a/internal/engine/diff/unified_test.go b/internal/engine/diff/unified_test.go
new file mode 100644
index 000000000..eebad1f32
--- /dev/null
+++ b/internal/engine/diff/unified_test.go
@@ -0,0 +1,103 @@
+package diff
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestParseUnifiedAddedLines_MultiFileMultiHunk(t *testing.T) {
+	diff := "diff --git a/a.go b/a.go\n" +
+		"--- a/a.go\n" +
+		"+++ b/a.go\n" +
+		"@@ -1,2 +1,3 @@\n" +
+		" ctx\n" +
+		"-old\n" +
+		"+new1\n" +
+		"+new2\n" +
+		"@@ -10,1 +11,2 @@\n" +
+		"+new3\n" +
+		"diff --git a/b.py b/b.py\n" +
+		"--- a/b.py\n" +
+		"+++ b/b.py\n" +
+		"@@ -5,2 +5,2 @@\n" +
+		"-x = 1\n" +
+		"+x = 2\n"
+
+	got := ParseUnifiedAddedLines(diff)
+	want := []AddedLine{
+		{File: "a.go", Line: 2, Text: "new1"},
+		{File: "a.go", Line: 3, Text: "new2"},
+		{File: "a.go", Line: 11, Text: "new3"},
+		{File: "b.py", Line: 5, Text: "x = 2"},
+	}
+	if !reflect.DeepEqual(got, want) {
+		t.Fatalf("ParseUnifiedAddedLines() = %+v, want %+v", got, want)
+	}
+}
+
+func TestParseUnifiedAddedLines_DevNullAndMalformedHeader(t *testing.T) {
+	diff := "diff --git a/old.go b/new.go\n" +
+		"--- /dev/null\n" +
+		"+++ b/new.go\n" +
+		"@@ -0,0 +1,1 @@\n" +
+		"+hello\n" +
+		"@@ malformed header\n" +
+		"+after-bad-header\n"
+
+	got := ParseUnifiedAddedLines(diff)
+	// The malformed @@ line is skipped without touching the counter, so the
+	// next added line continues from the previous hunk.
+	want := []AddedLine{
+		{File: "new.go", Line: 1, Text: "hello"},
+		{File: "new.go", Line: 2, Text: "after-bad-header"},
+	}
+	if !reflect.DeepEqual(got, want) {
+		t.Fatalf("ParseUnifiedAddedLines() = %+v, want %+v", got, want)
+	}
+}
+
+func TestParseUnifiedAddedLines_BareEmptyContextLine(t *testing.T) {
+	diff := "+++ b/f.txt\n" +
+		"@@ -1,3 +1,3 @@\n" +
+		" a\n" +
+		"\n" +
+		"+b\n"
+
+	got := ParseUnifiedAddedLines(diff)
+	want := []AddedLine{{File: "f.txt", Line: 3, Text: "b"}}
+	if !reflect.DeepEqual(got, want) {
+		t.Fatalf("ParseUnifiedAddedLines() = %+v, want %+v", got, want)
+	}
+}
+
+func TestParseUnifiedAddedLines_Empty(t *testing.T) {
+	if got := ParseUnifiedAddedLines(""); len(got) != 0 {
+		t.Fatalf("expected no added lines, got %+v", got)
+	}
+	if got := ParseUnifiedAddedLines("just some prose\n"); len(got) != 0 {
+		t.Fatalf("expected no added lines, got %+v", got)
+	}
+}
+
+func TestParseUnifiedAddedLines_NoFileHeader(t *testing.T) {
+	// Lines outside any file header are still reported with an empty path,
+	// matching the historical review pass.
+	got := ParseUnifiedAddedLines("+const apiKey = \"x\"\n")
+	want := []AddedLine{{File: "", Line: 1, Text: `const apiKey = "x"`}}
+	if !reflect.DeepEqual(got, want) {
+		t.Fatalf("ParseUnifiedAddedLines() = %+v, want %+v", got, want)
+	}
+}
+
+func TestParseHunkNewStart(t *testing.T) {
+	n, ok := ParseHunkNewStart("@@ -1,2 +3,4 @@")
+	if !ok || n != 3 {
+		t.Fatalf("ParseHunkNewStart = %d,%v, want 3,true", n, ok)
+	}
+	if _, ok := ParseHunkNewStart("@@ malformed"); ok {
+		t.Fatal("expected ok=false for malformed header")
+	}
+	if _, ok := ParseHunkNewStart("not a header"); ok {
+		t.Fatal("expected ok=false for non-header")
+	}
+}
diff --git a/internal/plugin/skillslock.go b/internal/plugin/skillslock.go
index dea3d43ff..484c65683 100644
--- a/internal/plugin/skillslock.go
+++ b/internal/plugin/skillslock.go
@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"sort"
 
 	"github.com/GrayCodeAI/rho/internal/installtxn"
 	"github.com/GrayCodeAI/rho/internal/storage"
@@ -79,6 +80,55 @@ func (l *SkillsLock) Save(scope string) error {
 	return installtxn.WriteFileAtomically(path, append(data, '\n'), 0o600)
 }
 
+// Verify recomputes the installed SKILL.md hash for every locked skill and
+// reports drift as human-readable lines: a missing SKILL.md, a hash mismatch
+// (content changed since install), or an entry with no recorded hash (older
+// lockfiles predate hash recording). It never modifies anything; callers
+// decide whether to warn or fail.
+func (l *SkillsLock) Verify(skillDir string) []string {
+	if l == nil {
+		return nil
+	}
+	names := make([]string, 0, len(l.Skills))
+	for name := range l.Skills {
+		names = append(names, name)
+	}
+	sort.Strings(names)
+	var drift []string
+	for _, name := range names {
+		entry := l.Skills[name]
+		path := filepath.Join(skillDir, sanitizeName(name), "SKILL.md")
+		data, err := os.ReadFile(path) // #nosec G304 -- path is the fixed per-skill file under the skills dir, not raw external input
+		if err != nil {
+			if os.IsNotExist(err) {
+				drift = append(drift, fmt.Sprintf("skill %q is in the lockfile but SKILL.md is missing", name))
+			} else {
+				drift = append(drift, fmt.Sprintf("skill %q SKILL.md is unreadable: %v", name, err))
+			}
+			continue
+		}
+		if entry.ComputedHash == "" {
+			drift = append(drift, fmt.Sprintf("skill %q has no recorded hash; reinstall to pin it", name))
+			continue
+		}
+		if got := HashSkillContent(data); got != entry.ComputedHash {
+			drift = append(drift, fmt.Sprintf("skill %q SKILL.md changed since install (lock drift)", name))
+		}
+	}
+	return drift
+}
+
+// VerifyInstalledSkills loads the scope lockfile and reports installed-skill
+// drift. A missing or unreadable lockfile yields no warnings (nothing pinned
+// means nothing to verify); the returned lines are for display only.
+func VerifyInstalledSkills(scope string) []string {
+	lock, err := LoadSkillsLock(scope)
+	if err != nil {
+		return nil
+	}
+	return lock.Verify(filepath.Dir(SkillsLockPath(scope)))
+}
+
 // Set records or updates the entry for one skill.
 func (l *SkillsLock) Set(name string, entry SkillsLockEntry) {
 	if l.Skills == nil {
diff --git a/internal/plugin/skillslock_test.go b/internal/plugin/skillslock_test.go
index 588725c77..7db23917c 100644
--- a/internal/plugin/skillslock_test.go
+++ b/internal/plugin/skillslock_test.go
@@ -2,6 +2,8 @@ package plugin
 
 import (
 	"os"
+	"path/filepath"
+	"strings"
 	"testing"
 )
 
@@ -72,3 +74,48 @@ func TestHashSkillContentStable(t *testing.T) {
 		t.Fatal("different content must hash differently")
 	}
 }
+
+func TestSkillsLockVerify(t *testing.T) {
+	dir := t.TempDir()
+	writeSkill := func(name, content string) {
+		t.Helper()
+		if err := os.MkdirAll(filepath.Join(dir, name), 0o750); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.WriteFile(filepath.Join(dir, name, "SKILL.md"), []byte(content), 0o600); err != nil {
+			t.Fatal(err)
+		}
+	}
+	writeSkill("good", "# good")
+	writeSkill("tampered", "# tampered")
+
+	lock := &SkillsLock{Skills: map[string]SkillsLockEntry{
+		"good":     {ComputedHash: HashSkillContent([]byte("# good"))},
+		"tampered": {ComputedHash: HashSkillContent([]byte("# original"))},
+		"missing":  {ComputedHash: HashSkillContent([]byte("# gone"))},
+		"unpinned": {},
+	}}
+	got := lock.Verify(dir)
+	if len(got) != 3 {
+		t.Fatalf("Verify() = %v, want 3 drift lines", got)
+	}
+	joined := strings.Join(got, "\n")
+	for _, want := range []string{`"tampered"`, `"missing"`, `"unpinned"`} {
+		if !strings.Contains(joined, want) {
+			t.Errorf("drift lines %v missing %s", got, want)
+		}
+	}
+	if strings.Contains(joined, `"good"`) {
+		t.Errorf("matching skill reported as drift: %v", got)
+	}
+}
+
+func TestSkillsLockVerifyEmpty(t *testing.T) {
+	if got := (&SkillsLock{Skills: map[string]SkillsLockEntry{}}).Verify(t.TempDir()); len(got) != 0 {
+		t.Fatalf("empty lock Verify() = %v, want none", got)
+	}
+	var nilLock *SkillsLock
+	if got := nilLock.Verify(t.TempDir()); len(got) != 0 {
+		t.Fatalf("nil lock Verify() = %v, want none", got)
+	}
+}
diff --git a/internal/tool/ask_user.go b/internal/tool/ask_user.go
index 4bbad54a7..95236cb59 100644
--- a/internal/tool/ask_user.go
+++ b/internal/tool/ask_user.go
@@ -15,29 +15,26 @@ func (AskUserQuestionTool) Description() string {
 }
 
 func (AskUserQuestionTool) Parameters() map[string]interface{} {
-	return map[string]interface{}{
-		"type": "object",
-		"properties": map[string]interface{}{
-			"question": map[string]interface{}{"type": "string", "description": "The question to ask"},
-			"options": map[string]interface{}{
-				"type":        "array",
-				"items":       map[string]interface{}{"type": "string"},
-				"description": "Optional list of choices (for single-select)",
-			},
-			"multi_select": map[string]interface{}{
-				"type":        "boolean",
-				"description": "Allow multiple selections (default: false)",
-			},
-			"other": map[string]interface{}{
-				"type":        "boolean",
-				"description": "Allow free-text 'other' option (default: true)",
-			},
-			"cancel_message": map[string]interface{}{
-				"type":        "string",
-				"description": "Message to show on cancel",
-			},
+	return askUserSchema.ToJSONSchema()
+}
+
+// askUserSchema is the single source of truth for AskUserQuestion's input
+// schema. The "options" Items entry preserves the existing array-of-string
+// wire shape.
+var askUserSchema = AskUserQuestionTool{}.Schema()
+
+// Schema returns the typed input schema for AskUserQuestion.
+func (AskUserQuestionTool) Schema() ToolSchema {
+	return ToolSchema{
+		Type: "object",
+		Properties: map[string]SchemaProperty{
+			"question":       {Type: "string", Description: "The question to ask"},
+			"options":        {Type: "array", Description: "Optional list of choices (for single-select)", Items: &SchemaProperty{Type: "string"}},
+			"multi_select":   {Type: "boolean", Description: "Allow multiple selections (default: false)"},
+			"other":          {Type: "boolean", Description: "Allow free-text 'other' option (default: true)"},
+			"cancel_message": {Type: "string", Description: "Message to show on cancel"},
 		},
-		"required": []string{"question"},
+		Required: []string{"question"},
 	}
 }
 
@@ -51,8 +48,8 @@ type AskUserInput struct {
 }
 
 func (AskUserQuestionTool) Execute(ctx context.Context, input json.RawMessage) (string, error) {
-	var p AskUserInput
-	if err := json.Unmarshal(input, &p); err != nil {
+	p, err := DecodeInput[AskUserInput]("AskUserQuestion", input)
+	if err != nil {
 		return "", err
 	}
 	if p.Question == "" {
diff --git a/internal/tool/ask_user_test.go b/internal/tool/ask_user_test.go
index acdf87543..53aba830b 100644
--- a/internal/tool/ask_user_test.go
+++ b/internal/tool/ask_user_test.go
@@ -96,3 +96,24 @@ func TestAskUserQuestionTool_Execute_ValidationErrors(t *testing.T) {
 		t.Error("expected error when ask_user is not configured")
 	}
 }
+
+func TestAskUserSchemaProvider(t *testing.T) {
+	var _ SchemaProvider = AskUserQuestionTool{}
+	params := AskUserQuestionTool{}.Parameters()
+	props, ok := params["properties"].(map[string]interface{})
+	if !ok {
+		t.Fatal("properties missing")
+	}
+	// Array-of-string wire shape for options must survive.
+	opts, ok := props["options"].(map[string]interface{})
+	if !ok || opts["type"] != "array" {
+		t.Fatalf("options prop = %v, want array type", props["options"])
+	}
+	items, ok := opts["items"].(map[string]interface{})
+	if !ok || items["type"] != "string" {
+		t.Fatalf("options items = %v, want string type", opts["items"])
+	}
+	if req := params["required"].([]string); len(req) != 1 || req[0] != "question" {
+		t.Fatalf("required = %v, want [question]", params["required"])
+	}
+}
diff --git a/internal/tool/sleep.go b/internal/tool/sleep.go
index 51ef24ae8..790edeeca 100644
--- a/internal/tool/sleep.go
+++ b/internal/tool/sleep.go
@@ -16,21 +16,33 @@ func (SleepTool) Description() string {
 	return "Pause execution for a specified number of seconds."
 }
 
-func (SleepTool) Parameters() map[string]interface{} {
-	return map[string]interface{}{
-		"type": "object",
-		"properties": map[string]interface{}{
-			"seconds": map[string]interface{}{"type": "number", "description": "Duration to sleep in seconds (max 300)"},
+// SleepInput is the typed input for SleepTool.
+type SleepInput struct {
+	Seconds float64 `json:"seconds"`
+}
+
+// Schema returns the typed input schema. Parameters() delegates to it so the
+// two cannot diverge.
+func (SleepTool) Schema() ToolSchema {
+	return ToolSchema{
+		Type: "object",
+		Properties: map[string]SchemaProperty{
+			"seconds": {Type: "number", Description: "Duration to sleep in seconds (max 300)"},
 		},
-		"required": []string{"seconds"},
+		Required: []string{"seconds"},
 	}
 }
 
+func (SleepTool) Parameters() map[string]interface{} {
+	return sleepSchema.ToJSONSchema()
+}
+
+// sleepSchema is the single source of truth for Sleep's input schema.
+var sleepSchema = SleepTool{}.Schema()
+
 func (SleepTool) Execute(ctx context.Context, input json.RawMessage) (string, error) {
-	var p struct {
-		Seconds float64 `json:"seconds"`
-	}
-	if err := json.Unmarshal(input, &p); err != nil {
+	p, err := DecodeInput[SleepInput]("Sleep", input)
+	if err != nil {
 		return "", err
 	}
 	if p.Seconds <= 0 {
diff --git a/internal/tool/sleep_test.go b/internal/tool/sleep_test.go
new file mode 100644
index 000000000..7dc0fc981
--- /dev/null
+++ b/internal/tool/sleep_test.go
@@ -0,0 +1,72 @@
+package tool
+
+import (
+	"context"
+	"encoding/json"
+	"strings"
+	"testing"
+	"time"
+)
+
+func TestSleepSchemaProvider(t *testing.T) {
+	var _ SchemaProvider = SleepTool{}
+	params := SleepTool{}.Parameters()
+	if params["type"] != "object" {
+		t.Fatalf("Parameters type = %v, want object", params["type"])
+	}
+	props, ok := params["properties"].(map[string]interface{})
+	if !ok {
+		t.Fatal("properties missing")
+	}
+	sec, ok := props["seconds"].(map[string]interface{})
+	if !ok || sec["type"] != "number" {
+		t.Fatalf("seconds prop = %v, want number type", props["seconds"])
+	}
+	if req, ok := params["required"].([]string); !ok || len(req) != 1 || req[0] != "seconds" {
+		t.Fatalf("required = %v, want [seconds]", params["required"])
+	}
+}
+
+func TestSleepExecute(t *testing.T) {
+	ctx := context.Background()
+	out, err := (SleepTool{}).Execute(ctx, json.RawMessage(`{"seconds": 0.05}`))
+	if err != nil {
+		t.Fatalf("Execute: %v", err)
+	}
+	if !strings.Contains(out, "Slept") {
+		t.Fatalf("unexpected output %q", out)
+	}
+}
+
+func TestSleepExecuteValidation(t *testing.T) {
+	ctx := context.Background()
+	if _, err := (SleepTool{}).Execute(ctx, json.RawMessage(`{"seconds": 0}`)); err == nil {
+		t.Fatal("expected error for non-positive seconds")
+	}
+	if _, err := (SleepTool{}).Execute(ctx, json.RawMessage(`not json`)); err == nil {
+		t.Fatal("expected error for malformed JSON")
+	}
+	if _, err := (SleepTool{}).Execute(ctx, nil); err == nil {
+		t.Fatal("expected error for missing input")
+	}
+	// Cap: 300s max sleeps must not actually sleep 300s here; verify the cap
+	// path via context cancellation instead of wall time.
+	capped, cancel := context.WithCancel(context.Background())
+	cancel()
+	if _, err := (SleepTool{}).Execute(capped, json.RawMessage(`{"seconds": 300}`)); err == nil {
+		t.Fatal("expected cancellation error for capped sleep")
+	}
+}
+
+func TestSleepRespectsContext(t *testing.T) {
+	ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
+	defer cancel()
+	start := time.Now()
+	_, err := (SleepTool{}).Execute(ctx, json.RawMessage(`{"seconds": 30}`))
+	if err == nil {
+		t.Fatal("expected context error")
+	}
+	if time.Since(start) > 5*time.Second {
+		t.Fatal("sleep did not respect context cancellation")
+	}
+}
diff --git a/internal/tool/typed.go b/internal/tool/typed.go
new file mode 100644
index 000000000..af1bacfd4
--- /dev/null
+++ b/internal/tool/typed.go
@@ -0,0 +1,22 @@
+package tool
+
+import (
+	"encoding/json"
+	"fmt"
+)
+
+// DecodeInput unmarshals a tool's JSON input into a typed struct with a
+// consistent error message. It replaces the hand-rolled json.Unmarshal blocks
+// that every tool used to embed in Execute (each with its own inconsistent
+// error text). Required-field and schema validation still happen in
+// ValidateToolInput before dispatch; this only handles JSON decoding.
+func DecodeInput[T any](toolName string, input json.RawMessage) (T, error) {
+	var v T
+	if len(input) == 0 {
+		return v, fmt.Errorf("tool %s: missing input", toolName)
+	}
+	if err := json.Unmarshal(input, &v); err != nil {
+		return v, fmt.Errorf("tool %s: invalid JSON input: %w", toolName, err)
+	}
+	return v, nil
+}