gitlore/plans/work-item-status-graphql.feedback-3.md

Top Revisions I Recommend

1. Fix auth semantics + a real inconsistency in your test plan Your ACs require graceful handling for 403, but the test list says the “403” test returns 401. That hides the exact behavior you care about and can let permission regressions slip through.

@@ AC-1: GraphQL Client (Unit)
 - [ ] HTTP 401 → `LoreError::GitLabAuthFailed`
 + [ ] HTTP 401 → `LoreError::GitLabAuthFailed`
 + [ ] HTTP 403 → `LoreError::GitLabForbidden`

@@ AC-3: Status Fetcher (Integration)
 - [ ] GraphQL 403 → returns `Ok(HashMap::new())` with warning log
 + [ ] GraphQL 403 (`GitLabForbidden`) → returns `Ok(HashMap::new())` with warning log

@@ TDD Plan (RED)
 - 13. `test_fetch_statuses_403_graceful` — mock returns 401 → `Ok(HashMap::new())`
 + 13. `test_fetch_statuses_403_graceful` — mock returns 403 → `Ok(HashMap::new())`

2. Make enrichment atomic and stale-safe Current plan can leave stale status values forever when a widget disappears or status becomes null. Make writes transactional and clear status fields for fetched scope before upserts.

@@ AC-6: Enrichment in Orchestrator (Integration)
 + [ ] Enrichment DB writes are transactional per project (all-or-nothing)
 + [ ] Status fields are cleared for fetched issue scope before applying new statuses
 + [ ] If enrichment fails mid-project, prior persisted statuses are unchanged (rollback)

@@ File 6: `src/ingestion/orchestrator.rs`
- fn enrich_issue_statuses(...)
+ fn enrich_issue_statuses_txn(...)
+ // BEGIN TRANSACTION
+ // clear status columns for fetched issue scope
+ // apply updates
+ // COMMIT

3. Add transient retry/backoff (429/5xx/network) Right now one transient failure loses status enrichment for that sync. Retrying with bounded backoff gives much better reliability at low cost.

@@ AC-1: GraphQL Client (Unit)
 + [ ] Retries 429/502/503/504/network errors with bounded exponential backoff + jitter (max 3 attempts)
 + [ ] Honors `Retry-After` on 429 before retrying

@@ AC-6: Enrichment in Orchestrator (Integration)
 + [ ] Cancellation signal is checked before each retry sleep and between paginated calls

4. Stop full GraphQL scans when nothing changed Running full pagination on every sync will dominate runtime on large repos. Trigger enrichment only when issue ingestion reports changes, with a manual override.

@@ AC-6: Enrichment in Orchestrator (Integration)
- [ ] Runs on every sync (not gated by `--full`)
+ [ ] Runs when issue ingestion changed at least one issue in the project
+ [ ] New override flag `--refresh-status` forces enrichment even with zero issue deltas
+ [ ] Optional periodic full refresh (e.g. every N syncs) to prevent long-tail drift

5. Do not expose raw token via client.token() Architecturally cleaner and safer: keep token encapsulated and expose a GraphQL-ready client factory from GitLabClient.

@@ File 13: `src/gitlab/client.rs`
- pub fn token(&self) -> &str
+ pub fn graphql_client(&self) -> crate::gitlab::graphql::GraphqlClient

@@ File 6: `src/ingestion/orchestrator.rs`
- let graphql_client = GraphqlClient::new(&config.gitlab.base_url, client.token());
+ let graphql_client = client.graphql_client();

6. Add indexes for new status filters --status on large tables will otherwise full-scan issues. Add compound indexes aligned with project-scoped list queries.

@@ AC-4: Migration 021 (Unit)
 + [ ] Adds index `idx_issues_project_status_name(project_id, status_name)`
 + [ ] Adds index `idx_issues_project_status_category(project_id, status_category)`

@@ File 14: `migrations/021_work_item_status.sql`
 ALTER TABLE issues ADD COLUMN status_name TEXT;
 ALTER TABLE issues ADD COLUMN status_category TEXT;
 ALTER TABLE issues ADD COLUMN status_color TEXT;
 ALTER TABLE issues ADD COLUMN status_icon_name TEXT;
+CREATE INDEX IF NOT EXISTS idx_issues_project_status_name
+  ON issues(project_id, status_name);
+CREATE INDEX IF NOT EXISTS idx_issues_project_status_category
+  ON issues(project_id, status_category);

7. Improve filter UX: add category filter + case-insensitive status Case-sensitive exact name matches are brittle with custom lifecycle names. Category filter is stable and useful for automation.

@@ AC-9: List Issues Filter (E2E)
- [ ] Filter is case-sensitive (matches GitLab's exact status name)
+ [ ] `--status` uses case-insensitive exact match by default (`COLLATE NOCASE`)
+ [ ] New filter `--status-category` supports `triage|to_do|in_progress|done|canceled`
+ [ ] `--status-exact` enables strict case-sensitive behavior when needed

8. Add capability probe/cache to avoid pointless calls Free tier / old GitLab versions will never return status widget. Cache that capability per project (with TTL) to reduce noise and wasted requests.

@@ GitLab API Constraints
+### Capability Probe
+On first sync per project, detect status-widget support and cache result for 24h.
+If unsupported, skip enrichment silently (debug log) until TTL expiry.

@@ AC-3: Status Fetcher (Integration)
+ [ ] Unsupported capability state bypasses GraphQL fetch and warning spam

9. Use a nested robot status object instead of 4 top-level fields This is cleaner schema design and scales better as status metadata grows (IDs, lifecycle, timestamps, etc.).

@@ AC-7: Show Issue Display (Robot)
- [ ] JSON includes `status_name`, `status_category`, `status_color`, `status_icon_name` fields
- [ ] Fields are `null` (not absent) when status not available
+ [ ] JSON includes `status` object:
+       `{ "name": "...", "category": "...", "color": "...", "icon_name": "..." }` or `null`

@@ AC-8: List Issues Display (Robot)
- [ ] JSON includes `status_name`, `status_category` fields on each issue
+ [ ] JSON includes `status` object (or `null`) on each issue

10. Add one compelling feature: status analytics, not just status display Right now this is mostly a transport/display enhancement. Make it genuinely useful with “stale in-progress” detection and age-in-status filters.

@@ Acceptance Criteria
+### AC-11: Status Aging & Triage Value (E2E)
+- [ ] `lore list issues --status-category in_progress --stale-days 14` filters to stale work
+- [ ] Human table shows `Status Age` (days) when status exists
+- [ ] Robot output includes `status_age_days` (nullable integer)

11. Harden test plan around failure modes you’ll actually hit The current tests are good, but miss rollback/staleness/retry behavior that drives real reliability.

@@ TDD Plan (RED) additions
+21. `test_enrich_clears_removed_status`
+22. `test_enrich_transaction_rolls_back_on_failure`
+23. `test_graphql_retry_429_then_success`
+24. `test_graphql_retry_503_then_success`
+25. `test_cancel_during_backoff_aborts_cleanly`
+26. `test_status_filter_query_uses_project_status_index` (EXPLAIN smoke test)

If you want, I can produce a fully revised v3 plan document end-to-end (frontmatter + reordered ACs + updated file list + updated TDD matrix) so it is ready to implement directly.