theirongolddev/gitlore
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

chore(plans): remove ephemeral review feedback files

Browse Source 
Remove iterative feedback files that were used during plan development.
These files captured review rounds but are no longer needed now that the
plans have been finalized:

- plans/lore-service.feedback-{1,2,3,4}.md
- plans/time-decay-expert-scoring.feedback-{1,2,3,4}.md
- plans/tui-prd-v2-frankentui.feedback-{1,2,3,4,5,6,7,8,9}.md

The canonical plan documents remain; only the review iteration artifacts
are removed to reduce clutter.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
teernisse
2026-03-06 11:15:58 -05:00
parent 77445f6903
commit 2ab57d8d14
17 changed files with 0 additions and 3033 deletions
Download Patch File Download Diff File Expand all files Collapse all files

186
plans/lore-service.feedback-1.md
View File

@@ -1,186 +0,0 @@
1. **Isolate scheduled behavior from manual `sync`**
Reasoning: Your current plan injects backoff into `handle_sync_cmd`, which affects all `lore sync` calls (including manual recovery runs). Scheduled behavior should be isolated so humans aren’t unexpectedly blocked by service backoff.
```diff
@@ Context
-`lore sync` runs a 4-stage pipeline (issues, MRs, docs, embeddings) that takes 2-4 minutes.
+`lore sync` remains the manual/operator command.
+`lore service run` (hidden/internal) is the scheduled execution entrypoint.
@@ Commands & User Journeys
+### `lore service run` (hidden/internal)
+**What it does:** Executes one scheduled sync attempt with service-only policy:
+- applies service backoff policy
+- records service run state
+- invokes sync pipeline with configured profile
+- updates retry state on success/failure
+
+**Invocation:** scheduler always runs:
+`lore --robot service run --reason timer`
@@ Backoff Integration into `handle_sync_cmd`
-Insert **after** config load but **before** the dry_run check:
+Do not add backoff checks to `handle_sync_cmd`.
+Backoff logic lives only in `handle_service_run`.
```
2. **Use DB as source-of-truth for service state (not a standalone JSON status file)**
Reasoning: You already have `sync_runs` in SQLite. A separate JSON status file creates split-brain and race/corruption risk. Keep JSON as optional cache/export only.
```diff
@@ Status File
-Location: `{get_data_dir()}/sync-status.json`
+Primary state location: SQLite (`service_state` table) + existing `sync_runs`.
+Optional mirror file: `{get_data_dir()}/sync-status.json` (best-effort export only).
@@ File-by-File Implementation Details
-### `src/core/sync_status.rs` (NEW)
+### `migrations/015_service_state.sql` (NEW)
+CREATE TABLE service_state (
+ id INTEGER PRIMARY KEY CHECK (id = 1),
+ installed INTEGER NOT NULL DEFAULT 0,
+ platform TEXT,
+ interval_seconds INTEGER,
+ profile TEXT NOT NULL DEFAULT 'balanced',
+ consecutive_failures INTEGER NOT NULL DEFAULT 0,
+ next_retry_at_ms INTEGER,
+ last_error_code TEXT,
+ last_error_message TEXT,
+ updated_at_ms INTEGER NOT NULL
+);
+
+### `src/core/service_state.rs` (NEW)
+- read/write state row
+- derive backoff/next_retry
+- join with latest `sync_runs` for status output
```
3. **Backoff policy should be configurable, jittered, and error-aware**
Reasoning: Fixed hardcoded backoff (`base=1800`) is wrong when user sets another interval. Also permanent failures (bad token/config) should not burn retries forever; they should enter paused/error state.
```diff
@@ Backoff Logic
-// Exponential: base * 2^failures, capped at 4 hours
+// Exponential with jitter: base * 2^(failures-1), capped, ±20% jitter
+// Applies only to transient errors.
+// Permanent errors set `paused_reason` and stop retries until user action.
@@ CLI Definition Changes
+ServiceCommand::Resume, // clear paused state / failures
+ServiceCommand::Run, // hidden
@@ Error Types
+ServicePaused, // scheduler paused due to permanent error
+ServiceCommandFailed, // OS command failure with stderr context
```
4. **Add a pipeline-level single-flight lock**
Reasoning: Current locking is in ingest stages; there’s still overlap risk across full sync pipelines (docs/embed can overlap with another run). Add a top-level lock for scheduled/manual sync pipeline execution.
```diff
@@ Architecture
+Add `sync_pipeline` lock at top-level sync execution.
+Keep existing ingest lock (`sync`) for ingest internals.
@@ Backoff Integration into `handle_sync_cmd`
+Before starting sync pipeline, acquire `AppLock` with:
+name = "sync_pipeline"
+stale_lock_minutes = config.sync.stale_lock_minutes
+heartbeat_interval_seconds = config.sync.heartbeat_interval_seconds
```
5. **Don’t embed token in service files by default**
Reasoning: Embedding PAT into unit/plist is a high-risk secret leak path. Make secure storage explicit and default-safe.
```diff
@@ `lore service install [--interval 30m]`
+`lore service install [--interval 30m] [--token-source env-file|embedded]`
+Default: `env-file` (0600 perms, user-owned)
+`embedded` allowed only with explicit opt-in and warning
@@ Robot output
- "token_embedded": true
+ "token_source": "env_file"
@@ Human output
- Note: Your GITLAB_TOKEN is embedded in the service file.
+ Note: Token is stored in a user-private env file (0600).
```
6. **Introduce a command-runner abstraction with timeout + stderr capture**
Reasoning: `launchctl/systemctl/schtasks` calls are failure-prone; you need consistent error mapping and deterministic tests.
```diff
@@ Platform Backends
-exports free functions that dispatch via `#[cfg(target_os)]`
+exports backend + shared `CommandRunner`:
+- run(cmd, args, timeout)
+- capture stdout/stderr/exit code
+- map failure to `ServiceCommandFailed { cmd, exit_code, stderr }`
```
7. **Persist install manifest to avoid brittle file parsing**
Reasoning: Parsing timer/plist for interval/state is fragile and platform-format dependent. Persist a manifest with checksums and expected artifacts.
```diff
@@ Platform Backends
-Same pattern for ... `get_interval_seconds()`
+Add manifest: `{data_dir}/service-manifest.json`
+Stores platform, interval, profile, generated files, and command.
+`service status` reads manifest first, then verifies platform state.
@@ Acceptance criteria
+Install is idempotent:
+- if manifest+files already match, report `no_change: true`
+- if drift detected, reconcile and rewrite
```
8. **Make schedule profile explicit (`fast|balanced|full`)**
Reasoning: This makes the feature more useful and performance-tunable without requiring users to understand internal flags.
```diff
@@ `lore service install [--interval 30m]`
+`lore service install [--interval 30m] [--profile fast|balanced|full]`
+
+Profiles:
+- fast: `sync --no-docs --no-embed`
+- balanced (default): `sync --no-embed`
+- full: `sync`
```
9. **Upgrade `service status` to include scheduler health + recent run summary**
Reasoning: Single last-sync snapshot is too shallow. Include recent attempts and whether scheduler is paused/backing off/running.
```diff
@@ `lore service status`
-What it does: Shows whether the service is installed, its configuration, last sync result, and next scheduled run.
+What it does: Shows install state, scheduler state (running/backoff/paused), recent runs, and next run estimate.
@@ Robot output
- "last_sync": { ... },
- "backoff": null
+ "scheduler_state": "running|backoff|paused|idle",
+ "last_sync": { ... },
+ "recent_runs": [{"run_id":"...","status":"...","started_at_iso":"..."}],
+ "backoff": null,
+ "paused_reason": null
```
10. **Strengthen tests around determinism and cross-platform generation**
Reasoning: Time-based backoff and shell quoting are classic flaky points. Add fake clock + fake command runner for deterministic tests.
```diff
@@ Testing Strategy
+Add deterministic test seams:
+- `Clock` trait for backoff/now calculations
+- `CommandRunner` trait for backend command execution
+
+Add tests:
+- transient vs permanent error classification
+- backoff schedule with jitter bounds
+- manifest drift reconciliation
+- quoting/escaping for paths with spaces and special chars
+- `service run` does not modify manual `sync` behavior
```
If you want, I can rewrite your full plan as a single clean revised document with these changes already integrated (instead of patch fragments).

182
plans/lore-service.feedback-2.md
View File

@@ -1,182 +0,0 @@
**High-Impact Revisions (ordered by priority)**
1. **Make service identity project-scoped (avoid collisions across repos/users)**
Analysis: Current fixed names (`com.gitlore.sync`, `LoreSync`, `lore-sync.timer`) will collide when users run multiple gitlore workspaces. This causes silent overwrites and broken uninstall/status behavior.
Diff:
```diff
--- a/plan.md
+++ b/plan.md
@@ Commands & User Journeys / install
- lore service install [--interval 30m] [--profile balanced] [--token-source env-file]
+ lore service install [--interval 30m] [--profile balanced] [--token-source auto] [--name <optional>]
@@ Install Manifest Schema
+ /// Stable per-install identity (default derived from project root hash)
+ pub service_id: String,
@@ Platform Backends
- Label: com.gitlore.sync
+ Label: com.gitlore.sync.{service_id}
- Task name: LoreSync
+ Task name: LoreSync-{service_id}
- ~/.config/systemd/user/lore-sync.service
+ ~/.config/systemd/user/lore-sync-{service_id}.service
```
2. **Replace token model with secure per-OS defaults**
Analysis: The current “env-file default” is not actually secure on macOS launchd (token still ends up in plist). On Windows, assumptions about inherited environment are fragile. Use OS-native secure stores by default and keep `embedded` as explicit opt-in only.
Diff:
```diff
--- a/plan.md
+++ b/plan.md
@@ Token storage strategies
-| env-file (default) | ...
+| auto (default) | macOS: Keychain, Linux: env-file (0600), Windows: Credential Manager |
+| env-file | Linux/systemd only |
| embedded | ... explicit warning ...
@@ macOS launchd section
- env-file strategy stores canonical token in service-env but embeds token in plist
+ default strategy is Keychain lookup at runtime; no token persisted in plist
+ env-file is not offered on macOS
@@ Windows schtasks section
- token must be in user's system environment
+ default strategy stores token in Windows Credential Manager and injects at runtime
```
3. **Version and atomically persist manifest/status**
Analysis: `Option<Self>` on read hides corruption, and non-atomic writes risk truncated JSON on crashes. This will create false “not installed” and scheduler confusion.
Diff:
```diff
--- a/plan.md
+++ b/plan.md
@@ Install Manifest Schema
+ pub schema_version: u32, // start at 1
+ pub updated_at_iso: String,
@@ Status File Schema
+ pub schema_version: u32, // start at 1
+ pub updated_at_iso: String,
@@ Read/Write
- read(path) -> Option<Self>
+ read(path) -> Result<Option<Self>, LoreError>
- write(...) -> std::io::Result<()>
+ write_atomic(...) -> std::io::Result<()> // tmp file + fsync + rename
```
4. **Persist `next_retry_at_ms` instead of recomputing jitter**
Analysis: Deterministic jitter from timestamp modulo is predictable and can herd retries. Persisting `next_retry_at_ms` at failure time makes status accurate, stable, and cheap to compute.
Diff:
```diff
--- a/plan.md
+++ b/plan.md
@@ SyncStatusFile
pub consecutive_failures: u32,
+ pub next_retry_at_ms: Option<i64>,
@@ Backoff Logic
- compute backoff from last_run.timestamp_ms and deterministic jitter each read
+ compute backoff once on failure, store next_retry_at_ms, read-only comparison afterward
+ jitter algorithm: full jitter in [0, cap], injectable RNG for tests
```
5. **Add circuit breaker for repeated transient failures**
Analysis: Infinite transient retries can run forever on systemic failures (DB corruption, bad network policy). After N transient failures, pause with actionable reason.
Diff:
```diff
--- a/plan.md
+++ b/plan.md
@@ Scheduler states
- backoff — transient failures, waiting to retry
+ backoff — transient failures, waiting to retry
+ paused — permanent error OR circuit breaker tripped after N transient failures
@@ Service run flow
- On transient failure: increment failures, compute backoff
+ On transient failure: increment failures, compute backoff, if failures >= max_transient_failures -> pause
```
6. **Stage-aware outcome policy (core freshness over all-or-nothing)**
Analysis: Failing embeddings/docs should not block issues/MRs freshness. Split stage outcomes and only treat core stages as hard-fail by default. This improves reliability and practical usefulness.
Diff:
```diff
--- a/plan.md
+++ b/plan.md
@@ Context
- lore sync runs a 4-stage pipeline ... treated as one run result
+ lore service run records per-stage outcomes (issues, mrs, docs, embeddings)
@@ Status File Schema
+ pub stage_results: Vec<StageResult>,
@@ service run flow
- Execute sync pipeline with flags derived from profile
+ Execute stage-by-stage and classify severity:
+ - critical: issues, mrs
+ - optional: docs, embeddings
+ optional stage failures mark run as degraded, not failed
```
7. **Replace cfg free-function backend with trait-based backend**
Analysis: Current backend API is hard to test end-to-end without real OS commands. A `SchedulerBackend` trait enables deterministic integration tests and cleaner architecture.
Diff:
```diff
--- a/plan.md
+++ b/plan.md
@@ Platform Backends / Architecture
- exports free functions dispatched via #[cfg]
+ define trait SchedulerBackend { install, uninstall, state, file_paths, next_run }
+ provide LaunchdBackend, SystemdBackend, SchtasksBackend implementations
+ include FakeBackend for integration tests
```
8. **Harden platform units and detect scheduler prerequisites**
Analysis: systemd user timers often fail silently without user manager/linger; launchd context can be wrong in headless sessions. Add explicit diagnostics and unit hardening.
Diff:
```diff
--- a/plan.md
+++ b/plan.md
@@ Linux systemd unit
[Service]
Type=oneshot
ExecStart=...
+TimeoutStartSec=900
+NoNewPrivileges=true
+PrivateTmp=true
+ProtectSystem=strict
+ProtectHome=read-only
@@ Linux install/status
+ detect user manager availability and linger state; surface warning/action
@@ macOS install/status
+ detect non-GUI bootstrap context and return actionable error
```
9. **Add operational commands: `trigger`, `doctor`, and non-interactive log tail**
Analysis: `logs` opening an editor is weak for automation and incident response. Operators need a preflight and immediate controlled run.
Diff:
```diff
--- a/plan.md
+++ b/plan.md
@@ ServiceCommand
+ Trigger, // run one attempt through service policy now
+ Doctor, // validate scheduler, token, paths, permissions
@@ logs
- opens editor
+ supports --tail <n> and --follow in human mode
+ robot mode can return last_n lines optionally
```
10. **Fix plan inconsistencies and edge-case correctness**
Analysis: There are internal mismatches that will cause implementation drift.
Diff:
```diff
--- a/plan.md
+++ b/plan.md
@@ Interval Parsing
- supports 's' suffix
+ remove 's' suffix (acceptance only allows 5m..24h)
@@ uninstall acceptance
- removes ALL service files only
+ explicitly also remove service-manifest and service-env (status/logs retained)
@@ SyncStatusFile schema
- pub last_run: SyncRunRecord
+ pub last_run: Option<SyncRunRecord> // matches idle/no runs state
```
---
**Recommended Architecture Upgrade Summary**
The strongest improvement set is: **(1) project-scoped IDs, (2) secure token defaults, (3) atomic/versioned state, (4) persisted retry schedule + circuit breaker, (5) stage-aware outcomes**. That combination materially improves correctness, multi-repo safety, security, operability, and real-world reliability without changing your core manual-vs-scheduled separation principle.

174
plans/lore-service.feedback-3.md
View File

@@ -1,174 +0,0 @@
Below are the highest-impact revisions I’d make, ordered by severity/ROI. These focus on correctness first, then security, then operability and UX.
1. **Fix multi-install ambiguity (`service_id` exists, but commands can’t target one explicitly)**
Analysis: The plan introduces `service-manifest-{service_id}.json`, but `status/uninstall/resume/logs` have no selector. In a multi-workspace or multi-name install scenario, behavior becomes ambiguous and error-prone. Add explicit targeting plus discovery.
```diff
@@ ## Commands & User Journeys
+### `lore service list`
+Lists installed services discovered from `{data_dir}/service-manifest-*.json`.
+Robot output includes `service_id`, `platform`, `interval_seconds`, `profile`, `installed_at_iso`.
@@ ### `lore service uninstall`
-### `lore service uninstall`
+### `lore service uninstall [--service <service_id|name>] [--all]`
@@
-2. CLI reads install manifest to find `service_id`
+2. CLI resolves target service via `--service` or current-project-derived default.
+3. If multiple candidates and no selector, return actionable error.
@@ ### `lore service status`
-### `lore service status`
+### `lore service status [--service <service_id|name>]`
```
2. **Make status state service-scoped (not global)**
Analysis: A single `sync-status.json` for all services causes cross-service contamination (pause/backoff/outcome from one profile affecting another). Keep lock global, but state per service.
```diff
@@ ## Status File
-### Location
-`{get_data_dir()}/sync-status.json`
+### Location
+`{get_data_dir()}/sync-status-{service_id}.json`
@@ ## Paths Module Additions
-pub fn get_service_status_path() -> PathBuf {
- get_data_dir().join("sync-status.json")
+pub fn get_service_status_path(service_id: &str) -> PathBuf {
+ get_data_dir().join(format!("sync-status-{service_id}.json"))
}
@@
-Note: `sync-status.json` is NOT scoped by `service_id`
+Note: status is scoped by `service_id`; lock remains global (`sync_pipeline`) to prevent overlapping writes.
```
3. **Stop classifying permanence via string matching**
Analysis: Matching `"401 Unauthorized"` in strings is brittle and will misclassify edge cases. Carry machine codes through stage results and classify by `ErrorCode` only.
```diff
@@ pub struct StageResult {
- pub error: Option<String>,
+ pub error: Option<String>,
+ pub error_code: Option<String>, // e.g., AUTH_FAILED, NETWORK_ERROR
}
@@ Error classification helpers
-fn is_permanent_error_message(msg: Option<&str>) -> bool { ...string contains... }
+fn is_permanent_error_code(code: Option<&str>) -> bool {
+ matches!(code, Some("TOKEN_NOT_SET" | "AUTH_FAILED" | "CONFIG_NOT_FOUND" | "CONFIG_INVALID" | "MIGRATION_FAILED"))
+}
```
4. **Install should be transactional (manifest written last)**
Analysis: Current order writes manifest before scheduler enable. If enable fails, you persist a false “installed” state. Use two-phase install with rollback.
```diff
@@ ### `lore service install` User journey
-9. CLI writes install manifest ...
-10. CLI runs the platform-specific enable command
+9. CLI runs the platform-specific enable command
+10. On success, CLI writes install manifest atomically
+11. On failure, CLI removes generated files and returns `ServiceCommandFailed`
```
5. **Fix launchd token security gap (env-file currently still embeds token)**
Analysis: Current “env-file” on macOS still writes token into plist, defeating the main security goal. Generate a private wrapper script that reads env file at runtime and execs `lore`.
```diff
@@ ### macOS: launchd
-<key>ProgramArguments</key>
-<array>
- <string>{binary_path}</string>
- <string>--robot</string>
- <string>service</string>
- <string>run</string>
-</array>
+<key>ProgramArguments</key>
+<array>
+ <string>{data_dir}/service-run-{service_id}.sh</string>
+</array>
@@
-`env-file`: ... token value must still appear in plist ...
+`env-file`: token never appears in plist; wrapper loads `{data_dir}/service-env-{service_id}` at runtime.
```
6. **Improve backoff math and add half-open circuit recovery**
Analysis: Current jitter + min clamp makes first retry deterministic and can over-pause. Also circuit-breaker requires manual resume forever. Add cooldown + half-open probe to self-heal.
```diff
@@ Backoff Logic
-let backoff_secs = ((base_backoff as f64) * jitter_factor) as u64;
-let backoff_secs = backoff_secs.max(base_interval_seconds);
+let max_backoff = base_backoff;
+let min_backoff = base_interval_seconds;
+let span = max_backoff.saturating_sub(min_backoff);
+let backoff_secs = min_backoff + ((span as f64) * jitter_factor) as u64;
@@ Scheduler states
-- `paused` — permanent error ... OR circuit breaker tripped ...
+- `paused` — permanent error requiring intervention
+- `half_open` — probe state after circuit cooldown; one trial run allowed
@@ Circuit breaker
-... transitions to `paused` ... Run: lore service resume
+... transitions to `half_open` after cooldown (default 30m). Successful probe closes breaker automatically; failed probe returns to backoff/paused.
```
7. **Promote backend trait to v1 (not v2) for deterministic integration tests**
Analysis: This is a reliability-critical feature spanning OS schedulers. A trait abstraction now gives true behavior tests and safer refactors.
```diff
@@ ### Platform Backends
-> Future architecture note: A `SchedulerBackend` trait ... for v2.
+Adopt `SchedulerBackend` trait in v1 with real backends (`launchd/systemd/schtasks`) and `FakeBackend` for tests.
+This enables deterministic install/uninstall/status/run-path integration tests without touching host scheduler.
```
8. **Harden `run_cmd` timeout behavior**
Analysis: If timeout occurs, child process must be killed and reaped. Otherwise you leak processes and can wedge repeated runs.
```diff
@@ fn run_cmd(...)
-// Wait with timeout
-let output = wait_with_timeout(output, timeout_secs)?;
+// Wait with timeout; on timeout kill child and wait to reap
+let output = wait_with_timeout_kill_and_reap(child, timeout_secs)?;
```
9. **Add manual control commands (`pause`, `trigger`, `repair`)**
Analysis: These are high-utility operational controls. `trigger` helps immediate sync without waiting interval. `pause` supports maintenance windows. `repair` avoids manual file deletion for corrupt state.
```diff
@@ pub enum ServiceCommand {
+ /// Pause scheduled execution without uninstalling
+ Pause { #[arg(long)] reason: Option<String> },
+ /// Trigger an immediate one-off run using installed profile
+ Trigger { #[arg(long)] ignore_backoff: bool },
+ /// Repair corrupt manifest/status by backing up and reinitializing
+ Repair { #[arg(long)] service: Option<String> },
}
```
10. **Make `logs` default non-interactive and add rotation policy**
Analysis: Opening editor by default is awkward for automation/SSH and slower for normal diagnosis. Defaulting to `tail` is more practical; `--open` can preserve editor behavior.
```diff
@@ ### `lore service logs`
-By default, opens in the user's preferred editor.
+By default, prints last 100 lines to stdout.
+Use `--open` to open editor.
@@
+Log rotation: rotate `service-stdout.log` / `service-stderr.log` at 10 MB, keep 5 files.
```
11. **Remove destructive/shell-unsafe suggested action**
Analysis: `actions(): ["rm {path}", ...]` is unsafe (shell injection + destructive guidance). Replace with safe command path.
```diff
@@ LoreError::actions()
-Self::ServiceCorruptState { path, .. } => vec![&format!("rm {path}"), "lore service install"],
+Self::ServiceCorruptState { .. } => vec!["lore service repair", "lore service install"],
```
12. **Tighten scheduler units for real-world reliability**
Analysis: Add explicit working directory and success-exit handling to reduce environment drift and edge failures.
```diff
@@ systemd service unit
[Service]
Type=oneshot
ExecStart={binary_path} --robot service run
+WorkingDirectory={data_dir}
+SuccessExitStatus=0
TimeoutStartSec=900
```
If you want, I can produce a single consolidated “v3 plan” markdown with these revisions already merged into your original structure.

190
plans/lore-service.feedback-4.md
View File

@@ -1,190 +0,0 @@
No `## Rejected Recommendations` section was present in the plan you shared, so the proposals below are all net-new.
1. **Make scheduled runs explicitly target a single service instance**
Analysis: right now `service run` has no selector, but the plan supports multiple installed services. That creates ambiguity and incorrect manifest/status selection. This is the most important architectural fix.
```diff
@@ `lore service install` What it does
- runs `lore --robot service run` at the specified interval
+ runs `lore --robot service run --service-id <service_id>` at the specified interval
@@ Robot output (`install`)
- "sync_command": "/usr/local/bin/lore --robot service run",
+ "sync_command": "/usr/local/bin/lore --robot service run --service-id a1b2c3d4",
@@ `ServiceCommand` enum
- #[command(hide = true)]
- Run,
+ #[command(hide = true)]
+ Run {
+ /// Internal selector injected by scheduler backend
+ #[arg(long, hide = true)]
+ service_id: String,
+ },
@@ `handle_service_run` signature
-pub fn handle_service_run(start: std::time::Instant) -> Result<(), Box<dyn std::error::Error>>
+pub fn handle_service_run(service_id: &str, start: std::time::Instant) -> Result<(), Box<dyn std::error::Error>>
@@ run flow step 1
- Read install manifest
+ Read install manifest for `service_id`
```
2. **Strengthen `service_id` derivation to avoid cross-workspace collisions**
Analysis: hashing config path alone can collide when many workspaces share one global config. Identity should represent what is being synced, not only where config lives.
```diff
@@ Key Design Principles / Project-Scoped Service Identity
- derive from a stable hash of the config file path
+ derive from a stable fingerprint of:
+ - canonical workspace root
+ - normalized configured GitLab project URLs
+ - canonical config path
+ then take first 12 hex chars of SHA-256
@@ `compute_service_id`
- Returns first 8 hex chars of SHA-256 of the canonical config path.
+ Returns first 12 hex chars of SHA-256 of a canonical identity tuple
+ (workspace_root + sorted project URLs + config_path).
```
3. **Introduce a service-state machine with a dedicated admin lock**
Analysis: install/uninstall/pause/resume/repair/status can race each other. A lock and explicit transition table prevents invalid states and file races.
```diff
@@ New section: Service State Model
+ All state mutations are serialized by `AppLock("service-admin-{service_id}")`.
+ Legal transitions:
+ - idle -> running -> success|degraded|backoff|paused
+ - backoff -> running|paused
+ - paused -> half_open|running (resume)
+ - half_open -> running|paused
+ Any invalid transition is rejected with `ServiceCorruptState`.
@@ `handle_install`, `handle_uninstall`, `handle_pause`, `handle_resume`, `handle_repair`
+ Acquire `service-admin-{service_id}` before mutating manifest/status/service files.
```
4. **Unify manual and scheduled sync execution behind one orchestrator**
Analysis: the plan currently duplicates stage logic and error classification in `service run`, increasing drift risk. A shared orchestrator gives one authoritative pipeline behavior.
```diff
@@ Key Design Principles
+ #### 6. Single Sync Orchestrator
+ Both `lore sync` and `lore service run` call `SyncOrchestrator`.
+ Service mode adds policy (backoff/circuit-breaker); manual mode bypasses policy.
@@ Service Run Implementation
- execute_sync_stages(&sync_args)
+ SyncOrchestrator::run(SyncMode::Service { profile, policy })
@@ manual sync
- separate pipeline path
+ SyncOrchestrator::run(SyncMode::Manual { flags })
```
5. **Add bounded in-run retries for transient core-stage failures**
Analysis: single-shot failure handling will over-trigger backoff on temporary network blips. One short retry per core stage significantly improves freshness without much extra runtime.
```diff
@@ Stage-aware execution
+ Core stages (`issues`, `mrs`) get up to 1 immediate retry on transient errors
+ (jittered 1-5s). Permanent errors are never retried.
+ Optional stages keep best-effort semantics.
@@ Acceptance criteria (`service run`)
+ Retries transient core stage failures once before counting run as failed.
```
6. **Harden persistence with full crash-safety semantics**
Analysis: current atomic write description is good but incomplete for power-loss durability. You should fsync parent directory after rename and include lightweight integrity metadata.
```diff
@@ `write_atomic`
- tmp file + fsync + rename
+ tmp file + fsync(file) + rename + fsync(parent_dir)
@@ `ServiceManifest` and `SyncStatusFile`
+ pub write_seq: u64,
+ pub content_sha256: String, // optional integrity guard for repair/doctor
```
7. **Fix token handling to avoid shell/env injection and add secure-store mode**
Analysis: sourcing env files in shell is brittle if token contains special chars/newlines. Also, secure OS credential stores should be first-class for production reliability/security.
```diff
@@ Token storage strategies
-| `env-file` (default) ...
+| `auto` (default) | use secure-store when available, else env-file |
+| `secure-store` | macOS Keychain / libsecret / Windows Credential Manager |
+| `env-file` | explicit fallback |
@@ macOS wrapper script
-. "{data_dir}/service-env-{service_id}"
-export {token_env_var}
+TOKEN_VALUE="$(cat "{data_dir}/service-token-{service_id}" )"
+export {token_env_var}="$TOKEN_VALUE"
@@ Acceptance criteria
+ Reject token values containing `\0` or newline for env-file mode.
+ Never eval/source untrusted token content.
```
8. **Correct platform/runtime implementation hazards**
Analysis: there are a few correctness risks that should be fixed in-plan now.
```diff
@@ macOS install steps
- Get UID via `unsafe { libc::getuid() }`
+ Get UID via safe API (`nix::unistd::Uid::current()` or equivalent safe helper)
@@ Command Runner Helper
- poll try_wait and read stdout/stderr after exit
+ avoid potential pipe backpressure deadlock:
+ use wait-with-timeout + concurrent stdout/stderr draining
@@ Linux timer
- OnUnitActiveSec={interval_seconds}s
+ OnUnitInactiveSec={interval_seconds}s
+ AccuracySec=1min
```
9. **Make logs fully service-scoped**
Analysis: you already scoped manifest/status by `service_id`; logs are still global in several places. Multi-service installs will overwrite each other’s logs.
```diff
@@ Paths Module Additions
-pub fn get_service_log_path() -> PathBuf
+pub fn get_service_log_path(service_id: &str, stream: LogStream) -> PathBuf
@@ log filenames
- logs/service-stderr.log
- logs/service-stdout.log
+ logs/service-{service_id}-stderr.log
+ logs/service-{service_id}-stdout.log
@@ `service logs`
- default path: `{data_dir}/logs/service-stderr.log`
+ default path: `{data_dir}/logs/service-{service_id}-stderr.log`
```
10. **Resolve internal spec contradictions and rollback gaps**
Analysis: there are a few contradictory statements and incomplete rollback behavior that will cause implementation churn.
```diff
@@ `service logs` behavior
- default (no flags): open in editor (human)
+ default (no flags): print last 100 lines (human and robot metadata mode)
+ `--open` is explicit opt-in
@@ install rollback
- On failure: removes generated service files
+ On failure: removes generated service files, env file, wrapper script, and temp manifest
@@ `handle_service_run` sample code
- let manifest_path = get_service_manifest_path();
+ let manifest_path = get_service_manifest_path(service_id);
```
If you want, I can take these revisions and produce a single consolidated “Iteration 4” replacement plan block with all sections rewritten coherently so it’s ready to hand to an implementer.

114
plans/time-decay-expert-scoring.feedback-1.md
View File

@@ -1,114 +0,0 @@
Your plan is strong directionally, but I’d revise it in 8 key places to avoid regressions and make it significantly more useful in production.
1. **Split reviewer signals into “participated” vs “assigned-only”**
Reason: today’s inflation problem is often assignment noise. Treating `mr_reviewers` equal to real review activity still over-ranks passive reviewers.
```diff
@@ Per-signal contributions
-| Reviewer (reviewed MR touching path) | 10 | 90 days |
+| ReviewerParticipated (left DiffNote on MR/path) | 10 | 90 days |
+| ReviewerAssignedOnly (in mr_reviewers, no DiffNote by that user on MR/path) | 3 | 45 days |
```
```diff
@@ Scoring Formula
-score = reviewer_mr * reviewer_weight + ...
+score = reviewer_participated * reviewer_weight
+ + reviewer_assigned_only * reviewer_assignment_weight
+ + ...
```
2. **Cap/saturate note intensity per MR**
Reason: raw per-note addition can still reward “comment storms.” Use diminishing returns.
```diff
@@ Rust-Side Aggregation
-- Notes: Vec<i64> (timestamps) from diffnote_reviewer
+-- Notes grouped per (username, mr_id): note_count + max_ts
+-- Note contribution per MR uses diminishing returns:
+-- note_score_mr = note_bonus * ln(1 + note_count) * decay(now - ts, note_hl)
```
3. **Use better event timestamps than `m.updated_at` for file-change signals**
Reason: `updated_at` is noisy (title edits, metadata touches) and creates false recency.
```diff
@@ SQL Restructure
- signal 3/4 seen_at = m.updated_at
+ signal 3/4 activity_ts = COALESCE(m.merged_at, m.closed_at, m.created_at, m.updated_at)
```
4. **Don’t stream raw note rows to Rust; pre-aggregate in SQL**
Reason: current plan removes SQL grouping and can blow up memory/latency on large repos.
```diff
@@ SQL Restructure
-SELECT username, signal, mr_id, note_id, ts FROM signals
+WITH raw_signals AS (...),
+aggregated AS (
+ -- 1 row per (username, signal_class, mr_id) for MR-level signals
+ -- 1 row per (username, mr_id) for note_count + max_ts
+)
+SELECT username, signal_class, mr_id, qty, ts FROM aggregated
```
5. **Replace fixed `"24m"` with model-driven cutoff**
Reason: hardcoded 24m is arbitrary and tied to current weights/half-lives only.
```diff
@@ Default --since Change
-Expert mode: "6m" -> "24m"
+Expert mode default window derived from scoring.max_age_days (default 1095 days / 36m).
+Formula guidance: choose max_age where max possible single-event contribution < epsilon (e.g. 0.25 points).
+Add `--all-history` to disable cutoff for diagnostics.
```
6. **Validate scoring config explicitly**
Reason: silent bad configs (`half_life_days = 0`, negative weights) create undefined behavior.
```diff
@@ ScoringConfig (config.rs)
pub struct ScoringConfig {
pub author_weight: i64,
pub reviewer_weight: i64,
pub note_bonus: i64,
+ pub reviewer_assignment_weight: i64, // default: 3
pub author_half_life_days: u32,
pub reviewer_half_life_days: u32,
pub note_half_life_days: u32,
+ pub reviewer_assignment_half_life_days: u32, // default: 45
+ pub max_age_days: u32, // default: 1095
}
@@ Config::load_from_path
+validate_scoring(&config.scoring)?; // weights >= 0, half_life_days > 0, max_age_days >= 30
```
7. **Keep raw float score internally; round only for display**
Reason: rounding before sort causes avoidable ties/rank instability.
```diff
@@ Rust-Side Aggregation
-Round to i64 for Expert.score field
+Compute `raw_score: f64`, sort by raw_score DESC.
+Expose integer `score` for existing UX.
+Optionally expose `score_raw` and `score_components` in robot JSON when `--explain-score`.
```
8. **Add confidence + data-completeness metadata**
Reason: rankings are misleading if `mr_file_changes` coverage is poor.
```diff
@@ ExpertResult / Output
+confidence: "high" | "medium" | "low"
+coverage: { mrs_with_file_changes, total_mrs_in_window, percent }
+warning when coverage < threshold (e.g. 70%)
```
```diff
@@ Verification
4. cargo test
+5. ubs src/cli/commands/who.rs src/core/config.rs
+6. Benchmark query_expert on representative DB (latency + rows scanned before/after)
```
If you want, I can rewrite your full plan document into a clean “v2” version that already incorporates these diffs end-to-end.

132
plans/time-decay-expert-scoring.feedback-2.md
View File

@@ -1,132 +0,0 @@
The plan is strong, but I’d revise it in 10 places to improve correctness, scalability, and operator trust.
1. **Add rename/old-path awareness (correctness gap)**
Analysis: right now both existing code and your plan still center on `position_new_path` / `new_path` matches (`src/cli/commands/who.rs:643`, `src/cli/commands/who.rs:681`). That misses expertise on renamed/deleted paths and under-ranks long-time owners after refactors.
```diff
@@ ## Context
-This produces two compounding problems:
+This produces three compounding problems:
@@
2. **Reviewer inflation**: ...
+3. **Path-history blindness**: Renamed/moved files lose historical expertise because matching relies on current-path fields only.
@@ ### 3. SQL Restructure (who.rs)
-AND n.position_new_path {path_op}
+AND (n.position_new_path {path_op} OR n.position_old_path {path_op})
-AND fc.new_path {path_op}
+AND (fc.new_path {path_op} OR fc.old_path {path_op})
```
2. **Follow rename chains for queried paths**
Analysis: matching `old_path` helps, but true continuity needs alias expansion (A→B→C). Without this, expertise before multi-hop renames is fragmented.
```diff
@@ ### 3. SQL Restructure (who.rs)
+**Path alias expansion**: Before scoring, resolve a bounded rename alias set (default max depth: 20)
+from `mr_file_changes(change_type='renamed')`. Query signals against all aliases.
+Output includes `path_aliases_used` for transparency.
```
3. **Use hybrid SQL pre-aggregation instead of fully raw rows**
Analysis: the “raw row” design is simpler but will degrade on large repos with heavy DiffNote volume. Pre-aggregating to `(user, mr)` for MR signals and `(user, mr, note_count)` for note signals keeps memory/latency predictable.
```diff
@@ ### 3. SQL Restructure (who.rs)
-The SQL CTE ... removes the outer GROUP BY aggregation. Instead, it returns raw signal rows:
-SELECT username, signal, mr_id, note_id, ts FROM signals
+Use hybrid aggregation:
+- SQL returns MR-level rows for author/reviewer signals (1 row per user+MR+signal_class)
+- SQL returns note groups (1 row per user+MR with note_count, max_ts)
+- Rust applies decay + ln(1+count) + final ranking.
```
4. **Make timestamp policy state-aware (merged vs opened)**
Analysis: replacing `updated_at` with only `COALESCE(merged_at, created_at)` over-decays long-running open MRs. Open MRs need recency from active lifecycle; merged MRs should anchor to merge time.
```diff
@@ ### 3. SQL Restructure (who.rs)
-Replace m.updated_at with COALESCE(m.merged_at, m.created_at)
+Use state-aware timestamp:
+activity_ts =
+ CASE
+ WHEN m.state = 'merged' THEN COALESCE(m.merged_at, m.updated_at, m.created_at, m.last_seen_at)
+ WHEN m.state = 'opened' THEN COALESCE(m.updated_at, m.created_at, m.last_seen_at)
+ END
```
5. **Replace fixed `24m` with config-driven max age**
Analysis: `24m` is reasonable now, but brittle after tuning weights/half-lives. Tie cutoff to config so model behavior remains coherent as parameters evolve.
```diff
@@ ### 1. ScoringConfig (config.rs)
+pub max_age_days: u32, // default: 730 (or 1095)
@@ ### 5. Default --since Change
-Expert mode: "6m" -> "24m"
+Expert mode default window derives from `scoring.max_age_days`
+unless user passes `--since` or `--all-history`.
```
6. **Add reproducible scoring time via `--as-of`**
Analysis: decayed ranking is time-sensitive; debugging and tests become flaky without a fixed reference clock. This improves reliability and incident triage.
```diff
@@ ## Files to Modify
-2. src/cli/commands/who.rs
+2. src/cli/commands/who.rs
+3. src/cli/mod.rs
+4. src/main.rs
@@ ### 5. Default --since Change
+Add `--as-of <RFC3339|YYYY-MM-DD>` to score at a fixed timestamp.
+`resolved_input` includes `as_of_ms` and `as_of_iso`.
```
7. **Add explainability output (`--explain-score`)**
Analysis: decayed multi-signal ranking will be disputed without decomposition. Show components and top evidence MRs to make results actionable and debuggable.
```diff
@@ ## Rejected Ideas (with rationale)
-- **`--explain-score` flag with component breakdown**: ... deferred
+**Included in this iteration**: `--explain-score` adds per-user score components
+(`author`, `review_participated`, `review_assigned`, `notes`) plus top evidence MRs.
```
8. **Add confidence/coverage metadata**
Analysis: rankings can look precise while data is incomplete (`mr_file_changes` gaps, sparse DiffNotes). Confidence fields prevent false certainty.
```diff
@@ ### 4. Rust-Side Aggregation (who.rs)
+Compute and emit:
+- `coverage`: {mrs_considered, mrs_with_file_changes, mrs_with_diffnotes, percent}
+- `confidence`: high|medium|low (threshold-based)
```
9. **Add index migration for new query shapes**
Analysis: your new `EXISTS/NOT EXISTS` reviewer split and path dual-matching will need better indexes; current `who` indexes are not enough for author+path+time combinations.
```diff
@@ ## Files to Modify
+3. **`migrations/021_who_decay_indexes.sql`** — indexes for decay query patterns:
+ - notes(diffnote path + author + created_at + discussion_id) partial
+ - notes(old_path variant) partial
+ - mr_file_changes(project_id, new_path, merge_request_id)
+ - mr_file_changes(project_id, old_path, merge_request_id) partial
+ - merge_requests(state, merged_at, updated_at, created_at)
```
10. **Expand tests to invariants and determinism**
Analysis: example-based tests are good, but ranking systems need invariant tests to avoid subtle regressions.
```diff
@@ ### 7. New Tests (TDD)
+**`test_score_monotonicity_by_age`**: same signal, older timestamp never scores higher
+**`test_row_order_independence`**: shuffled SQL row order yields identical ranking
+**`test_as_of_reproducibility`**: same data + same `--as-of` => identical output
+**`test_rename_alias_chain_scoring`**: expertise carries across A->B->C rename chain
+**`test_overlap_participated_vs_assigned_counts`**: overlap reflects split reviewer semantics
```
If you want, I can produce a full consolidated `v2` plan doc patch (single unified diff against `plans/time-decay-expert-scoring.md`) rather than per-change snippets.

167
plans/time-decay-expert-scoring.feedback-3.md
View File

@@ -1,167 +0,0 @@
**Critical Plan Findings First**
1. The proposed index `idx_notes_mr_path_author ON notes(noteable_id, ...)` will fail: `notes.noteable_id` does not exist in schema (`migrations/002_issues.sql:74`).
2. Rename awareness is only applied in scoring queries, not in path resolution probes; today `build_path_query()` and `suffix_probe()` only inspect `position_new_path`/`new_path` (`src/cli/commands/who.rs:465`, `src/cli/commands/who.rs:591`), so old-path queries can still miss.
3. A fixed `"24m"` default window is brittle once half-lives become configurable; it can silently truncate meaningful history for larger half-lives.
Below are the revisions I’d make to your plan.
1. **Fix migration/index architecture (blocking correctness + perf)**
Rationale: prevents migration failure and aligns indexes to actual query shapes.
```diff
diff --git a/plan.md b/plan.md
@@ ### 6. Index Migration (db.rs)
- -- Support EXISTS subquery for reviewer participation check
- CREATE INDEX IF NOT EXISTS idx_notes_mr_path_author
- ON notes(noteable_id, position_new_path, author_username)
- WHERE note_type = 'DiffNote' AND is_system = 0;
+ -- Support reviewer participation joins (notes -> discussions -> MR)
+ CREATE INDEX IF NOT EXISTS idx_notes_diffnote_discussion_author_created
+ ON notes(discussion_id, author_username, created_at)
+ WHERE note_type = 'DiffNote' AND is_system = 0;
+
+ -- Path-first indexes for global and project-scoped path lookups
+ CREATE INDEX IF NOT EXISTS idx_mfc_new_path_project_mr
+ ON mr_file_changes(new_path, project_id, merge_request_id);
+ CREATE INDEX IF NOT EXISTS idx_mfc_old_path_project_mr
+ ON mr_file_changes(old_path, project_id, merge_request_id)
+ WHERE old_path IS NOT NULL;
@@
- -- Support state-aware timestamp selection
- CREATE INDEX IF NOT EXISTS idx_mr_state_timestamps
- ON merge_requests(state, merged_at, closed_at, updated_at, created_at);
+ -- Removed: low-selectivity timestamp composite index; joins are MR-id driven.
```
2. **Restructure SQL around `matched_mrs` CTE instead of repeating OR path clauses**
Rationale: better index use, less duplicated logic, cleaner maintenance.
```diff
diff --git a/plan.md b/plan.md
@@ ### 3. SQL Restructure (who.rs)
- WITH raw AS (
- -- 5 UNION ALL subqueries (signals 1, 2, 3, 4a, 4b)
- ),
+ WITH matched_notes AS (
+ -- DiffNotes matching new_path
+ ...
+ UNION ALL
+ -- DiffNotes matching old_path
+ ...
+ ),
+ matched_file_changes AS (
+ -- file changes matching new_path
+ ...
+ UNION ALL
+ -- file changes matching old_path
+ ...
+ ),
+ matched_mrs AS (
+ SELECT DISTINCT mr_id, project_id FROM matched_notes
+ UNION
+ SELECT DISTINCT mr_id, project_id FROM matched_file_changes
+ ),
+ raw AS (
+ -- signals sourced from matched_mrs + matched_notes
+ ),
```
3. **Replace correlated `EXISTS/NOT EXISTS` reviewer split with one precomputed participation set**
Rationale: same semantics, lower query cost, easier reasoning.
```diff
diff --git a/plan.md b/plan.md
@@ Signal 4 splits into two
- Signal 4a uses an EXISTS subquery ...
- Signal 4b uses NOT EXISTS ...
+ Build `reviewer_participation(mr_id, username)` once from matched DiffNotes.
+ Then classify `mr_reviewers` rows via LEFT JOIN:
+ - participated: `rp.username IS NOT NULL`
+ - assigned-only: `rp.username IS NULL`
+ This avoids correlated EXISTS scans per reviewer row.
```
4. **Make default `--since` derived from half-life + decay floor, not hardcoded 24m**
Rationale: remains mathematically consistent when config changes.
```diff
diff --git a/plan.md b/plan.md
@@ ### 1. ScoringConfig (config.rs)
+ pub decay_floor: f64, // default: 0.05
@@ ### 5. Default --since Change
- Expert mode: "6m" -> "24m"
+ Expert mode default window is computed:
+ default_since_days = ceil(max_half_life_days * log2(1.0 / decay_floor))
+ With defaults (max_half_life=180, floor=0.05), this is ~26 months.
+ CLI `--since` still overrides; `--all-history` still disables windowing.
```
5. **Use `log2(1+count)` for notes instead of `ln(1+count)`**
Rationale: keeps 1 note ~= 1 unit (with `note_bonus=1`) while preserving diminishing returns.
```diff
diff --git a/plan.md b/plan.md
@@ Scoring Formula
- note_contribution(mr) = note_bonus * ln(1 + note_count_in_mr) * 2^(-days_elapsed / note_half_life)
+ note_contribution(mr) = note_bonus * log2(1 + note_count_in_mr) * 2^(-days_elapsed / note_half_life)
```
6. **Guarantee deterministic float aggregation and expose `score_raw`**
Rationale: avoids hash-order drift and explainability mismatch vs rounded integer score.
```diff
diff --git a/plan.md b/plan.md
@@ ### 4. Rust-Side Aggregation (who.rs)
- HashMap<i64, ...>
+ BTreeMap<i64, ...> (or sort keys before accumulation) for deterministic summation order
+ Use compensated summation (Kahan/Neumaier) for stable f64 totals
@@
- Sort on raw `f64` score ... round only for display
+ Keep `score_raw` internally and expose when `--explain-score` is active.
+ `score` remains integer for backward compatibility.
```
7. **Extend rename awareness to query resolution (not only scoring)**
Rationale: fixes user-facing misses for old path input and suffix lookup.
```diff
diff --git a/plan.md b/plan.md
@@ Path rename awareness
- All signal subqueries match both old and new path columns
+ Also update `build_path_query()` probes and suffix probe:
+ - exact_exists: new_path OR old_path (notes + mr_file_changes)
+ - prefix_exists: new_path LIKE OR old_path LIKE
+ - suffix_probe: union of notes.position_new_path, notes.position_old_path,
+ mr_file_changes.new_path, mr_file_changes.old_path
```
8. **Tighten CLI/output contracts for new flags**
Rationale: avoids payload bloat/ambiguity and keeps robot clients stable.
```diff
diff --git a/plan.md b/plan.md
@@ ### 5b. Score Explainability via `--explain-score`
+ `--explain-score` conflicts with `--detail` (mutually exclusive)
+ `resolved_input` includes `as_of_ms`, `as_of_iso`, `scoring_model_version`
+ robot output includes `score_raw` and `components` only when explain is enabled
```
9. **Add confidence metadata (promote from rejected to accepted)**
Rationale: makes ranking more actionable and trustworthy with sparse evidence.
```diff
diff --git a/plan.md b/plan.md
@@ Rejected Ideas (with rationale)
- Confidence/coverage metadata: ... Deferred to avoid scope creep
+ Confidence/coverage metadata: ACCEPTED (minimal v1)
+ Add per-user `confidence: low|medium|high` based on evidence breadth + recency.
+ Keep implementation lightweight (no extra SQL pass).
```
10. **Upgrade test and verification scope to include query-plan and clock semantics**
Rationale: catches regressions your current tests won’t.
```diff
diff --git a/plan.md b/plan.md
@@ 8. New Tests (TDD)
+ test_old_path_probe_exact_and_prefix
+ test_suffix_probe_uses_old_path_sources
+ test_since_relative_to_as_of_clock
+ test_explain_and_detail_are_mutually_exclusive
+ test_null_timestamp_fallback_to_created_at
+ test_query_plan_uses_path_indexes (EXPLAIN QUERY PLAN)
@@ Verification
+ 7. EXPLAIN QUERY PLAN snapshots for expert query (exact + prefix) confirm index usage
```
If you want, I can produce a single consolidated “revision 3” plan document that fully merges all of the above into your original structure.

133
plans/time-decay-expert-scoring.feedback-4.md
View File

@@ -1,133 +0,0 @@
Your plan is already strong. The biggest remaining gaps are temporal correctness, indexability at scale, and ranking reliability under sparse/noisy evidence. These are the revisions I’d make.
1. **Fix temporal correctness for `--as-of` (critical)**
Analysis: Right now the plan describes `--as-of`, but the SQL only enforces lower bounds (`>= since`). If `as_of` is in the past, “future” events can still enter and get full weight (because elapsed is clamped). This breaks reproducibility.
```diff
@@ 3. SQL Restructure
- AND n.created_at >= ?2
+ AND n.created_at BETWEEN ?2 AND ?4
@@ Signal 3/4a/4b
- AND {state_aware_ts} >= ?2
+ AND {state_aware_ts} BETWEEN ?2 AND ?4
@@ 5a. Reproducible Scoring via --as-of
- All decay computations use as_of_ms instead of SystemTime::now()
+ All event selection and decay computations are bounded by as_of_ms.
+ Query window is [since_ms, as_of_ms], never [since_ms, now_ms].
+ Add test: test_as_of_excludes_future_events.
```
2. **Resolve `closed`-state inconsistency**
Analysis: The CASE handles `closed`, but all signal queries filter to `('opened','merged')`, making the `closed_at` branch dead code. Either include closed MRs intentionally or remove that logic. I’d include closed with a reduced multiplier.
```diff
@@ ScoringConfig (config.rs)
+ pub closed_mr_multiplier: f64, // default: 0.5
@@ 3. SQL Restructure
- AND m.state IN ('opened','merged')
+ AND m.state IN ('opened','merged','closed')
@@ 4. Rust-Side Aggregation
+ if state == "closed" { contribution *= closed_mr_multiplier; }
```
3. **Replace `OR` path predicates with index-friendly `UNION ALL` branches**
Analysis: `(new_path ... OR old_path ...)` often degrades index usage in SQLite. Split into two indexed branches and dedupe once. This improves planner stability and latency on large datasets.
```diff
@@ 3. SQL Restructure
-WITH matched_notes AS (
- ... AND (n.position_new_path {path_op} OR n.position_old_path {path_op})
-),
+WITH matched_notes AS (
+ SELECT ... FROM notes n WHERE ... AND n.position_new_path {path_op}
+ UNION ALL
+ SELECT ... FROM notes n WHERE ... AND n.position_old_path {path_op}
+),
+matched_notes_dedup AS (
+ SELECT DISTINCT id, discussion_id, author_username, created_at, project_id
+ FROM matched_notes
+),
@@
- JOIN matched_notes mn ...
+ JOIN matched_notes_dedup mn ...
```
4. **Add canonical path identity (rename-chain support)**
Analysis: Direct `old_path/new_path` matching only handles one-hop rename scenarios. A small alias graph/table built at ingest time gives robust expertise continuity across A→B→C chains and avoids repeated SQL complexity.
```diff
@@ Files to Modify
- 3. src/core/db.rs — Add migration for indexes...
+ 3. src/core/db.rs — Add migration for indexes + path_identity table
+ 4. src/core/ingest/*.rs — populate path_identity on rename events
+ 5. src/cli/commands/who.rs — resolve query path to canonical path_id first
@@ Context
- The fix has three parts:
+ The fix has four parts:
+ - Introduce canonical path identity so multi-hop renames preserve expertise
```
5. **Split scoring engine into a versioned core module**
Analysis: `who.rs` is becoming a mixed CLI/query/math/output surface. Move scoring math and event normalization into `src/core/scoring/` with explicit model versions. This reduces regression risk and enables future model experiments.
```diff
@@ Files to Modify
+ 4. src/core/scoring/mod.rs — model interface + shared types
+ 5. src/core/scoring/model_v2_decay.rs — current implementation
+ 6. src/cli/commands/who.rs — orchestration only
@@ 5b. Score Explainability
+ resolved_input includes scoring_model_version and scoring_model_name
```
6. **Add evidence confidence to reduce sparse-data rank spikes**
Analysis: One recent MR can outrank broader, steadier expertise. Add a confidence factor derived from number of distinct evidence MRs and expose both `score_raw` and `score_adjusted`.
```diff
@@ Scoring Formula
+ confidence(user) = 1 - exp(-evidence_mr_count / 6.0)
+ score_adjusted = score_raw * confidence
@@ 4. Rust-Side Aggregation
+ compute evidence_mr_count from unique MR ids across all signals
+ sort by score_adjusted DESC, then score_raw DESC, then last_seen DESC
@@ 5b. --explain-score
+ include confidence and evidence_mr_count
```
7. **Add first-class bot/service-account filtering**
Analysis: Reviewer inflation is not just assignment; bots and automation users can still pollute rankings. Make exclusion explicit and configurable.
```diff
@@ ScoringConfig (config.rs)
+ pub excluded_username_patterns: Vec<String>, // defaults include "*bot*", "renovate", "dependabot"
@@ 3. SQL Restructure
+ AND username NOT MATCHES excluded patterns (applied in Rust post-query or SQL where feasible)
@@ CLI
+ --include-bots (override exclusions)
```
8. **Tighten reviewer “participated” with substantive-note threshold**
Analysis: A single “LGTM” note shouldn’t classify someone as engaged reviewer equivalent to real inline review. Use a minimum substantive threshold.
```diff
@@ ScoringConfig (config.rs)
+ pub reviewer_min_note_chars: u32, // default: 20
@@ reviewer_participation CTE
- SELECT DISTINCT ... FROM matched_notes
+ SELECT DISTINCT ... FROM matched_notes
+ WHERE LENGTH(TRIM(body)) >= ?reviewer_min_note_chars
```
9. **Add rollout safety: model compare mode + rank-delta diagnostics**
Analysis: This is a scoring-model migration. You need safe rollout mechanics, not just tests. Add a compare mode so you can inspect rank deltas before forcing v2.
```diff
@@ CLI (who)
+ --scoring-model v1|v2|compare (default: v2)
+ --max-rank-delta-report N (compare mode diagnostics)
@@ Robot output
+ include v1_score, v2_score, rank_delta when --scoring-model compare
```
If you want, I can produce a single consolidated “plan v4” document that applies all nine diffs cleanly into your original markdown.

209
plans/tui-prd-v2-frankentui.feedback-1.md
View File

@@ -1,209 +0,0 @@
No `## Rejected Recommendations` section was present, so these are all net-new improvements.
1. Keep core `lore` stable; isolate nightly to a TUI crate
Rationale: the current plan says “whole project nightly” but later assumes TUI is feature-gated. Isolating nightly removes unnecessary risk from non-TUI users, CI, and release cadence.
```diff
@@ 3.2 Nightly Rust Strategy
-- The entire gitlore project moves to pinned nightly, not just the TUI feature.
+- Keep core `lore` on stable Rust.
+- Add workspace member `lore-tui` pinned to nightly for FrankenTUI.
+- Ship `lore tui` only when `--features tui` (or separate `lore-tui` binary) is enabled.
@@ 10.1 New Files
+- crates/lore-tui/Cargo.toml
+- crates/lore-tui/src/main.rs
@@ 11. Assumptions
-17. TUI module is feature-gated.
+17. TUI is isolated in a workspace crate and feature-gated in root CLI integration.
```
2. Add a framework adapter boundary from day 1
Rationale: the “3-day ratatui escape hatch” is optimistic without a strict interface. A tiny `UiRuntime` + screen renderer trait makes fallback real, not aspirational.
```diff
@@ 4. Architecture
+### 4.9 UI Runtime Abstraction
+Introduce `UiRuntime` trait (`run`, `send`, `subscribe`) and `ScreenRenderer` trait.
+FrankenTUI implementation is default; ratatui adapter can be dropped in with no state/action rewrite.
@@ 3.5 Escape Hatch
-- The migration cost to ratatui is ~3 days
+- Migration cost target is ~3-5 days, validated by one ratatui spike screen in Phase 1.
```
3. Stop using CLI command modules as the TUI query API
Rationale: coupling TUI to CLI output-era structs creates long-term friction and accidental regressions. Create a shared domain query layer used by both CLI and TUI.
```diff
@@ 10.20 Refactor: Extract Query Functions
-- extract query_* from cli/commands/*
+- introduce `src/domain/query/*` as the canonical read model API.
+- CLI and TUI both depend on domain query layer.
+- CLI modules retain formatting/output only.
@@ 10.2 Modified Files
+- src/domain/query/mod.rs
+- src/domain/query/issues.rs
+- src/domain/query/mrs.rs
+- src/domain/query/search.rs
+- src/domain/query/who.rs
```
4. Replace single `Arc<Mutex<Connection>>` with connection manager
Rationale: one locked connection serializes everything and hurts responsiveness, especially during sync. Use separate read pool + writer connection with WAL and busy timeout.
```diff
@@ 4.4 App — Implementing the Model Trait
- pub db: Arc<Mutex<Connection>>,
+ pub db: Arc<DbManager>, // read pool + single writer coordination
@@ 4.5 Async Action System
- Each Cmd::task closure locks the mutex, runs the query, and returns a Msg
+ Reads use pooled read-only connections.
+ Sync/write path uses dedicated writer connection.
+ Enforce WAL, busy_timeout, and retry policy for SQLITE_BUSY.
```
5. Make debouncing/cancellation explicit and correct
Rationale: “runtime coalesces rapid keypresses” is not a safe correctness guarantee. Add request IDs and stale-response dropping to prevent flicker and wrong data.
```diff
@@ 4.3 Core Types (Msg)
+ SearchRequestStarted { request_id: u64, query: String }
- SearchExecuted(SearchResults),
+ SearchExecuted { request_id: u64, results: SearchResults },
@@ 4.4 maybe_debounced_query()
- runtime coalesces rapid keypresses
+ use explicit 200ms debounce timer + monotonic request_id
+ ignore results whose request_id != current_search_request_id
```
6. Implement true streaming sync, not batch-at-end pseudo-streaming
Rationale: the plan promises real-time logs/progress but code currently returns one completion message. This gap will disappoint users and complicate cancellation.
```diff
@@ 4.4 start_sync_task()
- Pragmatic approach: run sync synchronously, collect all progress events, return summary.
+ Use event channel subscription for `SyncProgress`/`SyncLogLine` streaming.
+ Keep `SyncCompleted` only as terminal event.
+ Add cooperative cancel token mapped to `Esc` while running.
@@ 5.9 Sync
+ Add "Resume from checkpoint" option for interrupted syncs.
```
7. Fix entity identity ambiguity across projects
Rationale: using `iid` alone is unsafe in multi-project datasets. Navigation and cross-refs should key by `(project_id, iid)` or global ID.
```diff
@@ 4.3 Core Types
- IssueDetail(i64)
- MrDetail(i64)
+ IssueDetail(EntityKey)
+ MrDetail(EntityKey)
+ pub struct EntityKey { pub project_id: i64, pub iid: i64, pub kind: EntityKind }
@@ 10.12.4 Cross-Reference Widget
- parse "group/project#123" -> iid only
+ parse into `{project_path, iid, kind}` then resolve to `project_id` before navigation
```
8. Resolve keybinding conflicts and formalize keymap precedence
Rationale: current spec conflicts (`Tab` sort vs focus filter; `gg` vs go-prefix). A deterministic keymap contract prevents UX bugs.
```diff
@@ 8.2 List Screens
- Tab | Cycle sort column
- f | Focus filter bar
+ Tab | Focus filter bar
+ S | Cycle sort column
+ / | Focus filter bar (alias)
@@ 4.4 interpret_key()
+ Add explicit precedence table:
+ 1) modal/palette
+ 2) focused input
+ 3) global
+ 4) screen-local
+ Add configurable go-prefix timeout (default 500ms) with cancel feedback.
```
9. Add performance SLOs and DB/index plan
Rationale: “fast enough” is vague. Add measurable budgets, required indexes, and query-plan gates in CI for predictable performance.
```diff
@@ 3.1 Risk Matrix
+ Add risk: "Query latency regressions on large datasets"
@@ 9.3 Phase 0 — Toolchain Gate
+7. p95 list query latency < 75ms on 100k issues synthetic fixture
+8. p95 search latency < 200ms on 1M docs (lexical mode)
@@ 11. Assumptions
-5. SQLite queries are fast enough for interactive use (<50ms for filtered results).
+5. Performance budgets are enforced by benchmark fixtures and query-plan checks.
+6. Required indexes documented and migration-backed before TUI GA.
```
10. Add reliability/observability model (error classes, retries, tracing)
Rationale: one string toast is not enough for production debugging. Add typed errors, retry policy, and an in-TUI diagnostics pane.
```diff
@@ 4.3 Core Types (Msg)
- Error(String),
+ Error(AppError),
+ pub enum AppError {
+ DbBusy, DbCorruption, NetworkRateLimited, NetworkUnavailable,
+ AuthFailed, ParseError, Internal(String)
+ }
@@ 5.11 Doctor / Stats
+ Add "Diagnostics" tab:
+ - last 100 errors
+ - retry counts
+ - current sync/backoff state
+ - DB contention metrics
```
11. Add “Saved Views + Watchlist” as high-value product features
Rationale: this makes the TUI compelling daily, not just navigable. Users can persist filters and monitor critical slices (e.g., “P1 auth issues updated in last 24h”).
```diff
@@ 1. Executive Summary
+ - Saved Views (named filters and layouts)
+ - Watchlist panel (tracked queries with delta badges)
@@ 5. Screen Taxonomy
+### 5.12 Saved Views / Watchlist
+Persistent named filters for Issues/MRs/Search.
+Dashboard shows per-watchlist deltas since last session.
@@ 6. User Flows
+### 6.9 Flow: "Run morning watchlist triage"
+Dashboard -> Watchlist -> filtered IssueList/MRList -> detail drilldown
```
12. Strengthen testing plan with deterministic behavior and chaos cases
Rationale: snapshot tests alone won’t catch race/staleness/cancellation issues. Add concurrency, cancellation, and flaky terminal behavior tests.
```diff
@@ 9.2 Phases
+Phase 5.5 Reliability Test Pack (2d)
+ - stale response drop tests
+ - sync cancel/resume tests
+ - SQLITE_BUSY retry tests
+ - resize storm and rapid key-chord tests
@@ 10.9 Snapshot Test Example
+ Add non-snapshot tests:
+ - property tests for navigation invariants
+ - integration tests for request ordering correctness
+ - benchmark tests for query budgets
```
If you want, I can produce a consolidated “PRD v2.1 patch” with all of the above merged into one coherent updated document structure.

203
plans/tui-prd-v2-frankentui.feedback-2.md
View File

@@ -1,203 +0,0 @@
I excluded the two items in your `## Rejected Recommendations` and focused on net-new improvements.
These are the highest-impact revisions I’d make.
### 1. Fix the package graph now (avoid a hard Cargo cycle)
Your current plan has `root -> optional lore-tui` and `lore-tui -> lore (root)`, which creates a cyclic dependency risk. Split shared logic into a dedicated core crate so CLI and TUI both depend downward.
```diff
diff --git a/PRD.md b/PRD.md
@@ ## 9.1 Dependency Changes
-[workspace]
-members = [".", "crates/lore-tui"]
+[workspace]
+members = [".", "crates/lore-core", "crates/lore-tui"]
@@
-[dependencies]
-lore-tui = { path = "crates/lore-tui", optional = true }
+[dependencies]
+lore-core = { path = "crates/lore-core" }
+lore-tui = { path = "crates/lore-tui", optional = true }
@@ # crates/lore-tui/Cargo.toml
-lore = { path = "../.." } # Core lore library
+lore-core = { path = "../lore-core" } # Shared domain/query crate (acyclic graph)
```
### 2. Stop coupling TUI to `cli/commands/*` internals
Calling CLI command modules from TUI is brittle and will drift. Introduce a shared query/service layer with DTOs owned by core.
```diff
diff --git a/PRD.md b/PRD.md
@@ ## 4.1 Module Structure
- action.rs # Async action runners (DB queries, GitLab calls)
+ action.rs # Task dispatch only
+ service/
+ mod.rs
+ query.rs # Shared read services (CLI + TUI)
+ sync.rs # Shared sync orchestration facade
+ dto.rs # UI-agnostic data contracts
@@ ## 10.2 Modified Files
-src/cli/commands/list.rs # Extract query_issues(), query_mrs() as pub fns
-src/cli/commands/show.rs # Extract query_issue_detail(), query_mr_detail() as pub fns
-src/cli/commands/who.rs # Extract query_experts(), etc. as pub fns
-src/cli/commands/search.rs # Extract run_search_query() as pub fn
+crates/lore-core/src/query/issues.rs # Canonical issue queries
+crates/lore-core/src/query/mrs.rs # Canonical MR queries
+crates/lore-core/src/query/show.rs # Canonical detail queries
+crates/lore-core/src/query/who.rs # Canonical people queries
+crates/lore-core/src/query/search.rs # Canonical search queries
+src/cli/commands/*.rs # Consume lore-core query services
+crates/lore-tui/src/action.rs # Consume lore-core query services
```
### 3. Add a real task supervisor (dedupe + cancellation + priority)
Right now tasks are ad hoc and can overrun each other. Add a scheduler keyed by screen+intent.
```diff
diff --git a/PRD.md b/PRD.md
@@ ## 4.5 Async Action System
-The `Cmd::task(|| { ... })` pattern runs a blocking closure on a background thread pool.
+The TUI uses a `TaskSupervisor`:
+- Keyed tasks (`TaskKey`) to dedupe redundant requests
+- Priority lanes (`Input`, `Navigation`, `Background`)
+- Cooperative cancellation tokens per task
+- Late-result drop via generation IDs (not just search)
@@ ## 4.3 Core Types
+pub enum TaskKey {
+ LoadScreen(Screen),
+ Search { generation: u64 },
+ SyncStream,
+}
```
### 4. Correct sync streaming architecture (current sketch loses streamed events)
The sample creates `tx/rx` then drops `rx`; events never reach update loop. Define an explicit stream subscription with bounded queue and backpressure policy.
```diff
diff --git a/PRD.md b/PRD.md
@@ ## 4.4 App — Implementing the Model Trait
- let (tx, _rx) = std::sync::mpsc::channel::<Msg>();
+ let (tx, rx) = std::sync::mpsc::sync_channel::<Msg>(1024);
+ // rx is registered via Subscription::from_receiver("sync-stream", rx)
@@
- let result = crate::ingestion::orchestrator::run_sync(
+ let result = crate::ingestion::orchestrator::run_sync(
&config,
&conn,
|event| {
@@
- let _ = tx.send(Msg::SyncProgress(event.clone()));
- let _ = tx.send(Msg::SyncLogLine(format!("{event:?}")));
+ if tx.try_send(Msg::SyncProgress(event.clone())).is_err() {
+ let _ = tx.try_send(Msg::SyncBackpressureDrop);
+ }
+ let _ = tx.try_send(Msg::SyncLogLine(format!("{event:?}")));
},
);
```
### 5. Upgrade data-plane performance plan (keyset pagination + index contracts)
Virtualized list without keyset paging still forces expensive scans. Add explicit keyset pagination and query-plan CI checks.
```diff
diff --git a/PRD.md b/PRD.md
@@ ## 9.3 Phase 0 — Toolchain Gate
-7. p95 list query latency < 75ms on synthetic fixture (10k issues, 5k MRs)
+7. p95 list page fetch latency < 75ms using keyset pagination (10k issues, 5k MRs)
+8. EXPLAIN QUERY PLAN must show index usage for top 10 TUI queries
+9. No full table scan on issues/MRs/discussions under default filters
@@
-8. p95 search latency < 200ms on synthetic fixture (50k documents, lexical mode)
+10. p95 search latency < 200ms on synthetic fixture (50k documents, lexical mode)
+## 9.4 Required Indexes (GA blocker)
+- `issues(project_id, state, updated_at DESC, iid DESC)`
+- `merge_requests(project_id, state, updated_at DESC, iid DESC)`
+- `discussions(project_id, entity_type, entity_iid, created_at DESC)`
+- `notes(discussion_id, created_at ASC)`
```
### 6. Enforce `EntityKey` everywhere (remove bare IID paths)
You correctly identified multi-project IID collisions, but many message/state signatures still use `i64`. Make `EntityKey` mandatory in all navigation and detail loaders.
```diff
diff --git a/PRD.md b/PRD.md
@@ ## 4.3 Core Types
- IssueSelected(i64),
+ IssueSelected(EntityKey),
@@
- MrSelected(i64),
+ MrSelected(EntityKey),
@@
- IssueDetailLoaded(IssueDetail),
+ IssueDetailLoaded { key: EntityKey, detail: IssueDetail },
@@
- MrDetailLoaded(MrDetail),
+ MrDetailLoaded { key: EntityKey, detail: MrDetail },
@@ ## 10.10 State Module — Complete
- Cmd::msg(Msg::NavigateTo(Screen::IssueDetail(iid)))
+ Cmd::msg(Msg::NavigateTo(Screen::IssueDetail(entity_key)))
```
### 7. Harden filter/search semantics (strict parser + inline diagnostics + explain scores)
Current filter parser silently ignores unknown fields; that causes hidden mistakes. Add strict parse diagnostics and search score explainability.
```diff
diff --git a/PRD.md b/PRD.md
@@ ## 10.12.1 Filter Bar Widget
- _ => {} // Unknown fields silently ignored
+ _ => self.errors.push(format!("Unknown filter field: {}", token.field))
+ pub errors: Vec<String>, // inline parse/validation errors
+ pub warnings: Vec<String>, // non-fatal coercions
@@ ## 5.6 Search
-- **Live preview:** Selected result shows snippet + metadata in right pane
+- **Live preview:** Selected result shows snippet + metadata in right pane
+- **Explain score:** Optional breakdown (lexical, semantic, recency, boosts) for trust/debug
```
### 8. Add operational resilience: safe mode + panic report + startup fallback
TUI failures should degrade gracefully, not block usage.
```diff
diff --git a/PRD.md b/PRD.md
@@ ## 3.1 Risk Matrix
+| Runtime panic leaves user blocked | High | Medium | Panic hook writes crash report, restores terminal, offers fallback CLI command |
@@ ## 10.3 Entry Point
+pub fn launch_tui(config: Config, db_path: &Path) -> Result<(), LoreError> {
+ install_panic_hook_for_tui(); // terminal restore + crash dump path
+ ...
+}
@@ ## 8.1 Global (Available Everywhere)
+| `:` | Show fallback equivalent CLI command for current screen/action |
```
### 9. Add a “jump list” (forward/back navigation, not only stack pop)
Current model has only push/pop and reset. Add browser-like history for investigation workflows.
```diff
diff --git a/PRD.md b/PRD.md
@@ ## 4.7 Navigation Stack Implementation
pub struct NavigationStack {
- stack: Vec<Screen>,
+ back_stack: Vec<Screen>,
+ current: Screen,
+ forward_stack: Vec<Screen>,
+ jump_list: Vec<Screen>, // recent entity/detail hops
}
@@ ## 8.1 Global (Available Everywhere)
+| `Ctrl+o` | Jump backward in jump list |
+| `Ctrl+i` | Jump forward in jump list |
```
If you want, I can produce a single consolidated “PRD v2.1” patch that applies all nine revisions coherently section-by-section.

163
plans/tui-prd-v2-frankentui.feedback-3.md
View File

@@ -1,163 +0,0 @@
I excluded everything already listed in `## Rejected Recommendations`.
These are the highest-impact net-new revisions I’d make.
1. **Enforce Entity Identity Consistency End-to-End (P0)**
Analysis: The PRD defines `EntityKey`, but many code paths still pass bare `iid` (`IssueSelected(item.iid)`, timeline refs, search refs). In multi-project datasets this will cause wrong-entity navigation and subtle data corruption in cached state. Make `EntityKey` mandatory in every navigation message and add compile-time constructors.
```diff
@@ 4.3 Core Types
pub struct EntityKey {
pub project_id: i64,
pub iid: i64,
pub kind: EntityKind,
}
+impl EntityKey {
+ pub fn issue(project_id: i64, iid: i64) -> Self { Self { project_id, iid, kind: EntityKind::Issue } }
+ pub fn mr(project_id: i64, iid: i64) -> Self { Self { project_id, iid, kind: EntityKind::MergeRequest } }
+}
@@ 10.10 state/issue_list.rs
- .map(|item| Msg::IssueSelected(item.iid))
+ .map(|item| Msg::IssueSelected(EntityKey::issue(item.project_id, item.iid)))
@@ 10.10 state/mr_list.rs
- .map(|item| Msg::MrSelected(item.iid))
+ .map(|item| Msg::MrSelected(EntityKey::mr(item.project_id, item.iid)))
```
2. **Make TaskSupervisor Mandatory for All Background Work (P0)**
Analysis: The plan introduces `TaskSupervisor` but still dispatches many direct `Cmd::task` calls. That will reintroduce stale updates, duplicate queries, and priority inversion under rapid input. Centralize all background task creation through one spawn path that enforces dedupe, cancellation tokening, and generation checks.
```diff
@@ 4.5.1 Task Supervisor (Dedup + Cancellation + Priority)
-The supervisor is owned by `LoreApp` and consulted before dispatching any `Cmd::task`.
+The supervisor is owned by `LoreApp` and is the ONLY allowed path for background work.
+All task launches use `LoreApp::spawn_task(TaskKey, TaskPriority, closure)`.
@@ 4.4 App — Implementing the Model Trait
- Cmd::task(move || { ... })
+ self.spawn_task(TaskKey::LoadScreen(screen.clone()), TaskPriority::Navigation, move |token| { ... })
```
3. **Remove the Sync Streaming TODO and Make Real-Time Streaming a GA Gate (P0)**
Analysis: Current text admits sync progress is buffered with a TODO. That undercuts one of the main value props. Make streaming progress/log delivery non-optional, with bounded buffers and dropped-line accounting.
```diff
@@ 4.4 start_sync_task()
- // TODO: Register rx as subscription when FrankenTUI supports it.
- // For now, the task returns the final Msg and progress is buffered.
+ // Register rx as a live subscription (`Subscription::from_receiver` adapter).
+ // Progress and logs must render in real time (no batch-at-end fallback).
+ // Keep a bounded ring buffer (N=5000) and surface `dropped_log_lines` in UI.
@@ 9.3 Phase 0 — Toolchain Gate
+11. Real-time sync stream verified: progress updates visible during run, not only at completion.
```
4. **Upgrade List/Search Data Strategy to Windowed Keyset + Prefetch (P0)**
Analysis: “Virtualized list” alone does not solve query/transfer cost if full result sets are loaded. Move to fixed-size keyset windows with next-window prefetch and fast first paint; this keeps latency predictable on 100k+ records.
```diff
@@ 5.2 Issue List
- Pagination: Virtual scrolling for large result sets
+ Pagination: Windowed keyset pagination (window=200 rows) with background prefetch of next window.
+ First paint uses current window only; no full-result materialization.
@@ 5.4 MR List
+ Same windowed keyset pagination strategy as Issue List.
@@ 9.3 Success criteria
- 7. p95 list page fetch latency < 75ms using keyset pagination on synthetic fixture (10k issues, 5k MRs)
+ 7. p95 first-paint latency < 50ms and p95 next-window fetch < 75ms on synthetic fixture (100k issues, 50k MRs)
```
5. **Add Resumable Sync Checkpoints + Per-Project Fault Isolation (P1)**
Analysis: If sync is interrupted or one project fails, current design mostly falls back to cancel/fail. Add checkpoints so long runs can resume, and isolate failures to project/resource scope while continuing others.
```diff
@@ 3.1 Risk Matrix
+| Interrupted sync loses progress | High | Medium | Persist phase checkpoints and offer resume |
@@ 5.9 Sync
+Running mode: failed project/resource lanes are marked degraded while other lanes continue.
+Summary mode: offer `[R]esume interrupted sync` from last checkpoint.
@@ 11 Assumptions
-16. No new SQLite tables needed (but required indexes must be verified — see Performance SLOs).
+16. Add minimal internal tables for reliability: `sync_runs` and `sync_checkpoints` (append-only metadata).
```
6. **Add Capability-Adaptive Rendering Modes (P1)**
Analysis: Terminal compatibility is currently test-focused, but runtime adaptation is under-specified. Add explicit degradations for no-truecolor, no-unicode, slow SSH/tmux paths to reduce rendering artifacts and support incidents.
```diff
@@ 3.4 Terminal Compatibility Testing
+Add capability matrix validation: truecolor/256/16 color, unicode/ascii glyphs, alt-screen on/off.
@@ 10.19 CLI Integration
+Tui {
+ #[arg(long, default_value="auto")] render_mode: String, // auto|full|minimal
+ #[arg(long)] ascii: bool,
+ #[arg(long)] no_alt_screen: bool,
+}
```
7. **Harden Browser/Open and Log Privacy (P1)**
Analysis: `open_current_in_browser` currently trusts stored URLs; sync logs may expose tokens/emails from upstream messages. Add host allowlisting and redaction pipeline by default.
```diff
@@ 4.4 open_current_in_browser()
- if let Some(url) = url { ... open ... }
+ if let Some(url) = url {
+ if !self.state.security.is_allowed_gitlab_url(&url) {
+ self.state.set_error("Blocked non-GitLab URL".into());
+ return;
+ }
+ ... open ...
+ }
@@ 5.9 Sync
+Log stream passes through redaction (tokens, auth headers, email local-parts) before render/storage.
```
8. **Add “My Workbench” Screen for Daily Pull (P1, new feature)**
Analysis: The PRD is strong on exploration, weaker on “what should I do now?”. Add a focused operator screen aggregating assigned issues, requested reviews, unresolved threads mentioning me, and stale approvals. This makes the TUI habit-forming.
```diff
@@ 5. Screen Taxonomy
+### 5.12 My Workbench
+Single-screen triage cockpit:
+- Assigned-to-me open issues/MRs
+- Review requests awaiting action
+- Threads mentioning me and unresolved
+- Recently stale approvals / blocked MRs
@@ 8.1 Global
+| `gb` | Go to My Workbench |
@@ 9.2 Phases
+section Phase 3.5 — Daily Workflow
+My Workbench screen + queries :p35a, after p3d, 2d
```
9. **Add Rollout, SLO Telemetry, and Kill-Switch Plan (P0)**
Analysis: You have implementation phases but no production rollout control. Add explicit experiment flags, health telemetry, and rollback criteria so risk is operationally bounded.
```diff
@@ Table of Contents
-11. [Assumptions](#11-assumptions)
+11. [Assumptions](#11-assumptions)
+12. [Rollout & Telemetry](#12-rollout--telemetry)
@@ NEW SECTION 12
+## 12. Rollout & Telemetry
+- Feature flags: `tui_experimental`, `tui_sync_streaming`, `tui_workbench`
+- Metrics: startup_ms, frame_render_p95_ms, db_busy_rate, panic_free_sessions, sync_drop_events
+- Kill-switch: disable `tui` feature path at runtime if panic rate > 0.5% sessions over 24h
+- Canary rollout: internal only -> opt-in beta -> default-on
```
10. **Strengthen Reliability Pack with Event-Fuzz + Soak Tests (P0)**
Analysis: Current tests are good but still light on prolonged event pressure. Add deterministic fuzzed key/resize/paste streams and a long soak to catch rare deadlocks/leaks and state corruption.
```diff
@@ 9.2 Phase 5.5 — Reliability Test Pack
+Event fuzz tests (key/resize/paste interleavings) :p55g, after p55e, 1d
+30-minute soak test (no panic, bounded memory) :p55h, after p55g, 1d
@@ 9.3 Success criteria
+12. Event-fuzz suite passes with zero invariant violations across 10k randomized traces.
+13. 30-minute soak: no panic, no deadlock, memory growth < 5%.
```
If you want, I can produce a single consolidated unified diff of the full PRD text next (all edits merged, ready to apply as v3).

157
plans/tui-prd-v2-frankentui.feedback-4.md
View File

@@ -1,157 +0,0 @@
Below are my strongest revisions, focused on correctness, reliability, and long-term maintainability, while avoiding all items in your `## Rejected Recommendations`.
1. **Fix the Cargo/toolchain architecture (current plan has a real dependency-cycle risk and shaky per-member toolchain behavior).**
Analysis: The current plan has `lore -> lore-tui (optional)` and `lore-tui -> lore`, which creates a package cycle when `tui` is enabled. Also, per-member `rust-toolchain.toml` in a workspace is easy to misapply in CI/dev workflows. The cleanest robust shape is: `lore-tui` is a separate binary crate (nightly), `lore` remains stable and delegates at runtime (`lore tui` shells out to `lore-tui`).
```diff
--- a/Gitlore_TUI_PRD_v2.md
+++ b/Gitlore_TUI_PRD_v2.md
@@ 3.2 Nightly Rust Strategy
-- The `lore` binary integrates TUI via `lore tui` subcommand. The `lore-tui` crate is a library dependency feature-gated in the root.
+- `lore-tui` is a separate binary crate built on pinned nightly.
+- `lore` (stable) does not compile-link `lore-tui`; `lore tui` delegates by spawning `lore-tui`.
+- This removes Cargo dependency-cycle risk and keeps stable builds nightly-free.
@@ 9.1 Dependency Changes
-[features]
-tui = ["dep:lore-tui"]
-[dependencies]
-lore-tui = { path = "crates/lore-tui", optional = true }
+[dependencies]
+# no compile-time dependency on lore-tui from lore
+# runtime delegation keeps toolchains isolated
@@ 10.19 CLI Integration
-Add Tui match arm that directly calls crate::tui::launch_tui(...)
+Add Tui match arm that resolves and spawns `lore-tui` with passthrough args.
+If missing, print actionable install/build command.
```
2. **Make `TaskSupervisor` the *actual* single async path (remove contradictory direct `Cmd::task` usage in state handlers).**
Analysis: You declare “direct `Cmd::task` is prohibited outside supervisor,” but later `handle_screen_msg` still launches tasks directly. That contradiction will reintroduce stale-result bugs and race conditions. Make state handlers pure (intent-only); all async launch/cancel/dedup goes through one supervised API.
```diff
--- a/Gitlore_TUI_PRD_v2.md
+++ b/Gitlore_TUI_PRD_v2.md
@@ 4.5.1 Task Supervisor
-The supervisor is the ONLY allowed path for background work.
+The supervisor is the ONLY allowed path for background work, enforced by architecture:
+`AppState` emits intents only; `LoreApp::update` launches tasks via `spawn_task(...)`.
@@ 10.10 State Module — Complete
-pub fn handle_screen_msg(..., db: &Arc<Mutex<Connection>>) -> Cmd<Msg>
+pub fn handle_screen_msg(...) -> ScreenIntent
+// no DB access, no Cmd::task in state layer
```
3. **Enforce `EntityKey` everywhere (remove raw IID navigation paths).**
Analysis: Multi-project identity is one of your strongest ideas, but multiple snippets still navigate by bare IID (`document_id`, `EntityRef::Issue(i64)`). That can misroute across projects and create silent correctness bugs. Make all navigation-bearing results carry `EntityKey` end-to-end.
```diff
--- a/Gitlore_TUI_PRD_v2.md
+++ b/Gitlore_TUI_PRD_v2.md
@@ 4.3 Core Types
-pub enum EntityRef { Issue(i64), MergeRequest(i64) }
+pub enum EntityRef { Issue(EntityKey), MergeRequest(EntityKey) }
@@ 10.10 state/search.rs
-Some(Msg::NavigateTo(Screen::IssueDetail(r.document_id)))
+Some(Msg::NavigateTo(Screen::IssueDetail(r.entity_key.clone())))
@@ 10.11 action.rs
-pub fn fetch_issue_detail(conn: &Connection, iid: i64) -> Result<IssueDetail, LoreError>
+pub fn fetch_issue_detail(conn: &Connection, key: &EntityKey) -> Result<IssueDetail, LoreError>
```
4. **Introduce a shared query boundary inside the existing crate (not a new crate) to decouple TUI from CLI presentation structs.**
Analysis: Reusing CLI command modules directly is fast initially, but it ties TUI to output-layer types and command concerns. A minimal internal `core::query::*` module gives a stable data contract used by both CLI and TUI without the overhead of a new crate split.
```diff
--- a/Gitlore_TUI_PRD_v2.md
+++ b/Gitlore_TUI_PRD_v2.md
@@ 10.2 Modified Files
-src/cli/commands/list.rs # extract query_issues/query_mrs as pub
-src/cli/commands/show.rs # extract query_issue_detail/query_mr_detail as pub
+src/core/query/mod.rs
+src/core/query/issues.rs
+src/core/query/mrs.rs
+src/core/query/detail.rs
+src/core/query/search.rs
+src/core/query/who.rs
+src/cli/commands/* now call core::query::* + format output
+TUI action.rs calls core::query::* directly
```
5. **Add terminal-safety sanitization for untrusted text (ANSI/OSC injection hardening).**
Analysis: Issue/MR bodies, notes, and logs are untrusted text in a terminal context. Without sanitization, terminal escape/control sequences can spoof UI or trigger unintended behavior. Add explicit sanitization and a strict URL policy before rendering/opening.
```diff
--- a/Gitlore_TUI_PRD_v2.md
+++ b/Gitlore_TUI_PRD_v2.md
@@ 3.1 Risk Matrix
+| Terminal escape/control-sequence injection via issue/note text | High | Medium | Strip ANSI/OSC/control chars before render; escape markdown output; allowlist URL scheme+host |
@@ 4.1 Module Structure
+ safety.rs # sanitize_for_terminal(), safe_url_policy()
@@ 10.5/10.8/10.14/10.16
+All user-sourced text passes through `sanitize_for_terminal()` before widget rendering.
+Disable markdown raw HTML and clickable links unless URL policy passes.
```
6. **Move resumable sync checkpoints into v1 (lightweight version).**
Analysis: You already identify interruption risk as real. Deferring resumability to post-v1 leaves a major reliability gap in exactly the heaviest workflow. A lightweight checkpoint table (resource cursor + updated-at watermark) gives large reliability gain with modest complexity.
```diff
--- a/Gitlore_TUI_PRD_v2.md
+++ b/Gitlore_TUI_PRD_v2.md
@@ 3.1 Risk Matrix
-- Resumable checkpoints planned for post-v1
+Resumable checkpoints included in v1 (lightweight cursors per project/resource lane)
@@ 9.3 Success Criteria
+14. Interrupt-and-resume test: sync resumes from checkpoint and reaches completion without full restart.
@@ 9.3.1 Required Indexes (GA Blocker)
+CREATE TABLE IF NOT EXISTS sync_checkpoints (
+ project_id INTEGER NOT NULL,
+ lane TEXT NOT NULL,
+ cursor TEXT,
+ updated_at_ms INTEGER NOT NULL,
+ PRIMARY KEY (project_id, lane)
+);
```
7. **Strengthen performance gates with tiered fixtures and memory ceilings.**
Analysis: Current thresholds are good, but fixture sizes are too close to mid-scale only. Add S/M/L fixtures and memory budget checks so regressions appear before real-world datasets hit them. This gives much more confidence in long-term scalability.
```diff
--- a/Gitlore_TUI_PRD_v2.md
+++ b/Gitlore_TUI_PRD_v2.md
@@ 9.3 Phase 0 — Toolchain Gate
-7. p95 first-paint latency < 50ms ... (100k issues, 50k MRs)
-10. p95 search latency < 200ms ... (50k documents)
+7. Tiered fixtures:
+ S: 10k issues / 5k MRs / 50k notes
+ M: 100k issues / 50k MRs / 500k notes
+ L: 250k issues / 100k MRs / 1M notes
+ Enforce p95 targets per tier and memory ceiling (<250MB RSS in M tier).
+10. Search SLO validated in S and M tiers, lexical and hybrid modes.
```
8. **Add session restore (last screen + filters + selection), with explicit `--fresh` opt-out.**
Analysis: This is high-value daily UX with low complexity, and it makes the TUI feel materially more “compelling/useful” without feature bloat. It also reduces friction when recovering from crash/restart.
```diff
--- a/Gitlore_TUI_PRD_v2.md
+++ b/Gitlore_TUI_PRD_v2.md
@@ 1. Executive Summary
+- **Session restore** — resume last screen, filters, and selection on startup.
@@ 4.1 Module Structure
+ session.rs # persisted UI session state
@@ 8.1 Global
+| `Ctrl+R` | Reset session state for current screen |
@@ 10.19 CLI Integration
+`lore tui --fresh` starts without restoring prior session state.
@@ 11. Assumptions
-12. No TUI-specific configuration initially.
+12. Minimal TUI state file is allowed for session restore only.
```
9. **Add parity tests between TUI data panels and `--robot` outputs.**
Analysis: You already have `ShowCliEquivalent`; parity tests make that claim trustworthy and prevent drift between interfaces. This is a strong reliability multiplier and helps future refactors.
```diff
--- a/Gitlore_TUI_PRD_v2.md
+++ b/Gitlore_TUI_PRD_v2.md
@@ 9.2 Phases / 9.3 Success Criteria
+Phase 5.6 — CLI/TUI Parity Pack
+ - Dashboard count parity vs `lore --robot count/status`
+ - List/detail parity for issues/MRs on sampled entities
+ - Search result identity parity (top-N ids) for lexical mode
+Success criterion: parity suite passes on CI fixtures.
```
If you want, I can produce a single consolidated patch of the PRD text (one unified diff) so you can drop it directly into the next iteration.

200
plans/tui-prd-v2-frankentui.feedback-5.md
View File

@@ -1,200 +0,0 @@
1. **Fix the structural inconsistency between `src/tui` and `crates/lore-tui/src`**
Analysis: The PRD currently defines two different code layouts for the same system. That will cause implementation drift, wrong imports, and duplicated modules. Locking to one canonical layout early prevents execution churn and makes every snippet/action item unambiguous.
```diff
@@ 4.1 Module Structure @@
-src/
- tui/
+crates/lore-tui/src/
mod.rs
app.rs
message.rs
@@
-### 10.5 Dashboard View (FrankenTUI Native)
-// src/tui/view/dashboard.rs
+### 10.5 Dashboard View (FrankenTUI Native)
+// crates/lore-tui/src/view/dashboard.rs
@@
-### 10.6 Sync View
-// src/tui/view/sync.rs
+### 10.6 Sync View
+// crates/lore-tui/src/view/sync.rs
```
2. **Add a small `ui_adapter` seam to contain FrankenTUI API churn**
Analysis: You already identified high likelihood of upstream breakage. Pinning a commit helps, but if every screen imports raw `ftui_*` types directly, churn ripples through dozens of files. A thin adapter layer reduces upgrade cost without introducing the rejected “full portability abstraction”.
```diff
@@ 3.1 Risk Matrix @@
| API breaking changes | High | High (v0.x) | Pin exact git commit; vendor source if needed |
+| API breakage blast radius across app code | High | High | Constrain ftui usage behind `ui_adapter/*` wrappers |
@@ 4.1 Module Structure @@
+ ui_adapter/
+ mod.rs # Re-export stable local UI primitives
+ runtime.rs # App launch/options wrappers
+ widgets.rs # Table/List/Modal wrapper constructors
+ input.rs # Text input + focus helpers
@@ 9.3 Phase 0 — Toolchain Gate @@
+14. `ui_adapter` compile-check: no screen module imports `ftui_*` directly (lint-enforced)
```
3. **Correct search mode behavior and replace sleep-based debounce with cancelable scheduling**
Analysis: Current plan hardcodes `"hybrid"` in `execute_search`, so mode switching is UI-only and incorrect. Also, spawning sleeping tasks per keypress is wasteful under fast typing. Make mode a first-class query parameter and debounce via one cancelable scheduled event per input domain.
```diff
@@ 4.4 maybe_debounced_query @@
-std::thread::sleep(std::time::Duration::from_millis(200));
-match crate::tui::action::execute_search(&conn, &query, &filters) {
+// no thread sleep; schedule SearchRequestStarted after 200ms via debounce scheduler
+match crate::tui::action::execute_search(&conn, &query, &filters, mode) {
@@ 10.11 Action Module — Query Bridge @@
-pub fn execute_search(conn: &Connection, query: &str, filters: &SearchCliFilters) -> Result<SearchResponse, LoreError> {
- let mode_str = "hybrid"; // default; TUI mode selector overrides
+pub fn execute_search(
+ conn: &Connection,
+ query: &str,
+ filters: &SearchCliFilters,
+ mode: SearchMode,
+) -> Result<SearchResponse, LoreError> {
+ let mode_str = match mode {
+ SearchMode::Hybrid => "hybrid",
+ SearchMode::Lexical => "lexical",
+ SearchMode::Semantic => "semantic",
+ };
@@ 9.3 Phase 0 — Toolchain Gate @@
+15. Search mode parity: lexical/hybrid/semantic each return mode-consistent top-N IDs on fixture
```
4. **Guarantee consistent multi-query reads and add query interruption for responsiveness**
Analysis: Detail screens combine multiple queries that can observe mixed states during sync writes. Wrap each detail fetch in a single read transaction for snapshot consistency. Add cancellation/interrupt checks for long-running queries so UI remains responsive under heavy datasets.
```diff
@@ 4.5 Async Action System @@
+All detail fetches (`issue_detail`, `mr_detail`, timeline expansion) run inside one read transaction
+to guarantee snapshot consistency across subqueries.
@@ 10.11 Action Module — Query Bridge @@
+pub fn with_read_snapshot<T>(
+ conn: &Connection,
+ f: impl FnOnce(&rusqlite::Transaction<'_>) -> Result<T, LoreError>,
+) -> Result<T, LoreError> { ... }
+// Long queries register interrupt checks tied to CancelToken
+// to avoid >1s uninterruptible stalls during rapid navigation/filtering.
```
5. **Formalize sync event streaming contract to prevent “stuck” states**
Analysis: Dropping events on backpressure is acceptable, but completion must never be dropped and event ordering must be explicit. Add a typed `SyncUiEvent` stream with guaranteed terminal sentinel and progress coalescing to reduce load while preserving correctness.
```diff
@@ 4.4 start_sync_task @@
-let (tx, rx) = std::sync::mpsc::sync_channel::<Msg>(1024);
+let (tx, rx) = std::sync::mpsc::sync_channel::<SyncUiEvent>(2048);
-// drop this progress update rather than blocking the sync thread
+// coalesce progress to max 30Hz per lane; never drop terminal events
+// always emit SyncUiEvent::StreamClosed { outcome }
@@ 5.9 Sync @@
-- Log viewer with streaming output
+- Log viewer with streaming output and explicit stream-finalization state
+- UI shows dropped/coalesced event counters for transparency
```
6. **Version and validate session restore payloads**
Analysis: A raw JSON session file without schema/version checks is fragile across releases and DB switches. Add schema version, DB fingerprint, and safe fallback rules so session restore never blocks startup or applies stale state incorrectly.
```diff
@@ 11. Assumptions @@
-12. Minimal TUI state file allowed for session restore only ...
+12. Versioned TUI state file allowed for session restore only:
+ fields include `schema_version`, `app_version`, `db_fingerprint`, `saved_at`, `state`.
@@ 10.1 New Files @@
crates/lore-tui/src/session.rs # Lightweight session state persistence
+ # + versioning, validation, corruption quarantine
@@ 4.1 Module Structure @@
session.rs # Lightweight session state persistence
+ # corrupted file -> `.bad-<timestamp>` and fresh start
```
7. **Harden terminal safety beyond ANSI stripping**
Analysis: ANSI stripping is necessary but not sufficient. Bidi controls and invisible Unicode controls can still spoof displayed content. URL checks should normalize host/port and disallow deceptive variants. This closes realistic terminal spoofing vectors.
```diff
@@ 3.1 Risk Matrix @@
| Terminal escape/control-sequence injection via issue/note text | High | Medium | Strip ANSI/OSC/control chars via sanitize_for_terminal() ... |
+| Bidi/invisible Unicode spoofing in rendered text | High | Medium | Strip bidi overrides + zero-width controls in untrusted text |
@@ 10.4.1 Terminal Safety — Untrusted Text Sanitization @@
-Strip ANSI escape sequences, OSC commands, and control characters
+Strip ANSI/OSC/control chars, bidi overrides (RLO/LRO/PDF/RLI/LRI/FSI/PDI),
+and zero-width/invisible controls from untrusted text
-pub fn is_safe_url(url: &str, allowed_hosts: &[String]) -> bool {
+pub fn is_safe_url(url: &str, allowed_origins: &[Origin]) -> bool {
+ // normalize host (IDNA), enforce scheme+host+port match
```
8. **Use progressive hydration for detail screens**
Analysis: Issue/MR detail first-paint can become slow when discussions are large. Split fetch into phases: metadata first, then discussions/file changes, then deep thread content on expand. This improves perceived performance and keeps navigation snappy on large repos.
```diff
@@ 5.3 Issue Detail @@
-Data source: `lore issues <iid>` + discussions + cross-references
+Data source (progressive):
+1) metadata/header (first paint)
+2) discussions summary + cross-refs
+3) full thread bodies loaded on demand when expanded
@@ 5.5 MR Detail @@
-Unique features: File changes list, Diff discussions ...
+Unique features (progressive hydration):
+- file change summary in first paint
+- diff discussion bodies loaded lazily per expanded thread
@@ 9.3 Phase 0 — Toolchain Gate @@
+16. Detail first-paint p95 < 75ms on M-tier fixtures (metadata-only phase)
```
9. **Make reliability tests reproducible with deterministic clocks/seeds**
Analysis: Relative-time rendering and fuzz tests are currently tied to wall clock/randomness, which makes CI flakes hard to diagnose. Introduce a `Clock` abstraction and deterministic fuzz seeds with failure replay output.
```diff
@@ 10.9.1 Non-Snapshot Tests @@
+/// All time-based rendering uses injected `Clock` in tests.
+/// Fuzz failures print deterministic seed for replay.
@@ 9.2 Phase 5.5 — Reliability Test Pack @@
-Event fuzz tests (key/resize/paste):p55g
+Event fuzz tests (key/resize/paste, deterministic seed replay):p55g
+Deterministic clock/render tests:p55i
```
10. **Add an “Actionable Insights” dashboard panel for stronger day-to-day utility**
Analysis: Current dashboard is informative, but not prioritizing. Adding ranked insights (stale P1s, blocked MRs, discussion hotspots) turns it into a decision surface, not just a metrics screen. This makes the TUI materially more compelling for triage workflows.
```diff
@@ 1. Executive Summary @@
- Dashboard — sync status, project health, counts at a glance
+- Dashboard — sync status, project health, counts, and ranked actionable insights
@@ 5.1 Dashboard (Home Screen) @@
-│ Recent Activity │
+│ Recent Activity │
+│ Actionable Insights │
+│ 1) 7 opened P1 issues >14d │
+│ 2) 3 MRs blocked by unresolved │
+│ 3) auth/ has +42% note velocity │
@@ 6. User Flows @@
+### 6.9 Flow: "Risk-first morning sweep"
+Dashboard -> select insight -> jump to pre-filtered list/detail
```
These 10 changes stay clear of your `Rejected Recommendations` list and materially improve correctness, operability, and product value without adding speculative architecture.

150
plans/tui-prd-v2-frankentui.feedback-6.md
View File

@@ -1,150 +0,0 @@
Your plan is strong and unusually detailed. The biggest upgrades I’d make are around build isolation, async correctness, terminal correctness, and turning existing data into sharper triage workflows.
## 1) Fix toolchain isolation so stable builds cannot accidentally pull nightly
Rationale: a `rust-toolchain.toml` inside `crates/lore-tui` is not a complete guard when running workspace commands from repo root. You should structurally prevent stable workflows from touching nightly-only code.
```diff
@@ 3.2 Nightly Rust Strategy
-[workspace]
-members = [".", "crates/lore-tui"]
+[workspace]
+members = ["."]
+exclude = ["crates/lore-tui"]
+`crates/lore-tui` is built as an isolated workspace/package with explicit toolchain invocation:
+ cargo +nightly-2026-02-08 check --manifest-path crates/lore-tui/Cargo.toml
+Core repo remains:
+ cargo +stable check --workspace
```
## 2) Add an explicit `lore` <-> `lore-tui` compatibility contract
Rationale: runtime delegation is correct, but version drift between binaries will become the #1 support failure mode. Add a handshake before launch.
```diff
@@ 10.19 CLI Integration — Adding `lore tui`
+Before spawning `lore-tui`, `lore` runs:
+ lore-tui --print-contract-json
+and validates:
+ - minimum_core_version
+ - supported_db_schema_range
+ - contract_version
+On mismatch, print actionable remediation:
+ cargo install --path crates/lore-tui
```
## 3) Make TaskSupervisor truly authoritative (remove split async paths)
Rationale: the document says supervisor is the only path, but examples still use direct `Cmd::task` and `search_request_id`. Close that contradiction now to avoid stale-data races.
```diff
@@ 4.4 App — Implementing the Model Trait
- search_request_id: u64,
+ task_supervisor: TaskSupervisor,
@@ 4.5.1 Task Supervisor
-The `search_request_id` field in `LoreApp` is superseded...
+`search_request_id` is removed. All async work uses TaskSupervisor generations.
+No direct `Cmd::task` from screen handlers or ad-hoc helpers.
```
## 4) Resolve keybinding conflicts and implement real go-prefix timeout
Rationale: `Ctrl+I` collides with `Tab` in terminals. Also your 500ms go-prefix timeout is described but not enforced in code.
```diff
@@ 8.1 Global (Available Everywhere)
-| `Ctrl+I` | Jump forward in jump list (entity hops) |
+| `Alt+o` | Jump forward in jump list (entity hops) |
@@ 8.2 Keybinding precedence
+Go-prefix timeout is enforced by timestamped state + tick check.
+Backspace global-back behavior is implemented (currently documented but not wired).
```
## 5) Add a shared display-width text utility (Unicode-safe truncation and alignment)
Rationale: current `truncate()` implementations use byte/char length and will misalign CJK/emoji/full-width text in tables and trees.
```diff
@@ 10.1 New Files
+crates/lore-tui/src/text_width.rs # grapheme-safe truncation + display width helpers
@@ 10.5 Dashboard View / 10.13 Issue List / 10.16 Who View
-fn truncate(s: &str, max: usize) -> String { ... }
+use crate::text_width::truncate_display_width;
+// all column fitting/truncation uses terminal display width, not bytes/chars
```
## 6) Upgrade sync streaming to a QoS event bus with sequence IDs
Rationale: today progress/log events can be dropped under load with weak observability. Keep UI responsive while guaranteeing completion semantics and visible gap accounting.
```diff
@@ 4.4 start_sync_task()
-let (tx, rx) = std::sync::mpsc::sync_channel::<SyncUiEvent>(2048);
+let (ctrl_tx, ctrl_rx) = std::sync::mpsc::sync_channel::<SyncCtrlEvent>(256); // never-drop
+let (data_tx, data_rx) = std::sync::mpsc::sync_channel::<SyncDataEvent>(4096); // coalescible
+Every streamed event carries seq_no.
+UI detects gaps and renders: "Dropped N log/progress events due to backpressure."
+Terminal events (started/completed/failed/cancelled) remain lossless.
```
## 7) Make list pagination truly keyset-driven in state, not just in prose
Rationale: plan text promises windowed keyset paging, but state examples still keep a single list without cursor model. Encode pagination state explicitly.
```diff
@@ 10.10 state/issue_list.rs
-pub items: Vec<IssueListRow>,
+pub window: Vec<IssueListRow>,
+pub next_cursor: Option<IssueCursor>,
+pub prev_cursor: Option<IssueCursor>,
+pub prefetch: Option<Vec<IssueListRow>>,
+pub window_size: usize, // default 200
@@ 5.2 Issue List
-Pagination: Windowed keyset pagination...
+Pagination: Keyset cursor model is first-class state with forward/back cursors and prefetch buffer.
```
## 8) Harden session restore with atomic persistence + integrity checksum
Rationale: versioning/quarantine is good, but you still need crash-safe write semantics and tamper/corruption detection to avoid random boot failures.
```diff
@@ 10.1 New Files
-crates/lore-tui/src/session.rs # Versioned session state persistence + validation + corruption quarantine
+crates/lore-tui/src/session.rs # + atomic write (tmp->fsync->rename), checksum, max-size guard
@@ 11. Assumptions
+Session writes are atomic and checksummed.
+Invalid checksum or oversized file triggers quarantine and fresh boot.
```
## 9) Evolve Doctor from read-only text into actionable remediation
Rationale: your CLI already returns machine-actionable `actions`. TUI should surface those as one-key fixes; this materially increases usefulness.
```diff
@@ 5.11 Doctor / Stats (Info Screens)
-Simple read-only views rendering the output...
+Doctor is interactive:
+ - shows health checks + severity
+ - exposes suggested `actions` from robot-mode errors
+ - Enter runs selected action command (with confirmation modal)
+Stats remains read-only.
```
## 10) Add a Dependency Lens to Issue/MR detail (high-value triage feature)
Rationale: you already have cross-refs + discussions + timeline. A compact dependency panel (blocked-by / blocks / unresolved threads) makes this data operational for prioritization.
```diff
@@ 5.3 Issue Detail
-│ ┌─ Cross-References ─────────────────────────────────────────┐ │
+│ ┌─ Dependency Lens ──────────────────────────────────────────┐ │
+│ │ Blocked by: #1198 (open, stale 9d) │ │
+│ │ Blocks: !458 (opened, 2 unresolved threads) │ │
+│ │ Risk: High (P1 + stale blocker + open MR discussion) │ │
+│ └────────────────────────────────────────────────────────────┘ │
@@ 9.2 Phases
+Dependency Lens (issue/mr detail, computed risk score) :p3e, after p2e, 1d
```
---
If you want, I can next produce a consolidated **“v2.1 patch”** of the PRD with all these edits merged into one coherent updated document structure.

264
plans/tui-prd-v2-frankentui.feedback-7.md
View File

@@ -1,264 +0,0 @@
1. **Fix a critical contradiction in workspace/toolchain isolation**
Rationale: Section `3.2` says `crates/lore-tui` is excluded from the root workspace, but Section `9.1` currently adds it as a member. That inconsistency will cause broken CI/tooling behavior and confusion about whether stable-only workflows remain safe.
```diff
--- a/PRD.md
+++ b/PRD.md
@@ 9.1 Dependency Changes
-# Root Cargo.toml changes
-[workspace]
-members = [".", "crates/lore-tui"]
+# Root Cargo.toml changes
+[workspace]
+members = ["."]
+exclude = ["crates/lore-tui"]
@@
-# Add workspace member (no lore-tui dep, no tui feature)
+# Keep lore-tui EXCLUDED from root workspace (nightly isolation boundary)
@@ 9.3 Phase 0 — Toolchain Gate
-1. `cargo check --all-targets` passes on pinned nightly (TUI crate) and stable (core)
+1. `cargo +stable check --workspace --all-targets` passes for root workspace
+2. `cargo +nightly-2026-02-08 check --manifest-path crates/lore-tui/Cargo.toml --all-targets` passes
```
2. **Replace global loading spinner with per-screen stale-while-revalidate**
Rationale: A single `is_loading` flag causes full-screen flicker and blocked context during quick refreshes. Per-screen load states keep existing data visible while background refresh runs, improving perceived performance and usability.
```diff
--- a/PRD.md
+++ b/PRD.md
@@ 10.10 State Module — Complete
- pub is_loading: bool,
+ pub load_state: ScreenLoadStateMap,
@@
- pub fn set_loading(&mut self, loading: bool) {
- self.is_loading = loading;
- }
+ pub fn set_loading(&mut self, screen: ScreenId, state: LoadState) {
+ self.load_state.insert(screen, state);
+ }
+
+pub enum LoadState {
+ Idle,
+ LoadingInitial,
+ Refreshing, // stale data remains visible
+ Error(String),
+}
@@ 4.4 App — Implementing the Model Trait
- // Loading spinner overlay (while async data is fetching)
- if self.state.is_loading {
- crate::tui::view::common::render_loading(frame, body);
- } else {
- match self.navigation.current() { ... }
- }
+ // Always render screen; show lightweight refresh indicator when needed.
+ match self.navigation.current() { ... }
+ crate::tui::view::common::render_refresh_indicator_if_needed(
+ self.navigation.current(), &self.state.load_state, frame, body
+ );
```
3. **Make `TaskSupervisor` a real scheduler (not just token registry)**
Rationale: Current design declares priority lanes but still dispatches directly with `Cmd::task`, and debounce uses `thread::sleep` per keystroke (wastes worker threads). A bounded scheduler with queued tasks and timer-driven debounce will reduce contention and tail latency.
```diff
--- a/PRD.md
+++ b/PRD.md
@@ 4.5.1 Task Supervisor (Dedup + Cancellation + Priority)
-pub struct TaskSupervisor {
- active: HashMap<TaskKey, Arc<CancelToken>>,
- generation: AtomicU64,
-}
+pub struct TaskSupervisor {
+ active: HashMap<TaskKey, Arc<CancelToken>>,
+ generation: AtomicU64,
+ queue: BinaryHeap<ScheduledTask>,
+ inflight: HashMap<TaskPriority, usize>,
+ limits: TaskLaneLimits, // e.g. Input=4, Navigation=2, Background=1
+}
@@
-// 200ms debounce via cancelable scheduled event (not thread::sleep).
-Cmd::task(move || {
- std::thread::sleep(std::time::Duration::from_millis(200));
- ...
-})
+// Debounce via runtime timer message; no sleeping worker thread.
+self.state.search.debounce_deadline = Some(now + 200ms);
+Cmd::none()
@@ 4.4 update()
+Msg::Tick => {
+ if self.state.search.debounce_expired(now) {
+ return self.dispatch_supervised(TaskKey::Search, TaskPriority::Input, ...);
+ }
+ self.task_supervisor.dispatch_ready(now)
+}
```
4. **Add a sync run ledger for exact “new since sync” navigation**
Rationale: “Since last sync” based on timestamps is ambiguous with partial failures, retries, and clock drift. A lightweight `sync_runs` + `sync_deltas` ledger makes summary-mode drill-down exact and auditable without implementing full resumable checkpoints.
```diff
--- a/PRD.md
+++ b/PRD.md
@@ 5.9 Sync
-- `i` navigates to Issue List pre-filtered to "since last sync"
-- `m` navigates to MR List pre-filtered to "since last sync"
+- `i` navigates to Issue List pre-filtered to `sync_run_id=<last_run>`
+- `m` navigates to MR List pre-filtered to `sync_run_id=<last_run>`
+- Filters are driven by persisted `sync_deltas` rows (exact entity keys changed in run)
@@ 10.1 New Files
+src/core/migrations/00xx_add_sync_run_ledger.sql
@@ New migration (appendix)
+CREATE TABLE sync_runs (
+ id INTEGER PRIMARY KEY,
+ started_at_ms INTEGER NOT NULL,
+ completed_at_ms INTEGER,
+ status TEXT NOT NULL
+);
+CREATE TABLE sync_deltas (
+ sync_run_id INTEGER NOT NULL,
+ entity_kind TEXT NOT NULL,
+ project_id INTEGER NOT NULL,
+ iid INTEGER NOT NULL,
+ change_kind TEXT NOT NULL
+);
+CREATE INDEX idx_sync_deltas_run_kind ON sync_deltas(sync_run_id, entity_kind);
@@ 11 Assumptions
-16. No new SQLite tables needed for v1
+16. Two small v1 tables are added: `sync_runs` and `sync_deltas` for deterministic post-sync UX.
```
5. **Expand the GA index set to match actual filter surface**
Rationale: Current required indexes only cover default sort paths; they do not match common filters like `author`, `assignee`, `reviewer`, `target_branch`, label-based filtering. This will likely miss p95 SLOs at M tier.
```diff
--- a/PRD.md
+++ b/PRD.md
@@ 9.3.1 Required Indexes (GA Blocker)
CREATE INDEX IF NOT EXISTS idx_issues_list_default
ON issues(project_id, state, updated_at DESC, iid DESC);
+CREATE INDEX IF NOT EXISTS idx_issues_author_updated
+ ON issues(project_id, state, author_username, updated_at DESC, iid DESC);
+CREATE INDEX IF NOT EXISTS idx_issues_assignee_updated
+ ON issues(project_id, state, assignee_username, updated_at DESC, iid DESC);
@@
CREATE INDEX IF NOT EXISTS idx_mrs_list_default
ON merge_requests(project_id, state, updated_at DESC, iid DESC);
+CREATE INDEX IF NOT EXISTS idx_mrs_reviewer_updated
+ ON merge_requests(project_id, state, reviewer_username, updated_at DESC, iid DESC);
+CREATE INDEX IF NOT EXISTS idx_mrs_target_updated
+ ON merge_requests(project_id, state, target_branch, updated_at DESC, iid DESC);
+CREATE INDEX IF NOT EXISTS idx_mrs_source_updated
+ ON merge_requests(project_id, state, source_branch, updated_at DESC, iid DESC);
@@
+-- If labels are normalized through join table:
+CREATE INDEX IF NOT EXISTS idx_issue_labels_label_issue ON issue_labels(label, issue_id);
+CREATE INDEX IF NOT EXISTS idx_mr_labels_label_mr ON mr_labels(label, mr_id);
@@ CI enforcement
-asserts that none show `SCAN TABLE` for the primary entity tables
+asserts that none show full scans for primary tables under default filters AND top 8 user-facing filter combinations
```
6. **Add DB schema compatibility preflight (separate from binary compat)**
Rationale: Binary compat (`--compat-version`) does not protect against schema mismatches. Add explicit schema version checks before booting the TUI to avoid runtime SQL errors deep in navigation paths.
```diff
--- a/PRD.md
+++ b/PRD.md
@@ 3.2 Nightly Rust Strategy
-- **Compatibility contract:** Before spawning `lore-tui`, the `lore tui` subcommand runs `lore-tui --compat-version` ...
+- **Compatibility contract:** Before spawning `lore-tui`, `lore tui` validates:
+ 1) binary compat version (`lore-tui --compat-version`)
+ 2) DB schema range (`lore-tui --check-schema <db-path>`)
+If schema is out-of-range, print remediation: `lore migrate`.
@@ 9.3 Phase 0 — Toolchain Gate
+17. Schema preflight test: incompatible DB schema yields actionable error and non-zero exit before entering TUI loop.
```
7. **Refine terminal sanitization to preserve legitimate Unicode while blocking control attacks**
Rationale: Current sanitizer strips zero-width joiners and similar characters, which breaks emoji/grapheme rendering and undermines your own `text_width` goals. Keep benign Unicode, remove only dangerous controls/bidi spoof vectors, and sanitize markdown link targets too.
```diff
--- a/PRD.md
+++ b/PRD.md
@@ 10.4.1 Terminal Safety — Untrusted Text Sanitization
-- Strip bidi overrides ... and zero-width/invisible controls ...
+- Strip ANSI/OSC/control chars and bidi spoof controls.
+- Preserve legitimate grapheme-joining characters (ZWJ/ZWNJ/combining marks) for correct Unicode rendering.
+- Sanitize markdown link targets with strict URL allowlist before rendering clickable links.
@@ safety.rs
- // Strip zero-width and invisible controls
- '\u{200B}' | '\u{200C}' | '\u{200D}' | '\u{FEFF}' | '\u{00AD}' => {}
+ // Preserve grapheme/emoji join behavior; remove only harmful controls.
+ // (ZWJ/ZWNJ/combining marks are retained)
@@ Enforcement rule
- Search result snippets
- Author names and labels
+- Markdown link destinations (scheme + origin validation before render/open)
```
8. **Add key normalization layer for terminal portability**
Rationale: Collision notes are good, but you still need a canonicalization layer because terminals emit different sequences for Alt/Meta/Backspace/Enter variants. This reduces “works in iTerm, broken in tmux/SSH” bugs.
```diff
--- a/PRD.md
+++ b/PRD.md
@@ 8.2 List Screens
**Terminal keybinding safety notes:**
@@
- `Ctrl+M` is NOT used — it collides with `Enter` ...
+
+**Key normalization layer (new):**
+- Introduce `KeyNormalizer` before `interpret_key()`:
+ - normalize Backspace variants (`^H`, `DEL`)
+ - normalize Alt/Meta prefixes
+ - normalize Shift+Tab vs Tab where terminal supports it
+ - normalize kitty/CSI-u enhanced key protocols when present
@@ 9.2 Phases
+ Key normalization integration tests :p5d, after p5c, 1d
+ Terminal profile replay tests :p5e, after p5d, 1d
```
9. **Add deterministic event-trace capture for crash reproduction**
Rationale: Panic logs without recent event context are often insufficient for TUI race bugs. Persist last-N normalized events + active screen + task state snapshot on panic for one-command repro.
```diff
--- a/PRD.md
+++ b/PRD.md
@@ 3.1 Risk Matrix
| Runtime panic leaves user blocked | High | Medium | Panic hook writes crash report, restores terminal, offers fallback CLI command |
+| Hard-to-reproduce input race bugs | Medium | Medium | Persist last 2k normalized events + state hash on panic for deterministic replay |
@@ 10.3 Entry Point / panic hook
- // 2. Write crash dump
+ // 2. Write crash dump + event trace snapshot
+ // Includes: last 2000 normalized events, current screen, in-flight task keys/generations
@@ 10.9.1 Non-Snapshot Tests
+/// Replay captured event trace from panic artifact and assert no panic.
+#[test]
+fn replay_trace_artifact_is_stable() { ... }
```
10. **Do a plan-wide consistency pass on pseudocode contracts**
Rationale: There are internal mismatches that will create implementation churn (`search_request_id` still referenced after replacement, `items` vs `window`, keybinding mismatch `Ctrl+I` vs `Alt+o`). Tightening these now saves real engineering time later.
```diff
--- a/PRD.md
+++ b/PRD.md
@@ 4.4 LoreApp::new
- search_request_id: 0,
+ // dedup generation handled by TaskSupervisor
@@ 8.1 Global
-| `Ctrl+O` | Jump backward in jump list (entity hops) |
-| `Alt+o` | Jump forward in jump list (entity hops) |
+| `Ctrl+O` | Jump backward in jump list (entity hops) |
+| `Alt+o` | Jump forward in jump list (entity hops) |
@@ 10.10 IssueListState
- pub fn selected_item(&self) -> Option<&IssueListRow> {
- self.items.get(self.selected_index)
- }
+ pub fn selected_item(&self) -> Option<&IssueListRow> {
+ self.window.get(self.selected_index)
+ }
```
If you want, I can now produce a single consolidated unified diff patch of the full PRD with these revisions merged end-to-end.

211
plans/tui-prd-v2-frankentui.feedback-8.md
View File

@@ -1,211 +0,0 @@
Below are the strongest revisions I’d make. I intentionally avoided anything in your `## Rejected Recommendations`.
1. **Unify commands/keybindings/help/palette into one registry**
Rationale: your plan currently duplicates action definitions across `execute_palette_action`, `ShowCliEquivalent`, help overlay text, and status hints. That will drift quickly and create correctness bugs. A single `CommandRegistry` makes behavior consistent and testable.
```diff
diff --git a/PRD.md b/PRD.md
@@ 4.1 Module Structure
+ commands.rs # Single source of truth for actions, keybindings, CLI equivalents
@@ 4.4 App — Implementing the Model Trait
- fn execute_palette_action(&self, action_id: &str) -> Cmd<Msg> { ... big match ... }
+ fn execute_palette_action(&self, action_id: &str) -> Cmd<Msg> {
+ if let Some(spec) = self.commands.get(action_id) {
+ return self.update(spec.to_msg(self.navigation.current()));
+ }
+ Cmd::none()
+ }
@@ 8. Keybinding Reference
+All keybinding/help/status/palette definitions are generated from `commands.rs`.
+No hardcoded duplicate maps in view/state modules.
```
2. **Replace ad-hoc key flags with explicit input state machine**
Rationale: `pending_go` + `go_prefix_instant` is fragile and already inconsistent with documented behavior. A typed `InputMode` removes edge-case bugs and makes prefix timeout deterministic.
```diff
diff --git a/PRD.md b/PRD.md
@@ 4.4 LoreApp struct
- pending_go: bool,
- go_prefix_instant: Option<std::time::Instant>,
+ input_mode: InputMode, // Normal | Text | Palette | GoPrefix { started_at }
@@ 8.2 List Screens
-| `g` `g` | Jump to top |
+| `g` `g` | Jump to top (current list screen) |
@@ 4.4 interpret_key
- KeyCode::Char('g') => Msg::IssueListScrollToTop
+ KeyCode::Char('g') => Msg::ScrollToTopCurrentScreen
```
3. **Fix TaskSupervisor contract and message schema drift**
Rationale: the plan mixes `request_id` and `generation`, and `TaskKey::Search { generation }` defeats dedup by making every key unique. This can silently reintroduce stale-result races.
```diff
diff --git a/PRD.md b/PRD.md
@@ 4.3 Core Types (Msg)
- SearchRequestStarted { request_id: u64, query: String },
- SearchExecuted { request_id: u64, results: SearchResults },
+ SearchRequestStarted { generation: u64, query: String },
+ SearchExecuted { generation: u64, results: SearchResults },
@@ 4.5.1 Task Supervisor
- Search { generation: u64 },
+ Search,
+ struct TaskStamp { key: TaskKey, generation: u64 }
@@ 10.9.1 Non-Snapshot Tests
- Msg::SearchExecuted { request_id: 3, ... }
+ Msg::SearchExecuted { generation: 3, ... }
```
4. **Add a `Clock` boundary everywhere time is computed**
Rationale: you call `SystemTime::now()` in many query/render paths, causing inconsistent relative-time labels inside one frame and flaky tests. Injected clock gives deterministic rendering and lower per-frame overhead.
```diff
diff --git a/PRD.md b/PRD.md
@@ 4.1 Module Structure
+ clock.rs # Clock trait: SystemClock/FakeClock
@@ 4.4 LoreApp struct
+ clock: Arc<dyn Clock>,
@@ 10.11 action.rs
- let now_ms = std::time::SystemTime::now()...
+ let now_ms = clock.now_ms();
@@ 9.3 Phase 0 success criteria
+19. Relative-time rendering deterministic under FakeClock across snapshot runs.
```
5. **Upgrade text truncation to grapheme-safe width handling**
Rationale: `unicode-width` alone is not enough for safe truncation; it can split grapheme clusters (emoji ZWJ sequences, skin tones, flags). You need width + grapheme segmentation together.
```diff
diff --git a/PRD.md b/PRD.md
@@ 10.1 New Files
-crates/lore-tui/src/text_width.rs # ... using unicode-width crate
+crates/lore-tui/src/text_width.rs # Grapheme-safe width/truncation using unicode-width + unicode-segmentation
@@ 10.1 New Files
+Cargo.toml (lore-tui): unicode-segmentation = "1"
@@ 9.3 Phase 0 success criteria
+20. Unicode rendering tests pass for CJK, emoji ZWJ, combining marks, RTL text.
```
6. **Redact sensitive values in logs and crash dumps**
Rationale: current crash/log strategy risks storing tokens/credentials in plain text. This is a serious operational/security gap for local tooling too.
```diff
diff --git a/PRD.md b/PRD.md
@@ 4.1 Module Structure
safety.rs # sanitize_for_terminal(), safe_url_policy()
+ redact.rs # redact_sensitive() for logs/crash reports
@@ 10.3 install_panic_hook_for_tui
- let _ = std::fs::write(&crash_path, format!("{panic_info:#?}"));
+ let report = redact_sensitive(format!("{panic_info:#?}"));
+ let _ = std::fs::write(&crash_path, report);
@@ 9.3 Phase 0 success criteria
+21. Redaction tests confirm tokens/Authorization headers never appear in persisted crash/log artifacts.
```
7. **Add search capability detection and mode fallback UX**
Rationale: semantic/hybrid mode should not silently degrade when embeddings are absent/stale. Explicit capability state increases trust and avoids “why are results weird?” confusion.
```diff
diff --git a/PRD.md b/PRD.md
@@ 5.6 Search
+Capability-aware modes:
+- If embeddings unavailable/stale, semantic mode is disabled with inline reason.
+- Hybrid mode auto-falls back to lexical and shows badge: "semantic unavailable".
@@ 4.3 Core Types
+ SearchCapabilitiesLoaded(SearchCapabilities)
@@ 9.3 Phase 0 success criteria
+22. Mode availability checks validated: lexical/hybrid/semantic correctly enabled/disabled by fixture capabilities.
```
8. **Define sync cancel latency SLO and enforce fine-grained checks**
Rationale: “check cancel between phases” is too coarse on big projects. Users need fast cancel acknowledgment and bounded stop time.
```diff
diff --git a/PRD.md b/PRD.md
@@ 5.9 Sync
-CANCELLATION: checked between sync phases
+CANCELLATION: checked at page boundaries, batch upsert boundaries, and before each network request.
+UX target: cancel acknowledged <250ms, sync stop p95 <2s after Esc.
@@ 9.3 Phase 0 success criteria
+23. Cancel latency test passes: p95 stop time <2s under M-tier fixtures.
```
9. **Add a “Hotspots” screen for risk/churn triage**
Rationale: this is high-value and uses existing data (events, unresolved discussions, stale items). It makes the TUI more compelling without needing new sync tables or rejected features.
```diff
diff --git a/PRD.md b/PRD.md
@@ 1. Executive Summary
+- **Hotspots** — file/path risk ranking by churn × unresolved discussion pressure × staleness
@@ 5. Screen Taxonomy
+### 5.12 Hotspots
+Shows top risky paths with drill-down to related issues/MRs/timeline.
@@ 8.1 Global
+| `gx` | Go to Hotspots |
@@ 10.1 New Files
+crates/lore-tui/src/state/hotspots.rs
+crates/lore-tui/src/view/hotspots.rs
```
10. **Add degraded startup mode when compat/schema checks fail**
Rationale: hard-exit on mismatch blocks users. A degraded mode that shells to `lore --robot` for read-only summary/doctor keeps the product usable and gives guided recovery.
```diff
diff --git a/PRD.md b/PRD.md
@@ 3.2 Nightly Rust Strategy
- On mismatch: actionable error and exit
+ On mismatch: actionable error with `--degraded` option.
+ `--degraded` launches limited TUI (Dashboard/Doctor/Stats via `lore --robot` subprocess calls).
@@ 10.3 TuiCli
+ /// Allow limited mode when schema/compat checks fail
+ #[arg(long)]
+ degraded: bool,
```
11. **Harden query-plan CI checks (don’t rely on `SCAN TABLE` string matching)**
Rationale: SQLite planner text varies by version. Parse opcode structure and assert index usage semantically; otherwise CI will be flaky or miss regressions.
```diff
diff --git a/PRD.md b/PRD.md
@@ 9.3.1 Required Indexes (CI enforcement)
- asserts that none show `SCAN TABLE`
+ parses EXPLAIN QUERY PLAN rows and asserts:
+ - top-level loop uses expected index families
+ - no full scan on primary entity tables under default and top filter combos
+ - join order remains bounded (no accidental cartesian expansions)
```
12. **Enforce single-instance lock for session/state safety**
Rationale: assumption says no concurrent TUI sessions, but accidental double-launch will still happen. Locking prevents state corruption and confusing interleaved sync actions.
```diff
diff --git a/PRD.md b/PRD.md
@@ 10.1 New Files
+crates/lore-tui/src/instance_lock.rs # lock file with stale-lock recovery
@@ 11. Assumptions
-21. No concurrent TUI sessions.
+21. Concurrent sessions unsupported and actively prevented by instance lock (with clear error message).
```
If you want, I can turn this into a consolidated patched PRD (single unified diff) next.

198
plans/tui-prd-v2-frankentui.feedback-9.md
View File

@@ -1,198 +0,0 @@
I reviewed the full PRD end-to-end and avoided all items already listed in `## Rejected Recommendations`.
These are the highest-impact revisions I’d make.
1. **Fix keybinding/state-machine correctness gaps (critical)**
The plan currently has an internal conflict: the doc says jump-forward is `Alt+o`, but code sample uses `Ctrl+i` (which collides with `Tab` in many terminals). Also, `g`-prefix timeout depends on `Tick`, but `Tick` isn’t guaranteed when idle, so prefix mode can get “stuck.” This is a correctness bug, not polish.
```diff
@@ 8.1 Global (Available Everywhere)
-| `Ctrl+O` | Jump backward in jump list (entity hops) |
-| `Alt+o` | Jump forward in jump list (entity hops) |
+| `Ctrl+O` | Jump backward in jump list (entity hops) |
+| `Alt+o` | Jump forward in jump list (entity hops) |
+| `Backspace` | Go back (when no text input is focused) |
@@ 4.4 LoreApp::interpret_key
- (KeyCode::Char('i'), m) if m.contains(Modifiers::CTRL) => {
- return Some(Msg::JumpForward);
- }
+ (KeyCode::Char('o'), m) if m.contains(Modifiers::ALT) => {
+ return Some(Msg::JumpForward);
+ }
+ (KeyCode::Backspace, Modifiers::NONE) => {
+ return Some(Msg::GoBack);
+ }
@@ 4.4 Model::subscriptions
+ // Go-prefix timeout enforcement must tick even when nothing is loading.
+ if matches!(self.input_mode, InputMode::GoPrefix { .. }) {
+ subs.push(Box::new(
+ Every::with_id(2, Duration::from_millis(50), || Msg::Tick)
+ ));
+ }
```
2. **Make `TaskSupervisor` API internally consistent and enforceable**
The plan uses `submit()`/`is_current()` in one place and `register()`/`next_generation()` in another. That inconsistency will cause implementation drift and stale-result bugs. Use one coherent API with a returned handle containing `{key, generation, cancel_token}`.
```diff
@@ 4.5.1 Task Supervisor (Dedup + Cancellation + Priority)
-pub struct TaskSupervisor {
- active: HashMap<TaskKey, Arc<CancelToken>>,
- generation: AtomicU64,
-}
+pub struct TaskSupervisor {
+ active: HashMap<TaskKey, TaskHandle>,
+}
+
+pub struct TaskHandle {
+ pub key: TaskKey,
+ pub generation: u64,
+ pub cancel: Arc<CancelToken>,
+}
- pub fn register(&mut self, key: TaskKey) -> Arc<CancelToken>
- pub fn next_generation(&self) -> u64
+ pub fn submit(&mut self, key: TaskKey) -> TaskHandle
+ pub fn is_current(&self, key: &TaskKey, generation: u64) -> bool
+ pub fn complete(&mut self, key: &TaskKey, generation: u64)
```
3. **Replace thread-sleep debounce with runtime timer messages**
`std::thread::sleep(200ms)` inside task closures wastes pool threads under fast typing and reduces responsiveness under contention. Use timer-driven debounce messages and only fire the latest generation. This improves latency stability on large datasets.
```diff
@@ 4.3 Core Types (Msg enum)
+ SearchDebounceArmed { generation: u64, query: String },
+ SearchDebounceFired { generation: u64 },
@@ 4.4 maybe_debounced_query
- Cmd::task(move || {
- std::thread::sleep(std::time::Duration::from_millis(200));
- ...
- })
+ // Arm debounce only; runtime timer emits SearchDebounceFired.
+ Cmd::msg(Msg::SearchDebounceArmed { generation, query })
@@ 4.4 subscriptions()
+ if self.state.search.debounce_pending() {
+ subs.push(Box::new(
+ Every::with_id(3, Duration::from_millis(200), || Msg::SearchDebounceFired { generation: ... })
+ ));
+ }
```
4. **Harden `DbManager` API to avoid lock-poison panics and accidental long-held guards**
Returning raw `MutexGuard<Connection>` invites accidental lock scope expansion and `expect("lock poisoned")` panics. Move to closure-based access (`with_reader`, `with_writer`) returning `Result`, and use cached statements. This reduces deadlock risk and tail latency.
```diff
@@ 4.4 DbManager
- pub fn reader(&self) -> MutexGuard<'_, Connection> { ...expect("reader lock poisoned") }
- pub fn writer(&self) -> MutexGuard<'_, Connection> { ...expect("writer lock poisoned") }
+ pub fn with_reader<T>(&self, f: impl FnOnce(&Connection) -> Result<T, LoreError>) -> Result<T, LoreError>
+ pub fn with_writer<T>(&self, f: impl FnOnce(&Connection) -> Result<T, LoreError>) -> Result<T, LoreError>
@@ 10.11 action.rs
- let conn = db.reader();
- match fetch_issues(&conn, &filter) { ... }
+ match db.with_reader(|conn| fetch_issues(conn, &filter)) { ... }
+ // Query hot paths use prepare_cached() to reduce parse overhead.
```
5. **Add read-path entity cache (LRU) for repeated drill-in/out workflows**
Your core daily flow is Enter/Esc bouncing between list/detail. Without caching, identical detail payloads are re-queried repeatedly. A bounded LRU by `EntityKey` with invalidation on sync completion gives near-instant reopen behavior and reduces DB pressure.
```diff
@@ 4.1 Module Structure
+ entity_cache.rs # Bounded LRU cache for detail payloads
@@ app.rs LoreApp fields
+ entity_cache: EntityCache,
@@ load_screen(Screen::IssueDetail / MrDetail)
+ if let Some(cached) = self.entity_cache.get_issue(&key) {
+ return Cmd::msg(Msg::IssueDetailLoaded { key, detail: cached.clone() });
+ }
@@ Msg::IssueDetailLoaded / Msg::MrDetailLoaded handlers
+ self.entity_cache.put_issue(key.clone(), detail.clone());
@@ Msg::SyncCompleted
+ self.entity_cache.invalidate_all();
```
6. **Tighten sync-stream observability and drop semantics without adding heavy architecture**
You already handle backpressure, but operators need visibility when it happens. Track dropped-progress count and max queue depth in state and surface it in running/summary views. This keeps the current simple design while making reliability measurable.
```diff
@@ 4.3 Msg
+ SyncStreamStats { dropped_progress: u64, max_queue_depth: usize },
@@ 5.9 Sync (Running mode footer)
-| Esc cancel f full sync e embed after d dry-run l log level|
+| Esc cancel f full sync e embed after d dry-run l log level stats:drop=12 qmax=1847 |
@@ 9.3 Success criteria
+24. Sync stream stats are emitted and rendered; terminal events (completed/failed/cancelled) delivery is 100% under induced backpressure.
```
7. **Make crash reporting match the promised diagnostic value**
The PRD promises event replay context, but sample hook writes only panic text. Add explicit crash context capture (`last events`, `current screen`, `task handles`, `build id`, `db fingerprint`) and retention policy. This materially improves post-mortem debugging.
```diff
@@ 4.1 Module Structure
+ crash_context.rs # ring buffer of normalized events + task/screen snapshot
@@ 10.3 install_panic_hook_for_tui()
- let report = crate::redact::redact_sensitive(&format!("{panic_info:#?}"));
+ let ctx = crate::crash_context::snapshot();
+ let report = crate::redact::redact_sensitive(&format!("{panic_info:#?}\n{ctx:#?}"));
+ // Retention: keep latest 20 crash files, delete oldest metadata entries only.
```
8. **Add Search Facets panel for faster triage (high-value feature, low risk)**
Search is central, but right now filtering requires manual field edits. Add facet counts (`issues`, `MRs`, `discussions`, top labels/projects/authors) with one-key apply. This makes search more compelling and actionable without introducing schema changes.
```diff
@@ 5.6 Search
-- Layout: Split pane — results list (left) + preview (right)
+- Layout: Three-pane on wide terminals — results (left) + preview (center) + facets (right)
+**Facets panel:**
+- Entity type counts (issue/MR/discussion)
+- Top labels/projects/authors for current query
+- `1/2/3` quick-apply type facet; `l` cycles top label facet
@@ 8.2 List/Search keybindings
+| `1` `2` `3` | Apply facet: Issue / MR / Discussion |
+| `l` | Apply next top-label facet |
```
9. **Strengthen text sanitization for terminal edge cases**
Current sanitizer is strong, but still misses some control-space edge cases (C1 controls, directional marks beyond the listed bidi set). Add those and test them. This closes spoofing/render confusion gaps with minimal complexity.
```diff
@@ 10.4.1 sanitize_for_terminal()
+ // Strip C1 control block (U+0080..U+009F) and additional directional marks
+ c if ('\u{0080}'..='\u{009F}').contains(&c) => {}
+ '\u{200E}' | '\u{200F}' | '\u{061C}' => {} // LRM, RLM, ALM
@@ tests
+ #[test] fn strips_c1_controls() { ... }
+ #[test] fn strips_lrm_rlm_alm() { ... }
```
10. **Add an explicit vertical-slice gate before broad screen expansion**
The plan is comprehensive, but risk is still front-loaded on framework + runtime behavior. Insert a strict vertical slice gate (`Dashboard + IssueList + IssueDetail + Sync running`) with perf and stability thresholds before Phase 3 features. This reduces rework if foundational assumptions break.
```diff
@@ 9.2 Phases
+section Phase 2.5 — Vertical Slice Gate
+Dashboard + IssueList + IssueDetail + Sync (running) integrated :p25a, after p2c, 3d
+Gate: p95 nav latency < 75ms on M tier; zero stuck-input-state bugs; cancel p95 < 2s :p25b, after p25a, 1d
+Only then proceed to Search/Timeline/Who/Palette expansion.
```
If you want, I can produce a full consolidated `diff` block against the entire PRD text (single patch), but the above is the set I’d prioritize first.