Skip to content

WH-007: Default project selection to most recently used#67

Open
edmofro wants to merge 6 commits intomainfrom
workhorse/wh-007-spec
Open

WH-007: Default project selection to most recently used#67
edmofro wants to merge 6 commits intomainfrom
workhorse/wh-007-spec

Conversation

@edmofro
Copy link
Copy Markdown
Member

@edmofro edmofro commented Apr 3, 2026

Summary

When a user loads Workhorse, the project selector defaults to the most recently selected project for that user. This ensures users are returned to their last working context without needing to reselect manually.

Journey

  • Spec draft generated — Default project on load section added to product-navigation.md
  • Implementation completed — cookie-based last project persistence added to Sidebar.tsx and page.tsx

Specs

  • .workhorse/specs/product-navigation.md

Changed files

  • src/app/(main)/page.tsx
  • src/components/Sidebar.tsx
  • src/app/api/agent-session/route.ts
  • src/lib/git/worktree.ts

Test plan

Review Hero

  • Run Review Hero
  • Auto-fix review suggestions
  • Auto-fix CI failures

@review-hero
Copy link
Copy Markdown

review-hero bot commented Apr 3, 2026

🦸 Review Hero Summary
8 agents reviewed this PR | 1 failed | 2 critical | 1 suggestion | 1 nitpick | Filtering: consensus 3 voters, 3 below threshold

