feat(dashboard): task & skill ledger views (#865)#955
Conversation
Adds two read-only ANSI views to the existing terminal dashboard: - TasksView (src/dashboard/views/tasks-view.ts) — surfaces the task ledger from #855. Reads ~/.openchrome/tasks/ (or OPENCHROME_TASK_ROOT). Renders task_id, kind, status, age, duration_ms. Status colour-coded: RUNNING cyan, COMPLETED green, FAILED red, CANCELLED yellow. - SkillsView (src/dashboard/views/skills-view.ts) — walks ~/.openchrome/skill-memory/<domain>/skills.json across all known domains. Renders domain, skill name, use count, replay badge (PASS/FAIL/—), and relative last-used timestamp. The replay badge reads the optional lastReplayPassedAt/lastReplayFailedAt fields defensively so this PR builds on either A (#855) alone or the full A+B stack — the cast becomes redundant once #856 is in develop. Wiring (src/dashboard/index.ts) - ViewMode extended with 'tasks' | 'skills' (src/dashboard/types.ts). - Two new view instances initialised next to the existing four. - Switch case in the render dispatcher routes to the new views. - Main-view keys: 'j' → tasks ledger, 'k' → skills ledger. (Existing 't' is already taken for tabs; 's' for sessions.) - Ledger-view key handler: 'escape' returns to activity. - Ledger snapshot is re-read on entry; subsequent dashboard ticks refresh via refreshLedgers(). Read-only — IO errors surface in the view's errorMessage field, never thrown. Per portability-harness contract - P2: os.homedir() + path.join everywhere; no platform-specific paths. - P4: views never mutate ledger files; the snapshot readers only open files for read. - P5: no new native deps. package.json is untouched. Tests (6 new) - TasksView: header + mixed-status rows render; empty-state; error message surfaces. - SkillsView: rows + PASS/FAIL replay badges; empty-state. - readSkillsSnapshot tolerates a missing skill-memory directory and does not write any state. Stacked on top of #855 (feat/855-task-ledger). PR base will be feat/855-task-ledger; once #855 merges to develop this branch will fast-forward. Closes #865
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad4e26c474
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.currentView = 'tasks'; | ||
| this.selectedIndex = 0; | ||
| void this.refreshLedgers(); |
There was a problem hiding this comment.
The new ledger views only invoke refreshLedgers() when j/k is pressed, so once the view is open the periodic loop continues to call refresh() without re-reading disk state; this leaves task/skill rows stale until the user exits and re-enters the view. In practice, status transitions and newly written records won't appear live despite the in-code comment claiming tick-based updates, so the ledger can show outdated operational state.
Useful? React with 👍 / 👎.
| const encodedDomain = entry.name; | ||
| const domain = encodedDomain; | ||
| try { | ||
| const store = new SkillMemoryStore({ domain }); |
There was a problem hiding this comment.
entry.name here is already the encoded on-disk domain folder name, but passing it as domain into new SkillMemoryStore({ domain }) causes encodeDomain in src/core/skill-memory/store.ts to run again; domains containing encoded characters (for example %2F, %3A, etc.) become %252F, %253A, so list() reads a non-existent directory and valid skills are silently omitted from the ledger.
Useful? React with 👍 / 👎.
Surface read-only TaskRun NEEDS_HELP metadata and active handoff context in the terminal task view so operators can see why automation paused and how to resume without digging through JSON files. Constraint: Stack on #955 dashboard task view and read #1039/#1040 metadata defensively from disk because those branches may merge in a different order. Rejected: Dashboard resume/cancel controls | recovery state must remain read-only and MCP tools own mutation. Confidence: high Scope-risk: moderate Directive: Keep dashboard recovery readers best-effort and non-throwing; never add writes or hotkey mutations to this pane. Tested: npm test -- --runTestsByPath tests/dashboard/views/ledger-views.test.ts --runInBand Tested: npm run build Tested: npm run lint:changed Co-authored-by: OmX <omx@oh-my-codex.dev>
Refresh task and skill ledger snapshots from the dashboard timer path so read-only ledger views continue to reflect task status and skill rows without leaving the view. Constraint: PR #955 promised entry plus tick refresh, but the timer only repainted cached data. Rejected: Polling ledgers from every dashboard view | non-ledger views do not need disk reads on each tick. Confidence: high Scope-risk: narrow Directive: Keep ledger readers read-only and guarded against overlapping async filesystem walks. Tested: npm test -- --runInBand tests/dashboard/views/ledger-views.test.ts; ./node_modules/.bin/tsc -p tsconfig.json --pretty false Not-tested: Interactive TTY dashboard smoke. Co-authored-by: OmX <omx@oh-my-codex.dev>
Routes task/skill view entry refreshes and dashboard timer refreshes through one guarded scheduler so snapshot reads cannot overlap while a ledger view remains open. Constraint: Re-review found entry-triggered refreshes bypassed the non-overlap guard added for timer ticks. Rejected: Making refreshLedgers silently reuse state | a small scheduler keeps the reader method simple and the guard explicit. Confidence: high Scope-risk: narrow Directive: Keep ledger refresh scheduling single-path for entry keys and timer ticks. Tested: npm test -- --runInBand tests/dashboard/views/ledger-views.test.ts; ./node_modules/.bin/tsc -p tsconfig.json --pretty false Not-tested: Interactive TTY dashboard smoke. Co-authored-by: OmX <omx@oh-my-codex.dev>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98ca862289
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const domain = encodedDomain; | ||
| try { | ||
| const store = new SkillMemoryStore({ domain }); |
There was a problem hiding this comment.
Fresh evidence from this commit: readSkillsSnapshot still passes entry.name directly as domain into SkillMemoryStore, and the store constructor re-applies domain encoding. When a persisted domain directory already contains percent escapes (for example evil%2F..%2Fescape), this becomes double-encoded (%252F...), so store.list() reads the wrong path and valid skills for that domain disappear from the ledger view.
Useful? React with 👍 / 👎.
Resolve the stacked dashboard test conflict by preserving both recovery-pane read-only coverage and ledger refresh timer coverage. Constraint: PR #1119 targets the dashboard-ledger-views branch, not develop, and only the shared ledger-views test file conflicted. Rejected: Taking either side wholesale | that would drop either recovery state coverage or dashboard refresh guard coverage. Confidence: high Scope-risk: narrow Directive: Keep dashboard stack conflict fixes test-only unless source conflicts require owner review. Tested: npx jest tests/dashboard/views/ledger-views.test.ts --runInBand --forceExit; git diff --check for touched file. Not-tested: Full GitHub Actions matrix. Co-authored-by: OmX <omx@oh-my-codex.dev>
SkillMemoryStore encodes the domain internally via encodeDomain(). Passing entry.name — which is the already-encoded filesystem basename — caused the store to look at a double-encoded directory and surface no skills. Decode the directory name with decodeURIComponent() so the store sees the raw domain it expects. Addresses Codex P2 finding on PR #955.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb73160841
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // encodeDomain() inside SkillMemoryStore. Decode it back to the raw | ||
| // domain string before constructing the store, which re-encodes | ||
| // internally — passing an already-encoded name would double-encode it. | ||
| const domain = decodeURIComponent(entry.name); |
There was a problem hiding this comment.
decodeURIComponent(entry.name) can throw URIError for malformed percent escapes (for example a directory named bad%), but this call currently happens before the per-entry try/catch, so a single bad directory aborts the whole snapshot and returns a global error instead of skipping just that entry. That contradicts the intended best-effort behavior in this loop and can hide all otherwise valid skills until the filesystem is cleaned up.
Useful? React with 👍 / 👎.
feat(dashboard): surface recovery state in task view (#1043)
Merge rationale (stack consolidation)Intent. Adds two read-only ANSI views (TasksView + SkillsView) to the existing terminal dashboard, stacked on top of #911 (task ledger). Why this is correct.
CI. Targets the task-ledger feature branch; CI workflow only runs on |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5d1af7b0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async function readTaskRuns(root: string, limit: number): Promise<TaskRunRecovery[]> { | ||
| const entries = await readdirSafe(root); | ||
| const rows: TaskRunRecovery[] = []; | ||
| for (const entry of entries.slice(0, limit)) { |
There was a problem hiding this comment.
readTaskRuns slices entries before reading metadata and sorting, so with more than limit run directories you only rank an arbitrary filesystem subset instead of the most recent requests. In environments with many task runs, newer NEEDS_HELP records can be omitted from the dashboard entirely because they were outside the first entries.slice(0, limit) batch.
Useful? React with 👍 / 👎.
| async function readHandoffs(root: string, limit: number): Promise<HandoffRecovery[]> { | ||
| const entries = await readdirSafe(root); | ||
| const rows: HandoffRecovery[] = []; | ||
| for (const entry of entries.slice(0, limit)) { |
There was a problem hiding this comment.
readHandoffs also truncates directory entries before parsing and sorting, which means the returned rows are not guaranteed to be the most relevant ACTIVE handoffs when the handoff directory contains more than limit entries. Because directory iteration order is not time-ordered, active/soon-expiring handoffs can be dropped from the recovery pane even though they should be visible.
Useful? React with 👍 / 👎.
1 participant
Progress / Review status
Auto-refreshed 2026-05-13 — owner comments cleaned up to reduce review noise.
feat/865-dashboard-ledger-views→feat/855-task-ledgerbb73160— fix(dashboard): decode skill domain before constructing storeOwner comment cleanup: 1 issue + 0 inline review comments deleted. Outstanding feedback from automated/external reviewers above is unchanged.
Summary
Adds two read-only ANSI views to the existing terminal dashboard. Stacked on top of #911 (task ledger). PR base is
feat/855-task-ledgerso the diff shows only F's lines; once #911 merges todevelopthis branch can be re-targeted atdevelop(or fast-forwarded by GitHub).What's in this PR
src/dashboard/views/tasks-view.ts) — surfaces the task ledger from feat(core): oc_task_ledger — persistent async task table with cancel & wait (mcp-browser-use adoption A) #855. Reads~/.openchrome/tasks/(honorsOPENCHROME_TASK_ROOT). Renderstask_id,kind,status,age,duration_ms. Status colour-coded.src/dashboard/views/skills-view.ts) — walks~/.openchrome/skill-memory/<domain>/skills.jsonacross all known domains. Renders domain, name, use count, replay badge (PASS/FAIL/—), relative last-used timestamp. Replay-badge fields are read defensively so the PR builds on A alone; the cast becomes redundant once feat(pilot): oc_skill_replay — deterministic CDP-step replay with oc_assert contract gate (mcp-browser-use adoption B) #856 (B) is in develop.src/dashboard/index.ts,src/dashboard/types.ts):ViewModeextended with'tasks' | 'skills'.j→ tasks,k→ skills. (Existingtis taken for tabs,sfor sessions.)escapereturns to activity.refreshLedgers(). Read-only — IO errors surface inerrorMessagerather than thrown.P-principle compliance
os.homedir()+path.joineverywhere; no platform-specific paths.package.jsonis untouched.Tests (6 new)
TasksView: header + mixed-status rows render; empty-state; error message surfaces.SkillsView: rows + PASS/FAIL replay badges; empty-state.readSkillsSnapshottolerates a missing skill-memory directory and does not write any state.Coordination
—when those fields are absent — no test breakage, just less information until feat(pilot): oc_skill_replay — deterministic CDP-step replay with oc_assert contract gate (#856) #928 lands.Test plan (post-merge)
j→ see live counts of running/completed/cancelled jobs after exercisingoc_task_startk→ see per-domain skill rows with replay badge reflecting feat(pilot): oc_skill_replay — deterministic CDP-step replay with oc_assert contract gate (mcp-browser-use adoption B) #856's outcomes (once it lands)escape→ returns to activity viewCloses #865
Part of the
mcp-browser-use adoptionseries (F).🤖 Generated with Claude Code