Below consensus threshold (3 unique issues not confirmed by majority)
Location Agent Severity Comment
src/app/(main)/page.tsx:8 Frontend Snappiness suggestion getProjects() fetches every project row from the database, then a linear find() scans all of them in memory to match the cookie slug. As the project list grows this is an O(n) table scan followed b...
src/app/api/agent-session/route.ts:472 Security critical The PR signal file .workhorse/pr-request.json is read from the checked-out worktree, which is a clone of the target repository. If the target repo already contains this file (e.g., a malicious or...
src/app/api/agent-session/route.ts:484 Frontend Snappiness suggestion Two separate prisma.card.update calls are issued per session finalisation: one here for prNumber/prUrl, and another shortly after for the session metadata (the updateData block visible just below t...

Nitpicks

File Line Agent Comment
src/components/Sidebar.tsx 69 Security Cookie is set without the Secure flag. If the app is ever served over HTTP (e.g., in staging or internal environments), the cookie will be transmitted in plaintext. The value itself is non-sensitive (a project slug), but adding '; Secure' costs nothing and prevents accidental leakage if TLS is mi...
Local fix prompt (copy to your coding agent)

Fix these issues identified on the pull request. One commit per issue fixed.


src/app/(main)/page.tsx:11: Cookie value is stored URI-encoded (via encodeURIComponent in Sidebar.tsx) but compared without decoding here. For any project name containing spaces or special characters, p.name.toLowerCase() (e.g. "my project") will never match lastSlug (e.g. "my%20project"), so the MRU redirect silently falls back to projects[0] every time. Fix: const lastProject = lastSlug ? projects.find((p) => p.name.toLowerCase() === decodeURIComponent(lastSlug)) : null


src/app/api/agent-session/route.ts:488: If prisma.card.update throws (e.g. DB error, card deleted), execution jumps to the catch block and deleteWorktreeFile is never called. On the next session finalisation the signal file is still present, so createPullRequest will be called again, creating a duplicate PR. Fix: delete the signal file before (or unconditionally after) attempting the DB update, or at minimum delete it inside the same try block before the prisma call so a DB failure doesn't leave the file around.


src/app/api/agent-session/route.ts:471: JSON.parse result is cast with 'as { title: string; body: string }' but not validated at runtime. If the agent writes a malformed signal file (e.g., title is an object or array), those values are passed directly to createPullRequest and serialised into a GitHub API POST body via JSON.stringify. Add a runtime guard: if (typeof title !== 'string' || typeof body !== 'string') throw new Error('Invalid PR signal'). This is a system boundary — data originates from the filesystem, not a trusted in-memory value.


src/components/Sidebar.tsx:69: Cookie is set without the Secure flag. If the app is ever served over HTTP (e.g., in staging or internal environments), the cookie will be transmitted in plaintext. The value itself is non-sensitive (a project slug), but adding '; Secure' costs nothing and prevents accidental leakage if TLS is misconfigured: workhorse_last_project=...; path=/; max-age=31536000; SameSite=Lax; Secure.

const wtPath = worktreePath(owner, repo, cardIdentifier)
const fullPath = safeResolvePath(wtPath, filePath)
try {
await fs.unlink(fullPath)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bugs & Correctness] suggestion

The catch block in deleteWorktreeFile silently swallows all errors, not just ENOENT (file not found). Permissions errors, I/O errors, or path traversal violations thrown by safeResolvePath would all be silently ignored. Fix: only ignore ENOENT — catch (err) { if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err }

const cookieStore = await cookies()
const lastSlug = cookieStore.get('workhorse_last_project')?.value
const lastProject = lastSlug
? projects.find((p) => p.name.toLowerCase() === lastSlug)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bugs & Correctness] critical

Cookie value is URI-encoded when written (Sidebar.tsx:69 uses encodeURIComponent(activeProject.name.toLowerCase())), but compared without decoding here. For any project name containing spaces or non-ASCII characters, lastSlug will be something like my%20project while p.name.toLowerCase() returns my project, so lastProject will always be null and the feature silently falls back to projects[0]. Fix: decode the cookie value before comparing — decodeURIComponent(lastSlug) — or store it unencoded.

const handleCloseModal = useCallback(() => setCreateModal(null), [])

// Persist the active project so the home page can redirect back to it
useEffect(() => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Security] suggestion

The cookie is set without the Secure flag: document.cookie = 'workhorse_last_project=...; path=/; max-age=31536000; SameSite=Lax'. Without Secure, the browser will send this cookie over plain HTTP connections, making it visible to network observers. While the value is just a project slug (low sensitivity), it is also read server-side during redirects — an attacker on the same network could silently swap it to redirect the user to a different project. Add ; Secure to restrict transmission to HTTPS only.

const prSignal = await readWorktreeFile(ctx.owner, ctx.repoName, ctx.card.identifier, '.workhorse/pr-request.json')
if (prSignal) {
const { title, body } = JSON.parse(prSignal) as { title: string; body: string }
const branchName = ctx.card.cardBranch
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Security] suggestion

JSON.parse(prSignal) is called on content written by the AI agent to a file on disk. There is no schema validation — the destructured title and body could be non-string types (e.g., objects, arrays) that stringify in unexpected ways when forwarded to the GitHub API. More critically, if the agent produces a crafted payload with a very long title (GitHub enforces a 256-character limit and will reject it with a 422), the error is silently swallowed by the outer catch, leaving the card in a state where prNumber/prUrl are never set and the signal file is never deleted — meaning the next session will re-attempt PR creation. Validate that title and body are non-empty strings (and optionally enforce a sane title length) before calling createPullRequest.

@review-hero
Copy link
Copy Markdown

review-hero bot commented Apr 3, 2026

🦸 Review Hero Summary
9 agents reviewed this PR | 1 critical | 3 suggestions | 0 nitpicks | Filtering: consensus 3 voters, 5 below threshold

Below consensus threshold (5 unique issues not confirmed by majority)
Location Agent Severity Comment
src/app/(main)/page.tsx:10 Frontend Snappiness suggestion getProjects() fetches all projects from the database, then a JS-level Array.find() scans them to match the cookie slug. As the project count grows this wastes both query bandwidth and memory. A tar...
src/app/api/agent-session/route.ts:463 Frontend Snappiness suggestion readWorktreeFile performs an unconditional filesystem read on every agent session finalisation, even when the agent never requested a PR. The signal file will be absent in the vast majority of case...
src/app/api/agent-session/route.ts:481 Frontend Snappiness nitpick prisma.card.update and deleteWorktreeFile are independent — one hits the database, the other hits the filesystem. They are awaited sequentially but could be run concurrently with Promise.all to hal...
src/app/api/agent-session/route.ts:487 Bugs & Correctness suggestion If prisma.card.update throws after createPullRequest succeeds, deleteWorktreeFile is never reached and pr-request.json remains on disk. The next call to finaliseSessionAfterExchange will attempt to...
src/components/Sidebar.tsx:71 Bugs & Correctness nitpick The useEffect dependency is [activeProject?.id] but the effect body reads activeProject.name. If a project's name is updated server-side while its id stays the same, the cookie will remain st...
Local fix prompt (copy to your coding agent)

Fix these issues identified on the pull request. One commit per issue fixed.


src/lib/git/worktree.ts:545: The catch block in deleteWorktreeFile silently swallows all errors, not just ENOENT (file not found). Permissions errors, I/O errors, or path traversal violations thrown by safeResolvePath would all be silently ignored. Fix: only ignore ENOENT — catch (err) { if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err }


src/app/(main)/page.tsx:13: Cookie value is URI-encoded when written (Sidebar.tsx:69 uses encodeURIComponent(activeProject.name.toLowerCase())), but compared without decoding here. For any project name containing spaces or non-ASCII characters, lastSlug will be something like my%20project while p.name.toLowerCase() returns my project, so lastProject will always be null and the feature silently falls back to projects[0]. Fix: decode the cookie value before comparing — decodeURIComponent(lastSlug) — or store it unencoded.


src/components/Sidebar.tsx:67: The cookie is set without the Secure flag: document.cookie = 'workhorse_last_project=...; path=/; max-age=31536000; SameSite=Lax'. Without Secure, the browser will send this cookie over plain HTTP connections, making it visible to network observers. While the value is just a project slug (low sensitivity), it is also read server-side during redirects — an attacker on the same network could silently swap it to redirect the user to a different project. Add ; Secure to restrict transmission to HTTPS only.


src/app/api/agent-session/route.ts:471: JSON.parse(prSignal) is called on content written by the AI agent to a file on disk. There is no schema validation — the destructured title and body could be non-string types (e.g., objects, arrays) that stringify in unexpected ways when forwarded to the GitHub API. More critically, if the agent produces a crafted payload with a very long title (GitHub enforces a 256-character limit and will reject it with a 422), the error is silently swallowed by the outer catch, leaving the card in a state where prNumber/prUrl are never set and the signal file is never deleted — meaning the next session will re-attempt PR creation. Validate that title and body are non-empty strings (and optionally enforce a sane title length) before calling createPullRequest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants